Re: [PATCH 1/6] firmware: arm_scmi: Simplify enable/disable Clock operations

[Cristian Marussi]

On Tue, Aug 22, 2023 at 01:17:15PM -0700, Stephen Boyd wrote:
> Quoting Cristian Marussi (2023-08-11 09:14:41)
> > Add a param to Clock enable/disable operation to ask for atomic operation
> > and remove _atomic version of such operations.
> 

Hi,

> Why?
> 

:D, given that the 2 flavours of SCMI enable/disable ops (and the upcoming
state_get) just differ in their operating mode (atomic or not) and the
Clock framework in turn wrap such calls into 4 related and explicitly
named clk_ops (scmi_clock_enable/scmi_clock_atomic_enable etc) that hint
at what is being done, seemed to me reasonable to reduce the churn and
remove a bit of code wrappers in favour of a param.

> > 
> > No functional change.
> > 
> > CC: Michael Turquette <mturquette@xxxxxxxxxxxx>
> > CC: Stephen Boyd <sboyd@xxxxxxxxxx>
> > CC: linux-clk@xxxxxxxxxxxxxxx
> > Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> > ---
> >  drivers/clk/clk-scmi.c            |  8 ++++----
> >  drivers/firmware/arm_scmi/clock.c | 24 ++++++------------------
> >  include/linux/scmi_protocol.h     |  9 ++++-----
> >  3 files changed, 14 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> > index 2c7a830ce308..ff003083e592 100644
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -78,28 +78,28 @@ static int scmi_clk_enable(struct clk_hw *hw)
> >  {
> >         struct scmi_clk *clk = to_scmi_clk(hw);
> >  
> > -       return scmi_proto_clk_ops->enable(clk->ph, clk->id);
> > +       return scmi_proto_clk_ops->enable(clk->ph, clk->id, false);
> >  }
> >  
> >  static void scmi_clk_disable(struct clk_hw *hw)
> >  {
> >         struct scmi_clk *clk = to_scmi_clk(hw);
> >  
> > -       scmi_proto_clk_ops->disable(clk->ph, clk->id);
> > +       scmi_proto_clk_ops->disable(clk->ph, clk->id, false);
> 
> I enjoyed how it was before because I don't know what 'false' means
> without looking at the ops now.
> 

Yes indeed, I can drop this and rework if you prefer to maintain the old
API calls, but this would mean that whenever we'll add new atomic
flavour to some new SCMI clk operations we'll have to add 2 ops instead
of a parametrized one...this is what would happen also in this series
with state_get (and what really triggered this refactor)

(and please consider that on the SCMI side, for testing purposes, I would
prefer to expose always both atomic and non-atomic flavours even if NOT
both actively used by the Clock framework...like state_get() that can only
be atomic for Clock frmwk...)

Thanks,
Cristian