Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932140AbcKGNzW (ORCPT ); Mon, 7 Nov 2016 08:55:22 -0500 Received: from eusmtp01.atmel.com ([212.144.249.243]:18959 "EHLO eusmtp01.atmel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753056AbcKGNzP (ORCPT ); Mon, 7 Nov 2016 08:55:15 -0500 Subject: Re: [PATCH] spi: atmel: use managed resource for gpio chip select To: Geert Uytterhoeven References: <20160921075528.14452-1-nicolas.ferre@atmel.com> CC: , Mark Brown , Alexandre Belloni , Boris BREZILLON , linux-spi , Ludovic Desroches , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Cyrille Pitchen , Wenyou Yang From: Nicolas Ferre Organization: atmel Message-ID: <3f5b93d7-ee40-bb1a-2cfb-49fa193e2b9d@atmel.com> Date: Mon, 7 Nov 2016 14:54:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.145.133.18] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2636 Lines: 74 Le 21/09/2016 à 10:20, Geert Uytterhoeven a écrit : > Hi Nicolas, > > On Wed, Sep 21, 2016 at 9:55 AM, Nicolas Ferre wrote: >> Use the managed gpio CS pin request so that we avoid having trouble >> in the cleanup code. >> In fact, if module was configured with DT, cleanup code released >> invalid pin. Since resource wasn't freed, module cannot be reinserted. >> >> Reported-by: Alexander Morozov >> Signed-off-by: Nicolas Ferre >> --- >> drivers/spi/spi-atmel.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c >> index 8feac599e9ab..4e3f2345844a 100644 >> --- a/drivers/spi/spi-atmel.c >> +++ b/drivers/spi/spi-atmel.c >> @@ -1248,7 +1248,8 @@ static int atmel_spi_setup(struct spi_device *spi) >> return -ENOMEM; >> >> if (as->use_cs_gpios) { >> - ret = gpio_request(npcs_pin, dev_name(&spi->dev)); >> + ret = devm_gpio_request(&spi->dev, >> + npcs_pin, dev_name(&spi->dev)); > > Note that spi_master.setup() can be called multiple times during the lifetime > of the spi_device. Sure, this is what I read in include/linux/spi/spi.h "It's always safe to call this unless transfers are pending on the device whose settings are being modified." It also means that the whole memory allocation for devices that is done a few lines above this gpio request is also completely wrong... This function needs a serious refactoring. Thanks for the heads-up. Best regards, >> if (ret) { >> kfree(asd); >> return ret; >> @@ -1471,13 +1472,11 @@ static int atmel_spi_transfer_one_message(struct spi_master *master, >> static void atmel_spi_cleanup(struct spi_device *spi) >> { >> struct atmel_spi_device *asd = spi->controller_state; >> - unsigned gpio = (unsigned long) spi->controller_data; >> >> if (!asd) >> return; >> >> spi->controller_state = NULL; >> - gpio_free(gpio); >> kfree(asd); >> } > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- Nicolas Ferre