| Message ID | 1363157014-9615-5-git-send-email-ks.giri@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 13, 2013 at 12:13:33PM +0530, Girish K S wrote: > From: Girish K S <girishks2000@gmail.com> > > The existing driver supports gpio based /cs signal. > For controller's that have one device per controller, > the slave device's /cs signal might be internally controlled > by the chip select bit of slave select register. They are not > externally asserted/deasserted using gpio pin. Applying this patch breaks my S3C64xx based system (Cragganmore, a non-DT platform). It'll break all existing non-DT platforms since... > + * @cs_gpio: CS line status, 'true' if CS line is asserted by gpio. > + * 'false' if asserted by internal dedicated pin. > * @line: Custom 'identity' of the CS line. > * > * This is per SPI-Slave Chipselect information. > @@ -25,6 +27,7 @@ struct platform_device; > */ > struct s3c64xx_spi_csinfo { > u8 fb_delay; > + bool cs_gpio; > unsigned line; > }; ...you've added this new cs_gpio field to the platform data but not updated any of the existing users (including Cragganmore). It would seem better to make the default behaviour stay as per the current default and make the new behaviour optional but at a minimum all existing in-tree users need to be updated. It's also a bit odd that we end up checking cs_gpio and then using line in the code, it'd be more idiomatic if cs_gpio were the GPIO number.
On Mon, Apr 1, 2013 at 6:27 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Wed, Mar 13, 2013 at 12:13:33PM +0530, Girish K S wrote: >> From: Girish K S <girishks2000@gmail.com> >> >> The existing driver supports gpio based /cs signal. >> For controller's that have one device per controller, >> the slave device's /cs signal might be internally controlled >> by the chip select bit of slave select register. They are not >> externally asserted/deasserted using gpio pin. > > Applying this patch breaks my S3C64xx based system (Cragganmore, a > non-DT platform). It'll break all existing non-DT platforms since... sure will rebase and take care of the non-dt case > >> + * @cs_gpio: CS line status, 'true' if CS line is asserted by gpio. >> + * 'false' if asserted by internal dedicated pin. >> * @line: Custom 'identity' of the CS line. >> * >> * This is per SPI-Slave Chipselect information. >> @@ -25,6 +27,7 @@ struct platform_device; >> */ >> struct s3c64xx_spi_csinfo { >> u8 fb_delay; >> + bool cs_gpio; >> unsigned line; >> }; > > ...you've added this new cs_gpio field to the platform data but not > updated any of the existing users (including Cragganmore). It would > seem better to make the default behaviour stay as per the current > default and make the new behaviour optional but at a minimum all > existing in-tree users need to be updated. > > It's also a bit odd that we end up checking cs_gpio and then using line > in the code, it'd be more idiomatic if cs_gpio were the GPIO number. In the original driver it was assumed that the cs line is always a gpio pin. But the current controller that i am working on has no gpio pin for cs selection. All the lines to the device are internally connected. There is no option to select the cs signal. So cs-gpio property parsing has to skipped for this controller, that means cs_gpio cannot be a GPIO number. If it has to be a number then it has to be < 0 to say it is not gpio. Any >= 0 number implies it is a valid gpio (in reality for this controller it is not.) So i introduced a new member cs_gpio. I will checkout the possibility to avoid this member.
On Mon, Apr 08, 2013 at 03:21:03PM +0530, Girish KS wrote: > On Mon, Apr 1, 2013 at 6:27 PM, Mark Brown > > It's also a bit odd that we end up checking cs_gpio and then using line > > in the code, it'd be more idiomatic if cs_gpio were the GPIO number. > In the original driver it was assumed that the cs line is always a gpio pin. > But the current controller that i am working on has no gpio pin for cs > selection. > All the lines to the device are internally connected. There is no > option to select > the cs signal. So cs-gpio property parsing has to skipped for this > controller, that means > cs_gpio cannot be a GPIO number. If it has to be a number then it has > to be < 0 to say > it is not gpio. Any >= 0 number implies it is a valid gpio (in reality > for this controller it is not.) Two options here, one is to just assume nobody will use GPIO 0 and the other is to set the number appopriately during probe so that only probe needs to worry about the issue.
On Mon, Apr 8, 2013 at 3:45 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Apr 08, 2013 at 03:21:03PM +0530, Girish KS wrote: >> On Mon, Apr 1, 2013 at 6:27 PM, Mark Brown > >> > It's also a bit odd that we end up checking cs_gpio and then using line >> > in the code, it'd be more idiomatic if cs_gpio were the GPIO number. > >> In the original driver it was assumed that the cs line is always a gpio pin. >> But the current controller that i am working on has no gpio pin for cs >> selection. >> All the lines to the device are internally connected. There is no >> option to select >> the cs signal. So cs-gpio property parsing has to skipped for this >> controller, that means >> cs_gpio cannot be a GPIO number. If it has to be a number then it has >> to be < 0 to say >> it is not gpio. Any >= 0 number implies it is a valid gpio (in reality >> for this controller it is not.) > > Two options here, one is to just assume nobody will use GPIO 0 and the > other is to set the number appopriately during probe so that only probe > needs to worry about the issue. regarding the first option, may be others also should agree to it (in case if somebody is using the gpio 0). In the second option if the gpio number is set in the probe, then we are overwriting the actual gpio number assigned in the platform data. If I move the cs_gpio from the platform data to controller private data then the dependency on other platforms can be removed, but still the check (true or false) before setting the gpio line will remain. If agreed upon this i can go ahead and post the patch
On Mon, Apr 8, 2013 at 5:15 PM, Girish KS <girishks2000@gmail.com> wrote: > On Mon, Apr 8, 2013 at 3:45 PM, Mark Brown > <broonie@opensource.wolfsonmicro.com> wrote: >> On Mon, Apr 08, 2013 at 03:21:03PM +0530, Girish KS wrote: >>> On Mon, Apr 1, 2013 at 6:27 PM, Mark Brown >> >>> > It's also a bit odd that we end up checking cs_gpio and then using line >>> > in the code, it'd be more idiomatic if cs_gpio were the GPIO number. >> >>> In the original driver it was assumed that the cs line is always a gpio pin. >>> But the current controller that i am working on has no gpio pin for cs >>> selection. >>> All the lines to the device are internally connected. There is no >>> option to select >>> the cs signal. So cs-gpio property parsing has to skipped for this >>> controller, that means >>> cs_gpio cannot be a GPIO number. If it has to be a number then it has >>> to be < 0 to say >>> it is not gpio. Any >= 0 number implies it is a valid gpio (in reality >>> for this controller it is not.) >> >> Two options here, one is to just assume nobody will use GPIO 0 and the >> other is to set the number appopriately during probe so that only probe >> needs to worry about the issue. > > regarding the first option, may be others also should agree to it (in > case if somebody is > using the gpio 0). > In the second option if the gpio number is set in the probe, then we > are overwriting the > actual gpio number assigned in the platform data. > If I move the cs_gpio from the platform data to controller private > data then the dependency > on other platforms can be removed, but still the check (true or false) > before setting the gpio > line will remain. If agreed upon this i can go ahead and post the patch Sorry for the allignment. there is some issue with my interface
On Mon, Apr 08, 2013 at 05:15:26PM +0530, Girish KS wrote: > On Mon, Apr 8, 2013 at 3:45 PM, Mark Brown > > Two options here, one is to just assume nobody will use GPIO 0 and the > > other is to set the number appopriately during probe so that only probe > > needs to worry about the issue. > > In the second option if the gpio number is set in the probe, then we > are overwriting the > actual gpio number assigned in the platform data. > If I move the cs_gpio from the platform data to controller private > data then the dependency > on other platforms can be removed, but still the check (true or false) > before setting the gpio > line will remain. If agreed upon this i can go ahead and post the patch I think logic in probe should be fine, it just felt really cumbersome having this on every single use but doing it once on probe ought to be OK.
On Mon, Apr 8, 2013 at 5:50 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Apr 08, 2013 at 05:15:26PM +0530, Girish KS wrote: >> On Mon, Apr 8, 2013 at 3:45 PM, Mark Brown > >> > Two options here, one is to just assume nobody will use GPIO 0 and the >> > other is to set the number appopriately during probe so that only probe >> > needs to worry about the issue. >> >> In the second option if the gpio number is set in the probe, then we >> are overwriting the >> actual gpio number assigned in the platform data. >> If I move the cs_gpio from the platform data to controller private >> data then the dependency >> on other platforms can be removed, but still the check (true or false) >> before setting the gpio >> line will remain. If agreed upon this i can go ahead and post the patch > > I think logic in probe should be fine, it just felt really cumbersome > having this on every single use but doing it once on probe ought to be > OK. I will move the populating cs_gpio logic to probe. But the enable_cs and disable_cs will have the check before calling the gpio_set_value api (because it takes cs-line as a parameter),
On Mon, Apr 08, 2013 at 07:19:05PM +0530, Girish KS wrote: > I will move the populating cs_gpio logic to probe. But the enable_cs > and disable_cs will have the > check before calling the gpio_set_value api (because it takes cs-line > as a parameter), Yes, you'll still need to have a check.
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 94716d1..ac557f8 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -414,14 +414,16 @@ static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd, if (sdd->tgl_spi != spi) { /* if last mssg on diff device */ /* Deselect the last toggled device */ cs = sdd->tgl_spi->controller_data; - gpio_set_value(cs->line, - spi->mode & SPI_CS_HIGH ? 0 : 1); + if (cs->cs_gpio) + gpio_set_value(cs->line, + spi->mode & SPI_CS_HIGH ? 0 : 1); } sdd->tgl_spi = NULL; } cs = spi->controller_data; - gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0); + if (cs->cs_gpio) + gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0); /* Start the signals */ writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL); @@ -550,7 +552,8 @@ static inline void disable_cs(struct s3c64xx_spi_driver_data *sdd, if (sdd->tgl_spi == spi) sdd->tgl_spi = NULL; - gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1); + if (cs->cs_gpio) + gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1); /* Quiese the signals */ writel(S3C64XX_SPI_SLAVE_SIG_INACT, @@ -889,7 +892,12 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata( return ERR_PTR(-ENOMEM); } - cs->line = of_get_named_gpio(data_np, "cs-gpio", 0); + if (of_find_property(data_np, "cs-gpio", NULL)) { + /* The CS line is asserted/deasserted by the gpio pin */ + cs->cs_gpio = true; + cs->line = of_get_named_gpio(data_np, "cs-gpio", 0); + } + if (!gpio_is_valid(cs->line)) { dev_err(&spi->dev, "chip select gpio is not specified or invalid\n"); kfree(cs); @@ -929,7 +937,8 @@ static int s3c64xx_spi_setup(struct spi_device *spi) return -ENODEV; } - if (!spi_get_ctldata(spi)) { + /* Request gpio only if cs line is asserted by gpio pins */ + if (cs->cs_gpio) { err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH, dev_name(&spi->dev)); if (err) { @@ -938,9 +947,11 @@ static int s3c64xx_spi_setup(struct spi_device *spi) cs->line, err); goto err_gpio_req; } - spi_set_ctldata(spi, cs); } + if (!spi_get_ctldata(spi)) + spi_set_ctldata(spi, cs); + sci = sdd->cntrlr_info; spin_lock_irqsave(&sdd->lock, flags); @@ -1028,7 +1039,7 @@ static void s3c64xx_spi_cleanup(struct spi_device *spi) { struct s3c64xx_spi_csinfo *cs = spi_get_ctldata(spi); - if (cs) { + if (cs && cs->cs_gpio) { gpio_free(cs->line); if (spi->dev.of_node) kfree(cs); diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h index ceba18d..0343d8d 100644 --- a/include/linux/platform_data/spi-s3c64xx.h +++ b/include/linux/platform_data/spi-s3c64xx.h @@ -17,6 +17,8 @@ struct platform_device; * struct s3c64xx_spi_csinfo - ChipSelect description * @fb_delay: Slave specific feedback delay. * Refer to FB_CLK_SEL register definition in SPI chapter. + * @cs_gpio: CS line status, 'true' if CS line is asserted by gpio. + * 'false' if asserted by internal dedicated pin. * @line: Custom 'identity' of the CS line. * * This is per SPI-Slave Chipselect information. @@ -25,6 +27,7 @@ struct platform_device; */ struct s3c64xx_spi_csinfo { u8 fb_delay; + bool cs_gpio; unsigned line; };