From: Jonathan Cameron <jonathan.cameron@huawei.com>
To: Maxime Roussin-Belanger <maxime.roussinbelanger@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
Hartmut Knaack <knaack.h@gmx.de>,
Lars-Peter Clausen <lars@metafoo.de>,
"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
"jeff.dagenais@gmail.com" <jeff.dagenais@gmail.com>
Subject: Re: [PATCH v2] iio: light: introduce si1133
Date: Fri, 6 Jul 2018 18:36:35 +0100 [thread overview]
Message-ID: <20180706183635.00007c43@huawei.com> (raw)
In-Reply-To: <20180705134901.vrrfq7lqcyjah2wa@mbedesk.sonatest.net>
On Thu, 5 Jul 2018 09:49:02 -0400
Maxime Roussin-Belanger <maxime.roussinbelanger@gmail.com> wrote:
> On Sat, Jun 30, 2018 at 06:18:54PM +0100, Jonathan Cameron wrote:
> > On Tue, 26 Jun 2018 13:01:09 -0400
> > Maxime Roussin-B=E9langer <maxime.roussinbelanger@gmail.com> wrote:
> >=20
> > Hi Maxime
> >=20
> > Coming together nicely though a few minor things inline to add to Peter=
's
> > review.
> >=20
> > It would make the device a lot more useful if we could provide a lux
> > measurement. There is code out there to do this from the IR and ambien=
t + IR
> > channels, but licensing is going to be a potential issue. See inline.
> >=20
> > Jonathan
> > =20
> > > Signed-off-by: Maxime Roussin-B=E9langer <maxime.roussinbelanger@gmai=
l.com>
> > > Reviewed-by: Jean-Francois Dagenais <dagenaisj@sonatest.com>
> > > ---
> > > Changes in v2:
> > > - Add ABI documentation
> > > - Drop part abstraction
> > > - Clean up error handling style
> > >=20
> > > .../ABI/testing/sysfs-bus-iio-light-si1133 | 57 ++
> > > drivers/iio/light/Kconfig | 11 +
> > > drivers/iio/light/Makefile | 1 +
> > > drivers/iio/light/si1133.c | 809 ++++++++++++++++=
++
> > > 4 files changed, 878 insertions(+)
> > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-light-si1=
133
> > > create mode 100644 drivers/iio/light/si1133.c
> > >=20
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-light-si1133 b/D=
ocumentation/ABI/testing/sysfs-bus-iio-light-si1133
> > > new file mode 100644
> > > index 000000000000..4533b5699c87
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-light-si1133
> > > @@ -0,0 +1,57 @@
> > > +What: /sys/bus/iio/devices/iio:deviceX/in_intensity0_ir_small_raw
> > > +KernelVersion: 4.18
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + Unit-less infrared intensity. The intensity is measured from 1
> > > + dark photodiode. "small" indicate the surface area capturing
> > > + infrared.
> > > +
> > > +What: /sys/bus/iio/devices/iio:deviceX/in_intensity1_ir_med_raw
> > > +KernelVersion: 4.18
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + Unit-less infrared intensity. The intensity is measured from 2
> > > + dark photodiode. "med" indicate the surface area capturing
> > > + infrared.
> > > +
> > > +What: /sys/bus/iio/devices/iio:deviceX/in_intensity2_ir_large_raw
> > > +KernelVersion: 4.18
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + Unit-less infrared intensity. The intensity is measured from 4
> > > + dark photodiode. "large" indicate the surface area capturing
> > > + infrared. =20
> >=20
> > I'll admit I'm lost. Why would we use the different numbers of photodi=
odes?
> > I get that the device supports them, but if there is a combination that=
is=20
> > normally right, I'd like that one to be the one we don't use an extende=
d name
> > for (so generic code might look at it)
> > =20
> > > +
> > > +What: /sys/bus/iio/devices/iio:deviceX/in_intensity11_raw
> > > +KernelVersion: 4.18
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + Get the current intensity of visible light which is influenced
> > > + by Infrared light. If an accurate lux measurement is desired,
> > > + the extra IR response of the visible-light photodiode must be
> > > + compensated.
> > > +
> > > +What: /sys/bus/iio/devices/iio:deviceX/in_intensity13_large_raw
> > > +KernelVersion: 4.18
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + Get the current intensity of visible light with more diodes than
> > > + the non-large version which is influenced by Infrared light. =20
> >=20
> > This sentence is confusing as the 'more diodes bit' is inserted in the =
middle
> > and it sounds a bit like that makes it influenced by IR. I would make =
it two
> > sentences.
> > =20
> > > + If an accurate lux measurement is desired, the extra IR response
> > > + of the visible-light photodiode must be compensated.
> > > +
> > > +What: /sys/bus/iio/devices/iio:deviceX/in_uvindex24_raw
> > > +KernelVersion: 4.18
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + UV light intensity index measuring the human skin's response to
> > > + different wavelength of sunlight weighted according to the
> > > + standardised CIE Erythemal Action Spectrum. =20
> >=20
> > This should be a standard bit of ABI. Please check if the generic docs
> > are good enough as it is best to avoid repetition.
> > =20
> > > +
> > > +What: /sys/bus/iio/devices/iio:deviceX/in_uvindex25_deep_raw
> > > +KernelVersion: 4.18
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + UV light intensity index measuring the human skin's response to
> > > + deep ultraviolet (DUV) wavelength of sunlight weighted according
> > > + to the standardised CIE Erythemal Action Spectrum. =20
> >=20
> > I think we need a new modifier for this one. Please propose something =
suitable.
> > I doubt it'll be the last time we see a sensor measuring this!
> > =20
> > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > > index c7ef8d1862d6..dfa3b1f956f8 100644
> > > --- a/drivers/iio/light/Kconfig
> > > +++ b/drivers/iio/light/Kconfig
> > > @@ -332,6 +332,17 @@ config SI1145
> > > To compile this driver as a module, choose M here: the module wil=
l be
> > > called si1145.
> > > =20
> > > +config SI1133
> > > + tristate "SI1133 UV Index Sensor and Ambient Light Sensor"
> > > + depends on I2C
> > > + select REGMAP_I2C
> > > + help
> > > + Say Y here if you want to build a driver for the Silicon Labs SI1=
133
> > > + UV Index Sensor and Ambient Light Sensor chip.
> > > +
> > > + To compile this driver as a module, choose M here: the module wil=
l be
> > > + called si1133.
> > > +
> > > config STK3310
> > > tristate "STK3310 ALS and proximity sensor"
> > > depends on I2C
> > > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > > index 80943af5d627..dd9ffc38beeb 100644
> > > --- a/drivers/iio/light/Makefile
> > > +++ b/drivers/iio/light/Makefile
> > > @@ -33,6 +33,7 @@ obj-$(CONFIG_PA12203001) +=3D pa12203001.o
> > > obj-$(CONFIG_RPR0521) +=3D rpr0521.o
> > > obj-$(CONFIG_SENSORS_TSL2563) +=3D tsl2563.o
> > > obj-$(CONFIG_SI1145) +=3D si1145.o
> > > +obj-$(CONFIG_SI1133) +=3D si1133.o
> > > obj-$(CONFIG_STK3310) +=3D stk3310.o
> > > obj-$(CONFIG_ST_UVIS25) +=3D st_uvis25_core.o
> > > obj-$(CONFIG_ST_UVIS25_I2C) +=3D st_uvis25_i2c.o
> > > diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
> > > new file mode 100644
> > > index 000000000000..6cb7fd8c35e4
> > > --- /dev/null
> > > +++ b/drivers/iio/light/si1133.c
> > > @@ -0,0 +1,809 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * si1133.c - Support for Silabs SI1133 combined ambient
> > > + * light and UV index sensors
> > > + *
> > > + * Copyright 2018 Maxime Roussin-Belanger <maxime.roussinbelanger@gm=
ail.com>
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +
> > > +#include <linux/util_macros.h>
> > > +
> > > +#define SI1133_REG_PART_ID 0x00
> > > +#define SI1133_REG_REV_ID 0x01
> > > +#define SI1133_REG_MFR_ID 0x02
> > > +#define SI1133_REG_INFO0 0x03
> > > +#define SI1133_REG_INFO1 0x04
> > > +
> > > +#define SI1133_PART_ID 0x33
> > > +
> > > +#define SI1133_REG_HOSTIN0 0x0A
> > > +#define SI1133_REG_COMMAND 0x0B
> > > +#define SI1133_REG_IRQ_ENABLE 0x0F
> > > +#define SI1133_REG_RESPONSE1 0x10
> > > +#define SI1133_REG_RESPONSE0 0x11
> > > +#define SI1133_REG_IRQ_STATUS 0x12
> > > +#define SI1133_REG_MEAS_RATE 0x1A
> > > +
> > > +#define SI1133_CMD_RESET_CTR 0x00
> > > +#define SI1133_CMD_RESET_SW 0x01
> > > +#define SI1133_CMD_FORCE 0x11
> > > +#define SI1133_CMD_START_AUTONOMOUS 0x13
> > > +#define SI1133_CMD_PARAM_SET 0x80
> > > +#define SI1133_CMD_PARAM_QUERY 0x40
> > > +#define SI1133_CMD_PARAM_MASK 0x3F
> > > +
> > > +#define SI1133_CMD_ERR_MASK BIT(4)
> > > +#define SI1133_CMD_SEQ_MASK 0xF
> > > +
> > > +#define SI1133_PARAM_REG_CHAN_LIST 0x01
> > > +#define SI1133_PARAM_REG_ADCCONFIG(x) (((x) * 4) + 2)
> > > +#define SI1133_PARAM_REG_ADCSENS(x) (((x) * 4) + 3)
> > > +
> > > +#define SI1133_ADCMUX_MASK 0x1F
> > > +#define SI1133_ADCSENS_SCALE_MASK 0x70
> > > +#define SI1133_ADCSENS_SCALE_SHIFT 4
> > > +
> > > +#define SI1133_ADCSENS_HSIG_MASK 0x80
> > > +#define SI1133_ADCSENS_HSIG_SHIFT 7
> > > +
> > > +#define SI1133_ADCSENS_HW_GAIN_MASK 0xF
> > > +
> > > +#define SI1133_PARAM_ADCMUX_SMALL_IR 0x0
> > > +#define SI1133_PARAM_ADCMUX_MED_IR 0x1
> > > +#define SI1133_PARAM_ADCMUX_LARGE_IR 0x2
> > > +#define SI1133_PARAM_ADCMUX_WHITE 0xB
> > > +#define SI1133_PARAM_ADCMUX_LARGE_WHITE 0xD
> > > +#define SI1133_PARAM_ADCMUX_UV 0x18
> > > +#define SI1133_PARAM_ADCMUX_UV_DEEP 0x19
> > > +
> > > +#define SI1133_ERR_INVALID_CMD 0x0
> > > +#define SI1133_ERR_INVALID_LOCATION_CMD 0x1
> > > +#define SI1133_ERR_SATURATION_ADC_OR_OVERFLOW_ACCUMULATION 0x2
> > > +#define SI1133_ERR_OUTPUT_BUFFER_OVERFLOW 0x3
> > > +
> > > +#define SI1133_CMD_MINSLEEP_US_LOW 5000
> > > +#define SI1133_CMD_MINSLEEP_US_HIGH 7500
> > > +#define SI1133_CMD_TIMEOUT_MS 25
> > > +#define SI1133_MAX_RETRIES 25
> > > +
> > > +#define SI1133_REG_HOSTOUT(x) ((x) + 0x13)
> > > +
> > > +#define SI1133_MAX_CMD_CTR 0xF
> > > +
> > > +#define SI1133_MEASUREMENT_FREQUENCY 1250
> > > +
> > > +static const int si1133_scale_available[] =3D {
> > > + 1, 2, 4, 8, 16, 32, 64, 128};
> > > +
> > > +static IIO_CONST_ATTR(scale_available, "1 2 4 8 16 32 64 128");
> > > +
> > > +static IIO_CONST_ATTR_INT_TIME_AVAIL("0.0244 0.0488 0.0975 0.195 0.3=
90 0.780 "
> > > + "1.560 3.120 6.24 12.48 25.0 50.0");
> > > +/* Integration time in milliseconds, nanoseconds */
> > > +static const int si1133_int_time_table[][2] =3D {
> > > + {0, 24400}, {0, 48800}, {0, 97500}, {0, 195000},
> > > + {0, 390000}, {0, 780000}, {1, 560000}, {3, 120000},
> > > + {6, 240000}, {12, 480000}, {25, 000000}, {50, 000000},
> > > +};
> > > +
> > > +static const struct regmap_range si1133_reg_ranges[] =3D {
> > > + regmap_reg_range(0x00, 0x02),
> > > + regmap_reg_range(0x0A, 0x0B),
> > > + regmap_reg_range(0x0F, 0x0F),
> > > + regmap_reg_range(0x10, 0x12),
> > > + regmap_reg_range(0x13, 0x2C),
> > > +};
> > > +
> > > +static const struct regmap_range si1133_reg_ro_ranges[] =3D {
> > > + regmap_reg_range(0x00, 0x02),
> > > + regmap_reg_range(0x10, 0x2C),
> > > +};
> > > +
> > > +static const struct regmap_range si1133_precious_ranges[] =3D {
> > > + regmap_reg_range(0x12, 0x12),
> > > +};
> > > +
> > > +static const struct regmap_access_table si1133_write_ranges_table =
=3D {
> > > + .yes_ranges =3D si1133_reg_ranges,
> > > + .n_yes_ranges =3D ARRAY_SIZE(si1133_reg_ranges),
> > > + .no_ranges =3D si1133_reg_ro_ranges,
> > > + .n_no_ranges =3D ARRAY_SIZE(si1133_reg_ro_ranges),
> > > +};
> > > +
> > > +static const struct regmap_access_table si1133_read_ranges_table =3D=
{
> > > + .yes_ranges =3D si1133_reg_ranges,
> > > + .n_yes_ranges =3D ARRAY_SIZE(si1133_reg_ranges),
> > > +};
> > > +
> > > +static const struct regmap_access_table si1133_precious_table =3D {
> > > + .yes_ranges =3D si1133_precious_ranges,
> > > + .n_yes_ranges =3D ARRAY_SIZE(si1133_precious_ranges),
> > > +};
> > > +
> > > +static const struct regmap_config si1133_regmap_config =3D {
> > > + .reg_bits =3D 8,
> > > + .val_bits =3D 8,
> > > +
> > > + .max_register =3D 0x2C,
> > > +
> > > + .wr_table =3D &si1133_write_ranges_table,
> > > + .rd_table =3D &si1133_read_ranges_table,
> > > +
> > > + .precious_table =3D &si1133_precious_table,
> > > +};
> > > +
> > > +struct si1133_data {
> > > + struct regmap *regmap;
> > > + struct i2c_client *client;
> > > +
> > > + /* Lock protecting one command at a time can be processed */
> > > + struct mutex mutex;
> > > +
> > > + int rsp_seq;
> > > + unsigned long scan_mask;
> > > + unsigned int adc_sens;
> > > + unsigned int adc_config;
> > > + unsigned int meas_rate;
> > > +
> > > + struct completion completion;
> > > +};
> > > +
> > > +/**
> > > + * si1133_cmd_reset_sw() - Reset the chip
> > > + *
> > > + * Wait till command reset completes or timeout
> > > + */
> > > +static int si1133_cmd_reset_sw(struct si1133_data *data)
> > > +{
> > > + struct device *dev =3D &data->client->dev;
> > > + unsigned int resp;
> > > + unsigned long timeout;
> > > + int err;
> > > +
> > > + err =3D regmap_write(data->regmap, SI1133_REG_COMMAND,
> > > + SI1133_CMD_RESET_SW);
> > > + if (err)
> > > + return err;
> > > +
> > > + timeout =3D jiffies + msecs_to_jiffies(SI1133_CMD_TIMEOUT_MS);
> > > + while (true) {
> > > + err =3D regmap_read(data->regmap, SI1133_REG_RESPONSE0, &resp);
> > > + if (err =3D=3D -ENXIO) {
> > > + usleep_range(SI1133_CMD_MINSLEEP_US_LOW,
> > > + SI1133_CMD_MINSLEEP_US_HIGH);
> > > + continue;
> > > + }
> > > +
> > > + if ((resp & SI1133_MAX_CMD_CTR) =3D=3D SI1133_MAX_CMD_CTR)
> > > + break;
> > > +
> > > + if (time_after(jiffies, timeout)) {
> > > + dev_warn(dev, "timeout on reset ctr resp: %d\n", resp);
> > > + return -ETIMEDOUT;
> > > + }
> > > + }
> > > +
> > > + if (!err)
> > > + data->rsp_seq =3D SI1133_MAX_CMD_CTR;
> > > +
> > > + return err;
> > > +}
> > > +
> > > +static int si1133_parse_response_err(struct device *dev, unsigned in=
t resp,
> > > + u8 cmd)
> > > +{
> > > + int ret =3D 0;
> > > +
> > > + resp &=3D 0xF;
> > > +
> > > + switch (resp) {
> > > + case SI1133_ERR_OUTPUT_BUFFER_OVERFLOW:
> > > + dev_warn(dev, "Output buffer overflow: %#02hhx\n", cmd);
> > > + ret =3D -EOVERFLOW;
> > > + break;
> > > + case SI1133_ERR_SATURATION_ADC_OR_OVERFLOW_ACCUMULATION:
> > > + dev_warn(dev, "Saturation of the ADC or overflow of accumulation: =
%#02hhx\n",
> > > + cmd);
> > > + ret =3D -EOVERFLOW;
> > > + break;
> > > + case SI1133_ERR_INVALID_LOCATION_CMD:
> > > + dev_warn(dev, "Parameter access to an invalid location: %#02hhx\n",
> > > + cmd);
> > > + ret =3D -EINVAL;
> > > + break;
> > > + case SI1133_ERR_INVALID_CMD:
> > > + dev_warn(dev, "Invalid command %#02hhx\n", cmd);
> > > + ret =3D -EINVAL; =20
> > return -EINVAL... Lots of these above.
> > =20
> > > + break;
> > > + default:
> > > + dev_warn(dev, "Unknown error %#02hhx\n", cmd);
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > +static int si1133_cmd_reset_counter(struct si1133_data *data)
> > > +{
> > > + int err =3D regmap_write(data->regmap, SI1133_REG_COMMAND,
> > > + SI1133_CMD_RESET_CTR);
> > > + if (err)
> > > + return err;
> > > +
> > > + data->rsp_seq =3D 0;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int si1133_command(struct si1133_data *data, u8 cmd)
> > > +{
> > > + struct device *dev =3D &data->client->dev;
> > > + unsigned int resp;
> > > + int err;
> > > + int expected_seq;
> > > +
> > > + mutex_lock(&data->mutex);
> > > +
> > > + expected_seq =3D (data->rsp_seq + 1) & SI1133_MAX_CMD_CTR;
> > > +
> > > + err =3D regmap_write(data->regmap, SI1133_REG_COMMAND, cmd);
> > > + if (err) {
> > > + dev_warn(dev, "failed to write command %#02hhx, ret=3D%d\n", cmd,
> > > + err);
> > > + goto out;
> > > + }
> > > +
> > > + err =3D regmap_read_poll_timeout(data->regmap, SI1133_REG_RESPONSE0=
, resp,
> > > + (resp & SI1133_CMD_SEQ_MASK) =3D=3D
> > > + expected_seq ||
> > > + (resp & SI1133_CMD_ERR_MASK),
> > > + SI1133_CMD_MINSLEEP_US_LOW,
> > > + SI1133_CMD_TIMEOUT_MS * 1000);
> > > +
> > > + if (resp & SI1133_CMD_ERR_MASK) {
> > > + err =3D si1133_parse_response_err(dev, resp, cmd);
> > > + si1133_cmd_reset_counter(data);
> > > + } else {
> > > + data->rsp_seq =3D resp & SI1133_CMD_SEQ_MASK;
> > > + }
> > > +
> > > +out:
> > > + mutex_unlock(&data->mutex);
> > > +
> > > + return err;
> > > +}
> > > +
> > > +static int si1133_param_set(struct si1133_data *data, u8 param,
> > > + unsigned int value)
> > > +{
> > > + int err =3D regmap_write(data->regmap, SI1133_REG_HOSTIN0, value);
> > > +
> > > + if (err)
> > > + return err;
> > > +
> > > + return si1133_command(data, SI1133_CMD_PARAM_SET |
> > > + (param & SI1133_CMD_PARAM_MASK));
> > > +}
> > > +
> > > +static int si1133_param_query(struct si1133_data *data, u8 param,
> > > + unsigned int *result)
> > > +{
> > > + int err =3D si1133_command(data, SI1133_CMD_PARAM_QUERY |
> > > + (param & SI1133_CMD_PARAM_MASK));
> > > + if (err)
> > > + return err;
> > > +
> > > + return regmap_read(data->regmap, SI1133_REG_RESPONSE1, result);
> > > +}
> > > +
> > > +#define SI1133_CHANNEL(_si, _ch, _type) \
> > > + .type =3D _type, \
> > > + .indexed =3D 1, \
> > > + .channel =3D _ch, \
> > > + .scan_index =3D _si, \
> > > + .scan_type =3D { \
> > > + .sign =3D 'u', \
> > > + .realbits =3D 16, \
> > > + .storagebits =3D 16, \
> > > + .endianness =3D IIO_LE, \
> > > + }, \
> > > + .info_mask_separate =3D BIT(IIO_CHAN_INFO_RAW), \
> > > + .info_mask_shared_by_all =3D BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > > + BIT(IIO_CHAN_INFO_INT_TIME) | \
> > > + BIT(IIO_CHAN_INFO_SCALE) | \
> > > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
> > > +
> > > +static const struct iio_chan_spec si1133_channels[] =3D {
> > > + {
> > > + SI1133_CHANNEL(0, SI1133_PARAM_ADCMUX_WHITE, IIO_INTENSITY)
> > > + .channel2 =3D IIO_MOD_LIGHT_BOTH,
> > > + },
> > > + {
> > > + SI1133_CHANNEL(1, SI1133_PARAM_ADCMUX_LARGE_WHITE,
> > > + IIO_INTENSITY)
> > > + .channel2 =3D IIO_MOD_LIGHT_BOTH,
> > > + .extend_name =3D "large",
> > > + },
> > > + { =20
> >=20
> > Having in reply to the other review said that you need to maintain chan=
nel
> > indexes, note that they don't have to continue across channels with=20
> > different channel modifiers as those do uniquely describe events etc.
> >=20
> > =20
> > > + SI1133_CHANNEL(2, SI1133_PARAM_ADCMUX_SMALL_IR, IIO_INTENSITY)
> > > + .extend_name =3D "small",
> > > + .modified =3D 1,
> > > + .channel2 =3D IIO_MOD_LIGHT_IR,
> > > + },
> > > + {
> > > + SI1133_CHANNEL(3, SI1133_PARAM_ADCMUX_MED_IR, IIO_INTENSITY)
> > > + .extend_name =3D "med",
> > > + .modified =3D 1,
> > > + .channel2 =3D IIO_MOD_LIGHT_IR,
> > > + },
> > > + {
> > > + SI1133_CHANNEL(4, SI1133_PARAM_ADCMUX_LARGE_IR, IIO_INTENSITY)
> > > + .extend_name =3D "large",
> > > + .modified =3D 1,
> > > + .channel2 =3D IIO_MOD_LIGHT_IR,
> > > + },
> > > + {
> > > + SI1133_CHANNEL(5, SI1133_PARAM_ADCMUX_UV, IIO_UVINDEX) =20
> >=20
> > This is an entirely different channel type, index doesn't need to conti=
nue to
> > this from the other channel types.
> >=20
> > Whilst there is nothing in the ABI specifying that it 'should' be done,
> > mostly we restart counting for each channel type / modifier pair.
> > If you want a unique index to identify a channel use 'address'.
> >=20
> > This particular channel is the only one that will be recognised by
> > generic code as it's the only one with out and extended_name.
> >=20
> > It's really annoying that they don't even 'suggest' an algorithm
> > to calculate an ambient light value which is what people will typically
> > want to get from the other channels. Normally we'd try to produce
> > that value from the driver as an other channel, but here we have no inf=
ormation
> > on how to do it!
> >=20
> > Actually there is example code at:
> > https://siliconlabs.github.io/Gecko_SDK_Doc/efm32g/html/si1133_8c_sourc=
e.html
> >=20
> > but - and it's a big but, the licence agreement is presumably=20
> >=20
> > https://github.com/hrshygoodness/EFM32-Library/blob/master/v2/an/an0820=
_efm32_smart_card/Silabs_License_Agreement.txt
> >=20
> > as that seems to be there standard license and it is a custom spun lice=
nse that
> > I doubt is GPL compatible..=20
> >=20
> > Worth asking if we can use it as it would make the driver of a great de=
al more
> > practical use?
> > =20
>=20
> Like I said into my other reply I sent an email and asked if we could use=
the
> algorithm inside the linux kernel based off a GPLv2 license and they repl=
ied this :
>=20
> "
> The LUX calculation code only works with Si1133.=20
> As long as the software is used with Silicon Lab's sensor product, I don'=
t see any problem.
> "
>=20
> Is that enough? If not, what kind of request can we ask from them? Ask th=
em to change
> the license so it's 100% GPLv2 compatible?
>=20
> It's the first time that I deal with some licensing problem, is there a s=
pecific kind of
> questions I should be asking for?=20
>=20
Hmm. Your question seems fine to me and the answer seems clear. Quote thei=
r email in the
description for the patch so we have it recorded in the git archive.
So I think we are fine, but as ever I'm not a lawyer. Perhaps send them an=
advance
copy of the code and ask them confirm that they are happy for it to be incl=
uded in
the mainline kernel under GPL v2?
That should make it clear exactly what code is covered etc.
Jonathan
prev parent reply other threads:[~2018-07-06 17:36 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-26 17:01 [PATCH v2] iio: light: introduce si1133 Maxime Roussin-Bélanger
2018-06-27 20:02 ` Peter Meerwald-Stadler
2018-06-28 14:34 ` Maxime Roussin-Belanger
2018-06-30 16:37 ` Jonathan Cameron
2018-07-03 16:28 ` Maxime Roussin-Belanger
2018-07-06 17:26 ` Jonathan Cameron
2018-06-30 17:18 ` Jonathan Cameron
2018-07-03 16:53 ` Maxime Roussin-Belanger
2018-07-06 17:27 ` Jonathan Cameron
2018-07-05 13:49 ` Maxime Roussin-Belanger
2018-07-06 17:36 ` Jonathan Cameron [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180706183635.00007c43@huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=jeff.dagenais@gmail.com \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=maxime.roussinbelanger@gmail.com \
--cc=pmeerw@pmeerw.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.