linux-kernel - Re: [PATCH V5] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAD=FV=WVuCNMcPKZjaefAvLXi4Lxxw01HQQc+mEBX1nk8ot-WQ@mail.gmail.com>
Date: Fri, 12 Sep 2025 14:27:00 -0700
From: Doug Anderson <dianders@...omium.org>
To: John Ripple <john.ripple@...sight.com>
Cc: Laurent.pinchart@...asonboard.com, airlied@...il.com, 
	andrzej.hajda@...el.com, blake.vermeer@...sight.com, 
	dri-devel@...ts.freedesktop.org, jernej.skrabec@...il.com, jonas@...boo.se, 
	linux-kernel@...r.kernel.org, maarten.lankhorst@...ux.intel.com, 
	matt_laubhan@...sight.com, mripard@...nel.org, neil.armstrong@...aro.org, 
	rfoss@...nel.org, simona@...ll.ch, tzimmermann@...e.de
Subject: Re: [PATCH V5] drm/bridge: ti-sn65dsi86: Add support for DisplayPort
 mode with HPD

Hi,

On Fri, Sep 12, 2025 at 2:08 PM John Ripple <john.ripple@...sight.com> wrote:
>
> @@ -413,6 +446,13 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
>         if (pdata->refclk)
>                 ti_sn65dsi86_enable_comms(pdata, NULL);
>
> +       if (client->irq) {
> +               ret = regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
> +                                        IRQ_EN);
> +               if (ret)
> +                       pr_err("Failed to enable IRQ events: %d\n", ret);

Shoot, I should have noticed it before. Sorry! :(

Probably most of the "pr_" calls in your patch should be "dev_" calls.
"struct ti_sn65dsi86" should have a dev pointer in it. That's probably
worth spinning the patch. It's really close now, though!


> @@ -1309,6 +1372,41 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>         return 0;
>  }
>
> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> +{
> +       struct ti_sn65dsi86 *pdata = private;
> +       struct drm_device *dev = pdata->bridge.dev;
> +       u8 status;
> +       int ret;
> +       bool hpd_event;
> +
> +       ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
> +       if (ret) {
> +               pr_err("Failed to read IRQ status: %d\n", ret);
> +               return IRQ_NONE;
> +       }
> +
> +       hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> +
> +       if (!status)
> +               return IRQ_NONE;

It wouldn't have been worth spinning just for this, but if we're
spinning anyway I'd probably put the "if (!status)" check down below
right before you grab the mutex, just to keep all the HPD processing
together.


> @@ -1931,6 +2029,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>         dev_set_drvdata(dev, pdata);
>         pdata->dev = dev;
>
> +       mutex_init(&pdata->hpd_mutex);
> +
>         mutex_init(&pdata->comms_mutex);

Again, it wouldn't be worth spinning on its own, but if you happened
to want to get rid of the blank line between the two I wouldn't
object. ;-)


> @@ -1971,6 +2071,18 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>         if (strncmp(id_buf, "68ISD   ", ARRAY_SIZE(id_buf)))
>                 return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n");
>
> +       if (client->irq) {
> +               ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
> +                                               ti_sn_bridge_interrupt,
> +                                               IRQF_ONESHOT,
> +                                               dev_name(pdata->dev), pdata);
> +
> +               if (ret) {
> +                       return dev_err_probe(dev, ret,
> +                                            "failed to request interrupt\n");
> +               }

Another tiny nitpick if you happen to want to fix up when you're
spinning. Officially the above "if" statement probably shouldn't have
braces. I think checkpatch won't yell since it's kinda two lines, but
it's really just one statement... You could even make it one line
since the 80-column rule has been relaxed a bit in the last few
years...


Sorry, I should have noticed the "pr_" stuff on the last review. Sorry
for making you spin again. Hopefully the last one? I think the patch
looks a lot nicer now FWIW! :-)

-Doug