Message-ID: <1569908.lXeeGn9YFn@avalon>
Date:   Tue, 16 Oct 2018 15:30:44 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>
Cc:     Vladimir Zapolskiy <vz@...ia.com>,
        Lee Jones <lee.jones@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Marek Vasut <marek.vasut@...il.com>,
        Wolfram Sang <wsa@...-dreams.de>, devicetree@...r.kernel.org,
        linux-gpio@...r.kernel.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Sandeep Jain <Sandeep_Jain@...tor.com>
Subject: Re: [PATCH 1/7] dt-bindings: mfd: ds90ux9xx: add description of TI DS90Ux9xx ICs

Hi Vladimir,

On Saturday, 13 October 2018 17:28:30 EEST Vladimir Zapolskiy wrote:
> On 10/12/2018 02:44 PM, Laurent Pinchart wrote:
> > On Tuesday, 9 October 2018 00:11:59 EEST Vladimir Zapolskiy wrote:
> >> From: Sandeep Jain <Sandeep_Jain@...tor.com>
> >> 
> >> The change adds device tree binding description of TI DS90Ux9xx
> >> series of serializer and deserializer controllers which support video,
> >> audio and control data transmission over FPD-III Link connection.
> >> 
> >> Signed-off-by: Sandeep Jain <Sandeep_Jain@...tor.com>
> >> [vzapolskiy: various updates and corrections of secondary importance]
> >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>
> >> ---
> >> 
> >>  .../devicetree/bindings/mfd/ti,ds90ux9xx.txt  | 66 +++++++++++++++++++
> >>  1 file changed, 66 insertions(+)
> >>  create mode 100644
> >>  Documentation/devicetree/bindings/mfd/ti,ds90ux9xx.txt
> >> 
> >> diff --git a/Documentation/devicetree/bindings/mfd/ti,ds90ux9xx.txt
> >> b/Documentation/devicetree/bindings/mfd/ti,ds90ux9xx.txt new file mode
> >> 100644
> >> index 000000000000..0733da88f7ef
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mfd/ti,ds90ux9xx.txt
> >> @@ -0,0 +1,66 @@
> >> +Texas Instruments DS90Ux9xx de-/serializer controllers
> >> +
> >> +Required properties:
> >> +- compatible: Must contain a generic "ti,ds90ux9xx" value and
> > 
> >> +	may contain one more specific value from the list:
> > If it "may" contain one more specific value, when should that value be
> > present, and when can it be absent ?
> 
> Practically you can always omit a specific compatible, because (with a
> number of minor exceptions like DS90UH925Q case, see a quirk in the code)
> it is possible to read out the IC type in runtime.
> 
> Nevertheless I prefer to have a complete list of all specific compatibles
> to avoid problems with maintenance in future,

I agree with you. Let's thus word this as

Shall contain exactly one value from the list below, and the generic 
"ti,ds90ux9xx" value, in that order.

> recently I had a long
> discussion with Jassi Brar about iMX* mailbox compatibles on the DT mailing
> list, the arguments remain the same, but I don't feel enough internal power
> to start another such an exhaustive discussion right at the moment.
> 
> >> +	"ti,ds90ub925q",
> >> +	"ti,ds90uh925q",
> >> +	"ti,ds90ub927q",
> >> +	"ti,ds90uh927q",
> >> +	"ti,ds90ub926q",
> >> +	"ti,ds90uh926q",
> >> +	"ti,ds90ub928q",
> >> +	"ti,ds90uh928q",
> >> +	"ti,ds90ub940q",
> >> +	"ti,ds90uh940q".
> >> +
> >> +Optional properties:
> >> +- reg : Specifies the I2C slave address of a local de-/serializer.
> > 
> > You should explain when the reg property is required and when it isn't.
> > This
> 
> Talking about TI DS90Ux9xx IC series, ideally I'd like to shift from
> serializer/deserializer concept and promote "remote" and "local" IC, by the
> way, and if I'm not mistaken, MOST ICs are truly identical on both ends.
> 
> So, here "reg" property is need only if the IC (serializer or deserializer,
> it does not matter) is on the "local" side, i.e. it is a slave I2C device
> discovered on an I2C bus, which is under control by an application
> processor.
> 
> If IC is on the "remote" side, in other words separated by the serial link
> from the "local" IC, then "reg" property is not needed.

Let's document this then. Please make sure to document the local and remote 
concepts in the introduction first.

> > will in my opinion require a more detailed explanation of the DT model for
> > this device.
> > 
> >> +- power-gpios : GPIO line to control supplied power to the device.
> > 
> > As Marek mentioned, a regulator would be better. I would make it a
> > mandatory property, as the device always needs to be powered.
> 
> I get a memory flashback. Did we discuss recently a right property name to
> control panel power by a GPIO or was it something else?
> 
> There are quite many properties of exactly the same functionality:
> * powerdown-gpios
> * pd-gpios
> * pdn-gpios
> * power-gpios
> * powerdn-gpio
> * power-down-gpios
> * ...
> 
> Probably device tree maintainers should unify the names, but my point is
> that your argument should be applicable to all such device tree nodes /
> property descriptions and usages. Do I understand you correctly?

The general agreement is that we should try to standardize the naming of 
GPIOs. As powerdown is the inverse of enable, it has been proposed to use 
enable-gpios instead.

> I would prefer to reference to a regulator while dealing with the power
> rails, and reference to a GPIO in case of power control only like in the
> case above.

The patch made me think that the GPIO controls a regulator, which is why I 
advised using regulators. If that's not the case, if the GPIO is connected to 
a pin of the device, you should document which pin, and rephrase the 
description to remove the ambiguity.

> >> +- ti,backward-compatible-mode : Overrides backward compatibility mode.
> >> +	Possible values are "<1>" or "<0>".
> >> +	If "ti,backward-compatible-mode" is not mentioned, the backward
> >> +	compatibility mode is not touched and given by hardware pin strapping.
> > 
> > This doesn't seem to be a device description to me, it's a software
> > configuration. You should handle it in drivers.
> 
> No, it is a hardware description which allows to connect/discover ICs of
> different series, please reference to the datasheet for examples of its
> usage.

Could you please point us to the right section of the right document ?

> >> +- ti,low-frequency-mode : Overrides low frequency mode.
> >> +	Possible values are "<1>" or "<0>".
> >> +	If "ti,low-frequency-mode" is not mentioned, the low frequency mode
> >> +	is not touched and given by hardware pin strapping.
> > 
> > This sounds the same. How about giving a real life example of a case where
> > you need to set these two properties to override the pin strapping, for
> > the purpose of discussing the DT bindings ?
> 
> I have to ask, what do you mean by "a software configuration"?
> 
> Both properties are IC controls (= hardware configuration in my language),
> and these hardware properties shall be set (if needed of course) on a
> "local" IC *before* a discovery of some "remote" IC, thus the property are
> in the DT.

Software configuration refers to configuration parameters that are modified in 
software based on a policy that can be established by the software. If you can 
provide real examples that show why and how pin strapping needs to be 
overridden, that would help discussing the DT bindings.

> >> +- ti,video-map-select-msb: Sets video bridge pins to MSB mode, if it is
> >> set +	MAPSEL pin value is ignored.
> >> +- ti,video-map-select-lsb: Sets video bridge pins to LSB mode, if it is
> >> set +	MAPSEL pin value is ignored.
> > 
> > I assume those two are mutually exclusive, this should be documented, or
> > you could merge the two properties into one. Same comment as above
> > though, why do you need an override in DT ?
> 
> The property are mutually exclusive, but it is a tristate property, please
> see my answer to a similar question from Marek.

You can implement a tristate property with 0, 1 and absent (or "lsb", "msb" or 
absent, or something similar), you don't need two properties.

> >> +- ti,pixel-clock-edge : Selects Pixel Clock Edge.
> >> +	Possible values are "<1>" or "<0>".
> >> +	If "ti,pixel-clock-edge" is High <1>, output data is strobed on the
> >> +	Rising edge of the PCLK. If ti,pixel-clock-edge is Low <0>, data is
> >> +	strobed on the Falling edge of the PCLK.
> >> +	If "ti,pixel-clock-edge" is not mentioned, the pixel clock edge
> >> +	value is not touched and given by hardware pin strapping.
> > 
> > We have a standard property in
> > Documentation/devicetree/bindings/media/video-interfaces.txt for this,
> > please use it.
> 
> Okay, thank you for the link.
> 
> >> +- ti,spread-spectrum-clock-generation : Spread Sprectrum Clock
> >> Generation.
> >> +	Possible values are from "<0>" to "<7>". The same value will be
> >> +	written to SSC register. If "ti,spread-spectrum-clock-gen" is not
> >> +	found, then SSCG will be disabled.
> > 
> > This makes sense in DT in my opinion, as EMC is a system property. I
> > wonder however if exposing the hardware register directly is the best
> > option. Could you elaborate on how a system designer will select which
> > value to use, in order to find the best DT description ?
> 
> Hm, I suppose IC datasheets should serve as a better source of information.

Could you please point us to the right section(s) of the right datasheet(s) ?

> >> +TI DS90Ux9xx serializers and deserializer device nodes may contain a
> >> number +of children device nodes to describe and enable particular
> >> subcomponents +found on ICs.
> > 
> > As mentioned in my review of the cover letter I don't think this is
> > necessary.
> 
> It is, in my humble opinion if an IC can be described as "a _pinmux_ + loads
> of other functions" it makes it an MFD.

We do disagree :-)

> > You can make the serializer and deserializer I2C controllers without
> > subnodes. Same goes for GPIO control.
> 
> I have to define pinmuxes, one of the complicated and essential parts of IC
> configuration is unfairly excluded from the consideration.
> 
> >> +Example:
> >> +
> >> +serializer: serializer@c {
> >> +	compatible = "ti,ds90ub927q", "ti,ds90ux9xx";
> >> +	reg = <0xc>;
> >> +	power-gpios = <&gpio5 12 GPIO_ACTIVE_HIGH>;
> >> +	ti,backward-compatible-mode = <0>;
> >> +	ti,low-frequency-mode = <0>;
> >> +	ti,pixel-clock-edge = <0>;
> >> +	...
> >> +}
> >> +
> >> +deserializer: deserializer@3c {
> >> +	compatible = "ti,ds90ub940q", "ti,ds90ux9xx";
> >> +	reg = <0x3c>;
> >> +	power-gpios = <&gpio6 31 GPIO_ACTIVE_HIGH>;
> >> +	...
> >> +}
> >> +
> > 
> > Extra blank line ?
> 
> Right, thank you for comments.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists