| Message ID | 20170626004434.2757-4-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Jun 26, 2017 at 12:44:32PM +1200, Chris Packham wrote: > Allow devices that use this driver to be registered via a > devicetree. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> ... > + if (IS_ERR(i2c->gpio)) > + return PTR_ERR(i2c->gpio); coccicheck reported: drivers/i2c/busses/i2c-pca-platform.c:209:3-9: ERROR: missing iounmap; ioremap on line 170 and execution via conditional on line 208 I fixed it like this (incremental diff): - if (IS_ERR(i2c->gpio)) - return PTR_ERR(i2c->gpio); + if (IS_ERR(i2c->gpio)) { + ret = PTR_ERR(i2c->gpio); + goto e_reqirq; + } and applied to for-next, thanks!
On Mon, Jun 26, 2017 at 3:44 AM, Chris Packham <chris.packham@alliedtelesis.co.nz> wrote: > Allow devices that use this driver to be registered via a > devicetree. Device Tree Wolfram was being a bit faster to apply it... > +#include <linux/of.h> > +#include <linux/of_device.h> > + struct device_node *np = pdev->dev.of_node; > /* If irq is 0, we do polling. */ > + if (irq < 0) > + irq = 0; This does not belong to what is written in commit message. Looks like a separate fix. > + i2c->adap.dev.of_node = np; > + } else if (np) { > + i2c->adap.timeout = HZ; > + i2c->gpio = devm_gpiod_get_optional(&pdev->dev, "reset-gpios", GPIOD_OUT_LOW); > + if (IS_ERR(i2c->gpio)) > + return PTR_ERR(i2c->gpio); > + of_property_read_u32_index(np, "clock-frequency", 0, > + &i2c->algo_data.i2c_clock); And what prevents you to use device_property_read_*() here and get rid of OFstuff? > +#ifdef CONFIG_OF Better not to put it... > + .of_match_table = of_match_ptr(i2c_pca_of_match_table), ...and get rid of of_match_ptr().
> > /* If irq is 0, we do polling. */ > > + if (irq < 0) > > + irq = 0; > > This does not belong to what is written in commit message. Looks like > a separate fix. True, this needs explanation. > > + of_property_read_u32_index(np, "clock-frequency", 0, > > + &i2c->algo_data.i2c_clock); > > And what prevents you to use device_property_read_*() here and get rid > of OFstuff? device_* functions are not super much known yet. Even I tend to forget them occasionally like here. We can fix it incrementally, too. But I agree, it would be nice to have it right from the beginning.
On 28/06/17 21:20, Andy Shevchenko wrote: >> + i2c->adap.dev.of_node = np; >> + } else if (np) { >> + i2c->adap.timeout = HZ; >> + i2c->gpio = devm_gpiod_get_optional(&pdev->dev, "reset-gpios", GPIOD_OUT_LOW); >> + if (IS_ERR(i2c->gpio)) >> + return PTR_ERR(i2c->gpio); >> + of_property_read_u32_index(np, "clock-frequency", 0, >> + &i2c->algo_data.i2c_clock); > And what prevents you to use device_property_read_*() here and get rid > of OFstuff? > Nothing particular. I think I just found more instances of parsing a "clock-frequency" this way. Happy to change this. >> +#ifdef CONFIG_OF > Better not to put it... > >> + .of_match_table = of_match_ptr(i2c_pca_of_match_table), > ...and get rid of of_match_ptr(). So what's the current best practice with this? I gather the intent is to keep the kernel size down by only including the of_match tables on platforms that actually use a device tree. There are just shy of 1000 instances of of_match_ptr in the current tree (a handful of which are in drivers/i2c). Have we now reached a point where there are more dt-aware platforms than unaware ones?
diff --git a/drivers/i2c/busses/i2c-pca-platform.c b/drivers/i2c/busses/i2c-pca-platform.c index 9f995b8ed587..a6df6b8d2289 100644 --- a/drivers/i2c/busses/i2c-pca-platform.c +++ b/drivers/i2c/busses/i2c-pca-platform.c @@ -24,6 +24,8 @@ #include <linux/gpio.h> #include <linux/gpio/consumer.h> #include <linux/io.h> +#include <linux/of.h> +#include <linux/of_device.h> #include <asm/irq.h> @@ -137,12 +139,15 @@ static int i2c_pca_pf_probe(struct platform_device *pdev) struct resource *res; struct i2c_pca9564_pf_platform_data *platform_data = dev_get_platdata(&pdev->dev); + struct device_node *np = pdev->dev.of_node; int ret = 0; int irq; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); irq = platform_get_irq(pdev, 0); /* If irq is 0, we do polling. */ + if (irq < 0) + irq = 0; if (res == NULL) { ret = -ENODEV; @@ -178,6 +183,7 @@ static int i2c_pca_pf_probe(struct platform_device *pdev) (unsigned long) res->start); i2c->adap.algo_data = &i2c->algo_data; i2c->adap.dev.parent = &pdev->dev; + i2c->adap.dev.of_node = np; if (platform_data) { i2c->adap.timeout = platform_data->timeout; @@ -196,6 +202,13 @@ static int i2c_pca_pf_probe(struct platform_device *pdev) i2c->gpio = NULL; } } + } else if (np) { + i2c->adap.timeout = HZ; + i2c->gpio = devm_gpiod_get_optional(&pdev->dev, "reset-gpios", GPIOD_OUT_LOW); + if (IS_ERR(i2c->gpio)) + return PTR_ERR(i2c->gpio); + of_property_read_u32_index(np, "clock-frequency", 0, + &i2c->algo_data.i2c_clock); } else { i2c->adap.timeout = HZ; i2c->algo_data.i2c_clock = 59000; @@ -270,11 +283,21 @@ static int i2c_pca_pf_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_OF +static const struct of_device_id i2c_pca_of_match_table[] = { + { .compatible = "nxp,pca9564" }, + { .compatible = "nxp,pca9665" }, + {}, +}; +MODULE_DEVICE_TABLE(of, i2c_pca_of_match_table); +#endif + static struct platform_driver i2c_pca_pf_driver = { .probe = i2c_pca_pf_probe, .remove = i2c_pca_pf_remove, .driver = { .name = "i2c-pca-platform", + .of_match_table = of_match_ptr(i2c_pca_of_match_table), }, };
Allow devices that use this driver to be registered via a devicetree. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- Changes in v2: - Set i2c->adap.dev.of_node so that child nodes are automatically probed - Split dt-binding to separate patch, use "reset-gpios" instead of "gpios". Changes in v3: - None drivers/i2c/busses/i2c-pca-platform.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)