Hi,
On 03/17, Jonathan Cameron wrote:
> On Tue, 13 Mar 2018 13:05:28 -0300
> Rodrigo Siqueira <rodrigosiqueiram...@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 <rodrigosiqueiram...@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
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,
>
