Synopsys Designware HDMI Video Capture Controller + PHY

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.

The controller + phy pipeline can then be integrated into a fully featured system that can be able to receive video up to 4k@60Hz with deep color 48bit RGB, depending on the platform. Although, this initial version does not yet handle deep color modes.

Signed-off-by Jose Abreu <joabreu-HKixBCOQz3hWk0Htik3J/[email protected]>

drivers/media/platform/dwc/dw-hdmi-rx.c new file +19 -0

+static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
+{
+	struct dw_phy_pdata *phy = &dw_dev->phy_config;
+	struct platform_device_info pdevinfo;
+
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+
+	phy->funcs = &dw_hdmi_phy_funcs;
+	phy->funcs_arg = dw_dev;
+
+	pdevinfo.parent = dw_dev->dev;
+	pdevinfo.id = PLATFORM_DEVID_NONE;
+	pdevinfo.name = dw_dev->phy_drv;
+	pdevinfo.data = phy;
+	pdevinfo.size_data = sizeof(*phy);
+	pdevinfo.dma_mask = DMA_BIT_MASK(32);
+
+	request_module(pdevinfo.name);
+

drivers/media/platform/dwc/dw-hdmi-rx.c new file +4 -0

+		dev_err(dw_dev->dev, "failed to get phy module\n");
+		goto err;
+	}
+

drivers/media/platform/dwc/dw-hdmi-rx.c new file +14 -0

+	module_put(dw_dev->phy_pdev->dev.driver->owner);
+	return 0;
+
+err_put:
+	module_put(dw_dev->phy_pdev->dev.driver->owner);
+err:
+	platform_device_unregister(dw_dev->phy_pdev);
+	return -EINVAL;
+}
+
+static void dw_hdmi_phy_exit(struct dw_hdmi_dev *dw_dev)
+{
+	if (!IS_ERR(dw_dev->phy_pdev))
+		platform_device_unregister(dw_dev->phy_pdev);

drivers/media/platform/dwc/dw-hdmi-rx.c new file +25 -0

+static int __dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
+{
+	unsigned long flags;
+	int ret;
+
+	ret = dw_hdmi_config(dw_dev, input);
+
+	spin_lock_irqsave(&dw_dev->lock, flags);
+	dw_dev->pending_config = false;
+	spin_unlock_irqrestore(&dw_dev->lock, flags);
+
+	return ret;
+}
+
+struct dw_hdmi_work_data {
+	struct dw_hdmi_dev *dw_dev;
+	struct work_struct work;
+	u32 input;
+};
+
+static void dw_hdmi_work_handler(struct work_struct *work)
+{
+	struct dw_hdmi_work_data *data = container_of(work,
+			struct dw_hdmi_work_data, work);
+

(quoted diff) +111 -0

+	if (!data)
+		return -ENOMEM;
+
+	INIT_WORK(&data->work, dw_hdmi_work_handler);
+	data->dw_dev = dw_dev;
+	data->input = input;
+
+	spin_lock_irqsave(&dw_dev->lock, flags);
+	if (dw_dev->pending_config) {
+		devm_kfree(dw_dev->dev, data);
+		spin_unlock_irqrestore(&dw_dev->lock, flags);
+		return 0;
+	}
+
+	queue_work(dw_dev->wq, &data->work);
+	dw_dev->pending_config = true;
+	spin_unlock_irqrestore(&dw_dev->lock, flags);
+	return 0;
+}
 
+static irqreturn_t dw_hdmi_irq_handler(int irq, void *dev_data)
+{
+	struct dw_hdmi_dev *dw_dev = dev_data;
+	u32 hdmi_ists = dw_hdmi_get_int_val(dw_dev, HDMI_ISTS, HDMI_IEN);
+	u32 md_ists = dw_hdmi_get_int_val(dw_dev, HDMI_MD_ISTS, HDMI_MD_IEN);
+
+	dw_hdmi_clear_ints(dw_dev);
+
+	if ((hdmi_ists & HDMI_ISTS_CLK_CHANGE) ||
+	    (hdmi_ists & HDMI_ISTS_PLL_LCK_CHG) || md_ists) {
+		dw_hdmi_power_off(dw_dev);
+		if (has_signal(dw_dev, dw_dev->configured_input))
+			dw_hdmi_power_on(dw_dev, dw_dev->configured_input);
 
+	}
+	return IRQ_HANDLED;
+}
 
+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_core_ops dw_hdmi_sd_core_ops = {
+	.log_status = dw_hdmi_log_status,
 
+	.subscribe_event = dw_hdmi_subscribe_event,
+};
+
+static const struct v4l2_subdev_video_ops dw_hdmi_sd_video_ops = {
+	.s_routing = dw_hdmi_s_routing,
+	.g_input_status = dw_hdmi_g_input_status,
+	.g_parm = dw_hdmi_g_parm,
+	.g_dv_timings = dw_hdmi_g_dv_timings,
+	.query_dv_timings = dw_hdmi_query_dv_timings,
+};
+
+static const struct v4l2_subdev_pad_ops dw_hdmi_sd_pad_ops = {
+	.enum_mbus_code = dw_hdmi_enum_mbus_code,
+	.get_fmt = dw_hdmi_get_fmt,
+	.set_fmt = dw_hdmi_set_fmt,
+	.dv_timings_cap = dw_hdmi_dv_timings_cap,
+	.enum_dv_timings = dw_hdmi_enum_dv_timings,
+};
+
+static const struct v4l2_subdev_ops dw_hdmi_sd_ops = {
+	.core = &dw_hdmi_sd_core_ops,
+	.video = &dw_hdmi_sd_video_ops,
+	.pad = &dw_hdmi_sd_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops dw_hdmi_internal_ops = {
+	.registered = dw_hdmi_registered,
+	.unregistered = dw_hdmi_unregistered,
+};
+
+static int dw_hdmi_parse_dt(struct dw_hdmi_dev *dw_dev)
+{
+	struct device_node *notifier, *np = dw_dev->of_node;
+	struct dw_phy_pdata *phy = &dw_dev->phy_config;
+
+	if (!np) {
+		dev_err(dw_dev->dev, "missing DT node\n");
+		return -EINVAL;
+	}
+
+	/* PHY properties parsing */
+	of_property_read_u8(np, "snps,hdmi-phy-jtag-addr",
+			&dw_dev->phy_jtag_addr);
+	if (!dw_dev->phy_jtag_addr) {
+		dev_err(dw_dev->dev, "missing hdmi-phy-jtag-addr in DT\n");
+		return -EINVAL;
+	}
+
+	of_property_read_u32(np, "snps,hdmi-phy-version", &phy->version);

drivers/media/platform/dwc/dw-hdmi-rx.c new file +9 -0

+		return -EINVAL;
+	}
+
+	/* Controller properties parsing */
+	of_property_read_u32(np, "snps,hdmi-ctl-cfg-clk", &dw_dev->cfg_clk);
+	if (!dw_dev->cfg_clk) {
+		dev_err(dw_dev->dev, "missing hdmi-ctl-cfg-clk in DT\n");
+		return -EINVAL;
+	}

drivers/media/platform/dwc/dw-hdmi-rx.c new file +17 -0

+	sd->internal_ops = &dw_hdmi_internal_ops;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
 
+}
+
+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);

drivers/media/platform/dwc/dw-hdmi-rx.c new file +6 -0

+	.driver = {
+		.name = DW_HDMI_RX_DRVNAME,
+	}
+};
+module_platform_driver(dw_hdmi_rx_driver);
 
+#endif /* __DW_HDMI_RX_H__ */

include/media/dwc/dw-hdmi-rx-pdata.h new file +15 -0

new file mode 100644
index 0000000..ff8554d
@@ -0,0 +1,63 @@
 
+#ifndef __DW_HDMI_RX_PDATA_H__
+#define __DW_HDMI_RX_PDATA_H__
+
+#define DW_HDMI_RX_DRVNAME			"dw-hdmi-rx"
+
+/* Notify events */
+#define DW_HDMI_NOTIFY_IS_OFF		1
+#define DW_HDMI_NOTIFY_INPUT_CHANGED	2
+#define DW_HDMI_NOTIFY_AUDIO_CHANGED	3
+#define DW_HDMI_NOTIFY_IS_STABLE	4
+
+/* HDCP 1.4 */
+#define DW_HDMI_HDCP14_BKSV_SIZE	2
+#define DW_HDMI_HDCP14_KEYS_SIZE	(2 * 40)
+

Quoted 29 lines · Jose Abreu

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.

The controller + phy pipeline can then be integrated into a fully featured system that can be able to receive video up to 4k@60Hz with deep color 48bit RGB, depending on the platform. Although, this initial version does not yet handle deep color modes.

Signed-off-by Jose Abreu <[email protected]>

drivers/media/platform/dwc/dw-hdmi-rx.c new file +19 -0

+static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
+{
+	struct dw_phy_pdata *phy = &dw_dev->phy_config;
+	struct platform_device_info pdevinfo;
+
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+
+	phy->funcs = &dw_hdmi_phy_funcs;
+	phy->funcs_arg = dw_dev;
+
+	pdevinfo.parent = dw_dev->dev;
+	pdevinfo.id = PLATFORM_DEVID_NONE;
+	pdevinfo.name = dw_dev->phy_drv;
+	pdevinfo.data = phy;
+	pdevinfo.size_data = sizeof(*phy);
+	pdevinfo.dma_mask = DMA_BIT_MASK(32);
+
+	request_module(pdevinfo.name);
+

drivers/media/platform/dwc/dw-hdmi-rx.c line 431

Quoted 10 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +10 -0

+	dw_dev->phy_pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(dw_dev->phy_pdev)) {
+		dev_err(dw_dev->dev, "failed to register phy device\n");
+		return PTR_ERR(dw_dev->phy_pdev);
+	}
+
+	if (!dw_dev->phy_pdev->dev.driver) {
+		dev_err(dw_dev->dev, "failed to initialize phy driver\n");
+		goto err;
+	}

I think this is not safe because there is nothing preventing unbinding or unloading the driver at this point.

drivers/media/platform/dwc/dw-hdmi-rx.c line 433

Quoted 1 line

drivers/media/platform/dwc/dw-hdmi-rx.c new file +1 -0

+	if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {

So dw_dev->phy_pdev->dev.driver may be already NULL here.

Quoted 5 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +4 -0

+		dev_err(dw_dev->dev, "failed to get phy module\n");
+		goto err;
+	}
+

drivers/media/platform/dwc/dw-hdmi-rx.c line 447

Quoted 10 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +10 -0

+	dw_dev->phy_sd = dev_get_drvdata(&dw_dev->phy_pdev->dev);
+	if (!dw_dev->phy_sd) {
+		dev_err(dw_dev->dev, "failed to get phy subdev\n");
+		goto err_put;
+	}
+
+	if (v4l2_device_register_subdev(&dw_dev->v4l2_dev, dw_dev->phy_sd)) {
+		dev_err(dw_dev->dev, "failed to register phy subdev\n");
+		goto err_put;
+	}

I'd suggest usign v4l2-async API, so we use a common pattern for sub-device registration. And with recent change [1] you could handle this PHY subdev in a standard way. That might be more complicated than it is now but should make any future platform integration easier.

[1] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.linuxtv.org_patch_41834&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=uHTyp6WsEj6vN19Zc09HqSNUhCx62OI8u-tAgi-EVts&s=WjyPjIwN1uGvPoV7ZlcmzOgdptakzluHywuKRA8ZG8M&e=

Quoted 17 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +16 -0

+	module_put(dw_dev->phy_pdev->dev.driver->owner);
+	return 0;
+
+err_put:
+	module_put(dw_dev->phy_pdev->dev.driver->owner);
+err:
+	platform_device_unregister(dw_dev->phy_pdev);
+	return -EINVAL;
+}
+
+static void dw_hdmi_phy_exit(struct dw_hdmi_dev *dw_dev)
+{
+	if (!IS_ERR(dw_dev->phy_pdev))
+		platform_device_unregister(dw_dev->phy_pdev);
+}
+static int dw_hdmi_config_hdcp(struct dw_hdmi_dev *dw_dev)

drivers/media/platform/dwc/dw-hdmi-rx.c line 964

Quoted 6 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +6 -0

+{
+	for (i = 0; i < DW_HDMI_HDCP14_KEYS_SIZE; i += 2) {
+		for (j = 0; j < key_write_tries; j++) {
+			if (is_hdcp14_key_write_allowed(dw_dev))
+				break;
+			mdelay(10);

usleep_range()? I've seen more (busy waiting) mdelay() calls in this patch series.

Quoted 34 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +32 -0

+static int __dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
+{
+	unsigned long flags;
+	int ret;
+
+	ret = dw_hdmi_config(dw_dev, input);
+
+	spin_lock_irqsave(&dw_dev->lock, flags);
+	dw_dev->pending_config = false;
+	spin_unlock_irqrestore(&dw_dev->lock, flags);
+
+	return ret;
+}
+
+struct dw_hdmi_work_data {
+	struct dw_hdmi_dev *dw_dev;
+	struct work_struct work;
+	u32 input;
+};
+
+static void dw_hdmi_work_handler(struct work_struct *work)
+{
+	struct dw_hdmi_work_data *data = container_of(work,
+			struct dw_hdmi_work_data, work);
+
+	__dw_hdmi_power_on(data->dw_dev, data->input);
+	devm_kfree(data->dw_dev->dev, data);
+}
+
+static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
+{
+	struct dw_hdmi_work_data *data;

drivers/media/platform/dwc/dw-hdmi-rx.c line 1017

Quoted 3 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +3 -0

+	unsigned long flags;
+
+	data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);

Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called in the device's probe() callback, but in other places, including interrupt handler? devm_* API is normally used when life time of a resource is more or less equal to life time of struct device or its matched driver. Were there any specific reasons to not just use kzalloc()/kfree() ?

Quoted 112 lines

(quoted diff) +111 -0

+	if (!data)
+		return -ENOMEM;
+
+	INIT_WORK(&data->work, dw_hdmi_work_handler);
+	data->dw_dev = dw_dev;
+	data->input = input;
+
+	spin_lock_irqsave(&dw_dev->lock, flags);
+	if (dw_dev->pending_config) {
+		devm_kfree(dw_dev->dev, data);
+		spin_unlock_irqrestore(&dw_dev->lock, flags);
+		return 0;
+	}
+
+	queue_work(dw_dev->wq, &data->work);
+	dw_dev->pending_config = true;
+	spin_unlock_irqrestore(&dw_dev->lock, flags);
+	return 0;
+}
+static irqreturn_t dw_hdmi_irq_handler(int irq, void *dev_data)
+{
+	struct dw_hdmi_dev *dw_dev = dev_data;
+	u32 hdmi_ists = dw_hdmi_get_int_val(dw_dev, HDMI_ISTS, HDMI_IEN);
+	u32 md_ists = dw_hdmi_get_int_val(dw_dev, HDMI_MD_ISTS, HDMI_MD_IEN);
+
+	dw_hdmi_clear_ints(dw_dev);
+
+	if ((hdmi_ists & HDMI_ISTS_CLK_CHANGE) ||
+	    (hdmi_ists & HDMI_ISTS_PLL_LCK_CHG) || md_ists) {
+		dw_hdmi_power_off(dw_dev);
+		if (has_signal(dw_dev, dw_dev->configured_input))
+			dw_hdmi_power_on(dw_dev, dw_dev->configured_input);
+	}
+	return IRQ_HANDLED;
+}
+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_core_ops dw_hdmi_sd_core_ops = {
+	.log_status = dw_hdmi_log_status,
+	.subscribe_event = dw_hdmi_subscribe_event,
+};
+
+static const struct v4l2_subdev_video_ops dw_hdmi_sd_video_ops = {
+	.s_routing = dw_hdmi_s_routing,
+	.g_input_status = dw_hdmi_g_input_status,
+	.g_parm = dw_hdmi_g_parm,
+	.g_dv_timings = dw_hdmi_g_dv_timings,
+	.query_dv_timings = dw_hdmi_query_dv_timings,
+};
+
+static const struct v4l2_subdev_pad_ops dw_hdmi_sd_pad_ops = {
+	.enum_mbus_code = dw_hdmi_enum_mbus_code,
+	.get_fmt = dw_hdmi_get_fmt,
+	.set_fmt = dw_hdmi_set_fmt,
+	.dv_timings_cap = dw_hdmi_dv_timings_cap,
+	.enum_dv_timings = dw_hdmi_enum_dv_timings,
+};
+
+static const struct v4l2_subdev_ops dw_hdmi_sd_ops = {
+	.core = &dw_hdmi_sd_core_ops,
+	.video = &dw_hdmi_sd_video_ops,
+	.pad = &dw_hdmi_sd_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops dw_hdmi_internal_ops = {
+	.registered = dw_hdmi_registered,
+	.unregistered = dw_hdmi_unregistered,
+};
+
+static int dw_hdmi_parse_dt(struct dw_hdmi_dev *dw_dev)
+{
+	struct device_node *notifier, *np = dw_dev->of_node;
+	struct dw_phy_pdata *phy = &dw_dev->phy_config;
+
+	if (!np) {
+		dev_err(dw_dev->dev, "missing DT node\n");
+		return -EINVAL;
+	}
+
+	/* PHY properties parsing */
+	of_property_read_u8(np, "snps,hdmi-phy-jtag-addr",
+			&dw_dev->phy_jtag_addr);
+	if (!dw_dev->phy_jtag_addr) {
+		dev_err(dw_dev->dev, "missing hdmi-phy-jtag-addr in DT\n");
+		return -EINVAL;
+	}
+
+	of_property_read_u32(np, "snps,hdmi-phy-version", &phy->version);

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (!phy->version) {
+		dev_err(dw_dev->dev, "missing hdmi-phy-version in DT\n");
+		return -EINVAL;
+	}
+
+	of_property_read_u32(np, "snps,hdmi-phy-cfg-clk", &phy->cfg_clk);
+	if (!phy->cfg_clk) {
+		dev_err(dw_dev->dev, "missing hdmi-phy-cfg-clk in DT\n");
+		return -EINVAL;
+	}

With changes as proposed in comments to patch "4/4 dt-bindings: ..." you could use the common clk API for retrieving the clock rate, e.g. devm_clk_get(), clk_get_rate().

When the HDMI RX IP block gets integrated within some SoC I'd expect the system clock controller to be already using the common clk DT bindings. Unless for some reason the platform doesn't support CCF.

drivers/media/platform/dwc/dw-hdmi-rx.c line 1571

Quoted 5 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +3 -0

+	if (of_property_read_string_index(np, "snps,hdmi-phy-driver", 0,
+				&dw_dev->phy_drv) < 0) {
+		dev_err(dw_dev->dev, "missing hdmi-phy-driver in DT\n");

I don't think we can put Linux driver names in DT like this, it seems rather a serious abuse. With proposed changes to the DT binding you could reference the PHY device by DT phandle or child node.

Quoted 9 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +9 -0

+		return -EINVAL;
+	}
+
+	/* Controller properties parsing */
+	of_property_read_u32(np, "snps,hdmi-ctl-cfg-clk", &dw_dev->cfg_clk);
+	if (!dw_dev->cfg_clk) {
+		dev_err(dw_dev->dev, "missing hdmi-ctl-cfg-clk in DT\n");
+		return -EINVAL;
+	}

drivers/media/platform/dwc/dw-hdmi-rx.c line 1590

Quoted 10 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +10 -0

+
+#if IS_ENABLED(CONFIG_VIDEO_DWC_HDMI_RX_CEC)
+	/* Notifier device parsing */
+	notifier = of_parse_phandle(np, "edid-phandle", 0);
+	if (!notifier) {
+		dev_err(dw_dev->dev, "missing edid-phandle in DT\n");
+		return -EINVAL;
+	}
+
+	dw_dev->notifier_pdev = of_find_device_by_node(np);

Shouldn't this be:

	dw_dev->notifier_pdev = of_find_device_by_node(notifier);

?

The caller of dw_hdmi_parse_dt() already knows about the device associated with np.

drivers/media/platform/dwc/dw-hdmi-rx.c line 1633

Quoted 12 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +11 -0

+	if (!dw_dev->notifier_pdev)
+		return -EPROBE_DEFER;
+#endif
+
+	return 0;
+}
+
+static int dw_hdmi_rx_probe(struct platform_device *pdev)
+{
+	/* Deferred work */
+	dw_dev->wq = create_workqueue(DW_HDMI_RX_DRVNAME);

Have you considered using create_singlethread_workqueue() ? create_workqueue() will spawn one thread per CPU.

drivers/media/platform/dwc/dw-hdmi-rx.c line 1642

Quoted 10 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +9 -0

+	if (!dw_dev->wq)
+		return -ENOMEM;
+
+	/* Registers mapping */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		ret = -ENXIO;
+		goto err_wq;
+	}

You can drop res testing here, devm_ioremap_resource() verifies internally if res is valid and returns proper error code.

Quoted 6 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +5 -0

+	dw_dev->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(dw_dev->regs)) {
+		ret = PTR_ERR(dw_dev->regs);
+		goto err_wq;
+	}

drivers/media/platform/dwc/dw-hdmi-rx.c line 1685

Quoted 4 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +4 -0

+	/* V4L2 initialization */
+	sd = &dw_dev->sd;
+	v4l2_subdev_init(sd, &dw_hdmi_sd_ops);
+	strlcpy(sd->name, DW_HDMI_RX_DRVNAME, sizeof(sd->name));

sd->name should be unique, you could, for instance, do something like

	strlcpy(sd->name, dev_name(&pdev->dev), sizeof(sd->name));

Quoted 18 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +17 -0

+	sd->internal_ops = &dw_hdmi_internal_ops;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
+}
+
+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);

drivers/media/platform/dwc/dw-hdmi-rx.c line 1774

Quoted 10 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +10 -0

+	flush_workqueue(dw_dev->wq);
+	destroy_workqueue(dw_dev->wq);
+	v4l2_device_unregister(&dw_dev->v4l2_dev);
+	dw_hdmi_phy_exit(dw_dev);> +	dev_info(dev, "driver removed\n");
+	return 0;
+}
+
+static struct platform_driver dw_hdmi_rx_driver = {
+	.probe = dw_hdmi_rx_probe,
+	.remove = dw_hdmi_rx_remove,

I think we need also .of_match_table here.

Quoted 30 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +6 -0

+	.driver = {
+		.name = DW_HDMI_RX_DRVNAME,
+	}
+};
+module_platform_driver(dw_hdmi_rx_driver);
+#endif /* __DW_HDMI_RX_H__ */

include/media/dwc/dw-hdmi-rx-pdata.h new file +18 -0

new file mode 100644
index 0000000..ff8554d
@@ -0,0 +1,63 @@
+#ifndef __DW_HDMI_RX_PDATA_H__
+#define __DW_HDMI_RX_PDATA_H__
+
+#define DW_HDMI_RX_DRVNAME			"dw-hdmi-rx"
+
+/* Notify events */
+#define DW_HDMI_NOTIFY_IS_OFF		1
+#define DW_HDMI_NOTIFY_INPUT_CHANGED	2
+#define DW_HDMI_NOTIFY_AUDIO_CHANGED	3
+#define DW_HDMI_NOTIFY_IS_STABLE	4
+
+/* HDCP 1.4 */
+#define DW_HDMI_HDCP14_BKSV_SIZE	2
+#define DW_HDMI_HDCP14_KEYS_SIZE	(2 * 40)
+
+struct dw_hdmi_hdcp14_key {
+	u32 seed;
+	u32 bksv[DW_HDMI_HDCP14_BKSV_SIZE];

include/media/dwc/dw-hdmi-rx-pdata.h line 51

Quoted 7 lines

include/media/dwc/dw-hdmi-rx-pdata.h new file +7 -0

+	u32 keys[DW_HDMI_HDCP14_KEYS_SIZE];
+	bool keys_valid;
+};
+
+struct dw_hdmi_rx_pdata {
+	/* Controller configuration */
+	unsigned int iref_clk; /* MHz */

Is this field unused?

include/media/dwc/dw-hdmi-rx-pdata.h line 60

Quoted 9 lines

include/media/dwc/dw-hdmi-rx-pdata.h new file +8 -0

+	struct dw_hdmi_hdcp14_key hdcp14_keys;
+	/* 5V sense interface */
+	bool (*dw_5v_status)(void __iomem *regs, int input);
+	void (*dw_5v_clear)(void __iomem *regs);
+	void __iomem *dw_5v_arg;> +	/* Zcal interface */
+	void (*dw_zcal_reset)(void __iomem *regs);
+	bool (*dw_zcal_done)(void __iomem *regs);
+	void __iomem *dw_zcal_arg;

I'm just wondering if these operations could be modeled with the regmap, so we could avoid callbacks in the platform data structure.

include/media/dwc/dw-hdmi-rx-pdata.h line 35

Quoted 3 lines

include/media/dwc/dw-hdmi-rx-pdata.h new file +2 -0

+};
+#endif /* __DW_HDMI_RX_PDATA_H__ */

--
Regards,
Sylwester

Quoted 30 lines · Sylwester Nawrocki

Quoted 16 lines · Jose Abreu

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.

The controller + phy pipeline can then be integrated into a fully featured system that can be able to receive video up to 4k@60Hz with deep color 48bit RGB, depending on the platform. Although, this initial version does not yet handle deep color modes.

Signed-off-by Jose Abreu <joabreu-HKixBCOQz3hWk0Htik3J/[email protected]>

drivers/media/platform/dwc/dw-hdmi-rx.c new file +4 -0

+static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
+{
 
+	request_module(pdevinfo.name);
+

drivers/media/platform/dwc/dw-hdmi-rx.c line 431

Quoted 10 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +10 -0

+	dw_dev->phy_pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(dw_dev->phy_pdev)) {
+		dev_err(dw_dev->dev, "failed to register phy device\n");
+		return PTR_ERR(dw_dev->phy_pdev);
+	}
+
+	if (!dw_dev->phy_pdev->dev.driver) {
+		dev_err(dw_dev->dev, "failed to initialize phy driver\n");
+		goto err;
+	}

I think this is not safe because there is nothing preventing unbinding or unloading the driver at this point.

Quoted 2 lines

drivers/media/platform/dwc/dw-hdmi-rx.c line 433

Quoted 1 line

drivers/media/platform/dwc/dw-hdmi-rx.c new file +1 -0

+	if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {

So dw_dev->phy_pdev->dev.driver may be already NULL here.

How can I make sure it wont be NULL? Because I've seen other media drivers do this and I don't think they do any kind of locking, but they do this mainly for I2C subdevs.

Quoted 11 lines

Quoted 11 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +11 -0

+		dev_err(dw_dev->dev, "failed to get phy module\n");
+		goto err;
+	}
+
+	dw_dev->phy_sd = dev_get_drvdata(&dw_dev->phy_pdev->dev);
+	if (!dw_dev->phy_sd) {
+		dev_err(dw_dev->dev, "failed to get phy subdev\n");
+		goto err_put;
+	}
+
+	if (v4l2_device_register_subdev(&dw_dev->v4l2_dev, dw_dev->phy_sd)) {

Quoted 8 lines

drivers/media/platform/dwc/dw-hdmi-rx.c line 447

Quoted 3 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +3 -0

+		dev_err(dw_dev->dev, "failed to register phy subdev\n");
+		goto err_put;
+	}

I'd suggest usign v4l2-async API, so we use a common pattern for sub-device registration. And with recent change [1] you could handle this PHY subdev in a standard way. That might be more complicated than it is now but should make any future platform integration easier.

So I will instantiate phy driver and then wait for phy driver to register into v4l2 core?

Quoted 10 lines

Quoted 9 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +9 -0

+	module_put(dw_dev->phy_pdev->dev.driver->owner);
+	return 0;
+
+err_put:
+	module_put(dw_dev->phy_pdev->dev.driver->owner);
+err:
+	platform_device_unregister(dw_dev->phy_pdev);
+	return -EINVAL;
+}

Quoted 12 lines

drivers/media/platform/dwc/dw-hdmi-rx.c line 1017

Quoted 6 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +6 -0

+static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
+{
+	struct dw_hdmi_work_data *data;
+	unsigned long flags;
+
+	data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);

Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called in the device's probe() callback, but in other places, including interrupt handler? devm_* API is normally used when life time of a resource is more or less equal to life time of struct device or its matched driver. Were there any specific reasons to not just use kzalloc()/kfree() ?

No specific reason, I just thought it would be safer because if I cancel a work before it started then memory will remain allocated. But I will change to kzalloc().

Quoted 23 lines

Quoted 22 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +21 -0

+	if (!data)
+		return -ENOMEM;
+
+	INIT_WORK(&data->work, dw_hdmi_work_handler);
+	data->dw_dev = dw_dev;
+	data->input = input;
+
+	spin_lock_irqsave(&dw_dev->lock, flags);
+	if (dw_dev->pending_config) {
+		devm_kfree(dw_dev->dev, data);
+		spin_unlock_irqrestore(&dw_dev->lock, flags);
+		return 0;
+	}
+
+	queue_work(dw_dev->wq, &data->work);
+	dw_dev->pending_config = true;
+	spin_unlock_irqrestore(&dw_dev->lock, flags);
+	return 0;
+}
 
+struct dw_hdmi_rx_pdata {
+	/* Controller configuration */

Quoted 11 lines

include/media/dwc/dw-hdmi-rx-pdata.h line 60

Quoted 8 lines

include/media/dwc/dw-hdmi-rx-pdata.h new file +8 -0

+	struct dw_hdmi_hdcp14_key hdcp14_keys;
+	/* 5V sense interface */
+	bool (*dw_5v_status)(void __iomem *regs, int input);
+	void (*dw_5v_clear)(void __iomem *regs);
+	void __iomem *dw_5v_arg;> +	/* Zcal interface */
+	void (*dw_zcal_reset)(void __iomem *regs);
+	bool (*dw_zcal_done)(void __iomem *regs);
+	void __iomem *dw_zcal_arg;

I'm just wondering if these operations could be modeled with the regmap, so we could avoid callbacks in the platform data structure.

Hmm, I don't think that is safe because registers may not be adjacent to each other. And maybe I was a little generous in passing a __iomem argument, maybe it should be just void instead because this can be not a regmap at all.

Quoted 32 lines · Jose Abreu

Quoted 31 lines · Sylwester Nawrocki

Quoted 15 lines · Jose Abreu

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.

The controller + phy pipeline can then be integrated into a fully featured system that can be able to receive video up to 4k@60Hz with deep color 48bit RGB, depending on the platform. Although, this initial version does not yet handle deep color modes.

Signed-off-by Jose Abreu <joabreu-HKixBCOQz3hWk0Htik3J/[email protected]>

drivers/media/platform/dwc/dw-hdmi-rx.c new file +4 -0

+static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
+{
+	request_module(pdevinfo.name);
+

drivers/media/platform/dwc/dw-hdmi-rx.c line 431

Quoted 10 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +10 -0

+	dw_dev->phy_pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(dw_dev->phy_pdev)) {
+		dev_err(dw_dev->dev, "failed to register phy device\n");
+		return PTR_ERR(dw_dev->phy_pdev);
+	}
+
+	if (!dw_dev->phy_pdev->dev.driver) {
+		dev_err(dw_dev->dev, "failed to initialize phy driver\n");
+		goto err;
+	}

I think this is not safe because there is nothing preventing unbinding or unloading the driver at this point.

drivers/media/platform/dwc/dw-hdmi-rx.c line 433

Quoted 1 line

drivers/media/platform/dwc/dw-hdmi-rx.c new file +1 -0

+	if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {

So dw_dev->phy_pdev->dev.driver may be already NULL here.

Quoted 3 lines

How can I make sure it wont be NULL? Because I've seen other media drivers do this and I don't think they do any kind of locking, but they do this mainly for I2C subdevs.

You could do device_lock(dev)/device_unlock(dev) to avoid possible races. And setting 'suppress_bind_attrs' field in the sub-device drivers would disable sysfs unbind attributes, so sub-device driver wouldn't get unbound unexpectedly trough sysfs.

Quoted 19 lines

Quoted 11 lines

Quoted 11 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +10 -0

+		dev_err(dw_dev->dev, "failed to get phy module\n");
+		goto err;
+	}
+
+	dw_dev->phy_sd = dev_get_drvdata(&dw_dev->phy_pdev->dev);
+	if (!dw_dev->phy_sd) {
+		dev_err(dw_dev->dev, "failed to get phy subdev\n");
+		goto err_put;
+	}
+

Quoted 8 lines

drivers/media/platform/dwc/dw-hdmi-rx.c line 447

Quoted 4 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +4 -0

+	if (v4l2_device_register_subdev(&dw_dev->v4l2_dev, dw_dev->phy_sd)) {
+		dev_err(dw_dev->dev, "failed to register phy subdev\n");
+		goto err_put;
+	}

I'd suggest usign v4l2-async API, so we use a common pattern for sub-device registration. And with recent change [1] you could handle this PHY subdev in a standard way. That might be more complicated than it is now but should make any future platform integration easier.

Quoted 2 lines

So I will instantiate phy driver and then wait for phy driver to register into v4l2 core?

Yes, for instance the RX controller driver registers a notifier, instantiates the child PHY device and then waits until the PHY driver completes initialization.

Quoted 20 lines

Quoted 20 lines

Quoted 5 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +5 -0

+	module_put(dw_dev->phy_pdev->dev.driver->owner);
+	return 0;
+
+err_put:
+	module_put(dw_dev->phy_pdev->dev.driver->owner);

drivers/media/platform/dwc/dw-hdmi-rx.c

Quoted 10 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +10 -0

+err:
+	platform_device_unregister(dw_dev->phy_pdev);
+	return -EINVAL;
+}
+static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
+{
+	struct dw_hdmi_work_data *data;
+	unsigned long flags;
+
+	data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);

Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called in the device's probe() callback, but in other places, including interrupt handler? devm_* API is normally used when life time of a resource is more or less equal to life time of struct device or its matched driver. Were there any specific reasons to not just use kzalloc()/kfree() ?

Quoted 3 lines

No specific reason, I just thought it would be safer because if I cancel a work before it started then memory will remain allocated. But I will change to kzalloc().

OK, I overlooked such situation. Since you allow one job queued maybe just embed struct work_struct in struct dw_hdmi_dev and retrieve it with container_of() macro in the work handler and use additional field in struct dw_hdmi_dev protected with dw_dev->lock for passing the input index?

Quoted 32 lines

Quoted 24 lines

Quoted 24 lines

drivers/media/platform/dwc/dw-hdmi-rx.c new file +23 -0

+	if (!data)
+		return -ENOMEM;
+
+	INIT_WORK(&data->work, dw_hdmi_work_handler);
+	data->dw_dev = dw_dev;
+	data->input = input;
+
+	spin_lock_irqsave(&dw_dev->lock, flags);
+	if (dw_dev->pending_config) {
+		devm_kfree(dw_dev->dev, data);
+		spin_unlock_irqrestore(&dw_dev->lock, flags);
+		return 0;
+	}
+
+	queue_work(dw_dev->wq, &data->work);
+	dw_dev->pending_config = true;
+	spin_unlock_irqrestore(&dw_dev->lock, flags);
+	return 0;
+}
+struct dw_hdmi_rx_pdata {
+	/* Controller configuration */
+	struct dw_hdmi_hdcp14_key hdcp14_keys;
+	/* 5V sense interface */

Quoted 8 lines

include/media/dwc/dw-hdmi-rx-pdata.h line 60

Quoted 6 lines

include/media/dwc/dw-hdmi-rx-pdata.h new file +6 -0

+	bool (*dw_5v_status)(void __iomem *regs, int input);
+	void (*dw_5v_clear)(void __iomem *regs);
+	void __iomem *dw_5v_arg;> +	/* Zcal interface */
+	void (*dw_zcal_reset)(void __iomem *regs);
+	bool (*dw_zcal_done)(void __iomem *regs);
+	void __iomem *dw_zcal_arg;

I'm just wondering if these operations could be modeled with the regmap, so we could avoid callbacks in the platform data structure.

Quoted 4 lines

Hmm, I don't think that is safe because registers may not be adjacent to each other. And maybe I was a little generous in passing a __iomem argument, maybe it should be just void instead because this can be not a regmap at all.

I meant two separate regmaps, but it's not that good anyway, since register address and register bit fields not specific to the HDMI RX block would need to be handled in this driver.

--
Regards,
Sylwester

Document the bindings for the Synopsys Designware HDMI RX.

Signed-off-by Jose Abreu <[email protected]>

new file mode 100644
index 0000000..d30cc1e

Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt new file +12 -0

@@ -0,0 +1,45 @@
+Synopsys DesignWare HDMI RX Decoder
+===================================
+
+This document defines device tree properties for the Synopsys DesignWare HDMI
+RX Decoder (DWC HDMI RX). It doesn't constitute a device tree binding
+specification by itself but is meant to be referenced by platform-specific
+device tree bindings.
+
+When referenced from platform device tree bindings the properties defined in
+this document are defined as follows.
+
+- reg: Memory mapped base address and length of the DWC HDMI RX registers.

Hi Jose,

Quoted 24 lines · Jose Abreu

Document the bindings for the Synopsys Designware HDMI RX.

Signed-off-by Jose Abreu <[email protected]>

new file mode 100644
index 0000000..d30cc1e

Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt new file +16 -0

@@ -0,0 +1,45 @@
+Synopsys DesignWare HDMI RX Decoder
+===================================
+
+This document defines device tree properties for the Synopsys DesignWare HDMI
+RX Decoder (DWC HDMI RX). It doesn't constitute a device tree binding
+specification by itself but is meant to be referenced by platform-specific
+device tree bindings.
+
+When referenced from platform device tree bindings the properties defined in
+this document are defined as follows.
+
+- reg: Memory mapped base address and length of the DWC HDMI RX registers.
+
+- interrupts: Reference to the DWC HDMI RX interrupt and 5v sense interrupt.
+
+- snps,hdmi-phy-jtag-addr: JTAG address of the phy to be used.

Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt line 22

Quoted 6 lines

Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt new file +6 -0

+
+- snps,hdmi-phy-version: Version of the phy to be used.
+
+- snps,hdmi-phy-cfg-clk: Value of the cfg clk that is delivered to phy.
+
+- snps,hdmi-phy-driver: *Exact* name of the phy driver to be used.

I don't think we can put Linux driver name in DT like this, devicetree is supposed to be describing hardware in OS agnostic way.

Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt line 25

Quoted 3 lines

Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt new file +2 -0

+- snps,hdmi-ctl-cfg-clk: Value of the cfg clk that is delivered to the
+controller.

How about creating a separate node for the PHY? The binding could then be something like:

/* Fixed clock needed only if respective clock is not already defined
in the system */

refclk: clock {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <25000000>;
};

hdmi_phy: phy@f3 {
	/* PHY version can be derived from the compatible string */
	compatible = "snps,dw-hdmi-phy-e405"; 	
	/* JTAG address of the PHY */
	reg = <0xf3>

	clocks = <&refclk>
	clock-names = "cfg-clk";
}

hdmi-controller@0 {
	compatible = "snps,dw-hdmi-rx-controller-vX.X";
	reg = <0x0 0x10000>;
	interrupts = <1 3>; 	
	phys = <&hdmi_phy>;

	clocks = <&refclk>
	clock-names = "cfg-clk";
};

Or it might be better to make the PHY node child node of the RX controller node, since the RX controller could be considered a controller of the JTAG bus IIUC:

refclk: clock {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <25000000>;
};

hdmi-controller@0 {
	compatible = "snps,dw-hdmi-rx-controller-xxx";
	reg = <0x0 0x10000>;
	interrupts = <1 3>; 	
	clocks = <&refclk>
	clock-names = "cfg-clk";
	#address-cells = <1>;
	#size-cells = <0>;

	phy@f3 {
		/* PHY version can be derived from the compatible string */
		compatible = "snps,dw-hdmi-phy-e405";
 		/* address of the PHY on the JTAG bus */
		reg = <0xf3>

		clocks = <&refclk>
		clock-names = "cfg-clk";
	}
};

Then rather than creating platform device in the RX controller driver for the PHY just of_platform_populate() could be used.

Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt line 31

Quoted 6 lines

Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt new file +5 -0

+- edid-phandle: phandle to the EDID driver. It can be, for example, the main
+wrapper driver.
+
+A sample binding is now provided. The compatible string is for a wrapper driver
+which then instantiates the Synopsys Designware HDMI RX decoder driver.

I would avoid talking about drivers in DT binding documentation, we should rather refer to hardware blocks.

Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt line 45

Quoted 13 lines

Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt new file +13 -0

+Example:
+
+dw_hdmi_wrapper: dw-hdmi-wrapper@0 {
+	compatible = "snps,dw-hdmi-rx-wrapper";
+	reg = <0x0 0x10000>; /* controller regbank */
+	interrupts = <1 3>; /* controller interrupt + 5v sense interrupt */
+	snps,hdmi-phy-driver = "dw-hdmi-phy-e405";
+	snps,hdmi-phy-version = <405>;
+	snps,hdmi-phy-cfg-clk = <25>; /* MHz */
+	snps,hdmi-ctl-cfg-clk = <25>; /* MHz */
+	snps,hdmi-phy-jtag-addr = /bits/ 8 <0xfc>;
+	edid-phandle = <&dw_hdmi_wrapper>;
+};

--
Regards,
Sylwester

Using fixed-clock was already in my todo list. Regarding phy I need to pass pdata so that the callbacks between controller and phy are established. I also need to make sure that phy driver will be loaded by the controller driver. Hmm, and also address of the phy on th JTAG bus is fed to the controller driver not to the phy driver. Maybe leave the property as is (the "snps,hdmi-phy-jtag-addr") or parse it from the phy node?

I also need to pass pdata to the controller driver (the callbacks for 5v handling) which are agnostic of the controller. These

reasons prevented me from adding compatible strings to both drivers and just use a wrapper driver instead. This way i do

"modprobe wrapper_driver" and I get all the drivers loaded via request_module(). Still, I like your approach much better. I saw that I can pass pdata using of_platform_populate, could you please confirm if I can still maintain this architecture (i.e. prevent modules from loading until I get all the chain setup)?

Following your approach I could get something like this:

hdmi_system@YYYY {
     compatible = "snps,dw-hdmi-rx-wrapper";

     reg = <0xYYYY 0xZZZZ>;
     interrupts = <3>;
     #address-cells = <1>;
     #size-cells = <1>;

     hdmi_controller@0 {
         compatible = "snps,dw-hdmi-rx-controller";
         reg = <0x0 0x10000>;
         interrupts = <1>;
         edid-phandle = <&hdmi_system>;
         clocks = <&refclk>;
         clock-names = "ref-clk";
         #address-cells = <1>;
         #size-cells = <0>;

         hdmi_phy@f3 {
             compatible = "snps,dw-hdmi-phy-e405";
             reg = <0xf3>;
             clocks = <&cfgclk>;
             clock-names = "cfg-clk";
         }
     }
};

And then snps,dw-hdmi-rx-wrapper would call of_platform_populate for controller which would instead call of_platform_populate for phy. Is this possible, and maintainable? Isn't the controller driver get auto loaded because of the compatible string match?

And one more thing: The reg address of the hdmi_controller: Isn't this relative to the parent node? I mean isn't this going to be 0xYYYY + 0x0? Because I don't want that :/

Quoted 7 lines · Jose Abreu

Using fixed-clock was already in my todo list. Regarding phy I need to pass pdata so that the callbacks between controller and phy are established. I also need to make sure that phy driver will be loaded by the controller driver. Hmm, and also address of the phy on th JTAG bus is fed to the controller driver not to the phy driver. Maybe leave the property as is (the "snps,hdmi-phy-jtag-addr") or parse it from the phy node?

I think the RX controller can parse it's child phy node to retrieve JTAG address from the reg property. That seems better than creating custom property for device address on the bus.

Does the PHY support any other type of control bus, e.g. I2C or SPI?

Quoted 3 lines

I also need to pass pdata to the controller driver (the callbacks for 5v handling) which are agnostic of the controller. These

Is this about detecting +5V coming from the HDMI connector? Or some other voltage supply?

Quoted 3 lines

reasons prevented me from adding compatible strings to both drivers and just use a wrapper driver instead. This way i do

If you add struct of_device_id instance to your module and define MODULE_DEVICE_ALIAS(of, ...) there, it will allow the module to be loaded when device with matching compatible string is created in the kernel. User space is notified with uevent about device creation.

Quoted 6 lines

"modprobe wrapper_driver" and I get all the drivers loaded via request_module(). Still, I like your approach much better. I saw that I can pass pdata using of_platform_populate, could you please confirm if I can still maintain this architecture (i.e. prevent modules from loading until I get all the chain setup)?

You could try to pass platform data this way, that should work. But I doubt it's the right directions, I would rather see things done in the V4L2 layer.

Quoted 5 lines

Following your approach I could get something like this:

hdmi_system@YYYY {
     compatible = "snps,dw-hdmi-rx-wrapper";

This would need to refer to some hardware block, we should avoid virtual device nodes in DT.

Quoted 5 lines

     reg = <0xYYYY 0xZZZZ>;
     interrupts = <3>;
     #address-cells = <1>;
     #size-cells = <1>;

You would need also an (empty) 'ranges' property here.

Quoted 20 lines

     hdmi_controller@0 {
         compatible = "snps,dw-hdmi-rx-controller";
         reg = <0x0 0x10000>;
         interrupts = <1>;
         edid-phandle = <&hdmi_system>;
         clocks = <&refclk>;
         clock-names = "ref-clk";
         #address-cells = <1>;
         #size-cells = <0>;

         hdmi_phy@f3 {
             compatible = "snps,dw-hdmi-phy-e405";
             reg = <0xf3>;
             clocks = <&cfgclk>;
             clock-names = "cfg-clk";
         }
     }
};

Quoted 4 lines

And then snps,dw-hdmi-rx-wrapper would call of_platform_populate for controller which would instead call of_platform_populate for phy. Is this possible, and maintainable? Isn't the controller driver get auto loaded because of the compatible string match?

As I mentioned above, auto loading should work if you have instance of MODULE_DEVICE_TABLE() defined in the module, but the module might not be available immediately after creating devices with of_platform_populate(). You may want to have a look at the v4l2-async API (drivers/media/v4l2-core/v4l2-async.c). It allows one driver to register a notifier for its sub-devices. And the parent driver can complete initialization when it gets all its v4l2 subdevs registered.

But I'm not sure about calls from the PHY back to the RX controller, possibly v4l2_set_subdev_hostdata()/v4l2_get_subdev_hostdata() could be used for passing the ops.

Quoted 4 lines

And one more thing: The reg address of the hdmi_controller: Isn't this relative to the parent node? I mean isn't this going to be 0xYYYY + 0x0? Because I don't want that :/

Address space of child nodes doesn't need to be relative to 'reg' range of parent node, these can be entirely distinct address ranges. See for example I2C bus controllers, the I2C addresses of slave devices are not much related to the memory mapped IO registers region of the bus controller.

--
Regards,
Sylwester