Re: [PATCH 2/2 V2] OMAP3+: PM: SR: add suspend/resume handlers

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3551 bytes --]

Hi,

On Mon, Jul 25, 2011 at 12:55:39PM -0500, Menon, Nishanth wrote:
> On Mon, Jul 25, 2011 at 12:13, Felipe Balbi <balbi@ti.com> 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.

fair enough :-)

> >> 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..

I didn't really say that, but ok ;-)

> >> >> @@ -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. :(

I see... ok then, I'll see if I find some time to play with that, should
be kinda fun - not. Is this the only patch pending for that driver ?
Should I base off of v3.0 and everything should be ok ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]