Hi,
On Mon, Sep 8, 2025 at 1:37 PM John Ripple <john.rip...@keysight.com> wrote:
>
> Add support for DisplayPort to the bridge, which entails the following:
> - Get and use an interrupt for HPD;
> - Properly clear all status bits in the interrupt handler;
>
> Signed-off-by: John Ripple <john.rip...@keysight.com>
> ---
> V1 -> V2: Cleaned up coding style and addressed review comments
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 93 +++++++++++++++++++++++++++
> 1 file changed, 93 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ae0d08e5e960..ad0393212279 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -106,10 +106,24 @@
> #define SN_PWM_EN_INV_REG 0xA5
> #define SN_PWM_INV_MASK BIT(0)
> #define SN_PWM_EN_MASK BIT(1)
> +
> +#define SN_IRQ_EN_REG 0xE0
> +#define IRQ_EN BIT(0)
> +
> +#define SN_IRQ_EVENTS_EN_REG 0xE6
> +#define IRQ_HPD_EN BIT(0)
> +#define HPD_INSERTION_EN BIT(1)
> +#define HPD_REMOVAL_EN BIT(2)
> +#define HPD_REPLUG_EN BIT(3)
> +#define PLL_UNLOCK_EN BIT(5)
> +
> #define SN_AUX_CMD_STATUS_REG 0xF4
> #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
> #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
> #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
> +#define SN_IRQ_STATUS_REG 0xF5
> +#define HPD_REMOVAL_STATUS BIT(2)
> +#define HPD_INSERTION_STATUS BIT(1)
>
> #define MIN_DSI_CLK_FREQ_MHZ 40
>
> @@ -221,6 +235,19 @@ static const struct regmap_config
> ti_sn65dsi86_regmap_config = {
> .max_register = 0xFF,
> };
>
> +static int ti_sn65dsi86_read(struct ti_sn65dsi86 *pdata, unsigned int reg,
> + unsigned int *val)
This is reading a byte, right? So "val" should be an "u8 *". Yeah,
that means you need a local variable to adjust for the generic regmap
call, but it makes a cleaner and more obvious API to the users in this
file.
> +{
> + int ret;
> +
> + ret = regmap_read(pdata->regmap, reg, val);
> + if (ret)
> + dev_err(pdata->dev, "fail to read raw reg %x: %d\n",
> + reg, ret);
nit: use %#x so that the 0x is included.
> @@ -1219,12 +1246,28 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge
> *bridge)
> */
>
> pm_runtime_get_sync(pdata->dev);
> +
> + /* Enable HPD and PLL events. */
> + regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
> + PLL_UNLOCK_EN |
> + HPD_REPLUG_EN |
> + HPD_REMOVAL_EN |
> + HPD_INSERTION_EN |
> + IRQ_HPD_EN);
* Shouldn't this be `regmap_update_bits()` to just update the bits
related to HPD?
* why enable "PLL_UNLOCK_EN" when you don't handle it?
* I also don't think your IRQ handler handles "replug" and "irq_hpd",
right? So you shouldn't enable those either?
Also: should the above only be if the IRQ is enabled? AKA obtain a
pointer to the i2c_client and check `client->irq`?
> +
> + regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
> + IRQ_EN);
I guess this is OK to be here if you want, but I would maybe consider
putting it in `ti_sn65dsi86_resume()` if `client->irq` is set. Then if
we ever enable more interrupts it'll already be in the correct place.
> @@ -1309,6 +1352,32 @@ 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;
I'm unsure if accessing "dev" here without any sort of locking is
safe... It feels like, in theory, "detach" could be called and race
with the IRQ handler? Maybe you need a spinlock to be sure?
> + u32 status = 0;
> + bool hpd_event = false;
> + int ret = 0;
Don't initialize variables unless they're needed. In this case it's
obvious that both `hpd_event` and `ret` don't need to be initialized.
Maybe you could argue that `status` should be initialized since it's
passed by reference, but even that's iffy...
> +
> + ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
> + if (ret)
> + return ret;
The return for this function is not an error code but an
`irqreturn_t`. You need to account for that.
> + hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> + if (status) {
> + drm_dbg(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
> + ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
> + if (ret)
> + return ret;
Same--you can't just return an error code.
> + }
> +
> + /* Only send the HPD event if we are bound with a device. */
> + if (dev && hpd_event)
> + drm_kms_helper_hotplug_event(dev);
> +
> + return IRQ_HANDLED;
If "status" gives back 0 (which would be weird), you probably want to
return IRQ_NONE, right?
> +}
> +
> static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> const struct auxiliary_device_id *id)
> {
> @@ -1971,6 +2040,30 @@ 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) {
> + enum drm_connector_status status;
> +
> + ret = devm_request_threaded_irq(pdata->dev, client->irq, NULL,
> + ti_sn_bridge_interrupt,
> + IRQF_TRIGGER_RISING |
> + IRQF_TRIGGER_FALLING |
> + IRQF_ONESHOT,
> + "ti_sn65dsi86", pdata);
> + if (ret) {
> + return dev_err_probe(dev, ret,
> + "failed to request interrupt\n");
> + }
> +
> + /*
> + * Cleaning status register at probe is needed because if the
> irq is
> + * already high, the rising/falling condition will never
> occurs
nit: s/occurs/occur
> + */
> + ret = ti_sn65dsi86_read(pdata, SN_IRQ_STATUS_REG, &status);
> + ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
> + if (ret)
> + pr_warn("Failed to clear IRQ initial state: %d\n",
> ret);
Do you even need to read? Can't you just write all possible bits and
that should be safe?
