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@xxxxxxxxxxxxxxxx> > >>>> --- > >>>> .../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@xxxxxxxxxxxxxxxx> > >>>> + > >>>> +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
