Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'

[Chen Gang <gang.chen.5i5j@gmail.com>]

On 07/17/2014 04:37 PM, Thierry Reding wrote:
> On Thu, Jul 17, 2014 at 09:27:58AM +0800, Chen Gang wrote:
> [...]
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index c2421e0..a7500c3 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -630,7 +630,16 @@ extern unsigned long devm_get_free_pages(struct device *dev,
>>  					 gfp_t gfp_mask, unsigned int order);
>>  extern void devm_free_pages(struct device *dev, unsigned long addr);
>>  
>> +#ifdef CONFIG_HAS_IOMEM
>>  void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
>> +#elif defined(CONFIG_COMPILE_TEST)
>> +static inline void __iomem *devm_ioremap_resource(struct device *dev,
>> +						struct resource *res)
>> +{
>> +	pr_warn("no hardware io memory, only for COMPILE_TEST\n");
> 
> Maybe: "Hardware doesn't support memory-mapped I/O"? I'm not sure if
> it's useful to include the reference to COMPILE_TEST, especially since
> the #elif will be dropped in favour of a simple #else.
> 

OK, thanks. I will use the comments which you provide.

And also use #else instead of #elif, use 'dev_warn' instead of 'pr_warn'
which Guenter suggests.

>> +	return (__force void __iomem *)ERR_PTR(-ENXIO);
> 
> There's apparently an IOMEM_ERR_PTR() for this nowadays...
> 

IOMEM_ERR_PTR() is defined within "lib/devres.c", not in "./include".
But may we move it from "lib/devres.c" to "./include/linux/err.h"?

For me, I am not quite sure, it may need additional discussion, but at
least, that will be another patch.

>> +}
>> +#endif /* CONFIG_HAS_IOMEM || CONFIG_COMPILE_TEST */
>>  
>>  /* allows to add/remove a custom action to devres stack */
>>  int devm_add_action(struct device *dev, void (*action)(void *), void *data);
>> diff --git a/include/linux/io.h b/include/linux/io.h
>> index b76e6e5..59128aa 100644
>> --- a/include/linux/io.h
>> +++ b/include/linux/io.h
>> @@ -58,14 +58,42 @@ static inline void devm_ioport_unmap(struct device *dev, void __iomem *addr)
>>  }
>>  #endif
>>  
>> +#ifdef CONFIG_HAS_IOMEM
>> +
>>  void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
>>  			    unsigned long size);
>>  void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
>>  				    unsigned long size);
>>  void devm_iounmap(struct device *dev, void __iomem *addr);
>> +void devm_ioremap_release(struct device *dev, void *res);
>> +
>> +#elif defined(CONFIG_COMPILE_TEST)
>> +
>> +static inline void __iomem *devm_ioremap(struct device *dev,
>> +				resource_size_t offset, unsigned long size)
> 
> For consistency with other functions above, please keep arguments on
> subsequent lines aligned with the first argument on the first line, like
> so:
> 
> static inline void __iomem *devm_ioremap(struct device *dev,
> 					 resource_size_t offset,
> 					 unsigned long size)
> 

That sounds fine to me, thanks.

>> +{
>> +	pr_warn("no hardware io memory, only for COMPILE_TEST\n");
>> +	return NULL;
>> +}
>> +static inline void __iomem *devm_ioremap_nocache(struct device *dev,
>> +				resource_size_t offset, unsigned long size)
>> +{
>> +	pr_warn("no hardware io memory, only for COMPILE_TEST\n");
>> +	return NULL;
>> +}
> 
> Perhaps this should call devm_ioremap() so we don't have to repeat the
> same error message? Or maybe make it:
> 
> 	#define devm_ioremap_nocache devm_ioremap
> 

That sounds fine to me, thanks.

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed