linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Wolfram Sang" <wsa@kernel.org>,
	"Luca Ceresoli" <luca.ceresoli@bootlin.com>,
	"Andy Shevchenko" <andriy.shevchenko@intel.com>,
	"Matti Vaittinen" <Matti.Vaittinen@fi.rohmeurope.com>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Peter Rosin" <peda@axentia.se>,
	"Liam Girdwood" <lgirdwood@gmail.com>,
	"Mark Brown" <broonie@kernel.org>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Michael Tretter" <m.tretter@pengutronix.de>,
	"Shawn Tu" <shawnx.tu@intel.com>,
	"Hans Verkuil" <hverkuil@xs4all.nl>,
	"Mike Pagano" <mpagano@gentoo.org>,
	"Krzysztof Hałasa" <khalasa@piap.pl>,
	"Marek Vasut" <marex@denx.de>
Subject: Re: [PATCH v7 7/7] media: i2c: add DS90UB953 driver
Date: Fri, 20 Jan 2023 11:23:18 +0200	[thread overview]
Message-ID: <Y8pdhtG7/Y1Oetqb@pendragon.ideasonboard.com> (raw)
In-Reply-To: <eab3c178-6a10-e949-ebbc-c45094d4b67a@ideasonboard.com>

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

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

  reply	other threads:[~2023-01-20  9:25 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-18 12:40 [PATCH v7 0/7] i2c-atr and FPDLink Tomi Valkeinen
2023-01-18 12:40 ` [PATCH v7 1/7] i2c: add I2C Address Translator (ATR) support Tomi Valkeinen
2023-01-18 14:23   ` Andy Shevchenko
2023-01-18 17:17     ` Luca Ceresoli
2023-01-18 17:39       ` Andy Shevchenko
2023-01-19  8:21         ` Luca Ceresoli
2023-01-19 10:09           ` Tomi Valkeinen
2023-01-19 11:35             ` Luca Ceresoli
2023-01-19 12:22               ` Tomi Valkeinen
2023-01-19 13:00                 ` Luca Ceresoli
2023-01-20  9:55                   ` Laurent Pinchart
2023-01-20 13:58                     ` Luca Ceresoli
2023-01-19 12:39           ` Tomi Valkeinen
2023-01-19 13:08             ` Luca Ceresoli
2023-01-19 10:01     ` Tomi Valkeinen
2023-01-20 15:58       ` Andy Shevchenko
2023-01-20 16:00         ` Tomi Valkeinen
2023-01-18 12:40 ` [PATCH v7 2/7] dt-bindings: media: add TI DS90UB913 FPD-Link III Serializer Tomi Valkeinen
2023-01-18 12:40 ` [PATCH v7 3/7] dt-bindings: media: add TI DS90UB953 " Tomi Valkeinen
2023-01-18 12:40 ` [PATCH v7 4/7] dt-bindings: media: add TI DS90UB960 FPD-Link III Deserializer Tomi Valkeinen
2023-01-19 23:24   ` Laurent Pinchart
2023-01-18 12:40 ` [PATCH v7 5/7] media: i2c: add DS90UB960 driver Tomi Valkeinen
2023-01-18 15:48   ` Andy Shevchenko
2023-01-19 16:27     ` Tomi Valkeinen
2023-01-19 23:19       ` Laurent Pinchart
2023-01-20 16:47       ` Andy Shevchenko
2023-01-25 11:15         ` Tomi Valkeinen
2023-01-25 12:10           ` Andy Shevchenko
2023-01-25 13:33             ` Tomi Valkeinen
2023-01-25 14:49               ` Andy Shevchenko
2023-01-25 15:14                 ` Tomi Valkeinen
2023-01-25 15:27                   ` Andy Shevchenko
2023-01-26  8:41                     ` Tomi Valkeinen
2023-01-26 10:21                       ` Andy Shevchenko
2023-01-26 10:51                         ` Laurent Pinchart
2023-01-27  8:24                           ` Tomi Valkeinen
2023-01-27  9:15                             ` Andy Shevchenko
2023-02-08 15:10                               ` Tomi Valkeinen
2023-02-09 10:54                                 ` Laurent Pinchart
2023-01-18 12:40 ` [PATCH v7 6/7] media: i2c: add DS90UB913 driver Tomi Valkeinen
2023-01-20  0:03   ` Laurent Pinchart
2023-01-20  7:04     ` Tomi Valkeinen
2023-01-20  9:06       ` Laurent Pinchart
2023-01-18 12:40 ` [PATCH v7 7/7] media: i2c: add DS90UB953 driver Tomi Valkeinen
2023-01-20  0:34   ` Laurent Pinchart
2023-01-20  8:13     ` Tomi Valkeinen
2023-01-20  9:23       ` Laurent Pinchart [this message]
2023-01-18 16:01 ` [PATCH v7 0/7] i2c-atr and FPDLink Andy Shevchenko
2023-01-18 17:28   ` Tomi Valkeinen
2023-01-18 17:43     ` Andy Shevchenko
2023-01-19  8:43       ` Luca Ceresoli
2023-01-19 12:40         ` Tomi Valkeinen
2023-01-19 13:19           ` Luca Ceresoli
2023-01-20 16:00         ` Andy Shevchenko
2023-01-20 16:17           ` Luca Ceresoli
2023-01-20 16:20             ` Tomi Valkeinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y8pdhtG7/Y1Oetqb@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=Matti.Vaittinen@fi.rohmeurope.com \
    --cc=andriy.shevchenko@intel.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=hverkuil@xs4all.nl \
    --cc=khalasa@piap.pl \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=luca.ceresoli@bootlin.com \
    --cc=m.tretter@pengutronix.de \
    --cc=marex@denx.de \
    --cc=mchehab@kernel.org \
    --cc=mpagano@gentoo.org \
    --cc=peda@axentia.se \
    --cc=robh+dt@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    --cc=shawnx.tu@intel.com \
    --cc=tomi.valkeinen@ideasonboard.com \
    --cc=wsa@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).