Received: by 2002:a05:6a10:17d3:0:0:0:0 with SMTP id hz19csp1783215pxb; Mon, 12 Apr 2021 06:43:00 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzMSRbTzLBrDw6COR7ozK8f+cdsSjvoLfiTZNBjkLqFkCjf+len+hRvqcezdYyrGN0LjYMP X-Received: by 2002:a17:90b:198c:: with SMTP id mv12mr1649500pjb.51.1618234980344; Mon, 12 Apr 2021 06:43:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1618234980; cv=none; d=google.com; s=arc-20160816; b=rZcn1foP4vqQ6u5rLF0QJpgYKAqTIq3cuvvp7cA4yI63QIi4pq33NOPDmWwqWSq2x8 CX3jEmjkiLMZmXd+aCjnliOGSVBhxM4q36jtKVFAeysIC+cSLPxlJe2/YjBf7TL2FnBa HR3KASizWfFwX7KWd6K0q/mJiX/QjIh4Om76bcH19Qw0833GN20LstgjqdTrasbeUZXE E4+C0TQIG/CX7ftYigwkhJWVklhOxT+o43/857M7NEGhksie4CTnP2orF3TaBpt+MGAI knlEENFVe0JUWWYRcAYG7MbnfuQmZ5JT6TU9Uzbm/Vc0qhJ5uG0uJIrd4zDAIQnVxA1l 2dFg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date; bh=rqtYuM/Avq7JVTsz2MDvplmNTaqVxfVW3B+mKk8o+TI=; b=lhne1v3CUea5nrh0O77apm1RaGOtXMBPLwK24GjVx20EQUA8sx8PaBYvmh0zaTHc9u n6Sk5a9RqzbOGYBjqaz2GUrrZ1SV+uCqlc6bpOEJtIOe6q+EneqGf+vnwSpLiKYJwR+L 1RkD6XoY/CS6NVI2ecnoSU6jVVUVoDQlNN5S3/x94vZCwMiP+IoHacidh47z/Z42YXh2 sF8rs3mjrdv+k1/nQ4iqEtAtWzUkzVycCYgFQLZEYvRrefScSMOISrnZOEmR8Koche+F QyhhDB729vObtPZLgx9aOkSDLMraBqa7hzpTyur3K4CXGJewGBvL7GgsZXumJztBZOaJ 08Rg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id d31si13314487pgm.492.2021.04.12.06.42.32; Mon, 12 Apr 2021 06:43:00 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242009AbhDLNlo (ORCPT + 99 others); Mon, 12 Apr 2021 09:41:44 -0400 Received: from relay4-d.mail.gandi.net ([217.70.183.196]:54005 "EHLO relay4-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239992AbhDLNlj (ORCPT ); Mon, 12 Apr 2021 09:41:39 -0400 X-Originating-IP: 93.34.118.233 Received: from uno.localdomain (93-34-118-233.ip49.fastwebnet.it [93.34.118.233]) (Authenticated sender: jacopo@jmondi.org) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 1FDF3E0012; Mon, 12 Apr 2021 13:41:18 +0000 (UTC) Date: Mon, 12 Apr 2021 15:41:57 +0200 From: Jacopo Mondi To: Eugen.Hristev@microchip.com Cc: devicetree@vger.kernel.org, linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 30/30] media: atmel: atmel-isc: add microchip-xisc driver Message-ID: <20210412134157.4cqva5thylmiqds7@uno.localdomain> References: <20210405155105.162529-1-eugen.hristev@microchip.com> <20210405155105.162529-31-eugen.hristev@microchip.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Eugene, On Mon, Apr 12, 2021 at 12:37:41PM +0000, Eugen.Hristev@microchip.com 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 */ } 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 = µchip_xisc_dev_pm_ops, > > + .of_match_table = of_match_ptr(microchip_xisc_of_match), > > + }, > > +}; > > + > > +module_platform_driver(microchip_xisc_driver); > > + > > +MODULE_AUTHOR("Eugen Hristev "); > > +MODULE_DESCRIPTION("The V4L2 driver for Microchip-XISC"); > > +MODULE_LICENSE("GPL v2"); > > +MODULE_SUPPORTED_DEVICE("video"); > > >