[PATCH v3 0/5] arm64: dts: renesas: Enable GMSL on R8A77970 V3M Eagle

[PATCH v3 0/5] arm64: dts: renesas: Enable GMSL on R8A77970 V3M Eagle

[Jacopo Mondi]

Hello,
   this series follows
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=448427
but implements a different way of handling the regulator-gpio_controller
circular dependency which is hunting us.

As suggested during review of v2 by Laurent, instead of using a gpio-hog to
force the MAX9286 gpio line that controls the remote cameras power, this series
introduces a new vendor property that allows to declare that the camera power
is controlled by the MAX9286 chip, instead than relying on a canonical supply,
which is impossible to establish as consumer of the gpio controller registered
by the driver.

The series introduces the new property and add support for parsing it in the
driver.

The DTS changes that follow are identical to v2, and comments there have not
been addressed waiting for a validation of patches 1 and 2.

Thanks
   j

Jacopo Mondi (2):
  dt-bindings: media: max9286: Define 'maxim,gpio-poc'
  media: i2c: max9286: Use "maxim,gpio-poc" property

Kieran Bingham (3):
  arm64: dts: renesas: eagle: Enable MAX9286
  arm64: dts: renesas: eagle: Add GMSL .dtsi
  arm64: dts: renesas: eagle: Include eagle-gmsl

 .../bindings/media/i2c/maxim,max9286.yaml     |  53 ++++-
 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi   | 186 ++++++++++++++++++
 .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 122 ++++++++++++
 drivers/media/i2c/max9286.c                   | 125 +++++++++---
 4 files changed, 456 insertions(+), 30 deletions(-)
 create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi

--
2.31.1

[PATCH v3 1/5] dt-bindings: media: max9286: Define 'maxim,gpio-poc'

[Jacopo Mondi]

Define a new vendor property in the maxim,max9286 binding schema.

The new property allows to declare that the remote camera
power-over-coax is controlled by one of the MAX9286 gpio lines.

As it is currently not possible to establish a regulator as consumer
of the MAX9286 gpio controller for this purpose, the property allows to
declare that the camera power is controlled by the MAX9286 directly.

The property accepts a gpio-index (0 or 1) and one line polarity
flag as defined by dt-bindings/gpio/gpio.h.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../bindings/media/i2c/maxim,max9286.yaml     | 53 ++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index ee16102fdfe7..480a491f3744 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -70,6 +70,24 @@ properties:
       a remote serializer whose high-threshold noise immunity is not enabled
       is 100000 micro volts
 
+  maxim,gpio-poc:
+    $ref: '/schemas/types.yaml#/definitions/uint32-array'
+    minItems: 2
+    maxItems: 2
+    description: |
+      Identifier of gpio line that controls Power over Coax to the cameras and
+      the associated polarity flag (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW)
+      as defined in <include/dt-bindings/gpio/gpio.h>.
+
+      When the remote cameras power is controlled by one of the MAX9286 gpio
+      lines, this property has to be used to specify which line among the two
+      available ones controls the remote camera power enablement.
+
+      When this property is used it is not possible to register a gpio
+      controller as the gpio lines are controlled directly by the MAX9286 and
+      not available for consumers, nor the 'poc-supply' property should be
+      specified.
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
@@ -182,7 +200,6 @@ required:
   - reg
   - ports
   - i2c-mux
-  - gpio-controller
 
 additionalProperties: false
 
@@ -327,4 +344,38 @@ examples:
           };
         };
       };
+
+      /*
+       * Example of a deserializer that controls the camera Power over Coax
+       * through one of its gpio lines.
+       */
+      gmsl-deserializer@6c {
+        compatible = "maxim,max9286";
+        reg = <0x6c>;
+        enable-gpios = <&gpio 14 GPIO_ACTIVE_HIGH>;
+
+        /*
+         * The remote camera power is controlled by MAX9286 GPIO line #0.
+         * No 'poc-supply' nor 'gpio-controller' are specified.
+         */
+        maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
+
+        /*
+         * Do not describe connections as they're the same as in the previous
+         * example.
+         */
+        ports {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          port@4 {
+            reg = <4>;
+          };
+        };
+
+        i2c-mux {
+          #address-cells = <1>;
+          #size-cells = <0>;
+        };
+      };
     };
-- 
2.31.1

Re: [PATCH v3 1/5] dt-bindings: media: max9286: Define 'maxim,gpio-poc'

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 14, 2021 at 03:51:24PM +0200, Jacopo Mondi wrote:
> Define a new vendor property in the maxim,max9286 binding schema.
> 
> The new property allows to declare that the remote camera
> power-over-coax is controlled by one of the MAX9286 gpio lines.
> 
> As it is currently not possible to establish a regulator as consumer
> of the MAX9286 gpio controller for this purpose, the property allows to
> declare that the camera power is controlled by the MAX9286 directly.
> 
> The property accepts a gpio-index (0 or 1) and one line polarity
> flag as defined by dt-bindings/gpio/gpio.h.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../bindings/media/i2c/maxim,max9286.yaml     | 53 ++++++++++++++++++-
>  1 file changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index ee16102fdfe7..480a491f3744 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -70,6 +70,24 @@ properties:
>        a remote serializer whose high-threshold noise immunity is not enabled
>        is 100000 micro volts
>  
> +  maxim,gpio-poc:

I would have written poc-gpio to match the order of the GPIO bindings
syntax.

> +    $ref: '/schemas/types.yaml#/definitions/uint32-array'
> +    minItems: 2
> +    maxItems: 2
> +    description: |
> +      Identifier of gpio line that controls Power over Coax to the cameras and

I'd write "Index of the MAX9286 GPIO output that ..." to make it clear
that this is not a generic GPIO.

> +      the associated polarity flag (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW)
> +      as defined in <include/dt-bindings/gpio/gpio.h>.
> +
> +      When the remote cameras power is controlled by one of the MAX9286 gpio
> +      lines, this property has to be used to specify which line among the two
> +      available ones controls the remote camera power enablement.
> +
> +      When this property is used it is not possible to register a gpio
> +      controller as the gpio lines are controlled directly by the MAX9286 and
> +      not available for consumers, nor the 'poc-supply' property should be
> +      specified.

Only one of the two lines would be controlled directly. Shouldn't we
still register a GPIO controller for the other line ?

Could you also mention somewhere that the first item in the array should
be 0 or 1 ? It may be hard to express in a YAML schema, so I'm fine just
documenting it in the description.

I've been wondering whether this would be a common enough issue that it
could justify support in the GPIO core to handle consumer-provider
loops, but even if that happens at some point in the future, I think the
proposal here is good enough and we won't need to switch.

> +
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
>  
> @@ -182,7 +200,6 @@ required:
>    - reg
>    - ports
>    - i2c-mux
> -  - gpio-controller
>  
>  additionalProperties: false
>  
> @@ -327,4 +344,38 @@ examples:
>            };
>          };
>        };
> +
> +      /*
> +       * Example of a deserializer that controls the camera Power over Coax
> +       * through one of its gpio lines.
> +       */
> +      gmsl-deserializer@6c {
> +        compatible = "maxim,max9286";
> +        reg = <0x6c>;
> +        enable-gpios = <&gpio 14 GPIO_ACTIVE_HIGH>;
> +
> +        /*
> +         * The remote camera power is controlled by MAX9286 GPIO line #0.
> +         * No 'poc-supply' nor 'gpio-controller' are specified.
> +         */
> +        maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
> +
> +        /*
> +         * Do not describe connections as they're the same as in the previous
> +         * example.
> +         */
> +        ports {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          port@4 {
> +            reg = <4>;
> +          };
> +        };
> +
> +        i2c-mux {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +        };
> +      };
>      };

It's customary to indent DT examples with 4 spaces. The existing
examples use two spaces, so maybe a patch on top of this would be useful
to increase readability ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/5] dt-bindings: media: max9286: Define 'maxim,gpio-poc'

[Jacopo Mondi]

Hi Laurent,

On Thu, Apr 15, 2021 at 02:47:12AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Apr 14, 2021 at 03:51:24PM +0200, Jacopo Mondi wrote:
> > Define a new vendor property in the maxim,max9286 binding schema.
> >
> > The new property allows to declare that the remote camera
> > power-over-coax is controlled by one of the MAX9286 gpio lines.
> >
> > As it is currently not possible to establish a regulator as consumer
> > of the MAX9286 gpio controller for this purpose, the property allows to
> > declare that the camera power is controlled by the MAX9286 directly.
> >
> > The property accepts a gpio-index (0 or 1) and one line polarity
> > flag as defined by dt-bindings/gpio/gpio.h.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  .../bindings/media/i2c/maxim,max9286.yaml     | 53 ++++++++++++++++++-
> >  1 file changed, 52 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > index ee16102fdfe7..480a491f3744 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > @@ -70,6 +70,24 @@ properties:
> >        a remote serializer whose high-threshold noise immunity is not enabled
> >        is 100000 micro volts
> >
> > +  maxim,gpio-poc:
>
> I would have written poc-gpio to match the order of the GPIO bindings
> syntax.
>

That's what I had :) but then the property gets matched against the
gpio schema and I get complains because it expects a phandle as first
argument... Maybe there's a way I've missed to prevent the property to
be matched with *-gpio ?

> > +    $ref: '/schemas/types.yaml#/definitions/uint32-array'
> > +    minItems: 2
> > +    maxItems: 2
> > +    description: |
> > +      Identifier of gpio line that controls Power over Coax to the cameras and
>
> I'd write "Index of the MAX9286 GPIO output that ..." to make it clear
> that this is not a generic GPIO.
>

Ack

> > +      the associated polarity flag (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW)
> > +      as defined in <include/dt-bindings/gpio/gpio.h>.
> > +
> > +      When the remote cameras power is controlled by one of the MAX9286 gpio
> > +      lines, this property has to be used to specify which line among the two
> > +      available ones controls the remote camera power enablement.
> > +
> > +      When this property is used it is not possible to register a gpio
> > +      controller as the gpio lines are controlled directly by the MAX9286 and
> > +      not available for consumers, nor the 'poc-supply' property should be
> > +      specified.
>
> Only one of the two lines would be controlled directly. Shouldn't we
> still register a GPIO controller for the other line ?

I considered that and thought it was a bit of an overkill (and I also
had a bit of troubles identifying how to register only gpio #1, as it
would be identified as gpio #0 if the actual #0 is not registered)

>
> Could you also mention somewhere that the first item in the array should
> be 0 or 1 ? It may be hard to express in a YAML schema, so I'm fine just
> documenting it in the description.
>

Sure, I tried identifying how to express that with yaml and failed :)

> I've been wondering whether this would be a common enough issue that it
> could justify support in the GPIO core to handle consumer-provider
> loops, but even if that happens at some point in the future, I think the
> proposal here is good enough and we won't need to switch.
>

Please note that with the suggestion offline from rob I will add to
the next version:

# If 'maxim,gpio-poc' is present, then 'poc-supply' and 'gpio-controller'
# are not allowed.
if:
  required:
    - maxim,gpio-poc
then:
  allOf:
    - not:
        required:
          - poc-supply
    - not:
        required:
          - gpio-controller

> > +
> >    ports:
> >      $ref: /schemas/graph.yaml#/properties/ports
> >
> > @@ -182,7 +200,6 @@ required:
> >    - reg
> >    - ports
> >    - i2c-mux
> > -  - gpio-controller
> >
> >  additionalProperties: false
> >
> > @@ -327,4 +344,38 @@ examples:
> >            };
> >          };
> >        };
> > +
> > +      /*
> > +       * Example of a deserializer that controls the camera Power over Coax
> > +       * through one of its gpio lines.
> > +       */
> > +      gmsl-deserializer@6c {
> > +        compatible = "maxim,max9286";
> > +        reg = <0x6c>;
> > +        enable-gpios = <&gpio 14 GPIO_ACTIVE_HIGH>;
> > +
> > +        /*
> > +         * The remote camera power is controlled by MAX9286 GPIO line #0.
> > +         * No 'poc-supply' nor 'gpio-controller' are specified.
> > +         */
> > +        maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
> > +
> > +        /*
> > +         * Do not describe connections as they're the same as in the previous
> > +         * example.
> > +         */
> > +        ports {
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +
> > +          port@4 {
> > +            reg = <4>;
> > +          };
> > +        };
> > +
> > +        i2c-mux {
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +        };
> > +      };
> >      };
>
> It's customary to indent DT examples with 4 spaces. The existing
> examples use two spaces, so maybe a patch on top of this would be useful
> to increase readability ?
>

Ah weird! I can add a patch before this one!

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>

Thanks
   j

> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v3 1/5] dt-bindings: media: max9286: Define 'maxim,gpio-poc'

[Geert Uytterhoeven]

Hi Jacopo,

On Thu, Apr 15, 2021 at 8:53 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> On Thu, Apr 15, 2021 at 02:47:12AM +0300, Laurent Pinchart wrote:
> > On Wed, Apr 14, 2021 at 03:51:24PM +0200, Jacopo Mondi wrote:
> > > Define a new vendor property in the maxim,max9286 binding schema.
> > >
> > > The new property allows to declare that the remote camera
> > > power-over-coax is controlled by one of the MAX9286 gpio lines.
> > >
> > > As it is currently not possible to establish a regulator as consumer
> > > of the MAX9286 gpio controller for this purpose, the property allows to
> > > declare that the camera power is controlled by the MAX9286 directly.
> > >
> > > The property accepts a gpio-index (0 or 1) and one line polarity
> > > flag as defined by dt-bindings/gpio/gpio.h.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  .../bindings/media/i2c/maxim,max9286.yaml     | 53 ++++++++++++++++++-
> > >  1 file changed, 52 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > index ee16102fdfe7..480a491f3744 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > @@ -70,6 +70,24 @@ properties:
> > >        a remote serializer whose high-threshold noise immunity is not enabled
> > >        is 100000 micro volts
> > >
> > > +  maxim,gpio-poc:
> >
> > I would have written poc-gpio to match the order of the GPIO bindings
> > syntax.
> >
>
> That's what I had :) but then the property gets matched against the
> gpio schema and I get complains because it expects a phandle as first
> argument... Maybe there's a way I've missed to prevent the property to
> be matched with *-gpio ?

GPIO hogs also use gpio properties lacking the phandle.
Hence the way this is handled for hogs may (or may not, it's yaml after all ;-)
inspire you how to handle this here.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property

[Jacopo Mondi]

The 'maxim,gpio-poc' property is used when the remote camera
power-over-coax is controlled by one of the MAX9286 gpio lines,
to instruct the driver about which line to use and what the line
polarity is.

Add to the max9286 driver support for parsing the newly introduce
property and use it if available in place of the usual supply, as it is
not possible to establish one as consumer of the max9286 gpio
controller.

If the new property is present, no gpio controller is registered and
'poc-supply' is ignored.

In order to maximize code re-use, break out the max9286 gpio handling
function so that they can be used by the gpio controller through the
gpio-consumer API, or directly by the driver code.

Wrap the power up and power down routines to their own function to
be able to use either the gpio line directly or the supply. This will
make it easier to control the remote camera power at run time.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 125 +++++++++++++++++++++++++++---------
 1 file changed, 96 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 6fd4d59fcc72..0c125f7b3d9b 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -15,6 +15,7 @@
 #include <linux/fwnode.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/module.h>
@@ -165,6 +166,9 @@ struct max9286_priv {
 
 	u32 reverse_channel_mv;
 
+	u32 gpio_poc;
+	u32 gpio_poc_flags;
+
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *pixelrate;
 
@@ -1022,20 +1026,27 @@ static int max9286_setup(struct max9286_priv *priv)
 	return 0;
 }
 
-static void max9286_gpio_set(struct gpio_chip *chip,
-			     unsigned int offset, int value)
+static int max9286_gpio_set(struct max9286_priv *priv, unsigned int offset,
+			    int value)
 {
-	struct max9286_priv *priv = gpiochip_get_data(chip);
-
 	if (value)
 		priv->gpio_state |= BIT(offset);
 	else
 		priv->gpio_state &= ~BIT(offset);
 
-	max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | priv->gpio_state);
+	return max9286_write(priv, 0x0f,
+			     MAX9286_0X0F_RESERVED | priv->gpio_state);
+}
+
+static void max9286_gpiochip_set(struct gpio_chip *chip,
+				 unsigned int offset, int value)
+{
+	struct max9286_priv *priv = gpiochip_get_data(chip);
+
+	max9286_gpio_set(priv, offset, value);
 }
 
-static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
+static int max9286_gpiochip_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct max9286_priv *priv = gpiochip_get_data(chip);
 
@@ -1055,8 +1066,8 @@ static int max9286_register_gpio(struct max9286_priv *priv)
 	gpio->of_node = dev->of_node;
 	gpio->ngpio = 2;
 	gpio->base = -1;
-	gpio->set = max9286_gpio_set;
-	gpio->get = max9286_gpio_get;
+	gpio->set = max9286_gpiochip_set;
+	gpio->get = max9286_gpiochip_get;
 	gpio->can_sleep = true;
 
 	/* GPIO values default to high */
@@ -1069,6 +1080,75 @@ static int max9286_register_gpio(struct max9286_priv *priv)
 	return ret;
 }
 
+static int max9286_parse_gpios(struct max9286_priv *priv)
+{
+	struct device *dev = &priv->client->dev;
+	u32 gpio_poc[2];
+	int ret;
+
+	/*
+	 * Parse the "gpio-poc" vendor property. If the camera power is
+	 * controlled by one of the MAX9286 gpio lines, do not register
+	 * the gpio controller and ignore 'poc-supply'.
+	 */
+	ret = of_property_read_u32_array(dev->of_node,
+					 "maxim,gpio-poc", gpio_poc, 2);
+	if (!ret) {
+		priv->gpio_poc = gpio_poc[0];
+		priv->gpio_poc_flags = gpio_poc[1];
+		if ((priv->gpio_poc != 0 && priv->gpio_poc != 1) ||
+		    (priv->gpio_poc_flags != GPIO_ACTIVE_HIGH &&
+		     priv->gpio_poc_flags != GPIO_ACTIVE_LOW)) {
+			dev_err(dev, "Invalid 'gpio-poc': (%u %u)\n",
+				priv->gpio_poc, priv->gpio_poc_flags);
+			return -EINVAL;
+		}
+
+		/* GPIO values default to high */
+		priv->gpio_state = BIT(0) | BIT(1);
+		priv->regulator = NULL;
+
+		return 0;
+	}
+
+	ret = max9286_register_gpio(priv);
+	if (ret)
+		return ret;
+
+	priv->regulator = devm_regulator_get(dev, "poc");
+	if (IS_ERR(priv->regulator)) {
+		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
+			dev_err(dev, "Unable to get PoC regulator (%ld)\n",
+				PTR_ERR(priv->regulator));
+		return PTR_ERR(priv->regulator);
+	}
+
+	return 0;
+}
+
+static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
+{
+	int ret;
+
+	/* If "poc-gpio" is used, toggle the line and do not use regulator. */
+	if (!priv->regulator)
+		return max9286_gpio_set(priv, priv->gpio_poc,
+					enable ^ priv->gpio_poc_flags);
+
+	/* Otherwise PoC is controlled using a regulator. */
+	if (enable) {
+		ret = regulator_enable(priv->regulator);
+		if (ret < 0) {
+			dev_err(&priv->client->dev, "Unable to turn PoC on\n");
+			return ret;
+		}
+
+		return 0;
+	}
+
+	return regulator_disable(priv->regulator);
+}
+
 static int max9286_init(struct device *dev)
 {
 	struct max9286_priv *priv;
@@ -1078,17 +1158,14 @@ static int max9286_init(struct device *dev)
 	client = to_i2c_client(dev);
 	priv = i2c_get_clientdata(client);
 
-	/* Enable the bus power. */
-	ret = regulator_enable(priv->regulator);
-	if (ret < 0) {
-		dev_err(&client->dev, "Unable to turn PoC on\n");
+	ret = max9286_poc_enable(priv, true);
+	if (ret)
 		return ret;
-	}
 
 	ret = max9286_setup(priv);
 	if (ret) {
 		dev_err(dev, "Unable to setup max9286\n");
-		goto err_regulator;
+		goto err_poc_disable;
 	}
 
 	/*
@@ -1098,7 +1175,7 @@ static int max9286_init(struct device *dev)
 	ret = max9286_v4l2_register(priv);
 	if (ret) {
 		dev_err(dev, "Failed to register with V4L2\n");
-		goto err_regulator;
+		goto err_poc_disable;
 	}
 
 	ret = max9286_i2c_mux_init(priv);
@@ -1114,8 +1191,8 @@ static int max9286_init(struct device *dev)
 
 err_v4l2_register:
 	max9286_v4l2_unregister(priv);
-err_regulator:
-	regulator_disable(priv->regulator);
+err_poc_disable:
+	max9286_poc_enable(priv, false);
 
 	return ret;
 }
@@ -1286,20 +1363,10 @@ static int max9286_probe(struct i2c_client *client)
 	 */
 	max9286_configure_i2c(priv, false);
 
-	ret = max9286_register_gpio(priv);
+	ret = max9286_parse_gpios(priv);
 	if (ret)
 		goto err_powerdown;
 
-	priv->regulator = devm_regulator_get(&client->dev, "poc");
-	if (IS_ERR(priv->regulator)) {
-		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
-			dev_err(&client->dev,
-				"Unable to get PoC regulator (%ld)\n",
-				PTR_ERR(priv->regulator));
-		ret = PTR_ERR(priv->regulator);
-		goto err_powerdown;
-	}
-
 	ret = max9286_parse_dt(priv);
 	if (ret)
 		goto err_powerdown;
@@ -1326,7 +1393,7 @@ static int max9286_remove(struct i2c_client *client)
 
 	max9286_v4l2_unregister(priv);
 
-	regulator_disable(priv->regulator);
+	max9286_poc_enable(priv, false);
 
 	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
 
-- 
2.31.1

Re: [PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 14, 2021 at 03:51:25PM +0200, Jacopo Mondi wrote:
> The 'maxim,gpio-poc' property is used when the remote camera
> power-over-coax is controlled by one of the MAX9286 gpio lines,
> to instruct the driver about which line to use and what the line
> polarity is.
> 
> Add to the max9286 driver support for parsing the newly introduce
s/introduce/introduced/

> property and use it if available in place of the usual supply, as it is
> not possible to establish one as consumer of the max9286 gpio
> controller.
> 
> If the new property is present, no gpio controller is registered and
> 'poc-supply' is ignored.
> 
> In order to maximize code re-use, break out the max9286 gpio handling
> function so that they can be used by the gpio controller through the
> gpio-consumer API, or directly by the driver code.
> 
> Wrap the power up and power down routines to their own function to
> be able to use either the gpio line directly or the supply. This will
> make it easier to control the remote camera power at run time.

I would have split the patch in two, with a first patch that refactors
the code, and a second one that extends it, but that's no big deal.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 125 +++++++++++++++++++++++++++---------
>  1 file changed, 96 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 6fd4d59fcc72..0c125f7b3d9b 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -15,6 +15,7 @@
>  #include <linux/fwnode.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
> +#include <linux/gpio/machine.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/module.h>
> @@ -165,6 +166,9 @@ struct max9286_priv {
>  
>  	u32 reverse_channel_mv;
>  
> +	u32 gpio_poc;
> +	u32 gpio_poc_flags;
> +
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *pixelrate;
>  
> @@ -1022,20 +1026,27 @@ static int max9286_setup(struct max9286_priv *priv)
>  	return 0;
>  }
>  
> -static void max9286_gpio_set(struct gpio_chip *chip,
> -			     unsigned int offset, int value)
> +static int max9286_gpio_set(struct max9286_priv *priv, unsigned int offset,
> +			    int value)
>  {
> -	struct max9286_priv *priv = gpiochip_get_data(chip);
> -
>  	if (value)
>  		priv->gpio_state |= BIT(offset);
>  	else
>  		priv->gpio_state &= ~BIT(offset);
>  
> -	max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | priv->gpio_state);
> +	return max9286_write(priv, 0x0f,
> +			     MAX9286_0X0F_RESERVED | priv->gpio_state);
> +}
> +
> +static void max9286_gpiochip_set(struct gpio_chip *chip,
> +				 unsigned int offset, int value)
> +{
> +	struct max9286_priv *priv = gpiochip_get_data(chip);
> +
> +	max9286_gpio_set(priv, offset, value);
>  }
>  
> -static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +static int max9286_gpiochip_get(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct max9286_priv *priv = gpiochip_get_data(chip);
>  
> @@ -1055,8 +1066,8 @@ static int max9286_register_gpio(struct max9286_priv *priv)
>  	gpio->of_node = dev->of_node;
>  	gpio->ngpio = 2;
>  	gpio->base = -1;
> -	gpio->set = max9286_gpio_set;
> -	gpio->get = max9286_gpio_get;
> +	gpio->set = max9286_gpiochip_set;
> +	gpio->get = max9286_gpiochip_get;
>  	gpio->can_sleep = true;
>  
>  	/* GPIO values default to high */
> @@ -1069,6 +1080,75 @@ static int max9286_register_gpio(struct max9286_priv *priv)
>  	return ret;
>  }
>  
> +static int max9286_parse_gpios(struct max9286_priv *priv)
> +{
> +	struct device *dev = &priv->client->dev;
> +	u32 gpio_poc[2];
> +	int ret;
> +
> +	/*
> +	 * Parse the "gpio-poc" vendor property. If the camera power is
> +	 * controlled by one of the MAX9286 gpio lines, do not register
> +	 * the gpio controller and ignore 'poc-supply'.
> +	 */
> +	ret = of_property_read_u32_array(dev->of_node,
> +					 "maxim,gpio-poc", gpio_poc, 2);
> +	if (!ret) {
> +		priv->gpio_poc = gpio_poc[0];
> +		priv->gpio_poc_flags = gpio_poc[1];
> +		if ((priv->gpio_poc != 0 && priv->gpio_poc != 1) ||

You could simply test priv->gpio_poc > 1.

> +		    (priv->gpio_poc_flags != GPIO_ACTIVE_HIGH &&
> +		     priv->gpio_poc_flags != GPIO_ACTIVE_LOW)) {
> +			dev_err(dev, "Invalid 'gpio-poc': (%u %u)\n",
> +				priv->gpio_poc, priv->gpio_poc_flags);
> +			return -EINVAL;
> +		}
> +
> +		/* GPIO values default to high */
> +		priv->gpio_state = BIT(0) | BIT(1);

Why is that ?

> +		priv->regulator = NULL;

As priv is initialized to 0, you can skip this.

> +
> +		return 0;
> +	}
> +
> +	ret = max9286_register_gpio(priv);
> +	if (ret)
> +		return ret;
> +
> +	priv->regulator = devm_regulator_get(dev, "poc");
> +	if (IS_ERR(priv->regulator)) {
> +		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> +			dev_err(dev, "Unable to get PoC regulator (%ld)\n",
> +				PTR_ERR(priv->regulator));
> +		return PTR_ERR(priv->regulator);
> +	}
> +
> +	return 0;
> +}
> +
> +static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
> +{
> +	int ret;
> +
> +	/* If "poc-gpio" is used, toggle the line and do not use regulator. */
> +	if (!priv->regulator)
> +		return max9286_gpio_set(priv, priv->gpio_poc,
> +					enable ^ priv->gpio_poc_flags);
> +
> +	/* Otherwise PoC is controlled using a regulator. */
> +	if (enable) {
> +		ret = regulator_enable(priv->regulator);
> +		if (ret < 0) {
> +			dev_err(&priv->client->dev, "Unable to turn PoC on\n");

As error message when max9286_gpio_set() fails (at least in the enable
case) would be good too. Bonus points if there's a single dev_err()
call.

> +			return ret;
> +		}
> +
> +		return 0;
> +	}
> +
> +	return regulator_disable(priv->regulator);
> +}
> +
>  static int max9286_init(struct device *dev)
>  {
>  	struct max9286_priv *priv;
> @@ -1078,17 +1158,14 @@ static int max9286_init(struct device *dev)
>  	client = to_i2c_client(dev);
>  	priv = i2c_get_clientdata(client);
>  
> -	/* Enable the bus power. */
> -	ret = regulator_enable(priv->regulator);
> -	if (ret < 0) {
> -		dev_err(&client->dev, "Unable to turn PoC on\n");
> +	ret = max9286_poc_enable(priv, true);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	ret = max9286_setup(priv);
>  	if (ret) {
>  		dev_err(dev, "Unable to setup max9286\n");
> -		goto err_regulator;
> +		goto err_poc_disable;
>  	}
>  
>  	/*
> @@ -1098,7 +1175,7 @@ static int max9286_init(struct device *dev)
>  	ret = max9286_v4l2_register(priv);
>  	if (ret) {
>  		dev_err(dev, "Failed to register with V4L2\n");
> -		goto err_regulator;
> +		goto err_poc_disable;
>  	}
>  
>  	ret = max9286_i2c_mux_init(priv);
> @@ -1114,8 +1191,8 @@ static int max9286_init(struct device *dev)
>  
>  err_v4l2_register:
>  	max9286_v4l2_unregister(priv);
> -err_regulator:
> -	regulator_disable(priv->regulator);
> +err_poc_disable:
> +	max9286_poc_enable(priv, false);
>  
>  	return ret;
>  }
> @@ -1286,20 +1363,10 @@ static int max9286_probe(struct i2c_client *client)
>  	 */
>  	max9286_configure_i2c(priv, false);
>  
> -	ret = max9286_register_gpio(priv);
> +	ret = max9286_parse_gpios(priv);
>  	if (ret)
>  		goto err_powerdown;
>  
> -	priv->regulator = devm_regulator_get(&client->dev, "poc");
> -	if (IS_ERR(priv->regulator)) {
> -		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> -			dev_err(&client->dev,
> -				"Unable to get PoC regulator (%ld)\n",
> -				PTR_ERR(priv->regulator));
> -		ret = PTR_ERR(priv->regulator);
> -		goto err_powerdown;
> -	}
> -
>  	ret = max9286_parse_dt(priv);
>  	if (ret)
>  		goto err_powerdown;
> @@ -1326,7 +1393,7 @@ static int max9286_remove(struct i2c_client *client)
>  
>  	max9286_v4l2_unregister(priv);
>  
> -	regulator_disable(priv->regulator);
> +	max9286_poc_enable(priv, false);
>  
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property

[Jacopo Mondi]

Hi Laurent,

On Thu, Apr 15, 2021 at 03:00:56AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Apr 14, 2021 at 03:51:25PM +0200, Jacopo Mondi wrote:
> > The 'maxim,gpio-poc' property is used when the remote camera
> > power-over-coax is controlled by one of the MAX9286 gpio lines,
> > to instruct the driver about which line to use and what the line
> > polarity is.
> >
> > Add to the max9286 driver support for parsing the newly introduce
> s/introduce/introduced/
>
> > property and use it if available in place of the usual supply, as it is
> > not possible to establish one as consumer of the max9286 gpio
> > controller.
> >
> > If the new property is present, no gpio controller is registered and
> > 'poc-supply' is ignored.
> >
> > In order to maximize code re-use, break out the max9286 gpio handling
> > function so that they can be used by the gpio controller through the
> > gpio-consumer API, or directly by the driver code.
> >
> > Wrap the power up and power down routines to their own function to
> > be able to use either the gpio line directly or the supply. This will
> > make it easier to control the remote camera power at run time.
>
> I would have split the patch in two, with a first patch that refactors
> the code, and a second one that extends it, but that's no big deal.
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 125 +++++++++++++++++++++++++++---------
> >  1 file changed, 96 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 6fd4d59fcc72..0c125f7b3d9b 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/fwnode.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/gpio/driver.h>
> > +#include <linux/gpio/machine.h>
> >  #include <linux/i2c.h>
> >  #include <linux/i2c-mux.h>
> >  #include <linux/module.h>
> > @@ -165,6 +166,9 @@ struct max9286_priv {
> >
> >  	u32 reverse_channel_mv;
> >
> > +	u32 gpio_poc;
> > +	u32 gpio_poc_flags;
> > +
> >  	struct v4l2_ctrl_handler ctrls;
> >  	struct v4l2_ctrl *pixelrate;
> >
> > @@ -1022,20 +1026,27 @@ static int max9286_setup(struct max9286_priv *priv)
> >  	return 0;
> >  }
> >
> > -static void max9286_gpio_set(struct gpio_chip *chip,
> > -			     unsigned int offset, int value)
> > +static int max9286_gpio_set(struct max9286_priv *priv, unsigned int offset,
> > +			    int value)
> >  {
> > -	struct max9286_priv *priv = gpiochip_get_data(chip);
> > -
> >  	if (value)
> >  		priv->gpio_state |= BIT(offset);
> >  	else
> >  		priv->gpio_state &= ~BIT(offset);
> >
> > -	max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | priv->gpio_state);
> > +	return max9286_write(priv, 0x0f,
> > +			     MAX9286_0X0F_RESERVED | priv->gpio_state);
> > +}
> > +
> > +static void max9286_gpiochip_set(struct gpio_chip *chip,
> > +				 unsigned int offset, int value)
> > +{
> > +	struct max9286_priv *priv = gpiochip_get_data(chip);
> > +
> > +	max9286_gpio_set(priv, offset, value);
> >  }
> >
> > -static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > +static int max9286_gpiochip_get(struct gpio_chip *chip, unsigned int offset)
> >  {
> >  	struct max9286_priv *priv = gpiochip_get_data(chip);
> >
> > @@ -1055,8 +1066,8 @@ static int max9286_register_gpio(struct max9286_priv *priv)
> >  	gpio->of_node = dev->of_node;
> >  	gpio->ngpio = 2;
> >  	gpio->base = -1;
> > -	gpio->set = max9286_gpio_set;
> > -	gpio->get = max9286_gpio_get;
> > +	gpio->set = max9286_gpiochip_set;
> > +	gpio->get = max9286_gpiochip_get;
> >  	gpio->can_sleep = true;
> >
> >  	/* GPIO values default to high */
> > @@ -1069,6 +1080,75 @@ static int max9286_register_gpio(struct max9286_priv *priv)
> >  	return ret;
> >  }
> >
> > +static int max9286_parse_gpios(struct max9286_priv *priv)
> > +{
> > +	struct device *dev = &priv->client->dev;
> > +	u32 gpio_poc[2];
> > +	int ret;
> > +
> > +	/*
> > +	 * Parse the "gpio-poc" vendor property. If the camera power is
> > +	 * controlled by one of the MAX9286 gpio lines, do not register
> > +	 * the gpio controller and ignore 'poc-supply'.
> > +	 */
> > +	ret = of_property_read_u32_array(dev->of_node,
> > +					 "maxim,gpio-poc", gpio_poc, 2);
> > +	if (!ret) {
> > +		priv->gpio_poc = gpio_poc[0];
> > +		priv->gpio_poc_flags = gpio_poc[1];
> > +		if ((priv->gpio_poc != 0 && priv->gpio_poc != 1) ||
>
> You could simply test priv->gpio_poc > 1.
>
> > +		    (priv->gpio_poc_flags != GPIO_ACTIVE_HIGH &&
> > +		     priv->gpio_poc_flags != GPIO_ACTIVE_LOW)) {
> > +			dev_err(dev, "Invalid 'gpio-poc': (%u %u)\n",
> > +				priv->gpio_poc, priv->gpio_poc_flags);
> > +			return -EINVAL;
> > +		}
> > +
> > +		/* GPIO values default to high */
> > +		priv->gpio_state = BIT(0) | BIT(1);
>
> Why is that ?
>

As the set/get functions of gpiochip use the gpio_state and I wanted
to use the same functions for the internal gpio handling I used
gpio_state in gpio_set(). My thinking was that in this way altering
the gpio line would be visibile to gpio consumers... which we don't
have as I won't register the gpio-controller :)

> > +		priv->regulator = NULL;
>
> As priv is initialized to 0, you can skip this.
>

Yes, I liked it explicit as it is used as flag, but it is not
required...

> > +
> > +		return 0;
> > +	}
> > +
> > +	ret = max9286_register_gpio(priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	priv->regulator = devm_regulator_get(dev, "poc");
> > +	if (IS_ERR(priv->regulator)) {
> > +		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > +			dev_err(dev, "Unable to get PoC regulator (%ld)\n",
> > +				PTR_ERR(priv->regulator));
> > +		return PTR_ERR(priv->regulator);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
> > +{
> > +	int ret;
> > +
> > +	/* If "poc-gpio" is used, toggle the line and do not use regulator. */
> > +	if (!priv->regulator)
> > +		return max9286_gpio_set(priv, priv->gpio_poc,
> > +					enable ^ priv->gpio_poc_flags);
> > +
> > +	/* Otherwise PoC is controlled using a regulator. */
> > +	if (enable) {
> > +		ret = regulator_enable(priv->regulator);
> > +		if (ret < 0) {
> > +			dev_err(&priv->client->dev, "Unable to turn PoC on\n");
>
> As error message when max9286_gpio_set() fails (at least in the enable
> case) would be good too. Bonus points if there's a single dev_err()
> call.
>

I'll see how it looks like

> > +			return ret;
> > +		}
> > +
> > +		return 0;
> > +	}
> > +
> > +	return regulator_disable(priv->regulator);
> > +}
> > +
> >  static int max9286_init(struct device *dev)
> >  {
> >  	struct max9286_priv *priv;
> > @@ -1078,17 +1158,14 @@ static int max9286_init(struct device *dev)
> >  	client = to_i2c_client(dev);
> >  	priv = i2c_get_clientdata(client);
> >
> > -	/* Enable the bus power. */
> > -	ret = regulator_enable(priv->regulator);
> > -	if (ret < 0) {
> > -		dev_err(&client->dev, "Unable to turn PoC on\n");
> > +	ret = max9286_poc_enable(priv, true);
> > +	if (ret)
> >  		return ret;
> > -	}
> >
> >  	ret = max9286_setup(priv);
> >  	if (ret) {
> >  		dev_err(dev, "Unable to setup max9286\n");
> > -		goto err_regulator;
> > +		goto err_poc_disable;
> >  	}
> >
> >  	/*
> > @@ -1098,7 +1175,7 @@ static int max9286_init(struct device *dev)
> >  	ret = max9286_v4l2_register(priv);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to register with V4L2\n");
> > -		goto err_regulator;
> > +		goto err_poc_disable;
> >  	}
> >
> >  	ret = max9286_i2c_mux_init(priv);
> > @@ -1114,8 +1191,8 @@ static int max9286_init(struct device *dev)
> >
> >  err_v4l2_register:
> >  	max9286_v4l2_unregister(priv);
> > -err_regulator:
> > -	regulator_disable(priv->regulator);
> > +err_poc_disable:
> > +	max9286_poc_enable(priv, false);
> >
> >  	return ret;
> >  }
> > @@ -1286,20 +1363,10 @@ static int max9286_probe(struct i2c_client *client)
> >  	 */
> >  	max9286_configure_i2c(priv, false);
> >
> > -	ret = max9286_register_gpio(priv);
> > +	ret = max9286_parse_gpios(priv);
> >  	if (ret)
> >  		goto err_powerdown;
> >
> > -	priv->regulator = devm_regulator_get(&client->dev, "poc");
> > -	if (IS_ERR(priv->regulator)) {
> > -		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > -			dev_err(&client->dev,
> > -				"Unable to get PoC regulator (%ld)\n",
> > -				PTR_ERR(priv->regulator));
> > -		ret = PTR_ERR(priv->regulator);
> > -		goto err_powerdown;
> > -	}
> > -
> >  	ret = max9286_parse_dt(priv);
> >  	if (ret)
> >  		goto err_powerdown;
> > @@ -1326,7 +1393,7 @@ static int max9286_remove(struct i2c_client *client)
> >
> >  	max9286_v4l2_unregister(priv);
> >
> > -	regulator_disable(priv->regulator);
> > +	max9286_poc_enable(priv, false);
> >
> >  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> >
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property

[Laurent Pinchart]

Hi Jacopo,

On Thu, Apr 15, 2021 at 08:58:48AM +0200, Jacopo Mondi wrote:
> On Thu, Apr 15, 2021 at 03:00:56AM +0300, Laurent Pinchart wrote:
> > On Wed, Apr 14, 2021 at 03:51:25PM +0200, Jacopo Mondi wrote:
> > > The 'maxim,gpio-poc' property is used when the remote camera
> > > power-over-coax is controlled by one of the MAX9286 gpio lines,
> > > to instruct the driver about which line to use and what the line
> > > polarity is.
> > >
> > > Add to the max9286 driver support for parsing the newly introduce
> >
> > s/introduce/introduced/
> >
> > > property and use it if available in place of the usual supply, as it is
> > > not possible to establish one as consumer of the max9286 gpio
> > > controller.
> > >
> > > If the new property is present, no gpio controller is registered and
> > > 'poc-supply' is ignored.
> > >
> > > In order to maximize code re-use, break out the max9286 gpio handling
> > > function so that they can be used by the gpio controller through the
> > > gpio-consumer API, or directly by the driver code.
> > >
> > > Wrap the power up and power down routines to their own function to
> > > be able to use either the gpio line directly or the supply. This will
> > > make it easier to control the remote camera power at run time.
> >
> > I would have split the patch in two, with a first patch that refactors
> > the code, and a second one that extends it, but that's no big deal.
> >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/max9286.c | 125 +++++++++++++++++++++++++++---------
> > >  1 file changed, 96 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index 6fd4d59fcc72..0c125f7b3d9b 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -15,6 +15,7 @@
> > >  #include <linux/fwnode.h>
> > >  #include <linux/gpio/consumer.h>
> > >  #include <linux/gpio/driver.h>
> > > +#include <linux/gpio/machine.h>
> > >  #include <linux/i2c.h>
> > >  #include <linux/i2c-mux.h>
> > >  #include <linux/module.h>
> > > @@ -165,6 +166,9 @@ struct max9286_priv {
> > >
> > >  	u32 reverse_channel_mv;
> > >
> > > +	u32 gpio_poc;
> > > +	u32 gpio_poc_flags;
> > > +
> > >  	struct v4l2_ctrl_handler ctrls;
> > >  	struct v4l2_ctrl *pixelrate;
> > >
> > > @@ -1022,20 +1026,27 @@ static int max9286_setup(struct max9286_priv *priv)
> > >  	return 0;
> > >  }
> > >
> > > -static void max9286_gpio_set(struct gpio_chip *chip,
> > > -			     unsigned int offset, int value)
> > > +static int max9286_gpio_set(struct max9286_priv *priv, unsigned int offset,
> > > +			    int value)
> > >  {
> > > -	struct max9286_priv *priv = gpiochip_get_data(chip);
> > > -
> > >  	if (value)
> > >  		priv->gpio_state |= BIT(offset);
> > >  	else
> > >  		priv->gpio_state &= ~BIT(offset);
> > >
> > > -	max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | priv->gpio_state);
> > > +	return max9286_write(priv, 0x0f,
> > > +			     MAX9286_0X0F_RESERVED | priv->gpio_state);
> > > +}
> > > +
> > > +static void max9286_gpiochip_set(struct gpio_chip *chip,
> > > +				 unsigned int offset, int value)
> > > +{
> > > +	struct max9286_priv *priv = gpiochip_get_data(chip);
> > > +
> > > +	max9286_gpio_set(priv, offset, value);
> > >  }
> > >
> > > -static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > > +static int max9286_gpiochip_get(struct gpio_chip *chip, unsigned int offset)
> > >  {
> > >  	struct max9286_priv *priv = gpiochip_get_data(chip);
> > >
> > > @@ -1055,8 +1066,8 @@ static int max9286_register_gpio(struct max9286_priv *priv)
> > >  	gpio->of_node = dev->of_node;
> > >  	gpio->ngpio = 2;
> > >  	gpio->base = -1;
> > > -	gpio->set = max9286_gpio_set;
> > > -	gpio->get = max9286_gpio_get;
> > > +	gpio->set = max9286_gpiochip_set;
> > > +	gpio->get = max9286_gpiochip_get;
> > >  	gpio->can_sleep = true;
> > >
> > >  	/* GPIO values default to high */
> > > @@ -1069,6 +1080,75 @@ static int max9286_register_gpio(struct max9286_priv *priv)
> > >  	return ret;
> > >  }
> > >
> > > +static int max9286_parse_gpios(struct max9286_priv *priv)
> > > +{
> > > +	struct device *dev = &priv->client->dev;
> > > +	u32 gpio_poc[2];
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * Parse the "gpio-poc" vendor property. If the camera power is
> > > +	 * controlled by one of the MAX9286 gpio lines, do not register
> > > +	 * the gpio controller and ignore 'poc-supply'.
> > > +	 */
> > > +	ret = of_property_read_u32_array(dev->of_node,
> > > +					 "maxim,gpio-poc", gpio_poc, 2);
> > > +	if (!ret) {
> > > +		priv->gpio_poc = gpio_poc[0];
> > > +		priv->gpio_poc_flags = gpio_poc[1];
> > > +		if ((priv->gpio_poc != 0 && priv->gpio_poc != 1) ||
> >
> > You could simply test priv->gpio_poc > 1.
> >
> > > +		    (priv->gpio_poc_flags != GPIO_ACTIVE_HIGH &&
> > > +		     priv->gpio_poc_flags != GPIO_ACTIVE_LOW)) {
> > > +			dev_err(dev, "Invalid 'gpio-poc': (%u %u)\n",
> > > +				priv->gpio_poc, priv->gpio_poc_flags);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		/* GPIO values default to high */
> > > +		priv->gpio_state = BIT(0) | BIT(1);
> >
> > Why is that ?
> >
> As the set/get functions of gpiochip use the gpio_state and I wanted
> to use the same functions for the internal gpio handling I used
> gpio_state in gpio_set(). My thinking was that in this way altering
> the gpio line would be visibile to gpio consumers... which we don't
> have as I won't register the gpio-controller :)

My question was why they default to high here, when they default to low
when there's a gpio-controller property.

> > > +		priv->regulator = NULL;
> >
> > As priv is initialized to 0, you can skip this.
> 
> Yes, I liked it explicit as it is used as flag, but it is not
> required...
> 
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > > +	ret = max9286_register_gpio(priv);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	priv->regulator = devm_regulator_get(dev, "poc");
> > > +	if (IS_ERR(priv->regulator)) {
> > > +		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > > +			dev_err(dev, "Unable to get PoC regulator (%ld)\n",
> > > +				PTR_ERR(priv->regulator));
> > > +		return PTR_ERR(priv->regulator);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
> > > +{
> > > +	int ret;
> > > +
> > > +	/* If "poc-gpio" is used, toggle the line and do not use regulator. */
> > > +	if (!priv->regulator)
> > > +		return max9286_gpio_set(priv, priv->gpio_poc,
> > > +					enable ^ priv->gpio_poc_flags);
> > > +
> > > +	/* Otherwise PoC is controlled using a regulator. */
> > > +	if (enable) {
> > > +		ret = regulator_enable(priv->regulator);
> > > +		if (ret < 0) {
> > > +			dev_err(&priv->client->dev, "Unable to turn PoC on\n");
> >
> > As error message when max9286_gpio_set() fails (at least in the enable
> > case) would be good too. Bonus points if there's a single dev_err()
> > call.
> 
> I'll see how it looks like
> 
> > > +			return ret;
> > > +		}
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > > +	return regulator_disable(priv->regulator);
> > > +}
> > > +
> > >  static int max9286_init(struct device *dev)
> > >  {
> > >  	struct max9286_priv *priv;
> > > @@ -1078,17 +1158,14 @@ static int max9286_init(struct device *dev)
> > >  	client = to_i2c_client(dev);
> > >  	priv = i2c_get_clientdata(client);
> > >
> > > -	/* Enable the bus power. */
> > > -	ret = regulator_enable(priv->regulator);
> > > -	if (ret < 0) {
> > > -		dev_err(&client->dev, "Unable to turn PoC on\n");
> > > +	ret = max9286_poc_enable(priv, true);
> > > +	if (ret)
> > >  		return ret;
> > > -	}
> > >
> > >  	ret = max9286_setup(priv);
> > >  	if (ret) {
> > >  		dev_err(dev, "Unable to setup max9286\n");
> > > -		goto err_regulator;
> > > +		goto err_poc_disable;
> > >  	}
> > >
> > >  	/*
> > > @@ -1098,7 +1175,7 @@ static int max9286_init(struct device *dev)
> > >  	ret = max9286_v4l2_register(priv);
> > >  	if (ret) {
> > >  		dev_err(dev, "Failed to register with V4L2\n");
> > > -		goto err_regulator;
> > > +		goto err_poc_disable;
> > >  	}
> > >
> > >  	ret = max9286_i2c_mux_init(priv);
> > > @@ -1114,8 +1191,8 @@ static int max9286_init(struct device *dev)
> > >
> > >  err_v4l2_register:
> > >  	max9286_v4l2_unregister(priv);
> > > -err_regulator:
> > > -	regulator_disable(priv->regulator);
> > > +err_poc_disable:
> > > +	max9286_poc_enable(priv, false);
> > >
> > >  	return ret;
> > >  }
> > > @@ -1286,20 +1363,10 @@ static int max9286_probe(struct i2c_client *client)
> > >  	 */
> > >  	max9286_configure_i2c(priv, false);
> > >
> > > -	ret = max9286_register_gpio(priv);
> > > +	ret = max9286_parse_gpios(priv);
> > >  	if (ret)
> > >  		goto err_powerdown;
> > >
> > > -	priv->regulator = devm_regulator_get(&client->dev, "poc");
> > > -	if (IS_ERR(priv->regulator)) {
> > > -		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > > -			dev_err(&client->dev,
> > > -				"Unable to get PoC regulator (%ld)\n",
> > > -				PTR_ERR(priv->regulator));
> > > -		ret = PTR_ERR(priv->regulator);
> > > -		goto err_powerdown;
> > > -	}
> > > -
> > >  	ret = max9286_parse_dt(priv);
> > >  	if (ret)
> > >  		goto err_powerdown;
> > > @@ -1326,7 +1393,7 @@ static int max9286_remove(struct i2c_client *client)
> > >
> > >  	max9286_v4l2_unregister(priv);
> > >
> > > -	regulator_disable(priv->regulator);
> > > +	max9286_poc_enable(priv, false);
> > >
> > >  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property

[Jacopo Mondi]

Hi Laurent,

On Thu, Apr 15, 2021 at 10:14:05PM +0300, Laurent Pinchart wrote:
> > > > +		/* GPIO values default to high */
> > > > +		priv->gpio_state = BIT(0) | BIT(1);
> > >
> > > Why is that ?
> > >
> > As the set/get functions of gpiochip use the gpio_state and I wanted
> > to use the same functions for the internal gpio handling I used
> > gpio_state in gpio_set(). My thinking was that in this way altering
> > the gpio line would be visibile to gpio consumers... which we don't
> > have as I won't register the gpio-controller :)
>
> My question was why they default to high here, when they default to low
> when there's a gpio-controller property.
>

Oh, got it now... the two output lines are high by default :)
Why do you say "they default to low when there's a gpio-controller
property" ? When does that requirement come from ?

Thanks
   j

> > > > +		priv->regulator = NULL;
> > >
> > > As priv is initialized to 0, you can skip this.
> >
> > Yes, I liked it explicit as it is used as flag, but it is not
> > required...
> >
> > > > +
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	ret = max9286_register_gpio(priv);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	priv->regulator = devm_regulator_get(dev, "poc");
> > > > +	if (IS_ERR(priv->regulator)) {
> > > > +		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > > > +			dev_err(dev, "Unable to get PoC regulator (%ld)\n",
> > > > +				PTR_ERR(priv->regulator));
> > > > +		return PTR_ERR(priv->regulator);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	/* If "poc-gpio" is used, toggle the line and do not use regulator. */
> > > > +	if (!priv->regulator)
> > > > +		return max9286_gpio_set(priv, priv->gpio_poc,
> > > > +					enable ^ priv->gpio_poc_flags);
> > > > +
> > > > +	/* Otherwise PoC is controlled using a regulator. */
> > > > +	if (enable) {
> > > > +		ret = regulator_enable(priv->regulator);
> > > > +		if (ret < 0) {
> > > > +			dev_err(&priv->client->dev, "Unable to turn PoC on\n");
> > >
> > > As error message when max9286_gpio_set() fails (at least in the enable
> > > case) would be good too. Bonus points if there's a single dev_err()
> > > call.
> >
> > I'll see how it looks like
> >
> > > > +			return ret;
> > > > +		}
> > > > +
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	return regulator_disable(priv->regulator);
> > > > +}
> > > > +
> > > >  static int max9286_init(struct device *dev)
> > > >  {
> > > >  	struct max9286_priv *priv;
> > > > @@ -1078,17 +1158,14 @@ static int max9286_init(struct device *dev)
> > > >  	client = to_i2c_client(dev);
> > > >  	priv = i2c_get_clientdata(client);
> > > >
> > > > -	/* Enable the bus power. */
> > > > -	ret = regulator_enable(priv->regulator);
> > > > -	if (ret < 0) {
> > > > -		dev_err(&client->dev, "Unable to turn PoC on\n");
> > > > +	ret = max9286_poc_enable(priv, true);
> > > > +	if (ret)
> > > >  		return ret;
> > > > -	}
> > > >
> > > >  	ret = max9286_setup(priv);
> > > >  	if (ret) {
> > > >  		dev_err(dev, "Unable to setup max9286\n");
> > > > -		goto err_regulator;
> > > > +		goto err_poc_disable;
> > > >  	}
> > > >
> > > >  	/*
> > > > @@ -1098,7 +1175,7 @@ static int max9286_init(struct device *dev)
> > > >  	ret = max9286_v4l2_register(priv);
> > > >  	if (ret) {
> > > >  		dev_err(dev, "Failed to register with V4L2\n");
> > > > -		goto err_regulator;
> > > > +		goto err_poc_disable;
> > > >  	}
> > > >
> > > >  	ret = max9286_i2c_mux_init(priv);
> > > > @@ -1114,8 +1191,8 @@ static int max9286_init(struct device *dev)
> > > >
> > > >  err_v4l2_register:
> > > >  	max9286_v4l2_unregister(priv);
> > > > -err_regulator:
> > > > -	regulator_disable(priv->regulator);
> > > > +err_poc_disable:
> > > > +	max9286_poc_enable(priv, false);
> > > >
> > > >  	return ret;
> > > >  }
> > > > @@ -1286,20 +1363,10 @@ static int max9286_probe(struct i2c_client *client)
> > > >  	 */
> > > >  	max9286_configure_i2c(priv, false);
> > > >
> > > > -	ret = max9286_register_gpio(priv);
> > > > +	ret = max9286_parse_gpios(priv);
> > > >  	if (ret)
> > > >  		goto err_powerdown;
> > > >
> > > > -	priv->regulator = devm_regulator_get(&client->dev, "poc");
> > > > -	if (IS_ERR(priv->regulator)) {
> > > > -		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > > > -			dev_err(&client->dev,
> > > > -				"Unable to get PoC regulator (%ld)\n",
> > > > -				PTR_ERR(priv->regulator));
> > > > -		ret = PTR_ERR(priv->regulator);
> > > > -		goto err_powerdown;
> > > > -	}
> > > > -
> > > >  	ret = max9286_parse_dt(priv);
> > > >  	if (ret)
> > > >  		goto err_powerdown;
> > > > @@ -1326,7 +1393,7 @@ static int max9286_remove(struct i2c_client *client)
> > > >
> > > >  	max9286_v4l2_unregister(priv);
> > > >
> > > > -	regulator_disable(priv->regulator);
> > > > +	max9286_poc_enable(priv, false);
> > > >
> > > >  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> > > >
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property

[Laurent Pinchart]

Hi Jacopo,

On Fri, Apr 16, 2021 at 09:43:07AM +0200, Jacopo Mondi wrote:
> On Thu, Apr 15, 2021 at 10:14:05PM +0300, Laurent Pinchart wrote:
> > > > > +		/* GPIO values default to high */
> > > > > +		priv->gpio_state = BIT(0) | BIT(1);
> > > >
> > > > Why is that ?
> > > >
> > > As the set/get functions of gpiochip use the gpio_state and I wanted
> > > to use the same functions for the internal gpio handling I used
> > > gpio_state in gpio_set(). My thinking was that in this way altering
> > > the gpio line would be visibile to gpio consumers... which we don't
> > > have as I won't register the gpio-controller :)
> >
> > My question was why they default to high here, when they default to low
> > when there's a gpio-controller property.
> 
> Oh, got it now... the two output lines are high by default :)
> Why do you say "they default to low when there's a gpio-controller
> property" ? When does that requirement come from ?

My bad, I thought they were set to 0 in that case, but they're not.

How about moving this initialization from max9286_register_gpio() to the
beginning of this function, as it's shared by both cases ?

> > > > > +		priv->regulator = NULL;
> > > >
> > > > As priv is initialized to 0, you can skip this.
> > >
> > > Yes, I liked it explicit as it is used as flag, but it is not
> > > required...
> > >
> > > > > +
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	ret = max9286_register_gpio(priv);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	priv->regulator = devm_regulator_get(dev, "poc");
> > > > > +	if (IS_ERR(priv->regulator)) {
> > > > > +		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > > > > +			dev_err(dev, "Unable to get PoC regulator (%ld)\n",
> > > > > +				PTR_ERR(priv->regulator));
> > > > > +		return PTR_ERR(priv->regulator);
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	/* If "poc-gpio" is used, toggle the line and do not use regulator. */
> > > > > +	if (!priv->regulator)
> > > > > +		return max9286_gpio_set(priv, priv->gpio_poc,
> > > > > +					enable ^ priv->gpio_poc_flags);
> > > > > +
> > > > > +	/* Otherwise PoC is controlled using a regulator. */
> > > > > +	if (enable) {
> > > > > +		ret = regulator_enable(priv->regulator);
> > > > > +		if (ret < 0) {
> > > > > +			dev_err(&priv->client->dev, "Unable to turn PoC on\n");
> > > >
> > > > As error message when max9286_gpio_set() fails (at least in the enable
> > > > case) would be good too. Bonus points if there's a single dev_err()
> > > > call.
> > >
> > > I'll see how it looks like
> > >
> > > > > +			return ret;
> > > > > +		}
> > > > > +
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	return regulator_disable(priv->regulator);
> > > > > +}
> > > > > +
> > > > >  static int max9286_init(struct device *dev)
> > > > >  {
> > > > >  	struct max9286_priv *priv;
> > > > > @@ -1078,17 +1158,14 @@ static int max9286_init(struct device *dev)
> > > > >  	client = to_i2c_client(dev);
> > > > >  	priv = i2c_get_clientdata(client);
> > > > >
> > > > > -	/* Enable the bus power. */
> > > > > -	ret = regulator_enable(priv->regulator);
> > > > > -	if (ret < 0) {
> > > > > -		dev_err(&client->dev, "Unable to turn PoC on\n");
> > > > > +	ret = max9286_poc_enable(priv, true);
> > > > > +	if (ret)
> > > > >  		return ret;
> > > > > -	}
> > > > >
> > > > >  	ret = max9286_setup(priv);
> > > > >  	if (ret) {
> > > > >  		dev_err(dev, "Unable to setup max9286\n");
> > > > > -		goto err_regulator;
> > > > > +		goto err_poc_disable;
> > > > >  	}
> > > > >
> > > > >  	/*
> > > > > @@ -1098,7 +1175,7 @@ static int max9286_init(struct device *dev)
> > > > >  	ret = max9286_v4l2_register(priv);
> > > > >  	if (ret) {
> > > > >  		dev_err(dev, "Failed to register with V4L2\n");
> > > > > -		goto err_regulator;
> > > > > +		goto err_poc_disable;
> > > > >  	}
> > > > >
> > > > >  	ret = max9286_i2c_mux_init(priv);
> > > > > @@ -1114,8 +1191,8 @@ static int max9286_init(struct device *dev)
> > > > >
> > > > >  err_v4l2_register:
> > > > >  	max9286_v4l2_unregister(priv);
> > > > > -err_regulator:
> > > > > -	regulator_disable(priv->regulator);
> > > > > +err_poc_disable:
> > > > > +	max9286_poc_enable(priv, false);
> > > > >
> > > > >  	return ret;
> > > > >  }
> > > > > @@ -1286,20 +1363,10 @@ static int max9286_probe(struct i2c_client *client)
> > > > >  	 */
> > > > >  	max9286_configure_i2c(priv, false);
> > > > >
> > > > > -	ret = max9286_register_gpio(priv);
> > > > > +	ret = max9286_parse_gpios(priv);
> > > > >  	if (ret)
> > > > >  		goto err_powerdown;
> > > > >
> > > > > -	priv->regulator = devm_regulator_get(&client->dev, "poc");
> > > > > -	if (IS_ERR(priv->regulator)) {
> > > > > -		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > > > > -			dev_err(&client->dev,
> > > > > -				"Unable to get PoC regulator (%ld)\n",
> > > > > -				PTR_ERR(priv->regulator));
> > > > > -		ret = PTR_ERR(priv->regulator);
> > > > > -		goto err_powerdown;
> > > > > -	}
> > > > > -
> > > > >  	ret = max9286_parse_dt(priv);
> > > > >  	if (ret)
> > > > >  		goto err_powerdown;
> > > > > @@ -1326,7 +1393,7 @@ static int max9286_remove(struct i2c_client *client)
> > > > >
> > > > >  	max9286_v4l2_unregister(priv);
> > > > >
> > > > > -	regulator_disable(priv->regulator);
> > > > > +	max9286_poc_enable(priv, false);
> > > > >
> > > > >  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> > > > >

-- 
Regards,

Laurent Pinchart

[PATCH v3 3/5] arm64: dts: renesas: eagle: Enable MAX9286

[Jacopo Mondi]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Enable the MAX9286 GMSL deserializer on the Eagle-V3M board.

Connected cameras should be defined in a device-tree overlay or included
after these definitions.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 119 ++++++++++++++++++
 1 file changed, 119 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index 874a7fc2730b..d2b6368d1e72 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -6,6 +6,8 @@
  * Copyright (C) 2017 Cogent Embedded, Inc.
  */
 
+#include <dt-bindings/gpio/gpio.h>
+
 /dts-v1/;
 #include "r8a77970.dtsi"
 
@@ -188,6 +190,11 @@ i2c0_pins: i2c0 {
 		function = "i2c0";
 	};
 
+	i2c3_pins: i2c3 {
+		groups = "i2c3_a";
+		function = "i2c3";
+	};
+
 	qspi0_pins: qspi0 {
 		groups = "qspi0_ctrl", "qspi0_data4";
 		function = "qspi0";
@@ -266,6 +273,118 @@ &rwdt {
 	status = "okay";
 };
 
+&csi40 {
+	status = "okay";
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			csi40_in: endpoint {
+				clock-lanes = <0>;
+				data-lanes = <1 2 3 4>;
+				remote-endpoint = <&max9286_out0>;
+			};
+		};
+	};
+};
+
+&i2c3 {
+	pinctrl-0 = <&i2c3_pins>;
+	pinctrl-names = "default";
+
+	status = "okay";
+	clock-frequency = <400000>;
+
+	gmsl: gmsl-deserializer@48 {
+		compatible = "maxim,max9286";
+		reg = <0x48>;
+
+		maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
+
+		/* eagle-pca9654-max9286-pwdn */
+		enable-gpios = <&io_expander 0 GPIO_ACTIVE_HIGH>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				max9286_in0: endpoint {
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				max9286_in1: endpoint {
+				};
+			};
+
+			port@2 {
+				reg = <2>;
+				max9286_in2: endpoint {
+				};
+			};
+
+			port@3 {
+				reg = <3>;
+				max9286_in3: endpoint {
+				};
+			};
+
+			port@4 {
+				reg = <4>;
+				max9286_out0: endpoint {
+					clock-lanes = <0>;
+					data-lanes = <1 2 3 4>;
+					remote-endpoint = <&csi40_in>;
+				};
+			};
+		};
+
+		i2c-mux {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			i2c@0 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0>;
+
+				status = "disabled";
+			};
+
+			i2c@1 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <1>;
+
+				status = "disabled";
+			};
+
+			i2c@2 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <2>;
+
+				status = "disabled";
+			};
+
+			i2c@3 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <3>;
+
+				status = "disabled";
+			};
+		};
+	};
+};
+
 &scif0 {
 	pinctrl-0 = <&scif0_pins>;
 	pinctrl-names = "default";
-- 
2.31.1

Re: [PATCH v3 3/5] arm64: dts: renesas: eagle: Enable MAX9286

[Laurent Pinchart]

Hi Jacopo and Kieran,

Thank you for the patch.

On Wed, Apr 14, 2021 at 03:51:26PM +0200, Jacopo Mondi wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Enable the MAX9286 GMSL deserializer on the Eagle-V3M board.
> 
> Connected cameras should be defined in a device-tree overlay or included
> after these definitions.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 119 ++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index 874a7fc2730b..d2b6368d1e72 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -6,6 +6,8 @@
>   * Copyright (C) 2017 Cogent Embedded, Inc.
>   */
>  
> +#include <dt-bindings/gpio/gpio.h>
> +
>  /dts-v1/;
>  #include "r8a77970.dtsi"
>  
> @@ -188,6 +190,11 @@ i2c0_pins: i2c0 {
>  		function = "i2c0";
>  	};
>  
> +	i2c3_pins: i2c3 {
> +		groups = "i2c3_a";
> +		function = "i2c3";
> +	};
> +
>  	qspi0_pins: qspi0 {
>  		groups = "qspi0_ctrl", "qspi0_data4";
>  		function = "qspi0";
> @@ -266,6 +273,118 @@ &rwdt {
>  	status = "okay";
>  };
>  
> +&csi40 {
> +	status = "okay";
> +
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@0 {
> +			reg = <0>;
> +
> +			csi40_in: endpoint {
> +				clock-lanes = <0>;
> +				data-lanes = <1 2 3 4>;
> +				remote-endpoint = <&max9286_out0>;
> +			};
> +		};
> +	};
> +};
> +
> +&i2c3 {
> +	pinctrl-0 = <&i2c3_pins>;
> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +	clock-frequency = <400000>;
> +
> +	gmsl: gmsl-deserializer@48 {
> +		compatible = "maxim,max9286";
> +		reg = <0x48>;
> +
> +		maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
> +
> +		/* eagle-pca9654-max9286-pwdn */
> +		enable-gpios = <&io_expander 0 GPIO_ACTIVE_HIGH>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				max9286_in0: endpoint {
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +				max9286_in1: endpoint {
> +				};
> +			};
> +
> +			port@2 {
> +				reg = <2>;
> +				max9286_in2: endpoint {
> +				};
> +			};
> +
> +			port@3 {
> +				reg = <3>;
> +				max9286_in3: endpoint {
> +				};
> +			};
> +
> +			port@4 {
> +				reg = <4>;
> +				max9286_out0: endpoint {
> +					clock-lanes = <0>;
> +					data-lanes = <1 2 3 4>;
> +					remote-endpoint = <&csi40_in>;
> +				};
> +			};
> +		};
> +
> +		i2c-mux {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			i2c@0 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <0>;
> +
> +				status = "disabled";
> +			};
> +
> +			i2c@1 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <1>;
> +
> +				status = "disabled";
> +			};
> +
> +			i2c@2 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <2>;
> +
> +				status = "disabled";
> +			};
> +
> +			i2c@3 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <3>;
> +
> +				status = "disabled";
> +			};
> +		};
> +	};
> +};
> +
>  &scif0 {
>  	pinctrl-0 = <&scif0_pins>;
>  	pinctrl-names = "default";

-- 
Regards,

Laurent Pinchart

[PATCH v3 4/5] arm64: dts: renesas: eagle: Add GMSL .dtsi

[Jacopo Mondi]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Describe the FAKRA connector available on Eagle board that allows
connecting GMSL camera modules such as IMI RDACM20 and RDACM21.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 186 ++++++++++++++++++++
 1 file changed, 186 insertions(+)
 create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi

diff --git a/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
new file mode 100644
index 000000000000..1836bca1e8b2
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Device Tree Source (overlay) for the Eagle V3M GMSL connectors
+ *
+ * Copyright (C) 2017 Ideas on Board <kieran.bingham@ideasonboard.com>
+ * Copyright (C) 2021 Jacopo Mondi <jacopo+renesas@jmondi.org>
+ *
+ * This overlay allows you to define GMSL cameras connected to the FAKRA
+ * connectors on the Eagle-V3M (or compatible) board.
+ *
+ * The following cameras are currently supported:
+ *    "imi,rdacm20"
+ *    "imi,rdacm21"
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+
+/*
+ * Select which cameras are in use:
+ * #define EAGLE_CAMERA0_RDACM20
+ * #define EAGLE_CAMERA0_RDACM21
+ *
+ * The two camera modules are configured with different image formats
+ * and cannot be mixed.
+ */
+#define EAGLE_CAMERA0_RDACM21
+#define EAGLE_CAMERA1_RDACM21
+#define EAGLE_CAMERA2_RDACM21
+#define EAGLE_CAMERA3_RDACM21
+
+/* Set the compatible string based on the camera model. */
+#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA1_RDACM21) || \
+    defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA3_RDACM21)
+#define EAGLE_CAMERA_MODEL	"imi,rdacm21"
+#define EAGLE_USE_RDACM21
+#elif defined(EAGLE_CAMERA0_RDACM20) || defined(EAGLE_CAMERA1_RDACM20) || \
+      defined(EAGLE_CAMERA2_RDACM20) || defined(EAGLE_CAMERA3_RDACM20)
+#define EAGLE_CAMERA_MODEL	"imi,rdacm20"
+#define EAGLE_USE_RDACM20
+#endif
+
+/* Define which cameras are available. */
+#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA0_RDACM20)
+#define EAGLE_USE_CAMERA_0
+#endif
+
+#if defined(EAGLE_CAMERA1_RDACM21) || defined(EAGLE_CAMERA1_RDACM20)
+#define EAGLE_USE_CAMERA_1
+#endif
+
+#if defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA2_RDACM20)
+#define EAGLE_USE_CAMERA_2
+#endif
+
+#if defined(EAGLE_CAMERA3_RDACM21) || defined(EAGLE_CAMERA3_RDACM20)
+#define EAGLE_USE_CAMERA_3
+#endif
+
+/* Define the endpoint links. */
+#ifdef EAGLE_USE_CAMERA_0
+&max9286_in0 {
+	remote-endpoint = <&fakra_con0>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+&max9286_in1 {
+	remote-endpoint = <&fakra_con1>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+&max9286_in2 {
+	remote-endpoint = <&fakra_con2>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_3
+&max9286_in3 {
+	remote-endpoint = <&fakra_con3>;
+};
+#endif
+
+/* Populate the GMSL i2c-mux bus with camera nodes. */
+#if defined(EAGLE_USE_RDACM21) || defined(EAGLE_USE_RDACM20)
+
+#ifdef EAGLE_USE_CAMERA_0
+&vin0 {
+	status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+&vin1 {
+	status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+&vin2 {
+	status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_3
+&vin3 {
+	status = "okay";
+};
+#endif
+
+&gmsl {
+
+	status = "okay";
+	maxim,reverse-channel-microvolt = <100000>;
+
+	i2c-mux {
+#ifdef EAGLE_USE_CAMERA_0
+		i2c@0 {
+			status = "okay";
+
+			camera@51 {
+				compatible = EAGLE_CAMERA_MODEL;
+				reg = <0x51>, <0x61>;
+
+				port {
+					fakra_con0: endpoint {
+						remote-endpoint = <&max9286_in0>;
+					};
+				};
+			};
+		};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+		i2c@1 {
+			status = "okay";
+
+			camera@52 {
+				compatible = EAGLE_CAMERA_MODEL;
+				reg = <0x52>, <0x62>;
+
+				port {
+					fakra_con1: endpoint {
+						remote-endpoint = <&max9286_in1>;
+					};
+				};
+			};
+		};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+		i2c@2 {
+			status = "okay";
+
+			camera@53 {
+				compatible = EAGLE_CAMERA_MODEL;
+				reg = <0x53>, <0x63>;
+
+				port {
+					fakra_con2: endpoint {
+						remote-endpoint = <&max9286_in2>;
+					};
+				};
+			};
+		};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_3
+		i2c@3 {
+			status = "okay";
+
+			camera@54 {
+				compatible = EAGLE_CAMERA_MODEL;
+				reg = <0x54>, <0x64>;
+
+				port {
+					fakra_con3: endpoint {
+						remote-endpoint = <&max9286_in3>;
+					};
+				};
+			};
+		};
+#endif
+	};
+};
+#endif
-- 
2.31.1

Re: [PATCH v3 4/5] arm64: dts: renesas: eagle: Add GMSL .dtsi

[Laurent Pinchart]

Hi Jacopo and Kieran,

Thank you for the patch.

On Wed, Apr 14, 2021 at 03:51:27PM +0200, Jacopo Mondi wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Describe the FAKRA connector available on Eagle board that allows
> connecting GMSL camera modules such as IMI RDACM20 and RDACM21.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 186 ++++++++++++++++++++
>  1 file changed, 186 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> 
> diff --git a/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> new file mode 100644
> index 000000000000..1836bca1e8b2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Device Tree Source (overlay) for the Eagle V3M GMSL connectors
> + *
> + * Copyright (C) 2017 Ideas on Board <kieran.bingham@ideasonboard.com>
> + * Copyright (C) 2021 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + *
> + * This overlay allows you to define GMSL cameras connected to the FAKRA
> + * connectors on the Eagle-V3M (or compatible) board.
> + *
> + * The following cameras are currently supported:
> + *    "imi,rdacm20"
> + *    "imi,rdacm21"
> + */
> +
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/*
> + * Select which cameras are in use:
> + * #define EAGLE_CAMERA0_RDACM20
> + * #define EAGLE_CAMERA0_RDACM21
> + *
> + * The two camera modules are configured with different image formats
> + * and cannot be mixed.
> + */
> +#define EAGLE_CAMERA0_RDACM21
> +#define EAGLE_CAMERA1_RDACM21
> +#define EAGLE_CAMERA2_RDACM21
> +#define EAGLE_CAMERA3_RDACM21

To avoid possible errors, I'd use one macro to set the camera model, and
another macro to select the ports. The second could be a bitmask if
desired.

These macros should be defined in the file that includes this file, not
here.

> +
> +/* Set the compatible string based on the camera model. */
> +#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA1_RDACM21) || \
> +    defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA3_RDACM21)
> +#define EAGLE_CAMERA_MODEL	"imi,rdacm21"
> +#define EAGLE_USE_RDACM21
> +#elif defined(EAGLE_CAMERA0_RDACM20) || defined(EAGLE_CAMERA1_RDACM20) || \
> +      defined(EAGLE_CAMERA2_RDACM20) || defined(EAGLE_CAMERA3_RDACM20)
> +#define EAGLE_CAMERA_MODEL	"imi,rdacm20"
> +#define EAGLE_USE_RDACM20
> +#endif
> +
> +/* Define which cameras are available. */
> +#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA0_RDACM20)
> +#define EAGLE_USE_CAMERA_0
> +#endif
> +
> +#if defined(EAGLE_CAMERA1_RDACM21) || defined(EAGLE_CAMERA1_RDACM20)
> +#define EAGLE_USE_CAMERA_1
> +#endif
> +
> +#if defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA2_RDACM20)
> +#define EAGLE_USE_CAMERA_2
> +#endif
> +
> +#if defined(EAGLE_CAMERA3_RDACM21) || defined(EAGLE_CAMERA3_RDACM20)
> +#define EAGLE_USE_CAMERA_3
> +#endif
> +
> +/* Define the endpoint links. */
> +#ifdef EAGLE_USE_CAMERA_0
> +&max9286_in0 {
> +	remote-endpoint = <&fakra_con0>;
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_1
> +&max9286_in1 {
> +	remote-endpoint = <&fakra_con1>;
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_2
> +&max9286_in2 {
> +	remote-endpoint = <&fakra_con2>;
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_3
> +&max9286_in3 {
> +	remote-endpoint = <&fakra_con3>;
> +};
> +#endif
> +
> +/* Populate the GMSL i2c-mux bus with camera nodes. */
> +#if defined(EAGLE_USE_RDACM21) || defined(EAGLE_USE_RDACM20)
> +
> +#ifdef EAGLE_USE_CAMERA_0
> +&vin0 {
> +	status = "okay";
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_1
> +&vin1 {
> +	status = "okay";
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_2
> +&vin2 {
> +	status = "okay";
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_3
> +&vin3 {
> +	status = "okay";
> +};
> +#endif
> +
> +&gmsl {
> +
> +	status = "okay";
> +	maxim,reverse-channel-microvolt = <100000>;
> +
> +	i2c-mux {
> +#ifdef EAGLE_USE_CAMERA_0
> +		i2c@0 {
> +			status = "okay";
> +
> +			camera@51 {
> +				compatible = EAGLE_CAMERA_MODEL;
> +				reg = <0x51>, <0x61>;
> +
> +				port {
> +					fakra_con0: endpoint {
> +						remote-endpoint = <&max9286_in0>;
> +					};
> +				};
> +			};
> +		};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_1
> +		i2c@1 {
> +			status = "okay";
> +
> +			camera@52 {
> +				compatible = EAGLE_CAMERA_MODEL;
> +				reg = <0x52>, <0x62>;
> +
> +				port {
> +					fakra_con1: endpoint {
> +						remote-endpoint = <&max9286_in1>;
> +					};
> +				};
> +			};
> +		};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_2
> +		i2c@2 {
> +			status = "okay";
> +
> +			camera@53 {
> +				compatible = EAGLE_CAMERA_MODEL;
> +				reg = <0x53>, <0x63>;
> +
> +				port {
> +					fakra_con2: endpoint {
> +						remote-endpoint = <&max9286_in2>;
> +					};
> +				};
> +			};
> +		};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_3
> +		i2c@3 {
> +			status = "okay";
> +
> +			camera@54 {
> +				compatible = EAGLE_CAMERA_MODEL;
> +				reg = <0x54>, <0x64>;
> +
> +				port {
> +					fakra_con3: endpoint {
> +						remote-endpoint = <&max9286_in3>;
> +					};
> +				};
> +			};
> +		};
> +#endif
> +	};
> +};
> +#endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 4/5] arm64: dts: renesas: eagle: Add GMSL .dtsi

[Jacopo Mondi]

Hi Laurent,

On Thu, Apr 15, 2021 at 02:53:17AM +0300, Laurent Pinchart wrote:
> Hi Jacopo and Kieran,
>
> Thank you for the patch.
>
> On Wed, Apr 14, 2021 at 03:51:27PM +0200, Jacopo Mondi wrote:
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > Describe the FAKRA connector available on Eagle board that allows
> > connecting GMSL camera modules such as IMI RDACM20 and RDACM21.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 186 ++++++++++++++++++++
> >  1 file changed, 186 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> > new file mode 100644
> > index 000000000000..1836bca1e8b2
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> > @@ -0,0 +1,186 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Device Tree Source (overlay) for the Eagle V3M GMSL connectors
> > + *
> > + * Copyright (C) 2017 Ideas on Board <kieran.bingham@ideasonboard.com>
> > + * Copyright (C) 2021 Jacopo Mondi <jacopo+renesas@jmondi.org>
> > + *
> > + * This overlay allows you to define GMSL cameras connected to the FAKRA
> > + * connectors on the Eagle-V3M (or compatible) board.
> > + *
> > + * The following cameras are currently supported:
> > + *    "imi,rdacm20"
> > + *    "imi,rdacm21"
> > + */
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> > +
> > +/*
> > + * Select which cameras are in use:
> > + * #define EAGLE_CAMERA0_RDACM20
> > + * #define EAGLE_CAMERA0_RDACM21
> > + *
> > + * The two camera modules are configured with different image formats
> > + * and cannot be mixed.
> > + */
> > +#define EAGLE_CAMERA0_RDACM21
> > +#define EAGLE_CAMERA1_RDACM21
> > +#define EAGLE_CAMERA2_RDACM21
> > +#define EAGLE_CAMERA3_RDACM21
>
> To avoid possible errors, I'd use one macro to set the camera model, and
> another macro to select the ports. The second could be a bitmask if
> desired.
>
> These macros should be defined in the file that includes this file, not
> here.

Don't worry, I have quite some comments from you on the dts patches
from the previous versio of the series still to address... As said in
the cover letter I was mostly interested in a validation of the new
property and the driver support.

Thanks
  j

>
> > +
> > +/* Set the compatible string based on the camera model. */
> > +#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA1_RDACM21) || \
> > +    defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA3_RDACM21)
> > +#define EAGLE_CAMERA_MODEL	"imi,rdacm21"
> > +#define EAGLE_USE_RDACM21
> > +#elif defined(EAGLE_CAMERA0_RDACM20) || defined(EAGLE_CAMERA1_RDACM20) || \
> > +      defined(EAGLE_CAMERA2_RDACM20) || defined(EAGLE_CAMERA3_RDACM20)
> > +#define EAGLE_CAMERA_MODEL	"imi,rdacm20"
> > +#define EAGLE_USE_RDACM20
> > +#endif
> > +
> > +/* Define which cameras are available. */
> > +#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA0_RDACM20)
> > +#define EAGLE_USE_CAMERA_0
> > +#endif
> > +
> > +#if defined(EAGLE_CAMERA1_RDACM21) || defined(EAGLE_CAMERA1_RDACM20)
> > +#define EAGLE_USE_CAMERA_1
> > +#endif
> > +
> > +#if defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA2_RDACM20)
> > +#define EAGLE_USE_CAMERA_2
> > +#endif
> > +
> > +#if defined(EAGLE_CAMERA3_RDACM21) || defined(EAGLE_CAMERA3_RDACM20)
> > +#define EAGLE_USE_CAMERA_3
> > +#endif
> > +
> > +/* Define the endpoint links. */
> > +#ifdef EAGLE_USE_CAMERA_0
> > +&max9286_in0 {
> > +	remote-endpoint = <&fakra_con0>;
> > +};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_1
> > +&max9286_in1 {
> > +	remote-endpoint = <&fakra_con1>;
> > +};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_2
> > +&max9286_in2 {
> > +	remote-endpoint = <&fakra_con2>;
> > +};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_3
> > +&max9286_in3 {
> > +	remote-endpoint = <&fakra_con3>;
> > +};
> > +#endif
> > +
> > +/* Populate the GMSL i2c-mux bus with camera nodes. */
> > +#if defined(EAGLE_USE_RDACM21) || defined(EAGLE_USE_RDACM20)
> > +
> > +#ifdef EAGLE_USE_CAMERA_0
> > +&vin0 {
> > +	status = "okay";
> > +};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_1
> > +&vin1 {
> > +	status = "okay";
> > +};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_2
> > +&vin2 {
> > +	status = "okay";
> > +};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_3
> > +&vin3 {
> > +	status = "okay";
> > +};
> > +#endif
> > +
> > +&gmsl {
> > +
> > +	status = "okay";
> > +	maxim,reverse-channel-microvolt = <100000>;
> > +
> > +	i2c-mux {
> > +#ifdef EAGLE_USE_CAMERA_0
> > +		i2c@0 {
> > +			status = "okay";
> > +
> > +			camera@51 {
> > +				compatible = EAGLE_CAMERA_MODEL;
> > +				reg = <0x51>, <0x61>;
> > +
> > +				port {
> > +					fakra_con0: endpoint {
> > +						remote-endpoint = <&max9286_in0>;
> > +					};
> > +				};
> > +			};
> > +		};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_1
> > +		i2c@1 {
> > +			status = "okay";
> > +
> > +			camera@52 {
> > +				compatible = EAGLE_CAMERA_MODEL;
> > +				reg = <0x52>, <0x62>;
> > +
> > +				port {
> > +					fakra_con1: endpoint {
> > +						remote-endpoint = <&max9286_in1>;
> > +					};
> > +				};
> > +			};
> > +		};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_2
> > +		i2c@2 {
> > +			status = "okay";
> > +
> > +			camera@53 {
> > +				compatible = EAGLE_CAMERA_MODEL;
> > +				reg = <0x53>, <0x63>;
> > +
> > +				port {
> > +					fakra_con2: endpoint {
> > +						remote-endpoint = <&max9286_in2>;
> > +					};
> > +				};
> > +			};
> > +		};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_3
> > +		i2c@3 {
> > +			status = "okay";
> > +
> > +			camera@54 {
> > +				compatible = EAGLE_CAMERA_MODEL;
> > +				reg = <0x54>, <0x64>;
> > +
> > +				port {
> > +					fakra_con3: endpoint {
> > +						remote-endpoint = <&max9286_in3>;
> > +					};
> > +				};
> > +			};
> > +		};
> > +#endif
> > +	};
> > +};
> > +#endif
>
> --
> Regards,
>
> Laurent Pinchart

[PATCH v3 5/5] arm64: dts: renesas: eagle: Include eagle-gmsl

[Jacopo Mondi]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Include the eagle-gmsl.dtsi to enable GMSL camera support on the
Eagle-V3M platform.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index d2b6368d1e72..9b8dfb5132fb 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -391,3 +391,6 @@ &scif0 {
 
 	status = "okay";
 };
+
+/* FAKRA Overlay */
+#include "eagle-gmsl.dtsi"
-- 
2.31.1

Re: [PATCH v3 5/5] arm64: dts: renesas: eagle: Include eagle-gmsl

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 14, 2021 at 03:51:28PM +0200, Jacopo Mondi wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Include the eagle-gmsl.dtsi to enable GMSL camera support on the
> Eagle-V3M platform.

This is useful for quick testing, but as I don't expect everybody to
have cameras connected to the Eagle board, it should really be handled
as an overlay.

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index d2b6368d1e72..9b8dfb5132fb 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -391,3 +391,6 @@ &scif0 {
>  
>  	status = "okay";
>  };
> +
> +/* FAKRA Overlay */
> +#include "eagle-gmsl.dtsi"

-- 
Regards,

Laurent Pinchart