| Message ID | 20210111054428.3273-3-dqfext@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dsa: add MT7530 GPIO support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 82 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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.
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?
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.
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
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
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
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
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(+)