On Thu, Jun 29, 2017 at 11:02:03AM +0200, Niklas Söderlund wrote:
> Hi Sakari,
>
> Thanks for your patch.
>
> On 2017-06-29 10:30:10 +0300, Sakari Ailus wrote:
> > The current practice is that drivers iterate over their endpoints and
> > parse each endpoint separately. This is very similar in a number of
> > drivers, implement a generic function for the job. Driver specific matters
> > can be taken into account in the driver specific callback.
> >
> > Convert the omap3isp as an example.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/platform/omap3isp/isp.c | 91 ++++++++++-----------------------
> > drivers/media/platform/omap3isp/isp.h | 3 --
> > drivers/media/v4l2-core/v4l2-fwnode.c | 94 +++++++++++++++++++++++++++++++++++
> > include/media/v4l2-async.h | 4 +-
> > include/media/v4l2-fwnode.h | 9 ++++
> > 5 files changed, 132 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> > index 9df64c1..9ccf883 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2008,43 +2008,42 @@ enum isp_of_phy {
> > ISP_OF_PHY_CSIPHY2,
> > };
> >
> > -static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
> > - struct isp_async_subdev *isd)
> > +static int isp_fwnode_parse(struct device *dev,
> > + struct v4l2_fwnode_endpoint *vep,
> > + struct v4l2_async_subdev *asd)
> > {
> > + struct isp_async_subdev *isd =
> > + container_of(asd, struct isp_async_subdev, asd);
> > struct isp_bus_cfg *buscfg = &isd->bus;
> > - struct v4l2_fwnode_endpoint vep;
> > unsigned int i;
> > - int ret;
> > -
> > - ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
> > - if (ret)
> > - return ret;
> >
> > dev_dbg(dev, "parsing endpoint %s, interface %u\n",
> > - to_of_node(fwnode)->full_name, vep.base.port);
> > + to_of_node(vep->base.local_fwnode)->full_name, vep->base.port);
> >
> > - switch (vep.base.port) {
> > + switch (vep->base.port) {
> > case ISP_OF_PHY_PARALLEL:
> > buscfg->interface = ISP_INTERFACE_PARALLEL;
> > buscfg->bus.parallel.data_lane_shift =
> > - vep.bus.parallel.data_shift;
> > + vep->bus.parallel.data_shift;
> > buscfg->bus.parallel.clk_pol =
> > - !!(vep.bus.parallel.flags
> > + !!(vep->bus.parallel.flags
> > & V4L2_MBUS_PCLK_SAMPLE_FALLING);
> > buscfg->bus.parallel.hs_pol =
> > - !!(vep.bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
> > + !!(vep->bus.parallel.flags &
> > + V4L2_MBUS_VSYNC_ACTIVE_LOW);
> > buscfg->bus.parallel.vs_pol =
> > - !!(vep.bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
> > + !!(vep->bus.parallel.flags &
> > + V4L2_MBUS_HSYNC_ACTIVE_LOW);
> > buscfg->bus.parallel.fld_pol =
> > - !!(vep.bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
> > + !!(vep->bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
> > buscfg->bus.parallel.data_pol =
> > - !!(vep.bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
> > + !!(vep->bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
> > break;
> >
> > case ISP_OF_PHY_CSIPHY1:
> > case ISP_OF_PHY_CSIPHY2:
> > /* FIXME: always assume CSI-2 for now. */
> > - switch (vep.base.port) {
> > + switch (vep->base.port) {
> > case ISP_OF_PHY_CSIPHY1:
> > buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> > break;
> > @@ -2052,18 +2051,19 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
> > buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> > break;
> > }
> > - buscfg->bus.csi2.lanecfg.clk.pos = vep.bus.mipi_csi2.clock_lane;
> > + buscfg->bus.csi2.lanecfg.clk.pos =
> > + vep->bus.mipi_csi2.clock_lane;
> > buscfg->bus.csi2.lanecfg.clk.pol =
> > - vep.bus.mipi_csi2.lane_polarities[0];
> > + vep->bus.mipi_csi2.lane_polarities[0];
> > dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> > buscfg->bus.csi2.lanecfg.clk.pol,
> > buscfg->bus.csi2.lanecfg.clk.pos);
> >
> > for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
> > buscfg->bus.csi2.lanecfg.data[i].pos =
> > - vep.bus.mipi_csi2.data_lanes[i];
> > + vep->bus.mipi_csi2.data_lanes[i];
> > buscfg->bus.csi2.lanecfg.data[i].pol =
> > - vep.bus.mipi_csi2.lane_polarities[i + 1];
> > + vep->bus.mipi_csi2.lane_polarities[i + 1];
> > dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> > buscfg->bus.csi2.lanecfg.data[i].pol,
> > buscfg->bus.csi2.lanecfg.data[i].pos);
> > @@ -2079,55 +2079,14 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
> >
> > default:
> > dev_warn(dev, "%s: invalid interface %u\n",
> > - to_of_node(fwnode)->full_name, vep.base.port);
> > + to_of_node(vep->base.local_fwnode)->full_name,
> > + vep->base.port);
> > break;
> > }
> >
> > return 0;
> > }
> >
> > -static int isp_fwnodes_parse(struct device *dev,
> > - struct v4l2_async_notifier *notifier)
> > -{
> > - struct fwnode_handle *fwnode = NULL;
> > -
> > - notifier->subdevs = devm_kcalloc(
> > - dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
> > - if (!notifier->subdevs)
> > - return -ENOMEM;
> > -
> > - while (notifier->num_subdevs < ISP_MAX_SUBDEVS &&
> > - (fwnode = fwnode_graph_get_next_endpoint(
> > - of_fwnode_handle(dev->of_node), fwnode))) {
> > - struct isp_async_subdev *isd;
> > -
> > - isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> > - if (!isd)
> > - goto error;
> > -
> > - notifier->subdevs[notifier->num_subdevs] = &isd->asd;
> > -
> > - if (isp_fwnode_parse(dev, fwnode, isd))
> > - goto error;
> > -
> > - isd->asd.match.fwnode.fwnode =
> > - fwnode_graph_get_remote_port_parent(fwnode);
> > - if (!isd->asd.match.fwnode.fwnode) {
> > - dev_warn(dev, "bad remote port parent\n");
> > - goto error;
> > - }
> > -
> > - isd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > - notifier->num_subdevs++;
> > - }
> > -
> > - return notifier->num_subdevs;
> > -
> > -error:
> > - fwnode_handle_put(fwnode);
> > - return -EINVAL;
> > -}
> > -
> > static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> > struct v4l2_subdev *subdev,
> > struct v4l2_async_subdev *asd)
> > @@ -2210,7 +2169,9 @@ static int isp_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> >
> > - ret = isp_fwnodes_parse(&pdev->dev, &isp->notifier);
> > + ret = v4l2_fwnode_endpoints_parse(
> > + &pdev->dev, &isp->notifier, sizeof(struct isp_async_subdev),
> > + isp_fwnode_parse);
> > if (ret < 0)
> > return ret;
> >
> > diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
> > index 2f2ae60..a852c11 100644
> > --- a/drivers/media/platform/omap3isp/isp.h
> > +++ b/drivers/media/platform/omap3isp/isp.h
> > @@ -220,9 +220,6 @@ struct isp_device {
> >
> > unsigned int sbl_resources;
> > unsigned int subclk_resources;
> > -
> > -#define ISP_MAX_SUBDEVS 8
> > - struct v4l2_subdev *subdevs[ISP_MAX_SUBDEVS];
> > };
> >
> > struct isp_async_subdev {
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 0ec6c14..b35d525 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -26,6 +26,7 @@
> > #include <linux/string.h>
> > #include <linux/types.h>
> >
> > +#include <media/v4l2-async.h>
> > #include <media/v4l2-fwnode.h>
> >
> > static int v4l2_fwnode_endpoint_parse_csi_bus(struct fwnode_handle *fwnode,
> > @@ -339,6 +340,99 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> > }
> > EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >
> > +static int notifier_realloc(struct device *dev,
> > + struct v4l2_async_notifier *notifier,
> > + unsigned int max_subdevs)
> > +{
> > + struct v4l2_async_subdev **subdevs;
> > + unsigned int i;
> > +
> > + if (max_subdevs <= notifier->max_subdevs)
> > + return 0;
> > +
> > + subdevs = devm_kcalloc(
> > + dev, max_subdevs, sizeof(*notifier->subdevs), GFP_KERNEL);
> > + if (!subdevs)
> > + return -ENOMEM;
> > +
> > + if (notifier->subdevs) {
> > + for (i = 0; i < notifier->num_subdevs; i++)
> > + subdevs[i] = notifier->subdevs[i];
> > +
> > + devm_kfree(dev, notifier->subdevs);
> > + }
> > +
> > + notifier->subdevs = subdevs;
> > + notifier->max_subdevs = max_subdevs;
> > +
> > + return 0;
> > +}
> > +
> > +int v4l2_fwnode_endpoints_parse(
> > + struct device *dev, struct v4l2_async_notifier *notifier,
> > + size_t asd_struct_size,
> > + int (*parse_single)(struct device *dev,
> > + struct v4l2_fwnode_endpoint *vep,
> > + struct v4l2_async_subdev *asd))
> > +{
> > + struct fwnode_handle *fwnode = NULL;
> > + unsigned int max_subdevs = notifier->max_subdevs;
> > + int ret;
> > +
> > + if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> > + return -EINVAL;
> > +
> > + while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev),
> > + fwnode)))
> > + max_subdevs++;
> > +
> > + ret = notifier_realloc(dev, notifier, max_subdevs);
> > + if (ret)
> > + return ret;
> > +
> > + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> > + dev_fwnode(dev), fwnode)) &&
> > + !WARN_ON(notifier->num_subdevs >= notifier->max_subdevs);
> > + ) {
> > + struct v4l2_fwnode_endpoint *vep;
> > + struct v4l2_async_subdev *asd;
> > +
> > + asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
> > + if (!asd) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > +
> > + notifier->subdevs[notifier->num_subdevs] = asd;
> > +
> > + /* Ignore endpoints the parsing of which failed. */
> > + vep = v4l2_fwnode_endpoint_alloc_parse(fwnode);
> > + if (IS_ERR(vep))
> > + continue;
> > +
> > + ret = parse_single(dev, vep, asd);
> > + v4l2_fwnode_endpoint_free(vep);
> > + if (ret)
> > + goto error;
>
> First off I think this is a good step in the right direction to create
> core functions for this task.
>
> However while reading this I got to think about a use-case I have with
> the Renesas R-Car VIN and CSI-2 drivers where I would not be able to use
> this helper. I have previously posted a patch-set to introduce
> incremental async, see [1]. If that gets picked-up not only the video
> device driver can have use of this helper but subdevices drivers. As
> the subdevice driver would also need to pars its DT node to search for
> subdevices to add to it's own subnotifier. In this case this helper wont
> suffice, I think.
>
> Since the subdevice DT node will contain endpoints pointing to both the
> subdevice which the incremental async notifier would like to find and
> endpoints pointing back to the root video device node it self.
>
> In my use-case for Renesas R-Car Gen3 VIN and CSI-2 DT (not yet accepted
> upstream) I have solved this by having the CSI-2 DT node having two port
> nodes, port0 and port1. In port0 endpoints describing the 'upstream'
> video source from the CSI-2 point of view are defined (these should be
> added to the subnotifier). In port1 endpoints describing connections
> back to the VIN video devices are described. Currently the CSI-2 driver
> simply only looks for endpoints in the port0 node to add to its
> subnotifier, and I don't think this would be possible with this helper?
What we could also do is to add a variant of this that parses the endpoints
by the port / endpoint IDs. Then you don't need to have a cumbersome way to
skip parsing of some ports --- you would have allocated the memory and
parsed the port for nothing by the time you call the callback anyway.
That would seem to be a better approach in general. I remember Rob was
pushing this for DRM as it greatly simplified driver code there. But then
the DT bindings have to be written accordingly: in current bindings the
endpoint IDs are mostly undefined.
We may still need to take extra measures to support cases where there are
multiple links between two sub-devices.
What do you think?
Cc Rob + devicetree list as well.
>
> Perhaps this patch can be modified so that en error code from the
> callback parse_single() could be taken in to account when making the
> decision if the parsed endpoint should be added to the notifiers list of
> subdevices?
>
> > +
> > + asd->match.fwnode.fwnode =
> > + fwnode_graph_get_remote_port_parent(fwnode);
> > + if (!asd->match.fwnode.fwnode) {
> > + dev_warn(dev, "bad remote port parent\n");
> > + ret = -EINVAL;
> > + goto error;
> > + }
> > +
> > + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > + notifier->num_subdevs++;
> > + }
> > +
> > +error:
> > + fwnode_handle_put(fwnode);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_parse);
> > +
> > MODULE_LICENSE("GPL");
> > MODULE_AUTHOR("Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>");
> > MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>");
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index c69d8c8..067f368 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -78,7 +78,8 @@ struct v4l2_async_subdev {
> > /**
> > * struct v4l2_async_notifier - v4l2_device notifier data
> > *
> > - * @num_subdevs: number of subdevices
> > + * @num_subdevs: number of subdevices used in subdevs array
> > + * @max_subdevs: number of subdevices allocated in subdevs array
> > * @subdevs: array of pointers to subdevice descriptors
> > * @v4l2_dev: pointer to struct v4l2_device
> > * @waiting: list of struct v4l2_async_subdev, waiting for their drivers
> > @@ -90,6 +91,7 @@ struct v4l2_async_subdev {
> > */
> > struct v4l2_async_notifier {
> > unsigned int num_subdevs;
> > + unsigned int max_subdevs;
> > struct v4l2_async_subdev **subdevs;
> > struct v4l2_device *v4l2_dev;
> > struct list_head waiting;
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index ecc1233..ff489f7 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -25,6 +25,8 @@
> > #include <media/v4l2-mediabus.h>
> >
> > struct fwnode_handle;
> > +struct v4l2_async_notifier;
> > +struct v4l2_async_subdev;
> >
> > /**
> > * struct v4l2_fwnode_bus_mipi_csi2 - MIPI CSI-2 bus data structure
> > @@ -101,4 +103,11 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> > struct v4l2_fwnode_link *link);
> > void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >
> > +int v4l2_fwnode_endpoints_parse(
> > + struct device *dev, struct v4l2_async_notifier *notifier,
> > + size_t asd_struct_size,
> > + int (*parse_single)(struct device *dev,
> > + struct v4l2_fwnode_endpoint *vep,
> > + struct v4l2_async_subdev *asd));
> > +
> > #endif /* _V4L2_FWNODE_H */
> > --
> > 2.1.4
> >
> >
>
> --
> Regards,
> Niklas Söderlund
--
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
