RE: [PATCH v2 3/8] firmware: arm_scmi: Add support for standalone transport drivers

[Peng Fan]

> Subject: [PATCH v2 3/8] firmware: arm_scmi: Add support for
> standalone transport drivers
> 
> Extend the core SCMI stack with structures and methods to allow for
> transports to be split out as standalone drivers, while still supporting
> old style transports, defined as built into the SCMI core stack.
> 
> No functional change.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> ---
> NOTE: old style transport support will be removed later in this series.
> 
> v1 --> v2
> - fixed comit message
> ---
>  drivers/firmware/arm_scmi/common.h | 84
> ++++++++++++++++++++++++++++++
> drivers/firmware/arm_scmi/driver.c | 44 +++++++++++++++-
>  drivers/firmware/arm_scmi/msg.c    |  5 ++
>  drivers/firmware/arm_scmi/shmem.c  |  5 ++
>  4 files changed, 136 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/common.h
> b/drivers/firmware/arm_scmi/common.h
> index 8e5751aaa600..4af06810eb39 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -349,6 +349,8 @@ struct scmi_shared_mem_operations {
>  				     bool tx, struct resource *res);  };
> 
> +const struct scmi_shared_mem_operations
> +*scmi_shared_mem_operations_get(void);
> +
>  /* declarations for message passing transports */  struct
> scmi_msg_payld;
> 
> @@ -376,6 +378,88 @@ struct scmi_message_operations {
>  				   size_t max_len, struct scmi_xfer
> *xfer);  };
> 
> +const struct scmi_message_operations
> +*scmi_message_operations_get(void);
> +
> +/**
> + * struct scmi_transport_core_operations  - Transpoert core
> operations
> + *
> + * @bad_message_trace: An helper to report a
> malformed/unexpected
> +message
> + * @rx_callback: Callback to report received messages
> + * @shmem: Datagram operations for shared memory based
> transports
> + * @msg: Datagram operations for message based transports  */
> struct
> +scmi_transport_core_operations {
> +	void (*bad_message_trace)(struct scmi_chan_info *cinfo,
> +				  u32 msg_hdr, enum scmi_bad_msg
> err);
> +	void (*rx_callback)(struct scmi_chan_info *cinfo, u32 msg_hdr,
> +			    void *priv);
> +	const struct scmi_shared_mem_operations *shmem;
> +	const struct scmi_message_operations *msg; };
> +
> +/**
> + * struct scmi_transport  - A structure representing a configured
> +transport
> + *
> + * @supplier: Device representimng the transport and acting as a
> supplier for
> + *	      the core SCMI stack
> + * @desc: Transport descriptor
> + * @core_ops: A pointer to a pointer used by the core SCMI stack to
> make the
> + *	      core transport operations accessible to the transports.
> + */
> +struct scmi_transport {
> +	struct device *supplier;
> +	const struct scmi_desc *desc;
> +	struct scmi_transport_core_operations **core_ops; };
> +
> +#define DEFINE_SCMI_TRANSPORT_DRIVER(__trans, __match_table,
> __core_ptr)\
> +static int __trans##_probe(struct platform_device *pdev)		\
> +{
> 	\
> +	struct scmi_transport *scmi_trans;
> 	\
> +	struct platform_device *scmi_pdev;
> 	\
> +	struct device *dev = &pdev->dev;				\
> +
> 	\
> +	scmi_trans = devm_kzalloc(dev, sizeof(*scmi_trans),
> GFP_KERNEL);\
> +	if (!scmi_trans)						\
> +		return -ENOMEM;
> 		\
> +
> 	\
> +	scmi_pdev = devm_kzalloc(dev, sizeof(*scmi_pdev),
> GFP_KERNEL);	\
> +	if (!scmi_pdev)
> 	\
> +		return -ENOMEM;
> 		\
> +
> 	\
> +	scmi_trans->supplier = dev;
> 	\
> +	scmi_trans->desc = &__trans##_desc;
> 	\
> +	scmi_trans->core_ops = __core_ptr;
> 	\
> +
> 	\
> +	scmi_pdev->name = "arm-scmi";
> 	\
> +	scmi_pdev->id = PLATFORM_DEVID_AUTO;
> 		\
> +	scmi_pdev->dev.platform_data = scmi_trans;
> 	\
> +
> 	\
> +	device_set_of_node_from_dev(&scmi_pdev->dev, dev);
> 	\
> +
> 	\
> +	dev_set_drvdata(dev, scmi_pdev);
> 	\
> +
> 	\
> +	return platform_device_register(scmi_pdev);
> 	\
> +}
> 	\
> +
> 	\
> +static void __trans##_remove(struct platform_device *pdev)
> 	\
> +{
> 	\
> +	struct platform_device *scmi_pdev;
> 	\
> +
> 	\
> +	scmi_pdev = dev_get_drvdata(&pdev->dev);
> 	\
> +
> 	\
> +	platform_device_unregister(scmi_pdev);
> 	\
> +}
> 	\
> +
> 	\
> +static struct platform_driver __trans##_driver = {			\
> +	.driver = {
> 	\
> +		   .name = #__trans "_transport",			\
> +		   .of_match_table = __match_table,
> 	\
> +		   },
> 	\
> +	.probe = __trans##_probe,
> 	\
> +	.remove_new = __trans##_remove,
> 		\
> +}
> +
>  extern const struct scmi_shared_mem_operations scmi_shmem_ops;
> extern const struct scmi_message_operations scmi_msg_ops;
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c
> b/drivers/firmware/arm_scmi/driver.c
> index 6b6957f4743f..a1892d4d8c69 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -194,6 +194,11 @@ struct scmi_info {
>  #define bus_nb_to_scmi_info(nb)	container_of(nb, struct
> scmi_info, bus_nb)
>  #define req_nb_to_scmi_info(nb)	container_of(nb, struct
> scmi_info, dev_req_nb)
> 
> +static struct scmi_transport_core_operations scmi_trans_core_ops = {
> +	.bad_message_trace = scmi_bad_message_trace,
> +	.rx_callback = scmi_rx_callback,
> +};
> +
>  static unsigned long
>  scmi_vendor_protocol_signature(unsigned int protocol_id, char
> *vendor_id,
>  			       char *sub_vendor_id, u32 impl_ver) @@
> -2950,6 +2955,28 @@ static int scmi_debugfs_raw_mode_setup(struct
> scmi_info *info)
>  	return ret;
>  }
> 
> +static const struct scmi_desc *scmi_transport_lookup(struct device
> +*dev) {
> +	struct scmi_transport *trans;
> +
> +	trans = dev_get_platdata(dev);
> +	if (!trans || !trans->desc || !trans->supplier || !trans-
> >core_ops)
> +		return NULL;
> +
> +	if (!device_link_add(dev, trans->supplier,
> DL_FLAG_AUTOREMOVE_CONSUMER)) {

Just wonder why this is needed?

> +		dev_err(dev,
> +			"Adding link to supplier transport device
> failed\n");
> +		return NULL;
> +	}
> +
> +	/* Provide core transport ops */
> +	*trans->core_ops = &scmi_trans_core_ops;
> +
> +	dev_info(dev, "Using %s\n", dev_driver_string(trans->supplier));
> +
> +	return trans->desc;
> +}
> +
>  static int scmi_probe(struct platform_device *pdev)  {
>  	int ret;
> @@ -2962,8 +2989,14 @@ static int scmi_probe(struct
> platform_device *pdev)
>  	struct device_node *child, *np = dev->of_node;
> 
>  	desc = of_device_get_match_data(dev);
> -	if (!desc)
> -		return -EINVAL;
> +	if (!desc) {
> +		desc = scmi_transport_lookup(dev);

from the code, It is not actually a lookup operation.
How about scmi_transport_setup?

Regards,
Peng.