Message-ID: <aJcw8icGvsDzFGpJ@dixit>
Date: Sat, 9 Aug 2025 16:58:50 +0530
From: Dixit Parmar <dixitparmar19@...il.com>
To: Andy Shevchenko <andy.shevchenko@...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 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?
> > +       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.
> > +       TLV493D_OP_MODE_FAST,
> > +       TLV493D_OP_MODE_LOWPOWER,
> > +       TLV493D_OP_MODE_ULTRA_LOWPOWER,
> > +       TLV493D_OP_MODE_MASTERCONTROLLED
> > +};
> 
> ...
> 
> > +struct tlv493d_data {
> > +       struct device *dev;
> > +       struct i2c_client *client;
> 
> Why do you need both?
Indeed, I should drop struct device *dev member.
> 
> > +       /* protects from simultaneous sensor access and register readings */
> > +       struct mutex lock;
> > +       enum tlv493d_op_mode mode;
> 
> > +       u8 wr_regs[TLV493D_WR_REG_MAX];
> > +};
> 
> ...
> 
> > +       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.
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.
> > +       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);
> > +}
> 
> ...
> 
> > +static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
> > +                               s16 *z, s16 *t)
> > +{
> > +       u8 buff[7] = {};
> > +       int err, ret;
> > +       u32 sleep_us = tlv493d_sample_rate_us[data->mode];
> > +
> > +       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.
> > +       ret = pm_runtime_resume_and_get(data->dev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       /*
> > +        * Poll until data is valid,
> > +        * For a valid data TLV493D_TEMP_CHANNEL bit of TLV493D_RD_REG_TEMP should be set to 0.
> > +        * The sampling time depends on the sensor mode. poll 3x the time of the sampling time.
> > +        */
> > +       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)?
> > +                       ARRAY_SIZE(buff));
> 
> Missing include for this macro.
> 
> > +       if (ret) {
> > +               dev_err(data->dev, "i2c read poll timeout, error:%d\n", ret);
> > +               goto out;
> > +       }
> > +       if (err < 0) {
> > +               dev_err(data->dev, "i2c read data failed, error:%d\n", err);
> > +               ret = err;
> > +               goto out;
> > +       }
> > +
> > +       *x = tlv493d_get_channel_data(buff, TLV493D_AXIS_X);
> > +       *y = tlv493d_get_channel_data(buff, TLV493D_AXIS_Y);
> > +       *z = tlv493d_get_channel_data(buff, TLV493D_AXIS_Z);
> > +       *t = tlv493d_get_channel_data(buff, TLV493D_TEMPERATURE);
> > +
> > +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?
> > +       pm_runtime_put_autosuspend(data->dev);
> > +       return ret;
> > +}
> 
> ...
> 
> > +       ret = tlv493d_set_operating_mode(data, data->mode);
> > +       if (ret < 0) {
> 
> Is ' < 0' part required here?
> 
> > +               dev_err(data->dev, "failed to set operating mode\n");
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> 
> If not, these all lines can be transformed to just
> 
>   return ret;
> 
Yes, return 0 and the check for ret has been kept as per the previous
review suggestion. The return value from the tlv493d_set_operating_mode
is returned from i2c_master_send() via few function inbetween and its result
has to be conveyed to the caller API as we are in initialization phase.
> > +}
> 
> ...
> 
> > +static irqreturn_t tlv493d_trigger_handler(int irq, void *ptr)
> > +{
> > +       struct iio_poll_func *pf = ptr;
> > +       struct iio_dev *indio_dev = pf->indio_dev;
> > +       struct tlv493d_data *data = iio_priv(indio_dev);
> > +
> > +       struct {
> > +               s16 channels[3];
> > +               s16 temperature;
> > +               aligned_s64 timestamp;
> > +       } scan;
> 
> > +
> 
> No blank lines in the definition block.
> 
> > +       s16 x, y, z, t;
> > +       int ret;
> > +
> > +       ret = tlv493d_get_measurements(data, &x, &y, &z, &t);
> > +       if (ret) {
> > +               dev_err(data->dev, "failed to read sensor data\n");
> > +               goto trig_out;
> > +       }
> > +
> > +       scan.channels[0] = x;
> > +       scan.channels[1] = y;
> > +       scan.channels[2] = z;
> > +       scan.temperature = t;
> > +       iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan),
> > +                               pf->timestamp);
> > +trig_out:
> 
> Make sure you use a consistent pattern for labels.
> 
> out_trigger_notify:
> 
> > +       iio_trigger_notify_done(indio_dev->trig);
> > +
> > +       return IRQ_HANDLED;
> > +}
> 
> ...
> 
> > +       data->dev = dev;
> > +       data->client = client;
> 
> Choose one of them, the other can be derived.
> 
ACK.
> ...
> 
> > +               return dev_err_probe(dev, ret, "failed to initialize\n");
> 
> Missing include for this API.
> 
> ...
> 
> > +static const struct i2c_device_id tlv493d_id[] = {
> > +       { "tlv493d" },
> > +       { }
> > +};
> 
> > +static const struct of_device_id tlv493d_of_match[] = {
> > +       { .compatible = "infineon,tlv493d-a1b6", },
> 
> Inner comma is redundant.
> 
> > +       { }
> > +};
> 
> 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?
> ...
> 
> > +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.
> > +       },
> > +       .probe = tlv493d_probe,
> > +       .id_table = tlv493d_id,
> > +};
> 
> > +
> 
> Remove this blank line.
> 
> > +module_i2c_driver(tlv493d_driver);
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Thank you for careful review,
Dixit

Powered by blists - more mailing lists