Re: [PATCH v1 01/11] dt-bindings: display/bridge: Add bindings for ti948

[Michael Drake]

Hi Laurent,

On 11/06/2019 19:03, Laurent Pinchart wrote:
> Hi Michael,
> 
> Thank you for the patch.

My pleasure, and thank you for the feedback!  I'm sorry it's
taken me a while to respond to it.

> On Tue, Jun 11, 2019 at 03:04:02PM +0100, Michael Drake wrote:
>> Adds device tree bindings for:
>>
>>   TI DS90UB948-Q1 2K FPD-Link III to OpenLDI Deserializer
>>
>> The device has the compatible string "ti,ds90ub948", and
>> and allows an arrray of strings to be provided as regulator
> 
> s/arrray/array/

Thanks.  Fixed for the next version.

>> names to enable for operation of the device.
>>
>> Signed-off-by: Michael Drake <michael.drake@xxxxxxxxxxxxxxx>
>> Cc: Patrick Glaser <pglaser@xxxxxxxxx>
>> Cc: Nate Case <ncase@xxxxxxxxx>
>> ---
>>  .../bindings/display/bridge/ti,ds90ub948.txt  | 24 +++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90ub948.txt
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ds90ub948.txt b/Documentation/devicetree/bindings/display/bridge/ti,ds90ub948.txt
>> new file mode 100644
>> index 000000000000..f9e86cb22900
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,ds90ub948.txt
>> @@ -0,0 +1,24 @@
>> +TI DS90UB948-Q1 2K FPD-Link III to OpenLDI Deserializer
>> +=======================================================
>> +
>> +This is the binding for Texas Instruments DS90UB948-Q1 bridge deserializer.
>> +
>> +This device supports I2C only.
> 
> Then the DT node should be a child of its I2C controller, and have a reg
> property.

OK, thank you.  I'm not actually using DT, I'm using ACPI tables.

For DT, would this cause the device to get bound to the driver on
startup?

I tried adding a reg property with the value set to the device's
I2C address with device tree compatible properties, coupled with
the DT "compatible" string property, but that was insufficient to
bind the device to the driver.  At the moment, I'm using the ACPI
mechanism for achieving this.

So all I've done for this is add the "reg" property to the device
tree ti,ds90ub948.txt documentation file, in the Example section.

Is this what you were expecting?

For the other point, that the DT node should be a child of its I2C
controller, I've made the devices descendants of the I2C controller
in ACPI, but one of them is not a direct child.

The ti949 is a child of its I2C controller node, but the ti948
is a child of the ti949 node in the ACPI table.  I did this to
ensure that linux was aware of the dependency of the devices, so
that they would be brought up in the correct order and suspended
and resumed in the correct order.

>> +
>> +Required properties:
>> +
>> +- compatible: "ti,ds90ub948"
>> +
>> +Optional properties:
>> +
>> +- regulators: List of regulator name strings to enable for operation of device.
> 
> There's a standard binding for regulators, using *-supply properties.
> Please use them.

Thank you!  I've switched to using that, and it is less code.  :)

> You should also use the OF graph DT bindings to model the data
> connections.

I'm not clear on the implications of this from the driver code.
Does it need any code changes to the driver to support these
endpoints?  How are they used?

If it's just a documentation thing, I can easily do it, but
otherwise, I can't test Device Tree because we're using ACPI.

>> +
>> +Example
>> +-------
>> +
>> +ti948: ds90ub948@0 {
>> +	compatible = "ti,ds90ub948";
>> +
>> +	regulators: "vcc",
>> +	            "vcc_disp";
>> +};
> 

-- 
Michael Drake                 https://www.codethink.co.uk/