| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 3 Jun 2016 09:40:50 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Daniel Baluta <daniel.baluta@...el.com>,
Rocky Hsiao <rocky.hsiao@...a-image.com>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald <pmeerw@...erw.net>,
Olof Johansson <olofj@...omium.org>,
Filipe Brandenburger <filbranden@...omium.org>,
Brian Norris <briannorris@...omium.org>,
Benson Leung <bleung@...omium.org>,
Douglas Anderson <dianders@...omium.org>,
Sonny Rao <sonnyrao@...omium.org>,
Daniel Kurtz <djkurtz@...omium.org>,
Adriana Reus <adriana.reus@...el.com>,
Gwendal Grignou <gwendal@...omium.org>,
Matt Ranostay <mranostay@...il.com>,
Puthikorn Voravootivat <puthik@...omium.org>,
Andreas Dannenberg <dannenberg@...com>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
John Huang <john.huang@...a-image.com>
Subject: Re: [PATCH] Dyna-Image AL3320A update, add AL3010 driver
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@...a-image.com> 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@...a-image.com>
>>> ---
>>> .../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@...el.com>
>>> + *
>>> + * 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@...a-image.com");
>>> +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@...el.com>
>>> + *
>>> + * 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@...el.com>");
>>> +MODULE_AUTHOR("Rocky Hsiao <rocky.hsiao@...a-image.com>");
>>
>> 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@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Powered by blists - more mailing lists