Message-ID: <aImVLWJP08_g23xu@dixit>
Date: Wed, 30 Jul 2025 09:14:45 +0530
From: Dixit Parmar <dixitparmar19@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
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, Jul 29, 2025 at 08:05:13PM +0100, Jonathan Cameron wrote:
> 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);
> }
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
> 

Powered by blists - more mailing lists