Re: [PATCH v2 4/6] can: sja1000: fuse of_platform into platform
From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: 2014-01-31 12:33:45
Also in:
linux-can, lkml
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 <redacted> --- 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
[...]
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
MODULE_AUTHOR("Sascha Hauer [off-list ref]"); +MODULE_AUTHOR("Wolfgang Grandegger [off-list ref]"); 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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
+ + 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 |
Attachments
- signature.asc [application/pgp-signature] 242 bytes