linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iio: light: acpi-als: Report data as processed rather than raw
@ 2016-01-07 15:21 Gabriele Mazzotta
  2016-01-09 16:31 ` Jonathan Cameron
  2016-01-12 15:21 ` [PATCH v2] iio: light: acpi-als: Report data as processed Gabriele Mazzotta
  0 siblings, 2 replies; 12+ messages in thread
From: Gabriele Mazzotta @ 2016-01-07 15:21 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw
  Cc: marex, marxin.liska, linux-iio, linux-kernel, Gabriele Mazzotta

As per the ACPI specification (Revision 5.0) [1], the data coming
from the sensor represent the ambient light illuminance reading
expressed in lux. Use IIO_CHAN_INFO_PROCESSED instead of
IIO_CHAN_INFO_RAW to signify that the data are pre-processed.

[1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/iio/light/acpi-als.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index 60537ec..a53be07 100644
--- a/drivers/iio/light/acpi-als.c
+++ b/drivers/iio/light/acpi-als.c
@@ -54,7 +54,7 @@ static const struct iio_chan_spec acpi_als_channels[] = {
 			.realbits	= 32,
 			.storagebits	= 32,
 		},
-		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_separate	= BIT(IIO_CHAN_INFO_PROCESSED),
 	},
 };
 
@@ -152,7 +152,7 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
 	s32 temp_val;
 	int ret;
 
-	if (mask != IIO_CHAN_INFO_RAW)
+	if (mask != IIO_CHAN_INFO_PROCESSED)
 		return -EINVAL;
 
 	/* we support only illumination (_ALI) so far. */
-- 
2.7.0.rc3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: light: acpi-als: Report data as processed rather than raw
  2016-01-07 15:21 [PATCH] iio: light: acpi-als: Report data as processed rather than raw Gabriele Mazzotta
@ 2016-01-09 16:31 ` Jonathan Cameron
  2016-01-09 17:27   ` Marek Vasut
  2016-01-11 10:16   ` Crt Mori
  2016-01-12 15:21 ` [PATCH v2] iio: light: acpi-als: Report data as processed Gabriele Mazzotta
  1 sibling, 2 replies; 12+ messages in thread
From: Jonathan Cameron @ 2016-01-09 16:31 UTC (permalink / raw)
  To: Gabriele Mazzotta, knaack.h, lars, pmeerw
  Cc: marex, marxin.liska, linux-iio, linux-kernel

On 07/01/16 15:21, Gabriele Mazzotta wrote:
> As per the ACPI specification (Revision 5.0) [1], the data coming
> from the sensor represent the ambient light illuminance reading
> expressed in lux. Use IIO_CHAN_INFO_PROCESSED instead of
> IIO_CHAN_INFO_RAW to signify that the data are pre-processed.
> 
> [1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
> 
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Hm. Whilst it's a fix in a sense, the original didn't really 'break'
the ABI so I worry a little that this change may break others.
Irritating as it is, perhaps we should keep the _RAW and add _PROCESSED
(which will then be exactly the same value).
We'll also then need a comment in the code, that leaving the _RAW
elements was for ABI compatibility.

What do others think?

Jonathan
> ---
>  drivers/iio/light/acpi-als.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> index 60537ec..a53be07 100644
> --- a/drivers/iio/light/acpi-als.c
> +++ b/drivers/iio/light/acpi-als.c
> @@ -54,7 +54,7 @@ static const struct iio_chan_spec acpi_als_channels[] = {
>  			.realbits	= 32,
>  			.storagebits	= 32,
>  		},
> -		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_PROCESSED),
>  	},
>  };
>  
> @@ -152,7 +152,7 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
>  	s32 temp_val;
>  	int ret;
>  
> -	if (mask != IIO_CHAN_INFO_RAW)
> +	if (mask != IIO_CHAN_INFO_PROCESSED)
>  		return -EINVAL;
>  
>  	/* we support only illumination (_ALI) so far. */
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: light: acpi-als: Report data as processed rather than raw
  2016-01-09 16:31 ` Jonathan Cameron
@ 2016-01-09 17:27   ` Marek Vasut
  2016-01-11 19:18     ` Jonathan Cameron
  2016-01-11 10:16   ` Crt Mori
  1 sibling, 1 reply; 12+ messages in thread
From: Marek Vasut @ 2016-01-09 17:27 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Gabriele Mazzotta, knaack.h, lars, pmeerw, marxin.liska,
	linux-iio, linux-kernel

On Saturday, January 09, 2016 at 05:31:24 PM, Jonathan Cameron wrote:
> On 07/01/16 15:21, Gabriele Mazzotta wrote:
> > As per the ACPI specification (Revision 5.0) [1], the data coming
> > from the sensor represent the ambient light illuminance reading
> > expressed in lux. Use IIO_CHAN_INFO_PROCESSED instead of
> > IIO_CHAN_INFO_RAW to signify that the data are pre-processed.
> > 
> > [1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
> > 
> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> 
> Hm. Whilst it's a fix in a sense, the original didn't really 'break'
> the ABI so I worry a little that this change may break others.
> Irritating as it is, perhaps we should keep the _RAW and add _PROCESSED
> (which will then be exactly the same value).
> We'll also then need a comment in the code, that leaving the _RAW
> elements was for ABI compatibility.
> 
> What do others think?

I'm not an IIO guru, but this does sound sensible. Do you know if any userland
code which actually uses the ACPI ALS already ?

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: light: acpi-als: Report data as processed rather than raw
  2016-01-09 16:31 ` Jonathan Cameron
  2016-01-09 17:27   ` Marek Vasut
@ 2016-01-11 10:16   ` Crt Mori
  2016-01-11 19:15     ` Jonathan Cameron
  1 sibling, 1 reply; 12+ messages in thread
From: Crt Mori @ 2016-01-11 10:16 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Gabriele Mazzotta, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, marex, marxin.liska, linux-iio,
	linux-kernel

On 9 January 2016 at 17:31, Jonathan Cameron <jic23@kernel.org> wrote:
> On 07/01/16 15:21, Gabriele Mazzotta wrote:
>> As per the ACPI specification (Revision 5.0) [1], the data coming
>> from the sensor represent the ambient light illuminance reading
>> expressed in lux. Use IIO_CHAN_INFO_PROCESSED instead of
>> IIO_CHAN_INFO_RAW to signify that the data are pre-processed.
>>
>> [1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
>>
>> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Hm. Whilst it's a fix in a sense, the original didn't really 'break'
> the ABI so I worry a little that this change may break others.
> Irritating as it is, perhaps we should keep the _RAW and add _PROCESSED
> (which will then be exactly the same value).
> We'll also then need a comment in the code, that leaving the _RAW
> elements was for ABI compatibility.
>
> What do others think?
For this case I agree to keep both for backwards ABI compatibility,
but then other drivers returning the data in basic units, should be
changed from raw to processed? I do not think this was the case
before?

Crt

>
> Jonathan
>> ---
>>  drivers/iio/light/acpi-als.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
>> index 60537ec..a53be07 100644
>> --- a/drivers/iio/light/acpi-als.c
>> +++ b/drivers/iio/light/acpi-als.c
>> @@ -54,7 +54,7 @@ static const struct iio_chan_spec acpi_als_channels[] = {
>>                       .realbits       = 32,
>>                       .storagebits    = 32,
>>               },
>> -             .info_mask_separate     = BIT(IIO_CHAN_INFO_RAW),
>> +             .info_mask_separate     = BIT(IIO_CHAN_INFO_PROCESSED),
>>       },
>>  };
>>
>> @@ -152,7 +152,7 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
>>       s32 temp_val;
>>       int ret;
>>
>> -     if (mask != IIO_CHAN_INFO_RAW)
>> +     if (mask != IIO_CHAN_INFO_PROCESSED)
>>               return -EINVAL;
>>
>>       /* we support only illumination (_ALI) so far. */
>>
>
> --
> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: light: acpi-als: Report data as processed rather than raw
  2016-01-11 10:16   ` Crt Mori
@ 2016-01-11 19:15     ` Jonathan Cameron
  0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2016-01-11 19:15 UTC (permalink / raw)
  To: Crt Mori
  Cc: Gabriele Mazzotta, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, marex, marxin.liska, linux-iio,
	linux-kernel

On 11/01/16 10:16, Crt Mori wrote:
> On 9 January 2016 at 17:31, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 07/01/16 15:21, Gabriele Mazzotta wrote:
>>> As per the ACPI specification (Revision 5.0) [1], the data coming
>>> from the sensor represent the ambient light illuminance reading
>>> expressed in lux. Use IIO_CHAN_INFO_PROCESSED instead of
>>> IIO_CHAN_INFO_RAW to signify that the data are pre-processed.
>>>
>>> [1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
>>>
>>> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
>> Hm. Whilst it's a fix in a sense, the original didn't really 'break'
>> the ABI so I worry a little that this change may break others.
>> Irritating as it is, perhaps we should keep the _RAW and add _PROCESSED
>> (which will then be exactly the same value).
>> We'll also then need a comment in the code, that leaving the _RAW
>> elements was for ABI compatibility.
>>
>> What do others think?
> For this case I agree to keep both for backwards ABI compatibility,
> but then other drivers returning the data in basic units, should be
> changed from raw to processed? I do not think this was the case
> before?
It's always been the case that devices that happen to output in the
right units (or do so after a non linear transform as tends to happen
with a lot of light sensors) should mark themselves as processed
to indicate that nothing else needs be done to convert to standard
units.

Sensor hubs in particular often do the unit conversion stuff on
a separate processor, though often their chosen 'base' units differ
from ours (not always though!)

Jonathan
> 
> Crt
> 
>>
>> Jonathan
>>> ---
>>>  drivers/iio/light/acpi-als.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
>>> index 60537ec..a53be07 100644
>>> --- a/drivers/iio/light/acpi-als.c
>>> +++ b/drivers/iio/light/acpi-als.c
>>> @@ -54,7 +54,7 @@ static const struct iio_chan_spec acpi_als_channels[] = {
>>>                       .realbits       = 32,
>>>                       .storagebits    = 32,
>>>               },
>>> -             .info_mask_separate     = BIT(IIO_CHAN_INFO_RAW),
>>> +             .info_mask_separate     = BIT(IIO_CHAN_INFO_PROCESSED),
>>>       },
>>>  };
>>>
>>> @@ -152,7 +152,7 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
>>>       s32 temp_val;
>>>       int ret;
>>>
>>> -     if (mask != IIO_CHAN_INFO_RAW)
>>> +     if (mask != IIO_CHAN_INFO_PROCESSED)
>>>               return -EINVAL;
>>>
>>>       /* we support only illumination (_ALI) so far. */
>>>
>>
>> --
>> 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
> --
> 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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: light: acpi-als: Report data as processed rather than raw
  2016-01-09 17:27   ` Marek Vasut
@ 2016-01-11 19:18     ` Jonathan Cameron
  2016-01-11 19:20       ` Marek Vasut
                         ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Jonathan Cameron @ 2016-01-11 19:18 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Gabriele Mazzotta, knaack.h, lars, pmeerw, marxin.liska,
	linux-iio, linux-kernel

On 09/01/16 17:27, Marek Vasut wrote:
> On Saturday, January 09, 2016 at 05:31:24 PM, Jonathan Cameron wrote:
>> On 07/01/16 15:21, Gabriele Mazzotta wrote:
>>> As per the ACPI specification (Revision 5.0) [1], the data coming
>>> from the sensor represent the ambient light illuminance reading
>>> expressed in lux. Use IIO_CHAN_INFO_PROCESSED instead of
>>> IIO_CHAN_INFO_RAW to signify that the data are pre-processed.
>>>
>>> [1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
>>>
>>> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
>>
>> Hm. Whilst it's a fix in a sense, the original didn't really 'break'
>> the ABI so I worry a little that this change may break others.
>> Irritating as it is, perhaps we should keep the _RAW and add _PROCESSED
>> (which will then be exactly the same value).
>> We'll also then need a comment in the code, that leaving the _RAW
>> elements was for ABI compatibility.
>>
>> What do others think?
> 
> I'm not an IIO guru, but this does sound sensible. Do you know if any userland
> code which actually uses the ACPI ALS already ?
It's more than likely as Gnome at least supports using them to control screen
brightness.  Hopefully that code is able to cope with the correct ABI though as
well as the old one.  Anyhow, we seem to have a reasonable consensus. 
Gabriele, are you happy to do a version of the patch with the _RAW version
left along side your _PROCESSED version and a comment saying that it is for
compatibility only?

Jonathan
> 
> Best regards,
> Marek Vasut
> --
> 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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: light: acpi-als: Report data as processed rather than raw
  2016-01-11 19:18     ` Jonathan Cameron
@ 2016-01-11 19:20       ` Marek Vasut
  2016-01-12  0:15       ` Gabriele Mazzotta
  2016-01-12 15:27       ` Gabriele Mazzotta
  2 siblings, 0 replies; 12+ messages in thread
From: Marek Vasut @ 2016-01-11 19:20 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Gabriele Mazzotta, knaack.h, lars, pmeerw, marxin.liska,
	linux-iio, linux-kernel

On Monday, January 11, 2016 at 08:18:27 PM, Jonathan Cameron wrote:
> On 09/01/16 17:27, Marek Vasut wrote:
> > On Saturday, January 09, 2016 at 05:31:24 PM, Jonathan Cameron wrote:
> >> On 07/01/16 15:21, Gabriele Mazzotta wrote:
> >>> As per the ACPI specification (Revision 5.0) [1], the data coming
> >>> from the sensor represent the ambient light illuminance reading
> >>> expressed in lux. Use IIO_CHAN_INFO_PROCESSED instead of
> >>> IIO_CHAN_INFO_RAW to signify that the data are pre-processed.
> >>> 
> >>> [1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
> >>> 
> >>> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> >> 
> >> Hm. Whilst it's a fix in a sense, the original didn't really 'break'
> >> the ABI so I worry a little that this change may break others.
> >> Irritating as it is, perhaps we should keep the _RAW and add _PROCESSED
> >> (which will then be exactly the same value).
> >> We'll also then need a comment in the code, that leaving the _RAW
> >> elements was for ABI compatibility.
> >> 
> >> What do others think?
> > 
> > I'm not an IIO guru, but this does sound sensible. Do you know if any
> > userland code which actually uses the ACPI ALS already ?
> 
> It's more than likely as Gnome at least supports using them to control
> screen brightness.  Hopefully that code is able to cope with the correct
> ABI though as well as the old one.  Anyhow, we seem to have a reasonable
> consensus. Gabriele, are you happy to do a version of the patch with the
> _RAW version left along side your _PROCESSED version and a comment saying
> that it is for compatibility only?

This makes sense in my mind, thanks :)

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: light: acpi-als: Report data as processed rather than raw
  2016-01-11 19:18     ` Jonathan Cameron
  2016-01-11 19:20       ` Marek Vasut
@ 2016-01-12  0:15       ` Gabriele Mazzotta
  2016-01-12 15:27       ` Gabriele Mazzotta
  2 siblings, 0 replies; 12+ messages in thread
From: Gabriele Mazzotta @ 2016-01-12  0:15 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Marek Vasut, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Martin Liška, linux-iio,
	linux-kernel

2016-01-11 20:18 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
> On 09/01/16 17:27, Marek Vasut wrote:
>> On Saturday, January 09, 2016 at 05:31:24 PM, Jonathan Cameron wrote:
>>> On 07/01/16 15:21, Gabriele Mazzotta wrote:
>>>> As per the ACPI specification (Revision 5.0) [1], the data coming
>>>> from the sensor represent the ambient light illuminance reading
>>>> expressed in lux. Use IIO_CHAN_INFO_PROCESSED instead of
>>>> IIO_CHAN_INFO_RAW to signify that the data are pre-processed.
>>>>
>>>> [1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
>>>>
>>>> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
>>>
>>> Hm. Whilst it's a fix in a sense, the original didn't really 'break'
>>> the ABI so I worry a little that this change may break others.
>>> Irritating as it is, perhaps we should keep the _RAW and add _PROCESSED
>>> (which will then be exactly the same value).
>>> We'll also then need a comment in the code, that leaving the _RAW
>>> elements was for ABI compatibility.
>>>
>>> What do others think?
>>
>> I'm not an IIO guru, but this does sound sensible. Do you know if any userland
>> code which actually uses the ACPI ALS already ?
> It's more than likely as Gnome at least supports using them to control screen
> brightness.  Hopefully that code is able to cope with the correct ABI though as
> well as the old one.  Anyhow, we seem to have a reasonable consensus.
> Gabriele, are you happy to do a version of the patch with the _RAW version
> left along side your _PROCESSED version and a comment saying that it is for
> compatibility only?

I'm definitely OK with this and I'll prepare a patch as soon as I can.

> Jonathan
>>
>> Best regards,
>> Marek Vasut
>> --
>> 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
>>
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] iio: light: acpi-als: Report data as processed
  2016-01-07 15:21 [PATCH] iio: light: acpi-als: Report data as processed rather than raw Gabriele Mazzotta
  2016-01-09 16:31 ` Jonathan Cameron
@ 2016-01-12 15:21 ` Gabriele Mazzotta
  2016-01-16 13:02   ` Jonathan Cameron
  1 sibling, 1 reply; 12+ messages in thread
From: Gabriele Mazzotta @ 2016-01-12 15:21 UTC (permalink / raw)
  To: jic23
  Cc: knaack.h, lars, pmeerw, marex, marxin.liska, linux-iio,
	linux-kernel, Gabriele Mazzotta

As per the ACPI specification (Revision 5.0) [1], the data coming
from the sensor represent the ambient light illuminance reading
expressed in lux. So use IIO_CHAN_INFO_PROCESSED to signify that
the data are pre-processed.

However, to keep backward ABI compatibility, the IIO_CHAN_INFO_RAW
bit is not removed.

[1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/iio/light/acpi-als.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index 60537ec..53201d9 100644
--- a/drivers/iio/light/acpi-als.c
+++ b/drivers/iio/light/acpi-als.c
@@ -54,7 +54,9 @@ static const struct iio_chan_spec acpi_als_channels[] = {
 			.realbits	= 32,
 			.storagebits	= 32,
 		},
-		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW),
+		/* _RAW is here for backward ABI compatibility */
+		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+					  BIT(IIO_CHAN_INFO_PROCESSED),
 	},
 };
 
@@ -152,7 +154,7 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
 	s32 temp_val;
 	int ret;
 
-	if (mask != IIO_CHAN_INFO_RAW)
+	if ((mask != IIO_CHAN_INFO_PROCESSED) && (mask != IIO_CHAN_INFO_RAW))
 		return -EINVAL;
 
 	/* we support only illumination (_ALI) so far. */
-- 
2.7.0.rc3

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: light: acpi-als: Report data as processed rather than raw
  2016-01-11 19:18     ` Jonathan Cameron
  2016-01-11 19:20       ` Marek Vasut
  2016-01-12  0:15       ` Gabriele Mazzotta
@ 2016-01-12 15:27       ` Gabriele Mazzotta
  2016-01-16 12:57         ` Jonathan Cameron
  2 siblings, 1 reply; 12+ messages in thread
From: Gabriele Mazzotta @ 2016-01-12 15:27 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Marek Vasut, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Martin Liška, linux-iio,
	linux-kernel

2016-01-11 20:18 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
> On 09/01/16 17:27, Marek Vasut wrote:
>> On Saturday, January 09, 2016 at 05:31:24 PM, Jonathan Cameron wrote:
>>> On 07/01/16 15:21, Gabriele Mazzotta wrote:
>>>> As per the ACPI specification (Revision 5.0) [1], the data coming
>>>> from the sensor represent the ambient light illuminance reading
>>>> expressed in lux. Use IIO_CHAN_INFO_PROCESSED instead of
>>>> IIO_CHAN_INFO_RAW to signify that the data are pre-processed.
>>>>
>>>> [1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
>>>>
>>>> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
>>>
>>> Hm. Whilst it's a fix in a sense, the original didn't really 'break'
>>> the ABI so I worry a little that this change may break others.
>>> Irritating as it is, perhaps we should keep the _RAW and add _PROCESSED
>>> (which will then be exactly the same value).
>>> We'll also then need a comment in the code, that leaving the _RAW
>>> elements was for ABI compatibility.
>>>
>>> What do others think?
>>
>> I'm not an IIO guru, but this does sound sensible. Do you know if any userland
>> code which actually uses the ACPI ALS already ?
> It's more than likely as Gnome at least supports using them to control screen
> brightness.  Hopefully that code is able to cope with the correct ABI though as
> well as the old one.  Anyhow, we seem to have a reasonable consensus.

I looked into this and Gnome uses iio-proxy-sensor to handle iio
devices. As of now it, only looks for in_illuminance_input, so it
currently doesn't work with acpi-als. I've also found a bug report [1]
of an acpi-als user stating the same.

[1] https://github.com/hadess/iio-sensor-proxy/issues/46

> Gabriele, are you happy to do a version of the patch with the _RAW version
> left along side your _PROCESSED version and a comment saying that it is for
> compatibility only?
>
> Jonathan
>>
>> Best regards,
>> Marek Vasut
>> --
>> 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
>>
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: light: acpi-als: Report data as processed rather than raw
  2016-01-12 15:27       ` Gabriele Mazzotta
@ 2016-01-16 12:57         ` Jonathan Cameron
  0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2016-01-16 12:57 UTC (permalink / raw)
  To: Gabriele Mazzotta
  Cc: Marek Vasut, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Martin Liška, linux-iio,
	linux-kernel

On 12/01/16 15:27, Gabriele Mazzotta wrote:
> 2016-01-11 20:18 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
>> On 09/01/16 17:27, Marek Vasut wrote:
>>> On Saturday, January 09, 2016 at 05:31:24 PM, Jonathan Cameron wrote:
>>>> On 07/01/16 15:21, Gabriele Mazzotta wrote:
>>>>> As per the ACPI specification (Revision 5.0) [1], the data coming
>>>>> from the sensor represent the ambient light illuminance reading
>>>>> expressed in lux. Use IIO_CHAN_INFO_PROCESSED instead of
>>>>> IIO_CHAN_INFO_RAW to signify that the data are pre-processed.
>>>>>
>>>>> [1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
>>>>>
>>>>> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
>>>>
>>>> Hm. Whilst it's a fix in a sense, the original didn't really 'break'
>>>> the ABI so I worry a little that this change may break others.
>>>> Irritating as it is, perhaps we should keep the _RAW and add _PROCESSED
>>>> (which will then be exactly the same value).
>>>> We'll also then need a comment in the code, that leaving the _RAW
>>>> elements was for ABI compatibility.
>>>>
>>>> What do others think?
>>>
>>> I'm not an IIO guru, but this does sound sensible. Do you know if any userland
>>> code which actually uses the ACPI ALS already ?
>> It's more than likely as Gnome at least supports using them to control screen
>> brightness.  Hopefully that code is able to cope with the correct ABI though as
>> well as the old one.  Anyhow, we seem to have a reasonable consensus.
> 
> I looked into this and Gnome uses iio-proxy-sensor to handle iio
> devices. As of now it, only looks for in_illuminance_input, so it
> currently doesn't work with acpi-als. I've also found a bug report [1]
> of an acpi-als user stating the same.
> 
> [1] https://github.com/hadess/iio-sensor-proxy/issues/46
Good fine. I'll apply this as a fix in that case and mark it for stable.

Jonathan
> 
>> Gabriele, are you happy to do a version of the patch with the _RAW version
>> left along side your _PROCESSED version and a comment saying that it is for
>> compatibility only?
>>
>> Jonathan
>>>
>>> Best regards,
>>> Marek Vasut
>>> --
>>> 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
>>>
>>
> --
> 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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] iio: light: acpi-als: Report data as processed
  2016-01-12 15:21 ` [PATCH v2] iio: light: acpi-als: Report data as processed Gabriele Mazzotta
@ 2016-01-16 13:02   ` Jonathan Cameron
  0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2016-01-16 13:02 UTC (permalink / raw)
  To: Gabriele Mazzotta
  Cc: knaack.h, lars, pmeerw, marex, marxin.liska, linux-iio, linux-kernel

On 12/01/16 15:21, Gabriele Mazzotta wrote:
> As per the ACPI specification (Revision 5.0) [1], the data coming
> from the sensor represent the ambient light illuminance reading
> expressed in lux. So use IIO_CHAN_INFO_PROCESSED to signify that
> the data are pre-processed.
> 
> However, to keep backward ABI compatibility, the IIO_CHAN_INFO_RAW
> bit is not removed.
> 
> [1] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
> 
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Applied to the fixes-togreg branch and marked for stable.

I added a bit to the commit message including that bug report you found for
iio-sensor-proxy as additional justification for the stable marking
and sending it as a fix.

Jonathan

> ---
>  drivers/iio/light/acpi-als.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> index 60537ec..53201d9 100644
> --- a/drivers/iio/light/acpi-als.c
> +++ b/drivers/iio/light/acpi-als.c
> @@ -54,7 +54,9 @@ static const struct iio_chan_spec acpi_als_channels[] = {
>  			.realbits	= 32,
>  			.storagebits	= 32,
>  		},
> -		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW),
> +		/* _RAW is here for backward ABI compatibility */
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +					  BIT(IIO_CHAN_INFO_PROCESSED),
>  	},
>  };
>  
> @@ -152,7 +154,7 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
>  	s32 temp_val;
>  	int ret;
>  
> -	if (mask != IIO_CHAN_INFO_RAW)
> +	if ((mask != IIO_CHAN_INFO_PROCESSED) && (mask != IIO_CHAN_INFO_RAW))
>  		return -EINVAL;
>  
>  	/* we support only illumination (_ALI) so far. */
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-01-16 13:02 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-07 15:21 [PATCH] iio: light: acpi-als: Report data as processed rather than raw Gabriele Mazzotta
2016-01-09 16:31 ` Jonathan Cameron
2016-01-09 17:27   ` Marek Vasut
2016-01-11 19:18     ` Jonathan Cameron
2016-01-11 19:20       ` Marek Vasut
2016-01-12  0:15       ` Gabriele Mazzotta
2016-01-12 15:27       ` Gabriele Mazzotta
2016-01-16 12:57         ` Jonathan Cameron
2016-01-11 10:16   ` Crt Mori
2016-01-11 19:15     ` Jonathan Cameron
2016-01-12 15:21 ` [PATCH v2] iio: light: acpi-als: Report data as processed Gabriele Mazzotta
2016-01-16 13:02   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).