lkml.org 
[lkml]   [2022]   [Sep]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX conversion
On Wed, 31 Aug 2022 15:40:49 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> 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) */;

This made me look a bit closer at the code.
rx_buf isn't necessarily aligned...
This may be a good usecase for an anonymous union.

Which would be fine except one of the callbacks (and original
code) uses be32_to_cpup() which is not unaligned safe.

I'm not keen to push unrelated work onto someone doing good stuff
on a driver, but in this particular case it does seem reasonable to
tidy all this up given you are moving the code anyway.

Jonathan

>
> ...
>
> > + 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.
>
> > },
>

\
 
 \ /
  Last update: 2022-09-04 18:50    [W:0.106 / U:0.960 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site