Re: [PATCH V2 1/7] ARM: SAMSUNG: add additional registers and SFR definitions for writeback

[Leela Krishna Amudala <l.krishna@samsung.com>]

Hello,

On Fri, Jul 20, 2012 at 3:30 PM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> On 07/20/2012 04:59 AM, Leela Krishna Amudala wrote:
>>>>>>> --- a/arch/arm/plat-samsung/include/plat/regs-fb-v4.h
>>>>>>> +++ b/arch/arm/plat-samsung/include/plat/regs-fb-v4.h
>>>>>>> @@ -30,9 +30,16 @@
>>>>>>>
>>>>>>>   #define VIDCON1_FSTATUS_EVEN (1<<  15)
>>>>>>>
>>>>>>>   /* Video timing controls */
>>>>>>>
>>>>>>> +#ifdef CONFIG_FB_EXYNOS_FIMD_V8
>>>>>>> +#define VIDTCON0                                (0x20010)
>>>>>>> +#define VIDTCON1                                (0x20014)
>>>>>>> +#define VIDTCON3                                (0x2001C)
>>>>>>> +#else
>>>>>>>
>>>>>>>   #define VIDTCON0                             (0x10)
>>>>>>>   #define VIDTCON1                             (0x14)
>>>>>>>   #define VIDTCON2                             (0x18)
>>>>>>>
>>>>>>> +#define VIDTCON3                             (0x1C)
>>>>>>> +#endif
>>>>>>
>>>>>> Wouldn't it break s3c-fb on SoCs with earlier FIMD versions with
>>>>>> CONFIG_FB_EXYNOS_FIMD_V8 selected? We are aiming at multi-platform ARM
>>>>>> kernels, aren't we (i.e support of both V8 and earlier FIMD in one
>>>>>> kernel)?
>>>>> Exynos5 has FIMD_V8 and other SoCs has older FIMD versions.
>>>>> As address offsets for certain registers has changed in FIMD_V8, we
>>>>> introduced these defines.
>>>>> So we have to select CONFIG_FB_EXYNOS_FIMD_V8 in case of Exynos5 SoC,
>>>>> and deselect for other SoCs.
>>>>
>>>> Yes, I'm aware of different FIMD versions in different SoCs. My point is
>>>> that it shouldn't be necessary to deselect CONFIG_FB_EXYNOS_FIMD_V8 to
>>>> support other SoCs than Exynos5, i.e. additional config options should be
>>>> incremental - should not break other setups. Ideally there should not be
>>>> any new config option for FIMD v8.
>>>>
>>>> The detection of FIMD version and selection of appropriate register offsets
>>>> should be done in the driver at runtime, based for example on
>>>> platform_device_id (see the s3c-fb driver and usage of s3c_fb_driverdata
>>>> and s3c_fb_win_variant structs).
>>>
>>> I could not agree with Tomasz Figa more.
>>> FIMD register offset should be selected at runtime.
>>>
>>> Leela Krishna Amudala,
>>> Please, don't use ugly "#ifdef".
>>>
>>> Best regards,
>>> Jingoo Han
>>>
>>
>> Yes, Each SoC having its own defconfigs (eg: exynos4_defconfig,
>> exynos5_defconfig etc.,)
>> So, CONFIG_FB_EXYNOS_FIMD_V8 will be added into exynos5_defconfig and
>> it won't affect the other SoCs.
>
> NACK.
>
> As others explained, and you don't seem to understand or you are stubborn
> enough not to change your approach, resolving hardware differences at
> compile time only is not acceptable, especially that Exynos SoCs are
> going to be DT only platforms. We shouldn't be short-sighted like this.
>
> Especially that the problem is relatively easy to solve at run-time, just
> add EXYNOS5_* register address definitions and create separate functions
> at the driver(s) touching those registers that changed on Exynos5.
>
> Or parametrize existing ones with an offset that would be stored in driver
> data passed trough struct platform_device_id::driver_data.
>
> @Jingoo: BTW, shouldn't we have
>
> plat-samsung/include/plat/regs-fb.h
> plat-samsung/include/plat/regs-fb-v4.h
>
> merged into one file and moved under include/video/ ?
>
> For example include/video/s3c-fb.h, and board files could also include
> that header. We have FIMD variant structures anyway, so why do we still
> need multiple headers ?
>

Will do the run-time approach, and post the next version patchset soon.

Thanks,
Leela Krishna Amudala

> --
>
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html