Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6e2c9f59-296c-7382-6490-8e344f3e0210@gmail.com>
Date:   Mon, 8 May 2023 15:40:05 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Shreeya Patel <shreeya.patel@collabora.com>,
        Zhigang Shi <Zhigang.Shi@liteon.com>,
        Paul Gazzillo <paul@pgazz.com>,
        Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <cover.1683105758.git.mazziesaccount@gmail.com>
 <6d1e37f95dd039d9c96a992b1855fd193bdded40.1683105758.git.mazziesaccount@gmail.com>
 <ZFPCUJ81aw/GkJgT@smile.fi.intel.com>
 <c63c5271-3973-3bd4-c683-ab9ab64b67e4@fi.rohmeurope.com>
 <ZFjpqwOo3DxnWahM@smile.fi.intel.com>
Content-Language: en-US, en-GB
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH v4 4/5] iio: light: ROHM BU27008 color sensor
In-Reply-To: <ZFjpqwOo3DxnWahM@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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