RE: [PATCH v6 1/4] dt-bindings: media: ti,ds90ub953: Add support for remote GPIO data source

[G.N. Zhou (OSS)]

Hi Conor,

Thanks for your review.

> -----Original Message-----
> From: Conor Dooley <conor@xxxxxxxxxx>
> Sent: Saturday, April 25, 2026 1:09 AM
> To: G.N. Zhou (OSS) <guoniu.zhou@xxxxxxxxxxx>
> Cc: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>; Mauro Carvalho
> Chehab <mchehab@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof
> Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Frank
> Li <frank.li@xxxxxxx>; Vladimir Zapolskiy <vz@xxxxxxxxx>; Linus Walleij
> <linusw@xxxxxxxxxx>; Bartosz Golaszewski <brgl@xxxxxxxxxx>; linux-
> media@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; G.N.
> Zhou <guoniu.zhou@xxxxxxx>
> Subject: Re: [PATCH v6 1/4] dt-bindings: media: ti,ds90ub953: Add support for
> remote GPIO data source
> 
> On Fri, Apr 24, 2026 at 09:42:24AM +0800, Guoniu Zhou wrote:
> > From: Guoniu Zhou <guoniu.zhou@xxxxxxx>
> >
> > The DS90UB953 supports four pins, GPIO0 through GPIO3. When enabled as
> > an output, it can be programed to output local data or remote data
> > coming from the remote compatible deserializer.
> >
> > Add GPIO flag in second cell to select remote GPIO data source.
> >
> > Signed-off-by: Guoniu Zhou <guoniu.zhou@xxxxxxx>
> > ---
> > Changes in v6:
> > - Added GPIO_DATA_SOURCE_REMOTE flag to dt-bindings/gpio/gpio.h
> > - Updated dt-bindings documentation accordingly
> > - Dropped Reviewed-by tag from Rob Herring due to significant binding
> > change
> >
> > Changes in v5:
> > - Improve the description for "#gpio-cells" as commented by Conor.
> >
> > Changes in v4:
> > - Use folder block instead of literal block for #gpio-cell property description.
> >
> > Changes in v3:
> > - Make GPIO range from 0-3 to 0-7 to support GPIO data from remote
> >   compatible deserializer suggested by Rob instead of adding third
> >   cell for GPIO controller.
> >
> > Changes in v2:
> > - Remove new property ti,gpio-data
> > - Add third cell for GPIO controller to select GPIO output source.
> > ---
> >  Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml | 6 ++++-
> -
> >  include/dt-bindings/gpio/gpio.h                               | 8 ++++++++
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml
> > b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml
> > index 2e129bf573b7..da63771bc236 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml
> > @@ -21,8 +21,10 @@ properties:
> >    '#gpio-cells':
> >      const: 2
> >      description:
> > -      First cell is the GPIO pin number, second cell is the flags. The GPIO pin
> > -      number must be in range of [0, 3].
> > +      First cell is the GPIO pin number (0-3) and the second cell is used
> > +      to specify flags. See <dt-bindings/gpio/gpio.h> for available flags
> > +      including GPIO_DATA_SOURCE_REMOTE for remote GPIO data source.
> > +      Flags can be OR'd together.
> >
> >    gpio-controller: true
> >
> > diff --git a/include/dt-bindings/gpio/gpio.h
> > b/include/dt-bindings/gpio/gpio.h index b5d531237448..d04a494d96ad
> > 100644
> > --- a/include/dt-bindings/gpio/gpio.h
> > +++ b/include/dt-bindings/gpio/gpio.h
> > @@ -42,4 +42,12 @@
> >  /* Bit 6 express pull disable */
> >  #define GPIO_PULL_DISABLE 64
> >
> > +/*
> > + * Bit 24 indicates the GPIO data source is from a remote device.
> 
> Why 24, not 7?

Good question. I chose bit 24 for the following reasons:
- There is no clear guidance in the documentation on how to add custom
   GPIO flags, so I wanted to ensure sufficient separation from the generic
   GPIO flags to avoid potential conflicts.
- The generic GPIO flags currently use the lower bits (0-6). By using
   bit 24, I'm leaving ample room (bits 7-23) for future expansion of
   generic flags without risking collision.
- This also reserves space (bits 25-31) for potential future custom
   flags that may be needed by this or other drivers.

However, if there's a preferred convention or range for driver-specific
GPIO flags that I'm not aware of, I'm happy to adjust this.

> 
> > + * This is used in serializer/deserializer setups where the GPIO pin
> > + * on the local device (e.g., TI DS90UB953 serializer) reflects the
> > + * state of a GPIO on the remote device (e.g., TI DS90UB960 deserializer).
> > + */
> > +#define GPIO_DATA_SOURCE_REMOTE		0x01000000
> 
> And why the divergent formatting compared to other defines in this file?

You're right about the formatting inconsistency.

The existing decimal notation (8, 16, 32, 64) works well for lower bits, but
16777216 (decimal for bit 24) is much less readable than 0x01000000. It's
also harder to verify correctness and more error-prone.

If consistency with the existing style is preferred, I can use decimal, but
I'd suggest considering updating all these defines to use BIT() hex for better
Readability and maintainability. What's your preference?

> 
> > +
> >  #endif
> >
> > --
> > 2.34.1
> >