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? > > /* 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. > > + 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. Cheers, Jeremy
