Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 31 Aug 2022 15:40:49 +0300 | Subject | Re: [PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX conversion |
| |
On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch <vincent.whitchurch@axis.com> wrote: > > Replace the device_index switch with callbacks from the chip_info > structure, so that the latter has all the information needed to handle > the variants.
Below are for the further patches, as I see the original code has the same disadvantages.
...
> struct mcp320x_chip_info { > const struct iio_chan_spec *channels; > unsigned int num_channels; > unsigned int resolution;
> unsigned int conv_time; /* usec */
Instead of a comment, I would rename it to conv_time_us.
> + int (*convert_rx)(struct mcp320x *adc); > };
...
> + return adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3;
> + return adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6;
> + return adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1;
> + return adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4;
> + return sign_extend32((adc->rx_buf[0] & 0x1f) << 8 | adc->rx_buf[1], 12);
All above should really use
u16 buf = be16_to_cpu(&adc->rx_buf[0]);
return buf >> 3 /* 6, 1, 4 (respectively) */;
...
> + if (raw & BIT(22) && raw & BIT(23))
> + return -EIO; /* cannot have overrange AND underrange */ > + else if (raw & BIT(22))
Redundant 'else'.
> + raw &= ~BIT(22); /* overrange */ > + else if (raw & BIT(23) || raw & BIT(21))
if (raw & (BIT(23) | BIT(21))) ?
> + raw |= GENMASK(31, 22); /* underrange or negative */
...
> [mcp3201] = { > .channels = mcp3201_channels, > .num_channels = ARRAY_SIZE(mcp3201_channels), > + .convert_rx = mcp3201_convert_rx,
> .resolution = 12
+ Comma in such lines.
> },
-- With Best Regards, Andy Shevchenko
| |