[U-Boot] [PATCH 02/13] lcd: split configuration_get_cmap

[Simon Glass]

Hi Nikita,

On 1 February 2015 at 10:02, Nikita Kiryanov <nikita at compulab.co.il> wrote:
> Hi Simon,
>
>
> On 01/31/2015 02:24 AM, Simon Glass wrote:
>>
>> Hi Nikita,
>>
>> On 29 January 2015 at 04:21, Nikita Kiryanov <nikita at compulab.co.il>
>> wrote:
>>>
>>> configuration_get_cmap() is multiple platform specific functions stuffed
>>> into
>>> one function. Split it into multiple versions, and move each version to
>>> the
>>> appropriate driver to reduce the #ifdef complexity.
>>>
>>> Signed-off-by: Nikita Kiryanov <nikita at compulab.co.il>
>>> Cc: Bo Shen <voice.shen at atmel.com>
>>> Cc: Simon Glass <sjg at chromium.org>
>>> Cc: Anatolij Gustschin <agust at denx.de>
>>> ---
>>>   common/lcd.c                 | 19 -------------------
>>>   drivers/video/atmel_hlcdfb.c | 13 +++++++++++++
>>>   drivers/video/atmel_lcdfb.c  |  5 +++++
>>>   drivers/video/exynos_fb.c    |  9 +++++++++
>>>   drivers/video/mpc8xx_lcd.c   |  7 +++++++
>>>   drivers/video/pxa_lcd.c      |  6 ++++++
>>>   include/lcd.h                |  9 +++++++++
>>>   7 files changed, 49 insertions(+), 19 deletions(-)
>>
>>
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>
>> See suggestion below.
>>
>> [snip]
>>>
>>> diff --git a/include/lcd.h b/include/lcd.h
>>> index fbba6a2..838f645 100644
>>> --- a/include/lcd.h
>>> +++ b/include/lcd.h
>>> @@ -42,13 +42,17 @@ void lcd_set_flush_dcache(int flush);
>>>
>>>   #if defined CONFIG_MPC823
>>>   #include <mpc823_lcd.h>
>>> +ushort *configuration_get_cmap(void);
>>>   #elif defined(CONFIG_CPU_PXA25X) || defined(CONFIG_CPU_PXA27X) || \
>>>          defined CONFIG_CPU_MONAHANS
>>>   #include <pxa_lcd.h>
>>> +ushort *configuration_get_cmap(void);
>>>   #elif defined(CONFIG_ATMEL_LCD) || defined(CONFIG_ATMEL_HLCD)
>>>   #include <atmel_lcd.h>
>>> +ushort *configuration_get_cmap(void);
>>>   #elif defined(CONFIG_EXYNOS_FB)
>>>   #include <exynos_lcd.h>
>>> +ushort *configuration_get_cmap(void);
>>>   #else
>>>   typedef struct vidinfo {
>>>          ushort  vl_col;         /* Number of columns (i.e. 160) */
>>> @@ -60,6 +64,11 @@ typedef struct vidinfo {
>>>
>>>          void    *priv;          /* Pointer to driver-specific data */
>>>   } vidinfo_t;
>>> +
>>> +static inline ushort *configuration_get_cmap(void)
>>> +{
>>> +       return panel_info.cmap;
>>> +}
>>>   #endif
>>
>>
>> I'd argue for dropping the inline version and just having the same
>> prototype in each case.
>
>
> Are you suggesting something along the lines of:
>
> static ushort *configuration_get_cmap(void)
> {
>        return panel_info.cmap;
> }
> #endif
>
> ushort *configuration_get_cmap(void);
>
> ?
>
> This is something I considered while preparing this, but I was under the
> impression
> that this isn't well defined by the standard. However, I actually have
> confirmation
> now that it is in fact defined behavior[1], so I will submit a V2 for this
> (even though
> I expect all of the above to disappear from lcd.h in the next refactor
> series).
>
> [1]
> http://stackoverflow.com/questions/28264874/non-static-function-prototype-follows-static-function-declaration

OK, although if you are planning to get rid of this code in the next
refactor, it's fine just as you have it in v1.

Regards,
Simon