On 04/06/2024 14:32, Paweł Anikiel wrote:
> On Mon, Jun 3, 2024 at 10:37 AM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>>
>> On 07/05/2024 17:54, Paweł Anikiel wrote:
>>> Add v4l2 subdev driver for the Intel Displayport receiver FPGA IP.
>>> It is a part of the DisplayPort Intel FPGA IP Core, and supports
>>> DisplayPort 1.4, HBR3 video capture and Multi-Stream Transport.
>>>
>>> Signed-off-by: Paweł Anikiel <panikiel@xxxxxxxxxx>
>>> ---
>>> drivers/media/platform/intel/Kconfig | 12 +
>>> drivers/media/platform/intel/Makefile | 1 +
>>> drivers/media/platform/intel/intel-dprx.c | 2283 +++++++++++++++++++++
>>> 3 files changed, 2296 insertions(+)
>>> create mode 100644 drivers/media/platform/intel/intel-dprx.c
>>>
<snip>
>>> +static int dprx_probe(struct platform_device *pdev)
>>> +{
>>> + struct dprx *dprx;
>>> + int irq;
>>> + int res;
>>> + int i;
>>> +
>>> + dprx = devm_kzalloc(&pdev->dev, sizeof(*dprx), GFP_KERNEL);
>>> + if (!dprx)
>>> + return -ENOMEM;
>>> + dprx->dev = &pdev->dev;
>>> + platform_set_drvdata(pdev, dprx);
>>> +
>>> + dprx->iobase = devm_platform_ioremap_resource(pdev, 0);
>>> + if (IS_ERR(dprx->iobase))
>>> + return PTR_ERR(dprx->iobase);
>>> +
>>> + irq = platform_get_irq(pdev, 0);
>>> + if (irq < 0)
>>> + return irq;
>>> +
>>> + res = devm_request_irq(dprx->dev, irq, dprx_isr, 0, "intel-dprx", dprx);
>>> + if (res)
>>> + return res;
>>> +
>>> + res = dprx_parse_fwnode(dprx);
>>> + if (res)
>>> + return res;
>>> +
>>> + dprx_init_caps(dprx);
>>> +
>>> + dprx->subdev.owner = THIS_MODULE;
>>> + dprx->subdev.dev = &pdev->dev;
>>> + v4l2_subdev_init(&dprx->subdev, &dprx_subdev_ops);
>>> + v4l2_set_subdevdata(&dprx->subdev, &pdev->dev);
>>> + snprintf(dprx->subdev.name, sizeof(dprx->subdev.name), "%s %s",
>>> + KBUILD_MODNAME, dev_name(&pdev->dev));
>>> + dprx->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
>>> +
>>> + dprx->subdev.entity.function = MEDIA_ENT_F_DV_DECODER;
>>> + dprx->subdev.entity.ops = &dprx_entity_ops;
>>> +
>>> + v4l2_ctrl_handler_init(&dprx->ctrl_handler, 1);
>>> + v4l2_ctrl_new_std(&dprx->ctrl_handler, NULL,
>>> + V4L2_CID_DV_RX_POWER_PRESENT, 0, 1, 0, 0);
>>
>> You are creating this control, but it is never set to 1 when the driver detects
>> that a source is connected. I am wondering if POWER_PRESENT makes sense for a
>> DisplayPort connector. Is there a clean way for a sink driver to detect if a
>> source is connected? For HDMI it detects the 5V pin, but it is not clear if
>> there is an equivalent to that in the DP spec.
>
> The DP spec says the source can be detected using the AUX lines:
>
> "The Downstream devices must very weakly pull up AUX+ line and very
> weakly pull down AUX- line with 1MΩ (+/-5%) resistors between the
> Downstream device Connector and the AC-coupling capacitors. When AUX+
> line DC voltage is L level, it means a DisplayPort Upstream device is
> connected. When AUX- line DC voltage is H level, it means that a
> powered DisplayPort Upstream device is connected."
>
> This exact IP has two input signals: rx_cable_detect, and
> rx_pwr_detect, which are meant to be connected to the AUX+/AUX- lines
> via 10k resistors (or rather that's what the reference design does).
> They're exposed to software via status registers, but there's no way
> to get interrupts from them, so it wouldn't be possible to set the
> control exactly when a source gets plugged in.
>
>>
>> If there is no good way to detect if a source is connected, then it might be
>> better to drop POWER_PRESENT support.
>>
>> This control is supposed to signal that a source is connected as early as possible,
>> ideally before link training etc. starts.
>>
>> It helps the software detect that there is a source, and report an error if a source
>> is detected, but you never get a stable signal (e.g. link training fails).
>
> This poses another problem, because the chameleon board doesn't have
> this detection circuitry, and instead sets the rx_cable_detect and
> rx_pwr_detect signals to always logical high. That would make the
> control read "always plugged in", which IIUC is not desired.
OK, so it is best to drop support for this control.
I recommend adding a comment in the source code explaining why it is not supported.
And in the cover letter you can mention this as well as an explanation of why
there is a v4l2-compliance warning.
Regards,
Hans
