On 23-12-16 14:39:48, Konrad Dybcio wrote:
> On 14.12.2023 17:49, Abel Vesa wrote:
> > From: Rajendra Nayak <quic_rjendra@xxxxxxxxxxx>
> >
> > Add the camcc clock driver for x1e80100
> >
> > Signed-off-by: Rajendra Nayak <quic_rjendra@xxxxxxxxxxx>
> > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> > ---
> [...]
>
> > +enum {
> > + DT_BI_TCXO,
> > + DT_BI_TCXO_AO,
> > + DT_SLEEP_CLK,
> > +};
> > +
> > +enum {
> > + P_BI_TCXO,
> Please don't overload this define with DT_BI_TCXO_AO, add a new one
> for the active-only clock. Please also do this in other drivers in
> this series.
Nope, that needs to stay if we want to align the dt bindings between
SM8550, SM8650 and this. At least for dispcc. But I would like to have
the same dt schema for the rest of the clock controller drivers between
platforms that share basically the same ip block.
>
> [...]
>
> > + clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
> > + clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
> > + clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
> > + clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
> > + clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
> > + clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap, &cam_cc_pll6_config);
> > + clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap, &cam_cc_pll8_config);
> Do we know whether these configure calls are actually necessary?
> > +
> > + /*
> > + * Keep clocks always enabled:
> > + * cam_cc_gdsc_clk
> > + * cam_cc_sleep_clk
> > + */
> > + regmap_update_bits(regmap, 0x13a9c, BIT(0), BIT(0));
> > + regmap_update_bits(regmap, 0x13ab8, BIT(0), BIT(0));
> Please make the comments inline with each line
>
> Konrad
