Re: [Resend Patch v6] driver/clk/clk-si5338: Add common clock framework driver for si5338

[York Sun <yorksun@freescale.com>]

On 10/19/2015 06:28 PM, York Sun wrote:
> 
> 
> On 10/19/2015 05:36 PM, Stephen Boyd wrote:
>> On 10/20, York Sun wrote:
>>> Sorry for top posting. I am on outlook web.
>>>
>>> We have no problem with device tree. Let's put it aside.
>>>
>>> When device is not used, the platform data is used to hold the data, filled by a platform device before probing the clock. For my case, the platform device is a PCIe device. It is a multifunction device with I2C controller on it. The pseudo code looks like
>>>
>>> struct i2c_board_info si5338_info[NUM_SI5338_CHIPS] = {
>>>         {
>>>                 .type = "si5338",
>>>                 .platform_data = &si5338_pdata[0],
>>>         },
>>>         {
>>>                 .type = "si5338",
>>>                 .platform_data = &si5338_pdata[1],
>>>         },
>>>         {
>>>                 .type = "si5338",
>>>                 .platform_data = &si5338_pdata[2],
>>>         },
>>>         {
>>>                 .type = "si5338",
>>>                 .platform_data = &si5338_pdata[3],
>>>         },
>>> };
>>> clk = clk_register_fixed_rate(&pdev->dev, "ref25", NULL, CLK_IS_ROOT, 25000000);
>>> for (i = 0; i < NUM_SI5338_CHIPS; i++) {
>>>     si5338_pdata[i].clk_xtal = clk;
>>>     adap = i2c_get_adapter(private->i2c_adp->nr + 1 + i);
>>>     private->i2c_client[i] = i2c_new_probed_device(adap, &si5338_info[i], i2c_si5338_addr, NULL);
>>> }
>>>
>>> You can see, when the fixed-rate clock is registered, the device id of si5338 is unknown yet. (I am using one 25MHz clock to feed multiple si5338, where maybe I should create multiple 25MHz clocks, but that's another discussion.)
>>>
>>> I hope I have made it clear.
>>
>> Yes. It would be great if we could modify the i2c framework to
>> let us create an i2c device but not call device_register() until a later
>> time. So something like this could be done in the platform
>> driver:
>>
>> 	for (i = 0; i < NUM_SI5338_CHIPS; i++) {
>> 	     adap = i2c_get_adapter(private->i2c_adp->nr + 1 + i);
>> 	     private->i2c_client[i] = i2c_new_device_unregistered(adap,
>> 	     				&si5338_info[i], i2c_si5338_addr);
>> 	     clkdev_create(clk, NULL, dev_name(private->i2c_client[i]->dev));
>> 	     device_register(private->i2c_client[i]->dev);
>> 	}
>>
>> Then in the si5338 driver we call devm_clk_get(i2c->dev, NULL)
>> and we get the xtal clock.
>>
> 
> Then what do we do before we have this i2c_new_device_unregistered()? Does my
> proposal make sense?
> 

Stephen,

How about this? I will use devm_clk_get() when parsing device tree, and still
pass clk pointer if using platform data. In this way, I don't have to call
clk_put() in the driver to release the parent clocks. The platform device driver
is responsible to get the clock and put the clock. It seems reasonable to me. If
you don't disagree, I will prepare v7 patch for review.

York