Re: [PATCH v5 7/8] media: i2c: add DS90UB913 driver

[Laurent Pinchart]

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