Re: [PATCH 1/2] bu27034: ROHM BU27034NUC to BU27034ANUC

[Matti Vaittinen]

On 6/15/24 20:47, Jonathan Cameron wrote:
> On Mon, 10 Jun 2024 13:01:23 +0300
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
>
> > The ROHM BU27034NUC was cancelled and BU27034ANUC is replacing this
> > sensor. Use the BU27034NUC driver to support the new BU27034ANUC.
> >
> > According to ROHM, the BU27034NUC was never mass-produced. Hence dropping
> > the BU27034NUC support and using this driver to support BU27034ANUC
> > should not be a problem to users.
> >
> > Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> > Fixes: e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")
>
> This is an odd case.  I don't think a fixes tag is appropriate

Last week I was thinking this purely from a new BU27034ANUC user's point 
of view. For user of the new sensor it appears the current driver is 
broken. Hence this felt like a fix. That was last week though...

 and I
> don't think we can use the original compatible.

I didn't think so either back in March. However, I forgot what so ever 
plans I had while I was waiting for some internal decision regarding 
upstreaming of this... I just went back to the mail I sent about this in 
March, and I see we discussed adding new compatible back then, and I 
also promised to send this patch as a series of smaller changes... Sorry!

>  I don't mind breaking
> support for the non existent port going forwards and indeed dropping
> all indication it ever existed, but the old kernel's are out there and
> even getting this into stable is far from a guarantee there won't be
> a kernel run on a board that has this compatible but has the old
> driver.

I agree it's not a guarantee, but it would be the best we can do. At 
least some stable users would upgrade - have upgraded - could pick 
stable with fixed driver.

>  It's also too big really to be stable material.

I am not really going to argue on that. Asking for the stable 
maintainers to look at this and port it is indeed a bit too much. I 
guess we just need to live with having the b0rked version out there.

> So I think the path forwards is a new compatible and drop the old
> one from the dt bindings and driver.  Thus any new dts for a board
> that actually has this device will use the new compatible and avoid
> any risk of encountering the old driver.

Yes. This should be the way forward.

> Maybe we can be more relaxed - what actually happens if you use the
> existing driver with the new part?

I am pretty sure the world does not explode. I've not tried this so I am 
not sure if reading the register area where the removed data channel 
used to be is succeeding. My assumption is it does, but just returns 
garbage. Furthermore, I am not sure what happens if those removed gains 
are tried to be set.

So, not tried but I would guess that the read data would just be insane.

> I'm trusting you copied the maths right for the computed
> channels (that take too long to review!)

I understand the reviewing problem. I feel it is time consuming and 
sometimes very much energy draining. :) And I _really_ appreciate the 
work you do with reviewing and maintaining! But trusting me? I suppose 
we all make mistakes XD

>  So everything inline is
> formatting type stuff.

Thanks! I will fix the compatible and formatting. I agree with all of 
the comments - but it may take a while until I send the next version. 
I'm having a vacation and trying to spend my time AFK for next couple of 
weeks. My old motorbike and tiny boat are calling for me - so amount of 
code I write is (probably) inversely proportional to the amount of 
sunshine in Finland ;)

> >   	/*
> >   	 * The BU27034 DATA0 and DATA1 channels are both on the visible light
> >   	 * area (mostly). The data0 sensitivity peaks at 500nm, DATA1 at 600nm.
> > -	 * These wave lengths are pretty much on the border of colours making
> > -	 * these a poor candidates for R/G/B standardization. Hence they're both
> > -	 * marked as clear channels
> > +	 * These wave lengths are cyan(ish) and orange(ish), making these
> > +	 * sub-optiomal candidates for R/G/B standardization. Hence they're
> > +	 * both marked as clear channels.
>
> I think just indexing them and not giving a modifier is probably better than
> claiming they are clear.  Leave it more vague basically.

Agree. I just didn't see leaving out the modifier as an option.

> >   	 */
> > -	BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR),
> > -	BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR),
> > -	BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR),
> > +	BU27034_CHAN_DATA(DATA0),
> > +	BU27034_CHAN_DATA(DATA1),
> >   	IIO_CHAN_SOFT_TIMESTAMP(4),
> >   };

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

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