Re: [PATCH 2/2 v2] iio: magnetometer: Add driver for Yamaha YAS5xx

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Fri 27 Nov 18:40 CST 2020, Linus Walleij wrote:

> This adds an IIO magnetometer driver for the Yamaha
> YAS53x magnetometer/compass chips YAS530 and YAS532.
> A quick survey of the source code released by different
> vendors reveal that we have these variants in the family
> with some deployments listed:
> 
>  * YAS529 MS-3C (2005 Samsung Aries)
>  * YAS530 MS-3E (2011 Samsung Galaxy S Advance)
>  * YAS532 MS-3R (2011 Samsung Galaxy S4)
>  * YAS533 MS-3F (Vivo 1633, 1707, V3, Y21L)
>  * (YAS534 is a magnetic switch)
>  * YAS535 MS-6C
>  * YAS536 MS-3W
>  * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5)
>  * YAS539 MS-3S (2018 Samsung Galaxy A7 SM-A750FN)
> 
> The YAS529 is so significantly different from the
> YAS53x variants that it will require its own driver.
> The YAS537 and YAS539 have slightly different register
> sets but have strong similarities so a common driver
> will probably be reasonable.
> 
> The source code for Samsung Galaxy A7's YAS539 is not
> that significantly different from the YAS530 in the
> Galaxy S Advance, so I believe we will only need this
> one driver with quirks to handle all of them.
> 
> The YAS539 is actively announced on Yamaha's devices
> site:
> https://device.yamaha.com/en/lsi/products/e_compass/
> 
> This is a driver written from scratch using buffered
> IIO and runtime PM handling regulators and reset.
> 

Looks quite nice, just spotted some small things as I was skimming
through the patch.

> Cc: phone-devel@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v3:
> - This is posted along with the DT bindings which are
>   in v3 so just number everything as v3.

$subject still says v2...

[..]
> diff --git a/drivers/iio/magnetometer/yamaha-yas53x.c b/drivers/iio/magnetometer/yamaha-yas53x.c
[..]
> +/* On YAS532 the x, y1 and y2 values are 13 bits */
> +static u16 yas532_extract_axis(u8 *data)
> +{
> +	u16 val;
> +
> +	/*
> +	 * These are the bits used in a 16bit word:
> +	 * 15 14 13 12 11 10 9  8  7  6  5  4  3  2  1  0
> +	 *    x  x  x  x  x  x  x  x  x  x  x  x  x
> +	 */
> +	val = get_unaligned_be16(&data[0]);
> +	val >>= 2;
> +	val &= GENMASK(12, 0);

Wouldn't it be easier to follow if you GENMASK out the bits you document
above, then shift them right?

> +	return val;
> +}
[..]
> +/**
> + * yas5xx_measure() - Make a measure from the hardware*
> + * @yas5xx: The device state
> + * @t: the raw temperature measurement
> + * @x: the raw x axis measurement
> + * @y1: the y1 axis measurement
> + * @y2: the y2 axis measurement

* Return: 

To complete the kerneldoc.

> + */
> +static int yas5xx_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y2)
> +{
[..]
> +/**

This will result in someone feeling inclined to send a patch to fix the
incomplete kerneldoc.

So please either fill it out, or drop the second '*'.

> + * yas5xx_get_measure() - Measure a sample of all axis and process
> + *
> + * Returned valued are in nanotesla according to some code.
> + */
> +static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
> +{
[..]
> +static int __maybe_unused yas5xx_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct yas5xx *yas5xx = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(yas5xx->regs), yas5xx->regs);
> +	if (ret) {
> +		dev_err(dev, "cannot enable regulators\n");

regulator_bulk_enable() will log which of the regs it failed ot enable,
so you can omit this.

Regards,
Bjorn