| Message ID | 20190326103146.24795-22-tomi.valkeinen@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/bridge: tc358767: DP support | expand |
On Tue, Mar 26, 2019 at 3:33 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > Add support for interrupt and hotplug handling. Both are optional. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/bridge/tc358767.c | 157 ++++++++++++++++++++++++++---- > 1 file changed, 139 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 8606de29c9b2..6978129428a8 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -71,6 +71,7 @@ > > /* System */ > #define TC_IDREG 0x0500 > +#define SYSSTAT 0x0508 > #define SYSCTRL 0x0510 > #define DP0_AUDSRC_NO_INPUT (0 << 3) > #define DP0_AUDSRC_I2S_RX (1 << 3) > @@ -79,9 +80,16 @@ > #define DP0_VIDSRC_DPI_RX (2 << 0) > #define DP0_VIDSRC_COLOR_BAR (3 << 0) > #define GPIOM 0x0540 > +#define GPIOC 0x0544 > +#define GPIOO 0x0548 > #define GPIOI 0x054c > #define INTCTL_G 0x0560 > #define INTSTS_G 0x0564 > + > +#define INT_SYSERR BIT(16) > +#define INT_GPIO_H(x) (1 << (x == 0 ? 2 : 10)) > +#define INT_GPIO_LC(x) (1 << (x == 0 ? 3 : 11)) > + > #define INT_GP0_LCNT 0x0584 > #define INT_GP1_LCNT 0x0588 > > @@ -219,6 +227,12 @@ struct tc_data { > struct gpio_desc *sd_gpio; > struct gpio_desc *reset_gpio; > struct clk *refclk; > + > + /* do we have IRQ */ > + bool have_irq; > + > + /* HPD pin number (0 or 1) or -ENODEV */ > + int hpd_num; > }; > > static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a) > @@ -1095,6 +1109,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge) > struct tc_data *tc = bridge_to_tc(bridge); > int ret; > > + ret = tc_get_display_props(tc); > + if (ret < 0) { > + dev_err(tc->dev, "failed to read display props: %d\n", ret); > + return; > + } > + > ret = tc_main_link_enable(tc); > if (ret < 0) { > dev_err(tc->dev, "main link enable error: %d\n", ret); > @@ -1200,19 +1220,42 @@ static int tc_connector_get_modes(struct drm_connector *connector) > return count; > } > > -static void tc_connector_set_polling(struct tc_data *tc, > - struct drm_connector *connector) > -{ > - /* TODO: add support for HPD */ > - connector->polled = DRM_CONNECTOR_POLL_CONNECT | > - DRM_CONNECTOR_POLL_DISCONNECT; > -} > - > static const struct drm_connector_helper_funcs tc_connector_helper_funcs = { > .get_modes = tc_connector_get_modes, > }; > > +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force) > +{ > + struct tc_data *tc = connector_to_tc(connector); > + bool conn; > + u32 val; > + int ret; > + > + if (tc->hpd_num < 0) { > + if (tc->panel) > + return connector_status_connected; > + else > + return connector_status_unknown; > + } > + The early return above causes tc_get_display_props() to never be called for eDP case, which in turn result in tc_mode_valid() returning MODE_BAD for every mode it is given since it depends on tc->link.base being initialized properly. I had to change this code to: if (tc->hpd_num < 0) { if (!tc->panel) return connector_status_unknown; conn = true; } else { tc_read(GPIOI, &val); conn = val & BIT(tc->hpd_num); } to fix the problem. Thanks, Andrey Smirnov
On 02/04/2019 05:16, Andrey Smirnov wrote: > The early return above causes tc_get_display_props() to never be > called for eDP case, which in turn result in tc_mode_valid() returning > MODE_BAD for every mode it is given since it depends on tc->link.base > being initialized properly. I had to change this code to: > > if (tc->hpd_num < 0) { > if (!tc->panel) > return connector_status_unknown; > > conn = true; > } else { > tc_read(GPIOI, &val); > > conn = val & BIT(tc->hpd_num); > } > > to fix the problem. Ah, right. There's tc_get_display_props() in tc_bridge_enable() but that's of course too late (and maybe even not needed at all). What you suggest here looks fine to me, so I'll change my patch accordingly. Thanks! Tomi
Hi Andrey, On 03/04/2019 14:34, Tomi Valkeinen wrote: > On 02/04/2019 05:16, Andrey Smirnov wrote: > >> The early return above causes tc_get_display_props() to never be >> called for eDP case, which in turn result in tc_mode_valid() returning >> MODE_BAD for every mode it is given since it depends on tc->link.base >> being initialized properly. I had to change this code to: >> >> if (tc->hpd_num < 0) { >> if (!tc->panel) >> return connector_status_unknown; >> >> conn = true; >> } else { >> tc_read(GPIOI, &val); >> >> conn = val & BIT(tc->hpd_num); >> } >> >> to fix the problem. > > Ah, right. There's tc_get_display_props() in tc_bridge_enable() but > that's of course too late (and maybe even not needed at all). > > What you suggest here looks fine to me, so I'll change my patch > accordingly. Thanks! With the change above, does the series work with your setup? Can I add your tested-by? I'll send v3 series soon with the DT change suggested by Rob and the above fix. Tomi
On Fri, Apr 12, 2019 at 1:02 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > Hi Andrey, > > On 03/04/2019 14:34, Tomi Valkeinen wrote: > > On 02/04/2019 05:16, Andrey Smirnov wrote: > > > >> The early return above causes tc_get_display_props() to never be > >> called for eDP case, which in turn result in tc_mode_valid() returning > >> MODE_BAD for every mode it is given since it depends on tc->link.base > >> being initialized properly. I had to change this code to: > >> > >> if (tc->hpd_num < 0) { > >> if (!tc->panel) > >> return connector_status_unknown; > >> > >> conn = true; > >> } else { > >> tc_read(GPIOI, &val); > >> > >> conn = val & BIT(tc->hpd_num); > >> } > >> > >> to fix the problem. > > > > Ah, right. There's tc_get_display_props() in tc_bridge_enable() but > > that's of course too late (and maybe even not needed at all). > > > > What you suggest here looks fine to me, so I'll change my patch > > accordingly. Thanks! > > With the change above, does the series work with your setup? Can I add > your tested-by? I'll send v3 series soon with the DT change suggested by > Rob and the above fix. > Yeah, other than that, the series seemed to work as expected on my setup. You can add Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com> to v3. Thanks, Andrey Smirnov
On 26.03.2019 11:31, Tomi Valkeinen wrote: > Add support for interrupt and hotplug handling. Both are optional. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/bridge/tc358767.c | 157 ++++++++++++++++++++++++++---- > 1 file changed, 139 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 8606de29c9b2..6978129428a8 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -71,6 +71,7 @@ > > /* System */ > #define TC_IDREG 0x0500 > +#define SYSSTAT 0x0508 > #define SYSCTRL 0x0510 > #define DP0_AUDSRC_NO_INPUT (0 << 3) > #define DP0_AUDSRC_I2S_RX (1 << 3) > @@ -79,9 +80,16 @@ > #define DP0_VIDSRC_DPI_RX (2 << 0) > #define DP0_VIDSRC_COLOR_BAR (3 << 0) > #define GPIOM 0x0540 > +#define GPIOC 0x0544 > +#define GPIOO 0x0548 > #define GPIOI 0x054c > #define INTCTL_G 0x0560 > #define INTSTS_G 0x0564 > + > +#define INT_SYSERR BIT(16) > +#define INT_GPIO_H(x) (1 << (x == 0 ? 2 : 10)) > +#define INT_GPIO_LC(x) (1 << (x == 0 ? 3 : 11)) > + > #define INT_GP0_LCNT 0x0584 > #define INT_GP1_LCNT 0x0588 > > @@ -219,6 +227,12 @@ struct tc_data { > struct gpio_desc *sd_gpio; > struct gpio_desc *reset_gpio; > struct clk *refclk; > + > + /* do we have IRQ */ > + bool have_irq; > + > + /* HPD pin number (0 or 1) or -ENODEV */ > + int hpd_num; > }; > > static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a) > @@ -1095,6 +1109,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge) > struct tc_data *tc = bridge_to_tc(bridge); > int ret; > > + ret = tc_get_display_props(tc); > + if (ret < 0) { > + dev_err(tc->dev, "failed to read display props: %d\n", ret); > + return; > + } > + > ret = tc_main_link_enable(tc); > if (ret < 0) { > dev_err(tc->dev, "main link enable error: %d\n", ret); > @@ -1200,19 +1220,42 @@ static int tc_connector_get_modes(struct drm_connector *connector) > return count; > } > > -static void tc_connector_set_polling(struct tc_data *tc, > - struct drm_connector *connector) > -{ > - /* TODO: add support for HPD */ > - connector->polled = DRM_CONNECTOR_POLL_CONNECT | > - DRM_CONNECTOR_POLL_DISCONNECT; > -} > - > static const struct drm_connector_helper_funcs tc_connector_helper_funcs = { > .get_modes = tc_connector_get_modes, > }; > > +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force) > +{ > + struct tc_data *tc = connector_to_tc(connector); > + bool conn; > + u32 val; > + int ret; > + > + if (tc->hpd_num < 0) { > + if (tc->panel) > + return connector_status_connected; > + else > + return connector_status_unknown; Ok we have here 4 combinations: 1. noHPD + eDP. 2. noHPD + DP. 3. HPD + eDP. 4. HPD + DP. Which ones do you want to support, disallow. It is not clear to me. Moreover what connector_status_unknown should mean here for users? > + } > + > + tc_read(GPIOI, &val); > + > + conn = val & BIT(tc->hpd_num); > + > + if (force && conn) > + tc_get_display_props(tc); > + > + if (conn) > + return connector_status_connected; > + else > + return connector_status_disconnected; > + > +err: > + return connector_status_unknown; > +} > + > static const struct drm_connector_funcs tc_connector_funcs = { > + .detect = tc_connector_detect, > .fill_modes = drm_helper_probe_single_connector_modes, > .destroy = drm_connector_cleanup, > .reset = drm_atomic_helper_connector_reset, > @@ -1227,7 +1270,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge) > struct drm_device *drm = bridge->dev; > int ret; > > - /* Create eDP connector */ > + /* Create DP/eDP connector */ > drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs); > ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs, > tc->panel ? DRM_MODE_CONNECTOR_eDP : > @@ -1235,6 +1278,15 @@ static int tc_bridge_attach(struct drm_bridge *bridge) > if (ret) > return ret; > > + /* Don't poll if don't have HPD connected */ > + if (tc->hpd_num >= 0) { > + if (tc->have_irq) > + tc->connector.polled = DRM_CONNECTOR_POLL_HPD; > + else > + tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT | > + DRM_CONNECTOR_POLL_DISCONNECT; > + } > + > if (tc->panel) > drm_panel_attach(tc->panel, &tc->connector); > > @@ -1301,6 +1353,43 @@ static const struct regmap_config tc_regmap_config = { > .val_format_endian = REGMAP_ENDIAN_LITTLE, > }; > > +static irqreturn_t tc_irq_handler(int irq, void *arg) > +{ > + struct tc_data *tc = arg; > + u32 val; > + int r; > + > + r = regmap_read(tc->regmap, INTSTS_G, &val); > + if (r) > + return IRQ_NONE; > + > + if (!val) > + return IRQ_NONE; > + > + if (val & INT_SYSERR) { > + u32 stat = 0; > + > + regmap_read(tc->regmap, SYSSTAT, &stat); > + > + dev_err(tc->dev, "syserr %x\n", stat); > + } > + > + if (tc->hpd_num >= 0 && tc->bridge.dev) { > + bool h = val & INT_GPIO_H(tc->hpd_num); > + bool lc = val & INT_GPIO_LC(tc->hpd_num); > + > + dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num, > + h ? "H" : "", lc ? "LC" : ""); What does "h" and "lc" mean? Regards Andrzej > + > + if (h || lc) > + drm_kms_helper_hotplug_event(tc->bridge.dev); > + } > + > + regmap_write(tc->regmap, INTSTS_G, val); > + > + return IRQ_HANDLED; > +} > + > static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > { > struct device *dev = &client->dev; > @@ -1352,6 +1441,31 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > return ret; > } > > + ret = of_property_read_u32(dev->of_node, "hpd-num", &tc->hpd_num); > + if (ret) { > + tc->hpd_num = -ENODEV; > + } else { > + if (tc->hpd_num < 0 || tc->hpd_num > 1) { > + dev_err(dev, "failed to parse HPD number\n"); > + return ret; > + } > + } > + > + if (client->irq > 0) { > + /* enable SysErr */ > + regmap_write(tc->regmap, INTCTL_G, INT_SYSERR); > + > + ret = devm_request_threaded_irq(dev, client->irq, > + NULL, tc_irq_handler, IRQF_ONESHOT, > + "tc358767-irq", tc); > + if (ret) { > + dev_err(dev, "failed to register dp interrupt\n"); > + return ret; > + } > + > + tc->have_irq = true; > + } > + > ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev); > if (ret) { > dev_err(tc->dev, "can not read device ID: %d\n", ret); > @@ -1365,6 +1479,22 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > > tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */ > > + if (tc->hpd_num >= 0) { > + u32 lcnt_reg = tc->hpd_num == 0 ? INT_GP0_LCNT : INT_GP1_LCNT; > + u32 h_lc = INT_GPIO_H(tc->hpd_num) | INT_GPIO_LC(tc->hpd_num); > + > + /* Set LCNT to 2ms */ > + regmap_write(tc->regmap, lcnt_reg, > + clk_get_rate(tc->refclk) * 2 / 1000); > + /* We need the "alternate" mode for HPD */ > + regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_num)); > + > + if (tc->have_irq) { > + /* enable H & LC */ > + regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc); > + } > + } > + > ret = tc_aux_link_setup(tc); > if (ret) > return ret; > @@ -1377,12 +1507,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > if (ret) > return ret; > > - ret = tc_get_display_props(tc); > - if (ret) > - goto err_unregister_aux; > - > - tc_connector_set_polling(tc, &tc->connector); > - > tc->bridge.funcs = &tc_bridge_funcs; > tc->bridge.of_node = dev->of_node; > drm_bridge_add(&tc->bridge); > @@ -1390,9 +1514,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > i2c_set_clientdata(client, tc); > > return 0; > -err_unregister_aux: > - drm_dp_aux_unregister(&tc->aux); > - return ret; > } > > static int tc_remove(struct i2c_client *client)
On 15/04/2019 13:42, Andrzej Hajda wrote: > Ok we have here 4 combinations: > > 1. noHPD + eDP. > > 2. noHPD + DP. > > 3. HPD + eDP. > > 4. HPD + DP. > > > Which ones do you want to support, disallow. It is not clear to me. They all should work. If there is HPD, we use it to return connected/disconnected. If we don't have HPD: - If there's a panel (i.e. eDP), we presume that it is always there, as by definition it can't be disconnected. - If there's no panel (i.e. DP), we can't know if the cable is connected or not, so we return unknown. I think this could be improved by trying to "ping" the monitor with an AUX transaction, but I didn't look at that. > Moreover what connector_status_unknown should mean here for users? The the driver does not know if there's a monitor or not. >> + if (tc->hpd_num >= 0 && tc->bridge.dev) { >> + bool h = val & INT_GPIO_H(tc->hpd_num); >> + bool lc = val & INT_GPIO_LC(tc->hpd_num); >> + >> + dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num, >> + h ? "H" : "", lc ? "LC" : ""); > > > What does "h" and "lc" mean? These are from the func spec. H means high, LC means low-counter. H is triggered immediately when the HPD line goes high, LC is triggered when the line has been low for the counter-specified time (the counter is configured at probe time). These could be used to implement a more elaborate DP HPD & interrupt handling, but for the time being the driver just takes them as "HPD may have changed". Tomi
On 15.04.2019 12:59, Tomi Valkeinen wrote: > On 15/04/2019 13:42, Andrzej Hajda wrote: > >> Ok we have here 4 combinations: >> >> 1. noHPD + eDP. >> >> 2. noHPD + DP. >> >> 3. HPD + eDP. >> >> 4. HPD + DP. >> >> >> Which ones do you want to support, disallow. It is not clear to me. > They all should work. > > If there is HPD, we use it to return connected/disconnected. OK, I though that eDP does not use HPD at all. > If we don't have HPD: > - If there's a panel (i.e. eDP), we presume that it is always there, as > by definition it can't be disconnected. > - If there's no panel (i.e. DP), we can't know if the cable is connected > or not, so we return unknown. I think this could be improved by trying > to "ping" the monitor with an AUX transaction, but I didn't look at that. > >> Moreover what connector_status_unknown should mean here for users? > The the driver does not know if there's a monitor or not. :) More specifically, what user can do with such information. OK, he can ensure monitor is connected and then force connected state. But shall we expect existence of such configurations, I hoped we could eliminate such combination (DP+noHPD) during probe. > >>> + if (tc->hpd_num >= 0 && tc->bridge.dev) { >>> + bool h = val & INT_GPIO_H(tc->hpd_num); >>> + bool lc = val & INT_GPIO_LC(tc->hpd_num); >>> + >>> + dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num, >>> + h ? "H" : "", lc ? "LC" : ""); >> >> What does "h" and "lc" mean? > These are from the func spec. H means high, LC means low-counter. H is > triggered immediately when the HPD line goes high, LC is triggered when > the line has been low for the counter-specified time (the counter is > configured at probe time). It would be good to add this info somewhere, it is hard to guess what lc means. Regards Andrzej > > These could be used to implement a more elaborate DP HPD & interrupt > handling, but for the time being the driver just takes them as "HPD may > have changed". > > Tomi >
Hi, On 17/04/2019 10:32, Andrzej Hajda wrote: > On 15.04.2019 12:59, Tomi Valkeinen wrote: >> On 15/04/2019 13:42, Andrzej Hajda wrote: >> >>> Ok we have here 4 combinations: >>> >>> 1. noHPD + eDP. >>> >>> 2. noHPD + DP. >>> >>> 3. HPD + eDP. >>> >>> 4. HPD + DP. >>> >>> >>> Which ones do you want to support, disallow. It is not clear to me. >> They all should work. >> >> If there is HPD, we use it to return connected/disconnected. > > > OK, I though that eDP does not use HPD at all. > > >> If we don't have HPD: >> - If there's a panel (i.e. eDP), we presume that it is always there, as >> by definition it can't be disconnected. >> - If there's no panel (i.e. DP), we can't know if the cable is connected >> or not, so we return unknown. I think this could be improved by trying >> to "ping" the monitor with an AUX transaction, but I didn't look at that. >> >>> Moreover what connector_status_unknown should mean here for users? >> The the driver does not know if there's a monitor or not. > > :) > > More specifically, what user can do with such information. > > OK, he can ensure monitor is connected and then force connected state. Yes, something like that. I haven't really been testing that kind of setups, but afaik, that's more about how DRM exposes and expects the userspace to handle "unknown" connection status than what TC358767 does. > But shall we expect existence of such configurations, I hoped we could > eliminate such combination (DP+noHPD) during probe. Eliminate how? Make HPD required for DP and fail if there's no HPD? I guess that's an option. Then again, the solutions the HW people come up with in the embedded space never ceases to amaze me, so while I don't expect such configurations, I would not be surprised to see one. >>>> + if (tc->hpd_num >= 0 && tc->bridge.dev) { >>>> + bool h = val & INT_GPIO_H(tc->hpd_num); >>>> + bool lc = val & INT_GPIO_LC(tc->hpd_num); >>>> + >>>> + dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num, >>>> + h ? "H" : "", lc ? "LC" : ""); >>> >>> What does "h" and "lc" mean? >> These are from the func spec. H means high, LC means low-counter. H is >> triggered immediately when the HPD line goes high, LC is triggered when >> the line has been low for the counter-specified time (the counter is >> configured at probe time). > > > It would be good to add this info somewhere, it is hard to guess what lc > means. Ok. Tomi
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 8606de29c9b2..6978129428a8 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -71,6 +71,7 @@ /* System */ #define TC_IDREG 0x0500 +#define SYSSTAT 0x0508 #define SYSCTRL 0x0510 #define DP0_AUDSRC_NO_INPUT (0 << 3) #define DP0_AUDSRC_I2S_RX (1 << 3) @@ -79,9 +80,16 @@ #define DP0_VIDSRC_DPI_RX (2 << 0) #define DP0_VIDSRC_COLOR_BAR (3 << 0) #define GPIOM 0x0540 +#define GPIOC 0x0544 +#define GPIOO 0x0548 #define GPIOI 0x054c #define INTCTL_G 0x0560 #define INTSTS_G 0x0564 + +#define INT_SYSERR BIT(16) +#define INT_GPIO_H(x) (1 << (x == 0 ? 2 : 10)) +#define INT_GPIO_LC(x) (1 << (x == 0 ? 3 : 11)) + #define INT_GP0_LCNT 0x0584 #define INT_GP1_LCNT 0x0588 @@ -219,6 +227,12 @@ struct tc_data { struct gpio_desc *sd_gpio; struct gpio_desc *reset_gpio; struct clk *refclk; + + /* do we have IRQ */ + bool have_irq; + + /* HPD pin number (0 or 1) or -ENODEV */ + int hpd_num; }; static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a) @@ -1095,6 +1109,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge) struct tc_data *tc = bridge_to_tc(bridge); int ret; + ret = tc_get_display_props(tc); + if (ret < 0) { + dev_err(tc->dev, "failed to read display props: %d\n", ret); + return; + } + ret = tc_main_link_enable(tc); if (ret < 0) { dev_err(tc->dev, "main link enable error: %d\n", ret); @@ -1200,19 +1220,42 @@ static int tc_connector_get_modes(struct drm_connector *connector) return count; } -static void tc_connector_set_polling(struct tc_data *tc, - struct drm_connector *connector) -{ - /* TODO: add support for HPD */ - connector->polled = DRM_CONNECTOR_POLL_CONNECT | - DRM_CONNECTOR_POLL_DISCONNECT; -} - static const struct drm_connector_helper_funcs tc_connector_helper_funcs = { .get_modes = tc_connector_get_modes, }; +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force) +{ + struct tc_data *tc = connector_to_tc(connector); + bool conn; + u32 val; + int ret; + + if (tc->hpd_num < 0) { + if (tc->panel) + return connector_status_connected; + else + return connector_status_unknown; + } + + tc_read(GPIOI, &val); + + conn = val & BIT(tc->hpd_num); + + if (force && conn) + tc_get_display_props(tc); + + if (conn) + return connector_status_connected; + else + return connector_status_disconnected; + +err: + return connector_status_unknown; +} + static const struct drm_connector_funcs tc_connector_funcs = { + .detect = tc_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = drm_connector_cleanup, .reset = drm_atomic_helper_connector_reset, @@ -1227,7 +1270,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge) struct drm_device *drm = bridge->dev; int ret; - /* Create eDP connector */ + /* Create DP/eDP connector */ drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs); ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs, tc->panel ? DRM_MODE_CONNECTOR_eDP : @@ -1235,6 +1278,15 @@ static int tc_bridge_attach(struct drm_bridge *bridge) if (ret) return ret; + /* Don't poll if don't have HPD connected */ + if (tc->hpd_num >= 0) { + if (tc->have_irq) + tc->connector.polled = DRM_CONNECTOR_POLL_HPD; + else + tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_DISCONNECT; + } + if (tc->panel) drm_panel_attach(tc->panel, &tc->connector); @@ -1301,6 +1353,43 @@ static const struct regmap_config tc_regmap_config = { .val_format_endian = REGMAP_ENDIAN_LITTLE, }; +static irqreturn_t tc_irq_handler(int irq, void *arg) +{ + struct tc_data *tc = arg; + u32 val; + int r; + + r = regmap_read(tc->regmap, INTSTS_G, &val); + if (r) + return IRQ_NONE; + + if (!val) + return IRQ_NONE; + + if (val & INT_SYSERR) { + u32 stat = 0; + + regmap_read(tc->regmap, SYSSTAT, &stat); + + dev_err(tc->dev, "syserr %x\n", stat); + } + + if (tc->hpd_num >= 0 && tc->bridge.dev) { + bool h = val & INT_GPIO_H(tc->hpd_num); + bool lc = val & INT_GPIO_LC(tc->hpd_num); + + dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num, + h ? "H" : "", lc ? "LC" : ""); + + if (h || lc) + drm_kms_helper_hotplug_event(tc->bridge.dev); + } + + regmap_write(tc->regmap, INTSTS_G, val); + + return IRQ_HANDLED; +} + static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct device *dev = &client->dev; @@ -1352,6 +1441,31 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) return ret; } + ret = of_property_read_u32(dev->of_node, "hpd-num", &tc->hpd_num); + if (ret) { + tc->hpd_num = -ENODEV; + } else { + if (tc->hpd_num < 0 || tc->hpd_num > 1) { + dev_err(dev, "failed to parse HPD number\n"); + return ret; + } + } + + if (client->irq > 0) { + /* enable SysErr */ + regmap_write(tc->regmap, INTCTL_G, INT_SYSERR); + + ret = devm_request_threaded_irq(dev, client->irq, + NULL, tc_irq_handler, IRQF_ONESHOT, + "tc358767-irq", tc); + if (ret) { + dev_err(dev, "failed to register dp interrupt\n"); + return ret; + } + + tc->have_irq = true; + } + ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev); if (ret) { dev_err(tc->dev, "can not read device ID: %d\n", ret); @@ -1365,6 +1479,22 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */ + if (tc->hpd_num >= 0) { + u32 lcnt_reg = tc->hpd_num == 0 ? INT_GP0_LCNT : INT_GP1_LCNT; + u32 h_lc = INT_GPIO_H(tc->hpd_num) | INT_GPIO_LC(tc->hpd_num); + + /* Set LCNT to 2ms */ + regmap_write(tc->regmap, lcnt_reg, + clk_get_rate(tc->refclk) * 2 / 1000); + /* We need the "alternate" mode for HPD */ + regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_num)); + + if (tc->have_irq) { + /* enable H & LC */ + regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc); + } + } + ret = tc_aux_link_setup(tc); if (ret) return ret; @@ -1377,12 +1507,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) if (ret) return ret; - ret = tc_get_display_props(tc); - if (ret) - goto err_unregister_aux; - - tc_connector_set_polling(tc, &tc->connector); - tc->bridge.funcs = &tc_bridge_funcs; tc->bridge.of_node = dev->of_node; drm_bridge_add(&tc->bridge); @@ -1390,9 +1514,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) i2c_set_clientdata(client, tc); return 0; -err_unregister_aux: - drm_dp_aux_unregister(&tc->aux); - return ret; } static int tc_remove(struct i2c_client *client)
Add support for interrupt and hotplug handling. Both are optional. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/bridge/tc358767.c | 157 ++++++++++++++++++++++++++---- 1 file changed, 139 insertions(+), 18 deletions(-)