| Message ID | 20181009205726.7664-2-kieran.bingham@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MAX9286 GMSL Support | expand |
Hi Kieran, Could you cc the devicetree list for the next version, please? On Tue, Oct 09, 2018 at 09:57:23PM +0100, Kieran Bingham wrote: > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > 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 <laurent.pinchart+renesas@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > --- > 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? > + > +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. > + > +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]): Isn't port 4 included? > + > +- 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. > + > +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. > + > + 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>; > + }; > + }; > + }; > + }; > + };
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 <laurent.pinchart+renesas@ideasonboard.com> >> >> 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 <laurent.pinchart+renesas@ideasonboard.com> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> >> --- >> 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. Ah yes ... now which polarity ;-) >> + >> +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]): > > Isn't port 4 included? Hrm ... yes well I guess these are mandatory for port 4. I'll look at the wording here. > >> + >> +- 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. >> + >> + 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>; >> + }; >> + }; >> + }; >> + }; >> + }; >
Hi Kieran, On 2018-10-15 18:37:40 +0100, Kieran Bingham wrote: > >> 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 ? Yes it's only D-PHY. > >> + > >> +- 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? The MAX9286 documentation contains information on lane remapping and support for any number (1-4) of enabled data-lanes. I have not tested if this works in practice but the registers are there and documented :-)
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 <laurent.pinchart+renesas@ideasonboard.com> > >> > >> 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 <laurent.pinchart+renesas@ideasonboard.com> > >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> > >> --- > >> 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). 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>; 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. 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: ^ 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. > > > > >> + > >> +- 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. > > > >> + > >> + 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>; > >> + }; > >> + }; > >> + }; > >> + }; > >> + }; > >
Hello, On Monday, 15 October 2018 22:01:21 EEST Niklas Söderlund wrote: > On 2018-10-15 18:37:40 +0100, Kieran Bingham wrote: > >>> 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 ? > > Yes it's only D-PHY. > > >>> + > >>> +- 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? > > The MAX9286 documentation contains information on lane remapping and > support for any number (1-4) of enabled data-lanes. I have not tested if > this works in practice but the registers are there and documented :-) That's my understanding too. Clock lane remapping doesn't seem to be supported though. We could thus omit the clock-lanes property.
On 16/10/2018 01:37, Laurent Pinchart wrote: > Hello, > > On Monday, 15 October 2018 22:01:21 EEST Niklas Söderlund wrote: >> On 2018-10-15 18:37:40 +0100, Kieran Bingham wrote: >>>>> 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? Updated. >>> >>> 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 ? >> >> Yes it's only D-PHY. >> >>>>> + >>>>> +- 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? >> >> The MAX9286 documentation contains information on lane remapping and >> support for any number (1-4) of enabled data-lanes. I have not tested if >> this works in practice but the registers are there and documented :-) > > That's my understanding too. Clock lane remapping doesn't seem to be supported > though. We could thus omit the clock-lanes property. Ok - no point describing something that can't be changed. Dropped. -- Regards Kieran
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 <laurent.pinchart+renesas@ideasonboard.com> >>>> >>>> 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 <laurent.pinchart+renesas@ideasonboard.com> >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >>>> >>>> --- >>>> 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? 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? > 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); 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>; >>>> + }; >>>> + }; >>>> + }; >>>> + }; >>>> + }; >>>
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 <laurent.pinchart+renesas@ideasonboard.com> > >>>> > >>>> 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 <laurent.pinchart+renesas@ideasonboard.com> > >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >>>> > >>>> --- > >>>> 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>; > >>>> + }; > >>>> + }; > >>>> + }; > >>>> + }; > >>>> + }; > >>> >
Hi Kieran, On Fri, Nov 02, 2018 at 01:29:54PM +0000, Kieran Bingham wrote: ... > >>>> +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. Ah, right. And now I recall Rob's position has been that remote-endpoint property doesn't really need documenting in per-device bindings as it's part of the graph bindings anyway; just refer to the graph bindings --- just like you refer to video-interfaces.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. + +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 + +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]): + +- 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. + +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>; + + 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>; + }; + }; + }; + }; + };