From: Alex Elder <elder@xxxxxxxxxx>
Date: Thu, 16 Feb 2023 12:11:11 -0600
> On 2/16/23 11:51 AM, Alexander Lobakin wrote:
>> From: Alex Elder <elder@xxxxxxxxxx>
>> Date: Wed, 15 Feb 2023 13:53:48 -0600
[...]
>>> gsi->regs = gsi_regs(gsi);
>>> if (!gsi->regs) {
>>> dev_err(dev, "unsupported IPA version %u (?)\n",
>>> gsi->version);
>>> return -EINVAL;
>>> }
>>> - gsi->virt_raw = ioremap(res->start, size);
>>> - if (!gsi->virt_raw) {
>>> + gsi->virt = ioremap(res->start, size);
>>
>> Now that at least one check above went away and the second one might be
>> or be not correct (I thought ioremap core takes care of this), can't
>> just devm_platform_ioremap_resource_byname() be used here for simplicity?
>
> Previously, virt_raw would be the "real" re-mapped pointer, and then
> virt would be adjusted downward from that. It was a weird thing to
> do, because the result pointed to a non-mapped address. But all uses
> of the virt pointer added an offset that was enough to put the result
> into the mapped range.
>
> The new code updates all offsets to account for what the adjustment
> previously did. The test that got removed isn't necessary any more.
Yeah I got it, just asked that maybe you can now use
platform_ioremap_resource_byname() instead of
platform_get_resource_byname() + ioremap() :)
>
>>
>>> + if (!gsi->virt) {
>>> dev_err(dev, "unable to remap \"gsi\" memory\n");
>>> return -ENOMEM;
>>> }
>>> - /* Most registers are accessed using an adjusted register range */
>>> - gsi->virt = gsi->virt_raw - adjust;
>>> return 0;
>>> }
>>> @@ -170,7 +145,7 @@ int gsi_reg_init(struct gsi *gsi, struct
>>> platform_device *pdev)
>>> /* Inverse of gsi_reg_init() */
>>> void gsi_reg_exit(struct gsi *gsi)
>>> {
>>> + iounmap(gsi->virt);
>>
>> (don't forget to remove this unmap if you decide to switch to devm_)
>
> As far as devm_*() calls, I don't use those anywhere in the driver
> currently. If I were going to use them in one place I'd want do
> it consistently, everywhere. I don't want to do that.
+
>
>>> gsi->virt = NULL;
>>> - iounmap(gsi->virt_raw);
>>> - gsi->virt_raw = NULL;
>>> + gsi->regs = NULL;
[...]
>> (offtopic)
>>
>> I hope all those gsi_reg-v*.c are autogenerated? They look pretty scary
>> to be written and edited manually each time :D
>
> I know they look scary, but no, they're manually generated and
> it's a real pain to review them. I try to be consistent enough
> that a "diff" is revealing and helpful. For the GSI registers,
> most of them don't change (until IPA v5.0). I intend to modify
> this a bit further so that registers that are the same as the
> previous version don't have to be re-stated (so each new version
> only has to highlight the differences).
No, it's +/- okay to review, as you say, they're pretty consistent in
terms of code.
>
> All that said, once created, they don't change.
>
> Thanks.
>
> -Alex
Thanks,
Olek
