Re: [PATCH v4 4/5] iio: light: ROHM BU27008 color sensor

[Matti Vaittinen <mazziesaccount@gmail.com>]

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@fi.rohmeurope.com/
>>
>> 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! ~~