lkml.org 
[lkml]   [2023]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 5/6] iio: light: ROHM BU27034 Ambient Light Sensor
Hi Andy,

Thanks again for the review. Some nice catches there.

On 3/2/23 17:34, Andy Shevchenko wrote:
> 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.

I have assumed we need this because:

We select REGMAP_I2C which depends on I2C.
What happens if I2C=n and we select REGMAP_I2C? I may be wrong but I
guess the I2C stays 'n' while REGMAP_I2C becomes y/m (?) I think that
would be unfortunate - but I can't claim I am confident with how config
dependencies are handled. I can drop this depends on if you're sure
that's not a problem.

>> + 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?

I am having a deja-vu.
https://lore.kernel.org/all/10c4663b-dd65-a545-786d-10aed6e6e5e9@fi.rohmeurope.com/

Module name is completely irrelevant when selecting a kernel configuration.

> ...
>
>> 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?

No. I don't.

Using the vendor prefix in _file name_ was suggested to me by Lee
already a few years ago. And I am actually grateful he did. I've found
that _very_ useful as it simplifies finding the files I am looking for.
What comes to the config option name, being able to easily search for
the configs by vendor name has also been helpful.

> ...
>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/kfifo_buf.h>
>
> Sorted?

Sure, thanks.

>
> ...
>
>> +#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.

I don't think this would improve anything. The _LO / _HI are descriptive
as we have only two registers for each channel, _LO and _HI being more
or less standard abbreviations for low and high.

> ...
>
>> +/*
>> + * 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?

Why?

> Otherwise I would like to understand bit mapping inside these hex values.

I like having register values in hex. It makes it obvious they don't
necessarily directly match any 'real world' human-readable values.

> ...
>
>> + .indexed = 1 \
>
> + Comma at the end.

ok.

>
> ...
>
>> + 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.

ok.

>
>> + };
>
> ...
>
>> + 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.
>
>> + };
>

ok

> ...
>
>> + if (chan == BU27034_CHAN_ALS) {
>> + if (val == 0 && val2 == 1000)
>> + return 0;
>> + else
>
> Redundant 'else'

Thanks for pointing out the unnecessary else. Killing it makes this nicer.


>. And probably here is better to use standard pattern for
> "checking for error first".

I prefer to check for this one specific exactly supported case for ALS
channel. Cheking for 'all other possibilities but what we are expecting'
would be counter intuitive.

>
>> + return -EINVAL;
>> + }
>
> ...
>
>> + if (helper64 < 0xFFFFFFFFFFFFFLLU) {
>
> Perhaps this needs a definition.

I like seeing the value here. It makes this less obfuscating. Comment
makes the purpose obvious so adding a define would not really give any
extra advantage.

>> + helper64 *= gain0;
>> + do_div(helper64, ch0);
>> + } else {
>> + do_div(helper64, ch0);
>> + helper64 *= gain0;
>> + }
>
>
>> + /* Same overflow check here */
>
> Why not a helper function?

I actually was thinking of it - but the check is smallish, only done
twice and felt a tad too specific to warrant own function. I am not
really against adding a function if you feel strongly about this :)

>
>> + if (helper64 < 0xFFFFFFFFFFFFFLLU) {
>> + helper64 *= gain0;
>> + do_div(helper64, helper);
>> + } else {
>> + do_div(helper64, helper);
>> + helper64 *= gain0;
>> + }
>
> ...
>
>> + return (val & BU27034_MASK_VALID);
>
> Unneeded parentheses.

ok.

>
> ...
>
>> +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?

Beyond hanging the user process? :)

I think you have a point here. I'll add a timeout.

>
>> + }
>
> ...
>
>> + 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?

No. Again, we check for the very specific case where res has all bits
zeroed. Inverse condition is counter intuitive.

>
>> + 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.

Because conversion is not needed if channel data is zero.

> 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).

and hence we check for !res[0]

>
> ...
>
>> + 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.
>

I think we have discussed this one in the past too. I like having return
at the end of a non void function.

> ...
>
>> + ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4,
>> + val, (val & BU27034_MASK_VALID),
>
> Redundant parentheses.

ok

>
>> + 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?

Because I check this for a specific case: "Was it a timeout?" - not for
unspecified "Was it something else but timeout?"

>
>> + return ret;
>> + }
>
> ...
>
>> + fwnode = dev_fwnode(dev);
>> + if (!fwnode)
>> + return -ENODEV;
>
> So, you deliberately disable a possibility to instantiate this from user space,
> why?

Thanks! (And Sorry. Jonathan pointed this out to me already in the RFC.)
I thought I already fixed this.

>
> ...
>
>> + 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?

Thanks!

>
>> + if (ret < 0)
>> + return dev_err_probe(dev, ret,
>> + "Unable to register iio device\n");
>
> ...
>
>> + { .compatible = "rohm,bu27034", },
>
> Inner comma is not needed.

ok




--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

\
 
 \ /
  Last update: 2023-03-27 00:42    [W:0.172 / U:0.396 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site