LinuxLists.cc - [PATCH v4 0/5] Support ROHM BU27008 RGB sensor

2023-05-03 09:47:05

Subject: [PATCH v4 0/5] Support ROHM BU27008 RGB sensor

Add support for ROHM BU27008 RGB sensor.

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.

This series supports reading the RGBC and IR channels using IIO
framework. However, only two of the BC+IR can be enabled at the same
time. Series adds also support for scale and integration time
configuration, where scale consists of impact of both the integration
time and hardware gain. The gain and time support is backed by the newly
introduced IIO GTS helper. This series depends on GTS helper patches
added in BU27034 support series which is already merged in iio/togreg
which this series is based on.

The hardware allows configuring gain setting by writing a 5-bit gain
selector value to a register. Part of the gain setting is common for all
channels (RGBC + IR) but part of the selector value can be set
separately for RGBC and IR:

MODE_CONTROL2 REG:
bit 7 6 5 4 3 2 1 0
-----------------------------------------------------------------
| RGB selector |
+---------------------------------------+
-----------------------------------------------------------------
| high bits IR | | low bits IR selector |
+---------------+ +-----------------------+

In theory it would be possible to set certain separate gain values for
RGBC and IR channels, but this gets pretty confusing because there are a
few 'unsupported' selector values. If only RGBC or IR was set, some
extra handling should be done to prevent the other channel from getting
unsupported value due to change in high-bits. Furthermore, allowing the
channels to be set different gain values (in some cases when gains are
such the HW supports it) would make the cases where also integration
time is changed to achieve correct scale ... interesting. It might also
be confusing for user to try predicting when setting different scales
succeeds and when it does not. Furthermore, if for example the scale
setting for RGBC caused IR selector to be invalid - it could also cause
the IR scale to "jump" very far from previous value.

To make the code simpler and more predictable for users, the current
logic is as follows:

1. Prevent setting IR scale. (My assumption is IR is less used than
RGBC)
2. When RGBC scale is set, set also the IR-selector to the same value.
This prevents unsupported selector values and makes the IR scale changes
predictable.

The 2) could mean we effectively have the same scale for all channels.
Unfortunately, the HW design is slightly peculiar and selector 0 means
gain 1X on RGBC but gain 2X on IR. Rest of the selectors equal same gain
values on RGBC and IR. The result is that while changin selector from 0
=> 1 causes RGBC gain to go from 1X => 4X, it causes IR gain to go from
2X => 4X.

So, the driver provides separate scale entries for all channels (also
RGB and C will have separate gain entries because these channels are of
same type as IR channel). This makes it possible for user applications
to go read the scales for all channels after setting scale for one (in
order to detect the IR scale difference).

Having the separate IR scale entry which applications can read to detect
"arbitrary scale changes" makes it possible for applications to be
written so they can cope if we need to implement the 'allow setting some
different gains for IR and RGBC' - later.

Finally, the scales_available is also provided for all other channels
except the IR channel, which does not allow the scale to be changed.

The sensor provides a data-ready IRQ and the driver implements a
triggered buffer mode using this IRQ as a trigger.

Finally, the series introduces generic iio_validate_own_trigger() helper
which can be used as a validate_trigger callback for drivers which
require the trigger and iio-device to be parented by same device. The
KX022A driver is converted to use this new callback instead of rolling
it's own function. The new helper and KX022A can be merged in as
independent changes if need be.

Revision history
v3 => v4:
bu27008 driver fixes
- Drop thread from device IRQ handler
- Styling and some minor improvements
- Use kernel-doc for enums
- Correctly order entries in Makefile
v2 => v3:
dt-bindings:
- No changes
iio_validate_own_trigger:
- subject fix
bu27008:
- Mostly styling based on comments from Andy and Andi

More accurate changelog in individual patches

v1 => v2:
dt-bindings:
- Fix issues pointed by Krzysztof.
bu27008 driver:
- Fix issues pointed by Jonathan
Add new helper for validating own trigger

More accurate changelog in individual patches

---

Matti Vaittinen (5):
dt-bindings: iio: light: ROHM BU27008
iio: trigger: Add simple trigger_validation helper
iio: kx022a: Use new iio_validate_own_trigger()
iio: light: ROHM BU27008 color sensor
MAINTAINERS: Add ROHM BU27008

.../bindings/iio/light/rohm,bu27008.yaml | 49 +
MAINTAINERS | 3 +-
drivers/iio/accel/kionix-kx022a.c | 13 +-
drivers/iio/industrialio-trigger.c | 22 +-
drivers/iio/light/Kconfig | 14 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/rohm-bu27008.c | 993 ++++++++++++++++++
include/linux/iio/trigger.h | 1 +
8 files changed, 1082 insertions(+), 14 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27008.yaml
create mode 100644 drivers/iio/light/rohm-bu27008.c

base-commit: 7fcbd72176076c44b47e8f68f0223c02c411f420
--
2.40.0

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]

Attachments:

(No filename) (6.13 kB)
signature.asc (499.00 B)
Download all attachments

2023-05-03 09:50:11

by Matti Vaittinen

[permalink] [raw]

On 5/13/23 20:52, Jonathan Cameron wrote:
> On Mon, 8 May 2023 09:32:28 +0300
> Matti Vaittinen <[email protected]> wrote:
>
>> Hi Jonathan,
>>
>> On 5/7/23 17:54, Jonathan Cameron wrote:
>>> On Wed, 3 May 2023 12:50:14 +0300
>>> Matti Vaittinen <[email protected]> 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 <[email protected]>
>>>>
>>> Mostly stuff that you asked about in response to earlier version but
>>> which I hadn't replied to until today.
>>>
>>> Upshot, don't need the manual irq handling in here.
>>>
>>> Whilst you aren't setting IRQF_ONESHOT for the pollfunc side of the trigger
>>> (the downstream IRQ / IRQ thread) the IIO utility functions are.
>>
>> I tried doing:
>>
>> static int bu27008_setup_trigger(struct bu27008_data *data, struct
>> iio_dev *idev)
>> {
>> struct iio_trigger *itrig;
>> char *name;
>> int ret;
>>
>> ret = devm_iio_triggered_buffer_setup(data->dev, idev,
>> &iio_pollfunc_store_time,
>> bu27008_trigger_handler,
>> &bu27008_buffer_ops);
>> if (ret)
>> return dev_err_probe(data->dev, ret,
>> "iio_triggered_buffer_setup_ext FAIL\n");
>>
>> itrig = devm_iio_trigger_alloc(data->dev, "%sdata-rdy-dev%d",
>> idev->name, iio_device_id(idev));
>> if (!itrig)
>> return -ENOMEM;
>>
>> data->trig = itrig;
>>
>> itrig->ops = &bu27008_trigger_ops;
>> iio_trigger_set_drvdata(itrig, data);
>>
>> name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bu27008",
>> dev_name(data->dev));
>>
>> ret = devm_request_irq(data->dev, data->irq,
>> /* No IRQ disabling */
>> &iio_trigger_generic_data_rdy_poll,
>> 0, name, itrig);
>> if (ret)
>> return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
>>
>> ret = devm_iio_trigger_register(data->dev, itrig);
>> if (ret)
>> return dev_err_probe(data->dev, ret,
>> "Trigger registration failed\n");
>>
>> /* set default trigger */
>> idev->trig = iio_trigger_get(itrig);
>>
>> return 0;
>> }
>>
>> It seems to me we get IRQ storm out of it, bu27008_trigger_handler never
>> being called. My assumption is that as soon as the IRQ handling code
>> exits the iio_trigger_generic_data_rdy_poll, it re-enables the IRQ - and
>> because we have level active IRQ and because the
>> bu27008_trigger_handler() has not yet had a chance to read the VALID bit
>> which restores the IRQ-line - we will immediately enter back to the IRQ
>> handling.
>
> Ah. I'd miss understood what was going on here. I thought we were talking
> race conditions only - not a level interrupt. Sorry for confusion / being
> half asleep. If it has an Ack like this I'd argue this is really an edge
> interrupt but that would require a guaranteed drop in the signal.

Yes. A failure to detect the edge (and skip acking) would leave the IRQ
no longer working. I think we have both seen some examples of that in
the past ;)

> I am assuming the sensor merrily carries on grabbing data, whether or
> not anyone reads it

This is also my understanding.

> and so if we treated this as an edge interrupt then
> the clear to set cycle could be very short (and hence not detected).
> If it instead doesn't read new data until previous has been read, then things
> are much simpler.

I think this is not how BU27008 works.

I think we could probably go on with edge IRQs and cook-up some "re-read
the VALID-bit again after the IRQ is for sure enabled to ensure the IRQ
does not go unasserted" - mechanism, which would work on 99.99% of the
cases. Problem is that some device always handles the 10000th
measurement ;) To tell the truth, I never really thought of that.

> Hmm. How to make this work cleanly assuming it's case 1. It might be that your
> current approach is the best though it would be nice to do something in the
> IIO code (with risk of breaking everyone :()

I didn't check if this would be doable.

I don't think we can though
> as we have no way from the trigger implementation side to know if we might
> get threaded interrupt handling or not on the downstream side.
>
> We have reference counting to reenable a trigger that actually has a hardware
> mask at the device end when all consumers are done - that should be used for
> the reenable, not do it in the pollfunc handler. As it's a level interrupt
> you avoid need to do a bonus read in there I think (sometimes that's necessary
> because of an edge trigger and a slow read back on a possible unrelated device).
>
> The subtle difference between IRQF_ONESHOT and irq_disable is one uses
> the irq_mask / unmask callbacks on the irq chip and the other is using
> the enable / disable ones. That may make no practical difference - I'm not
> entirely sure. A quick glance at some drivers suggests masking is usually
> lighter weight as less state is rewrite on reenable.
>
> So in short, move the irq_enable() into the iio_trig->reenable() callback.
>

This should be what I did at v5 :) Thanks for the help!

Yours,
-- Matti

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

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