Re: [PATCH v1 1/4] bus: hisi_lpc: Don't dereference fwnode handle

[John Garry]

On 05/07/2022 16:23, Andy Shevchenko wrote:
> On Tue, Jul 5, 2022 at 5:11 PM John Garry<john.garry@xxxxxxxxxx>  wrote:
> > On 05/07/2022 12:43, Andy Shevchenko wrote:
> > > Use dev_fwnode() and acpi_fwnode_handle() instead of dereferencing
> > > an fwnode handle directly.
> > ...which is a better coding practice, right? If so, it would be nice to
> > mention it - well at least I think so.
> Not only. In the case of fwnode it's a long story behind its corner
> case(s) where in the future we might switch from embedded structure to
> linked list, for example, in order to address those corner cases.
> Should I write a paragraph for that as well?

If you just say that it's a better coding practice to use available APIs 
to access structure members rather than access them directly, then that 
is good enough. Or maybe you can think of something better along the 
lines of what you wrote above. My issue is that there was no reason. So 
I'll leave it to you.

> ...
>
> > Apart from above and nit, below:
> See below my answer.
>
> > Acked-by: John Garry<john.garry@xxxxxxxxxx>
> Thanks.
>
> ...
>
> > > -     sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len);
> > > +     sys_port = logic_pio_trans_hwaddr(acpi_fwnode_handle(host), res->start, len);
> > nit: currently the driver keeps to the old 80 character line limit.
> > While the rules may have been relaxed, I'd rather we still maintain it.
> First of all, even before the 100 characters era the rule had two exceptions:
> 1) the string literals;

Sure

> 2) the readability over strictness of the 80 characters rule.
>
> While I agree in general with you, in this case I think keeping
> strictness makes readability worse.

ok, fine. I was going to suggest introduce a varible to hold 
acpi_fwnode_handle(host) but then we may not want a variable of 
fwnode_handle* type hanging around. Anyway I don't feel too strongly 
about it.

Thanks,
John