Re: [PATCH v5 2/2] iio: light: as73211: New driver
On 8/4/20 9:40 AM, Christian Eggers wrote: On Sunday, 2 August 2020, 20:02:35 CEST, Andy Shevchenko wrote: Thanks for an update, my comments below. Thanks for the review. Please see below for my questions. Best regards Christian On Sun, Aug 2, 2020 at 7:40 PM Christian Eggers wrote: Datasheet: https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0- b302-c2fd-e30a-c98df87616df Do we need the UUID after the document file name? I have send AMS an inquiry. Not sure whether I will get an answer. I will wait a few days until sending v6. +#define AS73211_OFFSET_TEMP (-66.9) +#define AS73211_SCALE_TEMP 0.05 In the kernel we don't do float arithmetic. How these are being used? Does this restriction also apply for compile time constants? I am quite sure that all calculations using these defines will be evaluated at compile time. If found a number of other places where probably the same is done: find . -name '*.c' | xargs grep "#define.*[0-9]\.[0-9]" | grep -v '"' | grep -v "\/\*.*[0-9]\.[0-9]" I believe it is implementation defined. The compiler is free to generate floating math and do the conversion at runtime. Although it is probably safe to assume that no reasonable compiler will do this for your code. If only we had constexpr in C, then there was a way to make it guaranteed that the conversion happens during compile time. But I agree with you, it would be nice to have a cleaner way of declaring fixed point numbers without having to pay attention to how many 0s you have to put after the least significant digit.
Re: [PATCH v5 2/2] iio: light: as73211: New driver
On Tue, Aug 4, 2020 at 10:42 AM Christian Eggers wrote: > On Sunday, 2 August 2020, 20:02:35 CEST, Andy Shevchenko wrote: > > On Sun, Aug 2, 2020 at 7:40 PM Christian Eggers wrote: ... > > > Datasheet: > > > https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0- > > > b302-c2fd-e30a-c98df87616df > > Do we need the UUID after the document file name? > I have send AMS an inquiry. Not sure whether I will get an answer. I will wait > a few days until sending v6. I have successfully opened a document w/o additional UUID at the end of URI. I think you may drop it. ... > > > +#define AS73211_OFFSET_TEMP (-66.9) > > > +#define AS73211_SCALE_TEMP 0.05 > > > > In the kernel we don't do float arithmetic. How these are being used? > Does this restriction also apply for compile time constants? I am quite > sure that all calculations using these defines will be evaluated at compile > time. If found a number of other places where probably the same is done: > > find . -name '*.c' | xargs grep "#define.*[0-9]\.[0-9]" | grep -v '"' | grep > -v "\/\*.*[0-9]\.[0-9]" Side note: `git grep ...` is much faster and better. % git grep -n -w '#define[^"/]\+[0-9]\+\.[0-9]\+' -- drivers/ include/ arch/ | wc -l 47 + DRM, yes. In any case... > > > + *val2 = (AS73211_OFFSET_TEMP - (int)AS73211_OFFSET_TEMP) * > > > 100; > > > > > > + *val2 = (AS73211_SCALE_TEMP - > > > (int)AS73211_SCALE_TEMP) * 100; > > Magic 100 multiplier. > I think that in the context of IIO_VAL_INT_PLUS_MICRO this isn't quite magic. > Using > 100 directly seems quite usual: > > find drivers/iio/ -type f | xargs grep "val2 = .*100" Hmm... Okay. > > I think here you got them always 0. And to fix that you need to > > redefine (with also units included in the name) above constants like > > #define ..._OFFSET_TEMP_mC 66500 > > ... _SCALE_TEMP_?? 50 > a scale factor has no unit > > > > > Consider to use definitions from > > https://elixir.bootlin.com/linux/latest/source/include/linux/units.h > There are only definition for milli celsius. For IIO_VAL_INT_PLUS_MICRO I > would > require micro celsius. > > If I have the freedom, I would keep it as it is. Else I would suggest the > following: > #define AS73211_OFFSET_TEMP_INT (-66) > #define AS73211_OFFSET_TEMP_MICRO 90 > #define AS73211_SCALE_TEMP_INT 0 > #define AS73211_SCALE_TEMP_MICRO 5 ...somewhat like above would be better. But your freedom is defined by maintainers (not by me), so wait for their comments. ... > > > + }} > > > + > > > + return -EINVAL; > > > > Make it default case. > changed. Is there any benefit? My IDE's syntax checker now complains > "No return, in a function returning non-void". But gcc is happy with this. Your IDE is buggy :-) Yes, there is a benefit of doing this, at some point compiler complains about switches that don't cover all cases. ... > > > + ret = devm_iio_device_register(dev, indio_dev); > > > + if (ret < 0) > > > + return ret; > > > + > > > + return 0; > > > > return devm_iio_device_register(); > changed. I prefer the original pattern as it would produce less changed lines > if something needs to inserted later. But if not, it will be a bulk of several lines of code which is the bait for all kinds of janitors and clean up scripts (I saw that IRL, so it's not unrealistic). In that case it will be twice the churn. -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 2/2] iio: light: as73211: New driver
On Sunday, 2 August 2020, 20:02:35 CEST, Andy Shevchenko wrote: > Thanks for an update, my comments below. Thanks for the review. Please see below for my questions. Best regards Christian > On Sun, Aug 2, 2020 at 7:40 PM Christian Eggers wrote: > > Datasheet: > > https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0- > > b302-c2fd-e30a-c98df87616df > Do we need the UUID after the document file name? I have send AMS an inquiry. Not sure whether I will get an answer. I will wait a few days until sending v6. > > +#define AS73211_OFFSET_TEMP (-66.9) > > +#define AS73211_SCALE_TEMP 0.05 > > In the kernel we don't do float arithmetic. How these are being used? Does this restriction also apply for compile time constants? I am quite sure that all calculations using these defines will be evaluated at compile time. If found a number of other places where probably the same is done: find . -name '*.c' | xargs grep "#define.*[0-9]\.[0-9]" | grep -v '"' | grep -v "\/\*.*[0-9]\.[0-9]" > > + *val2 = (AS73211_OFFSET_TEMP - (int)AS73211_OFFSET_TEMP) * > > 100; > > > > + *val2 = (AS73211_SCALE_TEMP - > > (int)AS73211_SCALE_TEMP) * 100; > Magic 100 multiplier. I think that in the context of IIO_VAL_INT_PLUS_MICRO this isn't quite magic. Using 100 directly seems quite usual: find drivers/iio/ -type f | xargs grep "val2 = .*100" > I think here you got them always 0. And to fix that you need to > redefine (with also units included in the name) above constants like > #define ..._OFFSET_TEMP_mC 66500 > ... _SCALE_TEMP_?? 50 a scale factor has no unit > > Consider to use definitions from > https://elixir.bootlin.com/linux/latest/source/include/linux/units.h There are only definition for milli celsius. For IIO_VAL_INT_PLUS_MICRO I would require micro celsius. If I have the freedom, I would keep it as it is. Else I would suggest the following: #define AS73211_OFFSET_TEMP_INT (-66) #define AS73211_OFFSET_TEMP_MICRO 90 #define AS73211_SCALE_TEMP_INT 0 #define AS73211_SCALE_TEMP_MICRO 5 > > + }} > > + > > + return -EINVAL; > > Make it default case. changed. Is there any benefit? My IDE's syntax checker now complains "No return, in a function returning non-void". But gcc is happy with this. > > + ret = devm_iio_device_register(dev, indio_dev); > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > return devm_iio_device_register(); changed. I prefer the original pattern as it would produce less changed lines if something needs to inserted later.
Re: [PATCH v5 2/2] iio: light: as73211: New driver
On Sun, Aug 2, 2020 at 7:40 PM Christian Eggers wrote:
>
> Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor.
>
> This driver has no built-in trigger. In order for making triggered
> measurements, an external (software) trigger driver like
> iio-trig-hrtimer or iio-trig-sysfs is required.
>
> The sensor supports single and continuous measurement modes. The latter
> is not used by design as this would require tight timing synchronization
> between hardware and driver without much benefit.
Thanks for an update, my comments below.
> Datasheet:
> https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0-b302-c2fd-e30a-c98df87616df
Do we need the UUID after the document file name?
...
> +/* Available sample frequencies are 1.024MHz multiplied by powers of two. */
> +static const int as73211_samp_freq_avail[] = {
> + AS73211_SAMPLE_FREQ_BASE * 1,
> + AS73211_SAMPLE_FREQ_BASE * 2,
> + AS73211_SAMPLE_FREQ_BASE * 4,
> + AS73211_SAMPLE_FREQ_BASE * 8
+ Comma.
> +};
...
> +#define AS73211_OFFSET_TEMP (-66.9)
> +#define AS73211_SCALE_TEMP 0.05
In the kernel we don't do float arithmetic. How these are being used?
...
> + *val2 = (AS73211_OFFSET_TEMP - (int)AS73211_OFFSET_TEMP) *
> 100;
> + *val2 = (AS73211_SCALE_TEMP -
> (int)AS73211_SCALE_TEMP) * 100;
Magic 100 multiplier.
I think here you got them always 0. And to fix that you need to
redefine (with also units included in the name) above constants like
#define ..._OFFSET_TEMP_mC 66500
... _SCALE_TEMP_?? 50
Consider to use definitions from
https://elixir.bootlin.com/linux/latest/source/include/linux/units.h
...
> + }}
> +
> + return -EINVAL;
Make it default case.
> + }
> +
> + return -EINVAL;
Ditto.
...
> + }}
> +
> + return -EINVAL;
Ditto.
...
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
return devm_iio_device_register();
And consider to drop ' < 0' for devm_*() calls. As far as I understood
your intention to explicitly leave them because of i2c_*() calls,
though devm_*() and such are different.
--
With Best Regards,
Andy Shevchenko
[PATCH v5 2/2] iio: light: as73211: New driver
Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor. This driver has no built-in trigger. In order for making triggered measurements, an external (software) trigger driver like iio-trig-hrtimer or iio-trig-sysfs is required. The sensor supports single and continuous measurement modes. The latter is not used by design as this would require tight timing synchronization between hardware and driver without much benefit. Datasheet: https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0-b302-c2fd-e30a-c98df87616df Signed-off-by: Christian Eggers --- MAINTAINERS | 7 + drivers/iio/light/Kconfig | 15 + drivers/iio/light/Makefile | 1 + drivers/iio/light/as73211.c | 799 4 files changed, 822 insertions(+) create mode 100644 drivers/iio/light/as73211.c diff --git a/MAINTAINERS b/MAINTAINERS index 068d6e94122b..673570414147 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -934,6 +934,13 @@ S: Supported F: arch/arm64/boot/dts/amd/amd-seattle-xgbe*.dtsi F: drivers/net/ethernet/amd/xgbe/ +AMS AS73211 DRIVER +M: Christian Eggers +L: linux-...@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/iio/light/ams,as73211.yaml +F: drivers/iio/light/as73211.c + ANALOG DEVICES INC AD5686 DRIVER M: Michael Hennerich L: linux...@vger.kernel.org diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 182bd18c4bb2..cade6dc0305b 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -86,6 +86,21 @@ config APDS9960 To compile this driver as a module, choose M here: the module will be called apds9960 +config AS73211 + tristate "AMS AS73211 XYZ color 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. + +For triggered measurements, you will need an additional trigger driver +like IIO_HRTIMER_TRIGGER or IIO_SYSFS_TRIGGER. + +This driver can also be built as a module. If so, the module +will be called as73211. + config BH1750 tristate "ROHM BH1750 ambient light sensor" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index d1c8aa30b9a8..ea376deaca54 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_AL3010) += al3010.o obj-$(CONFIG_AL3320A) += al3320a.o obj-$(CONFIG_APDS9300) += apds9300.o obj-$(CONFIG_APDS9960) += apds9960.o +obj-$(CONFIG_AS73211) += as73211.o obj-$(CONFIG_BH1750) += bh1750.o obj-$(CONFIG_BH1780) += bh1780.o obj-$(CONFIG_CM32181) += cm32181.o diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c new file mode 100644 index ..dd1bba4d029f --- /dev/null +++ b/drivers/iio/light/as73211.c @@ -0,0 +1,799 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor + * + * Author: Christian Eggers + * + * Copyright (c) 2020 ARRI Lighting + * + * 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/a65474c0-b302-c2fd-e30a-c98df87616df + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define HZ_PER_KHZ 1000 + +#define AS73211_DRV_NAME "as73211" + +/* AS73211 configuration registers */ +#define AS73211_REG_OSR0x0 +#define AS73211_REG_AGEN 0x2 +#define AS73211_REG_CREG1 0x6 +#define AS73211_REG_CREG2 0x7 +#define AS73211_REG_CREG3 0x8 + +/* AS73211 output register bank */ +#define AS73211_OUT_OSR_STATUS0 +#define AS73211_OUT_TEMP 1 +#define AS73211_OUT_MRES1 2 +#define AS73211_OUT_MRES2 3 +#define AS73211_OUT_MRES3 4 + +#define AS73211_OSR_SSBIT(7) +#define AS73211_OSR_PDBIT(6) +#define AS73211_OSR_SW_RESBIT(3) +#define AS73211_OSR_DOS_MASK GENMASK(2, 0) +#define AS73211_OSR_DOS_CONFIGFIELD_PREP(AS73211_OSR_DOS_MASK, 0x2) +#define AS73211_OSR_DOS_MEASURE FIELD_PREP(AS73211_OSR_DOS_MASK, 0x3) + +#define AS73211_AGEN_DEVID_MASK GENMASK(7, 4) +#define AS73211_AGEN_DEVID(x) FIELD_PREP(AS73211_AGEN_DEVID_MASK, (x)) +#define AS73211_AGEN_MUT_MASK GENMASK(3, 0) +#define AS73211_AGEN_MUT(x) FIELD_PREP(AS73211_AGEN_MUT_MASK, (x)) + +#define AS73211_CREG1_GAIN_MASK GENMASK(7, 4) +#define AS73211_CREG1_GAIN_1 13 +#define AS73211_CREG1_TIME_MASK GENMASK(3, 0) + +#define AS73211_CREG3_CCLK_MASK GENMASK(1, 0) + +#define AS73211_OSR_STATUS_OUTCONVOF BIT(15) +#define AS73211_OSR_STATUS_MRESOF BIT(14) +#define AS73211_OSR_STATUS_ADCOF
