| Message ID | 20140909192503.19023.33476@quantum (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@collabora.com> >>> Tested-by: Boris Brezillon <boris.brezillon@free-electrons.com> >>> Tested-by: Heiko Stuebner <heiko@sntech.de> >>> Acked-by: Boris Brezillon <boris.brezillon@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. 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
On 10 September 2014 10:38, Tomeu Vizoso <tomeu.vizoso@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@collabora.com> >>>> Tested-by: Boris Brezillon <boris.brezillon@free-electrons.com> >>>> Tested-by: Heiko Stuebner <heiko@sntech.de> >>>> Acked-by: Boris Brezillon <boris.brezillon@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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
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; };