| Message ID | 20230502130223.14719-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | fbdev: Move framebuffer I/O helpers to <asm/fb.h> | expand |
Hi Thomas. On Tue, May 02, 2023 at 03:02:23PM +0200, Thomas Zimmermann wrote: > Update the names of the fb_mem*() helpers to be consistent with their > regular counterparts. Hence, fb_memset() now becomes fb_memset_io(), > fb_memcpy_fromfb() now becomes fb_memcpy_fromio() and fb_memcpy_tofb() > becomes fb_memcpy_toio(). No functional changes. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- ... > > -#ifndef fb_memcpy_fromfb > -static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n) > +#ifndef fb_memcpy_fromio > +static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n) > { > memcpy_fromio(to, from, n); > } > -#define fb_memcpy_fromfb fb_memcpy_fromfb > +#define fb_memcpy_fromio fb_memcpy_fromio > #endif > > -#ifndef fb_memcpy_tofb > -static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n) > +#ifndef fb_memcpy_toio > +static inline void fb_memcpy_toio(volatile void __iomem *to, const void *from, size_t n) > { > memcpy_toio(to, from, n); > } > -#define fb_memcpy_tofb fb_memcpy_tofb > +#define fb_memcpy_toio fb_memcpy_toio > #endif > > #ifndef fb_memset > -static inline void fb_memset(volatile void __iomem *addr, int c, size_t n) > +static inline void fb_memset_io(volatile void __iomem *addr, int c, size_t n) > { > memset_io(addr, c, n); > } > -#define fb_memset fb_memset > +#define fb_memset fb_memset_io The static inlines wrappers does not provide any value, and could be replaced by direct calls to memcpy_fromio(), memcpy_toio(), memset_io(). If you decide to keep the wrappers I will not hold you back, so the patch has my: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> But I prefer the direct calls without the wrappers.... Sam
Hi Am 02.05.23 um 22:08 schrieb Sam Ravnborg: > Hi Thomas. > > On Tue, May 02, 2023 at 03:02:23PM +0200, Thomas Zimmermann wrote: >> Update the names of the fb_mem*() helpers to be consistent with their >> regular counterparts. Hence, fb_memset() now becomes fb_memset_io(), >> fb_memcpy_fromfb() now becomes fb_memcpy_fromio() and fb_memcpy_tofb() >> becomes fb_memcpy_toio(). No functional changes. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- > ... >> >> -#ifndef fb_memcpy_fromfb >> -static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n) >> +#ifndef fb_memcpy_fromio >> +static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n) >> { >> memcpy_fromio(to, from, n); >> } >> -#define fb_memcpy_fromfb fb_memcpy_fromfb >> +#define fb_memcpy_fromio fb_memcpy_fromio >> #endif >> >> -#ifndef fb_memcpy_tofb >> -static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n) >> +#ifndef fb_memcpy_toio >> +static inline void fb_memcpy_toio(volatile void __iomem *to, const void *from, size_t n) >> { >> memcpy_toio(to, from, n); >> } >> -#define fb_memcpy_tofb fb_memcpy_tofb >> +#define fb_memcpy_toio fb_memcpy_toio >> #endif >> >> #ifndef fb_memset >> -static inline void fb_memset(volatile void __iomem *addr, int c, size_t n) >> +static inline void fb_memset_io(volatile void __iomem *addr, int c, size_t n) >> { >> memset_io(addr, c, n); >> } >> -#define fb_memset fb_memset >> +#define fb_memset fb_memset_io > > The static inlines wrappers does not provide any value, and could be replaced by > direct calls to memcpy_fromio(), memcpy_toio(), memset_io(). > > If you decide to keep the wrappers I will not hold you back, so the > patch has my: > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > But I prefer the direct calls without the wrappers.... At first I was also skeptical if those fb_mem*() wrappers are needed. But Arnd mentioned that there are subtle differences between the current code and Linux' mem*_io() functions. Keeping the wrappers might be needed. Best regards Thomas > > Sam
Hi Thomas, On Tue, May 2, 2023 at 3:02 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > (was: fbdev: Use regular I/O function for framebuffers) > > Fbdev provides helpers for framebuffer I/O, such as fb_readl(), > fb_writel() or fb_memcpy_to_fb(). The implementation of each helper > depends on the architecture, but they are all equivalent to regular > I/O functions of similar names. So use regular functions instead and > move all helpers into <asm-generic/fb.h> > > The first patch a simple whitespace cleanup. > > Until now, <linux/fb.h> contained an include of <asm/io.h>. As this > will go away patches 2 to 4 prepare include statements in the various > drivers. Source files that use regular I/O helpers, such as readl(), > now include <linux/io.h>. Source files that use framebuffer I/O > helpers, such as fb_readl(), also include <asm/fb.h>. > > Patch 5 replaces the architecture-based if-else branching in > <linux/fb.h> by helpers in <asm-generic/fb.h>. All helpers use Linux' > existing I/O functions. > > Patch 6 harmonizes naming among fbdev and existing I/O functions. > > The patchset has been built for a variety of platforms, such as x86-64, > arm, aarch64, ppc64, parisc, m64k, mips and sparc. > > v3: > * add the new helpers in <asm-generic/fb.h> > * support reordering and native byte order (Geert, Arnd) Thanks, this fixes the mangled display I was seeing on ARAnyM with bpp=16. BTW, this series seems to have mixed dependencies: the change to include/asm-generic/fb.h depends on "[PATCH v3 00/19] arch: Consolidate <asm/fb.h>"[1], but with that applied, I had to manually fixup drivers/video/fbdev/core/fb_cfb_fops.c. [1] https://lore.kernel.org/all/20230417125651.25126-1-tzimmermann@suse.de, Gr{oetje,eeting}s, Geert
Hi Thomas, On Wed, May 03, 2023 at 10:15:46AM +0200, Thomas Zimmermann wrote: > Hi > > Am 02.05.23 um 22:08 schrieb Sam Ravnborg: > > Hi Thomas. > > > > On Tue, May 02, 2023 at 03:02:23PM +0200, Thomas Zimmermann wrote: > > > Update the names of the fb_mem*() helpers to be consistent with their > > > regular counterparts. Hence, fb_memset() now becomes fb_memset_io(), > > > fb_memcpy_fromfb() now becomes fb_memcpy_fromio() and fb_memcpy_tofb() > > > becomes fb_memcpy_toio(). No functional changes. > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > > --- > > ... > > > -#ifndef fb_memcpy_fromfb > > > -static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n) > > > +#ifndef fb_memcpy_fromio > > > +static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n) > > > { > > > memcpy_fromio(to, from, n); > > > } > > > -#define fb_memcpy_fromfb fb_memcpy_fromfb > > > +#define fb_memcpy_fromio fb_memcpy_fromio > > > #endif > > > -#ifndef fb_memcpy_tofb > > > -static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n) > > > +#ifndef fb_memcpy_toio > > > +static inline void fb_memcpy_toio(volatile void __iomem *to, const void *from, size_t n) > > > { > > > memcpy_toio(to, from, n); > > > } > > > -#define fb_memcpy_tofb fb_memcpy_tofb > > > +#define fb_memcpy_toio fb_memcpy_toio > > > #endif > > > #ifndef fb_memset > > > -static inline void fb_memset(volatile void __iomem *addr, int c, size_t n) > > > +static inline void fb_memset_io(volatile void __iomem *addr, int c, size_t n) > > > { > > > memset_io(addr, c, n); > > > } > > > -#define fb_memset fb_memset > > > +#define fb_memset fb_memset_io > > > > The static inlines wrappers does not provide any value, and could be replaced by > > direct calls to memcpy_fromio(), memcpy_toio(), memset_io(). > > > > If you decide to keep the wrappers I will not hold you back, so the > > patch has my: > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > > > But I prefer the direct calls without the wrappers.... > > At first I was also skeptical if those fb_mem*() wrappers are needed. But > Arnd mentioned that there are subtle differences between the current code > and Linux' mem*_io() functions. Keeping the wrappers might be needed. Saw the dialog, and agree that keeping current behaviour is the way to go for now even if this is more code and wrappers. Sam