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

[Jonathan Cameron]

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,

Thanks
Jonathan
> 
> thanks,
> Daniel.
>