On 1/10/24 10:53, Svyatoslav wrote:
>
>
> 10 січня 2024 р. 17:45:57 GMT+02:00, Sean Anderson <seanga2 at gmail.com> написав(-ла):
>> On 12/16/23 10:37, Sean Anderson wrote:
>>> On 12/16/23 03:48, 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 | 44 ++++++++++++++++++++++++++++++++++--------
>>>> 1 file changed, 36 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
>>>> index 26d795b978..72d9747a47 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 gpio_desc enable; /* GPIO, controlling the gate */
>>>> + struct clk *clk; /* Gated clock */
>>>> };
>>>> static int clk_gpio_enable(struct clk *clk)
>>>> {
>>>> struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
>>>> + clk_enable(priv->clk);
>>>> dm_gpio_set_value(&priv->enable, 1);
>>>> return 0;
>>>> @@ -26,21 +30,45 @@ static int clk_gpio_disable(struct clk *clk)
>>>> struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
>>>> dm_gpio_set_value(&priv->enable, 0);
>>>> + clk_disable(priv->clk);
>>>> return 0;
>>>> }
>>>> +static ulong clk_gpio_get_rate(struct clk *clk)
>>>> +{
>>>> + struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
>>>> +
>>>> + return clk_get_rate(priv->clk);
>>>> +}
>>>> +
>>>> const struct clk_ops clk_gpio_ops = {
>>>> .enable = clk_gpio_enable,
>>>> .disable = clk_gpio_disable,
>>>> + .get_rate = clk_gpio_get_rate,
>>>> };
>>>> -static int clk_gpio_probe(struct udevice *dev)
>>>> +static int clk_gpio_of_to_plat(struct udevice *dev)
>>
>> Same comment as the first time:
>>
>> Why the conversion from probe to of_to_plat?
>>
>
> Same answer as the first time.
Last time you said
>>>
>>> -static int clk_gpio_probe(struct udevice *dev)
>>> +static int clk_gpio_of_to_plat(struct udevice *dev)
>>
>> This change should be a separate commit.
>>
> This actually should not be accepted first place
>
> .of_to_plat - convert device tree data to plat
> .probe - make a device ready for use
>
> https://github.com/u-boot/u-boot/blob/master/doc/develop/driver-model/design.rst
>
and I thought "This actually should not be accepted first place" was referring to
the conversion (i.e. that you would be removing it next time).
This sort of conversion should be mentioned in the commit message.
And the weird thing to me is that I would expect of_to_plat to fill in a platdata
struct. If you are not doing that, why use this function?
> You propose to spam commits for this small adjustment?
Yes. One change per commit.
--Sean