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

[Laurent Pinchart]

Hi Tomi,

On Fri, Jan 20, 2023 at 10:13:25AM +0200, Tomi Valkeinen wrote:
> On 20/01/2023 02:34, Laurent Pinchart wrote:
> > On Wed, Jan 18, 2023 at 02:40:31PM +0200, Tomi Valkeinen wrote:
> >> Add driver for TI DS90UB953 FPD-Link III Serializer.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> >> ---
> >>   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]

> >> 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]

> >> +__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.
> > 
> >> +{
> >> +	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.

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