[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250731115156.GF1049189@google.com>
Date: Thu, 31 Jul 2025 12:51:56 +0100
From: Lee Jones <lee@...nel.org>
To: Lukas Timmermann <linux@...mermann.space>
Cc: pavel@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, linux-leds@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/2] leds: as3668: Driver for the ams Osram 4-channel
i2c LED driver
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 [李琼斯]
Powered by blists - more mailing lists