From mboxrd@z Thu Jan  1 00:00:00 1970
From: Nikita Kiryanov <nikita@compulab.co.il>
Date: Sun, 01 Feb 2015 19:02:52 +0200
Subject: [U-Boot] [PATCH 02/13] lcd: split configuration_get_cmap
In-Reply-To: <CAPnjgZ1Em3=SLL4wCR1WiqJyWsfPnaU79PG=d5RTzVap00QgYg@mail.gmail.com>
References: <1422530505-19054-1-git-send-email-nikita@compulab.co.il>	<1422530505-19054-3-git-send-email-nikita@compulab.co.il>
 <CAPnjgZ1Em3=SLL4wCR1WiqJyWsfPnaU79PG=d5RTzVap00QgYg@mail.gmail.com>
Message-ID: <54CE5C3C.6000209@compulab.co.il>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Simon,

On 01/31/2015 02:24 AM, Simon Glass wrote:
> Hi Nikita,
>
> On 29 January 2015 at 04:21, Nikita Kiryanov <nikita@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@compulab.co.il>
>> Cc: Bo Shen <voice.shen@atmel.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Anatolij Gustschin <agust@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@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

>
> Regards,
> Simon
>

-- 
Regards,
Nikita Kiryanov