| Message ID | 1405316506-27855-2-git-send-email-ch.naveen@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 14, 2014 at 11:11:44AM +0530, Naveen Krishna Chatradhi wrote: > @@ -812,6 +800,10 @@ static int s3c64xx_spi_setup(struct spi_device *spi) > spi->controller_data = cs; > } > > + /* For the non-DT platforms derive chip selects from controller data */ > + if (!spi->dev.of_node) > + spi->cs_gpio = cs->line; > + > if (IS_ERR_OR_NULL(cs)) { > dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select); > return -ENODEV; > @@ -819,17 +811,16 @@ static int s3c64xx_spi_setup(struct spi_device *spi) > > if (!spi_get_ctldata(spi)) { > /* Request gpio only if cs line is asserted by gpio pins */ > - if (sdd->cs_gpio) { > - err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH, > - dev_name(&spi->dev)); > + if (gpio_is_valid(spi->cs_gpio)) { As previously mentioned gpio_is_valid() is *not* a direct substitute for checking if the boolean flag cs_gpio has been set since 0 is a valid GPIO on at least some of these platforms and as discussed several times already some of the SoCs require the use of the built in chip select. In general it's quite hard to tie the description in the patch to the code changes, not helped by the decision to do separate refactorings like this conversion to gpio_is_valid() as part of the one patch. The description of the patch now makes some statements about what the problem that's intended to be fixed is but it still doesn't seem entirely clear that everything has been thought through fully and tied to the code. The original code appears to be buggy which isn't helping anything but it's hard to have confidence that this isn't going to break some other use case that currently works given the lack of clarity and the number of revisions that have been required so far. I think some combination of smaller changes and a clearer working through of the before and after states for both DT and non DT cases to show that everything has been considered would help a lot. I may have another stare at this but it's worrying how hard I'm needing to think.
Hello Mark, On 14 July 2014 22:55, Mark Brown <broonie@kernel.org> wrote: > On Mon, Jul 14, 2014 at 11:11:44AM +0530, Naveen Krishna Chatradhi wrote: > >> @@ -812,6 +800,10 @@ static int s3c64xx_spi_setup(struct spi_device *spi) >> spi->controller_data = cs; >> } >> >> + /* For the non-DT platforms derive chip selects from controller data */ >> + if (!spi->dev.of_node) >> + spi->cs_gpio = cs->line; >> + >> if (IS_ERR_OR_NULL(cs)) { >> dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select); >> return -ENODEV; >> @@ -819,17 +811,16 @@ static int s3c64xx_spi_setup(struct spi_device *spi) >> >> if (!spi_get_ctldata(spi)) { >> /* Request gpio only if cs line is asserted by gpio pins */ >> - if (sdd->cs_gpio) { >> - err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH, >> - dev_name(&spi->dev)); >> + if (gpio_is_valid(spi->cs_gpio)) { > > As previously mentioned gpio_is_valid() is *not* a direct substitute for > checking if the boolean flag cs_gpio has been set since 0 is a valid > GPIO on at least some of these platforms and as discussed several times > already some of the SoCs require the use of the built in chip select. Yes, gpio_is_valid() is not a direct substitute for boolean cs_gpio. It was a review comment to use gpio_is_valid() and quickly iterated another version with that change. Now that we see gpio_is_valid() is breaking the native chip select for gpio number 0. Shall i go back to using the boolean or do you have a better way to do this. > > In general it's quite hard to tie the description in the patch to the > code changes, not helped by the decision to do separate refactorings > like this conversion to gpio_is_valid() as part of the one patch. The > description of the patch now makes some statements about what the > problem that's intended to be fixed is but it still doesn't seem > entirely clear that everything has been thought through fully and tied > to the code. > > The original code appears to be buggy which isn't helping anything but > it's hard to have confidence that this isn't going to break some other > use case that currently works given the lack of clarity and the number > of revisions that have been required so far. spi-s3c64xx.c is supporting a wide range of SoCs ranging from s3c64xx series followed by s5p series and then exynos4 and exynos5 series. However, Exynos4 and Exynos5 series currently available are all DT based. Support for DT and non-DT was taken good care. Revision number went to 5 for minor changes like 1. Missing documentation 2. Using gpio_is_valid() instead of boolean cs_gpio 3. Adding SignedOff-By and Acked-By > > I think some combination of smaller changes and a clearer working > through of the before and after states for both DT and non DT cases to > show that everything has been considered would help a lot. I may have > another stare at this but it's worrying how hard I'm needing to think. The intention of the patch was to fix the broken platforms by implementing dt bindings similar to generic SPI core in spi-s3c64xx.c. Current dt bindings: spi-s3c64xx.c is expects the cs-gpio" property in the SPI DT node and also defined under the "controller-data". Solution 1: Add support for "cs-gpios" property in the SPI DT node and remove "cs-gpio" from sub node "controller-data". Solution 2: As Javier suggested // Inverting the default of cs_gpio. and By default not having the "cs-gpio" property in the SPI dev node should mean that the "cs-gpio" property in the controller-data node should be used to signal the chip-select and having the "cs-gpio" property in the SPI node should mean that the native chip select should be used instead of a GPIO. That preserves the old DT binding semantic while making the GPIO to be optional // in this case spi-s3c64xx.c will continue to ignore the generic SPI "cs-gpios" implementation. I'm willing to implement any suggestion to fix this issue.
On Tue, Jul 15, 2014 at 12:31:32AM +0530, Naveen Krishna Ch wrote: > in this case spi-s3c64xx.c will continue to ignore the generic SPI "cs-gpios" > implementation. > I'm willing to implement any suggestion to fix this issue. The problem isn't what you're trying to do, the problem is verifying that it has been done correctly - making sure that everything has been accounted for in the change itself.
Hello Mark, On 15 July 2014 00:45, Mark Brown <broonie@kernel.org> wrote: > On Tue, Jul 15, 2014 at 12:31:32AM +0530, Naveen Krishna Ch wrote: > >> in this case spi-s3c64xx.c will continue to ignore the generic SPI "cs-gpios" >> implementation. > >> I'm willing to implement any suggestion to fix this issue. > > The problem isn't what you're trying to do, the problem is verifying > that it has been done correctly - making sure that everything has been > accounted for in the change itself. Understand your concern. I will try to get it tested on as many platforms.
Hello Mark, I agree that the commit message could have a better description and I understand your concerns. I'm not an SPI expert by any means but I did my best to review the patches and provide feedback to Naveen on the first iterations of the series. On Mon, Jul 14, 2014 at 7:25 PM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Jul 14, 2014 at 11:11:44AM +0530, Naveen Krishna Chatradhi wrote: > >> @@ -812,6 +800,10 @@ static int s3c64xx_spi_setup(struct spi_device *spi) >> spi->controller_data = cs; >> } >> >> + /* For the non-DT platforms derive chip selects from controller data */ >> + if (!spi->dev.of_node) >> + spi->cs_gpio = cs->line; >> + >> if (IS_ERR_OR_NULL(cs)) { >> dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select); >> return -ENODEV; >> @@ -819,17 +811,16 @@ static int s3c64xx_spi_setup(struct spi_device *spi) >> >> if (!spi_get_ctldata(spi)) { >> /* Request gpio only if cs line is asserted by gpio pins */ >> - if (sdd->cs_gpio) { >> - err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH, >> - dev_name(&spi->dev)); >> + if (gpio_is_valid(spi->cs_gpio)) { > > As previously mentioned gpio_is_valid() is *not* a direct substitute for > checking if the boolean flag cs_gpio has been set since 0 is a valid > GPIO on at least some of these platforms and as discussed several times > already some of the SoCs require the use of the built in chip select. > Please correct me if I'm wrong but in this case I think that using gpio_is_valid(spi->cs_gpio) is the right thing to do. The SPI core will set spi->cs_gpio to -ENOENT if a Device Tree didn't use the "cs-gpios" property of if the format (explained in Documentation/devicetree/bindings/spi/spi-bus.txt) to specify a native/built-in line chip select is used: cs-gpios = <&gpio1 0 0> <0> cs0 : &gpio1 0 0 cs1 : native So in that case gpio_is_valid(spi->cs_gpio) will return false which is what Naveen wants in his patch. Now, a problem is that in the non-DT case the patch does not rely on the spi->cs_gpio value set by the SPI core but instead overwrites it with the value in cs->line. So as you said this would have been an issue if legacy non-DT board code used s3c64xx_spi_csinfo .line to specify that the built-in chip select should be used instead of a GPIO. But looking at the board files that sets struct spi_board_info .controller_data to struct s3c64xx_spi_csinfo I see that the the .line field is actually used to specify a GPIO pin and not a chip select. In fact there is only one user in mainline (arch/arm/mach-s3c64xx/mach-crag6410-module.c) that do this: #define S3C64XX_GPC(_nr) (S3C64XX_GPIO_C_START + (_nr)) static struct s3c64xx_spi_csinfo wm0010_spi_csinfo = { .line = S3C64XX_GPC(3), }; static struct spi_board_info wm1253_devs[] = { [0] = { .modalias = "wm0010", .max_speed_hz = 26 * 1000 * 1000, .bus_num = 0, .chip_select = 0, .mode = SPI_MODE_0, .irq = S3C_EINT(4), .controller_data = &wm0010_spi_csinfo, .platform_data = &wm0010_pdata, }, }; So, the .line field is used to specify a GPIO, not a chip select. So gpio_is_valid() should also be used to check that cs->line is a valid GPIO. If board file does not want to use a GPIO and wants to use a built-in chip select then it should either not use a struct s3c64xx_spi_csinfo at all or set .line to -ENOENT in order to be consistent with the SPI core. Now, looking at this I realize that there is a bug in the spi-s3c64xx driver since it does not check if spi->controller_data is NULL in the non-DT case which I believe it can be true. > In general it's quite hard to tie the description in the patch to the > code changes, not helped by the decision to do separate refactorings > like this conversion to gpio_is_valid() as part of the one patch. The > description of the patch now makes some statements about what the > problem that's intended to be fixed is but it still doesn't seem > entirely clear that everything has been thought through fully and tied > to the code. > AFAICT most of the refactoring is actually removing the buggy code that was introduced in commit 3146bee ("spi: s3c64xx: Added provision for dedicated cs pin"). So maybe Naveen can try doing a new series first reverting the offending commit and then on top of that adding the support for the generic "cs-gpios" DT property used by the SPI core? As I said before this patch is fixing a bug in the SPI driver, the first version of the series was posted on June, 10 so many other patches already depend on this fix. So it would be great if we can move this forward since this is hurting the platform support as a whole. Thanks a lot and best regards, Javier
On Tue, Jul 15, 2014 at 09:33:21AM +0530, Naveen Krishna Ch wrote: > On 15 July 2014 00:45, Mark Brown <broonie@kernel.org> wrote: > > The problem isn't what you're trying to do, the problem is verifying > > that it has been done correctly - making sure that everything has been > > accounted for in the change itself. > Understand your concern. I will try to get it tested on as many platforms. That's useful, but please note that it's mainly a code review thing.
On Tue, Jul 15, 2014 at 12:38:58PM +0200, Javier Martinez Canillas wrote: > Hello Mark, Don't top post. > On Mon, Jul 14, 2014 at 7:25 PM, Mark Brown <broonie@kernel.org> wrote: > > On Mon, Jul 14, 2014 at 11:11:44AM +0530, Naveen Krishna Chatradhi wrote: > So, the .line field is used to specify a GPIO, not a chip select. So > gpio_is_valid() should also be used to check that cs->line is a valid > GPIO. If board file does not want to use a GPIO and wants to use a > built-in chip select then it should either not use a struct > s3c64xx_spi_csinfo at all or set .line to -ENOENT in order to be > consistent with the SPI core. This sort of "let's make everyone instabuggy" change is not good practice, especially when it's mixed in with some other not obviously related change, has an unclear benefit and is barely mentioned in the commit log - I found this through code review, not through it being a clearly intentional and considered part of the change. > As I said before this patch is fixing a bug in the SPI driver, the > first version of the series was posted on June, 10 so many other > patches already depend on this fix. So it would be great if we can > move this forward since this is hurting the platform support as a > whole. So provide a clear, easy to understand patch series then. As I've said before this series is setting off lots of alarm bells - the large number of versions, the difficulty in understanding what it was supposed to do both for the overall goal of the series and the individual changes, and the fact that several people have found bugs for use cases other than your own (all the way up to removing non-DT support) are all saying that this is something that needs careful review and also making that review difficult. Code review is an important part of the process, we need people to work to make that review easy, address review comments and allow a reasonable time for the review to happen (something that's obviously going to be quicker with submissions that are easier to review).
Hi Naveen, On 14.07.2014 07:41, Naveen Krishna Chatradhi wrote: > Since, (3146bee spi: s3c64xx: Added provision for dedicated cs pin) > > spi-s3c64xx.c driver expects > 1. chip select gpios from "cs-gpio"(singular) under the > "controller-data" node of the client/slave device of the SPI. Please work a bit more on readability of your commit messages, especially in terms of using correct terminology to avoid unnecessary confusion. s/'chip select gpios'/'chip select GPIO pins'/ s/'"cs-gpio"(singular)'/'"cs-gpio" property'/ s/'client/slave device of the SPI'/'SPI device'/ More things like this follow in rest of the text below. Otherwise I believe the code after this patch does the right thing, so: Reviewed-by: Tomasz Figa <t.figa@samsung.com> Best regards, Tomasz
Hello Tomasz, On 15 July 2014 22:25, Tomasz Figa <t.figa@samsung.com> wrote: > Hi Naveen, > > On 14.07.2014 07:41, Naveen Krishna Chatradhi wrote: >> Since, (3146bee spi: s3c64xx: Added provision for dedicated cs pin) >> >> spi-s3c64xx.c driver expects >> 1. chip select gpios from "cs-gpio"(singular) under the >> "controller-data" node of the client/slave device of the SPI. > > Please work a bit more on readability of your commit messages, > especially in terms of using correct terminology to avoid unnecessary > confusion. > > s/'chip select gpios'/'chip select GPIO pins'/ > s/'"cs-gpio"(singular)'/'"cs-gpio" property'/ > s/'client/slave device of the SPI'/'SPI device'/ Sure, I'm working on that. > > More things like this follow in rest of the text below. > > Otherwise I believe the code after this patch does the right thing, so: > > Reviewed-by: Tomasz Figa <t.figa@samsung.com> I've submitted another patch set which does the same functionality as this patch set. I believe the new series is more cleaner in terms of patch separation and documentation. Kindly, let me know your comments on that series. http://www.spinics.net/lists/arm-kernel/msg347634.html > > Best regards, > Tomasz
On 15.07.2014 19:21, Naveen Krishna Ch wrote: > Hello Tomasz, > > On 15 July 2014 22:25, Tomasz Figa <t.figa@samsung.com> wrote: >> Hi Naveen, >> >> On 14.07.2014 07:41, Naveen Krishna Chatradhi wrote: >>> Since, (3146bee spi: s3c64xx: Added provision for dedicated cs pin) >>> >>> spi-s3c64xx.c driver expects >>> 1. chip select gpios from "cs-gpio"(singular) under the >>> "controller-data" node of the client/slave device of the SPI. >> >> Please work a bit more on readability of your commit messages, >> especially in terms of using correct terminology to avoid unnecessary >> confusion. >> >> s/'chip select gpios'/'chip select GPIO pins'/ >> s/'"cs-gpio"(singular)'/'"cs-gpio" property'/ >> s/'client/slave device of the SPI'/'SPI device'/ > > Sure, I'm working on that. > >> >> More things like this follow in rest of the text below. >> >> Otherwise I believe the code after this patch does the right thing, so: >> >> Reviewed-by: Tomasz Figa <t.figa@samsung.com> > > I've submitted another patch set which does the same functionality > as this patch set. I believe the new series is more cleaner in terms of > patch separation and documentation. > > Kindly, let me know your comments on that series. > http://www.spinics.net/lists/arm-kernel/msg347634.html Right. I noticed it just now and am looking at it. Best regards, Tomasz
diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt index 655b665..792efba 100644 --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt @@ -39,15 +39,13 @@ Optional Board Specific Properties: - num-cs: Specifies the number of chip select lines supported. If not specified, the default number of chip select lines is set to 1. +- cs-gpios: should specify GPIOs used for chipselects (see spi-bus.txt) + SPI Controller specific data in SPI slave nodes: - The spi slave nodes should provide the following information which is required by the spi controller. - - cs-gpio: A gpio specifier that specifies the gpio line used as - the slave select line by the spi controller. The format of the gpio - specifier depends on the gpio controller. - - samsung,spi-feedback-delay: The sampling phase shift to be applied on the miso line (to account for any lag in the miso line). The following are the valid values. @@ -85,6 +83,7 @@ Example: #size-cells = <0>; pinctrl-names = "default"; pinctrl-0 = <&spi0_bus>; + cs-gpios = <&gpa2 5 0>; w25q80bw@0 { #address-cells = <1>; @@ -94,7 +93,6 @@ Example: spi-max-frequency = <10000>; controller-data { - cs-gpio = <&gpa2 5 1 0 3>; samsung,spi-feedback-delay = <0>; }; diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 75a5696..b61ff3d 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -197,7 +197,6 @@ struct s3c64xx_spi_driver_data { struct s3c64xx_spi_dma_data tx_dma; struct s3c64xx_spi_port_config *port_conf; unsigned int port_id; - bool cs_gpio; }; static void flush_fifo(struct s3c64xx_spi_driver_data *sdd) @@ -776,17 +775,6 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata( return ERR_PTR(-ENOMEM); } - /* The CS line is asserted/deasserted by the gpio pin */ - if (sdd->cs_gpio) - 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); - of_node_put(data_np); - return ERR_PTR(-EINVAL); - } - of_property_read_u32(data_np, "samsung,spi-feedback-delay", &fb_delay); cs->fb_delay = fb_delay; of_node_put(data_np); @@ -812,6 +800,10 @@ static int s3c64xx_spi_setup(struct spi_device *spi) spi->controller_data = cs; } + /* For the non-DT platforms derive chip selects from controller data */ + if (!spi->dev.of_node) + spi->cs_gpio = cs->line; + if (IS_ERR_OR_NULL(cs)) { dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select); return -ENODEV; @@ -819,17 +811,16 @@ static int s3c64xx_spi_setup(struct spi_device *spi) if (!spi_get_ctldata(spi)) { /* Request gpio only if cs line is asserted by gpio pins */ - if (sdd->cs_gpio) { - err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH, - dev_name(&spi->dev)); + if (gpio_is_valid(spi->cs_gpio)) { + err = gpio_request_one(spi->cs_gpio, + GPIOF_OUT_INIT_HIGH, + dev_name(&spi->dev)); if (err) { dev_err(&spi->dev, "Failed to get /CS gpio [%d]: %d\n", - cs->line, err); + spi->cs_gpio, err); goto err_gpio_req; } - - spi->cs_gpio = cs->line; } spi_set_ctldata(spi, cs); @@ -884,7 +875,8 @@ setup_exit: /* setup() returns with device de-selected */ writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL); - gpio_free(cs->line); + if (gpio_is_valid(spi->cs_gpio)) + gpio_free(spi->cs_gpio); spi_set_ctldata(spi, NULL); err_gpio_req: @@ -900,10 +892,12 @@ 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); + else + spi->cs_gpio = -ENOENT; } spi_set_ctldata(spi, NULL); } @@ -1075,11 +1069,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) sdd->cntrlr_info = sci; sdd->pdev = pdev; sdd->sfr_start = mem_res->start; - sdd->cs_gpio = true; if (pdev->dev.of_node) { - if (!of_find_property(pdev->dev.of_node, "cs-gpio", NULL)) - sdd->cs_gpio = false; - ret = of_alias_get_id(pdev->dev.of_node, "spi"); if (ret < 0) { dev_err(&pdev->dev, "failed to get alias id, errno %d\n",
Since, (3146bee spi: s3c64xx: Added provision for dedicated cs pin) spi-s3c64xx.c driver expects 1. chip select gpios from "cs-gpio"(singular) under the "controller-data" node of the client/slave device of the SPI. 2. "cs-gpio"(singular) entry to be present in the SPI device node. Eg of current broken usage: &spi_1 { cs-gpio <>; /* this entry is checked during probe */ ... slave_node { controller-data { cs-gpio <&gpioa2 5 0>; /* This field is parsed during .setup() */ } }; }; The following dts files which were using this driver. But, din't have the "cs-gpio" entry under SPI node. -- arch/arm/boot/dts/exynos4210-smdkv310.dts -- arch/arm/boot/dts/exynos4412-trats2.dts -- arch/arm/boot/dts/exynos5250-smdk5250.dts Also, the SPI core and many drivers moved on to using "cs-gpios" from SPI node and removed the gpio handling code from drivers (including spi-s3c64xx.c). Hence, spi-s3c64xx.c is broken since "Jun 21 11:26:12 2013" and considering the time with no compliants about the breakage. We are assuming it is safe to remove the "cs-gpio"(singular) usage from device tree binding of spi-samsung.txt and makes appropriate changes in the driver to use "cs-gpios"(plural) from SPI device node. Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com> Acked-by: Rob Herring <robh@kernel.org> Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> Tested-by: Doug Anderson <dianders@chromium.org> Cc: Tomasz Figa <t.figa@samsung.com> --- Changes since v5: Fixed the "making a GPIO chip select mandatory" bug. Changes since v4: 1. Added reviewed by from Javier and Tested by from Doug Changes since v3: 1. Remove the sdd->cs_gpio and use gpio_is_valid(spi->cs_gpio) instead 2. Keep cs->line only for Non-DT platforms and use spi->cs_gpio for DT platforms .../devicetree/bindings/spi/spi-samsung.txt | 8 ++--- drivers/spi/spi-s3c64xx.c | 38 ++++++++------------ 2 files changed, 17 insertions(+), 29 deletions(-)