Re: [PATCH] Dyna-Image AL3320A update, add AL3010 driver

From: Daniel Baluta
Date: Fri Jun 03 2016 - 03:45:49 EST

Next message: Peter Chen: "Re: [PATCH v10 2/7] usb: mux: add generic code for dual role port mux"
Previous message: M'boumba Cedric Madianga: "Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver"
In reply to: Rocky Hsiao: "[PATCH] Dyna-Image AL3320A update, add AL3010 driver"
Next in thread: Jonathan Cameron: "Re: [PATCH] Dyna-Image AL3320A update, add AL3010 driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> .../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@xxxxxxxxx>
> + *
> + * 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@xxxxxxxxxxxxxx");
> +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@xxxxxxxxx>
> + *
> + * 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@xxxxxxxxx>");
> +MODULE_AUTHOR("Rocky Hsiao <rocky.hsiao@xxxxxxxxxxxxxx>");

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.

Next message: Peter Chen: "Re: [PATCH v10 2/7] usb: mux: add generic code for dual role port mux"
Previous message: M'boumba Cedric Madianga: "Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver"
In reply to: Rocky Hsiao: "[PATCH] Dyna-Image AL3320A update, add AL3010 driver"
Next in thread: Jonathan Cameron: "Re: [PATCH] Dyna-Image AL3320A update, add AL3010 driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]