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;
From:   Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com>
To:     "Y.b. Lu" <yangbo.lu@nxp.com>, Andrew Lunn <andrew@lunn.ch>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        Richard Cochran <richardcochran@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
Thread-Topic: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
Thread-Index: AQHUVlODydoczAdnGUGD+YTbzcfAFKUEHlyAgAE4nQCAACKAAA==
Date:   Fri, 28 Sep 2018 10:20:55 +0000
Message-ID: <DBXPR04MB3490DB9A1EF2D814568843B94EC0@DBXPR04MB349.eurprd04.prod.outlook.com>
References: <20180927111228.46118-1-yangbo.lu@nxp.com>
 <20180927132507.GB23375@lunn.ch>
 <VI1PR0401MB223737CACE4E4064AE8325A4F8EC0@VI1PR0401MB2237.eurprd04.prod.outlook.com>
In-Reply-To: <VI1PR0401MB223737CACE4E4064AE8325A4F8EC0@VI1PR0401MB2237.eurprd04.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 2641f0af-fc29-4231-c1e8-08d6252c13c4
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Sep 2018 10:20:55.8713
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBXPR04MB413
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----Original Message-----
> From: Y.b. Lu
> Sent: Friday, September 28, 2018 11:04 AM
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org;
> netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>;
> David S . Miller <davem@davemloft.net>; Ioana Ciocoi Radulescu
> <ruxandra.radulescu@nxp.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Subject: RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging=
/
>=20
> Hi Andrew,
>=20
> Thanks a lot for your comments.
> Please see my comments inline.
>=20
> Best regards,
> Yangbo Lu
>=20
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Thursday, September 27, 2018 9:25 PM
> > To: Y.b. Lu <yangbo.lu@nxp.com>
> > Cc: linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org;
> > netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>;
> > David S . Miller <davem@davemloft.net>; Ioana Ciocoi Radulescu
> > <ruxandra.radulescu@nxp.com>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>
> > Subject: Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of
> staging/
> >
> > On Thu, Sep 27, 2018 at 07:12:27PM +0800, Yangbo Lu wrote:
> > > This patch is to move DPAA2 PTP driver out of staging/ since the
> > > dpaa2-eth had been moved out.
> > >
> > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > ---
> > >  drivers/net/ethernet/freescale/Kconfig             |    9 +--------
> > >  drivers/net/ethernet/freescale/dpaa2/Kconfig       |   15
> > +++++++++++++++
> > >  drivers/net/ethernet/freescale/dpaa2/Makefile      |    6 ++++--
> > >  .../ethernet/freescale/dpaa2}/dprtc-cmd.h          |    0
> > >  .../rtc =3D> net/ethernet/freescale/dpaa2}/dprtc.c   |    0
> > >  .../rtc =3D> net/ethernet/freescale/dpaa2}/dprtc.h   |    0
> > >  .../rtc =3D> net/ethernet/freescale/dpaa2}/rtc.c     |    0
> > >  .../rtc =3D> net/ethernet/freescale/dpaa2}/rtc.h     |    0
> > >  drivers/staging/fsl-dpaa2/Kconfig                  |    8 --------
> > >  drivers/staging/fsl-dpaa2/Makefile                 |    1 -
> > >  drivers/staging/fsl-dpaa2/rtc/Makefile             |    7 -------
> > >  11 files changed, 20 insertions(+), 26 deletions(-)  create mode
> > > 100644 drivers/net/ethernet/freescale/dpaa2/Kconfig
> > >  rename drivers/{staging/fsl-dpaa2/rtc =3D>
> > > net/ethernet/freescale/dpaa2}/dprtc-cmd.h (100%)  rename
> > > drivers/{staging/fsl-dpaa2/rtc =3D>
> > > net/ethernet/freescale/dpaa2}/dprtc.c (100%)  rename
> > > drivers/{staging/fsl-dpaa2/rtc =3D>
> > > net/ethernet/freescale/dpaa2}/dprtc.h (100%)  rename
> > > drivers/{staging/fsl-dpaa2/rtc =3D> net/ethernet/freescale/dpaa2}/rtc=
.c
> > > (100%)  rename drivers/{staging/fsl-dpaa2/rtc =3D>
> > > net/ethernet/freescale/dpaa2}/rtc.h (100%)
> >
> > Hi Yangbo
> >
> > Calling a ptp driver rtc.[ch] seems rather odd. Could you fixup the nam=
e,
> > change it to ptp.[ch]. Also, some of the function names, and structures=
,
> > rtc_probe->ptp_probe, rtc_remove->ptp_remove, rtc_match_id_table->
> > ptp_match_id_table, etc.
>=20
> [Y.b. Lu] Indeed, it's odd and confusing...
> For dpaa2, all hardware resources are allocated and configured through th=
e
> Management Complex (MC) portals.
> MC abstracts most of these resources as DPAA2 objects and exposes ABIs
> through which they can be configured and controlled.
> This ptp timer was named as rtc in MC firmware and APIs as you saw in
> dprtc.*.
> So initially I wrote this driver using rtc as name.
>=20
> No worries, let me change it in next version.
>=20
> >
> > ptp_dpaa2_adjfreq() probably should return err, not 0.
> > ptp_dpaa2_gettime() again does not return the error.
> > If fact, it seems like all the main functions ignore errors.
>=20
> [Y.b. Lu] Will fix the returns in next version.
>=20
> >
> > kzalloc() could be changed to devm_kzalloc() to simplify the cleanup
>=20
> [Y.b. Lu] Will use devm_kzalloc() in next version.
>=20
>  Can
> > ptp_dpaa2_caps be made const?
>=20
> [Y.b. Lu] Yes. Will change it in next version.
>=20
> > dpaa2_phc_index does not appear to be used.
>=20
> [Y.b. Lu] It's used in dpaa2-ethtool.c for .get_ts_info interface of
> ethtool_ops.
>=20
> > dev_set_drvdata(dev, NULL); is not needed.
>=20
> [Y.b. Lu] Will remove it in next version.
>=20
> > Can rtc_drv be made const?
>=20
> [Y.b. Lu] Will use const in next version.
>=20
> > Is rtc.h used by anything other than rtc.c? It seems like it can be rem=
oved.
>=20
> [Y.b. Lu] Let me remove it in next version.
>=20
> >
> > It seems like there is a lot of code in dprtc.c which is unused. rtc.c =
does
> nothing
> > with interrupts for example. Do you plan to make use of this extra code=
? Or
> > can it be removed leaving just what is needed?
>=20
> [Y.b. Lu] Currently the ptp/rtc driver is not full-featured. The extra co=
de is
> being planed to be used.

Are there any interrupts associated to the real time clock module that will
actually be used by the driver? Also, I don't think the create/destroy func=
tions
are meant to be used by the PTP kernel driver, even though MC exposes the
APIs for them.

Generally speaking, I think it's better to remove unused code from the curr=
ent
driver and re-add it along with the feature actually using it.
=20
>=20
> >
> > struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct see=
ms
> very
> > odd. And it is not the only example.
>=20
> [Y.b. Lu] This should depended on MC firmware and APIs I think. Once the
> MC improves this, the APIs could be updated to fix this.

These structures map the command format expected by the MC firmware. I
agree that some of the command layouts are less than inspired, but I'm not
sure we can expect MC to "improve" them without a good reason, as this woul=
d
break backward compatibility.

I also want to bring up the question of where the dpaa2 ptp driver should b=
e
located. The qoriq_ptp driver (which targets previous gen Freescale/NXP
architectures) is located in drivers/ptp. I'm not sure if the dpaa2 ptp dri=
ver should
be moved there as well or it's better suited for the currently proposed loc=
ation.

Thanks,
Ioana