Re: [PATCH v2 30/30] media: atmel: atmel-isc: add microchip-xisc driver

From: Eugen.Hristev
Date: Mon Apr 12 2021 - 10:15:43 EST

On 4/12/21 4:41 PM, Jacopo Mondi wrote:
> Hi Eugene,
>
> On Mon, Apr 12, 2021 at 12:37:41PM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:
>>> +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 ?
Can't I simply check the value of v4l2_epn.bus_type , as I am doing it
already ?

>
> Not the greatest API, but it's more robust.
>
>> 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
>
>>
>> Thanks,
>> Eugen
>>
>>> +
>>> + 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 <eugen.hristev@xxxxxxxxxxxxx>");
>>> +MODULE_DESCRIPTION("The V4L2 driver for Microchip-XISC");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_SUPPORTED_DEVICE("video");
>>>
>>