Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver

[Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx> 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