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

Dixit Parmar
2025-07-30T03:44:53Z

On Tue, Jul 29, 2025 at 08:05:13PM +0100, Jonathan Cameron wrote:
> On Tue, 29 Jul 2025 09:19:59 +0530
> Dixit Parmar <dixitparmar19@gmail.com> wrote:
>
> > On Sun, Jul 27, 2025 at 02:05:59PM +0100, Jonathan Cameron wrote:
> > > On Sat, 26 Jul 2025 15:07:01 +0530
> > > Dixit Parmar <dixitparmar19@gmail.com> wrote:
> > >
> > > Hi Dixit,
> > >
> > > Very clean driver for a v1. Nice.
> > >
> > > > The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> > >
> > > Slightly odd wrap. Aim for 75 chars for patch descriptions.
> > >
> > Okay, 75.
> > > > 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.
> > > >
> > > > The device can be configured in to different operating modes by optional
> > > > device-tree "mode" property. Also, the temperature sensing part requires
> > > > raw offset captured at 25°C and that can be specified by "temp-offset"
> > > > optional device-tree property.
> > > >
> > > > While sensor has interrupt pin multiplexed with 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].
> > > >
> > > > Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> > > Tag, so in the tags block (no blank line to the SoB)
> > Sorry, didn't quite get it.
>
> You should have it as Datasheet:
> That will then be a formal 'tag' so belongs alongside the Signed-off-by: etc with no blank
> lines in that block.
>
> Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> Link: https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf #1
> Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
>
> > > > [1] https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf
> > > Link: https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf #1
> > >
> > > So make it a tag with a trailing comment to give the reference number.
> > >
> > > >
> > > > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
>
> > > > +
> > > > +#define TLV493D_DATA_X_GET(b) \
> > > > + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 | \
> > > > + (FIELD_GET(TLV493D_VAL_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4), 11)
> > >
> > > These are odd enough I'd make them c functions rather than macros. Burn a few lines
> > > for better readability.
> > >
> > I saw this kind of data retrival and formation from registers as macros so I sticked to
> > it. Having all these as function will also require a seperate function
> > for each channel coz the masks and the layout of the bits changes over
> > the register. Do you still recommend it as c functions?
>
> Is it more than 4 short functions? I'd burn the few lines that costs.
>
> s32 tlv493d_data_y_get(u8 *buff)
> {
> u16 val = FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
> FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
>
> return sign_extend32(val, 11);
> }
Okay.
Will a single function with channel as arguments will be better?
> > > > +/*
> > > > + * The datasheet mentions the sensor supports only direct byte-stream write starting from
> > > > + * register address 0x0. So for any modification to be made to any write registers, it must
> > > > + * be written starting from the register address 0x0.
> > > > + * I2C write operation should not contain register address in the I2C frame, it should
> > > > + * contains only raw byte stream for the write registers. As below,
> > > > + * I2C Frame: |S|SlaveAddr Wr|Ack|Byte[0]|Ack|Byte[1]|Ack|.....|Sp|
> > > > + */
> > > > +static int tlv493d_write_all_regs(struct tlv493d_data *data)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + if (!data || !data->client)
> > > > + return -EINVAL;
> > > > +
> > > > + /*
> > > > + * As regmap does not provide raw write API which perform I2C write without
> > > > + * specifying register address, direct i2c_master_send() API is used.
> > > > + */
> > > > + ret = i2c_master_send(data->client, data->wr_regs, ARRAY_SIZE(data->wr_regs));
> > >
> > > Given we have to do this, I'm a bit doubtful about regmap usage in general in here.
> > > Maybe it's just not a good fit and we should stick to direct use of the i2c stuff
> > > like here?
> > >
> > Sorry, didn't get entirely? From what I understood, you meant we could
> > drop regmap from this driver entirely and use direct I2C APIs. I believe
> > that would be too much, coz of the frequency we perform operations and
> > regmap is easier and clean imo.
>
> The mixture is nasty though :(
Indeed.
> > Also, we could have used regmap_raw_write() API by specifying register
> > 0x0 as address and rest of the 3 bytes as data. regmap will perform raw
> > write of these byte stream over the I2C the same way sensor expects. But
> > the problem with that approach is we are not using it as per the API
> > convention. let me know your thoughts? Is it a good option, it'll look
> > like this:
> > regmap_raw_write(data->map, data->wr_regs[0], &data->wr_regs[1],
> > ARRAY_SIZE(data->wr_regs) - 1);
>
> I'm not keen on that either.
>
> If you really want to mix i2c and regmap then that's fine, I was just dubious
> whether we were getting value for money from regmap here.
>
Agree. Let's switch to i2c APIs and drop regmap.
> > > > + if (ret < 0) {
> > > > + dev_err(data->dev, "failed to write registers. error %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > + *x = TLV493D_DATA_X_GET(buff);
> > > > + *y = TLV493D_DATA_Y_GET(buff);
> > > > + *z = TLV493D_DATA_Z_GET(buff);
> > > > + *t = TLV493D_DATA_TEMP_GET(buff);
> > > > +
> > > > +out:
> > > > + pm_runtime_mark_last_busy(data->dev);
> > >
> > > As below This should get simpler.
> > >
> > > Not directly relevant to this patch:
> > >
> > > If this cycle is quiet I plan to propose some cleanup.h based handling for runtime
> > > pm as it's annoying how often we need a goto for it. The new ACQUIRE() / ACQUIRE_ERR()
> > > should work for this.
> > >
> > Does this need any modifications with current codebase?
>
> Needs a bunch of work to show generality across many drivers and
> convincing Rafael (PM maintainer) it's a good idea.
> Don't try to do it in this series!
>
> > >
> > > > + pm_runtime_put_autosuspend(data->dev);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int tlv493d_init(struct tlv493d_data *data)
> > > > +{
> > > > + int ret;
> > > > + u8 buff[TLV493D_RD_REG_MAX];
> > > > +
> > > > + if (!data)
> > > > + return -EINVAL;
> > > > +
> > > > + /*
> > > > + * The sensor initialization requires below steps to be followed,
> > > > + * 1. Power-up sensor.
> > > > + * 2. Read and store read-registers map (0x0-0x9).
> > > > + * 3. Copy values from read reserved registers to write reserved fields (0x0-0x3).
> > > > + * 4. Set operating mode.
> > > > + * 5. Write to all registers.
> > > > + */
> > > > + ret = regmap_bulk_read(data->map, TLV493D_RD_REG_BX, buff, ARRAY_SIZE(buff));
> > > > + if (ret) {
> > > > + dev_err(data->dev, "bulk read failed, error %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + data->wr_regs[0] = 0; /* Write register 0x0 is reserved. Does not require to be updated.*/
> > > > + data->wr_regs[1] = buff[TLV493D_RD_REG_RES1] & TLV493D_RD_REG_RES1_WR_MASK;
> > > > + data->wr_regs[2] = buff[TLV493D_RD_REG_RES2] & TLV493D_RD_REG_RES2_WR_MASK;
> > > > + data->wr_regs[3] = buff[TLV493D_RD_REG_RES3] & TLV493D_RD_REG_RES3_WR_MASK;
> > > > +
> > > > + ret = tlv493d_set_operating_mode(data, data->mode);
> > > > + if (ret < 0) {
> > > > + dev_err(data->dev, "failed to set operating mode\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return ret;
> > > return 0?
> > Its the same though. ret from tlv493d_set_operating_mode is 0 on
> > success and -ve on failure.
>
> Then return 0 to make it explicit that if we get here we only return 0.
> That can be useful to compilers.
>
> Also check above as if (ret) is cleaner still.
>
>
> > > > + if (ret)
> > > > + val = 340;
> > > > + /*
> > > > + * The above is a raw offset; however, IIO expects a single effective offset.
> > > > + * Since final temperature includes an additional fixed 25°C (i.e. 25000 m°C),
> > > > + * we compute a combined offset using scale = 1100 (1.1 * 1000).
> > > > + */
> > > > + data->temp_offset = -val + (s32)div_u64(25000, 1100);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int tlv493d_read_raw(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
> > >
> > > wrap to keep this under 80. Doesn't look like it will hurt readability.
> > >
> > Ack. Is 80 standard for whole kernel or iio only?
>
> It's kind of the the 'old' standard. Used to be a fairly hard limit, but
> over time there has been some relaxation. So, if your code is nice and readable
> you will rarely get anyone complaining if you stick to 80 chars.
>
> >
> > Thank you for such detailed review. Appriciate it,
> You are welcome.
>
> J
> > Dixit
>