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

[Andy Shevchenko]

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