devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: Lars Povlsen <lars.povlsen@microchip.com>
Cc: 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: Sat, 12 Sep 2020 12:50:40 +0200	[thread overview]
Message-ID: <CACRpkdZUQG1T_Bx5G275tSjDez0skDKGSc370B57FZ35NA6iEA@mail.gmail.com> (raw)
In-Reply-To: <20200903133528.8595-2-lars.povlsen@microchip.com>

Hi Lars,

thanks for your patch!

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?

> +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?

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

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

> +  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?

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

Yours,
Linus Walleij

  reply	other threads:[~2020-09-12 10:51 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 [this message]
2020-09-13 19:11     ` Lars Povlsen
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=CACRpkdZUQG1T_Bx5G275tSjDez0skDKGSc370B57FZ35NA6iEA@mail.gmail.com \
    --to=linus.walleij@linaro.org \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=devicetree@vger.kernel.org \
    --cc=lars.povlsen@microchip.com \
    --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).