RE: [RFC PATCH v3 3/3] reset: reset-zynqmp: Adding support for Xilinx zynqmp reset controller.

[Nava kishore Manne]

Hi Philipp

Thanks for the quick response...
Please find my response inline.

> -----Original Message-----
> From: Philipp Zabel [mailto:p.zabel@xxxxxxxxxxxxxx]
> Sent: Wednesday, September 5, 2018 4:00 PM
> To: Nava kishore Manne <navam@xxxxxxxxxx>; robh+dt@xxxxxxxxxx;
> mark.rutland@xxxxxxx; Michal Simek <michals@xxxxxxxxxx>; Rajan Vaja
> <RAJANV@xxxxxxxxxx>; Jolly Shah <JOLLYS@xxxxxxxxxx>;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [RFC PATCH v3 3/3] reset: reset-zynqmp: Adding support for Xilinx
> zynqmp reset controller.
> 
> Hi,
> 
> thank you for the patch. I have a few comments below:
> 
> On Wed, 2018-09-05 at 12:39 +0530, Nava kishore Manne wrote:
> > Add a reset controller driver for Xilinx Zynq UltraScale+ MPSoC.
> > The zynqmp reset-controller has the ability to reset lines connected
> > to different blocks and peripheral in the Soc.
> >
> > Signed-off-by: Nava kishore Manne <nava.manne@xxxxxxxxxx>
> > ---
> > Changes for v3:
> > 		-None.
> > Changes for v2:
> > 		-Moved eemi_ops into a priv struct as suggested
> > 		 by philipp.
> >
> >  drivers/reset/Makefile       |   1 +
> >  drivers/reset/reset-zynqmp.c | 115
> > +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 116 insertions(+)
> >  create mode 100644 drivers/reset/reset-zynqmp.c
> >
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index
> > c1261dc..27e4a33 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -21,4 +21,5 @@ obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> >  obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> >  obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
> >  obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
> > +obj-$(CONFIG_ARCH_ZYNQMP) += reset-zynqmp.o
> >
> > diff --git a/drivers/reset/reset-zynqmp.c
> > b/drivers/reset/reset-zynqmp.c new file mode 100644 index
> > 0000000..f908492
> > --- /dev/null
> > +++ b/drivers/reset/reset-zynqmp.c
> > @@ -0,0 +1,115 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2018 Xilinx, Inc.
> > + *
> > + */
> > +
> > +#include <linux/io.h>
> 
> I think including io.h is not necessary.
> 
> [...]

Will fix in the next version.

> > +static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> > +			       unsigned long id)
> > +{
> > +	struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> > +	int val, err;
> > +
> > +	err = priv->eemi_ops->reset_get_status(ZYNQMP_RESET_ID + id, &val);
> > +	if (!err)
> > +		return -EINVAL;
> 
> This looks like it should be
> 
> 	if (err)
> 		return err;
> 
> instead.
> 
> [...]
Will fix in the next version.

> > +static struct reset_control_ops zynqmp_reset_ops = {
> 
> static const struct reset_control_ops zynqmp_reset_ops = {
> 
Will fix in the next version.

> > +	.reset = zynqmp_reset_reset,
> > +	.assert = zynqmp_reset_assert,
> > +	.deassert = zynqmp_reset_deassert,
> > +	.status = zynqmp_reset_status,
> > +};
> > +
> > +static int zynqmp_reset_probe(struct platform_device *pdev) {
> > +	struct zynqmp_reset_data *priv;
> > +
> > +	priv = devm_kzalloc(&pdev->dev,
> > +			    sizeof(*priv), GFP_KERNEL);
> 
> This should fit on one line.
> 
Will fix in the next version.

Regards,
Navakishore.