RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/

[Y.b. Lu]

Hi Andrew,

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

Best regards,
Yangbo Lu

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Thursday, September 27, 2018 9:25 PM
> To: Y.b. Lu <yangbo.lu@xxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; Richard Cochran <richardcochran@xxxxxxxxx>;
> David S . Miller <davem@xxxxxxxxxxxxx>; Ioana Ciocoi Radulescu
> <ruxandra.radulescu@xxxxxxx>; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxx>
> > ---
> >  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 => 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.

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

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

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

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

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

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

> 
>       Andrew