On 01/31/2014 11:35 AM, Florian Vaussard wrote: > The OpenFirmware probe can be merged into the standard platform > probe to leverage common code.
Good work, as we want to replace the existing driver, I'm quite picky on this patch, see more comments inline. Please don't delete of of_platform driver, yet. > Signed-off-by: Florian Vaussard <[email protected]> > --- > drivers/net/can/sja1000/Kconfig | 13 +- > drivers/net/can/sja1000/Makefile | 1 - > drivers/net/can/sja1000/sja1000_of_platform.c | 218 > -------------------------- > drivers/net/can/sja1000/sja1000_platform.c | 141 +++++++++++++---- > 4 files changed, 116 insertions(+), 257 deletions(-) > delete mode 100644 drivers/net/can/sja1000/sja1000_of_platform.c [...] > diff --git a/drivers/net/can/sja1000/sja1000_platform.c > b/drivers/net/can/sja1000/sja1000_platform.c > index 779679a..96a92a1 100644 > --- a/drivers/net/can/sja1000/sja1000_platform.c > +++ b/drivers/net/can/sja1000/sja1000_platform.c > @@ -27,12 +27,16 @@ > #include <linux/can/dev.h> > #include <linux/can/platform/sja1000.h> > #include <linux/io.h> > +#include <linux/of.h> > +#include <linux/of_irq.h> > > #include "sja1000.h" > > #define DRV_NAME "sja1000_platform" > +#define SJA1000_OFP_CAN_CLOCK (16000000 / 2) This driver uses "SP" short for sja100_platform as namespace, please convert. > > MODULE_AUTHOR("Sascha Hauer <[email protected]>"); > +MODULE_AUTHOR("Wolfgang Grandegger <[email protected]>"); > MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus"); > MODULE_ALIAS("platform:" DRV_NAME); > MODULE_LICENSE("GPL v2"); > @@ -67,24 +71,98 @@ static void sp_write_reg32(const struct sja1000_priv > *priv, int reg, u8 val) > iowrite8(val, priv->reg_base + reg * 4); > } > > -static int sp_probe(struct platform_device *pdev) > +static void sp_populate(struct sja1000_priv *priv, > + struct sja1000_platform_data *pdata, > + unsigned long resource_mem_flags) > +{ > + /* The CAN clock frequency is half the oscillator clock frequency */ > + priv->can.clock.freq = pdata->osc_freq / 2; > + priv->ocr = pdata->ocr; > + priv->cdr = pdata->cdr; > + > + switch (resource_mem_flags & IORESOURCE_MEM_TYPE_MASK) { > + case IORESOURCE_MEM_32BIT: > + priv->read_reg = sp_read_reg32; > + priv->write_reg = sp_write_reg32; > + break; > + case IORESOURCE_MEM_16BIT: > + priv->read_reg = sp_read_reg16; > + priv->write_reg = sp_write_reg16; > + break; > + case IORESOURCE_MEM_8BIT: > + default: > + priv->read_reg = sp_read_reg8; > + priv->write_reg = sp_write_reg8; > + break; > + } > +} > + > +#if defined(CONFIG_OF) The driver also compiles on systems without CONFIG_OF without these defines. Please remove. > +static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of) > { > int err; > + u32 prop; > + > + priv->read_reg = sp_read_reg8; > + priv->write_reg = sp_write_reg8; > + > + err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop); > + if (!err) > + priv->can.clock.freq = prop / 2; > + else > + priv->can.clock.freq = SJA1000_OFP_CAN_CLOCK; /* default */ > + > + err = of_property_read_u32(of, "nxp,tx-output-mode", &prop); > + if (!err) > + priv->ocr |= prop & OCR_MODE_MASK; > + else > + priv->ocr |= OCR_MODE_NORMAL; /* default */ > + > + err = of_property_read_u32(of, "nxp,tx-output-config", &prop); > + if (!err) > + priv->ocr |= (prop << OCR_TX_SHIFT) & OCR_TX_MASK; > + else > + priv->ocr |= OCR_TX0_PULLDOWN; /* default */ > + > + err = of_property_read_u32(of, "nxp,clock-out-frequency", &prop); > + if (!err && prop) { > + u32 divider = priv->can.clock.freq * 2 / prop; > + > + if (divider > 1) > + priv->cdr |= divider / 2 - 1; > + else > + priv->cdr |= CDR_CLKOUT_MASK; > + } else { > + priv->cdr |= CDR_CLK_OFF; /* default */ > + } > + > + if (!of_property_read_bool(of, "nxp,no-comparator-bypass")) > + priv->cdr |= CDR_CBP; /* default */ > +} > +#else > +static void sp_populate_of(struct sja1000_priv *priv, device_node *of) > +{ > +} > +#endif > + > +static int sp_probe(struct platform_device *pdev) > +{ > + int err, irq = 0; > void __iomem *addr; > struct net_device *dev; > struct sja1000_priv *priv; > - struct resource *res_mem, *res_irq; > + struct resource *res_mem, *res_irq = 0; Please use NULL to initialize pointers. > struct sja1000_platform_data *pdata; > + struct device_node *of = pdev->dev.of_node; > > pdata = dev_get_platdata(&pdev->dev); > - if (!pdata) { > + if (!pdata && !of) { > dev_err(&pdev->dev, "No platform data provided!\n"); > return -ENODEV; > } > > res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > - if (!res_mem || !res_irq) > + if (!res_mem) > return -ENODEV; > > if (!devm_request_mem_region(&pdev->dev, res_mem->start, > @@ -96,36 +174,34 @@ static int sp_probe(struct platform_device *pdev) > if (!addr) > return -ENOMEM; > > + if (of) > + irq = irq_of_parse_and_map(of, 0); > + else > + res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + > + if (!irq && !res_irq) > + return -ENODEV; > + > dev = alloc_sja1000dev(0); > if (!dev) > return -ENOMEM; > priv = netdev_priv(dev); > > - dev->irq = res_irq->start; > - priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK; > - if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE) > - priv->irq_flags |= IRQF_SHARED; > + if (res_irq) { > + irq = res_irq->start; > + priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK; > + if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE) > + priv->irq_flags |= IRQF_SHARED; > + } else > + priv->irq_flags = IRQF_SHARED; Please add { } for else, too. > + > + dev->irq = irq; > priv->reg_base = addr; > - /* The CAN clock frequency is half the oscillator clock frequency */ > - priv->can.clock.freq = pdata->osc_freq / 2; > - priv->ocr = pdata->ocr; > - priv->cdr = pdata->cdr; > > - switch (res_mem->flags & IORESOURCE_MEM_TYPE_MASK) { > - case IORESOURCE_MEM_32BIT: > - priv->read_reg = sp_read_reg32; > - priv->write_reg = sp_write_reg32; > - break; > - case IORESOURCE_MEM_16BIT: > - priv->read_reg = sp_read_reg16; > - priv->write_reg = sp_write_reg16; > - break; > - case IORESOURCE_MEM_8BIT: > - default: > - priv->read_reg = sp_read_reg8; > - priv->write_reg = sp_write_reg8; > - break; > - } > + if (of) > + sp_populate_of(priv, of); > + else > + sp_populate(priv, pdata, res_mem->flags); > > platform_set_drvdata(pdev, dev); > SET_NETDEV_DEV(dev, &pdev->dev); > @@ -156,12 +232,21 @@ static int sp_remove(struct platform_device *pdev) > return 0; > } > > +#if defined(CONFIG_OF) Please remove the ifdef. > +static struct of_device_id sja1000_ofp_table[] = { Please rename into sp_of_table or something similar. > + {.compatible = "nxp,sja1000"}, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, sja1000_ofp_table); > +#endif > + > static struct platform_driver sp_driver = { > .probe = sp_probe, > .remove = sp_remove, > .driver = { > .name = DRV_NAME, > .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(sja1000_ofp_table), The of_match_ptr is not needed anymore, please remove. > }, > }; > > 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 |
signature.asc
Description: OpenPGP digital signature
