| Message ID | 335cf727d0583ccbe95f8bd97c282bf1fa210cae.1520956563.git.rodrigosiqueiramelo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, -- 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
Hi, On 03/17, Jonathan Cameron wrote: > 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 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, > -- 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
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), + }, +}; + 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,
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> --- drivers/staging/iio/resolver/ad2s1210.c | 43 ++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 14 deletions(-)