On Thu, Mar 02, 2023 at 12:58:59PM +0200, Matti Vaittinen wrote:
> ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes
> capable of detecting a very wide range of illuminance. Typical application
> is adjusting LCD and backlight power of TVs and mobile phones.
>
> Add initial support for the ROHM BU27034 ambient light sensor.
>
> NOTE:
> - Driver exposes 4 channels. One IIO_LIGHT channel providing the
> calculated lux values based on measured data from diodes #0 and
> #1. Additionally 3 IIO_INTENSITY channels are emitting the raw
> register data from all diodes for more intense user-space
> computations.
> - Sensor has adjustible GAIN values ranging from 1x to 4096x.
> - Sensor has adjustible measurement times 5, 55, 100, 200 and
> 400 mS. Driver does not support 5 mS which has special
> limitations.
> - Driver exposes standard 'scale' adjustment which is
> implemented by:
> 1) Trying to adjust only the GAIN
> 2) If GAIN adjustment only can't provide requested
> scale, adjusting both the time and the gain is
> attempted.
> - Driver exposes writable INT_TIME property which can be used
> for adjusting the measurement time. Time adjustment will also
> cause the driver to adjust the GAIN so that the overall scale
> is not changed.
> - Runtime PM is not implemented.
> - Driver starts the measurement on the background when it is
> probed. This improves the respnse time to read-requests
> compared to starting the read only when data is requested.
> When the most accurate 400 mS measurement time is used, data reads
> would last quite long if measurement was started only on
> demand. This, however, is not appealing for users who would
> prefere power saving over measurement response time.
...
> +config ROHM_BU27034
> + tristate "ROHM BU27034 ambient light sensor"
> + depends on I2C
How? I do not see a such.
> + select REGMAP_I2C
> + select IIO_GTS_HELPER
> + help
> + Enable support for the ROHM BU27034 ambient light sensor. ROHM BU27034
> + is an ambient light sesnor with 3 channels and 3 photo diodes capable
> + of detecting a very wide range of illuminance.
> + Typical application is adjusting LCD and backlight power of TVs and
> + mobile phones.
Module name?
...
> obj-$(CONFIG_OPT3001) += opt3001.o
> obj-$(CONFIG_PA12203001) += pa12203001.o
> +obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o
If you see, most of the components are without vendor prefix, why rohm is
special? Like you are expecting the very same filename for something else?
> obj-$(CONFIG_RPR0521) += rpr0521.o
> obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
> obj-$(CONFIG_SI1133) += si1133.o
...
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
Sorted?
...
> +#define BU27034_REG_DATA0_LO 0x50
> +#define BU27034_REG_DATA1_LO 0x52
> +#define BU27034_REG_DATA2_LO 0x54
I would drop _LO in all these
> +#define BU27034_REG_DATA2_HI 0x55
and rename somehow this to something like _END / _MAX (similar to the fields.
Perhaps you would need _START / _MIN above.
...
> +/*
> + * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS
> + * Time impacts to gain: 1x, 2x, 4x, 8x.
> + *
> + * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768
> + *
> + * Using NANO precision for scale we must use scale 64x corresponding gain 1x
> + * to avoid precision loss. (32x would result scale 976 562.5(nanos).
> + */
> +#define BU27034_SCALE_1X 64
> +
> +#define BU27034_GSEL_1X 0x00
> +#define BU27034_GSEL_4X 0x08
> +#define BU27034_GSEL_16X 0x0a
> +#define BU27034_GSEL_32X 0x0b
> +#define BU27034_GSEL_64X 0x0c
> +#define BU27034_GSEL_256X 0x18
> +#define BU27034_GSEL_512X 0x19
> +#define BU27034_GSEL_1024X 0x1a
> +#define BU27034_GSEL_2048X 0x1b
> +#define BU27034_GSEL_4096X 0x1c
Shouldn't the values be in plain decimal?
Otherwise I would like to understand bit mapping inside these hex values.
...
> + .indexed = 1 \
+ Comma at the end.
...
> + static const int reg[] = {
> + [BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2,
> + [BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3,
> + [BU27034_CHAN_DATA2] = BU27034_REG_MODE_CONTROL2
Ditto.
> + };
...
> + struct bu27034_gain_check gains[3] = {
> + { .chan = BU27034_CHAN_DATA0, },
> + { .chan = BU27034_CHAN_DATA1, },
Inner commas are not needed.
> + { .chan = BU27034_CHAN_DATA2 }
But here the outer one is good to have.
> + };
...
> + if (chan == BU27034_CHAN_ALS) {
> + if (val == 0 && val2 == 1000)
> + return 0;
> + else
Redundant 'else'. And probably here is better to use standard pattern for
"checking for error first".
> + return -EINVAL;
> + }
...
> + if (helper64 < 0xFFFFFFFFFFFFFLLU) {
Perhaps this needs a definition.
> + helper64 *= gain0;
> + do_div(helper64, ch0);
> + } else {
> + do_div(helper64, ch0);
> + helper64 *= gain0;
> + }
> + /* Same overflow check here */
Why not a helper function?
> + if (helper64 < 0xFFFFFFFFFFFFFLLU) {
> + helper64 *= gain0;
> + do_div(helper64, helper);
> + } else {
> + do_div(helper64, helper);
> + helper64 *= gain0;
> + }
...
> + return (val & BU27034_MASK_VALID);
Unneeded parentheses.
...
> +retry:
> + /* Get new value from sensor if data is ready */
> + if (bu27034_has_valid_sample(data)) {
> + ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,
> + res, size);
> + if (ret)
> + return ret;
> +
> + bu27034_invalidate_read_data(data);
> + } else {
> + /* No new data in sensor. Wait and retry */
> + msleep(25);
> +
> + goto retry;
There is no way out. What might go wrong?
> + }
...
> + ret = bu27034_get_int_time(data);
_get_int_time_us() ? (Looking at the below code)
> + if (ret < 0)
> + return ret;
> +
> + msleep(ret / 1000);
...
> + * Avoid div by zeroi. Not using max() as the data may not be in
zeroi?
...
> + if (!res[0])
Positive conditional?
> + ch0 = 1;
> + else
> + ch0 = le16_to_cpu(res[0]);
> +
> + if (!res[1])
> + ch1 = 1;
Ditto.
> + else
> + ch1 = le16_to_cpu(res[1]);
But why not to read and convert first and then check. This at least will
correctly compare 0 to the LE16 0 (yes, it's the same for 0, but strictly
speaking the bits order of lvalue and rvalue is different).
...
> + switch (mask) {
> + case IIO_CHAN_INFO_INT_TIME:
> + return iio_gts_avail_times(&data->gts, vals, type, length);
> + case IIO_CHAN_INFO_SCALE:
> + return iio_gts_all_avail_scales(&data->gts, vals, type, length);
> + default:
> + break;
> + }
> +
> + return -EINVAL;
You may do it from default case.
...
> + ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4,
> + val, (val & BU27034_MASK_VALID),
Redundant parentheses.
> + BU27034_DATA_WAIT_TIME_US,
> + BU27034_TOTAL_DATA_WAIT_TIME_US);
> + if (ret) {
> + dev_err(data->dev, "data polling %s\n",
> + !(val & BU27034_MASK_VALID) ? "timeout" : "fail");
Why not positive conditional in ternary?
> + return ret;
> + }
...
> + fwnode = dev_fwnode(dev);
> + if (!fwnode)
> + return -ENODEV;
So, you deliberately disable a possibility to instantiate this from user space,
why?
...
> + ret = devm_iio_kfifo_buffer_setup(dev, idev, &bu27034_buffer_ops);
> +
> + ret = devm_iio_device_register(dev, idev);
Don't you find something strange in between?
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "Unable to register iio device\n");
...
> + { .compatible = "rohm,bu27034", },
Inner comma is not needed.
--
With Best Regards,
Andy Shevchenko
