Hi Sylwester,
On 11 June 2011 22:10, Sylwester Nawrocki <snjw23@xxxxxxxxx> wrote:
> Hello,
>
> On 06/10/2011 10:15 AM, Anand Kumar N wrote:
>> From: Jonghun Han<jonghun.han@xxxxxxxxxxx>
>>
>> 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 versionproviding 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.
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.
>
>>
>> - clk_enable(sfb->bus_clk);
>> -
>> pm_runtime_enable(sfb->dev);
>>
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> if (!res) {
>> dev_err(dev, "failed to find registers\n");
>> ret = -ENOENT;
>> - goto err_clk;
>> + goto err_lcd_clk;
>> }
>>
>> sfb->regs_res = request_mem_region(res->start, resource_size(res),
>> @@ -1369,7 +1417,7 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
>> if (!sfb->regs_res) {
>> dev_err(dev, "failed to claim register region\n");
>> ret = -ENOENT;
>> - goto err_clk;
>> + goto err_lcd_clk;
>> }
>>
>> sfb->regs = ioremap(res->start, resource_size(res));
>> @@ -1451,9 +1499,15 @@ err_ioremap:
>> err_req_region:
>> release_mem_region(sfb->regs_res->start, resource_size(sfb->regs_res));
>>
>> -err_clk:
>> - clk_disable(sfb->bus_clk);
>> - clk_put(sfb->bus_clk);
>> +err_lcd_clk:
>> + clk_disable(sfb->lcd_clk);
>> + clk_put(sfb->lcd_clk);
>> +
>> +err_bus_clk:
>> + if (sfb->variant.clk_type != FIMD_CLK_TYPE0) {
>> + clk_disable(sfb->bus_clk);
>> + clk_put(sfb->bus_clk);
>> + }
>>
>> err_sfb:
>> kfree(sfb);
>> @@ -1482,8 +1536,20 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
>>
>> iounmap(sfb->regs);
>>
>> - clk_disable(sfb->bus_clk);
>> - clk_put(sfb->bus_clk);
>> + switch (sfb->variant.clk_type) {
>> + case FIMD_CLK_TYPE1:
>> + clk_disable(sfb->lcd_clk);
>> + clk_put(sfb->lcd_clk);
>> + /* fall through to default case */
>> + case FIMD_CLK_TYPE0:
>> + clk_disable(sfb->bus_clk);
>> + clk_put(sfb->bus_clk);
>> + break;
>> + default:
>
> Considering clk_type data type this is unreachable code.
>
>> + dev_err(sfb->dev, "invalid clock type(%d)\n",
>> + sfb->variant.clk_type);
>> + break;
>> + }
> ...
>
> --
> Regards,
> Sylwester
>
> [1] http://www.spinics.net/lists/arm-kernel/msg127901.html
> http://www.spinics.net/lists/arm-kernel/msg127907.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
>
--
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
