Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220623170844.2189814-1-marcus.folkesson@gmail.com> <20220623170844.2189814-10-marcus.folkesson@gmail.com>
In-Reply-To: <20220623170844.2189814-10-marcus.folkesson@gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 23 Jun 2022 21:15:51 +0200
Message-ID: <CAHp75Vf7=Pk7T8ysrFS9u1sKQXUpqczGJkaeX-MLbwpx-iM2rg@mail.gmail.com>
Subject: Re: [PATCH 10/10] iio: adc: mcp3911: add support to set PGA
To:     Marcus Folkesson <marcus.folkesson@gmail.com>
Cc:     Kent Gustavsson <kent@minoris.se>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jun 23, 2022 at 7:41 PM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> Add support for setting the Programmable Gain Amplifiers by adjust the
> scale value.

...

> +       int ret = mcp3911_read(adc, MCP3911_REG_GAIN, val, 1);
> +
> +       if (ret)
> +               return ret;

Please split the assignment.

  int ret;

  ret = ...
  if (ret)


...

> +       *val >>= channel * 3;
> +       *val &= 0x07;

GENMASK() ?

> +       *val = (1 << *val);

Unneeded parentheses, perhaps BIT()?

...

> +                               ret = mcp3911_update(adc, MCP3911_REG_GAIN,
> +                                               MCP3911_GAIN_MASK(channel->channel),
> +                                               MCP3911_GAIN_VAL(channel->channel,
> +                                                       i), 1);

This is not good indentation, at least i), should be on the previous line.

...

> +static int mcp3911_calc_scale_table(struct mcp3911 *adc)
> +{
> +       u32 ref = MCP3911_INT_VREF_MV;
> +       u32 div;

> +       int ret = 0;

Useless assignment.

> +       int tmp0, tmp1;
> +       s64 tmp2;
> +
> +       if (adc->vref) {
> +               ret = regulator_get_voltage(adc->vref);
> +               if (ret < 0) {
> +                       dev_err(&adc->spi->dev,
> +                               "failed to get vref voltage: %d\n",
> +                              ret);

> +                       goto out;

Return directly.

> +               }
> +
> +               ref = ret / 1000;
> +       }
> +
> +       /*
> +        * For 24bit Conversion
> +        * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
> +        * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
> +        */
> +
> +       /* ref = Reference voltage
> +        * div = (2^23 * 1.5 * gain) = 12582912 * gain
> +        */
> +       for (int i = 0; i < MCP3911_NUM_SCALES; i++) {
> +               div = 12582912 * BIT(i);
> +               tmp2 = div_s64((s64)ref * 1000000000LL, div);
> +               tmp1 = div;
> +               tmp0 = (int)div_s64_rem(tmp2, 1000000000, &tmp1);
> +
> +               mcp3911_scale_table[i][0] = 0;
> +               mcp3911_scale_table[i][1] = tmp1;
> +       }

> +out:

The useless label.

> +       return ret;

return 0;

> +}

-- 
With Best Regards,
Andy Shevchenko