Re: [PATCH v8 08/21] rcar-vin: Use generic parser for parsing fwnode endpoints

[Sakari Ailus]

Hi Hans,

On Wed, Sep 06, 2017 at 09:44:32AM +0200, Hans Verkuil wrote:
> On 09/05/2017 03:05 PM, Sakari Ailus wrote:
> >  static int rvin_digital_graph_init(struct rvin_dev *vin)
> >  {
> > -   struct v4l2_async_subdev **subdevs = NULL;
> >     int ret;
> >  
> > -   ret = rvin_digital_graph_parse(vin);
> > +   ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > +           vin->dev, &vin->notifier,
> > +           sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
> >     if (ret)
> >             return ret;
> >  
> > -   if (!vin->digital.asd.match.fwnode.fwnode) {
> > -           vin_dbg(vin, "No digital subdevice found\n");
> > -           return -ENODEV;
> > -   }
> > -
> > -   /* Register the subdevices notifier. */
> > -   subdevs = devm_kzalloc(vin->dev, sizeof(*subdevs), GFP_KERNEL);
> > -   if (subdevs == NULL)
> > -           return -ENOMEM;
> > -
> > -   subdevs[0] = &vin->digital.asd;
> > -
> > -   vin_dbg(vin, "Found digital subdevice %pOF\n",
> > -           to_of_node(subdevs[0]->match.fwnode.fwnode));
> > +   if (vin->notifier.num_subdevs > 0)
> > +           vin_dbg(vin, "Found digital subdevice %pOF\n",
> > +                   to_of_node(
> > +                           vin->notifier.subdevs[0]->match.fwnode.fwnode));
> 
> As mentioned in my review of patch 6/21, this violates the documentation of 
> the
> v4l2_async_notifier_parse_fwnode_endpoints function.
> 
> However, I think the problem is with the documentation and not with this code,
> so:
> 
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>

Thanks. I still don't like to encourage accessing the notifier fields
directly from drivers, and in this case there isn't a need to either. The
following changes work, too, and I'll use them in the next version:

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
b/drivers/media/platform/rcar-vin/rcar-core.c
index bd551f0be213..d1e5909da087 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -177,10 +177,10 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
        if (ret)
                return ret;
 
-       if (vin->notifier.num_subdevs > 0)
+       if (vin->digital)
                vin_dbg(vin, "Found digital subdevice %pOF\n",
                        to_of_node(
-                               vin->notifier.subdevs[0]->match.fwnode.fwnode));
+                               vin->digital->asd.match.fwnode.fwnode));
 
        vin->notifier.bound = rvin_digital_notify_bound;
        vin->notifier.unbind = rvin_digital_notify_unbind;

I also changed EPERM still used in this version to ENOTCONN.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi