diff mbox series

[v7,1/5] leds: flash: Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH

Message ID 1605696462-391-2-git-send-email-gene.chen.richtek@gmail.com (mailing list archive)
State New, archived
Headers show
Series leds: mt6360: Add LED driver for MT6360 | expand

Commit Message

Gene Chen Nov. 18, 2020, 10:47 a.m. UTC 
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(+)

Comments

kernel test robot Nov. 18, 2020, 1:32 p.m. UTC | #1 
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 Nov. 19, 2020, 10:29 p.m. UTC | #2 
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.

> +}
> +
> +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.

> +					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
>
Gene Chen Nov. 23, 2020, 3:20 a.m. UTC | #3 
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
Jacek Anaszewski Nov. 23, 2020, 9:06 p.m. UTC | #4 
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.
Gene Chen Nov. 24, 2020, 6:08 a.m. UTC | #5 
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
Jacek Anaszewski Nov. 24, 2020, 9:15 p.m. UTC | #6 
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 mbox series

Patch

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