Messages in this thread | | | Date | Tue, 8 Aug 2023 17:23:39 +0300 | From | Andy Shevchenko <> | 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
| |