Re: [PATCH v6 7/7] pinctrl: qcom: Update clock voting as optional

[Srinivasa Rao Mandadapu <quic_srivasam@xxxxxxxxxxx>]

On 2/16/2022 7:50 PM, Srinivas Kandagatla wrote:
Thanks for Your Time Srini!!!
> On 14/02/2022 15:19, Srinivasa Rao Mandadapu wrote:
> > Update bulk clock voting to optional voting as ADSP bypass platform 
> > doesn't
> > need macro and decodec clocks, these are maintained as power domains and
> > operated from lpass audio core cc.
> >
> > Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@xxxxxxxxxxx>
> > Co-developed-by: Venkata Prasad Potturu <quic_potturu@xxxxxxxxxxx>
> > Signed-off-by: Venkata Prasad Potturu <quic_potturu@xxxxxxxxxxx>
> > ---
> >   drivers/pinctrl/qcom/pinctrl-lpass-lpi.c        | 16 +++++++++-------
> >   drivers/pinctrl/qcom/pinctrl-lpass-lpi.h        |  1 +
> >   drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c |  1 +
> >   3 files changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c 
> > b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c
> > index 8a82fd9..103f0a6c 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c
> > @@ -407,13 +407,15 @@ int lpi_pinctrl_probe(struct platform_device 
> > *pdev)
> >           return dev_err_probe(dev, PTR_ERR(pctrl->slew_base),
> >                        "Slew resource not provided\n");
> >   -    ret = devm_clk_bulk_get(dev, MAX_LPI_NUM_CLKS, pctrl->clks);
> > -    if (ret)
> > -        return dev_err_probe(dev, ret, "Can't get clocks\n");
> > -
> > -    ret = clk_bulk_prepare_enable(MAX_LPI_NUM_CLKS, pctrl->clks);
> > -    if (ret)
> > -        return dev_err_probe(dev, ret, "Can't enable clocks\n");
> > +    if (!data->is_clk_optional) {
> > +        ret = devm_clk_bulk_get(dev, MAX_LPI_NUM_CLKS, pctrl->clks);
> > +        if (ret)
> > +            return dev_err_probe(dev, ret, "Can't get clocks\n");
> > +
> > +        ret = clk_bulk_prepare_enable(MAX_LPI_NUM_CLKS, pctrl->clks);
> > +        if (ret)
> > +            return dev_err_probe(dev, ret, "Can't enable clocks\n");
> > +    }
> >         pctrl->desc.pctlops = &lpi_gpio_pinctrl_ops;
> >       pctrl->desc.pmxops = &lpi_gpio_pinmux_ops;
> > diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h 
> > b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h
> > index a511d72..c1079bf 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h
> > +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h
> > @@ -77,6 +77,7 @@ struct lpi_pinctrl_variant_data {
> >       int ngroups;
> >       const struct lpi_function *functions;
> >       int nfunctions;
> > +    int is_clk_optional;
> >   };
> >     int lpi_pinctrl_probe(struct platform_device *pdev);
> > diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c 
> > b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c
> > index 5bf30d97..4277e31 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c
> > @@ -143,6 +143,7 @@ static const struct lpi_pinctrl_variant_data 
> > sc7280_lpi_data = {
> >       .ngroups = ARRAY_SIZE(sc7280_groups),
> >       .functions = sc7280_functions,
> >       .nfunctions = ARRAY_SIZE(sc7280_functions),
> > +    .is_clk_optional = 1,
>
> This is forcefully set assuming that sc7280 is always used in ADSP 
> bypass mode. Which is not correct.
>
> Can't you use devm_clk_bulk_get_optional instead?

Yes. Agreed. Initially used devm_clk_bulk_get_optional, but Bjorn 
suggested for conditional check instead of optional.

Again Shall we go for optional clock voting?

> --srini
>
> >   };
> >     static const struct of_device_id lpi_pinctrl_of_match[] = {