Message-ID: <103531fc-e7c1-090f-a172-073a399f6380@fi.rohmeurope.com>
Date:   Thu, 2 Mar 2023 07:35:21 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Jonathan Cameron <jic23@...nel.org>,
        Matti Vaittinen <mazziesaccount@...il.com>
CC:     Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        Paul Gazzillo <paul@...zz.com>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        Liam Beguin <liambeguin@...il.com>,
        Peter Rosin <peda@...ntia.se>,
        Randy Dunlap <rdunlap@...radead.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 5/6] iio: light: ROHM BU27034 Ambient Light Sensor

Thanks for the review again!

I reworked the code for v2 I am about to send out. I think I ended up 
having quite a bit changes but I have tried to address most of what you 
pointed out. Thanks for all the improvements and the time you have 
invested to this already!

On 2/26/23 19:13, Jonathan Cameron wrote:
> On Wed, 22 Feb 2023 18:15:58 +0200
> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> 
>> +	.scan_type = {							\
>> +		.sign = 'u',						\
>> +		.realbits = 16,						\
>> +		.storagebits = 16,					\
>> +		.endianness = IIO_LE,					\
> 
> Unless you have buffered support, anything scan_* is unused and shouldn't be
> set.

Buffer implemented for v2 :)

> 
>> +	},								\
>> +	.indexed = 1							\
>> +}
>> +
>> +static const struct iio_chan_spec bu27034_channels[] = {
>> +	{
>> +		.type = IIO_LIGHT,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +		.channel = BU27034_CHAN_ALS,
>> +	},
>> +	BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR),
>> +	BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR),
> 
> That's unusual. Why does it have two clear channels?
> Perhaps add a comment on how they differ.  From a quick glance at the
> datasheet they have different sensitivities, but indeed both in the visible
> light range (mostly)
> 
> You could argue one is blue and one is red based on peaks of the curves but
> they are very broad so perhaps clear is the best naming.

Yep. I was not thrilled about this myself. The sensitivity peaks are at 
500 nm and 600 nm - which (according to some quickly checked spectrum 
pictures in the webs - no proper fact check done - 
https://chem.libretexts.org/Bookshelves/Physical_and_Theoretical_Chemistry_Textbook_Maps/Physical_Chemistry_(LibreTexts)/13%3A_Molecular_Spectroscopy/13.01%3A_The_Electromagnetic_Spectrum 
) are landing somewhere between the blue and green and near the orange 
areas. Yet, especially the data0 has a very wide sensitivity area as you 
pointed out. But yes, this warrants at least a comment.

This is actually also a topic I sent a very low priority email earlier 
:) I was wondering if exporting set of data-points representing these 
curves via sysfs entry would be something user-space applications could 
use... Describing/categorizing the arbitrarily shaped curves may be 
othervice hard. Userland could then be running different fitting 
algorithms depending on their needs - and see the error levels on the 
area they are interested in. But as I said in that mail thread - I don't 
have use-case for this so this was just some low priority pondering. 
Nothing that should be mixed with this patch mail ;)

> ...
>> +
>> +		for (i = 0; i < 2; i++) {
> 
> Why twice?

To test the sharpness of reviewers? BTW, You passed with excellent grade!

(No, really because I was interrupted in the middle of writing the code 
^_^; Was originally writing a loop to read the channels - but forgot it 
when continued. I find it impressive you spotted this - thanks a lot!)


>> +static int _bu27034_get_result(struct bu27034_data *data, u16 *res, bool lock)
>> +{
>> +	int ret = 0;
>> +
>> +retry:
>> +	if (lock)
>> +		mutex_lock(&data->mutex);
>> +	/* Get new value from sensor if data is ready - or use cached value */
>> +	if (bu27034_has_valid_sample(data)) {
>> +		ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO,
>> +				       &data->raw[0], sizeof(data->raw));
>> +		if (ret)
>> +			goto unlock_out;
>> +
>> +		data->cached = true;
>> +		bu27034_invalidate_read_data(data);
>> +	} else if (unlikely(!data->cached)) {
>> +		/* No new data in sensor and no value cached. Wait and retry */
>> +		if (lock)
>> +			mutex_unlock(&data->mutex);
> 
> Hmm. We don't really need to fix this in driver. Could just return -EAGAIN and let
> userspace work out that it needs to try again after a while?
> I guess not all userspace is going to be smart enough to handle that though and
> you need this to ensure we get a new value after a parameter change.
>  >
> If you do that then the locking dance gets much simpler.

I changed the approach for the v2 (to be sent soon(ish)). I did 
implement the buffering - and gave up any attempt of caching values for 
raw_reads. Instead, I will always start the measurement - wait - read 
result - stop the measurement for each read_raw.

It means that:

a) The (typical occasional?) user can still read the processed channel 
with read_raw every now and then. It will be slow, but it will only be 
1x meas-time slow.

b) Reading all the channels with read_raw using long integration times 
will be really slow - and if light levels are changing between the 
reads, the channel values are not reflecting the same light levels.

c) We get the power-saving as measurement is not running all the time.

d) Any user who needs some performance - or is interested in getting 
data for all the channels - can use the buffered mode.

So, a) and b) mean that read_raw is pretty much only usable for users 
interested in reading one channel at a time - or doing some debugging. I 
was actually considering dropping the read_raw support for 
intensity-channels, but didn't do it because those who use these 
raw-values should really know what they represent (they probably know 
the sensor and it's limitations). Also, reading for example only the 
data2 channel (which I think is not properly described in the 
data-sheet) can be a valid use-case for someone interesting in the IR-area.

In any case, the data-reading code got some changes for v2...

>> + * Then:
>> + * if (D1/D0 < 0.87)
>> + *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 3.45 + 1)
>> + * else if (D1/D0 < 1)
>> + *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 0.385 + 1)
>> + * else
>> + *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 2) * -0.05 + 1)
>> + *
>> + * we try implementing it here. Users who have for example some colored lens
> 
> There is no try, there is just do :)

It didn't feel like that when I was implementing the code :rolleyes:

> 
>> + * need to modify the calculation but I hope this gives a starting point for
>> + * those working with such devices.
> 
> That will need some dt bindings - though for now I guess we have no idea
> what they would be unless there are some hints on the datasheet?
> 

Yes. That is the problem. And even though we would hope we get the 
complete bindings from day 1 - I don't see it really a problem to add 
things like the 'lens-whateveritwillbe' when needed. What should be 
ensured then is the "property not found" case will default to the open-air.

The one thing we could add is sysfs attribute stating the 'open-air' for 
those who use the raw-values as they will be impacted by lens and won't 
be compensated by the driver like preprocessed values should be. Not 
sure if we want to do it yet though as I don't know if there will be any 
use for it in upstream.

>> + *
>> + * The first case (D1/D0 < 0.87) can be computed to a form:
>> + * lx = 0.004521097 * D1 - 0.002663996 * D0 + 0.00012213 * D1 * D1 / D0
>> + */
>> +static int bu27034_get_lux(struct bu27034_data *data, int *val)
>> +{
>> +	unsigned int gain0, gain1, meastime;
>> +	unsigned int d1_d0_ratio_scaled;
>> +	u16 res[3], ch0, ch1;
>> +	u64 helper64;
>> +	int ret;
>> +
>> +	mutex_lock(&data->mutex);
>> +	ret = bu27034_get_result_unlocked(data, &res[0]);
> 
> res
> as it is expecting a point to an array so that is more natural than pointing
> to the first element even if that's the same result.

This is pretty much the only thing I disagree with you :) For me it has 
always been much clearer to use pointer to first element - as the type 
of first element is what we are using. Type of an array (in my head) is 
something less well defined. I think this difference is best visible 
with the sizeof(arr) Vs. sizeof(&arr[0]).

I think I didn't change this for v2. I in any case expect to see v3 and 
probably a few others as well - so I will change this to some of the 
later versions if I didn't get you convinced that the &res[0] is Ok.

> 
>> +	if (ret)
>> +		goto unlock_out;
>> +
>> +	/* Avoid div by zero */
>> +	if (!res[0])
> 
> res[0] = max(1, res[0]); perhaps?

This would have been better, yes. However, I did change the data 
collection quite a bit for v2 - and there these values may not be in 
native byte order - so check for !res[0] feels more correct for v2 than 
comparing to value when the value format is not "correct".

>> +	case IIO_CHAN_INFO_RAW:
>> +	{
>> +		u16 res[3];
>> +
>> +		if (chan->type != IIO_INTENSITY)
>> +			return -EINVAL;
>> +
>> +		if (chan->channel < BU27034_CHAN_DATA0 ||
>> +		    chan->channel > BU27034_CHAN_DATA2)
>> +			return -EINVAL;
>> +		/*
>> +		 * Reading one channel at a time is inefficient.
>> +		 *
>> +		 * Hence we run the measurement on the background and always
>> +		 * read all the channels. There are following caveats:
>> +		 * 1) The VALID bit handling is racy. Valid bit clearing is not
>> +		 * tied to reading the data in the hardware. We clear the
>> +		 * valid-bit manually _after_ we have read the data - but this
>> +		 * means there is a small time-window where new result may
>> +		 * arrive between read and clear. This means we can miss a
>> +		 * sample. For normal use this should not be fatal because
>> +		 * usually the light is changing slowly. There might be
>> +		 * use-cases for measuring more rapidly changing light but this
>> +		 * driver is unsuitable for those cases anyways. (Smallest
>> +		 * measurement time we support is 55 mS.)
> 
> Given there is no general fix for that, not much you can do even if you don't want to
> miss the data.
> 
>> +		 * 2) Data readings more frequent than the meas_time will return
>> +		 * the same cached values. This should not be a problem for the
>> +		 * very same reason 1) is not a problem.
> 
> Hmm. I'm never that keen on drivers doing that if we can avoid it but perhaps we
> can't here.

Well, I dropped the caching of values for read_raw. I think it got rid 
of these particular problems. The issue 1) is still there for buffered 
mode but I guess we just need to live with it. On the bright side, 
missing a sample once in a blue moon is not fatal for most of the 
use-cases I can think of right now. (Besides, there is no general fix as 
you said so worrying about the unknown use-cases right now does not feel 
like the sanest thing. I have enough of worrying with the things that 
really are a problem...)

>> +	/*
>> +	 * Delay to allow IC to initialize. We don't care if we delay
>> +	 * for more than 1 ms so msleep() is Ok. We just don't want to
>> +	 * block
> 
> The msleep bit is kind of obvious for a reset. I'd not bother documenting that
> detail.

Well, the documentation is to suppress review comments regarding 1mS 
msleep :) And, I can't blame reviewers as the checkpatch is picking this 
up too. Hence I think it's Ok to tell that: "Yes, I know the sleep is 
likely to last longer than the requested 1 mS but it does not matter for 
our use-case so we still consciously chose to use msleep()."

> 
>> +	 */
>> +	msleep(1);
>> +
>> +	/*
>> +	 * Consider disabling the measurement (and powering off the sensor) for
>> +	 * runtime pm
> 
> Notes like this probably want to go away once the driver is 'finished'.

I hope I killed the worst power consumption at v2 by not running the 
measurement all the time at the background. I don't at the moment have a 
use-case for runtime pm - and as runtime pm tends to be "not trivial" - 
I will leave those bugs to be made only when needed... But yes, this 
comment can go as it adds pretty much no value.

> 
>> +	 */
>> +	ret = bu27034_meas_en(data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return devm_add_action_or_reset(data->dev, bu27034_meas_stop, data);
>> +}
>> +
>> +static int bu27034_probe(struct i2c_client *i2c)
>> +{
>> +	struct device *dev = &i2c->dev;
>> +	struct fwnode_handle *fwnode;
>> +	struct bu27034_data *data;
>> +	struct regmap *regmap;
>> +	struct iio_dev *idev;
>> +	unsigned int part_id;
>> +	int ret;
>> +
>> +	regmap = devm_regmap_init_i2c(i2c, &bu27034_regmap);
>> +	if (IS_ERR(regmap))
>> +		return dev_err_probe(dev, PTR_ERR(regmap),
>> +				     "Failed to initialize Regmap\n");
>> +
>> +	fwnode = dev_fwnode(dev);
> 
> why do we care?  So far this should work fine with the other types of i2c
> probe.

True! I didn't even think of such a case.

>> +	idev = devm_iio_device_alloc(dev, sizeof(*data));
>> +	if (!idev)
>> +		return -ENOMEM;
>> +
>> +	ret = devm_regulator_get_enable_optional(dev, "vdd");
> vdd isn't optional - or at least it would be an unusual device that doesn't
> need that supply line.
> 
> Key here is that optional in DT is different from this call.
> If not present in DT and devm_regulator_get_enable() called then we'll normally
> get a stub regulator.

Yes. I think we will also have a warning in a log - which I was not 
liking. OTOH, as the component clearly needs the VDD, maybe the warning 
about missing one in DT is also Ok.

>> +	if (ret != -ENODEV)
>> +		return dev_err_probe(dev, ret, "Failed to get regulator\n");
>> +
>> +	data = iio_priv(idev);
>> +
>> +	ret = regmap_read(regmap, BU27034_REG_SYSTEM_CONTROL, &part_id);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to access sensor\n");
>> +
>> +	part_id &= BU27034_MASK_PART_ID;
>> +
>> +	if (part_id != BU27034_ID) {
>> +		dev_err(dev, "unsupported device 0x%x\n", part_id);
> 
> Fallback compatibles require that on a failure to match ID we still let the driver
> carry on.  However we can print something in the log to say we don't recognise
> the device.  The intent is that at future part can be supported by old kernels just
> be having the dt list multiple compatibles if the device really are backwards
> compatible with parts already supported.

Makes sense. Besides, we should be able to trust the dt has correct 
compatibles - I'm not sure we should do these runtime checks for part 
IDs at all. I dropped the error - return and changed the print to warn.

>> +
>> +	idev->channels = bu27034_channels;
>> +	idev->num_channels = ARRAY_SIZE(bu27034_channels);
>> +	idev->name = "bu27034-als";
> 
> If the chip doesn't have a multiple functions (and multiple iio_devs), we'd normally
> not bother with the als part in the name.  Add a comment if there is a reason for
> it here.

Ok, thanks!

Yours,
	-- Matti

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

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

Powered by blists - more mailing lists