Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[Lee Jones <lee@kernel.org>]

On Thu, 11 Apr 2024, Lee Jones wrote:

> On Mon, 01 Apr 2024, André Apitzsch via B4 Relay wrote:
> 
> > From: André Apitzsch <git@apitzsch.eu>
> > 
> > Add support for SY7802 flash LED controller. It can support up to 1.8A
> > flash current.
> 
> This is a very small commit message for a 500+ line change!
> 
> Please, tell us more.
> 
> > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > ---
> >  drivers/leds/flash/Kconfig       |  11 +
> >  drivers/leds/flash/Makefile      |   1 +
> >  drivers/leds/flash/leds-sy7802.c | 532 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 544 insertions(+)
> > 
> > diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
> > index 809b6d98bb3e..f39f0bfe6eef 100644
> > --- a/drivers/leds/flash/Kconfig
> > +++ b/drivers/leds/flash/Kconfig
> > @@ -121,4 +121,15 @@ config LEDS_SGM3140
> >  	  This option enables support for the SGM3140 500mA Buck/Boost Charge
> >  	  Pump LED Driver.
> >  
> > +config LEDS_SY7802
> > +	tristate "LED support for the Silergy SY7802"
> > +	depends on I2C && OF
> > +	depends on GPIOLIB
> > +	select REGMAP_I2C
> > +	help
> > +	  This option enables support for the SY7802 flash LED controller.
> > +	  SY7802 includes torch and flash functions with programmable current.
> > +
> > +	  This driver can be built as a module, it will be called "leds-sy7802".
> > +
> >  endif # LEDS_CLASS_FLASH
> > diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
> > index 91d60a4b7952..48860eeced79 100644
> > --- a/drivers/leds/flash/Makefile
> > +++ b/drivers/leds/flash/Makefile
> > @@ -11,3 +11,4 @@ obj-$(CONFIG_LEDS_QCOM_FLASH)	+= leds-qcom-flash.o
> >  obj-$(CONFIG_LEDS_RT4505)	+= leds-rt4505.o
> >  obj-$(CONFIG_LEDS_RT8515)	+= leds-rt8515.o
> >  obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
> > +obj-$(CONFIG_LEDS_SY7802)	+= leds-sy7802.o
> > diff --git a/drivers/leds/flash/leds-sy7802.c b/drivers/leds/flash/leds-sy7802.c
> > new file mode 100644
> > index 000000000000..c03a571b0e08
> > --- /dev/null
> > +++ b/drivers/leds/flash/leds-sy7802.c
> > @@ -0,0 +1,532 @@

[...]

> > +static int sy7802_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> 
> s/level/brightness/
> 
> > +{
> > +	struct sy7802_led *led = container_of(lcdev, struct sy7802_led, flash.led_cdev);
> > +	u32 led_enable_mask = led->led_no == SY7802_LED_JOINT ? SY7802_LEDS_MASK_ALL :
> > +			      SY7802_LEDS_MASK(led->led_no);
> 
> Do all of the fancy multi-line assignment outside of the declaration block.
> 
> > +	u32 enable_mask = SY7802_MODE_MASK | led_enable_mask;
> > +	u32 val = level ? led_enable_mask : SY7802_MODE_OFF;
> > +	struct sy7802 *chip = led->chip;
> > +	u32 curr;
> 
> This is a temporary placeholder for fled_torch_used, right?
> 
> fled_torch_used_tmp?  Sometimes abbreviated to tmp.
> 
> > +	u32 mask;
> 
> That's a lot of masks.  Which one is this?
> 
> > +	int ret;
> > +
> > +	mutex_lock(&chip->mutex);
> > +
> > +	/*
> > +	 * There is only one set of flash control logic, and this flag is used to check if 'strobe'
> 
> The ',' before 'and' is superfluous.
> 
> > +	 * is currently being used.
> > +	 */
> 
> Doesn't the variable name kind of imply this?
> 
> > +	if (chip->fled_strobe_used) {
> > +		dev_warn(chip->dev, "Please disable strobe first [%d]\n", chip->fled_strobe_used);
> 
> "Cannot set torch brightness whilst strobe is enabled"
> 
> > +		ret = -EBUSY;
> > +		goto unlock;
> > +	}
> > +
> > +	if (level)
> > +		curr = chip->fled_torch_used | BIT(led->led_no);
> > +	else
> > +		curr = chip->fled_torch_used & ~BIT(led->led_no);
> > +
> > +	if (curr)
> > +		val |= SY7802_MODE_TORCH;
> > +
> > +	/* Torch needs to be disabled first to apply new brightness */
> 
> "Disable touch to apply brightness"
> 
> > +	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE, SY7802_MODE_MASK,
> > +				 SY7802_MODE_OFF);
> > +	if (ret)
> > +		goto unlock;
> > +
> > +	mask = led->led_no == SY7802_LED_JOINT ? SY7802_TORCH_CURRENT_MASK_ALL :
> 
> Why not just use led->led_no in place of mask?

mask and led->led_no are assigned the same value from this point on.

> Easier to read if you drop SY7802_TORCH_CURRENT_MASK_ALL to its own line.
> 
> > +	       SY7802_TORCH_CURRENT_MASK(led->led_no);
> > +
> > +	/* Register expects brightness between 0 and MAX_BRIGHTNESS - 1 */
> > +	if (level)
> > +		level -= 1;
> > +
> > +	level |= (level << SY7802_TORCH_CURRENT_SHIFT);
> > +
> > +	ret = regmap_update_bits(chip->regmap, SY7802_REG_TORCH_BRIGHTNESS, mask, level);

So why not kill the single-use 'mask' variable and use a cast version of
led->led_no here instead?

> > +	if (ret)
> > +		goto unlock;
> > +
> > +	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE, enable_mask, val);
> > +	if (ret)
> > +		goto unlock;
> > +
> > +	chip->fled_torch_used = curr;
> > +
> > +unlock:
> > +	mutex_unlock(&chip->mutex);
> > +	return ret;
> > +}

-- 
Lee Jones [李琼斯]