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] [day] [month] [year] [list]

Message-ID: <20180320013343.f4swlia3io574r5c@smtp.gmail.com>
Date:   Mon, 19 Mar 2018 22:33:43 -0300
From:   Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
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

Hi,

On 03/17, Jonathan Cameron wrote:
> 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

Thanks for the feedback.
Now I am thinking about this module (and IIO subsystem) from another
perspective :)

I will rethink and came with another approach. 
 
> 
> > ---
> >  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,
>