| Message ID | 1402394881-31564-2-git-send-email-ch.naveen@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 10/06/14 12:08, Naveen Krishna Chatradhi wrote: > Currently, spi-s3c64xx.c needs "cs-gpio" chip select GPIO to be > defined under "controller-data" node under each slave node. > > &spi_x { > cs-gpios <>; > ... > slave_node { > > controller-data { > cs-gpio = <>; > ... > }; > ... > }; > ... > }; > > Where as, SPI core and many other drivers uses "cs-gpios" for > from device tree node. > > Hence, make changes in spi-s3c64xx.c driver to make use of > "cs-gpios" from SPI node(parent) instead of "cs-gpio" defined in > slaves "controller-data"(child) node. > > Also updates the Device tree Documentation. > > Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com> > Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > Cc: Doug Anderson <dianders@chromium.org> > --- > Changes since v1: > 1. fixed a compilation warning thus squashing the other patch into this. > 2. Updated device tree description in spi-samsung.txt > > .../devicetree/bindings/spi/spi-samsung.txt | 8 ++- > drivers/spi/spi-s3c64xx.c | 56 ++++++++++++-------- > 2 files changed, 38 insertions(+), 26 deletions(-) > > diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt > index 86aa061..13bfcb5 100644 > --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt > +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt > @@ -42,15 +42,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 1 0 3>; While at it, please change the GPIO specifier format, this one is not valid any more. > 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..4594dde 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -750,47 +750,56 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master, > } > > static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata( > - struct spi_device *spi) > + struct spi_device *spi, > + struct s3c64xx_spi_csinfo *cs) > { > - struct s3c64xx_spi_csinfo *cs; > - struct device_node *slave_np, *data_np = NULL; > - struct s3c64xx_spi_driver_data *sdd; > + struct device_node *data_np = NULL; > u32 fb_delay = 0; > > - sdd = spi_master_get_devdata(spi->master); > - slave_np = spi->dev.of_node; > - if (!slave_np) { > - dev_err(&spi->dev, "device node not found\n"); > + data_np = of_get_child_by_name(spi->dev.of_node, "controller-data"); > + if (!data_np) { > + dev_err(&spi->dev, "child node 'controller-data' not found\n"); > 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"); > + of_property_read_u32(data_np, "samsung,spi-feedback-delay", &fb_delay); > + cs->fb_delay = fb_delay; > + of_node_put(data_np); > + > + return cs; > +} > + > +static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi) > +{ > + struct device_node *parent_np = NULL; > + struct s3c64xx_spi_driver_data *sdd; > + struct s3c64xx_spi_csinfo *cs; > + > + parent_np = of_get_parent(spi->dev.of_node); > + if (!parent_np) { > + dev_err(&spi->dev, "Parent node not found\n"); > return ERR_PTR(-EINVAL); > } > > + sdd = spi_master_get_devdata(spi->master); > + > cs = kzalloc(sizeof(*cs), GFP_KERNEL); > if (!cs) { > - of_node_put(data_np); > + of_node_put(parent_np); > 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); > + cs->line = of_get_named_gpio(parent_np, "cs-gpios", 0); Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ? After your change all DTBs using the original pattern will not work with new kernels any more. At least I would expect such backward compatibility maintained for few kernel releases. > 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); > + of_node_put(parent_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); > - return cs; > + return s3c64xx_get_slave_ctrldata(spi, cs); > } > > /* > @@ -806,9 +815,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi) > struct s3c64xx_spi_info *sci; > int err; > > + if (!spi->dev.of_node) { > + dev_err(&spi->dev, "device node not found\n"); > + return -EINVAL; > + } > + > sdd = spi_master_get_devdata(spi->master); > if (!cs && spi->dev.of_node) { > - cs = s3c64xx_get_slave_ctrldata(spi); > + cs = s3c64xx_get_cs_gpios(spi); > spi->controller_data = cs; > } > > @@ -1077,7 +1091,7 @@ static int s3c64xx_spi_probe(struct platform_device *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)) > + if (!of_find_property(pdev->dev.of_node, "cs-gpios", NULL)) > sdd->cs_gpio = false; Ditto. > ret = of_alias_get_id(pdev->dev.of_node, "spi"); -- Thanks! Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Sylwester, Thanks for the review. On 10 June 2014 16:09, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > On 10/06/14 12:08, Naveen Krishna Chatradhi wrote: >> Currently, spi-s3c64xx.c needs "cs-gpio" chip select GPIO to be >> defined under "controller-data" node under each slave node. >> >> &spi_x { >> cs-gpios <>; >> ... >> slave_node { >> >> controller-data { >> cs-gpio = <>; >> ... >> }; >> ... >> }; >> ... >> }; >> >> Where as, SPI core and many other drivers uses "cs-gpios" for >> from device tree node. >> >> Hence, make changes in spi-s3c64xx.c driver to make use of >> "cs-gpios" from SPI node(parent) instead of "cs-gpio" defined in >> slaves "controller-data"(child) node. >> >> Also updates the Device tree Documentation. >> >> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com> >> Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk> >> Cc: Doug Anderson <dianders@chromium.org> >> --- >> Changes since v1: >> 1. fixed a compilation warning thus squashing the other patch into this. >> 2. Updated device tree description in spi-samsung.txt >> >> .../devicetree/bindings/spi/spi-samsung.txt | 8 ++- >> drivers/spi/spi-s3c64xx.c | 56 ++++++++++++-------- >> 2 files changed, 38 insertions(+), 26 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt >> index 86aa061..13bfcb5 100644 >> --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt >> +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt >> @@ -42,15 +42,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 1 0 3>; > > While at it, please change the GPIO specifier format, this one is not > valid any more. Sure, i will club it with the other comments. > >> 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..4594dde 100644 >> --- a/drivers/spi/spi-s3c64xx.c >> +++ b/drivers/spi/spi-s3c64xx.c >> @@ -750,47 +750,56 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master, >> } >> >> static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata( >> - struct spi_device *spi) >> + struct spi_device *spi, >> + struct s3c64xx_spi_csinfo *cs) >> { >> - struct s3c64xx_spi_csinfo *cs; >> - struct device_node *slave_np, *data_np = NULL; >> - struct s3c64xx_spi_driver_data *sdd; >> + struct device_node *data_np = NULL; >> u32 fb_delay = 0; >> >> - sdd = spi_master_get_devdata(spi->master); >> - slave_np = spi->dev.of_node; >> - if (!slave_np) { >> - dev_err(&spi->dev, "device node not found\n"); >> + data_np = of_get_child_by_name(spi->dev.of_node, "controller-data"); >> + if (!data_np) { >> + dev_err(&spi->dev, "child node 'controller-data' not found\n"); >> 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"); >> + of_property_read_u32(data_np, "samsung,spi-feedback-delay", &fb_delay); >> + cs->fb_delay = fb_delay; >> + of_node_put(data_np); >> + >> + return cs; >> +} >> + >> +static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi) >> +{ >> + struct device_node *parent_np = NULL; >> + struct s3c64xx_spi_driver_data *sdd; >> + struct s3c64xx_spi_csinfo *cs; >> + >> + parent_np = of_get_parent(spi->dev.of_node); >> + if (!parent_np) { >> + dev_err(&spi->dev, "Parent node not found\n"); >> return ERR_PTR(-EINVAL); >> } >> >> + sdd = spi_master_get_devdata(spi->master); >> + >> cs = kzalloc(sizeof(*cs), GFP_KERNEL); >> if (!cs) { >> - of_node_put(data_np); >> + of_node_put(parent_np); >> 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); >> + cs->line = of_get_named_gpio(parent_np, "cs-gpios", 0); > > Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ? > After your change all DTBs using the original pattern will not work with > new kernels any more. At least I would expect such backward compatibility > maintained for few kernel releases. The reason behind removing the "cs-gpio" or not providing backward compatibility was 1. Since spi-core started using "cs-gpios" string from spi device node several months ago. The spi-s3c64xx.c driver is partially broken for more than 6 months. 2. Supporting "cs-gpio" would add extra bit of code. I've corrected the dts files that were using "cs-gpio" under "controller-data"(child node) to use "cs-gpio" from spi device node (parent node). I will make another version if you insist. > >> 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); >> + of_node_put(parent_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); >> - return cs; >> + return s3c64xx_get_slave_ctrldata(spi, cs); >> } >> >> /* >> @@ -806,9 +815,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi) >> struct s3c64xx_spi_info *sci; >> int err; >> >> + if (!spi->dev.of_node) { >> + dev_err(&spi->dev, "device node not found\n"); >> + return -EINVAL; >> + } >> + >> sdd = spi_master_get_devdata(spi->master); >> if (!cs && spi->dev.of_node) { >> - cs = s3c64xx_get_slave_ctrldata(spi); >> + cs = s3c64xx_get_cs_gpios(spi); >> spi->controller_data = cs; >> } >> >> @@ -1077,7 +1091,7 @@ static int s3c64xx_spi_probe(struct platform_device *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)) >> + if (!of_find_property(pdev->dev.of_node, "cs-gpios", NULL)) >> sdd->cs_gpio = false; > > Ditto. I can do that, once we are finalized on having backward compatibility. > >> ret = of_alias_get_id(pdev->dev.of_node, "spi"); > > -- > Thanks! > Sylwester
Naveen / Sylwester, On Tue, Jun 10, 2014 at 4:00 AM, Naveen Krishna Ch <naveenkrishna.ch@gmail.com> wrote: >> Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ? >> After your change all DTBs using the original pattern will not work with >> new kernels any more. At least I would expect such backward compatibility >> maintained for few kernel releases. > > The reason behind removing the "cs-gpio" or not providing backward > compatibility was > > 1. Since spi-core started using "cs-gpios" string from spi device node > several months ago. > The spi-s3c64xx.c driver is partially broken for more than 6 months. > > 2. Supporting "cs-gpio" would add extra bit of code. > > I've corrected the dts files that were using "cs-gpio" under > "controller-data"(child node) > to use "cs-gpio" from spi device node (parent node). > > I will make another version if you insist. Yup, as near as I can tell this has been broken since (3146bee spi: s3c64xx: Added provision for dedicated cs pin). That landed June 25th of 2013 into the SPI tree. ...so clearly nobody was really testing Samsung's SPI driver on ToT Linux. 1 year of unnoticed brokenness seems like enough time that we could do away with the old code. If someone comes out of the woodwork and claims a dire need to support the old binding then it can be added then. In-tree users of this appear to be: arch/arm/boot/dts/exynos4210-smdkv310.dts: cs-gpio = <&gpc1 2 0>; arch/arm/boot/dts/exynos4412-trats2.dts: cs-gpio = <&gpb 5 0>; arch/arm/boot/dts/exynos5250-smdk5250.dts: cs-gpio = <&gpa2 5 0>; ...and I guess nobody has bothered using the SPI flash on those boards for the last year? -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Naveen, Not a full review, but a few quick things I happened to notice: On Tue, Jun 10, 2014 at 3:08 AM, Naveen Krishna Chatradhi <ch.naveen@samsung.com> wrote: > @@ -94,7 +93,6 @@ Example: > spi-max-frequency = <10000>; > > controller-data { > - cs-gpio = <&gpa2 5 1 0 3>; > samsung,spi-feedback-delay = <0>; In a future patch (not this one!) it might make sense to also move spi-feedback-delay out. > +static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi) I believe that you can make use of the fact that the SPI core already got these GPIOs for you. See of_spi_register_master(). Everything you need ought to be in master->num_chipselect and master->cs_gpios. Strangely, it doesn't look like any other drivers use this, so possibly I'm confused... > @@ -806,9 +815,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi) > struct s3c64xx_spi_info *sci; > int err; > > + if (!spi->dev.of_node) { > + dev_err(&spi->dev, "device node not found\n"); > + return -EINVAL; > + } This seems incredibly broken. You're saying that the SPI driver no longer works without the device tree? I don't think that's desirable, but I suppose I could be wrong. Also note that you still left an "if" test below for trying to deal with the fact that there is no "of_node". -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10.06.2014 20:26, Doug Anderson wrote: > Naveen, > > Not a full review, but a few quick things I happened to notice: > > On Tue, Jun 10, 2014 at 3:08 AM, Naveen Krishna Chatradhi > <ch.naveen@samsung.com> wrote: >> @@ -94,7 +93,6 @@ Example: >> spi-max-frequency = <10000>; >> >> controller-data { >> - cs-gpio = <&gpa2 5 1 0 3>; >> samsung,spi-feedback-delay = <0>; > > In a future patch (not this one!) it might make sense to also move > spi-feedback-delay out. > > >> +static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi) > > I believe that you can make use of the fact that the SPI core already > got these GPIOs for you. See of_spi_register_master(). Everything > you need ought to be in master->num_chipselect and master->cs_gpios. > > Strangely, it doesn't look like any other drivers use this, so > possibly I'm confused... > > >> @@ -806,9 +815,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi) >> struct s3c64xx_spi_info *sci; >> int err; >> >> + if (!spi->dev.of_node) { >> + dev_err(&spi->dev, "device node not found\n"); >> + return -EINVAL; >> + } > > This seems incredibly broken. You're saying that the SPI driver no > longer works without the device tree? I don't think that's desirable, > but I suppose I could be wrong. > > Also note that you still left an "if" test below for trying to deal > with the fact that there is no "of_node". This driver can be also used by non-DT platforms (namely s3c2443, s3c64xx and s5p*) and so this assumption is wrong. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10.06.2014 20:09, Doug Anderson wrote: > Naveen / Sylwester, > > On Tue, Jun 10, 2014 at 4:00 AM, Naveen Krishna Ch > <naveenkrishna.ch@gmail.com> wrote: >>> Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ? >>> After your change all DTBs using the original pattern will not work with >>> new kernels any more. At least I would expect such backward compatibility >>> maintained for few kernel releases. >> >> The reason behind removing the "cs-gpio" or not providing backward >> compatibility was >> >> 1. Since spi-core started using "cs-gpios" string from spi device node >> several months ago. >> The spi-s3c64xx.c driver is partially broken for more than 6 months. >> >> 2. Supporting "cs-gpio" would add extra bit of code. >> >> I've corrected the dts files that were using "cs-gpio" under >> "controller-data"(child node) >> to use "cs-gpio" from spi device node (parent node). >> >> I will make another version if you insist. > > Yup, as near as I can tell this has been broken since (3146bee spi: > s3c64xx: Added provision for dedicated cs pin). That landed June 25th > of 2013 into the SPI tree. > > ...so clearly nobody was really testing Samsung's SPI driver on ToT > Linux. 1 year of unnoticed brokenness seems like enough time that we > could do away with the old code. If someone comes out of the woodwork > and claims a dire need to support the old binding then it can be added > then. > > In-tree users of this appear to be: > > arch/arm/boot/dts/exynos4210-smdkv310.dts: > cs-gpio = <&gpc1 2 0>; > arch/arm/boot/dts/exynos4412-trats2.dts: > cs-gpio = <&gpb 5 0>; > arch/arm/boot/dts/exynos5250-smdk5250.dts: > cs-gpio = <&gpa2 5 0>; > > ...and I guess nobody has bothered using the SPI flash on those boards > for the last year? On Trats2, it's actually a camera sensor, not SPI flash... Still, that's really a shame that: a) such patch went in (i.e. its brokenness was not spotted in review), b) the regression was not spotted. Anyway, IMHO this justifies removing the old binding then. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Naveen, On 10.06.2014 12:08, Naveen Krishna Chatradhi wrote: > Currently, spi-s3c64xx.c needs "cs-gpio" chip select GPIO to be > defined under "controller-data" node under each slave node. [snip] > @@ -85,6 +83,7 @@ Example: > #size-cells = <0>; > pinctrl-names = "default"; > pinctrl-0 = <&spi0_bus>; > + cs-gpios = <&gpa2 5 1 0 3>; While at it, you might also update the example to something more appropriate on Samsung platforms, e.g. <&gpa2 5 0>; > > w25q80bw@0 { > #address-cells = <1>; [snip] > +static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi) > +{ > + struct device_node *parent_np = NULL; > + struct s3c64xx_spi_driver_data *sdd; > + struct s3c64xx_spi_csinfo *cs; > + > + parent_np = of_get_parent(spi->dev.of_node); > + if (!parent_np) { > + dev_err(&spi->dev, "Parent node not found\n"); > return ERR_PTR(-EINVAL); > } > > + sdd = spi_master_get_devdata(spi->master); > + > cs = kzalloc(sizeof(*cs), GFP_KERNEL); > if (!cs) { > - of_node_put(data_np); > + of_node_put(parent_np); > 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); > + cs->line = of_get_named_gpio(parent_np, "cs-gpios", 0); This is wrong. The "cs-gpios" property is supposed to be an array, indexed by chip select number of SPI devices (indicated by their "reg" properties). Moreover, is there a need to parse this manually in this driver? I can see respective parsing code in of_spi_register_master(). Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomasz, On Tue, Jun 10, 2014 at 12:49 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > This is wrong. The "cs-gpios" property is supposed to be an array, > indexed by chip select number of SPI devices (indicated by their "reg" > properties). > > Moreover, is there a need to parse this manually in this driver? I can > see respective parsing code in of_spi_register_master(). I noticed this too (see my response), but I was confused about the fact that nobody else uses the array created by of_spi_register_master(). Any idea why? -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10.06.2014 21:58, Doug Anderson wrote: > Tomasz, > > On Tue, Jun 10, 2014 at 12:49 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> This is wrong. The "cs-gpios" property is supposed to be an array, >> indexed by chip select number of SPI devices (indicated by their "reg" >> properties). >> >> Moreover, is there a need to parse this manually in this driver? I can >> see respective parsing code in of_spi_register_master(). > > I noticed this too (see my response), but I was confused about the > fact that nobody else uses the array created by > of_spi_register_master(). Any idea why? Hmm, I can see of_spi_register_master() assigning allocated pointer to master->cs_gpios, which is then used in spi_add_device() as follows: if (master->cs_gpios) spi->cs_gpio = master->cs_gpios[spi->chip_select]; Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomasz, On Tue, Jun 10, 2014 at 12:59 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > On 10.06.2014 21:58, Doug Anderson wrote: >> Tomasz, >> >> On Tue, Jun 10, 2014 at 12:49 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >>> This is wrong. The "cs-gpios" property is supposed to be an array, >>> indexed by chip select number of SPI devices (indicated by their "reg" >>> properties). >>> >>> Moreover, is there a need to parse this manually in this driver? I can >>> see respective parsing code in of_spi_register_master(). >> >> I noticed this too (see my response), but I was confused about the >> fact that nobody else uses the array created by >> of_spi_register_master(). Any idea why? > > Hmm, I can see of_spi_register_master() assigning allocated pointer to > master->cs_gpios, which is then used in spi_add_device() as follows: > > if (master->cs_gpios) > spi->cs_gpio = master->cs_gpios[spi->chip_select]; OK. I haven't traced through all the code, but I did notice: * spi-gpio.c: has its own cs_gpios[0], initted with of_get_named_gpio() * spi-dw-mmio.c, spi-efm32.c, spi-imx.c, spi-pl022.c, spi-sirf.c all seem to be looking at "cs-gpios" directly. ...but I see now that you're right that most drivers don't need to actually look at cs_gpios and can just look at the device itself. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 10, 2014 at 1:09 PM, Doug Anderson <dianders@chromium.org> wrote: > Naveen / Sylwester, > > On Tue, Jun 10, 2014 at 4:00 AM, Naveen Krishna Ch > <naveenkrishna.ch@gmail.com> wrote: >>> Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ? >>> After your change all DTBs using the original pattern will not work with >>> new kernels any more. At least I would expect such backward compatibility >>> maintained for few kernel releases. >> >> The reason behind removing the "cs-gpio" or not providing backward >> compatibility was >> >> 1. Since spi-core started using "cs-gpios" string from spi device node >> several months ago. >> The spi-s3c64xx.c driver is partially broken for more than 6 months. >> >> 2. Supporting "cs-gpio" would add extra bit of code. >> >> I've corrected the dts files that were using "cs-gpio" under >> "controller-data"(child node) >> to use "cs-gpio" from spi device node (parent node). >> >> I will make another version if you insist. > > Yup, as near as I can tell this has been broken since (3146bee spi: > s3c64xx: Added provision for dedicated cs pin). That landed June 25th > of 2013 into the SPI tree. > > ...so clearly nobody was really testing Samsung's SPI driver on ToT > Linux. 1 year of unnoticed brokenness seems like enough time that we > could do away with the old code. If someone comes out of the woodwork > and claims a dire need to support the old binding then it can be added > then. > > In-tree users of this appear to be: > > arch/arm/boot/dts/exynos4210-smdkv310.dts: > cs-gpio = <&gpc1 2 0>; > arch/arm/boot/dts/exynos4412-trats2.dts: > cs-gpio = <&gpb 5 0>; > arch/arm/boot/dts/exynos5250-smdk5250.dts: > cs-gpio = <&gpa2 5 0>; > > ...and I guess nobody has bothered using the SPI flash on those boards > for the last year? I always try to warn people if they are breaking compatibility, but in this case I agree it is okay. Presumably no one expects BSD or vxworks support for this platform either? For the binding change: Acked-by: Rob Herring <robh@kernel.org> Rob -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Doug, On 10 June 2014 23:56, Doug Anderson <dianders@chromium.org> wrote: > Naveen, > > Not a full review, but a few quick things I happened to notice: > > On Tue, Jun 10, 2014 at 3:08 AM, Naveen Krishna Chatradhi > <ch.naveen@samsung.com> wrote: >> @@ -94,7 +93,6 @@ Example: >> spi-max-frequency = <10000>; >> >> controller-data { >> - cs-gpio = <&gpa2 5 1 0 3>; >> samsung,spi-feedback-delay = <0>; > > In a future patch (not this one!) it might make sense to also move > spi-feedback-delay out. After this change, "cs->line" in the struct s3c64xx_spi_csinfo {} is useless. Will take others opinion and remove "fb_delay" too. > > >> +static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi) > > I believe that you can make use of the fact that the SPI core already > got these GPIOs for you. See of_spi_register_master(). Everything > you need ought to be in master->num_chipselect and master->cs_gpios. cs->line is a duplicate and can be replaced with master->cs_gpios from SPI core. > > Strangely, it doesn't look like any other drivers use this, so > possibly I'm confused... > > >> @@ -806,9 +815,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi) >> struct s3c64xx_spi_info *sci; >> int err; >> >> + if (!spi->dev.of_node) { >> + dev_err(&spi->dev, "device node not found\n"); >> + return -EINVAL; >> + } > > This seems incredibly broken. You're saying that the SPI driver no > longer works without the device tree? I don't think that's desirable, > but I suppose I could be wrong. I will move this check to the s3c64xx_get_slave_ctrldata() function. I guess the non-DT version of the driver is broken too. For non-DT driver sdd->cs_gpio is true, but the cs->line is never initialized. > > Also note that you still left an "if" test below for trying to deal > with the fact that there is no "of_node". > > > -Doug
Hello Tomasz, On 11 June 2014 01:19, Tomasz Figa <tomasz.figa@gmail.com> wrote: > Hi Naveen, > > On 10.06.2014 12:08, Naveen Krishna Chatradhi wrote: >> Currently, spi-s3c64xx.c needs "cs-gpio" chip select GPIO to be >> defined under "controller-data" node under each slave node. > > [snip] > >> @@ -85,6 +83,7 @@ Example: >> #size-cells = <0>; >> pinctrl-names = "default"; >> pinctrl-0 = <&spi0_bus>; >> + cs-gpios = <&gpa2 5 1 0 3>; > > While at it, you might also update the example to something more > appropriate on Samsung platforms, e.g. <&gpa2 5 0>; Sylwester gave this comment, will club with the other changes. > >> >> w25q80bw@0 { >> #address-cells = <1>; > > [snip] > >> +static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi) >> +{ >> + struct device_node *parent_np = NULL; >> + struct s3c64xx_spi_driver_data *sdd; >> + struct s3c64xx_spi_csinfo *cs; >> + >> + parent_np = of_get_parent(spi->dev.of_node); >> + if (!parent_np) { >> + dev_err(&spi->dev, "Parent node not found\n"); >> return ERR_PTR(-EINVAL); >> } >> >> + sdd = spi_master_get_devdata(spi->master); >> + >> cs = kzalloc(sizeof(*cs), GFP_KERNEL); >> if (!cs) { >> - of_node_put(data_np); >> + of_node_put(parent_np); >> 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); >> + cs->line = of_get_named_gpio(parent_np, "cs-gpios", 0); > > This is wrong. The "cs-gpios" property is supposed to be an array, > indexed by chip select number of SPI devices (indicated by their "reg" > properties). > > Moreover, is there a need to parse this manually in this driver? I can > see respective parsing code in of_spi_register_master(). Right, spi_add_device() parses the "cs-gpios" and gives the "spi->cs_gpio" before calling the driver setup() function. Will respin with appropriate changes. > > Best regards, > Tomasz
On 10/06/14 22:32, Rob Herring wrote: > On Tue, Jun 10, 2014 at 1:09 PM, Doug Anderson <dianders@chromium.org> wrote: >> Naveen / Sylwester, >> >> On Tue, Jun 10, 2014 at 4:00 AM, Naveen Krishna Ch >> <naveenkrishna.ch@gmail.com> wrote: >>>> Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ? >>>> After your change all DTBs using the original pattern will not work with >>>> new kernels any more. At least I would expect such backward compatibility >>>> maintained for few kernel releases. >>> >>> The reason behind removing the "cs-gpio" or not providing backward >>> compatibility was >>> >>> 1. Since spi-core started using "cs-gpios" string from spi device node >>> several months ago. >>> The spi-s3c64xx.c driver is partially broken for more than 6 months. >>> >>> 2. Supporting "cs-gpio" would add extra bit of code. >>> >>> I've corrected the dts files that were using "cs-gpio" under >>> "controller-data"(child node) >>> to use "cs-gpio" from spi device node (parent node). Normally properties get removed from existing dts files and then marked in the binding documentation as deprecated. Or least in the commit messages it would be good to have clearly explained what was replaced with what and what were the main reasons. This is especially useful for out-of-tree dts files, when you port a board to newer kernel, things suddenly stop working and you're left with little or no clue why. >>> I will make another version if you insist. I'm OK with dropping the old binding, I'm generally not a proponent of a strict DT backward compatibility. But we _must_ have things documented properly, so it all doesn't turn into a debugging nightmare. >> Yup, as near as I can tell this has been broken since (3146bee spi: >> s3c64xx: Added provision for dedicated cs pin). That landed June 25th >> of 2013 into the SPI tree. >> >> ...so clearly nobody was really testing Samsung's SPI driver on ToT >> Linux. 1 year of unnoticed brokenness seems like enough time that we >> could do away with the old code. If someone comes out of the woodwork >> and claims a dire need to support the old binding then it can be added >> then. >> >> In-tree users of this appear to be: >> >> arch/arm/boot/dts/exynos4210-smdkv310.dts: >> cs-gpio = <&gpc1 2 0>; >> arch/arm/boot/dts/exynos4412-trats2.dts: >> cs-gpio = <&gpb 5 0>; >> arch/arm/boot/dts/exynos5250-smdk5250.dts: >> cs-gpio = <&gpa2 5 0>; >> >> ...and I guess nobody has bothered using the SPI flash on those boards >> for the last year? I guess the SMDK boards are not really well maintained now. I've got a working kernel for trats2 based on v3.15-rc4, the SPI is working fine there, despite the offending commit you point out. It seems the GPB[5]/SPI_1_nSS pin is in the reset default state, i.e. an input with pull down enabled (gpb-5 is not a part of the SPI interface pinctrl group). So in case we don't pass the CS GPIO in the SPI controller node or the driver doesn't handle it properly for any other reason the chip select signal is always active and the SPI communication works. > I always try to warn people if they are breaking compatibility, but in > this case I agree it is okay. Presumably no one expects BSD or vxworks > support for this platform either? > > For the binding change: > > Acked-by: Rob Herring <robh@kernel.org> > > Rob -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt index 86aa061..13bfcb5 100644 --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt @@ -42,15 +42,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 1 0 3>; 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..4594dde 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -750,47 +750,56 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master, } static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata( - struct spi_device *spi) + struct spi_device *spi, + struct s3c64xx_spi_csinfo *cs) { - struct s3c64xx_spi_csinfo *cs; - struct device_node *slave_np, *data_np = NULL; - struct s3c64xx_spi_driver_data *sdd; + struct device_node *data_np = NULL; u32 fb_delay = 0; - sdd = spi_master_get_devdata(spi->master); - slave_np = spi->dev.of_node; - if (!slave_np) { - dev_err(&spi->dev, "device node not found\n"); + data_np = of_get_child_by_name(spi->dev.of_node, "controller-data"); + if (!data_np) { + dev_err(&spi->dev, "child node 'controller-data' not found\n"); 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"); + of_property_read_u32(data_np, "samsung,spi-feedback-delay", &fb_delay); + cs->fb_delay = fb_delay; + of_node_put(data_np); + + return cs; +} + +static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi) +{ + struct device_node *parent_np = NULL; + struct s3c64xx_spi_driver_data *sdd; + struct s3c64xx_spi_csinfo *cs; + + parent_np = of_get_parent(spi->dev.of_node); + if (!parent_np) { + dev_err(&spi->dev, "Parent node not found\n"); return ERR_PTR(-EINVAL); } + sdd = spi_master_get_devdata(spi->master); + cs = kzalloc(sizeof(*cs), GFP_KERNEL); if (!cs) { - of_node_put(data_np); + of_node_put(parent_np); 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); + cs->line = of_get_named_gpio(parent_np, "cs-gpios", 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); + of_node_put(parent_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); - return cs; + return s3c64xx_get_slave_ctrldata(spi, cs); } /* @@ -806,9 +815,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi) struct s3c64xx_spi_info *sci; int err; + if (!spi->dev.of_node) { + dev_err(&spi->dev, "device node not found\n"); + return -EINVAL; + } + sdd = spi_master_get_devdata(spi->master); if (!cs && spi->dev.of_node) { - cs = s3c64xx_get_slave_ctrldata(spi); + cs = s3c64xx_get_cs_gpios(spi); spi->controller_data = cs; } @@ -1077,7 +1091,7 @@ static int s3c64xx_spi_probe(struct platform_device *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)) + if (!of_find_property(pdev->dev.of_node, "cs-gpios", NULL)) sdd->cs_gpio = false; ret = of_alias_get_id(pdev->dev.of_node, "spi");
Currently, spi-s3c64xx.c needs "cs-gpio" chip select GPIO to be defined under "controller-data" node under each slave node. &spi_x { cs-gpios <>; ... slave_node { controller-data { cs-gpio = <>; ... }; ... }; ... }; Where as, SPI core and many other drivers uses "cs-gpios" for from device tree node. Hence, make changes in spi-s3c64xx.c driver to make use of "cs-gpios" from SPI node(parent) instead of "cs-gpio" defined in slaves "controller-data"(child) node. Also updates the Device tree Documentation. Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com> Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk> Cc: Doug Anderson <dianders@chromium.org> --- Changes since v1: 1. fixed a compilation warning thus squashing the other patch into this. 2. Updated device tree description in spi-samsung.txt .../devicetree/bindings/spi/spi-samsung.txt | 8 ++- drivers/spi/spi-s3c64xx.c | 56 ++++++++++++-------- 2 files changed, 38 insertions(+), 26 deletions(-)