Re: [PATCH v3 4/7] iio: light: add Dyna-Image AL3010 driver

Michał Mirosław
2020-02-03T03:53:27Z

On Sat, Feb 01, 2020 at 07:00:21PM +0100, David Heidelberg wrote:
> Based on:
> - 3320A in-kernel driver
> - https://www.spinics.net/lists/linux-iio/msg25145.html
> - https://lore.kernel.org/patchwork/patch/684179/

Haha! This is such a simple device, that there is not many ways you can
write a driver quickly. Just look at [1]. ;-)

[1] https://rere.qmqm.pl/git/?p=linux;a=commit;h=f40bf1646c9166f9b8daeb7cf55703be3c5f78b3

[...]
> --- /dev/nulla
> +++ b/drivers/iio/light/al3010.c
[...]
> +#define AL3010_GAIN_SHIFT 4
> +#define AL3010_GAIN_MASK (BIT(6) | BIT(5) | BIT(4))
> +
> +#define AL3010_GAIN_READ(g) \
> + (((g) & AL3010_GAIN_MASK) >> AL3010_GAIN_SHIFT)
> +
> +#define AL3010_GAIN_WRITE(g) \
> + (((g) << AL3010_GAIN_SHIFT) & AL3010_GAIN_MASK)

There are FIELD_GET and FIELD_PREP macros available from
<linux/bitfield.h> that make accessing the registers a lot easier
to code and read.

> +#define AL3010_SCALE_AVAILABLE "1.1872 0.2968 0.0742 0.018"

I have used (albeit with a bug) more precise numbers. This doesn't
matter much, though, since I would expect noise to be much higher
than 0.1% for the measurement.

> +
> +enum al3xxxx_range {
> + AL3XXX_RANGE_1, /* 77806 lx */
> + AL3XXX_RANGE_2, /* 19542 lx */
> + AL3XXX_RANGE_3, /* 4863 lx */
> + AL3XXX_RANGE_4 /* 1216 lx */
> +};

The enums don't look very useful.

[...]

Tested-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Best Regards,
Michał Mirosław