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

[Jonathan Cameron]

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
>