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

[PATCH 00/13] 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>
---
Tomi Valkeinen (13):
      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: 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

 drivers/media/i2c/ds90ub913.c |  25 +++++--
 drivers/media/i2c/ds90ub953.c |  56 ++++++++++----
 drivers/media/i2c/ds90ub960.c | 168 +++++++++++++++++++++++++++++-------------
 3 files changed, 176 insertions(+), 73 deletions(-)
---
base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
change-id: 20241004-ub9xx-fixes-bba80dc48627

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

[PATCH 01/13] 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_puts.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
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 ca9bb29dab89..150d6641516f 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 02/13] 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>
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 03/13] 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>
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 04/13] 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>
Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
---
 drivers/media/i2c/ds90ub960.c | 92 ++++++++++++++++++++++++-------------------
 1 file changed, 52 insertions(+), 40 deletions(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 24198b803eff..5c393ec6c682 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -2950,6 +2950,56 @@ 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 +3047,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 +3082,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 05/13] 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>
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 5c393ec6c682..9dc36bba0a87 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 06/13] 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 9dc36bba0a87..5238088e23e2 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -54,6 +54,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
@@ -350,7 +359,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
 
@@ -4002,6 +4011,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 07/13] 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 5238088e23e2..09a19553ec53 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -580,11 +580,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 08/13] 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 09/13] 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 09a19553ec53..d4a3759bc568 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -366,7 +366,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 10/13] 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index d4a3759bc568..ab5330db4162 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -1575,7 +1575,7 @@ static int ub960_rxport_wait_locks(struct ub960_data *priv,
 		if (missing == 0)
 			break;
 
-		msleep(50);
+		fsleep(10 * 1000);
 	}
 
 	if (lock_mask)

-- 
2.43.0

[PATCH 11/13] 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 | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index ab5330db4162..47990aa1f007 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -2977,17 +2977,22 @@ static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
 	u8 eq_level;
 	s8 strobe_pos;
 	u8 v = 0;
+	int ret;
 
 	/* 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) -
@@ -2996,20 +3001,26 @@ static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
 				 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 12/13] 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 | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index 150d6641516f..8b540b360e79 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -146,6 +146,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 +746,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,
+				priv->pclk_polarity_rising ?
+					UB913_REG_GENERAL_CFG_PCLK_RISING :
+					0);
+	if (ret)
+		return ret;
 
 	return 0;
 }

-- 
2.43.0

[PATCH 13/13] 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

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

[Andy Shevchenko]

On Fri, Oct 04, 2024 at 05:46:34PM +0300, Tomi Valkeinen wrote:
> 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.

...

> +		if (priv->hw_data->is_ub9702) {
> +			dev_dbg(dev, "\trx%u: locked, freq %llu Hz\n",
> +				nport, (v * 1000000ULL) >> 8);

Perhaps HZ_PER_MHZ?

> +		} else {

...

> +			dev_dbg(dev,
> +				"\trx%u: locked, SP: %d, EQ: %u, freq %llu Hz\n",
> +				nport, strobe_pos, eq_level,
> +				(v * 1000000ULL) >> 8);

Ditto.

> +		}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Oct 04, 2024 at 05:46:35PM +0300, 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.

...

> +		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);

I believe the code is much more readable if those 7:s moved to the previous
lines. Btw, does driver use bitfield.h? And why not?


...

> -			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);

It will be even shorter in the new code!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/13] media: i2c: ds90ub960: Add support for I2C_RX_ID

[Andy Shevchenko]

On Fri, Oct 04, 2024 at 05:46:37PM +0300, 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.

...

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

Why pre-increment?

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 10/13] media: i2c: ds90ub960: Reduce sleep in ub960_rxport_wait_locks()

[Andy Shevchenko]

On Fri, Oct 04, 2024 at 05:46:41PM +0300, Tomi Valkeinen wrote:
> 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.

...

> -		msleep(50);
> +		fsleep(10 * 1000);

USEC_PER_MSEC

Can also a comment be added on top of this call to explain the choice?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 11/13] media: i2c: ds90ub960: Handle errors in ub960_log_status_ub960_sp_eq()

[Andy Shevchenko]

On Fri, Oct 04, 2024 at 05:46:42PM +0300, Tomi Valkeinen wrote:
> Add error handling for i2c read/write calls to
> ub960_log_status_ub960_sp_eq()

Missed period.

...

>  	u8 eq_level;
>  	s8 strobe_pos;
>  	u8 v = 0;

With that change in place you may remove redundant assignments to 0 here
and everywhere else where is not needed anymore.

> +	int ret;
>  
>  	/* Strobe */
>  
> -	ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
> +	ret = ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
> +	if (ret)
> +		return;

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Oct 04, 2024 at 05:46:32PM +0300, 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
> 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_puts.

fwnode_handle_put():s


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Oct 04, 2024 at 05:46:43PM +0300, Tomi Valkeinen wrote:
> Add error handling to ub913_hw_init() using a new helper function,
> ub913_update_bits().

...

> +	ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
> +				UB913_REG_GENERAL_CFG_PCLK_RISING,
> +				priv->pclk_polarity_rising ?
> +					UB913_REG_GENERAL_CFG_PCLK_RISING :
> +					0);

So, you can use regmap_set_bits() / regmap_clear_bits() instead of this
ternary. It also gives one parameter less to the regmap calls.

> +	if (ret)
> +		return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 13/13] media: i2c: ds90ub953: Add error handling for i2c reads/writes

[Andy Shevchenko]

On Fri, Oct 04, 2024 at 05:46:44PM +0300, Tomi Valkeinen wrote:
> Add error handling for i2c reads/writes in various places.

...

> +	ret = ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

This is just a more verbose version of

	return ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);

...

> -	ub953_write_clkout_regs(priv, &clkout_data);
> -
> -	return 0;
> +	return ub953_write_clkout_regs(priv, &clkout_data);

...and seems you use that pattern.

-- 
With Best Regards,
Andy Shevchenko

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

[Tomi Valkeinen]

Hi Andy,

On 10/10/2024 17:04, Andy Shevchenko wrote:
> On Fri, Oct 04, 2024 at 05:46:43PM +0300, Tomi Valkeinen wrote:
>> Add error handling to ub913_hw_init() using a new helper function,
>> ub913_update_bits().
> 
> ...
> 
>> +	ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
>> +				UB913_REG_GENERAL_CFG_PCLK_RISING,
>> +				priv->pclk_polarity_rising ?
>> +					UB913_REG_GENERAL_CFG_PCLK_RISING :
>> +					0);
> 
> So, you can use regmap_set_bits() / regmap_clear_bits() instead of this
> ternary. It also gives one parameter less to the regmap calls.

True... But is it better?

if (priv->pclk_polarity_rising)
	ret = regmap_set_bits(priv->regmap, UB913_REG_GENERAL_CFG,
			      UB913_REG_GENERAL_CFG_PCLK_RISING);
else
	ret = regmap_clear_bits(priv->regmap, UB913_REG_GENERAL_CFG,
				UB913_REG_GENERAL_CFG_PCLK_RISING);

The call itself is more readable there, but then again, as we're setting 
the value of a bit, I dislike having if/else with two calls for a single 
assignment.

Using FIELD_PREP is perhaps a bit better than the ternary:

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));

I think I'd like best a function to set/clear a bitmask with a boolean:

ret = regmap_toggle_bits(priv->regmap, UB913_REG_GENERAL_CFG,
			 UB913_REG_GENERAL_CFG_PCLK_RISING,
			 priv->pclk_polarity_rising);

For now, I think I'll go with the FIELD_PREP() version. It's perhaps a 
bit better than the ternary.

  Tomi

Re: [PATCH 13/13] media: i2c: ds90ub953: Add error handling for i2c reads/writes

[Tomi Valkeinen]

Hi,

On 10/10/2024 17:06, Andy Shevchenko wrote:
> On Fri, Oct 04, 2024 at 05:46:44PM +0300, Tomi Valkeinen wrote:
>> Add error handling for i2c reads/writes in various places.
> 
> ...
> 
>> +	ret = ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> This is just a more verbose version of
> 
> 	return ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);
> 
> ...
> 
>> -	ub953_write_clkout_regs(priv, &clkout_data);
>> -
>> -	return 0;
>> +	return ub953_write_clkout_regs(priv, &clkout_data);
> 
> ...and seems you use that pattern.

I use the pattern selectively =).

If the function has a bunch of

ret = foo()
if (ret)
	return ret;

blocks, I want to keep the pattern and thus I don't use "return foo();" 
as the last line.

Also, I think, I usually like to use "return foo();" only with small 
functions, as the function call becomes less visible with that format.

  Tomi

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

[Andy Shevchenko]

On Fri, Nov 08, 2024 at 11:34:09AM +0200, Tomi Valkeinen wrote:
> On 10/10/2024 17:04, Andy Shevchenko wrote:
> > On Fri, Oct 04, 2024 at 05:46:43PM +0300, Tomi Valkeinen wrote:
> > > Add error handling to ub913_hw_init() using a new helper function,
> > > ub913_update_bits().

...

> > > +	ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
> > > +				UB913_REG_GENERAL_CFG_PCLK_RISING,
> > > +				priv->pclk_polarity_rising ?
> > > +					UB913_REG_GENERAL_CFG_PCLK_RISING :
> > > +					0);
> > 
> > So, you can use regmap_set_bits() / regmap_clear_bits() instead of this
> > ternary. It also gives one parameter less to the regmap calls.
> 
> True... But is it better?

In my opinion yes, because it's clearer on what's going on.
It has no (semi-)hidden choice, so code wise it most likely
will be the same at the end. So we are speaking only about
C-level of readability.

> if (priv->pclk_polarity_rising)
> 	ret = regmap_set_bits(priv->regmap, UB913_REG_GENERAL_CFG,
> 			      UB913_REG_GENERAL_CFG_PCLK_RISING);
> else
> 	ret = regmap_clear_bits(priv->regmap, UB913_REG_GENERAL_CFG,
> 				UB913_REG_GENERAL_CFG_PCLK_RISING);
> 
> The call itself is more readable there, but then again, as we're setting the
> value of a bit, I dislike having if/else with two calls for a single
> assignment.

FTR, there was an attempt to add _assign() in similar way how it's done with
bitops (set/clear/assign) to regmap, but had been rejected by Mark. I don't
remember detail why, though.

> Using FIELD_PREP is perhaps a bit better than the ternary:
> 
> 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));
> 
> I think I'd like best a function to set/clear a bitmask with a boolean:
> 
> ret = regmap_toggle_bits(priv->regmap, UB913_REG_GENERAL_CFG,
> 			 UB913_REG_GENERAL_CFG_PCLK_RISING,
> 			 priv->pclk_polarity_rising);
> 
> For now, I think I'll go with the FIELD_PREP() version. It's perhaps a bit
> better than the ternary.

Okay.

-- 
With Best Regards,
Andy Shevchenko