On Friday, July 20, 2012 7:00 PM, Sylwester Nawrocki 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/ ? Yes, you're right. I think that these files need to be merged to one file. Also, 'include/video/' seems to be good. > > 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 ? > > -- > > Regards, > Sylwester > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |