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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- 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 --]