Hi Laurent Pinchart,
Thanks for the feedback.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Monday, March 18, 2024 10:39 PM
> Subject: Re: [PATCH v3 1/3] clk: Update API documentation related to clock disable
>
> Hi Biju,
>
> Thank you for the patch.
>
> On Mon, Mar 18, 2024 at 11:08:40AM +0000, Biju Das wrote:
> >
> > The API's related to clk disable operation does not explicitly states
> > the synchoronous or asynchrous behaviour as it is driver
>
> s/synchoronous/synchronous/
OK will fix this.
>
> > dependent. So make this part clear in API documentation.
>
> You need to explain the rationale here, why asynchronous behaviour is preferred.
OK. Because it's not important for functionality and moreover some systems
It can enter sleep state quickly and thereby saving power.
I will update it in commit message.
>
> > Suggested-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v2->v3:
> > * No change.
> > v2:
> > * New patch.
> > ---
> > drivers/clk/clk.c | 3 ++-
> > include/linux/clk-provider.h | 3 ++-
> > include/linux/clk.h | 3 ++-
> > 3 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
> > 25371c91a58f..f5fa91a339d7 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core)
> > * if the operation may sleep. One example is a clk which is accessed over
> > * I2c. In the complex case a clk gate operation may require a fast and a slow
> > * part. It is this reason that clk_unprepare and clk_disable are
> > not mutually
> > - * exclusive. In fact clk_disable must be called before clk_unprepare.
> > + * exclusive. In fact clk_disable must be called before
> > + clk_unprepare. The
> > + * synchronous or asynchronous clock gating operation is driver dependent.
>
> If synchronous operation is not guaranteed, then it's asynchonous.
> Asynchronous doesn't mean slow, even an asynchronous provider can complete the disable operation
> before the function returns to the caller. All it means is that there's no guarantee of synchronous
> operation. I would document it as such:
>
> * This function is asynchronous, if may return before the clock provider
> * completes the unprepare operation.
>
> However, below you're addressing the disable operation. Did you mean to patch the documentation for
> clk_prepare() instead ? Making
> clk_unprepare() asynchronous seems a bit weird, given that the function may sleep and is expected
> to take more time.
This patch aimed at the unprepare/disable functionality.
>
> > */
> > void clk_unprepare(struct clk *clk)
> > {
> > diff --git a/include/linux/clk-provider.h
> > b/include/linux/clk-provider.h index 4a537260f655..5b493024e1ec 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -113,7 +113,8 @@ struct clk_duty {
> > * sleep.
> > *
> > * @disable: Disable the clock atomically. Called with enable_lock held.
> > - * This function must not sleep.
> > + * This function must not sleep. The synchronous or asynchronous
> > + * disabling of the clock is driver dependent.
>
> As this is the documentation that targets clock providers, I would expand it and explain why a
> provider may want to make the disable operation not synchronous.
Will explain here as well.
>
> > *
> > * @is_enabled: Queries the hardware to determine if the clock is enabled.
> > * This function must not sleep. Optional, if this op is not
>
> .is_enabled() should become mandatory if .disable() is not synchronous.
> The relationship between the two operations should be better explained.
I will update the comment here.
>
> > diff --git a/include/linux/clk.h b/include/linux/clk.h index
> > 00623f4de5e1..84b02518791f 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -681,7 +681,8 @@ int __must_check clk_bulk_enable(int num_clks,
> > * @clk: clock source
> > *
> > * Inform the system that a clock source is no longer required by
> > - * a driver and may be shut down.
> > + * a driver and may be shut down. It is not guaranteed to ever
> > + actually
> > + * be stopped, that will be driver dependent.
>
> This is the documentation of clk_bulk_disable(), you should address
> clk_disable() too.
This update is for clk_disable(). Am I missing anything?
> I've just noticed that both functions are documented in two places, in
> include/linux/clk.h, and in drivers/clk/. I wonder why that is. It sounds like it should be fixed,
> or you'll have to patch both documentation blocks.
Sakari also pointed out the same, he suggested to remove it from .c.
Cheers,
Biju
>
> There's another issue that I'll raise in the review of 2/3.
>
> > *
> > * May be called from atomic contexts.
> > *
>
> --
> Regards,
>
> Laurent Pinchart
