| Message ID | 20221111112657.1521307-3-mitja@lxnav.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: mcp3422 improvements | expand |
On Fri, 11 Nov 2022 12:26:54 +0100 Mitja Spes <mitja@lxnav.com> wrote: > General improvements: > - allow setting gain and sampling per channel > - setting scale can also set sampling rate (combined setting) > - use per channel config setting > - do not update mcp register on setting write (we might be reading it...) > instead it's updated on next value read > - output all scale values (sample rates x gain) > > Signed-off-by: Mitja Spes <mitja@lxnav.com> Hi Mitja, Was it possible for these scales to differ before this change? If not, then why was the previous patch a fix rather than simply a precursor to this change (where it now matters). There are a number of changes in here which are more stylistic cleanup than anything to do with the functional change. Please pull those out to a precursor patch where we can quickly check they make not functional changes rather than having them mixed in here. > --- > drivers/iio/adc/mcp3422.c | 119 ++++++++++++++++++++------------------ > 1 file changed, 64 insertions(+), 55 deletions(-) > > diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c > index 3d53de300c89..cfb629b964af 100644 > --- a/drivers/iio/adc/mcp3422.c > +++ b/drivers/iio/adc/mcp3422.c > @@ -28,14 +28,19 @@ > #define MCP3422_CHANNEL_MASK 0x60 > #define MCP3422_PGA_MASK 0x03 > #define MCP3422_SRATE_MASK 0x0C > -#define MCP3422_SRATE_240 0x0 > -#define MCP3422_SRATE_60 0x1 > -#define MCP3422_SRATE_15 0x2 > -#define MCP3422_SRATE_3 0x3 > -#define MCP3422_PGA_1 0 > -#define MCP3422_PGA_2 1 > -#define MCP3422_PGA_4 2 > -#define MCP3422_PGA_8 3 > + > +#define MCP3422_SRATE_240 0 > +#define MCP3422_SRATE_60 1 > +#define MCP3422_SRATE_15 2 > +#define MCP3422_SRATE_3 3 I have no particular problem with taking these from hex to decimal, though I'm not really seeing the necessity. However, it is really a style question and should not be in this patch where it mostly adds noise making it slightly harder to spot the functional changes. > +#define MCP3422_SRATE_COUNT 4 > + > +#define MCP3422_PGA_1 0 > +#define MCP3422_PGA_2 1 > +#define MCP3422_PGA_4 2 > +#define MCP3422_PGA_8 3 > +#define MCP3422_PGA_COUNT 4 > + > #define MCP3422_CONT_SAMPLING 0x10 > > #define MCP3422_CHANNEL(config) (((config) & MCP3422_CHANNEL_MASK) >> 5) > @@ -52,32 +57,32 @@ > .indexed = 1, \ > .channel = _index, \ > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \ > - | BIT(IIO_CHAN_INFO_SCALE), \ > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + | BIT(IIO_CHAN_INFO_SCALE) \ > + | BIT(IIO_CHAN_INFO_SAMP_FREQ), \ Hmm. This is an ABI change. Hopefully no one will notice however. > } > > -static const int mcp3422_scales[4][4] = { > +static const int mcp3422_scales[MCP3422_SRATE_COUNT][MCP3422_PGA_COUNT] = { > { 1000000, 500000, 250000, 125000 }, > { 250000, 125000, 62500, 31250 }, > { 62500, 31250, 15625, 7812 }, > { 15625, 7812, 3906, 1953 } }; > > /* Constant msleep times for data acquisitions */ > -static const int mcp3422_read_times[4] = { > +static const int mcp3422_read_times[MCP3422_SRATE_COUNT] = { Reasonable to make this change, but I think it belongs in a precursor patch. > [MCP3422_SRATE_240] = 1000 / 240, > [MCP3422_SRATE_60] = 1000 / 60, > [MCP3422_SRATE_15] = 1000 / 15, > [MCP3422_SRATE_3] = 1000 / 3 }; > > /* sample rates to integer conversion table */ > -static const int mcp3422_sample_rates[4] = { > +static const int mcp3422_sample_rates[MCP3422_SRATE_COUNT] = { > [MCP3422_SRATE_240] = 240, > [MCP3422_SRATE_60] = 60, > [MCP3422_SRATE_15] = 15, > [MCP3422_SRATE_3] = 3 }; > > /* sample rates to sign extension table */ > -static const int mcp3422_sign_extend[4] = { > +static const int mcp3422_sign_extend[MCP3422_SRATE_COUNT] = { > [MCP3422_SRATE_240] = 11, > [MCP3422_SRATE_60] = 13, > [MCP3422_SRATE_15] = 15, > @@ -87,8 +92,8 @@ static const int mcp3422_sign_extend[4] = { > struct mcp3422 { > struct i2c_client *i2c; > u8 id; > - u8 config; > - u8 pga[4]; > + u8 active_config; // config currently set on mcp > + u8 ch_config[4]; // per channel config > struct mutex lock; > }; > > @@ -98,7 +103,7 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig) > > ret = i2c_master_send(adc->i2c, &newconfig, 1); > if (ret > 0) { > - adc->config = newconfig; > + adc->active_config = newconfig; > ret = 0; > } > > @@ -108,7 +113,7 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig) > static int mcp3422_read(struct mcp3422 *adc, int *value, u8 *config) > { > int ret = 0; > - u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config); > + u8 sample_rate = MCP3422_SAMPLE_RATE(adc->active_config); > u8 buf[4] = {0, 0, 0, 0}; > u32 temp; > > @@ -136,18 +141,13 @@ static int mcp3422_read_channel(struct mcp3422 *adc, > > mutex_lock(&adc->lock); > > - if (req_channel != MCP3422_CHANNEL(adc->config)) { > - config = adc->config; > - config &= ~MCP3422_CHANNEL_MASK; > - config |= MCP3422_CHANNEL_VALUE(req_channel); > - config &= ~MCP3422_PGA_MASK; > - config |= MCP3422_PGA_VALUE(adc->pga[req_channel]); > - ret = mcp3422_update_config(adc, config); > + if (adc->ch_config[req_channel] != adc->active_config) { > + ret = mcp3422_update_config(adc, adc->ch_config[req_channel]); > if (ret < 0) { > mutex_unlock(&adc->lock); > return ret; > } > - msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->config)]); > + msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->active_config)]); > } > > ret = mcp3422_read(adc, value, &config); > @@ -164,9 +164,9 @@ static int mcp3422_read_raw(struct iio_dev *iio, > struct mcp3422 *adc = iio_priv(iio); > int err; > > - u8 req_channel = channel->channel; > - u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config); > - u8 pga = adc->pga[req_channel]; > + u8 config = adc->ch_config[channel->channel]; > + u8 sample_rate = MCP3422_SAMPLE_RATE(config); > + u8 pga = MCP3422_PGA(config); > > switch (mask) { > case IIO_CHAN_INFO_RAW: > @@ -181,7 +181,7 @@ static int mcp3422_read_raw(struct iio_dev *iio, > return IIO_VAL_INT_PLUS_NANO; > > case IIO_CHAN_INFO_SAMP_FREQ: > - *val1 = mcp3422_sample_rates[MCP3422_SAMPLE_RATE(adc->config)]; > + *val1 = mcp3422_sample_rates[sample_rate]; > return IIO_VAL_INT; > > default: > @@ -197,26 +197,25 @@ static int mcp3422_write_raw(struct iio_dev *iio, > { > struct mcp3422 *adc = iio_priv(iio); > u8 temp; > - u8 config = adc->config; > u8 req_channel = channel->channel; > - u8 sample_rate = MCP3422_SAMPLE_RATE(config); > - u8 i; > + u8 config = adc->ch_config[req_channel]; > + u8 i, j; > > switch (mask) { > case IIO_CHAN_INFO_SCALE: > if (val1 != 0) > return -EINVAL; > > - for (i = 0; i < ARRAY_SIZE(mcp3422_scales[0]); i++) { > - if (val2 == mcp3422_scales[sample_rate][i]) { > - adc->pga[req_channel] = i; > - > - config &= ~MCP3422_CHANNEL_MASK; > - config |= MCP3422_CHANNEL_VALUE(req_channel); > - config &= ~MCP3422_PGA_MASK; > - config |= MCP3422_PGA_VALUE(adc->pga[req_channel]); > - > - return mcp3422_update_config(adc, config); > + for (j = 0; j < MCP3422_SRATE_COUNT; j++) { > + for (i = 0; i < MCP3422_PGA_COUNT; i++) { > + if (val2 == mcp3422_scales[j][i]) { > + config &= ~MCP3422_PGA_MASK; > + config |= MCP3422_PGA_VALUE(i); > + config &= ~MCP3422_SRATE_MASK; > + config |= MCP3422_SAMPLE_RATE_VALUE(j); > + adc->ch_config[req_channel] = config; > + return 0; > + } > } > } > return -EINVAL; > @@ -241,12 +240,10 @@ static int mcp3422_write_raw(struct iio_dev *iio, > return -EINVAL; > } > > - config &= ~MCP3422_CHANNEL_MASK; > - config |= MCP3422_CHANNEL_VALUE(req_channel); > config &= ~MCP3422_SRATE_MASK; > config |= MCP3422_SAMPLE_RATE_VALUE(temp); > - > - return mcp3422_update_config(adc, config); > + adc->ch_config[req_channel] = config; > + return 0; > > default: > break; > @@ -282,14 +279,18 @@ static ssize_t mcp3422_show_samp_freqs(struct device *dev, > static ssize_t mcp3422_show_scales(struct device *dev, > struct device_attribute *attr, char *buf) > { > - struct mcp3422 *adc = iio_priv(dev_to_iio_dev(dev)); > - u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config); > - > - return sprintf(buf, "0.%09u 0.%09u 0.%09u 0.%09u\n", > - mcp3422_scales[sample_rate][0], > - mcp3422_scales[sample_rate][1], > - mcp3422_scales[sample_rate][2], > - mcp3422_scales[sample_rate][3]); > + ssize_t count = 0; > + int i; > + > + for (i = 0; i < MCP3422_SRATE_COUNT; i++) { > + count += sprintf(buf + count, "0.%09u 0.%09u 0.%09u 0.%09u%s", > + mcp3422_scales[i][0], > + mcp3422_scales[i][1], > + mcp3422_scales[i][2], > + mcp3422_scales[i][3], > + (i < MCP3422_SRATE_COUNT - 1 ? " " : "\n")); What does the output of this now look like? For available attributes we tend to only show the values available assuming just the one thing is changing. Hence hold sampling frequency static, then show what scales are available It can get complex if there are nasty interactions so we might have a situation where one attribute allows to all the possible values. So maybe we have all scales available and on a write try to find the nearest frequency to the current at which we can deliver the required scale. Right now this looks like it prints repeated values... My gut feeling for this device is make the sampling frequency the dominant attribute. So just list the available sampling frequencies not taking scale into account. For current sampling frequency just list the available scales and only accept those to be written to the scale attr. > + } > + return count; > } > > static IIO_DEVICE_ATTR(sampling_frequency_available, S_IRUGO, > @@ -336,6 +337,7 @@ static int mcp3422_probe(struct i2c_client *client, > struct iio_dev *indio_dev; > struct mcp3422 *adc; > int err; > + int i; > u8 config; > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) > @@ -376,6 +378,13 @@ static int mcp3422_probe(struct i2c_client *client, > } > > /* meaningful default configuration */ > + for (i = 0; i < 4; i++) { > + adc->ch_config[i] = (MCP3422_CONT_SAMPLING > + | MCP3422_CHANNEL_VALUE(i) > + | MCP3422_PGA_VALUE(MCP3422_PGA_1) > + | MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240)); > + } > + > config = (MCP3422_CONT_SAMPLING > | MCP3422_CHANNEL_VALUE(0) > | MCP3422_PGA_VALUE(MCP3422_PGA_1) Perhaps use the first channel configuration for this?
Hi Jonathan, On Sat, Nov 12, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote: > Was it possible for these scales to differ before this change? Yes. The difference is that before this change you could only see and set available scales that were available for specified sampling rate. Now you're able to set gain and sampling rate via scale. So before the change you got these (@240sps): 0.001000000 0.000500000 0.000250000 0.000125000 Now you get the complete set: /* gain x1 gain x2 gain x4 gain x8 */ /* 240 sps */ 0.001000000 0.000500000 0.000250000 0.000125000 /* 60 sps */ 0.000250000 0.000125000 0.000062500 0.000031250 /* 15 sps */ 0.000062500 0.000031250 0.000015625 0.000007812 /* 3.75 sps */ 0.000015625 0.000007812 0.000003906 0.000001953 > If not, then why was the previous patch a fix rather than simply a precursor > to this change (where it now matters). I wanted to separate a bug fix from improvements, if these were rejected for for some reason. > There are a number of changes in here which are more stylistic cleanup > than anything to do with the functional change. Please pull those out > to a precursor patch where we can quickly check they make not functional changes > rather than having them mixed in here. Will do. > I have no particular problem with taking these from hex > to decimal, though I'm not really seeing the necessity. > > However, it is really a style question and should not be in this > patch where it mostly adds noise making it slightly harder > to spot the functional changes. My styling preference. I think indexes should be decimal so they are not confused with flags. > > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \ > > - | BIT(IIO_CHAN_INFO_SCALE), \ > > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > + | BIT(IIO_CHAN_INFO_SCALE) \ > > + | BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > Hmm. This is an ABI change. Hopefully no one will notice however. Indeed. I've noted this in the cover letter. > > -static const int mcp3422_read_times[4] = { > > +static const int mcp3422_read_times[MCP3422_SRATE_COUNT] = { > Reasonable to make this change, but I think it belongs in a precursor patch. Will fix. > > + for (i = 0; i < MCP3422_SRATE_COUNT; i++) { > > + count += sprintf(buf + count, "0.%09u 0.%09u 0.%09u 0.%09u%s", > > + mcp3422_scales[i][0], > > + mcp3422_scales[i][1], > > + mcp3422_scales[i][2], > > + mcp3422_scales[i][3], > > + (i < MCP3422_SRATE_COUNT - 1 ? " " : "\n")); > > What does the output of this now look like? 0.001000000 0.000500000 0.000250000 0.000125000 0.000250000 0.000125000 0.000062500 0.000031250 0.000062500 0.000031250 0.000015625 0.000007812 0.000015625 0.000007812 0.000003906 0.000001953 All in one line. > For available attributes we tend to only show the values available assuming > just the one thing is changing. Hence hold sampling frequency static, then > show what scales are available > It can get complex if there are nasty interactions so we might have a > situation where one attribute allows to all the possible values. > So maybe we have all scales available and on a write try to find > the nearest frequency to the current at which we can deliver the > required scale. That's what's being done here: + for (j = 0; j < MCP3422_SRATE_COUNT; j++) { + for (i = 0; i < MCP3422_PGA_COUNT; i++) { + if (val2 == mcp3422_scales[j][i]) { + config &= ~MCP3422_PGA_MASK; + config |= MCP3422_PGA_VALUE(i); + config &= ~MCP3422_SRATE_MASK; + config |= MCP3422_SAMPLE_RATE_VALUE(j); + adc->ch_config[req_channel] = config; + return 0; + } } } Before it looked at only one sampling frequency and all gains, now it looks at all combinations. Looking at this I agree that it would be better to find nearest instead of looking for an exact match. > My gut feeling for this device is make the sampling frequency the dominant > attribute. So just list the available sampling frequencies not taking > scale into account. For current sampling frequency just list the available > scales and only accept those to be written to the scale attr. That way the order in which you set attributes matters. Is that really better? This device has a settable sampling rate and gain and they are independent. But we could only set gain via scale which values were sampling rate dependent. > > /* meaningful default configuration */ > > + for (i = 0; i < 4; i++) { > > + adc->ch_config[i] = (MCP3422_CONT_SAMPLING > > + | MCP3422_CHANNEL_VALUE(i) > > + | MCP3422_PGA_VALUE(MCP3422_PGA_1) > > + | MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240)); > > + } > > + > > config = (MCP3422_CONT_SAMPLING > > | MCP3422_CHANNEL_VALUE(0) > > | MCP3422_PGA_VALUE(MCP3422_PGA_1) > > Perhaps use the first channel configuration for this? Will fix. Kind regards, Mitja
On Sat, 12 Nov 2022 21:51:36 +0100 Mitja Špes <mitja@lxnav.com> wrote: > Hi Jonathan, Hi Mitja, For future reference please keep the comments inline with the code and reply there. On this occasion I'm coming back this one day alter so it's not too bad - but if it's a week later as often happens it can be hard to follow if the code isn't there. > > On Sat, Nov 12, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote: > > Was it possible for these scales to differ before this change? > Yes. The difference is that before this change you could only see and set > available scales that were available for specified sampling rate. Now you're > able to set gain and sampling rate via scale. So before the change you got > these (@240sps): > > 0.001000000 0.000500000 0.000250000 0.000125000 > > Now you get the complete set: > /* gain x1 gain x2 gain x4 gain x8 */ > /* 240 sps */ 0.001000000 0.000500000 0.000250000 0.000125000 > /* 60 sps */ 0.000250000 0.000125000 0.000062500 0.000031250 > /* 15 sps */ 0.000062500 0.000031250 0.000015625 0.000007812 > /* 3.75 sps */ 0.000015625 0.000007812 0.000003906 0.000001953 Ok. That doesn't work as a standard interface because userspace code wants to pick say 0.00062500 which appears twice. I'm not understanding why you should see scales for other sampling rates. We always allow any IIO attribute to have side effects on others, precisely to allow for cases where they interact like here. It's not ideal but there isn't really a clean solution (as we are seeing here). > > > If not, then why was the previous patch a fix rather than simply a precursor > > to this change (where it now matters). > I wanted to separate a bug fix from improvements, if these were rejected for > for some reason. Is it a bug fix? The way I read it is that, before this patch there is only one scale that is applied to all channels. As such, the current value == the value set and the code works as expected. So the previous patch is only necessary once this one is applied. Hence no bug, just a rework that is useful to enabling this feature. > > > There are a number of changes in here which are more stylistic cleanup > > than anything to do with the functional change. Please pull those out > > to a precursor patch where we can quickly check they make not functional changes > > rather than having them mixed in here. > Will do. > > > I have no particular problem with taking these from hex > > to decimal, though I'm not really seeing the necessity. > > > > However, it is really a style question and should not be in this > > patch where it mostly adds noise making it slightly harder > > to spot the functional changes. > My styling preference. I think indexes should be decimal so they are not > confused with flags. They are also field values as used in the existing code - rather than simply indexes. Still field values can be either hex or decimal so given the combined use I'm fine with the change. However, precursor patch so we don't have noise in this patch where closer review is needed. > > > > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \ > > > - | BIT(IIO_CHAN_INFO_SCALE), \ > > > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > > + | BIT(IIO_CHAN_INFO_SCALE) \ > > > + | BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > > > Hmm. This is an ABI change. Hopefully no one will notice however. > Indeed. I've noted this in the cover letter. No one reads cover letters :) Also not in the git log so you want to call attention to it in the description of this patch as well. > > > > + for (i = 0; i < MCP3422_SRATE_COUNT; i++) { > > > + count += sprintf(buf + count, "0.%09u 0.%09u 0.%09u 0.%09u%s", > > > + mcp3422_scales[i][0], > > > + mcp3422_scales[i][1], > > > + mcp3422_scales[i][2], > > > + mcp3422_scales[i][3], > > > + (i < MCP3422_SRATE_COUNT - 1 ? " " : "\n")); > > > > What does the output of this now look like? > 0.001000000 0.000500000 0.000250000 0.000125000 > 0.000250000 0.000125000 0.000062500 0.000031250 > 0.000062500 0.000031250 0.000015625 0.000007812 > 0.000015625 0.000007812 0.000003906 0.000001953 > All in one line. Repetition is liable to confuse userspace so if we went this way you would definitely need to stop doing that. > > > For available attributes we tend to only show the values available assuming > > just the one thing is changing. Hence hold sampling frequency static, then > > show what scales are available > > It can get complex if there are nasty interactions so we might have a > > situation where one attribute allows to all the possible values. > > So maybe we have all scales available and on a write try to find > > the nearest frequency to the current at which we can deliver the > > required scale. > That's what's being done here: I don't follow. This seems to just take the scale and match the first instance of that it sees. That may not match the option that was available at the current sampling frequency because of the repetition of possible scales. > + for (j = 0; j < MCP3422_SRATE_COUNT; j++) { > + for (i = 0; i < MCP3422_PGA_COUNT; i++) { > + if (val2 == mcp3422_scales[j][i]) { > + config &= ~MCP3422_PGA_MASK; > + config |= MCP3422_PGA_VALUE(i); > + config &= ~MCP3422_SRATE_MASK; > + config |= MCP3422_SAMPLE_RATE_VALUE(j); > + adc->ch_config[req_channel] = config; > + return 0; > + } > } > } > > Before it looked at only one sampling frequency and all gains, now it looks at > all combinations. > Looking at this I agree that it would be better to find nearest instead of > looking for an exact match. > > > My gut feeling for this device is make the sampling frequency the dominant > > attribute. So just list the available sampling frequencies not taking > > scale into account. For current sampling frequency just list the available > > scales and only accept those to be written to the scale attr. > That way the order in which you set attributes matters. Is that really better? > This device has a settable sampling rate and gain and they are independent. > But we could only set gain via scale which values were sampling rate dependent. This is a very common situation. If you expose all the scales, user who cares about sampling frequency picks a scale that changes that as well as the pga value. That is not what they expect to happen. So there is no good answer in cases like this. Nature of ABI design is that we will always hit corner cases. The get of jail free card here was the fact we let changing one attribute affect others. Trade off needs to be made and that decision was made in original version of this driver. I'm not seeing a strong enough reason to change it. An improvement that does seem logical to me would be to have a change in either sampling frequency or scale attempt to maintain the nearest value of the other. In some cases it can be exactly the same. Any userspace code that already taking advantage of how the two attributes affect each other will set sampling frequency first then set the scale. This optimisation would not affect that so even though the ABI would change a little it would not be in a way likely to break userspace code. Jonathan > > > > /* meaningful default configuration */ > > > + for (i = 0; i < 4; i++) { > > > + adc->ch_config[i] = (MCP3422_CONT_SAMPLING > > > + | MCP3422_CHANNEL_VALUE(i) > > > + | MCP3422_PGA_VALUE(MCP3422_PGA_1) > > > + | MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240)); > > > + } > > > + > > > config = (MCP3422_CONT_SAMPLING > > > | MCP3422_CHANNEL_VALUE(0) > > > | MCP3422_PGA_VALUE(MCP3422_PGA_1) > > > > Perhaps use the first channel configuration for this? > Will fix. > > Kind regards, > Mitja
On Sun, Nov 13, 2022 at 12:53 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sat, Nov 12, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > Was it possible for these scales to differ before this change? > > Yes. The difference is that before this change you could only see and set > > available scales that were available for specified sampling rate. Now you're > > able to set gain and sampling rate via scale. So before the change you got > > these (@240sps): > > > > 0.001000000 0.000500000 0.000250000 0.000125000 > > > > Now you get the complete set: > > /* gain x1 gain x2 gain x4 gain x8 */ > > /* 240 sps */ 0.001000000 0.000500000 0.000250000 0.000125000 > > /* 60 sps */ 0.000250000 0.000125000 0.000062500 0.000031250 > > /* 15 sps */ 0.000062500 0.000031250 0.000015625 0.000007812 > > /* 3.75 sps */ 0.000015625 0.000007812 0.000003906 0.000001953 > > Ok. That doesn't work as a standard interface because userspace code wants to pick say > 0.00062500 which appears twice. I don't know how I missed that. It's clear to me now that this patch is wrong. > > > If not, then why was the previous patch a fix rather than simply a precursor > > > to this change (where it now matters). > > I wanted to separate a bug fix from improvements, if these were rejected for > > for some reason. > > Is it a bug fix? The way I read it is that, before this patch there is only > one scale that is applied to all channels. As such, the current value == the > value set and the code works as expected. > So the previous patch is only necessary once this one is applied. Hence no > bug, just a rework that is useful to enabling this feature. I'll post the previous snippet here and write the comments inline: ---- @@ -164,8 +164,9 @@ static int mcp3422_read_raw(struct iio_dev *iio, struct mcp3422 *adc = iio_priv(iio); int err; + u8 req_channel = channel->channel; u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config); - u8 pga = MCP3422_PGA(adc->config); /* <- this uses the "current" config which changes depending on the last read channel */ + u8 pga = adc->pga[req_channel]; /* this now returns the PGA for the selected channel */ switch (mask) { case IIO_CHAN_INFO_RAW: ---- I hope this clarifies the bugfix. Thanks for in depth look at this and sorry for wasting your time with this flawed patch. Kind regards, Mitja
On Sun, 13 Nov 2022 14:39:03 +0100 Mitja Špes <mitja@lxnav.com> wrote: > On Sun, Nov 13, 2022 at 12:53 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > On Sat, Nov 12, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > Was it possible for these scales to differ before this change? > > > Yes. The difference is that before this change you could only see and set > > > available scales that were available for specified sampling rate. Now you're > > > able to set gain and sampling rate via scale. So before the change you got > > > these (@240sps): > > > > > > 0.001000000 0.000500000 0.000250000 0.000125000 > > > > > > Now you get the complete set: > > > /* gain x1 gain x2 gain x4 gain x8 */ > > > /* 240 sps */ 0.001000000 0.000500000 0.000250000 0.000125000 > > > /* 60 sps */ 0.000250000 0.000125000 0.000062500 0.000031250 > > > /* 15 sps */ 0.000062500 0.000031250 0.000015625 0.000007812 > > > /* 3.75 sps */ 0.000015625 0.000007812 0.000003906 0.000001953 > > > > Ok. That doesn't work as a standard interface because userspace code wants to pick say > > 0.00062500 which appears twice. > I don't know how I missed that. It's clear to me now that this patch is wrong. > > > > > > If not, then why was the previous patch a fix rather than simply a precursor > > > > to this change (where it now matters). > > > I wanted to separate a bug fix from improvements, if these were rejected for > > > for some reason. > > > > Is it a bug fix? The way I read it is that, before this patch there is only > > one scale that is applied to all channels. As such, the current value == the > > value set and the code works as expected. > > So the previous patch is only necessary once this one is applied. Hence no > > bug, just a rework that is useful to enabling this feature. > I'll post the previous snippet here and write the comments inline: > ---- > @@ -164,8 +164,9 @@ static int mcp3422_read_raw(struct iio_dev *iio, > struct mcp3422 *adc = iio_priv(iio); > int err; > > + u8 req_channel = channel->channel; > u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config); > - u8 pga = MCP3422_PGA(adc->config); /* <- this uses the "current" config > which changes depending on the last read channel */ > + u8 pga = adc->pga[req_channel]; /* this now returns the PGA for the > selected channel */ > > switch (mask) { > case IIO_CHAN_INFO_RAW: > ---- > I hope this clarifies the bugfix. Ah I see. The bit that threw me off was the title of this patch. "allow setting gain ... per channel" which made me think that before this patch there was one gain for all channels. I was too lazy to actually check and discover that it has always been per channel on the write side of things. Jonathan > > > Thanks for in depth look at this and sorry for wasting your time with this > flawed patch. > > Kind regards, > Mitja
diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c index 3d53de300c89..cfb629b964af 100644 --- a/drivers/iio/adc/mcp3422.c +++ b/drivers/iio/adc/mcp3422.c @@ -28,14 +28,19 @@ #define MCP3422_CHANNEL_MASK 0x60 #define MCP3422_PGA_MASK 0x03 #define MCP3422_SRATE_MASK 0x0C -#define MCP3422_SRATE_240 0x0 -#define MCP3422_SRATE_60 0x1 -#define MCP3422_SRATE_15 0x2 -#define MCP3422_SRATE_3 0x3 -#define MCP3422_PGA_1 0 -#define MCP3422_PGA_2 1 -#define MCP3422_PGA_4 2 -#define MCP3422_PGA_8 3 + +#define MCP3422_SRATE_240 0 +#define MCP3422_SRATE_60 1 +#define MCP3422_SRATE_15 2 +#define MCP3422_SRATE_3 3 +#define MCP3422_SRATE_COUNT 4 + +#define MCP3422_PGA_1 0 +#define MCP3422_PGA_2 1 +#define MCP3422_PGA_4 2 +#define MCP3422_PGA_8 3 +#define MCP3422_PGA_COUNT 4 + #define MCP3422_CONT_SAMPLING 0x10 #define MCP3422_CHANNEL(config) (((config) & MCP3422_CHANNEL_MASK) >> 5) @@ -52,32 +57,32 @@ .indexed = 1, \ .channel = _index, \ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \ - | BIT(IIO_CHAN_INFO_SCALE), \ - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + | BIT(IIO_CHAN_INFO_SCALE) \ + | BIT(IIO_CHAN_INFO_SAMP_FREQ), \ } -static const int mcp3422_scales[4][4] = { +static const int mcp3422_scales[MCP3422_SRATE_COUNT][MCP3422_PGA_COUNT] = { { 1000000, 500000, 250000, 125000 }, { 250000, 125000, 62500, 31250 }, { 62500, 31250, 15625, 7812 }, { 15625, 7812, 3906, 1953 } }; /* Constant msleep times for data acquisitions */ -static const int mcp3422_read_times[4] = { +static const int mcp3422_read_times[MCP3422_SRATE_COUNT] = { [MCP3422_SRATE_240] = 1000 / 240, [MCP3422_SRATE_60] = 1000 / 60, [MCP3422_SRATE_15] = 1000 / 15, [MCP3422_SRATE_3] = 1000 / 3 }; /* sample rates to integer conversion table */ -static const int mcp3422_sample_rates[4] = { +static const int mcp3422_sample_rates[MCP3422_SRATE_COUNT] = { [MCP3422_SRATE_240] = 240, [MCP3422_SRATE_60] = 60, [MCP3422_SRATE_15] = 15, [MCP3422_SRATE_3] = 3 }; /* sample rates to sign extension table */ -static const int mcp3422_sign_extend[4] = { +static const int mcp3422_sign_extend[MCP3422_SRATE_COUNT] = { [MCP3422_SRATE_240] = 11, [MCP3422_SRATE_60] = 13, [MCP3422_SRATE_15] = 15, @@ -87,8 +92,8 @@ static const int mcp3422_sign_extend[4] = { struct mcp3422 { struct i2c_client *i2c; u8 id; - u8 config; - u8 pga[4]; + u8 active_config; // config currently set on mcp + u8 ch_config[4]; // per channel config struct mutex lock; }; @@ -98,7 +103,7 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig) ret = i2c_master_send(adc->i2c, &newconfig, 1); if (ret > 0) { - adc->config = newconfig; + adc->active_config = newconfig; ret = 0; } @@ -108,7 +113,7 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig) static int mcp3422_read(struct mcp3422 *adc, int *value, u8 *config) { int ret = 0; - u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config); + u8 sample_rate = MCP3422_SAMPLE_RATE(adc->active_config); u8 buf[4] = {0, 0, 0, 0}; u32 temp; @@ -136,18 +141,13 @@ static int mcp3422_read_channel(struct mcp3422 *adc, mutex_lock(&adc->lock); - if (req_channel != MCP3422_CHANNEL(adc->config)) { - config = adc->config; - config &= ~MCP3422_CHANNEL_MASK; - config |= MCP3422_CHANNEL_VALUE(req_channel); - config &= ~MCP3422_PGA_MASK; - config |= MCP3422_PGA_VALUE(adc->pga[req_channel]); - ret = mcp3422_update_config(adc, config); + if (adc->ch_config[req_channel] != adc->active_config) { + ret = mcp3422_update_config(adc, adc->ch_config[req_channel]); if (ret < 0) { mutex_unlock(&adc->lock); return ret; } - msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->config)]); + msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->active_config)]); } ret = mcp3422_read(adc, value, &config); @@ -164,9 +164,9 @@ static int mcp3422_read_raw(struct iio_dev *iio, struct mcp3422 *adc = iio_priv(iio); int err; - u8 req_channel = channel->channel; - u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config); - u8 pga = adc->pga[req_channel]; + u8 config = adc->ch_config[channel->channel]; + u8 sample_rate = MCP3422_SAMPLE_RATE(config); + u8 pga = MCP3422_PGA(config); switch (mask) { case IIO_CHAN_INFO_RAW: @@ -181,7 +181,7 @@ static int mcp3422_read_raw(struct iio_dev *iio, return IIO_VAL_INT_PLUS_NANO; case IIO_CHAN_INFO_SAMP_FREQ: - *val1 = mcp3422_sample_rates[MCP3422_SAMPLE_RATE(adc->config)]; + *val1 = mcp3422_sample_rates[sample_rate]; return IIO_VAL_INT; default: @@ -197,26 +197,25 @@ static int mcp3422_write_raw(struct iio_dev *iio, { struct mcp3422 *adc = iio_priv(iio); u8 temp; - u8 config = adc->config; u8 req_channel = channel->channel; - u8 sample_rate = MCP3422_SAMPLE_RATE(config); - u8 i; + u8 config = adc->ch_config[req_channel]; + u8 i, j; switch (mask) { case IIO_CHAN_INFO_SCALE: if (val1 != 0) return -EINVAL; - for (i = 0; i < ARRAY_SIZE(mcp3422_scales[0]); i++) { - if (val2 == mcp3422_scales[sample_rate][i]) { - adc->pga[req_channel] = i; - - config &= ~MCP3422_CHANNEL_MASK; - config |= MCP3422_CHANNEL_VALUE(req_channel); - config &= ~MCP3422_PGA_MASK; - config |= MCP3422_PGA_VALUE(adc->pga[req_channel]); - - return mcp3422_update_config(adc, config); + for (j = 0; j < MCP3422_SRATE_COUNT; j++) { + for (i = 0; i < MCP3422_PGA_COUNT; i++) { + if (val2 == mcp3422_scales[j][i]) { + config &= ~MCP3422_PGA_MASK; + config |= MCP3422_PGA_VALUE(i); + config &= ~MCP3422_SRATE_MASK; + config |= MCP3422_SAMPLE_RATE_VALUE(j); + adc->ch_config[req_channel] = config; + return 0; + } } } return -EINVAL; @@ -241,12 +240,10 @@ static int mcp3422_write_raw(struct iio_dev *iio, return -EINVAL; } - config &= ~MCP3422_CHANNEL_MASK; - config |= MCP3422_CHANNEL_VALUE(req_channel); config &= ~MCP3422_SRATE_MASK; config |= MCP3422_SAMPLE_RATE_VALUE(temp); - - return mcp3422_update_config(adc, config); + adc->ch_config[req_channel] = config; + return 0; default: break; @@ -282,14 +279,18 @@ static ssize_t mcp3422_show_samp_freqs(struct device *dev, static ssize_t mcp3422_show_scales(struct device *dev, struct device_attribute *attr, char *buf) { - struct mcp3422 *adc = iio_priv(dev_to_iio_dev(dev)); - u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config); - - return sprintf(buf, "0.%09u 0.%09u 0.%09u 0.%09u\n", - mcp3422_scales[sample_rate][0], - mcp3422_scales[sample_rate][1], - mcp3422_scales[sample_rate][2], - mcp3422_scales[sample_rate][3]); + ssize_t count = 0; + int i; + + for (i = 0; i < MCP3422_SRATE_COUNT; i++) { + count += sprintf(buf + count, "0.%09u 0.%09u 0.%09u 0.%09u%s", + mcp3422_scales[i][0], + mcp3422_scales[i][1], + mcp3422_scales[i][2], + mcp3422_scales[i][3], + (i < MCP3422_SRATE_COUNT - 1 ? " " : "\n")); + } + return count; } static IIO_DEVICE_ATTR(sampling_frequency_available, S_IRUGO, @@ -336,6 +337,7 @@ static int mcp3422_probe(struct i2c_client *client, struct iio_dev *indio_dev; struct mcp3422 *adc; int err; + int i; u8 config; if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) @@ -376,6 +378,13 @@ static int mcp3422_probe(struct i2c_client *client, } /* meaningful default configuration */ + for (i = 0; i < 4; i++) { + adc->ch_config[i] = (MCP3422_CONT_SAMPLING + | MCP3422_CHANNEL_VALUE(i) + | MCP3422_PGA_VALUE(MCP3422_PGA_1) + | MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240)); + } + config = (MCP3422_CONT_SAMPLING | MCP3422_CHANNEL_VALUE(0) | MCP3422_PGA_VALUE(MCP3422_PGA_1)
General improvements: - allow setting gain and sampling per channel - setting scale can also set sampling rate (combined setting) - use per channel config setting - do not update mcp register on setting write (we might be reading it...) instead it's updated on next value read - output all scale values (sample rates x gain) Signed-off-by: Mitja Spes <mitja@lxnav.com> --- drivers/iio/adc/mcp3422.c | 119 ++++++++++++++++++++------------------ 1 file changed, 64 insertions(+), 55 deletions(-)