Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Lee Jones]

On Fri, 14 Mar 2025, Matthias Fend wrote:

> Hi Lee,
> 
> Am 14.03.2025 um 12:45 schrieb Lee Jones:
> > On Fri, 14 Mar 2025, Matthias Fend wrote:
> > 
> > > Hi Lee,
> > > 
> > > Am 14.03.2025 um 11:52 schrieb Lee Jones:
> > > > On Fri, 14 Mar 2025, Matthias Fend wrote:
> > > > 
> > > > > Hi Lee,
> > > > > 
> > > > > thanks a lot for your feedback!
> > > > > 
> > > > > Am 10.03.2025 um 15:49 schrieb Lee Jones:
> > > > > > On Fri, 28 Feb 2025, Matthias Fend wrote:
> > > > > > 
> > > > > > > The TPS61310/TPS61311 is a flash LED driver with I2C interface. Its power
> > > > > > > stage is capable of supplying a maximum total current of roughly 1500mA.
> > > > > > > The TPS6131x provides three constant-current sinks, capable of sinking up
> > > > > > > to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode. In torch mode
> > > > > > > each sink (LED1, LED2, LED3) supports currents up to 175mA.
> > > > > > > 
> > > > > > > Signed-off-by: Matthias Fend <matthias.fend@xxxxxxxxx>
> > > > > > > ---
> > > > > > >     MAINTAINERS                        |   7 +
> > > > > > >     drivers/leds/flash/Kconfig         |  11 +
> > > > > > >     drivers/leds/flash/Makefile        |   1 +
> > > > > > >     drivers/leds/flash/leds-tps6131x.c | 798 +++++++++++++++++++++++++++++++++++++
> > > > > > >     4 files changed, 817 insertions(+)
> > 
> > [...]
> > 
> > > > > > > +static int tps6131x_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
> > > > > > > +{
> > > > > > > +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > > > > > > +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
> > > > > > > +
> > > > > > > +	guard(mutex)(&tps6131x->lock);
> > > > > > > +
> > > > > > /> +	return tps6131x_set_mode(tps6131x, enable ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
> > > > > > > +				 false);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static const struct v4l2_flash_ops tps6131x_v4l2_flash_ops = {
> > > > > > > +	.external_strobe_set = tps6131x_flash_external_strobe_set,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
> > > > > > > +{
> > > > > > > +	struct v4l2_flash_config v4l2_cfg = { 0 };
> > > > > > > +	struct led_flash_setting *intensity = &v4l2_cfg.intensity;
> > > > > > > +
> > > > > > > +	intensity->min = tps6131x->step_torch_current_ma;
> > > > > > > +	intensity->max = tps6131x->max_torch_current_ma;
> > > > > > > +	intensity->step = tps6131x->step_torch_current_ma;
> > > > > > > +	intensity->val = intensity->min;
> > > > > > > +
> > > > > > > +	strscpy(v4l2_cfg.dev_name, tps6131x->fled_cdev.led_cdev.dev->kobj.name,
> > > > > > 
> > > > > > tps6131x->client->dev?
> > > > > 
> > > > > Do you mean the name should be taken from the I2C device?
> > > > > The current name, for example, is 'white:flash-0', while the I2C device name
> > > > > would be '4-0033'. So I think the current version is appropriate, don't you
> > > > > think?
> > > > 
> > > > No, I'm implying that:
> > > > 
> > > >     tps6131x->client->dev == tps6131x->fled_cdev.led_cdev.dev
> > > > 
> > > > ... and that the former is shorter / neater.
> > > 
> > > Hmm. That's interesting. I thought these were two different devices, which
> > > seems to be actually the case for me. Hence the different names in the kobj.
> > > Are the devices really supposed to be identical?
> > 
> > Interesting.  What are their names?
> 
> tps6131x->fled_cdev.led_cdev.dev: 'white:flash-0'
> tps6131x->client->dev: '4-0033'

Presumably this is an I2C address?  What a helpful device name!

Okay, seeing as they are both clearly different, I think you are
correct.  Scrap this review comment.

-- 
Lee Jones [李琼斯]