[PATCHv3 bluetooth-next 0/2] at86rf230: add support for xtal trim register

[PATCHv3 bluetooth-next 0/2] at86rf230: add support for xtal trim register

[Alexander Aring]

Hi,

I want to clarify how xtal_trim value is calculated now.

First of all the xtal_mode set to 0x5 will require a 16 Mhz clock signal at
pin 26. I don't have such at86rf2xx board which can do that, so I don't want
to support it right now.

The xtal_trim is only necesarry for external crystal, when xtal_mode == 0xF.
The xtal_trim value is calculated by:

CL = capacitor of used crystal
CX = connected capacitors at xtal pins
CPAR = in all at86rf2xx datasheets this is a constant value 3 pF,
       but this is different on each board setup. You need to fine
       tuning this value via CTRIM.
CTRIM = variable capacitor setting. Resolution is 0.3 pF range is
        0 pF upto 4.5 pF.

CL = 0.5 * (CX + CTRIM + CPAR)


Examples:

On atben [0]:

CL = 8 pF
CX = 12 pF
CPAR = 3 pF (We assume the magic constant from datasheet)
CTRIM = 0.9 pF

(12+0.9+3)/2 = 7.95 which is nearly at 8 pF

openlabs [1]:

CL = 16 pF
CX = 22 pF
CPAR = 3 pF (We assume the magic constant from datasheet)
CTRIM = 4.5 pF

(22+4.5+3)/2 = 14.75 which is the nearest value to 16 pF

For more information it's the section "Integrated Oscillator Setup" inside
the at86rf2xx datasheets. (In my case 8111C–MCU Wireless–09/09).

For a better calculation of the CPAR value, Werner Almesberger developed some
diagnostic tool [2], which I don't tried out yet.

- Alex

[0] http://projects.qi-hardware.com/index.php/p/ben-wpan/source/tree/master/atben
    (atben.sch, requires kicad tool)
[1] http://openlabs.co/OSHW/Raspberry-Pi-802.15.4-radio-files/rpi802154-r1.pdf
[2] http://projects.qi-hardware.com/index.php/p/ben-wpan/source/tree/master/tools/atrf-xtal

changes since v3:
 - remove setting of xtal_mode. Instead we setting xtal_trim only.

changes since v2:
 - copy platform data to driver allocated space

Cc: Werner Almesberger <werner@almesberger.net>
Cc: Thomas Stilwell <stilwellt@openlabs.co>

Alexander Aring (2):
  at86rf230: copy pdata to driver allocated space
  at86rf230: add support for external xtal trim

 .../bindings/net/ieee802154/at86rf230.txt          |  3 ++
 drivers/net/ieee802154/at86rf230.c                 | 59 +++++++++++++---------
 include/linux/spi/at86rf230.h                      |  1 +
 3 files changed, 38 insertions(+), 25 deletions(-)

-- 
2.3.0

[PATCHv3 bluetooth-next 1/2] at86rf230: copy pdata to driver allocated space

[Alexander Aring]

This patch copies the platform data in driver allocated space at first.
With this change we ensure that we access the allocated platform data as
readonly space.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/ieee802154/at86rf230.c | 49 ++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index cbfc8c5..c495f23 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -1377,24 +1377,21 @@ static int at86rf230_hw_init(struct at86rf230_local *lp)
 	return at86rf230_write_subreg(lp, SR_SLOTTED_OPERATION, 0);
 }
 
-static struct at86rf230_platform_data *
-at86rf230_get_pdata(struct spi_device *spi)
+static int at86rf230_get_pdata(struct spi_device *spi,
+			       struct at86rf230_platform_data *cfg)
 {
-	struct at86rf230_platform_data *pdata;
+	if (!IS_ENABLED(CONFIG_OF) || !spi->dev.of_node) {
+		if (!spi->dev.platform_data)
+			return -ENOENT;
 
-	if (!IS_ENABLED(CONFIG_OF) || !spi->dev.of_node)
-		return spi->dev.platform_data;
-
-	pdata = devm_kzalloc(&spi->dev, sizeof(*pdata), GFP_KERNEL);
-	if (!pdata)
-		goto done;
+		memcpy(cfg, spi->dev.platform_data, sizeof(*cfg));
+		return 0;
+	}
 
-	pdata->rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
-	pdata->slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
+	cfg->rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
+	cfg->slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
 
-	spi->dev.platform_data = pdata;
-done:
-	return pdata;
+	return 0;
 }
 
 static int
@@ -1501,7 +1498,7 @@ at86rf230_setup_spi_messages(struct at86rf230_local *lp)
 
 static int at86rf230_probe(struct spi_device *spi)
 {
-	struct at86rf230_platform_data *pdata;
+	struct at86rf230_platform_data cfg;
 	struct ieee802154_hw *hw;
 	struct at86rf230_local *lp;
 	unsigned int status;
@@ -1512,32 +1509,32 @@ static int at86rf230_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	pdata = at86rf230_get_pdata(spi);
-	if (!pdata) {
-		dev_err(&spi->dev, "no platform_data\n");
-		return -EINVAL;
+	rc = at86rf230_get_pdata(spi, &cfg);
+	if (rc < 0) {
+		dev_err(&spi->dev, "failed to parse platform_data: %d\n", rc);
+		return rc;
 	}
 
-	if (gpio_is_valid(pdata->rstn)) {
-		rc = devm_gpio_request_one(&spi->dev, pdata->rstn,
+	if (gpio_is_valid(cfg.rstn)) {
+		rc = devm_gpio_request_one(&spi->dev, cfg.rstn,
 					   GPIOF_OUT_INIT_HIGH, "rstn");
 		if (rc)
 			return rc;
 	}
 
-	if (gpio_is_valid(pdata->slp_tr)) {
-		rc = devm_gpio_request_one(&spi->dev, pdata->slp_tr,
+	if (gpio_is_valid(cfg.slp_tr)) {
+		rc = devm_gpio_request_one(&spi->dev, cfg.slp_tr,
 					   GPIOF_OUT_INIT_LOW, "slp_tr");
 		if (rc)
 			return rc;
 	}
 
 	/* Reset */
-	if (gpio_is_valid(pdata->rstn)) {
+	if (gpio_is_valid(cfg.rstn)) {
 		udelay(1);
-		gpio_set_value(pdata->rstn, 0);
+		gpio_set_value(cfg.rstn, 0);
 		udelay(1);
-		gpio_set_value(pdata->rstn, 1);
+		gpio_set_value(cfg.rstn, 1);
 		usleep_range(120, 240);
 	}
 
-- 
2.3.0

[PATCHv3 bluetooth-next 2/2] at86rf230: add support for external xtal trim

[Alexander Aring]

This patch adds support for setting the xtal trim register. Some at86rf2xx
transceiver boards needs fine tuning the xtal capacitor.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 .../devicetree/bindings/net/ieee802154/at86rf230.txt         |  3 +++
 drivers/net/ieee802154/at86rf230.c                           | 12 ++++++++++++
 include/linux/spi/at86rf230.h                                |  1 +
 3 files changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ieee802154/at86rf230.txt b/Documentation/devicetree/bindings/net/ieee802154/at86rf230.txt
index d3bbdded..1ae5100 100644
--- a/Documentation/devicetree/bindings/net/ieee802154/at86rf230.txt
+++ b/Documentation/devicetree/bindings/net/ieee802154/at86rf230.txt
@@ -11,6 +11,8 @@ Required properties:
 Optional properties:
   - reset-gpio:		GPIO spec for the rstn pin
   - sleep-gpio:		GPIO spec for the slp_tr pin
+  - xtal-trim:		u8 value for fine tuning the internal capacitance
+			arrays of xtal pins: 0 = +0 pF, 0xf = +4.5 pF
 
 Example:
 
@@ -20,4 +22,5 @@ Example:
 		reg = <0>;
 		interrupts = <19 1>;
 		interrupt-parent = <&gpio3>;
+		xtal-trim = /bits/ 8 <0x06>;
 	};
diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index c495f23..e74b3cc 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -72,6 +72,7 @@ struct at86rf230_state_change {
 
 struct at86rf230_local {
 	struct spi_device *spi;
+	struct at86rf230_platform_data cfg;
 
 	struct ieee802154_hw *hw;
 	struct at86rf2xx_chip_data *data;
@@ -1362,6 +1363,10 @@ static int at86rf230_hw_init(struct at86rf230_local *lp)
 	usleep_range(lp->data->t_sleep_cycle,
 		     lp->data->t_sleep_cycle + 100);
 
+	rc = at86rf230_write_subreg(lp, SR_XTAL_TRIM, lp->cfg.xtal_trim);
+	if (rc)
+		return rc;
+
 	rc = at86rf230_read_subreg(lp, SR_DVDD_OK, &dvdd);
 	if (rc)
 		return rc;
@@ -1380,6 +1385,8 @@ static int at86rf230_hw_init(struct at86rf230_local *lp)
 static int at86rf230_get_pdata(struct spi_device *spi,
 			       struct at86rf230_platform_data *cfg)
 {
+	int ret;
+
 	if (!IS_ENABLED(CONFIG_OF) || !spi->dev.of_node) {
 		if (!spi->dev.platform_data)
 			return -ENOENT;
@@ -1390,6 +1397,10 @@ static int at86rf230_get_pdata(struct spi_device *spi,
 
 	cfg->rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
 	cfg->slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
+	ret = of_property_read_u8(spi->dev.of_node, "xtal-trim",
+				  &cfg->xtal_trim);
+	if (ret < 0 && ret != -EINVAL)
+		return ret;
 
 	return 0;
 }
@@ -1545,6 +1556,7 @@ static int at86rf230_probe(struct spi_device *spi)
 	lp = hw->priv;
 	lp->hw = hw;
 	lp->spi = spi;
+	lp->cfg = cfg;
 	hw->parent = &spi->dev;
 	hw->vif_data_size = sizeof(*lp);
 	ieee802154_random_extended_addr(&hw->phy->perm_extended_addr);
diff --git a/include/linux/spi/at86rf230.h b/include/linux/spi/at86rf230.h
index cd519a1..b63fe6f 100644
--- a/include/linux/spi/at86rf230.h
+++ b/include/linux/spi/at86rf230.h
@@ -22,6 +22,7 @@ struct at86rf230_platform_data {
 	int rstn;
 	int slp_tr;
 	int dig2;
+	u8 xtal_trim;
 };
 
 #endif
-- 
2.3.0

Re: [PATCHv3 bluetooth-next 2/2] at86rf230: add support for external xtal trim

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3317 bytes --]

On 02/24/2015 11:11 AM, Alexander Aring wrote:
> This patch adds support for setting the xtal trim register. Some at86rf2xx
> transceiver boards needs fine tuning the xtal capacitor.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>  .../devicetree/bindings/net/ieee802154/at86rf230.txt         |  3 +++
>  drivers/net/ieee802154/at86rf230.c                           | 12 ++++++++++++
>  include/linux/spi/at86rf230.h                                |  1 +
>  3 files changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ieee802154/at86rf230.txt b/Documentation/devicetree/bindings/net/ieee802154/at86rf230.txt
> index d3bbdded..1ae5100 100644
> --- a/Documentation/devicetree/bindings/net/ieee802154/at86rf230.txt
> +++ b/Documentation/devicetree/bindings/net/ieee802154/at86rf230.txt
> @@ -11,6 +11,8 @@ Required properties:
>  Optional properties:
>    - reset-gpio:		GPIO spec for the rstn pin
>    - sleep-gpio:		GPIO spec for the slp_tr pin
> +  - xtal-trim:		u8 value for fine tuning the internal capacitance
> +			arrays of xtal pins: 0 = +0 pF, 0xf = +4.5 pF
>  
>  Example:
>  
> @@ -20,4 +22,5 @@ Example:
>  		reg = <0>;
>  		interrupts = <19 1>;
>  		interrupt-parent = <&gpio3>;
> +		xtal-trim = /bits/ 8 <0x06>;
>  	};
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index c495f23..e74b3cc 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -72,6 +72,7 @@ struct at86rf230_state_change {
>  
>  struct at86rf230_local {
>  	struct spi_device *spi;
> +	struct at86rf230_platform_data cfg;
>  
>  	struct ieee802154_hw *hw;
>  	struct at86rf2xx_chip_data *data;
> @@ -1362,6 +1363,10 @@ static int at86rf230_hw_init(struct at86rf230_local *lp)
>  	usleep_range(lp->data->t_sleep_cycle,
>  		     lp->data->t_sleep_cycle + 100);
>  
> +	rc = at86rf230_write_subreg(lp, SR_XTAL_TRIM, lp->cfg.xtal_trim);
> +	if (rc)
> +		return rc;
> +
>  	rc = at86rf230_read_subreg(lp, SR_DVDD_OK, &dvdd);
>  	if (rc)
>  		return rc;
> @@ -1380,6 +1385,8 @@ static int at86rf230_hw_init(struct at86rf230_local *lp)
>  static int at86rf230_get_pdata(struct spi_device *spi,
>  			       struct at86rf230_platform_data *cfg)
>  {
> +	int ret;
> +
>  	if (!IS_ENABLED(CONFIG_OF) || !spi->dev.of_node) {
>  		if (!spi->dev.platform_data)
>  			return -ENOENT;
> @@ -1390,6 +1397,10 @@ static int at86rf230_get_pdata(struct spi_device *spi,
>  
>  	cfg->rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
>  	cfg->slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
> +	ret = of_property_read_u8(spi->dev.of_node, "xtal-trim",
> +				  &cfg->xtal_trim);
> +	if (ret < 0 && ret != -EINVAL)
> +		return ret;
>  
>  	return 0;
>  }
> @@ -1545,6 +1556,7 @@ static int at86rf230_probe(struct spi_device *spi)
>  	lp = hw->priv;
>  	lp->hw = hw;
>  	lp->spi = spi;
> +	lp->cfg = cfg;

This doesn't look correct.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv3 bluetooth-next 2/2] at86rf230: add support for external xtal trim

[Alexander Aring]

Hi Marc,

On Tue, Feb 24, 2015 at 11:21:43AM +0100, Marc Kleine-Budde wrote:
> On 02/24/2015 11:11 AM, Alexander Aring wrote:
> > This patch adds support for setting the xtal trim register. Some at86rf2xx
> > transceiver boards needs fine tuning the xtal capacitor.
> > 
> > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > ---
> >  .../devicetree/bindings/net/ieee802154/at86rf230.txt         |  3 +++
> >  drivers/net/ieee802154/at86rf230.c                           | 12 ++++++++++++
> >  include/linux/spi/at86rf230.h                                |  1 +
> >  3 files changed, 16 insertions(+)
> > 
...
> > @@ -1390,6 +1397,10 @@ static int at86rf230_get_pdata(struct spi_device *spi,
> >  
> >  	cfg->rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
> >  	cfg->slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
> > +	ret = of_property_read_u8(spi->dev.of_node, "xtal-trim",
> > +				  &cfg->xtal_trim);
> > +	if (ret < 0 && ret != -EINVAL)
> > +		return ret;
> >  
> >  	return 0;
> >  }
> > @@ -1545,6 +1556,7 @@ static int at86rf230_probe(struct spi_device *spi)
> >  	lp = hw->priv;
> >  	lp->hw = hw;
> >  	lp->spi = spi;
> > +	lp->cfg = cfg;
> 
> This doesn't look correct.

You mean the line:

"lp->cfg = cfg;" or everything?

I copy there the "temporary on stack" allocated platform data to
at86rf230_local cfg, which is the platform data allocated on the heap in
the at86rf230_local struct.

The "temporary on stack" platform data is to not use the "real
platform_data" for requesting several pins. Afterwards I copy the
platform_data from stack to the at86rf230_local. I can't do this
directly because we allocate space for at86rf230_local after requesting
pins and do reset, etc..

- Alex

Re: [PATCHv3 bluetooth-next 2/2] at86rf230: add support for external xtal trim

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]

On 02/24/2015 11:28 AM, Alexander Aring wrote:
> Hi Marc,
> 
> On Tue, Feb 24, 2015 at 11:21:43AM +0100, Marc Kleine-Budde wrote:
>> On 02/24/2015 11:11 AM, Alexander Aring wrote:
>>> This patch adds support for setting the xtal trim register. Some at86rf2xx
>>> transceiver boards needs fine tuning the xtal capacitor.
>>>
>>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
>>> ---
>>>  .../devicetree/bindings/net/ieee802154/at86rf230.txt         |  3 +++
>>>  drivers/net/ieee802154/at86rf230.c                           | 12 ++++++++++++
>>>  include/linux/spi/at86rf230.h                                |  1 +
>>>  3 files changed, 16 insertions(+)
>>>
> ...
>>> @@ -1390,6 +1397,10 @@ static int at86rf230_get_pdata(struct spi_device *spi,
>>>  
>>>  	cfg->rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
>>>  	cfg->slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
>>> +	ret = of_property_read_u8(spi->dev.of_node, "xtal-trim",
>>> +				  &cfg->xtal_trim);
>>> +	if (ret < 0 && ret != -EINVAL)
>>> +		return ret;
>>>  
>>>  	return 0;
>>>  }
>>> @@ -1545,6 +1556,7 @@ static int at86rf230_probe(struct spi_device *spi)
>>>  	lp = hw->priv;
>>>  	lp->hw = hw;
>>>  	lp->spi = spi;
>>> +	lp->cfg = cfg;
>>
>> This doesn't look correct.
> 
> You mean the line:
> 
> "lp->cfg = cfg;" or everything?

Just that line, it's in the wrong patch.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv3 bluetooth-next 2/2] at86rf230: add support for external xtal trim

[Alexander Aring]

On Tue, Feb 24, 2015 at 11:34:17AM +0100, Marc Kleine-Budde wrote:
> On 02/24/2015 11:28 AM, Alexander Aring wrote:
> > Hi Marc,
> > 
> > On Tue, Feb 24, 2015 at 11:21:43AM +0100, Marc Kleine-Budde wrote:
> >> On 02/24/2015 11:11 AM, Alexander Aring wrote:
> >>> This patch adds support for setting the xtal trim register. Some at86rf2xx
> >>> transceiver boards needs fine tuning the xtal capacitor.
> >>>
> >>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> >>> ---
> >>>  .../devicetree/bindings/net/ieee802154/at86rf230.txt         |  3 +++
> >>>  drivers/net/ieee802154/at86rf230.c                           | 12 ++++++++++++
> >>>  include/linux/spi/at86rf230.h                                |  1 +
> >>>  3 files changed, 16 insertions(+)
> >>>
> > ...
> >>> @@ -1390,6 +1397,10 @@ static int at86rf230_get_pdata(struct spi_device *spi,
> >>>  
> >>>  	cfg->rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
> >>>  	cfg->slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
> >>> +	ret = of_property_read_u8(spi->dev.of_node, "xtal-trim",
> >>> +				  &cfg->xtal_trim);
> >>> +	if (ret < 0 && ret != -EINVAL)
> >>> +		return ret;
> >>>  
> >>>  	return 0;
> >>>  }
> >>> @@ -1545,6 +1556,7 @@ static int at86rf230_probe(struct spi_device *spi)
> >>>  	lp = hw->priv;
> >>>  	lp->hw = hw;
> >>>  	lp->spi = spi;
> >>> +	lp->cfg = cfg;
> >>
> >> This doesn't look correct.
> > 
> > You mean the line:
> > 
> > "lp->cfg = cfg;" or everything?
> 
> Just that line, it's in the wrong patch.
> 

yes, but in the other patch it makes no sense. I only use the platform
data in probe function and can't access the platform data over lp->cfg
there, because lp isn't allocated. So I can add it in patch 1/2 but then
I will never access the lp->cfg then.

devm_request_gpio thing will remember which gpio was requested. I never
read the platform_data again.


Now in this patch (2/2) I will access the platform data in hw_init and the lp
is allocated already in this function and can access xtal_trim over
lp->cfg->xtal_trim.

We don't really need after probe the platform data again. Further when
we will add support for the slp_tr pin we need that. This pin is used
after probing. This means copying platform data in at86rf230_local is a
good thing to prepare for future.

Nevertheless I will move it to 1/2.

- Alex

Re: [PATCHv3 bluetooth-next 2/2] at86rf230: add support for external xtal trim

[Alexander Aring]

On Tue, Feb 24, 2015 at 11:42:36AM +0100, Alexander Aring wrote:
> On Tue, Feb 24, 2015 at 11:34:17AM +0100, Marc Kleine-Budde wrote:
> > On 02/24/2015 11:28 AM, Alexander Aring wrote:
> > > Hi Marc,
> > > 
> > > On Tue, Feb 24, 2015 at 11:21:43AM +0100, Marc Kleine-Budde wrote:
> > >> On 02/24/2015 11:11 AM, Alexander Aring wrote:
> > >>> This patch adds support for setting the xtal trim register. Some at86rf2xx
> > >>> transceiver boards needs fine tuning the xtal capacitor.
> > >>>
> > >>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > >>> ---
> > >>>  .../devicetree/bindings/net/ieee802154/at86rf230.txt         |  3 +++
> > >>>  drivers/net/ieee802154/at86rf230.c                           | 12 ++++++++++++
> > >>>  include/linux/spi/at86rf230.h                                |  1 +
> > >>>  3 files changed, 16 insertions(+)
> > >>>
> > > ...
> > >>> @@ -1390,6 +1397,10 @@ static int at86rf230_get_pdata(struct spi_device *spi,
> > >>>  
> > >>>  	cfg->rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
> > >>>  	cfg->slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
> > >>> +	ret = of_property_read_u8(spi->dev.of_node, "xtal-trim",
> > >>> +				  &cfg->xtal_trim);
> > >>> +	if (ret < 0 && ret != -EINVAL)
> > >>> +		return ret;
> > >>>  
> > >>>  	return 0;
> > >>>  }
> > >>> @@ -1545,6 +1556,7 @@ static int at86rf230_probe(struct spi_device *spi)
> > >>>  	lp = hw->priv;
> > >>>  	lp->hw = hw;
> > >>>  	lp->spi = spi;
> > >>> +	lp->cfg = cfg;
> > >>
> > >> This doesn't look correct.
> > > 
> > > You mean the line:
> > > 
> > > "lp->cfg = cfg;" or everything?
> > 
> > Just that line, it's in the wrong patch.
> > 
> 
> yes, but in the other patch it makes no sense. I only use the platform
> data in probe function and can't access the platform data over lp->cfg
> there, because lp isn't allocated. So I can add it in patch 1/2 but then
> I will never access the lp->cfg then.
> 
> devm_request_gpio thing will remember which gpio was requested. I never
> read the platform_data again.
> 
> 
> Now in this patch (2/2) I will access the platform data in hw_init and the lp
> is allocated already in this function and can access xtal_trim over
> lp->cfg->xtal_trim.
> 
> We don't really need after probe the platform data again. Further when
> we will add support for the slp_tr pin we need that. This pin is used
> after probing. This means copying platform data in at86rf230_local is a
> good thing to prepare for future.
> 
> Nevertheless I will move it to 1/2.
> 

Okay, I simple move the ieee802154_alloc_hw call a little bit earlier.
Since we have devm_request_... thing the error handling will not more
complex when doing this step.

- Alex

Re: [PATCHv3 bluetooth-next 2/2] at86rf230: add support for external xtal trim

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2501 bytes --]

On 02/24/2015 11:42 AM, Alexander Aring wrote:
> On Tue, Feb 24, 2015 at 11:34:17AM +0100, Marc Kleine-Budde wrote:
>> On 02/24/2015 11:28 AM, Alexander Aring wrote:
>>> Hi Marc,
>>>
>>> On Tue, Feb 24, 2015 at 11:21:43AM +0100, Marc Kleine-Budde wrote:
>>>> On 02/24/2015 11:11 AM, Alexander Aring wrote:
>>>>> This patch adds support for setting the xtal trim register. Some at86rf2xx
>>>>> transceiver boards needs fine tuning the xtal capacitor.
>>>>>
>>>>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
>>>>> ---
>>>>>  .../devicetree/bindings/net/ieee802154/at86rf230.txt         |  3 +++
>>>>>  drivers/net/ieee802154/at86rf230.c                           | 12 ++++++++++++
>>>>>  include/linux/spi/at86rf230.h                                |  1 +
>>>>>  3 files changed, 16 insertions(+)
>>>>>
>>> ...
>>>>> @@ -1390,6 +1397,10 @@ static int at86rf230_get_pdata(struct spi_device *spi,
>>>>>  
>>>>>  	cfg->rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
>>>>>  	cfg->slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
>>>>> +	ret = of_property_read_u8(spi->dev.of_node, "xtal-trim",
>>>>> +				  &cfg->xtal_trim);
>>>>> +	if (ret < 0 && ret != -EINVAL)
>>>>> +		return ret;
>>>>>  
>>>>>  	return 0;
>>>>>  }
>>>>> @@ -1545,6 +1556,7 @@ static int at86rf230_probe(struct spi_device *spi)
>>>>>  	lp = hw->priv;
>>>>>  	lp->hw = hw;
>>>>>  	lp->spi = spi;
>>>>> +	lp->cfg = cfg;
>>>>
>>>> This doesn't look correct.
>>>
>>> You mean the line:
>>>
>>> "lp->cfg = cfg;" or everything?
>>
>> Just that line, it's in the wrong patch.
>>
> 
> yes, but in the other patch it makes no sense. I only use the platform
> data in probe function and can't access the platform data over lp->cfg
> there, because lp isn't allocated. So I can add it in patch 1/2 but then
> I will never access the lp->cfg then.
> 
> devm_request_gpio thing will remember which gpio was requested. I never
> read the platform_data again.

This means it makes no sense of copying/embedding the whole platform
data into the private struct. You should initialize your private data
from the devicetree node if available, otherwise from the platform data.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]