[PATCH v10 2/9] clk: Move all drivers to use internal API
Tomeu Vizoso
tomeu.vizoso at collabora.com
Wed Sep 10 06:20:16 PDT 2014
On 10 September 2014 10:38, Tomeu Vizoso <tomeu.vizoso at collabora.com> wrote:
> On 09/09/2014 09:25 PM, Mike Turquette wrote:
>> Quoting Mike Turquette (2014-09-09 12:12:05)
>>> Quoting Tomeu Vizoso (2014-09-09 07:04:57)
>>>> In preparation to change the public API to return a per-user clk structure,
>>>> remove any usage of this public API from the clock implementations.
>>>>
>>>> The reason for having this in a separate commit from the one that introduces
>>>> the implementation of the new functions is to separate the changes generated
>>>> with Coccinelle from the rest, and keep the patches' size reasonable.
>>>>
>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso at collabora.com>
>>>> Tested-by: Boris Brezillon <boris.brezillon at free-electrons.com>
>>>> Tested-by: Heiko Stuebner <heiko at sntech.de>
>>>> Acked-by: Boris Brezillon <boris.brezillon at free-electrons.com>
>>>>
>>>> ---
>>>>
>>>> v10: * Add a few more files to be converted
>>>> * Re-generate the patch on top of the latest changes
>>>
>>> Hi Tomeu,
>>>
>>> Generating this on top of linux-next is a no-go. I can't apply it to my
>>> tree. The best thing is to generate it on top of -rc4, and that is what
>>> I will merge.
>>>
>>> Running the script against linux-next is still very useful and lets us
>>> patch up the stuff that is not going through the clk tree. E.g. the LPSS
>>> driver is already in mainline, so just running the semantic patch
>>> against -rc4 is sufficient for it. However a patch like Shawn's "ARM:
>>> imx: add an exclusive gate clock type" came in through the i.MX tree and
>>> we'll need to patch it after the fact.
>>>
>>> The best way to do that is for me to host a branch with just your
>>> changes in it that everyone can pull in as a dependency with the same
>>> commit ids.
>>>
>>> <snip>
>>>
>>>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>>>> index bcbdbd2..f4c6ccf 100644
>>>> --- a/drivers/acpi/acpi_lpss.c
>>>> +++ b/drivers/acpi/acpi_lpss.c
>>>> @@ -11,7 +11,6 @@
>>>> */
>>>>
>>>> #include <linux/acpi.h>
>>>> -#include <linux/clk.h>
>>>> #include <linux/clkdev.h>
>>>> #include <linux/clk-provider.h>
>>>> #include <linux/err.h>
>>>> @@ -78,7 +77,7 @@ struct lpss_private_data {
>>>> void __iomem *mmio_base;
>>>> resource_size_t mmio_size;
>>>> unsigned int fixed_clk_rate;
>>>> - struct clk *clk;
>>>> + struct clk_core *clk;
>>>> const struct lpss_device_desc *dev_desc;
>>>> u32 prv_reg_ctx[LPSS_PRV_REG_COUNT];
>>>> };
>>>> @@ -229,7 +228,7 @@ static int register_device_clock(struct acpi_device *adev,
>>>> {
>>>> const struct lpss_device_desc *dev_desc = pdata->dev_desc;
>>>> const char *devname = dev_name(&adev->dev);
>>>> - struct clk *clk = ERR_PTR(-ENODEV);
>>>> + struct clk_core *clk = ERR_PTR(-ENODEV);
>>>> struct lpss_clk_data *clk_data;
>>>> const char *parent, *clk_name;
>>>> void __iomem *prv_base;
>>>
>>> I think the following hunk is missing from your change:
>>>
>>> --- a/drivers/acpi/acpi_lpss.c
>>> +++ b/drivers/acpi/acpi_lpss.c
>>> @@ -57,7 +57,7 @@ ACPI_MODULE_NAME("acpi_lpss");
>>> struct lpss_shared_clock {
>>> const char *name;
>>> unsigned long rate;
>>> - struct clk *clk;
>>> + struct clk_core *clk;
>>> };
>>
>> Looks like this hunk is missing as well:
>>
>> diff --git a/include/linux/platform_data/clk-lpss.h b/include/linux/platform_data/clk-lpss.h
>> index 2390199..3c3237c 100644
>> --- a/include/linux/platform_data/clk-lpss.h
>> +++ b/include/linux/platform_data/clk-lpss.h
>> @@ -15,7 +15,7 @@
>>
>> struct lpss_clk_data {
>> const char *name;
>> - struct clk *clk;
>> + struct clk_core *clk;
>> };
>>
>>
>>
>> Without that change the following code will explode:
>>
>>
>> static int register_device_clock(struct acpi_device *adev,
>> struct lpss_private_data *pdata)
>> {
>> const struct lpss_device_desc *dev_desc = pdata->dev_desc;
>> struct lpss_shared_clock *shared_clock = dev_desc->shared_clock;
>> const char *devname = dev_name(&adev->dev);
>> struct clk_core *clk = ERR_PTR(-ENODEV);
>> struct lpss_clk_data *clk_data;
>> const char *parent, *clk_name;
>> void __iomem *prv_base;
>>
>> if (!lpss_clk_dev)
>> lpt_register_clock_device();
>>
>> clk_data = platform_get_drvdata(lpss_clk_dev);
>> if (!clk_data)
>> return -ENODEV;
>>
>> if (dev_desc->clkdev_name) {
>> clk_register_clkdev(clk_data->clk, dev_desc->clkdev_name,
>> devname);
>> return 0;
>> }
>>
>>
>>
>> I'm starting to get nervous about this Coccinelle script... Seems like a
>> lot of things are slipping through.
>
> I understand your concern. The Coccinelle part of it is very simple, and
> so far I haven't seen any problem caused by it. The step that is manual
> and that thus is prone to errors is listing the files that need to be
> transformed (clock_impl.txt).
>
> What I do to check that there aren't more files that need to be added is
> to grep for all files that contain "clk_ops" or include
> "clk-provider.h", diff it with the existing clock_impl.txt and visually
> inspect each of the new files to see if they are indeed clock
> implementations or helpers for them.
>
> The most problematic part of the process are the headers that define
> structs that are shared between clock implementations (and sometimes
> between both clock drivers and consumers), as is this case. So far I
> have only been able to find out what headers need modification by
> building and fixing the warnings. And as you have seen, I haven't been
> able to test builds as extensively as it would have been ideal.
>
> Was hoping that Intel's 0day build farm would help finding those, but
> there seems to be some problem with it and I'm not getting any reports.
> Have just asked Fengguang about it.
Just an update on this. Fengguang manually enqueued a build of my v10
branch a few hours ago, and I'm currently running a series of build
tests locally on my v11 branch, which is rebased on top of 3.17rc4:
http://cgit.collabora.com/git/user/tomeu/linux.git/log/?h=clk-refactoring-11
Regards,
Tomeu
> In the meantime I'm going to make my build script smarter and only spend
> time checking bisectability after I'm sure that the head builds fine in
> a fair set of defconfigs for all arches.
>
> Cheers,
>
> Tomeu
>
>> Regards,
>> Mike
>>
>>>
>>>
>>> Otherwise register_device_clock will blow up because we are assigning a
>>> struct clk * to a struct clk_core *.
>>>
>>> Do you mind testing with ARCH=x86_64 and allmodconfig? That will help
>>> catch issues like this.
>>>
>>> Regards,
>>> Mike
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
More information about the linux-arm-kernel
mailing list