| Message ID | 43f7cfe7bf868fb00e81a76246b9bc3a@heine.so (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/07/2018 05:17 PM, Michael Nosthoff wrote: > This commit is a follow-up to changes made to ad_sigma_delta.h > in staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ > which broke ad7793 as it was not altered to match those changes. > > This driver predates the availability of IIO_CHAN_INFO_SAMP_FREQ > attribute wherein usage has some advantages like it can be accessed by > in-kernel consumers as well as reduces the code size. > > Therefore, use IIO_CHAN_INFO_SAMP_FREQ to implement the > sampling_frequency attribute instead of using IIO_DEV_ATTR_SAMP_FREQ() > macro. > > Move code from the functions associated with IIO_DEV_ATTR_SAMP_FREQ() > into respective read and write hooks with the mask set to > IIO_CHAN_INFO_SAMP_FREQ. > > Signed-off-by: Michael Nosthoff <committed@heine.so> Hi, Thanks for fixing this. Patch looks good. We should add the fixes tag to the commit messages, so it gets picked up for the right stable branches. Fixes: a13e831fcaa7 ("staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ") Other than that just one minor comment inline. [...] > + case IIO_CHAN_INFO_SAMP_FREQ: > + if (!val) { > + ret = -EINVAL; > + break; > + } > + > + for (i = 0; i < 16; i++) > + if (val == st->chip_info->sample_freq_avail[i]) > + break; > + > + if (i == 16) { > + ret = -EINVAL; > + break; > + } > + > + st->mode &= ~AD7793_MODE_RATE(-1); > + st->mode |= AD7793_MODE_RATE(i); > + ad_sd_write_reg(&st->sd, AD7793_REG_MODE, sizeof(st->mode), st->mode); > + ret = 0; I don't think the ret = 0 is needed here, it should already be 0. But of course it does not hurt either. > + break; > default: > ret = -EINVAL; > } > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-03-07 17:43, Lars-Peter Clausen wrote: > On 03/07/2018 05:17 PM, Michael Nosthoff wrote: >> This commit is a follow-up to changes made to ad_sigma_delta.h >> in staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ >> which broke ad7793 as it was not altered to match those changes. >> >> This driver predates the availability of IIO_CHAN_INFO_SAMP_FREQ >> attribute wherein usage has some advantages like it can be accessed by >> in-kernel consumers as well as reduces the code size. >> >> Therefore, use IIO_CHAN_INFO_SAMP_FREQ to implement the >> sampling_frequency attribute instead of using IIO_DEV_ATTR_SAMP_FREQ() >> macro. >> >> Move code from the functions associated with IIO_DEV_ATTR_SAMP_FREQ() >> into respective read and write hooks with the mask set to >> IIO_CHAN_INFO_SAMP_FREQ. >> >> Signed-off-by: Michael Nosthoff <committed@heine.so> > > Hi, > > Thanks for fixing this. Patch looks good. We should add the fixes tag > to the > commit messages, so it gets picked up for the right stable branches. > > Fixes: a13e831fcaa7 ("staging: iio: ad7192: implement > IIO_CHAN_INFO_SAMP_FREQ") > > Other than that just one minor comment inline. Agree on the fixes tag. Further comment inline. > > [...] >> + case IIO_CHAN_INFO_SAMP_FREQ: >> + if (!val) { >> + ret = -EINVAL; >> + break; >> + } >> + >> + for (i = 0; i < 16; i++) >> + if (val == st->chip_info->sample_freq_avail[i]) >> + break; >> + >> + if (i == 16) { >> + ret = -EINVAL; >> + break; >> + } >> + >> + st->mode &= ~AD7793_MODE_RATE(-1); >> + st->mode |= AD7793_MODE_RATE(i); >> + ad_sd_write_reg(&st->sd, AD7793_REG_MODE, sizeof(st->mode), >> st->mode); >> + ret = 0; > > I don't think the ret = 0 is needed here, it should already be 0. But > of > course it does not hurt either. My fault. I tested against 4.14 which doesn't have the "use iio helper function to guarantee direct mode" commit which initializes ret. Which produces a compiler warning. So if I remove it a backport would require that commit to be pulled first. > >> + break; >> default: >> ret = -EINVAL; >> } >> -- >> 2.7.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-iio" >> in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-03-07 19:02, Michael Nosthoff wrote: > On 2018-03-07 17:43, Lars-Peter Clausen wrote: >> On 03/07/2018 05:17 PM, Michael Nosthoff wrote: >> [...] >>> + case IIO_CHAN_INFO_SAMP_FREQ: >>> + if (!val) { >>> + ret = -EINVAL; >>> + break; >>> + } >>> + >>> + for (i = 0; i < 16; i++) >>> + if (val == st->chip_info->sample_freq_avail[i]) >>> + break; >>> + >>> + if (i == 16) { >>> + ret = -EINVAL; >>> + break; >>> + } >>> + >>> + st->mode &= ~AD7793_MODE_RATE(-1); >>> + st->mode |= AD7793_MODE_RATE(i); >>> + ad_sd_write_reg(&st->sd, AD7793_REG_MODE, sizeof(st->mode), >>> st->mode); >>> + ret = 0; >> >> I don't think the ret = 0 is needed here, it should already be 0. But >> of >> course it does not hurt either. > > My fault. I tested against 4.14 which doesn't have the > "use iio helper function to guarantee direct mode" commit which > initializes > ret. Which produces a compiler warning. > So if I remove it a backport would require that commit to be pulled > first. I just noticed I mixed something up, the mentioned patch is already applied in 4.14. I'll remove the line and resubmit the patch. -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c index 47c3d7f..4079f40 100644 --- a/drivers/iio/adc/ad7793.c +++ b/drivers/iio/adc/ad7793.c @@ -348,55 +348,6 @@ static const u16 ad7793_sample_freq_avail[16] = {0, 470, 242, 123, 62, 50, 39, static const u16 ad7797_sample_freq_avail[16] = {0, 0, 0, 123, 62, 50, 0, 33, 0, 17, 16, 12, 10, 8, 6, 4}; -static ssize_t ad7793_read_frequency(struct device *dev, - struct device_attribute *attr, - char *buf) -{ - struct iio_dev *indio_dev = dev_to_iio_dev(dev); - struct ad7793_state *st = iio_priv(indio_dev); - - return sprintf(buf, "%d\n", - st->chip_info->sample_freq_avail[AD7793_MODE_RATE(st->mode)]); -} - -static ssize_t ad7793_write_frequency(struct device *dev, - struct device_attribute *attr, - const char *buf, - size_t len) -{ - struct iio_dev *indio_dev = dev_to_iio_dev(dev); - struct ad7793_state *st = iio_priv(indio_dev); - long lval; - int i, ret; - - ret = kstrtol(buf, 10, &lval); - if (ret) - return ret; - - if (lval == 0) - return -EINVAL; - - for (i = 0; i < 16; i++) - if (lval == st->chip_info->sample_freq_avail[i]) - break; - if (i == 16) - return -EINVAL; - - ret = iio_device_claim_direct_mode(indio_dev); - if (ret) - return ret; - st->mode &= ~AD7793_MODE_RATE(-1); - st->mode |= AD7793_MODE_RATE(i); - ad_sd_write_reg(&st->sd, AD7793_REG_MODE, sizeof(st->mode), st->mode); - iio_device_release_direct_mode(indio_dev); - - return len; -} - -static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO, - ad7793_read_frequency, - ad7793_write_frequency); -
This commit is a follow-up to changes made to ad_sigma_delta.h in staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ which broke ad7793 as it was not altered to match those changes. This driver predates the availability of IIO_CHAN_INFO_SAMP_FREQ attribute wherein usage has some advantages like it can be accessed by in-kernel consumers as well as reduces the code size. Therefore, use IIO_CHAN_INFO_SAMP_FREQ to implement the sampling_frequency attribute instead of using IIO_DEV_ATTR_SAMP_FREQ() macro. Move code from the functions associated with IIO_DEV_ATTR_SAMP_FREQ() into respective read and write hooks with the mask set to IIO_CHAN_INFO_SAMP_FREQ. Signed-off-by: Michael Nosthoff <committed@heine.so> --- drivers/iio/adc/ad7793.c | 74 +++++++++++++++--------------------------------- 1 file changed, 23 insertions(+), 51 deletions(-) static IIO_CONST_ATTR_SAMP_FREQ_AVAIL( "470 242 123 62 50 39 33 19 17 16 12 10 8 6 4"); @@ -424,7 +375,6 @@ static IIO_DEVICE_ATTR_NAMED(in_m_in_scale_available, ad7793_show_scale_available, NULL, 0); static struct attribute *ad7793_attributes[] = { - &iio_dev_attr_sampling_frequency.dev_attr.attr, &iio_const_attr_sampling_frequency_available.dev_attr.attr, &iio_dev_attr_in_m_in_scale_available.dev_attr.attr, NULL @@ -435,7 +385,6 @@ static const struct attribute_group ad7793_attribute_group = { }; static struct attribute *ad7797_attributes[] = { - &iio_dev_attr_sampling_frequency.dev_attr.attr, &iio_const_attr_sampling_frequency_available_ad7797.dev_attr.attr, NULL }; @@ -505,6 +454,9 @@ static int ad7793_read_raw(struct iio_dev *indio_dev, *val -= offset; } return IIO_VAL_INT; + case IIO_CHAN_INFO_SAMP_FREQ: + *val = st->chip_info->sample_freq_avail[AD7793_MODE_RATE(st->mode)]; + return IIO_VAL_INT; } return -EINVAL; } @@ -542,6 +494,26 @@ static int ad7793_write_raw(struct iio_dev *indio_dev, break; } break; + case IIO_CHAN_INFO_SAMP_FREQ: + if (!val) { + ret = -EINVAL; + break; + } + + for (i = 0; i < 16; i++) + if (val == st->chip_info->sample_freq_avail[i]) + break; + + if (i == 16) { + ret = -EINVAL; + break; + } + + st->mode &= ~AD7793_MODE_RATE(-1); + st->mode |= AD7793_MODE_RATE(i); + ad_sd_write_reg(&st->sd, AD7793_REG_MODE, sizeof(st->mode), st->mode); + ret = 0; + break; default: ret = -EINVAL; } -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html