Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751403AbdFYVNh (ORCPT ); Sun, 25 Jun 2017 17:13:37 -0400 Received: from mail.kernel.org ([198.145.29.99]:34060 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751343AbdFYVNf (ORCPT ); Sun, 25 Jun 2017 17:13:35 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 27D4322B5C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=snawrocki@kernel.org Subject: Re: [PATCH v4 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver To: Jose Abreu Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Carlos Palminha , Mauro Carvalho Chehab , Hans Verkuil References: <314b7ae92c9924d0991e14ccad80ca937a2d7b07.1497978962.git.joabreu@synopsys.com> From: Sylwester Nawrocki Message-ID: Date: Sun, 25 Jun 2017 23:13:26 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <314b7ae92c9924d0991e14ccad80ca937a2d7b07.1497978962.git.joabreu@synopsys.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9482 Lines: 343 On 06/20/2017 07:26 PM, Jose Abreu wrote: > This is an initial submission for the Synopsys Designware HDMI RX > Controller Driver. This driver interacts with a phy driver so that > a communication between them is created and a video pipeline is > configured. > Signed-off-by: Jose Abreu > Cc: Carlos Palminha > Cc: Mauro Carvalho Chehab > Cc: Hans Verkuil > Cc: Sylwester Nawrocki > > Changes from v3: > - Use v4l2 async API (Sylwester) > - Do not block waiting for phy > - Do not use busy waiting delays (Sylwester) > - Simplify dw_hdmi_power_on (Sylwester) > - Use clock API (Sylwester) > - Use compatible string (Sylwester) > - Minor fixes (Sylwester) > Changes from v2: > - Address review comments from Hans regarding CEC > - Use CEC notifier > - Enable SCDC > Changes from v1: > - Add support for CEC > - Correct typo errors > - Correctly detect interlaced video modes > - Correct VIC parsing > Changes from RFC: > - Add support for HDCP 1.4 > - Fixup HDMI_VIC not being parsed (Hans) > - Send source change signal when powering off (Hans) > - Add a "wait stable delay" > - Detect interlaced video modes (Hans) > - Restrain g/s_register from reading/writing to HDCP regs (Hans) > --- > drivers/media/platform/dwc/Kconfig | 15 + > drivers/media/platform/dwc/Makefile | 1 + > drivers/media/platform/dwc/dw-hdmi-rx.c | 1862 +++++++++++++++++++++++++++++++ > drivers/media/platform/dwc/dw-hdmi-rx.h | 441 ++++++++ > include/media/dwc/dw-hdmi-rx-pdata.h | 97 ++ > 5 files changed, 2416 insertions(+) > create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.c > create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.h > create mode 100644 include/media/dwc/dw-hdmi-rx-pdata.h > > diff --git a/drivers/media/platform/dwc/dw-hdmi-rx.c b/drivers/media/platform/dwc/dw-hdmi-rx.c > new file mode 100644 > index 0000000..22ee51d > --- /dev/null > +++ b/drivers/media/platform/dwc/dw-hdmi-rx.c > +static const struct of_device_id dw_hdmi_supported_phys[] = { > + { .compatible = "snps,dw-hdmi-phy-e405", .data = DW_PHY_E405_DRVNAME, }, > + { }, > +}; > + > +static struct device_node *dw_hdmi_get_phy_of_node(struct dw_hdmi_dev *dw_dev, > + const struct of_device_id **found_id) > +{ > + struct device_node *child = NULL; > + const struct of_device_id *id; > + > + for_each_child_of_node(dw_dev->of_node, child) { > + id = of_match_node(dw_hdmi_supported_phys, child); > + if (id) > + break; > + } > + > + if (found_id) > + *found_id = id; > + > + return child; > +} > + > +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev) > +{ > + struct dw_phy_pdata *phy = &dw_dev->phy_config; > + const struct of_device_id *of_id; > + struct of_dev_auxdata lookup; struct of_dev_auxdata lookup = { }; You could initialize to 0 here and... > + struct device_node *child; > + const char *drvname; > + int ret; > + > + child = dw_hdmi_get_phy_of_node(dw_dev, &of_id); > + if (!child || !of_id || !of_id->data) { > + dev_err(dw_dev->dev, "no supported phy found in DT\n"); > + return -EINVAL; > + } > + > + drvname = of_id->data; > + phy->funcs = &dw_hdmi_phy_funcs; > + phy->funcs_arg = dw_dev; > + > + lookup.compatible = (char *)of_id->compatible; > + lookup.phys_addr = 0x0; > + lookup.name = NULL; ...drop these two assignments. > + lookup.platform_data = phy; > + > + request_module(drvname); I'd say this request_module() is not needed when you use the v4l2-async subnotifiers and the modules are properly installed in the file system. I might be missing something though. > + ret = of_platform_populate(dw_dev->of_node, NULL, &lookup, dw_dev->dev); > + if (ret) { > + dev_err(dw_dev->dev, "failed to populate phy driver\n"); > + return ret; > + } > + > + return 0; > +} > + > +static void dw_hdmi_phy_exit(struct dw_hdmi_dev *dw_dev) > +{ > + of_platform_depopulate(dw_dev->dev); > +} > +static int dw_hdmi_registered(struct v4l2_subdev *sd) > +{ > + struct dw_hdmi_dev *dw_dev = to_dw_dev(sd); > + int ret; > + > + ret = cec_register_adapter(dw_dev->cec_adap, dw_dev->dev); > + if (ret) { > + dev_err(dw_dev->dev, "failed to register CEC adapter\n"); > + cec_delete_adapter(dw_dev->cec_adap); > + return ret; > + } > + > + cec_register_cec_notifier(dw_dev->cec_adap, dw_dev->cec_notifier); > + dw_dev->registered = true; > + return ret; > +} > + > +static void dw_hdmi_unregistered(struct v4l2_subdev *sd) > +{ > + struct dw_hdmi_dev *dw_dev = to_dw_dev(sd); > + > + cec_unregister_adapter(dw_dev->cec_adap); > + cec_notifier_put(dw_dev->cec_notifier); > +} > + > +static const struct v4l2_subdev_internal_ops dw_hdmi_internal_ops = { > + .registered = dw_hdmi_registered, > + .unregistered = dw_hdmi_unregistered, > +}; > + > +static int dw_hdmi_v4l2_notify_bound(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *subdev, struct v4l2_async_subdev *asd) > +{ > + struct dw_hdmi_dev *dw_dev = notifier_to_dw_dev(notifier); > + > + if (dw_dev->phy_async_sd.match.fwnode.fwnode == > + of_fwnode_handle(subdev->dev->of_node)) { > + dev_dbg(dw_dev->dev, "found new subdev '%s'\n", subdev->name); > + dw_dev->phy_sd = subdev; > + return 0; > + } > + > + return -EINVAL; > +} > + > +static void dw_hdmi_v4l2_notify_unbind(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *subdev, struct v4l2_async_subdev *asd) > +{ > + struct dw_hdmi_dev *dw_dev = notifier_to_dw_dev(notifier); > + > + if (dw_dev->phy_sd == subdev) { > + dev_dbg(dw_dev->dev, "unbinding '%s'\n", subdev->name); > + dw_dev->phy_sd = NULL; > + } > +} > + > +static int dw_hdmi_v4l2_notify_complete(struct v4l2_async_notifier *notifier) > +{ > + struct dw_hdmi_dev *dw_dev = notifier_to_dw_dev(notifier); > + int ret; > + > + ret = v4l2_device_register_subdev_nodes(&dw_dev->v4l2_dev); There shouldn't be multiple struct v4l2_device instances, instead we should have only one created by the main driver. AFAIU, in your case it would be driver associated with the dw-hdmi-soc DT node. And normally such a top level driver creates subdev device nodes when its all required sub-devices are available. I think this patch could be useful for you: https://patchwork.linuxtv.org/patch/41834 With that the dw-hdmi-soc driver would have it's v4l2-async notifier's notify_complete() callback called only when both the hdmi-rx and the hdmi-phy subdevs are registered. > + if (ret) { > + dev_err(dw_dev->dev, "failed to register subdev nodes\n"); > + return ret; > + } > + > + return 0; > +} > +static int dw_hdmi_parse_dt(struct dw_hdmi_dev *dw_dev) > +{ > + ret = clk_prepare_enable(dw_dev->clk); > + if (ret) { > + dev_err(dw_dev->dev, "failed to enable cfg-clk\n"); > + return ret; > + } > + > + dw_dev->cfg_clk = clk_get_rate(dw_dev->clk) / 1000000; 1000000U ? > + if (!dw_dev->cfg_clk) { > + dev_err(dw_dev->dev, "invalid cfg-clk frequency\n"); > + ret = -EINVAL; > + goto err_clk; > + } > + > + return 0; > + > +err_clk: > + clk_disable_unprepare(dw_dev->clk); > + return ret; > +} > + > +static int dw_hdmi_rx_probe(struct platform_device *pdev) > +{ > + /* V4L2 initialization */ > + sd = &dw_dev->sd; > + v4l2_subdev_init(sd, &dw_hdmi_sd_ops); > + strlcpy(sd->name, dev_name(dev), sizeof(sd->name)); > + sd->dev = dev; > + sd->internal_ops = &dw_hdmi_internal_ops; > + sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS; Don't you also need V4L2_SUBDEV_FL_HAS_DEVNODE flag set? > + /* Notifier for subdev binding */ > + ret = dw_hdmi_v4l2_init_notifier(dw_dev); > + if (ret) { > + dev_err(dev, "failed to init v4l2 notifier\n"); > + goto err_v4l2_dev; > + } > + > + /* Phy loading */ s/Phy/PHY/ ? > + ret = dw_hdmi_phy_init(dw_dev); > + if (ret) > + goto err_v4l2_notifier; > + > + ret = v4l2_async_register_subdev(sd); > + if (ret) { > + dev_err(dev, "failed to register subdev\n"); > + goto err_cec; > + } > + > + /* Fill initial format settings */ > + dw_dev->timings = timings_def; > + dw_dev->mbus_code = MEDIA_BUS_FMT_BGR888_1X24; > + > + /* All done */ This comment seems unneeded. > + dev_set_drvdata(dev, sd); > + dw_dev->state = HDMI_STATE_POWER_OFF; > + dw_hdmi_detect_tx_5v(dw_dev); > + dev_info(dev, "driver probed\n"); dev_dbg() > + return 0; > + > +err_cec: > + cec_delete_adapter(dw_dev->cec_adap); > +err_phy: > + dw_hdmi_phy_exit(dw_dev); > +err_v4l2_notifier: > + v4l2_async_notifier_unregister(&dw_dev->v4l2_notifier); > +err_v4l2_dev: > + v4l2_device_unregister(&dw_dev->v4l2_dev); > +err_wq: > + destroy_workqueue(dw_dev->wq); > + return ret; > +} > + > +static int dw_hdmi_rx_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > + struct dw_hdmi_dev *dw_dev = to_dw_dev(sd); > + > + dev_dbg(dev, "%s\n", __func__); > + > + dw_hdmi_disable_ints(dw_dev); > + dw_hdmi_disable_hpd(dw_dev); > + dw_hdmi_disable_scdc(dw_dev); > + dw_hdmi_power_off(dw_dev); > + dw_hdmi_phy_s_power(dw_dev, false); > + flush_workqueue(dw_dev->wq); > + destroy_workqueue(dw_dev->wq); > + v4l2_device_unregister(&dw_dev->v4l2_dev); > + dw_hdmi_phy_exit(dw_dev); > + v4l2_async_unregister_subdev(sd); > + clk_disable_unprepare(dw_dev->clk); > + dev_info(dev, "driver removed\n"); dev_dbg() > + return 0; > +} > + > +static const struct of_device_id dw_hdmi_rx_id[] = { > + { .compatible = "snps,dw-hdmi-rx" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, dw_hdmi_rx_id); -- Regards, Sylwester