Re: [PATCH v4 4/5] iio: light: ROHM BU27008 color sensor
From: Matti Vaittinen
Date: Mon May 08 2023 - 08:40:45 EST
Hi Andy,
On 5/8/23 15:23, Andy Shevchenko wrote:
On Fri, May 05, 2023 at 04:56:47AM +0000, Vaittinen, Matti wrote:
On 5/4/23 17:33, Andy Shevchenko wrote:
On Wed, May 03, 2023 at 12:50:14PM +0300, Matti Vaittinen wrote:
...
+config ROHM_BU27008
+ tristate "ROHM BU27008 color (RGB+C/IR) sensor"
+ depends on I2C
+ select REGMAP_I2C
+ select IIO_GTS_HELPER
+ help
+ Enable support for the ROHM BU27008 color 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.
Module name?
We have discussed this several times already.
https://lore.kernel.org/all/10c4663b-dd65-a545-786d-10aed6e6e5e9@xxxxxxxxxxxxxxxxx/
Module name is completely irrelevant when selecting a kernel configuration.
This option is also selectable by user.
I don't think the name is selectable. Yes, user selects whether to
compile driver as a module or in-kernel - but the module name is
completely irrelevant what comes to this decision.
...
Do you need regmap lock? If so, why (since you have mutex)?
I believe you know that regmap uses a default lock when no external lock
is given. So, I assume you mean that maybe we could set
'disable_locking' for the regmap here.
Correct.
It's nice to be occasionally pushed to think "out of the box". And yes,
disabling regmap lock is really out of my "normal box" :)
I didn't go through all of the code yet, but I think pretty much all of
the sequences which end up to register writes are indeed protected by
the mutex. (Well, probe is not but it is expected to only update one bit
while rest of the register should stay fixed).
It may be we could live without regmap_lock when driver is in it's
current state, but I am not convinced the performance improvement is
worth the risk. Having regmap unprotected is not common, and it is also
not easy to spot when making changes to the driver. In my opinion it is
a bit like asking for a nose-bleed unless there is really heavy reasons
to drop the lock... In this case, having the regmap_lock (which is
pretty much never locked because we have the mutex as you said) is
probably not a penalty that matters.
Basically you try to justify a hidden mine field in case somebody will think
"oh, we are protected by regmap lock, so why to bother call mutex_lock()" and
at the end it become a subtle bugs in the code. With disable_locking = true
I can see that code author _carefully thought through_ the locking schema and
understands the hardware and the code.
I added the disable_locking = true in v5 - but I am not convinced that
was a great idea. I am afraid disabling regmap lock is the hidden
minefield for average users. I didn't grep the kernel for it but I am
afraid the percentage of regmap users who disable locking is very low.
Thus, I'd say this is unexpected to many and may lead to bugs although I
try to watch out for them. Well, time will tell.
P.S. I'm wondering why your lines of text have a single trailing whitespace
but the last line.
I guess it must be Thunderbird client then. Well, at least it can send
out plain-text decently well while working with the exchange servers
used by ROHM as well as with the gmail. I am not super happy with
Thunderbird, it tends to eat way more resources I wished it did, but it
is a working compromise for me. I am interested in hearing if anyone
knows a way to configure the Thunderbird to drop these extra spaces.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~