From: Lars Povlsen <lars.povlsen@microchip.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Lars Povlsen <lars.povlsen@microchip.com>,
Rob Herring <robh+dt@kernel.org>,
Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Alexandre Belloni <alexandre.belloni@bootlin.com>
Subject: Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver
Date: Sun, 13 Sep 2020 21:11:48 +0200 [thread overview]
Message-ID: <87r1r5wky3.fsf@soft-dev15.microsemi.net> (raw)
In-Reply-To: <CACRpkdZUQG1T_Bx5G275tSjDez0skDKGSc370B57FZ35NA6iEA@mail.gmail.com>
Linus Walleij writes:
> Hi Lars,
>
> thanks for your patch!
You're welcome - thank you for you taking time to review it!
>
> On Thu, Sep 3, 2020 at 3:35 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>
>> This adds DT bindings for the Microsemi/Microchip SGPIO controller,
>
> What I do not understand is why this GPIO controller is placed in the
> bindings of the pin controllers? Do you plan to add pin control
> properties to the bindings in the future?
I have made provisions for some of the generic pinconf parameters, and
since the controller also has support for some alternate modes like
(syncronized) blink at various rates, I thought I better add it as
pinctrl straight away.
>
>> +description: |
>> + By using a serial interface, the SIO controller significantly extend
>> + the number of available GPIOs with a minimum number of additional
>> + pins on the device. The primary purpose of the SIO controllers is to
>> + connect control signals from SFP modules and to act as an LED
>> + controller.
>
> This doesn't sound like it will ever be pin control?
above.
>
>> + gpio-controller: true
>> +
>> + '#gpio-cells':
>> + description: GPIO consumers must specify four arguments, first the
>> + port number, then the bit number, then a input/output flag and
>> + finally the GPIO flags (from include/dt-bindings/gpio/gpio.h).
>> + The dt-bindings/gpio/mchp-sgpio.h file define manifest constants
>> + PIN_INPUT and PIN_OUTPUT.
>> + const: 4
>
> I do not follow this new third input/output flag at all.
>
Its actually a sort of bank address, since the individual "pins" are
unidirectional.
The PIN_INPUT/PIN_OUTPUT is defined in similar fashion in other pinctrl
binding header files... I can drop the define and use, but as it will be
used to address individual pins, I think it adds to readability.
Like this (excerpts from a DT with a switchdev driver using SFP's and
LED's on sgpio):
/{
leds {
compatible = "gpio-leds";
led@0 {
label = "eth60:yellow";
gpios = <&sgpio1 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>;
default-state = "off";
};
...
};
};
&axi {
sfp_eth60: sfp-eth60 {
compatible = "sff,sfp";
i2c-bus = <&i2c152>;
tx-disable-gpios = <&sgpio2 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>;
rate-select0-gpios = <&sgpio2 28 1 PIN_OUTPUT GPIO_ACTIVE_HIGH>;
los-gpios = <&sgpio2 28 0 PIN_INPUT GPIO_ACTIVE_HIGH>;
mod-def0-gpios = <&sgpio2 28 1 PIN_INPUT GPIO_ACTIVE_LOW>;
tx-fault-gpios = <&sgpio2 28 2 PIN_INPUT GPIO_ACTIVE_HIGH>;
};
...
};
> - If it is a property of the hardware, it is something the driver should
> handle by determining which hardware it is from the compatible
> string.
>
> - If it is a configuration it should be turned into something that is generic
> and useful for *all* GPIO controllers. If it is pin config it should use
> the pinconf bindings rather than shortcuts like this, but I think it is
> something the driver can do as an effect of the pin being requested
> as input or output in the operating system, depending on who the
> consumer is. Linux for example has GPIOD_OUT_LOW,
> GPIOD_OUT_HIGH, GPIOD_IN, GPIOD_ASIS...
>
> - Is it not just a hog? We have bindings for those.
I hope the above shed some light on this.
>
>> + microchip,sgpio-port-ranges:
>> + description: This is a sequence of tuples, defining intervals of
>> + enabled ports in the serial input stream. The enabled ports must
>> + match the hardware configuration in order for signals to be
>> + properly written/read to/from the controller holding
>> + registers. Being tuples, then number of arguments must be
>> + even. The tuples mast be ordered (low, high) and are
>> + inclusive. Arguments must be between 0 and 31.
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + minItems: 2
>> + maxItems: 64
>
> And you are *absolutely sure* that you can't just figure this out
> from the compatible string? Or add a few compatible strings for
> the existing variants?
>
Yes, this really needs to be configured for each board individually -
and cant be probed. It defines how the bitstream to/from the shift
registers is constructed/demuxed.
>> + microchip,sgpio-frequency:
>> + description: The sgpio controller frequency (Hz). This dictates
>> + the serial bitstream speed, which again affects the latency in
>> + getting control signals back and forth between external shift
>> + registers. The speed must be no larger than half the system
>> + clock, and larger than zero.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + minimum: 1
>> + default: 12500000
>
> I understand why you need this binding now, OK.
>
>> +/* mchp-sgpio specific pin type defines */
>> +#undef PIN_OUTPUT
>> +#undef PIN_INPUT
>> +#define PIN_OUTPUT 0
>> +#define PIN_INPUT 1
>
> I'm not a fan of this. It seems like something that should be set in
> response to the gpiochip callbacks .direction_input and
> .direction_output callbacks.
>
As I tried to explain above, its a part of the pin address - aka bank
selector - whether your are accessing the input or the output side. And
since the directions have totally different - and concurrent - use, they
need to be individually addressed, not "configured".
In the example presented, sgpio2-p28b0 IN is loss-of-signal, and the
OUT is the sfp tx-disable control.
> Yours,
> Linus Walleij
---Lars
--
Lars Povlsen,
Microchip
next prev parent reply other threads:[~2020-09-13 19:11 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-03 13:35 [PATCH v2 0/3] pinctrl: Adding support for Microchip/Microsemi serial GPIO controller Lars Povlsen
2020-09-03 13:35 ` [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver Lars Povlsen
2020-09-12 10:50 ` Linus Walleij
2020-09-13 19:11 ` Lars Povlsen [this message]
2020-09-30 9:21 ` Linus Walleij
2020-10-05 8:21 ` Lars Povlsen
2020-09-03 13:35 ` [PATCH v2 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO Lars Povlsen
2020-09-12 11:11 ` Linus Walleij
2020-09-13 19:28 ` Lars Povlsen
2020-09-03 13:35 ` [PATCH v2 3/3] arm64: dts: sparx5: Add SGPIO devices Lars Povlsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r1r5wky3.fsf@soft-dev15.microsemi.net \
--to=lars.povlsen@microchip.com \
--cc=UNGLinuxDriver@microchip.com \
--cc=alexandre.belloni@bootlin.com \
--cc=devicetree@vger.kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).