Hi Tomi and Sebastian,
Thank you for the patch.
On Thu, Nov 05, 2020 at 02:03:02PM +0200, Tomi Valkeinen wrote:
> From: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
>
> In preparation for removing custom DSS calls from the DSI
> panel driver, this moves support for external tearing event
> GPIOs into the DSI host driver. This way tearing events are
> always handled in the core resulting in simplification of
> the panel drivers.
>
> The TE GPIO acquisition follows works in the same way as the
> exynos DSI implementation.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> ---
> .../gpu/drm/omapdrm/displays/panel-dsi-cm.c | 101 +------------
> drivers/gpu/drm/omapdrm/dss/dsi.c | 138 ++++++++++++++++--
> 2 files changed, 133 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> index 8890ee2ba830..43f63b5a120b 100644
> --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> @@ -54,7 +54,6 @@ struct panel_drv_data {
>
> /* panel HW configuration from DT or platform data */
> struct gpio_desc *reset_gpio;
> - struct gpio_desc *ext_te_gpio;
>
> struct regulator_bulk_data supplies[DCS_REGULATOR_SUPPLY_NUM];
>
> @@ -68,10 +67,6 @@ struct panel_drv_data {
>
> bool te_enabled;
>
> - atomic_t do_update;
> -
> - struct delayed_work te_timeout_work;
> -
> bool intro_printed;
>
> struct workqueue_struct *workqueue;
> @@ -83,8 +78,6 @@ struct panel_drv_data {
>
> #define to_panel_data(p) container_of(p, struct panel_drv_data, dssdev)
>
> -static irqreturn_t dsicm_te_isr(int irq, void *data);
> -static void dsicm_te_timeout_work_callback(struct work_struct *work);
> static int _dsicm_enable_te(struct panel_drv_data *ddata, bool enable);
>
> static int dsicm_panel_reset(struct panel_drv_data *ddata);
> @@ -240,9 +233,6 @@ static int dsicm_enter_ulps(struct panel_drv_data *ddata)
> if (r)
> goto err;
>
> - if (ddata->ext_te_gpio)
> - disable_irq(gpiod_to_irq(ddata->ext_te_gpio));
> -
> src->ops->dsi.disable(src, false, true);
>
> ddata->ulps_enabled = true;
> @@ -271,15 +261,12 @@ static int dsicm_exit_ulps(struct panel_drv_data *ddata)
> src->ops->enable(src);
> ddata->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - r = _dsicm_enable_te(ddata, true);
> + r = _dsicm_enable_te(ddata, ddata->te_enabled);
> if (r) {
> dev_err(&ddata->dsi->dev, "failed to re-enable TE");
> goto err2;
> }
>
> - if (ddata->ext_te_gpio)
> - enable_irq(gpiod_to_irq(ddata->ext_te_gpio));
> -
> dsicm_queue_ulps_work(ddata);
>
> ddata->ulps_enabled = false;
> @@ -290,11 +277,8 @@ static int dsicm_exit_ulps(struct panel_drv_data *ddata)
> dev_err(&ddata->dsi->dev, "failed to exit ULPS");
>
> r = dsicm_panel_reset(ddata);
> - if (!r) {
> - if (ddata->ext_te_gpio)
> - enable_irq(gpiod_to_irq(ddata->ext_te_gpio));
> + if (!r)
> ddata->ulps_enabled = false;
> - }
>
> dsicm_queue_ulps_work(ddata);
>
> @@ -745,43 +729,6 @@ static void dsicm_framedone_cb(int err, void *data)
> src->ops->dsi.bus_unlock(src);
> }
>
> -static irqreturn_t dsicm_te_isr(int irq, void *data)
> -{
> - struct panel_drv_data *ddata = data;
> - struct omap_dss_device *src = ddata->src;
> - int old;
> - int r;
> -
> - old = atomic_cmpxchg(&ddata->do_update, 1, 0);
> -
> - if (old) {
> - cancel_delayed_work(&ddata->te_timeout_work);
> -
> - r = src->ops->dsi.update(src, ddata->dsi->channel, dsicm_framedone_cb,
> - ddata);
> - if (r)
> - goto err;
> - }
> -
> - return IRQ_HANDLED;
> -err:
> - dev_err(&ddata->dsi->dev, "start update failed\n");
> - src->ops->dsi.bus_unlock(src);
> - return IRQ_HANDLED;
> -}
> -
> -static void dsicm_te_timeout_work_callback(struct work_struct *work)
> -{
> - struct panel_drv_data *ddata = container_of(work, struct panel_drv_data,
> - te_timeout_work.work);
> - struct omap_dss_device *src = ddata->src;
> -
> - dev_err(&ddata->dsi->dev, "TE not received for 250ms!\n");
> -
> - atomic_set(&ddata->do_update, 0);
> - src->ops->dsi.bus_unlock(src);
> -}
> -
> static int dsicm_update(struct omap_dss_device *dssdev,
> u16 x, u16 y, u16 w, u16 h)
> {
> @@ -809,16 +756,10 @@ static int dsicm_update(struct omap_dss_device *dssdev,
> if (r)
> goto err;
>
> - if (ddata->te_enabled && ddata->ext_te_gpio) {
> - schedule_delayed_work(&ddata->te_timeout_work,
> - msecs_to_jiffies(250));
> - atomic_set(&ddata->do_update, 1);
> - } else {
> - r = src->ops->dsi.update(src, ddata->dsi->channel, dsicm_framedone_cb,
> - ddata);
> - if (r)
> - goto err;
> - }
> + r = src->ops->dsi.update(src, ddata->dsi->channel, dsicm_framedone_cb,
> + ddata);
> + if (r)
> + goto err;
>
> /* note: no bus_unlock here. unlock is src framedone_cb */
> mutex_unlock(&ddata->lock);
> @@ -840,8 +781,7 @@ static int _dsicm_enable_te(struct panel_drv_data *ddata, bool enable)
> else
> r = mipi_dsi_dcs_set_tear_off(dsi);
>
> - if (!ddata->ext_te_gpio)
> - src->ops->dsi.enable_te(src, enable);
> + src->ops->dsi.enable_te(src, enable);
>
> /* possible panel bug */
> msleep(100);
> @@ -934,14 +874,6 @@ static int dsicm_probe_of(struct mipi_dsi_device *dsi)
> return err;
> }
>
> - ddata->ext_te_gpio = devm_gpiod_get_optional(&dsi->dev, "te",
> - GPIOD_IN);
> - if (IS_ERR(ddata->ext_te_gpio)) {
> - err = PTR_ERR(ddata->ext_te_gpio);
> - dev_err(&dsi->dev, "TE gpio request failed: %d", err);
> - return err;
> - }
> -
> err = of_get_display_timing(node, "panel-timing", &timing);
> if (!err) {
> videomode_from_timing(&timing, &ddata->vm);
> @@ -1024,25 +956,6 @@ static int dsicm_probe(struct mipi_dsi_device *dsi)
>
> mutex_init(&ddata->lock);
>
> - atomic_set(&ddata->do_update, 0);
> -
> - if (ddata->ext_te_gpio) {
> - r = devm_request_irq(dev, gpiod_to_irq(ddata->ext_te_gpio),
> - dsicm_te_isr,
> - IRQF_TRIGGER_RISING,
> - "taal vsync", ddata);
> -
> - if (r) {
> - dev_err(dev, "IRQ request failed\n");
> - goto err_reg;
> - }
> -
> - INIT_DEFERRABLE_WORK(&ddata->te_timeout_work,
> - dsicm_te_timeout_work_callback);
> -
> - dev_dbg(dev, "Using GPIO TE\n");
> - }
> -
> ddata->workqueue = create_singlethread_workqueue("dsicm_wq");
> if (!ddata->workqueue) {
> r = -ENOMEM;
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index 003d26cead5a..921e7a1e1014 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -14,7 +14,9 @@
> #include <linux/device.h>
> #include <linux/err.h>
> #include <linux/interrupt.h>
> +#include <linux/irq.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
You could sort the headers while at it :-)
> #include <linux/mutex.h>
> #include <linux/module.h>
> #include <linux/semaphore.h>
> @@ -368,6 +370,11 @@ struct dsi_data {
> unsigned int update_bytes;
> #endif
>
> + /* external TE GPIO */
> + struct gpio_desc *te_gpio;
> + struct delayed_work te_timeout_work;
> + atomic_t do_ext_te_update;
> +
> bool te_enabled;
> bool ulps_enabled;
>
> @@ -3827,19 +3834,12 @@ static void dsi_framedone_irq_callback(void *data)
> dsi_handle_framedone(dsi, 0);
> }
>
> -static int dsi_update(struct omap_dss_device *dssdev, int channel,
> - void (*callback)(int, void *), void *data)
> +static int _dsi_update(struct dsi_data *dsi)
> {
> - struct dsi_data *dsi = to_dsi_data(dssdev);
> u16 dw, dh;
>
> dsi_perf_mark_setup(dsi);
>
> - dsi->update_channel = channel;
> -
> - dsi->framedone_callback = callback;
> - dsi->framedone_data = data;
> -
> dw = dsi->vm.hactive;
> dh = dsi->vm.vactive;
>
> @@ -3852,6 +3852,26 @@ static int dsi_update(struct omap_dss_device *dssdev, int channel,
> return 0;
> }
>
> +static int dsi_update(struct omap_dss_device *dssdev, int channel,
> + void (*callback)(int, void *), void *data)
> +{
> + struct dsi_data *dsi = to_dsi_data(dssdev);
> +
> + dsi->update_channel = channel;
> + dsi->framedone_callback = callback;
> + dsi->framedone_data = data;
> +
> + if (dsi->te_enabled && dsi->te_gpio) {
> + schedule_delayed_work(&dsi->te_timeout_work,
> + msecs_to_jiffies(250));
> + atomic_set(&dsi->do_ext_te_update, 1);
> + } else {
> + _dsi_update(dsi);
> + }
> +
> + return 0;
> +}
> +
> /* Display funcs */
>
> static int dsi_configure_dispc_clocks(struct dsi_data *dsi)
> @@ -4095,6 +4115,14 @@ static int dsi_enable_te(struct omap_dss_device *dssdev, bool enable)
> struct dsi_data *dsi = to_dsi_data(dssdev);
>
> dsi->te_enabled = enable;
> +
> + if (dsi->te_gpio) {
> + if (enable)
> + enable_irq(gpiod_to_irq(dsi->te_gpio));
> + else
> + disable_irq(gpiod_to_irq(dsi->te_gpio));
> + }
> +
> return 0;
> }
>
> @@ -4772,11 +4800,89 @@ static const struct omap_dss_device_ops dsi_ops = {
> },
> };
>
> +static irqreturn_t omap_dsi_te_irq_handler(int irq, void *dev_id)
> +{
> + struct dsi_data *dsi = (struct dsi_data *)dev_id;
> + int old;
> +
> + old = atomic_cmpxchg(&dsi->do_ext_te_update, 1, 0);
> + if (old) {
> + cancel_delayed_work(&dsi->te_timeout_work);
> + _dsi_update(dsi);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void omap_dsi_te_timeout_work_callback(struct work_struct *work)
> +{
> + struct dsi_data *dsi = container_of(work, struct dsi_data,
> + te_timeout_work.work);
Strange indentation.
> + int old;
> +
> + old = atomic_cmpxchg(&dsi->do_ext_te_update, 1, 0);
> + if (old) {
> + dev_err(dsi->dev, "TE not received for 250ms!\n");
> + _dsi_update(dsi);
> + }
> +}
> +
> +static int omap_dsi_register_te_irq(struct dsi_data *dsi,
> + struct mipi_dsi_device *client)
> +{
> + int err;
> + int te_irq;
> +
> + dsi->te_gpio = gpiod_get_from_of_node(client->dev.of_node,
Could we call gpiod_get(&client->dev, "te", ...) instead ?
> + "te-gpios", 0, GPIOD_IN,
> + "dsi-tearing-effect");
> + if (IS_ERR(dsi->te_gpio)) {
> + err = PTR_ERR(dsi->te_gpio);
> +
> + if (err == -ENOENT) {
> + dsi->te_gpio = NULL;
> + return 0;
> + }
> +
> + dev_err(dsi->dev, "Could not get TE gpio: %d\n", err);
> + return err;
> + }
> +
> + te_irq = gpiod_to_irq(dsi->te_gpio);
Could this fail ?
> + irq_set_status_flags(te_irq, IRQ_NOAUTOEN);
> +
> + err = request_threaded_irq(te_irq, omap_dsi_te_irq_handler, NULL,
> + IRQF_TRIGGER_RISING, "TE", dsi);
What's the reason to request a threaded IRQ instead of a regular IRQ if
the threaded handler is NULL ?
> + if (err) {
> + dev_err(dsi->dev, "request irq failed with %d\n", err);
> + gpiod_put(dsi->te_gpio);
> + return err;
> + }
> +
> + INIT_DEFERRABLE_WORK(&dsi->te_timeout_work,
> + omap_dsi_te_timeout_work_callback);
> +
> + dev_dbg(dsi->dev, "Using GPIO TE\n");
> +
> + return 0;
> +}
> +
> +static void omap_dsi_unregister_te_irq(struct dsi_data *dsi)
> +{
> + if (dsi->te_gpio) {
> + free_irq(gpiod_to_irq(dsi->te_gpio), dsi);
You could store the IRQ number in dsi_data to avoid recomputing it.
> + cancel_delayed_work(&dsi->te_timeout_work);
> + gpiod_put(dsi->te_gpio);
> + dsi->te_gpio = NULL;
> + }
> +}
> +
> static int omap_dsi_host_attach(struct mipi_dsi_host *host,
> struct mipi_dsi_device *client)
> {
> struct dsi_data *dsi = host_to_omap(host);
> unsigned int channel = client->channel;
> + int r;
>
> if (channel > 3)
> return -EINVAL;
> @@ -4791,13 +4897,20 @@ static int omap_dsi_host_attach(struct mipi_dsi_host *host,
> return -EINVAL;
> }
>
> - dsi->vc[channel].dest = client;
> + atomic_set(&dsi->do_ext_te_update, 0);
>
> - dsi->pix_fmt = client->format;
> - if (client->mode_flags & MIPI_DSI_MODE_VIDEO)
> + if (client->mode_flags & MIPI_DSI_MODE_VIDEO) {
> dsi->mode = OMAP_DSS_DSI_VIDEO_MODE;
> - else
> + } else {
> + r = omap_dsi_register_te_irq(dsi, client);
> + if (r)
> + return r;
> +
> dsi->mode = OMAP_DSS_DSI_CMD_MODE;
> + }
> +
> + dsi->vc[channel].dest = client;
> + dsi->pix_fmt = client->format;
>
> return 0;
> }
> @@ -4814,6 +4927,7 @@ static int omap_dsi_host_detach(struct mipi_dsi_host *host,
> if (dsi->vc[channel].dest != client)
> return -EINVAL;
>
> + omap_dsi_unregister_te_irq(dsi);
> dsi->vc[channel].dest = NULL;
> return 0;
> }
--
Regards,
Laurent Pinchart
