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. 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:
signature.asc
Description: OpenPGP digital signature
