Re: [PATCH v3 04/13] firmware: scmi: framework for installing additional protocols

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Fri, Sep 08, 2023 at 02:01:52PM +0000, Etienne CARRIERE wrote:
> 
> > From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Sent: Friday, September 8, 2023 04:51
> >  
> > This framework allows SCMI protocols to be installed and bound to the agent
> > so that the agent can manage and utilize them later.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > ---
> > v3
> > * move "per_device_plat_auto" from a earlier patch
> > * fix comments in "scmi_agent_priv"
> > * modify an order of include files in scmi_agent.h
> > v2
> > * check for availability of protocols
> > ---
> >  drivers/firmware/scmi/scmi_agent-uclass.c | 101 +++++++++++++++++++++-
> >  include/scmi_agent-uclass.h               |  15 +++-
> >  include/scmi_agent.h                      |  14 +++
> >  3 files changed, 126 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c b/drivers/firmware/scmi/scmi_agent-uclass.c
> > index 94e54eeb066b..e823d105a3eb 100644
> > --- a/drivers/firmware/scmi/scmi_agent-uclass.c
> > +++ b/drivers/firmware/scmi/scmi_agent-uclass.c
> > @@ -38,6 +38,86 @@ static const struct error_code scmi_linux_errmap[] = {
> >          { .scmi = SCMI_PROTOCOL_ERROR, .errno = -EPROTO, },
> >  };
> >  
> > +/*
> > + * NOTE: The only one instance should exist according to
> > + * the current specification and device tree bindings.
> > + */
> > +struct udevice *scmi_agent;
> > +
> > +struct udevice *scmi_get_protocol(struct udevice *dev,
> > +                                 enum scmi_std_protocol id)
> > +{
> > +       struct scmi_agent_priv *priv;
> > +       struct udevice *proto;
> > +
> > +       priv = dev_get_uclass_plat(dev);
> > +       if (!priv) {
> > +               dev_err(dev, "No priv data found\n");
> > +               return NULL;
> > +       }
> > +
> > +       switch (id) {
> > +       case SCMI_PROTOCOL_ID_CLOCK:
> > +               proto = priv->clock_dev;
> > +               break;
> > +       case SCMI_PROTOCOL_ID_RESET_DOMAIN:
> > +               proto = priv->resetdom_dev;
> > +               break;
> > +       case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
> > +               proto = priv->voltagedom_dev;
> > +               break;
> > +       default:
> > +               dev_err(dev, "Protocol not supported\n");
> > +               proto = NULL;
> > +               break;
> > +       }
> > +       if (proto && device_probe(proto))
> > +               dev_err(dev, "Probe failed\n");
> > +
> > +       return proto;
> > +}
> > +
> > +/**
> > + * scmi_add_protocol - add protocol to agent
> > + * @dev:       SCMI agent device
> > + * @proto_id:  SCMI protocol ID
> > + * @proto:     SCMI protocol device
> > + *
> > + * Associate the protocol instance, @proto, to the agent, @dev,
> > + * for later use.
> > + *
> > + * Return:     0 on success, error code otherwise
> > + */
> > +static int scmi_add_protocol(struct udevice *dev,
> > +                            enum scmi_std_protocol proto_id,
> > +                            struct udevice *proto)
> > +{
> > +       struct scmi_agent_priv *priv;
> > +
> > +       priv = dev_get_uclass_plat(dev);
> > +       if (!priv) {
> > +               dev_err(dev, "No priv data found\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       switch (proto_id) {
> > +       case SCMI_PROTOCOL_ID_CLOCK:
> > +               priv->clock_dev = proto;
> > +               break;
> > +       case SCMI_PROTOCOL_ID_RESET_DOMAIN:
> > +               priv->resetdom_dev = proto;
> > +               break;
> > +       case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
> > +               priv->voltagedom_dev = proto;
> > +               break;
> > +       default:
> > +               dev_err(dev, "Protocol not supported\n");
> > +               return -EPROTO;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  int scmi_to_linux_errno(s32 scmi_code)
> >  {
> >          int n;
> > @@ -164,12 +244,14 @@ int devm_scmi_process_msg(struct udevice *dev, struct scmi_msg *msg)
> >   */
> >  static int scmi_bind_protocols(struct udevice *dev)
> >  {
> > +       struct udevice *agent;
> 
> nit: my build reported 'agent' here is not used in the scope of this patch.
> Strictly speaking, it should be brought by patch v3 05/13.
> ("firmware: scmi: install base protocol to SCMI agent")

You're right.
I will move this variable declaration to patch#5.

-Takahiro Akashi

> BR,
> etienne
> 
> > (snip)