| Message ID | 1497248057-16550-1-git-send-email-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jeffy, On 2017/6/12 14:14, Jeffy Chen wrote: > Support using "cs-gpios" property to specify cs gpios. > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- > > .../devicetree/bindings/spi/spi-rockchip.txt | 2 + > drivers/spi/spi-rockchip.c | 52 ++++++++++++++++++++++ > 2 files changed, 54 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt b/Documentation/devicetree/bindings/spi/spi-rockchip.txt > index 83da493..02171b2 100644 > --- a/Documentation/devicetree/bindings/spi/spi-rockchip.txt > +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt The changes for doc should be another patch, and... > @@ -17,6 +17,7 @@ Required Properties: > region. > - interrupts: The interrupt number to the cpu. The interrupt specifier format > depends on the interrupt controller. > +- cs-gpios : Specifies the gpio pins to be used for chipselects. It's not a required property, otherwise how other boards work as your patch 2 only add this for rk3399-gru. > - clocks: Must contain an entry for each entry in clock-names. > - clock-names: Shall be "spiclk" for the transfer-clock, and "apb_pclk" for > the peripheral clock. > @@ -48,6 +49,7 @@ Example: > #address-cells = <1>; > #size-cells = <0>; > interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; > + cs-gpios = <&gpio2 23 GPIO_ACTIVE_HIGH>; > clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>; > clock-names = "spiclk", "apb_pclk"; > pinctrl-0 = <&spi1_pins>; > diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c > index acf31f3..04694e1 100644 > --- a/drivers/spi/spi-rockchip.c > +++ b/drivers/spi/spi-rockchip.c > @@ -15,6 +15,7 @@ > > #include <linux/clk.h> > #include <linux/dmaengine.h> > +#include <linux/gpio.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/pinctrl/consumer.h> > @@ -201,6 +202,10 @@ struct rockchip_spi { > struct dma_slave_caps dma_caps; > }; > > +struct rockchip_spi_data { > + bool cs_gpio_requested; > +}; > + Could you fold cs_gpio_requested into struct rockchip_spi? > static inline void spi_enable_chip(struct rockchip_spi *rs, int enable) > { > writel_relaxed((enable ? 1 : 0), rs->regs + ROCKCHIP_SPI_SSIENR); > @@ -297,6 +302,50 @@ static void rockchip_spi_set_cs(struct spi_device *spi, bool enable) > pm_runtime_put_sync(rs->dev); > } > > +static int rockchip_spi_setup(struct spi_device *spi) > +{ > + int ret = 0; > + unsigned long flags = (spi->mode & SPI_CS_HIGH) ? > + GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH; > + struct rockchip_spi_data *data = spi_get_ctldata(spi); > + > + if (!gpio_is_valid(spi->cs_gpio)) > + return 0; return -EINVAL? > + > + if (!data) { > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + spi_set_ctldata(spi, data); > + } > + > + if (!data->cs_gpio_requested) { > + ret = gpio_request_one(spi->cs_gpio, flags, > + dev_name(&spi->dev)); > + if (!ret) > + data->cs_gpio_requested = 1; > + } else > + ret = gpio_direction_output(spi->cs_gpio, flags); need brace around 'else' statement. Also I don't see data used elsewhere, so you need these code above. > + > + if (ret < 0) > + dev_err(&spi->dev, "Failed to setup cs gpio(%d): %d\n", > + spi->cs_gpio, ret); > + > + return ret; > +} > + > +static void rockchip_spi_cleanup(struct spi_device *spi) > +{ > + struct rockchip_spi_data *data = spi_get_ctldata(spi); > + > + if (data) { > + if (data->cs_gpio_requested) > + gpio_free(spi->cs_gpio); > + kfree(data); > + spi_set_ctldata(spi, NULL); > + } > +} > + > static int rockchip_spi_prepare_message(struct spi_master *master, > struct spi_message *msg) > { > @@ -744,11 +793,14 @@ static int rockchip_spi_probe(struct platform_device *pdev) > master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8); > > master->set_cs = rockchip_spi_set_cs; > + master->setup = rockchip_spi_setup; > + master->cleanup = rockchip_spi_cleanup; > master->prepare_message = rockchip_spi_prepare_message; > master->unprepare_message = rockchip_spi_unprepare_message; > master->transfer_one = rockchip_spi_transfer_one; > master->max_transfer_size = rockchip_spi_max_transfer_size; > master->handle_err = rockchip_spi_handle_err; > + master->flags = SPI_MASTER_GPIO_SS; > > rs->dma_tx.ch = dma_request_chan(rs->dev, "tx"); > if (IS_ERR(rs->dma_tx.ch)) { >
On Mon, Jun 12, 2017 at 9:15 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > On 2017/6/12 14:14, Jeffy Chen wrote: >> >> Support using "cs-gpios" property to specify cs gpios. >> >> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >> index 83da493..02171b2 100644 >> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.txt >> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt >> @@ -17,6 +17,7 @@ Required Properties: >> region. >> - interrupts: The interrupt number to the cpu. The interrupt specifier >> format >> depends on the interrupt controller. >> +- cs-gpios : Specifies the gpio pins to be used for chipselects. > > It's not a required property, otherwise how other boards work as your > patch 2 only add this for rk3399-gru. >> --- a/drivers/spi/spi-rockchip.c >> +++ b/drivers/spi/spi-rockchip.c >> @@ -297,6 +302,50 @@ static void rockchip_spi_set_cs(struct spi_device >> *spi, bool enable) >> pm_runtime_put_sync(rs->dev); >> } >> +static int rockchip_spi_setup(struct spi_device *spi) >> +{ >> + int ret = 0; >> + unsigned long flags = (spi->mode & SPI_CS_HIGH) ? >> + GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH; >> + struct rockchip_spi_data *data = spi_get_ctldata(spi); >> + >> + if (!gpio_is_valid(spi->cs_gpio)) >> + return 0; > return -EINVAL? Isn't this check meant to fall back to hardware CS if no cs-gpios property is present? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Shawn, On 06/12/2017 03:15 PM, Shawn Lin wrote: > Hi Jeffy, > > On 2017/6/12 14:14, Jeffy Chen wrote: >> Support using "cs-gpios" property to specify cs gpios. >> >> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >> --- >> >> .../devicetree/bindings/spi/spi-rockchip.txt | 2 + >> drivers/spi/spi-rockchip.c | 52 >> ++++++++++++++++++++++ >> 2 files changed, 54 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt >> b/Documentation/devicetree/bindings/spi/spi-rockchip.txt >> index 83da493..02171b2 100644 >> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.txt >> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt > > The changes for doc should be another patch, and... but i saw others didn't separate them: cf9e478 spi: sh-msiof: Add slave mode support 23e291c spi: rockchip: support "sleep" pin configuration > >> @@ -17,6 +17,7 @@ Required Properties: >> region. >> - interrupts: The interrupt number to the cpu. The interrupt >> specifier format >> depends on the interrupt controller. >> +- cs-gpios : Specifies the gpio pins to be used for chipselects. > > It's not a required property, otherwise how other boards work as your > patch 2 only add this for rk3399-gru. oops, i will fix it. > >> - clocks: Must contain an entry for each entry in clock-names. >> - clock-names: Shall be "spiclk" for the transfer-clock, and >> "apb_pclk" for >> the peripheral clock. >> @@ -48,6 +49,7 @@ Example: >> #address-cells = <1>; >> #size-cells = <0>; >> interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; >> + cs-gpios = <&gpio2 23 GPIO_ACTIVE_HIGH>; >> clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>; >> clock-names = "spiclk", "apb_pclk"; >> pinctrl-0 = <&spi1_pins>; >> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c >> index acf31f3..04694e1 100644 >> --- a/drivers/spi/spi-rockchip.c >> +++ b/drivers/spi/spi-rockchip.c >> @@ -15,6 +15,7 @@ >> #include <linux/clk.h> >> #include <linux/dmaengine.h> >> +#include <linux/gpio.h> >> #include <linux/module.h> >> #include <linux/of.h> >> #include <linux/pinctrl/consumer.h> >> @@ -201,6 +202,10 @@ struct rockchip_spi { >> struct dma_slave_caps dma_caps; >> }; >> +struct rockchip_spi_data { >> + bool cs_gpio_requested; >> +}; >> + > > Could you fold cs_gpio_requested into struct rockchip_spi? > >> static inline void spi_enable_chip(struct rockchip_spi *rs, int enable) >> { >> writel_relaxed((enable ? 1 : 0), rs->regs + ROCKCHIP_SPI_SSIENR); >> @@ -297,6 +302,50 @@ static void rockchip_spi_set_cs(struct spi_device >> *spi, bool enable) >> pm_runtime_put_sync(rs->dev); >> } >> +static int rockchip_spi_setup(struct spi_device *spi) >> +{ >> + int ret = 0; >> + unsigned long flags = (spi->mode & SPI_CS_HIGH) ? >> + GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH; >> + struct rockchip_spi_data *data = spi_get_ctldata(spi); >> + >> + if (!gpio_is_valid(spi->cs_gpio)) >> + return 0; > > return -EINVAL? i think we can still support the original way, which means no "cs-gpios" and do nothing in setup. > >> + >> + if (!data) { >> + data = kzalloc(sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + spi_set_ctldata(spi, data); >> + } >> + >> + if (!data->cs_gpio_requested) { >> + ret = gpio_request_one(spi->cs_gpio, flags, >> + dev_name(&spi->dev)); >> + if (!ret) >> + data->cs_gpio_requested = 1; >> + } else >> + ret = gpio_direction_output(spi->cs_gpio, flags); > > need brace around 'else' statement. Also I don't see data used > elsewhere, so you need these code above. ok. and the cs_gpio_requested is to mark cs_gpio requested, because the setup func might be called multiple times, we only need to request gpio at the first time. > >> + >> + if (ret < 0) >> + dev_err(&spi->dev, "Failed to setup cs gpio(%d): %d\n", >> + spi->cs_gpio, ret); >> + >> + return ret; >> +} >> + >> +static void rockchip_spi_cleanup(struct spi_device *spi) >> +{ >> + struct rockchip_spi_data *data = spi_get_ctldata(spi); >> + >> + if (data) { >> + if (data->cs_gpio_requested) >> + gpio_free(spi->cs_gpio); >> + kfree(data); >> + spi_set_ctldata(spi, NULL); >> + } >> +} >> + >> static int rockchip_spi_prepare_message(struct spi_master *master, >> struct spi_message *msg) >> { >> @@ -744,11 +793,14 @@ static int rockchip_spi_probe(struct >> platform_device *pdev) >> master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8); >> master->set_cs = rockchip_spi_set_cs; >> + master->setup = rockchip_spi_setup; >> + master->cleanup = rockchip_spi_cleanup; >> master->prepare_message = rockchip_spi_prepare_message; >> master->unprepare_message = rockchip_spi_unprepare_message; >> master->transfer_one = rockchip_spi_transfer_one; >> master->max_transfer_size = rockchip_spi_max_transfer_size; >> master->handle_err = rockchip_spi_handle_err; >> + master->flags = SPI_MASTER_GPIO_SS; >> rs->dma_tx.ch = dma_request_chan(rs->dev, "tx"); >> if (IS_ERR(rs->dma_tx.ch)) { >> > > >
Hi Shawn, On 06/12/2017 04:26 PM, jeffy wrote: > Hi Shawn, > > On 06/12/2017 03:15 PM, Shawn Lin wrote: >> Hi Jeffy, >> >> On 2017/6/12 14:14, Jeffy Chen wrote: >>> Support using "cs-gpios" property to specify cs gpios. >>> >>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >>> --- >>> >>> .../devicetree/bindings/spi/spi-rockchip.txt | 2 + >>> drivers/spi/spi-rockchip.c | 52 >>> ++++++++++++++++++++++ >>> 2 files changed, 54 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt >>> b/Documentation/devicetree/bindings/spi/spi-rockchip.txt >>> index 83da493..02171b2 100644 >>> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.txt >>> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt >> >> The changes for doc should be another patch, and... > > but i saw others didn't separate them: > cf9e478 spi: sh-msiof: Add slave mode support > 23e291c spi: rockchip: support "sleep" pin configuration > > >> >>> @@ -17,6 +17,7 @@ Required Properties: >>> region. >>> - interrupts: The interrupt number to the cpu. The interrupt >>> specifier format >>> depends on the interrupt controller. >>> +- cs-gpios : Specifies the gpio pins to be used for chipselects. >> >> It's not a required property, otherwise how other boards work as your >> patch 2 only add this for rk3399-gru. > oops, i will fix it. >> >>> - clocks: Must contain an entry for each entry in clock-names. >>> - clock-names: Shall be "spiclk" for the transfer-clock, and >>> "apb_pclk" for >>> the peripheral clock. >>> @@ -48,6 +49,7 @@ Example: >>> #address-cells = <1>; >>> #size-cells = <0>; >>> interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; >>> + cs-gpios = <&gpio2 23 GPIO_ACTIVE_HIGH>; >>> clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>; >>> clock-names = "spiclk", "apb_pclk"; >>> pinctrl-0 = <&spi1_pins>; >>> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c >>> index acf31f3..04694e1 100644 >>> --- a/drivers/spi/spi-rockchip.c >>> +++ b/drivers/spi/spi-rockchip.c >>> @@ -15,6 +15,7 @@ >>> #include <linux/clk.h> >>> #include <linux/dmaengine.h> >>> +#include <linux/gpio.h> >>> #include <linux/module.h> >>> #include <linux/of.h> >>> #include <linux/pinctrl/consumer.h> >>> @@ -201,6 +202,10 @@ struct rockchip_spi { >>> struct dma_slave_caps dma_caps; >>> }; >>> +struct rockchip_spi_data { >>> + bool cs_gpio_requested; >>> +}; >>> + >> >> Could you fold cs_gpio_requested into struct rockchip_spi? we might have multiple children with different cs_gpio, so i think we may need a separate data struct for them. >> >>> static inline void spi_enable_chip(struct rockchip_spi *rs, int >>> enable) >>> { >>> writel_relaxed((enable ? 1 : 0), rs->regs + ROCKCHIP_SPI_SSIENR); >>> @@ -297,6 +302,50 @@ static void rockchip_spi_set_cs(struct spi_device >>> *spi, bool enable) >>> pm_runtime_put_sync(rs->dev); >>> } >>> +static int rockchip_spi_setup(struct spi_device *spi) >>> +{ >>> + int ret = 0; >>> + unsigned long flags = (spi->mode & SPI_CS_HIGH) ? >>> + GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH; >>> + struct rockchip_spi_data *data = spi_get_ctldata(spi); >>> + >>> + if (!gpio_is_valid(spi->cs_gpio)) >>> + return 0; >> >> return -EINVAL? > i think we can still support the original way, which means no "cs-gpios" > and do nothing in setup. >> >>> + >>> + if (!data) { >>> + data = kzalloc(sizeof(*data), GFP_KERNEL); >>> + if (!data) >>> + return -ENOMEM; >>> + spi_set_ctldata(spi, data); >>> + } >>> + >>> + if (!data->cs_gpio_requested) { >>> + ret = gpio_request_one(spi->cs_gpio, flags, >>> + dev_name(&spi->dev)); >>> + if (!ret) >>> + data->cs_gpio_requested = 1; >>> + } else >>> + ret = gpio_direction_output(spi->cs_gpio, flags); >> >> need brace around 'else' statement. Also I don't see data used >> elsewhere, so you need these code above. > ok. > and the cs_gpio_requested is to mark cs_gpio requested, because the > setup func might be called multiple times, we only need to request gpio > at the first time. >> >>> + >>> + if (ret < 0) >>> + dev_err(&spi->dev, "Failed to setup cs gpio(%d): %d\n", >>> + spi->cs_gpio, ret); >>> + >>> + return ret; >>> +} >>> + >>> +static void rockchip_spi_cleanup(struct spi_device *spi) >>> +{ >>> + struct rockchip_spi_data *data = spi_get_ctldata(spi); >>> + >>> + if (data) { >>> + if (data->cs_gpio_requested) >>> + gpio_free(spi->cs_gpio); >>> + kfree(data); >>> + spi_set_ctldata(spi, NULL); >>> + } >>> +} >>> + >>> static int rockchip_spi_prepare_message(struct spi_master *master, >>> struct spi_message *msg) >>> { >>> @@ -744,11 +793,14 @@ static int rockchip_spi_probe(struct >>> platform_device *pdev) >>> master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8); >>> master->set_cs = rockchip_spi_set_cs; >>> + master->setup = rockchip_spi_setup; >>> + master->cleanup = rockchip_spi_cleanup; >>> master->prepare_message = rockchip_spi_prepare_message; >>> master->unprepare_message = rockchip_spi_unprepare_message; >>> master->transfer_one = rockchip_spi_transfer_one; >>> master->max_transfer_size = rockchip_spi_max_transfer_size; >>> master->handle_err = rockchip_spi_handle_err; >>> + master->flags = SPI_MASTER_GPIO_SS; >>> rs->dma_tx.ch = dma_request_chan(rs->dev, "tx"); >>> if (IS_ERR(rs->dma_tx.ch)) { >>> >> >> >> >
Am Montag, 12. Juni 2017, 16:26:07 CEST schrieb jeffy: > Hi Shawn, > > On 06/12/2017 03:15 PM, Shawn Lin wrote: > > Hi Jeffy, > > > > On 2017/6/12 14:14, Jeffy Chen wrote: > >> Support using "cs-gpios" property to specify cs gpios. > >> > >> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > >> --- > >> > >> .../devicetree/bindings/spi/spi-rockchip.txt | 2 + > >> drivers/spi/spi-rockchip.c | 52 > >> > >> ++++++++++++++++++++++ > >> > >> 2 files changed, 54 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt > >> b/Documentation/devicetree/bindings/spi/spi-rockchip.txt > >> index 83da493..02171b2 100644 > >> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.txt > >> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt > > > > The changes for doc should be another patch, and... > > but i saw others didn't separate them: > cf9e478 spi: sh-msiof: Add slave mode support > 23e291c spi: rockchip: support "sleep" pin configuration it sometimes falls through the cracks, but having dt-binding patches separate is meant to make it easier on DT-Maintainers to find patches they need to look at. > >> + if (!data->cs_gpio_requested) { > >> + ret = gpio_request_one(spi->cs_gpio, flags, > >> + dev_name(&spi->dev)); > >> + if (!ret) > >> + data->cs_gpio_requested = 1; > >> + } else > >> + ret = gpio_direction_output(spi->cs_gpio, flags); > > > > need brace around 'else' statement. Also I don't see data used > > elsewhere, so you need these code above. > > ok. > and the cs_gpio_requested is to mark cs_gpio requested, because the > setup func might be called multiple times, we only need to request gpio > at the first time. Aren't the gpiod* functions meant to be used for new things? Also you might actually do a bit of error handling there, especially EPROBE_DEFER. Heiko
Hi Geert, On 06/12/2017 04:18 PM, Geert Uytterhoeven wrote: > On Mon, Jun 12, 2017 at 9:15 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> On 2017/6/12 14:14, Jeffy Chen wrote: >>> >>> Support using "cs-gpios" property to specify cs gpios. >>> >>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > >>> index 83da493..02171b2 100644 >>> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.txt >>> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt >>> @@ -17,6 +17,7 @@ Required Properties: >>> region. >>> - interrupts: The interrupt number to the cpu. The interrupt specifier >>> format >>> depends on the interrupt controller. >>> +- cs-gpios : Specifies the gpio pins to be used for chipselects. >> >> It's not a required property, otherwise how other boards work as your >> patch 2 only add this for rk3399-gru. > >>> --- a/drivers/spi/spi-rockchip.c >>> +++ b/drivers/spi/spi-rockchip.c > >>> @@ -297,6 +302,50 @@ static void rockchip_spi_set_cs(struct spi_device >>> *spi, bool enable) >>> pm_runtime_put_sync(rs->dev); >>> } >>> +static int rockchip_spi_setup(struct spi_device *spi) >>> +{ >>> + int ret = 0; >>> + unsigned long flags = (spi->mode & SPI_CS_HIGH) ? >>> + GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH; >>> + struct rockchip_spi_data *data = spi_get_ctldata(spi); >>> + >>> + if (!gpio_is_valid(spi->cs_gpio)) >>> + return 0; > >> return -EINVAL? > > Isn't this check meant to fall back to hardware CS if no cs-gpios property > is present? Thanks for your comment, and yes it is. I'll add a comment in the code to explain it :) > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > > >
Hi Heiko, thanx for your comments. On 06/12/2017 04:36 PM, Heiko Stübner wrote: > Am Montag, 12. Juni 2017, 16:26:07 CEST schrieb jeffy: >> Hi Shawn, >> >> On 06/12/2017 03:15 PM, Shawn Lin wrote: >>> Hi Jeffy, >>> >>> On 2017/6/12 14:14, Jeffy Chen wrote: >>>> Support using "cs-gpios" property to specify cs gpios. >>>> >>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >>>> --- >>>> >>>> .../devicetree/bindings/spi/spi-rockchip.txt | 2 + >>>> drivers/spi/spi-rockchip.c | 52 >>>> >>>> ++++++++++++++++++++++ >>>> >>>> 2 files changed, 54 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt >>>> b/Documentation/devicetree/bindings/spi/spi-rockchip.txt >>>> index 83da493..02171b2 100644 >>>> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.txt >>>> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt >>> >>> The changes for doc should be another patch, and... >> >> but i saw others didn't separate them: >> cf9e478 spi: sh-msiof: Add slave mode support >> 23e291c spi: rockchip: support "sleep" pin configuration > > it sometimes falls through the cracks, but having dt-binding patches > separate is meant to make it easier on DT-Maintainers to find > patches they need to look at. ok, will do. > > >>>> + if (!data->cs_gpio_requested) { >>>> + ret = gpio_request_one(spi->cs_gpio, flags, >>>> + dev_name(&spi->dev)); >>>> + if (!ret) >>>> + data->cs_gpio_requested = 1; >>>> + } else >>>> + ret = gpio_direction_output(spi->cs_gpio, flags); >>> >>> need brace around 'else' statement. Also I don't see data used >>> elsewhere, so you need these code above. >> >> ok. >> and the cs_gpio_requested is to mark cs_gpio requested, because the >> setup func might be called multiple times, we only need to request gpio >> at the first time. > > Aren't the gpiod* functions meant to be used for new things? > Also you might actually do a bit of error handling there, especially > EPROBE_DEFER. so you are suggesting to use gpiod* functions here to replace gpio_* functions right? > > > Heiko > > >
Am Montag, 12. Juni 2017, 17:12:24 CEST schrieb jeffy: > Hi Heiko, > > thanx for your comments. > > On 06/12/2017 04:36 PM, Heiko Stübner wrote: > > Am Montag, 12. Juni 2017, 16:26:07 CEST schrieb jeffy: > >> Hi Shawn, > >> > >> On 06/12/2017 03:15 PM, Shawn Lin wrote: > >>> Hi Jeffy, > >>> > >>> On 2017/6/12 14:14, Jeffy Chen wrote: > >>>> Support using "cs-gpios" property to specify cs gpios. > >>>> > >>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > >>>> --- > >>>> > >>>> .../devicetree/bindings/spi/spi-rockchip.txt | 2 + > >>>> drivers/spi/spi-rockchip.c | 52 > >>>> > >>>> ++++++++++++++++++++++ > >>>> > >>>> 2 files changed, 54 insertions(+) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt > >>>> b/Documentation/devicetree/bindings/spi/spi-rockchip.txt > >>>> index 83da493..02171b2 100644 > >>>> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.txt > >>>> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt > >>> > >>> The changes for doc should be another patch, and... > >> > >> but i saw others didn't separate them: > >> cf9e478 spi: sh-msiof: Add slave mode support > >> 23e291c spi: rockchip: support "sleep" pin configuration > > > > it sometimes falls through the cracks, but having dt-binding patches > > separate is meant to make it easier on DT-Maintainers to find > > patches they need to look at. > > ok, will do. > > >>>> + if (!data->cs_gpio_requested) { > >>>> + ret = gpio_request_one(spi->cs_gpio, flags, > >>>> + dev_name(&spi->dev)); > >>>> + if (!ret) > >>>> + data->cs_gpio_requested = 1; > >>>> + } else > >>>> + ret = gpio_direction_output(spi->cs_gpio, flags); > >>> > >>> need brace around 'else' statement. Also I don't see data used > >>> elsewhere, so you need these code above. > >> > >> ok. > >> and the cs_gpio_requested is to mark cs_gpio requested, because the > >> setup func might be called multiple times, we only need to request gpio > >> at the first time. > > > > Aren't the gpiod* functions meant to be used for new things? > > Also you might actually do a bit of error handling there, especially > > EPROBE_DEFER. > > so you are suggesting to use gpiod* functions here to replace gpio_* > functions right? correct. And handle errors that may get returned, especially EPROBE_DEFER, but also other errors may indicate that your spi won't function as expected. Heiko
On Mon, Jun 12, 2017 at 10:18:06AM +0200, Geert Uytterhoeven wrote: > On Mon, Jun 12, 2017 at 9:15 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > > On 2017/6/12 14:14, Jeffy Chen wrote: > >> +static int rockchip_spi_setup(struct spi_device *spi) > >> +{ > >> + int ret = 0; > >> + unsigned long flags = (spi->mode & SPI_CS_HIGH) ? > >> + GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH; > >> + struct rockchip_spi_data *data = spi_get_ctldata(spi); > >> + > >> + if (!gpio_is_valid(spi->cs_gpio)) > >> + return 0; > > return -EINVAL? > Isn't this check meant to fall back to hardware CS if no cs-gpios property > is present? I'd expect it to, otherwise the cs-gpios property is actualy mandatory.
diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt b/Documentation/devicetree/bindings/spi/spi-rockchip.txt index 83da493..02171b2 100644 --- a/Documentation/devicetree/bindings/spi/spi-rockchip.txt +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt @@ -17,6 +17,7 @@ Required Properties: region. - interrupts: The interrupt number to the cpu. The interrupt specifier format depends on the interrupt controller. +- cs-gpios : Specifies the gpio pins to be used for chipselects. - clocks: Must contain an entry for each entry in clock-names. - clock-names: Shall be "spiclk" for the transfer-clock, and "apb_pclk" for the peripheral clock. @@ -48,6 +49,7 @@ Example: #address-cells = <1>; #size-cells = <0>; interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; + cs-gpios = <&gpio2 23 GPIO_ACTIVE_HIGH>; clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>; clock-names = "spiclk", "apb_pclk"; pinctrl-0 = <&spi1_pins>; diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index acf31f3..04694e1 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -15,6 +15,7 @@ #include <linux/clk.h> #include <linux/dmaengine.h> +#include <linux/gpio.h> #include <linux/module.h> #include <linux/of.h> #include <linux/pinctrl/consumer.h> @@ -201,6 +202,10 @@ struct rockchip_spi { struct dma_slave_caps dma_caps; }; +struct rockchip_spi_data { + bool cs_gpio_requested; +}; + static inline void spi_enable_chip(struct rockchip_spi *rs, int enable) { writel_relaxed((enable ? 1 : 0), rs->regs + ROCKCHIP_SPI_SSIENR); @@ -297,6 +302,50 @@ static void rockchip_spi_set_cs(struct spi_device *spi, bool enable) pm_runtime_put_sync(rs->dev); } +static int rockchip_spi_setup(struct spi_device *spi) +{ + int ret = 0; + unsigned long flags = (spi->mode & SPI_CS_HIGH) ? + GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH; + struct rockchip_spi_data *data = spi_get_ctldata(spi); + + if (!gpio_is_valid(spi->cs_gpio)) + return 0; + + if (!data) { + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + spi_set_ctldata(spi, data); + } + + if (!data->cs_gpio_requested) { + ret = gpio_request_one(spi->cs_gpio, flags, + dev_name(&spi->dev)); + if (!ret) + data->cs_gpio_requested = 1; + } else + ret = gpio_direction_output(spi->cs_gpio, flags); + + if (ret < 0) + dev_err(&spi->dev, "Failed to setup cs gpio(%d): %d\n", + spi->cs_gpio, ret); + + return ret; +} + +static void rockchip_spi_cleanup(struct spi_device *spi) +{ + struct rockchip_spi_data *data = spi_get_ctldata(spi); + + if (data) { + if (data->cs_gpio_requested) + gpio_free(spi->cs_gpio); + kfree(data); + spi_set_ctldata(spi, NULL); + } +} + static int rockchip_spi_prepare_message(struct spi_master *master, struct spi_message *msg) { @@ -744,11 +793,14 @@ static int rockchip_spi_probe(struct platform_device *pdev) master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8); master->set_cs = rockchip_spi_set_cs; + master->setup = rockchip_spi_setup; + master->cleanup = rockchip_spi_cleanup; master->prepare_message = rockchip_spi_prepare_message; master->unprepare_message = rockchip_spi_unprepare_message; master->transfer_one = rockchip_spi_transfer_one; master->max_transfer_size = rockchip_spi_max_transfer_size; master->handle_err = rockchip_spi_handle_err; + master->flags = SPI_MASTER_GPIO_SS; rs->dma_tx.ch = dma_request_chan(rs->dev, "tx"); if (IS_ERR(rs->dma_tx.ch)) {
Support using "cs-gpios" property to specify cs gpios. Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- .../devicetree/bindings/spi/spi-rockchip.txt | 2 + drivers/spi/spi-rockchip.c | 52 ++++++++++++++++++++++ 2 files changed, 54 insertions(+)