Re: [PATCH v6] devres: combine function devm_ioremap*

[Christophe LEROY]

Hi Yishen,

I don't have any comment.

Reviewed-by: <christophe.leroy@xxxxxx>

Christophe

Le 12/02/2018 à 12:07, Yisheng Xie a écrit :
> Hi Greg, Christophe,
>
> Any comment about this version?  And sorry to disturb  :)
>
> Thanks
> Yisheng
>
> On 2018/1/29 19:48, Yisheng Xie wrote:
> > 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 version, 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 8052 Bytes in my compile environment.
> >
> > Suggested-by: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Suggested-by: Christophe LEROY <christophe.leroy@xxxxxx>
> > Signed-off-by: Yisheng Xie <xieyisheng1@xxxxxxxxxx>
> > ---
> > v2:
> >   - use MARCO for ioremap
> > v3:
> >   - kill dev_ioremap_nocache
> > v4:
> >   - combine function devm_ioremap*       - per Christophe
> > v5:
> >   - fix code style.                      - per Christophe
> > v6:
> >   - just put the cleanup in the devres.c - per Greg
> >
> >   lib/devres.c | 78 +++++++++++++++++++++++++++++-------------------------------
> >   1 file changed, 38 insertions(+), 40 deletions(-)
> >
> > diff --git a/lib/devres.c b/lib/devres.c
> > index 5f2aedd..5bec112 100644
> > --- a/lib/devres.c
> > +++ b/lib/devres.c
> > @@ -5,6 +5,12 @@
> >   #include <linux/gfp.h>
> >   #include <linux/export.h>
> >   
> > +enum devm_ioremap_type {
> > +	DEVM_IOREMAP = 0,
> > +	DEVM_IOREMAP_NC,
> > +	DEVM_IOREMAP_WC,
> > +};
> > +
> >   void devm_ioremap_release(struct device *dev, void *res)
> >   {
> >   	iounmap(*(void __iomem **)res);
> > @@ -15,24 +21,28 @@ static int devm_ioremap_match(struct device *dev, void *res, void *match_data)
> >   	return *(void **)res == match_data;
> >   }
> >   
> > -/**
> > - * devm_ioremap - Managed ioremap()
> > - * @dev: Generic device to remap IO address for
> > - * @offset: Resource address to map
> > - * @size: Size of map
> > - *
> > - * Managed ioremap().  Map is automatically unmapped on driver detach.
> > - */
> > -void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> > -			   resource_size_t size)
> > +static void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset,
> > +				    resource_size_t size,
> > +				    enum devm_ioremap_type type)
> >   {
> > -	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(offset, size);
> > +	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;
> > +	}
> > +
> >   	if (addr) {
> >   		*ptr = addr;
> >   		devres_add(dev, ptr);
> > @@ -41,6 +51,20 @@ void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> >   
> >   	return addr;
> >   }
> > +
> > +/**
> > + * devm_ioremap - Managed ioremap()
> > + * @dev: Generic device to remap IO address for
> > + * @offset: Resource address to map
> > + * @size: Size of map
> > + *
> > + * Managed ioremap().  Map is automatically unmapped on driver detach.
> > + */
> > +void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> > +			   resource_size_t size)
> > +{
> > +	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP);
> > +}
> >   EXPORT_SYMBOL(devm_ioremap);
> >   
> >   /**
> > @@ -55,20 +79,7 @@ void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> >   void __iomem *devm_ioremap_nocache(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;
> > -
> > -	addr = ioremap_nocache(offset, size);
> > -	if (addr) {
> > -		*ptr = addr;
> > -		devres_add(dev, ptr);
> > -	} else
> > -		devres_free(ptr);
> > -
> > -	return addr;
> > +	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_NC);
> >   }
> >   EXPORT_SYMBOL(devm_ioremap_nocache);
> >   
> > @@ -83,20 +94,7 @@ void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
> >   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;
> > -
> > -	addr = ioremap_wc(offset, size);
> > -	if (addr) {
> > -		*ptr = addr;
> > -		devres_add(dev, ptr);
> > -	} else
> > -		devres_free(ptr);
> > -
> > -	return addr;
> > +	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_WC);
> >   }
> >   EXPORT_SYMBOL(devm_ioremap_wc);
> >