On Mon, Jul 25, 2011 at 12:13, Felipe Balbi <balbi@xxxxxx> wrote: [..] >> > while at that, SR's IRQ is never freed on exit path, could fix it while >> > you're already there ? >> >> This is not really related to this patch is it? IMHO IRQ handling is > > I didn't say to put it on the same patch ;-) I meant that while at that, > you could add that simple fix before this patch ;-) Not really that simple - it is just one part of the equation, my point being - if we are cleaning up, we better cleanup completely on that thread at least. > >> broken badly. Current support is for SmartReflex class3 - which does >> not use the IRQ, Class2 and Class1.5 use it, but the current code >> requires major fixes which I dont intend to support in this series. > > And that's exactly what I mean. IMHO it's far better to fix the mess > before adding more stuff, otherwise it just becomes an even bigger mess, > even more difficult to fix in the long run. We've seen that with GPIO > and sDMA drivers _at_least_ ;-( I tried pushing the cleanups in my series http://marc.info/?l=linux-omap&m=129933897910785&w=2 few of them did go through and I have since personally lost interest and depending on my next free slot (not forthcoming for next few months), I might want to retry it, but I guess there is more interest in turning things into regulators than add new code. I am ok if folks want to drop this patch - like previously, things tend to get forgotten.. > >> >> @@ -998,10 +1020,75 @@ static int __devexit omap_sr_remove(struct platform_device *pdev) >> >> return 0; >> >> } >> >> >> >> +static int omap_sr_suspend(struct device *dev) >> >> +{ >> >> + struct omap_sr_data *pdata; >> >> + struct omap_sr *sr_info; >> >> + >> >> + pdata = dev_get_platdata(dev); >> > >> > I'm not sure you need to use platform data here... >> see below >> > >> >> + if (!pdata) { >> >> + dev_err(dev, "%s: platform data missing\n", __func__); >> >> + return -EINVAL; >> >> + } >> >> + >> >> + sr_info = _sr_lookup(pdata->voltdm); >> > >> > this field is held on struct omap_sr. Can't you: >> > >> > struct omap_sr *info = dev_get_drvdata(dev); >> > >> > ?? I see that a platform_set_drvdata() is missing from the driver, but >> > maybe you should add that instead of accessing platform_data. >> omap_sr_data is added in arch/arm/mach-omap2/sr_device.c >> >> With the current handling - it needs sr_data from which sr_info is >> pulled out. in the current implementation, sr_data contains the voltdm >> pointer from which sr_info is pulled out. > > but sr_info is allocated on probe() isn't it ? if you add > platform_set_drvdata(pdev, sr_info) on probe, you won't need sr_data to > fetch sr_info, all you need is to use dev_get_drvdata(dev). Am I missing > something ? sr_info - I am not debating that this is not possible - I am just explaining how it is being done now. I just dont have the bandwidth to kill all evils in smartreflex driver. :( Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
