Re: [PATCH v1 2/3] iio: light: ROHM BU27008 color sensor

[Vaittinen, Matti]

On 4/23/23 15:57, Jonathan Cameron wrote:
> On Fri, 21 Apr 2023 12:39:36 +0300
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
> 
>> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
>> and IR) with four configurable channels. Red and green being always
>> available and two out of the rest three (blue, clear, IR) can be
>> selected to be simultaneously measured. Typical application is adjusting
>> LCD backlight of TVs, mobile phones and tablet PCs.
>>
>> Add initial support for the ROHM BU27008 color sensor.
>>   - raw_read() of RGB and clear channels
>>   - triggered buffer w/ DRDY interrtupt
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> 
> Hi Matti,
> 
> Biggest issue in here is some confusion over data packing when some channels
> are enabled.  The driver must pack the channels that are enabled (seen
> from active_scan_mask) according to general IIO rules.  So naturally aligned
> buffers.  Thus for this device it should always be packed into
> 
> struct scan {
> 	__le16 chans[4];
> 	s64 ts __aligned(8); /*it's aligned anyway but better not to make reviewers think ;) */
> };
> 
> Even though there are 5 possible channels.  If one in the middle isnt' enabled (e.g. blue)
> then clear and IR shift to lower words. of the buffer.

Ah, right. So I had misunderstood how the buffer works. I thought the 
scan_mask was only used to disallow unsupported channel-enabling 
configurations. If I understand your statement correctly, the scan_mask 
is used to determine the 'place of data' in the buffer when certain 
configuration is used. (I'll check this from the code but if the IIO 
handles data as I now think - that's cool! It should indeed simplify the 
buffer in driver side!).

> 
> The demux code in the IIO core then deals with userspace requesting less than this
> by repacking the data if needed.   That allows it to present different views to different
> consumers (e.g. userspace IIO access, and an inkernel buffer user - though there aren't
> any of those for light sensors AFAIK.
> 
> I'd not thought about the issue of the weird scales interacting with someone changing
> the integration time. That's nasty and I don't have a better suggestion than what you have
> currently.
> 
> Otherwise some trivial stuff inline.

Thanks for the non-trivial and trivial stuff! I'll re-work this driver 
based on your input - which is really invaluable - during this week. 
Your support is _very much_ appreciated!

> 
> Jonathan
> 
> 
> 
>> diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
>> new file mode 100644
>> index 000000000000..6fca193eeb9e
>> --- /dev/null
>> +++ b/drivers/iio/light/rohm-bu27008.c
>> @@ -0,0 +1,1028 @@
> 
> ...
> 
>> +
>> +enum {
>> +	BU27008_RED,	/* Always data0 */
>> +	BU27008_GREEN,	/* Always data1 */
>> +	BU27008_BLUE,	/* data2, configurable (blue / clear) */
>> +	BU27008_CLEAR,	/* data2 or data3 */
>> +	BU27008_IR,	/* data3 */
>> +	BU27008_NUM_CHANS
>> +};
>> +
>> +enum {
>> +	BU27008_DATA0, /* Always RED */
>> +	BU27008_DATA1, /* Always GREEN */
>> +	BU27008_DATA2, /* Blue or Clear */
>> +	BU27008_DATA3, /* IR or Clear */
>> +	BU27008_NUM_HW_CHANS
>> +};
>> +
>> +/* We can always measure red and green at same time */
>> +#define ALWAYS_SCANNABLE (BIT(BU27008_RED) | BIT(BU27008_GREEN))
>> +
>> +#define BU27008_CHAN_DATA_SIZE		2 /* Each channel has 16bits of data */
>> +#define BU27008_BUF_DATA_SIZE (BU27008_NUM_CHANS * BU27008_CHAN_DATA_SIZE)
> 
> Buffer should be same size as the number you can grab in one read.  So
> it should be same as BU27008_HW_DATA_SIZE. The data is packed when only some
> channels are enabled.
> 
>> +#define BU27008_HW_DATA_SIZE (BU27008_NUM_HW_CHANS * BU27008_CHAN_DATA_SIZE)
>> +#define NUM_U16_IN_TSTAMP (sizeof(s64) / sizeof(u16))
>> +
>> +static const unsigned long bu27008_scan_masks[] = {
>> +	ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_IR),
>> +	ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_BLUE),
>> +	ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_IR),
> 
> Good, this looks correct for allowing the IIO core to deal with demuxing
> the data as necessary.  Note that this is saying that if one of these
> modes is active scan mask it will be backed into lowers set of __le16
> buffer elements.

So basically, what I need to ensure is that the data in the buffer will 
be in the same order we tell here for "IIO demuxer". I think I got this 
now, thanks!

> 
> 
>> +static int bu27008_validate_trigger(struct iio_dev *idev,
>> +				   struct iio_trigger *trig)
>> +{
>> +	struct bu27008_data *data = iio_priv(idev);
>> +
>> +	if (data->trig != trig)
> 
> Hmm. I thought we had a standard function for this, but turns out we only have
> the one that validates in the other direction.  Perhaps it's wroth something
> generic like iio_device_validate_own_trigger which would be similar to
> iio_trigger_validate_own_device in that it would use the common parentage of
> the trigger and iio_dev to check the match.

I'll see if I can come up with something generic based on your suggestion :)

>> +	ret = regmap_update_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
>> +			   BU27008_MASK_SW_RESET, BU27008_MASK_SW_RESET);
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
>> +
>> +	msleep(1);
> 
> Good to state what reset time min is from datasheet.

I don't remember seeing that in datasheet, but I'll recheck.

>> +
>> +	/*
>> +	 * After some measurements, it seems reading the
>> +	 * BU27008_REG_MODE_CONTROL3 debounces the IRQ line
> 
> 'it seems' worries me.  In docs? Not that we have them but would be nice
> if this statement was stronger!

I have the - or at least a - datasheet. Unfortunately the datasheet does 
not bother explaining too much about how things are working... The 
'seems' here is based on my observation that without reading the 
BU27008_REG_MODE_CONTROL3 all the samples are read immediately 
regardless the integration time. Also, tracing the lines with saleae 
does not show the IRQ line being debounced when samples are read. This 
behaviour changes when I read the BU27008_REG_MODE_CONTROL3 - which 
contains the 'VALID'-bit. Also saleae shows the IRQ debounced when this 
reg is read. Hence, I am fairly convinced the IC works like this - and I 
am somewhat convinced the IC is meant to work like this - but no, I 
haven't seen this documented (nor did I receive confirmation from the 
hardware colleagues when I asked about this). So, this comment just 
states my current understanding - HW 'seems' to work like this :)


>> +
>> +	if (!i2c->irq) {
>> +		dev_err(dev, "No IRQ configured\n");
> 
> If it's possible to relax this requirement for an IRQ you should definitely
> do so, even if you loose a lot of functionality.  It's annoyingly common for
> board designers to think it's not necessary to wire up IRQs.

Well, knowing how IC designers implement the IRQs... I am not at all 
sure not wiring the IRQs is a bad thing XD

And yes, a light sensor is probably very much usable with plain polling. 
I'll just drop the buffer/trigger stuff and provide only the 
read/write_raw() if IRQ is not given.


All in all, thanks a lot for the review!

Yours,
	-- Matti


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

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