Re: [PATCH 1/3] dt-bindings: gpio: add Axiado SGPIO controller

[Petar Stepanovic]

Hi Linus,

thank you for the detailed review. I will go through the comments step by step.
Replies inline below.

On 4/24/2026 9:26 AM, Linus Walleij wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> Hi Petar,
>
> thanks for your patch!
>
> On Tue, Apr 14, 2026 at 3:49 PM Petar Stepanovic <pstepanovic@xxxxxxxxxx> wrote:
>
>> Add device tree binding for the Axiado SGPIO controller.
>>
>> The SGPIO controller provides a serialized interface for
>> controlling multiple GPIO signals over a limited number of
>> physical lines. It supports configurable data direction and
>> interrupt handling.
>>
>> The binding describes the properties required to instantiate
>> the controller and register it as a GPIO provider.
>>
>> Signed-off-by: Petar Stepanovic <pstepanovic@xxxxxxxxxx>
> (...)
>
>> +description: |
>> +  The SGPIO controller provides a serialized interface for controlling
>> +  multiple GPIO signals over a limited number of physical lines.
>> +  It supports configurable data direction and interrupt handling.
> This is pretty generic, can you write some details on how this happens?

Yes, I will improve the description and add details about the SGPIO
operation and data flow.

>
>> +  '#gpio-cells':
>> +    const: 2
> Are you sure you don't want to use 3 here instead and split the 128
> GPIOs into 4 "banks" second cell being the bank number?
> <&gpio 2 4>; ?
>
> Maybe this also solves the 512 GPIO by grouping the GPIOs into
> 8 banks...?

Thank you for the suggestion. We would prefer to keep #gpio-cells = <2> to stay aligned with existing SGPIO drivers and current DTS usage.
A single linear offset is sufficient to identify each GPIO, so introducing a bank cell would add additional complexity without a clear benefit.
Any internal bank handling can remain within the driver if needed.

>> +  '#interrupt-cells':
>> +    const: 2
> Same there.
>
>> +  design-variant:
>> +    description: SGPIO design variant size in bits (e.g. 128 or 512).
>> +    enum: [128, 512]
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> Just use two different compatible strings and infer the variant from
> that string instead.
>
>> +  ngpios:
>> +    description: The number of gpios this controller has.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> Same here, certainly the 128 variant has 128 gpios and
> the 512 has 512 GPIOs? Just use the compatible string
> to infer this.

This seems to be platform-specific rather than strictly hardware-dependent.
We were considering keeping it as a separate property (possibly renamed to |axiado,sgpio-ngpios|).
Would you prefer that, or deriving it from the compatible string?

>> +  bus-frequency:
>> +    description: The SGPIO shift clock frequency in Hz.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> Don't you want to use the clock bindings and a clk property
> for this?

Yes, I will rework this to use the standard clock binding instead of a custom
bus-frequency propert

>> +  apb-frequency:
>> +    description: The APB bus frequency in Hz.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> Dito.
>
>> +  dout-init:
>> +    description: Initial values for the dout registers.
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    minItems: 4
>> +    maxItems: 4
> In:
> Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml
>
> you find:
>
>   lines-initial-states:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     description:
>       Bitmask that specifies the initial state of each line.
>       When a bit is set to zero, the corresponding line will be initialized to
>       the input (pulled-up) state.
>       When the  bit is set to one, the line will be initialized to the
>       low-level output state.
>       If the property is not specified all lines will be initialized to the
>       input state.
>
> If this is what you want, use this standard binding instead.

In our case, the hardware provides dedicated DOUT registers where
each bit directly controls the output level (0 = low, 1 = high).

The lines-initial-states property also encodes input state semantics,
so it does not map directly to this hardware.

Would you prefer adapting to lines-initial-states despite this,
or using a separate property for output initialization?

>
> Yours,
> Linus Walleij

Thanks again for the guidance.

Best regards,
Petar Stepanovic