Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 26 Dec 2022 19:01:11 +0200
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 =?utf-8?Q?Ha=C5=82asa?= <khalasa@piap.pl>,
        Marek Vasut <marex@denx.de>
Subject: Re: [PATCH v5 7/8] media: i2c: add DS90UB913 driver
Message-ID: <Y6nTV16me9aQL3iT@pendragon.ideasonboard.com>
References: <20221208104006.316606-1-tomi.valkeinen@ideasonboard.com>
 <20221208104006.316606-8-tomi.valkeinen@ideasonboard.com>
 <Y5YiazDtaxtLJyL0@pendragon.ideasonboard.com>
 <4d349785-ca37-d930-db3c-2581bba9fde0@ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <4d349785-ca37-d930-db3c-2581bba9fde0@ideasonboard.com>
Precedence: bulk

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@ideasonboard.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