Re: [PATCH v2 1/3] input: keyboad: imx: add snvs power key driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Wed, May 13, 2015 at 02:57:00PM -0500, Zhi Li wrote:
> On Wed, May 13, 2015 at 2:44 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Wed, May 13, 2015 at 02:27:46PM -0500, Zhi Li wrote:
> >> On Wed, May 13, 2015 at 2:10 PM, Joshua Clayton
> >> <stillcompiling@gmail.com> wrote:
> >> >
> >> > Hi Frank.
> >> >
> >> > On Wednesday, May 13, 2015 01:27:39 PM Zhi Li wrote:
> >> >> On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov
> >> >>
> >> >> <dmitry.torokhov@gmail.com> wrote:
> >> >> > Hi Frank,
> >> >> >
> >> >
> >> >> >
> >> >> >> +     pdata->ioaddr = of_iomap(np, 0);
> >> >> >> +     if (IS_ERR(pdata->ioaddr))
> >> >> >> +             return PTR_ERR(pdata->ioaddr);
> >> >> >
> >> >> > Umm, you are still leaking it on error path. Can you try doing:
> >> >> >         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> >> >         pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res);
> >> >> >         if (IS_ERR(pdata->ioaddr))
> >> >> >
> >> >> >                 return PTR_ERR(pdata->ioaddr);
> >> >>
> >> >> Thank you for comments.
> >> >> but I don't use of_iomap here because the resource is shared with the
> >> >> other device.
> >> >>
> >> >> SNVS included a rtc, a power off, ON/OFF key.
> >> >> If I use platform_get_resource and devm_ioremap_resource, rtc driver
> >> >> fail to probe.
> >> >>
> >> >> best regards
> >> >> Frank Li
> >> >>
> >> >
> >> > The fact that you cannot use of_iomap() seems a clear warning of unsafe
> >> > access.
> >> > Since it is a shared resource, which you are writing to as well as reading,
> >> > perhaps you should set it up for safe access using the  regmap API like the
> >> > GPR registers.
> >> > /include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> >>
> >> I read driver again clearfully. It is safed shared with rtc driver directly.
> >> Because pwr key driver just read a irq status register and other one
> >> register, which used only in this driver.
> >>
> >> Just w1c (write 1 clear) register. it is atomic access by hardware.
> >
> > It does not really matter, driver has to claim resources it is using to
> > make sure other parties are not accessing its registers. If registers
> > are shared you need to have a 3rd party mediator.
> 
> I can change to use syscon.
> but other two driver(rtc and power off) need change also.

Makes sense.

> 
> >
> >>
> >> PowerKey use difference irq number with RTC part.
> >
> > Yes and so they can run simultaneously and step on each other's feet,
> > right?
> 
> Yes.
> 
> >
> > Thanks.
> >
> > --
> > Dmitry

-- 
Dmitry