| Message ID | 1650032528-118220-2-git-send-email-zhouyanjie@wanyeetech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve SPI support for Ingenic SoCs. | expand |
Hi Zhou, Le ven., avril 15 2022 at 22:22:06 +0800, 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> a écrit : > Add support for using GPIOs as chip select lines on Ingenic SoCs. > > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> > --- > drivers/spi/spi-ingenic.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/spi/spi-ingenic.c b/drivers/spi/spi-ingenic.c > index 03077a7..672e4ed 100644 > --- a/drivers/spi/spi-ingenic.c > +++ b/drivers/spi/spi-ingenic.c > @@ -380,7 +380,7 @@ static int spi_ingenic_probe(struct > platform_device *pdev) > struct spi_controller *ctlr; > struct ingenic_spi *priv; > void __iomem *base; > - int ret; > + int num_cs, ret; > > pdata = of_device_get_match_data(dev); > if (!pdata) { > @@ -416,6 +416,11 @@ static int spi_ingenic_probe(struct > platform_device *pdev) > if (IS_ERR(priv->flen_field)) > return PTR_ERR(priv->flen_field); > > + if (of_property_read_u32(dev->of_node, "num-cs", &num_cs)) { > + dev_warn(dev, "Number of chip select lines not specified.\n"); > + num_cs = 2; > + } > + > platform_set_drvdata(pdev, ctlr); > > ctlr->prepare_transfer_hardware = spi_ingenic_prepare_hardware; > @@ -429,7 +434,9 @@ static int spi_ingenic_probe(struct > platform_device *pdev) > ctlr->bits_per_word_mask = pdata->bits_per_word_mask; > ctlr->min_speed_hz = 7200; > ctlr->max_speed_hz = 54000000; > - ctlr->num_chipselect = 2; > + ctlr->use_gpio_descriptors = true; I wonder if this should be set conditionally instead. Maybe set it to "true" if the "num-cs" property exists? The rest looks good to me. Cheers, -Paul > + ctlr->max_native_cs = 2; > + ctlr->num_chipselect = num_cs; > ctlr->dev.of_node = pdev->dev.of_node; > > if (spi_ingenic_request_dma(ctlr, dev)) > -- > 2.7.4 >
Hi Paul, On 2022/4/15 下午11:00, Paul Cercueil wrote: > Hi Zhou, > > Le ven., avril 15 2022 at 22:22:06 +0800, 周琰杰 (Zhou Yanjie) > <zhouyanjie@wanyeetech.com> a écrit : >> Add support for using GPIOs as chip select lines on Ingenic SoCs. >> >> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> >> --- >> drivers/spi/spi-ingenic.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/spi/spi-ingenic.c b/drivers/spi/spi-ingenic.c >> index 03077a7..672e4ed 100644 >> --- a/drivers/spi/spi-ingenic.c >> +++ b/drivers/spi/spi-ingenic.c >> @@ -380,7 +380,7 @@ static int spi_ingenic_probe(struct >> platform_device *pdev) >> struct spi_controller *ctlr; >> struct ingenic_spi *priv; >> void __iomem *base; >> - int ret; >> + int num_cs, ret; >> >> pdata = of_device_get_match_data(dev); >> if (!pdata) { >> @@ -416,6 +416,11 @@ static int spi_ingenic_probe(struct >> platform_device *pdev) >> if (IS_ERR(priv->flen_field)) >> return PTR_ERR(priv->flen_field); >> >> + if (of_property_read_u32(dev->of_node, "num-cs", &num_cs)) { >> + dev_warn(dev, "Number of chip select lines not specified.\n"); >> + num_cs = 2; >> + } >> + >> platform_set_drvdata(pdev, ctlr); >> >> ctlr->prepare_transfer_hardware = spi_ingenic_prepare_hardware; >> @@ -429,7 +434,9 @@ static int spi_ingenic_probe(struct >> platform_device *pdev) >> ctlr->bits_per_word_mask = pdata->bits_per_word_mask; >> ctlr->min_speed_hz = 7200; >> ctlr->max_speed_hz = 54000000; >> - ctlr->num_chipselect = 2; >> + ctlr->use_gpio_descriptors = true; > > I wonder if this should be set conditionally instead. Maybe set it to > "true" if the "num-cs" property exists? > I'm not too sure, but it seems some other drivers like "spi-sun6i.c", "spi-stm32.c", "spi-s3c64xx.c", "spi-pic32.c", etc. set it unconditionally. > The rest looks good to me. > > Cheers, > -Paul > >> + ctlr->max_native_cs = 2; >> + ctlr->num_chipselect = num_cs; >> ctlr->dev.of_node = pdev->dev.of_node; >> >> if (spi_ingenic_request_dma(ctlr, dev)) >> -- >> 2.7.4 >> >
Hi Zhou, Le sam., avril 16 2022 at 19:55:13 +0800, Zhou Yanjie <zhouyanjie@wanyeetech.com> a écrit : > Hi Paul, > > On 2022/4/15 下午11:00, Paul Cercueil wrote: >> Hi Zhou, >> >> Le ven., avril 15 2022 at 22:22:06 +0800, 周琰杰 (Zhou Yanjie) >> <zhouyanjie@wanyeetech.com> a écrit : >>> Add support for using GPIOs as chip select lines on Ingenic SoCs. >>> >>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> >>> --- >>> drivers/spi/spi-ingenic.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/spi/spi-ingenic.c b/drivers/spi/spi-ingenic.c >>> index 03077a7..672e4ed 100644 >>> --- a/drivers/spi/spi-ingenic.c >>> +++ b/drivers/spi/spi-ingenic.c >>> @@ -380,7 +380,7 @@ static int spi_ingenic_probe(struct >>> platform_device *pdev) >>> struct spi_controller *ctlr; >>> struct ingenic_spi *priv; >>> void __iomem *base; >>> - int ret; >>> + int num_cs, ret; >>> >>> pdata = of_device_get_match_data(dev); >>> if (!pdata) { >>> @@ -416,6 +416,11 @@ static int spi_ingenic_probe(struct >>> platform_device *pdev) >>> if (IS_ERR(priv->flen_field)) >>> return PTR_ERR(priv->flen_field); >>> >>> + if (of_property_read_u32(dev->of_node, "num-cs", &num_cs)) { >>> + dev_warn(dev, "Number of chip select lines not >>> specified.\n"); >>> + num_cs = 2; >>> + } >>> + >>> platform_set_drvdata(pdev, ctlr); >>> >>> ctlr->prepare_transfer_hardware = spi_ingenic_prepare_hardware; >>> @@ -429,7 +434,9 @@ static int spi_ingenic_probe(struct >>> platform_device *pdev) >>> ctlr->bits_per_word_mask = pdata->bits_per_word_mask; >>> ctlr->min_speed_hz = 7200; >>> ctlr->max_speed_hz = 54000000; >>> - ctlr->num_chipselect = 2; >>> + ctlr->use_gpio_descriptors = true; >> >> I wonder if this should be set conditionally instead. Maybe set it >> to "true" if the "num-cs" property exists? >> > > I'm not too sure, but it seems some other drivers like "spi-sun6i.c", > "spi-stm32.c", "spi-s3c64xx.c", "spi-pic32.c", etc. set it > unconditionally. Ok, maybe Mark can enlighten us here. Cheers, -Paul >> The rest looks good to me. >> >> Cheers, >> -Paul >> >>> + ctlr->max_native_cs = 2; >>> + ctlr->num_chipselect = num_cs; >>> ctlr->dev.of_node = pdev->dev.of_node; >>> >>> if (spi_ingenic_request_dma(ctlr, dev)) >>> -- >>> 2.7.4 >>> >>
On Sat, Apr 16, 2022 at 05:36:46PM +0100, Paul Cercueil wrote: > Le sam., avril 16 2022 at 19:55:13 +0800, Zhou Yanjie > > On 2022/4/15 下午11:00, Paul Cercueil wrote: > > > > - ctlr->num_chipselect = 2; > > > > + ctlr->use_gpio_descriptors = true; > > > I wonder if this should be set conditionally instead. Maybe set it > > > to "true" if the "num-cs" property exists? > > I'm not too sure, but it seems some other drivers like "spi-sun6i.c", > > "spi-stm32.c", "spi-s3c64xx.c", "spi-pic32.c", etc. set it > > unconditionally. > Ok, maybe Mark can enlighten us here. use_gpio_descriptions is just selecting which version of the GPIO APIs we should use if we're handling GPIOs rather than if we should handle them. We've got one last driver using the numerical GPIO APIs, once that one is converted we should just be able to drop the flag since everything will be using descriptors.
Hi, Le mar., avril 19 2022 at 18:00:41 +0100, Mark Brown <broonie@kernel.org> a écrit : > On Sat, Apr 16, 2022 at 05:36:46PM +0100, Paul Cercueil wrote: >> Le sam., avril 16 2022 at 19:55:13 +0800, Zhou Yanjie >> > On 2022/4/15 下午11:00, Paul Cercueil wrote: > >> > > > - ctlr->num_chipselect = 2; >> > > > + ctlr->use_gpio_descriptors = true; > >> > > I wonder if this should be set conditionally instead. Maybe set >> it >> > > to "true" if the "num-cs" property exists? > >> > I'm not too sure, but it seems some other drivers like >> "spi-sun6i.c", >> > "spi-stm32.c", "spi-s3c64xx.c", "spi-pic32.c", etc. set it >> > unconditionally. > >> Ok, maybe Mark can enlighten us here. > > use_gpio_descriptions is just selecting which version of the GPIO APIs > we should use if we're handling GPIOs rather than if we should handle > them. We've got one last driver using the numerical GPIO APIs, once > that one is converted we should just be able to drop the flag since > everything will be using descriptors. Thank you Mark. Cheers, -Paul
Hi Zhou, Le ven., avril 15 2022 at 22:22:06 +0800, 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> a écrit : > Add support for using GPIOs as chip select lines on Ingenic SoCs. > > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> > --- > drivers/spi/spi-ingenic.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/spi/spi-ingenic.c b/drivers/spi/spi-ingenic.c > index 03077a7..672e4ed 100644 > --- a/drivers/spi/spi-ingenic.c > +++ b/drivers/spi/spi-ingenic.c > @@ -380,7 +380,7 @@ static int spi_ingenic_probe(struct > platform_device *pdev) > struct spi_controller *ctlr; > struct ingenic_spi *priv; > void __iomem *base; > - int ret; > + int num_cs, ret; > > pdata = of_device_get_match_data(dev); > if (!pdata) { > @@ -416,6 +416,11 @@ static int spi_ingenic_probe(struct > platform_device *pdev) > if (IS_ERR(priv->flen_field)) > return PTR_ERR(priv->flen_field); > > + if (of_property_read_u32(dev->of_node, "num-cs", &num_cs)) { One small comment here - I think it would be better to use device_property_read_u32(). The driver should also use device_get_match_data() instead of of_device_get_match_data(), but that's a cleanup that can be done later. Cheers, -Paul > + dev_warn(dev, "Number of chip select lines not specified.\n"); > + num_cs = 2; > + } > + > platform_set_drvdata(pdev, ctlr); > > ctlr->prepare_transfer_hardware = spi_ingenic_prepare_hardware; > @@ -429,7 +434,9 @@ static int spi_ingenic_probe(struct > platform_device *pdev) > ctlr->bits_per_word_mask = pdata->bits_per_word_mask; > ctlr->min_speed_hz = 7200; > ctlr->max_speed_hz = 54000000; > - ctlr->num_chipselect = 2; > + ctlr->use_gpio_descriptors = true; > + ctlr->max_native_cs = 2; > + ctlr->num_chipselect = num_cs; > ctlr->dev.of_node = pdev->dev.of_node; > > if (spi_ingenic_request_dma(ctlr, dev)) > -- > 2.7.4 >
Hi Mark, On 2022/4/20 上午1:00, Mark Brown wrote: > On Sat, Apr 16, 2022 at 05:36:46PM +0100, Paul Cercueil wrote: >> Le sam., avril 16 2022 at 19:55:13 +0800, Zhou Yanjie >>> On 2022/4/15 下午11:00, Paul Cercueil wrote: >>>>> - ctlr->num_chipselect = 2; >>>>> + ctlr->use_gpio_descriptors = true; >>>> I wonder if this should be set conditionally instead. Maybe set it >>>> to "true" if the "num-cs" property exists? >>> I'm not too sure, but it seems some other drivers like "spi-sun6i.c", >>> "spi-stm32.c", "spi-s3c64xx.c", "spi-pic32.c", etc. set it >>> unconditionally. >> Ok, maybe Mark can enlighten us here. > use_gpio_descriptions is just selecting which version of the GPIO APIs > we should use if we're handling GPIOs rather than if we should handle > them. We've got one last driver using the numerical GPIO APIs, once > that one is converted we should just be able to drop the flag since > everything will be using descriptors. Thanks for your answer!
Hi Paul, On 2022/4/20 上午1:13, Paul Cercueil wrote: > Hi Zhou, > > Le ven., avril 15 2022 at 22:22:06 +0800, 周琰杰 (Zhou Yanjie) > <zhouyanjie@wanyeetech.com> a écrit : >> Add support for using GPIOs as chip select lines on Ingenic SoCs. >> >> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> >> --- >> drivers/spi/spi-ingenic.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/spi/spi-ingenic.c b/drivers/spi/spi-ingenic.c >> index 03077a7..672e4ed 100644 >> --- a/drivers/spi/spi-ingenic.c >> +++ b/drivers/spi/spi-ingenic.c >> @@ -380,7 +380,7 @@ static int spi_ingenic_probe(struct >> platform_device *pdev) >> struct spi_controller *ctlr; >> struct ingenic_spi *priv; >> void __iomem *base; >> - int ret; >> + int num_cs, ret; >> >> pdata = of_device_get_match_data(dev); >> if (!pdata) { >> @@ -416,6 +416,11 @@ static int spi_ingenic_probe(struct >> platform_device *pdev) >> if (IS_ERR(priv->flen_field)) >> return PTR_ERR(priv->flen_field); >> >> + if (of_property_read_u32(dev->of_node, "num-cs", &num_cs)) { > > One small comment here - I think it would be better to use > device_property_read_u32(). > > The driver should also use device_get_match_data() instead of > of_device_get_match_data(), but that's a cleanup that can be done later. > Sure, I will send v2. Thanks and best regards! > Cheers, > -Paul > >> + dev_warn(dev, "Number of chip select lines not specified.\n"); >> + num_cs = 2; >> + } >> + >> platform_set_drvdata(pdev, ctlr); >> >> ctlr->prepare_transfer_hardware = spi_ingenic_prepare_hardware; >> @@ -429,7 +434,9 @@ static int spi_ingenic_probe(struct >> platform_device *pdev) >> ctlr->bits_per_word_mask = pdata->bits_per_word_mask; >> ctlr->min_speed_hz = 7200; >> ctlr->max_speed_hz = 54000000; >> - ctlr->num_chipselect = 2; >> + ctlr->use_gpio_descriptors = true; >> + ctlr->max_native_cs = 2; >> + ctlr->num_chipselect = num_cs; >> ctlr->dev.of_node = pdev->dev.of_node; >> >> if (spi_ingenic_request_dma(ctlr, dev)) >> -- >> 2.7.4 >> >
diff --git a/drivers/spi/spi-ingenic.c b/drivers/spi/spi-ingenic.c index 03077a7..672e4ed 100644 --- a/drivers/spi/spi-ingenic.c +++ b/drivers/spi/spi-ingenic.c @@ -380,7 +380,7 @@ static int spi_ingenic_probe(struct platform_device *pdev) struct spi_controller *ctlr; struct ingenic_spi *priv; void __iomem *base; - int ret; + int num_cs, ret; pdata = of_device_get_match_data(dev); if (!pdata) { @@ -416,6 +416,11 @@ static int spi_ingenic_probe(struct platform_device *pdev) if (IS_ERR(priv->flen_field)) return PTR_ERR(priv->flen_field); + if (of_property_read_u32(dev->of_node, "num-cs", &num_cs)) { + dev_warn(dev, "Number of chip select lines not specified.\n"); + num_cs = 2; + } + platform_set_drvdata(pdev, ctlr); ctlr->prepare_transfer_hardware = spi_ingenic_prepare_hardware; @@ -429,7 +434,9 @@ static int spi_ingenic_probe(struct platform_device *pdev) ctlr->bits_per_word_mask = pdata->bits_per_word_mask; ctlr->min_speed_hz = 7200; ctlr->max_speed_hz = 54000000; - ctlr->num_chipselect = 2; + ctlr->use_gpio_descriptors = true; + ctlr->max_native_cs = 2; + ctlr->num_chipselect = num_cs; ctlr->dev.of_node = pdev->dev.of_node; if (spi_ingenic_request_dma(ctlr, dev))
Add support for using GPIOs as chip select lines on Ingenic SoCs. Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> --- drivers/spi/spi-ingenic.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)