Hi Kieran, On Fri, Nov 02, 2018 at 01:29:54PM +0000, Kieran Bingham wrote: > Hi Jacopo, > > On 15/10/2018 20:37, jacopo mondi wrote: > > Hi Kieran, > > > > On Mon, Oct 15, 2018 at 06:37:40PM +0100, Kieran Bingham wrote: > >> Hi Sakari, > >> > >> Thank you for the review, > >> > >> On 15/10/18 17:45, Sakari Ailus wrote: > >>> Hi Kieran, > >>> > >>> Could you cc the devicetree list for the next version, please? > >> > >> Argh - looks like I've missed the DT list on all three postings. > >> > >> No wonder the responses have been quiet :-) > >> > >> I'll do a v4 post after I've gone through some of your comments, and > >> make sure I remember the DT guys this time :) > >> > >> > >>> On Tue, Oct 09, 2018 at 09:57:23PM +0100, Kieran Bingham wrote: > >>>> From: Laurent Pinchart <[email protected]> > >>>> > >>>> The MAX9286 deserializes video data received on up to 4 Gigabit > >>>> Multimedia Serial Links (GMSL) and outputs them on a CSI-2 port using up > >>>> to 4 data lanes. > >>>> > >>>> Signed-off-by: Laurent Pinchart > >>>> <[email protected]> > >>>> Signed-off-by: Jacopo Mondi <[email protected]> > >>>> Signed-off-by: Kieran Bingham <[email protected]> > >>>> > >>>> --- > >>>> v3: > >>>> - Update binding descriptions > >>>> --- > >>>> .../bindings/media/i2c/maxim,max9286.txt | 182 ++++++++++++++++++ > >>>> 1 file changed, 182 insertions(+) > >>>> create mode 100644 > >>>> Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt > >>>> > >>>> diff --git > >>>> a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt > >>>> b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt > >>>> new file mode 100644 > >>>> index 000000000000..a73e3c0dc31b > >>>> --- /dev/null > >>>> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt > >>>> @@ -0,0 +1,182 @@ > >>>> +Maxim Integrated Quad GMSL Deserializer > >>>> +--------------------------------------- > >>>> + > >>>> +The MAX9286 deserializer receives video data on up to 4 Gigabit > >>>> Multimedia > >>>> +Serial Links (GMSL) and outputs them on a CSI-2 port using up to 4 data > >>>> lanes. > >>> > >>> CSI-2 D-PHY I presume? > >> > >> Yes, that's how I've adapted the driver based on the latest bus changes. > >> > >> Niklas - Could you confirm that everything in VIN/CSI2 is configured to > >> use D-PHY and not C-PHY at all ? > >> > >> > >>>> + > >>>> +In addition to video data, the GMSL links carry a bidirectional control > >>>> +channel that encapsulates I2C messages. The MAX9286 forwards all I2C > >>>> traffic > >>>> +not addressed to itself to the other side of the links, where a GMSL > >>>> +serializer will output it on a local I2C bus. In the other direction > >>>> all I2C > >>>> +traffic received over GMSL by the MAX9286 is output on the local I2C > >>>> bus. > >>>> + > >>>> +Required Properties: > >>>> + > >>>> +- compatible: Shall be "maxim,max9286" > >>>> +- reg: I2C device address > >>>> + > >>>> +Optional Properties: > >>>> + > >>>> +- poc-supply: Regulator providing Power over Coax to the cameras > >>>> +- pwdn-gpios: GPIO connected to the #PWDN pin > >>> > >>> If this is basically a hardware reset pin, then you could call it e.g. > >>> enable-gpios or reset-gpios. There was recently a similar discussion > >>> related to ad5820 DT bindings. > > > > Techinically is a powerdown control, it shuts the current to the chip, > > not reset it. > > > >> > >> Ah yes ... now which polarity ;-) > > > > The signal is active low (when is at physical level 0, the chip is > > off). > > Great - so this is the same as having an enable-gpio which is > GPIO_ACTIVE_HIGH right ? > > > > According to the gpio bindings documentation > > > > "The gpio-specifier's polarity flag should represent the physical level at > > the > > GPIO controller that achieves (or represents, for inputs) a logically > > asserted > > value at the device." > > > > Sakari's argument, which I understand and has been debated before, is > > to use generic names (ie. don't use the pin name as specified by the HW > > manual, but name it after its function. It doesn't matter if your pin > > is called "#RST", just call it "reset-gpios' and state the pin name in the > > documentation if you like to.) > > > > I count much more 'enable-gpios' compared to 'powerdown-gpios', so > > that seems the obvious choice, as generic names have not yet been > > documented anywhere as far as I know, but the most common ones should > > be used. > > > > Using generic names is good because in the power up/down routines, > > you don't have to care about the signals polarities, but only > > about their logical levels. At power-up if you have an "enable-gpio" > > just set it to logical 1, the gpio framework translates it to the > > appropriate physical level. Easier to write and to review. > > > > To comply with GPIO bindings we would have to > > > > enable-gpios: <&gpio 13 GPIO_ACTIVE_LOW>; > > I think I misunderstand your point here. > > Why do we have to set the polarity as ACTIVE_LOW to comply with the > bindings? >
As I read this
> > "The gpio-specifier's polarity flag should represent the physical level at
> > the
> > GPIO controller that achieves (or represents, for inputs) a logically
> > asserted
> > value at the device."
> A logically asserted 'enable' value of the device is GPIO_ACTIVE_HIGH on
> this pin?
>
> If 'powerdown' is inverted to 'enable' then so is the signal polarity?
Ok then, I see. An 'enable' is logically asserted when the pin is
physically HIGH. A 'powerdown' is logically asserted when the pin is
physically LOW. My previous reasoning was taking into account the
actual pin function (eg. the #PWDN pin is logicall asserted when is
physically LOW). But yeah, if we call it enable, it should be
described as physically active HIGH.
>
>
> > And at power up we would have to use the logical value, and for an
> > enable signal, setting it to "1" at power up would be the natural choice.
> > However to have our line set to physical 1 and have the chip powered
> > we would have to:
> >
> > gpiod_set_value(&enable, 0);
> >
> > Which makes any reason to use generic names vanish.
>
> So then we should be able to use:
>
>
> enable-gpios: <&gpio 13 GPIO_ACTIVE_HIGH>;
>
> At power up:
> gpiod_set_value(&enable, 1);
>
> At power down:
> gpiod_set_value(&enable, 0);
Yes, seems good to me :) I've been overthinking this possibly.
Thanks
j
>
>
> Interestingly though - we don't yet touch this in the driver at all!
>
> So I assume the device is conveniently at the right level for us already?
>
> But if we want any runtime-pm we would need to add in this support.
>
>
> > All of this to say that, even if less popular, I would call it
> > "powerdown-gpios", which is anyway quite generic, and describe it as:
> >
> > powerdown-gpios: Power down GPIO signal, pin name "PWDN". Active low.
> >
> > So that at power_up:
> > gpiod_set_value(&pwdn, 0);
> >
> > And at power down:
> > gpiod_set_value(&pwdn, 1);
> >
> >>
> >>
> >>>> +
> >>>> +Required endpoint nodes:
> >>>> +-----------------------
> >>>> +
> >>>> +The connections to the MAX9286 GMSL and its endpoint nodes are modeled
> >>>> using
> >>>> +the OF graph bindings in accordance with the video interface bindings
> >>>> defined
> >>>> +in Documentation/devicetree/bindings/media/video-interfaces.txt.
> >>>> +
> >>>> +The following table lists the port number corresponding to each device
> >>>> port.
> >>>> +
> >>>> + Port Description
> >>>> + ----------------------------------------
> >>>> + Port 0 GMSL Input 0
> >>>> + Port 1 GMSL Input 1
> >>>> + Port 2 GMSL Input 2
> >>>> + Port 3 GMSL Input 3
> >>>> + Port 4 CSI-2 Output
> >>>> +
> >>>> +Optional Endpoint Properties for GSML Input Ports (Port [0-3]):
> >
> > I guess Sakari means s/3/4 here: ^
> >
>
> That would be incorrect, because Port 4 is an output port, not an input
> port.
>
> > Or didn't I get his questions and then neither your answer :) ?
> >
> > Thanks
> > j
> >
> >>>
> >>> Isn't port 4 included?
> >>
> >> Hrm ... yes well I guess these are mandatory for port 4. I'll look at
> >> the wording here.
>
> Port 4 does also need a remote-endpoint, but it is to a CSI2 sink
> endpoint node. Not a GMSL source endpoint node - hence it's not
> appropriate to just 's/3/4/' above.
>
>
>
> >>>> +
> >>>> +- remote-endpoint: phandle to the remote GMSL source endpoint subnode
> >>>> in the
> >>>> + remote node port.
> >>>> +
> >>>> +Required Endpoint Properties for CSI-2 Output Port (Port 4):
> >>>> +
> >>>> +- data-lanes: array of physical CSI-2 data lane indexes.
> >>>> +- clock-lanes: index of CSI-2 clock lane.
> >>>
> >>> Is any number of lanes supported? How about lane remapping? If you do not
> >>> have lane remapping, the clock-lanes property is redundant.
> >>
> >>
> >> Uhm ... Niklas?
> >>
> >>
> >>>
> >>>> +
> >>>> +Required i2c-mux nodes:
> >>>> +----------------------
> >>>> +
> >>>> +Each GMSL link is modeled as a child bus of an i2c bus
> >>>> multiplexer/switch, in
> >>>> +accordance with bindings described in
> >>>> +Documentation/devicetree/bindings/i2c/i2c-mux.txt. The serializer
> >>>> device on
> >>>> +the remote end of the GMSL link shall be modelled as a child node of the
> >>>> +corresponding I2C bus.
> >>>> +
> >>>> +Required i2c child bus properties:
> >>>> +- all properties described as required i2c child bus nodes properties in
> >>>> + Documentation/devicetree/bindings/i2c/i2c-mux.txt.
> >>>> +
> >>>> +Example:
> >>>> +-------
> >>>> +
> >>>> + gmsl-deserializer@2c {
> >>>> + compatible = "maxim,max9286";
> >>>> + reg = <0x2c>;
> >>>> + poc-supply = <&camera_poc_12v>;
> >>>> + pwdn-gpios = <&gpio 13 GPIO_ACTIVE_LOW>;
> >>>> +
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> +
> >>>> + ports {
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> +
> >>>> + port@0 {
> >>>> + reg = <0>;
> >>>> + max9286_in0: endpoint {
> >>>> + remote-endpoint =
> >>>> <&rdacm20_out0>;
> >>>> + };
> >>>> + };
> >>>> +
> >>>> + port@1 {
> >>>> + reg = <1>;
> >>>> + max9286_in1: endpoint {
> >>>> + remote-endpoint =
> >>>> <&rdacm20_out1>;
> >>>> + };
> >>>> + };
> >>>> +
> >>>> + port@2 {
> >>>> + reg = <2>;
> >>>> + max9286_in2: endpoint {
> >>>> + remote-endpoint =
> >>>> <&rdacm20_out2>;
> >>>> + };
> >>>> + };
> >>>> +
> >>>> + port@3 {
> >>>> + reg = <3>;
> >>>> + max9286_in3: endpoint {
> >>>> + remote-endpoint =
> >>>> <&rdacm20_out3>;
> >>>> + };
> >>>> + };
> >>>> +
> >>>> + port@4 {
> >>>> + reg = <4>;
> >>>> + max9286_out: endpoint {
> >>>> + clock-lanes = <0>;
> >>>> + data-lanes = <1 2 3 4>;
> >>>> + remote-endpoint = <&csi40_in>;
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> +
> >>>> + i2c@0 {
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> + reg = <0>;
> >>>> +
> >>>> + camera@51 {
> >>>> + compatible = "imi,rdacm20";
> >>>> + reg = <0x51 0x61>;
> >>>
> >>> What's the second value for in the reg property? There's more of the same
> >>> below.
> >>>
> >>
> >> These are specific to the RDACM20:
> >>
> >> From the RDACM20 documentation:
> >>
> >> - reg: Pair of I2C device addresses, the first to be assigned to the
> >> serializer, the second to be assigned to the camera sensor.
> >>
> >> Each RDACM20 camera boots up with the same I2C addresses. The driver
> >> remaps them to the new values specified here.
> >>
> >> But they are not relevant to the max9286 except for the example of
> >> adding in the rdacm20.
>
> I wonder if perhaps we should specify a reg-names field here to make
> this clear? Especially as we could/should also add a third register
> "mcu" on this at some point.
>
>
> >>>> +
> >>>> + port {
> >>>> + rdacm20_out0: endpoint {
> >>>> + remote-endpoint =
> >>>> <&max9286_in0>;
> >>>> + };
> >>>> + };
> >>>> +
> >>>> + };
> >>>> + };
> >>>> +
> >>>> + i2c@1 {
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> + reg = <1>;
> >>>> +
> >>>> + camera@52 {
> >>>> + compatible = "imi,rdacm20";
> >>>> + reg = <0x52 0x62>;
> >>>> + port {
> >>>> + rdacm20_out1: endpoint {
> >>>> + remote-endpoint =
> >>>> <&max9286_in1>;
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> +
> >>>> + i2c@2 {
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> + reg = <2>;
> >>>> +
> >>>> + camera@53 {
> >>>> + compatible = "imi,rdacm20";
> >>>> + reg = <0x53 0x63>;
> >>>> + port {
> >>>> + rdacm20_out2: endpoint {
> >>>> + remote-endpoint =
> >>>> <&max9286_in2>;
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> +
> >>>> + i2c@3 {
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> + reg = <3>;
> >>>> +
> >>>> + camera@54 {
> >>>> + compatible = "imi,rdacm20";
> >>>> + reg = <0x54 0x64>;
> >>>> + port {
> >>>> + rdacm20_out3: endpoint {
> >>>> + remote-endpoint =
> >>>> <&max9286_in3>;
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> + };
> >>>
>
signature.asc
Description: PGP signature
