Re: [PATCH v4 2/2] iio: magnetometer: si7210: add driver for Si7210

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 16 Jan 2025 10:26:55 +0200
Andy Shevchenko <andy@kernel.org> wrote:

> On Wed, Jan 15, 2025 at 09:16:22PM +0100, Antoni Pokusinski wrote:
> > Silicon Labs Si7210 is an I2C Hall effect magnetic position and
> > temperature sensor. The driver supports the following functionalities:
> > * reading the temperature measurements
> > * reading the magnetic field measurements in a single-shot mode
> > * choosing the magnetic field measurement scale (20 or 200 mT)  
> 
> ...
> 
> > +obj-$(CONFIG_SI7210) 			+= si7210.o  
> 
> Looks like TAB/space mixture in the middle.
> 
> ...
> 
> > +#include <asm/byteorder.h>  
> 
> asm/* usually goes after linux/*
> 
> > +#include <linux/array_size.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/math64.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/types.h>
> > +#include <linux/units.h>  
> 
> ...
> 
> Despite a good formatting I would still add a comment with a formula in
> math-human-readable form.

The rest of the comments are the sort of thing I might tweak.
This one not so much as I'll probably get the maths wrong.

Given where we are with the cycle (too late 6.14, too early for 6.15
beyond me queuing it up in testing for autobuilders to get an early lok)
we have plenty of time so I'd prefer to pick up a v5 with this comment
added and the other minor stuff tidied up.

FWIW I took a look at the overall driver and have nothing to add
in the way of review comments!

Jonathan

> 
> > +		temp = FIELD_GET(GENMASK(14, 3), dspsig);
> > +		temp = div_s64(-383 * temp * temp, 100) + 160940 * temp - 279800000;
> > +		temp *= (1 + (data->temp_gain / 2048));
> > +		temp += (int)(MICRO / 16) * data->temp_offset;  
> 
> > +		ret = regulator_get_voltage(data->vdd);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		temp -= 222 * div_s64(ret, MILLI);  
> 
> Including this piece.
> 
> > +		*val = div_s64(temp, MILLI);  
> 
> ...
> 
> > +static int si7210_set_scale(struct si7210_data *data, unsigned int scale)
> > +{
> > +	s8 *a_otp_values;
> > +	int ret;
> > +
> > +	if (scale == 20)
> > +		a_otp_values = data->scale_20_a;
> > +	else if (scale == 200)
> > +		a_otp_values = data->scale_200_a;
> > +	else
> > +		return -EINVAL;
> > +
> > +	guard(mutex)(&data->fetch_lock);
> > +
> > +	/* Write the registers 0xCA - 0xCC */
> > +	ret = regmap_bulk_write(data->regmap, SI7210_REG_A0, a_otp_values, 3);
> > +	if (ret)
> > +		return ret;  
> 
> > +	/* Write the registers 0xCE - 0xD0 */
> > +	ret = regmap_bulk_write(data->regmap, SI7210_REG_A3, &a_otp_values[3], 3);
> > +	if (ret)
> > +		return ret;  
> 
> Just to be sure I understand the above. There are two of 24-bit values or there are
> two sets of 3 byte arrays? How does datasheet refers to them? What does common sense
> tell us here?
> 
> > +	data->curr_scale = scale;
> > +
> > +	return 0;
> > +}  
> 
> ...
> 
> Overall LGTM, there is no need for resend as I believe the three things above
> may be tweaked by Jonathan. The last one can go even if there are 2 24-bit
> values, but ideally in that case we should use those as a such and apply
> put_unaligned_be24/le24() whichever suits better.
>