* Re: [PATCH v2] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ [not found] ` <a1258b09-192c-3870-e7b2-057ceeb1dc62@kernel.org> @ 2016-10-03 4:00 ` Eva Rachel Retuya 2016-10-03 19:56 ` Jonathan Cameron 0 siblings, 1 reply; 4+ messages in thread From: Eva Rachel Retuya @ 2016-10-03 4:00 UTC (permalink / raw) To: Jonathan Cameron Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, outreachy-kernel, linux-iio On Sat, Oct 01, 2016 at 12:35:26PM +0100, Jonathan Cameron wrote: > On 01/10/16 08:49, Eva Rachel Retuya wrote: > > 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: Eva Rachel Retuya <eraretuya@gmail.com> > Given this is heading away from the generic staging fixes and into the > list of specific IIO tasks, please do make sure to cc the linux-iio > list. > > (I'd prefer that for all IIO touching patches - but give that's somewhat > of an oddity for staging I don't really mind that much) > My apologies for that. I will include the linux-iio list in the future revisions and patch submissions. (cc'ing the list now..) > Otherwise, almost perfect, but there is a weird corner in this driver. > > Take a look at what write_raw_get_fmt is set to... > For this write it should return be IIO_VAL_INT; > I had set the return to IIO_VAL_INT already. Can you please point out where else I had missed? > Lars / Michael, this driver is only a very small distance from being > fine to move out of staging. I'm basically seeing two bits of > custom ABI that need documenting and review, but otherwise post > this cleanup looks in pretty good state to me. > By any chance, are you referring to these: static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available, in_voltage-voltage_scale_available, S_IRUGO, ad7192_show_scale_available, NULL, 0); static IIO_DEVICE_ATTR(in_voltage_scale_available, S_IRUGO, ad7192_show_scale_available, NULL, 0); Can you please guide me as to what to do next on the "documenting" part, and should I send a patchset instead with this and the documentation bit put together? Thanks for the feedback, Eva > Thanks, > > Jonathan > > --- > > Changes in v2: > > * Make commit message more detailed > > * Fix tiny bug about bypassing iio_device_release_direct_mode() > > * Remove unneeded goto and use break instead > > > > drivers/staging/iio/adc/ad7192.c | 75 ++++++++++------------------------ > > include/linux/iio/adc/ad_sigma_delta.h | 1 + > > 2 files changed, 22 insertions(+), 54 deletions(-) > > > > diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c > > index 1cf6b79..e6e4505 100644 > > --- a/drivers/staging/iio/adc/ad7192.c > > +++ b/drivers/staging/iio/adc/ad7192.c > > @@ -322,57 +322,6 @@ out: > > return ret; > > } > > > > -static ssize_t ad7192_read_frequency(struct device *dev, > > - struct device_attribute *attr, > > - char *buf) > > -{ > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > - struct ad7192_state *st = iio_priv(indio_dev); > > - > > - return sprintf(buf, "%d\n", st->mclk / > > - (st->f_order * 1024 * AD7192_MODE_RATE(st->mode))); > > -} > > - > > -static ssize_t ad7192_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 ad7192_state *st = iio_priv(indio_dev); > > - unsigned long lval; > > - int div, ret; > > - > > - ret = kstrtoul(buf, 10, &lval); > > - if (ret) > > - return ret; > > - if (lval == 0) > > - return -EINVAL; > > - > > - ret = iio_device_claim_direct_mode(indio_dev); > > - if (ret) > > - return ret; > > - > > - div = st->mclk / (lval * st->f_order * 1024); > > - if (div < 1 || div > 1023) { > > - ret = -EINVAL; > > - goto out; > > - } > > - > > - st->mode &= ~AD7192_MODE_RATE(-1); > > - st->mode |= AD7192_MODE_RATE(div); > > - ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode); > > - > > -out: > > - iio_device_release_direct_mode(indio_dev); > > - > > - return ret ? ret : len; > > -} > > - > > -static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO, > > - ad7192_read_frequency, > > - ad7192_write_frequency); > > - > > static ssize_t > > ad7192_show_scale_available(struct device *dev, > > struct device_attribute *attr, char *buf) > > @@ -471,7 +420,6 @@ static IIO_DEVICE_ATTR(ac_excitation_en, S_IRUGO | S_IWUSR, > > AD7192_REG_MODE); > > > > static struct attribute *ad7192_attributes[] = { > > - &iio_dev_attr_sampling_frequency.dev_attr.attr, > > &iio_dev_attr_in_v_m_v_scale_available.dev_attr.attr, > > &iio_dev_attr_in_voltage_scale_available.dev_attr.attr, > > &iio_dev_attr_bridge_switch_en.dev_attr.attr, > > @@ -484,7 +432,6 @@ static const struct attribute_group ad7192_attribute_group = { > > }; > > > > static struct attribute *ad7195_attributes[] = { > > - &iio_dev_attr_sampling_frequency.dev_attr.attr, > > &iio_dev_attr_in_v_m_v_scale_available.dev_attr.attr, > > &iio_dev_attr_in_voltage_scale_available.dev_attr.attr, > > &iio_dev_attr_bridge_switch_en.dev_attr.attr, > > @@ -536,6 +483,10 @@ static int ad7192_read_raw(struct iio_dev *indio_dev, > > if (chan->type == IIO_TEMP) > > *val -= 273 * ad7192_get_temp_scale(unipolar); > > return IIO_VAL_INT; > > + case IIO_CHAN_INFO_SAMP_FREQ: > > + *val = st->mclk / > > + (st->f_order * 1024 * AD7192_MODE_RATE(st->mode)); > > + return IIO_VAL_INT; > > } > > > > return -EINVAL; > > @@ -548,7 +499,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, > > long mask) > > { > > struct ad7192_state *st = iio_priv(indio_dev); > > - int ret, i; > > + int ret, i, div; > > unsigned int tmp; > > > > ret = iio_device_claim_direct_mode(indio_dev); > > @@ -572,6 +523,22 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, > > break; > > } > > break; > > + case IIO_CHAN_INFO_SAMP_FREQ: > > + if (!val) { > > + ret = -EINVAL; > > + break; > > + } > > + > > + div = st->mclk / (val * st->f_order * 1024); > > + if (div < 1 || div > 1023) { > > + ret = -EINVAL; > > + break; > > + } > > + > > + st->mode &= ~AD7192_MODE_RATE(-1); > > + st->mode |= AD7192_MODE_RATE(div); > > + ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode); > > + break; > > default: > > ret = -EINVAL; > > } > > diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h > > index e7fdec4..5ba430c 100644 > > --- a/include/linux/iio/adc/ad_sigma_delta.h > > +++ b/include/linux/iio/adc/ad_sigma_delta.h > > @@ -136,6 +136,7 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig); > > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > > BIT(IIO_CHAN_INFO_OFFSET), \ > > .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > .scan_index = (_si), \ > > .scan_type = { \ > > .sign = 'u', \ > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ 2016-10-03 4:00 ` [PATCH v2] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ Eva Rachel Retuya @ 2016-10-03 19:56 ` Jonathan Cameron 2016-10-04 18:09 ` Alison Schofield 2016-10-04 19:10 ` Lars-Peter Clausen 0 siblings, 2 replies; 4+ messages in thread From: Jonathan Cameron @ 2016-10-03 19:56 UTC (permalink / raw) To: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, outreachy-kernel, linux-iio On 03/10/16 05:00, Eva Rachel Retuya wrote: > On Sat, Oct 01, 2016 at 12:35:26PM +0100, Jonathan Cameron wrote: >> On 01/10/16 08:49, Eva Rachel Retuya wrote: >>> 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: Eva Rachel Retuya <eraretuya@gmail.com> >> Given this is heading away from the generic staging fixes and into the >> list of specific IIO tasks, please do make sure to cc the linux-iio >> list. >> >> (I'd prefer that for all IIO touching patches - but give that's somewhat >> of an oddity for staging I don't really mind that much) >> > > My apologies for that. I will include the linux-iio list in the future > revisions and patch submissions. (cc'ing the list now..) > >> Otherwise, almost perfect, but there is a weird corner in this driver. >> >> Take a look at what write_raw_get_fmt is set to... >> For this write it should return be IIO_VAL_INT; >> > > I had set the return to IIO_VAL_INT already. Can you please point out where > else I had missed? You return that for the read which is quite correct, the interesting one is the write_raw callback change. Have a bit of a dig into how that knows what it is getting (in val and val2). > >> Lars / Michael, this driver is only a very small distance from being >> fine to move out of staging. I'm basically seeing two bits of >> custom ABI that need documenting and review, but otherwise post >> this cleanup looks in pretty good state to me. >> > > By any chance, are you referring to these: > static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available, > in_voltage-voltage_scale_available, > S_IRUGO, ad7192_show_scale_available, NULL, 0); > > static IIO_DEVICE_ATTR(in_voltage_scale_available, S_IRUGO, > ad7192_show_scale_available, NULL, 0); Those two are actually standard ABI (though there is a long term plan to handle the available attributes in a nicer fashion) The custom pair are bridge_switch_en and ac_excitation_en, > > Can you please guide me as to what to do next on the "documenting" part, > and should I send a patchset instead with this and the documentation bit > put together? This one definitely wants input from Lars-Peter. I don't know the hardware at all. It might be possible to work out what these are well enough to figure out if the ABI is 'right' (by which I mean well defined within the full set of IIO ABIs) and to work out some documentation. However such docs would still want a review from Lars or one of his colleagues. > > Thanks for the feedback, You are welcome. Jonathan > Eva > >> Thanks, >> >> Jonathan >>> --- >>> Changes in v2: >>> * Make commit message more detailed >>> * Fix tiny bug about bypassing iio_device_release_direct_mode() >>> * Remove unneeded goto and use break instead >>> >>> drivers/staging/iio/adc/ad7192.c | 75 ++++++++++------------------------ >>> include/linux/iio/adc/ad_sigma_delta.h | 1 + >>> 2 files changed, 22 insertions(+), 54 deletions(-) >>> >>> diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c >>> index 1cf6b79..e6e4505 100644 >>> --- a/drivers/staging/iio/adc/ad7192.c >>> +++ b/drivers/staging/iio/adc/ad7192.c >>> @@ -322,57 +322,6 @@ out: >>> return ret; >>> } >>> >>> -static ssize_t ad7192_read_frequency(struct device *dev, >>> - struct device_attribute *attr, >>> - char *buf) >>> -{ >>> - struct iio_dev *indio_dev = dev_to_iio_dev(dev); >>> - struct ad7192_state *st = iio_priv(indio_dev); >>> - >>> - return sprintf(buf, "%d\n", st->mclk / >>> - (st->f_order * 1024 * AD7192_MODE_RATE(st->mode))); >>> -} >>> - >>> -static ssize_t ad7192_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 ad7192_state *st = iio_priv(indio_dev); >>> - unsigned long lval; >>> - int div, ret; >>> - >>> - ret = kstrtoul(buf, 10, &lval); >>> - if (ret) >>> - return ret; >>> - if (lval == 0) >>> - return -EINVAL; >>> - >>> - ret = iio_device_claim_direct_mode(indio_dev); >>> - if (ret) >>> - return ret; >>> - >>> - div = st->mclk / (lval * st->f_order * 1024); >>> - if (div < 1 || div > 1023) { >>> - ret = -EINVAL; >>> - goto out; >>> - } >>> - >>> - st->mode &= ~AD7192_MODE_RATE(-1); >>> - st->mode |= AD7192_MODE_RATE(div); >>> - ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode); >>> - >>> -out: >>> - iio_device_release_direct_mode(indio_dev); >>> - >>> - return ret ? ret : len; >>> -} >>> - >>> -static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO, >>> - ad7192_read_frequency, >>> - ad7192_write_frequency); >>> - >>> static ssize_t >>> ad7192_show_scale_available(struct device *dev, >>> struct device_attribute *attr, char *buf) >>> @@ -471,7 +420,6 @@ static IIO_DEVICE_ATTR(ac_excitation_en, S_IRUGO | S_IWUSR, >>> AD7192_REG_MODE); >>> >>> static struct attribute *ad7192_attributes[] = { >>> - &iio_dev_attr_sampling_frequency.dev_attr.attr, >>> &iio_dev_attr_in_v_m_v_scale_available.dev_attr.attr, >>> &iio_dev_attr_in_voltage_scale_available.dev_attr.attr, >>> &iio_dev_attr_bridge_switch_en.dev_attr.attr, >>> @@ -484,7 +432,6 @@ static const struct attribute_group ad7192_attribute_group = { >>> }; >>> >>> static struct attribute *ad7195_attributes[] = { >>> - &iio_dev_attr_sampling_frequency.dev_attr.attr, >>> &iio_dev_attr_in_v_m_v_scale_available.dev_attr.attr, >>> &iio_dev_attr_in_voltage_scale_available.dev_attr.attr, >>> &iio_dev_attr_bridge_switch_en.dev_attr.attr, >>> @@ -536,6 +483,10 @@ static int ad7192_read_raw(struct iio_dev *indio_dev, >>> if (chan->type == IIO_TEMP) >>> *val -= 273 * ad7192_get_temp_scale(unipolar); >>> return IIO_VAL_INT; >>> + case IIO_CHAN_INFO_SAMP_FREQ: >>> + *val = st->mclk / >>> + (st->f_order * 1024 * AD7192_MODE_RATE(st->mode)); >>> + return IIO_VAL_INT; >>> } >>> >>> return -EINVAL; >>> @@ -548,7 +499,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, >>> long mask) >>> { >>> struct ad7192_state *st = iio_priv(indio_dev); >>> - int ret, i; >>> + int ret, i, div; >>> unsigned int tmp; >>> >>> ret = iio_device_claim_direct_mode(indio_dev); >>> @@ -572,6 +523,22 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, >>> break; >>> } >>> break; >>> + case IIO_CHAN_INFO_SAMP_FREQ: >>> + if (!val) { >>> + ret = -EINVAL; >>> + break; >>> + } >>> + >>> + div = st->mclk / (val * st->f_order * 1024); >>> + if (div < 1 || div > 1023) { >>> + ret = -EINVAL; >>> + break; >>> + } >>> + >>> + st->mode &= ~AD7192_MODE_RATE(-1); >>> + st->mode |= AD7192_MODE_RATE(div); >>> + ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode); >>> + break; >>> default: >>> ret = -EINVAL; >>> } >>> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h >>> index e7fdec4..5ba430c 100644 >>> --- a/include/linux/iio/adc/ad_sigma_delta.h >>> +++ b/include/linux/iio/adc/ad_sigma_delta.h >>> @@ -136,6 +136,7 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig); >>> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>> BIT(IIO_CHAN_INFO_OFFSET), \ >>> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ >>> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ >>> .scan_index = (_si), \ >>> .scan_type = { \ >>> .sign = 'u', \ >>> >> > -- > 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ 2016-10-03 19:56 ` Jonathan Cameron @ 2016-10-04 18:09 ` Alison Schofield 2016-10-04 19:10 ` Lars-Peter Clausen 1 sibling, 0 replies; 4+ messages in thread From: Alison Schofield @ 2016-10-04 18:09 UTC (permalink / raw) To: eraretuya, Jonathan Cameron Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, outreachy-kernel, linux-iio, daniel.baluta On Mon, Oct 03, 2016 at 08:56:07PM +0100, Jonathan Cameron wrote: > On 03/10/16 05:00, Eva Rachel Retuya wrote: > > On Sat, Oct 01, 2016 at 12:35:26PM +0100, Jonathan Cameron wrote: > >> On 01/10/16 08:49, Eva Rachel Retuya wrote: > >>> 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: Eva Rachel Retuya <eraretuya@gmail.com> > >> Given this is heading away from the generic staging fixes and into the > >> list of specific IIO tasks, please do make sure to cc the linux-iio > >> list. > >> > >> (I'd prefer that for all IIO touching patches - but give that's somewhat > >> of an oddity for staging I don't really mind that much) > >> > > > > My apologies for that. I will include the linux-iio list in the future > > revisions and patch submissions. (cc'ing the list now..) > > > >> Otherwise, almost perfect, but there is a weird corner in this driver. > >> > >> Take a look at what write_raw_get_fmt is set to... > >> For this write it should return be IIO_VAL_INT; > >> > > > > I had set the return to IIO_VAL_INT already. Can you please point out where > > else I had missed? > You return that for the read which is quite correct, the interesting one > is the write_raw callback change. Have a bit of a dig into how that knows > what it is getting (in val and val2). Hi Eva, Replying back in this main thread to keep all in one place. If you go look at the iio_info struct definition (be sure to scroll up and see the comments), you'll find this info you need. I believe you'll add a case for your new attribute to *write_raw_get_fmt(). See if that gets you further along. Questions, please ask in this thread. Thanks, alisons > > > > >> Lars / Michael, this driver is only a very small distance from being > >> fine to move out of staging. I'm basically seeing two bits of > >> custom ABI that need documenting and review, but otherwise post > >> this cleanup looks in pretty good state to me. > >> > > > > By any chance, are you referring to these: > > static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available, > > in_voltage-voltage_scale_available, > > S_IRUGO, ad7192_show_scale_available, NULL, 0); > > > > static IIO_DEVICE_ATTR(in_voltage_scale_available, S_IRUGO, > > ad7192_show_scale_available, NULL, 0); > Those two are actually standard ABI (though there is a long term plan to handle > the available attributes in a nicer fashion) > > The custom pair are bridge_switch_en and ac_excitation_en, > > > > Can you please guide me as to what to do next on the "documenting" part, > > and should I send a patchset instead with this and the documentation bit > > put together? > This one definitely wants input from Lars-Peter. I don't know the hardware > at all. It might be possible to work out what these are well enough to > figure out if the ABI is 'right' (by which I mean well defined within the > full set of IIO ABIs) and to work out some documentation. > > However such docs would still want a review from Lars or one of his colleagues. > > > > Thanks for the feedback, > You are welcome. > > Jonathan > > Eva > > > >> Thanks, > >> > >> Jonathan > >>> --- > >>> Changes in v2: > >>> * Make commit message more detailed > >>> * Fix tiny bug about bypassing iio_device_release_direct_mode() > >>> * Remove unneeded goto and use break instead > >>> > >>> drivers/staging/iio/adc/ad7192.c | 75 ++++++++++------------------------ > >>> include/linux/iio/adc/ad_sigma_delta.h | 1 + > >>> 2 files changed, 22 insertions(+), 54 deletions(-) > >>> > >>> diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c > >>> index 1cf6b79..e6e4505 100644 > >>> --- a/drivers/staging/iio/adc/ad7192.c > >>> +++ b/drivers/staging/iio/adc/ad7192.c > >>> @@ -322,57 +322,6 @@ out: > >>> return ret; > >>> } > >>> > >>> -static ssize_t ad7192_read_frequency(struct device *dev, > >>> - struct device_attribute *attr, > >>> - char *buf) > >>> -{ > >>> - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > >>> - struct ad7192_state *st = iio_priv(indio_dev); > >>> - > >>> - return sprintf(buf, "%d\n", st->mclk / > >>> - (st->f_order * 1024 * AD7192_MODE_RATE(st->mode))); > >>> -} > >>> - > >>> -static ssize_t ad7192_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 ad7192_state *st = iio_priv(indio_dev); > >>> - unsigned long lval; > >>> - int div, ret; > >>> - > >>> - ret = kstrtoul(buf, 10, &lval); > >>> - if (ret) > >>> - return ret; > >>> - if (lval == 0) > >>> - return -EINVAL; > >>> - > >>> - ret = iio_device_claim_direct_mode(indio_dev); > >>> - if (ret) > >>> - return ret; > >>> - > >>> - div = st->mclk / (lval * st->f_order * 1024); > >>> - if (div < 1 || div > 1023) { > >>> - ret = -EINVAL; > >>> - goto out; > >>> - } > >>> - > >>> - st->mode &= ~AD7192_MODE_RATE(-1); > >>> - st->mode |= AD7192_MODE_RATE(div); > >>> - ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode); > >>> - > >>> -out: > >>> - iio_device_release_direct_mode(indio_dev); > >>> - > >>> - return ret ? ret : len; > >>> -} > >>> - > >>> -static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO, > >>> - ad7192_read_frequency, > >>> - ad7192_write_frequency); > >>> - > >>> static ssize_t > >>> ad7192_show_scale_available(struct device *dev, > >>> struct device_attribute *attr, char *buf) > >>> @@ -471,7 +420,6 @@ static IIO_DEVICE_ATTR(ac_excitation_en, S_IRUGO | S_IWUSR, > >>> AD7192_REG_MODE); > >>> > >>> static struct attribute *ad7192_attributes[] = { > >>> - &iio_dev_attr_sampling_frequency.dev_attr.attr, > >>> &iio_dev_attr_in_v_m_v_scale_available.dev_attr.attr, > >>> &iio_dev_attr_in_voltage_scale_available.dev_attr.attr, > >>> &iio_dev_attr_bridge_switch_en.dev_attr.attr, > >>> @@ -484,7 +432,6 @@ static const struct attribute_group ad7192_attribute_group = { > >>> }; > >>> > >>> static struct attribute *ad7195_attributes[] = { > >>> - &iio_dev_attr_sampling_frequency.dev_attr.attr, > >>> &iio_dev_attr_in_v_m_v_scale_available.dev_attr.attr, > >>> &iio_dev_attr_in_voltage_scale_available.dev_attr.attr, > >>> &iio_dev_attr_bridge_switch_en.dev_attr.attr, > >>> @@ -536,6 +483,10 @@ static int ad7192_read_raw(struct iio_dev *indio_dev, > >>> if (chan->type == IIO_TEMP) > >>> *val -= 273 * ad7192_get_temp_scale(unipolar); > >>> return IIO_VAL_INT; > >>> + case IIO_CHAN_INFO_SAMP_FREQ: > >>> + *val = st->mclk / > >>> + (st->f_order * 1024 * AD7192_MODE_RATE(st->mode)); > >>> + return IIO_VAL_INT; > >>> } > >>> > >>> return -EINVAL; > >>> @@ -548,7 +499,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, > >>> long mask) > >>> { > >>> struct ad7192_state *st = iio_priv(indio_dev); > >>> - int ret, i; > >>> + int ret, i, div; > >>> unsigned int tmp; > >>> > >>> ret = iio_device_claim_direct_mode(indio_dev); > >>> @@ -572,6 +523,22 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, > >>> break; > >>> } > >>> break; > >>> + case IIO_CHAN_INFO_SAMP_FREQ: > >>> + if (!val) { > >>> + ret = -EINVAL; > >>> + break; > >>> + } > >>> + > >>> + div = st->mclk / (val * st->f_order * 1024); > >>> + if (div < 1 || div > 1023) { > >>> + ret = -EINVAL; > >>> + break; > >>> + } > >>> + > >>> + st->mode &= ~AD7192_MODE_RATE(-1); > >>> + st->mode |= AD7192_MODE_RATE(div); > >>> + ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode); > >>> + break; > >>> default: > >>> ret = -EINVAL; > >>> } > >>> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h > >>> index e7fdec4..5ba430c 100644 > >>> --- a/include/linux/iio/adc/ad_sigma_delta.h > >>> +++ b/include/linux/iio/adc/ad_sigma_delta.h > >>> @@ -136,6 +136,7 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig); > >>> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > >>> BIT(IIO_CHAN_INFO_OFFSET), \ > >>> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > >>> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > >>> .scan_index = (_si), \ > >>> .scan_type = { \ > >>> .sign = 'u', \ > >>> > >> > > -- > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ 2016-10-03 19:56 ` Jonathan Cameron 2016-10-04 18:09 ` Alison Schofield @ 2016-10-04 19:10 ` Lars-Peter Clausen 1 sibling, 0 replies; 4+ messages in thread From: Lars-Peter Clausen @ 2016-10-04 19:10 UTC (permalink / raw) To: Jonathan Cameron, Michael.Hennerich, knaack.h, pmeerw, gregkh, outreachy-kernel, linux-iio On 10/03/2016 09:56 PM, Jonathan Cameron wrote: > On 03/10/16 05:00, Eva Rachel Retuya wrote: >> On Sat, Oct 01, 2016 at 12:35:26PM +0100, Jonathan Cameron wrote: >>> On 01/10/16 08:49, Eva Rachel Retuya wrote: >>>> 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: Eva Rachel Retuya <eraretuya@gmail.com> >>> Given this is heading away from the generic staging fixes and into the >>> list of specific IIO tasks, please do make sure to cc the linux-iio >>> list. >>> >>> (I'd prefer that for all IIO touching patches - but give that's somewhat >>> of an oddity for staging I don't really mind that much) >>> >> >> My apologies for that. I will include the linux-iio list in the future >> revisions and patch submissions. (cc'ing the list now..) >> >>> Otherwise, almost perfect, but there is a weird corner in this driver. >>> >>> Take a look at what write_raw_get_fmt is set to... >>> For this write it should return be IIO_VAL_INT; >>> >> >> I had set the return to IIO_VAL_INT already. Can you please point out where >> else I had missed? > You return that for the read which is quite correct, the interesting one > is the write_raw callback change. Have a bit of a dig into how that knows > what it is getting (in val and val2). > >> >>> Lars / Michael, this driver is only a very small distance from being >>> fine to move out of staging. I'm basically seeing two bits of >>> custom ABI that need documenting and review, but otherwise post >>> this cleanup looks in pretty good state to me. >>> >> >> By any chance, are you referring to these: >> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available, >> in_voltage-voltage_scale_available, >> S_IRUGO, ad7192_show_scale_available, NULL, 0); >> >> static IIO_DEVICE_ATTR(in_voltage_scale_available, S_IRUGO, >> ad7192_show_scale_available, NULL, 0); > Those two are actually standard ABI (though there is a long term plan to handle > the available attributes in a nicer fashion) > > The custom pair are bridge_switch_en and ac_excitation_en, I've looked at this a while ago and I think the bridge switch should really be a GPIO, since that is what iti s. At last logically, it's an external pin that can be on or off. At least logically, it is special physically and intended to be used for specific function in a measurement applications. It is capable of sinking a much higher current than the other GPIOs. The only downside of making it a GPIO is that it is no longer part of the IIO device and needs to be accessed via other methods which makes some applications more complicated to implement. The excitation is a bit different, it controls a circuit that toggles two external pins in relation to the sampling frequency. They are meant to be connected to external circuitry that reverses the polarities of the excitation voltage of the bridge. This is done to remove any onesided bias. This is a bit more difficult to express in generic API, maybe we could make it non user controllable for now and enable it unconditionally if it is connected (reported by either platform data or DT) and otherwise leave it off. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-10-04 19:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1475308164-3461-1-git-send-email-eraretuya@gmail.com>
[not found] ` <a1258b09-192c-3870-e7b2-057ceeb1dc62@kernel.org>
2016-10-03 4:00 ` [PATCH v2] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ Eva Rachel Retuya
2016-10-03 19:56 ` Jonathan Cameron
2016-10-04 18:09 ` Alison Schofield
2016-10-04 19:10 ` Lars-Peter Clausen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).