Date: Sat, 9 Mar 2024 17:50:56 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 linux-iio@...r.kernel.org, Shreeya Patel <shreeya.patel@...labora.com>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.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

On Mon, 4 Mar 2024 14:38:38 +0200
Matti Vaittinen <mazziesaccount@...il.com> wrote:

> 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.

ouch. We all have some cancelled products in our history!
When that happens I usually go eat cake and moan at anyone standing
near by. At least this seems like there will be some direct use of
the work done (sometimes you just have to list the things learnt along
the way).

> 
> 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:

*sigh* Not even a version number?  Even unreleased / prototype parts should have
different IDs if anything in the interface changed.

> 
> 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.

Agreed, modify the existing driver. Just needs a clear statement in
patch descriptions that the original part is not expected to be in the wild.

> 
> 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". 

Separate patches preferred for each feature / type of change. Mostly
they'll hopefully be trivial to review.

> Should that compatible be removed and a new one with "anuc"-suffix be 
> added to denote the new sensor?

Yes. The binding patch in particular will need a really clear statement
that we believe there are none in products in the wild.

> 
> 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...

True enough. It's always 'interesting' to not know if / when a product
you've upstreamed code for will launch.

Jonathan

> 
> Yours,
> 	-- Matti
> 

Powered by blists - more mailing lists