Re: [PATCH v3 04/15] ARM: at91: replace at91sam9_ddrsdr.h with at91bootstrap's at91_ddrsdrc.h

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 2/4/19 21:18, Sam Ravnborg wrote:
> Hi Ahmad.
> 
>>>
>>> If we introduced the following small helpers we could simplify the call sites
>>> to just use at91_get_ddram_size(AT91SAM9G45_BASE_DDRSDRC0) and similar.
>>>
>>> static inline u32 at91_get_ddram_size(void __force __iomem * addr)
>>> {
>>> 	return __at91_get_ddram_size(IOMEM(addr), true);
>>> }
>>>
>>> static inline u32 at91sam9g45_get_ddram_size(void __force __iomem * addr)
>>> {
>>> 	return __at91_get_ddram_size(IOMEM(addr), false);
>>> }
>>
>> That would still require the pointer cast to pacify the -Wint-conversion.
>> And (void*)ADDR looks only marginally better IOMEM(ADDR) IMO
>> at the cost of having sparse miss passing memory pointers
>> (__force __iomem would be equivalent to just __attribute__((noderef)), wouldn't it?)
>>
>> My preference would've been that AT91SAM9G45_BASE_DDRSDRC0 already expands
>> to a void __iomem *.
> The main idea was to get rid of the bool argument and use more explicit function
> names. Moving IOMEM() was the added extra.

Ah, I see now what you mean. I'll respin the series anyway, so I'll revert to
what I did in the previous series version (SoC-family-specific helper).

> 
>> We could do that in a follow-up patch.
> Agreed, maybe later if we do it.
> The original patch in this mail was fine.
> 
> You can add my:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> If you respin the series.
> 
> 	Sam
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox