| Message ID | 1425655578-22400-2-git-send-email-iivanov@mm-sol.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/06/15 07:26, Ivan T. Ivanov wrote: > Ensure that driver is owner of the GPIO's used for CS signals. Why? What happens if we don't? > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > --- > drivers/spi/spi-qup.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c > index 2b2c359..a07ba46 100644 > --- a/drivers/spi/spi-qup.c > +++ b/drivers/spi/spi-qup.c > @@ -14,11 +14,13 @@ > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/err.h> > +#include <linux/gpio.h> > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/list.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_gpio.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/spi/spi.h> > @@ -499,7 +501,7 @@ static int spi_qup_probe(struct platform_device *pdev) > struct device *dev; > void __iomem *base; > u32 max_freq, iomode, num_cs; > - int ret, irq, size; > + int ret, irq, size, cs, cs_gpio; > > dev = &pdev->dev; > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > @@ -556,6 +558,19 @@ static int spi_qup_probe(struct platform_device *pdev) > else > master->num_chipselect = num_cs; > > + for (cs = 0; cs < master->num_chipselect; cs++) { > + cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs); > + > + if (!gpio_is_valid(cs_gpio)) > + continue; > + > + ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs"); > + if (ret) { > + dev_err(&pdev->dev, "can't get cs gpios\n"); > + goto error; > + } > + } > + > master->bus_num = pdev->id; Is this related to [1]? In that case I was just relying on DT/pinctrl to properly request the gpios. [1] https://lkml.org/lkml/2014/5/27/784
On Fri, Mar 06, 2015 at 05:26:18PM +0200, Ivan T. Ivanov wrote: > Ensure that driver is owner of the GPIO's used for CS signals. > + for (cs = 0; cs < master->num_chipselect; cs++) { > + cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs); > + Any new GPIO users should really be using the gpiod API, however this is duplicating core functionality so as Stephen says why are we doing this?
Hi Stephen, > On Mar 6, 2015, at 8:34 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 03/06/15 07:26, Ivan T. Ivanov wrote: > > Ensure that driver is owner of the GPIO's used for CS signals. > Why? What happens if we don’t? We can have wrong DT configuration, which could reconfigure GPIO’s without any warning or error. > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > > --- > > drivers/spi/spi-qup.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c > > index 2b2c359..a07ba46 100644 > > --- a/drivers/spi/spi-qup.c > > +++ b/drivers/spi/spi-qup.c > > @@ -14,11 +14,13 @@ > > #include <linux/clk.h> > > #include <linux/delay.h> > > #include <linux/err.h> > > +#include <linux/gpio.h> > > #include <linux/interrupt.h> > > #include <linux/io.h> > > #include <linux/list.h> > > #include <linux/module.h> > > #include <linux/of.h> > > +#include <linux/of_gpio.h> > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > #include <linux/spi/spi.h> > > @@ -499,7 +501,7 @@ static int spi_qup_probe(struct platform_device *pdev) > > struct device *dev; > > void __iomem *base; > > u32 max_freq, iomode, num_cs; > > - int ret, irq, size; > > + int ret, irq, size, cs, cs_gpio; > > > > dev = &pdev->dev; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > @@ -556,6 +558,19 @@ static int spi_qup_probe(struct platform_device *pdev) > > else > > master->num_chipselect = num_cs; > > > > + for (cs = 0; cs < master->num_chipselect; cs++) { > > + cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs); > > + > > + if (!gpio_is_valid(cs_gpio)) > > + continue; > > + > > + ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs"); > > + if (ret) { > > + dev_err(&pdev->dev, "can't get cs gpios\n"); > > + goto error; > > + } > > + } > > + > > master->bus_num = pdev->id; > Is this related to [1]? In that case I was just relying on DT/pinctrl to > properly request the gpios. But the DT/pinctrl did not request GPIO’s, it just configure them. For some reason we are ending without any pinctrl_map of type PIN_MAP_TYPE_MUX_GROUP, which is used for pin reservation. > [1] https://lkml.org/lkml/2014/5/27/784 I have missed this one. I have considered same approach, but abandoned it, because of issues related to double request reasons mentioned by Mark. Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/09/15 01:20, Ivan T. Ivanov wrote: > Hi Stephen, > >> On Mar 6, 2015, at 8:34 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 03/06/15 07:26, Ivan T. Ivanov wrote: >>> Ensure that driver is owner of the GPIO's used for CS signals. >> Why? What happens if we don’t? > We can have wrong DT configuration, which could reconfigure > GPIO’s without any warning or error. Ouch. That sounds bad. Can you please add this information to the commit text? > >>> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> >>> --- >>> drivers/spi/spi-qup.c | 17 ++++++++++++++++- >>> 1 file changed, 16 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c >>> index 2b2c359..a07ba46 100644 >>> --- a/drivers/spi/spi-qup.c >>> +++ b/drivers/spi/spi-qup.c >>> @@ -14,11 +14,13 @@ >>> #include <linux/clk.h> >>> #include <linux/delay.h> >>> #include <linux/err.h> >>> +#include <linux/gpio.h> >>> #include <linux/interrupt.h> >>> #include <linux/io.h> >>> #include <linux/list.h> >>> #include <linux/module.h> >>> #include <linux/of.h> >>> +#include <linux/of_gpio.h> >>> #include <linux/platform_device.h> >>> #include <linux/pm_runtime.h> >>> #include <linux/spi/spi.h> >>> @@ -499,7 +501,7 @@ static int spi_qup_probe(struct platform_device *pdev) >>> struct device *dev; >>> void __iomem *base; >>> u32 max_freq, iomode, num_cs; >>> - int ret, irq, size; >>> + int ret, irq, size, cs, cs_gpio; >>> >>> dev = &pdev->dev; >>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> @@ -556,6 +558,19 @@ static int spi_qup_probe(struct platform_device *pdev) >>> else >>> master->num_chipselect = num_cs; >>> >>> + for (cs = 0; cs < master->num_chipselect; cs++) { >>> + cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs); >>> + >>> + if (!gpio_is_valid(cs_gpio)) >>> + continue; >>> + >>> + ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs"); >>> + if (ret) { >>> + dev_err(&pdev->dev, "can't get cs gpios\n"); >>> + goto error; >>> + } >>> + } >>> + >>> master->bus_num = pdev->id; >> Is this related to [1]? In that case I was just relying on DT/pinctrl to >> properly request the gpios. > But the DT/pinctrl did not request GPIO’s, it just configure them. > For some reason we are ending without any pinctrl_map of type > PIN_MAP_TYPE_MUX_GROUP, which is used for pin reservation. > Ah ok. I seem to be misremembering the details. Can you please use the gpiod interfaces here (devm_gpiod_get_index)? That's more modern. Also, don't print any error because -EPROBE_DEFER may come out and because __gpiod_get_index() already prints an error on failure at the debug level.
On Mon, 2015-03-09 at 18:28 +0000, Mark Brown wrote: > On Mon, Mar 09, 2015 at 10:23:35AM +0200, Ivan T. Ivanov wrote: > > Hi, > > Don't reply off list unless there's a good reason... Sorry, it was not intentional. Wrong button. > > > > Any new GPIO users should really be using the gpiod API, however this is > > > duplicating core functionality so as Stephen says why are we doing this? > > > About the API usage, point taken. GPIO requesting part is more important > > in this case. pinctrl core did not request pins and wrong DT configuration > > could lead to surprises without any warnings or errors. > > That doesn't answer my concern at all. I am not sure that I am following you. I can not use spi_master::cs_gpios, which is populated by of_spi_register_master(), because spi_register_master() populate SPI devices and they could issue setup method. Requesting GPIO's in core framework is also not a easy option because of arguments here[1]. Probably I am still missing something. Regards, Ivan [1] https://lkml.org/lkml/2014/5/24/75 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, 2015-03-09 at 11:53 -0700, Stephen Boyd wrote: > On 03/09/15 01:20, Ivan T. Ivanov wrote: > > Hi Stephen, > > > > > On Mar 6, 2015, at 8:34 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > > On 03/06/15 07:26, Ivan T. Ivanov wrote: > > > > Ensure that driver is owner of the GPIO's used for CS signals. > > > Why? What happens if we don’t? > > We can have wrong DT configuration, which could reconfigure > > GPIO’s without any warning or error. > > Ouch. That sounds bad. Can you please add this information to the commit > text? Sure. > > > > + for (cs = 0; cs < master->num_chipselect; cs++) { > > > > + cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs); > > > > + > > > > + if (!gpio_is_valid(cs_gpio)) > > > > + continue; > > > > + > > > > + ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs"); > > > > + if (ret) { > > > > + dev_err(&pdev->dev, "can't get cs gpios\n"); > > > > + goto error; > > > > + } > > > > + } > > > > + > > > > master->bus_num = pdev->id; > > > Is this related to [1]? In that case I was just relying on DT/pinctrl to > > > properly request the gpios. > > But the DT/pinctrl did not request GPIO’s, it just configure them. > > For some reason we are ending without any pinctrl_map of type > > PIN_MAP_TYPE_MUX_GROUP, which is used for pin reservation. > > > > Ah ok. I seem to be misremembering the details. :-) I still have version of downstream "msm-pinctrl" driver, which create PIN_MAP_TYPE_MUX_GROUP maps during node to map parsing. > > Can you please use the gpiod interfaces here (devm_gpiod_get_index)? Sure. > That's more modern. Also, don't print any error because -EPROBE_DEFER may come out and Well, this could not happen. Driver probe will not called until default pinctrl state is not available and we rely on this for proper driver functionality. > because __gpiod_get_index() already prints an error on > failure at the debug level. Ok. Thanks, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 10, 2015 at 10:10:56AM +0200, Ivan T. Ivanov wrote: > On Mon, 2015-03-09 at 18:28 +0000, Mark Brown wrote: > > > About the API usage, point taken. GPIO requesting part is more important > > > in this case. pinctrl core did not request pins and wrong DT configuration > > > could lead to surprises without any warnings or errors. > > That doesn't answer my concern at all. > I am not sure that I am following you. > I can not use spi_master::cs_gpios, which is populated by > of_spi_register_master(), because spi_register_master() > populate SPI devices and they could issue setup method. I'm sorry but I can't parse the above. What does "they could issue setup method" mean and why is it a problem? > Requesting GPIO's in core framework is also not a easy > option because of arguments here[1]. We should really fix that though.
On Tue, 2015-03-10 at 11:06 +0000, Mark Brown wrote: > On Tue, Mar 10, 2015 at 10:10:56AM +0200, Ivan T. Ivanov wrote: > > On Mon, 2015-03-09 at 18:28 +0000, Mark Brown wrote: > > > > > About the API usage, point taken. GPIO requesting part is more important > > > > in this case. pinctrl core did not request pins and wrong DT configuration > > > > could lead to surprises without any warnings or errors. > > > > That doesn't answer my concern at all. > > > I am not sure that I am following you. > > > I can not use spi_master::cs_gpios, which is populated by > > of_spi_register_master(), because spi_register_master() > > populate SPI devices and they could issue setup method. > > I'm sorry but I can't parse the above. What does "they could issue > setup method" mean and why is it a problem? Client drivers could execute spi_setup() in probe(), so we have to ensure that CS GPIO's are available before this, no? > > > Requesting GPIO's in core framework is also not a easy > > option because of arguments here[1]. > > We should really fix that though. > I think that pinctrl framework should automatically request pins belonging to group when state is selected. Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 10, 2015 at 02:53:17PM +0200, Ivan T. Ivanov wrote: > > On Tue, 2015-03-10 at 11:06 +0000, Mark Brown wrote: > > I'm sorry but I can't parse the above. What does "they could issue > > setup method" mean and why is it a problem? > Client drivers could execute spi_setup() in probe(), so we have > to ensure that CS GPIO's are available before this, no? I see. Yes, that's a concern. It should be in the changelog. > > > Requesting GPIO's in core framework is also not a easy > > > option because of arguments here[1]. > > We should really fix that though. > I think that pinctrl framework should automatically > request pins belonging to group when state is selected. That doesn't help users as not every GPIO is associated with a pin controller - they'd still have to handle the case where they had to do things themselves.
Hi, On Mon, 2015-03-09 at 11:53 -0700, Stephen Boyd wrote: > On 03/09/15 01:20, Ivan T. Ivanov wrote: > > Hi Stephen, > > > > > On Mar 6, 2015, at 8:34 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > > On 03/06/15 07:26, Ivan T. Ivanov wrote: > > > > Ensure that driver is owner of the GPIO's used for CS signals. > > > Why? What happens if we don’t? > > We can have wrong DT configuration, which could reconfigure > > GPIO’s without any warning or error. > > Ouch. That sounds bad. Can you please add this information to the commit > text? > > > > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > > > > --- > > > > drivers/spi/spi-qup.c | 17 ++++++++++++++++- > > > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c > > > > index 2b2c359..a07ba46 100644 > > > > --- a/drivers/spi/spi-qup.c > > > > +++ b/drivers/spi/spi-qup.c > > > > @@ -14,11 +14,13 @@ > > > > #include <linux/clk.h> > > > > #include <linux/delay.h> > > > > #include <linux/err.h> > > > > +#include <linux/gpio.h> > > > > #include <linux/interrupt.h> > > > > #include <linux/io.h> > > > > #include <linux/list.h> > > > > #include <linux/module.h> > > > > #include <linux/of.h> > > > > +#include <linux/of_gpio.h> > > > > #include <linux/platform_device.h> > > > > #include <linux/pm_runtime.h> > > > > #include <linux/spi/spi.h> > > > > @@ -499,7 +501,7 @@ static int spi_qup_probe(struct platform_device *pdev) > > > > struct device *dev; > > > > void __iomem *base; > > > > u32 max_freq, iomode, num_cs; > > > > - int ret, irq, size; > > > > + int ret, irq, size, cs, cs_gpio; > > > > > > > > dev = &pdev->dev; > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > @@ -556,6 +558,19 @@ static int spi_qup_probe(struct platform_device *pdev) > > > > else > > > > master->num_chipselect = num_cs; > > > > > > > > + for (cs = 0; cs < master->num_chipselect; cs++) { > > > > + cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs); > > > > + > > > > + if (!gpio_is_valid(cs_gpio)) > > > > + continue; > > > > + > > > > + ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs"); > > > > + if (ret) { > > > > + dev_err(&pdev->dev, "can't get cs gpios\n"); > > > > + goto error; > > > > + } > > > > + } > > > > + > > > > master->bus_num = pdev->id; > > > Is this related to [1]? In that case I was just relying on DT/pinctrl to > > > properly request the gpios. > > But the DT/pinctrl did not request GPIO’s, it just configure them. > > For some reason we are ending without any pinctrl_map of type > > PIN_MAP_TYPE_MUX_GROUP, which is used for pin reservation. I will like to withdraw this patch. It fix the problem only for CS lines, but misconfiguration could happen also for the rest for the lines. I will take a look in the pinctrl core code to see would it be possible to print warning or something, when one driver reconfigure pins already configured by another driver. Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" 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/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c index 2b2c359..a07ba46 100644 --- a/drivers/spi/spi-qup.c +++ b/drivers/spi/spi-qup.c @@ -14,11 +14,13 @@ #include <linux/clk.h> #include <linux/delay.h> #include <linux/err.h> +#include <linux/gpio.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/list.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_gpio.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/spi/spi.h> @@ -499,7 +501,7 @@ static int spi_qup_probe(struct platform_device *pdev) struct device *dev; void __iomem *base; u32 max_freq, iomode, num_cs; - int ret, irq, size; + int ret, irq, size, cs, cs_gpio; dev = &pdev->dev; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -556,6 +558,19 @@ static int spi_qup_probe(struct platform_device *pdev) else master->num_chipselect = num_cs; + for (cs = 0; cs < master->num_chipselect; cs++) { + cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs); + + if (!gpio_is_valid(cs_gpio)) + continue; + + ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs"); + if (ret) { + dev_err(&pdev->dev, "can't get cs gpios\n"); + goto error; + } + } + master->bus_num = pdev->id; master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP; master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
Ensure that driver is owner of the GPIO's used for CS signals. Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> --- drivers/spi/spi-qup.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html