Hi Uli,
On Tue, Sep 6, 2016 at 2:41 PM, Ulrich Hecht
<ulrich.hecht+renesas@xxxxxxxxx> wrote:
> Required for the CS signal to actually be visible to the device.
>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@xxxxxxxxx>
> ---
>
> This revision uses devm_gpio_request_one() instead of gpio_request()/
> gpio_direction_output(), as suggested by Geert.
I suggested to use a devm_*() function as it relieves a device's .remove()
function of the burden to do manual resource management.
However, I had missed that freeing is not done in the device's .remove()
function, but in spi_master.cleanup(). Life cycle is completely different here,
and .setup() and .cleanup() may be called multiple times if SPI slaves are
added/removed dynamically (e.g. DT overlays). Fortunately you kept the
invocation of devm_gpio_free(), so calling .setup() and .cleanup() multiple
times (pairwise) still works.
> drivers/spi/spi-sh-msiof.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 6852552a..ed01289 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -541,15 +541,32 @@ static int sh_msiof_spi_setup(struct spi_device *spi)
> !!(spi->mode & SPI_LSB_FIRST),
> !!(spi->mode & SPI_CS_HIGH));
>
> - if (spi->cs_gpio >= 0)
> - gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
> + if (spi->cs_gpio >= 0 && !spi_get_ctldata(spi)) {
> + int ret = devm_gpio_request_one(&p->pdev->dev, spi->cs_gpio,
> + spi->mode & SPI_CS_HIGH ? GPIOF_OUT_INIT_LOW :
> + GPIOF_OUT_INIT_HIGH, dev_name(&spi->dev));
> +
> + if (ret < 0)
> + return ret;
>
> + spi_set_ctldata(spi, (void *)spi->cs_gpio);
> + }
>
> pm_runtime_put(&p->pdev->dev);
>
> return 0;
> }
>
> +static void sh_msiof_spi_cleanup(struct spi_device *spi)
> +{
> + struct sh_msiof_spi_priv *p = spi_master_get_devdata(spi->master);
> +
> + if (spi->cs_gpio >= 0)
> + devm_gpio_free(&p->pdev->dev, spi->cs_gpio);
I'm wondering if this can lead to a double free? E.g. if .setup() fails?
> +
> + spi_set_ctldata(spi, NULL);
> +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
