Re: [PATCH v7 2/2] leds: as3668: Driver for the ams Osram 4-channel i2c LED driver

[Lee Jones <lee@kernel.org>]

On Sun, 27 Jul 2025, Lukas Timmermann wrote:

> Formatting was malformed in the last message, sorry. Next try:
> 
> > > +#define AS3668_CHIP_REV1 0x01
> > 
> > How many REVs can one chip have?
> > 
> Would be 4-bit/16. I thought I do a little check about the revision and
> print a warning message to inform about the probably untested revision. Or
> is that not necessary?
> Removing the REV constant results in an if-statement similar to
> if(rev == 1). Is this considered a magic number?

I would omit this until there is another revision.

> > > +static int as3668_read_value(struct i2c_client *client, u8 reg)
> > > +{
> > > +	return i2c_smbus_read_byte_data(client, reg);
> > > +}
> > > +
> > > +static int as3668_write_value(struct i2c_client *client, u8 reg, u8 value)
> > > +{
> > > +	int err = i2c_smbus_write_byte_data(client, reg, value);
> > > +
> > > +	if (err)
> > > +		dev_err(&client->dev, "error writing to reg 0x%02x, returned %d\n", reg, err);
> > > +
> > > +	return err;
> > > +}
> > 
> > These look like abstractions for the sake of abstractions.
> > 
> > Just use the i2c_smbus_*() calls directly.
> > 
> Should I omit the write function as well? I do some error handling there. Is
> it okay to err |= write() the returned error codes in init or should I
> handle every possible write error by itself?

The handling in write() is standard error handling.

It doesn't justify another function.

> > > +	/* Read identifier from chip */
> > > +	chip_id1 = as3668_read_value(client, AS3668_CHIP_ID1);
> > > +
> > > +	if (chip_id1 != AS3668_CHIP_IDENT)
> > > +		return dev_err_probe(&client->dev, -ENODEV,
> > > +				"chip reported wrong id: 0x%02x\n", chip_id1);
> > 
> > Unlikely.  This too is unexpected, as above.
> > 
> Error message not descriptive, understood. Changing that to "unexpected ..."
> as above. Or am I misunderstanding and the check should be omitted entirely?

No, that's fine.

> > > +	/* Check the revision */
> > > +	chip_id2 = as3668_read_value(client, AS3668_CHIP_ID2);
> > 
> > Is child_id2 not for another chip?
> > 
> > This is ambiguous, please improve the variable nomenclature.
> > 
> chip_id2 is directly related to the defined register CHIP_ID2 which name is
> taken from the datasheet of the AS3668. (Not sure if I'm allowed to link it)
> Should we diverge from the datasheet in case of naming?
> Or is only chip_id2 to be renamed, even tho it holds the values of CHIP_ID2?
> I would consider chip_ident for chip_id1 and chip_subident for chip_id2.
> chip_subident would break down into chip_rev and chip_serial.
> Of course reading chip_id2 would be unnecessary if I omit checking the
> revision in the first place (see above).

I would encourage people to match up defines with the datasheet.

Variables should instead be easy to read / maintain.

> > > +	err = as3668_dt_init(as3668);
> > > +	if (err)
> > > +		return dev_err_probe(&client->dev, err, "failed to initialize device\n");
> > 
> > No need for 2 error messages.
> > 
> Doing if(error) return error; then...?

Right.

-- 
Lee Jones [李琼斯]