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

[Jingoo Han <jg1.han@samsung.com>]

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html