Date: Mon, 4 Mar 2024 14:38:38 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc: linux-iio@...r.kernel.org, Shreeya Patel <shreeya.patel@...labora.com>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Jonathan Cameron <jic23@...nel.org>, devicetree@...r.kernel.org,
 Lars-Peter Clausen <lars@...afoo.de>, Paul Gazzillo <paul@...zz.com>,
 Rob Herring <robh+dt@...nel.org>,
 Dmitry Osipenko <dmitry.osipenko@...labora.com>,
 linux-kernel@...r.kernel.org,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v7 0/5] Support ROHM BU27034 ALS sensor

Hi deee Ho peeps!

On 3/31/23 15:40, Matti Vaittinen wrote:
> Support ROHM BU27034 ALS sensor
> 
> This series adds support for ROHM BU27034 Ambient Light Sensor.

I have one word for all of you who worked to get the ROHM BU27034NUC 
driver working in upstream.

Meh.

I just found out that the BU27034 sensor which was developed when I 
wrote this driver had some "manufacturing issues"... The full model 
number was BU27034NUC. The has been cancelled, and, as far as I know, no 
significant number of those were manufactured.

The issues of BU27034NUC were solved, and new model BU27034ANUC was 
developed and is available in the ROHM catalog.

I did also learn that this new model BU27034ANUC is _not_ functionally 
equivalent to the BU27034NUC. I am currently clarifying all the 
differences, and I have requested the HQ to send me a sample for driver 
development and verification work.

This far I've come to know at least following differences:

- The DATA2 (IR) channel is removed. So is the gain setting for it. This
   should very much simplify the gain logic.
- Some of the gains were removed.
- The 5ms integration time was removed. (The support of 5ms was severely
   limited on original BU27034NUC too so driver did not support that
   anyways).
- The light sensitivity curves had changed so the lux calculation will
   be changed.

One thing that has _not_ changed though is the part-id :rolleyes:

My preferred approach would be to convert the in-tree bu27034 driver to 
support this new variant. I think it makes sense because:
- (I expect) the amount of code to review will be much smaller this way
   than it would be if current driver was completely removed, and new one
   submitted.
- This change will not break existing users as there should not be such
   (judging the statement that the original BU27034NUC was cancelled
   before it was sold "en masse").

It sure is possible to drop the existing driver and submit a new one 
too, but I think it will be quite a bit more work with no strong benefits.

I expect the rest of the information to be shared to me during the next 
couple of days, and I hope I can start testing the driver immediately 
when I get the HW.

My question is, do you prefer the changes to be sent as one "support 
BU27034ANUC patch, of would you rather see changes splitted to pieces 
like: "adapt lux calculation to support BU27034ANUC", "remove obsolete 
DATA2 channel", "remove unsupported gains"...? Furthermore, the DT 
compatible was just rohm,bu27034 and did not include the ending "nuc". 
Should that compatible be removed and a new one with "anuc"-suffix be 
added to denote the new sensor?

I am truly sorry for all the unnecessary reviewing and maintenance work 
you guys did. I can assure you I didn't go through it for fun either - 
even if the coding was fun :) I guess even the "upstream early" process 
has it's weaknesses...

Yours,
	-- Matti

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

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

Powered by blists - more mailing lists