| Message ID | 20181008211205.2900-2-vz@mleia.com |
|---|---|
| State | New |
| Headers | show |
| Series | mfd/pinctrl: add initial support of TI DS90Ux9xx ICs | expand |
On 10/08/2018 11:11 PM, Vladimir Zapolskiy wrote: > From: Sandeep Jain <Sandeep_Jain@mentor.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@mentor.com> > [vzapolskiy: various updates and corrections of secondary importance] > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.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: > + "ti,ds90ub925q", > + "ti,ds90uh925q", > + "ti,ds90ub927q", > + "ti,ds90uh927q", > + "ti,ds90ub926q", > + "ti,ds90uh926q", Keep the list sorted. > + "ti,ds90ub928q", > + "ti,ds90uh928q", > + "ti,ds90ub940q", > + "ti,ds90uh940q". > + > +Optional properties: > +- reg : Specifies the I2C slave address of a local de-/serializer. > +- power-gpios : GPIO line to control supplied power to the device. Shouldn't this be regulator phandle ? > +- ti,backward-compatible-mode : Overrides backward compatibility mode. > + Possible values are "<1>" or "<0>". Make this bool , ie. present or not. > + If "ti,backward-compatible-mode" is not mentioned, the backward > + compatibility mode is not touched and given by hardware pin strapping. > +- 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. > +- 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. This needs some additional explanation, what's this about ? > +- 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. > +- 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. > + > +TI DS90Ux9xx serializers and deserializer device nodes may contain a number > +of children device nodes to describe and enable particular subcomponents > +found on ICs. > + > +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>; > + ... > +} > + >
Hi Marek, On 10/09/2018 03:13 AM, Marek Vasut wrote: > On 10/08/2018 11:11 PM, Vladimir Zapolskiy wrote: >> From: Sandeep Jain <Sandeep_Jain@mentor.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@mentor.com> >> [vzapolskiy: various updates and corrections of secondary importance] >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.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: >> + "ti,ds90ub925q", >> + "ti,ds90uh925q", >> + "ti,ds90ub927q", >> + "ti,ds90uh927q", >> + "ti,ds90ub926q", >> + "ti,ds90uh926q", > > Keep the list sorted. > actually the list is a concatenation of two sorted lists, one for serializers, another for deserializers. Perhaps it makes sense to keep the list as it is done now and just mention the selected order, but then it will complicate the formal description. >> + "ti,ds90ub928q", >> + "ti,ds90uh928q", >> + "ti,ds90ub940q", >> + "ti,ds90uh940q". >> + >> +Optional properties: >> +- reg : Specifies the I2C slave address of a local de-/serializer. >> +- power-gpios : GPIO line to control supplied power to the device. > > Shouldn't this be regulator phandle ? It could be, right. I'll ponder upon it. >> +- ti,backward-compatible-mode : Overrides backward compatibility mode. >> + Possible values are "<1>" or "<0>". > > Make this bool , ie. present or not. > It is a real tristate property which is represented by non-present, 0, 1. It shall not be bool IMHO. >> + If "ti,backward-compatible-mode" is not mentioned, the backward >> + compatibility mode is not touched and given by hardware pin strapping. >> +- 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. >> +- 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. > > This needs some additional explanation, what's this about ? > Please reference to datasheet, for instance search for MAPSEL pin description and overriding I2C commands in http://www.ti.com/lit/ds/symlink/ds90ub927q-q1.pdf I believe it makes little sense to copy excessive information from an open datasheet into bindings documentation. >> +- 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. >> +- 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. >> + >> +TI DS90Ux9xx serializers and deserializer device nodes may contain a number >> +of children device nodes to describe and enable particular subcomponents >> +found on ICs. >> + >> +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>; >> + ... >> +} >> + >> > -- Best wishes, Vladimir
On 10/09/2018 02:11 PM, Vladimir Zapolskiy wrote: > Hi Marek, > > On 10/09/2018 03:13 AM, Marek Vasut wrote: >> On 10/08/2018 11:11 PM, Vladimir Zapolskiy wrote: >>> From: Sandeep Jain <Sandeep_Jain@mentor.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. >>> [snip] >>> +Optional properties: >>> +- reg : Specifies the I2C slave address of a local de-/serializer. >>> +- power-gpios : GPIO line to control supplied power to the device. >> >> Shouldn't this be regulator phandle ? > > It could be, right. I'll ponder upon it. > No, it can not. The property describes PDB "Power-down Mode Input Pin", it is a control pin with the predefined voltage, so regulator phandle is not applicable here. -- Best wishes, Vladimir
On 10/09/2018 10:55 PM, Vladimir Zapolskiy wrote: > On 10/09/2018 02:11 PM, Vladimir Zapolskiy wrote: >> Hi Marek, >> >> On 10/09/2018 03:13 AM, Marek Vasut wrote: >>> On 10/08/2018 11:11 PM, Vladimir Zapolskiy wrote: >>>> From: Sandeep Jain <Sandeep_Jain@mentor.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. >>>> > > [snip] > >>>> +Optional properties: >>>> +- reg : Specifies the I2C slave address of a local de-/serializer. >>>> +- power-gpios : GPIO line to control supplied power to the device. >>> >>> Shouldn't this be regulator phandle ? >> >> It could be, right. I'll ponder upon it. >> > > No, it can not. > > The property describes PDB "Power-down Mode Input Pin", it is a control > pin with the predefined voltage, so regulator phandle is not applicable > here. Then the DT binding document needs updating, because this is completely unclear and confusing.
On Mon, Oct 8, 2018 at 11:12 PM Vladimir Zapolskiy <vz@mleia.com> wrote: > From: Sandeep Jain <Sandeep_Jain@mentor.com> (...) > +- 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. Please use the existing binding in Documentation/devicetree/bindings/display/panel/display-timing.txt for this: pixelclk-active = [<0>|<1>]; Please reference the above document in your binding. Yours, Linus Walleij
Hi Vladimir, Thank you for the patch. On Tuesday, 9 October 2018 00:11:59 EEST Vladimir Zapolskiy wrote: > From: Sandeep Jain <Sandeep_Jain@mentor.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@mentor.com> > [vzapolskiy: various updates and corrections of secondary importance] > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.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 ? > + "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 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. > +- 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. > +- 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 ? > +- 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 ? > +- 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. > +- 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 ? > +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. You can make the serializer and deserializer I2C controllers without subnodes. Same goes for GPIO control. > +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 ?
Hi Laurent, thank you for review, please find my comments below. On 10/12/2018 02:44 PM, Laurent Pinchart wrote: > Hi Vladimir, > > Thank you for the patch. > > On Tuesday, 9 October 2018 00:11:59 EEST Vladimir Zapolskiy wrote: >> From: Sandeep Jain <Sandeep_Jain@mentor.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@mentor.com> >> [vzapolskiy: various updates and corrections of secondary importance] >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.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, 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. > 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? 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. >> +- 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. >> +- 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. >> +- 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. >> +- 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. >> +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. > 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. -- Best wishes, Vladimir
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@mentor.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@mentor.com> > >> [vzapolskiy: various updates and corrections of secondary importance] > >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.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.
Hi Vladimir, On 08/10/18 23:11, Vladimir Zapolskiy wrote: > From: Sandeep Jain <Sandeep_Jain@mentor.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. [...] > +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>; > + ... > +} Interesting patchset, thanks. At the moment I'm working on a driver for the TI FPD-III camera serdes chips [0]. At very first sight they have many commonalities with the display chipsets [1] you implemented. Did you have a look into them? Do you think they could be implemented by the same driver? The camera serdes chips lack some features found on the display chips (e.g. audio, white balance). OTOH they have dual or quad input deserializers, which adds complexity. I'm commenting on the details in reply to the following patches documenting the DT bindings. [0] http://www.ti.com/interface/fpd-link-serdes/camera-serdes/overview.html [1] http://www.ti.com/interface/fpd-link-serdes/display-serdes/overview.html Bye,
Hi Luca, thank you for review, please find my comments below. On 10/30/2018 06:43 PM, Luca Ceresoli wrote: > Hi Vladimir, > > On 08/10/18 23:11, Vladimir Zapolskiy wrote: >> From: Sandeep Jain <Sandeep_Jain@mentor.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. > [...] >> +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>; >> + ... >> +} > > Interesting patchset, thanks. At the moment I'm working on a driver for > the TI FPD-III camera serdes chips [0]. At very first sight they have > many commonalities with the display chipsets [1] you implemented. Did > you have a look into them? Do you think they could be implemented by the > same driver? Absolutely, I believe that it should be no more than a matter of adding the correspondent data fields to describe IC specifics to the set of the published drivers. In general, and from my experience, there is no big difference between camera and display ICs from the series, my understanding is that it's just a marketing or common usecase difference. > > The camera serdes chips lack some features found on the display chips > (e.g. audio, white balance). OTOH they have dual or quad input > deserializers, which adds complexity. For what it's worth the shown core drivers support DS90Ux940 (2 mutually exclusive links, the support is already added to the series of drivers) and DS90UB964 (4 parallel independent links) ICs, both deserializers are used in connection to camera sensors. So, the short answer is that multi-link ICs are also well supported, and my intention is to push the essential core drivers firstly, then add remarkably more trivial DRM and V4L2 drivers as cell drivers. > I'm commenting on the details in reply to the following patches > documenting the DT bindings. > Thank you for review, I'm planning to collect more review comments and publish v2 in about two weeks, any kind of essential rework is not expected, the selected design of having an MFD and the drivers are proven to be easily scalable, as usual any additional wanted features could be added later on. > [0] http://www.ti.com/interface/fpd-link-serdes/camera-serdes/overview.html > [1] http://www.ti.com/interface/fpd-link-serdes/display-serdes/overview.html > -- Best wishes, Vladimir
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: + "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. +- power-gpios : GPIO line to control supplied power to the device. +- 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. +- 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. +- 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. +- 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. +- 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. + +TI DS90Ux9xx serializers and deserializer device nodes may contain a number +of children device nodes to describe and enable particular subcomponents +found on ICs. + +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>; + ... +} +