Re: [PATCH v5 5/8] dt-bindings: media: add bindings for TI DS90UB960

Laurent Pinchart
2023-01-04T12:58:17Z

Hi Tomi,

On Wed, Jan 04, 2023 at 10:59:00AM +0200, Tomi Valkeinen wrote:
> On 26/12/2022 18:52, Laurent Pinchart wrote:
> > On Tue, Dec 13, 2022 at 04:25:46PM +0200, Tomi Valkeinen wrote:
> >> On 11/12/2022 19:58, Laurent Pinchart wrote:
> >>> On Thu, Dec 08, 2022 at 12:40:03PM +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 | 358 ++++++++++++++++++
> >>>> 1 file changed, 358 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..d8b5e219d420
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >>>> @@ -0,0 +1,358 @@
> >>>> +# 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:
> >>>> + 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
> >>>
> >>> s/i2c/I2C/
> >>>
> >>> Same below.
> >>>
> >>> A bit more details would be nice, for instance the order in which
> >>> addresses should be specified should be documented. The example below
> >>> has one address only, so it's quite unclear. Or is this a left-over,
> >>> from before the i2c-alias-pool ?
> >>
> >> That's a left over, but not related to i2c-alias-pool but the i2c-alias
> >> for the serializers. It already says above 'maxItems: 1', so now it only
> >> contains the deserializer address. I'll drop the desc.
> >
> > Looks good to me.
> >
> >>>> +
> >>>> + 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:
> >>>> + $ref: /schemas/types.yaml#/definitions/uint16-array
> >>>> + description:
> >>>> + i2c alias pool is a pool of i2c addresses on the main i2c bus that can be
> >>>> + used to access the remote peripherals. The addresses must be available,
> >>>> + not used by any other peripheral. 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.
> >>>
> >>> As this property is optional, should you describe what happens when it's
> >>> not specified ?
> >>>
> >>> I would also indicate that the pool doesn't cover the serializers, only
> >>> the devices behind them.
> >>
> >> Yep, I'll clarify these.
> >>
> >>>> +
> >>>> + links:
> >>>> + type: object
> >>>> + additionalProperties: false
> >>>> +
> >>>> + properties:
> >>>> + '#address-cells':
> >>>> + const: 1
> >>>> +
> >>>> + '#size-cells':
> >>>> + const: 0
> >>>> +
> >>>> + ti,manual-strobe:
> >>>> + type: boolean
> >>>> + description:
> >>>> + Enable manual strobe position and EQ level
> >>>> +
> >>>> + patternProperties:
> >>>> + '^link@[0-9a-f]+$':
> >>>
> >>> There can be up to 4 links only, right ? I would then use
> >>>
> >>> '^link@[0-3]$':
> >>
> >> Yes, I'll change that.
> >>
> >>>> + type: object
> >>>> + additionalProperties: false
> >>>> + properties:
> >>>> + reg:
> >>>> + description: The link number
> >>>> + maxItems: 1
> >>>> +
> >>>> + i2c-alias:
> >>>> + description:
> >>>> + The i2c address used for the serializer. Transactions to this
> >>>> + address on the i2c bus where the deserializer resides are
> >>>> + forwarded to the serializer.
> >>>> +
> >>>> + ti,rx-mode:
> >>>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>>> + enum:
> >>>> + - 0 # RAW10
> >>>> + - 1 # RAW12 HF
> >>>> + - 2 # RAW12 LF
> >>>> + - 3 # CSI2 SYNC
> >>>> + - 4 # CSI2 NON-SYNC
> >>>> + description: FPD-Link Input Mode
> >>>
> >>> Are there use cases for controlling this dynamically (in particular the
> >>> sync/non-sync modes) ? Is there anything that could be queried at
> >>> runtime from the serializers instead of being specified in DT ?
> >>
> >> We need a link to the serializer before we can query anything from the
> >> serializer.
> >
> > I meant querying it from the serializer driver, not the serializer
> > hardware. This being said, it would likely be difficult to do so, as the
> > serializer driver would need to probe first. I think I'm thus fine
> > selecting the mode in DT on the deserializer side.
> >
> >> To have a link, we need the mode... So, as I mentioned in
> >> the other reply, we could define these in some way in the serializer's
> >> properties instead of here, but I'm not sure if that's a good change.
> >>
> >> The driver can change the mode at runtime (say, from sync to non-sync
> >> mode, if the HW supports that). But I think this property should reflect
> >> the HW strapped configuration of the serializer.
> >
> > That would possibly work for the DS90UB953, but the DS90UB913 has no
> > strapped mode selected at boot time but is instead configured
> > automatically through the back-channel (see my last reply to patch 3/8).
>
> Indeed.
>
> > When connecting a DS90UB913 to a DS90UB914 deserializer, we can probably
> > start without mode selection in software, as the MODE pin is meant to
> > bootstrap that to a correct value which is then automatically
> > transmitted to the serializer (hardware designs where the mode would
> > need to be overridden should be rate). However, when connecting multiple
>
> I don't know if that's true. I guess it depends on how you see the deser
> and the camera module. Are they part of the same HW design or not? In my
> setups they are quite separate, and I connect different kinds of camera
> modules to my deserializers. But I can see that if you create a, say,
> car, you'd have both sides known at design time and would never change.
>
> > DS90UB913 to a DS90UB960, I can imagine connecting different types of
> > cameras on the four input ports, so the need to specify the mode
> > per-port in DT would be more common.
>
> Right, and even with UB914, you might well design the deserializer side
> with, say, RAW10 sensors, but later in the cycle you'd need to change to
> a RAW12 sensor. Depending on the deser mode strap would require you to
> do a HW change on the deser side too.
>
> As I said in the other mail, I don't like the deser's strap, and I think
> we should just basically ignore it as we can provide the necessary data
> in the DT.

What I meant is that, given that the UB914 is meant to be used with a
single camera, using a RAW mode, there's a much higher chance that
hardware strapping will work as intended there. We could thus start
without support for overrides in a UB914 driver (but as far as I
understand we're not planning to work on such a driver in the near
future, so it's hypothetical only), while in the UB960 driver we
probably need override support from the beginning.

> > For these reasons, I don't think the ti,rx-mode property can be defined
> > as reflecting the hardware MODE strap with the DS90UB913. I also think
> > it would be quite confusing to define it as the desired runtime
> > configuration for the DS90UB913 and as the hardware MODE strap for the
> > DS90UB953. Could it be (explicitly) defined as the desired runtime
> > configuration in all cases ?
>
> That sounds bad in a DT context =). You're right that the rx-mode can't
> be defined as reflecting the serializer mode strap, but I think we can
> define it as reflecting the default operation mode of the serializer
> hardware (or maybe rather the camera module).

What do you mean by "default operation mode" in this case ?

--
Regards,

Laurent Pinchart