| Message ID | 1497891889-8038-6-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Jacopo, Thanks for your patch. On 2017-06-19 19:04:42 +0200, Jacopo Mondi wrote: > Support parsing digital input on configurable port id and reg > properties. Also make the function return -ENOENT when no subdevice is > found. > This change is needed to support parsing digital input on Gen3. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index 175f138..ef61bcc 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -511,7 +511,9 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier, > return 0; > } > > -static int rvin_digital_graph_parse(struct rvin_dev *vin) > +static int rvin_digital_graph_parse(struct rvin_dev *vin, > + unsigned int port, > + unsigned int reg) > { > struct device_node *ep, *np; > int ret; > @@ -519,13 +521,9 @@ static int rvin_digital_graph_parse(struct rvin_dev *vin) > vin->digital.asd.match.fwnode.fwnode = NULL; > vin->digital.subdev = NULL; > > - /* > - * Port 0 id 0 is local digital input, try to get it. > - * Not all instances can or will have this, that is OK > - */ > - ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0); > + ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, port, reg); I don't think it's necessary to supply the port and reg from the caller. If the DT proposed in rcar_vin.txt is used. > if (!ep) > - return 0; > + return -ENOENT; I'm not saying this is wrong, but why do you change this? I can't see a clear advantage in doing so. And if do it I think it should be done in a separate patch explaining why. > > np = of_graph_get_remote_port_parent(ep); > if (!np) { > @@ -555,11 +553,15 @@ static int rvin_digital_graph_init(struct rvin_dev *vin) > if (ret) > return ret; > > - ret = rvin_digital_graph_parse(vin); > - if (ret) > + /* > + * Port 0 id 0 is local digital input, try to get it. > + * Not all instances can or will have this, that is OK > + */ > + ret = rvin_digital_graph_parse(vin, 0, 0); > + if (ret && ret != -ENOENT) > return ret; > > - if (!vin->digital.asd.match.fwnode.fwnode) { > + if (ret == -ENOENT) { > vin_dbg(vin, "No digital subdevice found\n"); > return -ENODEV; > } > -- > 2.7.4 >
Hi Niklas,
thanks for review
On Mon, Jun 19, 2017 at 09:39:46PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2017-06-19 19:04:42 +0200, Jacopo Mondi wrote:
> > Support parsing digital input on configurable port id and reg
> > properties. Also make the function return -ENOENT when no subdevice is
> > found.
> > This change is needed to support parsing digital input on Gen3.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > drivers/media/platform/rcar-vin/rcar-core.c | 22 ++++++++++++----------
> > 1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 175f138..ef61bcc 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -511,7 +511,9 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> > return 0;
> > }
> >
> > -static int rvin_digital_graph_parse(struct rvin_dev *vin)
> > +static int rvin_digital_graph_parse(struct rvin_dev *vin,
> > + unsigned int port,
> > + unsigned int reg)
> > {
> > struct device_node *ep, *np;
> > int ret;
> > @@ -519,13 +521,9 @@ static int rvin_digital_graph_parse(struct rvin_dev *vin)
> > vin->digital.asd.match.fwnode.fwnode = NULL;
> > vin->digital.subdev = NULL;
> >
> > - /*
> > - * Port 0 id 0 is local digital input, try to get it.
> > - * Not all instances can or will have this, that is OK
> > - */
> > - ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
> > + ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, port, reg);
>
> I don't think it's necessary to supply the port and reg from the caller.
> If the DT proposed in rcar_vin.txt is used.
>
> > if (!ep)
> > - return 0;
> > + return -ENOENT;
>
> I'm not saying this is wrong, but why do you change this? I can't see a
> clear advantage in doing so. And if do it I think it should be done in a
> separate patch explaining why.
For both questions, as I parse entries in port@2 and
rvin_digital_notify_bound was intended to use port@0 by default I made
port and reg parameters. I use -ENOENT to distinguish the case where
no endpoint has been found
>
> >
> > np = of_graph_get_remote_port_parent(ep);
> > if (!np) {
> > @@ -555,11 +553,15 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
> > if (ret)
> > return ret;
> >
> > - ret = rvin_digital_graph_parse(vin);
> > - if (ret)
> > + /*
> > + * Port 0 id 0 is local digital input, try to get it.
> > + * Not all instances can or will have this, that is OK
> > + */
> > + ret = rvin_digital_graph_parse(vin, 0, 0);
> > + if (ret && ret != -ENOENT)
> > return ret;
> >
> > - if (!vin->digital.asd.match.fwnode.fwnode) {
> > + if (ret == -ENOENT) {
> > vin_dbg(vin, "No digital subdevice found\n");
> > return -ENODEV;
> > }
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index 175f138..ef61bcc 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -511,7 +511,9 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier, return 0; } -static int rvin_digital_graph_parse(struct rvin_dev *vin) +static int rvin_digital_graph_parse(struct rvin_dev *vin, + unsigned int port, + unsigned int reg) { struct device_node *ep, *np; int ret; @@ -519,13 +521,9 @@ static int rvin_digital_graph_parse(struct rvin_dev *vin) vin->digital.asd.match.fwnode.fwnode = NULL; vin->digital.subdev = NULL; - /* - * Port 0 id 0 is local digital input, try to get it. - * Not all instances can or will have this, that is OK - */ - ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0); + ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, port, reg); if (!ep) - return 0; + return -ENOENT; np = of_graph_get_remote_port_parent(ep); if (!np) { @@ -555,11 +553,15 @@ static int rvin_digital_graph_init(struct rvin_dev *vin) if (ret) return ret; - ret = rvin_digital_graph_parse(vin); - if (ret) + /* + * Port 0 id 0 is local digital input, try to get it. + * Not all instances can or will have this, that is OK + */ + ret = rvin_digital_graph_parse(vin, 0, 0); + if (ret && ret != -ENOENT) return ret; - if (!vin->digital.asd.match.fwnode.fwnode) { + if (ret == -ENOENT) { vin_dbg(vin, "No digital subdevice found\n"); return -ENODEV; }
Support parsing digital input on configurable port id and reg properties. Also make the function return -ENOENT when no subdevice is found. This change is needed to support parsing digital input on Gen3. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/platform/rcar-vin/rcar-core.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)