From: "Han, Nandor (GE Healthcare)" <nandor.han@ge.com>
To: Linus Walleij <linus.walleij@linaro.org>
CC: Alexandre Courbot <gnurou@gmail.com>, Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Malinen, Semi (GE Healthcare)" <semi.malinen@ge.com>
Thread-Topic: Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver
Thread-Index: AQHSpsKisO+GNwMHKEiVTGcNOcZLiaGrFNgAgAuzFXCAAvgRAIABZ1fA
Date: Sat, 8 Apr 2017 07:38:50 +0000
Message-ID: <HE1P101MB01883D37543C63B3B020F91DE60F0@HE1P101MB0188.NAMP101.PROD.OUTLOOK.COM>
References: <cover.1490595641.git.nandor.han@ge.com>
 <6bf04ba1761f0692cb461558f0c8836f0d1f7ad8.1490595641.git.nandor.han@ge.com>
 <CACRpkdZB_cxqw0=M4YOhcwdZe7BxJtVKA290yGH-jxPU2MDgBg@mail.gmail.com>
 <HE1P101MB01883B516828F26921267A4FE60A0@HE1P101MB0188.NAMP101.PROD.OUTLOOK.COM>
 <CACRpkdYZVy2t9=K0jVm5=BxhWWUwiNsGDTMVBbSiB9+1+uafJQ@mail.gmail.com>
In-Reply-To: <CACRpkdYZVy2t9=K0jVm5=BxhWWUwiNsGDTMVBbSiB9+1+uafJQ@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Apr 2017 07:38:50.2143
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 15ccb6d1-d335-4996-b6f9-7b6925f08121
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1P101MB0188
X-OriginatorOrg: ge.com
Subject: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-04-08_04:,,
 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0
 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam
 adjust=0 reason=mlx scancount=1 engine=8.0.1-1702020001
 definitions=main-1704080070
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v387dJbU005166
Content-Length: 2242
Lines: 53


> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: 07 April 2017 13:07
> To: Han, Nandor (GE Healthcare) <nandor.han@ge.com>
> Cc: Alexandre Courbot <gnurou@gmail.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>; linux-gpio@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> Malinen, Semi (GE Healthcare) <semi.malinen@ge.com>
> Subject: EXT: Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver
> 
> On Wed, Apr 5, 2017 at 3:24 PM, Han, Nandor (GE Healthcare)
> <nandor.han@ge.com> wrote:
> > [Me]
> >> > +       /* bring the chip out of reset */
> >> > +       reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
> >> > +       if (IS_ERR(reset_gpio))
> >> > +               dev_warn(&spi->dev, "could not get reset-gpios\n");
> >> > +       else if (reset_gpio)
> >> > +               gpiod_put(reset_gpio);
> >>
> >> I don't think you should put it, other than in the remove()
> >> function and in that case you need to have it in the
> >> state container.
> >
> > Can you please be more explicit here.
> >
> > Currently I'm trying to bring the device out from reset in case reset GPIO is provided.
> > I don't see how this could be done in remove()  :)
> 
> If you issue gpiod_put() you release the GPIO hande so something else
> can go in and grab the GPIO and assert the reset.
> 
> This is not what you want to make possible: you want to hold this gpiod handle
> as long as the driver is running. devm_gpiod_get_optional() will do the
> trick if you don't want to put the handle under explicit control.
> 

That was my first intention to release the reset line in case somebody else wants to control it. I did it
like that because usually reset line controls multiple devices and probably some upper layer wants to
control that. 

After your comment I did some analysing and I will follow your advice and change the reset line handling. 
Once the GPIO is provided to the driver the driver will own it and bring out the device from reset. In case not
provided the reset line is somebody else responsibility. 

This way we are able to cover multiple use-cases. 

Thanks Linus,
Nandy

> Yours,
> Linus Walleij