Re: [PATCH 3/6] soc: qcom: geni-se: add desc struct to specify clocks from device match data

[Neil Armstrong <neil.armstrong@linaro.org>]

On 16/11/2022 11:32, Konrad Dybcio wrote:
> 
> 
> On 16/11/2022 11:21, Neil Armstrong wrote:
>> The I2C Master Hub is a stripped down version of the GENI Serial Engine
>> QUP Wrapper Controller but only supporting I2C serial engines without
>> DMA support.
>>
>> This prepares support for the I2C Master Hub variant, by moving
>> the required clocks list to a new desc struct then passing it through the
>> compatible match data.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/soc/qcom/qcom-geni-se.c | 57 +++++++++++++++++++++++++++++++----------
>>   1 file changed, 43 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> index a0ceeede450f..f4f54d92a01a 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -81,19 +81,31 @@
>>    */
>>   #define MAX_CLK_PERF_LEVEL 32
>> -#define NUM_AHB_CLKS 2
>> +#define MAX_CLKS 2
>>   /**
>>    * struct geni_wrapper - Data structure to represent the QUP Wrapper Core
>>    * @dev:        Device pointer of the QUP wrapper core
>>    * @base:        Base address of this instance of QUP wrapper core
>> - * @ahb_clks:        Handle to the primary & secondary AHB clocks
>> + * @clks:        Handle to the primary & optional secondary AHB clocks
>> + * @num_clks:        Count of clocks
>>    * @to_core:        Core ICC path
>>    */
>>   struct geni_wrapper {
>>       struct device *dev;
>>       void __iomem *base;
>> -    struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
>> +    struct clk_bulk_data clks[MAX_CLKS];
>> +    unsigned int num_clks;
>> +};
>> +
>> +/**
>> + * struct geni_se_desc - Data structure to represent the QUP Wrapper resources
>> + * @clks:        Name of the primary & optional secondary AHB clocks
>> + * @num_clks:        Count of clock names
>> + */
>> +struct geni_se_desc {
>> +    unsigned int num_clks;
>> +    const char * const *clks;
>>   };
>>   static const char * const icc_path_names[] = {"qup-core", "qup-config",
>> @@ -496,8 +508,7 @@ static void geni_se_clks_off(struct geni_se *se)
>>       struct geni_wrapper *wrapper = se->wrapper;
>>       clk_disable_unprepare(se->clk);
>> -    clk_bulk_disable_unprepare(ARRAY_SIZE(wrapper->ahb_clks),
>> -                        wrapper->ahb_clks);
>> +    clk_bulk_disable_unprepare(wrapper->num_clks, wrapper->clks);
>>   }
>>   /**
>> @@ -528,15 +539,13 @@ static int geni_se_clks_on(struct geni_se *se)
>>       int ret;
>>       struct geni_wrapper *wrapper = se->wrapper;
>> -    ret = clk_bulk_prepare_enable(ARRAY_SIZE(wrapper->ahb_clks),
>> -                        wrapper->ahb_clks);
>> +    ret = clk_bulk_prepare_enable(wrapper->num_clks, wrapper->clks);
>>       if (ret)
>>           return ret;
>>       ret = clk_prepare_enable(se->clk);
>>       if (ret)
>> -        clk_bulk_disable_unprepare(ARRAY_SIZE(wrapper->ahb_clks),
>> -                            wrapper->ahb_clks);
>> +        clk_bulk_disable_unprepare(wrapper->num_clks, wrapper->clks);
>>       return ret;
>>   }
>> @@ -887,11 +896,21 @@ static int geni_se_probe(struct platform_device *pdev)
>>           return PTR_ERR(wrapper->base);
>>       if (!has_acpi_companion(&pdev->dev)) {
>> -        wrapper->ahb_clks[0].id = "m-ahb";
>> -        wrapper->ahb_clks[1].id = "s-ahb";
>> -        ret = devm_clk_bulk_get(dev, NUM_AHB_CLKS, wrapper->ahb_clks);
>> +        const struct geni_se_desc *desc;
>> +        int i;
>> +
>> +        desc = device_get_match_data(&pdev->dev);
>> +        if (!desc)
>> +            return -EINVAL;
>> +
>> +        wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS);
> Maybe a warning would be suitable if num mismatch happens?

Indeed, will add that, thx.

> 
> Other than that,
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> 
> Konrad
>> +
>> +        for (i = 0; i < wrapper->num_clks; ++i)
>> +            wrapper->clks[i].id = desc->clks[i];
>> +
>> +        ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
>>           if (ret) {
>> -            dev_err(dev, "Err getting AHB clks %d\n", ret);
>> +            dev_err(dev, "Err getting clks %d\n", ret);
>>               return ret;
>>           }
>>       }
>> @@ -901,8 +920,18 @@ static int geni_se_probe(struct platform_device *pdev)
>>       return devm_of_platform_populate(dev);
>>   }
>> +static const char * const qup_clks[] = {
>> +    "m-ahb",
>> +    "s-ahb",
>> +};
>> +
>> +static const struct geni_se_desc qup_desc = {
>> +    .clks = qup_clks,
>> +    .num_clks = ARRAY_SIZE(qup_clks),
>> +};
>> +
>>   static const struct of_device_id geni_se_dt_match[] = {
>> -    { .compatible = "qcom,geni-se-qup", },
>> +    { .compatible = "qcom,geni-se-qup", .data = &qup_desc },
>>       {}
>>   };
>>   MODULE_DEVICE_TABLE(of, geni_se_dt_match);
>>