Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 23 Jan 2020 08:09:24 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     Cristian Marussi <cristian.marussi@arm.com>
Cc:     arnd@arndb.de, Sudeep Holla <sudeep.holla@arm.com>,
        jassisinghbrar@gmail.com, peng.fan@nxp.com,
        peter.hilber@opensynergy.com, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH V3] firmware: arm_scmi: Make scmi core independent of the
 transport type
Message-ID: <20200123023924.roqc2iyx4wmukk4p@vireshk-i7>
References: <4b74f1b6c1f9653241a1b5754525e230b3d76a3f.1579595093.git.viresh.kumar@linaro.org>
 <3a8836dd-99d3-faff-af05-2032d609f594@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3a8836dd-99d3-faff-af05-2032d609f594@arm.com>
User-Agent: NeoMutt/20180716-391-311a52
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

> >  /* 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>".

-- 
viresh