Re: [PATCH net-next 2/6] net: ipa: kill gsi->virt_raw

[Alexander Lobakin]

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