Hejssan!
On Fri, Jun 06, 2025 at 03:50:00PM +0200, Niklas Söderlund wrote:
> Hej Sakari,
>
> Thanks for your feedback.
Var så god!
>
> On 2025-05-27 07:01:47 +0000, Sakari Ailus wrote:
> > Hej Niklas,
> >
> > On Wed, May 21, 2025 at 03:20:35PM +0200, Niklas Söderlund wrote:
> > > The R-Car VIN driver is needless complex and uses more then one
> >
> > s/needless\K/ly/
> >
> > > v4l-async notifier to attach to all its subdevices. Prepare for unifying
> > > them by moving rvin_parallel_parse_of() to where it needs to be when
> > > they are unified.
> > >
> > > The function is moved verbatim and there is no change in behavior.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > > ---
> > > .../platform/renesas/rcar-vin/rcar-core.c | 106 +++++++++---------
> > > 1 file changed, 53 insertions(+), 53 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > index d9ad56fb2aa9..60ec57d73a12 100644
> > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > @@ -337,6 +337,59 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
> > > }
> > > }
> > >
> > > +static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > > +{
> > > + struct fwnode_handle *ep, *fwnode;
> > > + struct v4l2_fwnode_endpoint vep = {
> > > + .bus_type = V4L2_MBUS_UNKNOWN,
> > > + };
> > > + struct v4l2_async_connection *asc;
> > > + int ret;
> > > +
> > > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > > + if (!ep)
> > > + return 0;
> > > +
> > > + fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > + fwnode_handle_put(ep);
> > > + if (ret) {
> > > + vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + switch (vep.bus_type) {
> > > + case V4L2_MBUS_PARALLEL:
> > > + case V4L2_MBUS_BT656:
> > > + vin_dbg(vin, "Found %s media bus\n",
> > > + vep.bus_type == V4L2_MBUS_PARALLEL ?
> > > + "PARALLEL" : "BT656");
> > > + vin->parallel.mbus_type = vep.bus_type;
> > > + vin->parallel.bus = vep.bus.parallel;
> > > + break;
> > > + default:
> > > + vin_err(vin, "Unknown media bus type\n");
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > > + struct v4l2_async_connection);
> >
> > If you use v4l2_async_nf_add_fwnode_remote() here, you can omit
> > fwnode_graph_get_remote_endpoint() call above. Also the error handling
> > becomes more simple.
>
> Indeed it would, but I do use fwnode in the debug print at the end of
> the function. And I do find that print out use-full when debugging, so I
> would like to keep it.
The drivers really shouldn't have a need for this. How about adding that
debug print to the V4L2 async framework instead? I think it might be useful
for other drivers as well even though the information is available via
debugfs (or sysfs?) already.
>
> Laurent's suggestion of using __free(fwnode_handle) can instead be used
> to make error handling easier, and since it would be needed for the ep
> variable anyhow I think I will try that.
Sounds good to me.
>
> >
> > > + if (IS_ERR(asc)) {
> > > + ret = PTR_ERR(asc);
> > > + goto out;
> > > + }
> > > +
> > > + vin->parallel.asc = asc;
> > > +
> > > + vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
Please use %pfw instead for the fwnode (at least for the possible V4L2
async patch).
> > > +out:
> > > + fwnode_handle_put(fwnode);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> > > unsigned int max_id)
> > > {
> > > @@ -635,59 +688,6 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> > > .complete = rvin_parallel_notify_complete,
> > > };
> > >
> > > -static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > > -{
> > > - struct fwnode_handle *ep, *fwnode;
> > > - struct v4l2_fwnode_endpoint vep = {
> > > - .bus_type = V4L2_MBUS_UNKNOWN,
> > > - };
> > > - struct v4l2_async_connection *asc;
> > > - int ret;
> > > -
> > > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > > - if (!ep)
> > > - return 0;
> > > -
> > > - fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > - ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > - fwnode_handle_put(ep);
> > > - if (ret) {
> > > - vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > > - ret = -EINVAL;
> > > - goto out;
> > > - }
> > > -
> > > - switch (vep.bus_type) {
> > > - case V4L2_MBUS_PARALLEL:
> > > - case V4L2_MBUS_BT656:
> > > - vin_dbg(vin, "Found %s media bus\n",
> > > - vep.bus_type == V4L2_MBUS_PARALLEL ?
> > > - "PARALLEL" : "BT656");
> > > - vin->parallel.mbus_type = vep.bus_type;
> > > - vin->parallel.bus = vep.bus.parallel;
> > > - break;
> > > - default:
> > > - vin_err(vin, "Unknown media bus type\n");
> > > - ret = -EINVAL;
> > > - goto out;
> > > - }
> > > -
> > > - asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > > - struct v4l2_async_connection);
> > > - if (IS_ERR(asc)) {
> > > - ret = PTR_ERR(asc);
> > > - goto out;
> > > - }
> > > -
> > > - vin->parallel.asc = asc;
> > > -
> > > - vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> > > -out:
> > > - fwnode_handle_put(fwnode);
> > > -
> > > - return ret;
> > > -}
> > > -
> > > static void rvin_parallel_cleanup(struct rvin_dev *vin)
> > > {
> > > v4l2_async_nf_unregister(&vin->notifier);
--
Med vänliga hälsningar,
Sakari Ailus
