[PATCH v4 00/15] media: i2c: ds90ub9xx: Misc fixes and improvements

[PATCH v4 00/15] media: i2c: ds90ub9xx: Misc fixes and improvements

[Tomi Valkeinen]

This series fixes various small issues in the drivers, and adds a few
things (a couple of pixel formats and a debugging feature).

It also takes a few steps in adding more i2c read/write error handlings
to the drivers, but covers only the easy places.

Adding error handling to all reads/writes needs more thinking, perhaps
adding a "ret" parameter to the calls, similar to the cci_* functions,
or perhaps adding helpers for writing multiple registers from a given
table. Also, in some places rolling back from an error will require
work.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Changes in v4:
- Add Jai's Rb
- Use HZ_PER_MHZ in MHZ() macro
- Use num_rxports when setting up the DEBUG_I2C_RX_ID
- Add Reported-by's to patches that add error handling. Note: The
  patches don't close the issue, so I use "Link:" instead of "Closes:"
  as directed in Documentation/process/5.Posting.rst. However, checkpatch
  seems to want "Closes", so it warns about these.
- Link to v3: https://lore.kernel.org/r/20241204-ub9xx-fixes-v3-0-a933c109b323@ideasonboard.com

Changes in v3:
- Include bitfield.h for FIELD_PREP()
- Cc stable for relevant fixes
- Link to v2: https://lore.kernel.org/r/20241108-ub9xx-fixes-v2-0-c7db3b2ad89f@ideasonboard.com

Changes in v2:
- Address comments from Andy
- Add two new patches:
	- media: i2c: ds90ub960: Fix shadowing of local variables
	- media: i2c: ds90ub960: Use HZ_PER_MHZ
- Link to v1: https://lore.kernel.org/r/20241004-ub9xx-fixes-v1-0-e30a4633c786@ideasonboard.com

---
Tomi Valkeinen (15):
      media: i2c: ds90ub9x3: Fix extra fwnode_handle_put()
      media: i2c: ds90ub960: Fix UB9702 refclk register access
      media: i2c: ds90ub960: Fix use of non-existing registers on UB9702
      media: i2c: ds90ub960: Fix logging SP & EQ status only for UB9702
      media: i2c: ds90ub960: Fix UB9702 VC map
      media: i2c: ds90ub960: Use HZ_PER_MHZ
      media: i2c: ds90ub960: Add support for I2C_RX_ID
      media: i2c: ds90ub960: Add RGB24, RAW8 and RAW10 formats
      media: i2c: ds90ub953: Clear CRC errors in ub953_log_status()
      media: i2c: ds90ub960: Drop unused indirect block define
      media: i2c: ds90ub960: Reduce sleep in ub960_rxport_wait_locks()
      media: i2c: ds90ub960: Handle errors in ub960_log_status_ub960_sp_eq()
      media: i2c: ds90ub913: Add error handling to ub913_hw_init()
      media: i2c: ds90ub953: Add error handling for i2c reads/writes
      media: i2c: ds90ub960: Fix shadowing of local variables

 drivers/media/i2c/ds90ub913.c |  26 ++++--
 drivers/media/i2c/ds90ub953.c |  56 +++++++++----
 drivers/media/i2c/ds90ub960.c | 188 ++++++++++++++++++++++++++++--------------
 3 files changed, 188 insertions(+), 82 deletions(-)
---
base-commit: adc218676eef25575469234709c2d87185ca223a
change-id: 20241004-ub9xx-fixes-bba80dc48627

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH v4 01/15] media: i2c: ds90ub9x3: Fix extra fwnode_handle_put()

[Tomi Valkeinen]

The ub913 and ub953 drivers call fwnode_handle_put(priv->sd.fwnode) as
part of their remove process, and if the driver is removed multiple
times, eventually leads to put "overflow", possibly causing memory
corruption or crash.

The fwnode_handle_put() is a leftover from commit 905f88ccebb1 ("media:
i2c: ds90ub9x3: Fix sub-device matching"), which changed the code
related to the sd.fwnode, but missed removing these fwnode_handle_put()
calls.

Cc: stable@vger.kernel.org
Fixes: 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching")
Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub913.c | 1 -
 drivers/media/i2c/ds90ub953.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index 8eed4a200fd8..b5375d736629 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -793,7 +793,6 @@ static void ub913_subdev_uninit(struct ub913_data *priv)
 	v4l2_async_unregister_subdev(&priv->sd);
 	ub913_v4l2_nf_unregister(priv);
 	v4l2_subdev_cleanup(&priv->sd);
-	fwnode_handle_put(priv->sd.fwnode);
 	media_entity_cleanup(&priv->sd.entity);
 }
 
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index 16f88db14981..10daecf6f457 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -1291,7 +1291,6 @@ static void ub953_subdev_uninit(struct ub953_data *priv)
 	v4l2_async_unregister_subdev(&priv->sd);
 	ub953_v4l2_notifier_unregister(priv);
 	v4l2_subdev_cleanup(&priv->sd);
-	fwnode_handle_put(priv->sd.fwnode);
 	media_entity_cleanup(&priv->sd.entity);
 }
 

-- 
2.43.0

[PATCH v4 02/15] media: i2c: ds90ub960: Fix UB9702 refclk register access

[Tomi Valkeinen]

UB9702 has the refclk freq register at a different offset than UB960,
but the code uses the UB960's offset for both chips. Fix this.

The refclk freq is only used for a debug print, so there's no functional
change here.

Cc: stable@vger.kernel.org
Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index ffe5f25f8647..b1e848678218 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -352,6 +352,8 @@
 
 #define UB960_SR_I2C_RX_ID(n)			(0xf8 + (n)) /* < UB960_FPD_RX_NPORTS */
 
+#define UB9702_SR_REFCLK_FREQ			0x3d
+
 /* Indirect register blocks */
 #define UB960_IND_TARGET_PAT_GEN		0x00
 #define UB960_IND_TARGET_RX_ANA(n)		(0x01 + (n))
@@ -3837,7 +3839,10 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
 	if (ret)
 		goto err_pd_gpio;
 
-	ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq);
+	if (priv->hw_data->is_ub9702)
+		ret = ub960_read(priv, UB9702_SR_REFCLK_FREQ, &refclk_freq);
+	else
+		ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq);
 	if (ret)
 		goto err_pd_gpio;
 

-- 
2.43.0

[PATCH v4 03/15] media: i2c: ds90ub960: Fix use of non-existing registers on UB9702

[Tomi Valkeinen]

UB9702 doesn't have the registers for SP and EQ. Adjust the code in
ub960_rxport_wait_locks() to not use those registers for UB9702. As
these values are only used for a debug print here, there's no functional
change.

Cc: stable@vger.kernel.org
Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index b1e848678218..24198b803eff 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -1577,16 +1577,24 @@ static int ub960_rxport_wait_locks(struct ub960_data *priv,
 
 		ub960_rxport_read16(priv, nport, UB960_RR_RX_FREQ_HIGH, &v);
 
-		ret = ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);
-		if (ret)
-			return ret;
+		if (priv->hw_data->is_ub9702) {
+			dev_dbg(dev, "\trx%u: locked, freq %llu Hz\n",
+				nport, (v * 1000000ULL) >> 8);
+		} else {
+			ret = ub960_rxport_get_strobe_pos(priv, nport,
+							  &strobe_pos);
+			if (ret)
+				return ret;
 
-		ret = ub960_rxport_get_eq_level(priv, nport, &eq_level);
-		if (ret)
-			return ret;
+			ret = ub960_rxport_get_eq_level(priv, nport, &eq_level);
+			if (ret)
+				return ret;
 
-		dev_dbg(dev, "\trx%u: locked, SP: %d, EQ: %u, freq %llu Hz\n",
-			nport, strobe_pos, eq_level, (v * 1000000ULL) >> 8);
+			dev_dbg(dev,
+				"\trx%u: locked, SP: %d, EQ: %u, freq %llu Hz\n",
+				nport, strobe_pos, eq_level,
+				(v * 1000000ULL) >> 8);
+		}
 	}
 
 	return 0;

-- 
2.43.0

[PATCH v4 04/15] media: i2c: ds90ub960: Fix logging SP & EQ status only for UB9702

[Tomi Valkeinen]

UB9702 does not have SP and EQ registers, but the driver uses them in
log_status(). Fix this by separating the SP and EQ related log_status()
work into a separate function (for clarity) and calling that function
only for UB960.

Cc: stable@vger.kernel.org
Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 90 ++++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 40 deletions(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 24198b803eff..94c8acf171b4 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -2950,6 +2950,54 @@ static const struct v4l2_subdev_pad_ops ub960_pad_ops = {
 	.set_fmt = ub960_set_fmt,
 };
 
+static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
+					 unsigned int nport)
+{
+	struct device *dev = &priv->client->dev;
+	u8 eq_level;
+	s8 strobe_pos;
+	u8 v = 0;
+
+	/* Strobe */
+
+	ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
+
+	dev_info(dev, "\t%s strobe\n",
+		 (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" :
+							  "Manual");
+
+	if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) {
+		ub960_read(priv, UB960_XR_SFILTER_CFG, &v);
+
+		dev_info(dev, "\tStrobe range [%d, %d]\n",
+			 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7,
+			 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7);
+	}
+
+	ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);
+
+	dev_info(dev, "\tStrobe pos %d\n", strobe_pos);
+
+	/* EQ */
+
+	ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);
+
+	dev_info(dev, "\t%s EQ\n",
+		 (v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" :
+						    "Adaptive");
+
+	if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) {
+		ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);
+
+		dev_info(dev, "\tEQ range [%u, %u]\n",
+			 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf,
+			 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf);
+	}
+
+	if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0)
+		dev_info(dev, "\tEQ level %u\n", eq_level);
+}
+
 static int ub960_log_status(struct v4l2_subdev *sd)
 {
 	struct ub960_data *priv = sd_to_ub960(sd);
@@ -2997,8 +3045,6 @@ static int ub960_log_status(struct v4l2_subdev *sd)
 
 	for (nport = 0; nport < priv->hw_data->num_rxports; nport++) {
 		struct ub960_rxport *rxport = priv->rxports[nport];
-		u8 eq_level;
-		s8 strobe_pos;
 		unsigned int i;
 
 		dev_info(dev, "RX %u\n", nport);
@@ -3034,44 +3080,8 @@ static int ub960_log_status(struct v4l2_subdev *sd)
 		ub960_rxport_read(priv, nport, UB960_RR_CSI_ERR_COUNTER, &v);
 		dev_info(dev, "\tcsi_err_counter %u\n", v);
 
-		/* Strobe */
-
-		ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
-
-		dev_info(dev, "\t%s strobe\n",
-			 (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" :
-								  "Manual");
-
-		if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) {
-			ub960_read(priv, UB960_XR_SFILTER_CFG, &v);
-
-			dev_info(dev, "\tStrobe range [%d, %d]\n",
-				 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7,
-				 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7);
-		}
-
-		ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);
-
-		dev_info(dev, "\tStrobe pos %d\n", strobe_pos);
-
-		/* EQ */
-
-		ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);
-
-		dev_info(dev, "\t%s EQ\n",
-			 (v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" :
-							    "Adaptive");
-
-		if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) {
-			ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);
-
-			dev_info(dev, "\tEQ range [%u, %u]\n",
-				 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf,
-				 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf);
-		}
-
-		if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0)
-			dev_info(dev, "\tEQ level %u\n", eq_level);
+		if (!priv->hw_data->is_ub9702)
+			ub960_log_status_ub960_sp_eq(priv, nport);
 
 		/* GPIOs */
 		for (i = 0; i < UB960_NUM_BC_GPIOS; i++) {

-- 
2.43.0

[PATCH v4 05/15] media: i2c: ds90ub960: Fix UB9702 VC map

[Tomi Valkeinen]

The driver uses a static CSI-2 virtual channel mapping where all virtual
channels from an RX port are mapped to a virtual channel number matching
the RX port number.

The UB960 and UB9702 have different registers for the purpose, and the
UB9702 version is not correct. Each of the VC_ID_MAP registers do not
contain a single mapping, as the driver currently thinks, but two.

This can cause received VCs other than 0 to be mapped in a wrong way.

Fix this by writing both mappings to each register.

Cc: stable@vger.kernel.org
Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 94c8acf171b4..bfffa14e2049 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -2533,7 +2533,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv,
 				for (i = 0; i < 8; i++)
 					ub960_rxport_write(priv, nport,
 							   UB960_RR_VC_ID_MAP(i),
-							   nport);
+							   (nport << 4) | nport);
 			}
 
 			break;

-- 
2.43.0

[PATCH v4 06/15] media: i2c: ds90ub960: Use HZ_PER_MHZ

[Tomi Valkeinen]

Use HZ_PER_MHZ instead of 1000000U.

Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index bfffa14e2049..84631909635c 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -43,6 +43,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
+#include <linux/units.h>
 #include <linux/workqueue.h>
 
 #include <media/i2c/ds90ub9xx.h>
@@ -52,7 +53,7 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
-#define MHZ(v) ((u32)((v) * 1000000U))
+#define MHZ(v) ((u32)((v) * HZ_PER_MHZ))
 
 #define UB960_POLL_TIME_MS	500
 
@@ -1579,7 +1580,7 @@ static int ub960_rxport_wait_locks(struct ub960_data *priv,
 
 		if (priv->hw_data->is_ub9702) {
 			dev_dbg(dev, "\trx%u: locked, freq %llu Hz\n",
-				nport, (v * 1000000ULL) >> 8);
+				nport, ((u64)v * HZ_PER_MHZ) >> 8);
 		} else {
 			ret = ub960_rxport_get_strobe_pos(priv, nport,
 							  &strobe_pos);
@@ -1593,7 +1594,7 @@ static int ub960_rxport_wait_locks(struct ub960_data *priv,
 			dev_dbg(dev,
 				"\trx%u: locked, SP: %d, EQ: %u, freq %llu Hz\n",
 				nport, strobe_pos, eq_level,
-				(v * 1000000ULL) >> 8);
+				((u64)v * HZ_PER_MHZ) >> 8);
 		}
 	}
 
@@ -3066,7 +3067,7 @@ static int ub960_log_status(struct v4l2_subdev *sd)
 		dev_info(dev, "\trx_port_sts2 %#02x\n", v);
 
 		ub960_rxport_read16(priv, nport, UB960_RR_RX_FREQ_HIGH, &v16);
-		dev_info(dev, "\tlink freq %llu Hz\n", (v16 * 1000000ULL) >> 8);
+		dev_info(dev, "\tlink freq %llu Hz\n", ((u64)v16 * HZ_PER_MHZ) >> 8);
 
 		ub960_rxport_read16(priv, nport, UB960_RR_RX_PAR_ERR_HI, &v16);
 		dev_info(dev, "\tparity errors %u\n", v16);
@@ -3866,7 +3867,7 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
 
 	dev_dbg(dev, "refclk valid %u freq %u MHz (clk fw freq %lu MHz)\n",
 		!!(dev_sts & BIT(4)), refclk_freq,
-		clk_get_rate(priv->refclk) / 1000000);
+		clk_get_rate(priv->refclk) / HZ_PER_MHZ);
 
 	/* Disable all RX ports by default */
 	ret = ub960_write(priv, UB960_SR_RX_PORT_CTL, 0);

-- 
2.43.0

[PATCH v4 07/15] media: i2c: ds90ub960: Add support for I2C_RX_ID

[Tomi Valkeinen]

Normally the driver accesses both the RX and the TX port registers via a
paging mechanism: one register is used to select the page (i.e. the
port), which dictates the port used when accessing the port specific
registers.

The downside to this is that while debugging it's almost impossible to
access the port specific registers from the userspace, as the driver can
change the page at any moment.

The hardware supports another access mechanism: using the I2C_RX_ID
registers (one for each RX port), i2c addresses can be chosen which,
when accessed, will always use the specific port's registers, skipping
the paging mechanism.

The support is only for the RX port, but it has proven very handy while
debugging and testing. So let's add the code for this, but hide it
behind a disabled define.

Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 84631909635c..bd575bca2b42 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -55,6 +55,15 @@
 
 #define MHZ(v) ((u32)((v) * HZ_PER_MHZ))
 
+/*
+ * If this is defined, the i2c addresses from UB960_DEBUG_I2C_RX_ID to
+ * UB960_DEBUG_I2C_RX_ID + 3 can be used to access the paged RX port registers
+ * directly.
+ *
+ * Only for debug purposes.
+ */
+/* #define UB960_DEBUG_I2C_RX_ID	0x40 */
+
 #define UB960_POLL_TIME_MS	500
 
 #define UB960_MAX_RX_NPORTS	4
@@ -351,7 +360,7 @@
 #define UB960_SR_FPD3_RX_ID(n)			(0xf0 + (n))
 #define UB960_SR_FPD3_RX_ID_LEN			6
 
-#define UB960_SR_I2C_RX_ID(n)			(0xf8 + (n)) /* < UB960_FPD_RX_NPORTS */
+#define UB960_SR_I2C_RX_ID(n)			(0xf8 + (n))
 
 #define UB9702_SR_REFCLK_FREQ			0x3d
 
@@ -4001,6 +4010,12 @@ static int ub960_probe(struct i2c_client *client)
 	schedule_delayed_work(&priv->poll_work,
 			      msecs_to_jiffies(UB960_POLL_TIME_MS));
 
+#ifdef UB960_DEBUG_I2C_RX_ID
+	for (unsigned int i = 0; i < priv->hw_data->num_rxports; i++)
+		ub960_write(priv, UB960_SR_I2C_RX_ID(i),
+			    (UB960_DEBUG_I2C_RX_ID + i) << 1);
+#endif
+
 	return 0;
 
 err_free_sers:

-- 
2.43.0

[PATCH v4 08/15] media: i2c: ds90ub960: Add RGB24, RAW8 and RAW10 formats

[Tomi Valkeinen]

Add RGB24 and RAW8 and RAW10 bayer formats. RGB24 is mostly for TPG
purposes, but RAW8 and RAW10 are widely used by sensors.

Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index bd575bca2b42..721898e5c913 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -581,11 +581,23 @@ struct ub960_format_info {
 };
 
 static const struct ub960_format_info ub960_formats[] = {
+	{ .code = MEDIA_BUS_FMT_RGB888_1X24, .bpp = 24, .datatype = MIPI_CSI2_DT_RGB888, },
+
 	{ .code = MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, .datatype = MIPI_CSI2_DT_YUV422_8B, },
 	{ .code = MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, .datatype = MIPI_CSI2_DT_YUV422_8B, },
 	{ .code = MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, .datatype = MIPI_CSI2_DT_YUV422_8B, },
 	{ .code = MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, .datatype = MIPI_CSI2_DT_YUV422_8B, },
 
+	{ .code = MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, .datatype = MIPI_CSI2_DT_RAW8, },
+	{ .code = MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, .datatype = MIPI_CSI2_DT_RAW8, },
+	{ .code = MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, .datatype = MIPI_CSI2_DT_RAW8, },
+	{ .code = MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, .datatype = MIPI_CSI2_DT_RAW8, },
+
+	{ .code = MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, .datatype = MIPI_CSI2_DT_RAW10, },
+	{ .code = MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, .datatype = MIPI_CSI2_DT_RAW10, },
+	{ .code = MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, .datatype = MIPI_CSI2_DT_RAW10, },
+	{ .code = MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, .datatype = MIPI_CSI2_DT_RAW10, },
+
 	{ .code = MEDIA_BUS_FMT_SBGGR12_1X12, .bpp = 12, .datatype = MIPI_CSI2_DT_RAW12, },
 	{ .code = MEDIA_BUS_FMT_SGBRG12_1X12, .bpp = 12, .datatype = MIPI_CSI2_DT_RAW12, },
 	{ .code = MEDIA_BUS_FMT_SGRBG12_1X12, .bpp = 12, .datatype = MIPI_CSI2_DT_RAW12, },

-- 
2.43.0

[PATCH v4 09/15] media: i2c: ds90ub953: Clear CRC errors in ub953_log_status()

[Tomi Valkeinen]

Clear the CRC error counter after showing it in ub953_log_status() to
make its behavior match the other counter values.

Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub953.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index 10daecf6f457..b6451811f906 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -66,6 +66,9 @@
 #define UB953_REG_GPIO_INPUT_CTRL_OUT_EN(n)	BIT(4 + (n))
 #define UB953_REG_GPIO_INPUT_CTRL_INPUT_EN(n)	BIT(0 + (n))
 
+#define UB953_REG_BC_CTRL			0x49
+#define UB953_REG_BC_CTRL_CRC_ERR_CLR		BIT(3)
+
 #define UB953_REG_REV_MASK_ID			0x50
 #define UB953_REG_GENERAL_STATUS		0x52
 
@@ -619,6 +622,12 @@ static int ub953_log_status(struct v4l2_subdev *sd)
 	ub953_read(priv, UB953_REG_CRC_ERR_CNT2, &v2);
 	dev_info(dev, "CRC error count %u\n", v1 | (v2 << 8));
 
+	/* Clear CRC error counter */
+	if (v1 || v2)
+		regmap_update_bits(priv->regmap, UB953_REG_BC_CTRL,
+				   UB953_REG_BC_CTRL_CRC_ERR_CLR,
+				   UB953_REG_BC_CTRL_CRC_ERR_CLR);
+
 	ub953_read(priv, UB953_REG_CSI_ERR_CNT, &v);
 	dev_info(dev, "CSI error count %u\n", v);
 

-- 
2.43.0

[PATCH v4 10/15] media: i2c: ds90ub960: Drop unused indirect block define

[Tomi Valkeinen]

Drop the unused UB960_IND_TARGET_CSI_CSIPLL_REG_1 define. It does not
even match to any block in the more recent documents, so it's possible
it is not only unused but also wrong.

Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 721898e5c913..51b4aead6ac6 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -367,7 +367,6 @@
 /* Indirect register blocks */
 #define UB960_IND_TARGET_PAT_GEN		0x00
 #define UB960_IND_TARGET_RX_ANA(n)		(0x01 + (n))
-#define UB960_IND_TARGET_CSI_CSIPLL_REG_1	0x92	/* UB9702 */
 #define UB960_IND_TARGET_CSI_ANA		0x07
 
 /* UB960_IR_PGEN_*: Indirect Registers for Test Pattern Generator */

-- 
2.43.0

[PATCH v4 11/15] media: i2c: ds90ub960: Reduce sleep in ub960_rxport_wait_locks()

[Tomi Valkeinen]

We currently sleep for 50 ms at the end of each iteration in
ub960_rxport_wait_locks(). This feels a bit excessive, especially as we
always do at least two loops, so there's always at least one sleep, even
if we already have a stable lock.

Change the sleep to 10 ms.

Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 51b4aead6ac6..8e8cda4475af 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -1576,7 +1576,12 @@ static int ub960_rxport_wait_locks(struct ub960_data *priv,
 		if (missing == 0)
 			break;
 
-		msleep(50);
+		/*
+		 * The sleep time of 10 ms was found by testing to give a lock
+		 * with a few iterations. It can be decreased if on some setups
+		 * the lock can be achieved much faster.
+		 */
+		fsleep(10 * USEC_PER_MSEC);
 	}
 
 	if (lock_mask)

-- 
2.43.0

[PATCH v4 12/15] media: i2c: ds90ub960: Handle errors in ub960_log_status_ub960_sp_eq()

[Tomi Valkeinen]

Add error handling for i2c read/write calls to
ub960_log_status_ub960_sp_eq().

Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Link: https://lore.kernel.org/all/Zv40EQSR__JDN_0M@kekkonen.localdomain/
Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 8e8cda4475af..0db4fc980912 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -2982,38 +2982,49 @@ static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
 	struct device *dev = &priv->client->dev;
 	u8 eq_level;
 	s8 strobe_pos;
-	u8 v = 0;
+	int ret;
+	u8 v;
 
 	/* Strobe */
 
-	ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
+	ret = ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
+	if (ret)
+		return;
 
 	dev_info(dev, "\t%s strobe\n",
 		 (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" :
 							  "Manual");
 
 	if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) {
-		ub960_read(priv, UB960_XR_SFILTER_CFG, &v);
+		ret = ub960_read(priv, UB960_XR_SFILTER_CFG, &v);
+		if (ret)
+			return;
 
 		dev_info(dev, "\tStrobe range [%d, %d]\n",
 			 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7,
 			 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7);
 	}
 
-	ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);
+	ret = ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);
+	if (ret)
+		return;
 
 	dev_info(dev, "\tStrobe pos %d\n", strobe_pos);
 
 	/* EQ */
 
-	ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);
+	ret = ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);
+	if (ret)
+		return;
 
 	dev_info(dev, "\t%s EQ\n",
 		 (v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" :
 						    "Adaptive");
 
 	if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) {
-		ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);
+		ret = ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);
+		if (ret)
+			return;
 
 		dev_info(dev, "\tEQ range [%u, %u]\n",
 			 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf,

-- 
2.43.0

[PATCH v4 13/15] media: i2c: ds90ub913: Add error handling to ub913_hw_init()

[Tomi Valkeinen]

Add error handling to ub913_hw_init() using a new helper function,
ub913_update_bits().

Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Link: https://lore.kernel.org/all/Zv40EQSR__JDN_0M@kekkonen.localdomain/
Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub913.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index b5375d736629..7670d6c82d92 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -8,6 +8,7 @@
  * Copyright (c) 2023 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk-provider.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
@@ -146,6 +147,19 @@ static int ub913_write(const struct ub913_data *priv, u8 reg, u8 val)
 	return ret;
 }
 
+static int ub913_update_bits(const struct ub913_data *priv, u8 reg, u8 mask,
+			     u8 val)
+{
+	int ret;
+
+	ret = regmap_update_bits(priv->regmap, reg, mask, val);
+	if (ret < 0)
+		dev_err(&priv->client->dev,
+			"Cannot update register 0x%02x %d!\n", reg, ret);
+
+	return ret;
+}
+
 /*
  * GPIO chip
  */
@@ -733,10 +747,13 @@ static int ub913_hw_init(struct ub913_data *priv)
 	if (ret)
 		return dev_err_probe(dev, ret, "i2c master init failed\n");
 
-	ub913_read(priv, UB913_REG_GENERAL_CFG, &v);
-	v &= ~UB913_REG_GENERAL_CFG_PCLK_RISING;
-	v |= priv->pclk_polarity_rising ? UB913_REG_GENERAL_CFG_PCLK_RISING : 0;
-	ub913_write(priv, UB913_REG_GENERAL_CFG, v);
+	ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
+				UB913_REG_GENERAL_CFG_PCLK_RISING,
+				FIELD_PREP(UB913_REG_GENERAL_CFG_PCLK_RISING,
+					   priv->pclk_polarity_rising));
+
+	if (ret)
+		return ret;
 
 	return 0;
 }

-- 
2.43.0

[PATCH v4 14/15] media: i2c: ds90ub953: Add error handling for i2c reads/writes

[Tomi Valkeinen]

Add error handling for i2c reads/writes in various places.

Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Link: https://lore.kernel.org/all/Zv40EQSR__JDN_0M@kekkonen.localdomain/
Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub953.c | 46 ++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index b6451811f906..f8f3e31f0077 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -401,8 +401,13 @@ static int ub953_gpiochip_probe(struct ub953_data *priv)
 	int ret;
 
 	/* Set all GPIOs to local input mode */
-	ub953_write(priv, UB953_REG_LOCAL_GPIO_DATA, 0);
-	ub953_write(priv, UB953_REG_GPIO_INPUT_CTRL, 0xf);
+	ret = ub953_write(priv, UB953_REG_LOCAL_GPIO_DATA, 0);
+	if (ret)
+		return ret;
+
+	ret = ub953_write(priv, UB953_REG_GPIO_INPUT_CTRL, 0xf);
+	if (ret)
+		return ret;
 
 	gc->label = dev_name(dev);
 	gc->parent = dev;
@@ -970,10 +975,11 @@ static void ub953_calc_clkout_params(struct ub953_data *priv,
 	clkout_data->rate = clkout_rate;
 }
 
-static void ub953_write_clkout_regs(struct ub953_data *priv,
-				    const struct ub953_clkout_data *clkout_data)
+static int ub953_write_clkout_regs(struct ub953_data *priv,
+				   const struct ub953_clkout_data *clkout_data)
 {
 	u8 clkout_ctrl0, clkout_ctrl1;
+	int ret;
 
 	if (priv->hw_data->is_ub971)
 		clkout_ctrl0 = clkout_data->m;
@@ -983,8 +989,15 @@ static void ub953_write_clkout_regs(struct ub953_data *priv,
 
 	clkout_ctrl1 = clkout_data->n;
 
-	ub953_write(priv, UB953_REG_CLKOUT_CTRL0, clkout_ctrl0);
-	ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);
+	ret = ub953_write(priv, UB953_REG_CLKOUT_CTRL0, clkout_ctrl0);
+	if (ret)
+		return ret;
+
+	ret = ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 static unsigned long ub953_clkout_recalc_rate(struct clk_hw *hw,
@@ -1064,9 +1077,7 @@ static int ub953_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
 	dev_dbg(&priv->client->dev, "%s %lu (requested %lu)\n", __func__,
 		clkout_data.rate, rate);
 
-	ub953_write_clkout_regs(priv, &clkout_data);
-
-	return 0;
+	return ub953_write_clkout_regs(priv, &clkout_data);
 }
 
 static const struct clk_ops ub953_clkout_ops = {
@@ -1091,7 +1102,9 @@ static int ub953_register_clkout(struct ub953_data *priv)
 
 	/* Initialize clkout to 25MHz by default */
 	ub953_calc_clkout_params(priv, UB953_DEFAULT_CLKOUT_RATE, &clkout_data);
-	ub953_write_clkout_regs(priv, &clkout_data);
+	ret = ub953_write_clkout_regs(priv, &clkout_data);
+	if (ret)
+		return ret;
 
 	priv->clkout_clk_hw.init = &init;
 
@@ -1238,10 +1251,15 @@ static int ub953_hw_init(struct ub953_data *priv)
 	if (ret)
 		return dev_err_probe(dev, ret, "i2c init failed\n");
 
-	ub953_write(priv, UB953_REG_GENERAL_CFG,
-		    (priv->non_continous_clk ? 0 : UB953_REG_GENERAL_CFG_CONT_CLK) |
-		    ((priv->num_data_lanes - 1) << UB953_REG_GENERAL_CFG_CSI_LANE_SEL_SHIFT) |
-		    UB953_REG_GENERAL_CFG_CRC_TX_GEN_ENABLE);
+	v = 0;
+	v |= priv->non_continous_clk ? 0 : UB953_REG_GENERAL_CFG_CONT_CLK;
+	v |= (priv->num_data_lanes - 1) <<
+		UB953_REG_GENERAL_CFG_CSI_LANE_SEL_SHIFT;
+	v |= UB953_REG_GENERAL_CFG_CRC_TX_GEN_ENABLE;
+
+	ret = ub953_write(priv, UB953_REG_GENERAL_CFG, v);
+	if (ret)
+		return ret;
 
 	return 0;
 }

-- 
2.43.0

[PATCH v4 15/15] media: i2c: ds90ub960: Fix shadowing of local variables

[Tomi Valkeinen]

Fix a few cases where a local shadows a previously declared local of the
same name.

Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 0db4fc980912..4cd31b1acf14 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -2449,7 +2449,6 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv,
 	} rx_data[UB960_MAX_RX_NPORTS] = {};
 	u8 vc_map[UB960_MAX_RX_NPORTS] = {};
 	struct v4l2_subdev_route *route;
-	unsigned int nport;
 	int ret;
 
 	ret = ub960_validate_stream_vcs(priv);
@@ -2519,7 +2518,8 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv,
 	 */
 	fwd_ctl = GENMASK(7, 4);
 
-	for (nport = 0; nport < priv->hw_data->num_rxports; nport++) {
+	for (unsigned int nport = 0; nport < priv->hw_data->num_rxports;
+	     nport++) {
 		struct ub960_rxport *rxport = priv->rxports[nport];
 		u8 vc = vc_map[nport];
 
@@ -3041,14 +3041,13 @@ static int ub960_log_status(struct v4l2_subdev *sd)
 	struct device *dev = &priv->client->dev;
 	struct v4l2_subdev_state *state;
 	unsigned int nport;
-	unsigned int i;
 	u16 v16 = 0;
 	u8 v = 0;
 	u8 id[UB960_SR_FPD3_RX_ID_LEN];
 
 	state = v4l2_subdev_lock_and_get_active_state(sd);
 
-	for (i = 0; i < sizeof(id); i++)
+	for (unsigned int i = 0; i < sizeof(id); i++)
 		ub960_read(priv, UB960_SR_FPD3_RX_ID(i), &id[i]);
 
 	dev_info(dev, "ID '%.*s'\n", (int)sizeof(id), id);
@@ -3082,7 +3081,6 @@ static int ub960_log_status(struct v4l2_subdev *sd)
 
 	for (nport = 0; nport < priv->hw_data->num_rxports; nport++) {
 		struct ub960_rxport *rxport = priv->rxports[nport];
-		unsigned int i;
 
 		dev_info(dev, "RX %u\n", nport);
 
@@ -3121,7 +3119,7 @@ static int ub960_log_status(struct v4l2_subdev *sd)
 			ub960_log_status_ub960_sp_eq(priv, nport);
 
 		/* GPIOs */
-		for (i = 0; i < UB960_NUM_BC_GPIOS; i++) {
+		for (unsigned int i = 0; i < UB960_NUM_BC_GPIOS; i++) {
 			u8 ctl_reg;
 			u8 ctl_shift;
 

-- 
2.43.0

Re: [PATCH v4 00/15] media: i2c: ds90ub9xx: Misc fixes and improvements

[Andy Shevchenko]

On Fri, Dec 06, 2024 at 10:26:36AM +0200, Tomi Valkeinen wrote:
> This series fixes various small issues in the drivers, and adds a few
> things (a couple of pixel formats and a debugging feature).
> 
> It also takes a few steps in adding more i2c read/write error handlings
> to the drivers, but covers only the easy places.
> 
> Adding error handling to all reads/writes needs more thinking, perhaps
> adding a "ret" parameter to the calls, similar to the cci_* functions,
> or perhaps adding helpers for writing multiple registers from a given
> table. Also, in some places rolling back from an error will require
> work.

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 04/15] media: i2c: ds90ub960: Fix logging SP & EQ status only for UB9702

[Sakari Ailus]

Huomenta,

On Fri, Dec 06, 2024 at 10:26:40AM +0200, Tomi Valkeinen wrote:
> UB9702 does not have SP and EQ registers, but the driver uses them in
> log_status(). Fix this by separating the SP and EQ related log_status()
> work into a separate function (for clarity) and calling that function
> only for UB960.
> 
> Cc: stable@vger.kernel.org
> Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
> Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/i2c/ds90ub960.c | 90 ++++++++++++++++++++++++-------------------
>  1 file changed, 50 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index 24198b803eff..94c8acf171b4 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -2950,6 +2950,54 @@ static const struct v4l2_subdev_pad_ops ub960_pad_ops = {
>  	.set_fmt = ub960_set_fmt,
>  };
>  
> +static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
> +					 unsigned int nport)
> +{
> +	struct device *dev = &priv->client->dev;
> +	u8 eq_level;
> +	s8 strobe_pos;
> +	u8 v = 0;
> +
> +	/* Strobe */
> +
> +	ub960_read(priv, UB960_XR_AEQ_CTL1, &v);

How about adding __must_check to the ub960_read()?

> +
> +	dev_info(dev, "\t%s strobe\n",
> +		 (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" :
> +							  "Manual");
> +
> +	if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) {
> +		ub960_read(priv, UB960_XR_SFILTER_CFG, &v);
> +
> +		dev_info(dev, "\tStrobe range [%d, %d]\n",
> +			 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7,
> +			 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7);
> +	}
> +
> +	ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);
> +
> +	dev_info(dev, "\tStrobe pos %d\n", strobe_pos);
> +
> +	/* EQ */
> +
> +	ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);
> +
> +	dev_info(dev, "\t%s EQ\n",
> +		 (v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" :
> +						    "Adaptive");
> +
> +	if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) {
> +		ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);
> +
> +		dev_info(dev, "\tEQ range [%u, %u]\n",
> +			 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf,
> +			 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf);
> +	}
> +
> +	if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0)
> +		dev_info(dev, "\tEQ level %u\n", eq_level);
> +}
> +
>  static int ub960_log_status(struct v4l2_subdev *sd)
>  {
>  	struct ub960_data *priv = sd_to_ub960(sd);
> @@ -2997,8 +3045,6 @@ static int ub960_log_status(struct v4l2_subdev *sd)
>  
>  	for (nport = 0; nport < priv->hw_data->num_rxports; nport++) {
>  		struct ub960_rxport *rxport = priv->rxports[nport];
> -		u8 eq_level;
> -		s8 strobe_pos;
>  		unsigned int i;
>  
>  		dev_info(dev, "RX %u\n", nport);
> @@ -3034,44 +3080,8 @@ static int ub960_log_status(struct v4l2_subdev *sd)
>  		ub960_rxport_read(priv, nport, UB960_RR_CSI_ERR_COUNTER, &v);
>  		dev_info(dev, "\tcsi_err_counter %u\n", v);
>  
> -		/* Strobe */
> -
> -		ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
> -
> -		dev_info(dev, "\t%s strobe\n",
> -			 (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" :
> -								  "Manual");
> -
> -		if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) {
> -			ub960_read(priv, UB960_XR_SFILTER_CFG, &v);
> -
> -			dev_info(dev, "\tStrobe range [%d, %d]\n",
> -				 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7,
> -				 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7);
> -		}
> -
> -		ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);
> -
> -		dev_info(dev, "\tStrobe pos %d\n", strobe_pos);
> -
> -		/* EQ */
> -
> -		ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);
> -
> -		dev_info(dev, "\t%s EQ\n",
> -			 (v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" :
> -							    "Adaptive");
> -
> -		if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) {
> -			ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);
> -
> -			dev_info(dev, "\tEQ range [%u, %u]\n",
> -				 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf,
> -				 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf);
> -		}
> -
> -		if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0)
> -			dev_info(dev, "\tEQ level %u\n", eq_level);
> +		if (!priv->hw_data->is_ub9702)
> +			ub960_log_status_ub960_sp_eq(priv, nport);
>  
>  		/* GPIOs */
>  		for (i = 0; i < UB960_NUM_BC_GPIOS; i++) {
> 

-- 
Terveisin,

Sakari Ailus

Re: [PATCH v4 04/15] media: i2c: ds90ub960: Fix logging SP & EQ status only for UB9702

[Tomi Valkeinen]

Hi,

On 09/12/2024 11:11, Sakari Ailus wrote:
> Huomenta,
> 
> On Fri, Dec 06, 2024 at 10:26:40AM +0200, Tomi Valkeinen wrote:
>> UB9702 does not have SP and EQ registers, but the driver uses them in
>> log_status(). Fix this by separating the SP and EQ related log_status()
>> work into a separate function (for clarity) and calling that function
>> only for UB960.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
>> Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/i2c/ds90ub960.c | 90 ++++++++++++++++++++++++-------------------
>>   1 file changed, 50 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
>> index 24198b803eff..94c8acf171b4 100644
>> --- a/drivers/media/i2c/ds90ub960.c
>> +++ b/drivers/media/i2c/ds90ub960.c
>> @@ -2950,6 +2950,54 @@ static const struct v4l2_subdev_pad_ops ub960_pad_ops = {
>>   	.set_fmt = ub960_set_fmt,
>>   };
>>   
>> +static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
>> +					 unsigned int nport)
>> +{
>> +	struct device *dev = &priv->client->dev;
>> +	u8 eq_level;
>> +	s8 strobe_pos;
>> +	u8 v = 0;
>> +
>> +	/* Strobe */
>> +
>> +	ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
> 
> How about adding __must_check to the ub960_read()?

Yes, that's a good idea.

We also have a patch in works that'll add error handling to all the i2c 
reads and writes (and some other ub960 improvements), on top of this series.

  Tomi

Re: [PATCH v4 04/15] media: i2c: ds90ub960: Fix logging SP & EQ status only for UB9702

[Tomi Valkeinen]

Hi,

On 09/12/2024 11:11, Sakari Ailus wrote:
> Huomenta,
> 
> On Fri, Dec 06, 2024 at 10:26:40AM +0200, Tomi Valkeinen wrote:
>> UB9702 does not have SP and EQ registers, but the driver uses them in
>> log_status(). Fix this by separating the SP and EQ related log_status()
>> work into a separate function (for clarity) and calling that function
>> only for UB960.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
>> Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/i2c/ds90ub960.c | 90 ++++++++++++++++++++++++-------------------
>>   1 file changed, 50 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
>> index 24198b803eff..94c8acf171b4 100644
>> --- a/drivers/media/i2c/ds90ub960.c
>> +++ b/drivers/media/i2c/ds90ub960.c
>> @@ -2950,6 +2950,54 @@ static const struct v4l2_subdev_pad_ops ub960_pad_ops = {
>>   	.set_fmt = ub960_set_fmt,
>>   };
>>   
>> +static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
>> +					 unsigned int nport)
>> +{
>> +	struct device *dev = &priv->client->dev;
>> +	u8 eq_level;
>> +	s8 strobe_pos;
>> +	u8 v = 0;
>> +
>> +	/* Strobe */
>> +
>> +	ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
> 
> How about adding __must_check to the ub960_read()?

Actually, this is just moving code around (behind an if), so I'd rather 
not add more to this patch, especially as this is a fix.

We'll add the error handling separately on top.

  Tomi

> 
>> +
>> +	dev_info(dev, "\t%s strobe\n",
>> +		 (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" :
>> +							  "Manual");
>> +
>> +	if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) {
>> +		ub960_read(priv, UB960_XR_SFILTER_CFG, &v);
>> +
>> +		dev_info(dev, "\tStrobe range [%d, %d]\n",
>> +			 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7,
>> +			 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7);
>> +	}
>> +
>> +	ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);
>> +
>> +	dev_info(dev, "\tStrobe pos %d\n", strobe_pos);
>> +
>> +	/* EQ */
>> +
>> +	ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);
>> +
>> +	dev_info(dev, "\t%s EQ\n",
>> +		 (v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" :
>> +						    "Adaptive");
>> +
>> +	if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) {
>> +		ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);
>> +
>> +		dev_info(dev, "\tEQ range [%u, %u]\n",
>> +			 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf,
>> +			 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf);
>> +	}
>> +
>> +	if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0)
>> +		dev_info(dev, "\tEQ level %u\n", eq_level);
>> +}
>> +
>>   static int ub960_log_status(struct v4l2_subdev *sd)
>>   {
>>   	struct ub960_data *priv = sd_to_ub960(sd);
>> @@ -2997,8 +3045,6 @@ static int ub960_log_status(struct v4l2_subdev *sd)
>>   
>>   	for (nport = 0; nport < priv->hw_data->num_rxports; nport++) {
>>   		struct ub960_rxport *rxport = priv->rxports[nport];
>> -		u8 eq_level;
>> -		s8 strobe_pos;
>>   		unsigned int i;
>>   
>>   		dev_info(dev, "RX %u\n", nport);
>> @@ -3034,44 +3080,8 @@ static int ub960_log_status(struct v4l2_subdev *sd)
>>   		ub960_rxport_read(priv, nport, UB960_RR_CSI_ERR_COUNTER, &v);
>>   		dev_info(dev, "\tcsi_err_counter %u\n", v);
>>   
>> -		/* Strobe */
>> -
>> -		ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
>> -
>> -		dev_info(dev, "\t%s strobe\n",
>> -			 (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" :
>> -								  "Manual");
>> -
>> -		if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) {
>> -			ub960_read(priv, UB960_XR_SFILTER_CFG, &v);
>> -
>> -			dev_info(dev, "\tStrobe range [%d, %d]\n",
>> -				 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7,
>> -				 ((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7);
>> -		}
>> -
>> -		ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);
>> -
>> -		dev_info(dev, "\tStrobe pos %d\n", strobe_pos);
>> -
>> -		/* EQ */
>> -
>> -		ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);
>> -
>> -		dev_info(dev, "\t%s EQ\n",
>> -			 (v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" :
>> -							    "Adaptive");
>> -
>> -		if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) {
>> -			ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);
>> -
>> -			dev_info(dev, "\tEQ range [%u, %u]\n",
>> -				 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf,
>> -				 (v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf);
>> -		}
>> -
>> -		if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0)
>> -			dev_info(dev, "\tEQ level %u\n", eq_level);
>> +		if (!priv->hw_data->is_ub9702)
>> +			ub960_log_status_ub960_sp_eq(priv, nport);
>>   
>>   		/* GPIOs */
>>   		for (i = 0; i < UB960_NUM_BC_GPIOS; i++) {
>>
> 

Re: [PATCH v4 04/15] media: i2c: ds90ub960: Fix logging SP & EQ status only for UB9702

[Sakari Ailus]

Huomenta,

On Tue, Dec 10, 2024 at 09:38:30AM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 09/12/2024 11:11, Sakari Ailus wrote:
> > Huomenta,
> > 
> > On Fri, Dec 06, 2024 at 10:26:40AM +0200, Tomi Valkeinen wrote:
> > > UB9702 does not have SP and EQ registers, but the driver uses them in
> > > log_status(). Fix this by separating the SP and EQ related log_status()
> > > work into a separate function (for clarity) and calling that function
> > > only for UB960.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
> > > Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > ---
> > >   drivers/media/i2c/ds90ub960.c | 90 ++++++++++++++++++++++++-------------------
> > >   1 file changed, 50 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> > > index 24198b803eff..94c8acf171b4 100644
> > > --- a/drivers/media/i2c/ds90ub960.c
> > > +++ b/drivers/media/i2c/ds90ub960.c
> > > @@ -2950,6 +2950,54 @@ static const struct v4l2_subdev_pad_ops ub960_pad_ops = {
> > >   	.set_fmt = ub960_set_fmt,
> > >   };
> > > +static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
> > > +					 unsigned int nport)
> > > +{
> > > +	struct device *dev = &priv->client->dev;
> > > +	u8 eq_level;
> > > +	s8 strobe_pos;
> > > +	u8 v = 0;
> > > +
> > > +	/* Strobe */
> > > +
> > > +	ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
> > 
> > How about adding __must_check to the ub960_read()?
> 
> Actually, this is just moving code around (behind an if), so I'd rather not
> add more to this patch, especially as this is a fix.
> 
> We'll add the error handling separately on top.

Works for me.

-- 
Sakari Ailus

Re: [PATCH v4 12/15] media: i2c: ds90ub960: Handle errors in ub960_log_status_ub960_sp_eq()

[Sakari Ailus]

On Fri, Dec 06, 2024 at 10:26:48AM +0200, Tomi Valkeinen wrote:
> Add error handling for i2c read/write calls to
> ub960_log_status_ub960_sp_eq().
> 
> Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Link: https://lore.kernel.org/all/Zv40EQSR__JDN_0M@kekkonen.localdomain/

I've replaced these with Closes: to make checkpatch.pl happy. The CI should
be able to tell this nowadays. Same for a few others in the set.

-- 
Sakari Ailus