diff mbox series

[v2,1/4] drm/via: drop use of DRM(READ|WRITE) macros

Message ID 20190720084527.12593-2-sam@ravnborg.org (mailing list archive)
State New, archived
Headers show
Series drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h | expand

Commit Message

Sam Ravnborg July 20, 2019, 8:45 a.m. UTC 
The DRM_READ, DRM_WRITE macros comes from the deprecated drm_os_linux.h
header file. Remove their use to remove this dependency.

Replace the use of the macros with open coded variants.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Kevin Brace <kevinbrace@gmx.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: Michel Dänzer <michel@daenzer.net>
---
 drivers/gpu/drm/via/via_drv.h | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Emil Velikov July 22, 2019, 3:38 p.m. UTC | #1 
On Sat, 20 Jul 2019 at 09:46, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> The DRM_READ, DRM_WRITE macros comes from the deprecated drm_os_linux.h
> header file. Remove their use to remove this dependency.
>
> Replace the use of the macros with open coded variants.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Kevin Brace <kevinbrace@gmx.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
> Cc: Mike Marshall <hubcap@omnibond.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Emil Velikov <emil.velikov@collabora.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> ---
>  drivers/gpu/drm/via/via_drv.h | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/via/via_drv.h b/drivers/gpu/drm/via/via_drv.h
> index 6d1ae834484c..d5a2b1ffd8c1 100644
> --- a/drivers/gpu/drm/via/via_drv.h
> +++ b/drivers/gpu/drm/via/via_drv.h
> @@ -115,10 +115,17 @@ enum via_family {
>  /* VIA MMIO register access */
>  #define VIA_BASE ((dev_priv->mmio))
>
> -#define VIA_READ(reg)          DRM_READ32(VIA_BASE, reg)
> -#define VIA_WRITE(reg, val)    DRM_WRITE32(VIA_BASE, reg, val)
> -#define VIA_READ8(reg)         DRM_READ8(VIA_BASE, reg)
> -#define VIA_WRITE8(reg, val)   DRM_WRITE8(VIA_BASE, reg, val)
> +#define VIA_READ(reg) \
> +       readl(((void __iomem *)VIA_BASE->handle) + (reg))
> +
> +#define VIA_WRITE(reg, val) \
> +       writel(val, ((void __iomem *)VIA_BASE->handle) + (reg))
> +
> +#define VIA_READ8(reg) \
> +       readb(((void __iomem *)VIA_BASE->handle) + (reg))
> +
> +#define VIA_WRITE8(reg, val) \
> +       writeb(val, ((void __iomem *)VIA_BASE->handle) + (reg))
>
IMHO a far better idea is to expand these macros as static inline functions.
The extra bonus here is that the pseudo-magical VIA_BASE will also disappear.

Since all the VIA_READ8 are used for masking, one might as well drop
them in favour of via_mask().

WDYT?
-Emil
Sam Ravnborg July 22, 2019, 4:17 p.m. UTC | #2 
Hi Emil.

On Mon, Jul 22, 2019 at 04:38:39PM +0100, Emil Velikov wrote:
> On Sat, 20 Jul 2019 at 09:46, Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > The DRM_READ, DRM_WRITE macros comes from the deprecated drm_os_linux.h
> > header file. Remove their use to remove this dependency.
> >
> > Replace the use of the macros with open coded variants.
> >
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Kevin Brace <kevinbrace@gmx.com>
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
> > Cc: Mike Marshall <hubcap@omnibond.com>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Emil Velikov <emil.velikov@collabora.com>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > ---
> >  drivers/gpu/drm/via/via_drv.h | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/via/via_drv.h b/drivers/gpu/drm/via/via_drv.h
> > index 6d1ae834484c..d5a2b1ffd8c1 100644
> > --- a/drivers/gpu/drm/via/via_drv.h
> > +++ b/drivers/gpu/drm/via/via_drv.h
> > @@ -115,10 +115,17 @@ enum via_family {
> >  /* VIA MMIO register access */
> >  #define VIA_BASE ((dev_priv->mmio))
> >
> > -#define VIA_READ(reg)          DRM_READ32(VIA_BASE, reg)
> > -#define VIA_WRITE(reg, val)    DRM_WRITE32(VIA_BASE, reg, val)
> > -#define VIA_READ8(reg)         DRM_READ8(VIA_BASE, reg)
> > -#define VIA_WRITE8(reg, val)   DRM_WRITE8(VIA_BASE, reg, val)
> > +#define VIA_READ(reg) \
> > +       readl(((void __iomem *)VIA_BASE->handle) + (reg))
> > +
> > +#define VIA_WRITE(reg, val) \
> > +       writel(val, ((void __iomem *)VIA_BASE->handle) + (reg))
> > +
> > +#define VIA_READ8(reg) \
> > +       readb(((void __iomem *)VIA_BASE->handle) + (reg))
> > +
> > +#define VIA_WRITE8(reg, val) \
> > +       writeb(val, ((void __iomem *)VIA_BASE->handle) + (reg))
> >
> IMHO a far better idea is to expand these macros as static inline functions.
> The extra bonus here is that the pseudo-magical VIA_BASE will also disappear.
> 
> Since all the VIA_READ8 are used for masking, one might as well drop
> them in favour of via_mask().
> 
> WDYT?

As this is a legacy driver I like the steps to be small.
So we keep it like this but maybe address it in a follow-up patch?

	Sam
Emil Velikov July 22, 2019, 4:27 p.m. UTC | #3 
On Mon, 22 Jul 2019 at 17:17, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Emil.
>
> On Mon, Jul 22, 2019 at 04:38:39PM +0100, Emil Velikov wrote:
> > On Sat, 20 Jul 2019 at 09:46, Sam Ravnborg <sam@ravnborg.org> wrote:
> > >
> > > The DRM_READ, DRM_WRITE macros comes from the deprecated drm_os_linux.h
> > > header file. Remove their use to remove this dependency.
> > >
> > > Replace the use of the macros with open coded variants.
> > >
> > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > > Cc: Kevin Brace <kevinbrace@gmx.com>
> > > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > > Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
> > > Cc: Mike Marshall <hubcap@omnibond.com>
> > > Cc: Ira Weiny <ira.weiny@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Emil Velikov <emil.velikov@collabora.com>
> > > Cc: Michel Dänzer <michel@daenzer.net>
> > > ---
> > >  drivers/gpu/drm/via/via_drv.h | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/via/via_drv.h b/drivers/gpu/drm/via/via_drv.h
> > > index 6d1ae834484c..d5a2b1ffd8c1 100644
> > > --- a/drivers/gpu/drm/via/via_drv.h
> > > +++ b/drivers/gpu/drm/via/via_drv.h
> > > @@ -115,10 +115,17 @@ enum via_family {
> > >  /* VIA MMIO register access */
> > >  #define VIA_BASE ((dev_priv->mmio))
> > >
> > > -#define VIA_READ(reg)          DRM_READ32(VIA_BASE, reg)
> > > -#define VIA_WRITE(reg, val)    DRM_WRITE32(VIA_BASE, reg, val)
> > > -#define VIA_READ8(reg)         DRM_READ8(VIA_BASE, reg)
> > > -#define VIA_WRITE8(reg, val)   DRM_WRITE8(VIA_BASE, reg, val)
> > > +#define VIA_READ(reg) \
> > > +       readl(((void __iomem *)VIA_BASE->handle) + (reg))
> > > +
> > > +#define VIA_WRITE(reg, val) \
> > > +       writel(val, ((void __iomem *)VIA_BASE->handle) + (reg))
> > > +
> > > +#define VIA_READ8(reg) \
> > > +       readb(((void __iomem *)VIA_BASE->handle) + (reg))
> > > +
> > > +#define VIA_WRITE8(reg, val) \
> > > +       writeb(val, ((void __iomem *)VIA_BASE->handle) + (reg))
> > >
> > IMHO a far better idea is to expand these macros as static inline functions.
> > The extra bonus here is that the pseudo-magical VIA_BASE will also disappear.
> >
> > Since all the VIA_READ8 are used for masking, one might as well drop
> > them in favour of via_mask().
> >
> > WDYT?
>
> As this is a legacy driver I like the steps to be small.
> So we keep it like this but maybe address it in a follow-up patch?
>
Sounds like unnecessary churn BTH.

Looking from another angle - machines with such GPUs are likely to be
slow at building the kernel.
So people testing this, then another series (or two?) which does the
above polish would be time consuming.
Perhaps even a bit annoying.

That said, I don't have a strong preference.

-Emil
Sam Ravnborg July 22, 2019, 6:27 p.m. UTC | #4 
Hi Email.

> > > IMHO a far better idea is to expand these macros as static inline functions.
> > > The extra bonus here is that the pseudo-magical VIA_BASE will also disappear.
> > >
> > > Since all the VIA_READ8 are used for masking, one might as well drop
> > > them in favour of via_mask().

Like this:
static inline u32 via_read(struct drm_via_private *dev_priv, u32 reg)
{
        return readl((void __iomem *)(dev_priv->mmio->handle + reg));
}

static inline void via_write(struct drm_via_private *dev_priv, u32 reg,
                             u32 val)
{
        writel(val, (void __iomem *)(dev_priv->mmio->handle + reg));
}

static inline void via_write8(struct drm_via_private *dev_priv, u32 reg,
                              u32 val)
{
        writeb(val, (void __iomem *)(dev_priv->mmio->handle + reg));
}

static inline void via_write8_mask_or(struct drm_via_private *dev_priv,
                                      u32 reg, u32 mask)
{
        u32 val;

        val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
        writeb(val | mask, (void __iomem *)(dev_priv->mmio->handle + reg));
}

static inline void via_write8_mask_and(struct drm_via_private *dev_priv,
                                       u32 reg, u32 mask)
{
        u32 val;

        val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
        writeb(val & mask, (void __iomem *)(dev_priv->mmio->handle + reg));
}

Patches are almost ready, but if there is any quick feedback let me
know.

	Sam
Emil Velikov July 23, 2019, 11:49 a.m. UTC | #5 
On 2019/07/22, Sam Ravnborg wrote:
> Hi Email.
> 
> > > > IMHO a far better idea is to expand these macros as static inline functions.
> > > > The extra bonus here is that the pseudo-magical VIA_BASE will also disappear.
> > > >
> > > > Since all the VIA_READ8 are used for masking, one might as well drop
> > > > them in favour of via_mask().
> 
> Like this:
> static inline u32 via_read(struct drm_via_private *dev_priv, u32 reg)
> {
>         return readl((void __iomem *)(dev_priv->mmio->handle + reg));
> }
> 
> static inline void via_write(struct drm_via_private *dev_priv, u32 reg,
>                              u32 val)
> {
>         writel(val, (void __iomem *)(dev_priv->mmio->handle + reg));
> }
> 
> static inline void via_write8(struct drm_via_private *dev_priv, u32 reg,
>                               u32 val)
> {
>         writeb(val, (void __iomem *)(dev_priv->mmio->handle + reg));
> }
> 
> static inline void via_write8_mask_or(struct drm_via_private *dev_priv,
>                                       u32 reg, u32 mask)
> {
>         u32 val;
> 
>         val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
>         writeb(val | mask, (void __iomem *)(dev_priv->mmio->handle + reg));
> }
> 
> static inline void via_write8_mask_and(struct drm_via_private *dev_priv,
>                                        u32 reg, u32 mask)
> {
>         u32 val;
> 
>         val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
>         writeb(val & mask, (void __iomem *)(dev_priv->mmio->handle + reg));
> }
> 
> Patches are almost ready, but if there is any quick feedback let me
> know.
> 

Don't think I've seen any "mask_and" "mask_or" API in DRM. The common
theme seems to be:

mtk_cec_mask(driver_priv, offset, value, mask)
malidp_write32_mask(driver_priv, offset, mask, value)
nvif_mask(driver_priv, address, mask, value)

HTH
Emil
Sam Ravnborg July 23, 2019, 7:01 p.m. UTC | #6 
Hi Emil.

> > 
> > Like this:
> > 
> > static inline void via_write8_mask_or(struct drm_via_private *dev_priv,
> >                                       u32 reg, u32 mask)
> > {
> >         u32 val;
> > 
> >         val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
> >         writeb(val | mask, (void __iomem *)(dev_priv->mmio->handle + reg));
> > }
> > 
> > static inline void via_write8_mask_and(struct drm_via_private *dev_priv,
> >                                        u32 reg, u32 mask)
> > {
> >         u32 val;
> > 
> >         val = readb((void __iomem *)(dev_priv->mmio->handle + reg));
> >         writeb(val & mask, (void __iomem *)(dev_priv->mmio->handle + reg));
> > }
> > 
> > Patches are almost ready, but if there is any quick feedback let me
> > know.
> > 
> 
> Don't think I've seen any "mask_and" "mask_or" API in DRM. The common
> theme seems to be:
> 
> mtk_cec_mask(driver_priv, offset, value, mask)
> malidp_write32_mask(driver_priv, offset, mask, value)
> nvif_mask(driver_priv, address, mask, value)
Yep, this is better. Will send out an updated version.

Hmm, some inconsistency in order of parameters.
Decided for mask, value - seems a little bit more logical to me.

	Sam
diff mbox series

Patch

diff --git a/drivers/gpu/drm/via/via_drv.h b/drivers/gpu/drm/via/via_drv.h
index 6d1ae834484c..d5a2b1ffd8c1 100644
--- a/drivers/gpu/drm/via/via_drv.h
+++ b/drivers/gpu/drm/via/via_drv.h
@@ -115,10 +115,17 @@  enum via_family {
 /* VIA MMIO register access */
 #define VIA_BASE ((dev_priv->mmio))
 
-#define VIA_READ(reg)		DRM_READ32(VIA_BASE, reg)
-#define VIA_WRITE(reg, val)	DRM_WRITE32(VIA_BASE, reg, val)
-#define VIA_READ8(reg)		DRM_READ8(VIA_BASE, reg)
-#define VIA_WRITE8(reg, val)	DRM_WRITE8(VIA_BASE, reg, val)
+#define VIA_READ(reg) \
+	readl(((void __iomem *)VIA_BASE->handle) + (reg))
+
+#define VIA_WRITE(reg, val) \
+	writel(val, ((void __iomem *)VIA_BASE->handle) + (reg))
+
+#define VIA_READ8(reg) \
+	readb(((void __iomem *)VIA_BASE->handle) + (reg))
+
+#define VIA_WRITE8(reg, val) \
+	writeb(val, ((void __iomem *)VIA_BASE->handle) + (reg))
 
 extern const struct drm_ioctl_desc via_ioctls[];
 extern int via_max_ioctl;