Re: [PATCH 5/5] reset: Add support for resets provided by SCMI

From: Sudeep Holla <sudeep.holla@arm.com>
To: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>,
	Etienne Carriere <etienne.carriere@linaro.org>,
	Souvik Chakravarty <Souvik.Chakravarty@arm.com>,
	wesleys@xilinx.com, aidapala@qti.qualcomm.com,
	linux-kernel@vger.kernel.org,
	Saeed Nowshadi <saeed.nowshadi@xilinx.com>,
	Bo Zhang <bozhang.zhang@broadcom.com>,
	Felix Burton <fburton@xilinx.com>,
	Jim Quinlan <james.quinlan@broadcom.com>,
	pajay@qti.qualcomm.com, Gaku Inami <gaku.inami.xh@renesas.com>,
	Volodymyr Babchuk <volodymyr_babchuk@epam.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 5/5] reset: Add support for resets provided by SCMI
Date: Mon, 29 Jul 2019 11:34:24 +0100	[thread overview]
Message-ID: <20190729103424.GB831@e107155-lin> (raw)
In-Reply-To: <1564394355.7633.5.camel@pengutronix.de>

On Mon, Jul 29, 2019 at 11:59:15AM +0200, Philipp Zabel wrote:
> Hi Sudeep,
>
> On Fri, 2019-07-26 at 14:59 +0100, Sudeep Holla wrote:
> > On some ARM based systems, a separate Cortex-M based System Control
> > Processor(SCP) provides the overall power, clock, reset and system
> > control. System Control and Management Interface(SCMI) Message Protocol
> > is defined for the communication between the Application Cores(AP)
> > and the SCP.
> >
> > Adds support for the resets provided using SCMI protocol for performing
> > reset management of various devices present on the SoC. Various reset
> > functionalities are achieved by the means of different ARM SCMI device
> > operations provided by the ARM SCMI framework.
> >
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> thank you for the patch. I have a few suggestions below.
>

Thanks for reviewing so quickly, all points taken and fixed locally.
Will wait for some more time before posting v2.

[...]

> > +static int
> > +scmi_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > +	struct scmi_reset_data *data = to_scmi_reset_data(rcdev);
> > +	const struct scmi_handle *handle = data->handle;
>
> This could be shortened to to_scmi_handle(rcdev), since none of the
> other fields in scmi_reset_data are used by the reset_control_ops
> callbacks.
>

Makes sense, missed to see that.

[...]

> > +
> > +static int scmi_reset_probe(struct scmi_device *sdev)
> > +{
> > +	struct scmi_reset_data *data;
> > +	struct device *dev = &sdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	const struct scmi_handle *handle = sdev->handle;
> > +
> > +	if (!handle || !handle->reset_ops)
> > +		return -ENODEV;
> > +
> > +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	data->rcdev.ops = &scmi_reset_ops;
> > +	data->rcdev.owner = THIS_MODULE;
> > +	data->rcdev.of_node = np;
>
> This is missing rcdev.nr_resets. When nr_resets is kept at zero, the
> check in of_reset_simple_xlate will fail for any id.
>

I clearly missed to do git add for the above :(. I did find these
after testing.

> > +	data->dev = dev;
> > +
> > +	return devm_reset_controller_register(dev, &data->rcdev);
> > +}
> > +
> > +static const struct scmi_device_id scmi_id_table[] = {
> > +	{ SCMI_PROTOCOL_RESET },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> > +
> > +static struct scmi_driver scmi_reset_driver = {
> > +	.name = "scmi-reset",
> > +	.probe = scmi_reset_probe,
> > +	.id_table = scmi_id_table,
> > +};
> > +module_scmi_driver(scmi_reset_driver);
> > +
> > +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
> > +MODULE_DESCRIPTION("ARM SCMI clock driver");
>
> s/clock/reset controller/
>

Stupid copy-paste :(

Thanks again.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel