[PATCH net-next 0/2] dsa: add MT7530 GPIO support

[PATCH net-next 0/2] dsa: add MT7530 GPIO support

[DENG Qingfang]

MT7530's LED controller can be used as GPIO controller. Add support for
it.

DENG Qingfang (2):
  dt-bindings: net: dsa: add MT7530 GPIO controller binding
  drivers: net: dsa: mt7530: MT7530 optional GPIO support

 .../devicetree/bindings/net/dsa/mt7530.txt    |  6 ++
 drivers/net/dsa/mt7530.c                      | 96 +++++++++++++++++++
 drivers/net/dsa/mt7530.h                      | 20 ++++
 3 files changed, 122 insertions(+)

-- 
2.25.1

[PATCH net-next 1/2] dt-bindings: net: dsa: add MT7530 GPIO controller binding

[DENG Qingfang]

Add device tree binding to support MT7530 GPIO controller.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 Documentation/devicetree/bindings/net/dsa/mt7530.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/mt7530.txt b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
index 560369efad6c..de04626a8e9d 100644
--- a/Documentation/devicetree/bindings/net/dsa/mt7530.txt
+++ b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
@@ -76,6 +76,12 @@ phy-mode must be set, see also example 2 below!
  * mt7621: phy-mode = "rgmii-txid";
  * mt7623: phy-mode = "rgmii";
 
+Optional properties:
+
+- gpio-controller: Boolean; if defined, MT7530's LED controller will run on
+	GPIO mode.
+- #gpio-cells: Must be 2 if gpio-controller is defined.
+
 See Documentation/devicetree/bindings/net/dsa/dsa.txt for a list of additional
 required, optional properties and how the integrated switch subnodes must
 be specified.
-- 
2.25.1

[PATCH net-next 2/2] drivers: net: dsa: mt7530: MT7530 optional GPIO support

[DENG Qingfang]

MT7530's LED controller can drive up to 15 LED/GPIOs.

Add support for GPIO control and allow users to use its GPIOs by
setting gpio-controller property in device tree.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 drivers/net/dsa/mt7530.c | 96 ++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mt7530.h | 20 +++++++++
 2 files changed, 116 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index a67cac15a724..0686d8cbd086 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -18,6 +18,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
 #include <net/dsa.h>
 
 #include "mt7530.h"
@@ -1639,6 +1640,95 @@ mtk_get_tag_protocol(struct dsa_switch *ds, int port,
 	}
 }
 
+static u32
+mt7530_gpio_to_bit(unsigned int offset)
+{
+	return BIT(offset + offset / 3);
+}
+
+static int
+mt7530_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct mt7530_priv *priv = gpiochip_get_data(gc);
+	u32 bit = mt7530_gpio_to_bit(offset);
+
+	return !!(mt7530_read(priv, MT7530_LED_GPIO_DATA) & bit);
+}
+
+static void
+mt7530_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct mt7530_priv *priv = gpiochip_get_data(gc);
+	u32 bit = mt7530_gpio_to_bit(offset);
+
+	if (value)
+		mt7530_set(priv, MT7530_LED_GPIO_DATA, bit);
+	else
+		mt7530_clear(priv, MT7530_LED_GPIO_DATA, bit);
+}
+
+static int
+mt7530_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct mt7530_priv *priv = gpiochip_get_data(gc);
+	u32 bit = mt7530_gpio_to_bit(offset);
+
+	return (mt7530_read(priv, MT7530_LED_GPIO_DIR) & bit) ?
+		GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+}
+
+static int
+mt7530_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	struct mt7530_priv *priv = gpiochip_get_data(gc);
+	u32 bit = mt7530_gpio_to_bit(offset);
+
+	mt7530_clear(priv, MT7530_LED_GPIO_DIR, bit);
+	mt7530_clear(priv, MT7530_LED_GPIO_OE, bit);
+
+	return 0;
+}
+
+static int
+mt7530_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct mt7530_priv *priv = gpiochip_get_data(gc);
+	u32 bit = mt7530_gpio_to_bit(offset);
+
+	mt7530_set(priv, MT7530_LED_GPIO_DIR, bit);
+	mt7530_set(priv, MT7530_LED_GPIO_OE, bit);
+	mt7530_gpio_set(gc, offset, value);
+
+	return 0;
+}
+
+static int
+mt7530_setup_gpio(struct mt7530_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct gpio_chip *gc;
+
+	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
+	if (!gc)
+		return -ENOMEM;
+
+	mt7530_write(priv, MT7530_LED_IO_MODE, 0);
+
+	gc->label = "mt7530";
+	gc->parent = dev;
+	gc->owner = THIS_MODULE;
+	gc->get_direction = mt7530_gpio_get_direction;
+	gc->direction_input = mt7530_gpio_direction_input;
+	gc->direction_output = mt7530_gpio_direction_output;
+	gc->get = mt7530_gpio_get;
+	gc->set = mt7530_gpio_set;
+	gc->base = -1;
+	gc->ngpio = 15;
+	gc->can_sleep = true;
+
+	return devm_gpiochip_add_data(dev, gc, priv);
+}
+
 static int
 mt7530_setup(struct dsa_switch *ds)
 {
@@ -1781,6 +1871,12 @@ mt7530_setup(struct dsa_switch *ds)
 		}
 	}
 
+	if (of_property_read_bool(priv->dev->of_node, "gpio-controller")) {
+		ret = mt7530_setup_gpio(priv);
+		if (ret)
+			return ret;
+	}
+
 	mt7530_setup_port5(ds, interface);
 
 	/* Flush the FDB table */
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 32d8969b3ace..e7903ecc6a7c 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -554,6 +554,26 @@ enum mt7531_clk_skew {
 #define  MT7531_GPIO12_RG_RXD3_MASK	GENMASK(19, 16)
 #define  MT7531_EXT_P_MDIO_12		(2 << 16)
 
+/* Registers for LED GPIO control (MT7530 only)
+ * All registers follow this pattern:
+ * [2:0]    port 0
+ * [6:4]    port 1
+ * [10:8]   port 2
+ * [14:12]  port 3
+ * [18:16]  port 4
+ */
+
+/* LED enable, 0: Disable, 1: Enable (Default) */
+#define MT7530_LED_EN			0x7d00
+/* LED mode, 0: GPIO mode, 1: PHY mode (Default) */
+#define MT7530_LED_IO_MODE		0x7d04
+/* GPIO direction, 0: Input, 1: Output */
+#define MT7530_LED_GPIO_DIR		0x7d10
+/* GPIO output enable, 0: Disable, 1: Enable */
+#define MT7530_LED_GPIO_OE		0x7d14
+/* GPIO value, 0: Low, 1: High */
+#define MT7530_LED_GPIO_DATA		0x7d18
+
 #define MT7530_CREV			0x7ffc
 #define  CHIP_NAME_SHIFT		16
 #define  MT7530_ID			0x7530
-- 
2.25.1

Re: [PATCH net-next 2/2] drivers: net: dsa: mt7530: MT7530 optional GPIO support

[Russell King - ARM Linux admin]

On Mon, Jan 11, 2021 at 01:44:28PM +0800, DENG Qingfang wrote:
> +static int
> +mt7530_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> +	struct mt7530_priv *priv = gpiochip_get_data(gc);
> +	u32 bit = mt7530_gpio_to_bit(offset);
> +
> +	mt7530_set(priv, MT7530_LED_GPIO_DIR, bit);
> +	mt7530_set(priv, MT7530_LED_GPIO_OE, bit);
> +	mt7530_gpio_set(gc, offset, value);

FYI, Documentation/driver-api/gpio/consumer.rst says:

  For output GPIOs, the value provided becomes the initial output value.
  This helps avoid signal glitching during system startup.

Setting the pin to be an output, and then setting its initial value
does not avoid the glitch. You may wish to investigate whether you
can set the value before setting the pin as an output to avoid this
issue.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/2] drivers: net: dsa: mt7530: MT7530 optional GPIO support

[DENG Qingfang]

On Mon, Jan 11, 2021 at 7:04 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> FYI, Documentation/driver-api/gpio/consumer.rst says:
>
>   For output GPIOs, the value provided becomes the initial output value.
>   This helps avoid signal glitching during system startup.
>
> Setting the pin to be an output, and then setting its initial value
> does not avoid the glitch. You may wish to investigate whether you
> can set the value before setting the pin as an output to avoid this
> issue.
>

So, setting the Output Enable bit _after_ setting the direction and
initial value should avoid this issue. Right?

Re: [PATCH net-next 0/2] dsa: add MT7530 GPIO support

[Vladimir Oltean]

On Mon, Jan 11, 2021 at 01:44:26PM +0800, DENG Qingfang wrote:
> MT7530's LED controller can be used as GPIO controller. Add support for
> it.
> 
> DENG Qingfang (2):
>   dt-bindings: net: dsa: add MT7530 GPIO controller binding
>   drivers: net: dsa: mt7530: MT7530 optional GPIO support
> 
>  .../devicetree/bindings/net/dsa/mt7530.txt    |  6 ++
>  drivers/net/dsa/mt7530.c                      | 96 +++++++++++++++++++
>  drivers/net/dsa/mt7530.h                      | 20 ++++
>  3 files changed, 122 insertions(+)
> 
> -- 
> 2.25.1

Adding GPIO and LED maintainers to also have a look.
https://patchwork.kernel.org/project/netdevbpf/cover/20210111054428.3273-1-dqfext@gmail.com/

Re: [PATCH net-next 2/2] drivers: net: dsa: mt7530: MT7530 optional GPIO support

[Russell King - ARM Linux admin]

On Mon, Jan 11, 2021 at 09:40:00PM +0800, DENG Qingfang wrote:
> On Mon, Jan 11, 2021 at 7:04 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > FYI, Documentation/driver-api/gpio/consumer.rst says:
> >
> >   For output GPIOs, the value provided becomes the initial output value.
> >   This helps avoid signal glitching during system startup.
> >
> > Setting the pin to be an output, and then setting its initial value
> > does not avoid the glitch. You may wish to investigate whether you
> > can set the value before setting the pin as an output to avoid this
> > issue.
> >
> 
> So, setting the Output Enable bit _after_ setting the direction and
> initial value should avoid this issue. Right?

It depends on the hardware. I don't know how your hardware works, so
I can't say whether doing anything will result in correct behaviour,
or even work.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/2] dsa: add MT7530 GPIO support

[Marek Behún]

Qingfang,

what modes does the LED support? Does it support blinking on rx/tx?
What about link status?
I'd like to know because I am still working on patches which add
ethernet PHY/switch LEDs, with transparent offloading of netdev trigger.

Marek

On Mon, 11 Jan 2021 13:44:26 +0800
DENG Qingfang <dqfext@gmail.com> wrote:

> MT7530's LED controller can be used as GPIO controller. Add support for
> it.
> 
> DENG Qingfang (2):
>   dt-bindings: net: dsa: add MT7530 GPIO controller binding
>   drivers: net: dsa: mt7530: MT7530 optional GPIO support
> 
>  .../devicetree/bindings/net/dsa/mt7530.txt    |  6 ++
>  drivers/net/dsa/mt7530.c                      | 96 +++++++++++++++++++
>  drivers/net/dsa/mt7530.h                      | 20 ++++
>  3 files changed, 122 insertions(+)
> 

Re: [PATCH net-next 0/2] dsa: add MT7530 GPIO support

[DENG Qingfang]

Hi Marek,

On Mon, Jan 11, 2021 at 11:46 PM Marek Behún <kabel@kernel.org> wrote:
>
> what modes does the LED support? Does it support blinking on rx/tx?
> What about link status?

Yes. But unfortunately they cannot be controlled individually, unless
on GPIO mode.

> I'd like to know because I am still working on patches which add
> ethernet PHY/switch LEDs, with transparent offloading of netdev trigger.
>
> Marek

Re: [PATCH net-next 0/2] dsa: add MT7530 GPIO support

[Bartosz Golaszewski]

On Mon, Jan 11, 2021 at 2:43 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Jan 11, 2021 at 01:44:26PM +0800, DENG Qingfang wrote:
> > MT7530's LED controller can be used as GPIO controller. Add support for
> > it.
> >
> > DENG Qingfang (2):
> >   dt-bindings: net: dsa: add MT7530 GPIO controller binding
> >   drivers: net: dsa: mt7530: MT7530 optional GPIO support
> >
> >  .../devicetree/bindings/net/dsa/mt7530.txt    |  6 ++
> >  drivers/net/dsa/mt7530.c                      | 96 +++++++++++++++++++
> >  drivers/net/dsa/mt7530.h                      | 20 ++++
> >  3 files changed, 122 insertions(+)
> >
> > --
> > 2.25.1
>
> Adding GPIO and LED maintainers to also have a look.
> https://patchwork.kernel.org/project/netdevbpf/cover/20210111054428.3273-1-dqfext@gmail.com/

Can you resend the series with GPIO maintainers in CC?

Bart

Re: [PATCH net-next 0/2] dsa: add MT7530 GPIO support

[Florian Fainelli]

On 1/11/21 6:50 PM, DENG Qingfang wrote:
> Hi Marek,
> 
> On Mon, Jan 11, 2021 at 11:46 PM Marek Behún <kabel@kernel.org> wrote:
>>
>> what modes does the LED support? Does it support blinking on rx/tx?
>> What about link status?

Just to be crystal clear here, if you configure the LEDs to be in GPIO
mode, you can defer to the leds-gpio driver for all configuration, and
you can still offload blinking of the LEDs to the hardware or does
blinking require you to use a software managed timer?
-- 
Florian

Re: [PATCH net-next 1/2] dt-bindings: net: dsa: add MT7530 GPIO controller binding

[Rob Herring]

On Mon, 11 Jan 2021 13:44:27 +0800, DENG Qingfang wrote:
> Add device tree binding to support MT7530 GPIO controller.
> 
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/dsa/mt7530.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH net-next 1/2] dt-bindings: net: dsa: add MT7530 GPIO controller binding

[Linus Walleij]

On Mon, Jan 11, 2021 at 6:46 AM DENG Qingfang <dqfext@gmail.com> wrote:

> Add device tree binding to support MT7530 GPIO controller.
>
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>

This is a good way to turn it into a GPIO controller.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next 2/2] drivers: net: dsa: mt7530: MT7530 optional GPIO support

[Linus Walleij]

On Mon, Jan 11, 2021 at 6:46 AM DENG Qingfang <dqfext@gmail.com> wrote:

> MT7530's LED controller can drive up to 15 LED/GPIOs.
>
> Add support for GPIO control and allow users to use its GPIOs by
> setting gpio-controller property in device tree.
>
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>

Double-check the initial output conditions as indicated by
Russell, if you really want to be thorough, use an oscilloscope
but check the specs at least.

> +static u32
> +mt7530_gpio_to_bit(unsigned int offset)
> +{
> +       return BIT(offset + offset / 3);
> +}

So for offset 0..14 this becomes bits
0, 1, 2, 4, 5, 6, 8, 9, 10, 12  ... 18

What is the logic in this and is it what you intend?
Please add a comment explaining what the offset is supposed
to become for offsets 0..14 and why.

> +       gc->ngpio = 15;

And it really IS 15 not 16? Not that I know network equipment
very well...

Yours,
Linus Walleij

Re: [PATCH net-next 2/2] drivers: net: dsa: mt7530: MT7530 optional GPIO support

[DENG Qingfang]

Hi Linus,

On Mon, Jan 18, 2021 at 10:55 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> So for offset 0..14 this becomes bits
> 0, 1, 2, 4, 5, 6, 8, 9, 10, 12  ... 18
>
> What is the logic in this and is it what you intend?

Yes. Bit 0..2 are phy 0's LED 0..2, bit 4..6 are phy 1's LED 0..2, etc.

> Please add a comment explaining what the offset is supposed
> to become for offsets 0..14 and why.

I already added to mt7530.h, perhaps I should copy it here?

>
> > +       gc->ngpio = 15;
>
> And it really IS 15 not 16? Not that I know network equipment
> very well...

Yes, 3 LEDs for each phy.

>
> Yours,
> Linus Walleij

Re: [PATCH net-next 2/2] drivers: net: dsa: mt7530: MT7530 optional GPIO support

[Linus Walleij]

On Tue, Jan 19, 2021 at 4:20 AM DENG Qingfang <dqfext@gmail.com> wrote:
> On Mon, Jan 18, 2021 at 10:55 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > So for offset 0..14 this becomes bits
> > 0, 1, 2, 4, 5, 6, 8, 9, 10, 12  ... 18
> >
> > What is the logic in this and is it what you intend?
>
> Yes. Bit 0..2 are phy 0's LED 0..2, bit 4..6 are phy 1's LED 0..2, etc.

OK add a comment and explain how the bits relate
to each PHY and how the lines are arranged per-phy
so it is crystal clear for people reading the driver.

Thanks!
Linus Walleij