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.
Signed-off-by: Vincent Whitchurch <[email protected]>
---
drivers/iio/adc/mcp320x.c | 115 ++++++++++++++++++++++----------------
1 file changed, 67 insertions(+), 48 deletions(-)
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index b4c69acb33e3..c71d90babb39 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -61,11 +61,14 @@ enum {
mcp3553,
};
+struct mcp320x;
+
struct mcp320x_chip_info {
const struct iio_chan_spec *channels;
unsigned int num_channels;
unsigned int resolution;
unsigned int conv_time; /* usec */
+ int (*convert_rx)(struct mcp320x *adc);
};
/**
@@ -96,6 +99,54 @@ struct mcp320x {
u8 rx_buf[4];
};
+static int mcp3001_convert_rx(struct mcp320x *adc)
+{
+ return adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3;
+}
+
+static int mcp3002_convert_rx(struct mcp320x *adc)
+{
+ return adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6;
+}
+
+static int mcp3201_convert_rx(struct mcp320x *adc)
+{
+ return adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1;
+}
+
+static int mcp3202_convert_rx(struct mcp320x *adc)
+{
+ return adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4;
+}
+
+static int mcp3301_convert_rx(struct mcp320x *adc)
+{
+ return sign_extend32((adc->rx_buf[0] & 0x1f) << 8 | adc->rx_buf[1], 12);
+}
+
+static int mcp3550_convert_rx(struct mcp320x *adc)
+{
+ u32 raw = be32_to_cpup((__be32 *)adc->rx_buf);
+
+ if (!(adc->spi->mode & SPI_CPOL))
+ raw <<= 1; /* strip Data Ready bit in SPI mode 0,0 */
+
+ /*
+ * If the input is within -vref and vref, bit 21 is the sign.
+ * Up to 12% overrange or underrange are allowed, in which case
+ * bit 23 is the sign and bit 0 to 21 is the value.
+ */
+ raw >>= 8;
+ if (raw & BIT(22) && raw & BIT(23))
+ return -EIO; /* cannot have overrange AND underrange */
+ else if (raw & BIT(22))
+ raw &= ~BIT(22); /* overrange */
+ else if (raw & BIT(23) || raw & BIT(21))
+ raw |= GENMASK(31, 22); /* underrange or negative */
+
+ return (s32)raw;
+}
+
static int mcp320x_channel_to_tx_data(int device_index,
const unsigned int channel, bool differential)
{
@@ -120,6 +171,7 @@ static int mcp320x_channel_to_tx_data(int device_index,
static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
bool differential, int device_index, int *val)
{
+ const struct mcp320x_chip_info *info = adc->chip_info;
int ret;
if (adc->chip_info->conv_time) {
@@ -140,55 +192,9 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
if (ret < 0)
return ret;
- switch (device_index) {
- case mcp3001:
- *val = (adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3);
- return 0;
- case mcp3002:
- case mcp3004:
- case mcp3008:
- *val = (adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6);
- return 0;
- case mcp3201:
- *val = (adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1);
- return 0;
- case mcp3202:
- case mcp3204:
- case mcp3208:
- *val = (adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4);
- return 0;
- case mcp3301:
- *val = sign_extend32((adc->rx_buf[0] & 0x1f) << 8
- | adc->rx_buf[1], 12);
- return 0;
- case mcp3550_50:
- case mcp3550_60:
- case mcp3551:
- case mcp3553: {
- u32 raw = be32_to_cpup((__be32 *)adc->rx_buf);
-
- if (!(adc->spi->mode & SPI_CPOL))
- raw <<= 1; /* strip Data Ready bit in SPI mode 0,0 */
+ *val = info->convert_rx(adc);
- /*
- * If the input is within -vref and vref, bit 21 is the sign.
- * Up to 12% overrange or underrange are allowed, in which case
- * bit 23 is the sign and bit 0 to 21 is the value.
- */
- raw >>= 8;
- if (raw & BIT(22) && raw & BIT(23))
- return -EIO; /* cannot have overrange AND underrange */
- else if (raw & BIT(22))
- raw &= ~BIT(22); /* overrange */
- else if (raw & BIT(23) || raw & BIT(21))
- raw |= GENMASK(31, 22); /* underrange or negative */
-
- *val = (s32)raw;
- return 0;
- }
- default:
- return -EINVAL;
- }
+ return 0;
}
static int mcp320x_read_raw(struct iio_dev *indio_dev,
@@ -302,51 +308,61 @@ static const struct mcp320x_chip_info mcp320x_chip_infos[] = {
[mcp3001] = {
.channels = mcp3201_channels,
.num_channels = ARRAY_SIZE(mcp3201_channels),
+ .convert_rx = mcp3001_convert_rx,
.resolution = 10
},
[mcp3002] = {
.channels = mcp3202_channels,
.num_channels = ARRAY_SIZE(mcp3202_channels),
+ .convert_rx = mcp3002_convert_rx,
.resolution = 10
},
[mcp3004] = {
.channels = mcp3204_channels,
.num_channels = ARRAY_SIZE(mcp3204_channels),
+ .convert_rx = mcp3002_convert_rx,
.resolution = 10
},
[mcp3008] = {
.channels = mcp3208_channels,
.num_channels = ARRAY_SIZE(mcp3208_channels),
+ .convert_rx = mcp3002_convert_rx,
.resolution = 10
},
[mcp3201] = {
.channels = mcp3201_channels,
.num_channels = ARRAY_SIZE(mcp3201_channels),
+ .convert_rx = mcp3201_convert_rx,
.resolution = 12
},
[mcp3202] = {
.channels = mcp3202_channels,
.num_channels = ARRAY_SIZE(mcp3202_channels),
+ .convert_rx = mcp3202_convert_rx,
.resolution = 12
},
[mcp3204] = {
.channels = mcp3204_channels,
.num_channels = ARRAY_SIZE(mcp3204_channels),
+ .convert_rx = mcp3202_convert_rx,
.resolution = 12
},
[mcp3208] = {
.channels = mcp3208_channels,
.num_channels = ARRAY_SIZE(mcp3208_channels),
+ .convert_rx = mcp3202_convert_rx,
.resolution = 12
},
[mcp3301] = {
.channels = mcp3201_channels,
.num_channels = ARRAY_SIZE(mcp3201_channels),
+ .convert_rx = mcp3301_convert_rx,
.resolution = 13
},
[mcp3550_50] = {
.channels = mcp3201_channels,
.num_channels = ARRAY_SIZE(mcp3201_channels),
+ .convert_rx = mcp3550_convert_rx,
.resolution = 21,
/* 2% max deviation + 144 clock periods to exit shutdown */
.conv_time = 80000 * 1.02 + 144000 / 102.4,
@@ -354,18 +370,21 @@ static const struct mcp320x_chip_info mcp320x_chip_infos[] = {
[mcp3550_60] = {
.channels = mcp3201_channels,
.num_channels = ARRAY_SIZE(mcp3201_channels),
+ .convert_rx = mcp3550_convert_rx,
.resolution = 21,
.conv_time = 66670 * 1.02 + 144000 / 122.88,
},
[mcp3551] = {
.channels = mcp3201_channels,
.num_channels = ARRAY_SIZE(mcp3201_channels),
+ .convert_rx = mcp3550_convert_rx,
.resolution = 21,
.conv_time = 73100 * 1.02 + 144000 / 112.64,
},
[mcp3553] = {
.channels = mcp3201_channels,
.num_channels = ARRAY_SIZE(mcp3201_channels),
+ .convert_rx = mcp3550_convert_rx,
.resolution = 21,
.conv_time = 16670 * 1.02 + 144000 / 122.88,
},
--
2.34.1
On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
<[email protected]> 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
On Wed, Aug 31, 2022 at 02:40:49PM +0200, Andy Shevchenko wrote:
> On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
> <[email protected]> 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.
Right, these comments are in the original code that is either being just
being moved or that even just happens to be in the context of the diff.
Just to clarify, do you mean by "further patches" that you expect
patches to fix these comments to be added to this series which adds
triggered buffer support?
On Wed, Aug 31, 2022 at 5:19 PM Vincent Whitchurch
<[email protected]> wrote:
> On Wed, Aug 31, 2022 at 02:40:49PM +0200, Andy Shevchenko wrote:
> > On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
> > <[email protected]> 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.
>
> Right, these comments are in the original code that is either being just
> being moved or that even just happens to be in the context of the diff.
>
> Just to clarify, do you mean by "further patches" that you expect
> patches to fix these comments to be added to this series which adds
> triggered buffer support?
Yes.
--
With Best Regards,
Andy Shevchenko
On Wed, 31 Aug 2022 15:40:49 +0300
Andy Shevchenko <[email protected]> wrote:
> On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
> <[email protected]> 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.
>
> > },
>
On Sun, Sep 04, 2022 at 06:15:59PM +0200, Jonathan Cameron wrote:
> 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.
Well, even the moving of the code is unrelated to the original goal of
adding triggered buffered support and isn't necessary for that. The
moving of the code was only to eliminate the use of the "device_index",
which was already used in the existing code.
I'm of course happy to fix problems with the code I'm actually adding,
but it seems to me that it would really be simpler for everyone if the
trivial comments (especially the purely cosmetic ones) on the existing,
unrelated code would be fixed by the people with the opinions about how
the existing code should look like. I don't have any special ability to
test the dozen different chips this driver supports, so having me do it
by proxy seems rather suboptimal. I can only run it in roadtest, which
anyone can do with the following commands (against v5.19 due to the
regressions in mainline I mentioned in my other email), without special
hardware:
git checkout v5.19
git remote add vwax https://github.com/vwax/linux.git
git fetch vwax
git archive vwax/roadtest/mcp320x tools/testing/roadtest | tar xf -
make -C tools/testing/roadtest/ -j24 OPTS="-v -k 'mcp and not trigger'"
On Mon, Sep 5, 2022 at 11:27 AM Vincent Whitchurch
<[email protected]> wrote:
>
> On Sun, Sep 04, 2022 at 06:15:59PM +0200, Jonathan Cameron wrote:
> > 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.
>
> Well, even the moving of the code is unrelated to the original goal of
> adding triggered buffered support and isn't necessary for that. The
> moving of the code was only to eliminate the use of the "device_index",
> which was already used in the existing code.
>
> I'm of course happy to fix problems with the code I'm actually adding,
> but it seems to me that it would really be simpler for everyone if the
> trivial comments (especially the purely cosmetic ones) on the existing,
> unrelated code would be fixed by the people with the opinions about how
> the existing code should look like.
That's what Jonathan basically says, but with one remark that some of
the fixes are affecting the code one is going to add. In any case, you
may look at the "people with the opinions about how the existing code
should look like" at different angle, i.e. that those people may be
way overloaded while doing _a lot_ (and I mean it) of the stuff, so
from their perspective it's easier if somebody who is already working
on the driver can add a few trivial things which takes from her/him a
couple of hours to accomplish. In the collaboration we get the product
(Linux kernel) better!
--
With Best Regards,
Andy Shevchenko