Messages in this thread | | | From | Emil Renner Berthing <> | Date | Fri, 22 Oct 2021 15:34:58 +0200 | Subject | Re: [PATCH v2 09/16] reset: starfive-jh7100: Add StarFive JH7100 reset driver |
| |
On Fri, 22 Oct 2021 at 14:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Oct 21, 2021 at 8:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > +static const u32 jh7100_reset_asserted[4] = { > > > + BIT(JH7100_RST_U74 % 32) | > > + BIT(JH7100_RST_VP6_DRESET % 32) | > > + BIT(JH7100_RST_VP6_BRESET % 32), > > It's hard to notice that this is only one entry. See also below.
Yeah, so what would be a better way to style it?
> > + BIT(JH7100_RST_HIFI4_DRESET % 32) | > > + BIT(JH7100_RST_HIFI4_BRESET % 32), > > + > > + BIT(JH7100_RST_E24 % 32) > > + Comma. > > > +}; > > Why all these ugly % 32 against constants?
Because the JH7100_RST_ values goes higher than 31. There is a BIT_MASK macro, but that does % BITS_PER_LONG and this is a 64bit machine.
> ... > > > + if (!assert) > > + done ^= mask; > > Can you convert this to simple > > if (assert) > ret = readl_... > else > ret = readl_... > > below?
I don't see how that would work. We're using the done value in in the readl_poll_timeout. Maybe you can be a bit more explicit.
The reason is that for most reset lines a 0 in the status register means it's asserted and a 1 means it's deasserted. For the few reset lines above this is reversed though.
/Emil
| |