Messages in this thread | | | Subject | Re: [PATCH V3] firmware: arm_scmi: Make scmi core independent of the transport type | From | Cristian Marussi <> | Date | Thu, 23 Jan 2020 11:06:38 +0000 |
| |
On 23/01/2020 02:39, Viresh Kumar wrote: > On 22-01-20, 12:44, Cristian Marussi wrote: >> On 21/01/2020 08:27, Viresh Kumar wrote: >> commment is obsolete > > Right, they need to be checked everywhere again. Sorry for missing > that earlier. > >>> +struct scmi_chan_info { >>> + struct scmi_info *info; >>> + struct device *dev; >>> + struct scmi_handle *handle; >>> + void *transport_info; >>> +}; >>> + >>> +/** >>> + * struct scmi_transport_ops - Structure representing a SCMI transport ops >>> + * >>> + * @send_message: Callback to send a message >>> + * @mark_txdone: Callback to mark tx as done >>> + * @chan_setup: Callback to allocate and setup a channel >>> + * @chan_free: Callback to free a channel >>> + */ >> commment is obsolete but I would also ask: are all of these operations supposed to be mandatory supported >> on any possible foreseeable transport ? (beside the obviously needed like send_message) >> >> I'm asking because they are all called straight away from the driver core without any NULL check >> so that if a new transport should not need one of them it will be forced to anyway implement a dummy one >> to comply, which it will be needlessly invoked every time. > > They are kept as mandatory for now as we don't really know how it > will look for other transport types. Lets make them optional only when > someone don't need to define them. It would be a simple change anyway.
Ok, fine. > >>> /* Each compatible listed below must have descriptor associated with it */ >>> static const struct of_device_id scmi_of_match[] = { >>> - { .compatible = "arm,scmi", .data = &scmi_generic_desc }, >>> + { .compatible = "arm,scmi", .data = &scmi_mailbox_desc }, >>> { /* Sentinel */ }, >>> }; >> >> minor thing: shouldn't the chosen transport being configurable at compile time with some >> option like CONFIG_SCMI_TRANSPORT_MBOX ? or via DT ? > > It is configurable via DT. The compatible should look different in > that case, something like: "arm,scmi-<newtranport>". >
Ah ok, we're assuming mailbox transport as the default, being the only one existing as of now. Fine for me, thanks for the explanation.
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Regards
Cristian
| |