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

[Dixit Parmar <dixitparmar19@gmail.com>]

On Wed, Aug 20, 2025 at 05:08:38PM +0300, Andy Shevchenko wrote:
> > --- a/drivers/iio/magnetometer/Makefile
> > +++ b/drivers/iio/magnetometer/Makefile
> > @@ -23,6 +23,8 @@ st_magn-$(CONFIG_IIO_BUFFER) += st_magn_buffer.o
> >  obj-$(CONFIG_IIO_ST_MAGN_I2C_3AXIS) += st_magn_i2c.o
> >  obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
> >  
> > +obj-$(CONFIG_INFINEON_TLV493D)		+= tlv493d.o
> > +
> >  obj-$(CONFIG_SENSORS_HMC5843)		+= hmc5843_core.o
> >  obj-$(CONFIG_SENSORS_HMC5843_I2C)	+= hmc5843_i2c.o
> >  obj-$(CONFIG_SENSORS_HMC5843_SPI)	+= hmc5843_spi.o
> 
> I haven't got the ordering rules here and in Kconfig. Can it be alphabetical?
From what I can see, the order is alphabetical based on the CONFIG option in the
Makefile and Kconfig, and I kept CONFIG_INFINEO_TLV493D after CONFIG_IIO_ST*.
Isn't it in correct order? or my understanding is incorrect?
> 
> > +static s16 tlv493d_get_channel_data(u8 *b, enum tlv493d_channels ch)
> > +{
> > +	u16 val;
> > +
> > +	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;
> 
> Wrong indentation, make both 'F':s to be in the same column.
To have 'F' in the same column, it will need spaces after tab (I think its not
advisable to mix spaces and tabs). With just tabs the second FIELD_GET will be
align to 'v' of val. What will be the correct indentation?
> 
> > +static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
> > +				s16 *z, s16 *t)
> 
> Indentation issue. Please, check fully the code for such issues.
I followed the single tab after default as suggested. At which column the s16 *z should be align to?
> > +	/*
> > +	 * 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,
> 
> Please, resplit logically, i.e leave the condition on the single line.
> Also to make it shorter you can use '!' instead of ' == 0'.
Having both conditions in same line will go out of 80 char length limit, even with !.
Is it fine?
> 
> Just wondering if you have tested for negative coordinates, does it propagate
> correctly?
Yes I have validated, it reports correct negative coordinate values.
> > +	iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan),
> > +				pf->timestamp);
> 
> Interestingly that you have used 100 limit and suddenly don't do it here
> and maybe elsewhere. Why inconsistent style? Please, go through the whole
> file and make sure the style is consistent in all of the aspects:
> - C style used
> - comments style (one-line and multi-line)
> - indentation
> - etc.
I tried to follow 80 limit(except few places where it was just on border or not
clear to read). I belive the standard is to use 80 limit(correct me if I referred
wrong place) and I will recheck to meet that.

Thank you for careful review,
Dixit Parmar