Message-ID: <ae769902-4bb5-a970-be1e-05b26f4c770c@gmail.com>
Date:   Fri, 3 Mar 2023 11:17:52 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Content-Language: en-US, en-GB
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Paul Gazzillo <paul@pgazz.com>,
        Zhigang Shi <Zhigang.Shi@liteon.com>,
        Shreeya Patel <shreeya.patel@collabora.com>,
        Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
References: <cover.1677750859.git.mazziesaccount@gmail.com>
 <874d59be98703bb58a98fea72138de5b94d71a52.1677750859.git.mazziesaccount@gmail.com>
 <ZADCEPc1ZKczhEpE@smile.fi.intel.com>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH v2 5/6] iio: light: ROHM BU27034 Ambient Light Sensor
In-Reply-To: <ZADCEPc1ZKczhEpE@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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! ~~