On 03/06/16 11:03, Rocky Hsiao wrote: > This driver is based on drivers/iio/light/al3320a.c > > Differences form the original file: > - Difference REG map > - Difference scale > > Signed-off-by: Rocky Hsiao <rocky.hsiao@xxxxxxxxxxxxxx> I suspect this would be better integrated as an additional part within the existing al3320.c driver. I'll highlight how below. Note that I'm highlighting it in here as your code was convenient but you want to do the equivalent in the al3320a driver rather than bring that in here. Standard trick really - used to integrate lots of similar parts with slightly different register maps. Usually the underlying hardware parts from a given manufacturer are similar enough in how they interact with the driver to make this worthwhile. Give it a go and I think you'll find the result is actually very compact and still easy to read. Jonathan > --- > drivers/iio/light/Kconfig | 10 ++ > drivers/iio/light/Makefile | 1 + > drivers/iio/light/al3010.c | 292 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 303 insertions(+) > create mode 100644 drivers/iio/light/al3010.c > > 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..84da37c > --- /dev/null > +++ b/drivers/iio/light/al3010.c > @@ -0,0 +1,292 @@ > +/* > + * AL3010A - 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 > + * > + * This driver is base on the original driver for AL3320A that was distributed > + * by Intel Corporation. > + * > + * last change: 2016/06/03 Git will store this - no need to list here (and almost certain to rapidly get out of date!) > + * editor: Rocky Hsiao <rocky.hsiao@xxxxxxxxxxxxxx> > + * > + * Datasheet: > + * http://www.dyna-image.com/english/product/optical-sensor-detail.php?cpid=1&dpid=10#doc > + */ > + > +#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 So this is the same as AL3320A_REG_CONFIG which should make life easier ;) > +#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} > +}; > + So to add support to the existing driver looks reasonably straight forward (and should give less code to maintain long term which is good for us all. You want something along the lines of enum al3320a_chip { AL3320a, AL3010 }; struct al3320a_chip_info { int (*set_gain)(struct al3320a_data *data, int gain); int (*get_gain)(struct al3320a_data *data, int *val, int *val2); const int **available_gains; int dataaddress; } al3320a_chips [] = { [AL3320a] = { .set_gain = al3320a_set_gain, .get_gain = al3320a_get_gain, .available_gains = al3320a_scales, .dataaddress = AL3020a_REG_DATA_LOW, }, [AL3010] = { .set_gain = al3310_set_gain, .get_gain = al3310_get_gain, .available_gains = al3310_scales, .dataaddress = al3310_REG_DATA_LOW }, }; Will not where changes are needed lower down as well to use this. > +struct al3010_data { > + struct i2c_client *client; Add struct al3320a_chip_info *chip_info; > +}; > + > +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); So we have a differnet register and location here between the drivers. Callback or some values stored in data structure doesn't really matter. > + > + return ret; > +} > + Little clarity as added by these three wrappers so drop them in favour of having the calls inlien. > +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, Same comments on this as for patch 1. > + &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); Same as for al3320a > + 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; > + } This is different, but we already concluded we need a callback for this. data->chip_info->set_gain(...) > + > + 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); So the address is different but otherwise same as al3320a ret = i2c_smbus_read_word_data(data->client, data->chip_info->dataaddress); > + if (ret < 0) > + return ret; > + *val = ret; > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: This is different between the drivers - simplest approach will be a callback in the data structure. return data->chip_info->get_gain(data, val, val2); (put rest of this into the appropriate get_gain function for each device). > + 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); So the scales are diffent... and so is the write function. We need a callback to set the scale for each device. Something like. case IIO_CHAN_INFO_SCALE: //conveniently they have the same number of options - if // not you'll need a count field as well in our chip_info for (i = 0; i < ARRAY_SIZE(al3010_scales); i++) { if (val == data->chip_info->avialble_gains[i][0] && val2 == data->chip_info->available_gains[i][1]) return data->chip_info->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; > + This line will setup the right chip callbacks. data->chip_info = al3320a_chips[client->data]; > + 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); Provide a define for the 0x00 as it is otherwise a 'magic' number. Note this is the same as for the al3320a.c driver so no need to have any difference here. > +} > + > +static const struct i2c_device_id al3010_id[] = { > + {"al3010", 0}, To combine drivers. { "al3010", AL3010 }, { "al3020a", AL3020a }, > + {} > +}; > +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"); > + > -- 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
