Messages in this thread | | | Subject | Re: [PATCH v4] devres: combine function devm_ioremap* | From | Yisheng Xie <> | Date | Mon, 15 Jan 2018 09:06:17 +0800 |
| |
Hi Christophe,
Thanks for your review and comment, I will follow your suggestion listed in line.
Thanks Yisheng
On 2018/1/14 0:39, christophe leroy wrote: > Hi Yisheng, > > Le 12/01/2018 à 12:51, Yisheng Xie a écrit : >> When I tried to use devm_ioremap function and review related >> code, I found devm_ioremap_* almost have the similar realize >> with each other, which can be combined. >> >> In the former versions, I have tried to kill ioremap_cache to >> reduce the size of devres, which can not work for ioremap is >> not the same as ioremap_nocache in some ARCHs likes ia64. >> Therefore, as the suggestion of Christophe, I introduce a help >> function __devm_ioremap, let devm_ioremap* inline and call >> __devm_ioremap with different devm_ioremap_type. >> >> After apply the patch, the size of devres.o can be reduce from >> 8216 Bytes to 7352Bytes in my compile environment. >> >> Suggested-by: Christophe LEROY <christophe.leroy@c-s.fr> >> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com> >> --- >> v2: >> - use MARCO for ioremap >> v3: >> - kill dev_ioremap_nocache >> v4: >> - combine function devm_ioremap* >> >> include/linux/io.h | 33 +++++++++++++++++++----- >> lib/devres.c | 76 +++++++++++------------------------------------------- >> 2 files changed, 42 insertions(+), 67 deletions(-) >> >> diff --git a/include/linux/io.h b/include/linux/io.h >> index 32e30e8..da33a5f 100644 >> --- a/include/linux/io.h >> +++ b/include/linux/io.h >> @@ -73,12 +73,33 @@ static inline void devm_ioport_unmap(struct device *dev, void __iomem *addr) >> #define IOMEM_ERR_PTR(err) (__force void __iomem *)ERR_PTR(err) >> -void __iomem *devm_ioremap(struct device *dev, resource_size_t offset, >> - resource_size_t size); >> -void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset, >> - resource_size_t size); >> -void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset, >> - resource_size_t size); >> +typedef enum { >> + DEVM_IOREMAP = 0, >> + DEVM_IOREMAP_NC, >> + DEVM_IOREMAP_WC, >> +} devm_ioremap_type; > > In accordance with Kernel codying style, you should not define a new typedef for that. > > Instead, use: > > enum devm_ioremap_type { > DEVM_IOREMAP = 0, > DEVM_IOREMAP_NC, > DEVM_IOREMAP_WC, > }; > >> + >> +void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset, >> + resource_size_t size, devm_ioremap_type type); > > Use 'enum devm_ioremap_type type' instead of 'devm_ioremap_type' > > And alignment of second line should match open parenthesis. > >> + >> +static inline void __iomem *devm_ioremap(struct device *dev, >> + resource_size_t offset, resource_size_t size) > > Alignment of second line should match open parenthesis. > >> +{ >> + return __devm_ioremap(dev, offset, size, DEVM_IOREMAP); >> +} >> + >> +static inline void __iomem *devm_ioremap_nocache(struct device *dev, >> + resource_size_t offset, resource_size_t size) > > Alignment of second line should match open parenthesis. > >> +{ >> + return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_NC); >> +} >> + >> +static inline void __iomem *devm_ioremap_wc(struct device *dev, >> + resource_size_t offset, resource_size_t size) > > Alignment of second line should match open parenthesis. > >> +{ >> + return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_WC); >> +} >> + >> void devm_iounmap(struct device *dev, void __iomem *addr); >> int check_signature(const volatile void __iomem *io_addr, >> const unsigned char *signature, int length); >> diff --git a/lib/devres.c b/lib/devres.c >> index 5f2aedd..e00eb4f 100644 >> --- a/lib/devres.c >> +++ b/lib/devres.c >> @@ -23,73 +23,27 @@ static int devm_ioremap_match(struct device *dev, void *res, void *match_data) >> * >> * Managed ioremap(). Map is automatically unmapped on driver detach. >> */ > > That comment should be moved to io.h > >> -void __iomem *devm_ioremap(struct device *dev, resource_size_t offset, >> - resource_size_t size) >> +void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset, >> + resource_size_t size, devm_ioremap_type type) > > Alignment of second line should match open parenthesis. > >> { >> - void __iomem **ptr, *addr; >> - >> - ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL); >> - if (!ptr) >> - return NULL; >> - >> - addr = ioremap(offset, size); >> - if (addr) { >> - *ptr = addr; >> - devres_add(dev, ptr); >> - } else >> - devres_free(ptr); >> - >> - return addr; >> -} >> -EXPORT_SYMBOL(devm_ioremap); >> - >> -/** >> - * devm_ioremap_nocache - Managed ioremap_nocache() >> - * @dev: Generic device to remap IO address for >> - * @offset: Resource address to map >> - * @size: Size of map >> - * >> - * Managed ioremap_nocache(). Map is automatically unmapped on driver >> - * detach. >> - */ > > That comment should be moved to io.h > >> -void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset, >> - resource_size_t size) >> -{ >> - void __iomem **ptr, *addr; >> + void __iomem **ptr, *addr = NULL; >> ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL); >> if (!ptr) >> return NULL; >> - addr = ioremap_nocache(offset, size); >> - if (addr) { >> - *ptr = addr; >> - devres_add(dev, ptr); >> - } else >> - devres_free(ptr); >> - >> - return addr; >> -} >> -EXPORT_SYMBOL(devm_ioremap_nocache); >> - >> -/** >> - * devm_ioremap_wc - Managed ioremap_wc() >> - * @dev: Generic device to remap IO address for >> - * @offset: Resource address to map >> - * @size: Size of map >> - * >> - * Managed ioremap_wc(). Map is automatically unmapped on driver detach. >> - */ > > That comment should be moved to io.h > >> -void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset, >> - resource_size_t size) >> -{ >> - void __iomem **ptr, *addr; >> - >> - ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL); >> - if (!ptr) >> - return NULL; >> + switch (type) { >> + case DEVM_IOREMAP: >> + addr = ioremap(offset, size); >> + break; >> + case DEVM_IOREMAP_NC: >> + addr = ioremap_nocache(offset, size); >> + break; >> + case DEVM_IOREMAP_WC: >> + addr = ioremap_wc(offset, size); >> + break; >> + } >> - addr = ioremap_wc(offset, size); >> if (addr) { >> *ptr = addr; >> devres_add(dev, ptr); >> @@ -98,7 +52,7 @@ void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset, >> return addr; >> } >> -EXPORT_SYMBOL(devm_ioremap_wc); >> +EXPORT_SYMBOL(__devm_ioremap); >> /** >> * devm_iounmap - Managed iounmap() >> > > Christophe > > --- > L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast. > https://www.avast.com/antivirus > > > . >
| |