[PATCHv2 0/2] cc2520: add support for CC2591

[PATCHv2 0/2] cc2520: add support for CC2591

[Brad Campbell]

Hi,

I've updated the patch. I changed the `amplified` param to be a bool            
which was straightforward once I learned that device tree entries can           
have empty properties.                                                          
                                                                                
I added a second commit that should take of the issue of platform_data not      
being read-only in cc2520_hw_init. Let me know if I can improve it, although    
it is mostly copied from the at86rf230.c version. I didn't think it was         
necessary to pass each item in the cc2520_platform_data struct individually     
and instead just pass the struct to get_platform_data.                          
                                                                                
I have tested the patch with both a CC2520 and a CC2520+CC2591 combination.     
The patch boils down to just sending different register settings for the        
CC2520 and seems to work because I can send and receive with both radios.       
                                                                                
Thank you for looking over the patch and let me know if there is anything       
else I should fix.

Brad

changes since v1:
- change how platform_data is handled
- make amplified a bool instead of int

Brad Campbell (2):
  cc2520: Do not store platform_data in spi_device
  cc2520: Add support for CC2591 amplifier.

 .../devicetree/bindings/net/ieee802154/cc2520.txt  |   4 +
 drivers/net/ieee802154/cc2520.c                    | 148 +++++++++++++--------
 include/linux/spi/cc2520.h                         |   1 +
 3 files changed, 96 insertions(+), 57 deletions(-)

-- 
2.3.2

[PATCHv2 1/2] cc2520: Do not store platform_data in spi_device

[Brad Campbell]

Storing the `platform_data` struct inside of the SPI struct when using
the device tree allows for a later function to edit the content of that
struct. This patch refactors the `cc2520_get_platformat_data` function
to accept a pointer to a `cc2520_platform_data` struct and populates
the fields inside of it.

This changes mirrors a similar change in the at86rf230.c driver.

Signed-off-by: Brad Campbell <bradjc5@gmail.com>
---
 drivers/net/ieee802154/cc2520.c | 93 +++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
index 181b349..275e6fb 100644
--- a/drivers/net/ieee802154/cc2520.c
+++ b/drivers/net/ieee802154/cc2520.c
@@ -714,6 +714,31 @@ static irqreturn_t cc2520_sfd_isr(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static int cc2520_get_platform_data(struct spi_device *spi,
+				    struct cc2520_platform_data *pdata)
+{
+	struct device_node *np = spi->dev.of_node;
+	struct cc2520_private *priv = spi_get_drvdata(spi);
+
+	if (!np) {
+		struct cc2520_platform_data *spi_pdata = spi->dev.platform_data;
+		*pdata = *spi_pdata;
+		return 0;
+	}
+
+	pdata->fifo = of_get_named_gpio(np, "fifo-gpio", 0);
+	priv->fifo_pin = pdata->fifo;
+
+	pdata->fifop = of_get_named_gpio(np, "fifop-gpio", 0);
+
+	pdata->sfd = of_get_named_gpio(np, "sfd-gpio", 0);
+	pdata->cca = of_get_named_gpio(np, "cca-gpio", 0);
+	pdata->vreg = of_get_named_gpio(np, "vreg-gpio", 0);
+	pdata->reset = of_get_named_gpio(np, "reset-gpio", 0);
+
+	return 0;
+}
+
 static int cc2520_hw_init(struct cc2520_private *priv)
 {
 	u8 status = 0, state = 0xff;
@@ -808,40 +833,10 @@ err_ret:
 	return ret;
 }
 
-static struct cc2520_platform_data *
-cc2520_get_platform_data(struct spi_device *spi)
-{
-	struct cc2520_platform_data *pdata;
-	struct device_node *np = spi->dev.of_node;
-	struct cc2520_private *priv = spi_get_drvdata(spi);
-
-	if (!np)
-		return spi->dev.platform_data;
-
-	pdata = devm_kzalloc(&spi->dev, sizeof(*pdata), GFP_KERNEL);
-	if (!pdata)
-		goto done;
-
-	pdata->fifo = of_get_named_gpio(np, "fifo-gpio", 0);
-	priv->fifo_pin = pdata->fifo;
-
-	pdata->fifop = of_get_named_gpio(np, "fifop-gpio", 0);
-
-	pdata->sfd = of_get_named_gpio(np, "sfd-gpio", 0);
-	pdata->cca = of_get_named_gpio(np, "cca-gpio", 0);
-	pdata->vreg = of_get_named_gpio(np, "vreg-gpio", 0);
-	pdata->reset = of_get_named_gpio(np, "reset-gpio", 0);
-
-	spi->dev.platform_data = pdata;
-
-done:
-	return pdata;
-}
-
 static int cc2520_probe(struct spi_device *spi)
 {
 	struct cc2520_private *priv;
-	struct cc2520_platform_data *pdata;
+	struct cc2520_platform_data pdata;
 	int ret;
 
 	priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
@@ -850,8 +845,8 @@ static int cc2520_probe(struct spi_device *spi)
 
 	spi_set_drvdata(spi, priv);
 
-	pdata = cc2520_get_platform_data(spi);
-	if (!pdata) {
+	ret = cc2520_get_platform_data(spi, &pdata);
+	if (ret < 0) {
 		dev_err(&spi->dev, "no platform data\n");
 		return -EINVAL;
 	}
@@ -869,76 +864,76 @@ static int cc2520_probe(struct spi_device *spi)
 	init_completion(&priv->tx_complete);
 
 	/* Request all the gpio's */
-	if (!gpio_is_valid(pdata->fifo)) {
+	if (!gpio_is_valid(pdata.fifo)) {
 		dev_err(&spi->dev, "fifo gpio is not valid\n");
 		ret = -EINVAL;
 		goto err_hw_init;
 	}
 
-	ret = devm_gpio_request_one(&spi->dev, pdata->fifo,
+	ret = devm_gpio_request_one(&spi->dev, pdata.fifo,
 				    GPIOF_IN, "fifo");
 	if (ret)
 		goto err_hw_init;
 
-	if (!gpio_is_valid(pdata->cca)) {
+	if (!gpio_is_valid(pdata.cca)) {
 		dev_err(&spi->dev, "cca gpio is not valid\n");
 		ret = -EINVAL;
 		goto err_hw_init;
 	}
 
-	ret = devm_gpio_request_one(&spi->dev, pdata->cca,
+	ret = devm_gpio_request_one(&spi->dev, pdata.cca,
 				    GPIOF_IN, "cca");
 	if (ret)
 		goto err_hw_init;
 
-	if (!gpio_is_valid(pdata->fifop)) {
+	if (!gpio_is_valid(pdata.fifop)) {
 		dev_err(&spi->dev, "fifop gpio is not valid\n");
 		ret = -EINVAL;
 		goto err_hw_init;
 	}
 
-	ret = devm_gpio_request_one(&spi->dev, pdata->fifop,
+	ret = devm_gpio_request_one(&spi->dev, pdata.fifop,
 				    GPIOF_IN, "fifop");
 	if (ret)
 		goto err_hw_init;
 
-	if (!gpio_is_valid(pdata->sfd)) {
+	if (!gpio_is_valid(pdata.sfd)) {
 		dev_err(&spi->dev, "sfd gpio is not valid\n");
 		ret = -EINVAL;
 		goto err_hw_init;
 	}
 
-	ret = devm_gpio_request_one(&spi->dev, pdata->sfd,
+	ret = devm_gpio_request_one(&spi->dev, pdata.sfd,
 				    GPIOF_IN, "sfd");
 	if (ret)
 		goto err_hw_init;
 
-	if (!gpio_is_valid(pdata->reset)) {
+	if (!gpio_is_valid(pdata.reset)) {
 		dev_err(&spi->dev, "reset gpio is not valid\n");
 		ret = -EINVAL;
 		goto err_hw_init;
 	}
 
-	ret = devm_gpio_request_one(&spi->dev, pdata->reset,
+	ret = devm_gpio_request_one(&spi->dev, pdata.reset,
 				    GPIOF_OUT_INIT_LOW, "reset");
 	if (ret)
 		goto err_hw_init;
 
-	if (!gpio_is_valid(pdata->vreg)) {
+	if (!gpio_is_valid(pdata.vreg)) {
 		dev_err(&spi->dev, "vreg gpio is not valid\n");
 		ret = -EINVAL;
 		goto err_hw_init;
 	}
 
-	ret = devm_gpio_request_one(&spi->dev, pdata->vreg,
+	ret = devm_gpio_request_one(&spi->dev, pdata.vreg,
 				    GPIOF_OUT_INIT_LOW, "vreg");
 	if (ret)
 		goto err_hw_init;
 
-	gpio_set_value(pdata->vreg, HIGH);
+	gpio_set_value(pdata.vreg, HIGH);
 	usleep_range(100, 150);
 
-	gpio_set_value(pdata->reset, HIGH);
+	gpio_set_value(pdata.reset, HIGH);
 	usleep_range(200, 250);
 
 	ret = cc2520_hw_init(priv);
@@ -947,7 +942,7 @@ static int cc2520_probe(struct spi_device *spi)
 
 	/* Set up fifop interrupt */
 	ret = devm_request_irq(&spi->dev,
-			       gpio_to_irq(pdata->fifop),
+			       gpio_to_irq(pdata.fifop),
 			       cc2520_fifop_isr,
 			       IRQF_TRIGGER_RISING,
 			       dev_name(&spi->dev),
@@ -959,7 +954,7 @@ static int cc2520_probe(struct spi_device *spi)
 
 	/* Set up sfd interrupt */
 	ret = devm_request_irq(&spi->dev,
-			       gpio_to_irq(pdata->sfd),
+			       gpio_to_irq(pdata.sfd),
 			       cc2520_sfd_isr,
 			       IRQF_TRIGGER_FALLING,
 			       dev_name(&spi->dev),
-- 
2.3.2

[PATCHv2 2/2] cc2520: Add support for CC2591 amplifier.

[Brad Campbell]

The TI CC2521 is an RF power amplifier that is designed to interface
with the CC2520. Conveniently, it directly interfaces with the CC2520
and does not require any pins to be connected to a
microcontroller/processor. Adding a CC2591 increases the CC2520's range,
which is useful for border router and other wall-powered applications.

Using the CC2591 with the CC2520 requires configuring the CC2520 GPIOs
that are connected to the CC2591 to correctly set the CC2591 into TX and
RX modes. Further, TI recommends that the CC2520_TXPOWER and
CC2520_AGCCTRL1 registers are set differently to maximize the CC2591's
performance. These settings are covered in TI Application Note AN065.

This patch adds an optional `amplified` field to the cc2520 entry in the
device tree. If present, the CC2520 will be configured to operate with a
CC2591.

The expected pin mapping is:
CC2520 GPIO0 --> CC2591 EN
CC2520 GPIO5 --> CC2591 PAEN

Signed-off-by: Brad Campbell <bradjc5@gmail.com>
---
 .../devicetree/bindings/net/ieee802154/cc2520.txt  |  4 ++
 drivers/net/ieee802154/cc2520.c                    | 55 ++++++++++++++++++----
 include/linux/spi/cc2520.h                         |  1 +
 3 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ieee802154/cc2520.txt b/Documentation/devicetree/bindings/net/ieee802154/cc2520.txt
index 0071883..fb6d49f 100644
--- a/Documentation/devicetree/bindings/net/ieee802154/cc2520.txt
+++ b/Documentation/devicetree/bindings/net/ieee802154/cc2520.txt
@@ -13,11 +13,15 @@ Required properties:
 	- cca-gpio:		GPIO spec for the CCA pin
 	- vreg-gpio:		GPIO spec for the VREG pin
 	- reset-gpio:		GPIO spec for the RESET pin
+Optional properties:
+	- amplified:		include if the CC2520 is connected to a CC2591 amplifier
+
 Example:
 	cc2520@0 {
 		compatible = "ti,cc2520";
 		reg = <0>;
 		spi-max-frequency = <4000000>;
+		amplified;
 		pinctrl-names = "default";
 		pinctrl-0 = <&cc2520_cape_pins>;
 		fifo-gpio = <&gpio1 18 0>;
diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
index 275e6fb..0639af6 100644
--- a/drivers/net/ieee802154/cc2520.c
+++ b/drivers/net/ieee802154/cc2520.c
@@ -736,6 +736,8 @@ static int cc2520_get_platform_data(struct spi_device *spi,
 	pdata->vreg = of_get_named_gpio(np, "vreg-gpio", 0);
 	pdata->reset = of_get_named_gpio(np, "reset-gpio", 0);
 
+	pdata->amplified = of_property_read_bool(np, "amplified");
+
 	return 0;
 }
 
@@ -744,6 +746,11 @@ static int cc2520_hw_init(struct cc2520_private *priv)
 	u8 status = 0, state = 0xff;
 	int ret;
 	int timeout = 100;
+	struct cc2520_platform_data pdata;
+
+	ret = cc2520_get_platform_data(priv->spi, &pdata);
+	if (ret)
+		goto err_ret;
 
 	ret = cc2520_read_register(priv, CC2520_FSMSTAT1, &state);
 	if (ret)
@@ -766,11 +773,47 @@ static int cc2520_hw_init(struct cc2520_private *priv)
 
 	dev_vdbg(&priv->spi->dev, "oscillator brought up\n");
 
-	/* Registers default value: section 28.1 in Datasheet */
-	ret = cc2520_write_register(priv, CC2520_TXPOWER, 0xF7);
-	if (ret)
-		goto err_ret;
+	/* If the CC2520 is connected to a CC2591 amplifier, we must both
+	 * configure GPIOs on the CC2520 to correctly configure the CC2591
+	 * and change a couple settings of the CC2520 to work with the
+	 * amplifier. See section 8 page 17 of TI application note AN065.
+	 * http://www.ti.com/lit/an/swra229a/swra229a.pdf
+	 */
+	if (pdata.amplified == 1) {
+		ret = cc2520_write_register(priv, CC2520_TXPOWER, 0xF9);
+		if (ret)
+			goto err_ret;
 
+		ret = cc2520_write_register(priv, CC2520_AGCCTRL1, 0x16);
+		if (ret)
+			goto err_ret;
+
+		ret = cc2520_write_register(priv, CC2520_GPIOCTRL0, 0x46);
+		if (ret)
+			goto err_ret;
+
+		ret = cc2520_write_register(priv, CC2520_GPIOCTRL5, 0x47);
+		if (ret)
+			goto err_ret;
+
+		ret = cc2520_write_register(priv, CC2520_GPIOPOLARITY, 0x1e);
+		if (ret)
+			goto err_ret;
+
+		ret = cc2520_write_register(priv, CC2520_TXCTRL, 0xc1);
+		if (ret)
+			goto err_ret;
+	} else {
+		ret = cc2520_write_register(priv, CC2520_TXPOWER, 0xF7);
+		if (ret)
+			goto err_ret;
+
+		ret = cc2520_write_register(priv, CC2520_AGCCTRL1, 0x11);
+		if (ret)
+			goto err_ret;
+	}
+
+	/* Registers default value: section 28.1 in Datasheet */
 	ret = cc2520_write_register(priv, CC2520_CCACTRL0, 0x1A);
 	if (ret)
 		goto err_ret;
@@ -795,10 +838,6 @@ static int cc2520_hw_init(struct cc2520_private *priv)
 	if (ret)
 		goto err_ret;
 
-	ret = cc2520_write_register(priv, CC2520_AGCCTRL1, 0x11);
-	if (ret)
-		goto err_ret;
-
 	ret = cc2520_write_register(priv, CC2520_ADCTEST0, 0x10);
 	if (ret)
 		goto err_ret;
diff --git a/include/linux/spi/cc2520.h b/include/linux/spi/cc2520.h
index 85b8ee6..e741e8b 100644
--- a/include/linux/spi/cc2520.h
+++ b/include/linux/spi/cc2520.h
@@ -21,6 +21,7 @@ struct cc2520_platform_data {
 	int sfd;
 	int reset;
 	int vreg;
+	bool amplified;
 };
 
 #endif
-- 
2.3.2

Re: [PATCHv2 2/2] cc2520: Add support for CC2591 amplifier.

[Alexander Aring]

Hi,

On Mon, Mar 16, 2015 at 06:16:43PM -0400, Brad Campbell wrote:
> The TI CC2521 is an RF power amplifier that is designed to interface
> with the CC2520. Conveniently, it directly interfaces with the CC2520
> and does not require any pins to be connected to a
> microcontroller/processor. Adding a CC2591 increases the CC2520's range,
> which is useful for border router and other wall-powered applications.
> 
> Using the CC2591 with the CC2520 requires configuring the CC2520 GPIOs
> that are connected to the CC2591 to correctly set the CC2591 into TX and
> RX modes. Further, TI recommends that the CC2520_TXPOWER and
> CC2520_AGCCTRL1 registers are set differently to maximize the CC2591's
> performance. These settings are covered in TI Application Note AN065.
> 
> This patch adds an optional `amplified` field to the cc2520 entry in the
> device tree. If present, the CC2520 will be configured to operate with a
> CC2591.
> 
> The expected pin mapping is:
> CC2520 GPIO0 --> CC2591 EN
> CC2520 GPIO5 --> CC2591 PAEN
> 
> Signed-off-by: Brad Campbell <bradjc5@gmail.com>
> ---
>  .../devicetree/bindings/net/ieee802154/cc2520.txt  |  4 ++
>  drivers/net/ieee802154/cc2520.c                    | 55 ++++++++++++++++++----
>  include/linux/spi/cc2520.h                         |  1 +
>  3 files changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/ieee802154/cc2520.txt b/Documentation/devicetree/bindings/net/ieee802154/cc2520.txt
> index 0071883..fb6d49f 100644
> --- a/Documentation/devicetree/bindings/net/ieee802154/cc2520.txt
> +++ b/Documentation/devicetree/bindings/net/ieee802154/cc2520.txt
> @@ -13,11 +13,15 @@ Required properties:
>  	- cca-gpio:		GPIO spec for the CCA pin
>  	- vreg-gpio:		GPIO spec for the VREG pin
>  	- reset-gpio:		GPIO spec for the RESET pin
> +Optional properties:
> +	- amplified:		include if the CC2520 is connected to a CC2591 amplifier
> +
>  Example:
>  	cc2520@0 {
>  		compatible = "ti,cc2520";
>  		reg = <0>;
>  		spi-max-frequency = <4000000>;
> +		amplified;
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&cc2520_cape_pins>;
>  		fifo-gpio = <&gpio1 18 0>;
> diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
> index 275e6fb..0639af6 100644
> --- a/drivers/net/ieee802154/cc2520.c
> +++ b/drivers/net/ieee802154/cc2520.c
> @@ -736,6 +736,8 @@ static int cc2520_get_platform_data(struct spi_device *spi,
>  	pdata->vreg = of_get_named_gpio(np, "vreg-gpio", 0);
>  	pdata->reset = of_get_named_gpio(np, "reset-gpio", 0);
>  
> +	pdata->amplified = of_property_read_bool(np, "amplified");
> +
>  	return 0;
>  }
>  
> @@ -744,6 +746,11 @@ static int cc2520_hw_init(struct cc2520_private *priv)
>  	u8 status = 0, state = 0xff;
>  	int ret;
>  	int timeout = 100;
> +	struct cc2520_platform_data pdata;
> +
> +	ret = cc2520_get_platform_data(priv->spi, &pdata);
> +	if (ret)
> +		goto err_ret;
>  
>  	ret = cc2520_read_register(priv, CC2520_FSMSTAT1, &state);
>  	if (ret)
> @@ -766,11 +773,47 @@ static int cc2520_hw_init(struct cc2520_private *priv)
>  
>  	dev_vdbg(&priv->spi->dev, "oscillator brought up\n");
>  
> -	/* Registers default value: section 28.1 in Datasheet */
> -	ret = cc2520_write_register(priv, CC2520_TXPOWER, 0xF7);
> -	if (ret)
> -		goto err_ret;
> +	/* If the CC2520 is connected to a CC2591 amplifier, we must both
> +	 * configure GPIOs on the CC2520 to correctly configure the CC2591
> +	 * and change a couple settings of the CC2520 to work with the
> +	 * amplifier. See section 8 page 17 of TI application note AN065.
> +	 * http://www.ti.com/lit/an/swra229a/swra229a.pdf
> +	 */
> +	if (pdata.amplified == 1) {

just if (pdata.amplified) here.

- Alex

Re: [PATCHv2 1/2] cc2520: Do not store platform_data in spi_device

[Alexander Aring]

Hi,

On Mon, Mar 16, 2015 at 06:16:42PM -0400, Brad Campbell wrote:
> Storing the `platform_data` struct inside of the SPI struct when using
> the device tree allows for a later function to edit the content of that
> struct. This patch refactors the `cc2520_get_platformat_data` function
> to accept a pointer to a `cc2520_platform_data` struct and populates
> the fields inside of it.
> 
> This changes mirrors a similar change in the at86rf230.c driver.

For referencing other fixes use $COMMIT_ID ("$SHORT_COMMIT_MSG").

like: aaa1c4d226e4cd730075d3dac99a6d599a0190c7 ("at86rf230: copy pdata
to driver allocated space")

> 
> Signed-off-by: Brad Campbell <bradjc5@gmail.com>
> ---
>  drivers/net/ieee802154/cc2520.c | 93 +++++++++++++++++++----------------------
>  1 file changed, 44 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
> index 181b349..275e6fb 100644
> --- a/drivers/net/ieee802154/cc2520.c
> +++ b/drivers/net/ieee802154/cc2520.c
> @@ -714,6 +714,31 @@ static irqreturn_t cc2520_sfd_isr(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static int cc2520_get_platform_data(struct spi_device *spi,
> +				    struct cc2520_platform_data *pdata)
> +{
> +	struct device_node *np = spi->dev.of_node;
> +	struct cc2520_private *priv = spi_get_drvdata(spi);
> +
> +	if (!np) {
> +		struct cc2520_platform_data *spi_pdata = spi->dev.platform_data;

This can be null if somebody given no platform_data data.
Additional check with on (!spi_pdata) is required.

something like:

if (!spi_pdata)
	return -ENOENT;

before dereferencing.

> +		*pdata = *spi_pdata;
> +		return 0;
> +	}
> 

Otherwise looking good.

- Alex