| Message ID | 1387140255-11797-1-git-send-email-rashika.kheria@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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