Hi Eugene

On Mon, Apr 12, 2021 at 02:15:32PM +0000, Eugen.Hristev@microchip.com wrote:

On 4/12/21 4:41 PM, Jacopo Mondi wrote:

quoted

Hi Eugene,

On Mon, Apr 12, 2021 at 12:37:41PM +0000, Eugen.Hristev@microchip.com wrote:

quoted

quoted

+static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
+{
+   struct device_node *np = dev->of_node;
+   struct device_node *epn = NULL;
+   struct isc_subdev_entity *subdev_entity;
+   unsigned int flags;
+   int ret;
+   bool mipi_mode;
+
+   INIT_LIST_HEAD(&isc->subdev_entities);
+
+   mipi_mode = of_property_read_bool(np, "microchip,mipi-mode");
+
+   while (1) {
+           struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
+
+           epn = of_graph_get_next_endpoint(np, epn);
+           if (!epn)
+                   return 0;
+
+           ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
+                                            &v4l2_epn);
+           if (ret) {
+                   ret = -EINVAL;
+                   dev_err(dev, "Could not parse the endpoint\n");
+                   break;
+           }
+
+           subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
+                                        GFP_KERNEL);
+           if (!subdev_entity) {
+                   ret = -ENOMEM;
+                   break;
+           }
+           subdev_entity->epn = epn;
+
+           flags = v4l2_epn.bus.parallel.flags;
+
+           if (flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
+                   subdev_entity->pfe_cfg0 = ISC_PFE_CFG0_HPOL_LOW;
+
+           if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+                   subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_VPOL_LOW;
+
+           if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
+                   subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_PPOL_LOW;
+
+           if (v4l2_epn.bus_type == V4L2_MBUS_BT656)
+                   subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_CCIR_CRC |
+                                   ISC_PFE_CFG0_CCIR656;

Hi Jacopo,

If I use the bus-type property for the 'port' , do I actually have to
change something here ?

You can set bus_type to the desired type, if it doesn't match the
'bus-type' property you will have an immediate error and a more strict
check on the properties.

You would likely:

         v4l2_epn.bus_type = V4L2_MBUS_PARALLEL;
         ret = v4l2_fwnode_endpoint_parse()
         if (!ret) {
                 /* It's parallel */
         } else {
                 v4l2_epn.bus_type = V4L2_MBUS_BT656;
                 ret = v4l2_fwnode_endpoint_parse()
                 if (ret) {
                         /* Unsupported bus type: error out. */
                 }

                 /* It's BT656 */
         }

if the v4l2_fwnode_endpoint_parse will already fill in the
v4l2_epn.bus_type based on what is found in the 'bus-type' , why do I
need to do this assumption-fail-assumption-fail scenario ?

Because you would know exactly what bus type you're parsing, as
v4l2_fwnode_endpoint_parse() would make sure the type you intend to
parse matches what the endpoint reports

	if (vep->bus_type != V4L2_MBUS_UNKNOWN) {
		if (mbus_type != V4L2_MBUS_UNKNOWN &&
		    vep->bus_type != mbus_type) {
			pr_debug("expecting bus type %s\n",
				 v4l2_fwnode_mbus_type_to_string(vep->bus_type));
			return -ENXIO;
		}

I guess this is mostly useful if you want to specify default
values the v4l2_fwnode_endpoint. If you go with autoguessing, a
parallel endpoint without polarities flag specified can be identified
as BT656 if I'm not mistaken.

Can't I simply check the value of v4l2_epn.bus_type , as I am doing it
already ?

Only if you make bus-type required in DTS, then you can specify here
bus_type = 0 like you're doing and rely on
v4l2_fwnode_endpoint_parse() to set it to what it has found in DTS.

Otherwise you rely on auto-guessing, if I got this right.

Thanks
   j

quoted

Not the greatest API, but it's more robust.

quoted

the v4l2_epn.bus_type won't be set automatically ? by the endpoint
parser I mean.

Yes, that's what auto-discovery is, the endpoint parser tries to
deduce the bus type from the properties that are there specified. It
works, but leaves quite some ambiguity between ie PARALLEL and BT656
as some polarities might not be necessarily specified even for
PARALLEL bus types.

As I've said, there's still plenty of code that relies on
auto-discovery so I don't think this is blocking, also because making
bus-type mandatory on existing DTS is quite painful. Since this is a
new DTS you can consider this solution if you want to.

Thanks
    j

quoted

Thanks,
Eugen

quoted

+
+           if (mipi_mode)
+                   subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_MIPI;
+
+           list_add_tail(&subdev_entity->list, &isc->subdev_entities);
+   }
+   of_node_put(epn);
+
+   return ret;
+}
+
+static int microchip_xisc_probe(struct platform_device *pdev)
+{
+   struct device *dev = &pdev->dev;
+   struct isc_device *isc;
+   struct resource *res;
+   void __iomem *io_base;
+   struct isc_subdev_entity *subdev_entity;
+   int irq;
+   int ret;
+   u32 ver;
+
+   isc = devm_kzalloc(dev, sizeof(*isc), GFP_KERNEL);
+   if (!isc)
+           return -ENOMEM;
+
+   platform_set_drvdata(pdev, isc);
+   isc->dev = dev;
+
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   io_base = devm_ioremap_resource(dev, res);
+   if (IS_ERR(io_base))
+           return PTR_ERR(io_base);
+
+   isc->regmap = devm_regmap_init_mmio(dev, io_base, &isc_regmap_config);
+   if (IS_ERR(isc->regmap)) {
+           ret = PTR_ERR(isc->regmap);
+           dev_err(dev, "failed to init register map: %d\n", ret);
+           return ret;
+   }
+
+   irq = platform_get_irq(pdev, 0);
+   if (irq < 0)
+           return irq;
+
+   ret = devm_request_irq(dev, irq, isc_interrupt, 0,
+                          "microchip-sama7g5-xisc", isc);
+   if (ret < 0) {
+           dev_err(dev, "can't register ISR for IRQ %u (ret=%i)\n",
+                   irq, ret);
+           return ret;
+   }
+
+   isc->gamma_table = isc_sama7g5_gamma_table;
+   isc->gamma_max = 0;
+
+   isc->max_width = ISC_SAMA7G5_MAX_SUPPORT_WIDTH;
+   isc->max_height = ISC_SAMA7G5_MAX_SUPPORT_HEIGHT;
+
+   isc->config_dpc = isc_sama7g5_config_dpc;
+   isc->config_csc = isc_sama7g5_config_csc;
+   isc->config_cbc = isc_sama7g5_config_cbc;
+   isc->config_cc = isc_sama7g5_config_cc;
+   isc->config_gam = isc_sama7g5_config_gam;
+   isc->config_rlp = isc_sama7g5_config_rlp;
+   isc->config_ctrls = isc_sama7g5_config_ctrls;
+
+   isc->adapt_pipeline = isc_sama7g5_adapt_pipeline;
+
+   isc->offsets.csc = ISC_SAMA7G5_CSC_OFFSET;
+   isc->offsets.cbc = ISC_SAMA7G5_CBC_OFFSET;
+   isc->offsets.sub422 = ISC_SAMA7G5_SUB422_OFFSET;
+   isc->offsets.sub420 = ISC_SAMA7G5_SUB420_OFFSET;
+   isc->offsets.rlp = ISC_SAMA7G5_RLP_OFFSET;
+   isc->offsets.his = ISC_SAMA7G5_HIS_OFFSET;
+   isc->offsets.dma = ISC_SAMA7G5_DMA_OFFSET;
+   isc->offsets.version = ISC_SAMA7G5_VERSION_OFFSET;
+   isc->offsets.his_entry = ISC_SAMA7G5_HIS_ENTRY_OFFSET;
+
+   isc->controller_formats = sama7g5_controller_formats;
+   isc->controller_formats_size = ARRAY_SIZE(sama7g5_controller_formats);
+   isc->formats_list = sama7g5_formats_list;
+   isc->formats_list_size = ARRAY_SIZE(sama7g5_formats_list);
+
+   /* sama7g5-isc RAM access port is full AXI4 - 32 bits per beat */
+   isc->dcfg = ISC_DCFG_YMBSIZE_BEATS32 | ISC_DCFG_CMBSIZE_BEATS32;
+
+   ret = isc_pipeline_init(isc);
+   if (ret)
+           return ret;
+
+   isc->hclock = devm_clk_get(dev, "hclock");
+   if (IS_ERR(isc->hclock)) {
+           ret = PTR_ERR(isc->hclock);
+           dev_err(dev, "failed to get hclock: %d\n", ret);
+           return ret;
+   }
+
+   ret = clk_prepare_enable(isc->hclock);
+   if (ret) {
+           dev_err(dev, "failed to enable hclock: %d\n", ret);
+           return ret;
+   }
+
+   ret = isc_clk_init(isc);
+   if (ret) {
+           dev_err(dev, "failed to init isc clock: %d\n", ret);
+           goto unprepare_hclk;
+   }
+
+   isc->ispck = isc->isc_clks[ISC_ISPCK].clk;
+
+   ret = clk_prepare_enable(isc->ispck);
+   if (ret) {
+           dev_err(dev, "failed to enable ispck: %d\n", ret);
+           goto unprepare_hclk;
+   }
+
+   /* ispck should be greater or equal to hclock */
+   ret = clk_set_rate(isc->ispck, clk_get_rate(isc->hclock));
+   if (ret) {
+           dev_err(dev, "failed to set ispck rate: %d\n", ret);
+           goto unprepare_clk;
+   }
+
+   ret = v4l2_device_register(dev, &isc->v4l2_dev);
+   if (ret) {
+           dev_err(dev, "unable to register v4l2 device.\n");
+           goto unprepare_clk;
+   }
+
+   ret = xisc_parse_dt(dev, isc);
+   if (ret) {
+           dev_err(dev, "fail to parse device tree\n");
+           goto unregister_v4l2_device;
+   }
+
+   if (list_empty(&isc->subdev_entities)) {
+           dev_err(dev, "no subdev found\n");
+           ret = -ENODEV;
+           goto unregister_v4l2_device;
+   }
+
+   list_for_each_entry(subdev_entity, &isc->subdev_entities, list) {
+           struct v4l2_async_subdev *asd;
+
+           v4l2_async_notifier_init(&subdev_entity->notifier);
+
+           asd = v4l2_async_notifier_add_fwnode_remote_subdev(
+                                   &subdev_entity->notifier,
+                                   of_fwnode_handle(subdev_entity->epn),
+                                   struct v4l2_async_subdev);
+
+           of_node_put(subdev_entity->epn);
+           subdev_entity->epn = NULL;
+
+           if (IS_ERR(asd)) {
+                   ret = PTR_ERR(asd);
+                   goto cleanup_subdev;
+           }
+
+           subdev_entity->notifier.ops = &isc_async_ops;
+
+           ret = v4l2_async_notifier_register(&isc->v4l2_dev,
+                                              &subdev_entity->notifier);
+           if (ret) {
+                   dev_err(dev, "fail to register async notifier\n");
+                   goto cleanup_subdev;
+           }
+
+           if (video_is_registered(&isc->video_dev))
+                   break;
+   }
+
+   pm_runtime_set_active(dev);
+   pm_runtime_enable(dev);
+   pm_request_idle(dev);
+
+   regmap_read(isc->regmap, ISC_VERSION + isc->offsets.version, &ver);
+   dev_info(dev, "Microchip XISC version %x\n", ver);
+
+   return 0;
+
+cleanup_subdev:
+   isc_subdev_cleanup(isc);
+
+unregister_v4l2_device:
+   v4l2_device_unregister(&isc->v4l2_dev);
+
+unprepare_clk:
+   clk_disable_unprepare(isc->ispck);
+unprepare_hclk:
+   clk_disable_unprepare(isc->hclock);
+
+   isc_clk_cleanup(isc);
+
+   return ret;
+}
+
+static int microchip_xisc_remove(struct platform_device *pdev)
+{
+   struct isc_device *isc = platform_get_drvdata(pdev);
+
+   pm_runtime_disable(&pdev->dev);
+
+   isc_subdev_cleanup(isc);
+
+   v4l2_device_unregister(&isc->v4l2_dev);
+
+   clk_disable_unprepare(isc->ispck);
+   clk_disable_unprepare(isc->hclock);
+
+   isc_clk_cleanup(isc);
+
+   return 0;
+}
+
+static int __maybe_unused xisc_runtime_suspend(struct device *dev)
+{
+   struct isc_device *isc = dev_get_drvdata(dev);
+
+   clk_disable_unprepare(isc->ispck);
+   clk_disable_unprepare(isc->hclock);
+
+   return 0;
+}
+
+static int __maybe_unused xisc_runtime_resume(struct device *dev)
+{
+   struct isc_device *isc = dev_get_drvdata(dev);
+   int ret;
+
+   ret = clk_prepare_enable(isc->hclock);
+   if (ret)
+           return ret;
+
+   ret = clk_prepare_enable(isc->ispck);
+   if (ret)
+           clk_disable_unprepare(isc->hclock);
+
+   return ret;
+}
+
+static const struct dev_pm_ops microchip_xisc_dev_pm_ops = {
+   SET_RUNTIME_PM_OPS(xisc_runtime_suspend, xisc_runtime_resume, NULL)
+};
+
+static const struct of_device_id microchip_xisc_of_match[] = {
+   { .compatible = "microchip,sama7g5-isc" },
+   { }
+};
+MODULE_DEVICE_TABLE(of, microchip_xisc_of_match);
+
+static struct platform_driver microchip_xisc_driver = {
+   .probe  = microchip_xisc_probe,
+   .remove = microchip_xisc_remove,
+   .driver = {
+           .name           = "microchip-sama7g5-xisc",
+           .pm             = &microchip_xisc_dev_pm_ops,
+           .of_match_table = of_match_ptr(microchip_xisc_of_match),
+   },
+};
+
+module_platform_driver(microchip_xisc_driver);
+
+MODULE_AUTHOR("Eugen Hristev [off-list ref]");
+MODULE_DESCRIPTION("The V4L2 driver for Microchip-XISC");
+MODULE_LICENSE("GPL v2");
+MODULE_SUPPORTED_DEVICE("video");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel