Hi Naveen, Please see my comments inline.
On 15.07.2014 14:20, Naveen Krishna Chatradhi wrote: > This patch validates the cs->line (Chip select gpio) and > struct s3c64xx_spi_csinfo *cs object for both DT and NON-DT > platforms before using in .setup(). > > Also, check gpio_is_valid(spi->cs_gpio) in cleanup() before > freeing up. Missing reason of the change. > > Signed-off-by: Naveen Krishna Chatradhi <[email protected]> > Cc: Javier Martinez Canillas <[email protected]> > Cc: Doug Anderson <[email protected]> > --- > drivers/spi/spi-s3c64xx.c | 15 ++++----------- > 1 file changed, 4 insertions(+), 11 deletions(-) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index 72bfba6..8971076 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -773,12 +773,6 @@ static struct s3c64xx_spi_csinfo > *s3c64xx_get_slave_ctrldata( > /* The CS line is asserted/deasserted by the gpio pin */ > cs->line = spi->cs_gpio; > > - if (!gpio_is_valid(cs->line)) { > - dev_err(&spi->dev, "chip select gpio is not specified or > invalid\n"); > - kfree(cs); > - return ERR_PTR(-EINVAL); > - } > - > data_np = of_get_child_by_name(slave_np, "controller-data"); > if (!data_np) { > dev_err(&spi->dev, "child node 'controller-data' not found\n"); > @@ -805,15 +799,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi) > int err; > > sdd = spi_master_get_devdata(spi->master); > - if (!cs && spi->dev.of_node) { > + if (spi->dev.of_node) This check is equivalent, i.e. cs will always be NULL whenever spi->dev.of_node is not. > cs = s3c64xx_get_slave_ctrldata(spi); > - spi->controller_data = cs; > - } > > - if (IS_ERR_OR_NULL(cs)) { > + if (IS_ERR_OR_NULL(cs) || !gpio_is_valid(cs->line)) { I'm not sure this is correct. It will error out even if hardware chip select is used. Otherwise you need to free cs here if wrong GPIO was the cause of entering this block, so if this extra check is to stay, I'd suggest splitting this into two separate if blocks. > dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select); > return -ENODEV; > } > + spi->controller_data = cs; I'm not sure what's the point of moving this assignment here. > > if (!spi_get_ctldata(spi)) { > /* Request gpio only if cs line is asserted by gpio pins */ > @@ -898,7 +891,7 @@ static void s3c64xx_spi_cleanup(struct spi_device *spi) > struct s3c64xx_spi_driver_data *sdd; > > sdd = spi_master_get_devdata(spi->master); > - if (spi->cs_gpio) { > + if (gpio_is_valid(spi->cs_gpio)) { > gpio_free(spi->cs_gpio); > if (spi->dev.of_node) > kfree(cs); I believe this is completely wrong. cs is allocated even if GPIO chip select is not used, so the only thing that should be done conditionally is gpio_free(). In general, I liked previous version of this series much more, as it was doing what it should as opposed to this one. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
