[PATCH v1 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 12/15/23 15:00, Svyatoslav Ryhel wrote:
> пт, 15 груд. 2023 р. о 20:45 Sean Anderson <seanga2 at gmail.com> пише:
>>
>> On 12/15/23 13:26, Svyatoslav Ryhel wrote:
>>> пт, 15 груд. 2023 р. о 19:25 Sean Anderson <seanga2 at gmail.com> пише:
>>>>
>>>> On 11/17/23 02:52, Svyatoslav Ryhel wrote:
>>>>> Existing gpio-gate-clock driver acts like a simple GPIO switch without any
>>>>> effect on gated clock. Add actual clock actions into enable/disable ops and
>>>>> implement get_rate op by passing gated clock if it is enabled.
>>>>>
>>>>> Signed-off-by: Svyatoslav Ryhel <clamor95 at gmail.com>
>>>>> ---
>>>>>     drivers/clk/clk-gpio.c | 47 +++++++++++++++++++++++++++++++++++-------
>>>>>     1 file changed, 40 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
>>>>> index 26d795b978..a55470cfbf 100644
>>>>> --- a/drivers/clk/clk-gpio.c
>>>>> +++ b/drivers/clk/clk-gpio.c
>>>>> @@ -3,19 +3,23 @@
>>>>>      * Copyright (C) 2023 Marek Vasut <marek.vasut+renesas at mailbox.org>
>>>>>      */
>>>>>
>>>>> -#include <asm/gpio.h>
>>>>> -#include <common.h>
>>>>> -#include <clk-uclass.h>
>>>>> +#include <clk.h>
>>>>>     #include <dm.h>
>>>>> +#include <clk-uclass.h>
>>>>> +#include <linux/clk-provider.h>
>>>>> +
>>>>> +#include <asm/gpio.h>
>>>>>
>>>>>     struct clk_gpio_priv {
>>>>>         struct gpio_desc        enable;
>>>>> +     struct clk              *clk;
>>>>
>>>> Please name this "parent".
>>>>
>>> I see no need in this since clk is generic name and this driver can
>>> accept only one clock with any name.
>>
>> It's good documentation. It makes it clear what the purpose of the member is.
>>
> This driver uses a single (1) clock with a single purpose.
> 
> Why do you think you need a clock in a clock-gpio driver? Hm, I don't
> know. This will remain a mystery for future archeo-coders yeah.

If you don't want to change the name, then add a doc comment.

>>>>>     };
>>>>>
>>>>>     static int clk_gpio_enable(struct clk *clk)
>>>>>     {
>>>>>         struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
>>>>>
>>>>> +     clk_prepare_enable(priv->clk);
>>>>
>>>> Just clk_enable is fine. We don't have a separate prepare stage line Linux.
>>>>
>>> U-Boot supports clk_prepare_enable and since this support is present I
>>> see no contradiction in using it.
>>
>> These functions are only there for to make it easier to port code from Linux.
>> Since this code was written for U-Boot, just use the actual function.
>>
> Then document that its use is discouraged. Until then it is not restricted.

There are many functions like this in U-Boot (e.g. kmalloc which takes an unused
flags argument, or all the pr_* functions which are almost-but-not-quite the
same as log_*). Not all of them are explicitly document as such, but generally
you should use the native functions where it is not inconvenient.

--Sean