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;
Subject: Re: [PATCH V3] firmware: arm_scmi: Make scmi core independent of the
 transport type
To:     Viresh Kumar <viresh.kumar@linaro.org>
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
References: <4b74f1b6c1f9653241a1b5754525e230b3d76a3f.1579595093.git.viresh.kumar@linaro.org>
 <3a8836dd-99d3-faff-af05-2032d609f594@arm.com>
 <20200123023924.roqc2iyx4wmukk4p@vireshk-i7>
From:   Cristian Marussi <cristian.marussi@arm.com>
Message-ID: <d5d71818-e68f-7688-4378-64d96bea922d@arm.com>
Date:   Thu, 23 Jan 2020 11:06:38 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <20200123023924.roqc2iyx4wmukk4p@vireshk-i7>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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