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
