Re: [PATCH v4 3/8] dt-bindings: media: add bindings for TI DS90UB960

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hi Tomi,

On Thu, 3 Nov 2022 13:50:43 +0200
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:

> Hi Rob,
> 
> On 02/11/2022 19:26, Rob Herring wrote:
> > On Tue, Nov 01, 2022 at 03:20:27PM +0200, Tomi Valkeinen wrote:  
> >> Add DT bindings for TI DS90UB960 FPDLink-3 Deserializer.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >>   .../bindings/media/i2c/ti,ds90ub960.yaml      | 392 ++++++++++++++++++
> >>   1 file changed, 392 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >> new file mode 100644
> >> index 000000000000..4456d9b3e2c7
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >> @@ -0,0 +1,392 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub960.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Texas Instruments DS90UB9XX Family FPD-Link Deserializer Hubs
> >> +
> >> +maintainers:
> >> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> +
> >> +description: |  
> > 
> > Don't need '|'  
> 
> Hmm, ok... But why does that work? I can only find yaml examples for 
> multi-line with either | or >.
> 
> >> +  The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO
> >> +  forwarding.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - ti,ds90ub960-q1
> >> +      - ti,ds90ub9702-q1
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +    description:
> >> +      i2c addresses for the deserializer and the serializers
> >> +
> >> +  reg-names:
> >> +    items:
> >> +      - const: main  
> > 
> > 'reg-names' is not all that useful with only 1 entry.  
> 
> True.
> 
> >> +
> >> +  clocks:
> >> +    maxItems: 1
> >> +    description:
> >> +      Reference clock connected to the REFCLK pin.
> >> +
> >> +  clock-names:
> >> +    items:
> >> +      - const: refclk
> >> +
> >> +  powerdown-gpios:
> >> +    maxItems: 1
> >> +    description:
> >> +      Specifier for the GPIO connected to the PDB pin.
> >> +
> >> +  i2c-alias-pool:  
> > 
> > Something common or could be? If not, then needs a vendor prefix.  
> 
> I'll have to think about this. It is related to the i2c-atr, so I think 
> it might be a common thing.
> 
> >> +    $ref: /schemas/types.yaml#/definitions/uint16-array
> >> +    description:
> >> +      i2c alias pool for remote devices.  
> > 
> > Needs a better description. What's an 'alias pool'?  
> 
> Right.
> 
> "i2c alias pool is a pool of i2c addresses on the main i2c bus that can 
> be used to access the remote peripherals. Each remote peripheral is 
> assigned an alias from the pool, and transactions to that address will 
> be forwarded to the remote peripheral, with the address translated to 
> the remote peripheral's real address."

Good description, but I think re-adding this sentence from my v2 would
be useful:

  list of I2C addresses that are known to be available on the"local"
  (SoC-to-deser) I2C bus

> > 0-0xffff are valid values?  
> 
> They are i2c addresses, and linux i2c uses u16 for addresses. Then 
> again, the fpdlink devices only support 7-bit addresses, so maybe this 
> could be an uint8 array. I am not sure what's the best way to define this.

In DT the Linux implementation is irrelevant. Also if we want the ATR
code to be generic we must prepare DT bindings for future devices.
Thus, being these I2C addresses, they could be 7 or 10 bits, thus the
need for at least 16 bits.

All the above will become irrelevant in case the alias pool will
disappear, though.

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com