From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: "Paweł Anikiel" <panikiel@google.com>
Cc: airlied@gmail.com, akpm@linux-foundation.org,
conor+dt@kernel.org, daniel@ffwll.ch, dinguyen@kernel.org,
krzysztof.kozlowski+dt@linaro.org,
maarten.lankhorst@linux.intel.com, mchehab@kernel.org,
mripard@kernel.org, robh+dt@kernel.org, tzimmermann@suse.de,
devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org,
linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
chromeos-krk-upstreaming@google.com
Subject: Re: [PATCH v3 07/10] media: intel: Add Displayport RX IP driver
Date: Fri, 7 Jun 2024 14:04:23 +0200 [thread overview]
Message-ID: <d76a40de-7e42-4870-86c7-f168666b3e59@xs4all.nl> (raw)
In-Reply-To: <CAM5zL5qNJfQCYAm9iUh5UgKouO_R9NxJpV-04EJz9wsV0n9deQ@mail.gmail.com>
On 04/06/2024 14:32, Paweł Anikiel wrote:
> On Mon, Jun 3, 2024 at 10:37 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@google.com>
>>> ---
>>> 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
next prev parent reply other threads:[~2024-06-07 12:04 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-07 15:54 [PATCH v3 00/10] Add Chameleon v3 video support Paweł Anikiel
2024-05-07 15:54 ` [PATCH v3 01/10] media: Add Chameleon v3 video interface driver Paweł Anikiel
2024-06-03 7:57 ` Hans Verkuil
2024-06-03 14:32 ` Paweł Anikiel
2024-06-03 14:56 ` Hans Verkuil
2024-06-04 12:03 ` Paweł Anikiel
2024-06-04 12:46 ` Hans Verkuil
2024-05-07 15:54 ` [PATCH v3 02/10] drm/dp_mst: Move DRM-independent structures to separate header Paweł Anikiel
2024-05-07 15:54 ` [PATCH v3 03/10] lib: Move DisplayPort CRC functions to common lib Paweł Anikiel
2024-05-07 15:54 ` [PATCH v3 04/10] drm/display: Add mask definitions for DP_PAYLOAD_ALLOCATE_* registers Paweł Anikiel
2024-05-07 15:54 ` [PATCH v3 05/10] media: dt-bindings: video-interfaces: Support DisplayPort MST Paweł Anikiel
2024-05-10 21:16 ` Rob Herring
2024-05-13 11:07 ` Paweł Anikiel
2024-05-13 14:56 ` Rob Herring (Arm)
2024-05-07 15:54 ` [PATCH v3 06/10] media: v4l2-mediabus: Add support for DisplayPort media bus Paweł Anikiel
2024-05-07 15:54 ` [PATCH v3 07/10] media: intel: Add Displayport RX IP driver Paweł Anikiel
2024-06-03 8:37 ` Hans Verkuil
2024-06-04 12:32 ` Paweł Anikiel
2024-06-07 12:04 ` Hans Verkuil [this message]
2024-05-07 15:54 ` [PATCH v3 08/10] media: dt-bindings: Add Chameleon v3 video interface Paweł Anikiel
2024-05-10 21:25 ` Rob Herring (Arm)
2024-05-07 15:54 ` [PATCH v3 09/10] media: dt-bindings: Add Intel Displayport RX IP Paweł Anikiel
2024-05-10 21:24 ` Rob Herring
2024-05-13 10:39 ` Paweł Anikiel
2024-05-07 15:54 ` [PATCH v3 10/10] ARM: dts: chameleonv3: Add video device nodes Paweł Anikiel
2024-06-03 8:44 ` [PATCH v3 00/10] Add Chameleon v3 video support Hans Verkuil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d76a40de-7e42-4870-86c7-f168666b3e59@xs4all.nl \
--to=hverkuil-cisco@xs4all.nl \
--cc=airlied@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=chromeos-krk-upstreaming@google.com \
--cc=conor+dt@kernel.org \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dinguyen@kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mchehab@kernel.org \
--cc=mripard@kernel.org \
--cc=panikiel@google.com \
--cc=robh+dt@kernel.org \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).