[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 8 Aug 2023 17:23:39 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Marcus Folkesson <marcus.folkesson@...il.com>
Cc: Kent Gustavsson <kent@...oris.se>,
Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Cosmin Tanislav <demonsingur@...il.com>,
Arnd Bergmann <arnd@...db.de>,
ChiYuan Huang <cy_huang@...htek.com>,
Haibo Chen <haibo.chen@....com>,
Ramona Bolboaca <ramona.bolboaca@...log.com>,
Ibrahim Tilki <Ibrahim.Tilki@...log.com>,
ChiaEn Wu <chiaen_wu@...htek.com>,
William Breathitt Gray <william.gray@...aro.org>,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 4/4] iio: adc: mcp3911: add support for the whole
MCP39xx family
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
Powered by blists - more mailing lists