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@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. > 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@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 :). > 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
