Message-ID: <CAHp75Vc7Jftvmgb0EgnYmiKtT2TTYb2uQGNgaqm7hvkFWpJ9cg@mail.gmail.com>
Date: Sat, 9 Aug 2025 14:44:00 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Dixit Parmar <dixitparmar19@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>, 
	Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 1/2] iio: magnetometer: add support for Infineon
 TLV493D 3D Magentic sensor

On Sat, Aug 9, 2025 at 1:29 PM Dixit Parmar <dixitparmar19@...il.com> wrote:
> On Thu, Aug 07, 2025 at 10:57:16PM +0200, Andy Shevchenko wrote:
> > On Thu, Aug 7, 2025 at 4:57 AM Dixit Parmar <dixitparmar19@...il.com> wrote:

...

> > > +enum tlv493d_channels {
> > > +       TLV493D_AXIS_X = 0,
> >
> > Why assignment? Is this HW defined value? Then you must assign all of
> > them explicitly to make code robust to changes.
> >
> No, this is not HW defined value, these are used for iio channel and
> some internall indexing. Most of the driver I referred had this enum
> having assigned to 0 which i think gives clear intention and better
> understanding. either I can keep as it is assuming its good for
> readabilty or keep it unassigned. What do you suggest?

Read C standard?
If it's only driver specific things and not related to HW, assigning
to any value makes no sense, The only requirement here they should be
unique and that's what enum does.

> > > +       TLV493D_AXIS_Y,
> > > +       TLV493D_AXIS_Z,
> > > +       TLV493D_TEMPERATURE
> > > +};
> > > +
> > > +enum tlv493d_op_mode {
> > > +       TLV493D_OP_MODE_POWERDOWN = 0,
> >
> > Ditto.
> >
> Same as above. Just different usecase as this is driver specific enums.

Same as above.

> > > +       TLV493D_OP_MODE_FAST,
> > > +       TLV493D_OP_MODE_LOWPOWER,
> > > +       TLV493D_OP_MODE_ULTRA_LOWPOWER,
> > > +       TLV493D_OP_MODE_MASTERCONTROLLED
> > > +};

...

> > > +       data->wr_regs[TLV493D_WR_REG_MODE1] |= mode1_cfg;
> > > +       data->wr_regs[TLV493D_WR_REG_MODE2] |= mode2_cfg;
> >
> > No mask for the existing values in the respective wr_regs? Wouldn't
> > you need to use FIELD_MODIFY() instead?
> >
> I believe, we are doing OR op with the value created using FIELD_PREP,
> so it should not interefere with the existing non-masked values.

I am talking about existing values in the array.

> However, as FIELD_MODIFY is there, I should utilize it.

...

> > > +static s16 tlv493d_get_channel_data(u8 *b, enum tlv493d_channels ch)
> > > +{
> > > +       u16 val = 0;
> >
> > I would move the default assignment to the 'default' case. This makes
> > the intention clearer.
> >
> As per the suggestion on privious version of the patch, we are having
> ch datatype as enum and as suggested, with enum as swicth-case, it
> should not have default case. so I think this initialisation to 0 at the
> beginning should be fine.

It will make no sense. Please, remove it. and perhaps the compiler
won't warn, otherwise the default case will be needed.

> > > +       switch (ch) {
> > > +       case TLV493D_AXIS_X:
> > > +               val = FIELD_GET(TLV493D_BX_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 |
> > > +                       FIELD_GET(TLV493D_BX2_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4;
> > > +               break;
> > > +       case TLV493D_AXIS_Y:
> > > +               val = FIELD_GET(TLV493D_BY_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
> > > +                       FIELD_GET(TLV493D_BX2_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
> > > +               break;
> > > +       case TLV493D_AXIS_Z:
> > > +               val = FIELD_GET(TLV493D_BZ_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 |
> > > +                       FIELD_GET(TLV493D_BZ2_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]);
> > > +               break;
> > > +       case TLV493D_TEMPERATURE:
> > > +               val = FIELD_GET(TLV493D_TEMP_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 |
> > > +                       FIELD_GET(TLV493D_TEMP2_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]);
> > > +               break;
> > > +       }
> > > +
> > > +       return sign_extend32(val, 11);
> > > +}

...

> > > +       guard(mutex)(&data->lock);
> >
> > No include for this API.
> >
> General question for all the include related suggestions, all the
> required headers are being included by one of the included header(i2c.h,
> iio.h etc), in such case, is it necessary to have specific include for
> given API mentioned in source file? Will it not make it more clumsy
> in terms of repeatative header includes? I understand having all the
> includes mentioned in given source file makes it clear to understand the
> dependency the driver is having. Just want to undertand it bit more as
> learning.

It's called the IWYU principle.
Read more about it here: https://include-what-you-use.org/

...

> > > +       ret = read_poll_timeout(i2c_master_recv, err, err ||
> > > +                       FIELD_GET(TLV493D_TEMP_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> > > +                       sleep_us, (3 * sleep_us), false, data->client, buff,
> >
> > Redundant parentheses.
> >
> For (3 * sleep_us)?

Yes.

> > > +                       ARRAY_SIZE(buff));

...

> > > +out:
> >
> > Labels are better made when they define what they are going to perform.
> >
> > out_put_autosuspend:
> >
> Does it mean it should have whatever is being skipped in the flow?

I don't understand the question. They should answer the question:
"What will happen if I goto $LABEL?"

> > > +       pm_runtime_put_autosuspend(data->dev);
> > > +       return ret;

...

> > > +       data->dev = dev;
> > > +       data->client = client;
> >
> > Choose one of them, the other can be derived.
> >
> ACK.

Many of the comments were being aimply ignored. Are they ACKed or
what? The rule of thumb is to completely remove the pieces you fully
agree with, this will reduce churn in the emails and make a reviewer
happier.

...

> > Missing include for both of the ID tables.
> The ID tables are defined mod_devicetable.h in which intern gets
> included in i2c.h and i2c.h is already included in this driver file,
> should I explicitely include mod_devicetable.h here?

See above, follow IWYU.

...

> > > +static struct i2c_driver tlv493d_driver = {
> > > +       .driver = {
> > > +               .name = "tlv493d",
> > > +               .of_match_table = tlv493d_of_match,
> >
> > > +               .pm = pm_ptr(&tlv493d_pm_ops),
> >
> > Missing include for this macro I believe.
> >
> No I guess. DEFINE_RUNTIME_DEV_PM_OPS is part of pm_runtime.h and its
> already included.

And how is it related to my comment _here_ in the code?

> > > +       },
> > > +       .probe = tlv493d_probe,
> > > +       .id_table = tlv493d_id,
> > > +};

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists