Re: [PATCH v7 7/7] media: i2c: add DS90UB953 driver

Hi Tomi,

On Fri, Jan 20, 2023 at 10:13:25AM +0200, Tomi Valkeinen wrote:

On 20/01/2023 02:34, Laurent Pinchart wrote:

quoted

On Wed, Jan 18, 2023 at 02:40:31PM +0200, Tomi Valkeinen wrote:

quoted

Add driver for TI DS90UB953 FPD-Link III Serializer.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
  drivers/media/i2c/Kconfig     |   13 +
  drivers/media/i2c/Makefile    |    1 +
  drivers/media/i2c/ds90ub953.c | 1576 +++++++++++++++++++++++++++++++++
  3 files changed, 1590 insertions(+)
  create mode 100644 drivers/media/i2c/ds90ub953.c

[snip]

quoted

quoted

diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
new file mode 100644
index 000000000000..ec33e16da3d1
--- /dev/null
+++ b/drivers/media/i2c/ds90ub953.c

@@ -0,0 +1,1576 @@

[snip]

quoted

quoted

+__maybe_unused static int ub953_read_ind(struct ub953_data *priv, u8 block,
+					 u8 reg, u8 *val)

I'd still prefer dropping this function, but I won't insist.

quoted

+{
+	unsigned int v;
+	int ret;
+
+	mutex_lock(&priv->reg_lock);
+
+	ret = _ub953_select_ind_reg_block(priv, block);
+	if (ret)
+		goto out;
+
+	ret = regmap_write(priv->regmap, UB953_REG_IND_ACC_ADDR, reg);
+	if (ret) {
+		dev_err(&priv->client->dev,
+			"Write to IND_ACC_ADDR failed when reading %u:%x02x: %d\n",
+			block, reg, ret);
+		goto out;
+	}

Would it make sense to cache the address as you do with
current_indirect_block, and skip this write if the address is correct
already ? If the device implements auto-increment of the address (I
haven't checked), this could save quite a few I2C writes.

Yes, there's auto increment for these indirect register accesses 
(IA_AUTO_INC). There's also IA_READ bit, but I don't understand what it 
does:

Indirect Access Read:
Setting this allows generation of a read strobe to the selected register 
block upon setting of the IND_ACC_ADDR register. In auto-increment mode, 
read strobes are also asserted following a read of the IND_ACC_DATA 
register. This function is only required for blocks that need to 
pre-fetch register data.

In any case, the indirect accesses are only used when configuring the 
TPG. I'm sure this can be optimized, but I question the need to do this 
optimization.

The UB960 driver uses indirect accesses more often. There it might make 
a bit more sense, although, again, I don't really see why it matters in 
practice.

It doesn't sound like a complex optimization, so maybe it wouldn't 
increase the chances of bugs much, but... still. Maybe something for later?

I'm fine doing this on top. As you said, it shouldn't be difficult, but
it's also not a must-have right away.

quoted

quoted

+
+	ret = regmap_read(priv->regmap, UB953_REG_IND_ACC_DATA, &v);
+	if (ret) {
+		dev_err(&priv->client->dev,
+			"Write to IND_ACC_DATA failed when reading %u:%x02x: %d\n",
+			block, reg, ret);
+		goto out;
+	}
+
+	*val = v;
+
+out:
+	mutex_unlock(&priv->reg_lock);
+
+	return ret;
+}

[snip]

-- 
Regards,

Laurent Pinchart