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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 2 Aug 2018 22:52:00 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Marcus Folkesson <marcus.folkesson@...il.com>
Cc:     Kent Gustavsson <kent@...oris.se>,
        Jonathan Cameron <jic23@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-iio <linux-iio@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] iio: adc: add support for mcp3911

On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
<marcus.folkesson@...il.com> 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@...il.com>

> Signed-off-by: Kent Gustavsson <kent@...oris.se>

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

> + * Copyright (C) 2018 Kent Gustavsson <kent@...oris.se>

> + *

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