linux-kernel - Re: [PATCH 10/10] iio: adc: mcp3911: add support to set PGA

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]

Message-ID: <CAHp75Vf7=Pk7T8ysrFS9u1sKQXUpqczGJkaeX-MLbwpx-iM2rg@mail.gmail.com>
Date:   Thu, 23 Jun 2022 21:15:51 +0200
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>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 10/10] iio: adc: mcp3911: add support to set PGA

On Thu, Jun 23, 2022 at 7:41 PM Marcus Folkesson
<marcus.folkesson@...il.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