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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BYAPR02MB47101009A8B5F7E1BB20C7A2C2050@BYAPR02MB4710.namprd02.prod.outlook.com>
Date:   Mon, 10 Sep 2018 04:49:50 +0000
From:   Nava kishore Manne <navam@...inx.com>
To:     Philipp Zabel <p.zabel@...gutronix.de>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        Michal Simek <michals@...inx.com>,
        Rajan Vaja <RAJANV@...inx.com>, Jolly Shah <JOLLYS@...inx.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC PATCH v3 3/3] reset: reset-zynqmp: Adding support for Xilinx
 zynqmp reset controller.

Hi Philipp

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

> -----Original Message-----
> From: Philipp Zabel [mailto:p.zabel@...gutronix.de]
> Sent: Wednesday, September 5, 2018 4:00 PM
> To: Nava kishore Manne <navam@...inx.com>; robh+dt@...nel.org;
> mark.rutland@....com; Michal Simek <michals@...inx.com>; Rajan Vaja
> <RAJANV@...inx.com>; Jolly Shah <JOLLYS@...inx.com>;
> devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org
> 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@...inx.com>
> > ---
> > 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.