linux-kernel - Re: [PATCH v5 7/8] media: i2c: add DS90UB913 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]

Message-ID: <Y6nTV16me9aQL3iT@pendragon.ideasonboard.com>
Date:   Mon, 26 Dec 2022 19:01:11 +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 v5 7/8] media: i2c: add DS90UB913 driver

Hi Tomi,

On Wed, Dec 14, 2022 at 08:29:48AM +0200, Tomi Valkeinen wrote:
> On 11/12/2022 20:33, Laurent Pinchart wrote:
> > On Thu, Dec 08, 2022 at 12:40:05PM +0200, Tomi Valkeinen wrote:
> >> Add driver for TI DS90UB913 FPDLink-3 Serializer.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
> >> ---
> >>   drivers/media/i2c/Kconfig     |  13 +
> >>   drivers/media/i2c/Makefile    |   2 +-
> >>   drivers/media/i2c/ds90ub913.c | 892 ++++++++++++++++++++++++++++++++++
> >>   3 files changed, 906 insertions(+), 1 deletion(-)
> >>   create mode 100644 drivers/media/i2c/ds90ub913.c

[snip]

> >> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
> >> new file mode 100644
> >> index 000000000000..6001a622e622
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/ds90ub913.c
> >> @@ -0,0 +1,892 @@

[snip]

> >> +static int ub913_notify_bound(struct v4l2_async_notifier *notifier,
> >> +			      struct v4l2_subdev *source_subdev,
> >> +			      struct v4l2_async_subdev *asd)
> >> +{
> >> +	struct ub913_data *priv = sd_to_ub913(notifier->sd);
> >> +	struct device *dev = &priv->client->dev;
> >> +	unsigned int src_pad;
> >> +	int ret;
> >> +
> >> +	dev_dbg(dev, "Bind %s\n", source_subdev->name);
> > 
> > I'd drop this message.
> 
> Why is that? Do we get this easily from the v4l2 core? These debug 
> prints in the bind/unbind process have been valuable for me.

Because debug messages are not meant to be a tracing infrastructure, and
because, if we want to keep this message, it would be best handled in
the v4l2-async core instead of being duplicated across drivers. Same for
the messages at the end of the function.

> >> +
> >> +	ret = media_entity_get_fwnode_pad(&source_subdev->entity,
> >> +					  source_subdev->fwnode,
> >> +					  MEDIA_PAD_FL_SOURCE);
> >> +	if (ret < 0) {
> >> +		dev_err(dev, "Failed to find pad for %s\n",
> >> +			source_subdev->name);
> >> +		return ret;
> >> +	}
> >> +
> >> +	priv->source_sd = source_subdev;
> >> +	src_pad = ret;
> >> +
> >> +	ret = media_create_pad_link(&source_subdev->entity, src_pad,
> >> +				    &priv->sd.entity, 0,
> > 
> > 				    &priv->sd.entity, UB913_PAD_SINK,
> 
> Yep.
> 
> >> +				    MEDIA_LNK_FL_ENABLED |
> >> +				    MEDIA_LNK_FL_IMMUTABLE);
> >> +	if (ret) {
> >> +		dev_err(dev, "Unable to link %s:%u -> %s:0\n",
> >> +			source_subdev->name, src_pad, priv->sd.name);
> >> +		return ret;
> >> +	}
> >> +
> >> +	dev_dbg(dev, "Bound %s:%u\n", source_subdev->name, src_pad);
> >> +
> >> +	dev_dbg(dev, "All subdevs bound\n");
> > 
> > I'd drop this message.
> > 
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void ub913_notify_unbind(struct v4l2_async_notifier *notifier,
> >> +				struct v4l2_subdev *source_subdev,
> >> +				struct v4l2_async_subdev *asd)
> >> +{
> >> +	struct ub913_data *priv = sd_to_ub913(notifier->sd);
> >> +	struct device *dev = &priv->client->dev;
> >> +
> >> +	dev_dbg(dev, "Unbind %s\n", source_subdev->name);
> >> +}
> > 
> > This is a no-op so you can drop it.
> 
> This has been useful for development, but, yes, perhaps it's time to 
> drop it.
> 
> >> +
> >> +static const struct v4l2_async_notifier_operations ub913_notify_ops = {
> >> +	.bound = ub913_notify_bound,
> >> +	.unbind = ub913_notify_unbind,
> >> +};

[snip]

-- 
Regards,

Laurent Pinchart