Support using "cs-gpios" property to specify cs gpios.
Signed-off-by: Jeffy Chen <[email protected]>
---
.../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
@@ -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)) {
--
2.1.4
The cros_ec requires CS line to be active after last message. But the CS
would be toggled when powering off/on rockchip spi, which breaks ec xfer.
Use GPIO CS to prevent that.
Signed-off-by: Jeffy Chen <[email protected]>
---
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index eb50593..329a634 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -790,6 +790,8 @@ ap_i2c_audio: &i2c8 {
&spi5 {
status = "okay";
+ cs-gpios = <&gpio2 23 GPIO_ACTIVE_HIGH>;
+
cros_ec: ec@0 {
compatible = "google,cros-ec-spi";
reg = <0>;
@@ -813,6 +815,10 @@ ap_i2c_audio: &i2c8 {
};
};
+&spi5_cs0 {
+ rockchip,pins = <2 23 RK_FUNC_GPIO &pcfg_pull_up>;
+};
+
&tsadc {
status = "okay";
--
2.1.4
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 <[email protected]>
> ---
>
> .../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 <[email protected]> wrote:
> On 2017/6/12 14:14, Jeffy Chen wrote:
>>
>> Support using "cs-gpios" property to specify cs gpios.
>>
>> Signed-off-by: Jeffy Chen <[email protected]>
>> 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 -- [email protected]
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 <[email protected]>
>> ---
>>
>> .../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 <[email protected]>
>>> ---
>>>
>>> .../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 <[email protected]>
> >> ---
> >>
> >> .../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 <[email protected]> wrote:
>> On 2017/6/12 14:14, Jeffy Chen wrote:
>>>
>>> Support using "cs-gpios" property to specify cs gpios.
>>>
>>> Signed-off-by: Jeffy Chen <[email protected]>
>
>>> 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 -- [email protected]
>
> 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 <[email protected]>
>>>> ---
>>>>
>>>> .../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 <[email protected]>
> >>>> ---
> >>>>
> >>>> .../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 <[email protected]> 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.