Re: [PATCH v2 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK

[Martin Blumenstingl]

Hi Uwe,

On Tue, Jun 11, 2019 at 6:33 PM Uwe Kleine-KÃnig
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
[...]
> > @@ -463,7 +463,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
> >
> >               channel->mux.reg = meson->base + REG_MISC_AB;
> >               channel->mux.shift = mux_reg_shifts[i];
> > -             channel->mux.mask = BIT(MISC_CLK_SEL_WIDTH) - 1;
> > +             channel->mux.mask = MISC_CLK_SEL_MASK;
> >               channel->mux.flags = 0;
> >               channel->mux.lock = &meson->lock;
> >               channel->mux.table = NULL;
>
> IMHO clk_mux is ugly here. It could easily just take
>
>         .mask = 3 << mux_reg_shifts[i],
in most cases that would be even nicer to read because it could be expressed as:
  .mask = GENMASK(5, 4)

so I like your idea in general
though I think it should not block this patch

[...]
> Apart from that, I wonder if the pwm-meson driver should better use
> clk_register_mux instead of open coding it. (Though there doesn't seem
> to exists a devm_ variant of it.)
I tried to use clk_register_mux in the past. it works but it's not as
nice to read as the open-coded variant because it takes 10 parameters.
I find it easier to read 13 separate lines compared to reading a
function call with 10 parameters


Martin