| Message ID | E1YSTnC-0001JU-CX@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Russell, On Monday 02 March 2015 17:06:06 Russell King wrote: > No merged platform supplies xclks via platform data. As we want to > slightly change the clkdev interface, rather than fixing this unused > code, remove it instead. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> There are quite a few out of tree users that I know of that might be impacted. On the other hand, out of tree isn't an excuse, and OMAP3 platforms should move to DT. The good news is that DT support for the omap3isp driver is about to get submitted, and hopefully merged in v4.1. I thus have no objection to this patch. Sakari, does it conflict with the omap3isp DT support ? If so, how would you prefer to resolve the conflict ? Russell, would it be fine to merge this through Mauro's tree ? Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/omap3isp/isp.c | 18 ------------------ > drivers/media/platform/omap3isp/isp.h | 1 - > include/media/omap3isp.h | 6 ------ > 3 files changed, 25 deletions(-) > > diff --git a/drivers/media/platform/omap3isp/isp.c > b/drivers/media/platform/omap3isp/isp.c index deca80903c3a..4d8078b9d010 > 100644 > --- a/drivers/media/platform/omap3isp/isp.c > +++ b/drivers/media/platform/omap3isp/isp.c > @@ -281,7 +281,6 @@ static const struct clk_init_data isp_xclk_init_data = { > > static int isp_xclk_init(struct isp_device *isp) > { > - struct isp_platform_data *pdata = isp->pdata; > struct clk_init_data init; > unsigned int i; > > @@ -311,20 +310,6 @@ static int isp_xclk_init(struct isp_device *isp) > xclk->clk = clk_register(NULL, &xclk->hw); > if (IS_ERR(xclk->clk)) > return PTR_ERR(xclk->clk); > - > - if (pdata->xclks[i].con_id == NULL && > - pdata->xclks[i].dev_id == NULL) > - continue; > - > - xclk->lookup = kzalloc(sizeof(*xclk->lookup), GFP_KERNEL); > - if (xclk->lookup == NULL) > - return -ENOMEM; > - > - xclk->lookup->con_id = pdata->xclks[i].con_id; > - xclk->lookup->dev_id = pdata->xclks[i].dev_id; > - xclk->lookup->clk = xclk->clk; > - > - clkdev_add(xclk->lookup); > } > > return 0; > @@ -339,9 +324,6 @@ static void isp_xclk_cleanup(struct isp_device *isp) > > if (!IS_ERR(xclk->clk)) > clk_unregister(xclk->clk); > - > - if (xclk->lookup) > - clkdev_drop(xclk->lookup); > } > } > > diff --git a/drivers/media/platform/omap3isp/isp.h > b/drivers/media/platform/omap3isp/isp.h index cfdfc8714b6b..d41c98bbdfe7 > 100644 > --- a/drivers/media/platform/omap3isp/isp.h > +++ b/drivers/media/platform/omap3isp/isp.h > @@ -122,7 +122,6 @@ enum isp_xclk_id { > struct isp_xclk { > struct isp_device *isp; > struct clk_hw hw; > - struct clk_lookup *lookup; > struct clk *clk; > enum isp_xclk_id id; > > diff --git a/include/media/omap3isp.h b/include/media/omap3isp.h > index 398279dd1922..a9798525d01e 100644 > --- a/include/media/omap3isp.h > +++ b/include/media/omap3isp.h > @@ -152,13 +152,7 @@ struct isp_v4l2_subdevs_group { > } bus; /* gcc < 4.6.0 chokes on anonymous union initializers */ > }; > > -struct isp_platform_xclk { > - const char *dev_id; > - const char *con_id; > -}; > - > struct isp_platform_data { > - struct isp_platform_xclk xclks[2]; > struct isp_v4l2_subdevs_group *subdevs; > void (*set_constraints)(struct isp_device *isp, bool enable); > };
Hi Laurent and Russell, On Tue, Mar 03, 2015 at 12:33:44AM +0200, Laurent Pinchart wrote: > Hi Russell, > > On Monday 02 March 2015 17:06:06 Russell King wrote: > > No merged platform supplies xclks via platform data. As we want to > > slightly change the clkdev interface, rather than fixing this unused > > code, remove it instead. > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > There are quite a few out of tree users that I know of that might be impacted. > On the other hand, out of tree isn't an excuse, and OMAP3 platforms should > move to DT. The good news is that DT support for the omap3isp driver is about > to get submitted, and hopefully merged in v4.1. I thus have no objection to > this patch. > > Sakari, does it conflict with the omap3isp DT support ? If so, how would you > prefer to resolve the conflict ? Russell, would it be fine to merge this > through Mauro's tree ? I first thought it wouldn't conflict, but apparently it does. The conflicting patches are here: <URL:http://vihersipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=commitdiff;h=a56c38208ee9200e57421b60b770fb8249935b95> <URL:http://vihersipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=commitdiff;h=72374c7a69a12afc76f220ef4de983be4583f164> <URL:http://vihersipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=commitdiff;h=0f0b86d64a555e308079f985812b011866e2c8f0> Tne entire set: <URL:http://vihersipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=shortlog;h=refs/heads/rm696-051-upstream> I haven't sent that to linux-media yet but I'll do that during the coming couple of days.
(Combining replies...) On Tue, Mar 03, 2015 at 12:53:37AM +0200, Sakari Ailus wrote: > Hi Laurent and Russell, > > On Tue, Mar 03, 2015 at 12:33:44AM +0200, Laurent Pinchart wrote: > > Sakari, does it conflict with the omap3isp DT support ? If so, how would you > > prefer to resolve the conflict ? Russell, would it be fine to merge this > > through Mauro's tree ? As other changes will depend on this, I'd prefer not to. The whole "make clk_get() return a unique struct clk" wasn't well tested, and several places broke - and currently clk_add_alias() is broken as a result of that. I'm trying to get to the longer term solution, where clkdev internally uses a struct clk_hw pointer rather than a struct clk pointer, and I want to clean stuff up first. If omap3isp needs to keep this code, then so be it - I'll come up with a different patch improving its use of clkdev instead.
Hi Russell, On Monday 02 March 2015 23:54:35 Russell King - ARM Linux wrote: > (Combining replies...) > > On Tue, Mar 03, 2015 at 12:53:37AM +0200, Sakari Ailus wrote: > > Hi Laurent and Russell, > > > > On Tue, Mar 03, 2015 at 12:33:44AM +0200, Laurent Pinchart wrote: > > > Sakari, does it conflict with the omap3isp DT support ? If so, how would > > > you prefer to resolve the conflict ? Russell, would it be fine to merge > > > this through Mauro's tree ? > > As other changes will depend on this, I'd prefer not to. The whole > "make clk_get() return a unique struct clk" wasn't well tested, and > several places broke - and currently clk_add_alias() is broken as a > result of that. > > I'm trying to get to the longer term solution, where clkdev internally > uses a struct clk_hw pointer rather than a struct clk pointer, and I > want to clean stuff up first. > > If omap3isp needs to keep this code, then so be it - I'll come up with > a different patch improving its use of clkdev instead. I'm totally fine with removing clkdev from the omap3isp driver if that's easier for you, I'm just concerned about the merge conflict that will result.
Hi Laurent, On Tue, Mar 03, 2015 at 02:39:14AM +0200, Laurent Pinchart wrote: > Hi Russell, > > On Monday 02 March 2015 23:54:35 Russell King - ARM Linux wrote: > > (Combining replies...) > > > > On Tue, Mar 03, 2015 at 12:53:37AM +0200, Sakari Ailus wrote: > > > Hi Laurent and Russell, > > > > > > On Tue, Mar 03, 2015 at 12:33:44AM +0200, Laurent Pinchart wrote: > > > > Sakari, does it conflict with the omap3isp DT support ? If so, how would > > > > you prefer to resolve the conflict ? Russell, would it be fine to merge > > > > this through Mauro's tree ? > > > > As other changes will depend on this, I'd prefer not to. The whole > > "make clk_get() return a unique struct clk" wasn't well tested, and > > several places broke - and currently clk_add_alias() is broken as a > > result of that. > > > > I'm trying to get to the longer term solution, where clkdev internally > > uses a struct clk_hw pointer rather than a struct clk pointer, and I > > want to clean stuff up first. > > > > If omap3isp needs to keep this code, then so be it - I'll come up with > > a different patch improving its use of clkdev instead. > > I'm totally fine with removing clkdev from the omap3isp driver if that's > easier for you, I'm just concerned about the merge conflict that will result. There actually appears to be one more dependency, so four patches in total. What we could also do is to rebase the omap3isp DT support set on top of Russell's single patch. This way there probably would be no merge conflict, assuming the patches are exactly the same, and you have no other patches changing the omap3isp driver. Alternatively we could postpone the DT support for the omap3isp, but I'd rather want to avoid that.
Hi Russell, On Mon, Mar 02, 2015 at 11:54:35PM +0000, Russell King - ARM Linux wrote: > (Combining replies...) > > On Tue, Mar 03, 2015 at 12:53:37AM +0200, Sakari Ailus wrote: > > Hi Laurent and Russell, > > > > On Tue, Mar 03, 2015 at 12:33:44AM +0200, Laurent Pinchart wrote: > > > Sakari, does it conflict with the omap3isp DT support ? If so, how would you > > > prefer to resolve the conflict ? Russell, would it be fine to merge this > > > through Mauro's tree ? > > As other changes will depend on this, I'd prefer not to. The whole > "make clk_get() return a unique struct clk" wasn't well tested, and > several places broke - and currently clk_add_alias() is broken as a > result of that. > > I'm trying to get to the longer term solution, where clkdev internally > uses a struct clk_hw pointer rather than a struct clk pointer, and I > want to clean stuff up first. > > If omap3isp needs to keep this code, then so be it - I'll come up with > a different patch improving its use of clkdev instead. I discussed this with Laurent and the two options we thought are - You provide a stable branch on which I can rebase the patches, in order to avoid a merge conflict or - We ignore the conflict and let Stephen Rothwell handle it. The conflict itself is relatively simple to resolve.
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c index deca80903c3a..4d8078b9d010 100644 --- a/drivers/media/platform/omap3isp/isp.c +++ b/drivers/media/platform/omap3isp/isp.c @@ -281,7 +281,6 @@ static const struct clk_init_data isp_xclk_init_data = { static int isp_xclk_init(struct isp_device *isp) { - struct isp_platform_data *pdata = isp->pdata; struct clk_init_data init; unsigned int i; @@ -311,20 +310,6 @@ static int isp_xclk_init(struct isp_device *isp) xclk->clk = clk_register(NULL, &xclk->hw); if (IS_ERR(xclk->clk)) return PTR_ERR(xclk->clk); - - if (pdata->xclks[i].con_id == NULL && - pdata->xclks[i].dev_id == NULL) - continue; - - xclk->lookup = kzalloc(sizeof(*xclk->lookup), GFP_KERNEL); - if (xclk->lookup == NULL) - return -ENOMEM; - - xclk->lookup->con_id = pdata->xclks[i].con_id; - xclk->lookup->dev_id = pdata->xclks[i].dev_id; - xclk->lookup->clk = xclk->clk; - - clkdev_add(xclk->lookup); } return 0; @@ -339,9 +324,6 @@ static void isp_xclk_cleanup(struct isp_device *isp) if (!IS_ERR(xclk->clk)) clk_unregister(xclk->clk); - - if (xclk->lookup) - clkdev_drop(xclk->lookup); } } diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h index cfdfc8714b6b..d41c98bbdfe7 100644 --- a/drivers/media/platform/omap3isp/isp.h +++ b/drivers/media/platform/omap3isp/isp.h @@ -122,7 +122,6 @@ enum isp_xclk_id { struct isp_xclk { struct isp_device *isp; struct clk_hw hw; - struct clk_lookup *lookup; struct clk *clk; enum isp_xclk_id id; diff --git a/include/media/omap3isp.h b/include/media/omap3isp.h index 398279dd1922..a9798525d01e 100644 --- a/include/media/omap3isp.h +++ b/include/media/omap3isp.h @@ -152,13 +152,7 @@ struct isp_v4l2_subdevs_group { } bus; /* gcc < 4.6.0 chokes on anonymous union initializers */ }; -struct isp_platform_xclk { - const char *dev_id; - const char *con_id; -}; - struct isp_platform_data { - struct isp_platform_xclk xclks[2]; struct isp_v4l2_subdevs_group *subdevs; void (*set_constraints)(struct isp_device *isp, bool enable); };
No merged platform supplies xclks via platform data. As we want to slightly change the clkdev interface, rather than fixing this unused code, remove it instead. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/media/platform/omap3isp/isp.c | 18 ------------------ drivers/media/platform/omap3isp/isp.h | 1 - include/media/omap3isp.h | 6 ------ 3 files changed, 25 deletions(-)