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

[Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>]

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@quicinc.com>
>> Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
>> Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
>> ---
>>   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[] = {