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.za...@pengutronix.de>
> > Signed-off-by: Sudeep Holla <sudeep.ho...@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.ho...@arm.com>");
> > +MODULE_DESCRIPTION("ARM SCMI clock driver");
>
> s/clock/reset controller/
>
Stupid copy-paste :(
Thanks again.
--
Regards,
Sudeep
