Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 28 Aug 2020 19:55:48 +0200
From:   Sam Ravnborg <sam@ravnborg.org>
To:     Kevin Tang <kevin3.tang@gmail.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Orson Zhai <orsonzhai@gmail.com>,
        "Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>,
        ML dri-devel <dri-devel@lists.freedesktop.org>,
        Chunyan Zhang <zhang.lyra@gmail.com>
Subject: Re: [PATCH RFC v6 2/6] drm/sprd: add Unisoc's drm kms master
Message-ID: <20200828175548.GA660103@ravnborg.org>
References: <1595930879-2478-1-git-send-email-kevin3.tang@gmail.com>
 <1595930879-2478-3-git-send-email-kevin3.tang@gmail.com>
 <20200728204532.GC1277651@ravnborg.org>
 <CAFPSGXaXxr+Du0ptbBHguEPgoHULozSUb-rS3-61pef1fF6b-g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAFPSGXaXxr+Du0ptbBHguEPgoHULozSUb-rS3-61pef1fF6b-g@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Kevin

> >
> > > +
> > > +     drm->dev_private = sprd;
> > dev_private is deprecated. Alwyas use upclassing.
> dev_private is deprecated ? I see everyone is still using it, so it
> will be deprecated in the future?
It is deprecated - and a lot of drivers is in need up an update to get
away from using it.

From the kernel-doc:

        /**
         * @dev_private:
         *
         * DRM driver private data. This is deprecated and should be left set to
         * NULL.
         *
         * Instead of using this pointer it is recommended that drivers use
         * drm_dev_init() and embed struct &drm_device in their larger
         * per-device structure.
         */
        void *dev_private;

> > > +     }
> > > +     /* with irq_enabled = true, we can use the vblank feature. */
> > > +     drm->irq_enabled = true;
> > Can drm_irq_install() be used?
> > Then this flag shall not be set by the driver, And the interrupt numbers on different Soc are not necessarily the same
> >
> We need to set "IRQ_NOAUTOEN" flag for CRTC IRQ, you can see that we
> manually turn the interrupt on and off by crtc
> atomic_enable/atomic_disable
> and the interrupt number on different Soc are not necessarily the
> same. So intall interrupt in kms maybe is not suitable for us...

Maybe drm_irq_install() does not fit the need here. But please take an
extra look as what is does is quite basic. And no need to roll your
own if there is available infrstructure that can be used.

But there is nothing wrong rolloing your own if needed.

	Sam