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

[Lee Jones <lee@kernel.org>]

On Fri, 09 May 2025, Matthias Fend wrote:

> Hi Lee,
> 
> thank you for your answers and additional explanations.
> Except for one point, I think I understand the rest and will amend it
> accordingly.
> 
> Am 08.05.2025 um 16:31 schrieb Lee Jones:
> > On Fri, 02 May 2025, Matthias Fend wrote:
> > 
> > > Hi Lee,
> > > 
> > > thank you for taking the time for this review.
> > > 
> > > Am 01.05.2025 um 13:03 schrieb Lee Jones:
> > > > On Wed, 23 Apr 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@emfend.at>
> > > > > ---
> > > > >    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_probe(struct i2c_client *client)
> > > > > +{
> > > > > +	struct tps6131x *tps6131x;
> > > > > +	int ret;
> > > > > +
> > > > > +	tps6131x = devm_kzalloc(&client->dev, sizeof(*tps6131x), GFP_KERNEL);
> > > > > +	if (!tps6131x)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	tps6131x->dev = &client->dev;
> > > > > +	i2c_set_clientdata(client, tps6131x);
> > > > 
> > > > If you already have client, to fetch this, you'll already have access to dev.
> > > 
> > > I understand that in principle. However, I'm still not entirely sure what
> > > exactly I should change. Could you please provide me with some further
> > > guidance?
> > 
> > Yes, don't store 'dev' in 'tps6131x'.
> 
> Ah, I see. Yes, the functions currently using 'dev' are all called from the
> probe path, so I could just pass 'dev' as a separate argument and remove it
> from 'tps6131x'.
> But since I now also output a message with dev_err in
> tps6131x_torch_refresh_handler() in case of an error, I need 'tps6131x->dev'
> there. I haven't thought of any other way to get 'dev' here.
> 
> In this context, is it okay for you if 'dev' remains a member of 'tps6131x'?

Ah yes.  Looks like you do need it then.  No problem.

-- 
Lee Jones [李琼斯]