Re: [PATCH v3 1/3] iio: adc: add support for mcp3911

[Andy Shevchenko]

On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
<marcus.folkesson@xxxxxxxxx> wrote:
> MCP3911 is a dual channel Analog Front End (AFE) containing two
> synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx>

> Signed-off-by: Kent Gustavsson <kent@xxxxxxxxxx>

What is this? Why it's here (presense and location in the message)?

> + * Copyright (C) 2018 Kent Gustavsson <kent@xxxxxxxxxx>

> + *

Redundant.

> +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> +{
> +       int ret;
> +
> +       reg = MCP3911_REG_READ(reg, adc->dev_addr);
> +       ret = spi_write_then_read(adc->spi, &reg, 1, val, len);
> +       if (ret < 0)
> +               return ret;
> +
> +       be32_to_cpus(val);
> +       *val >>= ((4 - len) * 8);
> +       dev_dbg(&adc->spi->dev, "reading 0x%x from register 0x%x\n", *val,
> +                       reg>>1);
> +       return ret;
> +}
> +
> +static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
> +{
> +       dev_dbg(&adc->spi->dev, "writing 0x%x to register 0x%x\n", val, reg);
> +
> +       val <<= (3 - len) * 8;
> +       cpu_to_be32s(&val);
> +       val |= MCP3911_REG_WRITE(reg, adc->dev_addr);
> +
> +       return spi_write(adc->spi, &val, len + 1);
> +}

Can't you use REGMAP_SPI ?

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

Isn't what we called CS (chip select)?

> +       if (IS_ERR(adc->vref)) {

> +

Redundant.

> +       if (adc->clki)

Seems to be redundant if clock is optional (it should be NULL and API
is NULL-aware).

> +               clk_disable_unprepare(adc->clki);

> +       if (adc->clki)
> +               clk_disable_unprepare(adc->clki);

Ditto.

> +#if defined(CONFIG_OF)

This prevents playing with the device on ACPI enabled platforms.

> +               .of_match_table = of_match_ptr(mcp3911_dt_ids),

Ditto for macro.

-- 
With Best Regards,
Andy Shevchenko