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
