Re: [PATCH v2 5/6] clk: qcom: Add camera clock controller driver for SM8150

[Satya Priya Kakitapalli (Temp)]

On 7/3/2024 3:50 PM, Dmitry Baryshkov wrote:
> On Tue, Jul 02, 2024 at 09:20:43PM GMT, Satya Priya Kakitapalli wrote:
> > Add support for the camera clock controller for camera clients
> > to be able to request for camcc clocks on SM8150 platform.
> >
> > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> > Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@xxxxxxxxxxx>
> > ---
> >   drivers/clk/qcom/Kconfig        |    9 +
> >   drivers/clk/qcom/Makefile       |    1 +
> >   drivers/clk/qcom/camcc-sm8150.c | 2159 +++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 2169 insertions(+)
> The patch mostly LGTM, several quesitons:
>
> - There are no cam_cc_sleep_clk and no cam_cc_xo_clk_src. Why?


These are not required for camcc sm8150 hence not modelled.


> - Why is cam_cc_gdsc_clk not modelled in the clock framework?


This clock is kept enabled from probe, hence not required to be modelled 
explicitly.


> - I see that most if not all RCG clocks use rcg2_shared ops instead of
>    using simple rcg2 ops, could you please clarify that?


As per the HW design recommendation, RCG needs to be parked at a safe 
clock source(XO) in the disable path, shared_ops is used to achieve the 
same.


> - RETAIN_FF_ENABLE has been used for GDSCs for sc7280, sc8280xp, sm8550,
>    sm8650 and x1e8 platforms. Should it really be set for sm8150? If so,
>    should it also be added to other camcc drivers (if so, for which
>    platforms)?


I have rechecked this in downstream and seems it is not really needed 
for sm8150, I'll drop in next post.