| Message ID | 20181207215136.2808-1-marex@denx.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [V2] net: dsa: ksz: Add reset GPIO handling | expand |
On Fri, Dec 07, 2018 at 10:51:36PM +0100, Marek Vasut wrote: > Add code to handle optional reset GPIO in the KSZ switch driver. The switch > has a reset GPIO line which can be controlled by the CPU, so make sure it is > configured correctly in such setups. Hi Marek Please make this a patch series, not two individual patches. And as David has already said, include a cover letter. Otherwise, this looks O.K. Thanks Andrew
On 12/07/2018 11:24 PM, Andrew Lunn wrote: > On Fri, Dec 07, 2018 at 10:51:36PM +0100, Marek Vasut wrote: >> Add code to handle optional reset GPIO in the KSZ switch driver. The switch >> has a reset GPIO line which can be controlled by the CPU, so make sure it is >> configured correctly in such setups. > > Hi Marek Hi Andrew, > Please make this a patch series, not two individual patches. This actually is an individual patch, it doesn't depend on anything. Or do you mean a series with the DT documentation change ? > And as David has already said, include a cover letter. > > Otherwise, this looks O.K. > > Thanks > Andrew >
From: Marek Vasut <marex@denx.de> Date: Fri, 7 Dec 2018 23:59:58 +0100 > On 12/07/2018 11:24 PM, Andrew Lunn wrote: >> On Fri, Dec 07, 2018 at 10:51:36PM +0100, Marek Vasut wrote: >>> Add code to handle optional reset GPIO in the KSZ switch driver. The switch >>> has a reset GPIO line which can be controlled by the CPU, so make sure it is >>> configured correctly in such setups. >> >> Hi Marek > > Hi Andrew, > >> Please make this a patch series, not two individual patches. > > This actually is an individual patch, it doesn't depend on anything. > Or do you mean a series with the DT documentation change ? Yes, but all of this stuff is building up for one single purpose, and that is to support a new mode of operation with DSA or whatever. So please group them together in a series with an appropriate header posting.
On 12/08/2018 12:46 AM, David Miller wrote: > From: Marek Vasut <marex@denx.de> > Date: Fri, 7 Dec 2018 23:59:58 +0100 > >> On 12/07/2018 11:24 PM, Andrew Lunn wrote: >>> On Fri, Dec 07, 2018 at 10:51:36PM +0100, Marek Vasut wrote: >>>> Add code to handle optional reset GPIO in the KSZ switch driver. The switch >>>> has a reset GPIO line which can be controlled by the CPU, so make sure it is >>>> configured correctly in such setups. >>> >>> Hi Marek >> >> Hi Andrew, >> >>> Please make this a patch series, not two individual patches. >> >> This actually is an individual patch, it doesn't depend on anything. >> Or do you mean a series with the DT documentation change ? > > Yes, but all of this stuff is building up for one single purpose, > and that is to support a new mode of operation with DSA or whatever. I'll group together the ones which make sense to group together and are not orthogonal if that's OK with you. The reset handling really is orthogonal from the rest and can go in independently of the rest. > So please group them together in a series with an appropriate > header posting. Sure
> This actually is an individual patch, it doesn't depend on anything. > Or do you mean a series with the DT documentation change ? Yes, i mean together with the DT documentation change. Those two belong together, they are one functional change. Part of this is also to do with scalability. It takes less effort to merge one patchset of two patches, as two individual patches. The truth is, developer time is cheap, maintainer time is expensive, so the process is optimized towards making the maintainers life easy. So sometimes you do combine orthogonal changes together into one patchset, if there is a high purpose, eg. adding support for a new device on a new board. However, given the situation of two overlapping patchsets, it might be better to submit smaller patchsets. Andrew
On 12/08/2018 12:11 PM, Andrew Lunn wrote: >> This actually is an individual patch, it doesn't depend on anything. >> Or do you mean a series with the DT documentation change ? > > Yes, i mean together with the DT documentation change. Those two > belong together, they are one functional change. Fine > Part of this is also to do with scalability. It takes less effort to > merge one patchset of two patches, as two individual patches. The > truth is, developer time is cheap, maintainer time is expensive This is _not_ fine and I am actually offended by this statement. The way I read this is that maintainer time has more value than developer time, which justifies spending the developer time by maintainers without having any appreciation for it. I hope I am misreading your statement ? >, so > the process is optimized towards making the maintainers life easy. > > So sometimes you do combine orthogonal changes together into one > patchset, if there is a high purpose, eg. adding support for a new > device on a new board. However, given the situation of two overlapping > patchsets, it might be better to submit smaller patchsets. > > Andrew >
On Mon, Dec 10, 2018 at 02:26:51PM +0100, Marek Vasut wrote: > On 12/08/2018 12:11 PM, Andrew Lunn wrote: > >> This actually is an individual patch, it doesn't depend on anything. > >> Or do you mean a series with the DT documentation change ? > > > > Yes, i mean together with the DT documentation change. Those two > > belong together, they are one functional change. > > Fine > > > Part of this is also to do with scalability. It takes less effort to > > merge one patchset of two patches, as two individual patches. The > > truth is, developer time is cheap, maintainer time is expensive > > This is _not_ fine and I am actually offended by this statement. > The way I read this is that maintainer time has more value than > developer time, which justifies spending the developer time by > maintainers without having any appreciation for it. I hope I am > misreading your statement ? Hi Marek Sorry, i was not meaning to be offence. But it is part of the economics of the Linux Kernel. There are many more developers than maintainers. There are lots of studies over the years which suggests there are not enough maintainers, and those maintainers we have are overloaded. So the development process is based towards making the maintainer role easier, by asking the developers to do a bit more work. Writing easy to review patchsets, adding reviewed by tags to new versions of patches, including a summary of changes between each version, meaningful patchset, etc. Much of that is to make the reviewers job, who are mostly maintainers, easier. And to make the job of people like David, who does the actually merge, easier, scalable to the number of patches he needs to work on every day. Andrew
From: Marek Vasut <marex@denx.de> Date: Mon, 10 Dec 2018 14:26:51 +0100 > This is _not_ fine and I am actually offended by this statement. > The way I read this is that maintainer time has more value than > developer time, which justifies spending the developer time by > maintainers without having any appreciation for it. I hope I am > misreading your statement ? Absolutely maintainer time, which is devoted to handling the work done by _EVERYONE_, is more important than individual developer time. Therefore as much work as possible is pushed down to the end nodes, the developers, to make the top level maintainer's job easier. You may be offended, but this is exactly how things are and have been for a very long time.
On 12/10/2018 03:37 PM, Andrew Lunn wrote: > On Mon, Dec 10, 2018 at 02:26:51PM +0100, Marek Vasut wrote: >> On 12/08/2018 12:11 PM, Andrew Lunn wrote: >>>> This actually is an individual patch, it doesn't depend on anything. >>>> Or do you mean a series with the DT documentation change ? >>> >>> Yes, i mean together with the DT documentation change. Those two >>> belong together, they are one functional change. >> >> Fine >> >>> Part of this is also to do with scalability. It takes less effort to >>> merge one patchset of two patches, as two individual patches. The >>> truth is, developer time is cheap, maintainer time is expensive >> >> This is _not_ fine and I am actually offended by this statement. >> The way I read this is that maintainer time has more value than >> developer time, which justifies spending the developer time by >> maintainers without having any appreciation for it. I hope I am >> misreading your statement ? > > Hi Marek Hello Andrew, > Sorry, i was not meaning to be offence. But it is part of the > economics of the Linux Kernel. There are many more developers than > maintainers. There are lots of studies over the years which suggests > there are not enough maintainers, and those maintainers we have are > overloaded. That's understandable, it's not just Linux kernel that's suffering from lack of maintainers. > So the development process is based towards making the > maintainer role easier, by asking the developers to do a bit more > work. Writing easy to review patchsets, adding reviewed by tags to new > versions of patches, including a summary of changes between each > version, meaningful patchset, etc. And this is all fine and normal. > Much of that is to make the > reviewers job, who are mostly maintainers, easier. And to make the job > of people like David, who does the actually merge, easier, scalable to > the number of patches he needs to work on every day. That's also fine by me. Although, a single maintainer cannot scale indefinitely, but that's another discussion altogether. Notice how none of the above implied that someone's time is more important than someone else's. I'll just assume the above is what you wanted to express all along.
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 9705808c3af7a..3b12e2dcff31b 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -8,12 +8,14 @@ #include <linux/delay.h> #include <linux/export.h> #include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/platform_data/microchip-ksz.h> #include <linux/phy.h> #include <linux/etherdevice.h> #include <linux/if_bridge.h> +#include <linux/of_gpio.h> #include <linux/of_net.h> #include <net/dsa.h> #include <net/switchdev.h> @@ -294,6 +296,17 @@ int ksz_switch_register(struct ksz_device *dev, if (dev->pdata) dev->chip_id = dev->pdata->chip_id; + dev->reset_gpio = devm_gpiod_get_optional(dev->dev, "reset", + GPIOD_OUT_LOW); + if (IS_ERR(dev->reset_gpio)) + return PTR_ERR(dev->reset_gpio); + + if (dev->reset_gpio) { + gpiod_set_value(dev->reset_gpio, 1); + mdelay(10); + gpiod_set_value(dev->reset_gpio, 0); + } + mutex_init(&dev->reg_mutex); mutex_init(&dev->stats_mutex); mutex_init(&dev->alu_mutex); @@ -329,6 +342,10 @@ void ksz_switch_remove(struct ksz_device *dev) { dev->dev_ops->exit(dev); dsa_unregister_switch(dev->ds); + + if (dev->reset_gpio) + gpiod_set_value(dev->reset_gpio, 1); + } EXPORT_SYMBOL(ksz_switch_remove); diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h index a38ff0841ed4e..60b49010904bf 100644 --- a/drivers/net/dsa/microchip/ksz_priv.h +++ b/drivers/net/dsa/microchip/ksz_priv.h @@ -59,6 +59,8 @@ struct ksz_device { void *priv; + struct gpio_desc *reset_gpio; /* Optional reset GPIO */ + /* chip specific data */ u32 chip_id; int num_vlans;
Add code to handle optional reset GPIO in the KSZ switch driver. The switch has a reset GPIO line which can be controlled by the CPU, so make sure it is configured correctly in such setups. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com> Cc: Woojung Huh <woojung.huh@microchip.com> Cc: David S. Miller <davem@davemloft.net> Cc: Tristram Ha <Tristram.Ha@microchip.com> --- V2: Switch to devm_gpiod_get_optional() --- drivers/net/dsa/microchip/ksz_common.c | 17 +++++++++++++++++ drivers/net/dsa/microchip/ksz_priv.h | 2 ++ 2 files changed, 19 insertions(+)