Re: [PATCH 03/11] firmware: arm_scmi: Simplify clock rates exposed interface

[Cristian Marussi]

On Mon, Mar 02, 2026 at 02:09:14PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> On Mon, 2 Mar 2026 at 13:48, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > On Fri, 27 Feb 2026 at 16:33, Cristian Marussi <cristian.marussi@xxxxxxx> wrote:
> > > Move needlessly exposed fields away from scmi_clock_info into the new
> > > internal struct scmi_clock_desc while keeping exposed only the two new
> > > min_rate and max_rate fields for each clock.
> > >
> > > No functional change.
> > >
> > > Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> >
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -157,13 +157,27 @@ struct scmi_clock_rate_notify_payld {
> > >         __le32 rate_high;
> > >  };
> > >
> > > +struct scmi_clock_desc {
> > > +       u32 id;
> > > +       bool rate_discrete;
> > > +       unsigned int num_rates;
> > > +       u64 rates[SCMI_MAX_NUM_RATES];
> > > +#define        RATE_MIN        0
> > > +#define        RATE_MAX        1
> > > +#define        RATE_STEP       2
> >
> > Any specific reason you are not using a union here, like in
> > scmi_clock_info?
> 
> Ah, "[PATCH 06/11] firmware: arm_scmi: Make clock rates allocation
> dynamic" answers that.
> 
> >
> > > +       struct scmi_clock_info info;
> > > +};
> >
> > > --- a/include/linux/scmi_protocol.h
> > > +++ b/include/linux/scmi_protocol.h
> > > @@ -51,6 +51,8 @@ struct scmi_clock_info {
> > >         bool rate_ctrl_forbidden;
> > >         bool parent_ctrl_forbidden;
> > >         bool extended_config;
> > > +       u64 min_rate;
> > > +       u64 max_rate;
> > >         union {
> > >                 struct {
> > >                         int num_rates;
> >
> > You patch description read like the actual rates would be moved
> > from scmi_clock_info to scmi_clock_desc, i.e. _removed_ here?
> 
> And these members are actually removed in "[PATCH 05/11] firmware:
> arm_scmi: Drop unused clock rate interfaces".  Please reflect that in
> this patch description.
> 

Ok I will better explain.

Thanks,
Cristian