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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180317211208.3cd990d8@archlinux>
Date:   Sat, 17 Mar 2018 21:12:08 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Hartmut Knaack <knaack.h@....de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Graff Yang <graff.yang@...il.com>, daniel.baluta@....com,
        linux-iio@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and
 fexcit

On Tue, 13 Mar 2018 13:05:28 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@...il.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@...il.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,