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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 20 Jan 2023 11:23:18 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Wolfram Sang <wsa@...nel.org>,
        Luca Ceresoli <luca.ceresoli@...tlin.com>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Matti Vaittinen <Matti.Vaittinen@...rohmeurope.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Peter Rosin <peda@...ntia.se>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Michael Tretter <m.tretter@...gutronix.de>,
        Shawn Tu <shawnx.tu@...el.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mike Pagano <mpagano@...too.org>,
        Krzysztof Hałasa <khalasa@...p.pl>,
        Marek Vasut <marex@...x.de>
Subject: 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:
> > 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@...asonboard.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]

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