| Message ID | 20210111054428.3273-1-dqfext@gmail.com |
|---|---|
| Headers | show |
| Series | dsa: add MT7530 GPIO support | expand |
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 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/
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.
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(+) >
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
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
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?
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