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! ~~
