Message-ID: <fd65183f-b577-9ac6-a56e-689121e82e73@kernel.org>
Date:   Sun, 18 Jun 2017 20:04:28 +0200
From:   Sylwester Nawrocki <snawrocki@...nel.org>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org,
        Carlos Palminha <CARLOS.PALMINHA@...opsys.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hans.verkuil@...co.com>
Subject: Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX
 Controller Driver

On 06/16/2017 06:38 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.
> 
> The controller + phy pipeline can then be integrated into a fully
> featured system that can be able to receive video up to 4k@...z
> 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@...opsys.com>

> +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);
> +
> +	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.

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

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

> +		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)) {
> +		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://patchwork.linuxtv.org/patch/41834

> +	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)
> +{

> +	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.


> +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;
> +	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() ?

> +	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);
> +	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.


> +	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.

> +		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;
> +	}
> +
> +#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.

> +	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.

> +	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.

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


> +	/* 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));

> +	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);
> +	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.

> +	.driver = {
> +		.name = DW_HDMI_RX_DRVNAME,
> +	}
> +};
> +module_platform_driver(dw_hdmi_rx_driver);

> +#endif /* __DW_HDMI_RX_H__ */
> diff --git a/include/media/dwc/dw-hdmi-rx-pdata.h b/include/media/dwc/dw-hdmi-rx-pdata.h
> new file mode 100644
> index 0000000..ff8554d
> --- /dev/null
> +++ b/include/media/dwc/dw-hdmi-rx-pdata.h
> @@ -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];
> +	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?

> +	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.
 
> +};

> +#endif /* __DW_HDMI_RX_PDATA_H__ */

--
Regards,
Sylwester

Powered by blists - more mailing lists