| Message ID | 1605696462-391-2-git-send-email-gene.chen.richtek@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: mt6360: Add LED driver for MT6360 | expand |
Hi Gene,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on pavel-linux-leds/for-next]
[also build test WARNING on robh/for-next linus/master v5.10-rc4 next-20201118]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Gene-Chen/leds-mt6360-Add-LED-driver-for-MT6360/20201118-185208
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: nds32-randconfig-r001-20201118 (attached as .config)
compiler: nds32le-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
# https://github.com/0day-ci/linux/commit/a088d9624f18b888ede6b0344163b8464db82f3b
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Gene-Chen/leds-mt6360-Add-LED-driver-for-MT6360/20201118-185208
git checkout a088d9624f18b888ede6b0344163b8464db82f3b
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/staging/greybus/light.c:11:
>> include/linux/led-class-flash.h:160:6: warning: no previous prototype for 'devm_led_classdev_flash_unregister' [-Wmissing-prototypes]
160 | void devm_led_classdev_flash_unregister(struct device *parent,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/devm_led_classdev_flash_unregister +160 include/linux/led-class-flash.h
159
> 160 void devm_led_classdev_flash_unregister(struct device *parent,
161 struct led_classdev_flash *fled_cdev)
162 {};
163
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年11月20日 週五 上午6:29寫道： > > Hi Gene, > > On 11/18/20 11:47 AM, Gene Chen wrote: > > From: Gene Chen <gene_chen@richtek.com> > > > > Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH > > > > Signed-off-by: Gene Chen <gene_chen@richtek.com> > > --- > > include/linux/led-class-flash.h | 36 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h > > index 21a3358..4f56c28 100644 > > --- a/include/linux/led-class-flash.h > > +++ b/include/linux/led-class-flash.h > > @@ -85,6 +85,7 @@ static inline struct led_classdev_flash *lcdev_to_flcdev( > > return container_of(lcdev, struct led_classdev_flash, led_cdev); > > } > > > > +#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) > > /** > > * led_classdev_flash_register_ext - register a new object of LED class with > > * init data and with support for flash LEDs > > @@ -127,6 +128,41 @@ static inline int devm_led_classdev_flash_register(struct device *parent, > > void devm_led_classdev_flash_unregister(struct device *parent, > > struct led_classdev_flash *fled_cdev); > > > > +#else > > + > > +static inline int led_classdev_flash_register_ext(struct device *parent, > > + struct led_classdev_flash *fled_cdev, > > + struct led_init_data *init_data) > > +{ > > + return -EINVAL; > > s/-EINVAL/0/ > > The goal here is to assure that client will not fail when using no-op. > > > +} > > + > > +static inline int led_classdev_flash_register(struct device *parent, > > + struct led_classdev_flash *fled_cdev) > > +{ > > + return led_classdev_flash_register_ext(parent, fled_cdev, NULL); > > +} > > This function should be placed after #ifdef block because its > shape is the same for both cases. > > > +static inline void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev) {}; > > +static inline int devm_led_classdev_flash_register_ext(struct device *parent, > > + struct led_classdev_flash *fled_cdev, > > + struct led_init_data *init_data) > > +{ > > + return -EINVAL; > > /-EINVAL/0/ > > Please do the same fix in all no-ops in the led-class-multicolor.h, > as we've discussed. > I think return -EINVAL is correct, because I should register flash light device if I define FLED in DTS node. > > +} > > + > > +static inline int devm_led_classdev_flash_register(struct device *parent, > > + struct led_classdev_flash *fled_cdev) > > +{ > > + return devm_led_classdev_flash_register_ext(parent, fled_cdev, NULL); > > +} > > > This function should also be placed after #ifdef block. > Please make the same optimizations in the led-class-multicolor.h as you > are at it. > > > + > > +void devm_led_classdev_flash_unregister(struct device *parent, > > s/void/static inline void/ > > That's the reason why you got warning from buildbot. > ACK > > + struct led_classdev_flash *fled_cdev) > > +{}; > > + > > +#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) */ > > + > > /** > > * led_set_flash_strobe - setup flash strobe > > * @fled_cdev: the flash LED to set strobe on > > > > -- > Best regards, > Jacek Anaszewski
On 11/23/20 4:20 AM, Gene Chen wrote: > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年11月20日 週五 上午6:29寫道： >> >> Hi Gene, >> >> On 11/18/20 11:47 AM, Gene Chen wrote: >>> From: Gene Chen <gene_chen@richtek.com> >>> >>> Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH >>> >>> Signed-off-by: Gene Chen <gene_chen@richtek.com> >>> --- >>> include/linux/led-class-flash.h | 36 ++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 36 insertions(+) >>> >>> diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h >>> index 21a3358..4f56c28 100644 >>> --- a/include/linux/led-class-flash.h >>> +++ b/include/linux/led-class-flash.h >>> @@ -85,6 +85,7 @@ static inline struct led_classdev_flash *lcdev_to_flcdev( >>> return container_of(lcdev, struct led_classdev_flash, led_cdev); >>> } >>> >>> +#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) >>> /** >>> * led_classdev_flash_register_ext - register a new object of LED class with >>> * init data and with support for flash LEDs >>> @@ -127,6 +128,41 @@ static inline int devm_led_classdev_flash_register(struct device *parent, >>> void devm_led_classdev_flash_unregister(struct device *parent, >>> struct led_classdev_flash *fled_cdev); >>> >>> +#else >>> + >>> +static inline int led_classdev_flash_register_ext(struct device *parent, >>> + struct led_classdev_flash *fled_cdev, >>> + struct led_init_data *init_data) >>> +{ >>> + return -EINVAL; >> >> s/-EINVAL/0/ >> >> The goal here is to assure that client will not fail when using no-op. >> >>> +} >>> + >>> +static inline int led_classdev_flash_register(struct device *parent, >>> + struct led_classdev_flash *fled_cdev) >>> +{ >>> + return led_classdev_flash_register_ext(parent, fled_cdev, NULL); >>> +} >> >> This function should be placed after #ifdef block because its >> shape is the same for both cases. >> >>> +static inline void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev) {}; >>> +static inline int devm_led_classdev_flash_register_ext(struct device *parent, >>> + struct led_classdev_flash *fled_cdev, >>> + struct led_init_data *init_data) >>> +{ >>> + return -EINVAL; >> >> /-EINVAL/0/ >> >> Please do the same fix in all no-ops in the led-class-multicolor.h, >> as we've discussed. >> > > I think return -EINVAL is correct, because I should register flash > light device if I define FLED in DTS node. I don't quite follow your logic here. No-op function's purpose is to simplify the code on the caller's side. Therefore it should report success. Please return 0 from it. -- Best regards, Jacek Anaszewski
Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年11月24日 週二 上午5:07寫道： > > On 11/23/20 4:20 AM, Gene Chen wrote: > > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年11月20日 週五 上午6:29寫道： > >> > >> Hi Gene, > >> > >> On 11/18/20 11:47 AM, Gene Chen wrote: > >>> From: Gene Chen <gene_chen@richtek.com> > >>> > >>> Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH > >>> > >>> Signed-off-by: Gene Chen <gene_chen@richtek.com> > >>> --- > >>> include/linux/led-class-flash.h | 36 ++++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 36 insertions(+) > >>> > >>> diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h > >>> index 21a3358..4f56c28 100644 > >>> --- a/include/linux/led-class-flash.h > >>> +++ b/include/linux/led-class-flash.h > >>> @@ -85,6 +85,7 @@ static inline struct led_classdev_flash *lcdev_to_flcdev( > >>> return container_of(lcdev, struct led_classdev_flash, led_cdev); > >>> } > >>> > >>> +#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) > >>> /** > >>> * led_classdev_flash_register_ext - register a new object of LED class with > >>> * init data and with support for flash LEDs > >>> @@ -127,6 +128,41 @@ static inline int devm_led_classdev_flash_register(struct device *parent, > >>> void devm_led_classdev_flash_unregister(struct device *parent, > >>> struct led_classdev_flash *fled_cdev); > >>> > >>> +#else > >>> + > >>> +static inline int led_classdev_flash_register_ext(struct device *parent, > >>> + struct led_classdev_flash *fled_cdev, > >>> + struct led_init_data *init_data) > >>> +{ > >>> + return -EINVAL; > >> > >> s/-EINVAL/0/ > >> > >> The goal here is to assure that client will not fail when using no-op. > >> > >>> +} > >>> + > >>> +static inline int led_classdev_flash_register(struct device *parent, > >>> + struct led_classdev_flash *fled_cdev) > >>> +{ > >>> + return led_classdev_flash_register_ext(parent, fled_cdev, NULL); > >>> +} > >> > >> This function should be placed after #ifdef block because its > >> shape is the same for both cases. > >> > >>> +static inline void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev) {}; > >>> +static inline int devm_led_classdev_flash_register_ext(struct device *parent, > >>> + struct led_classdev_flash *fled_cdev, > >>> + struct led_init_data *init_data) > >>> +{ > >>> + return -EINVAL; > >> > >> /-EINVAL/0/ > >> > >> Please do the same fix in all no-ops in the led-class-multicolor.h, > >> as we've discussed. > >> > > > > I think return -EINVAL is correct, because I should register flash > > light device if I define FLED in DTS node. > > I don't quite follow your logic here. > > No-op function's purpose is to simplify the code on the caller's side. > Therefore it should report success. > > Please return 0 from it. > Just like those functions in led-class-multicolor.h, caller may use return value to check whether FLED is registered successfully or not. For this case, is returning 0 a little bit misleading? > -- > Best regards, > Jacek Anaszewski
On 11/24/20 7:08 AM, Gene Chen wrote: [...] >>>> This function should be placed after #ifdef block because its >>>> shape is the same for both cases. >>>> >>>>> +static inline void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev) {}; >>>>> +static inline int devm_led_classdev_flash_register_ext(struct device *parent, >>>>> + struct led_classdev_flash *fled_cdev, >>>>> + struct led_init_data *init_data) >>>>> +{ >>>>> + return -EINVAL; >>>> >>>> /-EINVAL/0/ >>>> >>>> Please do the same fix in all no-ops in the led-class-multicolor.h, >>>> as we've discussed. >>>> >>> >>> I think return -EINVAL is correct, because I should register flash >>> light device if I define FLED in DTS node. OK, I think I'm getting your concerns now. So you're only partially correct - the driver should register flash LED device if there is corresponding node in DT, but only if CONFIG_LEDS_CLASS_FLASH is enabled. In case it is disabled the no-op will come into play and return 0, allowing the probe() to proceed as if the registration succeeded. From the driver point of view nothing changes, except that flash LED ops will not be called afterwards. This is common pattern. If in doubt skim through the headers in include/linux. >> >> I don't quite follow your logic here. >> >> No-op function's purpose is to simplify the code on the caller's side. >> Therefore it should report success. >> >> Please return 0 from it. >> > > Just like those functions in led-class-multicolor.h, caller may use > return value to check whether FLED is registered successfully or not. > For this case, is returning 0 a little bit misleading? Please note that I've already admitted that led-class-multicolor.h class is buggy and should also be fixed to return 0 from its no-ops. Please apply the "s/-EINVAL/0/" fixes to it as well - your driver will need that.
diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h index 21a3358..4f56c28 100644 --- a/include/linux/led-class-flash.h +++ b/include/linux/led-class-flash.h @@ -85,6 +85,7 @@ static inline struct led_classdev_flash *lcdev_to_flcdev( return container_of(lcdev, struct led_classdev_flash, led_cdev); } +#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) /** * led_classdev_flash_register_ext - register a new object of LED class with * init data and with support for flash LEDs @@ -127,6 +128,41 @@ static inline int devm_led_classdev_flash_register(struct device *parent, void devm_led_classdev_flash_unregister(struct device *parent, struct led_classdev_flash *fled_cdev); +#else + +static inline int led_classdev_flash_register_ext(struct device *parent, + struct led_classdev_flash *fled_cdev, + struct led_init_data *init_data) +{ + return -EINVAL; +} + +static inline int led_classdev_flash_register(struct device *parent, + struct led_classdev_flash *fled_cdev) +{ + return led_classdev_flash_register_ext(parent, fled_cdev, NULL); +} + +static inline void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev) {}; +static inline int devm_led_classdev_flash_register_ext(struct device *parent, + struct led_classdev_flash *fled_cdev, + struct led_init_data *init_data) +{ + return -EINVAL; +} + +static inline int devm_led_classdev_flash_register(struct device *parent, + struct led_classdev_flash *fled_cdev) +{ + return devm_led_classdev_flash_register_ext(parent, fled_cdev, NULL); +} + +void devm_led_classdev_flash_unregister(struct device *parent, + struct led_classdev_flash *fled_cdev) +{}; + +#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) */ + /** * led_set_flash_strobe - setup flash strobe * @fled_cdev: the flash LED to set strobe on