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. -- 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
