* [PATCH] Dyna-Image AL3320A update, add AL3010 driver
@ 2016-06-03 4:02 Rocky Hsiao
2016-06-03 7:45 ` Daniel Baluta
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Rocky Hsiao @ 2016-06-03 4:02 UTC (permalink / raw)
To: linux-kernel
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Olof Johansson, Filipe Brandenburger,
Brian Norris, Benson Leung, Douglas Anderson, Sonny Rao,
Rocky Hsiao, Daniel Kurtz, Daniel Baluta, Adriana Reus,
Gwendal Grignou, Matt Ranostay, Puthikorn Voravootivat,
Andreas Dannenberg, linux-iio, John Huang
1. Change al3320a.c to match light sensor test flow.
2. Add al3010.c to add new device AL3010.
original file copy from al3320a.c
Signed-off-by: Rocky Hsiao <rocky.hsiao@dyna-image.com>
---
.../config/x86_64/chromiumos-x86_64.flavour.config | 2 +
drivers/iio/light/Kconfig | 10 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/al3010.c | 301 +++++++++++++++++++++
drivers/iio/light/al3320a.c | 73 ++++-
5 files changed, 378 insertions(+), 9 deletions(-)
create mode 100644 drivers/iio/light/al3010.c
diff --git a/chromeos/config/x86_64/chromiumos-x86_64.flavour.config b/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
index 7d2bed4..7980a14 100644
--- a/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
+++ b/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
@@ -2,6 +2,8 @@
# Config options generated by splitconfig
#
CONFIG_ACERHDF=m
+CONFIG_AL3010=m
+CONFIG_AL3320A=m
CONFIG_B43=m
CONFIG_B43_BCMA=y
CONFIG_B43_BCMA_PIO=y
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index ca89b26..57a2a7e 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -40,6 +40,16 @@ config AL3320A
To compile this driver as a module, choose M here: the
module will be called al3320a.
+config AL3010
+ tristate "AL3010 ambient light sensor"
+ depends on I2C
+ help
+ Say Y here if you want to build a driver for the Dyna Image AL3010
+ ambient light sensor.
+
+ To compile this driver as a module, choose M here: the
+ module will be called al3010.
+
config APDS9300
tristate "APDS9300 ambient light sensor"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 5df1118..3f615d7 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -5,6 +5,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_AL3010) += al3010.o
obj-$(CONFIG_AL3320A) += al3320a.o
obj-$(CONFIG_APDS9300) += apds9300.o
obj-$(CONFIG_APDS9960) += apds9960.o
diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
new file mode 100644
index 0000000..2df425b
--- /dev/null
+++ b/drivers/iio/light/al3010.c
@@ -0,0 +1,301 @@
+/*
+ * AL3010 - Dyna Image Ambient Light Sensor
+ *
+ * Copyright (C) 2016 Dyna-Image Corp.
+ *
+ * This software is licedsed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * Note about the original authors:
+ *
+ * This driver is based on the original driver for AL3320A that was distributed
+ * by Intel Corporation.
+ * The following is part of the header found in that file
+ *
+ * >> AL3320A - Dyna Image Ambient Light Sensor
+ *
+ * >> Copyright (c) 2014, Intel Corporation.
+ *
+ * >> This file is subject to the terms and conditions of version 2 of
+ * >> the GNU General Public License. See the file COPYING in the main
+ * >> directory of this archive for more details.
+ *
+ * >> IIO driver for AL3010 (7-bit I2C slave address 0x1C).
+ *
+ * >> TODO: interrupt support, thresholds
+ *
+ * >> Author:Daniel Baluta <daniel.baluta@intel.com>
+ *
+ * The kernel version is 4.4
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define AL3010_DRV_NAME "al3010"
+
+#define AL3010_REG_SYSTEM 0x00
+#define AL3010_REG_CONFIG 0x10
+#define AL3010_REG_DATA_LOW 0x0c
+
+#define AL3010_GAIN_MASK (BIT(6) | BIT(5) | BIT(4))
+#define AL3010_GAIN_SHIFT 4
+
+#define AL3010_CONFIG_DISABLE 0x00
+#define AL3010_CONFIG_ENABLE 0x01
+
+#define AL3010_SCALE_AVAILABLE "1.1872 0.2968 0.0742 0.0186"
+
+enum al3010_range {
+ AL3010_RANGE_1, /* 77806 lx */
+ AL3010_RANGE_2, /* 19452 lx */
+ AL3010_RANGE_3, /* 4863 lx */
+ AL3010_RANGE_4 /* 1216 lx */
+};
+
+static const int al3010_scales[][2] = {
+ {0, 1187200}, {0, 296800}, {0, 74200}, {0, 18600}
+};
+
+struct al3010_data {
+ struct i2c_client *client;
+};
+
+static const struct iio_chan_spec al3010_channels[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
+ }
+};
+
+static int al3010_set_gain(struct al3010_data *data, int gain)
+{
+ int ret;
+
+ ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
+ (gain<<AL3010_GAIN_SHIFT)&AL3010_GAIN_MASK);
+
+ return ret;
+}
+
+static int al3010_set_mode(struct al3010_data *data, int mode)
+{
+ int ret;
+
+ ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_SYSTEM, mode);
+
+ return ret;
+}
+
+static int al3010_get_mode(struct al3010_data *data)
+{
+ int ret;
+
+ ret = i2c_smbus_read_byte_data(data->client, AL3010_REG_SYSTEM);
+
+ return ret;
+}
+
+static int al3010_get_adc_value(struct al3010_data *data)
+{
+ int ret;
+
+ ret = i2c_smbus_read_word_data(data->client, AL3010_REG_DATA_LOW);
+
+ return ret;
+}
+
+static int al3010_get_lux(struct al3010_data *data)
+{
+ int ret;
+ long int ret64;
+
+ ret = al3010_get_adc_value(data);
+ ret64 = ret;
+ ret64 = (ret64 * 74200) / 1000000;
+ ret = ret64;
+
+ return ret;
+}
+
+/* lux */
+static ssize_t al3010_show_lux(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct al3010_data *data = iio_priv(dev_to_iio_dev(dev));
+ int ret;
+
+ /* No LUX data if not operational */
+ if (al3010_get_mode(data) != AL3010_CONFIG_ENABLE)
+ return -EBUSY;
+
+ ret = al3010_get_lux(data);
+
+ return sprintf(buf, "%d\n", ret);
+}
+
+static IIO_CONST_ATTR(in_illuminance_scale_available, AL3010_SCALE_AVAILABLE);
+
+static DEVICE_ATTR(illuminance0_input, S_IRUGO, al3010_show_lux, NULL);
+
+static struct attribute *al3010_attributes[] = {
+ &dev_attr_illuminance0_input.attr,
+ &iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group al3010_attribute_group = {
+ .attrs = al3010_attributes,
+};
+
+static int al3010_init(struct al3010_data *data)
+{
+ int err = 0;
+
+ err = al3010_set_mode(data, AL3010_CONFIG_ENABLE);
+ if (err) {
+ dev_err(&data->client->dev,
+ "%s: al3010_set_mode returned error %d\n",
+ __func__, err);
+ return err;
+ }
+
+ /*
+ * set sensor range to 4863 lux.
+ * (If panel luminousness is 10% , the range of pad is 0 ~ 48630 lux.)
+ */
+ err = al3010_set_gain(data, AL3010_RANGE_3);
+ if (err) {
+ dev_err(&data->client->dev,
+ "%s: al3010_set_range returned error %d\n",
+ __func__, err);
+ return err;
+ }
+
+ return 0;
+}
+
+static int al3010_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ struct al3010_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ /*
+ * ALS ADC value is stored in two adjacent registers:
+ * - low byte of output is stored at AL3320A_REG_DATA_LOW
+ * - high byte of output is stored at AL3320A_REG_DATA_LOW + 1
+ */
+ ret = i2c_smbus_read_word_data(data->client,
+ AL3010_REG_DATA_LOW);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ ret = i2c_smbus_read_byte_data(data->client,
+ AL3010_REG_CONFIG);
+ if (ret < 0)
+ return ret;
+
+ ret = (ret & AL3010_GAIN_MASK) >> AL3010_GAIN_SHIFT;
+ *val = al3010_scales[ret][0];
+ *val2 = al3010_scales[ret][1];
+
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
+ return -EINVAL;
+}
+
+static int al3010_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ int val2, long mask)
+{
+ struct al3010_data *data = iio_priv(indio_dev);
+ int i;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ for (i = 0; i < ARRAY_SIZE(al3010_scales); i++) {
+ if (val == al3010_scales[i][0] &&
+ val2 == al3010_scales[i][1])
+ return al3010_set_gain(data, i);
+ }
+ break;
+ }
+ return -EINVAL;
+}
+
+static const struct iio_info al3010_info = {
+ .driver_module = THIS_MODULE,
+ .read_raw = al3010_read_raw,
+ .write_raw = al3010_write_raw,
+ .attrs = &al3010_attribute_group,
+};
+
+static int al3010_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct al3010_data *data;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ data->client = client;
+
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->info = &al3010_info;
+ indio_dev->name = AL3010_DRV_NAME;
+ indio_dev->channels = al3010_channels;
+ indio_dev->num_channels = ARRAY_SIZE(al3010_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = al3010_init(data);
+ if (ret < 0) {
+ dev_err(&client->dev, "al3010 chip init failed\n");
+ return ret;
+ }
+
+ return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static int al3010_remove(struct i2c_client *client)
+{
+ return i2c_smbus_write_byte_data(client, AL3010_REG_SYSTEM, 0x00);
+}
+
+static const struct i2c_device_id al3010_id[] = {
+ {"al3010", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, al3010_id);
+
+static struct i2c_driver al3010_driver = {
+ .driver = {
+ .name = AL3010_DRV_NAME,
+ },
+ .probe = al3010_probe,
+ .remove = al3010_remove,
+ .id_table = al3010_id,
+};
+
+module_i2c_driver(al3010_driver);
+
+MODULE_AUTHOR("Rocky Hsiao <rocky.hsiao@dyna-image.com");
+MODULE_DESCRIPTION("AL3010 Ambient Light Sensor driver");
+MODULE_LICENSE("GPL v2");
+
diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
index 6aac651..4212772 100644
--- a/drivers/iio/light/al3320a.c
+++ b/drivers/iio/light/al3320a.c
@@ -1,16 +1,32 @@
/*
* AL3320A - Dyna Image Ambient Light Sensor
*
- * Copyright (c) 2014, Intel Corporation.
+ * Copyright (C) 2016 Dyna Image Corp.
*
- * This file is subject to the terms and conditions of version 2 of
- * the GNU General Public License. See the file COPYING in the main
- * directory of this archive for more details.
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
*
- * IIO driver for AL3320A (7-bit I2C slave address 0x1C).
+ * This program is distributed in the hope that 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.
*
- * TODO: interrupt support, thresholds
+ * Note about the original authors:
*
+ * >> Copyright (c) 2014, Intel Corporation.
+ *
+ * >> This file is subject to the terms and conditions of version 2 of
+ * >> the GNU General Public License. See the file COPYING in the main
+ * >> directory of this archive for more details.
+ *
+ * >> IIO driver for AL3320A (7-bit I2C slave address 0x1C).
+ *
+ * >> TODO: interrupt support, thresholds
+ *
+ * >> Author:Daniel Baluta <daniel.baluta@intel.com>
+ *
+ * The kernel version is 4.4
*/
#include <linux/module.h>
@@ -72,9 +88,47 @@ static const struct iio_chan_spec al3320a_channels[] = {
}
};
+static int al3320a_get_adc_value(struct al3320a_data *data)
+{
+ int val;
+
+ val = i2c_smbus_read_word_data(data->client, AL3320A_REG_DATA_LOW);
+
+ return val;
+}
+
+static int al3320a_get_lux(struct al3320a_data *data)
+{
+ int ret;
+ long ret64;
+
+ ret = al3320a_get_adc_value(data);
+ ret64 = ret;
+ ret64 = (ret64 * 32000) / 1000000;
+ ret = ret64;
+
+ return ret;
+}
+
+static ssize_t al3320a_lux_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct al3320a_data *data = iio_priv(dev_to_iio_dev(dev));
+ int val;
+
+ val = al3320a_get_lux(data);
+
+ return sprintf(buf, "%d\n", val);
+}
+
+static IIO_DEVICE_ATTR(illuminance0_input, S_IRUGO,
+ al3320a_lux_show, NULL, 0);
+
static IIO_CONST_ATTR(in_illuminance_scale_available, AL3320A_SCALE_AVAILABLE);
static struct attribute *al3320a_attributes[] = {
+ &iio_dev_attr_illuminance0_input.dev_attr.attr,
&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
NULL,
};
@@ -125,8 +179,8 @@ static int al3320a_read_raw(struct iio_dev *indio_dev,
* - low byte of output is stored at AL3320A_REG_DATA_LOW
* - high byte of output is stored at AL3320A_REG_DATA_LOW + 1
*/
- ret = i2c_smbus_read_word_data(data->client,
- AL3320A_REG_DATA_LOW);
+ ret = al3320a_get_adc_value(data);
+
if (ret < 0)
return ret;
*val = ret;
@@ -201,6 +255,7 @@ static int al3320a_probe(struct i2c_client *client,
dev_err(&client->dev, "al3320a chip init failed\n");
return ret;
}
+
return devm_iio_device_register(&client->dev, indio_dev);
}
@@ -227,6 +282,6 @@ static struct i2c_driver al3320a_driver = {
module_i2c_driver(al3320a_driver);
-MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
+MODULE_AUTHOR("Rocky Hsiao <rocky.hsiao@dyna-image.com>");
MODULE_DESCRIPTION("AL3320A Ambient Light Sensor driver");
MODULE_LICENSE("GPL v2");
--
2.1.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Dyna-Image AL3320A update, add AL3010 driver
2016-06-03 4:02 [PATCH] Dyna-Image AL3320A update, add AL3010 driver Rocky Hsiao
@ 2016-06-03 7:45 ` Daniel Baluta
2016-06-03 8:35 ` Jonathan Cameron
2016-06-03 7:51 ` Benson Leung
2016-06-03 8:20 ` Lars-Peter Clausen
2 siblings, 1 reply; 6+ messages in thread
From: Daniel Baluta @ 2016-06-03 7:45 UTC (permalink / raw)
To: Rocky Hsiao
Cc: Linux Kernel Mailing List, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Olof Johansson,
Filipe Brandenburger, Brian Norris, Benson Leung,
Douglas Anderson, Sonny Rao, Daniel Kurtz, Daniel Baluta,
Adriana Reus, Gwendal Grignou, Matt Ranostay,
Puthikorn Voravootivat, Andreas Dannenberg,
linux-iio@vger.kernel.org, John Huang
Hi Rocky,
Few bits inline, before I will take my time to do an in depth review.
On Fri, Jun 3, 2016 at 7:02 AM, Rocky Hsiao <rocky.hsiao@dyna-image.com> wrote:
> 1. Change al3320a.c to match light sensor test flow.
> 2. Add al3010.c to add new device AL3010.
> original file copy from al3320a.c
Please split this into several patches, each patch implementing one
single functionality.
Can you add support for AL3010 inside al3320a.c. I don't know exactly
how different
this sensors are.
Do you have a link to the datasheets?
>
> Signed-off-by: Rocky Hsiao <rocky.hsiao@dyna-image.com>
> ---
> .../config/x86_64/chromiumos-x86_64.flavour.config | 2 +
What tree are you using? Please use Jonathan's linux-iio git tree.
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/?h=togreg
> drivers/iio/light/Kconfig | 10 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/al3010.c | 301 +++++++++++++++++++++
> drivers/iio/light/al3320a.c | 73 ++++-
> 5 files changed, 378 insertions(+), 9 deletions(-)
> create mode 100644 drivers/iio/light/al3010.c
>
> diff --git a/chromeos/config/x86_64/chromiumos-x86_64.flavour.config b/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
> index 7d2bed4..7980a14 100644
> --- a/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
> +++ b/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
> @@ -2,6 +2,8 @@
> # Config options generated by splitconfig
> #
> CONFIG_ACERHDF=m
> +CONFIG_AL3010=m
> +CONFIG_AL3320A=m
> CONFIG_B43=m
> CONFIG_B43_BCMA=y
> CONFIG_B43_BCMA_PIO=y
This is part of your distribution OS and it shouldn't be submitted here.
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index ca89b26..57a2a7e 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -40,6 +40,16 @@ config AL3320A
> To compile this driver as a module, choose M here: the
> module will be called al3320a.
>
> +config AL3010
> + tristate "AL3010 ambient light sensor"
> + depends on I2C
> + help
> + Say Y here if you want to build a driver for the Dyna Image AL3010
> + ambient light sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called al3010.
> +
> config APDS9300
> tristate "APDS9300 ambient light sensor"
> depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 5df1118..3f615d7 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -5,6 +5,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_AL3010) += al3010.o
> obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_APDS9960) += apds9960.o
> diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
> new file mode 100644
> index 0000000..2df425b
> --- /dev/null
> +++ b/drivers/iio/light/al3010.c
> @@ -0,0 +1,301 @@
> +/*
> + * AL3010 - Dyna Image Ambient Light Sensor
> + *
> + * Copyright (C) 2016 Dyna-Image Corp.
> + *
> + * This software is licedsed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * Note about the original authors:
> + *
> + * This driver is based on the original driver for AL3320A that was distributed
> + * by Intel Corporation.
> + * The following is part of the header found in that file
> + *
> + * >> AL3320A - Dyna Image Ambient Light Sensor
> + *
> + * >> Copyright (c) 2014, Intel Corporation.
> + *
> + * >> This file is subject to the terms and conditions of version 2 of
> + * >> the GNU General Public License. See the file COPYING in the main
> + * >> directory of this archive for more details.
> + *
> + * >> IIO driver for AL3010 (7-bit I2C slave address 0x1C).
> + *
> + * >> TODO: interrupt support, thresholds
> + *
> + * >> Author:Daniel Baluta <daniel.baluta@intel.com>
> + *
> + * The kernel version is 4.4
Again please rebase this on Jonathan latest sources as specified above.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define AL3010_DRV_NAME "al3010"
> +
> +#define AL3010_REG_SYSTEM 0x00
> +#define AL3010_REG_CONFIG 0x10
> +#define AL3010_REG_DATA_LOW 0x0c
> +
> +#define AL3010_GAIN_MASK (BIT(6) | BIT(5) | BIT(4))
> +#define AL3010_GAIN_SHIFT 4
> +
> +#define AL3010_CONFIG_DISABLE 0x00
> +#define AL3010_CONFIG_ENABLE 0x01
> +
> +#define AL3010_SCALE_AVAILABLE "1.1872 0.2968 0.0742 0.0186"
> +
> +enum al3010_range {
> + AL3010_RANGE_1, /* 77806 lx */
> + AL3010_RANGE_2, /* 19452 lx */
> + AL3010_RANGE_3, /* 4863 lx */
> + AL3010_RANGE_4 /* 1216 lx */
> +};
> +
> +static const int al3010_scales[][2] = {
> + {0, 1187200}, {0, 296800}, {0, 74200}, {0, 18600}
> +};
> +
> +struct al3010_data {
> + struct i2c_client *client;
> +};
> +
> +static const struct iio_chan_spec al3010_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + }
> +};
> +
> +static int al3010_set_gain(struct al3010_data *data, int gain)
> +{
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
> + (gain<<AL3010_GAIN_SHIFT)&AL3010_GAIN_MASK);
> +
> + return ret;
> +}
> +
> +static int al3010_set_mode(struct al3010_data *data, int mode)
> +{
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_SYSTEM, mode);
> +
> + return ret;
> +}
> +
> +static int al3010_get_mode(struct al3010_data *data)
> +{
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(data->client, AL3010_REG_SYSTEM);
> +
> + return ret;
> +}
> +
> +static int al3010_get_adc_value(struct al3010_data *data)
> +{
> + int ret;
> +
> + ret = i2c_smbus_read_word_data(data->client, AL3010_REG_DATA_LOW);
> +
> + return ret;
> +}
> +
> +static int al3010_get_lux(struct al3010_data *data)
> +{
> + int ret;
> + long int ret64;
> +
> + ret = al3010_get_adc_value(data);
> + ret64 = ret;
> + ret64 = (ret64 * 74200) / 1000000;
> + ret = ret64;
> +
> + return ret;
> +}
> +
> +/* lux */
> +static ssize_t al3010_show_lux(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct al3010_data *data = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> +
> + /* No LUX data if not operational */
> + if (al3010_get_mode(data) != AL3010_CONFIG_ENABLE)
> + return -EBUSY;
> +
> + ret = al3010_get_lux(data);
> +
> + return sprintf(buf, "%d\n", ret);
> +}
> +
> +static IIO_CONST_ATTR(in_illuminance_scale_available, AL3010_SCALE_AVAILABLE);
> +
> +static DEVICE_ATTR(illuminance0_input, S_IRUGO, al3010_show_lux, NULL);
> +
> +static struct attribute *al3010_attributes[] = {
> + &dev_attr_illuminance0_input.attr,
> + &iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group al3010_attribute_group = {
> + .attrs = al3010_attributes,
> +};
> +
> +static int al3010_init(struct al3010_data *data)
> +{
> + int err = 0;
> +
> + err = al3010_set_mode(data, AL3010_CONFIG_ENABLE);
> + if (err) {
> + dev_err(&data->client->dev,
> + "%s: al3010_set_mode returned error %d\n",
> + __func__, err);
> + return err;
> + }
> +
> + /*
> + * set sensor range to 4863 lux.
> + * (If panel luminousness is 10% , the range of pad is 0 ~ 48630 lux.)
> + */
> + err = al3010_set_gain(data, AL3010_RANGE_3);
> + if (err) {
> + dev_err(&data->client->dev,
> + "%s: al3010_set_range returned error %d\n",
> + __func__, err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int al3010_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct al3010_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + /*
> + * ALS ADC value is stored in two adjacent registers:
> + * - low byte of output is stored at AL3320A_REG_DATA_LOW
> + * - high byte of output is stored at AL3320A_REG_DATA_LOW + 1
> + */
> + ret = i2c_smbus_read_word_data(data->client,
> + AL3010_REG_DATA_LOW);
> + if (ret < 0)
> + return ret;
> + *val = ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + ret = i2c_smbus_read_byte_data(data->client,
> + AL3010_REG_CONFIG);
> + if (ret < 0)
> + return ret;
> +
> + ret = (ret & AL3010_GAIN_MASK) >> AL3010_GAIN_SHIFT;
> + *val = al3010_scales[ret][0];
> + *val2 = al3010_scales[ret][1];
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + return -EINVAL;
> +}
> +
> +static int al3010_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct al3010_data *data = iio_priv(indio_dev);
> + int i;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + for (i = 0; i < ARRAY_SIZE(al3010_scales); i++) {
> + if (val == al3010_scales[i][0] &&
> + val2 == al3010_scales[i][1])
> + return al3010_set_gain(data, i);
> + }
> + break;
> + }
> + return -EINVAL;
> +}
> +
> +static const struct iio_info al3010_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = al3010_read_raw,
> + .write_raw = al3010_write_raw,
> + .attrs = &al3010_attribute_group,
> +};
> +
> +static int al3010_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct al3010_data *data;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + data->client = client;
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->info = &al3010_info;
> + indio_dev->name = AL3010_DRV_NAME;
> + indio_dev->channels = al3010_channels;
> + indio_dev->num_channels = ARRAY_SIZE(al3010_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = al3010_init(data);
> + if (ret < 0) {
> + dev_err(&client->dev, "al3010 chip init failed\n");
> + return ret;
> + }
> +
> + return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static int al3010_remove(struct i2c_client *client)
> +{
> + return i2c_smbus_write_byte_data(client, AL3010_REG_SYSTEM, 0x00);
> +}
> +
> +static const struct i2c_device_id al3010_id[] = {
> + {"al3010", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, al3010_id);
> +
> +static struct i2c_driver al3010_driver = {
> + .driver = {
> + .name = AL3010_DRV_NAME,
> + },
> + .probe = al3010_probe,
> + .remove = al3010_remove,
> + .id_table = al3010_id,
> +};
> +
> +module_i2c_driver(al3010_driver);
> +
> +MODULE_AUTHOR("Rocky Hsiao <rocky.hsiao@dyna-image.com");
> +MODULE_DESCRIPTION("AL3010 Ambient Light Sensor driver");
> +MODULE_LICENSE("GPL v2");
This looks pretty similar with al3320a. Can you try to implement the support for
al3010 inside al3320a.c file?
Avoid duplicating code.
> +
> diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
> index 6aac651..4212772 100644
> --- a/drivers/iio/light/al3320a.c
> +++ b/drivers/iio/light/al3320a.c
> @@ -1,16 +1,32 @@
> /*
> * AL3320A - Dyna Image Ambient Light Sensor
> *
> - * Copyright (c) 2014, Intel Corporation.
> + * Copyright (C) 2016 Dyna Image Corp.
Please don't remove the copyright from Intel. Just add your copyright notice.
> *
> - * This file is subject to the terms and conditions of version 2 of
> - * the GNU General Public License. See the file COPYING in the main
> - * directory of this archive for more details.
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> *
> - * IIO driver for AL3320A (7-bit I2C slave address 0x1C).
> + * This program is distributed in the hope that 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.
> *
> - * TODO: interrupt support, thresholds
> + * Note about the original authors:
> *
> + * >> Copyright (c) 2014, Intel Corporation.
> + *
> + * >> This file is subject to the terms and conditions of version 2 of
> + * >> the GNU General Public License. See the file COPYING in the main
> + * >> directory of this archive for more details.
> + *
> + * >> IIO driver for AL3320A (7-bit I2C slave address 0x1C).
> + *
> + * >> TODO: interrupt support, thresholds
> + *
> + * >> Author:Daniel Baluta <daniel.baluta@intel.com>
> + *
> + * The kernel version is 4.4
> */
>
> #include <linux/module.h>
> @@ -72,9 +88,47 @@ static const struct iio_chan_spec al3320a_channels[] = {
> }
> };
>
> +static int al3320a_get_adc_value(struct al3320a_data *data)
> +{
> + int val;
> +
> + val = i2c_smbus_read_word_data(data->client, AL3320A_REG_DATA_LOW);
> +
> + return val;
> +}
> +
> +static int al3320a_get_lux(struct al3320a_data *data)
> +{
> + int ret;
> + long ret64;
> +
> + ret = al3320a_get_adc_value(data);
> + ret64 = ret;
> + ret64 = (ret64 * 32000) / 1000000;
> + ret = ret64;
> +
> + return ret;
> +}
> +
> +static ssize_t al3320a_lux_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct al3320a_data *data = iio_priv(dev_to_iio_dev(dev));
> + int val;
> +
> + val = al3320a_get_lux(data);
> +
> + return sprintf(buf, "%d\n", val);
> +}
> +
> +static IIO_DEVICE_ATTR(illuminance0_input, S_IRUGO,
> + al3320a_lux_show, NULL, 0);
> +
> static IIO_CONST_ATTR(in_illuminance_scale_available, AL3320A_SCALE_AVAILABLE);
>
> static struct attribute *al3320a_attributes[] = {
> + &iio_dev_attr_illuminance0_input.dev_attr.attr,
> &iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
> NULL,
> };
> @@ -125,8 +179,8 @@ static int al3320a_read_raw(struct iio_dev *indio_dev,
> * - low byte of output is stored at AL3320A_REG_DATA_LOW
> * - high byte of output is stored at AL3320A_REG_DATA_LOW + 1
> */
> - ret = i2c_smbus_read_word_data(data->client,
> - AL3320A_REG_DATA_LOW);
> + ret = al3320a_get_adc_value(data);
> +
> if (ret < 0)
> return ret;
> *val = ret;
> @@ -201,6 +255,7 @@ static int al3320a_probe(struct i2c_client *client,
> dev_err(&client->dev, "al3320a chip init failed\n");
> return ret;
> }
> +
> return devm_iio_device_register(&client->dev, indio_dev);
> }
>
> @@ -227,6 +282,6 @@ static struct i2c_driver al3320a_driver = {
>
> module_i2c_driver(al3320a_driver);
>
> -MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
> +MODULE_AUTHOR("Rocky Hsiao <rocky.hsiao@dyna-image.com>");
Again, mark your contribution here. Do not remove initial author :).
> MODULE_DESCRIPTION("AL3320A Ambient Light Sensor driver");
> MODULE_LICENSE("GPL v2");
I am happy to see Dyna Image starting to do upstream work on their sensors :).
thanks,
Daniel.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Dyna-Image AL3320A update, add AL3010 driver
2016-06-03 7:45 ` Daniel Baluta
@ 2016-06-03 8:35 ` Jonathan Cameron
2016-06-03 8:40 ` Jonathan Cameron
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2016-06-03 8:35 UTC (permalink / raw)
To: Daniel Baluta, Rocky Hsiao
Cc: Linux Kernel Mailing List, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Olof Johansson, Filipe Brandenburger,
Brian Norris, Benson Leung, Douglas Anderson, Sonny Rao,
Daniel Kurtz, Adriana Reus, Gwendal Grignou, Matt Ranostay,
Puthikorn Voravootivat, Andreas Dannenberg,
linux-iio@vger.kernel.org, John Huang
On 03/06/16 08:45, Daniel Baluta wrote:
> Hi Rocky,
>
> Few bits inline, before I will take my time to do an in depth review.
>
> On Fri, Jun 3, 2016 at 7:02 AM, Rocky Hsiao <rocky.hsiao@dyna-image.com> wrote:
>> 1. Change al3320a.c to match light sensor test flow.
>> 2. Add al3010.c to add new device AL3010.
>> original file copy from al3320a.c
>
> Please split this into several patches, each patch implementing one
> single functionality.
>
> Can you add support for AL3010 inside al3320a.c. I don't know exactly
> how different
> this sensors are.
>
> Do you have a link to the datasheets?
>
>>
>> Signed-off-by: Rocky Hsiao <rocky.hsiao@dyna-image.com>
>> ---
>> .../config/x86_64/chromiumos-x86_64.flavour.config | 2 +
>
> What tree are you using? Please use Jonathan's linux-iio git tree.
>
> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/?h=togreg
>
>> drivers/iio/light/Kconfig | 10 +
>> drivers/iio/light/Makefile | 1 +
>> drivers/iio/light/al3010.c | 301 +++++++++++++++++++++
>> drivers/iio/light/al3320a.c | 73 ++++-
>> 5 files changed, 378 insertions(+), 9 deletions(-)
>> create mode 100644 drivers/iio/light/al3010.c
>>
>> diff --git a/chromeos/config/x86_64/chromiumos-x86_64.flavour.config b/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
>> index 7d2bed4..7980a14 100644
>> --- a/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
>> +++ b/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
>> @@ -2,6 +2,8 @@
>> # Config options generated by splitconfig
>> #
>> CONFIG_ACERHDF=m
>> +CONFIG_AL3010=m
>> +CONFIG_AL3320A=m
>> CONFIG_B43=m
>> CONFIG_B43_BCMA=y
>> CONFIG_B43_BCMA_PIO=y
>
> This is part of your distribution OS and it shouldn't be submitted here.
>
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index ca89b26..57a2a7e 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -40,6 +40,16 @@ config AL3320A
>> To compile this driver as a module, choose M here: the
>> module will be called al3320a.
>>
>> +config AL3010
>> + tristate "AL3010 ambient light sensor"
>> + depends on I2C
>> + help
>> + Say Y here if you want to build a driver for the Dyna Image AL3010
>> + ambient light sensor.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called al3010.
>> +
>> config APDS9300
>> tristate "APDS9300 ambient light sensor"
>> depends on I2C
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index 5df1118..3f615d7 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -5,6 +5,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_AL3010) += al3010.o
>> obj-$(CONFIG_AL3320A) += al3320a.o
>> obj-$(CONFIG_APDS9300) += apds9300.o
>> obj-$(CONFIG_APDS9960) += apds9960.o
>> diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
>> new file mode 100644
>> index 0000000..2df425b
>> --- /dev/null
>> +++ b/drivers/iio/light/al3010.c
>> @@ -0,0 +1,301 @@
>> +/*
>> + * AL3010 - Dyna Image Ambient Light Sensor
>> + *
>> + * Copyright (C) 2016 Dyna-Image Corp.
>> + *
>> + * This software is licedsed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * Note about the original authors:
>> + *
>> + * This driver is based on the original driver for AL3320A that was distributed
>> + * by Intel Corporation.
>> + * The following is part of the header found in that file
>> + *
>> + * >> AL3320A - Dyna Image Ambient Light Sensor
>> + *
>> + * >> Copyright (c) 2014, Intel Corporation.
>> + *
>> + * >> This file is subject to the terms and conditions of version 2 of
>> + * >> the GNU General Public License. See the file COPYING in the main
>> + * >> directory of this archive for more details.
>> + *
>> + * >> IIO driver for AL3010 (7-bit I2C slave address 0x1C).
>> + *
>> + * >> TODO: interrupt support, thresholds
>> + *
>> + * >> Author:Daniel Baluta <daniel.baluta@intel.com>
>> + *
>> + * The kernel version is 4.4
>
> Again please rebase this on Jonathan latest sources as specified above.
For a new driver it's almost always fine to base on the latest release
mainline kernel if you would prefer. Any tree wide reworks we can sort
out when applying the patch. Here, as you are modifying an existing
driver you may want to check if there are any related series already
under review or applied to my togreg branch at:
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/log/?h=togreg
Rule of thumb for branches in that tree is that testing is the very
latest stuff, but may well rebase so is not a good tree to use for
new patches. By the time it hits togreg it should be non rebasing and
hence you should be safe that what you see there is what your patches
will go on top of.
>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/i2c.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define AL3010_DRV_NAME "al3010"
>> +
>> +#define AL3010_REG_SYSTEM 0x00
>> +#define AL3010_REG_CONFIG 0x10
>> +#define AL3010_REG_DATA_LOW 0x0c
>> +
>> +#define AL3010_GAIN_MASK (BIT(6) | BIT(5) | BIT(4))
>> +#define AL3010_GAIN_SHIFT 4
>> +
>> +#define AL3010_CONFIG_DISABLE 0x00
>> +#define AL3010_CONFIG_ENABLE 0x01
>> +
>> +#define AL3010_SCALE_AVAILABLE "1.1872 0.2968 0.0742 0.0186"
>> +
>> +enum al3010_range {
>> + AL3010_RANGE_1, /* 77806 lx */
>> + AL3010_RANGE_2, /* 19452 lx */
>> + AL3010_RANGE_3, /* 4863 lx */
>> + AL3010_RANGE_4 /* 1216 lx */
>> +};
>> +
>> +static const int al3010_scales[][2] = {
>> + {0, 1187200}, {0, 296800}, {0, 74200}, {0, 18600}
>> +};
>> +
>> +struct al3010_data {
>> + struct i2c_client *client;
>> +};
>> +
>> +static const struct iio_chan_spec al3010_channels[] = {
>> + {
>> + .type = IIO_LIGHT,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_SCALE),
>> + }
>> +};
>> +
>> +static int al3010_set_gain(struct al3010_data *data, int gain)
>> +{
>> + int ret;
>> +
>> + ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
>> + (gain<<AL3010_GAIN_SHIFT)&AL3010_GAIN_MASK);
>> +
>> + return ret;
>> +}
>> +
>> +static int al3010_set_mode(struct al3010_data *data, int mode)
>> +{
>> + int ret;
>> +
>> + ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_SYSTEM, mode);
>> +
>> + return ret;
>> +}
>> +
>> +static int al3010_get_mode(struct al3010_data *data)
>> +{
>> + int ret;
>> +
>> + ret = i2c_smbus_read_byte_data(data->client, AL3010_REG_SYSTEM);
>> +
>> + return ret;
>> +}
>> +
>> +static int al3010_get_adc_value(struct al3010_data *data)
>> +{
>> + int ret;
>> +
>> + ret = i2c_smbus_read_word_data(data->client, AL3010_REG_DATA_LOW);
>> +
>> + return ret;
>> +}
>> +
>> +static int al3010_get_lux(struct al3010_data *data)
>> +{
>> + int ret;
>> + long int ret64;
>> +
>> + ret = al3010_get_adc_value(data);
>> + ret64 = ret;
>> + ret64 = (ret64 * 74200) / 1000000;
>> + ret = ret64;
>> +
>> + return ret;
>> +}
>> +
>> +/* lux */
>> +static ssize_t al3010_show_lux(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct al3010_data *data = iio_priv(dev_to_iio_dev(dev));
>> + int ret;
>> +
>> + /* No LUX data if not operational */
>> + if (al3010_get_mode(data) != AL3010_CONFIG_ENABLE)
>> + return -EBUSY;
>> +
>> + ret = al3010_get_lux(data);
>> +
>> + return sprintf(buf, "%d\n", ret);
>> +}
>> +
>> +static IIO_CONST_ATTR(in_illuminance_scale_available, AL3010_SCALE_AVAILABLE);
>> +
>> +static DEVICE_ATTR(illuminance0_input, S_IRUGO, al3010_show_lux, NULL);
>> +
>> +static struct attribute *al3010_attributes[] = {
>> + &dev_attr_illuminance0_input.attr,
>> + &iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group al3010_attribute_group = {
>> + .attrs = al3010_attributes,
>> +};
>> +
>> +static int al3010_init(struct al3010_data *data)
>> +{
>> + int err = 0;
>> +
>> + err = al3010_set_mode(data, AL3010_CONFIG_ENABLE);
>> + if (err) {
>> + dev_err(&data->client->dev,
>> + "%s: al3010_set_mode returned error %d\n",
>> + __func__, err);
>> + return err;
>> + }
>> +
>> + /*
>> + * set sensor range to 4863 lux.
>> + * (If panel luminousness is 10% , the range of pad is 0 ~ 48630 lux.)
>> + */
>> + err = al3010_set_gain(data, AL3010_RANGE_3);
>> + if (err) {
>> + dev_err(&data->client->dev,
>> + "%s: al3010_set_range returned error %d\n",
>> + __func__, err);
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int al3010_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int *val,
>> + int *val2, long mask)
>> +{
>> + struct al3010_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + /*
>> + * ALS ADC value is stored in two adjacent registers:
>> + * - low byte of output is stored at AL3320A_REG_DATA_LOW
>> + * - high byte of output is stored at AL3320A_REG_DATA_LOW + 1
>> + */
>> + ret = i2c_smbus_read_word_data(data->client,
>> + AL3010_REG_DATA_LOW);
>> + if (ret < 0)
>> + return ret;
>> + *val = ret;
>> + return IIO_VAL_INT;
>> + case IIO_CHAN_INFO_SCALE:
>> + ret = i2c_smbus_read_byte_data(data->client,
>> + AL3010_REG_CONFIG);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = (ret & AL3010_GAIN_MASK) >> AL3010_GAIN_SHIFT;
>> + *val = al3010_scales[ret][0];
>> + *val2 = al3010_scales[ret][1];
>> +
>> + return IIO_VAL_INT_PLUS_MICRO;
>> + }
>> + return -EINVAL;
>> +}
>> +
>> +static int al3010_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int val,
>> + int val2, long mask)
>> +{
>> + struct al3010_data *data = iio_priv(indio_dev);
>> + int i;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_SCALE:
>> + for (i = 0; i < ARRAY_SIZE(al3010_scales); i++) {
>> + if (val == al3010_scales[i][0] &&
>> + val2 == al3010_scales[i][1])
>> + return al3010_set_gain(data, i);
>> + }
>> + break;
>> + }
>> + return -EINVAL;
>> +}
>> +
>> +static const struct iio_info al3010_info = {
>> + .driver_module = THIS_MODULE,
>> + .read_raw = al3010_read_raw,
>> + .write_raw = al3010_write_raw,
>> + .attrs = &al3010_attribute_group,
>> +};
>> +
>> +static int al3010_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct al3010_data *data;
>> + struct iio_dev *indio_dev;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + data = iio_priv(indio_dev);
>> + i2c_set_clientdata(client, indio_dev);
>> + data->client = client;
>> +
>> + indio_dev->dev.parent = &client->dev;
>> + indio_dev->info = &al3010_info;
>> + indio_dev->name = AL3010_DRV_NAME;
>> + indio_dev->channels = al3010_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(al3010_channels);
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + ret = al3010_init(data);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "al3010 chip init failed\n");
>> + return ret;
>> + }
>> +
>> + return devm_iio_device_register(&client->dev, indio_dev);
>> +}
>> +
>> +static int al3010_remove(struct i2c_client *client)
>> +{
>> + return i2c_smbus_write_byte_data(client, AL3010_REG_SYSTEM, 0x00);
>> +}
>> +
>> +static const struct i2c_device_id al3010_id[] = {
>> + {"al3010", 0},
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, al3010_id);
>> +
>> +static struct i2c_driver al3010_driver = {
>> + .driver = {
>> + .name = AL3010_DRV_NAME,
>> + },
>> + .probe = al3010_probe,
>> + .remove = al3010_remove,
>> + .id_table = al3010_id,
>> +};
>> +
>> +module_i2c_driver(al3010_driver);
>> +
>> +MODULE_AUTHOR("Rocky Hsiao <rocky.hsiao@dyna-image.com");
>> +MODULE_DESCRIPTION("AL3010 Ambient Light Sensor driver");
>> +MODULE_LICENSE("GPL v2");
>
> This looks pretty similar with al3320a. Can you try to implement the support for
> al3010 inside al3320a.c file?
>
> Avoid duplicating code.
>
>> +
>> diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
>> index 6aac651..4212772 100644
>> --- a/drivers/iio/light/al3320a.c
>> +++ b/drivers/iio/light/al3320a.c
>> @@ -1,16 +1,32 @@
>> /*
>> * AL3320A - Dyna Image Ambient Light Sensor
>> *
>> - * Copyright (c) 2014, Intel Corporation.
>> + * Copyright (C) 2016 Dyna Image Corp.
>
> Please don't remove the copyright from Intel. Just add your copyright notice.
>
>> *
>> - * This file is subject to the terms and conditions of version 2 of
>> - * the GNU General Public License. See the file COPYING in the main
>> - * directory of this archive for more details.
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> *
>> - * IIO driver for AL3320A (7-bit I2C slave address 0x1C).
>> + * This program is distributed in the hope that 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.
>> *
>> - * TODO: interrupt support, thresholds
>> + * Note about the original authors:
>> *
>> + * >> Copyright (c) 2014, Intel Corporation.
>> + *
>> + * >> This file is subject to the terms and conditions of version 2 of
>> + * >> the GNU General Public License. See the file COPYING in the main
>> + * >> directory of this archive for more details.
>> + *
>> + * >> IIO driver for AL3320A (7-bit I2C slave address 0x1C).
>> + *
>> + * >> TODO: interrupt support, thresholds
>> + *
>> + * >> Author:Daniel Baluta <daniel.baluta@intel.com>
>> + *
>> + * The kernel version is 4.4
>> */
>>
>> #include <linux/module.h>
>> @@ -72,9 +88,47 @@ static const struct iio_chan_spec al3320a_channels[] = {
>> }
>> };
>>
>> +static int al3320a_get_adc_value(struct al3320a_data *data)
>> +{
>> + int val;
>> +
>> + val = i2c_smbus_read_word_data(data->client, AL3320A_REG_DATA_LOW);
>> +
>> + return val;
>> +}
>> +
>> +static int al3320a_get_lux(struct al3320a_data *data)
>> +{
>> + int ret;
>> + long ret64;
>> +
>> + ret = al3320a_get_adc_value(data);
>> + ret64 = ret;
>> + ret64 = (ret64 * 32000) / 1000000;
>> + ret = ret64;
>> +
>> + return ret;
>> +}
>> +
>> +static ssize_t al3320a_lux_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct al3320a_data *data = iio_priv(dev_to_iio_dev(dev));
>> + int val;
>> +
>> + val = al3320a_get_lux(data);
>> +
>> + return sprintf(buf, "%d\n", val);
>> +}
>> +
>> +static IIO_DEVICE_ATTR(illuminance0_input, S_IRUGO,
>> + al3320a_lux_show, NULL, 0);
>> +
>> static IIO_CONST_ATTR(in_illuminance_scale_available, AL3320A_SCALE_AVAILABLE);
>>
>> static struct attribute *al3320a_attributes[] = {
>> + &iio_dev_attr_illuminance0_input.dev_attr.attr,
>> &iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
>> NULL,
>> };
>> @@ -125,8 +179,8 @@ static int al3320a_read_raw(struct iio_dev *indio_dev,
>> * - low byte of output is stored at AL3320A_REG_DATA_LOW
>> * - high byte of output is stored at AL3320A_REG_DATA_LOW + 1
>> */
>> - ret = i2c_smbus_read_word_data(data->client,
>> - AL3320A_REG_DATA_LOW);
>> + ret = al3320a_get_adc_value(data);
>> +
>> if (ret < 0)
>> return ret;
>> *val = ret;
>> @@ -201,6 +255,7 @@ static int al3320a_probe(struct i2c_client *client,
>> dev_err(&client->dev, "al3320a chip init failed\n");
>> return ret;
>> }
>> +
>> return devm_iio_device_register(&client->dev, indio_dev);
>> }
>>
>> @@ -227,6 +282,6 @@ static struct i2c_driver al3320a_driver = {
>>
>> module_i2c_driver(al3320a_driver);
>>
>> -MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
>> +MODULE_AUTHOR("Rocky Hsiao <rocky.hsiao@dyna-image.com>");
>
> Again, mark your contribution here. Do not remove initial author :).
>
>> MODULE_DESCRIPTION("AL3320A Ambient Light Sensor driver");
>> MODULE_LICENSE("GPL v2");
>
> I am happy to see Dyna Image starting to do upstream work on their sensors :).
Seconded! Always good to have direct support from the manufacturer where
possible as normally you have better access to information :)
So welcome to IIO Rocky,
Thanks
Jonathan
>
> thanks,
> Daniel.
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Dyna-Image AL3320A update, add AL3010 driver
2016-06-03 8:35 ` Jonathan Cameron
@ 2016-06-03 8:40 ` Jonathan Cameron
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2016-06-03 8:40 UTC (permalink / raw)
To: Daniel Baluta, Rocky Hsiao
Cc: Linux Kernel Mailing List, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Olof Johansson, Filipe Brandenburger,
Brian Norris, Benson Leung, Douglas Anderson, Sonny Rao,
Daniel Kurtz, Adriana Reus, Gwendal Grignou, Matt Ranostay,
Puthikorn Voravootivat, Andreas Dannenberg,
linux-iio@vger.kernel.org, John Huang
On 03/06/16 09:35, Jonathan Cameron wrote:
> On 03/06/16 08:45, Daniel Baluta wrote:
>> Hi Rocky,
>>
>> Few bits inline, before I will take my time to do an in depth review.
>>
>> On Fri, Jun 3, 2016 at 7:02 AM, Rocky Hsiao <rocky.hsiao@dyna-image.com> wrote:
>>> 1. Change al3320a.c to match light sensor test flow.
>>> 2. Add al3010.c to add new device AL3010.
>>> original file copy from al3320a.c
>>
>> Please split this into several patches, each patch implementing one
>> single functionality.
>>
>> Can you add support for AL3010 inside al3320a.c. I don't know exactly
>> how different
>> this sensors are.
>>
>> Do you have a link to the datasheets?
>>
>>>
>>> Signed-off-by: Rocky Hsiao <rocky.hsiao@dyna-image.com>
>>> ---
>>> .../config/x86_64/chromiumos-x86_64.flavour.config | 2 +
>>
>> What tree are you using? Please use Jonathan's linux-iio git tree.
>>
>> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/?h=togreg
>>
>>> drivers/iio/light/Kconfig | 10 +
>>> drivers/iio/light/Makefile | 1 +
>>> drivers/iio/light/al3010.c | 301 +++++++++++++++++++++
>>> drivers/iio/light/al3320a.c | 73 ++++-
>>> 5 files changed, 378 insertions(+), 9 deletions(-)
>>> create mode 100644 drivers/iio/light/al3010.c
>>>
>>> diff --git a/chromeos/config/x86_64/chromiumos-x86_64.flavour.config b/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
>>> index 7d2bed4..7980a14 100644
>>> --- a/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
>>> +++ b/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
>>> @@ -2,6 +2,8 @@
>>> # Config options generated by splitconfig
>>> #
>>> CONFIG_ACERHDF=m
>>> +CONFIG_AL3010=m
>>> +CONFIG_AL3320A=m
>>> CONFIG_B43=m
>>> CONFIG_B43_BCMA=y
>>> CONFIG_B43_BCMA_PIO=y
>>
>> This is part of your distribution OS and it shouldn't be submitted here.
>>
>>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>>> index ca89b26..57a2a7e 100644
>>> --- a/drivers/iio/light/Kconfig
>>> +++ b/drivers/iio/light/Kconfig
>>> @@ -40,6 +40,16 @@ config AL3320A
>>> To compile this driver as a module, choose M here: the
>>> module will be called al3320a.
>>>
>>> +config AL3010
>>> + tristate "AL3010 ambient light sensor"
>>> + depends on I2C
>>> + help
>>> + Say Y here if you want to build a driver for the Dyna Image AL3010
>>> + ambient light sensor.
>>> +
>>> + To compile this driver as a module, choose M here: the
>>> + module will be called al3010.
>>> +
>>> config APDS9300
>>> tristate "APDS9300 ambient light sensor"
>>> depends on I2C
>>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>>> index 5df1118..3f615d7 100644
>>> --- a/drivers/iio/light/Makefile
>>> +++ b/drivers/iio/light/Makefile
>>> @@ -5,6 +5,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_AL3010) += al3010.o
>>> obj-$(CONFIG_AL3320A) += al3320a.o
>>> obj-$(CONFIG_APDS9300) += apds9300.o
>>> obj-$(CONFIG_APDS9960) += apds9960.o
>>> diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
>>> new file mode 100644
>>> index 0000000..2df425b
>>> --- /dev/null
>>> +++ b/drivers/iio/light/al3010.c
>>> @@ -0,0 +1,301 @@
>>> +/*
>>> + * AL3010 - Dyna Image Ambient Light Sensor
>>> + *
>>> + * Copyright (C) 2016 Dyna-Image Corp.
>>> + *
>>> + * This software is licedsed under the terms of the GNU General Public
>>> + * License version 2, as published by the Free Software Foundation, and
>>> + * may be copied, distributed, and modified under those terms.
>>> + *
>>> + * Note about the original authors:
>>> + *
>>> + * This driver is based on the original driver for AL3320A that was distributed
>>> + * by Intel Corporation.
>>> + * The following is part of the header found in that file
>>> + *
>>> + * >> AL3320A - Dyna Image Ambient Light Sensor
>>> + *
>>> + * >> Copyright (c) 2014, Intel Corporation.
>>> + *
>>> + * >> This file is subject to the terms and conditions of version 2 of
>>> + * >> the GNU General Public License. See the file COPYING in the main
>>> + * >> directory of this archive for more details.
>>> + *
>>> + * >> IIO driver for AL3010 (7-bit I2C slave address 0x1C).
>>> + *
>>> + * >> TODO: interrupt support, thresholds
>>> + *
>>> + * >> Author:Daniel Baluta <daniel.baluta@intel.com>
>>> + *
>>> + * The kernel version is 4.4
>>
>> Again please rebase this on Jonathan latest sources as specified above.
> For a new driver it's almost always fine to base on the latest release
> mainline kernel if you would prefer. Any tree wide reworks we can sort
> out when applying the patch. Here, as you are modifying an existing
> driver you may want to check if there are any related series already
> under review or applied to my togreg branch at:
> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/log/?h=togreg
>
> Rule of thumb for branches in that tree is that testing is the very
> latest stuff, but may well rebase so is not a good tree to use for
> new patches. By the time it hits togreg it should be non rebasing and
> hence you should be safe that what you see there is what your patches
> will go on top of.
>
>
>>
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/i2c.h>
>>> +
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +
>>> +#define AL3010_DRV_NAME "al3010"
>>> +
>>> +#define AL3010_REG_SYSTEM 0x00
>>> +#define AL3010_REG_CONFIG 0x10
>>> +#define AL3010_REG_DATA_LOW 0x0c
>>> +
>>> +#define AL3010_GAIN_MASK (BIT(6) | BIT(5) | BIT(4))
>>> +#define AL3010_GAIN_SHIFT 4
>>> +
>>> +#define AL3010_CONFIG_DISABLE 0x00
>>> +#define AL3010_CONFIG_ENABLE 0x01
>>> +
>>> +#define AL3010_SCALE_AVAILABLE "1.1872 0.2968 0.0742 0.0186"
>>> +
>>> +enum al3010_range {
>>> + AL3010_RANGE_1, /* 77806 lx */
>>> + AL3010_RANGE_2, /* 19452 lx */
>>> + AL3010_RANGE_3, /* 4863 lx */
>>> + AL3010_RANGE_4 /* 1216 lx */
>>> +};
>>> +
>>> +static const int al3010_scales[][2] = {
>>> + {0, 1187200}, {0, 296800}, {0, 74200}, {0, 18600}
>>> +};
>>> +
>>> +struct al3010_data {
>>> + struct i2c_client *client;
>>> +};
>>> +
>>> +static const struct iio_chan_spec al3010_channels[] = {
>>> + {
>>> + .type = IIO_LIGHT,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>> + BIT(IIO_CHAN_INFO_SCALE),
>>> + }
>>> +};
>>> +
>>> +static int al3010_set_gain(struct al3010_data *data, int gain)
>>> +{
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
>>> + (gain<<AL3010_GAIN_SHIFT)&AL3010_GAIN_MASK);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int al3010_set_mode(struct al3010_data *data, int mode)
>>> +{
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_SYSTEM, mode);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int al3010_get_mode(struct al3010_data *data)
>>> +{
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_read_byte_data(data->client, AL3010_REG_SYSTEM);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int al3010_get_adc_value(struct al3010_data *data)
>>> +{
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_read_word_data(data->client, AL3010_REG_DATA_LOW);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int al3010_get_lux(struct al3010_data *data)
>>> +{
>>> + int ret;
>>> + long int ret64;
>>> +
>>> + ret = al3010_get_adc_value(data);
>>> + ret64 = ret;
>>> + ret64 = (ret64 * 74200) / 1000000;
>>> + ret = ret64;
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/* lux */
>>> +static ssize_t al3010_show_lux(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct al3010_data *data = iio_priv(dev_to_iio_dev(dev));
>>> + int ret;
>>> +
>>> + /* No LUX data if not operational */
>>> + if (al3010_get_mode(data) != AL3010_CONFIG_ENABLE)
>>> + return -EBUSY;
>>> +
>>> + ret = al3010_get_lux(data);
>>> +
>>> + return sprintf(buf, "%d\n", ret);
>>> +}
>>> +
>>> +static IIO_CONST_ATTR(in_illuminance_scale_available, AL3010_SCALE_AVAILABLE);
>>> +
>>> +static DEVICE_ATTR(illuminance0_input, S_IRUGO, al3010_show_lux, NULL);
>>> +
>>> +static struct attribute *al3010_attributes[] = {
>>> + &dev_attr_illuminance0_input.attr,
>>> + &iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
>>> + NULL,
>>> +};
>>> +
>>> +static const struct attribute_group al3010_attribute_group = {
>>> + .attrs = al3010_attributes,
>>> +};
>>> +
>>> +static int al3010_init(struct al3010_data *data)
>>> +{
>>> + int err = 0;
>>> +
>>> + err = al3010_set_mode(data, AL3010_CONFIG_ENABLE);
>>> + if (err) {
>>> + dev_err(&data->client->dev,
>>> + "%s: al3010_set_mode returned error %d\n",
>>> + __func__, err);
>>> + return err;
>>> + }
>>> +
>>> + /*
>>> + * set sensor range to 4863 lux.
>>> + * (If panel luminousness is 10% , the range of pad is 0 ~ 48630 lux.)
>>> + */
>>> + err = al3010_set_gain(data, AL3010_RANGE_3);
>>> + if (err) {
>>> + dev_err(&data->client->dev,
>>> + "%s: al3010_set_range returned error %d\n",
>>> + __func__, err);
>>> + return err;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int al3010_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan, int *val,
>>> + int *val2, long mask)
>>> +{
>>> + struct al3010_data *data = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + /*
>>> + * ALS ADC value is stored in two adjacent registers:
>>> + * - low byte of output is stored at AL3320A_REG_DATA_LOW
>>> + * - high byte of output is stored at AL3320A_REG_DATA_LOW + 1
>>> + */
>>> + ret = i2c_smbus_read_word_data(data->client,
>>> + AL3010_REG_DATA_LOW);
>>> + if (ret < 0)
>>> + return ret;
>>> + *val = ret;
>>> + return IIO_VAL_INT;
>>> + case IIO_CHAN_INFO_SCALE:
>>> + ret = i2c_smbus_read_byte_data(data->client,
>>> + AL3010_REG_CONFIG);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = (ret & AL3010_GAIN_MASK) >> AL3010_GAIN_SHIFT;
>>> + *val = al3010_scales[ret][0];
>>> + *val2 = al3010_scales[ret][1];
>>> +
>>> + return IIO_VAL_INT_PLUS_MICRO;
>>> + }
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static int al3010_write_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan, int val,
>>> + int val2, long mask)
>>> +{
>>> + struct al3010_data *data = iio_priv(indio_dev);
>>> + int i;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_SCALE:
>>> + for (i = 0; i < ARRAY_SIZE(al3010_scales); i++) {
>>> + if (val == al3010_scales[i][0] &&
>>> + val2 == al3010_scales[i][1])
>>> + return al3010_set_gain(data, i);
>>> + }
>>> + break;
>>> + }
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static const struct iio_info al3010_info = {
>>> + .driver_module = THIS_MODULE,
>>> + .read_raw = al3010_read_raw,
>>> + .write_raw = al3010_write_raw,
>>> + .attrs = &al3010_attribute_group,
>>> +};
>>> +
>>> +static int al3010_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + struct al3010_data *data;
>>> + struct iio_dev *indio_dev;
>>> + int ret;
>>> +
>>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>> + if (!indio_dev)
>>> + return -ENOMEM;
>>> +
>>> + data = iio_priv(indio_dev);
>>> + i2c_set_clientdata(client, indio_dev);
>>> + data->client = client;
>>> +
>>> + indio_dev->dev.parent = &client->dev;
>>> + indio_dev->info = &al3010_info;
>>> + indio_dev->name = AL3010_DRV_NAME;
>>> + indio_dev->channels = al3010_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(al3010_channels);
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> +
>>> + ret = al3010_init(data);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "al3010 chip init failed\n");
>>> + return ret;
>>> + }
>>> +
>>> + return devm_iio_device_register(&client->dev, indio_dev);
>>> +}
>>> +
>>> +static int al3010_remove(struct i2c_client *client)
>>> +{
>>> + return i2c_smbus_write_byte_data(client, AL3010_REG_SYSTEM, 0x00);
>>> +}
>>> +
>>> +static const struct i2c_device_id al3010_id[] = {
>>> + {"al3010", 0},
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, al3010_id);
>>> +
>>> +static struct i2c_driver al3010_driver = {
>>> + .driver = {
>>> + .name = AL3010_DRV_NAME,
>>> + },
>>> + .probe = al3010_probe,
>>> + .remove = al3010_remove,
>>> + .id_table = al3010_id,
>>> +};
>>> +
>>> +module_i2c_driver(al3010_driver);
>>> +
>>> +MODULE_AUTHOR("Rocky Hsiao <rocky.hsiao@dyna-image.com");
>>> +MODULE_DESCRIPTION("AL3010 Ambient Light Sensor driver");
>>> +MODULE_LICENSE("GPL v2");
>>
>> This looks pretty similar with al3320a. Can you try to implement the support for
>> al3010 inside al3320a.c file?
>>
>> Avoid duplicating code.
>>
>>> +
>>> diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
>>> index 6aac651..4212772 100644
>>> --- a/drivers/iio/light/al3320a.c
>>> +++ b/drivers/iio/light/al3320a.c
>>> @@ -1,16 +1,32 @@
>>> /*
>>> * AL3320A - Dyna Image Ambient Light Sensor
>>> *
>>> - * Copyright (c) 2014, Intel Corporation.
>>> + * Copyright (C) 2016 Dyna Image Corp.
>>
>> Please don't remove the copyright from Intel. Just add your copyright notice.
>>
>>> *
>>> - * This file is subject to the terms and conditions of version 2 of
>>> - * the GNU General Public License. See the file COPYING in the main
>>> - * directory of this archive for more details.
>>> + * This software is licensed under the terms of the GNU General Public
>>> + * License version 2, as published by the Free Software Foundation, and
>>> + * may be copied, distributed, and modified under those terms.
>>> *
>>> - * IIO driver for AL3320A (7-bit I2C slave address 0x1C).
>>> + * This program is distributed in the hope that 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.
>>> *
>>> - * TODO: interrupt support, thresholds
>>> + * Note about the original authors:
>>> *
>>> + * >> Copyright (c) 2014, Intel Corporation.
>>> + *
>>> + * >> This file is subject to the terms and conditions of version 2 of
>>> + * >> the GNU General Public License. See the file COPYING in the main
>>> + * >> directory of this archive for more details.
>>> + *
>>> + * >> IIO driver for AL3320A (7-bit I2C slave address 0x1C).
>>> + *
>>> + * >> TODO: interrupt support, thresholds
>>> + *
>>> + * >> Author:Daniel Baluta <daniel.baluta@intel.com>
>>> + *
>>> + * The kernel version is 4.4
>>> */
>>>
>>> #include <linux/module.h>
>>> @@ -72,9 +88,47 @@ static const struct iio_chan_spec al3320a_channels[] = {
>>> }
>>> };
>>>
>>> +static int al3320a_get_adc_value(struct al3320a_data *data)
>>> +{
>>> + int val;
>>> +
>>> + val = i2c_smbus_read_word_data(data->client, AL3320A_REG_DATA_LOW);
>>> +
>>> + return val;
>>> +}
>>> +
>>> +static int al3320a_get_lux(struct al3320a_data *data)
>>> +{
>>> + int ret;
>>> + long ret64;
>>> +
>>> + ret = al3320a_get_adc_value(data);
>>> + ret64 = ret;
>>> + ret64 = (ret64 * 32000) / 1000000;
>>> + ret = ret64;
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static ssize_t al3320a_lux_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct al3320a_data *data = iio_priv(dev_to_iio_dev(dev));
>>> + int val;
>>> +
>>> + val = al3320a_get_lux(data);
>>> +
>>> + return sprintf(buf, "%d\n", val);
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(illuminance0_input, S_IRUGO,
>>> + al3320a_lux_show, NULL, 0);
>>> +
>>> static IIO_CONST_ATTR(in_illuminance_scale_available, AL3320A_SCALE_AVAILABLE);
>>>
>>> static struct attribute *al3320a_attributes[] = {
>>> + &iio_dev_attr_illuminance0_input.dev_attr.attr,
>>> &iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
>>> NULL,
>>> };
>>> @@ -125,8 +179,8 @@ static int al3320a_read_raw(struct iio_dev *indio_dev,
>>> * - low byte of output is stored at AL3320A_REG_DATA_LOW
>>> * - high byte of output is stored at AL3320A_REG_DATA_LOW + 1
>>> */
>>> - ret = i2c_smbus_read_word_data(data->client,
>>> - AL3320A_REG_DATA_LOW);
>>> + ret = al3320a_get_adc_value(data);
>>> +
>>> if (ret < 0)
>>> return ret;
>>> *val = ret;
>>> @@ -201,6 +255,7 @@ static int al3320a_probe(struct i2c_client *client,
>>> dev_err(&client->dev, "al3320a chip init failed\n");
>>> return ret;
>>> }!
>>> +
>>> return devm_iio_device_register(&client->dev, indio_dev);
>>> }
>>>
>>> @@ -227,6 +282,6 @@ static struct i2c_driver al3320a_driver = {
>>>
>>> module_i2c_driver(al3320a_driver);
>>>
>>> -MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
>>> +MODULE_AUTHOR("Rocky Hsiao <rocky.hsiao@dyna-image.com>");
>>
>> Again, mark your contribution here. Do not remove initial author :).
>>
>>> MODULE_DESCRIPTION("AL3320A Ambient Light Sensor driver");
>>> MODULE_LICENSE("GPL v2");
>>
>> I am happy to see Dyna Image starting to do upstream work on their sensors :).
> Seconded! Always good to have direct support from the manufacturer where
> possible as normally you have better access to information :)
>
> So welcome to IIO Rocky,
Just been browsing the Dyna Image website. You guys have some very cool sensors.
*cross fingers that this is the start of mainline support for more devices*.
Not many datasheets though that I can immediately locate.
Is dyna-image likely to be willing to release them to reviewers (more generally is
of course even better). Makes review much easier / more thorough. Note there
are structures set up to do NDAs etc if needed (though I'm sure we all prefer not!).
Jonathan
>
> Thanks
> Jonathan
>>
>> thanks,
>> Daniel.
>>
>
> --
> 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] 6+ messages in thread
* Re: [PATCH] Dyna-Image AL3320A update, add AL3010 driver
2016-06-03 4:02 [PATCH] Dyna-Image AL3320A update, add AL3010 driver Rocky Hsiao
2016-06-03 7:45 ` Daniel Baluta
@ 2016-06-03 7:51 ` Benson Leung
2016-06-03 8:20 ` Lars-Peter Clausen
2 siblings, 0 replies; 6+ messages in thread
From: Benson Leung @ 2016-06-03 7:51 UTC (permalink / raw)
To: Rocky Hsiao
Cc: linux-kernel@vger.kernel.org, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Olof Johansson,
Filipe Brandenburger, Brian Norris, Douglas Anderson, Sonny Rao,
Daniel Kurtz, Daniel Baluta, Adriana Reus, Gwendal Grignou,
Matt Ranostay, Puthikorn Voravootivat, Andreas Dannenberg,
linux-iio, John Huang
Hi Rocky,
On Fri, Jun 3, 2016 at 6:02 AM, Rocky Hsiao <rocky.hsiao@dyna-image.com> wrote:
>
> 1. Change al3320a.c to match light sensor test flow.
> 2. Add al3010.c to add new device AL3010.
> original file copy from al3320a.c
>
> Signed-off-by: Rocky Hsiao <rocky.hsiao@dyna-image.com>
> ---
> .../config/x86_64/chromiumos-x86_64.flavour.config | 2 +
> drivers/iio/light/Kconfig | 10 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/al3010.c | 301 +++++++++++++++++++++
> drivers/iio/light/al3320a.c | 73 ++++-
> 5 files changed, 378 insertions(+), 9 deletions(-)
> create mode 100644 drivers/iio/light/al3010.c
>
> diff --git a/chromeos/config/x86_64/chromiumos-x86_64.flavour.config b/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
> index 7d2bed4..7980a14 100644
> --- a/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
> +++ b/chromeos/config/x86_64/chromiumos-x86_64.flavour.config
This looks like this patch was based on a chromeos-kernel. As was
mentioned by Daniel, don't do this for submissions to linux-kernel or
linux-iio. Base this on the linux-iio tree.
--
Benson Leung
Senior Software Engineer, Chrom* OS
bleung@chromium.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Dyna-Image AL3320A update, add AL3010 driver
2016-06-03 4:02 [PATCH] Dyna-Image AL3320A update, add AL3010 driver Rocky Hsiao
2016-06-03 7:45 ` Daniel Baluta
2016-06-03 7:51 ` Benson Leung
@ 2016-06-03 8:20 ` Lars-Peter Clausen
2 siblings, 0 replies; 6+ messages in thread
From: Lars-Peter Clausen @ 2016-06-03 8:20 UTC (permalink / raw)
To: Rocky Hsiao, linux-kernel
Cc: Jonathan Cameron, Hartmut Knaack, Peter Meerwald, Olof Johansson,
Filipe Brandenburger, Brian Norris, Benson Leung,
Douglas Anderson, Sonny Rao, Daniel Kurtz, Daniel Baluta,
Adriana Reus, Gwendal Grignou, Matt Ranostay,
Puthikorn Voravootivat, Andreas Dannenberg, linux-iio, John Huang
On 06/03/2016 06:02 AM, Rocky Hsiao wrote:
> 1. Change al3320a.c to match light sensor test flow.
> 2. Add al3010.c to add new device AL3010.
> original file copy from al3320a.c
Hi,
Thanks for the patch. In addition to what Daniel said, for upstream patch
submissions it is important to have proper commit messages explaining what
is done, why it is done and how it is done. The commit messages should
follow the general style of the subsystem you are sending the patch to. E.g.
in the IIO case this means that the subject line should start with "iio: ...".
http://chris.beams.io/posts/git-commit/ is a very good document that
describes how to create a good commit message and I recommend reading it.
- Lars
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-03 8:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-03 4:02 [PATCH] Dyna-Image AL3320A update, add AL3010 driver Rocky Hsiao
2016-06-03 7:45 ` Daniel Baluta
2016-06-03 8:35 ` Jonathan Cameron
2016-06-03 8:40 ` Jonathan Cameron
2016-06-03 7:51 ` Benson Leung
2016-06-03 8:20 ` Lars-Peter Clausen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox