lkml.org 
[lkml]   [2020]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver
    Date

    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

    \
     
     \ /
      Last update: 2020-10-05 10:21    [W:3.327 / U:1.436 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site