Re: [PATCH bluetooth-next] at86rf230: add support for setting external xtal

[Alexander Aring <alex.aring@gmail.com>]

On Mon, Feb 23, 2015 at 04:54:15PM +0100, Marc Kleine-Budde wrote:
> On 02/23/2015 04:41 PM, Alexander Aring wrote:
> > Marc,
> > 
> > On Mon, Feb 23, 2015 at 04:13:29PM +0100, Alexander Aring wrote:
> >> On Mon, Feb 23, 2015 at 04:10:58PM +0100, Marc Kleine-Budde wrote:
> >>> On 02/23/2015 04:05 PM, Alexander Aring wrote:
> >>>> This patch adds support for setting external xtal. This is recommended
> > ...
> >>>>  	rc = at86rf230_read_subreg(lp, SR_DVDD_OK, &dvdd);
> >>>>  	if (rc)
> >>>>  		return rc;
> >>>> @@ -1392,6 +1403,16 @@ at86rf230_get_pdata(struct spi_device *spi)
> >>>>  	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);
> >>>>  
> >>>> +	pdata->xtal = of_property_read_bool(spi->dev.of_node, "external-xtal");
> >>>
> >>> The platform data should be considered read only by the driver. Better
> >>> put the information into your per-driver private data struct.
> >>>
> >>
> >> ah, yes you are completely right here. Sorry :)
> >>
> > 
> > I am not sure if this is okay here in that case. For non devicetree this
> > code will never touched and I grab this code from some i2c driver like
> > [0].
> > 
> > They also use the platform-data for setting the devicetree properties.
> 
> That doesn't make it better.
> 
> > The only thing which I can see now is the check if (xtal > 0xF) which is
> > not available on non devicetree settings... but I can also remove this
> > check, somebody should make something complete wrong if this value is
> > above 0xF and regmap should mask this value.
> 
> IMHO this driver has been poorly converted to DT. The usual way would be
> to add data to at86rf230_local.

Okay, then I will do that and fix the other things and never use another
code for an example. (except the can branch) ;-)

- Alex