On Wed, 1 Mar 2023 at 00:58, Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx> wrote: > > Hi Joel, > > Thanks for the review. Some replies inline: > > > > @@ -15,7 +16,7 @@ > > > > > > #include "clk-aspeed.h" > > > > > > -#define ASPEED_G6_NUM_CLKS 71 > > > +#define ASPEED_G6_NUM_CLKS 72 > > > > NUM_CLKS seems dangerous. Should we instead use > > ARRAY_SIZE(aspeed_g6_gates)? > > Yep, that would have saved me some time debugging. That would suit as a > separate change though, would you like it in the same series? Doesn't matter much. Perhaps include it at the end, for both the aspeed drivers? But separately is fine too. > > > > /* USB 2.0 port1 phy 40MHz clock */ > > > hw = clk_hw_register_fixed_rate(NULL, "usb-phy-40m", NULL, > > > 0, 40000000); > > > aspeed_g6_clk_data->hws[ASPEED_CLK_USBPHY_40M] = hw; > > > + > > > + /* i3c clock: source from apll, divide by 8 */ > > > + regmap_read(map, ASPEED_G6_CLK_SELECTION5, &val); > > > + val &= ~(I3C_CLK_SELECTION | APLL_DIV_SELECTION); > > > > Is there any value in registering a mux device here? See the emmc > > extclk device. > > We won't be doing any mux configuration here, so I figure the static > setup is fine. ack > > > > + val |= FIELD_PREP(I3C_CLK_SELECTION, > > > I3C_CLK_SELECT_APLL_DIV); > > > + val |= FIELD_PREP(APLL_DIV_SELECTION, APLL_DIV_8); > > > + regmap_write(map, ASPEED_G6_CLK_SELECTION5, val); > > > > This is a departure in style from the existing code. The existing > > code did things like this: > > > > regmap_update_bits(map, ASPEED_G6_CLK_SELECTION1, GENMASK(10, 8), BIT(10)); > > > > Which uses the regmap API instead of FIELD_PREP macros. > > Yep, that's much nicer, I'll change. The FIELD_PREP parts are just from > the initial ASPEED implementation. Cool.
