* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-29 11:27 [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor Gabriele Mazzotta
@ 2015-04-29 11:51 ` Gabriele Mazzotta
2015-04-29 14:33 ` Marek Vasut
2015-04-30 12:24 ` Daniel Baluta
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Gabriele Mazzotta @ 2015-04-29 11:51 UTC (permalink / raw)
To: jic23
Cc: knaack.h, lars, pmeerw, marxin.liska, marex, rui.zhang,
linux-kernel, linux-iio
On Wednesday 29 April 2015 13:27:25 Gabriele Mazzotta wrote:
> Add basic implementation of the ACPI0008 Ambient Light Sensor driver.
> This driver currently supports only the ALI property, yet is ready to
> be easily extended to handle ALC, ALT, ALP ones as well.
>
> Signed-off-by: Martin Liska <marxin.liska@gmail.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> ---
> This continues http://marc.info/?t=140163463200002
> I've made few adjustments over the original patch:
>
> - Code aligned with 4.1-rc1 and cleaned up
> - Use signed integers to store values: sensors report 32bit signed
> values. In particular, -1 is reported when the current reading
> is above the supported range of sensitivity.
>
> Most of the changes are just a consequence of the changes in the
> iio subsystem.
>
> Gabriele
I'm sorry, I've just noticed that I haven't changed the value of
realbits in acpi_als_channels. This makes me wonder what would be the
proper value, given that this is a generic driver and all the
information I have are those in the ACPI specification (which states
what I reported here above).
Should I just set realbits to 32?
Thanks,
Gabriele
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-29 11:51 ` Gabriele Mazzotta
@ 2015-04-29 14:33 ` Marek Vasut
2015-04-29 15:36 ` Gabriele Mazzotta
0 siblings, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2015-04-29 14:33 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: jic23, knaack.h, lars, pmeerw, marxin.liska, rui.zhang,
linux-kernel, linux-iio
On Wednesday, April 29, 2015 at 01:51:21 PM, Gabriele Mazzotta wrote:
> On Wednesday 29 April 2015 13:27:25 Gabriele Mazzotta wrote:
> > Add basic implementation of the ACPI0008 Ambient Light Sensor driver.
> > This driver currently supports only the ALI property, yet is ready to
> > be easily extended to handle ALC, ALT, ALP ones as well.
> >
> > Signed-off-by: Martin Liska <marxin.liska@gmail.com>
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > ---
> > This continues http://marc.info/?t=140163463200002
> >
> > I've made few adjustments over the original patch:
> > - Code aligned with 4.1-rc1 and cleaned up
> > - Use signed integers to store values: sensors report 32bit signed
> >
> > values. In particular, -1 is reported when the current reading
> > is above the supported range of sensitivity.
> >
> > Most of the changes are just a consequence of the changes in the
> > iio subsystem.
> >
> > Gabriele
>
> I'm sorry, I've just noticed that I haven't changed the value of
> realbits in acpi_als_channels. This makes me wonder what would be the
> proper value, given that this is a generic driver and all the
> information I have are those in the ACPI specification (which states
> what I reported here above).
>
> Should I just set realbits to 32?
I believe the ALS reports only 16bit signel value, no ?
My observation with a strong coherent light source is that
the saturated sensor reported 0xffff .
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-29 14:33 ` Marek Vasut
@ 2015-04-29 15:36 ` Gabriele Mazzotta
2015-04-30 9:44 ` Marek Vasut
0 siblings, 1 reply; 14+ messages in thread
From: Gabriele Mazzotta @ 2015-04-29 15:36 UTC (permalink / raw)
To: Marek Vasut
Cc: jic23, knaack.h, lars, pmeerw, marxin.liska, rui.zhang,
linux-kernel, linux-iio
On Wednesday 29 April 2015 16:33:18 Marek Vasut wrote:
> On Wednesday, April 29, 2015 at 01:51:21 PM, Gabriele Mazzotta wrote:
> > On Wednesday 29 April 2015 13:27:25 Gabriele Mazzotta wrote:
> > > Add basic implementation of the ACPI0008 Ambient Light Sensor driver.
> > > This driver currently supports only the ALI property, yet is ready to
> > > be easily extended to handle ALC, ALT, ALP ones as well.
> > >
> > > Signed-off-by: Martin Liska <marxin.liska@gmail.com>
> > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > > This continues http://marc.info/?t=140163463200002
> > >
> > > I've made few adjustments over the original patch:
> > > - Code aligned with 4.1-rc1 and cleaned up
> > > - Use signed integers to store values: sensors report 32bit signed
> > >
> > > values. In particular, -1 is reported when the current reading
> > > is above the supported range of sensitivity.
> > >
> > > Most of the changes are just a consequence of the changes in the
> > > iio subsystem.
> > >
> > > Gabriele
> >
> > I'm sorry, I've just noticed that I haven't changed the value of
> > realbits in acpi_als_channels. This makes me wonder what would be the
> > proper value, given that this is a generic driver and all the
> > information I have are those in the ACPI specification (which states
> > what I reported here above).
> >
> > Should I just set realbits to 32?
>
> I believe the ALS reports only 16bit signel value, no ?
> My observation with a strong coherent light source is that
> the saturated sensor reported 0xffff .
Probably it's the same for me. I couldn't get to the point where
ALI reports 0xffff, just really close, I will have to try with some
stronger lights. However, looking at my ACPI table, I can see that
the value returned by _ALI is just the composition of two 8 bits
variables put side by side, so yes, I can say that even on my system
it's a 16bit value.
The problem here is that I'm not sure we can assume this as true in
general since the ACPI specification doesn't say anything.
Gabriele
> Best regards,
> Marek Vasut
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-29 15:36 ` Gabriele Mazzotta
@ 2015-04-30 9:44 ` Marek Vasut
2015-04-30 11:27 ` Gabriele Mazzotta
0 siblings, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2015-04-30 9:44 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: jic23, knaack.h, lars, pmeerw, marxin.liska, rui.zhang,
linux-kernel, linux-iio
On Wednesday, April 29, 2015 at 05:36:32 PM, Gabriele Mazzotta wrote:
[...]
> > > I'm sorry, I've just noticed that I haven't changed the value of
> > > realbits in acpi_als_channels. This makes me wonder what would be the
> > > proper value, given that this is a generic driver and all the
> > > information I have are those in the ACPI specification (which states
> > > what I reported here above).
> > >
> > > Should I just set realbits to 32?
> >
> > I believe the ALS reports only 16bit signel value, no ?
> > My observation with a strong coherent light source is that
> > the saturated sensor reported 0xffff .
>
> Probably it's the same for me. I couldn't get to the point where
> ALI reports 0xffff, just really close, I will have to try with some
> stronger lights. However, looking at my ACPI table, I can see that
> the value returned by _ALI is just the composition of two 8 bits
> variables put side by side, so yes, I can say that even on my system
> it's a 16bit value.
What kind of hardware are you testing this on ?
> The problem here is that I'm not sure we can assume this as true in
> general since the ACPI specification doesn't say anything.
Maybe someone more knowledgable can speak up.
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-30 9:44 ` Marek Vasut
@ 2015-04-30 11:27 ` Gabriele Mazzotta
2015-04-30 11:30 ` Marek Vasut
0 siblings, 1 reply; 14+ messages in thread
From: Gabriele Mazzotta @ 2015-04-30 11:27 UTC (permalink / raw)
To: Marek Vasut
Cc: jic23, knaack.h, lars, pmeerw, marxin.liska, rui.zhang,
linux-kernel, linux-iio
On Thursday 30 April 2015 11:44:36 Marek Vasut wrote:
> On Wednesday, April 29, 2015 at 05:36:32 PM, Gabriele Mazzotta wrote:
> [...]
> > > > I'm sorry, I've just noticed that I haven't changed the value of
> > > > realbits in acpi_als_channels. This makes me wonder what would be the
> > > > proper value, given that this is a generic driver and all the
> > > > information I have are those in the ACPI specification (which states
> > > > what I reported here above).
> > > >
> > > > Should I just set realbits to 32?
> > >
> > > I believe the ALS reports only 16bit signel value, no ?
> > > My observation with a strong coherent light source is that
> > > the saturated sensor reported 0xffff .
> >
> > Probably it's the same for me. I couldn't get to the point where
> > ALI reports 0xffff, just really close, I will have to try with some
> > stronger lights. However, looking at my ACPI table, I can see that
> > the value returned by _ALI is just the composition of two 8 bits
> > variables put side by side, so yes, I can say that even on my system
> > it's a 16bit value.
>
> What kind of hardware are you testing this on ?
Dell XPS13 9333.
> > The problem here is that I'm not sure we can assume this as true in
> > general since the ACPI specification doesn't say anything.
>
> Maybe someone more knowledgable can speak up.
I'm looking at some documentation from Microsoft [1] on how to use these
sensors in Windows and values as high as 100,000 lux are mentioned.
[1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn613948%28v=vs.85%29.aspx
> Best regards,
> Marek Vasut
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-30 11:27 ` Gabriele Mazzotta
@ 2015-04-30 11:30 ` Marek Vasut
0 siblings, 0 replies; 14+ messages in thread
From: Marek Vasut @ 2015-04-30 11:30 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: jic23, knaack.h, lars, pmeerw, marxin.liska, rui.zhang,
linux-kernel, linux-iio
On Thursday, April 30, 2015 at 01:27:43 PM, Gabriele Mazzotta wrote:
> On Thursday 30 April 2015 11:44:36 Marek Vasut wrote:
> > On Wednesday, April 29, 2015 at 05:36:32 PM, Gabriele Mazzotta wrote:
> > [...]
> >
> > > > > I'm sorry, I've just noticed that I haven't changed the value of
> > > > > realbits in acpi_als_channels. This makes me wonder what would be
> > > > > the proper value, given that this is a generic driver and all the
> > > > > information I have are those in the ACPI specification (which
> > > > > states what I reported here above).
> > > > >
> > > > > Should I just set realbits to 32?
> > > >
> > > > I believe the ALS reports only 16bit signel value, no ?
> > > > My observation with a strong coherent light source is that
> > > > the saturated sensor reported 0xffff .
> > >
> > > Probably it's the same for me. I couldn't get to the point where
> > > ALI reports 0xffff, just really close, I will have to try with some
> > > stronger lights. However, looking at my ACPI table, I can see that
> > > the value returned by _ALI is just the composition of two 8 bits
> > > variables put side by side, so yes, I can say that even on my system
> > > it's a 16bit value.
> >
> > What kind of hardware are you testing this on ?
Hi!
> Dell XPS13 9333.
Cool, even better :)
I tested it on the following hardware:
Acer W500
Retina MacBook Pro 10,1
MacBook Air (I don't know the revision, sorry)
> > > The problem here is that I'm not sure we can assume this as true in
> > > general since the ACPI specification doesn't say anything.
> >
> > Maybe someone more knowledgable can speak up.
>
> I'm looking at some documentation from Microsoft [1] on how to use these
> sensors in Windows and values as high as 100,000 lux are mentioned.
I see, thank you for investigating !
> [1]
> https://msdn.microsoft.com/en-us/library/windows/hardware/dn613948%28v=vs.
> 85%29.aspx
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-29 11:27 [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor Gabriele Mazzotta
2015-04-29 11:51 ` Gabriele Mazzotta
@ 2015-04-30 12:24 ` Daniel Baluta
2015-04-30 19:14 ` Gabriele Mazzotta
2015-04-30 20:33 ` Paul Bolle
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Daniel Baluta @ 2015-04-30 12:24 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, marxin.liska, marex, rui.zhang,
Linux Kernel Mailing List, linux-iio@vger.kernel.org
Hi Gabriele,
Please keep the version numbering from previous patches,
I think this should be v8 :).
On Wed, Apr 29, 2015 at 2:27 PM, Gabriele Mazzotta
<gabriele.mzt@gmail.com> wrote:
> Add basic implementation of the ACPI0008 Ambient Light Sensor driver.
> This driver currently supports only the ALI property, yet is ready to
> be easily extended to handle ALC, ALT, ALP ones as well.
I would like this commit message to contain more info about ACPI0008 ALS, like
this commit.
https://lwn.net/Articles/345648/
Also, would be nice to point readers to Chapter 9.2 from ACPI spec:
http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
>
> Signed-off-by: Martin Liska <marxin.liska@gmail.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> ---
> This continues http://marc.info/?t=140163463200002
> I've made few adjustments over the original patch:
>
> - Code aligned with 4.1-rc1 and cleaned up
> - Use signed integers to store values: sensors report 32bit signed
> values. In particular, -1 is reported when the current reading
> is above the supported range of sensitivity.
>
> Most of the changes are just a consequence of the changes in the
> iio subsystem.
>
> Gabriele
>
> drivers/iio/light/Kconfig | 12 +++
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/acpi-als.c | 240 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 253 insertions(+)
> create mode 100644 drivers/iio/light/acpi-als.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 01a1a16..898b2b5 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -5,6 +5,18 @@
>
> menu "Light sensors"
>
> +config ACPI_ALS
> + tristate "ACPI Ambient Light Sensor"
> + depends on ACPI
> + select IIO_TRIGGERED_BUFFER
> + select IIO_KFIFO_BUF
> + help
> + Say Y here if you want to build a driver for the ACPI0008
> + Ambient Light Sensor.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called acpi-als.
> +
> config ADJD_S311
> tristate "ADJD-S311-CR999 digital color sensor"
> select IIO_BUFFER
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index ad7c30f..d9aad52a 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -3,6 +3,7 @@
> #
>
> # When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_ACPI_ALS) += acpi-als.o
> obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> new file mode 100644
> index 0000000..fe9a8a0
> --- /dev/null
> +++ b/drivers/iio/light/acpi-als.c
> @@ -0,0 +1,240 @@
> +/*
> + * ACPI Ambient Light Sensor Driver
> + *
> + * Based on ALS driver:
> + * Copyright (C) 2009 Zhang Rui <rui.zhang@intel.com>
> + *
> + * Rework for IIO subsystem:
> + * Copyright (C) 2012-2013 Martin Liska <marxin.liska@gmail.com>
> + *
> + * Final cleanup and debugging:
> + * Copyright (C) 2013-2014 Marek Vasut <marex@denx.de>
> + * Copyright (C) 2015 Gabriele Mazzotta <gabriele.mzt@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +
> +#define ACPI_ALS_CLASS "als"
> +#define ACPI_ALS_DEVICE_NAME "acpi-als"
> +#define ACPI_ALS_NOTIFY_ILLUMINANCE 0x80
> +
> +ACPI_MODULE_NAME("acpi-als");
> +
> +/*
> + * So far, there's only one channel in here, but the specification for
> + * ACPI0008 says there can be more to what the block can report. Like
> + * chromaticity and such. We are ready for incoming additions!
> + */
> +static const struct iio_chan_spec acpi_als_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 10,
> + .storagebits = 32,
> + },
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + },
> +};
> +
> +/*
> + * The event buffer contains timestamp and all the data from
> + * the ACPI0008 block. There are multiple, but so far we only
> + * support _ALI (illuminance). Once someone adds new channels
> + * to acpi_als_channels[], the evt_buffer below will grow
> + * automatically.
> + */
> +#define EVT_NR_SOURCES ARRAY_SIZE(acpi_als_channels)
> +#define EVT_BUFFER_SIZE \
> + (sizeof(s64) + (EVT_NR_SOURCES * sizeof(s32)))
> +
> +struct acpi_als {
> + struct acpi_device *device;
> + struct mutex lock;
> +
> + s32 evt_buffer[EVT_BUFFER_SIZE];
> +};
> +
> +/*
> + * All types of properties the ACPI0008 block can report. The ALI, ALC, ALT
> + * and ALP can all be handled by als_read_value() below, while the ALR is
> + * special.
> + *
> + * The _ALR property returns tables that can be used to fine-tune the values
> + * reported by the other props based on the particular hardware type and it's
> + * location (it contains tables for "rainy", "bright inhouse lighting" etc.).
> + *
> + * So far, we support only ALI (illuminance).
> + */
> +#define ACPI_ALS_ILLUMINANCE "_ALI"
> +#define ACPI_ALS_CHROMATICITY "_ALC"
> +#define ACPI_ALS_COLOR_TEMP "_ALT"
> +#define ACPI_ALS_POLLING "_ALP"
> +#define ACPI_ALS_TABLES "_ALR"
> +
> +static int als_read_value(struct acpi_als *als, char *prop, s32 *val)
> +{
> + unsigned long long temp_val;
> + acpi_status status;
> +
> + status = acpi_evaluate_integer(als->device->handle, prop, NULL,
> + &temp_val);
> +
> + if (ACPI_FAILURE(status)) {
> + ACPI_EXCEPTION((AE_INFO, status, "Error reading ALS %s", prop));
> + return -EIO;
> + }
> +
> + *val = temp_val;
> +
> + return 0;
> +}
> +
> +static void acpi_als_notify(struct acpi_device *device, u32 event)
> +{
> + struct iio_dev *indio_dev = acpi_driver_data(device);
> + struct acpi_als *als = iio_priv(indio_dev);
> + s32 *buffer = als->evt_buffer;
> + s64 time_ns = iio_get_time_ns();
> + s32 val;
> + int ret;
> +
> + mutex_lock(&als->lock);
> +
> + memset(buffer, 0, EVT_BUFFER_SIZE);
> +
> + switch (event) {
> + case ACPI_ALS_NOTIFY_ILLUMINANCE:
> + ret = als_read_value(als, ACPI_ALS_ILLUMINANCE, &val);
> + if (ret < 0)
> + goto out;
> + *buffer++ = val;
> + break;
> + default:
> + /* Unhandled event */
> + dev_dbg(&device->dev, "Unhandled ACPI ALS event (%08x)!\n",
> + event);
> + goto out;
> + }
> +
> + iio_push_to_buffers_with_timestamp(indio_dev,
> + (u8 *)als->evt_buffer, time_ns);
> +
> +out:
> + mutex_unlock(&als->lock);
> +}
> +
> +static int acpi_als_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct acpi_als *als = iio_priv(indio_dev);
> + s32 temp_val;
> + int ret;
> +
> + if (mask != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + /* we support only illumination (_ALI) so far. */
> + if (chan->type != IIO_LIGHT)
> + return -EINVAL;
> +
> + ret = als_read_value(als, ACPI_ALS_ILLUMINANCE, &temp_val);
> + if (ret < 0)
> + return ret;
> +
> + *val = temp_val;
> +
> + return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info acpi_als_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = acpi_als_read_raw,
> +};
> +
> +static int acpi_als_add(struct acpi_device *device)
> +{
> + struct acpi_als *als;
> + struct iio_dev *indio_dev;
> + struct iio_buffer *buffer;
> +
> + indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + als = iio_priv(indio_dev);
> +
> + device->driver_data = indio_dev;
> + als->device = device;
> + mutex_init(&als->lock);
> +
> + indio_dev->name = ACPI_ALS_DEVICE_NAME;
> + indio_dev->dev.parent = &device->dev;
> + indio_dev->info = &acpi_als_info;
> + indio_dev->modes = INDIO_BUFFER_SOFTWARE;
> + indio_dev->channels = acpi_als_channels;
> + indio_dev->num_channels = ARRAY_SIZE(acpi_als_channels);
> +
> + buffer = devm_iio_kfifo_allocate(&device->dev);
> + if (!buffer)
> + return -ENOMEM;
> +
> + iio_device_attach_buffer(indio_dev, buffer);
> +
> + return iio_device_register(indio_dev);
> +}
> +
> +static int acpi_als_remove(struct acpi_device *device)
> +{
> + struct iio_dev *indio_dev = acpi_driver_data(device);
> +
> + iio_device_unregister(indio_dev);
> +
> + return 0;
> +}
> +
> +static const struct acpi_device_id acpi_als_device_ids[] = {
> + {"ACPI0008", 0},
> + {"", 0},
Use empty struct here {}.
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, acpi_als_device_ids);
> +
> +static struct acpi_driver acpi_als_driver = {
> + .name = "acpi_als",
> + .class = ACPI_ALS_CLASS,
> + .ids = acpi_als_device_ids,
> + .ops = {
> + .add = acpi_als_add,
> + .remove = acpi_als_remove,
> + .notify = acpi_als_notify,
> + },
> +};
> +
> +module_acpi_driver(acpi_als_driver);
> +
> +MODULE_AUTHOR("Zhang Rui <rui.zhang@intel.com>");
> +MODULE_AUTHOR("Martin Liska <marxin.liska@gmail.com>");
> +MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
> +MODULE_DESCRIPTION("ACPI Ambient Light Sensor Driver");
> +MODULE_LICENSE("GPL");
> --
Other than that, it looks good to me.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-30 12:24 ` Daniel Baluta
@ 2015-04-30 19:14 ` Gabriele Mazzotta
0 siblings, 0 replies; 14+ messages in thread
From: Gabriele Mazzotta @ 2015-04-30 19:14 UTC (permalink / raw)
To: Daniel Baluta
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, marxin.liska, marex, rui.zhang,
Linux Kernel Mailing List, linux-iio@vger.kernel.org
On Thursday 30 April 2015 15:24:07 Daniel Baluta wrote:
> Hi Gabriele,
>
> Please keep the version numbering from previous patches,
> I think this should be v8 :).
OK, I'll use v9 for the next submission.
> On Wed, Apr 29, 2015 at 2:27 PM, Gabriele Mazzotta
> <gabriele.mzt@gmail.com> wrote:
> > Add basic implementation of the ACPI0008 Ambient Light Sensor driver.
> > This driver currently supports only the ALI property, yet is ready to
> > be easily extended to handle ALC, ALT, ALP ones as well.
>
> I would like this commit message to contain more info about ACPI0008 ALS, like
> this commit.
>
> https://lwn.net/Articles/345648/
>
> Also, would be nice to point readers to Chapter 9.2 from ACPI spec:
>
> http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
I will do it, I simply used the same message of the previous
submissions.
Thanks,
Gabriele
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-29 11:27 [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor Gabriele Mazzotta
2015-04-29 11:51 ` Gabriele Mazzotta
2015-04-30 12:24 ` Daniel Baluta
@ 2015-04-30 20:33 ` Paul Bolle
2015-04-30 20:58 ` Marek Vasut
2015-05-01 16:12 ` Gabriele Mazzotta
2015-05-02 11:25 ` Jonathan Cameron
4 siblings, 1 reply; 14+ messages in thread
From: Paul Bolle @ 2015-04-30 20:33 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: jic23, knaack.h, lars, pmeerw, marxin.liska, marex, rui.zhang,
linux-kernel, linux-iio
Just a nit: a license mismatch.
On Wed, 2015-04-29 at 13:27 +0200, Gabriele Mazzotta wrote:
> --- /dev/null
> +++ b/drivers/iio/light/acpi-als.c
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
And, according to include/linux/module.h, this states the license is GPL
v2 or later. So I think either the comment at the top of this file or
the ident used in the MODULE_LICENSE() macro should be changed.
Paul Bolle
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-30 20:33 ` Paul Bolle
@ 2015-04-30 20:58 ` Marek Vasut
0 siblings, 0 replies; 14+ messages in thread
From: Marek Vasut @ 2015-04-30 20:58 UTC (permalink / raw)
To: Paul Bolle
Cc: Gabriele Mazzotta, jic23, knaack.h, lars, pmeerw, marxin.liska,
rui.zhang, linux-kernel, linux-iio
On Thursday, April 30, 2015 at 10:33:46 PM, Paul Bolle wrote:
> Just a nit: a license mismatch.
>
> On Wed, 2015-04-29 at 13:27 +0200, Gabriele Mazzotta wrote:
> > --- /dev/null
> > +++ b/drivers/iio/light/acpi-als.c
> >
> > + * This program is free software; you can redistribute it and/or modify
> > it + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > WITHOUT + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > General Public License for + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>
> This states the license is GPL v2.
>
> > +MODULE_LICENSE("GPL");
>
> And, according to include/linux/module.h, this states the license is GPL
> v2 or later. So I think either the comment at the top of this file or
> the ident used in the MODULE_LICENSE() macro should be changed.
I'm OK with v2+ or later .
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-29 11:27 [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor Gabriele Mazzotta
` (2 preceding siblings ...)
2015-04-30 20:33 ` Paul Bolle
@ 2015-05-01 16:12 ` Gabriele Mazzotta
2015-05-02 11:22 ` Jonathan Cameron
2015-05-02 11:25 ` Jonathan Cameron
4 siblings, 1 reply; 14+ messages in thread
From: Gabriele Mazzotta @ 2015-05-01 16:12 UTC (permalink / raw)
Cc: jic23, knaack.h, lars, pmeerw, marxin.liska, marex, rui.zhang,
linux-kernel, linux-iio
Hi,
probably this is not completely related to this driver, but I
noticed something while testing it.
When I load acpi-als, /sys/bus/iio/devices/iio:device0/buffer/enable
is 0. If I try to set it to 1, I get the following error:
"Buffer not started: buffer parameter update failed (-22)"
After I got the error, I can successfully toggle "enable".
What I found is that the first time iio_request_update_kfifo() is
called, __iio_allocate_kfifo() gets called. Since bytes_per_datum
is 0 (as set iio_compute_scan_bytes()), it returns -EINVAL and so
does iio_request_update_kfifo(), causing the error above.
Subsequent calls of iio_request_update_kfifo() will return 0 as
update_needed is false, so "enable" can be toggled with no errors.
Isn't there something wrong here?
Regards,
Gabriele
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-05-01 16:12 ` Gabriele Mazzotta
@ 2015-05-02 11:22 ` Jonathan Cameron
0 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2015-05-02 11:22 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: knaack.h, lars, pmeerw, marxin.liska, marex, rui.zhang,
linux-kernel, linux-iio
On 01/05/15 17:12, Gabriele Mazzotta wrote:
> Hi,
>
> probably this is not completely related to this driver, but I
> noticed something while testing it.
>
> When I load acpi-als, /sys/bus/iio/devices/iio:device0/buffer/enable
> is 0. If I try to set it to 1, I get the following error:
> "Buffer not started: buffer parameter update failed (-22)"
>
> After I got the error, I can successfully toggle "enable".
>
> What I found is that the first time iio_request_update_kfifo() is
> called, __iio_allocate_kfifo() gets called. Since bytes_per_datum
> is 0 (as set iio_compute_scan_bytes()), it returns -EINVAL and so
> does iio_request_update_kfifo(), causing the error above.
>
> Subsequent calls of iio_request_update_kfifo() will return 0 as
> update_needed is false, so "enable" can be toggled with no errors.
>
> Isn't there something wrong here?
Definitely looks like it!
Thanks for pointing this out. Anyhow, the issue is exactly what you've
identified; the line below the __iio_allocate_kfifo clears the
update needed whether or not it succeeded. Clearly it should not
be doing that if a failure has occurred.
Would you mind submitting a fix patch for this?
Simply checking ret before setting updateneeded to false
should do the job.
Thanks,
Jonathan
>
> Regards,
> Gabriele
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor
2015-04-29 11:27 [PATCH] iio: acpi: Add ACPI0008 Ambient Light Sensor Gabriele Mazzotta
` (3 preceding siblings ...)
2015-05-01 16:12 ` Gabriele Mazzotta
@ 2015-05-02 11:25 ` Jonathan Cameron
4 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2015-05-02 11:25 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: knaack.h, lars, pmeerw, marxin.liska, marex, rui.zhang,
linux-kernel, linux-iio
On 29/04/15 12:27, Gabriele Mazzotta wrote:
> Add basic implementation of the ACPI0008 Ambient Light Sensor driver.
> This driver currently supports only the ALI property, yet is ready to
> be easily extended to handle ALC, ALT, ALP ones as well.
>
> Signed-off-by: Martin Liska <marxin.liska@gmail.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> ---
> This continues http://marc.info/?t=140163463200002
> I've made few adjustments over the original patch:
>
> - Code aligned with 4.1-rc1 and cleaned up
> - Use signed integers to store values: sensors report 32bit signed
> values. In particular, -1 is reported when the current reading
> is above the supported range of sensitivity.
>
> Most of the changes are just a consequence of the changes in the
> iio subsystem.
>
> Gabriele
Just one minor comment inline.
Looks good to me.
>
> drivers/iio/light/Kconfig | 12 +++
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/acpi-als.c | 240 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 253 insertions(+)
> create mode 100644 drivers/iio/light/acpi-als.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 01a1a16..898b2b5 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -5,6 +5,18 @@
>
> menu "Light sensors"
>
> +config ACPI_ALS
> + tristate "ACPI Ambient Light Sensor"
> + depends on ACPI
> + select IIO_TRIGGERED_BUFFER
> + select IIO_KFIFO_BUF
> + help
> + Say Y here if you want to build a driver for the ACPI0008
> + Ambient Light Sensor.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called acpi-als.
> +
> config ADJD_S311
> tristate "ADJD-S311-CR999 digital color sensor"
> select IIO_BUFFER
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index ad7c30f..d9aad52a 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -3,6 +3,7 @@
> #
>
> # When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_ACPI_ALS) += acpi-als.o
> obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> new file mode 100644
> index 0000000..fe9a8a0
> --- /dev/null
> +++ b/drivers/iio/light/acpi-als.c
> @@ -0,0 +1,240 @@
> +/*
> + * ACPI Ambient Light Sensor Driver
> + *
> + * Based on ALS driver:
> + * Copyright (C) 2009 Zhang Rui <rui.zhang@intel.com>
> + *
> + * Rework for IIO subsystem:
> + * Copyright (C) 2012-2013 Martin Liska <marxin.liska@gmail.com>
> + *
> + * Final cleanup and debugging:
> + * Copyright (C) 2013-2014 Marek Vasut <marex@denx.de>
> + * Copyright (C) 2015 Gabriele Mazzotta <gabriele.mzt@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +
> +#define ACPI_ALS_CLASS "als"
> +#define ACPI_ALS_DEVICE_NAME "acpi-als"
> +#define ACPI_ALS_NOTIFY_ILLUMINANCE 0x80
> +
> +ACPI_MODULE_NAME("acpi-als");
> +
> +/*
> + * So far, there's only one channel in here, but the specification for
> + * ACPI0008 says there can be more to what the block can report. Like
> + * chromaticity and such. We are ready for incoming additions!
> + */
> +static const struct iio_chan_spec acpi_als_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 10,
> + .storagebits = 32,
> + },
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + },
> +};
> +
> +/*
> + * The event buffer contains timestamp and all the data from
> + * the ACPI0008 block. There are multiple, but so far we only
> + * support _ALI (illuminance). Once someone adds new channels
> + * to acpi_als_channels[], the evt_buffer below will grow
> + * automatically.
> + */
> +#define EVT_NR_SOURCES ARRAY_SIZE(acpi_als_channels)
> +#define EVT_BUFFER_SIZE \
> + (sizeof(s64) + (EVT_NR_SOURCES * sizeof(s32)))
> +
> +struct acpi_als {
> + struct acpi_device *device;
> + struct mutex lock;
> +
> + s32 evt_buffer[EVT_BUFFER_SIZE];
> +};
> +
> +/*
> + * All types of properties the ACPI0008 block can report. The ALI, ALC, ALT
> + * and ALP can all be handled by als_read_value() below, while the ALR is
> + * special.
> + *
> + * The _ALR property returns tables that can be used to fine-tune the values
> + * reported by the other props based on the particular hardware type and it's
> + * location (it contains tables for "rainy", "bright inhouse lighting" etc.).
> + *
> + * So far, we support only ALI (illuminance).
> + */
> +#define ACPI_ALS_ILLUMINANCE "_ALI"
> +#define ACPI_ALS_CHROMATICITY "_ALC"
> +#define ACPI_ALS_COLOR_TEMP "_ALT"
> +#define ACPI_ALS_POLLING "_ALP"
> +#define ACPI_ALS_TABLES "_ALR"
> +
> +static int als_read_value(struct acpi_als *als, char *prop, s32 *val)
> +{
> + unsigned long long temp_val;
> + acpi_status status;
> +
> + status = acpi_evaluate_integer(als->device->handle, prop, NULL,
> + &temp_val);
> +
> + if (ACPI_FAILURE(status)) {
> + ACPI_EXCEPTION((AE_INFO, status, "Error reading ALS %s", prop));
> + return -EIO;
> + }
> +
> + *val = temp_val;
> +
> + return 0;
> +}
> +
> +static void acpi_als_notify(struct acpi_device *device, u32 event)
> +{
> + struct iio_dev *indio_dev = acpi_driver_data(device);
> + struct acpi_als *als = iio_priv(indio_dev);
> + s32 *buffer = als->evt_buffer;
> + s64 time_ns = iio_get_time_ns();
> + s32 val;
> + int ret;
> +
> + mutex_lock(&als->lock);
> +
> + memset(buffer, 0, EVT_BUFFER_SIZE);
> +
> + switch (event) {
> + case ACPI_ALS_NOTIFY_ILLUMINANCE:
> + ret = als_read_value(als, ACPI_ALS_ILLUMINANCE, &val);
> + if (ret < 0)
> + goto out;
> + *buffer++ = val;
> + break;
> + default:
> + /* Unhandled event */
> + dev_dbg(&device->dev, "Unhandled ACPI ALS event (%08x)!\n",
> + event);
> + goto out;
> + }
> +
> + iio_push_to_buffers_with_timestamp(indio_dev,
> + (u8 *)als->evt_buffer, time_ns);
> +
> +out:
> + mutex_unlock(&als->lock);
> +}
> +
> +static int acpi_als_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct acpi_als *als = iio_priv(indio_dev);
> + s32 temp_val;
> + int ret;
> +
> + if (mask != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + /* we support only illumination (_ALI) so far. */
> + if (chan->type != IIO_LIGHT)
> + return -EINVAL;
> +
> + ret = als_read_value(als, ACPI_ALS_ILLUMINANCE, &temp_val);
> + if (ret < 0)
> + return ret;
> +
> + *val = temp_val;
> +
> + return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info acpi_als_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = acpi_als_read_raw,
> +};
> +
> +static int acpi_als_add(struct acpi_device *device)
> +{
> + struct acpi_als *als;
> + struct iio_dev *indio_dev;
> + struct iio_buffer *buffer;
> +
> + indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + als = iio_priv(indio_dev);
> +
> + device->driver_data = indio_dev;
> + als->device = device;
> + mutex_init(&als->lock);
> +
> + indio_dev->name = ACPI_ALS_DEVICE_NAME;
> + indio_dev->dev.parent = &device->dev;
> + indio_dev->info = &acpi_als_info;
> + indio_dev->modes = INDIO_BUFFER_SOFTWARE;
> + indio_dev->channels = acpi_als_channels;
> + indio_dev->num_channels = ARRAY_SIZE(acpi_als_channels);
> +
> + buffer = devm_iio_kfifo_allocate(&device->dev);
> + if (!buffer)
> + return -ENOMEM;
> +
> + iio_device_attach_buffer(indio_dev, buffer);
> +
> + return iio_device_register(indio_dev);
> +}
> +
> +static int acpi_als_remove(struct acpi_device *device)
> +{
> + struct iio_dev *indio_dev = acpi_driver_data(device);
> +
> + iio_device_unregister(indio_dev);
As this is all you have here, you can use devm_iio_device_register
and then getting rid of the remove entirely.
> +
> + return 0;
> +}
> +
> +static const struct acpi_device_id acpi_als_device_ids[] = {
> + {"ACPI0008", 0},
> + {"", 0},
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, acpi_als_device_ids);
> +
> +static struct acpi_driver acpi_als_driver = {
> + .name = "acpi_als",
> + .class = ACPI_ALS_CLASS,
> + .ids = acpi_als_device_ids,
> + .ops = {
> + .add = acpi_als_add,
> + .remove = acpi_als_remove,
> + .notify = acpi_als_notify,
> + },
> +};
> +
> +module_acpi_driver(acpi_als_driver);
> +
> +MODULE_AUTHOR("Zhang Rui <rui.zhang@intel.com>");
> +MODULE_AUTHOR("Martin Liska <marxin.liska@gmail.com>");
> +MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
> +MODULE_DESCRIPTION("ACPI Ambient Light Sensor Driver");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 14+ messages in thread