| Message ID | 20231220-as7331-v1-2-745b73c27703@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: add support for AMS AS7331 | expand |
On Sat, 23 Dec 2023 11:46:14 +0100 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > The AMS AS7331 is a UV light sensor with three channels: UVA, UVB and > UVC (also known as deep UV and referenced as DUV in the iio core). > Its internal structure and forming blocks are practically identical to > the ones the AS73211 contains: API, internal DAC, I2C interface and > registers, measurement modes, number of channels and pinout. > > The only difference between them is the photodiodes used to acquire > light, which means that only some modifications are required to add > support for the AS7331 in the existing driver. > > The temperature channel is identical for both devices and only the > channel modifiers of the IIO_INTENSITY channels need to account for the > device type. > > The scale values have been obtained from the chapter "7.5 Transfer > Function" of the official datasheet[1] for the configuration chosen as > basis (Nclk = 1024 and GAIN = 1). Those values keep the units from the > datasheet (nW/cm^2) because no additional upscaling is required to work > with integers as opposed to the scale values for the AS73211. Actually > if the same upscaling is used, their values will not fit in 4-byte > integers without affecting its sign. > > Instead, the AS7331-specific function to retrieve the intensity scales > returns decimal values as listed in the datasheet for every > combination of GAIN and Nclk, keeping the unit as nW/cm^2. > To achieve that, a fractional value is returned. > The AS73211 scales use nW/m^2 units to work with integers that fit in > a 4-byte integer, and in that case there is no need to modify the value > type. No need, but in general it's a nice to have if it works well with IIO_VAL_FRACTIONAL because in kernel users (there aren't really any for light sensors) can handle the maths better if they need to apply other scalings etc. > > Add a new device-specific data structure to account for the device > differences: channel types and scale of LSB per channel. A may not be worth doing it in this case, but usual approach to refactoring a driver to allow support of additional devices is to do it in two steps. 1) Refactor with no new support - so should be no operational changes. 2) Add the new device support. > > [1] https://ams.com/documents/20143/9106314/AS7331_DS001047_4-00.pdf > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> ... > diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c > index ec97a3a46839..d53a0ae5255a 100644 > --- a/drivers/iio/light/as73211.c > +++ b/drivers/iio/light/as73211.c ... > > +static const struct iio_chan_spec as7331_channels[] = { > + { > + .type = IIO_TEMP, > + .info_mask_separate = > + BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_OFFSET) | > + BIT(IIO_CHAN_INFO_SCALE), > + .address = AS73211_OUT_TEMP, > + .scan_index = AS73211_SCAN_INDEX_TEMP, > + .scan_type = { > + .sign = 'u', > + .realbits = 16, > + .storagebits = 16, > + .endianness = IIO_LE, > + } > + }, > + AS73211_COLOR_CHANNEL(LIGHT_UVA, AS73211_SCAN_INDEX_X, AS73211_OUT_MRES1), > + AS73211_COLOR_CHANNEL(LIGHT_UVB, AS73211_SCAN_INDEX_Y, AS73211_OUT_MRES2), > + AS73211_COLOR_CHANNEL(LIGHT_DUV, AS73211_SCAN_INDEX_Z, AS73211_OUT_MRES3), > + IIO_CHAN_SOFT_TIMESTAMP(AS73211_SCAN_INDEX_TS), > +}; > + > static unsigned int as73211_integration_time_1024cyc(struct as73211_data *data) > { > /* > @@ -316,6 +361,50 @@ static int as73211_req_data(struct as73211_data *data) > return 0; > } > > +static int as73211_intensity_scale(struct as73211_data *data, int chan, int *val, int *val2) > +{ > + unsigned int scale; > + > + switch (chan) { > + case IIO_MOD_X: > + scale = AS73211_SCALE_X; > + break; > + case IIO_MOD_Y: > + scale = AS73211_SCALE_Y; > + break; > + case IIO_MOD_Z: > + scale = AS73211_SCALE_Z; > + break; > + default: > + return -EINVAL; > + } > + scale /= as73211_gain(data); > + scale /= as73211_integration_time_1024cyc(data); > + *val = scale; > + > + return IIO_VAL_INT; Obviously it's really a question about the original code but why not use IIO_VAL_FRACTIONAL here as well as below? Superficially looks like it should work in a similar fashion. If not, perhaps a comment here somewhere? > +} > + > +static int as7331_intensity_scale(struct as73211_data *data, int chan, int *val, int *val2) > +{ > + switch (chan) { > + case IIO_MOD_LIGHT_UVA: > + *val = AS7331_SCALE_UVA; > + break; > + case IIO_MOD_LIGHT_UVB: > + *val = AS7331_SCALE_UVB; > + break; > + case IIO_MOD_LIGHT_DUV: > + *val = AS7331_SCALE_UVC; > + break; > + default: > + return -EINVAL; > + } > + *val2 = as73211_gain(data) * as73211_integration_time_1024cyc(data); > + > + return IIO_VAL_FRACTIONAL; > +} > + > static int as73211_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, > int *val, int *val2, long mask) > { > @@ -355,30 +444,12 @@ static int as73211_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons > *val2 = AS73211_SCALE_TEMP_MICRO; > return IIO_VAL_INT_PLUS_MICRO; > > - case IIO_INTENSITY: { > - unsigned int scale; > - > - switch (chan->channel2) { > - case IIO_MOD_X: > - scale = AS73211_SCALE_X; > - break; > - case IIO_MOD_Y: > - scale = AS73211_SCALE_Y; > - break; > - case IIO_MOD_Z: > - scale = AS73211_SCALE_Z; > - break; > - default: > - return -EINVAL; > - } > - scale /= as73211_gain(data); > - scale /= as73211_integration_time_1024cyc(data); > - *val = scale; > - return IIO_VAL_INT; > + case IIO_INTENSITY: > + return data->spec_dev->intensity_scale(data, chan->channel2, val, val2); Where it doesn't hurt readability, I'd prefer we stayed as close to 80 chars or below as reasonably possible. So here wrap so val, val2); is on the next line. > > default: > return -EINVAL; > - }} > + } > > case IIO_CHAN_INFO_SAMP_FREQ: > /* f_samp is configured in CREG3 in powers of 2 (x 1.024 MHz) */ > @@ -676,13 +747,20 @@ static int as73211_probe(struct i2c_client *client) > i2c_set_clientdata(client, indio_dev); > data->client = client; > > + if (dev_fwnode(dev)) > + data->spec_dev = device_get_match_data(dev); > + else > + data->spec_dev = i2c_get_match_data(client); Take a look at how i2c_get_match_data() is defined... https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L117 and in particular what it calls first.. > + if (!data->spec_dev) > + return -EINVAL; > + > mutex_init(&data->mutex); > init_completion(&data->completion); > > indio_dev->info = &as73211_info; > indio_dev->name = AS73211_DRV_NAME; > - indio_dev->channels = as73211_channels; > - indio_dev->num_channels = ARRAY_SIZE(as73211_channels); > + indio_dev->channels = data->spec_dev->channel; > + indio_dev->num_channels = data->spec_dev->num_channels; > indio_dev->modes = INDIO_DIRECT_MODE; >
On Saturday, 23 December 2023, 11:46:14 CET, Javier Carrasco wrote: > The AMS AS7331 is a UV light sensor with three channels: UVA, UVB and > UVC (also known as deep UV and referenced as DUV in the iio core). > Its internal structure and forming blocks are practically identical to > the ones the AS73211 contains: API, internal DAC, I2C interface and > registers, measurement modes, number of channels and pinout. > > The only difference between them is the photodiodes used to acquire > light, which means that only some modifications are required to add > support for the AS7331 in the existing driver. > > The temperature channel is identical for both devices and only the > channel modifiers of the IIO_INTENSITY channels need to account for the > device type. > > The scale values have been obtained from the chapter "7.5 Transfer > Function" of the official datasheet[1] for the configuration chosen as > basis (Nclk = 1024 and GAIN = 1). Those values keep the units from the > datasheet (nW/cm^2) because no additional upscaling is required to work > with integers as opposed to the scale values for the AS73211. Actually > if the same upscaling is used, their values will not fit in 4-byte > integers without affecting its sign. > > Instead, the AS7331-specific function to retrieve the intensity scales > returns decimal values as listed in the datasheet for every > combination of GAIN and Nclk, keeping the unit as nW/cm^2. > To achieve that, a fractional value is returned. > The AS73211 scales use nW/m^2 units to work with integers that fit in > a 4-byte integer, and in that case there is no need to modify the value > type. > > Add a new device-specific data structure to account for the device > differences: channel types and scale of LSB per channel. > > [1] https://ams.com/documents/20143/9106314/AS7331_DS001047_4-00.pdf > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > --- > drivers/iio/light/Kconfig | 5 +- > drivers/iio/light/as73211.c | 146 ++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 122 insertions(+), 29 deletions(-) > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > index 143003232d1c..fd5a9879a582 100644 > --- a/drivers/iio/light/Kconfig > +++ b/drivers/iio/light/Kconfig > @@ -87,13 +87,14 @@ config APDS9960 > module will be called apds9960 > > config AS73211 > - tristate "AMS AS73211 XYZ color sensor" > + tristate "AMS AS73211 XYZ color sensor and AMS AS7331 UV sensor" > depends on I2C > select IIO_BUFFER > select IIO_TRIGGERED_BUFFER > help > If you say yes here you get support for the AMS AS73211 > - JENCOLOR(R) Digital XYZ Sensor. > + JENCOLOR(R) Digital XYZ and the AMS AS7331 UVA, UVB and UVC > + ultraviolet sensors. > > For triggered measurements, you will need an additional trigger driver > like IIO_HRTIMER_TRIGGER or IIO_SYSFS_TRIGGER. > diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c > index ec97a3a46839..d53a0ae5255a 100644 > --- a/drivers/iio/light/as73211.c > +++ b/drivers/iio/light/as73211.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* > - * Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor > + * Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor and AMS AS7331 > + * UVA, UVB and UVC (DUV) Ultraviolet Sensor > * > * Author: Christian Eggers <ceggers@arri.de> > * > @@ -9,7 +10,9 @@ > * Color light sensor with 16-bit channels for x, y, z and temperature); > * 7-bit I2C slave address 0x74 .. 0x77. > * > - * Datasheet: https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf > + * Datasheets: > + * AS73211: https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf > + * AS7331: https://ams.com/documents/20143/9106314/AS7331_DS001047_4-00.pdf > */ > > #include <linux/bitfield.h> > @@ -84,6 +87,20 @@ static const int as73211_hardwaregain_avail[] = { > 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, > }; > > +struct as73211_data; > + > +/** > + * struct spec_dev_data - device-specific data > + * @intensity_scale: Function to retrieve intensity scale values. > + * @channel: Device channels. > + * @num_channels: Number of channels of the device. > + */ > +struct spec_dev_data { > + int (*intensity_scale)(struct as73211_data *data, int chan, int *val, int *val2); > + struct iio_chan_spec const *channel; > + int num_channels; > +}; > + > /** > * struct as73211_data - Instance data for one AS73211 > * @client: I2C client. > @@ -94,6 +111,7 @@ static const int as73211_hardwaregain_avail[] = { > * @mutex: Keeps cached registers in sync with the device. > * @completion: Completion to wait for interrupt. > * @int_time_avail: Available integration times (depend on sampling frequency). > + * @spec_dev: device-specific configuration. > */ > struct as73211_data { > struct i2c_client *client; > @@ -104,6 +122,7 @@ struct as73211_data { > struct mutex mutex; > struct completion completion; > int int_time_avail[AS73211_SAMPLE_TIME_NUM * 2]; > + const struct spec_dev_data *spec_dev; > }; > > #define AS73211_COLOR_CHANNEL(_color, _si, _addr) { \ > @@ -138,6 +157,10 @@ struct as73211_data { > #define AS73211_SCALE_Y 298384270 /* nW/m^2 */ > #define AS73211_SCALE_Z 160241927 /* nW/m^2 */ > > +#define AS7331_SCALE_UVA 340000 /* nW/cm^2 */ > +#define AS7331_SCALE_UVB 378000 /* nW/cm^2 */ > +#define AS7331_SCALE_UVC 166000 /* nW/cm^2 */ > + > /* Channel order MUST match devices result register order */ > #define AS73211_SCAN_INDEX_TEMP 0 > #define AS73211_SCAN_INDEX_X 1 > @@ -176,6 +199,28 @@ static const struct iio_chan_spec as73211_channels[] = { > IIO_CHAN_SOFT_TIMESTAMP(AS73211_SCAN_INDEX_TS), > }; > > +static const struct iio_chan_spec as7331_channels[] = { > + { > + .type = IIO_TEMP, > + .info_mask_separate = > + BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_OFFSET) | > + BIT(IIO_CHAN_INFO_SCALE), > + .address = AS73211_OUT_TEMP, > + .scan_index = AS73211_SCAN_INDEX_TEMP, > + .scan_type = { > + .sign = 'u', > + .realbits = 16, > + .storagebits = 16, > + .endianness = IIO_LE, > + } > + }, > + AS73211_COLOR_CHANNEL(LIGHT_UVA, AS73211_SCAN_INDEX_X, AS73211_OUT_MRES1), > + AS73211_COLOR_CHANNEL(LIGHT_UVB, AS73211_SCAN_INDEX_Y, AS73211_OUT_MRES2), > + AS73211_COLOR_CHANNEL(LIGHT_DUV, AS73211_SCAN_INDEX_Z, AS73211_OUT_MRES3), > + IIO_CHAN_SOFT_TIMESTAMP(AS73211_SCAN_INDEX_TS), > +}; > + > static unsigned int as73211_integration_time_1024cyc(struct as73211_data *data) > { > /* > @@ -316,6 +361,50 @@ static int as73211_req_data(struct as73211_data *data) > return 0; > } > > +static int as73211_intensity_scale(struct as73211_data *data, int chan, int *val, int *val2) > +{ > + unsigned int scale; > + > + switch (chan) { > + case IIO_MOD_X: > + scale = AS73211_SCALE_X; > + break; > + case IIO_MOD_Y: > + scale = AS73211_SCALE_Y; > + break; > + case IIO_MOD_Z: > + scale = AS73211_SCALE_Z; > + break; > + default: > + return -EINVAL; > + } > + scale /= as73211_gain(data); > + scale /= as73211_integration_time_1024cyc(data); > + *val = scale; > + > + return IIO_VAL_INT; > +} > + > +static int as7331_intensity_scale(struct as73211_data *data, int chan, int *val, int *val2) > +{ > + switch (chan) { > + case IIO_MOD_LIGHT_UVA: > + *val = AS7331_SCALE_UVA; > + break; > + case IIO_MOD_LIGHT_UVB: > + *val = AS7331_SCALE_UVB; > + break; > + case IIO_MOD_LIGHT_DUV: > + *val = AS7331_SCALE_UVC; > + break; > + default: > + return -EINVAL; > + } > + *val2 = as73211_gain(data) * as73211_integration_time_1024cyc(data); > + > + return IIO_VAL_FRACTIONAL; > +} > + > static int as73211_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, > int *val, int *val2, long mask) > { > @@ -355,30 +444,12 @@ static int as73211_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons > *val2 = AS73211_SCALE_TEMP_MICRO; > return IIO_VAL_INT_PLUS_MICRO; > > - case IIO_INTENSITY: { > - unsigned int scale; > - > - switch (chan->channel2) { > - case IIO_MOD_X: > - scale = AS73211_SCALE_X; > - break; > - case IIO_MOD_Y: > - scale = AS73211_SCALE_Y; > - break; > - case IIO_MOD_Z: > - scale = AS73211_SCALE_Z; > - break; > - default: > - return -EINVAL; > - } > - scale /= as73211_gain(data); > - scale /= as73211_integration_time_1024cyc(data); > - *val = scale; > - return IIO_VAL_INT; > + case IIO_INTENSITY: > + return data->spec_dev->intensity_scale(data, chan->channel2, val, val2); > > default: > return -EINVAL; > - }} > + } > > case IIO_CHAN_INFO_SAMP_FREQ: > /* f_samp is configured in CREG3 in powers of 2 (x 1.024 MHz) */ > @@ -676,13 +747,20 @@ static int as73211_probe(struct i2c_client *client) > i2c_set_clientdata(client, indio_dev); > data->client = client; > > + if (dev_fwnode(dev)) > + data->spec_dev = device_get_match_data(dev); > + else > + data->spec_dev = i2c_get_match_data(client); > + if (!data->spec_dev) > + return -EINVAL; > + > mutex_init(&data->mutex); > init_completion(&data->completion); > > indio_dev->info = &as73211_info; > indio_dev->name = AS73211_DRV_NAME; > - indio_dev->channels = as73211_channels; > - indio_dev->num_channels = ARRAY_SIZE(as73211_channels); > + indio_dev->channels = data->spec_dev->channel; > + indio_dev->num_channels = data->spec_dev->num_channels; > indio_dev->modes = INDIO_DIRECT_MODE; > > ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_OSR); > @@ -772,14 +850,28 @@ static int as73211_resume(struct device *dev) > static DEFINE_SIMPLE_DEV_PM_OPS(as73211_pm_ops, as73211_suspend, > as73211_resume); > > +static const struct spec_dev_data as73211_spec = { > + .intensity_scale = as73211_intensity_scale, > + .channel = as73211_channels, > + .num_channels = ARRAY_SIZE(as73211_channels), > +}; > + > +static const struct spec_dev_data as7331_spec = { > + .intensity_scale = as7331_intensity_scale, > + .channel = as7331_channels, > + .num_channels = ARRAY_SIZE(as7331_channels), > +}; > + > static const struct of_device_id as73211_of_match[] = { > - { .compatible = "ams,as73211" }, > + { .compatible = "ams,as73211", &as73211_spec }, > + { .compatible = "ams,as7331", &as7331_spec }, > { } > }; > MODULE_DEVICE_TABLE(of, as73211_of_match); > > static const struct i2c_device_id as73211_id[] = { > - { "as73211", 0 }, > + { "as73211", (kernel_ulong_t)&as73211_spec }, > + { "as7331", (kernel_ulong_t)&as7331_spec }, > { } > }; > MODULE_DEVICE_TABLE(i2c, as73211_id); > > As I still work with Linux 6.1, I backported this patch for that version. A short test with an as73211 didn't show any differences. If I shall test further revisions of this patch, I can do this after the 9th of January. If I shall test further revisions of this patch, I can do this after the 9th of January. Tested-by: Christian Eggers <ceggers@arri.de>
On 26.12.23 17:14, Jonathan Cameron wrote: >> Add a new device-specific data structure to account for the device >> differences: channel types and scale of LSB per channel. > A may not be worth doing it in this case, but usual approach to refactoring > a driver to allow support of additional devices is to do it in two steps. > 1) Refactor with no new support - so should be no operational changes. > 2) Add the new device support. > I considered that in the first place, but the "refactoring" was so simple that the modification was just adding a pointer to an empty struct (you don't know what is chip-specific until you have another chip) and the patch alone had no real value (otherwise it could be applied to all drivers that only support one device, just in case). As you said, it may not be worth it in this case, but thank you for the clarification. >> +static int as73211_intensity_scale(struct as73211_data *data, int chan, int *val, int *val2) >> +{ >> + unsigned int scale; >> + >> + switch (chan) { >> + case IIO_MOD_X: >> + scale = AS73211_SCALE_X; >> + break; >> + case IIO_MOD_Y: >> + scale = AS73211_SCALE_Y; >> + break; >> + case IIO_MOD_Z: >> + scale = AS73211_SCALE_Z; >> + break; >> + default: >> + return -EINVAL; >> + } >> + scale /= as73211_gain(data); >> + scale /= as73211_integration_time_1024cyc(data); >> + *val = scale; >> + >> + return IIO_VAL_INT; > > Obviously it's really a question about the original code but why not > use IIO_VAL_FRACTIONAL here as well as below? Superficially looks > like it should work in a similar fashion. > > If not, perhaps a comment here somewhere? > You are right, the use of IIO_VAL_INT comes from the original implementation. I did not modify that because the expected precision (according to the datasheet is 3 decimal places) is guaranteed with the use of nW/m^2 instead of nW/cm^2 (the units used in the datasheet). I think the best approach would have been using IIO_VAL_FRACTIONAL and the units provided in the datasheet, but changing units now could cause problems to current users. We could still use IIO_VAL_FRACTIONAL unless that might affect current users in any way. Otherwise I will add a comment as suggested. >> @@ -355,30 +444,12 @@ static int as73211_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons >> *val2 = AS73211_SCALE_TEMP_MICRO; >> return IIO_VAL_INT_PLUS_MICRO; >> >> - case IIO_INTENSITY: { >> - unsigned int scale; >> - >> - switch (chan->channel2) { >> - case IIO_MOD_X: >> - scale = AS73211_SCALE_X; >> - break; >> - case IIO_MOD_Y: >> - scale = AS73211_SCALE_Y; >> - break; >> - case IIO_MOD_Z: >> - scale = AS73211_SCALE_Z; >> - break; >> - default: >> - return -EINVAL; >> - } >> - scale /= as73211_gain(data); >> - scale /= as73211_integration_time_1024cyc(data); >> - *val = scale; >> - return IIO_VAL_INT; >> + case IIO_INTENSITY: >> + return data->spec_dev->intensity_scale(data, chan->channel2, val, val2); > Where it doesn't hurt readability, I'd prefer we stayed as close to 80 chars or below > as reasonably possible. So here wrap so val, val2); is on the next line. > In order to meet the 80-char rule, three lines will be required (wrapping val, val2 is not enough; chan->channel2 must have its own line). It looks a bit weird, but I have nothing against it. On the other hand, the original code did not always follow the 80-char rule (up to 99 chars per line are used), so using two lines with a first one of 84 chars could be an option. >> + if (dev_fwnode(dev)) >> + data->spec_dev = device_get_match_data(dev); >> + else >> + data->spec_dev = i2c_get_match_data(client); > > Take a look at how i2c_get_match_data() is defined... > https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L117 > and in particular what it calls first.. > Oops! I missed that one. I will simplify the code to a simple call to i2c_get_match_data() and error check: data->spec_dev = i2c_get_match_data(client); if (!data->spec_dev) return -EINVAL; > Thanks for your review and best regards, Javier Carrasco
On 01.01.24 09:20, Christian Eggers wrote: > > As I still work with Linux 6.1, I backported this patch for > that version. A short test with an as73211 didn't show any > differences. > > If I shall test further revisions of this patch, I can do this > after the 9th of January. > > > If I shall test further revisions of this patch, I can do this > after the 9th of January. > > > Tested-by: Christian Eggers <ceggers@arri.de> > Thanks a lot for testing, I am glad that everything still works as before. Best regards, Javier Carrasco
... > > >> +static int as73211_intensity_scale(struct as73211_data *data, int chan, int *val, int *val2) > >> +{ > >> + unsigned int scale; > >> + > >> + switch (chan) { > >> + case IIO_MOD_X: > >> + scale = AS73211_SCALE_X; > >> + break; > >> + case IIO_MOD_Y: > >> + scale = AS73211_SCALE_Y; > >> + break; > >> + case IIO_MOD_Z: > >> + scale = AS73211_SCALE_Z; > >> + break; > >> + default: > >> + return -EINVAL; > >> + } > >> + scale /= as73211_gain(data); > >> + scale /= as73211_integration_time_1024cyc(data); > >> + *val = scale; > >> + > >> + return IIO_VAL_INT; > > > > Obviously it's really a question about the original code but why not > > use IIO_VAL_FRACTIONAL here as well as below? Superficially looks > > like it should work in a similar fashion. > > > > If not, perhaps a comment here somewhere? > > > You are right, the use of IIO_VAL_INT comes from the original > implementation. I did not modify that because the expected precision > (according to the datasheet is 3 decimal places) is guaranteed with the > use of nW/m^2 instead of nW/cm^2 (the units used in the datasheet). > > I think the best approach would have been using IIO_VAL_FRACTIONAL and > the units provided in the datasheet, but changing units now could cause > problems to current users. We could still use IIO_VAL_FRACTIONAL unless > that might affect current users in any way. Otherwise I will add a > comment as suggested. It's possible we'd get slightly better precision from IIO_VAL_FRACTIONAL as the string formatter does 64 bit maths and will print far too many decimal places (matters for high precision ADCs where the rounding errors are otherwise a problem). I'd be surprised if anyone noticed as this is read only anyway. So as far as I'm concerned switch to IIO_VAL_FRACTIONAL but keeping the same units for this would be a good change. Perhaps doesn't belong in this patch however. > > >> @@ -355,30 +444,12 @@ static int as73211_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons > >> *val2 = AS73211_SCALE_TEMP_MICRO; > >> return IIO_VAL_INT_PLUS_MICRO; > >> > >> - case IIO_INTENSITY: { > >> - unsigned int scale; > >> - > >> - switch (chan->channel2) { > >> - case IIO_MOD_X: > >> - scale = AS73211_SCALE_X; > >> - break; > >> - case IIO_MOD_Y: > >> - scale = AS73211_SCALE_Y; > >> - break; > >> - case IIO_MOD_Z: > >> - scale = AS73211_SCALE_Z; > >> - break; > >> - default: > >> - return -EINVAL; > >> - } > >> - scale /= as73211_gain(data); > >> - scale /= as73211_integration_time_1024cyc(data); > >> - *val = scale; > >> - return IIO_VAL_INT; > >> + case IIO_INTENSITY: > >> + return data->spec_dev->intensity_scale(data, chan->channel2, val, val2); > > Where it doesn't hurt readability, I'd prefer we stayed as close to 80 chars or below > > as reasonably possible. So here wrap so val, val2); is on the next line. > > > In order to meet the 80-char rule, three lines will be required > (wrapping val, val2 is not enough; chan->channel2 must have its own > line). It looks a bit weird, but I have nothing against it. > > On the other hand, the original code did not always follow the 80-char > rule (up to 99 chars per line are used), so using two lines with a first > one of 84 chars could be an option. Up to you. I'd be fine with 84 chars. Jonathan
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 143003232d1c..fd5a9879a582 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -87,13 +87,14 @@ config APDS9960 module will be called apds9960 config AS73211 - tristate "AMS AS73211 XYZ color sensor" + tristate "AMS AS73211 XYZ color sensor and AMS AS7331 UV sensor" depends on I2C select IIO_BUFFER select IIO_TRIGGERED_BUFFER help If you say yes here you get support for the AMS AS73211 - JENCOLOR(R) Digital XYZ Sensor. + JENCOLOR(R) Digital XYZ and the AMS AS7331 UVA, UVB and UVC + ultraviolet sensors. For triggered measurements, you will need an additional trigger driver like IIO_HRTIMER_TRIGGER or IIO_SYSFS_TRIGGER. diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c index ec97a3a46839..d53a0ae5255a 100644 --- a/drivers/iio/light/as73211.c +++ b/drivers/iio/light/as73211.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor + * Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor and AMS AS7331 + * UVA, UVB and UVC (DUV) Ultraviolet Sensor * * Author: Christian Eggers <ceggers@arri.de> * @@ -9,7 +10,9 @@ * Color light sensor with 16-bit channels for x, y, z and temperature); * 7-bit I2C slave address 0x74 .. 0x77. * - * Datasheet: https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf + * Datasheets: + * AS73211: https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf + * AS7331: https://ams.com/documents/20143/9106314/AS7331_DS001047_4-00.pdf */ #include <linux/bitfield.h> @@ -84,6 +87,20 @@ static const int as73211_hardwaregain_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, }; +struct as73211_data; + +/** + * struct spec_dev_data - device-specific data + * @intensity_scale: Function to retrieve intensity scale values. + * @channel: Device channels. + * @num_channels: Number of channels of the device. + */ +struct spec_dev_data { + int (*intensity_scale)(struct as73211_data *data, int chan, int *val, int *val2); + struct iio_chan_spec const *channel; + int num_channels; +}; + /** * struct as73211_data - Instance data for one AS73211 * @client: I2C client. @@ -94,6 +111,7 @@ static const int as73211_hardwaregain_avail[] = { * @mutex: Keeps cached registers in sync with the device. * @completion: Completion to wait for interrupt. * @int_time_avail: Available integration times (depend on sampling frequency). + * @spec_dev: device-specific configuration. */ struct as73211_data { struct i2c_client *client; @@ -104,6 +122,7 @@ struct as73211_data { struct mutex mutex; struct completion completion; int int_time_avail[AS73211_SAMPLE_TIME_NUM * 2]; + const struct spec_dev_data *spec_dev; }; #define AS73211_COLOR_CHANNEL(_color, _si, _addr) { \ @@ -138,6 +157,10 @@ struct as73211_data { #define AS73211_SCALE_Y 298384270 /* nW/m^2 */ #define AS73211_SCALE_Z 160241927 /* nW/m^2 */ +#define AS7331_SCALE_UVA 340000 /* nW/cm^2 */ +#define AS7331_SCALE_UVB 378000 /* nW/cm^2 */ +#define AS7331_SCALE_UVC 166000 /* nW/cm^2 */ + /* Channel order MUST match devices result register order */ #define AS73211_SCAN_INDEX_TEMP 0 #define AS73211_SCAN_INDEX_X 1 @@ -176,6 +199,28 @@ static const struct iio_chan_spec as73211_channels[] = { IIO_CHAN_SOFT_TIMESTAMP(AS73211_SCAN_INDEX_TS), }; +static const struct iio_chan_spec as7331_channels[] = { + { + .type = IIO_TEMP, + .info_mask_separate = + BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_OFFSET) | + BIT(IIO_CHAN_INFO_SCALE), + .address = AS73211_OUT_TEMP, + .scan_index = AS73211_SCAN_INDEX_TEMP, + .scan_type = { + .sign = 'u', + .realbits = 16, + .storagebits = 16, + .endianness = IIO_LE, + } + }, + AS73211_COLOR_CHANNEL(LIGHT_UVA, AS73211_SCAN_INDEX_X, AS73211_OUT_MRES1), + AS73211_COLOR_CHANNEL(LIGHT_UVB, AS73211_SCAN_INDEX_Y, AS73211_OUT_MRES2), + AS73211_COLOR_CHANNEL(LIGHT_DUV, AS73211_SCAN_INDEX_Z, AS73211_OUT_MRES3), + IIO_CHAN_SOFT_TIMESTAMP(AS73211_SCAN_INDEX_TS), +}; + static unsigned int as73211_integration_time_1024cyc(struct as73211_data *data) { /* @@ -316,6 +361,50 @@ static int as73211_req_data(struct as73211_data *data) return 0; } +static int as73211_intensity_scale(struct as73211_data *data, int chan, int *val, int *val2) +{ + unsigned int scale; + + switch (chan) { + case IIO_MOD_X: + scale = AS73211_SCALE_X; + break; + case IIO_MOD_Y: + scale = AS73211_SCALE_Y; + break; + case IIO_MOD_Z: + scale = AS73211_SCALE_Z; + break; + default: + return -EINVAL; + } + scale /= as73211_gain(data); + scale /= as73211_integration_time_1024cyc(data); + *val = scale; + + return IIO_VAL_INT; +} + +static int as7331_intensity_scale(struct as73211_data *data, int chan, int *val, int *val2) +{ + switch (chan) { + case IIO_MOD_LIGHT_UVA: + *val = AS7331_SCALE_UVA; + break; + case IIO_MOD_LIGHT_UVB: + *val = AS7331_SCALE_UVB; + break; + case IIO_MOD_LIGHT_DUV: + *val = AS7331_SCALE_UVC; + break; + default: + return -EINVAL; + } + *val2 = as73211_gain(data) * as73211_integration_time_1024cyc(data); + + return IIO_VAL_FRACTIONAL; +} + static int as73211_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) { @@ -355,30 +444,12 @@ static int as73211_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons *val2 = AS73211_SCALE_TEMP_MICRO; return IIO_VAL_INT_PLUS_MICRO; - case IIO_INTENSITY: { - unsigned int scale; - - switch (chan->channel2) { - case IIO_MOD_X: - scale = AS73211_SCALE_X; - break; - case IIO_MOD_Y: - scale = AS73211_SCALE_Y; - break; - case IIO_MOD_Z: - scale = AS73211_SCALE_Z; - break; - default: - return -EINVAL; - } - scale /= as73211_gain(data); - scale /= as73211_integration_time_1024cyc(data); - *val = scale; - return IIO_VAL_INT; + case IIO_INTENSITY: + return data->spec_dev->intensity_scale(data, chan->channel2, val, val2); default: return -EINVAL; - }} + } case IIO_CHAN_INFO_SAMP_FREQ: /* f_samp is configured in CREG3 in powers of 2 (x 1.024 MHz) */ @@ -676,13 +747,20 @@ static int as73211_probe(struct i2c_client *client) i2c_set_clientdata(client, indio_dev); data->client = client; + if (dev_fwnode(dev)) + data->spec_dev = device_get_match_data(dev); + else + data->spec_dev = i2c_get_match_data(client); + if (!data->spec_dev) + return -EINVAL; + mutex_init(&data->mutex); init_completion(&data->completion); indio_dev->info = &as73211_info; indio_dev->name = AS73211_DRV_NAME; - indio_dev->channels = as73211_channels; - indio_dev->num_channels = ARRAY_SIZE(as73211_channels); + indio_dev->channels = data->spec_dev->channel; + indio_dev->num_channels = data->spec_dev->num_channels; indio_dev->modes = INDIO_DIRECT_MODE; ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_OSR); @@ -772,14 +850,28 @@ static int as73211_resume(struct device *dev) static DEFINE_SIMPLE_DEV_PM_OPS(as73211_pm_ops, as73211_suspend, as73211_resume); +static const struct spec_dev_data as73211_spec = { + .intensity_scale = as73211_intensity_scale, + .channel = as73211_channels, + .num_channels = ARRAY_SIZE(as73211_channels), +}; + +static const struct spec_dev_data as7331_spec = { + .intensity_scale = as7331_intensity_scale, + .channel = as7331_channels, + .num_channels = ARRAY_SIZE(as7331_channels), +}; + static const struct of_device_id as73211_of_match[] = { - { .compatible = "ams,as73211" }, + { .compatible = "ams,as73211", &as73211_spec }, + { .compatible = "ams,as7331", &as7331_spec }, { } }; MODULE_DEVICE_TABLE(of, as73211_of_match); static const struct i2c_device_id as73211_id[] = { - { "as73211", 0 }, + { "as73211", (kernel_ulong_t)&as73211_spec }, + { "as7331", (kernel_ulong_t)&as7331_spec }, { } }; MODULE_DEVICE_TABLE(i2c, as73211_id);
The AMS AS7331 is a UV light sensor with three channels: UVA, UVB and UVC (also known as deep UV and referenced as DUV in the iio core). Its internal structure and forming blocks are practically identical to the ones the AS73211 contains: API, internal DAC, I2C interface and registers, measurement modes, number of channels and pinout. The only difference between them is the photodiodes used to acquire light, which means that only some modifications are required to add support for the AS7331 in the existing driver. The temperature channel is identical for both devices and only the channel modifiers of the IIO_INTENSITY channels need to account for the device type. The scale values have been obtained from the chapter "7.5 Transfer Function" of the official datasheet[1] for the configuration chosen as basis (Nclk = 1024 and GAIN = 1). Those values keep the units from the datasheet (nW/cm^2) because no additional upscaling is required to work with integers as opposed to the scale values for the AS73211. Actually if the same upscaling is used, their values will not fit in 4-byte integers without affecting its sign. Instead, the AS7331-specific function to retrieve the intensity scales returns decimal values as listed in the datasheet for every combination of GAIN and Nclk, keeping the unit as nW/cm^2. To achieve that, a fractional value is returned. The AS73211 scales use nW/m^2 units to work with integers that fit in a 4-byte integer, and in that case there is no need to modify the value type. Add a new device-specific data structure to account for the device differences: channel types and scale of LSB per channel. [1] https://ams.com/documents/20143/9106314/AS7331_DS001047_4-00.pdf Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/iio/light/Kconfig | 5 +- drivers/iio/light/as73211.c | 146 ++++++++++++++++++++++++++++++++++++-------- 2 files changed, 122 insertions(+), 29 deletions(-)