Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 8 Aug 2023 17:23:39 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Marcus Folkesson <marcus.folkesson@gmail.com>
Cc:     Kent Gustavsson <kent@minoris.se>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Cosmin Tanislav <demonsingur@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>,
        ChiYuan Huang <cy_huang@richtek.com>,
        Haibo Chen <haibo.chen@nxp.com>,
        Ramona Bolboaca <ramona.bolboaca@analog.com>,
        Ibrahim Tilki <Ibrahim.Tilki@analog.com>,
        ChiaEn Wu <chiaen_wu@richtek.com>,
        William Breathitt Gray <william.gray@linaro.org>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 4/4] iio: adc: mcp3911: add support for the whole
 MCP39xx family
Message-ID: <ZNJP6xpOvRJigtMx@smile.fi.intel.com>
References: <20230808110432.240773-1-marcus.folkesson@gmail.com>
 <20230808110432.240773-4-marcus.folkesson@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230808110432.240773-4-marcus.folkesson@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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