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

[PATCH v3 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 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 | 186 ++++++++++++++++++++++++++++--------------
 3 files changed, 187 insertions(+), 81 deletions(-)
---
base-commit: adc218676eef25575469234709c2d87185ca223a
change-id: 20241004-ub9xx-fixes-bba80dc48627

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

[PATCH v3 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.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: stable@vger.kernel.org
Fixes: 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching")
---
 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 v3 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.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: stable@vger.kernel.org
Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
---
 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 v3 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.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: stable@vger.kernel.org
Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
---
 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 v3 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.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: stable@vger.kernel.org
Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
---
 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 v3 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.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: stable@vger.kernel.org
Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
---
 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 v3 06/15] media: i2c: ds90ub960: Use HZ_PER_MHZ

[Tomi Valkeinen]

Use HZ_PER_MHZ instead of 1000000U.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index bfffa14e2049..98d815526341 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>
@@ -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 v3 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.

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 98d815526341..03938def6ae9 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -55,6 +55,15 @@
 
 #define MHZ(v) ((u32)((v) * 1000000U))
 
+/*
+ * 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 < 4; 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 v3 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.

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 03938def6ae9..c2035cabf579 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 v3 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.

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 v3 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.

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 c2035cabf579..a4a624816d8b 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 v3 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.

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 a4a624816d8b..64b5297c5f22 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 v3 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().

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 64b5297c5f22..1dc10b2ba1f1 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 v3 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().

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 v3 14/15] media: i2c: ds90ub953: Add error handling for i2c reads/writes

[Tomi Valkeinen]

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

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 v3 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.

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 1dc10b2ba1f1..a9afada7bdc1 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 v3 07/15] media: i2c: ds90ub960: Add support for I2C_RX_ID

[Andy Shevchenko]

On Wed, Dec 04, 2024 at 01:05:21PM +0200, Tomi Valkeinen wrote:
> 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.

...

>  #define MHZ(v) ((u32)((v) * 1000000U))

Missed HZ_PER_MHZ from previous patch?

...

> +#ifdef UB960_DEBUG_I2C_RX_ID
> +	for (unsigned int i = 0; i < 4; i++)

Should it use _MAX_RX_NPORTS instead of 4?

> +		ub960_write(priv, UB960_SR_I2C_RX_ID(i),
> +			    (UB960_DEBUG_I2C_RX_ID + i) << 1);
> +#endif

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Dec 04, 2024 at 01:05:26PM +0200, Tomi Valkeinen wrote:
> Add error handling for i2c read/write calls to
> ub960_log_status_ub960_sp_eq().

Wasn't this being reported, I mean add Reported-by / Closes?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Dec 04, 2024 at 01:05:28PM +0200, Tomi Valkeinen wrote:
> Add error handling for i2c reads/writes in various places.

...and this one?

-- 
With Best Regards,
Andy Shevchenko

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

[Jai Luthra]

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

Hi Tomi,

On Dec 05, 2024 at 10:31:42 +0200, Andy Shevchenko wrote:
> On Wed, Dec 04, 2024 at 01:05:21PM +0200, Tomi Valkeinen wrote:
> > 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.
> 
> ...
> 
> >  #define MHZ(v) ((u32)((v) * 1000000U))
> 
> Missed HZ_PER_MHZ from previous patch?
> 
> ...
> 
> > +#ifdef UB960_DEBUG_I2C_RX_ID
> > +	for (unsigned int i = 0; i < 4; i++)
> 
> Should it use _MAX_RX_NPORTS instead of 4?
> 

Instead of hardcoded value or the macro, it is better to use 
priv->hw_data->num_rxports.

The cut-down variant of this deser only has 2 ports for example.
https://www.ti.com/lit/gpn/ds90ub954-q1

> > +		ub960_write(priv, UB960_SR_I2C_RX_ID(i),
> > +			    (UB960_DEBUG_I2C_RX_ID + i) << 1);
> > +#endif
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
Thanks,
Jai

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

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

[Jai Luthra]

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

Hi Tomi,

On Dec 04, 2024 at 13:05:14 +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.
> 

With the minor comment addressed, for the series:

Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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 | 186 ++++++++++++++++++++++++++++--------------
>  3 files changed, 187 insertions(+), 81 deletions(-)
> ---
> base-commit: adc218676eef25575469234709c2d87185ca223a
> change-id: 20241004-ub9xx-fixes-bba80dc48627
> 
> Best regards,
> -- 
> Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 

-- 
Thanks,
Jai

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

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

[Tomi Valkeinen]

Hi,

On 05/12/2024 10:31, Andy Shevchenko wrote:
> On Wed, Dec 04, 2024 at 01:05:21PM +0200, Tomi Valkeinen wrote:
>> 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.
> 
> ...
> 
>>   #define MHZ(v) ((u32)((v) * 1000000U))
> 
> Missed HZ_PER_MHZ from previous patch?

Yes, and no. I did leave the MHZ uses on purpose. I think the use of 
HZ_PER_MHZ was fine in the calculations, but when having table-ish use 
of MHZ, with hardcoded numbers, I found the MHZ() macro much nicer to read:

	case MHZ(1200):

vs.
	case 1200 * HZ_PER_MHZ:

> 
> ...
> 
>> +#ifdef UB960_DEBUG_I2C_RX_ID
>> +	for (unsigned int i = 0; i < 4; i++)
> 
> Should it use _MAX_RX_NPORTS instead of 4?

Indeed, thanks!

> 
>> +		ub960_write(priv, UB960_SR_I2C_RX_ID(i),
>> +			    (UB960_DEBUG_I2C_RX_ID + i) << 1);
>> +#endif
> 

  Tomi

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

[Tomi Valkeinen]

Hi,

On 05/12/2024 14:00, Jai Luthra wrote:
> Hi Tomi,
> 
> On Dec 05, 2024 at 10:31:42 +0200, Andy Shevchenko wrote:
>> On Wed, Dec 04, 2024 at 01:05:21PM +0200, Tomi Valkeinen wrote:
>>> 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.
>>
>> ...
>>
>>>   #define MHZ(v) ((u32)((v) * 1000000U))
>>
>> Missed HZ_PER_MHZ from previous patch?
>>
>> ...
>>
>>> +#ifdef UB960_DEBUG_I2C_RX_ID
>>> +	for (unsigned int i = 0; i < 4; i++)
>>
>> Should it use _MAX_RX_NPORTS instead of 4?
>>
> 
> Instead of hardcoded value or the macro, it is better to use
> priv->hw_data->num_rxports.
> 
> The cut-down variant of this deser only has 2 ports for example.
> https://www.ti.com/lit/gpn/ds90ub954-q1

Yes, that's true. I'll use the hw_data.

  Tomi

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

[Andy Shevchenko]

On Thu, Dec 05, 2024 at 03:59:58PM +0200, Tomi Valkeinen wrote:
> On 05/12/2024 10:31, Andy Shevchenko wrote:
> > On Wed, Dec 04, 2024 at 01:05:21PM +0200, Tomi Valkeinen wrote:

...

> > >   #define MHZ(v) ((u32)((v) * 1000000U))
> > 
> > Missed HZ_PER_MHZ from previous patch?
> 
> Yes, and no. I did leave the MHZ uses on purpose. I think the use of
> HZ_PER_MHZ was fine in the calculations, but when having table-ish use of
> MHZ, with hardcoded numbers, I found the MHZ() macro much nicer to read:
> 
> 	case MHZ(1200):
> 
> vs.
> 	case 1200 * HZ_PER_MHZ:

Had I talked about tables? :-)
I was only commented the calculations.

-- 
With Best Regards,
Andy Shevchenko

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

[Tomi Valkeinen]

On 05/12/2024 21:36, Andy Shevchenko wrote:
> On Thu, Dec 05, 2024 at 03:59:58PM +0200, Tomi Valkeinen wrote:
>> On 05/12/2024 10:31, Andy Shevchenko wrote:
>>> On Wed, Dec 04, 2024 at 01:05:21PM +0200, Tomi Valkeinen wrote:
> 
> ...
> 
>>>>    #define MHZ(v) ((u32)((v) * 1000000U))
>>>
>>> Missed HZ_PER_MHZ from previous patch?
>>
>> Yes, and no. I did leave the MHZ uses on purpose. I think the use of
>> HZ_PER_MHZ was fine in the calculations, but when having table-ish use of
>> MHZ, with hardcoded numbers, I found the MHZ() macro much nicer to read:
>>
>> 	case MHZ(1200):
>>
>> vs.
>> 	case 1200 * HZ_PER_MHZ:
> 
> Had I talked about tables? :-)
> I was only commented the calculations.

I see your point now =)

  Tomi

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

[Sakari Ailus]

Huomenta,

On Wed, Dec 04, 2024 at 01:05:15PM +0200, Tomi Valkeinen wrote:
> 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

This is, in fact, an extra put. It'll lead to underflow, not overflow.
I'd expect removing it once would be already too much.

> 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.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Cc: stable@vger.kernel.org
> Fixes: 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching")
> ---
>  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);
>  }
>  
> 

-- 
Terveisin,

Sakari Ailus

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

[Tomi Valkeinen]

Hi,

On 09/12/2024 11:09, Sakari Ailus wrote:
> Huomenta,
> 
> On Wed, Dec 04, 2024 at 01:05:15PM +0200, Tomi Valkeinen wrote:
>> 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
> 
> This is, in fact, an extra put. It'll lead to underflow, not overflow.

Well, there are too many puts, so "put overflow" =). I don't think 
underflow is the right word here either, but it's probably better than 
overflow. Maybe I'll reword it so that it doesn't have a flow at all.

> I'd expect removing it once would be already too much.

True, but there's something keeping some refs to the fwnode even without 
these drivers (otherwise they'd be freed when these drivers are not 
around), so the issue shows only when those refs are taken out by the 
puts in these drivers.

  Tomi

> 
>> 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.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Cc: stable@vger.kernel.org
>> Fixes: 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching")
>> ---
>>   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);
>>   }
>>   
>>
> 

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

[Sakari Ailus]

Moi,

On Mon, Dec 09, 2024 at 11:46:45AM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 09/12/2024 11:09, Sakari Ailus wrote:
> > Huomenta,
> > 
> > On Wed, Dec 04, 2024 at 01:05:15PM +0200, Tomi Valkeinen wrote:
> > > 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
> > 
> > This is, in fact, an extra put. It'll lead to underflow, not overflow.
> 
> Well, there are too many puts, so "put overflow" =). I don't think underflow
> is the right word here either, but it's probably better than overflow. Maybe
> I'll reword it so that it doesn't have a flow at all.

Sound good.

> 
> > I'd expect removing it once would be already too much.
> 
> True, but there's something keeping some refs to the fwnode even without
> these drivers (otherwise they'd be freed when these drivers are not around),
> so the issue shows only when those refs are taken out by the puts in these
> drivers.

Port nodes perhaps?

-- 
Terveisin,

Sakari Ailus