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
