Re: [PATCH v2] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ

Re: [PATCH v2] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ

[Eva Rachel Retuya]

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', \
> > 
> 

Re: [PATCH v2] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ

[Jonathan Cameron]

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
> 

Re: [PATCH v2] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ

[Alison Schofield]

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

Re: [PATCH v2] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ

[Lars-Peter Clausen]

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.