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

From: Andy Shevchenko
Date: Thu Aug 02 2018 - 15:52:05 EST


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