From: Hans Verkuil <hverkuil@xs4all.nl>
To: Sakari Ailus <sakari.ailus@linux.intel.com>, linux-media@vger.kernel.org
Cc: niklas.soderlund@ragnatech.se, laurent.pinchart@ideasonboard.com
Subject: Re: [RFC 4/8] omap3isp: Rework OF endpoint parsing
Date: Thu, 4 Apr 2019 15:43:51 +0200 [thread overview]
Message-ID: <47cf12e5-25a1-b315-d834-e940d953aadf@xs4all.nl> (raw)
In-Reply-To: <20190318191653.7197-5-sakari.ailus@linux.intel.com>
On 3/18/19 8:16 PM, Sakari Ailus wrote:
> Rework OF endpoint parsing for the omap3isp driver. This does add some
> lines of code. The benefits are still clear:
>
> - the great complication related to callbacks in endpoint parsing is gone;
> instead endpoints are obtained port by port and
>
> - endpoints may now have a default bus configuration which was not
> possible while using callbacks. This driver does not benefit from that
> feature, but as the omap3isp is one of the exemplary drivers, this works
> as an example for driver developers.
>
> Depends-on: ("device property: Add fwnode_graph_get_endpoint_by_id")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/platform/omap3isp/isp.c | 334 ++++++++++++++++++++--------------
> 1 file changed, 200 insertions(+), 134 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index bd57174d81a7..ac23948f6ade 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2015,136 +2015,6 @@ enum isp_of_phy {
> ISP_OF_PHY_CSIPHY2,
> };
>
> -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;
> - bool csi1 = false;
> - unsigned int i;
> -
> - dev_dbg(dev, "parsing endpoint %pOF, interface %u\n",
> - to_of_node(vep->base.local_fwnode), 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;
> - buscfg->bus.parallel.clk_pol =
> - !!(vep->bus.parallel.flags
> - & V4L2_MBUS_PCLK_SAMPLE_FALLING);
> - buscfg->bus.parallel.hs_pol =
> - !!(vep->bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
> - buscfg->bus.parallel.vs_pol =
> - !!(vep->bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
> - buscfg->bus.parallel.fld_pol =
> - !!(vep->bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
> - buscfg->bus.parallel.data_pol =
> - !!(vep->bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
> - buscfg->bus.parallel.bt656 = vep->bus_type == V4L2_MBUS_BT656;
> - break;
> -
> - case ISP_OF_PHY_CSIPHY1:
> - case ISP_OF_PHY_CSIPHY2:
> - switch (vep->bus_type) {
> - case V4L2_MBUS_CCP2:
> - case V4L2_MBUS_CSI1:
> - dev_dbg(dev, "CSI-1/CCP-2 configuration\n");
> - csi1 = true;
> - break;
> - case V4L2_MBUS_CSI2_DPHY:
> - dev_dbg(dev, "CSI-2 configuration\n");
> - csi1 = false;
> - break;
> - default:
> - dev_err(dev, "unsupported bus type %u\n",
> - vep->bus_type);
> - return -EINVAL;
> - }
> -
> - switch (vep->base.port) {
> - case ISP_OF_PHY_CSIPHY1:
> - if (csi1)
> - buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
> - else
> - buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> - break;
> - case ISP_OF_PHY_CSIPHY2:
> - if (csi1)
> - buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
> - else
> - buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> - break;
> - }
> - if (csi1) {
> - buscfg->bus.ccp2.lanecfg.clk.pos =
> - vep->bus.mipi_csi1.clock_lane;
> - buscfg->bus.ccp2.lanecfg.clk.pol =
> - vep->bus.mipi_csi1.lane_polarity[0];
> - dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> - buscfg->bus.ccp2.lanecfg.clk.pol,
> - buscfg->bus.ccp2.lanecfg.clk.pos);
> -
> - buscfg->bus.ccp2.lanecfg.data[0].pos =
> - vep->bus.mipi_csi1.data_lane;
> - buscfg->bus.ccp2.lanecfg.data[0].pol =
> - vep->bus.mipi_csi1.lane_polarity[1];
> -
> - dev_dbg(dev, "data lane polarity %u, pos %u\n",
> - buscfg->bus.ccp2.lanecfg.data[0].pol,
> - buscfg->bus.ccp2.lanecfg.data[0].pos);
> -
> - buscfg->bus.ccp2.strobe_clk_pol =
> - vep->bus.mipi_csi1.clock_inv;
> - buscfg->bus.ccp2.phy_layer = vep->bus.mipi_csi1.strobe;
> - buscfg->bus.ccp2.ccp2_mode =
> - vep->bus_type == V4L2_MBUS_CCP2;
> - buscfg->bus.ccp2.vp_clk_pol = 1;
> -
> - buscfg->bus.ccp2.crc = 1;
> - } else {
> - 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];
> - dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> - buscfg->bus.csi2.lanecfg.clk.pol,
> - buscfg->bus.csi2.lanecfg.clk.pos);
> -
> - buscfg->bus.csi2.num_data_lanes =
> - vep->bus.mipi_csi2.num_data_lanes;
> -
> - for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
> - buscfg->bus.csi2.lanecfg.data[i].pos =
> - vep->bus.mipi_csi2.data_lanes[i];
> - buscfg->bus.csi2.lanecfg.data[i].pol =
> - 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);
> - }
> - /*
> - * FIXME: now we assume the CRC is always there.
> - * Implement a way to obtain this information from the
> - * sensor. Frame descriptors, perhaps?
> - */
> - buscfg->bus.csi2.crc = 1;
> - }
> - break;
> -
> - default:
> - dev_warn(dev, "%pOF: invalid interface %u\n",
> - to_of_node(vep->base.local_fwnode), vep->base.port);
> - return -EINVAL;
> - }
> -
> - return 0;
> -}
> -
> static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
> {
> struct isp_device *isp = container_of(async, struct isp_device,
> @@ -2174,6 +2044,204 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
> return media_device_register(&isp->media_dev);
> }
>
> +static void isp_parse_of_parallel_endpoint(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct isp_bus_cfg *buscfg)
> +{
> + buscfg->interface = ISP_INTERFACE_PARALLEL;
> + buscfg->bus.parallel.data_lane_shift = vep->bus.parallel.data_shift;
> + buscfg->bus.parallel.clk_pol =
> + !!(vep->bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_FALLING);
> + buscfg->bus.parallel.hs_pol =
> + !!(vep->bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
> + buscfg->bus.parallel.vs_pol =
> + !!(vep->bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
> + buscfg->bus.parallel.fld_pol =
> + !!(vep->bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
> + buscfg->bus.parallel.data_pol =
> + !!(vep->bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
> + buscfg->bus.parallel.bt656 = vep->bus_type == V4L2_MBUS_BT656;
> +}
> +
> +static void isp_parse_of_csi2_endpoint(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct isp_bus_cfg *buscfg)
> +{
> + unsigned int i;
> +
> + 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];
> + dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> + buscfg->bus.csi2.lanecfg.clk.pol,
> + buscfg->bus.csi2.lanecfg.clk.pos);
> +
> + buscfg->bus.csi2.num_data_lanes = vep->bus.mipi_csi2.num_data_lanes;
> +
> + for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
> + buscfg->bus.csi2.lanecfg.data[i].pos =
> + vep->bus.mipi_csi2.data_lanes[i];
> + buscfg->bus.csi2.lanecfg.data[i].pol =
> + 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);
> + }
> + /*
> + * FIXME: now we assume the CRC is always there. Implement a way to
> + * obtain this information from the sensor. Frame descriptors, perhaps?
> + */
> + buscfg->bus.csi2.crc = 1;
> +}
> +
> +static void isp_parse_of_csi1_endpoint(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct isp_bus_cfg *buscfg)
> +{
> + buscfg->bus.ccp2.lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
> + buscfg->bus.ccp2.lanecfg.clk.pol = vep->bus.mipi_csi1.lane_polarity[0];
> + dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> + buscfg->bus.ccp2.lanecfg.clk.pol,
> + buscfg->bus.ccp2.lanecfg.clk.pos);
> +
> + buscfg->bus.ccp2.lanecfg.data[0].pos = vep->bus.mipi_csi1.data_lane;
> + buscfg->bus.ccp2.lanecfg.data[0].pol =
> + vep->bus.mipi_csi1.lane_polarity[1];
> +
> + dev_dbg(dev, "data lane polarity %u, pos %u\n",
> + buscfg->bus.ccp2.lanecfg.data[0].pol,
> + buscfg->bus.ccp2.lanecfg.data[0].pos);
> +
> + buscfg->bus.ccp2.strobe_clk_pol = vep->bus.mipi_csi1.clock_inv;
> + buscfg->bus.ccp2.phy_layer = vep->bus.mipi_csi1.strobe;
> + buscfg->bus.ccp2.ccp2_mode = vep->bus_type == V4L2_MBUS_CCP2;
> + buscfg->bus.ccp2.vp_clk_pol = 1;
> +
> + buscfg->bus.ccp2.crc = 1;
> +}
> +
> +static int isp_alloc_isd(struct isp_async_subdev **isd,
> + struct isp_bus_cfg **buscfg)
> +{
> + struct isp_async_subdev *__isd;
> +
> + __isd = kzalloc(sizeof(*isd), GFP_KERNEL);
> + if (!__isd)
> + return -ENOMEM;
> +
> + *isd = __isd;
> + *buscfg = &__isd->bus;
> +
> + return 0;
> +}
> +
> +static struct {
> + u32 phy;
> + u32 csi2_if;
> + u32 csi1_if;
> +} isp_bus_interfaces[2] = {
> + { ISP_OF_PHY_CSIPHY1,
> + ISP_INTERFACE_CSI2C_PHY1, ISP_INTERFACE_CCP2B_PHY1 },
> + { ISP_OF_PHY_CSIPHY2,
> + ISP_INTERFACE_CSI2A_PHY2, ISP_INTERFACE_CCP2B_PHY2 },
> +};
> +
> +static int isp_parse_of_endpoints(struct isp_device *isp)
> +{
> + struct fwnode_handle *ep;
> + struct isp_async_subdev *isd;
> + struct isp_bus_cfg *buscfg;
> + unsigned int i;
> +
> + ep = fwnode_graph_get_endpoint_by_id(
> + dev_fwnode(isp->dev), ISP_OF_PHY_PARALLEL, 0,
> + FWNODE_GRAPH_ENDPOINT_NEXT);
> +
> + if (ep) {
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_PARALLEL
> + };
> + int ret;
> +
> + dev_dbg(isp->dev, "parsing parallel interface\n");
> +
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> + if (!ret)
> + ret = isp_alloc_isd(&isd, &buscfg);
> +
> + if (!ret) {
> + isp_parse_of_parallel_endpoint(isp->dev, &vep, buscfg);
> + ret = v4l2_async_notifier_add_fwnode_remote_subdev(
> + &isp->notifier, ep, &isd->asd);
> + }
> +
> + if (ret) {
> + kfree(isd);
> + fwnode_handle_put(ep);
> + }
If ret == 0, then who calls 'fwnode_handle_put(ep);'?
Am I missing something?
Regards,
Hans
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(isp_bus_interfaces); i++) {
> + ep = fwnode_graph_get_endpoint_by_id(
> + dev_fwnode(isp->dev), isp_bus_interfaces[i].phy, 0,
> + FWNODE_GRAPH_ENDPOINT_NEXT);
> +
> + if (ep) {
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY
> + };
> + int ret;
> +
> + dev_dbg(isp->dev,
> + "parsing serial interface %u, node %pOF\n", i,
> + to_of_node(ep));
> +
> + ret = isp_alloc_isd(&isd, &buscfg);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> + if (!ret) {
> + buscfg->interface =
> + isp_bus_interfaces[i].csi2_if;
> + isp_parse_of_csi2_endpoint(isp->dev, &vep,
> + buscfg);
> + } else if (ret == -ENXIO) {
> + vep = (struct v4l2_fwnode_endpoint)
> + { .bus_type = V4L2_MBUS_CSI1 };
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> +
> + if (ret == -ENXIO) {
> + vep = (struct v4l2_fwnode_endpoint)
> + { .bus_type = V4L2_MBUS_CCP2 };
> + ret = v4l2_fwnode_endpoint_parse(ep,
> + &vep);
> + }
> + if (!ret) {
> + buscfg->interface =
> + isp_bus_interfaces[i].csi1_if;
> + isp_parse_of_csi1_endpoint(isp->dev,
> + &vep,
> + buscfg);
> + }
> + }
> +
> + if (!ret)
> + ret = v4l2_async_notifier_add_fwnode_remote_subdev(
> + &isp->notifier, ep, &isd->asd);
> +
> + if (ret) {
> + kfree(isd);
> + fwnode_handle_put(ep);
> + return ret;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> static const struct v4l2_async_notifier_operations isp_subdev_notifier_ops = {
> .complete = isp_subdev_notifier_complete,
> };
> @@ -2222,14 +2290,12 @@ static int isp_probe(struct platform_device *pdev)
> mutex_init(&isp->isp_mutex);
> spin_lock_init(&isp->stat_lock);
> v4l2_async_notifier_init(&isp->notifier);
> + isp->dev = &pdev->dev;
>
> - ret = v4l2_async_notifier_parse_fwnode_endpoints(
> - &pdev->dev, &isp->notifier, sizeof(struct isp_async_subdev),
> - isp_fwnode_parse);
> + ret = isp_parse_of_endpoints(isp);
> if (ret < 0)
> goto error;
>
> - isp->dev = &pdev->dev;
> isp->ref_count = 0;
>
> ret = dma_coerce_mask_and_coherent(isp->dev, DMA_BIT_MASK(32));
>
next prev parent reply other threads:[~2019-04-04 13:43 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-18 19:16 [RFC 0/8] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
2019-03-18 19:16 ` [RFC 1/8] v4l2-async: Use endpoint node, not device node, for fwnode match Sakari Ailus
2019-03-19 13:02 ` [RFC v1.1 " Sakari Ailus
2019-03-20 15:23 ` Niklas Söderlund
2019-04-04 13:37 ` [RFC " Hans Verkuil
2019-04-04 21:50 ` Sakari Ailus
2019-03-18 19:16 ` [RFC 2/8] v4l2-async: Add v4l2_async_notifier_add_fwnode_remote_subdev Sakari Ailus
2019-03-19 13:00 ` [RFC v1.1 " Sakari Ailus
2019-04-04 13:39 ` [RFC " Hans Verkuil
2019-04-04 23:04 ` Sakari Ailus
2019-03-18 19:16 ` [RFC 3/8] v4l2-fwnode: Use v4l2_async_notifier_add_fwnode_remote_subdev Sakari Ailus
2019-03-20 16:10 ` Niklas Söderlund
2019-03-18 19:16 ` [RFC 4/8] omap3isp: Rework OF endpoint parsing Sakari Ailus
2019-04-04 13:43 ` Hans Verkuil [this message]
2019-04-05 0:22 ` Sakari Ailus
2019-03-18 19:16 ` [RFC 5/8] v4l2-async: Safely clean up an uninitialised notifier Sakari Ailus
2019-03-18 19:16 ` [RFC 6/8] ipu3-cio2: Clean up notifier's subdev list if parsing endpoints fails Sakari Ailus
2019-03-18 19:16 ` [RFC 7/8] ipu3-cio2: Proceed with notifier init even if there are no subdevs Sakari Ailus
2019-03-18 19:16 ` [RFC 8/8] ipu3-cio2: Parse information from firmware without using callbacks Sakari Ailus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=47cf12e5-25a1-b315-d834-e940d953aadf@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=niklas.soderlund@ragnatech.se \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).