mbox series

[v7,0/5] leds: mt6360: Add LED driver for MT6360

Message ID 1605696462-391-1-git-send-email-gene.chen.richtek@gmail.com
Headers show
Series leds: mt6360: Add LED driver for MT6360 | expand

Message

Gene Chen Nov. 18, 2020, 10:47 a.m. UTC 
In-Reply-To: 

Gene Chen (5)
 leds: flash: Add flash registration with undefined CONFIG_LEDS_CLASS_FLASH
 dt-bindings: leds: Add LED_COLOR_ID_MOONLIGHT definitions
 dt-bindings: leds: common: Increase LED_COLOR_ID_* maximum size
 dt-bindings: leds: Add bindings for MT6360 LED
 leds: mt6360: Add LED driver for MT6360

 Documentation/devicetree/bindings/leds/common.yaml      |    2 
 Documentation/devicetree/bindings/leds/leds-mt6360.yaml |  164 +++
 drivers/leds/Kconfig                                    |   13 
 drivers/leds/Makefile                                   |    1 
 drivers/leds/leds-mt6360.c                              |  808 ++++++++++++++++
 include/dt-bindings/leds/common.h                       |    3 
 include/linux/led-class-flash.h                         |   36 
 7 files changed, 1025 insertions(+), 2 deletions(-)

changelogs between v1 & v2
 - add led driver with mfd

changelogs between v2 & v3
 - independent add led driver
 - add dt-binding document
 - refactor macros definition for easy to debug
 - parse device tree by fwnode
 - use devm*ext to register led class device

changelogs between v3 & v4
 - fix binding document description
 - use GENMASK and add unit postfix to definition
 - isink register led class device

changelogs between v4 & v5
 - change rgb isink to multicolor control
 - add binding reference to mfd yaml

changelogs between v5 & v6
 - Use DT to decide RGB LED is multicolor device or indicator device only

changelogs between v6 & v7
 - Add binding multicolor device sample code
 - Add flash ops mutex lock
 - Remove V4L2 init with indicator device

Comments

Jacek Anaszewski Nov. 19, 2020, 10:29 p.m. UTC | #1 
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

> 


-- 
Best regards,
Jacek Anaszewski
Gene Chen Nov. 23, 2020, 3:20 a.m. UTC | #2 
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 | #3 
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 | #4 
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 | #5 
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.

-- 
Best regards,
Jacek Anaszewski