Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver

[Cristian Marussi]

On Wed, Apr 12, 2023 at 11:04:05PM +0100, Cristian Marussi wrote:
> On Fri, Apr 07, 2023 at 10:18:27AM +0000, Oleksii Moisieiev wrote:
> > Implementation of the SCMI client driver, which implements
> > PINCTRL_PROTOCOL. This protocol has ID 19 and is described
> > in the latest DEN0056 document.
> 
> Hi,
> 

Hi Oleksii,

one more thing that I missed in my previous review down below.

> > This protocol is part of the feature that was designed to
> > separate the pinctrl subsystem from the SCP firmware.
> > The idea is to separate communication of the pin control
> > subsystem with the hardware to SCP firmware
> > (or a similar system, such as ATF), which provides an interface
> > to give the OS ability to control the hardware through SCMI protocol.
> > This is a generic driver that implements SCMI protocol,
> > independent of the platform type.
> > 
> > DEN0056 document:
> > https://developer.arm.com/documentation/den0056/latest
> > 
> 

[snip]

> > +static int scmi_pinctrl_request_config(const struct scmi_handle *handle,
> > +				       u32 selector,
> > +				       enum scmi_pinctrl_selector_type type,
> > +				       u32 *config)
> > +{
> > +	struct scmi_xfer *t;
> > +	struct scmi_conf_tx *tx;
> > +	__le32 *packed_config;
> > +	u32 attributes = 0;
> > +	int ret;
> > +
> > +	if (!handle || !config || type == FUNCTION_TYPE)
> > +		return -EINVAL;
> > +
> > +	ret = scmi_pinctrl_validate_id(handle, selector, type);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = scmi_xfer_get_init(handle, PINCTRL_CONFIG_GET,
> > +				 SCMI_PROTOCOL_PINCTRL,
> > +				 sizeof(*tx), sizeof(*packed_config), &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	tx = t->tx.buf;
> > +	packed_config = t->rx.buf;
> > +	tx->identifier = cpu_to_le32(selector);
> > +	attributes = SET_TYPE_BITS(attributes, type);
> > +	attributes = SET_CONFIG(attributes, *config);
> > +

Looking at scmi_conf_tx and these pinctrl get/set functions, you do not
seem to consider the ConfigType field in the SCMI related messages, so
basically using always the Default 0 Type, and as a consequence you dont
either expose any way to choose a Different type in the related SCMI
protocol ops; I imagine this is because the pinctrl driver currently using
this protocol, at the end, does not need any of the other available types
(as in Table 23 of the spec).

This is fine for pinctrl driver as it is now, BUT the SCMI pinctrl
protocol implementation in the core SCMI stack and its related
protocol_operations as exposed in scmi_protocol.h should be generic
enough to serve any possible user of the SCMI pinctrl protocol (and there
is already a request to extend/amend the spec somehow to send multiple pin
setup of different types in one go as you may have seen), so I'd say it's
better if you add also a ConfigType param to the get/set_config scmi_pinctrl_ops
and expose the whole ConfigType enums (Table23) in scmi_protocol.h (like we do for
sensor classes on scmi_protocol.h) to address this; the pinctrl driver
can then anyway call such new protocol_ops with a Default type, but at
least the SCMI protocol_ops interface will remain generic and could be
reused iin other scenarios.

This is equally true for all the other protocol messages (should I have
miss something else for now...I'll review again you next V2 anyway).

Thanks,
Cristian