Re: [RE-SEND] [PATCH 3/4] s3c-fb: Add support EXYNOS4 FIMD

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

Hi Thomas,

On 06/15/2011 08:14 AM, Thomas Abraham wrote:
> Hi Sylwester,
> 
> On 11 June 2011 22:10, Sylwester Nawrocki <snjw23@gmail.com> wrote:
>> Hello,
>>
>> On 06/10/2011 10:15 AM, Anand Kumar N wrote:
>>> From: Jonghun Han<jonghun.han@samsung.com>
>>>
>>> This patch adds struct s3c_fb_driverdata s3c_fb_data_exynos4 for EXYNOS4.
>>> The clk_type is added to distinguish clock type in it and lcd_clk is added
>>> in structure s3c_fb to calculate divider for lcd panel.
> 
> <snip>
> 
>>> +     default:
>>> +             dev_err(dev, "failed to enable clock for FIMD\n");
>>>               goto err_sfb;
>>>       }
>>
>> I'm not really a clock expert, but IMHO there is an issue in Thomas'
>> migration to clkdev proposal [1] that the device clock connection ids
>> (con_id) and clock names related to them are identical. Mostly it works
>> but in situation like this one it is not possible to remap two clocks
>> of different names onto a single connection id.
>>
>> For instance, looking at arch/arm/mach-omap2/clock3xxx_data.c:
>>
>> static struct clk i2c3_fck = {
>>        .name           = "i2c3_fck",
>>                           ^^^^^^^^^^
>>        .ops            = &clkops_omap2_dflt_wait,
>>        .parent         = &core_96m_fck,
>>        ...
>> };
>> static struct clk i2c2_fck = {
>>        .name           = "i2c2_fck",
>>                          ^^^^^^^^^^^
>>        .ops            = &clkops_omap2_dflt_wait,
>>        ...
>> };
>>
>> static struct omap_clk omap3xxx_clks[] = {
>>        ...
>>        CLK("omap_i2c.3", "fck", &i2c3_fck, CK_3XXX),
>>                          ^^^^^^^^^^^^^^^^^
>>        CLK("omap_i2c.2", "fck", &i2c2_fck, CK_3XXX),
>>                          ^^^^^^^^^^^^^^^^^
>>        ...
>>
>> Different clock names (i2c3_fck, i2c3_fck,..) are mapped to single
>> unified id (fck), which the driver only needs to care about.
>> The name is common across H/W instances and also SoC variants.
>> So it doesn't have to be driver's business to resolve clock differences.
>>
>> The same (con_id) name can be used on distinct SoC version providing
>> similar/same peripheral device IP.
>> It's aeasy to figure out by just comparing omap3xxx_clks[] and
>> omap44xx_clks arrays.
> 
> Sorry, I am not quite sure if I understand the problem here. For
> instance, all Samsung SoC's and each instance of i2c device in a SoC,
> use the same con_id for the i2c 'struct clock' instance. The con_id is
> 'i2c'. The i2c driver uses the con_id 'i2c' to lookup the 'struct
> clock' instance and it works for all Samsung SoC's and all instances
> of i2c device in the SoC.
> 
> Is your point different than what I understand? Please help.

I appreciate your efforts in converting all Samsung platforms to clkdev.

With the above I2C example I was trying to illustrate the additional
level of indirection that is present in case of OMAP clock mappings,
comparing to your implementation.
As long as IP clock names are same among various SoC there is no issue
at all with your clk lookup approach.
But this is already not the case with LCD controller IP. 
On S3C series the IP bus clock name is "lcd" while on S5P series it's "fimd".
There is a common framebuffer driver for S3C and S5P SoCs.
It just does not look right to me to be adding in the driver switch/case
for the clock names. I thought clkdev is meant to resolve such differences.

Then how it would be possible to map those clocks into single con_id with 
your patches? E.g.
 
S3C clk | S5P  clk | con_id
---------------------------
 "lcd"  | "fimd"   | "lcd" ?

I believe there will come more differences like that in the future.

Regards,
Sylwester
 
> Thanks,
> Thomas.
>>
>> That said I wouldn't go for a "devname" in struct clk, just create
>> an additional table matching device names, con_id and struct clk and
>> use it while registering clk_lookup items into clkdev.
...
>> [1] http://www.spinics.net/lists/arm-kernel/msg127901.html
>>    http://www.spinics.net/lists/arm-kernel/msg127907.html