[RFT 0/4] GMSL Refresh (would be v6)

[RFT 0/4] GMSL Refresh (would be v6)

[Jacopo Mondi]

Hello this is refersh of our GMSL work.

Current situation is the following:
- Kieran sent a full v4 with multiplexed stream support and has a v5 branch in
  his repository with v4 review comment fixes on top

	I rebased the multiplexed stream's series on latest media-master
	https://jmondi.org/cgit/linux/log/?h=v4l2-mux/media-master/v6

	On top of that I took Kieran's v5 and re-applied on top:
	https://jmondi.org/cgit/linux/log/?h=jmondi/gmsl/kieran/v6

- Niklas sent a v1 (which sould have been a v5) which removes multiplexed
  streams and only support one camera and was meant for inclusion but is still
  floating around in linux-media, mostly because some of the comments on
  Kieran's v4 still applied there, if I'm not mistaken.

	I took Niklas' single stream max9286 and replaced the original
	bindings with a json-schema version
	https://jmondi.org/cgit/linux/log/?h=jmondi/gmsl/niklas/v6

I bumped all versions to v6 to avoid further confusion.

Not having a working GMSL setup I would ask to Kieran or Niklas to test this
version so that we can try re-send the single stream max9286 version with new
yaml bindings for inclusion.

(I kept linux-media e devicetree out as I would like to test the patches before
expanding the receivers list)

Thanks
   j

Jacopo Mondi (2):
  arm64: dts: renesas: Add Maxim GMSL expansion board
  arm64: dts: renesas: r8a7796=salvator-x: Include GMSL

Laurent Pinchart (1):
  dt-bindings: media: i2c: Add bindings for Maxim Integrated MAX9286

Niklas Söderlund (1):
  max9286: Add MAX9286 driver

 .../bindings/media/i2c/maxim,max9286.yaml     |  286 +++++
 MAINTAINERS                                   |   10 +
 .../boot/dts/renesas/r8a7795-salvator-x.dts   |    1 +
 .../boot/dts/renesas/salvator-x-max9286.dtsi  |  394 ++++++
 drivers/media/i2c/Kconfig                     |   11 +
 drivers/media/i2c/Makefile                    |    1 +
 drivers/media/i2c/max9286.c                   | 1081 +++++++++++++++++
 7 files changed, 1784 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
 create mode 100644 arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
 create mode 100644 drivers/media/i2c/max9286.c

--
2.23.0

[RFT 1/4] dt-bindings: media: i2c: Add bindings for Maxim Integrated MAX9286

[Jacopo Mondi]

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>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 .../bindings/media/i2c/maxim,max9286.yaml     | 286 ++++++++++++++++++
 1 file changed, 286 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
new file mode 100644
index 000000000000..731e317ef4ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -0,0 +1,286 @@
+# SPDX-License-Identifier: GPL-2.0-only
+# Copyright (C) 2019 Renesas Electronics Corp.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/maxim,max9286.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim Integrated Quad GMSL Deserializer
+
+maintainers:
+  - Jacopo Mondi <jacopo+renesas@jmondi.org>
+  - Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
+  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
+  - Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
+
+description: -|
+  The MAX9286 deserializer receives video data on up to 4 Gigabit Multimedia
+  Serial Links (GMSL) and outputs them on a CSI-2 D-PHY 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.
+
+properties:
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  compatible:
+    const: maxim,max9286
+
+  reg:
+    description: I2C device address
+    maxItems: 1
+
+  poc-supply:
+    description: Regulator providing Power over Coax to the cameras
+    maxItems: 1
+
+  enable-gpios:
+    description: GPIO connected to the \#PWDN pin with inverted polarity
+    maxItems: 1
+
+  ports:
+    type: object
+    description: -|
+      The connections to the MAX9286 GMSL and its endpoint nodes are modelled
+      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
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+      port@[0-3]:
+        properties:
+          reg:
+            enum: [ 0, 1, 2, 3 ]
+
+          endpoint:
+            type: object
+
+            properties:
+              remote-endpoint:
+                description: -|
+                 phandle to the remote GMSL source endpoint subnode in the
+                 remote node port.
+                maxItems: 1
+
+            required:
+              - remote-endpoint
+
+        required:
+          - reg
+          - endpoint
+
+        additionalProperties: false
+
+      port@4:
+        properties:
+          reg:
+            const: 4
+
+          endpoint:
+            type: object
+
+            properties:
+              remote-endpoint:
+                description: phandle to the remote CSI-2 sink endpoint.
+                maxItems: 1
+
+              data-lanes:
+                description: array of physical CSI-2 data lane indexes.
+
+            required:
+              - remote-endpoint
+              - data-lanes
+
+        required:
+          - reg
+          - endpoint
+
+        additionalProperties: false
+
+    required:
+      - port@4
+
+  i2c-mux:
+    description: -|
+      Each GMSL link is modelled 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.
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+  additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - ports
+  - i2c-mux
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c@e66d8000 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      reg = <0 0xe66d8000 0 0x40>;
+
+      gmsl-deserializer@2c {
+        compatible = "maxim,max9286";
+        reg = <0x2c>;
+        poc-supply = <&camera_poc_12v>;
+        enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
+
+        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 {
+              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>;
+
+            camera@51 {
+              reg = <0x51>;
+
+              port {
+                rdacm20_out0: endpoint {
+                  remote-endpoint = <&max9286_in0>;
+                };
+              };
+
+            };
+          };
+
+          i2c@1 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reg = <1>;
+
+            camera@52 {
+              reg = <0x52>;
+
+              port {
+                rdacm20_out1: endpoint {
+                  remote-endpoint = <&max9286_in1>;
+                };
+              };
+            };
+          };
+
+          i2c@2 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reg = <2>;
+
+            camera@53 {
+              reg = <0x53>;
+
+              port {
+                rdacm20_out2: endpoint {
+                  remote-endpoint = <&max9286_in2>;
+                };
+              };
+            };
+          };
+
+          i2c@3 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reg = <3>;
+
+            camera@54 {
+              reg = <0x54>;
+
+              port {
+                rdacm20_out3: endpoint {
+                  remote-endpoint = <&max9286_in3>;
+                };
+              };
+            };
+          };
+        };
+      };
+    };
-- 
2.23.0

[RFT 2/4] max9286: Add MAX9286 driver

[Jacopo Mondi]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The MAX9286 is a 4-channel GMSL deserializer with coax or STP input and
CSI-2 output. The device supports multicamera streaming applications,
and features the ability to synchronise the attached cameras. The
driver however limits the number of cameras to one as there are no way
to express in V4L2 today that multiple video streams are transmitted
over one CSI-2 bus.

CSI-2 output can be configured with 1 to 4 lanes, and a control channel
is supported over I2C, which implements an I2C mux to facilitate
communications with connected cameras across the reverse control
channel.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 MAINTAINERS                 |   10 +
 drivers/media/i2c/Kconfig   |   11 +
 drivers/media/i2c/Makefile  |    1 +
 drivers/media/i2c/max9286.c | 1081 +++++++++++++++++++++++++++++++++++
 4 files changed, 1103 insertions(+)
 create mode 100644 drivers/media/i2c/max9286.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 37a977cbac6f..f463d00d25c2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9900,6 +9900,16 @@ F:	Documentation/devicetree/bindings/hwmon/max6697.txt
 F:	drivers/hwmon/max6697.c
 F:	include/linux/platform_data/max6697.h
 
+MAX9286 QUAD GMSL DESERIALIZER DRIVER
+M:	Jacopo Mondi <jacopo+renesas@jmondi.org>
+M:	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
+M:	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
+M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt
+F:	drivers/media/i2c/max9286.c
+
 MAX9860 MONO AUDIO VOICE CODEC DRIVER
 M:	Peter Rosin <peda@axentia.se>
 L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index c68e002d26ea..32a4deb90617 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -442,6 +442,17 @@ config VIDEO_VPX3220
 	  To compile this driver as a module, choose M here: the
 	  module will be called vpx3220.
 
+config VIDEO_MAX9286
+	tristate "Maxim MAX9286 GMSL deserializer support"
+	depends on I2C && I2C_MUX
+	depends on VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER
+	select V4L2_FWNODE
+	help
+	  This driver supports the Maxim MAX9286 GMSL deserializer.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called max9286.
+
 comment "Video and audio decoders"
 
 config VIDEO_SAA717X
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index c147bb9d28db..8896cf8bfc4f 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -117,5 +117,6 @@ obj-$(CONFIG_VIDEO_IMX290)	+= imx290.o
 obj-$(CONFIG_VIDEO_IMX319)	+= imx319.o
 obj-$(CONFIG_VIDEO_IMX355)	+= imx355.o
 obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
+obj-$(CONFIG_VIDEO_MAX9286)	+= max9286.o
 
 obj-$(CONFIG_SDR_MAX2175) += max2175.o
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
new file mode 100644
index 000000000000..debd11ac269d
--- /dev/null
+++ b/drivers/media/i2c/max9286.c
@@ -0,0 +1,1081 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Maxim MAX9286 GMSL Deserializer Driver
+ *
+ * Copyright (C) 2017-2018 Jacopo Mondi
+ * Copyright (C) 2017-2018 Kieran Bingham
+ * Copyright (C) 2017-2018 Laurent Pinchart
+ * Copyright (C) 2017-2018 Niklas Söderlund
+ * Copyright (C) 2016 Renesas Electronics Corporation
+ * Copyright (C) 2015 Cogent Embedded, Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/fwnode.h>
+#include <linux/i2c.h>
+#include <linux/i2c-mux.h>
+#include <linux/module.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+/* Register 0x00 */
+#define MAX9286_MSTLINKSEL_AUTO		(7 << 5)
+#define MAX9286_MSTLINKSEL(n)		((n) << 5)
+#define MAX9286_EN_VS_GEN		BIT(4)
+#define MAX9286_LINKEN(n)		(1 << (n))
+/* Register 0x01 */
+#define MAX9286_FSYNCMODE_ECU		(3 << 6)
+#define MAX9286_FSYNCMODE_EXT		(2 << 6)
+#define MAX9286_FSYNCMODE_INT_OUT	(1 << 6)
+#define MAX9286_FSYNCMODE_INT_HIZ	(0 << 6)
+#define MAX9286_GPIEN			BIT(5)
+#define MAX9286_ENLMO_RSTFSYNC		BIT(2)
+#define MAX9286_FSYNCMETH_AUTO		(2 << 0)
+#define MAX9286_FSYNCMETH_SEMI_AUTO	(1 << 0)
+#define MAX9286_FSYNCMETH_MANUAL	(0 << 0)
+#define MAX9286_REG_FSYNC_PERIOD_L	0x06
+#define MAX9286_REG_FSYNC_PERIOD_M	0x07
+#define MAX9286_REG_FSYNC_PERIOD_H	0x08
+/* Register 0x0a */
+#define MAX9286_FWDCCEN(n)		(1 << ((n) + 4))
+#define MAX9286_REVCCEN(n)		(1 << (n))
+/* Register 0x0c */
+#define MAX9286_HVEN			BIT(7)
+#define MAX9286_EDC_6BIT_HAMMING	(2 << 5)
+#define MAX9286_EDC_6BIT_CRC		(1 << 5)
+#define MAX9286_EDC_1BIT_PARITY		(0 << 5)
+#define MAX9286_DESEL			BIT(4)
+#define MAX9286_INVVS			BIT(3)
+#define MAX9286_INVHS			BIT(2)
+#define MAX9286_HVSRC_D0		(2 << 0)
+#define MAX9286_HVSRC_D14		(1 << 0)
+#define MAX9286_HVSRC_D18		(0 << 0)
+/* Register 0x12 */
+#define MAX9286_CSILANECNT(n)		(((n) - 1) << 6)
+#define MAX9286_CSIDBL			BIT(5)
+#define MAX9286_DBL			BIT(4)
+#define MAX9286_DATATYPE_USER_8BIT	(11 << 0)
+#define MAX9286_DATATYPE_USER_YUV_12BIT	(10 << 0)
+#define MAX9286_DATATYPE_USER_24BIT	(9 << 0)
+#define MAX9286_DATATYPE_RAW14		(8 << 0)
+#define MAX9286_DATATYPE_RAW11		(7 << 0)
+#define MAX9286_DATATYPE_RAW10		(6 << 0)
+#define MAX9286_DATATYPE_RAW8		(5 << 0)
+#define MAX9286_DATATYPE_YUV422_10BIT	(4 << 0)
+#define MAX9286_DATATYPE_YUV422_8BIT	(3 << 0)
+#define MAX9286_DATATYPE_RGB555		(2 << 0)
+#define MAX9286_DATATYPE_RGB565		(1 << 0)
+#define MAX9286_DATATYPE_RGB888		(0 << 0)
+/* Register 0x15 */
+#define MAX9286_VC(n)			((n) << 5)
+#define MAX9286_VCTYPE			BIT(4)
+#define MAX9286_CSIOUTEN		BIT(3)
+#define MAX9286_0X15_RESV		(3 << 0)
+/* Register 0x1b */
+#define MAX9286_SWITCHIN(n)		(1 << ((n) + 4))
+#define MAX9286_ENEQ(n)			(1 << (n))
+/* Register 0x27 */
+#define MAX9286_LOCKED			BIT(7)
+/* Register 0x31 */
+#define MAX9286_FSYNC_LOCKED		BIT(6)
+/* Register 0x34 */
+#define MAX9286_I2CLOCACK		BIT(7)
+#define MAX9286_I2CSLVSH_1046NS_469NS	(3 << 5)
+#define MAX9286_I2CSLVSH_938NS_352NS	(2 << 5)
+#define MAX9286_I2CSLVSH_469NS_234NS	(1 << 5)
+#define MAX9286_I2CSLVSH_352NS_117NS	(0 << 5)
+#define MAX9286_I2CMSTBT_837KBPS	(7 << 2)
+#define MAX9286_I2CMSTBT_533KBPS	(6 << 2)
+#define MAX9286_I2CMSTBT_339KBPS	(5 << 2)
+#define MAX9286_I2CMSTBT_173KBPS	(4 << 2)
+#define MAX9286_I2CMSTBT_105KBPS	(3 << 2)
+#define MAX9286_I2CMSTBT_84KBPS		(2 << 2)
+#define MAX9286_I2CMSTBT_28KBPS		(1 << 2)
+#define MAX9286_I2CMSTBT_8KBPS		(0 << 2)
+#define MAX9286_I2CSLVTO_NONE		(3 << 0)
+#define MAX9286_I2CSLVTO_1024US		(2 << 0)
+#define MAX9286_I2CSLVTO_256US		(1 << 0)
+#define MAX9286_I2CSLVTO_64US		(0 << 0)
+/* Register 0x3b */
+#define MAX9286_REV_TRF(n)		((n) << 4)
+#define MAX9286_REV_AMP(n)		((((n) - 30) / 10) << 1) /* in mV */
+#define MAX9286_REV_AMP_X		BIT(0)
+/* Register 0x3f */
+#define MAX9286_EN_REV_CFG		BIT(6)
+#define MAX9286_REV_FLEN(n)		((n) - 20)
+/* Register 0x49 */
+#define MAX9286_VIDEO_DETECT_MASK	0x0f
+/* Register 0x69 */
+#define MAX9286_LFLTBMONMASKED		BIT(7)
+#define MAX9286_LOCKMONMASKED		BIT(6)
+#define MAX9286_AUTOCOMBACKEN		BIT(5)
+#define MAX9286_AUTOMASKEN		BIT(4)
+#define MAX9286_MASKLINK(n)		((n) << 0)
+
+#define MAX9286_NUM_GMSL		4
+#define MAX9286_N_SINKS			4
+#define MAX9286_N_PADS			5
+#define MAX9286_SRC_PAD			4
+
+#define MAXIM_I2C_I2C_SPEED_400KHZ	MAX9286_I2CMSTBT_339KBPS
+#define MAXIM_I2C_I2C_SPEED_100KHZ	MAX9286_I2CMSTBT_105KBPS
+#define MAXIM_I2C_SPEED			MAXIM_I2C_I2C_SPEED_100KHZ
+
+struct max9286_source {
+	struct v4l2_async_subdev asd;
+	struct v4l2_subdev *sd;
+	struct fwnode_handle *fwnode;
+};
+
+#define asd_to_max9286_source(_asd) \
+	container_of(_asd, struct max9286_source, asd)
+
+struct max9286_priv {
+	struct i2c_client *client;
+	struct v4l2_subdev sd;
+	struct media_pad pads[MAX9286_N_PADS];
+	struct regulator *regulator;
+	bool poc_enabled;
+	int streaming;
+
+	struct i2c_mux_core *mux;
+	unsigned int mux_channel;
+
+	struct v4l2_ctrl_handler ctrls;
+
+	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
+
+	unsigned int nsources;
+	unsigned int source_mask;
+	unsigned int route_mask;
+	unsigned int csi2_data_lanes;
+	struct max9286_source sources[MAX9286_NUM_GMSL];
+	struct v4l2_async_notifier notifier;
+};
+
+static struct max9286_source *next_source(struct max9286_priv *priv,
+					  struct max9286_source *source)
+{
+	if (!source)
+		source = &priv->sources[0];
+	else
+		source++;
+
+	for (; source < &priv->sources[MAX9286_NUM_GMSL]; source++) {
+		if (source->fwnode)
+			return source;
+	}
+
+	return NULL;
+}
+
+#define for_each_source(max9286, source) \
+	for (source = NULL; (source = next_source(max9286, source)); )
+
+#define to_index(max9286, source) (source - &max9286->sources[0])
+
+static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct max9286_priv, sd);
+}
+
+/* -----------------------------------------------------------------------------
+ * I2C IO
+ */
+
+static int max9286_read(struct max9286_priv *priv, u8 reg)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(priv->client, reg);
+	if (ret < 0)
+		dev_err(&priv->client->dev,
+			"%s: register 0x%02x read failed (%d)\n",
+			__func__, reg, ret);
+
+	return ret;
+}
+
+static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(priv->client, reg, val);
+	if (ret < 0)
+		dev_err(&priv->client->dev,
+			"%s: register 0x%02x write failed (%d)\n",
+			__func__, reg, ret);
+
+	return ret;
+}
+
+/* -----------------------------------------------------------------------------
+ * I2C Multiplexer
+ */
+
+static int max9286_i2c_mux_close(struct max9286_priv *priv)
+{
+	/* FIXME: See note in max9286_i2c_mux_select() */
+	if (priv->streaming)
+		return 0;
+	/*
+	 * Ensure that both the forward and reverse channel are disabled on the
+	 * mux, and that the channel ID is invalidated to ensure we reconfigure
+	 * on the next select call.
+	 */
+	priv->mux_channel = -1;
+	max9286_write(priv, 0x0a, 0x00);
+	usleep_range(3000, 5000);
+
+	return 0;
+}
+
+static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
+{
+	struct max9286_priv *priv = i2c_mux_priv(muxc);
+
+	/*
+	 * FIXME: This state keeping is a hack and do the job. It should
+	 * be should be reworked. One option to consider is that once all
+	 * cameras are programmed the mux selection logic should be disabled
+	 * and all all reverse and forward channels enable all the time.
+	 *
+	 * In any case this logic with a int that have two states should be
+	 * reworked!
+	 */
+	if (priv->streaming == 1) {
+		max9286_write(priv, 0x0a, 0xff);
+		priv->streaming = 2;
+		return 0;
+	} else if (priv->streaming == 2) {
+		return 0;
+	}
+
+	if (priv->mux_channel == chan)
+		return 0;
+
+	priv->mux_channel = chan;
+
+	max9286_write(priv, 0x0a,
+		      MAX9286_FWDCCEN(chan) | MAX9286_REVCCEN(chan));
+
+	/*
+	 * We must sleep after any change to the forward or reverse channel
+	 * configuration.
+	 */
+	usleep_range(3000, 5000);
+
+	return 0;
+}
+
+static int max9286_i2c_mux_init(struct max9286_priv *priv)
+{
+	struct max9286_source *source;
+	int ret;
+
+	if (!i2c_check_functionality(priv->client->adapter,
+				     I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
+		return -ENODEV;
+
+	priv->mux = i2c_mux_alloc(priv->client->adapter, &priv->client->dev,
+				 priv->nsources, 0, I2C_MUX_LOCKED,
+				 max9286_i2c_mux_select, NULL);
+	if (!priv->mux)
+		return -ENOMEM;
+
+	priv->mux->priv = priv;
+
+	for_each_source(priv, source) {
+		unsigned int index = to_index(priv, source);
+
+		ret = i2c_mux_add_adapter(priv->mux, 0, index, 0);
+		if (ret < 0)
+			goto error;
+	}
+
+	return 0;
+
+error:
+	i2c_mux_del_adapters(priv->mux);
+	return ret;
+}
+
+static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
+{
+	u8 config = MAX9286_I2CSLVSH_469NS_234NS | MAX9286_I2CSLVTO_1024US |
+		    MAXIM_I2C_SPEED;
+
+	if (localack)
+		config |= MAX9286_I2CLOCACK;
+
+	max9286_write(priv, 0x34, config);
+	usleep_range(3000, 5000);
+}
+
+/* -----------------------------------------------------------------------------
+ * V4L2 Subdev
+ */
+
+static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
+				struct v4l2_subdev *subdev,
+				struct v4l2_async_subdev *asd)
+{
+	struct max9286_priv *priv = sd_to_max9286(notifier->sd);
+	struct max9286_source *source = asd_to_max9286_source(asd);
+	unsigned int index = to_index(priv, source);
+	unsigned int src_pad;
+	int ret;
+
+	ret = media_entity_get_fwnode_pad(&subdev->entity,
+					  source->fwnode,
+					  MEDIA_PAD_FL_SOURCE);
+	if (ret < 0) {
+		dev_err(&priv->client->dev,
+			"Failed to find pad for %s\n", subdev->name);
+		return ret;
+	}
+
+	source->sd = subdev;
+	src_pad = ret;
+
+	ret = media_create_pad_link(&source->sd->entity, src_pad,
+				    &priv->sd.entity, index,
+				    MEDIA_LNK_FL_ENABLED |
+				    MEDIA_LNK_FL_IMMUTABLE);
+	if (ret) {
+		dev_err(&priv->client->dev,
+			"Unable to link %s:%u -> %s:%u\n",
+			source->sd->name, src_pad, priv->sd.name, index);
+		return ret;
+	}
+
+	dev_dbg(&priv->client->dev, "Bound %s pad: %u on index %u\n",
+		subdev->name, src_pad, index);
+
+	return 0;
+}
+
+static void max9286_notify_unbind(struct v4l2_async_notifier *notifier,
+				  struct v4l2_subdev *subdev,
+				  struct v4l2_async_subdev *asd)
+{
+	struct max9286_source *source = asd_to_max9286_source(asd);
+
+	source->sd = NULL;
+}
+
+static const struct v4l2_async_notifier_operations max9286_notify_ops = {
+	.bound = max9286_notify_bound,
+	.unbind = max9286_notify_unbind,
+};
+
+/*
+ * max9286_check_video_links() - Make sure video links are detected and locked
+ *
+ * Performs safety checks on video link status. Make sure they are detected
+ * and all enabled links are locked.
+ *
+ * Returns 0 for success, -EIO for errors.
+ */
+static int max9286_check_video_links(struct max9286_priv *priv)
+{
+	unsigned int i;
+	int ret;
+
+	/*
+	 * Make sure valid video links are detected.
+	 * The delay is not characterized in de-serializer manual, wait up
+	 * to 5 ms.
+	 */
+	for (i = 0; i < 10; i++) {
+		ret = max9286_read(priv, 0x49);
+		if (ret < 0)
+			return -EIO;
+
+		if ((ret & MAX9286_VIDEO_DETECT_MASK) == priv->source_mask)
+			break;
+
+		usleep_range(350, 500);
+	}
+
+	if (i == 10) {
+		dev_err(&priv->client->dev,
+			"Unable to detect video links: 0x%02x\n", ret);
+		return -EIO;
+	}
+
+	/* Make sure all enabled links are locked (4ms max). */
+	for (i = 0; i < 10; i++) {
+		ret = max9286_read(priv, 0x27);
+		if (ret < 0)
+			return -EIO;
+
+		if (ret & MAX9286_LOCKED)
+			break;
+
+		usleep_range(350, 450);
+	}
+
+	if (i == 10) {
+		dev_err(&priv->client->dev, "Not all enabled links locked\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct max9286_priv *priv = sd_to_max9286(sd);
+	struct max9286_source *source;
+	unsigned int i;
+	bool sync = false;
+	int ret;
+
+	if (enable) {
+		/* FIXME: See note in max9286_i2c_mux_select() */
+		priv->streaming = 1;
+
+		/* Start all cameras. */
+		for_each_source(priv, source) {
+			ret = v4l2_subdev_call(source->sd, video, s_stream, 1);
+			if (ret)
+				return ret;
+		}
+
+		ret = max9286_check_video_links(priv);
+		if (ret)
+			return ret;
+
+		/*
+		 * Wait until frame synchronization is locked.
+		 *
+		 * Manual says frame sync locking should take ~6 VTS.
+		 * From pratical experience at least 8 are required. Give
+		 * 12 complete frames time (~33ms at 30 fps) to achieve frame
+		 * locking before returning error.
+		 */
+		for (i = 0; i < 36; i++) {
+			if (max9286_read(priv, 0x31) & MAX9286_FSYNC_LOCKED) {
+				sync = true;
+				break;
+			}
+			usleep_range(9000, 11000);
+		}
+
+		if (!sync) {
+			dev_err(&priv->client->dev,
+				"Failed to get frame synchronization\n");
+			return -EINVAL;
+		}
+
+		/*
+		 * Enable CSI output, VC set according to link number.
+		 * Bit 7 must be set (chip manual says it's 0 and reserved).
+		 */
+		max9286_write(priv, 0x15, 0x80 | MAX9286_VCTYPE |
+			      MAX9286_CSIOUTEN | MAX9286_0X15_RESV);
+	} else {
+		max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
+
+		/* Stop all cameras. */
+		for_each_source(priv, source)
+			v4l2_subdev_call(source->sd, video, s_stream, 0);
+
+		/* FIXME: See note in max9286_i2c_mux_select() */
+		priv->streaming = 0;
+	}
+
+	return 0;
+}
+
+static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_pad_config *cfg,
+				  struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->pad || code->index > 0)
+		return -EINVAL;
+
+	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
+
+	return 0;
+}
+
+static struct v4l2_mbus_framefmt *
+max9286_get_pad_format(struct max9286_priv *priv,
+		       struct v4l2_subdev_pad_config *cfg,
+		       unsigned int pad, u32 which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_format(&priv->sd, cfg, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &priv->fmt[pad];
+	default:
+		return NULL;
+	}
+}
+
+static int max9286_set_fmt(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_pad_config *cfg,
+			   struct v4l2_subdev_format *format)
+{
+	struct max9286_priv *priv = sd_to_max9286(sd);
+	struct v4l2_mbus_framefmt *cfg_fmt;
+
+	if (format->pad >= MAX9286_SRC_PAD)
+		return -EINVAL;
+
+	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
+	switch (format->format.code) {
+	case MEDIA_BUS_FMT_UYVY8_2X8:
+	case MEDIA_BUS_FMT_VYUY8_2X8:
+	case MEDIA_BUS_FMT_YUYV8_2X8:
+	case MEDIA_BUS_FMT_YVYU8_2X8:
+		break;
+	default:
+		format->format.code = MEDIA_BUS_FMT_YUYV8_2X8;
+		break;
+	}
+
+	cfg_fmt = max9286_get_pad_format(priv, cfg, format->pad, format->which);
+	if (!cfg_fmt)
+		return -EINVAL;
+
+	*cfg_fmt = format->format;
+
+	return 0;
+}
+
+static int max9286_get_fmt(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_pad_config *cfg,
+			   struct v4l2_subdev_format *format)
+{
+	struct max9286_priv *priv = sd_to_max9286(sd);
+	struct v4l2_mbus_framefmt *cfg_fmt;
+
+	if (format->pad >= MAX9286_SRC_PAD)
+		return -EINVAL;
+
+	cfg_fmt = max9286_get_pad_format(priv, cfg, format->pad, format->which);
+	if (!cfg_fmt)
+		return -EINVAL;
+
+	format->format = *cfg_fmt;
+
+	return 0;
+}
+
+static const struct v4l2_subdev_video_ops max9286_video_ops = {
+	.s_stream	= max9286_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
+	.enum_mbus_code = max9286_enum_mbus_code,
+	.get_fmt	= max9286_get_fmt,
+	.set_fmt	= max9286_set_fmt,
+};
+
+static const struct v4l2_subdev_ops max9286_subdev_ops = {
+	.video		= &max9286_video_ops,
+	.pad		= &max9286_pad_ops,
+};
+
+static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
+{
+	fmt->width		= 1280;
+	fmt->height		= 800;
+	fmt->code		= MEDIA_BUS_FMT_UYVY8_2X8;
+	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
+	fmt->field		= V4L2_FIELD_NONE;
+	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
+	fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
+	fmt->xfer_func		= V4L2_XFER_FUNC_DEFAULT;
+}
+
+static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
+{
+	struct v4l2_mbus_framefmt *format;
+	unsigned int i;
+
+	for (i = 0; i < MAX9286_N_SINKS; i++) {
+		format = v4l2_subdev_get_try_format(subdev, fh->pad, i);
+		max9286_init_format(format);
+	}
+
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
+	.open = max9286_open,
+};
+
+/* -----------------------------------------------------------------------------
+ * Probe/Remove
+ */
+
+static int max9286_setup(struct max9286_priv *priv)
+{
+	/*
+	 * Link ordering values for all enabled links combinations. Orders must
+	 * be assigned sequentially from 0 to the number of enabled links
+	 * without leaving any hole for disabled links. We thus assign orders to
+	 * enabled links first, and use the remaining order values for disabled
+	 * links are all links must have a different order value;
+	 */
+	static const u8 link_order[] = {
+		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxxx */
+		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxx0 */
+		(3 << 6) | (2 << 4) | (0 << 2) | (1 << 0), /* xx0x */
+		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xx10 */
+		(3 << 6) | (0 << 4) | (2 << 2) | (1 << 0), /* x0xx */
+		(3 << 6) | (1 << 4) | (2 << 2) | (0 << 0), /* x1x0 */
+		(3 << 6) | (1 << 4) | (0 << 2) | (2 << 0), /* x10x */
+		(3 << 6) | (1 << 4) | (1 << 2) | (0 << 0), /* x210 */
+		(0 << 6) | (3 << 4) | (2 << 2) | (1 << 0), /* 0xxx */
+		(1 << 6) | (3 << 4) | (2 << 2) | (0 << 0), /* 1xx0 */
+		(1 << 6) | (3 << 4) | (0 << 2) | (2 << 0), /* 1x0x */
+		(2 << 6) | (3 << 4) | (1 << 2) | (0 << 0), /* 2x10 */
+		(1 << 6) | (0 << 4) | (3 << 2) | (2 << 0), /* 10xx */
+		(2 << 6) | (1 << 4) | (3 << 2) | (0 << 0), /* 21x0 */
+		(2 << 6) | (1 << 4) | (0 << 2) | (3 << 0), /* 210x */
+		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* 3210 */
+	};
+
+	/*
+	 * Set the I2C bus speed.
+	 *
+	 * Enable I2C Local Acknowledge during the probe sequences of the camera
+	 * only. This should be disabled after the mux is initialised.
+	 */
+	max9286_configure_i2c(priv, true);
+
+	/*
+	 * Reverse channel setup.
+	 *
+	 * - Enable custom reverse channel configuration (through register 0x3f)
+	 *   and set the first pulse length to 35 clock cycles.
+	 * - Increase the reverse channel amplitude to 170mV to accommodate the
+	 *   high threshold enabled by the serializer driver.
+	 */
+	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
+	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
+		      MAX9286_REV_AMP_X);
+
+	/*
+	 * Enable GMSL links, mask unused ones and autodetect link
+	 * used as CSI clock source.
+	 */
+	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
+	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
+	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
+
+	/*
+	 * Video format setup:
+	 * Disable CSI output, VC is set accordingly to Link number.
+	 */
+	max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
+
+	/* Enable CSI-2 Lane D0-D3 only, DBL mode, YUV422 8-bit. */
+	max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
+		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
+		      MAX9286_DATATYPE_YUV422_8BIT);
+
+	/* Automatic: FRAMESYNC taken from the slowest Link. */
+	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
+		      MAX9286_FSYNCMETH_AUTO);
+
+	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
+	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
+		      MAX9286_HVSRC_D14);
+
+	/*
+	 * Wait for 2ms to allow the link to resynchronize after the
+	 * configuration change.
+	 */
+	usleep_range(2000, 5000);
+
+	return 0;
+}
+
+static const struct of_device_id max9286_dt_ids[] = {
+	{ .compatible = "maxim,max9286" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, max9286_dt_ids);
+
+static int max9286_init(struct device *dev)
+{
+	struct max9286_priv *priv;
+	struct i2c_client *client;
+	struct fwnode_handle *ep;
+	unsigned int i;
+	int ret;
+
+	/* Skip non-max9286 devices. */
+	if (!dev->of_node || !of_match_node(max9286_dt_ids, dev->of_node))
+		return 0;
+
+	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");
+		return ret;
+	}
+
+	priv->poc_enabled = true;
+
+	ret = max9286_setup(priv);
+	if (ret) {
+		dev_err(dev, "Unable to setup max9286\n");
+		goto err_regulator;
+	}
+
+	v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops);
+	priv->sd.internal_ops = &max9286_subdev_internal_ops;
+	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+	v4l2_ctrl_handler_init(&priv->ctrls, 1);
+	/*
+	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
+	 * hardcoded frequency in the BSP CSI-2 receiver driver.
+	 */
+	v4l2_ctrl_new_std(&priv->ctrls, NULL, V4L2_CID_PIXEL_RATE,
+			  50000000, 50000000, 1, 50000000);
+	priv->sd.ctrl_handler = &priv->ctrls;
+	ret = priv->ctrls.error;
+	if (ret)
+		goto err_regulator;
+
+	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+
+	priv->pads[MAX9286_SRC_PAD].flags = MEDIA_PAD_FL_SOURCE;
+	for (i = 0; i < MAX9286_SRC_PAD; i++)
+		priv->pads[i].flags = MEDIA_PAD_FL_SINK;
+	ret = media_entity_pads_init(&priv->sd.entity, MAX9286_N_PADS,
+				     priv->pads);
+	if (ret)
+		goto err_regulator;
+
+	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), MAX9286_SRC_PAD,
+					     0, 0);
+	if (!ep) {
+		dev_err(dev, "Unable to retrieve endpoint on \"port@4\"\n");
+		ret = -ENOENT;
+		goto err_regulator;
+	}
+	priv->sd.fwnode = ep;
+
+	ret = v4l2_async_register_subdev(&priv->sd);
+	if (ret < 0) {
+		dev_err(dev, "Unable to register subdevice\n");
+		goto err_put_node;
+	}
+
+	ret = max9286_i2c_mux_init(priv);
+	if (ret) {
+		dev_err(dev, "Unable to initialize I2C multiplexer\n");
+		goto err_subdev_unregister;
+	}
+
+	/*
+	 * Re-configure I2C with local acknowledge disabled after cameras
+	 * have probed.
+	 */
+	max9286_configure_i2c(priv, false);
+
+	/* Leave the mux channels disabled until they are selected. */
+	max9286_i2c_mux_close(priv);
+
+	return 0;
+
+err_subdev_unregister:
+	v4l2_async_unregister_subdev(&priv->sd);
+	max9286_i2c_mux_close(priv);
+err_put_node:
+	fwnode_handle_put(ep);
+err_regulator:
+	regulator_disable(priv->regulator);
+	priv->poc_enabled = false;
+
+	return ret;
+}
+
+static struct device_node *max9286_get_i2c_by_id(struct device_node *parent,
+						 u32 id)
+{
+	struct device_node *child;
+
+	for_each_child_of_node(parent, child) {
+		u32 i2c_id = 0;
+
+		if (of_node_cmp(child->name, "i2c") != 0)
+			continue;
+		of_property_read_u32(child, "reg", &i2c_id);
+		if (id == i2c_id)
+			return child;
+	}
+
+	return NULL;
+}
+
+static int max9286_check_i2c_bus_by_id(struct device *dev, int id)
+{
+	struct device_node *i2c_np;
+
+	i2c_np = max9286_get_i2c_by_id(dev->of_node, id);
+	if (!i2c_np) {
+		dev_err(dev, "Failed to find corresponding i2c@%u\n", id);
+		return -ENODEV;
+	}
+
+	if (!of_device_is_available(i2c_np)) {
+		dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
+		of_node_put(i2c_np);
+		return -ENODEV;
+	}
+
+	of_node_put(i2c_np);
+
+	return 0;
+}
+
+static void max9286_cleanup_dt(struct max9286_priv *priv)
+{
+	struct max9286_source *source;
+
+	/*
+	 * Not strictly part of the DT, but the notifier is registered during
+	 * max9286_parse_dt(), and the notifier holds references to the fwnodes
+	 * thus the cleanup is here to mirror the registration.
+	 */
+	v4l2_async_notifier_unregister(&priv->notifier);
+	v4l2_async_notifier_cleanup(&priv->notifier);
+
+	for_each_source(priv, source) {
+		fwnode_handle_put(source->fwnode);
+		source->fwnode = NULL;
+	}
+}
+
+static int max9286_parse_dt(struct max9286_priv *priv)
+{
+	struct device *dev = &priv->client->dev;
+	struct device_node *ep_np = NULL;
+	int ret;
+
+	v4l2_async_notifier_init(&priv->notifier);
+
+	for_each_endpoint_of_node(dev->of_node, ep_np) {
+		struct max9286_source *source;
+		struct of_endpoint ep;
+
+		of_graph_parse_endpoint(ep_np, &ep);
+		dev_dbg(dev, "Endpoint %pOF on port %d",
+			ep.local_node, ep.port);
+
+		if (ep.port > MAX9286_NUM_GMSL) {
+			dev_err(dev, "Invalid endpoint %s on port %d",
+				of_node_full_name(ep.local_node), ep.port);
+			continue;
+		}
+
+		/* For the source endpoint just parse the bus configuration. */
+		if (ep.port == MAX9286_SRC_PAD) {
+			struct v4l2_fwnode_endpoint vep = { .bus_type = 0 };
+			int ret;
+
+			ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_np),
+							 &vep);
+			if (ret)
+				return ret;
+
+			if (vep.bus_type != V4L2_MBUS_CSI2_DPHY) {
+				dev_err(dev,
+					"Media bus %u type not supported\n",
+					vep.bus_type);
+				v4l2_fwnode_endpoint_free(&vep);
+				return -EINVAL;
+			}
+
+			priv->csi2_data_lanes =
+				vep.bus.mipi_csi2.num_data_lanes;
+			v4l2_fwnode_endpoint_free(&vep);
+
+			continue;
+		}
+
+		/* Skip if the corresponding GMSL link is unavailable. */
+		if (max9286_check_i2c_bus_by_id(dev, ep.port))
+			continue;
+
+		if (priv->sources[ep.port].fwnode) {
+			dev_err(dev,
+				"Multiple port endpoints are not supported: %d",
+				ep.port);
+
+			continue;
+		}
+
+		source = &priv->sources[ep.port];
+		source->fwnode = fwnode_graph_get_remote_endpoint(
+						of_fwnode_handle(ep_np));
+		if (!source->fwnode) {
+			dev_err(dev,
+				"Endpoint %pOF has no remote endpoint connection\n",
+				ep.local_node);
+
+			continue;
+		}
+
+		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+		source->asd.match.fwnode = source->fwnode;
+
+		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
+						     &source->asd);
+		if (ret) {
+			v4l2_async_notifier_cleanup(&priv->notifier);
+			return ret;
+		}
+
+		priv->source_mask |= BIT(ep.port);
+		priv->nsources++;
+	}
+
+	/* Do not register the subdev notifier if there are no devices. */
+	if (!priv->nsources)
+		return 0;
+
+	priv->route_mask = priv->source_mask;
+	priv->notifier.ops = &max9286_notify_ops;
+
+	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
+	if (ret)
+		v4l2_async_notifier_cleanup(&priv->notifier);
+
+	return 0;
+}
+
+static int max9286_probe(struct i2c_client *client,
+			 const struct i2c_device_id *did)
+{
+	struct max9286_priv *priv;
+	unsigned int i;
+	int ret;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+	i2c_set_clientdata(client, priv);
+
+	for (i = 0; i < MAX9286_N_SINKS; i++)
+		max9286_init_format(&priv->fmt[i]);
+
+	ret = max9286_parse_dt(priv);
+	if (ret)
+		return ret;
+
+	priv->regulator = 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_free;
+	}
+
+	/*
+	 * We can have multiple MAX9286 instances on the same physical I2C
+	 * bus, and I2C children behind ports of separate MAX9286 instances
+	 * having the same I2C address. As the MAX9286 starts by default with
+	 * all ports enabled, we need to disable all ports on all MAX9286
+	 * instances before proceeding to further initialize the devices and
+	 * instantiate children.
+	 *
+	 * Start by just disabling all channels on the current device. Then,
+	 * if all other MAX9286 on the parent bus have been probed, proceed
+	 * to initialize them all, including the current one.
+	 */
+	max9286_i2c_mux_close(priv);
+
+	/*
+	 * The MAX9286 initialises with auto-acknowledge enabled by default.
+	 * This means that if multiple MAX9286 devices are connected to an I2C
+	 * bus, another MAX9286 could ack I2C transfers meant for a device on
+	 * the other side of the GMSL links for this MAX9286 (such as a
+	 * MAX9271). To prevent that disable auto-acknowledge early on; it
+	 * will be enabled later as needed.
+	 */
+	max9286_configure_i2c(priv, false);
+
+	ret = max9286_init(&client->dev);
+	if (ret < 0)
+		goto err_regulator;
+
+	return 0;
+
+err_regulator:
+	regulator_put(priv->regulator);
+	max9286_i2c_mux_close(priv);
+	max9286_configure_i2c(priv, false);
+err_free:
+	max9286_cleanup_dt(priv);
+	kfree(priv);
+
+	return ret;
+}
+
+static int max9286_remove(struct i2c_client *client)
+{
+	struct max9286_priv *priv = i2c_get_clientdata(client);
+
+	i2c_mux_del_adapters(priv->mux);
+
+	fwnode_handle_put(priv->sd.fwnode);
+	v4l2_async_unregister_subdev(&priv->sd);
+
+	if (priv->poc_enabled)
+		regulator_disable(priv->regulator);
+	regulator_put(priv->regulator);
+
+	max9286_cleanup_dt(priv);
+	kfree(priv);
+
+	return 0;
+}
+
+static const struct i2c_device_id max9286_id[] = {
+	{ "max9286", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max9286_id);
+
+static struct i2c_driver max9286_i2c_driver = {
+	.driver	= {
+		.name		= "max9286",
+		.of_match_table	= of_match_ptr(max9286_dt_ids),
+	},
+	.probe		= max9286_probe,
+	.remove		= max9286_remove,
+	.id_table	= max9286_id,
+};
+
+module_i2c_driver(max9286_i2c_driver);
+
+MODULE_DESCRIPTION("Maxim MAX9286 GMSL Deserializer Driver");
+MODULE_AUTHOR("Jacopo Mondi, Kieran Bingham, Laurent Pinchart, Niklas Söderlund, Vladimir Barinov");
+MODULE_LICENSE("GPL");
-- 
2.23.0

[RFT 3/4] arm64: dts: renesas: Add Maxim GMSL expansion board

[Jacopo Mondi]

Add Maxim MAX9286 expansion board for Salvator-X Renesas board.

This patch updates the salvator-x-max9286.dtsi to comply with the newly
defined yaml bindings and its based on Kieran's
"arm64: dts: renesas: salvator-x: Add MAX9286 expansion board" from
git://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git#gmsl-v5

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../boot/dts/renesas/salvator-x-max9286.dtsi  | 394 ++++++++++++++++++
 1 file changed, 394 insertions(+)
 create mode 100644 arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi

diff --git a/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi b/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
new file mode 100644
index 000000000000..d9b3edb4ed80
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
@@ -0,0 +1,394 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Device Tree Source for the Salvator-X MAX9286 expansion board
+ *
+ * Copyright (C) 2017 Ideas on Board <laurent.pinchart@ideasonboard.com>
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+
+/*
+ * MAX9286 A
+ */
+#define MAXIM_CAMERA0 "imi,rdacm20"
+#define MAXIM_CAMERA1 "imi,rdacm20"
+#define MAXIM_CAMERA2 "imi,rdacm20"
+#define MAXIM_CAMERA3 "imi,rdacm20"
+
+/*
+ * MAX9286 B
+ */
+//#define MAXIM_CAMERA4 "imi,rdacm20"
+//#define MAXIM_CAMERA5 "imi,rdacm20"
+//#define MAXIM_CAMERA6 "imi,rdacm20"
+//#define MAXIM_CAMERA7 "imi,rdacm20"
+
+/ {
+/*
+ * Powered MCU IMI cameras need delay between power-on and R-Car access
+ * to avoid I2C bus conflicts since the R-Car I2C does not support I2C
+ * multi-master. The I2C bus conflict would result in R-Car I2C IP stall.
+ */
+#define IMI_MCU_V0_DELAY	8000000	/* delay for powered MCU firmware v0 */
+#define IMI_MCU_V1_DELAY	3000000	/* delay for powered MCU firmware v1 */
+#define IMI_MCU_NO_DELAY	0	/* delay for unpowered MCU  */
+#define IMI_MCU_DELAY		IMI_MCU_V0_DELAY
+
+	poc_12v: regulator-vcc-poc-12v {
+		compatible = "regulator-fixed";
+
+		regulator-name = "Camera PoC 12V";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		startup-delay-us = <(250000 + IMI_MCU_DELAY)>;
+
+		gpio = <&gpio6 30 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
+};
+
+&vin0 {
+	status = "okay";
+};
+
+&vin1 {
+	status = "okay";
+};
+
+&vin2 {
+	status = "okay";
+};
+
+&vin3 {
+	status = "okay";
+};
+
+&vin4 {
+	status = "okay";
+};
+
+&vin5 {
+	status = "okay";
+};
+
+&vin6 {
+	status = "okay";
+};
+
+&vin7 {
+	status = "okay";
+};
+
+/* Disconnect the csi40 endpoint from the ADV748x TXA (HDMI) */
+&adv7482_txa {
+	/delete-property/ remote-endpoint;
+	status = "disabled";
+};
+
+&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>;
+			};
+		};
+	};
+};
+
+&csi41 {
+	status = "okay";
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			csi41_in: endpoint {
+				clock-lanes = <0>;
+				data-lanes = <1 2 3 4>;
+				remote-endpoint = <&max9286_out1>;
+			};
+		};
+	};
+};
+
+&i2c4 {
+	gmsl-deserializer@4c {
+		compatible = "maxim,max9286";
+		reg = <0x4c>;
+		poc-supply = <&poc_12v>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				max9286_in0: endpoint {
+#ifdef MAXIM_CAMERA0
+					remote-endpoint = <&rdacm20_out0>;
+#endif
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				max9286_in1: endpoint {
+#ifdef MAXIM_CAMERA1
+					remote-endpoint = <&rdacm20_out1>;
+#endif
+				};
+			};
+
+			port@2 {
+				reg = <2>;
+				max9286_in2: endpoint {
+#ifdef MAXIM_CAMERA2
+					remote-endpoint = <&rdacm20_out2>;
+#endif
+				};
+			};
+
+			port@3 {
+				reg = <3>;
+				max9286_in3: endpoint {
+#ifdef MAXIM_CAMERA3
+					remote-endpoint = <&rdacm20_out3>;
+#endif
+				};
+			};
+
+			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>;
+
+#ifdef MAXIM_CAMERA0
+				camera@31 {
+					compatible = MAXIM_CAMERA0;
+					reg = <0x31 0x41 0x51>;
+
+					port {
+						rdacm20_out0: endpoint {
+							remote-endpoint = <&max9286_in0>;
+						};
+					};
+
+				};
+#endif
+			};
+
+			i2c@1 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <1>;
+
+#ifdef MAXIM_CAMERA1
+				camera@32 {
+					compatible = MAXIM_CAMERA1;
+					reg = <0x32 0x42 0x52>;
+					port {
+						rdacm20_out1: endpoint {
+							remote-endpoint = <&max9286_in1>;
+						};
+					};
+				};
+#endif
+			};
+
+			i2c@2 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <2>;
+
+#ifdef MAXIM_CAMERA2
+				camera@33 {
+					compatible = MAXIM_CAMERA2;
+					reg = <0x33 0x43 0x53>;
+					port {
+						rdacm20_out2: endpoint {
+							remote-endpoint = <&max9286_in2>;
+						};
+					};
+				};
+#endif
+			};
+
+			i2c@3 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <3>;
+
+#ifdef MAXIM_CAMERA3
+				camera@34 {
+					compatible = MAXIM_CAMERA3;
+					reg = <0x34 0x44 0x54>;
+					port {
+						rdacm20_out3: endpoint {
+							remote-endpoint = <&max9286_in3>;
+						};
+					};
+				};
+#endif
+			};
+		};
+	};
+
+	gmsl-deserializer@6c {
+		compatible = "maxim,max9286";
+		reg = <0x6c>;
+		poc-supply = <&poc_12v>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				max9286_in4: endpoint {
+#ifdef MAXIM_CAMERA4
+					remote-endpoint = <&rdacm20_out4>;
+#endif
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				max9286_in5: endpoint {
+#ifdef MAXIM_CAMERA5
+					remote-endpoint = <&rdacm20_out5>;
+#endif
+				};
+			};
+
+			port@2 {
+				reg = <2>;
+				max9286_in6: endpoint {
+#ifdef MAXIM_CAMERA6
+					remote-endpoint = <&rdacm20_out6>;
+#endif
+				};
+			};
+
+			port@3 {
+				reg = <3>;
+				max9286_in7: endpoint {
+#ifdef MAXIM_CAMERA7
+					remote-endpoint = <&rdacm20_out7>;
+#endif
+				};
+			};
+
+			port@4 {
+				reg = <4>;
+				max9286_out1: endpoint {
+					clock-lanes = <0>;
+					data-lanes = <1 2 3 4>;
+					remote-endpoint = <&csi41_in>;
+				};
+			};
+		};
+
+		i2c-mux {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			i2c@0 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0>;
+
+#ifdef MAXIM_CAMERA4
+				camera@35 {
+					compatible = MAXIM_CAMERA4;
+					reg = <0x35 0x45 0x55>;
+					port {
+						rdacm20_out4: endpoint {
+							remote-endpoint = <&max9286_in4>;
+						};
+					};
+				};
+#endif
+			};
+
+			i2c@1 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <1>;
+
+#ifdef MAXIM_CAMERA5
+				camera@36 {
+					compatible = MAXIM_CAMERA5;
+					reg = <0x36 0x46 0x56>;
+					port {
+						rdacm20_out5: endpoint {
+							remote-endpoint = <&max9286_in5>;
+						};
+					};
+				};
+#endif
+			};
+
+			i2c@2 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <2>;
+
+#ifdef MAXIM_CAMERA6
+				camera@37 {
+					compatible = MAXIM_CAMERA6;
+					reg = <0x37 0x47 0x57>;
+					port {
+						rdacm20_out6: endpoint {
+							remote-endpoint = <&max9286_in6>;
+						};
+					};
+				};
+#endif
+			};
+
+			i2c@3 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <3>;
+
+#ifdef MAXIM_CAMERA7
+				camera@38 {
+					compatible = MAXIM_CAMERA7;
+					reg = <0x38 0x48 0x58>;
+					port {
+						rdacm20_out7: endpoint {
+							remote-endpoint = <&max9286_in7>;
+						};
+					};
+				};
+#endif
+			};
+		};
+	};
+};
-- 
2.23.0

[RFT 4/4] arm64: dts: renesas: r8a7796=salvator-x: Include GMSL

[Jacopo Mondi]

Include the Maxim GMSL expansion board DTSi for M3-W Salvator-x board.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
index 72874f675359..3f523c52c2b8 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
@@ -155,3 +155,4 @@
 
 	status = "okay";
 };
+#include "salvator-x-max9286.dtsi"
-- 
2.23.0

Re: [RFT 4/4] arm64: dts: renesas: r8a7796=salvator-x: Include GMSL

[Geert Uytterhoeven]

Hi Jacopo,

On Sat, Nov 16, 2019 at 5:56 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Include the Maxim GMSL expansion board DTSi for M3-W Salvator-x board.

This and subject says M3-W...

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts | 1 +

... but this is for the (rare) combination of Salvator-X and H3 ES2.0?

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

Re: [RFT 0/4] GMSL Refresh (would be v6)

[Kieran Bingham]

Hi Jacopo,

Thanks for this refresh. The key part here is the refresh of the
V4L2-Mux series, which is what had previously blocked the GMSL series.

Will you post/publish the vl42-mux series on the linux-media mailinglist?

I know there are other interested parties who are looking at such topics
- so highlighting the latest version might be beneficial to promoting
discussions.


On 16/11/2019 16:50, Jacopo Mondi wrote:
> Hello this is refersh of our GMSL work.
> 
> Current situation is the following:
> - Kieran sent a full v4 with multiplexed stream support and has a v5 branch in
>   his repository with v4 review comment fixes on top
> 
> 	I rebased the multiplexed stream's series on latest media-master
> 	https://jmondi.org/cgit/linux/log/?h=v4l2-mux/media-master/v6
> 
> 	On top of that I took Kieran's v5 and re-applied on top:
> 	https://jmondi.org/cgit/linux/log/?h=jmondi/gmsl/kieran/v6
> 
> - Niklas sent a v1 (which sould have been a v5) which removes multiplexed
>   streams and only support one camera and was meant for inclusion but is still
>   floating around in linux-media, mostly because some of the comments on
>   Kieran's v4 still applied there, if I'm not mistaken.
> 
> 	I took Niklas' single stream max9286 and replaced the original
> 	bindings with a json-schema version
> 	https://jmondi.org/cgit/linux/log/?h=jmondi/gmsl/niklas/v6
> 
> I bumped all versions to v6 to avoid further confusion.
> 
> Not having a working GMSL setup I would ask to Kieran or Niklas to test this
> version so that we can try re-send the single stream max9286 version with new
> yaml bindings for inclusion.

Thanks, I can confirm that the rebase to current master was successful
(based on your branch with the version of patches based on my gmsl/v5)


As we now have two forks of the GMSL I'm going to rebase these such that
the separation between current topics is clear:

 - MAX9286 with support for a single camera (and only a single MAX9286)
	- This we could/should hope to get upstream
 - MAX9286 VC support
	- (requires the V4L2-Mux support of course)
 - MAX9286 dual device workaround (not suitable for upstream currently)
	- Required to function on the Salvator-XS GMSL board.

Once I've done (and tested this) I'll make a new posting (should we call
this v6? or v6.1?)


In the meantime, I will not be making changes to the RDACM20, so if you
wish to get started investigating the separation topic here - then I
don't think you are blocked on me.

--
Regards

Kieran




> (I kept linux-media e devicetree out as I would like to test the patches before
> expanding the receivers list)
> 
> Thanks
>    j
> 
> Jacopo Mondi (2):
>   arm64: dts: renesas: Add Maxim GMSL expansion board
>   arm64: dts: renesas: r8a7796=salvator-x: Include GMSL
> 
> Laurent Pinchart (1):
>   dt-bindings: media: i2c: Add bindings for Maxim Integrated MAX9286
> 
> Niklas Söderlund (1):
>   max9286: Add MAX9286 driver
> 
>  .../bindings/media/i2c/maxim,max9286.yaml     |  286 +++++
>  MAINTAINERS                                   |   10 +
>  .../boot/dts/renesas/r8a7795-salvator-x.dts   |    1 +
>  .../boot/dts/renesas/salvator-x-max9286.dtsi  |  394 ++++++
>  drivers/media/i2c/Kconfig                     |   11 +
>  drivers/media/i2c/Makefile                    |    1 +
>  drivers/media/i2c/max9286.c                   | 1081 +++++++++++++++++
>  7 files changed, 1784 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
>  create mode 100644 arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
>  create mode 100644 drivers/media/i2c/max9286.c
> 
> --
> 2.23.0
> 

Re: [RFT 0/4] GMSL Refresh (would be v6)

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 4902 bytes --]

Hi Kieran,

On Wed, Nov 20, 2019 at 04:33:15PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> Thanks for this refresh. The key part here is the refresh of the
> V4L2-Mux series, which is what had previously blocked the GMSL series.
>
> Will you post/publish the vl42-mux series on the linux-media mailinglist?

I considered that, but we already know that series is not meant to go
far, so I could just as reference...

>
> I know there are other interested parties who are looking at such topics
> - so highlighting the latest version might be beneficial to promoting
> discussions.
>

Might be, yes...

>
> On 16/11/2019 16:50, Jacopo Mondi wrote:
> > Hello this is refersh of our GMSL work.
> >
> > Current situation is the following:
> > - Kieran sent a full v4 with multiplexed stream support and has a v5 branch in
> >   his repository with v4 review comment fixes on top
> >
> > 	I rebased the multiplexed stream's series on latest media-master
> > 	https://jmondi.org/cgit/linux/log/?h=v4l2-mux/media-master/v6
> >
> > 	On top of that I took Kieran's v5 and re-applied on top:
> > 	https://jmondi.org/cgit/linux/log/?h=jmondi/gmsl/kieran/v6
> >
> > - Niklas sent a v1 (which sould have been a v5) which removes multiplexed
> >   streams and only support one camera and was meant for inclusion but is still
> >   floating around in linux-media, mostly because some of the comments on
> >   Kieran's v4 still applied there, if I'm not mistaken.
> >
> > 	I took Niklas' single stream max9286 and replaced the original
> > 	bindings with a json-schema version
> > 	https://jmondi.org/cgit/linux/log/?h=jmondi/gmsl/niklas/v6
> >
> > I bumped all versions to v6 to avoid further confusion.
> >
> > Not having a working GMSL setup I would ask to Kieran or Niklas to test this
> > version so that we can try re-send the single stream max9286 version with new
> > yaml bindings for inclusion.
>
> Thanks, I can confirm that the rebase to current master was successful
> (based on your branch with the version of patches based on my gmsl/v5)
>

Great! Thanks for testing!

>
> As we now have two forks of the GMSL I'm going to rebase these such that
> the separation between current topics is clear:
>
>  - MAX9286 with support for a single camera (and only a single MAX9286)
> 	- This we could/should hope to get upstream
>  - MAX9286 VC support
> 	- (requires the V4L2-Mux support of course)
>  - MAX9286 dual device workaround (not suitable for upstream currently)
> 	- Required to function on the Salvator-XS GMSL board.
>
> Once I've done (and tested this) I'll make a new posting (should we call
> this v6? or v6.1?)

I think you can keep v6, as this one is RFT..

>
>
> In the meantime, I will not be making changes to the RDACM20, so if you
> wish to get started investigating the separation topic here - then I
> don't think you are blocked on me.
>

I think this will need a bit more thoughts... Separating max9271 and
ov10653 might be trivial, as the only action the current rdacm20
driver does on the sensor is an initial configuration with a set of
register-value entries. It's a bit more work to properly define the
data connection in DT and setting up a sub-notifier in the serializer
driver, but that's totally feasible...

Although, as Laurent pointed out we have a uController in the camera
module, regulators (iirc) etc, and so it makes sense to keep a single
rdacm20 driver, breaking out the serializer driver code into a
re-usable library instead of making a proper driver out of it.

What's your opinion on this ?

Thanks
  j

> --
> Regards
>
> Kieran
>
>
>
>
> > (I kept linux-media e devicetree out as I would like to test the patches before
> > expanding the receivers list)
> >
> > Thanks
> >    j
> >
> > Jacopo Mondi (2):
> >   arm64: dts: renesas: Add Maxim GMSL expansion board
> >   arm64: dts: renesas: r8a7796=salvator-x: Include GMSL
> >
> > Laurent Pinchart (1):
> >   dt-bindings: media: i2c: Add bindings for Maxim Integrated MAX9286
> >
> > Niklas Söderlund (1):
> >   max9286: Add MAX9286 driver
> >
> >  .../bindings/media/i2c/maxim,max9286.yaml     |  286 +++++
> >  MAINTAINERS                                   |   10 +
> >  .../boot/dts/renesas/r8a7795-salvator-x.dts   |    1 +
> >  .../boot/dts/renesas/salvator-x-max9286.dtsi  |  394 ++++++
> >  drivers/media/i2c/Kconfig                     |   11 +
> >  drivers/media/i2c/Makefile                    |    1 +
> >  drivers/media/i2c/max9286.c                   | 1081 +++++++++++++++++
> >  7 files changed, 1784 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> >  create mode 100644 arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
> >  create mode 100644 drivers/media/i2c/max9286.c
> >
> > --
> > 2.23.0
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] max9286: simplify i2c-mux parsing

[Kieran Bingham]

 - Identify each enabled i2c-mux channel in a single pass

The parse_dt function iterates each node in the i2c-mux for each endpoint looking for a match.
The only purpose of these iterations is to determine if the corresponding i2c-channel
is enabled. (status = 'okay').

Iterate the i2c-mux nodes in a single pass storing the enable state
in a local i2c_mux_mask for use when parsing the endpoints.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 84 +++++++++++++++----------------------
 1 file changed, 34 insertions(+), 50 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 34cb6f3b40c2..a36132becdc7 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1097,55 +1097,6 @@ static int max9286_is_bound(struct device *dev, void *data)
 	return 0;
 }
 
-static struct device_node *max9286_get_i2c_by_id(struct device_node *parent,
-						 u32 id)
-{
-	struct device_node *i2c_mux;
-	struct device_node *child;
-
-	/* Balance the of_node_put() performed by of_find_node_by_name() */
-	of_node_get(parent);
-
-	i2c_mux = of_find_node_by_name(parent, "i2c-mux");
-	if (!i2c_mux) {
-		printk("max9286: Failed to find i2c-mux node\n");
-		return NULL;
-	}
-
-	for_each_child_of_node(i2c_mux, child) {
-		u32 i2c_id = 0;
-
-		if (of_node_cmp(child->name, "i2c") != 0)
-			continue;
-		of_property_read_u32(child, "reg", &i2c_id);
-		if (id == i2c_id)
-			return child;
-	}
-
-	return NULL;
-}
-
-static int max9286_check_i2c_bus_by_id(struct device *dev, int id)
-{
-	struct device_node *i2c_np;
-
-	i2c_np = max9286_get_i2c_by_id(dev->of_node, id);
-	if (!i2c_np) {
-		dev_err(dev, "Failed to find corresponding i2c@%u\n", id);
-		return -ENODEV;
-	}
-
-	if (!of_device_is_available(i2c_np)) {
-		dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
-		of_node_put(i2c_np);
-		return -ENODEV;
-	}
-
-	of_node_put(i2c_np);
-
-	return 0;
-}
-
 static void max9286_cleanup_dt(struct max9286_priv *priv)
 {
 	struct max9286_source *source;
@@ -1167,11 +1118,44 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
 static int max9286_parse_dt(struct max9286_priv *priv)
 {
 	struct device *dev = &priv->client->dev;
+	struct device_node *i2c_mux;
+	struct device_node *child = NULL;
 	struct device_node *ep_np = NULL;
+	unsigned int i2c_mux_mask = 0;
 	int ret;
 
+	/* Balance the of_node_put() performed by of_find_node_by_name() */
+	of_node_get(dev->of_node);
+	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
+	if (!i2c_mux) {
+		dev_err(dev, "Failed to find i2c-mux node\n");
+		return -EINVAL;
+	}
+
+	/* Identify which i2c-mux channels are enabled */
+	for_each_child_of_node(i2c_mux, child) {
+		u32 id = 0;
+
+		if (of_node_cmp(child->name, "i2c") != 0)
+			continue;
+
+		of_property_read_u32(child, "reg", &id);
+		if (id >= MAX9286_NUM_GMSL)
+			continue;
+
+		if (!of_device_is_available(child)) {
+			dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
+			continue;
+		}
+
+		i2c_mux_mask |= BIT(id);
+	}
+	of_node_put(child);
+	of_node_put(i2c_mux);
+
 	v4l2_async_notifier_init(&priv->notifier);
 
+	/* Parse the endpoints */
 	for_each_endpoint_of_node(dev->of_node, ep_np) {
 		struct max9286_source *source;
 		struct of_endpoint ep;
@@ -1214,7 +1198,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 		}
 
 		/* Skip if the corresponding GMSL link is unavailable. */
-		if (max9286_check_i2c_bus_by_id(dev, ep.port))
+		if (!(i2c_mux_mask & BIT(ep.port)))
 			continue;
 
 		if (priv->sources[ep.port].fwnode) {
-- 
2.20.1

Re: [PATCH] max9286: simplify i2c-mux parsing

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 4277 bytes --]

Hi Kieran,

On Thu, Nov 21, 2019 at 04:56:31PM +0000, Kieran Bingham wrote:
>  - Identify each enabled i2c-mux channel in a single pass
>
> The parse_dt function iterates each node in the i2c-mux for each endpoint looking for a match.
> The only purpose of these iterations is to determine if the corresponding i2c-channel
> is enabled. (status = 'okay').
>
> Iterate the i2c-mux nodes in a single pass storing the enable state
> in a local i2c_mux_mask for use when parsing the endpoints.
>

I very much agree with this :)

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 84 +++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 34cb6f3b40c2..a36132becdc7 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -1097,55 +1097,6 @@ static int max9286_is_bound(struct device *dev, void *data)
>  	return 0;
>  }
>
> -static struct device_node *max9286_get_i2c_by_id(struct device_node *parent,
> -						 u32 id)
> -{
> -	struct device_node *i2c_mux;
> -	struct device_node *child;
> -
> -	/* Balance the of_node_put() performed by of_find_node_by_name() */
> -	of_node_get(parent);
> -
> -	i2c_mux = of_find_node_by_name(parent, "i2c-mux");
> -	if (!i2c_mux) {
> -		printk("max9286: Failed to find i2c-mux node\n");
> -		return NULL;
> -	}
> -
> -	for_each_child_of_node(i2c_mux, child) {
> -		u32 i2c_id = 0;
> -
> -		if (of_node_cmp(child->name, "i2c") != 0)
> -			continue;
> -		of_property_read_u32(child, "reg", &i2c_id);
> -		if (id == i2c_id)
> -			return child;
> -	}
> -
> -	return NULL;
> -}
> -
> -static int max9286_check_i2c_bus_by_id(struct device *dev, int id)
> -{
> -	struct device_node *i2c_np;
> -
> -	i2c_np = max9286_get_i2c_by_id(dev->of_node, id);
> -	if (!i2c_np) {
> -		dev_err(dev, "Failed to find corresponding i2c@%u\n", id);
> -		return -ENODEV;
> -	}
> -
> -	if (!of_device_is_available(i2c_np)) {
> -		dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
> -		of_node_put(i2c_np);
> -		return -ENODEV;
> -	}
> -
> -	of_node_put(i2c_np);
> -
> -	return 0;
> -}
> -
>  static void max9286_cleanup_dt(struct max9286_priv *priv)
>  {
>  	struct max9286_source *source;
> @@ -1167,11 +1118,44 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
>  static int max9286_parse_dt(struct max9286_priv *priv)
>  {
>  	struct device *dev = &priv->client->dev;
> +	struct device_node *i2c_mux;
> +	struct device_node *child = NULL;
>  	struct device_node *ep_np = NULL;

Nit: could you re-use child or ep_np ?

> +	unsigned int i2c_mux_mask = 0;
>  	int ret;
>
> +	/* Balance the of_node_put() performed by of_find_node_by_name() */

Do you need this comment ?

> +	of_node_get(dev->of_node);
> +	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
> +	if (!i2c_mux) {
> +		dev_err(dev, "Failed to find i2c-mux node\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Identify which i2c-mux channels are enabled */
> +	for_each_child_of_node(i2c_mux, child) {
> +		u32 id = 0;
> +
> +		if (of_node_cmp(child->name, "i2c") != 0)
> +			continue;

With the new bindings in yaml format and the associated verification,
this should not happen.

> +
> +		of_property_read_u32(child, "reg", &id);
> +		if (id >= MAX9286_NUM_GMSL)
> +			continue;
> +
> +		if (!of_device_is_available(child)) {
> +			dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
> +			continue;
> +		}
> +
> +		i2c_mux_mask |= BIT(id);
> +	}
> +	of_node_put(child);
> +	of_node_put(i2c_mux);
> +
>  	v4l2_async_notifier_init(&priv->notifier);
>
> +	/* Parse the endpoints */
>  	for_each_endpoint_of_node(dev->of_node, ep_np) {

dev->of_node is reused here, do you need to get it again ?

All minors though, squash this on the next max9286 submission if you
feel to.

Thanks
   j

>  		struct max9286_source *source;
>  		struct of_endpoint ep;
> @@ -1214,7 +1198,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  		}
>
>  		/* Skip if the corresponding GMSL link is unavailable. */
> -		if (max9286_check_i2c_bus_by_id(dev, ep.port))
> +		if (!(i2c_mux_mask & BIT(ep.port)))
>  			continue;
>
>  		if (priv->sources[ep.port].fwnode) {
> --
> 2.20.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] max9286: Improve mux-state readbility

[Kieran Bingham]

The MAX9286 implements an I2C mux which we maintain in an open state
while we are streaming from the cameras.

The development code for the MAX9286 uses an integer value with
arbitrary state values to control these state transitions. This is
highlighted ith a FIXME and is difficult to interpret the meaning of the
values 0, 1, 2.

Introduce an enum to declare the intent of each state, and comment the
states accordingly.

This state transition is only needed for the multi-channel support, and
will not be included in the single-channel max9286 posting.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 63 +++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index eed00ff1dee4..a9c3e7411bda 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -144,7 +144,7 @@ struct max9286_priv {
 	struct media_pad pads[MAX9286_N_PADS];
 	struct regulator *regulator;
 	bool poc_enabled;
-	int streaming;
+	int mux_state;
 
 	struct i2c_mux_core *mux;
 	unsigned int mux_channel;
@@ -221,16 +221,39 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
  * I2C Multiplexer
  */
 
+enum max9286_i2c_mux_state {
+	/*
+	 * The I2C Mux will enable only a single channel (both forward, and
+	 * reverse) at a time, and to reduce I2C bus bandwidth, it will only
+	 * reconfigure the channel when a write is requested to a different
+	 * channel.
+	 */
+	MAX9286_I2C_MUX_STATE_CHANNEL = 0,
+
+	/*
+	 * The I2C mux will be configured with all ports open. All I2C writes
+	 * will be transmitted to all remote I2C devices, and where multiple
+	 * devices have the same address, the write will be received by all of
+	 * them.
+	 */
+	MAX9286_I2C_MUX_STATE_OPEN,
+
+	/*
+	 * The I2C mux is configured with all ports open.
+	 *
+	 * No reconfiguration of the I2C mux channel select is required.
+	 */
+	MAX9286_I2C_MUX_STATE_DISABLE_SELECT,
+};
+
 static int max9286_i2c_mux_close(struct max9286_priv *priv)
 {
-	/* FIXME: See note in max9286_i2c_mux_select() */
-	if (priv->streaming)
-		return 0;
 	/*
 	 * Ensure that both the forward and reverse channel are disabled on the
-	 * mux, and that the channel ID is invalidated to ensure we reconfigure
-	 * on the next select call.
+	 * mux, and that the channel state and ID is invalidated to ensure we
+	 * reconfigure on the next max9286_i2c_mux_select() call.
 	 */
+	priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;
 	priv->mux_channel = -1;
 	max9286_write(priv, 0x0a, 0x00);
 	usleep_range(3000, 5000);
@@ -242,23 +265,19 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
 {
 	struct max9286_priv *priv = i2c_mux_priv(muxc);
 
-	/*
-	 * FIXME: This state keeping is a hack and do the job. It should
-	 * be should be reworked. One option to consider is that once all
-	 * cameras are programmed the mux selection logic should be disabled
-	 * and all all reverse and forward channels enable all the time.
-	 *
-	 * In any case this logic with a int that have two states should be
-	 * reworked!
-	 */
-	if (priv->streaming == 1) {
-		max9286_write(priv, 0x0a, 0xff);
-		priv->streaming = 2;
+	/* channel select is disabled when configured in the opened state. */
+	if (priv->mux_state == MAX9286_I2C_MUX_STATE_DISABLE_SELECT)
 		return 0;
-	} else if (priv->streaming == 2) {
+
+	if (priv->mux_state == MAX9286_I2C_MUX_STATE_OPEN) {
+		/* Open all channels on the MAX9286 */
+		max9286_write(priv, 0x0a, 0xff);
+		priv->mux_state = MAX9286_I2C_MUX_STATE_DISABLE_SELECT;
 		return 0;
 	}
 
+	/* Handling for MAX9286_I2C_MUX_STATE_CHANNEL */
+
 	if (priv->mux_channel == chan)
 		return 0;
 
@@ -441,8 +460,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	int ret;
 
 	if (enable) {
-		/* FIXME: See note in max9286_i2c_mux_select() */
-		priv->streaming = 1;
+		priv->mux_state = MAX9286_I2C_MUX_STATE_OPEN;
 
 		/* Start all cameras. */
 		for_each_source(priv, source) {
@@ -490,8 +508,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		for_each_source(priv, source)
 			v4l2_subdev_call(source->sd, video, s_stream, 0);
 
-		/* FIXME: See note in max9286_i2c_mux_select() */
-		priv->streaming = 0;
+		priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;
 	}
 
 	return 0;
-- 
2.20.1

Re: [PATCH] max9286: Improve mux-state readbility

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 6221 bytes --]

Hi Kieran,

On Thu, Nov 28, 2019 at 04:27:06PM +0000, Kieran Bingham wrote:
> The MAX9286 implements an I2C mux which we maintain in an open state
> while we are streaming from the cameras.
>
> The development code for the MAX9286 uses an integer value with
> arbitrary state values to control these state transitions. This is
> highlighted ith a FIXME and is difficult to interpret the meaning of the

s/ith/with

> values 0, 1, 2.
>
> Introduce an enum to declare the intent of each state, and comment the
> states accordingly.
>
> This state transition is only needed for the multi-channel support, and
> will not be included in the single-channel max9286 posting.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 63 +++++++++++++++++++++++--------------
>  1 file changed, 40 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index eed00ff1dee4..a9c3e7411bda 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -144,7 +144,7 @@ struct max9286_priv {
>  	struct media_pad pads[MAX9286_N_PADS];
>  	struct regulator *regulator;
>  	bool poc_enabled;
> -	int streaming;
> +	int mux_state;
>
>  	struct i2c_mux_core *mux;
>  	unsigned int mux_channel;
> @@ -221,16 +221,39 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
>   * I2C Multiplexer
>   */
>
> +enum max9286_i2c_mux_state {

Let the bikeshedding begin here

> +	/*
> +	 * The I2C Mux will enable only a single channel (both forward, and

s/Mux/mux

> +	 * reverse) at a time, and to reduce I2C bus bandwidth, it will only
> +	 * reconfigure the channel when a write is requested to a different
> +	 * channel.

I won't here explain what a mux channel select does

> +	 */
> +	MAX9286_I2C_MUX_STATE_CHANNEL = 0,

To me, this should be the initial state of the mux, where all channels
are closed.

The state machine to me should look like:

        init() -> i2c_mux_close() -> mux_state = CLOSED;
        all transaction selects/deselect a single channel

        s_stream(1) -> mux_state = REQUEST_OPEN
        first transaction opens all channels -> mux_state = OPEN
        all successive transactions with (mux_state = OPEN) are nop

        s_stream(0) -> i2c_mux_close() -> mux_state = CLOSED
        all transaction selects/deselect a single channel until next s_stream(1)

For this state I propose MAX9286_I2C_MUX_STATE_CLOSED

> +
> +	/*
> +	 * The I2C mux will be configured with all ports open. All I2C writes
> +	 * will be transmitted to all remote I2C devices, and where multiple
> +	 * devices have the same address, the write will be received by all of
> +	 * them.
> +	 */
> +	MAX9286_I2C_MUX_STATE_OPEN,

I propose MAX9286_I2C_MUX_STATE_REQUEST_OPEN

> +
> +	/*
> +	 * The I2C mux is configured with all ports open.
> +	 *
> +	 * No reconfiguration of the I2C mux channel select is required.
> +	 */
> +	MAX9286_I2C_MUX_STATE_DISABLE_SELECT,

I propose MAX9286_I2C_MUX_STATE_OPEN

Could all these be shorten to MAX9286_MUX_.... ?

> +};
> +
>  static int max9286_i2c_mux_close(struct max9286_priv *priv)
>  {
> -	/* FIXME: See note in max9286_i2c_mux_select() */
> -	if (priv->streaming)
> -		return 0;

I don't get why we had this one here. Do you agree it was not
necessary ? I guess so, since you dropped it...

>  	/*
>  	 * Ensure that both the forward and reverse channel are disabled on the
> -	 * mux, and that the channel ID is invalidated to ensure we reconfigure
> -	 * on the next select call.
> +	 * mux, and that the channel state and ID is invalidated to ensure we
> +	 * reconfigure on the next max9286_i2c_mux_select() call.
>  	 */
> +	priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;
>  	priv->mux_channel = -1;
>  	max9286_write(priv, 0x0a, 0x00);
>  	usleep_range(3000, 5000);
> @@ -242,23 +265,19 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
>  {
>  	struct max9286_priv *priv = i2c_mux_priv(muxc);
>
> -	/*
> -	 * FIXME: This state keeping is a hack and do the job. It should
> -	 * be should be reworked. One option to consider is that once all
> -	 * cameras are programmed the mux selection logic should be disabled
> -	 * and all all reverse and forward channels enable all the time.
> -	 *
> -	 * In any case this logic with a int that have two states should be
> -	 * reworked!
> -	 */
> -	if (priv->streaming == 1) {
> -		max9286_write(priv, 0x0a, 0xff);
> -		priv->streaming = 2;
> +	/* channel select is disabled when configured in the opened state. */

Channel

> +	if (priv->mux_state == MAX9286_I2C_MUX_STATE_DISABLE_SELECT)
>  		return 0;
> -	} else if (priv->streaming == 2) {
> +
> +	if (priv->mux_state == MAX9286_I2C_MUX_STATE_OPEN) {
> +		/* Open all channels on the MAX9286 */
> +		max9286_write(priv, 0x0a, 0xff);
> +		priv->mux_state = MAX9286_I2C_MUX_STATE_DISABLE_SELECT;

Shouldn't we sleep 3-5ms when changing the forward/reverse channel
configuration ?

>  		return 0;
>  	}
>
> +	/* Handling for MAX9286_I2C_MUX_STATE_CHANNEL */
> +

Empty line
Do you need this comment ?

>  	if (priv->mux_channel == chan)
>  		return 0;
>
> @@ -441,8 +460,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	int ret;
>
>  	if (enable) {
> -		/* FIXME: See note in max9286_i2c_mux_select() */
> -		priv->streaming = 1;
> +		priv->mux_state = MAX9286_I2C_MUX_STATE_OPEN;
>
>  		/* Start all cameras. */
>  		for_each_source(priv, source) {
> @@ -490,8 +508,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  		for_each_source(priv, source)
>  			v4l2_subdev_call(source->sd, video, s_stream, 0);
>
> -		/* FIXME: See note in max9286_i2c_mux_select() */
> -		priv->streaming = 0;
> +		priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;

Shouldn't we call i2c_mux_close() here, and let it close all channels
and reset the mux state ? If the mux is not closed by writing
0x0a = 0x00 but the state is here reset to STATE_CHANNEL all
successive i2c_mux_select() call will re-open channel-by-channel a mux
that is already open, won't they ?

Overall, I very much agree we need this patch, so thanks for having
addressed it!

Thanks
   j

>  	}
>
>  	return 0;
> --
> 2.20.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] max9286: Improve mux-state readbility

[Kieran Bingham]

Hi Jacopo,

Thanks for reviewing this :D

On 29/11/2019 09:14, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Thu, Nov 28, 2019 at 04:27:06PM +0000, Kieran Bingham wrote:
>> The MAX9286 implements an I2C mux which we maintain in an open state
>> while we are streaming from the cameras.
>>
>> The development code for the MAX9286 uses an integer value with
>> arbitrary state values to control these state transitions. This is
>> highlighted ith a FIXME and is difficult to interpret the meaning of the
> 
> s/ith/with

ack.

> 
>> values 0, 1, 2.
>>
>> Introduce an enum to declare the intent of each state, and comment the
>> states accordingly.
>>
>> This state transition is only needed for the multi-channel support, and
>> will not be included in the single-channel max9286 posting.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  drivers/media/i2c/max9286.c | 63 +++++++++++++++++++++++--------------
>>  1 file changed, 40 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index eed00ff1dee4..a9c3e7411bda 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -144,7 +144,7 @@ struct max9286_priv {
>>  	struct media_pad pads[MAX9286_N_PADS];
>>  	struct regulator *regulator;
>>  	bool poc_enabled;
>> -	int streaming;
>> +	int mux_state;
>>
>>  	struct i2c_mux_core *mux;
>>  	unsigned int mux_channel;
>> @@ -221,16 +221,39 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
>>   * I2C Multiplexer
>>   */
>>
>> +enum max9286_i2c_mux_state {
> 
> Let the bikeshedding begin here
> 
>> +	/*
>> +	 * The I2C Mux will enable only a single channel (both forward, and
> 
> s/Mux/mux

Ack.

> 
>> +	 * reverse) at a time, and to reduce I2C bus bandwidth, it will only
>> +	 * reconfigure the channel when a write is requested to a different
>> +	 * channel.
> 
> I won't here explain what a mux channel select does

I was trying to explain what /this state/ does ...
I can streamline this.

> 
>> +	 */
>> +	MAX9286_I2C_MUX_STATE_CHANNEL = 0,
> 
> To me, this should be the initial state of the mux, where all channels
> are closed.
> 

I actually started with a _CLOSED here, but I determined that _CLOSED
was redundant, as _CLOSED is simply _CHANNEL with a channel id of -1.

And when in _CLOSED, the next 'write' should be of type _CHANNEL, as in
it should configure only a single channel, and open only that channel.


> The state machine to me should look like:
> 
>         init() -> i2c_mux_close() -> mux_state = CLOSED;
>         all transaction selects/deselect a single channel>
>         s_stream(1) -> mux_state = REQUEST_OPEN

I also had a REQUEST_OPEN, but I deemed it also to be a bit redundant,
as the external (not mux components) which adapt the mux_state should
only care about two states - Either it's open or on channel.

I almost wonder if I should put in a helper function to make mux_state
private to the i2c_mux functions ... but I think that's overkill.


>         first transaction opens all channels -> mux_state = OPEN
>         all successive transactions with (mux_state = OPEN) are nop
> 
>         s_stream(0) -> i2c_mux_close() -> mux_state = CLOSED
>         all transaction selects/deselect a single channel until next s_stream(1)
> 
> For this state I propose MAX9286_I2C_MUX_STATE_CLOSED
> 
>> +
>> +	/*
>> +	 * The I2C mux will be configured with all ports open. All I2C writes
>> +	 * will be transmitted to all remote I2C devices, and where multiple
>> +	 * devices have the same address, the write will be received by all of
>> +	 * them.
>> +	 */
>> +	MAX9286_I2C_MUX_STATE_OPEN,
> 
> I propose MAX9286_I2C_MUX_STATE_REQUEST_OPEN
> 
>> +
>> +	/*
>> +	 * The I2C mux is configured with all ports open.
>> +	 *
>> +	 * No reconfiguration of the I2C mux channel select is required.
>> +	 */
>> +	MAX9286_I2C_MUX_STATE_DISABLE_SELECT,
> 
> I propose MAX9286_I2C_MUX_STATE_OPEN

I had this as 'MUX_STATE_OPENED', but it felt like what it was really
doing was just 'disabling select' operations, hence I renamed it.

It's also 'internal' and I wouldn't expect the no nmax9286_i2c_mux_
functions to reference this enum value.

My further reasoning to keep this as DISABLE_SELECT is that ifsomeone
set this state (not that anyone ever should), when the mux was closed -
it would remain closed.


> Could all these be shorten to MAX9286_MUX_.... ?

I think so, I was just following the function naming.


>> +};
>> +
>>  static int max9286_i2c_mux_close(struct max9286_priv *priv)
>>  {
>> -	/* FIXME: See note in max9286_i2c_mux_select() */
>> -	if (priv->streaming)
>> -		return 0;
> 
> I don't get why we had this one here. Do you agree it was not
> necessary ? I guess so, since you dropped it...


Exactly, I couldn't see any reason for this to be here, and I also
couldn't see it being used, as _close It doesn't get called after an
s_stream operation as far as I can tell. It's only currently closed
during _probe and _init.

However, if at some other point, someone wanted to call _close, I would
expect it to close all of the channels.

> 
>>  	/*
>>  	 * Ensure that both the forward and reverse channel are disabled on the
>> -	 * mux, and that the channel ID is invalidated to ensure we reconfigure
>> -	 * on the next select call.
>> +	 * mux, and that the channel state and ID is invalidated to ensure we
>> +	 * reconfigure on the next max9286_i2c_mux_select() call.
>>  	 */
>> +	priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;

Note here that we set the mux_channel to -1, and thus the state is set
to _CHANNEL as discussed above, because on the next operation - we
expect either the write to go to the specific channel, /or/ if someone
has set MAX9286_I2C_MUX_STATE_OPEN explicitly the select call will send
it to all channels.

Those are the only two options as far as I can tell, so adding extra
states of '_CLOSED' seems redundant, and adds unecessary complexity to me.

>>  	priv->mux_channel = -1;
>>  	max9286_write(priv, 0x0a, 0x00);
>>  	usleep_range(3000, 5000);
>> @@ -242,23 +265,19 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
>>  {
>>  	struct max9286_priv *priv = i2c_mux_priv(muxc);
>>
>> -	/*
>> -	 * FIXME: This state keeping is a hack and do the job. It should
>> -	 * be should be reworked. One option to consider is that once all
>> -	 * cameras are programmed the mux selection logic should be disabled
>> -	 * and all all reverse and forward channels enable all the time.
>> -	 *
>> -	 * In any case this logic with a int that have two states should be
>> -	 * reworked!
>> -	 */
>> -	if (priv->streaming == 1) {
>> -		max9286_write(priv, 0x0a, 0xff);
>> -		priv->streaming = 2;
>> +	/* channel select is disabled when configured in the opened state. */
> 
> Channel

Ack.


> 
>> +	if (priv->mux_state == MAX9286_I2C_MUX_STATE_DISABLE_SELECT)
>>  		return 0;
>> -	} else if (priv->streaming == 2) {
>> +
>> +	if (priv->mux_state == MAX9286_I2C_MUX_STATE_OPEN) {
>> +		/* Open all channels on the MAX9286 */
>> +		max9286_write(priv, 0x0a, 0xff);
>> +		priv->mux_state = MAX9286_I2C_MUX_STATE_DISABLE_SELECT;
> 
> Shouldn't we sleep 3-5ms when changing the forward/reverse channel
> configuration ?

Based on the comments below, we probably do - and this has been missing.

> 
>>  		return 0;
>>  	}
>>
>> +	/* Handling for MAX9286_I2C_MUX_STATE_CHANNEL */
>> +
> 
> Empty line
> Do you need this comment ?

I wanted to clarify that of the 3 states, the lines above explicitly
handle the MAX9286_I2C_MUX_STATE_DISABLE_SELECT,  and the
MAX9286_I2C_MUX_STATE_OPEN states, so it's left 'implicit' that the code
below is MAX9286_I2C_MUX_STATE_CHANNEL.

I added the comment to make it more explicit.

I didn't want to move all the code into a switch statement which would
be my only alternative otherwise I think.




>>  	if (priv->mux_channel == chan)
>>  		return 0;
>>
>> @@ -441,8 +460,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>  	int ret;
>>
>>  	if (enable) {
>> -		/* FIXME: See note in max9286_i2c_mux_select() */
>> -		priv->streaming = 1;
>> +		priv->mux_state = MAX9286_I2C_MUX_STATE_OPEN;
>>
>>  		/* Start all cameras. */
>>  		for_each_source(priv, source) {
>> @@ -490,8 +508,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>  		for_each_source(priv, source)
>>  			v4l2_subdev_call(source->sd, video, s_stream, 0);
>>
>> -		/* FIXME: See note in max9286_i2c_mux_select() */
>> -		priv->streaming = 0;
>> +		priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;
> 
> Shouldn't we call i2c_mux_close() here, and let it close all channels
> and reset the mux state ? If the mux is not closed by writing
> 0x0a = 0x00 but the state is here reset to STATE_CHANNEL all
> successive i2c_mux_select() call will re-open channel-by-channel a mux
> that is already open, won't they ?

I have not modified the actual state transitions from your original
implementation, so I think you're the expert here. This is your code,
just renamed.

(Ok perhaps that's not true, I removed the state check at
max9286_i2c_mux_close above)

So - thinking it through ... Yes, you are right - this will leave all
the channels open. This is the behaviour we had before this patch though
so I wonder if this could explain any of the issues we've had.

I don't /think/ so - because
  A) we probably reset the board a lot,
  B) I don't think we've had issues starting and stopping streams, but
     we haven't done enough testing there.



> Overall, I very much agree we need this patch, so thanks for having
> addressed it!

No problem, I needed to go through to understand what the three states
(0, 1, 2) were, so this is what I came up with.

Thanks for your comments, I'll await any further comments to the above
then do a respin before collapsing it into the multi-stream support patch.

Or do you think we should keep things as separate patches on top of the
'single' camera support? I don't want to end up in a GMSL==100 patches
to track case again if we can avoid it .., So I'd like to keep it down
to three managable topics :

 Patch 1) A single camera support, (should apply and run on linux-media)
 Patch 2) Support for multiple streams (requires v4l2-mux)
 Patch 3) Support for 2 MAX9286 on one bus (not upstreamable currently)

--
Kieran


> Thanks
>    j
> 
>>  	}
>>
>>  	return 0;
>> --
>> 2.20.1
>>

Re: [PATCH] max9286: Improve mux-state readbility

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 11516 bytes --]

Hi Kieran,

On Fri, Nov 29, 2019 at 11:13:00AM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> Thanks for reviewing this :D
>
> On 29/11/2019 09:14, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Thu, Nov 28, 2019 at 04:27:06PM +0000, Kieran Bingham wrote:
> >> The MAX9286 implements an I2C mux which we maintain in an open state
> >> while we are streaming from the cameras.
> >>
> >> The development code for the MAX9286 uses an integer value with
> >> arbitrary state values to control these state transitions. This is
> >> highlighted ith a FIXME and is difficult to interpret the meaning of the
> >
> > s/ith/with
>
> ack.
>
> >
> >> values 0, 1, 2.
> >>
> >> Introduce an enum to declare the intent of each state, and comment the
> >> states accordingly.
> >>
> >> This state transition is only needed for the multi-channel support, and
> >> will not be included in the single-channel max9286 posting.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  drivers/media/i2c/max9286.c | 63 +++++++++++++++++++++++--------------
> >>  1 file changed, 40 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> >> index eed00ff1dee4..a9c3e7411bda 100644
> >> --- a/drivers/media/i2c/max9286.c
> >> +++ b/drivers/media/i2c/max9286.c
> >> @@ -144,7 +144,7 @@ struct max9286_priv {
> >>  	struct media_pad pads[MAX9286_N_PADS];
> >>  	struct regulator *regulator;
> >>  	bool poc_enabled;
> >> -	int streaming;
> >> +	int mux_state;
> >>
> >>  	struct i2c_mux_core *mux;
> >>  	unsigned int mux_channel;
> >> @@ -221,16 +221,39 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
> >>   * I2C Multiplexer
> >>   */
> >>
> >> +enum max9286_i2c_mux_state {
> >
> > Let the bikeshedding begin here
> >
> >> +	/*
> >> +	 * The I2C Mux will enable only a single channel (both forward, and
> >
> > s/Mux/mux
>
> Ack.
>
> >
> >> +	 * reverse) at a time, and to reduce I2C bus bandwidth, it will only
> >> +	 * reconfigure the channel when a write is requested to a different
> >> +	 * channel.
> >
> > I won't here explain what a mux channel select does
>
> I was trying to explain what /this state/ does ...
> I can streamline this.
>
> >
> >> +	 */
> >> +	MAX9286_I2C_MUX_STATE_CHANNEL = 0,
> >
> > To me, this should be the initial state of the mux, where all channels
> > are closed.
> >
>
> I actually started with a _CLOSED here, but I determined that _CLOSED
> was redundant, as _CLOSED is simply _CHANNEL with a channel id of -1.
>
> And when in _CLOSED, the next 'write' should be of type _CHANNEL, as in
> it should configure only a single channel, and open only that channel.
>
>
> > The state machine to me should look like:
> >
> >         init() -> i2c_mux_close() -> mux_state = CLOSED;
> >         all transaction selects/deselect a single channel>
> >         s_stream(1) -> mux_state = REQUEST_OPEN
>
> I also had a REQUEST_OPEN, but I deemed it also to be a bit redundant,
> as the external (not mux components) which adapt the mux_state should
> only care about two states - Either it's open or on channel.
>
> I almost wonder if I should put in a helper function to make mux_state
> private to the i2c_mux functions ... but I think that's overkill.
>
>
> >         first transaction opens all channels -> mux_state = OPEN
> >         all successive transactions with (mux_state = OPEN) are nop
> >
> >         s_stream(0) -> i2c_mux_close() -> mux_state = CLOSED
> >         all transaction selects/deselect a single channel until next s_stream(1)
> >
> > For this state I propose MAX9286_I2C_MUX_STATE_CLOSED
> >
> >> +
> >> +	/*
> >> +	 * The I2C mux will be configured with all ports open. All I2C writes
> >> +	 * will be transmitted to all remote I2C devices, and where multiple
> >> +	 * devices have the same address, the write will be received by all of
> >> +	 * them.
> >> +	 */
> >> +	MAX9286_I2C_MUX_STATE_OPEN,
> >
> > I propose MAX9286_I2C_MUX_STATE_REQUEST_OPEN
> >
> >> +
> >> +	/*
> >> +	 * The I2C mux is configured with all ports open.
> >> +	 *
> >> +	 * No reconfiguration of the I2C mux channel select is required.
> >> +	 */
> >> +	MAX9286_I2C_MUX_STATE_DISABLE_SELECT,
> >
> > I propose MAX9286_I2C_MUX_STATE_OPEN
>
> I had this as 'MUX_STATE_OPENED', but it felt like what it was really
> doing was just 'disabling select' operations, hence I renamed it.
>
> It's also 'internal' and I wouldn't expect the no nmax9286_i2c_mux_
> functions to reference this enum value.
>
> My further reasoning to keep this as DISABLE_SELECT is that ifsomeone
> set this state (not that anyone ever should), when the mux was closed -
> it would remain closed.
>

Ok, let's see what other thinks... anyway, that's your code so if you
feel strong about this, I'm fine with what you have

>
> > Could all these be shorten to MAX9286_MUX_.... ?
>
> I think so, I was just following the function naming.
>
>
> >> +};
> >> +
> >>  static int max9286_i2c_mux_close(struct max9286_priv *priv)
> >>  {
> >> -	/* FIXME: See note in max9286_i2c_mux_select() */
> >> -	if (priv->streaming)
> >> -		return 0;
> >
> > I don't get why we had this one here. Do you agree it was not
> > necessary ? I guess so, since you dropped it...
>
>
> Exactly, I couldn't see any reason for this to be here, and I also
> couldn't see it being used, as _close It doesn't get called after an
> s_stream operation as far as I can tell. It's only currently closed
> during _probe and _init.
>
> However, if at some other point, someone wanted to call _close, I would
> expect it to close all of the channels.
>
> >
> >>  	/*
> >>  	 * Ensure that both the forward and reverse channel are disabled on the
> >> -	 * mux, and that the channel ID is invalidated to ensure we reconfigure
> >> -	 * on the next select call.
> >> +	 * mux, and that the channel state and ID is invalidated to ensure we
> >> +	 * reconfigure on the next max9286_i2c_mux_select() call.
> >>  	 */
> >> +	priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;
>
> Note here that we set the mux_channel to -1, and thus the state is set
> to _CHANNEL as discussed above, because on the next operation - we
> expect either the write to go to the specific channel, /or/ if someone
> has set MAX9286_I2C_MUX_STATE_OPEN explicitly the select call will send
> it to all channels.
>
> Those are the only two options as far as I can tell, so adding extra
> states of '_CLOSED' seems redundant, and adds unecessary complexity to me.

_CLOSED was meant to replace _CHANNEL in my proposal.
No worries though

>
> >>  	priv->mux_channel = -1;
> >>  	max9286_write(priv, 0x0a, 0x00);
> >>  	usleep_range(3000, 5000);
> >> @@ -242,23 +265,19 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
> >>  {
> >>  	struct max9286_priv *priv = i2c_mux_priv(muxc);
> >>
> >> -	/*
> >> -	 * FIXME: This state keeping is a hack and do the job. It should
> >> -	 * be should be reworked. One option to consider is that once all
> >> -	 * cameras are programmed the mux selection logic should be disabled
> >> -	 * and all all reverse and forward channels enable all the time.
> >> -	 *
> >> -	 * In any case this logic with a int that have two states should be
> >> -	 * reworked!
> >> -	 */
> >> -	if (priv->streaming == 1) {
> >> -		max9286_write(priv, 0x0a, 0xff);
> >> -		priv->streaming = 2;
> >> +	/* channel select is disabled when configured in the opened state. */
> >
> > Channel
>
> Ack.
>
>
> >
> >> +	if (priv->mux_state == MAX9286_I2C_MUX_STATE_DISABLE_SELECT)
> >>  		return 0;
> >> -	} else if (priv->streaming == 2) {
> >> +
> >> +	if (priv->mux_state == MAX9286_I2C_MUX_STATE_OPEN) {
> >> +		/* Open all channels on the MAX9286 */
> >> +		max9286_write(priv, 0x0a, 0xff);
> >> +		priv->mux_state = MAX9286_I2C_MUX_STATE_DISABLE_SELECT;
> >
> > Shouldn't we sleep 3-5ms when changing the forward/reverse channel
> > configuration ?
>
> Based on the comments below, we probably do - and this has been missing.
>

Yes, was not there in first place

> >
> >>  		return 0;
> >>  	}
> >>
> >> +	/* Handling for MAX9286_I2C_MUX_STATE_CHANNEL */
> >> +
> >
> > Empty line
> > Do you need this comment ?
>
> I wanted to clarify that of the 3 states, the lines above explicitly
> handle the MAX9286_I2C_MUX_STATE_DISABLE_SELECT,  and the
> MAX9286_I2C_MUX_STATE_OPEN states, so it's left 'implicit' that the code
> below is MAX9286_I2C_MUX_STATE_CHANNEL.
>
> I added the comment to make it more explicit.
>
> I didn't want to move all the code into a switch statement which would
> be my only alternative otherwise I think.
>
>
>
>
> >>  	if (priv->mux_channel == chan)
> >>  		return 0;
> >>
> >> @@ -441,8 +460,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >>  	int ret;
> >>
> >>  	if (enable) {
> >> -		/* FIXME: See note in max9286_i2c_mux_select() */
> >> -		priv->streaming = 1;
> >> +		priv->mux_state = MAX9286_I2C_MUX_STATE_OPEN;
> >>
> >>  		/* Start all cameras. */
> >>  		for_each_source(priv, source) {
> >> @@ -490,8 +508,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >>  		for_each_source(priv, source)
> >>  			v4l2_subdev_call(source->sd, video, s_stream, 0);
> >>
> >> -		/* FIXME: See note in max9286_i2c_mux_select() */
> >> -		priv->streaming = 0;
> >> +		priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;
> >
> > Shouldn't we call i2c_mux_close() here, and let it close all channels
> > and reset the mux state ? If the mux is not closed by writing
> > 0x0a = 0x00 but the state is here reset to STATE_CHANNEL all
> > successive i2c_mux_select() call will re-open channel-by-channel a mux
> > that is already open, won't they ?
>
> I have not modified the actual state transitions from your original
> implementation, so I think you're the expert here. This is your code,
> just renamed.

I know, this was a question not strictly related to your changes

>
> (Ok perhaps that's not true, I removed the state check at
> max9286_i2c_mux_close above)
>
> So - thinking it through ... Yes, you are right - this will leave all
> the channels open. This is the behaviour we had before this patch though
> so I wonder if this could explain any of the issues we've had.
>
> I don't /think/ so - because
>   A) we probably reset the board a lot,
>   B) I don't think we've had issues starting and stopping streams, but
>      we haven't done enough testing there.

Also, it won't hurt to have the mux open all the time after we have
configured addresses properly. It just does not feel 'right'

>
>
>
> > Overall, I very much agree we need this patch, so thanks for having
> > addressed it!
>
> No problem, I needed to go through to understand what the three states
> (0, 1, 2) were, so this is what I came up with.
>
> Thanks for your comments, I'll await any further comments to the above
> then do a respin before collapsing it into the multi-stream support patch.
>
> Or do you think we should keep things as separate patches on top of the
> 'single' camera support? I don't want to end up in a GMSL==100 patches
> to track case again if we can avoid it .., So I'd like to keep it down
> to three managable topics :
>
>  Patch 1) A single camera support, (should apply and run on linux-media)
>  Patch 2) Support for multiple streams (requires v4l2-mux)
>  Patch 3) Support for 2 MAX9286 on one bus (not upstreamable currently)

I very much agree with this plan!

Thanks
   j

>
> --
> Kieran
>
>
> > Thanks
> >    j
> >
> >>  	}
> >>
> >>  	return 0;
> >> --
> >> 2.20.1
> >>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] max9286: simplify i2c-mux parsing

[Kieran Bingham]

Hi Jacopo,

On 21/11/2019 17:35, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Thu, Nov 21, 2019 at 04:56:31PM +0000, Kieran Bingham wrote:
>>  - Identify each enabled i2c-mux channel in a single pass
>>
>> The parse_dt function iterates each node in the i2c-mux for each endpoint looking for a match.
>> The only purpose of these iterations is to determine if the corresponding i2c-channel
>> is enabled. (status = 'okay').
>>
>> Iterate the i2c-mux nodes in a single pass storing the enable state
>> in a local i2c_mux_mask for use when parsing the endpoints.
>>
> 
> I very much agree with this :)

Great,

> 
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  drivers/media/i2c/max9286.c | 84 +++++++++++++++----------------------
>>  1 file changed, 34 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index 34cb6f3b40c2..a36132becdc7 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -1097,55 +1097,6 @@ static int max9286_is_bound(struct device *dev, void *data)
>>  	return 0;
>>  }
>>
>> -static struct device_node *max9286_get_i2c_by_id(struct device_node *parent,
>> -						 u32 id)
>> -{
>> -	struct device_node *i2c_mux;
>> -	struct device_node *child;
>> -
>> -	/* Balance the of_node_put() performed by of_find_node_by_name() */
>> -	of_node_get(parent);
>> -
>> -	i2c_mux = of_find_node_by_name(parent, "i2c-mux");
>> -	if (!i2c_mux) {
>> -		printk("max9286: Failed to find i2c-mux node\n");
>> -		return NULL;
>> -	}
>> -
>> -	for_each_child_of_node(i2c_mux, child) {
>> -		u32 i2c_id = 0;
>> -
>> -		if (of_node_cmp(child->name, "i2c") != 0)
>> -			continue;
>> -		of_property_read_u32(child, "reg", &i2c_id);
>> -		if (id == i2c_id)
>> -			return child;
>> -	}
>> -
>> -	return NULL;
>> -}
>> -
>> -static int max9286_check_i2c_bus_by_id(struct device *dev, int id)
>> -{
>> -	struct device_node *i2c_np;
>> -
>> -	i2c_np = max9286_get_i2c_by_id(dev->of_node, id);
>> -	if (!i2c_np) {
>> -		dev_err(dev, "Failed to find corresponding i2c@%u\n", id);
>> -		return -ENODEV;
>> -	}
>> -
>> -	if (!of_device_is_available(i2c_np)) {
>> -		dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
>> -		of_node_put(i2c_np);
>> -		return -ENODEV;
>> -	}
>> -
>> -	of_node_put(i2c_np);
>> -
>> -	return 0;
>> -}
>> -
>>  static void max9286_cleanup_dt(struct max9286_priv *priv)
>>  {
>>  	struct max9286_source *source;
>> @@ -1167,11 +1118,44 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
>>  static int max9286_parse_dt(struct max9286_priv *priv)
>>  {
>>  	struct device *dev = &priv->client->dev;
>> +	struct device_node *i2c_mux;
>> +	struct device_node *child = NULL;
>>  	struct device_node *ep_np = NULL;
> 
> Nit: could you re-use child or ep_np ?

Yes, that would reduce local vars. I'll do this now.


> 
>> +	unsigned int i2c_mux_mask = 0;
>>  	int ret;
>>
>> +	/* Balance the of_node_put() performed by of_find_node_by_name() */
> 
> Do you need this comment ?


It was non-obvious to me at least when I added it /why/ I had to get it.
But perhaps now I've got further along, it's more clear why.

DT references are a pain :-D
Lots of places where they are implicitly reduced in loops etc..



>> +	of_node_get(dev->of_node);
>> +	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>> +	if (!i2c_mux) {
>> +		dev_err(dev, "Failed to find i2c-mux node\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Identify which i2c-mux channels are enabled */
>> +	for_each_child_of_node(i2c_mux, child) {
>> +		u32 id = 0;
>> +
>> +		if (of_node_cmp(child->name, "i2c") != 0)
>> +			continue;
> 
> With the new bindings in yaml format and the associated verification,
> this should not happen.

Aha, yes I think you're right - well in that case I'm happy to drop it,
and simplify the code.
>> +
>> +		of_property_read_u32(child, "reg", &id);
>> +		if (id >= MAX9286_NUM_GMSL)
>> +			continue;
>> +
>> +		if (!of_device_is_available(child)) {
>> +			dev_dbg(dev, "Skipping port %u with disabled I2C bus\n", id);
>> +			continue;
>> +		}
>> +
>> +		i2c_mux_mask |= BIT(id);
>> +	}
>> +	of_node_put(child);
>> +	of_node_put(i2c_mux);
>> +
>>  	v4l2_async_notifier_init(&priv->notifier);
>>
>> +	/* Parse the endpoints */
>>  	for_each_endpoint_of_node(dev->of_node, ep_np) {
> 
> dev->of_node is reused here, do you need to get it again ?

Urggh, no idea ... I'll investigate.
The earlier one crashed on me, this one did not ... but better to get it
'right'.


> All minors though, squash this on the next max9286 submission if you
> feel to.

Thanks.

--
Kieran


> 
> Thanks
>    j
> 
>>  		struct max9286_source *source;
>>  		struct of_endpoint ep;
>> @@ -1214,7 +1198,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>  		}
>>
>>  		/* Skip if the corresponding GMSL link is unavailable. */
>> -		if (max9286_check_i2c_bus_by_id(dev, ep.port))
>> +		if (!(i2c_mux_mask & BIT(ep.port)))
>>  			continue;
>>
>>  		if (priv->sources[ep.port].fwnode) {
>> --
>> 2.20.1
>>

-- 
Regards
--
Kieran

Re: [PATCH] max9286: Improve mux-state readbility

[Kieran Bingham]

Hi Jacopo,

On 29/11/2019 11:35, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Fri, Nov 29, 2019 at 11:13:00AM +0000, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> Thanks for reviewing this :D
>>
>> On 29/11/2019 09:14, Jacopo Mondi wrote:
>>> Hi Kieran,
>>>
>>> On Thu, Nov 28, 2019 at 04:27:06PM +0000, Kieran Bingham wrote:
>>>> The MAX9286 implements an I2C mux which we maintain in an open state
>>>> while we are streaming from the cameras.
>>>>
>>>> The development code for the MAX9286 uses an integer value with
>>>> arbitrary state values to control these state transitions. This is
>>>> highlighted ith a FIXME and is difficult to interpret the meaning of the
>>>
>>> s/ith/with
>>
>> ack.
>>
>>>
>>>> values 0, 1, 2.
>>>>
>>>> Introduce an enum to declare the intent of each state, and comment the
>>>> states accordingly.
>>>>
>>>> This state transition is only needed for the multi-channel support, and
>>>> will not be included in the single-channel max9286 posting.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> ---
>>>>  drivers/media/i2c/max9286.c | 63 +++++++++++++++++++++++--------------
>>>>  1 file changed, 40 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>>>> index eed00ff1dee4..a9c3e7411bda 100644
>>>> --- a/drivers/media/i2c/max9286.c
>>>> +++ b/drivers/media/i2c/max9286.c
>>>> @@ -144,7 +144,7 @@ struct max9286_priv {
>>>>  	struct media_pad pads[MAX9286_N_PADS];
>>>>  	struct regulator *regulator;
>>>>  	bool poc_enabled;
>>>> -	int streaming;
>>>> +	int mux_state;
>>>>
>>>>  	struct i2c_mux_core *mux;
>>>>  	unsigned int mux_channel;
>>>> @@ -221,16 +221,39 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
>>>>   * I2C Multiplexer
>>>>   */
>>>>
>>>> +enum max9286_i2c_mux_state {
>>>
>>> Let the bikeshedding begin here
>>>
>>>> +	/*
>>>> +	 * The I2C Mux will enable only a single channel (both forward, and
>>>
>>> s/Mux/mux
>>
>> Ack.
>>
>>>
>>>> +	 * reverse) at a time, and to reduce I2C bus bandwidth, it will only
>>>> +	 * reconfigure the channel when a write is requested to a different
>>>> +	 * channel.
>>>
>>> I won't here explain what a mux channel select does
>>
>> I was trying to explain what /this state/ does ...
>> I can streamline this.
>>
>>>
>>>> +	 */
>>>> +	MAX9286_I2C_MUX_STATE_CHANNEL = 0,
>>>
>>> To me, this should be the initial state of the mux, where all channels
>>> are closed.
>>>
>>
>> I actually started with a _CLOSED here, but I determined that _CLOSED
>> was redundant, as _CLOSED is simply _CHANNEL with a channel id of -1.
>>
>> And when in _CLOSED, the next 'write' should be of type _CHANNEL, as in
>> it should configure only a single channel, and open only that channel.
>>
>>
>>> The state machine to me should look like:
>>>
>>>         init() -> i2c_mux_close() -> mux_state = CLOSED;
>>>         all transaction selects/deselect a single channel>
>>>         s_stream(1) -> mux_state = REQUEST_OPEN
>>
>> I also had a REQUEST_OPEN, but I deemed it also to be a bit redundant,
>> as the external (not mux components) which adapt the mux_state should
>> only care about two states - Either it's open or on channel.
>>
>> I almost wonder if I should put in a helper function to make mux_state
>> private to the i2c_mux functions ... but I think that's overkill.
>>
>>
>>>         first transaction opens all channels -> mux_state = OPEN
>>>         all successive transactions with (mux_state = OPEN) are nop
>>>
>>>         s_stream(0) -> i2c_mux_close() -> mux_state = CLOSED
>>>         all transaction selects/deselect a single channel until next s_stream(1)
>>>
>>> For this state I propose MAX9286_I2C_MUX_STATE_CLOSED
>>>
>>>> +
>>>> +	/*
>>>> +	 * The I2C mux will be configured with all ports open. All I2C writes
>>>> +	 * will be transmitted to all remote I2C devices, and where multiple
>>>> +	 * devices have the same address, the write will be received by all of
>>>> +	 * them.
>>>> +	 */
>>>> +	MAX9286_I2C_MUX_STATE_OPEN,
>>>
>>> I propose MAX9286_I2C_MUX_STATE_REQUEST_OPEN
>>>
>>>> +
>>>> +	/*
>>>> +	 * The I2C mux is configured with all ports open.
>>>> +	 *
>>>> +	 * No reconfiguration of the I2C mux channel select is required.
>>>> +	 */
>>>> +	MAX9286_I2C_MUX_STATE_DISABLE_SELECT,
>>>
>>> I propose MAX9286_I2C_MUX_STATE_OPEN
>>
>> I had this as 'MUX_STATE_OPENED', but it felt like what it was really
>> doing was just 'disabling select' operations, hence I renamed it.
>>
>> It's also 'internal' and I wouldn't expect the no nmax9286_i2c_mux_
>> functions to reference this enum value.
>>
>> My further reasoning to keep this as DISABLE_SELECT is that ifsomeone
>> set this state (not that anyone ever should), when the mux was closed -
>> it would remain closed.
>>
> 
> Ok, let's see what other thinks... anyway, that's your code so if you
> feel strong about this, I'm fine with what you have

Ok,


>>> Could all these be shorten to MAX9286_MUX_.... ?
>>
>> I think so, I was just following the function naming.
>>
>>
>>>> +};
>>>> +
>>>>  static int max9286_i2c_mux_close(struct max9286_priv *priv)
>>>>  {
>>>> -	/* FIXME: See note in max9286_i2c_mux_select() */
>>>> -	if (priv->streaming)
>>>> -		return 0;
>>>
>>> I don't get why we had this one here. Do you agree it was not
>>> necessary ? I guess so, since you dropped it...
>>
>>
>> Exactly, I couldn't see any reason for this to be here, and I also
>> couldn't see it being used, as _close It doesn't get called after an
>> s_stream operation as far as I can tell. It's only currently closed
>> during _probe and _init.
>>
>> However, if at some other point, someone wanted to call _close, I would
>> expect it to close all of the channels.
>>
>>>
>>>>  	/*
>>>>  	 * Ensure that both the forward and reverse channel are disabled on the
>>>> -	 * mux, and that the channel ID is invalidated to ensure we reconfigure
>>>> -	 * on the next select call.
>>>> +	 * mux, and that the channel state and ID is invalidated to ensure we
>>>> +	 * reconfigure on the next max9286_i2c_mux_select() call.
>>>>  	 */
>>>> +	priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;
>>
>> Note here that we set the mux_channel to -1, and thus the state is set
>> to _CHANNEL as discussed above, because on the next operation - we
>> expect either the write to go to the specific channel, /or/ if someone
>> has set MAX9286_I2C_MUX_STATE_OPEN explicitly the select call will send
>> it to all channels.
>>
>> Those are the only two options as far as I can tell, so adding extra
>> states of '_CLOSED' seems redundant, and adds unecessary complexity to me.
> 
> _CLOSED was meant to replace _CHANNEL in my proposal.
> No worries though

Ah I see, as in just a different name. My concern with _CLOSED though is
that we do not close the channel after each write, and _CLOSED could
potentially give the impression that we do ...

>>>>  	priv->mux_channel = -1;
>>>>  	max9286_write(priv, 0x0a, 0x00);
>>>>  	usleep_range(3000, 5000);
>>>> @@ -242,23 +265,19 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
>>>>  {
>>>>  	struct max9286_priv *priv = i2c_mux_priv(muxc);
>>>>
>>>> -	/*
>>>> -	 * FIXME: This state keeping is a hack and do the job. It should
>>>> -	 * be should be reworked. One option to consider is that once all
>>>> -	 * cameras are programmed the mux selection logic should be disabled
>>>> -	 * and all all reverse and forward channels enable all the time.
>>>> -	 *
>>>> -	 * In any case this logic with a int that have two states should be
>>>> -	 * reworked!
>>>> -	 */
>>>> -	if (priv->streaming == 1) {
>>>> -		max9286_write(priv, 0x0a, 0xff);
>>>> -		priv->streaming = 2;
>>>> +	/* channel select is disabled when configured in the opened state. */
>>>
>>> Channel
>>
>> Ack.
>>
>>
>>>
>>>> +	if (priv->mux_state == MAX9286_I2C_MUX_STATE_DISABLE_SELECT)
>>>>  		return 0;
>>>> -	} else if (priv->streaming == 2) {
>>>> +
>>>> +	if (priv->mux_state == MAX9286_I2C_MUX_STATE_OPEN) {
>>>> +		/* Open all channels on the MAX9286 */
>>>> +		max9286_write(priv, 0x0a, 0xff);
>>>> +		priv->mux_state = MAX9286_I2C_MUX_STATE_DISABLE_SELECT;
>>>
>>> Shouldn't we sleep 3-5ms when changing the forward/reverse channel
>>> configuration ?
>>
>> Based on the comments below, we probably do - and this has been missing.
>>
> 
> Yes, was not there in first place
> 
>>>
>>>>  		return 0;
>>>>  	}
>>>>
>>>> +	/* Handling for MAX9286_I2C_MUX_STATE_CHANNEL */
>>>> +
>>>
>>> Empty line
>>> Do you need this comment ?
>>
>> I wanted to clarify that of the 3 states, the lines above explicitly
>> handle the MAX9286_I2C_MUX_STATE_DISABLE_SELECT,  and the
>> MAX9286_I2C_MUX_STATE_OPEN states, so it's left 'implicit' that the code
>> below is MAX9286_I2C_MUX_STATE_CHANNEL.
>>
>> I added the comment to make it more explicit.
>>
>> I didn't want to move all the code into a switch statement which would
>> be my only alternative otherwise I think.
>>
>>
>>
>>
>>>>  	if (priv->mux_channel == chan)
>>>>  		return 0;
>>>>
>>>> @@ -441,8 +460,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>>>  	int ret;
>>>>
>>>>  	if (enable) {
>>>> -		/* FIXME: See note in max9286_i2c_mux_select() */
>>>> -		priv->streaming = 1;
>>>> +		priv->mux_state = MAX9286_I2C_MUX_STATE_OPEN;
>>>>
>>>>  		/* Start all cameras. */
>>>>  		for_each_source(priv, source) {
>>>> @@ -490,8 +508,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>>>  		for_each_source(priv, source)
>>>>  			v4l2_subdev_call(source->sd, video, s_stream, 0);
>>>>
>>>> -		/* FIXME: See note in max9286_i2c_mux_select() */
>>>> -		priv->streaming = 0;
>>>> +		priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;
>>>
>>> Shouldn't we call i2c_mux_close() here, and let it close all channels
>>> and reset the mux state ? If the mux is not closed by writing
>>> 0x0a = 0x00 but the state is here reset to STATE_CHANNEL all
>>> successive i2c_mux_select() call will re-open channel-by-channel a mux
>>> that is already open, won't they ?
>>
>> I have not modified the actual state transitions from your original
>> implementation, so I think you're the expert here. This is your code,
>> just renamed.
> 
> I know, this was a question not strictly related to your changes
> 
>>
>> (Ok perhaps that's not true, I removed the state check at
>> max9286_i2c_mux_close above)
>>
>> So - thinking it through ... Yes, you are right - this will leave all
>> the channels open. This is the behaviour we had before this patch though
>> so I wonder if this could explain any of the issues we've had.
>>
>> I don't /think/ so - because
>>   A) we probably reset the board a lot,
>>   B) I don't think we've had issues starting and stopping streams, but
>>      we haven't done enough testing there.
> 
> Also, it won't hurt to have the mux open all the time after we have
> configured addresses properly. It just does not feel 'right'

Indeed, and while it may not hurt, it leads to a point where we get the
following conditions :

(O=Open, _=Closed)

1 2 3 4
O _ _ _ // Write to 1
_ O _ _ // Write to 2
_ _ O _ // Write to 3
_ _ _ O // Write to 4

O O O O // StreamOn   // MAX9286_I2C_MUX_STATE_OPEN
O O O O // StreamOff  // MAX9286_I2C_MUX_STATE_CHANNEL

O O O O // Write to 1
_ O O O // Write to 2
_ _ O O // Write to 3
_ _ _ O // Write to 4

O _ _ _ // Write to 1

O O O O // StreamOn   // MAX9286_I2C_MUX_STATE_OPEN
O O O O // StreamOff  // MAX9286_I2C_MUX_STATE_CHANNEL

... Continues

I will indeed fix this by calling mux_close when we stop streaming.
At least then we will have a consistent state as we expect.


In fact from all that I think that means we would likely be better to have:


/* Opens all channels on the MUX and disables individual select lines */
static int max9286_i2c_mux_open(struct max9286_priv *priv)
{
	/* Open all channels on the MAX9286 */
	max9286_write(priv, 0x0a, 0xff);
	usleep_range(3000, 5000);

	priv->mux_state = MAX9286_I2C_MUX_STATE_DISABLE_SELECT;

	return 0;
}

static int max9286_i2c_mux_close(struct max9286_priv *priv)
{
	priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;
	priv->mux_channel = -1;
	max9286_write(priv, 0x0a, 0x00);
	usleep_range(3000, 5000);
	return 0;
}

static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
{
	struct max9286_priv *priv = i2c_mux_priv(muxc);

	/* channel select is disabled when configured in the opend state. */
	if (priv->mux_state == MAX9286_I2C_MUX_STATE_DISABLE_SELECT)
		return 0;

	/* Handling for MAX9286_I2C_MUX_STATE_CHANNEL */

	if (priv->mux_channel == chan)
		return 0;

	...
}

Which makes the intentions much clearer, and stops other sections of the
driver from directly modifying the 'private' mux_state.


>>> Overall, I very much agree we need this patch, so thanks for having
>>> addressed it!
>>
>> No problem, I needed to go through to understand what the three states
>> (0, 1, 2) were, so this is what I came up with.
>>
>> Thanks for your comments, I'll await any further comments to the above
>> then do a respin before collapsing it into the multi-stream support patch.
>>
>> Or do you think we should keep things as separate patches on top of the
>> 'single' camera support? I don't want to end up in a GMSL==100 patches
>> to track case again if we can avoid it .., So I'd like to keep it down
>> to three managable topics :
>>
>>  Patch 1) A single camera support, (should apply and run on linux-media)
>>  Patch 2) Support for multiple streams (requires v4l2-mux)
>>  Patch 3) Support for 2 MAX9286 on one bus (not upstreamable currently)
> 
> I very much agree with this plan!

Great, I'm on the right track then.

> Thanks
>    j
> 

-- 
Regards
--
Kieran

[PATCH] max9286: Improve mux-state readbility [v2]

[Kieran Bingham]

The MAX9286 implements an I2C mux which we maintain in an open state
while we are streaming from the cameras.

The development code for the MAX9286 uses an integer value with
arbitrary state values to control these state transitions. This is
highlighted with a FIXME and is difficult to interpret the meaning of the
values 0, 1, 2.

Introduce a new function call max9286_i2c_mux_open() to make it clear
when a component opens all the mux channels, and update the usage
in s_stream() to max9286_i2c_mux_close() the mux on stop stream.

We previously had missed an occasion to sleep after an update to the I2C
Fwd/Rev channels, so all writes to this configuration register are moved
to a helper: max9286_i2c_mux_configure() which guarantees the delay.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 74 ++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 5b8dfa652d50..b34fb31c6db5 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -144,7 +144,7 @@ struct max9286_priv {
 	struct media_pad pads[MAX9286_N_PADS];
 	struct regulator *regulator;
 	bool poc_enabled;
-	int streaming;
+	int mux_state;
 
 	struct i2c_mux_core *mux;
 	unsigned int mux_channel;
@@ -221,57 +221,59 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
  * I2C Multiplexer
  */
 
-static int max9286_i2c_mux_close(struct max9286_priv *priv)
+enum max9286_i2c_mux_state {
+	MAX9286_MUX_CLOSED = 0,
+	MAX9286_MUX_OPEN,
+};
+
+static void max9286_i2c_mux_configure(struct max9286_priv *priv, u8 conf)
+{
+	max9286_write(priv, 0x0a, conf);
+
+	/*
+	 * We must sleep after any change to the forward or reverse channel
+	 * configuration.
+	 */
+	usleep_range(3000, 5000);
+}
+
+static void max9286_i2c_mux_open(struct max9286_priv *priv)
+{
+	/* Open all channels on the MAX9286 */
+	max9286_i2c_mux_configure(priv, 0xff);
+
+	priv->mux_state = MAX9286_MUX_OPEN;
+}
+
+static void max9286_i2c_mux_close(struct max9286_priv *priv)
 {
-	/* FIXME: See note in max9286_i2c_mux_select() */
-	if (priv->streaming)
-		return 0;
 	/*
 	 * Ensure that both the forward and reverse channel are disabled on the
 	 * mux, and that the channel ID is invalidated to ensure we reconfigure
-	 * on the next select call.
+	 * on the next max9286_i2c_mux_select() call.
 	 */
-	priv->mux_channel = -1;
-	max9286_write(priv, 0x0a, 0x00);
-	usleep_range(3000, 5000);
+	max9286_i2c_mux_configure(priv, 0x00);
 
-	return 0;
+	priv->mux_state = MAX9286_MUX_CLOSED;
+	priv->mux_channel = -1;
 }
 
 static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
 {
 	struct max9286_priv *priv = i2c_mux_priv(muxc);
 
-	/*
-	 * FIXME: This state keeping is a hack and do the job. It should
-	 * be should be reworked. One option to consider is that once all
-	 * cameras are programmed the mux selection logic should be disabled
-	 * and all all reverse and forward channels enable all the time.
-	 *
-	 * In any case this logic with a int that have two states should be
-	 * reworked!
-	 */
-	if (priv->streaming == 1) {
-		max9286_write(priv, 0x0a, 0xff);
-		priv->streaming = 2;
-		return 0;
-	} else if (priv->streaming == 2) {
+	/* channel select is disabled when configured in the opened state. */
+	if (priv->mux_state == MAX9286_MUX_OPEN)
 		return 0;
-	}
 
 	if (priv->mux_channel == chan)
 		return 0;
 
 	priv->mux_channel = chan;
 
-	max9286_write(priv, 0x0a,
-		      MAX9286_FWDCCEN(chan) | MAX9286_REVCCEN(chan));
-
-	/*
-	 * We must sleep after any change to the forward or reverse channel
-	 * configuration.
-	 */
-	usleep_range(3000, 5000);
+	max9286_i2c_mux_configure(priv,
+				  MAX9286_FWDCCEN(chan) |
+				  MAX9286_REVCCEN(chan));
 
 	return 0;
 }
@@ -441,8 +443,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	int ret;
 
 	if (enable) {
-		/* FIXME: See note in max9286_i2c_mux_select() */
-		priv->streaming = 1;
+		max9286_i2c_mux_open(priv);
 
 		/* Start all cameras. */
 		for_each_source(priv, source) {
@@ -490,8 +491,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		for_each_source(priv, source)
 			v4l2_subdev_call(source->sd, video, s_stream, 0);
 
-		/* FIXME: See note in max9286_i2c_mux_select() */
-		priv->streaming = 0;
+		max9286_i2c_mux_close(priv);
 	}
 
 	return 0;
-- 
2.20.1

Re: [PATCH] max9286: Improve mux-state readbility [v2]

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 5014 bytes --]

Hi Kieran,
    only a really minor nit, you could fix when squashing this patch

On Fri, Nov 29, 2019 at 01:26:43PM +0000, Kieran Bingham wrote:
> The MAX9286 implements an I2C mux which we maintain in an open state
> while we are streaming from the cameras.
>
> The development code for the MAX9286 uses an integer value with
> arbitrary state values to control these state transitions. This is
> highlighted with a FIXME and is difficult to interpret the meaning of the
> values 0, 1, 2.
>
> Introduce a new function call max9286_i2c_mux_open() to make it clear
> when a component opens all the mux channels, and update the usage
> in s_stream() to max9286_i2c_mux_close() the mux on stop stream.
>
> We previously had missed an occasion to sleep after an update to the I2C
> Fwd/Rev channels, so all writes to this configuration register are moved
> to a helper: max9286_i2c_mux_configure() which guarantees the delay.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 74 ++++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 5b8dfa652d50..b34fb31c6db5 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -144,7 +144,7 @@ struct max9286_priv {
>  	struct media_pad pads[MAX9286_N_PADS];
>  	struct regulator *regulator;
>  	bool poc_enabled;
> -	int streaming;
> +	int mux_state;
>
>  	struct i2c_mux_core *mux;
>  	unsigned int mux_channel;
> @@ -221,57 +221,59 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
>   * I2C Multiplexer
>   */
>
> -static int max9286_i2c_mux_close(struct max9286_priv *priv)
> +enum max9286_i2c_mux_state {
> +	MAX9286_MUX_CLOSED = 0,
> +	MAX9286_MUX_OPEN,
> +};
> +
> +static void max9286_i2c_mux_configure(struct max9286_priv *priv, u8 conf)
> +{
> +	max9286_write(priv, 0x0a, conf);
> +
> +	/*
> +	 * We must sleep after any change to the forward or reverse channel
> +	 * configuration.
> +	 */
> +	usleep_range(3000, 5000);
> +}
> +
> +static void max9286_i2c_mux_open(struct max9286_priv *priv)
> +{
> +	/* Open all channels on the MAX9286 */
> +	max9286_i2c_mux_configure(priv, 0xff);
> +
> +	priv->mux_state = MAX9286_MUX_OPEN;
> +}
> +
> +static void max9286_i2c_mux_close(struct max9286_priv *priv)
>  {
> -	/* FIXME: See note in max9286_i2c_mux_select() */
> -	if (priv->streaming)
> -		return 0;
>  	/*
>  	 * Ensure that both the forward and reverse channel are disabled on the
>  	 * mux, and that the channel ID is invalidated to ensure we reconfigure
> -	 * on the next select call.
> +	 * on the next max9286_i2c_mux_select() call.
>  	 */
> -	priv->mux_channel = -1;
> -	max9286_write(priv, 0x0a, 0x00);
> -	usleep_range(3000, 5000);
> +	max9286_i2c_mux_configure(priv, 0x00);
>
> -	return 0;
> +	priv->mux_state = MAX9286_MUX_CLOSED;
> +	priv->mux_channel = -1;
>  }
>
>  static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
>  {
>  	struct max9286_priv *priv = i2c_mux_priv(muxc);
>
> -	/*
> -	 * FIXME: This state keeping is a hack and do the job. It should
> -	 * be should be reworked. One option to consider is that once all
> -	 * cameras are programmed the mux selection logic should be disabled
> -	 * and all all reverse and forward channels enable all the time.
> -	 *
> -	 * In any case this logic with a int that have two states should be
> -	 * reworked!
> -	 */
> -	if (priv->streaming == 1) {
> -		max9286_write(priv, 0x0a, 0xff);
> -		priv->streaming = 2;
> -		return 0;
> -	} else if (priv->streaming == 2) {
> +	/* channel select is disabled when configured in the opened state. */

All other comments in this driver start with a capital letter.

Otherwise, I really like this patch! thanks!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> +	if (priv->mux_state == MAX9286_MUX_OPEN)
>  		return 0;
> -	}
>
>  	if (priv->mux_channel == chan)
>  		return 0;
>
>  	priv->mux_channel = chan;
>
> -	max9286_write(priv, 0x0a,
> -		      MAX9286_FWDCCEN(chan) | MAX9286_REVCCEN(chan));
> -
> -	/*
> -	 * We must sleep after any change to the forward or reverse channel
> -	 * configuration.
> -	 */
> -	usleep_range(3000, 5000);
> +	max9286_i2c_mux_configure(priv,
> +				  MAX9286_FWDCCEN(chan) |
> +				  MAX9286_REVCCEN(chan));
>
>  	return 0;
>  }
> @@ -441,8 +443,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	int ret;
>
>  	if (enable) {
> -		/* FIXME: See note in max9286_i2c_mux_select() */
> -		priv->streaming = 1;
> +		max9286_i2c_mux_open(priv);
>
>  		/* Start all cameras. */
>  		for_each_source(priv, source) {
> @@ -490,8 +491,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  		for_each_source(priv, source)
>  			v4l2_subdev_call(source->sd, video, s_stream, 0);
>
> -		/* FIXME: See note in max9286_i2c_mux_select() */
> -		priv->streaming = 0;
> +		max9286_i2c_mux_close(priv);
>  	}
>
>  	return 0;
> --
> 2.20.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] max9286: Improve mux-state readbility [v2]

[Kieran Bingham]

Hi Jacopo,

On 03/12/2019 08:19, Jacopo Mondi wrote:
> Hi Kieran,
>     only a really minor nit, you could fix when squashing this patch
> 
> On Fri, Nov 29, 2019 at 01:26:43PM +0000, Kieran Bingham wrote:
>> The MAX9286 implements an I2C mux which we maintain in an open state
>> while we are streaming from the cameras.
>>
>> The development code for the MAX9286 uses an integer value with
>> arbitrary state values to control these state transitions. This is
>> highlighted with a FIXME and is difficult to interpret the meaning of the
>> values 0, 1, 2.
>>
>> Introduce a new function call max9286_i2c_mux_open() to make it clear
>> when a component opens all the mux channels, and update the usage
>> in s_stream() to max9286_i2c_mux_close() the mux on stop stream.
>>
>> We previously had missed an occasion to sleep after an update to the I2C
>> Fwd/Rev channels, so all writes to this configuration register are moved
>> to a helper: max9286_i2c_mux_configure() which guarantees the delay.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  drivers/media/i2c/max9286.c | 74 ++++++++++++++++++-------------------
>>  1 file changed, 37 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index 5b8dfa652d50..b34fb31c6db5 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -144,7 +144,7 @@ struct max9286_priv {
>>  	struct media_pad pads[MAX9286_N_PADS];
>>  	struct regulator *regulator;
>>  	bool poc_enabled;
>> -	int streaming;
>> +	int mux_state;
>>
>>  	struct i2c_mux_core *mux;
>>  	unsigned int mux_channel;
>> @@ -221,57 +221,59 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
>>   * I2C Multiplexer
>>   */
>>
>> -static int max9286_i2c_mux_close(struct max9286_priv *priv)
>> +enum max9286_i2c_mux_state {
>> +	MAX9286_MUX_CLOSED = 0,
>> +	MAX9286_MUX_OPEN,
>> +};

This is now down to just two states, so I think this probably warrants
being changed to a bool mux_open;

I've already folded /this/ patch into the main max9286 driver, so I'll
make this small fix and post it here along with the next two fixes I have.

>> +
>> +static void max9286_i2c_mux_configure(struct max9286_priv *priv, u8 conf)
>> +{
>> +	max9286_write(priv, 0x0a, conf);
>> +
>> +	/*
>> +	 * We must sleep after any change to the forward or reverse channel
>> +	 * configuration.
>> +	 */
>> +	usleep_range(3000, 5000);
>> +}
>> +
>> +static void max9286_i2c_mux_open(struct max9286_priv *priv)
>> +{
>> +	/* Open all channels on the MAX9286 */
>> +	max9286_i2c_mux_configure(priv, 0xff);
>> +
>> +	priv->mux_state = MAX9286_MUX_OPEN;
>> +}
>> +
>> +static void max9286_i2c_mux_close(struct max9286_priv *priv)
>>  {
>> -	/* FIXME: See note in max9286_i2c_mux_select() */
>> -	if (priv->streaming)
>> -		return 0;
>>  	/*
>>  	 * Ensure that both the forward and reverse channel are disabled on the
>>  	 * mux, and that the channel ID is invalidated to ensure we reconfigure
>> -	 * on the next select call.
>> +	 * on the next max9286_i2c_mux_select() call.
>>  	 */
>> -	priv->mux_channel = -1;
>> -	max9286_write(priv, 0x0a, 0x00);
>> -	usleep_range(3000, 5000);
>> +	max9286_i2c_mux_configure(priv, 0x00);
>>
>> -	return 0;
>> +	priv->mux_state = MAX9286_MUX_CLOSED;
>> +	priv->mux_channel = -1;
>>  }
>>
>>  static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
>>  {
>>  	struct max9286_priv *priv = i2c_mux_priv(muxc);
>>
>> -	/*
>> -	 * FIXME: This state keeping is a hack and do the job. It should
>> -	 * be should be reworked. One option to consider is that once all
>> -	 * cameras are programmed the mux selection logic should be disabled
>> -	 * and all all reverse and forward channels enable all the time.
>> -	 *
>> -	 * In any case this logic with a int that have two states should be
>> -	 * reworked!
>> -	 */
>> -	if (priv->streaming == 1) {
>> -		max9286_write(priv, 0x0a, 0xff);
>> -		priv->streaming = 2;
>> -		return 0;
>> -	} else if (priv->streaming == 2) {
>> +	/* channel select is disabled when configured in the opened state. */
> 
> All other comments in this driver start with a capital letter.

Thanks, this is fixed up.


> Otherwise, I really like this patch! thanks!
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Thanks
>    j
> 
>> +	if (priv->mux_state == MAX9286_MUX_OPEN)
>>  		return 0;
>> -	}
>>
>>  	if (priv->mux_channel == chan)
>>  		return 0;
>>
>>  	priv->mux_channel = chan;
>>
>> -	max9286_write(priv, 0x0a,
>> -		      MAX9286_FWDCCEN(chan) | MAX9286_REVCCEN(chan));
>> -
>> -	/*
>> -	 * We must sleep after any change to the forward or reverse channel
>> -	 * configuration.
>> -	 */
>> -	usleep_range(3000, 5000);
>> +	max9286_i2c_mux_configure(priv,
>> +				  MAX9286_FWDCCEN(chan) |
>> +				  MAX9286_REVCCEN(chan));
>>
>>  	return 0;
>>  }
>> @@ -441,8 +443,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>  	int ret;
>>
>>  	if (enable) {
>> -		/* FIXME: See note in max9286_i2c_mux_select() */
>> -		priv->streaming = 1;
>> +		max9286_i2c_mux_open(priv);
>>
>>  		/* Start all cameras. */
>>  		for_each_source(priv, source) {
>> @@ -490,8 +491,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>  		for_each_source(priv, source)
>>  			v4l2_subdev_call(source->sd, video, s_stream, 0);
>>
>> -		/* FIXME: See note in max9286_i2c_mux_select() */
>> -		priv->streaming = 0;
>> +		max9286_i2c_mux_close(priv);
>>  	}
>>
>>  	return 0;
>> --
>> 2.20.1
>>

-- 
Regards
--
Kieran

[PATCH 1/3] media: i2c: max9286: Remove redundant max9286_i2c_mux_state

[Kieran Bingham]

While simplifying the i2c-mux state, the states were stored in an enum
(initially there were three).

This has now simplified down to 2 states, open and closed - and can be
represented easily in a bool.

It 'could' also be represented within the mux_channel, but I don't want
to pollute that further than the '-1' value which is already stored in
there to represent no channel selected.

Remove the max9286_i2c_mux_state and replace with a bool mux_open flag,
and move the location within the private struct to be more appropriate.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index e5b3f78318db..6ea08fd87811 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -144,10 +144,10 @@ struct max9286_priv {
 	struct media_pad pads[MAX9286_N_PADS];
 	struct regulator *regulator;
 	bool poc_enabled;
-	int mux_state;
 
 	struct i2c_mux_core *mux;
 	unsigned int mux_channel;
+	bool mux_open;
 
 	struct v4l2_ctrl_handler ctrls;
 
@@ -221,11 +221,6 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
  * I2C Multiplexer
  */
 
-enum max9286_i2c_mux_state {
-	MAX9286_MUX_CLOSED = 0,
-	MAX9286_MUX_OPEN,
-};
-
 static void max9286_i2c_mux_configure(struct max9286_priv *priv, u8 conf)
 {
 	max9286_write(priv, 0x0a, conf);
@@ -242,7 +237,7 @@ static void max9286_i2c_mux_open(struct max9286_priv *priv)
 	/* Open all channels on the MAX9286 */
 	max9286_i2c_mux_configure(priv, 0xff);
 
-	priv->mux_state = MAX9286_MUX_OPEN;
+	priv->mux_open = true;
 }
 
 static void max9286_i2c_mux_close(struct max9286_priv *priv)
@@ -254,7 +249,7 @@ static void max9286_i2c_mux_close(struct max9286_priv *priv)
 	 */
 	max9286_i2c_mux_configure(priv, 0x00);
 
-	priv->mux_state = MAX9286_MUX_CLOSED;
+	priv->mux_open = false;
 	priv->mux_channel = -1;
 }
 
@@ -263,7 +258,7 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
 	struct max9286_priv *priv = i2c_mux_priv(muxc);
 
 	/* Channel select is disabled when configured in the opened state. */
-	if (priv->mux_state == MAX9286_MUX_OPEN)
+	if (priv->mux_open)
 		return 0;
 
 	if (priv->mux_channel == chan)
-- 
2.20.1

[PATCH 2/3] media: i2c: max9286: Add GPIO chip controller

[Kieran Bingham]

Provide a GPIO chip to control the two output lines available on the
MAX9286.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 68 +++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 6ea08fd87811..c34e7b5c7447 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/fwnode.h>
+#include <linux/gpio/driver.h>
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/module.h>
@@ -58,6 +59,8 @@
 #define MAX9286_HVSRC_D0		(2 << 0)
 #define MAX9286_HVSRC_D14		(1 << 0)
 #define MAX9286_HVSRC_D18		(0 << 0)
+/* Register 0x0f */
+#define MAX9286_0X0F_RESERVED		BIT(3)
 /* Register 0x12 */
 #define MAX9286_CSILANECNT(n)		(((n) - 1) << 6)
 #define MAX9286_CSIDBL			BIT(5)
@@ -145,6 +148,9 @@ struct max9286_priv {
 	struct regulator *regulator;
 	bool poc_enabled;
 
+	struct gpio_chip gpio;
+	u8 gpio_state;
+
 	struct i2c_mux_core *mux;
 	unsigned int mux_channel;
 	bool mux_open;
@@ -712,6 +718,60 @@ static const struct of_device_id max9286_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, max9286_dt_ids);
 
+static void max9286_gpio_set(struct gpio_chip *chip,
+			     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);
+}
+
+static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct max9286_priv *priv = gpiochip_get_data(chip);
+
+	return priv->gpio_state & BIT(offset);
+}
+
+static int max9286_gpio(struct max9286_priv *priv)
+{
+	struct device *dev = &priv->client->dev;
+	struct gpio_chip *gpio = &priv->gpio;
+	int ret;
+
+	static const char * const names[] = {
+		"GPIO0OUT",
+		"GPIO1OUT",
+	};
+
+	/* Configure the GPIO */
+	gpio->label = dev_name(dev);
+	gpio->parent = dev;
+	gpio->owner = THIS_MODULE;
+	gpio->of_node = dev->of_node;
+	gpio->ngpio = 2;
+	gpio->set = max9286_gpio_set;
+	gpio->get = max9286_gpio_get;
+	gpio->can_sleep = true;
+	gpio->names = names;
+
+	/* GPIO values default to high */
+	priv->gpio_state = BIT(0) | BIT(1);
+
+	ret = devm_gpiochip_add_data(dev, gpio, priv);
+	if (ret)
+		dev_err(dev, "Unable to create gpio_chip\n");
+
+	dev_err(dev, "Created gpio_chip for MAX9286\n");
+
+	return ret;
+}
+
 static int max9286_init(struct device *dev)
 {
 	struct max9286_priv *priv;
@@ -984,6 +1044,14 @@ static int max9286_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	/*
+	 * It is possible to set up the power regulator from the GPIO lines,
+	 * so it needs to be set up early.
+	 */
+	ret = max9286_gpio(priv);
+	if (ret)
+		return ret;
+
 	priv->regulator = regulator_get(&client->dev, "poc");
 	if (IS_ERR(priv->regulator)) {
 		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
-- 
2.20.1

[PATCH 3/3] media: i2c: max9286: Provide optional enable-gpio

[Kieran Bingham]

The MAX9286 has a negated PWDN line which must be raised to enable the
device.  On the Eagle-V3M this pin is connected to a GPIO on the
io_expander.

Provide an enable-gpio dt property to specify the link, and ensure that
the line is handled in the driver accordingly.

This can also provide the abiltiy to manage low power states and
runtime-pm at a later date by fully powering down the chip.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index c34e7b5c7447..67065cd99d8d 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/fwnode.h>
+#include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
@@ -143,6 +144,7 @@ struct max9286_source {
 
 struct max9286_priv {
 	struct i2c_client *client;
+	struct gpio_desc *gpiod_pwdn;
 	struct v4l2_subdev sd;
 	struct media_pad pads[MAX9286_N_PADS];
 	struct regulator *regulator;
@@ -1044,6 +1046,14 @@ static int max9286_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
+						   GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->gpiod_pwdn))
+		return PTR_ERR(priv->gpiod_pwdn);
+
+	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
+	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
+
 	/*
 	 * It is possible to set up the power regulator from the GPIO lines,
 	 * so it needs to be set up early.
@@ -1117,6 +1127,9 @@ static int max9286_remove(struct i2c_client *client)
 	regulator_put(priv->regulator);
 
 	max9286_cleanup_dt(priv);
+
+	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
+
 	kfree(priv);
 
 	return 0;
-- 
2.20.1

Re: [PATCH 2/3] media: i2c: max9286: Add GPIO chip controller

[Kieran Bingham]

Hello me,

On 06/12/2019 14:05, Kieran Bingham wrote:
> Provide a GPIO chip to control the two output lines available on the
> MAX9286.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 68 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 6ea08fd87811..c34e7b5c7447 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -13,6 +13,7 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/fwnode.h>
> +#include <linux/gpio/driver.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/module.h>
> @@ -58,6 +59,8 @@
>  #define MAX9286_HVSRC_D0		(2 << 0)
>  #define MAX9286_HVSRC_D14		(1 << 0)
>  #define MAX9286_HVSRC_D18		(0 << 0)
> +/* Register 0x0f */
> +#define MAX9286_0X0F_RESERVED		BIT(3)
>  /* Register 0x12 */
>  #define MAX9286_CSILANECNT(n)		(((n) - 1) << 6)
>  #define MAX9286_CSIDBL			BIT(5)
> @@ -145,6 +148,9 @@ struct max9286_priv {
>  	struct regulator *regulator;
>  	bool poc_enabled;
>  
> +	struct gpio_chip gpio;
> +	u8 gpio_state;
> +
>  	struct i2c_mux_core *mux;
>  	unsigned int mux_channel;
>  	bool mux_open;
> @@ -712,6 +718,60 @@ static const struct of_device_id max9286_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, max9286_dt_ids);
>  
> +static void max9286_gpio_set(struct gpio_chip *chip,
> +			     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);
> +}
> +
> +static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct max9286_priv *priv = gpiochip_get_data(chip);
> +
> +	return priv->gpio_state & BIT(offset);
> +}
> +
> +static int max9286_gpio(struct max9286_priv *priv)
> +{
> +	struct device *dev = &priv->client->dev;
> +	struct gpio_chip *gpio = &priv->gpio;
> +	int ret;
> +
> +	static const char * const names[] = {
> +		"GPIO0OUT",
> +		"GPIO1OUT",
> +	};
> +
> +	/* Configure the GPIO */
> +	gpio->label = dev_name(dev);
> +	gpio->parent = dev;
> +	gpio->owner = THIS_MODULE;
> +	gpio->of_node = dev->of_node;
> +	gpio->ngpio = 2;
> +	gpio->set = max9286_gpio_set;
> +	gpio->get = max9286_gpio_get;
> +	gpio->can_sleep = true;
> +	gpio->names = names;
> +
> +	/* GPIO values default to high */
> +	priv->gpio_state = BIT(0) | BIT(1);
> +
> +	ret = devm_gpiochip_add_data(dev, gpio, priv);
> +	if (ret)
> +		dev_err(dev, "Unable to create gpio_chip\n");
> +
> +	dev_err(dev, "Created gpio_chip for MAX9286\n");

This debug line should be removed of course.

Now removed.

> +
> +	return ret;
> +}
> +
>  static int max9286_init(struct device *dev)
>  {
>  	struct max9286_priv *priv;
> @@ -984,6 +1044,14 @@ static int max9286_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * It is possible to set up the power regulator from the GPIO lines,
> +	 * so it needs to be set up early.
> +	 */
> +	ret = max9286_gpio(priv);
> +	if (ret)
> +		return ret;
> +
>  	priv->regulator = regulator_get(&client->dev, "poc");
>  	if (IS_ERR(priv->regulator)) {
>  		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> 

-- 
Regards
--
Kieran

Re: [PATCH 1/3] media: i2c: max9286: Remove redundant max9286_i2c_mux_state

[Geert Uytterhoeven]

Hi Kieran,

On Fri, Dec 6, 2019 at 3:05 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> While simplifying the i2c-mux state, the states were stored in an enum
> (initially there were three).
>
> This has now simplified down to 2 states, open and closed - and can be
> represented easily in a bool.
>
> It 'could' also be represented within the mux_channel, but I don't want
> to pollute that further than the '-1' value which is already stored in
> there to represent no channel selected.
>
> Remove the max9286_i2c_mux_state and replace with a bool mux_open flag,
> and move the location within the private struct to be more appropriate.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Thanks for your patch!

> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -144,10 +144,10 @@ struct max9286_priv {
>         struct media_pad pads[MAX9286_N_PADS];
>         struct regulator *regulator;
>         bool poc_enabled;
> -       int mux_state;
>
>         struct i2c_mux_core *mux;
>         unsigned int mux_channel;
> +       bool mux_open;

Please keep all booleans together, to fill up holes due to alignment
restrictions.

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

Re: [PATCH 1/3] media: i2c: max9286: Remove redundant max9286_i2c_mux_state

[Kieran Bingham]

Hi Geert,

On 06/12/2019 14:10, Geert Uytterhoeven wrote:
> Hi Kieran,
> 
> On Fri, Dec 6, 2019 at 3:05 PM Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
>> While simplifying the i2c-mux state, the states were stored in an enum
>> (initially there were three).
>>
>> This has now simplified down to 2 states, open and closed - and can be
>> represented easily in a bool.
>>
>> It 'could' also be represented within the mux_channel, but I don't want
>> to pollute that further than the '-1' value which is already stored in
>> there to represent no channel selected.
>>
>> Remove the max9286_i2c_mux_state and replace with a bool mux_open flag,
>> and move the location within the private struct to be more appropriate.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -144,10 +144,10 @@ struct max9286_priv {
>>         struct media_pad pads[MAX9286_N_PADS];
>>         struct regulator *regulator;
>>         bool poc_enabled;
>> -       int mux_state;
>>
>>         struct i2c_mux_core *mux;
>>         unsigned int mux_channel;
>> +       bool mux_open;
> 
> Please keep all booleans together, to fill up holes due to alignment
> restrictions.

I was trying to group related i2c_mux items, but I do indeed see a
strong argument there...

/me digs out pahole just to have a look :-D (but I know what the answer is)

struct max9286_priv {
struct i2c_client *        client;               /*     0     8 */
struct gpio_desc *         gpiod_pwdn;           /*     8     8 */
struct v4l2_subdev         sd;                   /*    16   320 */
/* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
struct media_pad           pads[5];              /*   336   280 */
/* --- cacheline 9 boundary (576 bytes) was 40 bytes ago --- */
struct regulator *         regulator;            /*   616     8 */
struct dentry *            dbgroot;              /*   624     8 */
bool                       poc_enabled;          /*   632     1 */

/* XXX 7 bytes hole, try to pack */
/* --- cacheline 10 boundary (640 bytes) --- */

struct gpio_chip           gpio;                 /*   640   600 */
/* --- cacheline 19 boundary (1216 bytes) was 24 bytes ago --- */

u8                         gpio_state;           /*  1240     1 */
/* XXX 7 bytes hole, try to pack */

struct i2c_mux_core *      mux;                  /*  1248     8 /
unsigned int               mux_channel;          /*  1256     4 */
bool                       mux_open;             /*  1260     1 */
/* XXX 3 bytes hole, try to pack */

struct v4l2_ctrl_handler   ctrls;                /*  1264   296 */
/* --- cacheline 24 boundary (1536 bytes) was 24 bytes ago --- */
struct v4l2_mbus_framefmt  fmt[4];               /*  1560   192 */
/* --- cacheline 27 boundary (1728 bytes) was 24 bytes ago --- */
unsigned int               nsources;             /*  1752     4 */
unsigned int               source_mask;          /*  1756     4 */
unsigned int               route_mask;           /*  1760     4 */
unsigned int               csi2_data_lanes;      /*  1764     4 */
struct max9286_source      sources[4];           /*  1768   288 */
/* --- cacheline 32 boundary (2048 bytes) was 8 bytes ago --- */
struct v4l2_async_notifier notifier;             /*  2056    96 */

/* size: 2152, cachelines: 34, members: 20 */
/* sum members: 2135, holes: 3, sum holes: 17 */
/* last cacheline: 40 bytes */
};



Hrm ... this one really pulls me in both directions ...
Which is the lesser evil - memory holes or ungrouped variables?

--
Kieran



> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH 3/3] media: i2c: max9286: Provide optional enable-gpio

[Kieran Bingham]

Hi all,

On 06/12/2019 14:05, Kieran Bingham wrote:
> The MAX9286 has a negated PWDN line which must be raised to enable the
> device.  On the Eagle-V3M this pin is connected to a GPIO on the
> io_expander.
> 
> Provide an enable-gpio dt property to specify the link, and ensure that
> the line is handled in the driver accordingly.
> 
> This can also provide the abiltiy to manage low power states and
> runtime-pm at a later date by fully powering down the chip.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

I'm happy that this one isn't going to cause any contention, and I'm
going to fold this one into the driver.

--
Kieran


> ---
>  drivers/media/i2c/max9286.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index c34e7b5c7447..67065cd99d8d 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -13,6 +13,7 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/fwnode.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
> @@ -143,6 +144,7 @@ struct max9286_source {
>  
>  struct max9286_priv {
>  	struct i2c_client *client;
> +	struct gpio_desc *gpiod_pwdn;
>  	struct v4l2_subdev sd;
>  	struct media_pad pads[MAX9286_N_PADS];
>  	struct regulator *regulator;
> @@ -1044,6 +1046,14 @@ static int max9286_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
> +						   GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->gpiod_pwdn))
> +		return PTR_ERR(priv->gpiod_pwdn);
> +
> +	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
> +	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
> +
>  	/*
>  	 * It is possible to set up the power regulator from the GPIO lines,
>  	 * so it needs to be set up early.
> @@ -1117,6 +1127,9 @@ static int max9286_remove(struct i2c_client *client)
>  	regulator_put(priv->regulator);
>  
>  	max9286_cleanup_dt(priv);
> +
> +	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> +
>  	kfree(priv);
>  
>  	return 0;
> 

-- 
Regards
--
Kieran

Re: [PATCH 2/3] media: i2c: max9286: Add GPIO chip controller

[Kieran Bingham]

Hi All,

On 06/12/2019 14:08, Kieran Bingham wrote:
> Hello me,
> 
> On 06/12/2019 14:05, Kieran Bingham wrote:
>> Provide a GPIO chip to control the two output lines available on the
>> MAX9286.

A separate mail thread to Mark Brown regarding the regulator topic [0]
has suggested that this should be implemented as an MFD.

Does anyone have any comments on this approach? I know we've discussed
the use of the MFD framework on this driver before ... thus perhaps it
might be a bit contentious ...

Due to this, I am not yet going to fold this patch into the max9286 driver.

[0] Regulator probe on demand (or circular dependencies)
 https://lore.kernel.org/linux-renesas-soc/20191209171639.GA27340@bigcity.dyn.berto.se/T/#t

--
Regards

Kieran



>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  drivers/media/i2c/max9286.c | 68 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 68 insertions(+)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index 6ea08fd87811..c34e7b5c7447 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/device.h>
>>  #include <linux/fwnode.h>
>> +#include <linux/gpio/driver.h>
>>  #include <linux/i2c.h>
>>  #include <linux/i2c-mux.h>
>>  #include <linux/module.h>
>> @@ -58,6 +59,8 @@
>>  #define MAX9286_HVSRC_D0		(2 << 0)
>>  #define MAX9286_HVSRC_D14		(1 << 0)
>>  #define MAX9286_HVSRC_D18		(0 << 0)
>> +/* Register 0x0f */
>> +#define MAX9286_0X0F_RESERVED		BIT(3)
>>  /* Register 0x12 */
>>  #define MAX9286_CSILANECNT(n)		(((n) - 1) << 6)
>>  #define MAX9286_CSIDBL			BIT(5)
>> @@ -145,6 +148,9 @@ struct max9286_priv {
>>  	struct regulator *regulator;
>>  	bool poc_enabled;
>>  
>> +	struct gpio_chip gpio;
>> +	u8 gpio_state;
>> +
>>  	struct i2c_mux_core *mux;
>>  	unsigned int mux_channel;
>>  	bool mux_open;
>> @@ -712,6 +718,60 @@ static const struct of_device_id max9286_dt_ids[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, max9286_dt_ids);
>>  
>> +static void max9286_gpio_set(struct gpio_chip *chip,
>> +			     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);
>> +}
>> +
>> +static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +	struct max9286_priv *priv = gpiochip_get_data(chip);
>> +
>> +	return priv->gpio_state & BIT(offset);
>> +}
>> +
>> +static int max9286_gpio(struct max9286_priv *priv)
>> +{
>> +	struct device *dev = &priv->client->dev;
>> +	struct gpio_chip *gpio = &priv->gpio;
>> +	int ret;
>> +
>> +	static const char * const names[] = {
>> +		"GPIO0OUT",
>> +		"GPIO1OUT",
>> +	};
>> +
>> +	/* Configure the GPIO */
>> +	gpio->label = dev_name(dev);
>> +	gpio->parent = dev;
>> +	gpio->owner = THIS_MODULE;
>> +	gpio->of_node = dev->of_node;
>> +	gpio->ngpio = 2;
>> +	gpio->set = max9286_gpio_set;
>> +	gpio->get = max9286_gpio_get;
>> +	gpio->can_sleep = true;
>> +	gpio->names = names;
>> +
>> +	/* GPIO values default to high */
>> +	priv->gpio_state = BIT(0) | BIT(1);
>> +
>> +	ret = devm_gpiochip_add_data(dev, gpio, priv);
>> +	if (ret)
>> +		dev_err(dev, "Unable to create gpio_chip\n");
>> +
>> +	dev_err(dev, "Created gpio_chip for MAX9286\n");
> 
> This debug line should be removed of course.
> 
> Now removed.
> 
>> +
>> +	return ret;
>> +}
>> +
>>  static int max9286_init(struct device *dev)
>>  {
>>  	struct max9286_priv *priv;
>> @@ -984,6 +1044,14 @@ static int max9286_probe(struct i2c_client *client)
>>  	if (ret)
>>  		return ret;
>>  
>> +	/*
>> +	 * It is possible to set up the power regulator from the GPIO lines,
>> +	 * so it needs to be set up early.
>> +	 */
>> +	ret = max9286_gpio(priv);
>> +	if (ret)
>> +		return ret;
>> +
>>  	priv->regulator = regulator_get(&client->dev, "poc");
>>  	if (IS_ERR(priv->regulator)) {
>>  		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
>>
> 

-- 
Regards
--
Kieran

Re: [PATCH 1/3] media: i2c: max9286: Remove redundant max9286_i2c_mux_state

[Kieran Bingham]

Hi All,

On 06/12/2019 14:22, Kieran Bingham wrote:
> Hi Geert,
> 
> On 06/12/2019 14:10, Geert Uytterhoeven wrote:
>> Hi Kieran,
>>
>> On Fri, Dec 6, 2019 at 3:05 PM Kieran Bingham
>> <kieran.bingham@ideasonboard.com> wrote:
>>> While simplifying the i2c-mux state, the states were stored in an enum
>>> (initially there were three).
>>>
>>> This has now simplified down to 2 states, open and closed - and can be
>>> represented easily in a bool.
>>>
>>> It 'could' also be represented within the mux_channel, but I don't want
>>> to pollute that further than the '-1' value which is already stored in
>>> there to represent no channel selected.
>>>
>>> Remove the max9286_i2c_mux_state and replace with a bool mux_open flag,
>>> and move the location within the private struct to be more appropriate.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Folding this patch into the max9286 driver without correcting for holes,
as I believe it's better to group the associated variables together, and
accept the small loss, as the structure is very large so it's a small
proportion.

--
Kieran


>>
>> Thanks for your patch!
>>
>>> --- a/drivers/media/i2c/max9286.c
>>> +++ b/drivers/media/i2c/max9286.c
>>> @@ -144,10 +144,10 @@ struct max9286_priv {
>>>         struct media_pad pads[MAX9286_N_PADS];
>>>         struct regulator *regulator;
>>>         bool poc_enabled;
>>> -       int mux_state;
>>>
>>>         struct i2c_mux_core *mux;
>>>         unsigned int mux_channel;
>>> +       bool mux_open;
>>
>> Please keep all booleans together, to fill up holes due to alignment
>> restrictions.
> 
> I was trying to group related i2c_mux items, but I do indeed see a
> strong argument there...
> 
> /me digs out pahole just to have a look :-D (but I know what the answer is)
> 
> struct max9286_priv {
> struct i2c_client *        client;               /*     0     8 */
> struct gpio_desc *         gpiod_pwdn;           /*     8     8 */
> struct v4l2_subdev         sd;                   /*    16   320 */
> /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
> struct media_pad           pads[5];              /*   336   280 */
> /* --- cacheline 9 boundary (576 bytes) was 40 bytes ago --- */
> struct regulator *         regulator;            /*   616     8 */
> struct dentry *            dbgroot;              /*   624     8 */
> bool                       poc_enabled;          /*   632     1 */
> 
> /* XXX 7 bytes hole, try to pack */
> /* --- cacheline 10 boundary (640 bytes) --- */
> 
> struct gpio_chip           gpio;                 /*   640   600 */
> /* --- cacheline 19 boundary (1216 bytes) was 24 bytes ago --- */
> 
> u8                         gpio_state;           /*  1240     1 */
> /* XXX 7 bytes hole, try to pack */
> 
> struct i2c_mux_core *      mux;                  /*  1248     8 /
> unsigned int               mux_channel;          /*  1256     4 */
> bool                       mux_open;             /*  1260     1 */
> /* XXX 3 bytes hole, try to pack */
> 
> struct v4l2_ctrl_handler   ctrls;                /*  1264   296 */
> /* --- cacheline 24 boundary (1536 bytes) was 24 bytes ago --- */
> struct v4l2_mbus_framefmt  fmt[4];               /*  1560   192 */
> /* --- cacheline 27 boundary (1728 bytes) was 24 bytes ago --- */
> unsigned int               nsources;             /*  1752     4 */
> unsigned int               source_mask;          /*  1756     4 */
> unsigned int               route_mask;           /*  1760     4 */
> unsigned int               csi2_data_lanes;      /*  1764     4 */
> struct max9286_source      sources[4];           /*  1768   288 */
> /* --- cacheline 32 boundary (2048 bytes) was 8 bytes ago --- */
> struct v4l2_async_notifier notifier;             /*  2056    96 */
> 
> /* size: 2152, cachelines: 34, members: 20 */
> /* sum members: 2135, holes: 3, sum holes: 17 */
> /* last cacheline: 40 bytes */
> };
> 
> 
> 
> Hrm ... this one really pulls me in both directions ...
> Which is the lesser evil - memory holes or ungrouped variables?
> 
> --
> Kieran
> 
> 
> 
>> Gr{oetje,eeting}s,
>>
>>                         Geert
>>

-- 
Regards
--
Kieran

[PATCH] dt-bindings: media: i2c: max9286: Specify 'type'

[Jacopo Mondi]

Add the 'type' tag to a few entries to fix the warning reported by the
most recent dtschema validator

Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml:
properties:i2c-mux: 'type' is a dependency of 'properties'

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Hi Kieran,
   I know you're collecting patches to re-submit max9286 support.

Please squash this on top of the dt bindings documentation patch to fix
an issue reported by recent version of the dtschema validator.

Thanks
  j

---
 Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 731e317ef4ce..d37ea2c432f6 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -73,6 +73,7 @@ properties:
         const: 0

       port@[0-3]:
+        type: object
         properties:
           reg:
             enum: [ 0, 1, 2, 3 ]
@@ -97,6 +98,7 @@ properties:
         additionalProperties: false

       port@4:
+        type: object
         properties:
           reg:
             const: 4
@@ -126,6 +128,7 @@ properties:
       - port@4

   i2c-mux:
+    type: object
     description: -|
       Each GMSL link is modelled as a child bus of an i2c bus
       multiplexer/switch, in accordance with bindings described in
--
2.24.0