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:   "Y.b. Lu" <yangbo.lu@nxp.com>
To:     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>,
        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/
Thread-Topic: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
Thread-Index: AQHUVlODYLzUr6TXRkCk3Aso2dGK1KUEHlyAgAEqd6A=
Date:   Fri, 28 Sep 2018 08:04:00 +0000
Message-ID: <VI1PR0401MB223737CACE4E4064AE8325A4F8EC0@VI1PR0401MB2237.eurprd04.prod.outlook.com>
References: <20180927111228.46118-1-yangbo.lu@nxp.com>
 <20180927132507.GB23375@lunn.ch>
In-Reply-To: <20180927132507.GB23375@lunn.ch>
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: e44f3c18-8910-4e2f-4bcc-08d62518f302
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Sep 2018 08:04:00.4866
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0401MB2557
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Andrew,

Thanks a lot for your comments.
Please see my comments inline.

Best regards,
Yangbo Lu

> -----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=
/
>=20
> 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%)
>=20
> Hi Yangbo
>=20
> Calling a ptp driver rtc.[ch] seems rather odd. Could you fixup the name,
> 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.

[Y.b. Lu] Indeed, it's odd and confusing...
For dpaa2, all hardware resources are allocated and configured through the =
Management Complex (MC) portals.
MC abstracts most of these resources as DPAA2 objects and exposes ABIs thro=
ugh 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.

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.

[Y.b. Lu] Will fix the returns in next version.

>=20
> kzalloc() could be changed to devm_kzalloc() to simplify the cleanup

[Y.b. Lu] Will use devm_kzalloc() in next version.

 Can
> ptp_dpaa2_caps be made const?

[Y.b. Lu] Yes. Will change it in next version.

> dpaa2_phc_index does not appear to be used.

[Y.b. Lu] It's used in dpaa2-ethtool.c for .get_ts_info interface of ethtoo=
l_ops.

> dev_set_drvdata(dev, NULL); is not needed.

[Y.b. Lu] Will remove it in next version.

> Can rtc_drv be made const?

[Y.b. Lu] Will use const in next version.

> Is rtc.h used by anything other than rtc.c? It seems like it can be remov=
ed.

[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 do=
es 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?

[Y.b. Lu] Currently the ptp/rtc driver is not full-featured. The extra code=
 is being planed to be used.

>=20
> struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct seems=
 very
> odd. And it is not the only example.

[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.

>=20
>       Andrew