Re: [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and fexcit

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 13 Mar 2018 13:05:28 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> The ad2s1210 does not contain any channel for the fclkin and fexcit. As
> a result, it uses IIO_DEVICE_ATTR to expose this information. This patch
> adds one channel for fclkin and another for fexcit. It also adds an enum
> to easily address the correct channel.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Take a step back.  What are these new channels actually for?
We aren't looking at a general purpose frequency input and output.
(mind you they are both currently inputs which makes even less sense!)

These are controls for the excitation frequency for a resolver.
What is userspace going to do with them?  Nothing of any use certainly.

So what do they actually change that matters to an application?
1) The speed at which we can detect a loss of signal condition.
2) The achievable resolution of the sensor.

So how would userspace know how to configure it?  I'm not sure it
would, but it is these things that should be driving the choice
not the actual value of the frequency (which is really just
a weird internal value in the way the resolver system works).

There is a pretty strong argument that we should leave the excitation
frequency alone at 10kHz unless the platform designer knows better,
in which case they should supply it via devicetree rather than
from userspace.

Anyhow, it needs a rethink - exposing these as channels is not
the way to go!

Jonathan


> ---
>  drivers/staging/iio/resolver/ad2s1210.c | 43 ++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index ac13b99bd9cb..28c3fd439663 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -67,6 +67,11 @@ enum ad2s1210_mode {
>  	MOD_RESERVED,
>  };
>  
> +enum ad2s1210_frequency_channel {
> +	FCLKIN = 0,
> +	FEXCIT,
> +};
> +
>  static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
>  
>  struct ad2s1210_state {
> @@ -88,6 +93,30 @@ static const int ad2s1210_mode_vals[4][2] = {
>  	[MOD_CONFIG] = { 1, 0 },
>  };
>  
> +static const struct iio_chan_spec ad2s1210_channels[] = {
> +	{
> +		.type = IIO_ANGL,
> +		.indexed = 1,
> +		.channel = 0,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	}, {
> +		.type = IIO_ANGL_VEL,
> +		.indexed = 1,
> +		.channel = 0,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	}, {
> +		.type = IIO_CHAN_INFO_FREQUENCY,
> +		.indexed = 1,
> +		.channel = FCLKIN,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	}, {
> +		.type = IIO_CHAN_INFO_FREQUENCY,
> +		.indexed = 1,
> +		.channel = FEXCIT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +};
> +
This seems broken, you can't just add a channel and not support
it until the following patches.

>  static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
>  				     struct ad2s1210_state *st)
>  {
> @@ -552,20 +581,6 @@ static IIO_DEVICE_ATTR(lot_low_thrd, 0644,
>  		       ad2s1210_show_reg, ad2s1210_store_reg,
>  		       AD2S1210_REG_LOT_LOW_THRD);
>  
> -static const struct iio_chan_spec ad2s1210_channels[] = {
> -	{
> -		.type = IIO_ANGL,
> -		.indexed = 1,
> -		.channel = 0,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> -	}, {
> -		.type = IIO_ANGL_VEL,
> -		.indexed = 1,
> -		.channel = 0,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> -	}
> -};
> -
>  static struct attribute *ad2s1210_attributes[] = {
>  	&iio_dev_attr_fclkin.dev_attr.attr,
>  	&iio_dev_attr_fexcit.dev_attr.attr,