devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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: Mon, 5 Oct 2020 10:21:02 +0200	[thread overview]
Message-ID: <87lfglxevl.fsf@soft-dev15.microsemi.net> (raw)
In-Reply-To: <CACRpkdYxK6Uf1_3Me7hbJZ+rPAUXCj4k7D2e5je7iBNZosEtQw@mail.gmail.com>


Linus Walleij writes:

> Hi Lars,
>
> thanks for working on this!
>
> On Sun, Sep 13, 2020 at 9:11 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>
>> > 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.
>
> OK fair enough let's keep the bindings here.
>
> BTW the latter function sounds like some kind of PWM?

Yes, it has PWM functionality as well.

>
>> >> +  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.
>
> I'm a bit confused here...
> The standard advice for any "banked" GPIOs is to represent
> each "bank" as a separate node (with a corresponding gpio_chip
> in the Linux kernel). Then you can just use the standard
> bindings to pick a line from one of these nodes.

Yes, that seems to be a good model.

>
>> 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.
>
> Hmmm. What makes these names expecially confusing is the
> Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml defines:
> input-enable
> input-disable
> output-enable
> output-high
> output-low
>
> In the Linux kernel further there is:
> include/linux/pinctrl/pinconf-generic.h that defines:
> PIN_CONFIG_INPUT_ENABLE
> PIN_CONFIG_OUTPUT_ENABLE
> PIN_CONFIG_OUTPUT
>
> Since you are using the pin control framework this gets really
> hard to hash out.
>

Yes, as the pins are fixed-function, the "input-enable", "input-disable"
and "output-enable" are not really useful.

> I don't really understand why it is needed.
>
>> 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";
>>                 };
>>                 ...
>>         };
>> };
>
> If what you intend to achieve is to make the GPIO come up in output mode,
> you can either just have the driver do that as needed by the consumer.
> If you absolutely have to do it in the device tree, then implement
> pin control (pin config) and have it something like this:
>
> leds {
>         compatible = "gpio-leds";
>         pinctrl-names = "default";
>         pinctrl-0 = <&my_led_pinctrl>;
>         led@0 {
>                 label = "eth60:yellow";
>                 gpios = <&sgpio1 28 GPIO_ACTIVE_LOW>;
>                 default-state = "off";
>         };
>         ...
>
>         my_led_pinctrl: pinctrl-led {
>                 pins = "gpio95"; // Just an example way of referring to the pin
>                 bias-disable;
>                 output-enable;
>         };
> };

No, the PIN_OUTPUT is purely for adressing. But as you suggested, I'll
split the into separate nodes. That will eliminate the "PIN_OUTPUT" and
the bindings header.

>
>> >> +  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.
>
> And you have considered the option of simply letting the driver
> check which board we are then? The property at the very
> top of the device tree.
>
> if (of_machine_is_compatible("my_board")) {
>     ....
> } else if (of_machine_is_compatible("my_other_board")) {
>     ....
> }

No, board-specific code is undesireable, as our customers should be able
to design own boards without driver changes.

>
> So that you simply use the board compatible string to determine
> this?
>
>> >> +/* 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.
>
> I suspect the proper way to do it is to create one node for
> the input side and one node for the output side and also create
> two different gpio chips in the kernel.
>
> my-device {
>        compatible = "my-device";
>        gpioin: input-gpio {
>            ....
>        };
>        gpioout: output-gpio {
>            ....
>        };
> };
>
> Note: I didn't think over the naming in this example.
>
> You will need code in your driver to parse the subnodes and
> populate two gpio_chips.

Yes, I will modify the driver to use separate nodes for each direction.

Thank you for your comments, it is highly appreciated.

---Lars

>
> Yours,
> Linus Walleij

-- 
Lars Povlsen,
Microchip

  reply	other threads:[~2020-10-05  8:21 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
2020-09-30  9:21       ` Linus Walleij
2020-10-05  8:21         ` Lars Povlsen [this message]
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=87lfglxevl.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 \
    --subject='Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver' \
    /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

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).