[PATCH 1/2] iio:accel:bma180: Use modifier instead of index in channel specification

[PATCH 1/2] iio:accel:bma180: Use modifier instead of index in channel specification

[Peter Meerwald]

should use channel modifiers (X/Y/Z), not channel indices
timestamp channel has scan index 3, not 4

Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
Cc: Kravchenko Oleksandr <x0199363@ti.com>
---
 drivers/iio/accel/bma180.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
index 3bec922..bfec313 100644
--- a/drivers/iio/accel/bma180.c
+++ b/drivers/iio/accel/bma180.c
@@ -447,14 +447,14 @@ static const struct iio_chan_spec_ext_info bma180_ext_info[] = {
 	{ },
 };
 
-#define BMA180_CHANNEL(_index) {					\
+#define BMA180_CHANNEL(_axis) {					\
 	.type = IIO_ACCEL,						\
-	.indexed = 1,							\
-	.channel = (_index),						\
+	.modified = 1,							\
+	.channel2 = IIO_MOD_##_axis,					\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
 		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),	\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
-	.scan_index = (_index),						\
+	.scan_index = AXIS_##_axis,					\
 	.scan_type = {							\
 		.sign = 's',						\
 		.realbits = 14,						\
@@ -465,10 +465,10 @@ static const struct iio_chan_spec_ext_info bma180_ext_info[] = {
 }
 
 static const struct iio_chan_spec bma180_channels[] = {
-	BMA180_CHANNEL(AXIS_X),
-	BMA180_CHANNEL(AXIS_Y),
-	BMA180_CHANNEL(AXIS_Z),
-	IIO_CHAN_SOFT_TIMESTAMP(4),
+	BMA180_CHANNEL(X),
+	BMA180_CHANNEL(Y),
+	BMA180_CHANNEL(Z),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
 static irqreturn_t bma180_trigger_handler(int irq, void *p)
-- 
1.8.3.2

[PATCH 2/2] iio:accel:bma180: Make LOW_PASS_FILTER_3DB_FREQUENCY shared_by_type

[Peter Meerwald]

the property is not per-channel, but shared by type

Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
Cc: Kravchenko Oleksandr <x0199363@ti.com>
---
 drivers/iio/accel/bma180.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
index bfec313..a7e68c8 100644
--- a/drivers/iio/accel/bma180.c
+++ b/drivers/iio/accel/bma180.c
@@ -451,9 +451,9 @@ static const struct iio_chan_spec_ext_info bma180_ext_info[] = {
 	.type = IIO_ACCEL,						\
 	.modified = 1,							\
 	.channel2 = IIO_MOD_##_axis,					\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
 		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),	\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
 	.scan_index = AXIS_##_axis,					\
 	.scan_type = {							\
 		.sign = 's',						\
-- 
1.8.3.2

Re: [PATCH 1/2] iio:accel:bma180: Use modifier instead of index in channel specification

[Jonathan Cameron]

On 10/01/14 21:37, Peter Meerwald wrote:
> should use channel modifiers (X/Y/Z), not channel indices
> timestamp channel has scan index 3, not 4
>
Aag.  Whilst a correct fix, this is going to result in a userspace ABI
change.  Driver has been in since 3.12.  We may need to maintain
both ABI's for now and then deprecate it at some point.  Easiest way to do this
will be to have two sets of iio_chan_specs pointing at the same underlying
real channels.

Anyone have a strong opinion about this?

Good spot by the way!

> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
> Cc: Kravchenko Oleksandr <x0199363@ti.com>
> ---
>   drivers/iio/accel/bma180.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
> index 3bec922..bfec313 100644
> --- a/drivers/iio/accel/bma180.c
> +++ b/drivers/iio/accel/bma180.c
> @@ -447,14 +447,14 @@ static const struct iio_chan_spec_ext_info bma180_ext_info[] = {
>   	{ },
>   };
>
> -#define BMA180_CHANNEL(_index) {					\
> +#define BMA180_CHANNEL(_axis) {					\
>   	.type = IIO_ACCEL,						\
> -	.indexed = 1,							\
> -	.channel = (_index),						\
> +	.modified = 1,							\
> +	.channel2 = IIO_MOD_##_axis,					\
>   	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
>   		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),	\
>   	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> -	.scan_index = (_index),						\
> +	.scan_index = AXIS_##_axis,					\
>   	.scan_type = {							\
>   		.sign = 's',						\
>   		.realbits = 14,						\
> @@ -465,10 +465,10 @@ static const struct iio_chan_spec_ext_info bma180_ext_info[] = {
>   }
>
>   static const struct iio_chan_spec bma180_channels[] = {
> -	BMA180_CHANNEL(AXIS_X),
> -	BMA180_CHANNEL(AXIS_Y),
> -	BMA180_CHANNEL(AXIS_Z),
> -	IIO_CHAN_SOFT_TIMESTAMP(4),
> +	BMA180_CHANNEL(X),
> +	BMA180_CHANNEL(Y),
> +	BMA180_CHANNEL(Z),
> +	IIO_CHAN_SOFT_TIMESTAMP(3),
>   };
>
>   static irqreturn_t bma180_trigger_handler(int irq, void *p)
>

Re: [PATCH 2/2] iio:accel:bma180: Make LOW_PASS_FILTER_3DB_FREQUENCY shared_by_type

[Jonathan Cameron]

On 10/01/14 21:37, Peter Meerwald wrote:
> the property is not per-channel, but shared by type
>
Now this one is in a sense even more interesting.

1) Your fix is correct.
2) Other than poluting sysfs with extra attributes and setting a bad
    example, it doesn't actually matter as any attribute is defined
    as being able to effect any other. e.g. if we had two different
    filters, one for the z axis and another effecting both x and y
    we would have to define all 3 as separate.
3) In theory correctly written generic userspace code should not care
    as it should handle both cases without a user being any the wiser.
    Hence this should fall into the category of being able to change
    userspace ABI if no one notices...

So what shall we do?
> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
> Cc: Kravchenko Oleksandr <x0199363@ti.com>
> ---
>   drivers/iio/accel/bma180.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
> index bfec313..a7e68c8 100644
> --- a/drivers/iio/accel/bma180.c
> +++ b/drivers/iio/accel/bma180.c
> @@ -451,9 +451,9 @@ static const struct iio_chan_spec_ext_info bma180_ext_info[] = {
>   	.type = IIO_ACCEL,						\
>   	.modified = 1,							\
>   	.channel2 = IIO_MOD_##_axis,					\
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
>   		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),	\
> -	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
>   	.scan_index = AXIS_##_axis,					\
>   	.scan_type = {							\
>   		.sign = 's',						\
>

Re: [PATCH 1/2] iio:accel:bma180: Use modifier instead of index in channel specification

[Peter Meerwald]

Hello,

> > should use channel modifiers (X/Y/Z), not channel indices
> > timestamp channel has scan index 3, not 4
> > 
> Aag.  Whilst a correct fix, this is going to result in a userspace ABI
> change.  Driver has been in since 3.12.  We may need to maintain
> both ABI's for now and then deprecate it at some point.  Easiest way to do
> this
> will be to have two sets of iio_chan_specs pointing at the same underlying
> real channels.
> 
> Anyone have a strong opinion about this?
> Good spot by the way!

adding another email address of Oleksandr, x0199363@ti.com bounces

the IIO ABI is under "testing", not "stable" so the interface may be 
changed due to 'grave errors'

given the short timespan since 3.12 and the clear violation of the 
documented ABI, I think this should just be fixed

another suggestion:

often (at least for me) it is difficult to figure out what the user-space 
sysfs ABI is from reading the source code and I find it even more 
difficult to sometimes guess what a particular sysfs is supposed to do 
(e.g. what writing to a _scale actually sets in terms of the hardware); 

I'd like to suggest to ask for a listing of the sysfs entries relating 
to a new driver under review (e.g. the output of 'find 
/sys/bus/iio/devices/iio:deviceX'); it should be feasible to provide a
tool which compares the submitted listing against the sysfs-bus-iio 
documentation and output appropriate warnings

regards, p.
 
> > Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
> > Cc: Kravchenko Oleksandr <x0199363@ti.com>
> > ---
> >   drivers/iio/accel/bma180.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
> > index 3bec922..bfec313 100644
> > --- a/drivers/iio/accel/bma180.c
> > +++ b/drivers/iio/accel/bma180.c
> > @@ -447,14 +447,14 @@ static const struct iio_chan_spec_ext_info
> > bma180_ext_info[] = {
> >   	{ },
> >   };
> > 
> > -#define BMA180_CHANNEL(_index) {					\
> > +#define BMA180_CHANNEL(_axis) {					\
> >   	.type = IIO_ACCEL,						\
> > -	.indexed = 1,							\
> > -	.channel = (_index),						\
> > +	.modified = 1,							\
> > +	.channel2 = IIO_MOD_##_axis,					\
> >   	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> >   		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),	\
> >   	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> > -	.scan_index = (_index),						\
> > +	.scan_index = AXIS_##_axis,					\
> >   	.scan_type = {							\
> >   		.sign = 's',						\
> >   		.realbits = 14,						\
> > @@ -465,10 +465,10 @@ static const struct iio_chan_spec_ext_info
> > bma180_ext_info[] = {
> >   }
> > 
> >   static const struct iio_chan_spec bma180_channels[] = {
> > -	BMA180_CHANNEL(AXIS_X),
> > -	BMA180_CHANNEL(AXIS_Y),
> > -	BMA180_CHANNEL(AXIS_Z),
> > -	IIO_CHAN_SOFT_TIMESTAMP(4),
> > +	BMA180_CHANNEL(X),
> > +	BMA180_CHANNEL(Y),
> > +	BMA180_CHANNEL(Z),
> > +	IIO_CHAN_SOFT_TIMESTAMP(3),
> >   };
> > 
> >   static irqreturn_t bma180_trigger_handler(int irq, void *p)
> > 
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH 1/2] iio:accel:bma180: Use modifier instead of index in channel specification

[Jonathan Cameron]

On 11/01/14 15:25, Peter Meerwald wrote:
> Hello,
>
>>> should use channel modifiers (X/Y/Z), not channel indices
>>> timestamp channel has scan index 3, not 4
>>>
>> Aag.  Whilst a correct fix, this is going to result in a userspace ABI
>> change.  Driver has been in since 3.12.  We may need to maintain
>> both ABI's for now and then deprecate it at some point.  Easiest way to do
>> this
>> will be to have two sets of iio_chan_specs pointing at the same underlying
>> real channels.
>>
>> Anyone have a strong opinion about this?
>> Good spot by the way!
>
> adding another email address of Oleksandr, x0199363@ti.com bounces
>
> the IIO ABI is under "testing", not "stable" so the interface may be
> changed due to 'grave errors'
>
> given the short timespan since 3.12 and the clear violation of the
> documented ABI, I think this should just be fixed
Fine, I've applied this to fixes-togreg and marked it for stable.
>
> another suggestion:
>
> often (at least for me) it is difficult to figure out what the user-space
> sysfs ABI is from reading the source code and I find it even more
> difficult to sometimes guess what a particular sysfs is supposed to do
> (e.g. what writing to a _scale actually sets in terms of the hardware);
>
> I'd like to suggest to ask for a listing of the sysfs entries relating
> to a new driver under review (e.g. the output of 'find
> /sys/bus/iio/devices/iio:deviceX'); it should be feasible to provide a
> tool which compares the submitted listing against the sysfs-bus-iio
> documentation and output appropriate warnings
It would certainly be helpful to do this. I'll start asking for complex
drivers, but am not going to bother for really simple ones.
(obviously I'd be happy if the cover letter contained it in all drivers!)
>
> regards, p.
>
>>> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
>>> Cc: Kravchenko Oleksandr <x0199363@ti.com>
>>> ---
>>>    drivers/iio/accel/bma180.c | 16 ++++++++--------
>>>    1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
>>> index 3bec922..bfec313 100644
>>> --- a/drivers/iio/accel/bma180.c
>>> +++ b/drivers/iio/accel/bma180.c
>>> @@ -447,14 +447,14 @@ static const struct iio_chan_spec_ext_info
>>> bma180_ext_info[] = {
>>>    	{ },
>>>    };
>>>
>>> -#define BMA180_CHANNEL(_index) {					\
>>> +#define BMA180_CHANNEL(_axis) {					\
>>>    	.type = IIO_ACCEL,						\
>>> -	.indexed = 1,							\
>>> -	.channel = (_index),						\
>>> +	.modified = 1,							\
>>> +	.channel2 = IIO_MOD_##_axis,					\
>>>    	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
>>>    		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),	\
>>>    	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
>>> -	.scan_index = (_index),						\
>>> +	.scan_index = AXIS_##_axis,					\
>>>    	.scan_type = {							\
>>>    		.sign = 's',						\
>>>    		.realbits = 14,						\
>>> @@ -465,10 +465,10 @@ static const struct iio_chan_spec_ext_info
>>> bma180_ext_info[] = {
>>>    }
>>>
>>>    static const struct iio_chan_spec bma180_channels[] = {
>>> -	BMA180_CHANNEL(AXIS_X),
>>> -	BMA180_CHANNEL(AXIS_Y),
>>> -	BMA180_CHANNEL(AXIS_Z),
>>> -	IIO_CHAN_SOFT_TIMESTAMP(4),
>>> +	BMA180_CHANNEL(X),
>>> +	BMA180_CHANNEL(Y),
>>> +	BMA180_CHANNEL(Z),
>>> +	IIO_CHAN_SOFT_TIMESTAMP(3),
>>>    };
>>>
>>>    static irqreturn_t bma180_trigger_handler(int irq, void *p)
>>>
>>
>

Re: [PATCH 2/2] iio:accel:bma180: Make LOW_PASS_FILTER_3DB_FREQUENCY shared_by_type

[Jonathan Cameron]

On 11/01/14 11:44, Jonathan Cameron wrote:
>
>
> On 10/01/14 21:37, Peter Meerwald wrote:
>> the property is not per-channel, but shared by type
>>
> Now this one is in a sense even more interesting.
>
> 1) Your fix is correct.
> 2) Other than poluting sysfs with extra attributes and setting a bad
>     example, it doesn't actually matter as any attribute is defined
>     as being able to effect any other. e.g. if we had two different
>     filters, one for the z axis and another effecting both x and y
>     we would have to define all 3 as separate.
> 3) In theory correctly written generic userspace code should not care
>     as it should handle both cases without a user being any the wiser.
>     Hence this should fall into the category of being able to change
>     userspace ABI if no one notices...
>
> So what shall we do?
>> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
>> Cc: Kravchenko Oleksandr <x0199363@ti.com>
Peter, I'm not going to put this one through as a fix as the current interface
is fine, if messy.  Given it's dependency on patch 1 of this series, I'll  wait
for that to propogate through. Could you remind me to apply this if I forget!
>> ---
>>   drivers/iio/accel/bma180.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
>> index bfec313..a7e68c8 100644
>> --- a/drivers/iio/accel/bma180.c
>> +++ b/drivers/iio/accel/bma180.c
>> @@ -451,9 +451,9 @@ static const struct iio_chan_spec_ext_info bma180_ext_info[] = {
>>       .type = IIO_ACCEL,                        \
>>       .modified = 1,                            \
>>       .channel2 = IIO_MOD_##_axis,                    \
>> -    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |            \
>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),            \
>> +    .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |        \
>>           BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),    \
>> -    .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),        \
>>       .scan_index = AXIS_##_axis,                    \
>>       .scan_type = {                            \
>>           .sign = 's',                        \
>>

Re: [PATCH 2/2] iio:accel:bma180: Make LOW_PASS_FILTER_3DB_FREQUENCY shared_by_type

[Jonathan Cameron]

On 08/02/14 11:40, Jonathan Cameron wrote:
> On 11/01/14 11:44, Jonathan Cameron wrote:
>>
>>
>> On 10/01/14 21:37, Peter Meerwald wrote:
>>> the property is not per-channel, but shared by type
>>>
>> Now this one is in a sense even more interesting.
>>
>> 1) Your fix is correct.
>> 2) Other than poluting sysfs with extra attributes and setting a bad
>>     example, it doesn't actually matter as any attribute is defined
>>     as being able to effect any other. e.g. if we had two different
>>     filters, one for the z axis and another effecting both x and y
>>     we would have to define all 3 as separate.
>> 3) In theory correctly written generic userspace code should not care
>>     as it should handle both cases without a user being any the wiser.
>>     Hence this should fall into the category of being able to change
>>     userspace ABI if no one notices...
>>
>> So what shall we do?
>>> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
>>> Cc: Kravchenko Oleksandr <x0199363@ti.com>
> Peter, I'm not going to put this one through as a fix as the current interface
> is fine, if messy.  Given it's dependency on patch 1 of this series, I'll  wait
> for that to propogate through. Could you remind me to apply this if I forget!
Applied to the togreg branch of iio.git

Thanks,

>>> ---
>>>   drivers/iio/accel/bma180.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
>>> index bfec313..a7e68c8 100644
>>> --- a/drivers/iio/accel/bma180.c
>>> +++ b/drivers/iio/accel/bma180.c
>>> @@ -451,9 +451,9 @@ static const struct iio_chan_spec_ext_info bma180_ext_info[] = {
>>>       .type = IIO_ACCEL,                        \
>>>       .modified = 1,                            \
>>>       .channel2 = IIO_MOD_##_axis,                    \
>>> -    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |            \
>>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),            \
>>> +    .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |        \
>>>           BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),    \
>>> -    .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),        \
>>>       .scan_index = AXIS_##_axis,                    \
>>>       .scan_type = {                            \
>>>           .sign = 's',                        \
>>>
> --
> 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