* [PATCH 1/4 v2] mfd: add STw481x driver
@ 2013-09-13 17:54 Linus Walleij
2013-09-16 9:19 ` Lee Jones
0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2013-09-13 17:54 UTC (permalink / raw)
To: Samuel Ortiz, Lee Jones
Cc: linux-kernel, Mark Brown, Wang Shilong, Linus Walleij
This adds a driver for the STw481x PMICs found in the Nomadik
family of platforms. This one uses pure device tree probing.
Print some of the OTP registers on boot and register a regulator
MFD child.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Hi Sam/Lee, I'm seeking an ACK for this driver to take it through
the ARM SoC tree with the enablement patches.
ChangeLog v1->v2:
- Fix remnant checkpatch errors.
- #define some more registers to make things clear.
- Document the power control register access function.
- Don't free devm_*-allocated devices.
- Use module_i2c_driver() macro.
---
drivers/mfd/Kconfig | 10 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/stw481x.c | 250 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/stw481x.h | 56 ++++++++++
4 files changed, 317 insertions(+)
create mode 100644 drivers/mfd/stw481x.c
create mode 100644 include/linux/mfd/stw481x.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index e0e46f5..cf6c370 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1157,6 +1157,16 @@ config MFD_WM8994
core support for the WM8994, in order to use the actual
functionaltiy of the device other drivers must be enabled.
+config MFD_STW481X
+ bool "Support for ST Microelectronics STw481x"
+ depends on I2C && ARCH_NOMADIK
+ select REGMAP_I2C
+ select MFD_CORE
+ help
+ Select this option to enable the STw481x chip driver used
+ in various ST Microelectronics and ST-Ericsson embedded
+ Nomadik series.
+
endmenu
endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 15b905c..656cec9 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -162,3 +162,4 @@ obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o
obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o vexpress-sysreg.o
obj-$(CONFIG_MFD_RETU) += retu-mfd.o
obj-$(CONFIG_MFD_AS3711) += as3711.o
+obj-$(CONFIG_MFD_STW481X) += stw481x.o
diff --git a/drivers/mfd/stw481x.c b/drivers/mfd/stw481x.c
new file mode 100644
index 0000000..d154e68
--- /dev/null
+++ b/drivers/mfd/stw481x.c
@@ -0,0 +1,250 @@
+/*
+ * Core driver for STw4810/STw4811
+ *
+ * Copyright (C) 2013 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/stw481x.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+
+/*
+ * This driver can only access the non-USB portions of STw4811, the register
+ * range 0x00-0x10 dealing with USB is bound to the two special I2C pins used
+ * for USB control.
+ */
+
+/* Registers inside the power control address space */
+#define STW_PC_VCORE_SEL 0x05U
+#define STW_PC_VAUX_SEL 0x06U
+#define STW_PC_VPLL_SEL 0x07U
+
+/**
+ * stw481x_get_pctl_reg() - get a power control register
+ * @stw481x: handle to the stw481x chip
+ * @reg: power control register to fetch
+ *
+ * The power control registers is a set of one-time-programmable registers
+ * in its own register space, accessed by writing addess bits to these
+ * two registers: bits 7,6,5 of PCTL_REG_LO corresponds to the 3 LSBs of
+ * the address and bits 8,9 of PCTL_REG_HI corresponds to the 2 MSBs of
+ * the address, forming an address space of 5 bits, i.e. 32 registers
+ * 0x00 ... 0x1f can be obtained.
+ */
+static int stw481x_get_pctl_reg(struct stw481x *stw481x, u8 reg)
+{
+ u8 msb = (reg >> 3) & 0x03;
+ u8 lsb = (reg << 5) & 0xe0;
+ unsigned int val;
+ u8 vrfy;
+ int ret;
+
+ ret = regmap_write(stw481x->map, STW_PCTL_REG_HI, msb);
+ if (ret)
+ return ret;
+ ret = regmap_write(stw481x->map, STW_PCTL_REG_LO, lsb);
+ if (ret)
+ return ret;
+ ret = regmap_read(stw481x->map, STW_PCTL_REG_HI, &val);
+ if (ret)
+ return ret;
+ vrfy = (val & 0x03) << 3;
+ ret = regmap_read(stw481x->map, STW_PCTL_REG_LO, &val);
+ if (ret)
+ return ret;
+ vrfy |= ((val >> 5) & 0x07);
+ if (vrfy != reg)
+ return -EIO;
+ return (val >> 1) & 0x0f;
+}
+
+static int stw481x_startup(struct stw481x *stw481x)
+{
+ /* Voltages multiplied by 100 */
+ u8 vcore_val[] = { 100, 105, 110, 115, 120, 122, 124, 126, 128,
+ 130, 132, 134, 136, 138, 140, 145 };
+ u8 vpll_val[] = { 105, 120, 130, 180 };
+ u8 vaux_val[] = { 15, 18, 25, 28 };
+ u8 vcore;
+ u8 vcore_slp;
+ u8 vpll;
+ u8 vaux;
+ bool vaux_en;
+ bool it_warn;
+ int ret;
+ unsigned int val;
+
+ ret = regmap_read(stw481x->map, STW_CONF1, &val);
+ if (ret)
+ return ret;
+ vaux_en = !!(val & STW_CONF1_PDN_VAUX);
+ it_warn = !!(val & STW_CONF1_IT_WARN);
+
+ dev_info(&stw481x->client->dev, "voltages %s\n",
+ (val & STW_CONF1_V_MONITORING) ? "OK" : "LOW");
+ dev_info(&stw481x->client->dev, "MMC level shifter %s\n",
+ (val & STW_CONF1_MMC_LS_STATUS) ? "high impedance" : "ON");
+ dev_info(&stw481x->client->dev, "VMMC: %s\n",
+ (val & STW_CONF1_PDN_VMMC) ? "ON" : "disabled");
+
+ dev_info(&stw481x->client->dev, "STw481x power control registers:\n");
+
+ ret = stw481x_get_pctl_reg(stw481x, STW_PC_VCORE_SEL);
+ if (ret < 0)
+ return ret;
+ vcore = ret & 0x0f;
+
+ ret = stw481x_get_pctl_reg(stw481x, STW_PC_VAUX_SEL);
+ if (ret < 0)
+ return ret;
+ vaux = (ret >> 2) & 3;
+ vpll = (ret >> 4) & 1; /* Save bit 4 */
+
+ ret = stw481x_get_pctl_reg(stw481x, STW_PC_VPLL_SEL);
+ if (ret < 0)
+ return ret;
+ vpll |= (ret >> 1) & 2;
+
+ dev_info(&stw481x->client->dev, "VCORE: %u.%uV %s\n",
+ vcore_val[vcore] / 100, vcore_val[vcore] % 100,
+ (ret & 4) ? "ON" : "OFF");
+
+ dev_info(&stw481x->client->dev, "VPLL: %u.%uV %s\n",
+ vpll_val[vpll] / 100, vpll_val[vpll] % 100,
+ (ret & 0x10) ? "ON" : "OFF");
+
+ dev_info(&stw481x->client->dev, "VAUX: %u.%uV %s\n",
+ vaux_val[vaux] / 10, vaux_val[vaux] % 10,
+ vaux_en ? "ON" : "OFF");
+
+ ret = regmap_read(stw481x->map, STW_CONF2, &val);
+ if (ret)
+ return ret;
+
+ dev_info(&stw481x->client->dev, "TWARN: %s threshold, %s\n",
+ it_warn ? "below" : "above",
+ (val & STW_CONF2_MASK_TWARN) ?
+ "enabled" : "mask through VDDOK");
+ dev_info(&stw481x->client->dev, "VMMC: %s\n",
+ (val & STW_CONF2_VMMC_EXT) ? "internal" : "external");
+ dev_info(&stw481x->client->dev, "IT WAKE UP: %s\n",
+ (val & STW_CONF2_MASK_IT_WAKE_UP) ? "enabled" : "masked");
+ dev_info(&stw481x->client->dev, "GPO1: %s\n",
+ (val & STW_CONF2_GPO1) ? "low" : "high impedance");
+ dev_info(&stw481x->client->dev, "GPO2: %s\n",
+ (val & STW_CONF2_GPO2) ? "low" : "high impedance");
+
+ ret = regmap_read(stw481x->map, STW_VCORE_SLEEP, &val);
+ if (ret)
+ return ret;
+ vcore_slp = val & 0x0f;
+ dev_info(&stw481x->client->dev, "VCORE SLEEP: %u.%uV\n",
+ vcore_val[vcore_slp] / 100, vcore_val[vcore_slp] % 100);
+
+ return 0;
+}
+
+/*
+ * MFD cells - we have one cell which is selected operation
+ * mode, and we always have a GPIO cell.
+ */
+static struct mfd_cell stw481x_cells[] = {
+ {
+ .of_compatible = "st,stw481x-vmmc",
+ .name = "stw481x-vmmc-regulator",
+ .id = -1,
+ },
+};
+
+const struct regmap_config stw481x_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+static int stw481x_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct stw481x *stw481x;
+ int ret;
+ int i;
+
+ stw481x = devm_kzalloc(&client->dev, sizeof(*stw481x), GFP_KERNEL);
+ if (!stw481x)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, stw481x);
+ stw481x->client = client;
+ stw481x->map = devm_regmap_init_i2c(client, &stw481x_regmap_config);
+
+ ret = stw481x_startup(stw481x);
+ if (ret) {
+ dev_err(&client->dev, "chip initialization failed\n");
+ return ret;
+ }
+
+ /* Set up and register the platform devices. */
+ for (i = 0; i < ARRAY_SIZE(stw481x_cells); i++) {
+ /* One state holder for all drivers, this is simple */
+ stw481x_cells[i].platform_data = stw481x;
+ stw481x_cells[i].pdata_size = sizeof(*stw481x);
+ }
+
+ ret = mfd_add_devices(&client->dev, 0, stw481x_cells,
+ ARRAY_SIZE(stw481x_cells), NULL, 0, NULL);
+ if (ret)
+ return ret;
+
+ dev_info(&client->dev, "initialized STw481x device\n");
+
+ return ret;
+}
+
+static int stw481x_remove(struct i2c_client *client)
+{
+ mfd_remove_devices(&client->dev);
+ return 0;
+}
+
+/*
+ * This ID table is completely unused, as this is a pure
+ * device-tree probed driver, but it has to be here due to
+ * the structure of the I2C core.
+ */
+static const struct i2c_device_id dummy_id[] = {
+ { "dummy", 0 },
+ { }
+};
+
+static const struct of_device_id stw481x_match[] = {
+ { .compatible = "st,stw4810", },
+ { .compatible = "st,stw4811", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, stw481x_match);
+
+static struct i2c_driver stw481x_driver = {
+ .driver = {
+ .name = "stw481x",
+ .of_match_table = stw481x_match,
+ },
+ .probe = stw481x_probe,
+ .remove = stw481x_remove,
+ .id_table = dummy_id,
+};
+
+module_i2c_driver(stw481x_driver);
+
+MODULE_AUTHOR("Linus Walleij");
+MODULE_DESCRIPTION("STw481x PMIC driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/stw481x.h b/include/linux/mfd/stw481x.h
new file mode 100644
index 0000000..eda1215
--- /dev/null
+++ b/include/linux/mfd/stw481x.h
@@ -0,0 +1,56 @@
+/*
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#ifndef MFD_STW481X_H
+#define MFD_STW481X_H
+
+#include <linux/i2c.h>
+#include <linux/regulator/machine.h>
+#include <linux/regmap.h>
+#include <linux/bitops.h>
+
+/* These registers are accessed from more than one driver */
+#define STW_CONF1 0x11U
+#define STW_CONF1_PDN_VMMC 0x01U
+#define STW_CONF1_VMMC_MASK 0x0eU
+#define STW_CONF1_VMMC_1_8V 0x02U
+#define STW_CONF1_VMMC_2_85V 0x04U
+#define STW_CONF1_VMMC_3V 0x06U
+#define STW_CONF1_VMMC_1_85V 0x08U
+#define STW_CONF1_VMMC_2_6V 0x0aU
+#define STW_CONF1_VMMC_2_7V 0x0cU
+#define STW_CONF1_VMMC_3_3V 0x0eU
+#define STW_CONF1_MMC_LS_STATUS 0x10U
+#define STW_PCTL_REG_LO 0x1eU
+#define STW_PCTL_REG_HI 0x1fU
+#define STW_CONF1_V_MONITORING 0x20U
+#define STW_CONF1_IT_WARN 0x40U
+#define STW_CONF1_PDN_VAUX 0x80U
+#define STW_CONF2 0x20U
+#define STW_CONF2_MASK_TWARN 0x01U
+#define STW_CONF2_VMMC_EXT 0x02U
+#define STW_CONF2_MASK_IT_WAKE_UP 0x04U
+#define STW_CONF2_GPO1 0x08U
+#define STW_CONF2_GPO2 0x10U
+#define STW_VCORE_SLEEP 0x21U
+
+/**
+ * struct stw481x - state holder for the Stw481x drivers
+ * @mutex: mutex to serialize I2C accesses
+ * @i2c_client: corresponding I2C client
+ * @regulator: regulator device for regulator children
+ * @map: regmap handle to access device registers
+ */
+struct stw481x {
+ struct mutex lock;
+ struct i2c_client *client;
+ struct regulator_dev *vmmc_regulator;
+ struct regmap *map;
+};
+
+#endif
--
1.8.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/4 v2] mfd: add STw481x driver
2013-09-13 17:54 [PATCH 1/4 v2] mfd: add STw481x driver Linus Walleij
@ 2013-09-16 9:19 ` Lee Jones
2013-09-16 10:40 ` Mark Brown
0 siblings, 1 reply; 12+ messages in thread
From: Lee Jones @ 2013-09-16 9:19 UTC (permalink / raw)
To: Linus Walleij; +Cc: Samuel Ortiz, linux-kernel, Mark Brown, Wang Shilong
On Fri, 13 Sep 2013, Linus Walleij wrote:
> This adds a driver for the STw481x PMICs found in the Nomadik
> family of platforms. This one uses pure device tree probing.
> Print some of the OTP registers on boot and register a regulator
> MFD child.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Hi Sam/Lee, I'm seeking an ACK for this driver to take it through
> the ARM SoC tree with the enablement patches.
Sorry, not just yet. I need to straighten out one last thing.
> ChangeLog v1->v2:
> - Fix remnant checkpatch errors.
> - #define some more registers to make things clear.
> - Document the power control register access function.
> - Don't free devm_*-allocated devices.
> - Use module_i2c_driver() macro.
> ---
> drivers/mfd/Kconfig | 10 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/stw481x.c | 250 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/stw481x.h | 56 ++++++++++
> 4 files changed, 317 insertions(+)
> create mode 100644 drivers/mfd/stw481x.c
> create mode 100644 include/linux/mfd/stw481x.h
>
> +/*
> + * This ID table is completely unused, as this is a pure
> + * device-tree probed driver, but it has to be here due to
> + * the structure of the I2C core.
> + */
>From what I can gather, the code doesn't agree with you.
> +static const struct i2c_device_id dummy_id[] = {
> + { "dummy", 0 },
> + { }
> +};
Can't you just NULL .id_table?
Here's the code which would use it:
> /* match on an id table if there is one */
> if (driver->id_table)
> return i2c_match_id(driver->id_table, client) != NULL;
Matching for "dummy" will just waste cycles.
> +static struct i2c_driver stw481x_driver = {
> + .driver = {
> + .name = "stw481x",
> + .of_match_table = stw481x_match,
> + },
> + .probe = stw481x_probe,
> + .remove = stw481x_remove,
> + .id_table = dummy_id,
> +};
> +
> +module_i2c_driver(stw481x_driver);
> +
> +MODULE_AUTHOR("Linus Walleij");
> +MODULE_DESCRIPTION("STw481x PMIC driver");
> +MODULE_LICENSE("GPL v2");
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/4 v2] mfd: add STw481x driver
2013-09-16 9:19 ` Lee Jones
@ 2013-09-16 10:40 ` Mark Brown
2013-09-16 11:06 ` Lee Jones
2013-09-16 12:44 ` Linus Walleij
0 siblings, 2 replies; 12+ messages in thread
From: Mark Brown @ 2013-09-16 10:40 UTC (permalink / raw)
To: Lee Jones; +Cc: Linus Walleij, Samuel Ortiz, linux-kernel, Wang Shilong
[-- Attachment #1: Type: text/plain, Size: 454 bytes --]
On Mon, Sep 16, 2013 at 10:19:56AM +0100, Lee Jones wrote:
> Can't you just NULL .id_table?
> Here's the code which would use it:
> > /* match on an id table if there is one */
> > if (driver->id_table)
> > return i2c_match_id(driver->id_table, client) != NULL;
> Matching for "dummy" will just waste cycles.
i2c_device_probe() will return -ENODEV if id_table is NULL before we get
to actually matching. We could remove that check though...
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4 v2] mfd: add STw481x driver
2013-09-16 10:40 ` Mark Brown
@ 2013-09-16 11:06 ` Lee Jones
2013-09-16 12:44 ` Linus Walleij
1 sibling, 0 replies; 12+ messages in thread
From: Lee Jones @ 2013-09-16 11:06 UTC (permalink / raw)
To: Mark Brown; +Cc: Linus Walleij, Samuel Ortiz, linux-kernel, Wang Shilong
On Mon, 16 Sep 2013, Mark Brown wrote:
> On Mon, Sep 16, 2013 at 10:19:56AM +0100, Lee Jones wrote:
>
> > Can't you just NULL .id_table?
>
> > Here's the code which would use it:
> > > /* match on an id table if there is one */
> > > if (driver->id_table)
> > > return i2c_match_id(driver->id_table, client) != NULL;
>
> > Matching for "dummy" will just waste cycles.
>
> i2c_device_probe() will return -ENODEV if id_table is NULL before we get
> to actually matching. We could remove that check though...
Unless there will be any repercussions that I am naive to I think that
sounds suitable. Any volunteers?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4 v2] mfd: add STw481x driver
2013-09-16 10:40 ` Mark Brown
2013-09-16 11:06 ` Lee Jones
@ 2013-09-16 12:44 ` Linus Walleij
2013-09-16 13:51 ` Mark Brown
1 sibling, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2013-09-16 12:44 UTC (permalink / raw)
To: Mark Brown, Wolfram Sang, linux-i2c@vger.kernel.org
Cc: Lee Jones, Samuel Ortiz, linux-kernel@vger.kernel.org,
Wang Shilong
On Mon, Sep 16, 2013 at 12:40 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Sep 16, 2013 at 10:19:56AM +0100, Lee Jones wrote:
>
>> Can't you just NULL .id_table?
No. That is not OK with the I2C core. It's
not easy to get rid of the requirement for .id_table
not to be NULL either.
>> Here's the code which would use it:
>> > /* match on an id table if there is one */
>> > if (driver->id_table)
>> > return i2c_match_id(driver->id_table, client) != NULL;
>
>> Matching for "dummy" will just waste cycles.
>
> i2c_device_probe() will return -ENODEV if id_table is NULL before we get
> to actually matching. We could remove that check though...
Copy+pasting from my own reply earlier:
I've tried to fix this for DT-only I2C devices
and this very driver was the reason.
But a tiresome regression due to drivers relying on this
i2c_device_id not being NULL and inability to remove it from the I2C
core without refactoring the world ensued, see:
commit c80f52847c50109ca248c22efbf71ff10553dca4
Reverted in:
commit 661f6c1cd926c6c973e03c6b5151d161f3a666ed
For this reason I think:
http://marc.info/?l=linux-next&m=137148411231784&w=2
I have tentatively given up getting pure DT I2C drivers
to probe, I don't think I have the whole picture, but
Wolfram has serious doubts about this and say we have
to be careful ....
Wolfram, do you have some ideas on how we should
proceed or ar you happy with merging this as-is?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4 v2] mfd: add STw481x driver
2013-09-16 12:44 ` Linus Walleij
@ 2013-09-16 13:51 ` Mark Brown
2013-09-16 15:21 ` Wolfram Sang
0 siblings, 1 reply; 12+ messages in thread
From: Mark Brown @ 2013-09-16 13:51 UTC (permalink / raw)
To: Linus Walleij
Cc: Wolfram Sang, linux-i2c@vger.kernel.org, Lee Jones, Samuel Ortiz,
linux-kernel@vger.kernel.org, Wang Shilong
[-- Attachment #1: Type: text/plain, Size: 2291 bytes --]
On Mon, Sep 16, 2013 at 02:44:35PM +0200, Linus Walleij wrote:
> I've tried to fix this for DT-only I2C devices
> and this very driver was the reason.
> But a tiresome regression due to drivers relying on this
> i2c_device_id not being NULL and inability to remove it from the I2C
> core without refactoring the world ensued, see:
> commit c80f52847c50109ca248c22efbf71ff10553dca4
Oh, that was the change...
> Reverted in:
> commit 661f6c1cd926c6c973e03c6b5151d161f3a666ed
> For this reason I think:
> http://marc.info/?l=linux-next&m=137148411231784&w=2
> I have tentatively given up getting pure DT I2C drivers
> to probe, I don't think I have the whole picture, but
> Wolfram has serious doubts about this and say we have
> to be careful ....
> Wolfram, do you have some ideas on how we should
> proceed or ar you happy with merging this as-is?
I'd have expected that it should be possible to change things such that
the change in the core doesn't produce any change in behaviour for
existing drivers. Can we not change the patch so that i2c_match_id()
copes with getting a NULL id_table? Something like this:
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 29d3f04..61087ea 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -67,6 +67,9 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver);
static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
const struct i2c_client *client)
{
+ if (!id)
+ return NULL;
+
while (id->name[0]) {
if (strcmp(client->name, id->name) == 0)
return id;
@@ -92,11 +95,8 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
return 1;
driver = to_i2c_driver(drv);
- /* match on an id table if there is one */
- if (driver->id_table)
- return i2c_match_id(driver->id_table, client) != NULL;
- return 0;
+ return i2c_match_id(driver->id_table, client) != NULL;
}
@@ -246,7 +246,7 @@ static int i2c_device_probe(struct device *dev)
return 0;
driver = to_i2c_driver(dev->driver);
- if (!driver->probe || !driver->id_table)
+ if (!driver->probe)
return -ENODEV;
client->driver = driver;
if (!device_can_wakeup(&client->dev))
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/4 v2] mfd: add STw481x driver
2013-09-16 13:51 ` Mark Brown
@ 2013-09-16 15:21 ` Wolfram Sang
2013-09-16 15:34 ` Lee Jones
0 siblings, 1 reply; 12+ messages in thread
From: Wolfram Sang @ 2013-09-16 15:21 UTC (permalink / raw)
To: Mark Brown
Cc: Linus Walleij, linux-i2c@vger.kernel.org, Lee Jones, Samuel Ortiz,
linux-kernel@vger.kernel.org, Wang Shilong
[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]
On Mon, Sep 16, 2013 at 02:51:18PM +0100, Mark Brown wrote:
> On Mon, Sep 16, 2013 at 02:44:35PM +0200, Linus Walleij wrote:
>
> > I've tried to fix this for DT-only I2C devices
> > and this very driver was the reason.
>
> > But a tiresome regression due to drivers relying on this
> > i2c_device_id not being NULL and inability to remove it from the I2C
> > core without refactoring the world ensued, see:
> > commit c80f52847c50109ca248c22efbf71ff10553dca4
>
> Oh, that was the change...
>
> > Reverted in:
> > commit 661f6c1cd926c6c973e03c6b5151d161f3a666ed
>
> > For this reason I think:
> > http://marc.info/?l=linux-next&m=137148411231784&w=2
>
> > I have tentatively given up getting pure DT I2C drivers
> > to probe, I don't think I have the whole picture, but
> > Wolfram has serious doubts about this and say we have
> > to be careful ....
>
> > Wolfram, do you have some ideas on how we should
> > proceed or ar you happy with merging this as-is?
>
> I'd have expected that it should be possible to change things such that
> the change in the core doesn't produce any change in behaviour for
> existing drivers. Can we not change the patch so that i2c_match_id()
> copes with getting a NULL id_table? Something like this:
I hacked something like this after Linus posted his approach. However, I
found out that run time instanciating ('new_device' file) needs an
id_table. I wasn't to keen on disabling the feature for dt-only drivers.
That's where I stopped, due to lack of time.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/4 v2] mfd: add STw481x driver
2013-09-16 15:21 ` Wolfram Sang
@ 2013-09-16 15:34 ` Lee Jones
2013-09-16 16:00 ` Wolfram Sang
0 siblings, 1 reply; 12+ messages in thread
From: Lee Jones @ 2013-09-16 15:34 UTC (permalink / raw)
To: Wolfram Sang
Cc: Mark Brown, Linus Walleij, linux-i2c@vger.kernel.org,
Samuel Ortiz, linux-kernel@vger.kernel.org, Wang Shilong
On Mon, 16 Sep 2013, Wolfram Sang wrote:
>
> On Mon, Sep 16, 2013 at 02:51:18PM +0100, Mark Brown wrote:
> > On Mon, Sep 16, 2013 at 02:44:35PM +0200, Linus Walleij wrote:
> >
> > > I've tried to fix this for DT-only I2C devices
> > > and this very driver was the reason.
> >
> > > But a tiresome regression due to drivers relying on this
> > > i2c_device_id not being NULL and inability to remove it from the I2C
> > > core without refactoring the world ensued, see:
> > > commit c80f52847c50109ca248c22efbf71ff10553dca4
> >
> > Oh, that was the change...
> >
> > > Reverted in:
> > > commit 661f6c1cd926c6c973e03c6b5151d161f3a666ed
> >
> > > For this reason I think:
> > > http://marc.info/?l=linux-next&m=137148411231784&w=2
> >
> > > I have tentatively given up getting pure DT I2C drivers
> > > to probe, I don't think I have the whole picture, but
> > > Wolfram has serious doubts about this and say we have
> > > to be careful ....
> >
> > > Wolfram, do you have some ideas on how we should
> > > proceed or ar you happy with merging this as-is?
> >
> > I'd have expected that it should be possible to change things such that
> > the change in the core doesn't produce any change in behaviour for
> > existing drivers. Can we not change the patch so that i2c_match_id()
> > copes with getting a NULL id_table? Something like this:
>
> I hacked something like this after Linus posted his approach. However, I
> found out that run time instanciating ('new_device' file) needs an
> id_table. I wasn't to keen on disabling the feature for dt-only drivers.
> That's where I stopped, due to lack of time.
So in the mean time are you happy with this "dummy" approach?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/4 v2] mfd: add STw481x driver
2013-09-16 15:34 ` Lee Jones
@ 2013-09-16 16:00 ` Wolfram Sang
2013-09-16 16:05 ` Lee Jones
0 siblings, 1 reply; 12+ messages in thread
From: Wolfram Sang @ 2013-09-16 16:00 UTC (permalink / raw)
To: Lee Jones
Cc: Mark Brown, Linus Walleij, linux-i2c@vger.kernel.org,
Samuel Ortiz, linux-kernel@vger.kernel.org, Wang Shilong
[-- Attachment #1: Type: text/plain, Size: 250 bytes --]
> So in the mean time are you happy with this "dummy" approach?
No. "dummy" is reserved for a dummy device in case an i2c slave needs
more than one address. The proper solution would be if
i2c_sysfs_new_device() could recognize the of_device_ids.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4 v2] mfd: add STw481x driver
2013-09-16 16:00 ` Wolfram Sang
@ 2013-09-16 16:05 ` Lee Jones
2013-09-16 16:24 ` Mark Brown
0 siblings, 1 reply; 12+ messages in thread
From: Lee Jones @ 2013-09-16 16:05 UTC (permalink / raw)
To: Wolfram Sang
Cc: Mark Brown, Linus Walleij, linux-i2c@vger.kernel.org,
Samuel Ortiz, linux-kernel@vger.kernel.org, Wang Shilong
> > So in the mean time are you happy with this "dummy" approach?
>
> No. "dummy" is reserved for a dummy device in case an i2c slave needs
> more than one address. The proper solution would be if
> i2c_sysfs_new_device() could recognize the of_device_ids.
Okay, thanks for clarifying.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4 v2] mfd: add STw481x driver
2013-09-16 16:05 ` Lee Jones
@ 2013-09-16 16:24 ` Mark Brown
2013-09-17 8:06 ` Lee Jones
0 siblings, 1 reply; 12+ messages in thread
From: Mark Brown @ 2013-09-16 16:24 UTC (permalink / raw)
To: Lee Jones
Cc: Wolfram Sang, Linus Walleij, linux-i2c@vger.kernel.org,
Samuel Ortiz, linux-kernel@vger.kernel.org, Wang Shilong
[-- Attachment #1: Type: text/plain, Size: 458 bytes --]
On Mon, Sep 16, 2013 at 05:05:37PM +0100, Lee Jones wrote:
> > > So in the mean time are you happy with this "dummy" approach?
> > No. "dummy" is reserved for a dummy device in case an i2c slave needs
> > more than one address. The proper solution would be if
> > i2c_sysfs_new_device() could recognize the of_device_ids.
> Okay, thanks for clarifying.
Or for now to use something like stw481x (even if it's never expected to
actually make a difference).
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4 v2] mfd: add STw481x driver
2013-09-16 16:24 ` Mark Brown
@ 2013-09-17 8:06 ` Lee Jones
0 siblings, 0 replies; 12+ messages in thread
From: Lee Jones @ 2013-09-17 8:06 UTC (permalink / raw)
To: Mark Brown
Cc: Wolfram Sang, Linus Walleij, linux-i2c@vger.kernel.org,
Samuel Ortiz, linux-kernel@vger.kernel.org, Wang Shilong
On Mon, 16 Sep 2013, Mark Brown wrote:
> On Mon, Sep 16, 2013 at 05:05:37PM +0100, Lee Jones wrote:
> > > > So in the mean time are you happy with this "dummy" approach?
>
> > > No. "dummy" is reserved for a dummy device in case an i2c slave needs
> > > more than one address. The proper solution would be if
> > > i2c_sysfs_new_device() could recognize the of_device_ids.
>
> > Okay, thanks for clarifying.
>
> Or for now to use something like stw481x (even if it's never expected to
> actually make a difference).
If Linus wants to resubmit, I'm happy with that.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-09-17 8:06 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-13 17:54 [PATCH 1/4 v2] mfd: add STw481x driver Linus Walleij
2013-09-16 9:19 ` Lee Jones
2013-09-16 10:40 ` Mark Brown
2013-09-16 11:06 ` Lee Jones
2013-09-16 12:44 ` Linus Walleij
2013-09-16 13:51 ` Mark Brown
2013-09-16 15:21 ` Wolfram Sang
2013-09-16 15:34 ` Lee Jones
2013-09-16 16:00 ` Wolfram Sang
2013-09-16 16:05 ` Lee Jones
2013-09-16 16:24 ` Mark Brown
2013-09-17 8:06 ` Lee Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox