Hi Uwe, On Tue, Jun 11, 2019 at 6:33 PM Uwe Kleine-König <u.kleine-koe...@pengutronix.de> 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
