[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aJc0rZmfc_zSzaG_@dixit>
Date: Sat, 9 Aug 2025 17:14:45 +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:
> >
> > 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
Powered by blists - more mailing lists