Re: [PATCH v4 4/4] iio: adc: mcp3911: add support for the whole MCP39xx family

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Aug 08, 2023 at 01:04:32PM +0200, Marcus Folkesson wrote:
> Microchip does have many similar chips, add support for those.

...

>  	help
> -	  Say yes here to build support for Microchip Technology's MCP3911
> -	  analog to digital converter.
> +	  Say yes here to build support for Microchip Technology's MCP3910,
> +	  MCP3911, MCP3912, MCP3913, MCP3914, MCP3918 and MCP3919
> +	  analog to digital converters.

For maintenance this can be written as

	  Say yes here to build support for one of the following
	  Microchip Technology's analog to digital converters:
	    MCP3910, MCP3911, MCP3912, MCP3913, MCP3914,
	    MCP3918, MCP3919

>  	  This driver can also be built as a module. If so, the module will be
>  	  called mcp3911.

...

> +#define MCP3910_OFFCAL(x)		(MCP3910_REG_OFFCAL_CH0 + x * 6)

Inconsistent macro implementation, i.e. you need to use (x).

...

> +static int mcp3910_get_osr(struct mcp3911 *adc, int *val)
> +{
> +	int ret, osr;

Strictly speaking osr can't be negative, otherwise it's a UB below.

	u32 osr = FIELD_GET(MCP3910_CONFIG0_OSR, *val);
	int ret;

and why val is int?

> +	ret = mcp3911_read(adc, MCP3910_REG_CONFIG0, val, 3);
> +	if (ret)
> +		return ret;
> +
> +	osr = FIELD_GET(MCP3910_CONFIG0_OSR, *val);
> +	*val = 32 << osr;
> +	return ret;
> +}

...

> +static int mcp3910_set_osr(struct mcp3911 *adc, int val)
> +{

> +	int osr;
> +
> +	osr = FIELD_PREP(MCP3910_CONFIG0_OSR, val);

Can be on one line.

> +	return mcp3911_update(adc, MCP3910_REG_CONFIG0,
> +			      MCP3910_CONFIG0_OSR, osr, 3);
> +}

...

> +static int mcp3911_set_osr(struct mcp3911 *adc, int val)
> +static int mcp3911_get_osr(struct mcp3911 *adc, int *val)

As per above comments.

...

> +	if (adc->vref) {
> +		dev_dbg(&adc->spi->dev, "use external voltage reference\n");
> +		regval |= FIELD_PREP(MCP3910_CONFIG1_VREFEXT, 1);
> +	} else {

> +		dev_dbg(&adc->spi->dev,
> +			"use internal voltage reference (1.2V)\n");


As per previous patch comments

		dev_dbg(dev, "use internal voltage reference (1.2V)\n");

> +		regval |= FIELD_PREP(MCP3910_CONFIG1_VREFEXT, 0);
> +	}

...

> +	if (adc->clki) {
> +		dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
> +		regval |= FIELD_PREP(MCP3910_CONFIG1_CLKEXT, 1);
> +	} else {
> +		dev_dbg(&adc->spi->dev,
> +			"use crystal oscillator as clocksource\n");

Ditto.

> +		regval |= FIELD_PREP(MCP3910_CONFIG1_CLKEXT, 0);
> +	}

...

> +	if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))

This also becomes shorter.

One trick to make it even shorter:

	if (device_property_present(dev, "microchip,data-ready-hiz"))

> +		regval |= FIELD_PREP(MCP3910_STATUSCOM_DRHIZ, 0);
> +	else
> +		regval |= FIELD_PREP(MCP3910_STATUSCOM_DRHIZ, 1);

...

> +	ret = device_property_read_u32(&spi->dev, "microchip,device-addr", &adc->dev_addr);

I would move it after the comment. It will be more visible what we are
expecting and what the legacy is.


> +	/*
> +	 * Fallback to "device-addr" due to historical mismatch between
> +	 * dt-bindings and implementation.
> +	 */

	ret = device_property_read_u32(dev, "microchip,device-addr", &adc->dev_addr);

>  	if (ret)
> -		return ret;
> +		device_property_read_u32(&spi->dev, "device-addr", &adc->dev_addr);

> +	if (adc->dev_addr > 3) {
> +		dev_err(&spi->dev,
> +			"invalid device address (%i). Must be in range 0-3.\n",
> +			adc->dev_addr);
> +		return -EINVAL;

		return dev_err_probe(...);

> +	}

...

> +	dev_dbg(&spi->dev, "use device address %i\n", adc->dev_addr);

Is it useful?

-- 
With Best Regards,
Andy Shevchenko