Re: [PATCH v1 1/2] leds: rt4505: Add support for Richtek RT4505 flash led controller

[ChiYuan Huang]

Pavel Machek <pavel@xxxxxx> 於 2020年10月27日 週二 下午4:29寫道：
>
> Hi!
>
> > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> >
> > Add support for RT4505 flash led controller. It can support up to 1.5A
> > flash current with hardware timeout and low input voltage
> > protection.
>
> Please use upper-case "LED" everywhere.
>
> This should be 2nd in the series, after DT changes.
Sure, will ack in next series patch.
>
> > Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > ---
> >  drivers/leds/Kconfig       |  11 ++
> >  drivers/leds/Makefile      |   1 +
> >  drivers/leds/leds-rt4505.c | 397 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 409 insertions(+)
> >  create mode 100644 drivers/leds/leds-rt4505.c
>
> Lets put this into drivers/leds/flash/. (Yes, you'll have to create
> it).
OK
>
>
> > +     help
> > +       This option enables support for the RT4505 flash led controller.
>
> Information where it is used would be welcome here.
How about to add the below line for the extra information?
Usually used to company with the camera device on smartphone/tablet products
>
> > +       It can support up to 1.5A flash strobe current with hardware timeout
> > +       and low input voltage protection.
>
> This does not / should not be here.
OK
> > +
> > +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> > +{
>
> 80 columns, where easy.
I'll review all source code to meet the 80 column limit.
>
> > +     struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv, flash.led_cdev);
> > +     u32 val = 0;
> > +     int ret;
> > +
> > +     mutex_lock(&priv->lock);
> > +
> > +     if (level != LED_OFF) {
> > +             ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED, RT4505_ITORCH_MASK,
> > +                                      (level - 1) << RT4505_ITORCH_SHIFT);
> > +             if (ret)
> > +                     goto unlock;
> > +
> > +             val = RT4505_TORCH_SET;
> > +     }
> > +
> > +     ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
> > +
> > +unlock:
> > +     mutex_unlock(&priv->lock);
> > +     return ret;
> > +}
>
> Why is the locking needed? What will the /sys/class/leds interface
> look like on system with your flash?

The original thought is because there's still another way to control
flash like as v4l2.
But after reviewing the source code, led sysfs node will be protected
by led_cdev->led_access.
And V4L2 flash will also be protected by v4l2_fh_is_singular API.
I think the whole locking in the source code code may be removed. Right?
>
> > +static int rt4505_flash_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
> > +{
> > +     struct rt4505_priv *priv = container_of(fled_cdev, struct rt4505_priv, flash);
> > +     u32 val;
> > +     int ret;
> > +
> > +     mutex_lock(&priv->lock);
> > +
> > +     ret = regmap_read(priv->regmap, RT4505_REG_ENABLE, &val);
> > +     if (ret)
> > +             goto unlock;
> > +
> > +     *state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true : false;
>
> No need for ? ... part.
Do you mean this function is not needed? If yes, it can be removed.
But if it removed, led sysfs flash_strobe show will be not supported.
>
> > +static bool rt4505_is_accessible_reg(struct device *dev, unsigned int reg)
> > +{
> > +     if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
> > +             return true;
>
> Make this two stagements.
Like as the below one?? Or separate it into two if case.
if (reg == RT4505_REG_RESET ||
       reg >= RT4505_REG_CONFIG  && reg <= RT4505_REG_FLAGS))
>
> Otherwise... looks like easy simple driver, thanks.
>
> Best regards,
>                                                                 Pavel
> --
> http://www.livejournal.com/~pavelmachek