Re: [PATCH v3 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Dixit Parmar <dixitparmar19@gmail.com>]

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@gmail.com> wrote:
> >
> > The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> > applications includes joysticks, control elements (white goods,
> > multifunction knops), or electric meters (anti tampering) and any
> > other application that requires accurate angular measurements at
> > low power consumptions.
> >
> > The Sensor is configured over I2C, and as part of Sensor measurement
> > data it provides 3-Axis magnetic fields and temperature core measurement.
> >
> > The driver supports raw value read and buffered input via external trigger
> > to allow streaming values with the same sensing timestamp.
> >
> > While the sensor has an interrupt pin multiplexed with an I2C SCL pin.
> > But for bus configurations interrupt(INT) is not recommended, unless timing
> > constraints between I2C data transfers and interrupt pulses are monitored
> > and aligned.
> >
> > The Sensor's I2C register map and mode information is described in product
> > User Manual [1].
> 
> ...
> 
> > +       help
> > +         Say Y here to add support for the Infineon TLV493D-A1B6 Low-
> > +         Power 3D Megnetic Sensor.
> 
> Megnetic?
> 
> > +         This driver can also be compiled as a module.
> > +         To compile this driver as a module, choose M here: the module
> > +         will be called tlv493d.
> 
> ...
> 
> > +#define TLV493D_RD_REG_BX      0x00
> > +#define TLV493D_RD_REG_BY      0x01
> > +#define TLV493D_RD_REG_BZ      0x02
> > +#define TLV493D_RD_REG_TEMP    0x03
> > +#define TLV493D_RD_REG_BX2     0x04
> > +#define TLV493D_RD_REG_BZ2     0x05
> > +#define TLV493D_RD_REG_TEMP2   0x06
> > +#define TLV493D_RD_REG_RES1    0x07
> > +#define TLV493D_RD_REG_RES2    0x08
> > +#define TLV493D_RD_REG_RES3    0x09
> > +#define TLV493D_RD_REG_MAX     0x0a
> 
> + blank line
> 
> > +#define TLV493D_WR_REG_RES     0x00
> 
> I would name it _RES0 in analogue with the _RES2 below.
>
We are not using these TLV493D_WR_REG_RES* registers anywhere,
so I shall drop TLV493D_WR_REG_RES2 too.
> > +#define TLV493D_WR_REG_MODE1   0x01
> > +#define TLV493D_WR_REG_RES2    0x02
> > +#define TLV493D_WR_REG_MODE2   0x03
> > +#define TLV493D_WR_REG_MAX     0x04
> 
> ...
> 
> > +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.
> 
> > +       TLV493D_AXIS_Y,
> > +       TLV493D_AXIS_Z,
> > +       TLV493D_TEMPERATURE
> > +};
> > +
> > +enum tlv493d_op_mode {
> > +       TLV493D_OP_MODE_POWERDOWN = 0,
> 
> Ditto.
> 
> > +       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?
> 
> > +       /* 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?
> 
> ...
> 
> > +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.
> 
> > +       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.
> 
> > +       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.
> 
> > +                       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:
> 
> > +       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;
> 
> > +}
> 
> ...
> 
> > +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.
> 
> ...
> 
> > +               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.
> 
> ...
> 
> > +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.
> 
> > +       },
> > +       .probe = tlv493d_probe,
> > +       .id_table = tlv493d_id,
> > +};
> 
> > +
> 
> Remove this blank line.
> 
> > +module_i2c_driver(tlv493d_driver);
> 
> -- 
> With Best Regards,
> Andy Shevchenko