| Message ID | ae0d528a-d602-f399-9d75-65bf9bbf1264@sigmadesigns.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes: > Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> Missing patch description. Don't bother though. I won't approve of this implementation. Suspend support has to depend on the chip since the EMAC core doesn't have anything of the kind. > --- > drivers/net/ethernet/aurora/nb8800.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c > index b71d8fb80610..9af2423aed03 100644 > --- a/drivers/net/ethernet/aurora/nb8800.c > +++ b/drivers/net/ethernet/aurora/nb8800.c > @@ -1437,6 +1437,26 @@ static int nb8800_remove(struct platform_device *pdev) > return 0; > } > > +static int nb8800_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct net_device *dev = platform_get_drvdata(pdev); > + > + if (netif_running(dev)) > + return nb8800_stop(dev); > + > + return 0; > +} > + > +static int nb8800_resume(struct platform_device *pdev) > +{ > + struct net_device *dev = platform_get_drvdata(pdev); > + > + if (netif_running(dev)) > + return nb8800_open(dev); > + > + return 0; > +} > + > static struct platform_driver nb8800_driver = { > .driver = { > .name = "nb8800", > @@ -1444,6 +1464,8 @@ static struct platform_driver nb8800_driver = { > }, > .probe = nb8800_probe, > .remove = nb8800_remove, > + .suspend = nb8800_suspend, > + .resume = nb8800_resume, > }; > > module_platform_driver(nb8800_driver); > -- > 2.15.0 >
On 14/11/2017 14:02, Måns Rullgård wrote: > Missing patch description. Don't bother though. I won't approve of > this implementation. I hope I can convince David that you should not have veto power over this driver just because it was you who submitted it upstream first. Per the copyright notice, it was rewritten using Sigma's driver as a reference. And as of today, only Sigma chips use this driver, a fact which is unlikely to change, since the IP is no longer sold. http://www.auroravlsi.com/products.html > Suspend support has to depend on the chip since the EMAC core doesn't > have anything of the kind. Your statement makes no sense to me. If suspending the platform loses context, then all that is required is saving said context at suspend, and restoring it at resume.
On Tue, Nov 14, 2017 at 03:22:04PM +0100, Marc Gonzalez wrote: > On 14/11/2017 14:02, Måns Rullgård wrote: > > > Missing patch description. Don't bother though. I won't approve of > > this implementation. > > I hope I can convince David that you should not have veto power over > this driver just because it was you who submitted it upstream first. In practice, anybody can request a veto on a patch. If a patch introduces a regression, and somebody reports it, that is pretty much an automatic veto. Please work with Mans to ensure you are not breaking the driver for the hardware he cares about. Andrew
On 14/11/2017 17:31, Andrew Lunn wrote: > On Tue, Nov 14, 2017 at 03:22:04PM +0100, Marc Gonzalez wrote: > >> On 14/11/2017 14:02, Måns Rullgård wrote: >> >>> Missing patch description. Don't bother though. I won't approve of >>> this implementation. >> >> I hope I can convince David that you should not have veto power over >> this driver just because it was you who submitted it upstream first. > > In practice, anybody can request a veto on a patch. If a patch > introduces a regression, and somebody reports it, that is pretty much > an automatic veto. > > Please work with Mans to ensure you are not breaking the driver for > the hardware he cares about. FWIW, removing generic support ("aurora,nb8800") does not break any existing hardware. The ethernet controller in the board Mans uses is supported by "sigma,smp8642-ethernet". I can't test my exact changes on a similar board because support for that board is not (yet?) upstream.
On Tue, Nov 14, 2017 at 06:08:47PM +0100, Marc Gonzalez wrote: > On 14/11/2017 17:31, Andrew Lunn wrote: > > > On Tue, Nov 14, 2017 at 03:22:04PM +0100, Marc Gonzalez wrote: > > > >> On 14/11/2017 14:02, Måns Rullgård wrote: > >> > >>> Missing patch description. Don't bother though. I won't approve of > >>> this implementation. > >> > >> I hope I can convince David that you should not have veto power over > >> this driver just because it was you who submitted it upstream first. > > > > In practice, anybody can request a veto on a patch. If a patch > > introduces a regression, and somebody reports it, that is pretty much > > an automatic veto. > > > > Please work with Mans to ensure you are not breaking the driver for > > the hardware he cares about. > > FWIW, removing generic support ("aurora,nb8800") does not break any existing hardware. Hi Marc I also did a quick search, and no board appears to use this, at the moment. But it does appear that the changes to the pause configuration while the DMA is running will break stuff. So there appears to be a legitimate reason for that patch to get a NACK. Andrew
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c index b71d8fb80610..9af2423aed03 100644 --- a/drivers/net/ethernet/aurora/nb8800.c +++ b/drivers/net/ethernet/aurora/nb8800.c @@ -1437,6 +1437,26 @@ static int nb8800_remove(struct platform_device *pdev) return 0; } +static int nb8800_suspend(struct platform_device *pdev, pm_message_t state) +{ + struct net_device *dev = platform_get_drvdata(pdev); + + if (netif_running(dev)) + return nb8800_stop(dev); + + return 0; +} + +static int nb8800_resume(struct platform_device *pdev) +{ + struct net_device *dev = platform_get_drvdata(pdev); + + if (netif_running(dev)) + return nb8800_open(dev); + + return 0; +} + static struct platform_driver nb8800_driver = { .driver = { .name = "nb8800", @@ -1444,6 +1464,8 @@ static struct platform_driver nb8800_driver = { }, .probe = nb8800_probe, .remove = nb8800_remove, + .suspend = nb8800_suspend, + .resume = nb8800_resume, }; module_platform_driver(nb8800_driver);
Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> --- drivers/net/ethernet/aurora/nb8800.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)