-----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 [off-list ref];
David S . Miller [off-list ref]; Ioana Ciocoi Radulescu
[off-list ref]; Greg Kroah-Hartman
[off-list ref]
Subject: RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/

Hi Andrew,

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

Best regards,
Yangbo Lu

quoted

-----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 [off-list ref];
David S . Miller [off-list ref]; Ioana Ciocoi Radulescu
[off-list ref]; Greg Kroah-Hartman
[off-list ref]
Subject: Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of

staging/

quoted

On Thu, Sep 27, 2018 at 07:12:27PM +0800, Yangbo Lu wrote:

quoted

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

+++++++++++++++

quoted

 drivers/net/ethernet/freescale/dpaa2/Makefile      |    6 ++++--
 .../ethernet/freescale/dpaa2}/dprtc-cmd.h          |    0
 .../rtc => net/ethernet/freescale/dpaa2}/dprtc.c   |    0
 .../rtc => net/ethernet/freescale/dpaa2}/dprtc.h   |    0
 .../rtc => net/ethernet/freescale/dpaa2}/rtc.c     |    0
 .../rtc => 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 =>
net/ethernet/freescale/dpaa2}/dprtc-cmd.h (100%)  rename
drivers/{staging/fsl-dpaa2/rtc =>
net/ethernet/freescale/dpaa2}/dprtc.c (100%)  rename
drivers/{staging/fsl-dpaa2/rtc =>
net/ethernet/freescale/dpaa2}/dprtc.h (100%)  rename
drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/rtc.c
(100%)  rename drivers/{staging/fsl-dpaa2/rtc =>
net/ethernet/freescale/dpaa2}/rtc.h (100%)

Hi Yangbo

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

No worries, let me change it in next version.

quoted

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.

quoted

kzalloc() could be changed to devm_kzalloc() to simplify the cleanup

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

 Can

quoted

ptp_dpaa2_caps be made const?

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

quoted

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

quoted

dev_set_drvdata(dev, NULL); is not needed.

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

quoted

Can rtc_drv be made const?

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

quoted

Is rtc.h used by anything other than rtc.c? It seems like it can be removed.

[Y.b. Lu] Let me remove it in next version.

quoted

It seems like there is a lot of code in dprtc.c which is unused. rtc.c does

nothing

quoted

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.

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 functions
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 current
driver and re-add it along with the feature actually using it.
 

quoted

struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct seems

very

quoted

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.

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 would
break backward compatibility.

I also want to bring up the question of where the dpaa2 ptp driver should be
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 driver should
be moved there as well or it's better suited for the currently proposed location.

Thanks,
Ioana