| Message ID | 20240122170442.729374-1-romain.naour@smile.fr (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | regulator: ti-abb: don't use devm_platform_ioremap_resource_byname for shared interrupt register | expand |
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. > > 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.
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
diff --git a/drivers/regulator/ti-abb-regulator.c b/drivers/regulator/ti-abb-regulator.c index f48214e2c3b4..21392b9261f4 100644 --- a/drivers/regulator/ti-abb-regulator.c +++ b/drivers/regulator/ti-abb-regulator.c @@ -726,9 +726,22 @@ static int ti_abb_probe(struct platform_device *pdev) return PTR_ERR(abb->setup_reg); } - abb->int_base = devm_platform_ioremap_resource_byname(pdev, "int-address"); - if (IS_ERR(abb->int_base)) - return PTR_ERR(abb->int_base); + pname = "int-address"; + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname); + if (!res) { + dev_err(dev, "Missing '%s' IO resource\n", pname); + return -ENODEV; + } + /* + * We may have shared interrupt register offsets which are + * write-1-to-clear between domains ensuring exclusivity. + */ + abb->int_base = devm_ioremap(dev, res->start, + resource_size(res)); + if (!abb->int_base) { + dev_err(dev, "Unable to map '%s'\n", pname); + return -ENOMEM; + } /* Map Optional resources */ pname = "efuse-address";