Message-ID: <20250729200513.275e0d98@jic23-huawei>
Date: Tue, 29 Jul 2025 20:05:13 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Dixit Parmar <dixitparmar19@...il.com>
Cc: 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 1/2] iio: magnetometer: add support for Infineon TLV493D
 3D Magentic sensor

On Tue, 29 Jul 2025 09:19:59 +0530
Dixit Parmar <dixitparmar19@...il.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@...il.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: <link>
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@...il.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@...il.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);
}
> > > +/*
> > > + * 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 :( 
> 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.

> > > +	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

Powered by blists - more mailing lists