[PATCH] drivers: ptp: Include new header file in ptp_pch.c

[PATCH] drivers: ptp: Include new header file in ptp_pch.c

[Rashika Kheria]

Create a new header file include/linux/ptp_pch.h which contains
prototype declaration of functions pch_ch_control_read(),
pch_ch_control_write(), pch_ch_event_read(), pch_ch_event_write(),
pch_src_uuid_lo_read(), pch_src_uuid_hi_read(), pch_rx_snap_read(),
pch_tx_snap_read() and pch_set_station_address().

Include the new header file in drivers/ptp/ptp_pch.c and
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c because they
use these functions. Remove these functions prototypes from header
file drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h.

This eliminates the following warnings in ptp_pch.c:

drivers/ptp/ptp_pch.c:194:5: warning: no previous prototype for ‘pch_ch_control_read’ [-Wmissing-prototypes]
drivers/ptp/ptp_pch.c:205:6: warning: no previous prototype for ‘pch_ch_control_write’ [-Wmissing-prototypes]
drivers/ptp/ptp_pch.c:213:5: warning: no previous prototype for ‘pch_ch_event_read’ [-Wmissing-prototypes]
drivers/ptp/ptp_pch.c:224:6: warning: no previous prototype for ‘pch_ch_event_write’ [-Wmissing-prototypes]
drivers/ptp/ptp_pch.c:232:5: warning: no previous prototype for ‘pch_src_uuid_lo_read’ [-Wmissing-prototypes]
drivers/ptp/ptp_pch.c:243:5: warning: no previous prototype for ‘pch_src_uuid_hi_read’ [-Wmissing-prototypes]
drivers/ptp/ptp_pch.c:254:5: warning: no previous prototype for ‘pch_rx_snap_read’ [-Wmissing-prototypes]
drivers/ptp/ptp_pch.c:271:5: warning: no previous prototype for ‘pch_tx_snap_read’ [-Wmissing-prototypes]
drivers/ptp/ptp_pch.c:312:5: warning: no previous prototype for ‘pch_set_station_address’ [-Wmissing-prototypes]

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    |    9 ---------
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    1 +
 drivers/ptp/ptp_pch.c                              |    1 +
 include/linux/ptp_pch.h                            |   16 ++++++++++++++++
 4 files changed, 18 insertions(+), 9 deletions(-)
 create mode 100644 include/linux/ptp_pch.h

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
index 2a900307..79533c8 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
@@ -666,15 +666,6 @@ void pch_gbe_free_tx_resources(struct pch_gbe_adapter *adapter,
 void pch_gbe_free_rx_resources(struct pch_gbe_adapter *adapter,
 			       struct pch_gbe_rx_ring *rx_ring);
 void pch_gbe_update_stats(struct pch_gbe_adapter *adapter);
-u32 pch_ch_control_read(struct pci_dev *pdev);
-void pch_ch_control_write(struct pci_dev *pdev, u32 val);
-u32 pch_ch_event_read(struct pci_dev *pdev);
-void pch_ch_event_write(struct pci_dev *pdev, u32 val);
-u32 pch_src_uuid_lo_read(struct pci_dev *pdev);
-u32 pch_src_uuid_hi_read(struct pci_dev *pdev);
-u64 pch_rx_snap_read(struct pci_dev *pdev);
-u64 pch_tx_snap_read(struct pci_dev *pdev);
-int pch_set_station_address(u8 *addr, struct pci_dev *pdev);
 
 /* pch_gbe_param.c */
 void pch_gbe_check_options(struct pch_gbe_adapter *adapter);
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 27ffe0e..03e2c14 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -24,6 +24,7 @@
 #include <linux/net_tstamp.h>
 #include <linux/ptp_classify.h>
 #include <linux/gpio.h>
+#include <linux/ptp_pch.h>
 
 #define DRV_VERSION     "1.01"
 const char pch_driver_version[] = DRV_VERSION;
diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c
index 71a2559..ec5cf7d 100644
--- a/drivers/ptp/ptp_pch.c
+++ b/drivers/ptp/ptp_pch.c
@@ -20,6 +20,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.
  */
 
+#include <linux/ptp_pch.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/init.h>
diff --git a/include/linux/ptp_pch.h b/include/linux/ptp_pch.h
new file mode 100644
index 0000000..bd23d53
--- /dev/null
+++ b/include/linux/ptp_pch.h
@@ -0,0 +1,16 @@
+#include <linux/types.h>
+#include <linux/pci.h>
+
+/* Function prototype declaration common between
+ * drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c and
+ * drivers/ptp/ptp_pch.c.*/
+
+u32 pch_ch_control_read(struct pci_dev *pdev);
+void pch_ch_control_write(struct pci_dev *pdev, u32 val);
+u32 pch_ch_event_read(struct pci_dev *pdev);
+void pch_ch_event_write(struct pci_dev *pdev, u32 val);
+u32 pch_src_uuid_lo_read(struct pci_dev *pdev);
+u32 pch_src_uuid_hi_read(struct pci_dev *pdev);
+u64 pch_rx_snap_read(struct pci_dev *pdev);
+u64 pch_tx_snap_read(struct pci_dev *pdev);
+int pch_set_station_address(u8 *addr, struct pci_dev *pdev);
-- 
1.7.9.5

Re: [PATCH] drivers: ptp: Include new header file in ptp_pch.c

[Josh Triplett]

On Mon, Dec 16, 2013 at 02:14:15AM +0530, Rashika Kheria wrote:
> Create a new header file include/linux/ptp_pch.h which contains
> prototype declaration of functions pch_ch_control_read(),
> pch_ch_control_write(), pch_ch_event_read(), pch_ch_event_write(),
> pch_src_uuid_lo_read(), pch_src_uuid_hi_read(), pch_rx_snap_read(),
> pch_tx_snap_read() and pch_set_station_address().
> 
> Include the new header file in drivers/ptp/ptp_pch.c and
> drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c because they
> use these functions. Remove these functions prototypes from header
> file drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h.
> 
> This eliminates the following warnings in ptp_pch.c:
> 
> drivers/ptp/ptp_pch.c:194:5: warning: no previous prototype for ‘pch_ch_control_read’ [-Wmissing-prototypes]
> drivers/ptp/ptp_pch.c:205:6: warning: no previous prototype for ‘pch_ch_control_write’ [-Wmissing-prototypes]
> drivers/ptp/ptp_pch.c:213:5: warning: no previous prototype for ‘pch_ch_event_read’ [-Wmissing-prototypes]
> drivers/ptp/ptp_pch.c:224:6: warning: no previous prototype for ‘pch_ch_event_write’ [-Wmissing-prototypes]
> drivers/ptp/ptp_pch.c:232:5: warning: no previous prototype for ‘pch_src_uuid_lo_read’ [-Wmissing-prototypes]
> drivers/ptp/ptp_pch.c:243:5: warning: no previous prototype for ‘pch_src_uuid_hi_read’ [-Wmissing-prototypes]
> drivers/ptp/ptp_pch.c:254:5: warning: no previous prototype for ‘pch_rx_snap_read’ [-Wmissing-prototypes]
> drivers/ptp/ptp_pch.c:271:5: warning: no previous prototype for ‘pch_tx_snap_read’ [-Wmissing-prototypes]
> drivers/ptp/ptp_pch.c:312:5: warning: no previous prototype for ‘pch_set_station_address’ [-Wmissing-prototypes]
> 
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    |    9 ---------
>  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    1 +
>  drivers/ptp/ptp_pch.c                              |    1 +
>  include/linux/ptp_pch.h                            |   16 ++++++++++++++++
>  4 files changed, 18 insertions(+), 9 deletions(-)
>  create mode 100644 include/linux/ptp_pch.h
> 
> diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
> index 2a900307..79533c8 100644
> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
> @@ -666,15 +666,6 @@ void pch_gbe_free_tx_resources(struct pch_gbe_adapter *adapter,
>  void pch_gbe_free_rx_resources(struct pch_gbe_adapter *adapter,
>  			       struct pch_gbe_rx_ring *rx_ring);
>  void pch_gbe_update_stats(struct pch_gbe_adapter *adapter);
> -u32 pch_ch_control_read(struct pci_dev *pdev);
> -void pch_ch_control_write(struct pci_dev *pdev, u32 val);
> -u32 pch_ch_event_read(struct pci_dev *pdev);
> -void pch_ch_event_write(struct pci_dev *pdev, u32 val);
> -u32 pch_src_uuid_lo_read(struct pci_dev *pdev);
> -u32 pch_src_uuid_hi_read(struct pci_dev *pdev);
> -u64 pch_rx_snap_read(struct pci_dev *pdev);
> -u64 pch_tx_snap_read(struct pci_dev *pdev);
> -int pch_set_station_address(u8 *addr, struct pci_dev *pdev);
>  
>  /* pch_gbe_param.c */
>  void pch_gbe_check_options(struct pch_gbe_adapter *adapter);
> diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> index 27ffe0e..03e2c14 100644
> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> @@ -24,6 +24,7 @@
>  #include <linux/net_tstamp.h>
>  #include <linux/ptp_classify.h>
>  #include <linux/gpio.h>
> +#include <linux/ptp_pch.h>
>  
>  #define DRV_VERSION     "1.01"
>  const char pch_driver_version[] = DRV_VERSION;
> diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c
> index 71a2559..ec5cf7d 100644
> --- a/drivers/ptp/ptp_pch.c
> +++ b/drivers/ptp/ptp_pch.c
> @@ -20,6 +20,7 @@
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.
>   */
>  
> +#include <linux/ptp_pch.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/init.h>
> diff --git a/include/linux/ptp_pch.h b/include/linux/ptp_pch.h
> new file mode 100644
> index 0000000..bd23d53
> --- /dev/null
> +++ b/include/linux/ptp_pch.h
> @@ -0,0 +1,16 @@
> +#include <linux/types.h>
> +#include <linux/pci.h>
> +
> +/* Function prototype declaration common between
> + * drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c and
> + * drivers/ptp/ptp_pch.c.*/
> +
> +u32 pch_ch_control_read(struct pci_dev *pdev);
> +void pch_ch_control_write(struct pci_dev *pdev, u32 val);
> +u32 pch_ch_event_read(struct pci_dev *pdev);
> +void pch_ch_event_write(struct pci_dev *pdev, u32 val);
> +u32 pch_src_uuid_lo_read(struct pci_dev *pdev);
> +u32 pch_src_uuid_hi_read(struct pci_dev *pdev);
> +u64 pch_rx_snap_read(struct pci_dev *pdev);
> +u64 pch_tx_snap_read(struct pci_dev *pdev);
> +int pch_set_station_address(u8 *addr, struct pci_dev *pdev);
> -- 
> 1.7.9.5
> 

Re: [PATCH] drivers: ptp: Include new header file in ptp_pch.c

[Richard Cochran]

On Mon, Dec 16, 2013 at 02:14:15AM +0530, Rashika Kheria wrote:
> ---
>  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    |    9 ---------
>  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    1 +
>  drivers/ptp/ptp_pch.c                              |    1 +
>  include/linux/ptp_pch.h                            |   16 ++++++++++++++++
>  4 files changed, 18 insertions(+), 9 deletions(-)
>  create mode 100644 include/linux/ptp_pch.h

Instead of adding a random driver header into include/linux, I would
prefer that you just move the ptp_pch.c from drivers/ptp to
drivers/net/ethernet/oki-semi/pch_gbe. Then you can just include
pch_gbe.h directly.

Thanks,
Richard

Re: [PATCH] drivers: ptp: Include new header file in ptp_pch.c

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Mon, 16 Dec 2013 09:58:40 +0100

> On Mon, Dec 16, 2013 at 02:14:15AM +0530, Rashika Kheria wrote:
>> ---
>>  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    |    9 ---------
>>  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    1 +
>>  drivers/ptp/ptp_pch.c                              |    1 +
>>  include/linux/ptp_pch.h                            |   16 ++++++++++++++++
>>  4 files changed, 18 insertions(+), 9 deletions(-)
>>  create mode 100644 include/linux/ptp_pch.h
> 
> Instead of adding a random driver header into include/linux, I would
> prefer that you just move the ptp_pch.c from drivers/ptp to
> drivers/net/ethernet/oki-semi/pch_gbe. Then you can just include
> pch_gbe.h directly.

I think this begs an even more fundamental question, why isn't the PTP
driver abstraction providing the necessary methods and interfaces so
that pch_gbe doesn't have to call into the ptp_pch.c code directly?

Moving ptp_pch.c elsehwere is not desirable, it's a PTP driver so
it belongs under drivers/ptp.

Someone who understands all of these components needs to sort this
out cleanly, this patch isn't it.

Re: [PATCH] drivers: ptp: Include new header file in ptp_pch.c

[josh]

On Wed, Dec 18, 2013 at 05:43:59PM -0500, David Miller wrote:
> From: Richard Cochran <richardcochran@gmail.com>
> Date: Mon, 16 Dec 2013 09:58:40 +0100
> 
> > On Mon, Dec 16, 2013 at 02:14:15AM +0530, Rashika Kheria wrote:
> >> ---
> >>  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    |    9 ---------
> >>  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    1 +
> >>  drivers/ptp/ptp_pch.c                              |    1 +
> >>  include/linux/ptp_pch.h                            |   16 ++++++++++++++++
> >>  4 files changed, 18 insertions(+), 9 deletions(-)
> >>  create mode 100644 include/linux/ptp_pch.h
> > 
> > Instead of adding a random driver header into include/linux, I would
> > prefer that you just move the ptp_pch.c from drivers/ptp to
> > drivers/net/ethernet/oki-semi/pch_gbe. Then you can just include
> > pch_gbe.h directly.
> 
> I think this begs an even more fundamental question, why isn't the PTP
> driver abstraction providing the necessary methods and interfaces so
> that pch_gbe doesn't have to call into the ptp_pch.c code directly?
> 
> Moving ptp_pch.c elsehwere is not desirable, it's a PTP driver so
> it belongs under drivers/ptp.

For the moment, at least, would it be reasonable to have a proper header
for these functions since pch_gbe is currently calling them?  Making
that driver *not* call those functions might well be a sensible cleanup,
but does fixing this issue need to wait for that cleanup to happen?

- Josh Triplett

Re: [PATCH] drivers: ptp: Include new header file in ptp_pch.c

[Richard Cochran]

On Wed, Dec 18, 2013 at 05:43:59PM -0500, David Miller wrote:
> 
> I think this begs an even more fundamental question, why isn't the PTP
> driver abstraction providing the necessary methods and interfaces so
> that pch_gbe doesn't have to call into the ptp_pch.c code directly?

Really it is more of a driver issue. For MAC based PTP clocks, they
are so tightly coupled to the MAC driver that it is better *not* to
have the PTP functions in a separate module. Instead, if having the
PTP stuff optional makes sense, then the PTP parts are just a compile
time option in the MAC driver. This is how the other MAC drivers with
optional PTP are structured.

> Moving ptp_pch.c elsehwere is not desirable, it's a PTP driver so
> it belongs under drivers/ptp.
> 
> Someone who understands all of these components needs to sort this
> out cleanly, this patch isn't it.

I am getting a bit tired of all the trouble over the past year caused
by this particular driver. After it got merged, the original authors
disappeared. I would really like to see this driver follow the pattern
of bfin_mac, ti/cpts, and tile. Maybe I'll have to do it myself, like
the Red Hen.

Thanks,
Richard

Re: [PATCH] drivers: ptp: Include new header file in ptp_pch.c

[Ben Hutchings]

On Thu, 2013-12-19 at 08:27 +0100, Richard Cochran wrote:
> On Wed, Dec 18, 2013 at 05:43:59PM -0500, David Miller wrote:
> > 
> > I think this begs an even more fundamental question, why isn't the PTP
> > driver abstraction providing the necessary methods and interfaces so
> > that pch_gbe doesn't have to call into the ptp_pch.c code directly?
> 
> Really it is more of a driver issue. For MAC based PTP clocks, they
> are so tightly coupled to the MAC driver that it is better *not* to
> have the PTP functions in a separate module. Instead, if having the
> PTP stuff optional makes sense, then the PTP parts are just a compile
> time option in the MAC driver. This is how the other MAC drivers with
> optional PTP are structured.
[...]

PTP is no longer a compile-time option in most net drivers.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] drivers: ptp: Include new header file in ptp_pch.c

[Richard Cochran]

On Thu, Dec 19, 2013 at 07:13:20PM +0000, Ben Hutchings wrote:
> 
> PTP is no longer a compile-time option in most net drivers.

Right, for those drivers where there is little or no performance
impact, there is no good reason not to enable the PTP bits.

But if the PTP code entails a run time cost, then we want it to be
optional, but not as a separate module. I suspect that the pch device
falls into this category, but I am not sure without taking another
look.

Thanks,
Richard