lkml.org 
[lkml]   [2020]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [RESEND PATCH v27 11/15] leds: lp55xx: Add multicolor framework support to lp55xx
    From
    Date
    Dan,

    On 6/21/20 10:24 PM, Jacek Anaszewski wrote:
    > Dan,
    >
    > On 6/21/20 4:12 PM, Dan Murphy wrote:
    >> Jacek
    >>
    >> On 6/19/20 5:10 PM, Jacek Anaszewski wrote:
    >>> Dan,
    >>>
    >>> On 6/19/20 6:35 PM, Dan Murphy wrote:
    >>>> Jacek
    >>>>
    >>>> On 6/18/20 6:26 PM, Jacek Anaszewski wrote:
    >>>>> On 6/19/20 12:09 AM, Jacek Anaszewski wrote:
    >>>>>> Dan,
    >>>>>>
    >>>>>> On 6/18/20 11:44 PM, Dan Murphy wrote:
    >>>>>>> Jacek
    >>>>>>>
    >>>>>>> On 6/18/20 4:21 PM, Jacek Anaszewski wrote:
    >>>>>>>> Dan,
    >>>>>>>>
    >>>>>>>> On 6/18/20 12:33 AM, Dan Murphy wrote:
    >>>>>>>>> Jacek
    >>>>>>>>>
    >>>>>>>>> On 6/17/20 4:41 PM, Jacek Anaszewski wrote:
    >>>>>>>>>> Dan,
    >>>>>>>>>>
    >>>>>>>>>> On 6/17/20 9:22 PM, Dan Murphy wrote:
    >>>>>>>>>>> Pavel/Jacek
    >>>>>>>>>>>
    >>>>>>>>>>> On 6/17/20 11:28 AM, kernel test robot wrote:
    >>>>>>>>>>>> Hi Dan,
    >>>>>>>>>>>>
    >>>>>>>>>>>> I love your patch! Yet something to improve:
    >>>>>>>>>>>>
    >>>>>>>>>>>> [auto build test ERROR on pavel-linux-leds/for-next]
    >>>>>>>>>>>> [cannot apply to j.anaszewski-leds/for-next]
    >>>>>>>>>>>> [if your patch is applied to the wrong git tree, please drop
    >>>>>>>>>>>> us a note to help
    >>>>>>>>>>>> improve the system. BTW, we also suggest to use '--base'
    >>>>>>>>>>>> option to specify the
    >>>>>>>>>>>> base tree in git format-patch, please see
    >>>>>>>>>>>> https://stackoverflow.com/a/37406982]
    >>>>>>>>>>>>
    >>>>>>>>>>>> url:
    >>>>>>>>>>>> https://github.com/0day-ci/linux/commits/Dan-Murphy/Multicolor-Framework-v27/20200616-042217
    >>>>>>>>>>>>
    >>>>>>>>>>>> base:
    >>>>>>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git
    >>>>>>>>>>>> for-next
    >>>>>>>>>>>> config: ia64-randconfig-r015-20200617 (attached as .config)
    >>>>>>>>>>>> compiler: ia64-linux-gcc (GCC) 9.3.0
    >>>>>>>>>>>> reproduce (this is a W=1 build):
    >>>>>>>>>>>>          wget
    >>>>>>>>>>>> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
    >>>>>>>>>>>> -O ~/bin/make.cross
    >>>>>>>>>>>>          chmod +x ~/bin/make.cross
    >>>>>>>>>>>>          # save the attached .config to linux build tree
    >>>>>>>>>>>>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0
    >>>>>>>>>>>> make.cross ARCH=ia64
    >>>>>>>>>>>>
    >>>>>>>>>>>> If you fix the issue, kindly add following tag as appropriate
    >>>>>>>>>>>> Reported-by: kernel test robot <lkp@intel.com>
    >>>>>>>>>>>>
    >>>>>>>>>>>> All errors (new ones prefixed by >>, old ones prefixed by <<):
    >>>>>>>>>>>>
    >>>>>>>>>>>> ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in
    >>>>>>>>>>>> function `lp55xx_set_mc_brightness':
    >>>>>>>>>>>>>> drivers/leds/leds-lp55xx-common.c:146: undefined reference
    >>>>>>>>>>>>>> to `led_mc_calc_color_components'
    >>>>>>>>>>>> ia64-linux-ld: drivers/leds/leds-lp55xx-common.o: in
    >>>>>>>>>>>> function `devm_led_classdev_multicolor_register':
    >>>>>>>>>>>>>> include/linux/led-class-multicolor.h:74: undefined
    >>>>>>>>>>>>>> reference to `devm_led_classdev_multicolor_register_ext'
    >>>>>>>>>>>> vim +146 drivers/leds/leds-lp55xx-common.c
    >>>>>>>>>>>>
    >>>>>>>>>>>>     138
    >>>>>>>>>>>>     139    static int lp55xx_set_mc_brightness(struct
    >>>>>>>>>>>> led_classdev *cdev,
    >>>>>>>>>>>>     140                        enum led_brightness brightness)
    >>>>>>>>>>>>     141    {
    >>>>>>>>>>>>     142        struct led_classdev_mc *mc_dev =
    >>>>>>>>>>>> lcdev_to_mccdev(cdev);
    >>>>>>>>>>>>     143        struct lp55xx_led *led =
    >>>>>>>>>>>> mcled_cdev_to_led(mc_dev);
    >>>>>>>>>>>>     144        struct lp55xx_device_config *cfg =
    >>>>>>>>>>>> led->chip->cfg;
    >>>>>>>>>>>>     145
    >>>>>>>>>>>>   > 146 led_mc_calc_color_components(&led->mc_cdev,
    >>>>>>>>>>>> brightness);
    >>>>>>>>>>>>     147        return cfg->multicolor_brightness_fn(led);
    >>>>>>>>>>>>     148
    >>>>>>>>>>>
    >>>>>>>>>>> Well this was a mess to figure out.
    >>>>>>>>>>>
    >>>>>>>>>>> The only fix I can figure out here is to remove the
    >>>>>>>>>>>
    >>>>>>>>>>>      depends on LEDS_CLASS_MULTI_COLOR ||
    >>>>>>>>>>> !LEDS_CLASS_MULTI_COLOR
    >>>>>>>>>>>
    >>>>>>>>>>> from each child device and add
    >>>>>>>>>>>
    >>>>>>>>>>>      select LEDS_CLASS_MULTI_COLOR
    >>>>>>>>>>>
    >>>>>>>>>>> to the LP55XX_COMMON
    >>>>>>>>>>>
    >>>>>>>>>>> This way the Multi color framework will inherit the symbol
    >>>>>>>>>>> that was set by the COMMON flag which is inherited by
    >>>>>>>>>>> majority from the child flags.
    >>>>>>>>>>
    >>>>>>>>>> Did you try this?
    >>>>>>>>>>
    >>>>>>>>>> --- a/drivers/leds/Kconfig
    >>>>>>>>>> +++ b/drivers/leds/Kconfig
    >>>>>>>>>> @@ -398,6 +398,7 @@ config LEDS_LP50XX
    >>>>>>>>>>  config LEDS_LP55XX_COMMON
    >>>>>>>>>>         tristate "Common Driver for TI/National
    >>>>>>>>>> LP5521/5523/55231/5562/8501"
    >>>>>>>>>>         depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562
    >>>>>>>>>> || LEDS_LP8501
    >>>>>>>>>> +       depends on LEDS_CLASS_MULTI_COLOR ||
    >>>>>>>>>> !LEDS_CLASS_MULTI_COLOR
    >>>>>>>>>>         depends on OF
    >>>>>>>>>>         select FW_LOADER
    >>>>>>>>>>         select FW_LOADER_USER_HELPER
    >>>>>>>>>>
    >>>>>>>>>>
    >>>>>>>>> Yes I did
    >>>>>>>>>
    >>>>>>>>> That gave unmet dependencies.
    >>>>>>>>>
    >>>>>>>>> WARNING: unmet direct dependencies detected for LEDS_LP55XX_COMMON
    >>>>>>>>>    Depends on [m]: NEW_LEDS [=y] && (LEDS_LP5521 [=n] ||
    >>>>>>>>> LEDS_LP5523 [=m] || LEDS_LP5562 [=y] || LEDS_LP8501 [=y]) &&
    >>>>>>>>> (LEDS_CLASS_MULTI_COLOR [=m] || !LEDS_CLASS_MULTI_COLOR [=m])
    >>>>>>>>> && OF [=y]
    >>>>>>>>>    Selected by [y]:
    >>>>>>>>>    - LEDS_LP5562 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] &&
    >>>>>>>>> I2C [=y]
    >>>>>>>>>    - LEDS_LP8501 [=y] && NEW_LEDS [=y] && LEDS_CLASS [=y] &&
    >>>>>>>>> I2C [=y]
    >>>>>>>>>    Selected by [m]:
    >>>>>>>>>    - LEDS_LP5523 [=m] && NEW_LEDS [=y] && LEDS_CLASS [=y] &&
    >>>>>>>>> I2C [=y] && (LEDS_CLASS_MULTI_COLOR [=m] ||
    >>>>>>>>> !LEDS_CLASS_MULTI_COLOR [=m])
    >>>>>>>>>
    >>>>>>>>
    >>>>>>>> When I was testing that yesterday I also had the same warning at
    >>>>>>>> some
    >>>>>>>> point of testing different Kconfig setups, but with what I
    >>>>>>>> showed above
    >>>>>>>> it ceased to appear. Now every time I am doing "make oldconfig" the
    >>>>>>>> CONFIG_LEDS_LP55XX_COMMON=y entry gets changed to =m with the
    >>>>>>>> config
    >>>>>>>> from the test bot.
    >>>>>>>>
    >>>>>>> That is not what I saw in my testing especially after doing a
    >>>>>>> distclean
    >>>>>>
    >>>>>> Could you please give your exact steps after "make distclean" and
    >>>>>> copying test bot config to the kernel root directory?
    >>>>>>
    >>>>>> Also, please share the toolchain you're using for tests.
    >>>>>
    >>>>> Actually at this stage the toolchain is of lesser relevance.
    >>>>>
    >>>>> I've tried once more and indeed the problem shows up.
    >>>>>
    >>>>> It is caused by the driver entries doing
    >>>>>
    >>>>> "select LEDS_LP55XX_COMMON".
    >>>>>
    >>>>> Select sets config to "y" so it conflicts with
    >>>>> "depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR"
    >>>>> in the "config LEDS_LP55XX_COMMON".
    >>>>>
    >>>>> Your proposed fix will block the possibility of building
    >>>>> LED_CLASS_MULTI_COLOR as a module when LP55XX drivers
    >>>>> are enabled so this is also not an option.
    >>>>>
    >>>>> Solving this riddle will require some more thinking.
    >>>>> I haven't analyzed it in detail but maybe "imply" statement from
    >>>>> kconfig-language.rst could help somehow here.
    >>>>
    >>>> The multicolor framework will build as a module if the LED_CLASS is
    >>>> defined as a module.
    >>>>
    >>>> See attached test_defconfig
    >>>
    >>> But it will be impossible to enable CONFIG_LEDS_LP50XX without
    >>> CONFIG_LEDS_CLASS_MULTI_COLOR if you will remove
    >>>
    >>> depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR.
    >>>
    >> I was not removing the dependency for the LP50xx only the LP55xx.
    >
    > It was my typo, I meant LP55XX.
    >
    >>
    >>> This is actually why the above entry was needed.
    >>>
    >>> LP55XX drivers have to work also without multicolor class.
    >>>
    >> Well I am not sure how else to resolve this problem.  Because the
    >> LP55xx has multi level dependencies.
    >>
    >> Only the LP55xx_common has the dependency on the MC framework. The
    >> device drivers do not.
    >>
    >> The issue is the mixing and matching of the MC fw as a module vs the
    >> LP55XX_COMMON as a built-in.
    >
    > The simplest solution is to avoid selecting LP55XX_COMMON in favor
    > of making child drivers depending on it. Of course, maybe there exists
    > some trickier option to keep the selection, but I would have to play
    > a bit more with it to be sure.

    Yep, this seems to be the only viable solution.

    My proposed change to the Kconfig:

    diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
    index adfa762cb291..10de7d237ffd 100644
    --- a/drivers/leds/Kconfig
    +++ b/drivers/leds/Kconfig
    @@ -397,10 +397,10 @@ config LEDS_LP50XX

    config LEDS_LP55XX_COMMON
    tristate "Common Driver for TI/National
    LP5521/5523/55231/5562/8501"
    - depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
    depends on OF
    select FW_LOADER
    select FW_LOADER_USER_HELPER
    + depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
    help
    This option supports common operations for
    LP5521/5523/55231/5562/8501
    devices.
    @@ -408,8 +408,7 @@ config LEDS_LP55XX_COMMON
    config LEDS_LP5521
    tristate "LED Support for N.S. LP5521 LED driver chip"
    depends on LEDS_CLASS && I2C
    - depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
    - select LEDS_LP55XX_COMMON
    + depends on LEDS_LP55XX_COMMON
    help
    If you say yes here you get support for the National
    Semiconductor
    LP5521 LED driver. It is 3 channel chip with programmable
    engines.
    @@ -419,8 +418,7 @@ config LEDS_LP5521
    config LEDS_LP5523
    tristate "LED Support for TI/National LP5523/55231 LED driver chip"
    depends on LEDS_CLASS && I2C
    - depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
    - select LEDS_LP55XX_COMMON
    + depends on LEDS_LP55XX_COMMON
    help
    If you say yes here you get support for TI/National Semiconductor
    LP5523/55231 LED driver.
    @@ -431,7 +429,7 @@ config LEDS_LP5523
    config LEDS_LP5562
    tristate "LED Support for TI LP5562 LED driver chip"
    depends on LEDS_CLASS && I2C
    - select LEDS_LP55XX_COMMON
    + depends on LEDS_LP55XX_COMMON
    help
    If you say yes here you get support for TI LP5562 LED driver.
    It is 4 channels chip with programmable engines.
    @@ -441,7 +439,7 @@ config LEDS_LP5562
    config LEDS_LP8501
    tristate "LED Support for TI LP8501 LED driver chip"
    depends on LEDS_CLASS && I2C
    - select LEDS_LP55XX_COMMON
    + depends on LEDS_LP55XX_COMMON
    help
    If you say yes here you get support for TI LP8501 LED driver.
    It is 9 channel chip with programmable engines.

    --
    Best regards,
    Jacek Anaszewski

    \
     
     \ /
      Last update: 2020-06-22 14:42    [W:6.962 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site