Re: [PATCH v3 04/10] PCI: apple: Add initial hardware bring-up

[Hector Martin]

On 17/09/2021 18.20, Marc Zyngier wrote:
> On Mon, 13 Sep 2021 21:48:47 +0100,
> > On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote:
> > > +static inline void rmwl(u32 clr, u32 set, void __iomem *addr)
> > > +{
> > > +	writel_relaxed((readl_relaxed(addr) & ~clr) | set, addr);
> > > +}
> >
> > This helper is a bit strange, especially since it's always only used
> > with either clr != 0 or set != 0 but never (clr = 0 and set = 0) afaict.
> > Maybe create two instead for setting and clearing bits?
>
> That's indeed nicer, and it makes the code more readable.

This seems to be a pattern in Corellium code; the cpufreq code is the 
same and I honestly found it quite hard to read when used for simple set 
or clear operations. I find set32/clear32 style helpers much more 
readable, so it's probably a good idea to make this change any time we 
upstream stuff derived from their tree.

--
Hector Martin (marcan@xxxxxxxxx)
Public Key: https://mrcn.st/pub