Re: [PATCH 4/5] clk: qcom: camcc-sm8650: Add camera clock controller driver for SM8650

[Jagadeesh Kona]

On 2/12/2024 6:46 PM, Dmitry Baryshkov wrote:
> On Mon, 12 Feb 2024 at 15:09, Jagadeesh Kona <quic_jkona@xxxxxxxxxxx> wrote:
> > On 2/7/2024 6:41 PM, Bryan O'Donoghue wrote:
> > > On 06/02/2024 11:31, Jagadeesh Kona wrote:
> > > > Add support for the camera clock controller for camera clients to be
> > > > able to request for camcc clocks on SM8650 platform.
> > > >
> > > > Signed-off-by: Jagadeesh Kona <quic_jkona@xxxxxxxxxxx>
> > >
> > > > +static struct clk_rcg2 cam_cc_mclk1_clk_src = {
> > > > +    .cmd_rcgr = 0x1501c,
> > > > +    .mnd_width = 8,
> > > > +    .hid_width = 5,
> > > > +    .parent_map = cam_cc_parent_map_1,
> > > > +    .freq_tbl = ftbl_cam_cc_mclk0_clk_src,
> > > > +    .clkr.hw.init = &(const struct clk_init_data) {
> > > > +        .name = "cam_cc_mclk1_clk_src",
> > > > +        .parent_data = cam_cc_parent_data_1,
> > > > +        .num_parents = ARRAY_SIZE(cam_cc_parent_data_1),
> > > > +        .flags = CLK_SET_RATE_PARENT,
> > > > +        .ops = &clk_rcg2_shared_ops,
> > >
> > > Nice.
> > >
> > > I compared this to WIP for x1e80100 which looks nearly register
> > > compatible. Use of the shared_ops indicates to me you've thought about
> > > which clocks should not be switched all the way off.
> >
> > Thanks Bryan for your review, We want all RCG's to be parked at safe
> > config(XO) when they are disabled, hence using shared ops for all the
> > RCG's.
>
> What is the reason for parking it instead of fully disabling the clock?

We don't do anything explicit in RCG disable, normally when all branch 
clocks are disabled, RCG gets disabled in HW. But as per the HW design 
recommendation, RCG needs to be parked at a safe clock source(XO) during 
disable path, hence we use shared_ops to achieve the same. After parking 
at XO, RCG gets disabled as all the branches are disabled.

Thanks,
Jagadeesh

> > > > +static struct platform_driver cam_cc_sm8650_driver = {
> > > > +    .probe = cam_cc_sm8650_probe,
> > > > +    .driver = {
> > > > +        .name = "cam_cc-sm8650",
> > >
> > > That said .. please fix the name here "cam_cc-sm8650". The title of your
> > > series is "camcc-sm8650" which IMO is a much more appropriate name.
> > >
> > > The admixture of hyphen "-" and underscore "_" is some kind of
> > > tokenisation sin.
> >
> > Sure, will fix this in next series.
> >
> > Thanks,
> > Jagadeesh
> >
> > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>