On Tue, May 27, 2025 at 07:01:47AM +0000, Sakari Ailus wrote:
> 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));
I just noticed that this error message isn't correct. The endpoint
before parsed is ep, not fwnode, so you should write
vin_err(vin, "Failed to parse %pOF\n", to_of_node(ep));
> > + 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.
That would contradict the commit message that indicates the function is
moved without being modified. I'd rather keep the patch as-is, and then
improve the function in a separate patch.
Regarding improvements, declaring ep and fwnode as
struct fwnode_handle __free(fwnode_handle) *ep = NULL;
struct fwnode_handle __free(fwnode_handle) *fwnode = NULL;
would also simplify error handling.
> > + 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 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);
> > --
> > 2.49.0
> >
>
> --
> Sakari Ailus
--
Regards,
Laurent Pinchart
