Message ID | 20220831100506.3368103-2-vincent.whitchurch@axis.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | iio: adc: mcp320x: Add triggered buffer support | expand |
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. > },
On Wed, Aug 31, 2022 at 02:40:49PM +0200, Andy Shevchenko 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. 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 <vincent.whitchurch@axis.com> 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 > > <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. > > 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.
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. > > > }, >
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 <vincent.whitchurch@axis.com> 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!
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, },
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 <vincent.whitchurch@axis.com> --- drivers/iio/adc/mcp320x.c | 115 ++++++++++++++++++++++---------------- 1 file changed, 67 insertions(+), 48 deletions(-)