On 4/22/22 16:36, Fabien Dessenne wrote:
> Instead of enabling/disabling the clock at each IO configuration update,
> just keep the clock enabled from the probe.
> This makes things simpler and more efficient (e.g. the time required to
> toggle an output IO is drastically decreased) without significantly
> increasing the power consumption.
[...]
> static struct irq_domain *stm32_pctrl_get_irq_domain(struct device_node *np)
> @@ -1575,6 +1537,10 @@ int stm32_pctl_probe(struct platform_device *pdev)
> ret = stm32_gpiolib_register_bank(pctl, child);
> if (ret) {
> of_node_put(child);
> +
> + for (i = 0; i < pctl->nbanks; i++)
> + clk_disable_unprepare(pctl->banks[i].clk);
> +
There are clk_bulk_*() functions, maybe you can use those to get rid of
these loops ?
The rest looks good to me.
Hi Marek
On 22/04/2022 18:26, Marek Vasut wrote:
> On 4/22/22 16:36, Fabien Dessenne wrote:
>> Instead of enabling/disabling the clock at each IO configuration update,
>> just keep the clock enabled from the probe.
>> This makes things simpler and more efficient (e.g. the time required to
>> toggle an output IO is drastically decreased) without significantly
>> increasing the power consumption.
>
> [...]
>
>> static struct irq_domain *stm32_pctrl_get_irq_domain(struct
>> device_node *np)
>> @@ -1575,6 +1537,10 @@ int stm32_pctl_probe(struct platform_device *pdev)
>> ret = stm32_gpiolib_register_bank(pctl, child);
>> if (ret) {
>> of_node_put(child);
>> +
>> + for (i = 0; i < pctl->nbanks; i++)
>> + clk_disable_unprepare(pctl->banks[i].clk);
>> +
>
> There are clk_bulk_*() functions, maybe you can use those to get rid of
> these loops ?
This sounds goods, but checking more in details I see that moving to the
'bulk' implementation would require to move the clk information from the
"struct stm32_gpio_bank *banks" member to its parent "struct stm32_pinctrl".
This would make the clk device information stored in a different
structure from the other device-related information (base address, reset
control, ...). It's better to keep all those information together in the
same struct.
As another drawback we would loose access to 'clk' from any function
that have 'bank' (or 'struct gpio_chip *chip') as input parameter (e.g.
stm32_gpio_get() called from gpiolib).
So I really prefer to keep the current implementation.
BR
Fabien
>
> The rest looks good to me.
On 4/25/22 11:27, Fabien DESSENNE wrote:
> Hi Marek
>
>
> On 22/04/2022 18:26, Marek Vasut wrote:
>> On 4/22/22 16:36, Fabien Dessenne wrote:
>>> Instead of enabling/disabling the clock at each IO configuration update,
>>> just keep the clock enabled from the probe.
>>> This makes things simpler and more efficient (e.g. the time required to
>>> toggle an output IO is drastically decreased) without significantly
>>> increasing the power consumption.
>>
>> [...]
>>
>>> static struct irq_domain *stm32_pctrl_get_irq_domain(struct
>>> device_node *np)
>>> @@ -1575,6 +1537,10 @@ int stm32_pctl_probe(struct platform_device
>>> *pdev)
>>> ret = stm32_gpiolib_register_bank(pctl, child);
>>> if (ret) {
>>> of_node_put(child);
>>> +
>>> + for (i = 0; i < pctl->nbanks; i++)
>>> + clk_disable_unprepare(pctl->banks[i].clk);
>>> +
>>
>> There are clk_bulk_*() functions, maybe you can use those to get rid
>> of these loops ?
>
> This sounds goods, but checking more in details I see that moving to the
> 'bulk' implementation would require to move the clk information from the
> "struct stm32_gpio_bank *banks" member to its parent "struct
> stm32_pinctrl".
>
> This would make the clk device information stored in a different
> structure from the other device-related information (base address, reset
> control, ...). It's better to keep all those information together in the
> same struct.
>
> As another drawback we would loose access to 'clk' from any function
> that have 'bank' (or 'struct gpio_chip *chip') as input parameter (e.g.
> stm32_gpio_get() called from gpiolib).
>
> So I really prefer to keep the current implementation.
All right, I agree.
>> The rest looks good to me.
Reviewed-by: Marek Vasut <[email protected]>