Hi Tomi,
Thank you for the patch.
On Wed, Nov 01, 2023 at 11:17:46AM +0200, Tomi Valkeinen wrote:
> The IRQ setup code is overly complex. All we really need to do is
> initialize the related fields in struct tidss_device, and request the
> IRQ.
>
> We can drop all the HW accesses, as they are pointless: the driver will
> set the IRQs correctly when it needs any of the IRQs, and at probe time
> we have done a reset, so we know that all the IRQs are masked by default
> in the hardware.
Even for K2G ?
> Thus we can combine the tidss_irq_preinstall() and
> tidss_irq_postinstall() into the tidss_irq_install() function, drop the
> HW accesses, and drop the use of spinlock, as this is done at init time
> and there can be no races.
>
> We can also drop the HW access from the tidss_irq_uninstall(), as the
> driver will anyway disable and suspend the hardware at remove time.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/tidss/tidss_drv.c | 2 ++
> drivers/gpu/drm/tidss/tidss_irq.c | 54 ++++++---------------------------------
> 2 files changed, 10 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 64914331715a..37693f30d98b 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -138,6 +138,8 @@ static int tidss_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, tidss);
>
> + spin_lock_init(&tidss->wait_lock);
> +
> ret = dispc_init(tidss);
> if (ret) {
> dev_err(dev, "failed to initialize dispc: %d\n", ret);
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
> index 0c681c7600bc..604334ef526a 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.c
> +++ b/drivers/gpu/drm/tidss/tidss_irq.c
> @@ -93,33 +93,21 @@ void tidss_irq_resume(struct tidss_device *tidss)
> spin_unlock_irqrestore(&tidss->wait_lock, flags);
> }
>
> -static void tidss_irq_preinstall(struct drm_device *ddev)
> -{
> - struct tidss_device *tidss = to_tidss(ddev);
> -
> - spin_lock_init(&tidss->wait_lock);
> -
> - tidss_runtime_get(tidss);
> -
> - dispc_set_irqenable(tidss->dispc, 0);
> - dispc_read_and_clear_irqstatus(tidss->dispc);
> -
> - tidss_runtime_put(tidss);
> -}
> -
> -static void tidss_irq_postinstall(struct drm_device *ddev)
> +int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
> {
> struct tidss_device *tidss = to_tidss(ddev);
> - unsigned long flags;
> - unsigned int i;
> + int ret;
>
> - tidss_runtime_get(tidss);
> + if (irq == IRQ_NOTCONNECTED)
> + return -ENOTCONN;
>
> - spin_lock_irqsave(&tidss->wait_lock, flags);
> + ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
> + if (ret)
> + return ret;
>
> tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR;
>
> - for (i = 0; i < tidss->num_crtcs; ++i) {
> + for (unsigned int i = 0; i < tidss->num_crtcs; ++i) {
> struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]);
>
> tidss->irq_mask |= DSS_IRQ_VP_SYNC_LOST(tcrtc->hw_videoport);
> @@ -127,28 +115,6 @@ static void tidss_irq_postinstall(struct drm_device *ddev)
> tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport);
> }
>
> - tidss_irq_update(tidss);
> -
> - spin_unlock_irqrestore(&tidss->wait_lock, flags);
> -
> - tidss_runtime_put(tidss);
> -}
> -
> -int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
> -{
> - int ret;
> -
> - if (irq == IRQ_NOTCONNECTED)
> - return -ENOTCONN;
> -
> - tidss_irq_preinstall(ddev);
> -
> - ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
> - if (ret)
> - return ret;
> -
> - tidss_irq_postinstall(ddev);
> -
> return 0;
> }
>
> @@ -156,9 +122,5 @@ void tidss_irq_uninstall(struct drm_device *ddev)
> {
> struct tidss_device *tidss = to_tidss(ddev);
>
> - tidss_runtime_get(tidss);
> - dispc_set_irqenable(tidss->dispc, 0);
> - tidss_runtime_put(tidss);
> -
> free_irq(tidss->irq, ddev);
> }
>
--
Regards,
Laurent Pinchart
