Received-SPF: pass (google.com: domain of linux-kernel+bounces-35037-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <b5decdec-f33a-4997-a607-e31290fcd83c@smile.fr>
Date: Tue, 23 Jan 2024 10:48:50 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] regulator: ti-abb: don't use
 devm_platform_ioremap_resource_byname for shared interrupt register
Content-Language: en-US
To: Mark Brown <broonie@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
 tony@atomide.com, lgirdwood@gmail.com, Romain Naour <romain.naour@skf.com>
References: <20240122170442.729374-1-romain.naour@smile.fr>
 <1b2b5afc-308f-48bb-924a-2c29371abfc9@sirena.org.uk>
From: Romain Naour <romain.naour@smile.fr>
In-Reply-To: <1b2b5afc-308f-48bb-924a-2c29371abfc9@sirena.org.uk>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hello,

Le 22/01/2024 à 18:30, Mark Brown a écrit :
> On Mon, Jan 22, 2024 at 06:04:42PM +0100, Romain Naour wrote:
> 
>> We can't use devm_platform_ioremap_resource_byname() to remap the
>> interrupt register that can be shared between
>> regulator-abb-{ivahd,dspeve,gpu} drivers instance.
> 
> ...
> 
>> The commit b36c6b1887ff (regulator: ti-abb: Make use of the helper
>> function devm_ioremap related) overlooked the following comment
>> explaining why devm_ioremap() is used in this case:
> 
>> /*
>>  * We may have shared interrupt register offsets which are
>>  * write-1-to-clear between domains ensuring exclusivity.
>>  */
> 
> I have to say that I wouldn't infer from that comment that there is any
> reason why _byname() won't work - one would generally expect that a
> get_resource_by_name() followed by an ioremap() of that resource would
> be equivalent to the combined helper.  Based on the commit log here I
> frankly have no idea what the issue is.  You should also add something
> to the code which makes it clear what the issue is so the same
> conversion isn't performed again, assuming that the fix isn't in the
> helper.

I'm agree with you about the existing comment that is not really crystal clear.

The combined helper introduce a call to devm_request_mem_region() that create a
new busy resource region on PRM_IRQSTATUS_MPU register (0x4ae06010). The first
devm_request_mem_region() call succeed for regulator-abb-ivahd but fail for the
two other regulator-abb-dspeve and regulator-abb-gpu.

Here is the iomem content without this patch:
# cat /proc/iomem | grep -i 4ae06
4ae06010-4ae06013 : 4ae07e34.regulator-abb-ivahd int-address
4ae06014-4ae06017 : 4ae07ddc.regulator-abb-mpu int-address

regulator-abb-dspeve and regulator-abb-gpu are missing due to
devm_request_mem_region() failure (EBUSY)

I don't know how to fix this issue keeping
devm_platform_ioremap_resource_byname() when the same address is used several
time... suggestion welcome.

> 
>>
>> Fixes:
> 
> You're missing the commit here.
> 
>> This partially reverts commit b36c6b1887ffc6b58b556120bfbd511880515247.
> 
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.

I added such human description above in the commit log but forgot to update this
one, sorry.

Thank you for the review.

Best regards,
Romain