Hi,
Latest linux-next head (next-20130617) seems to have some backwards-incompatible
changes to the i2c core, which breaks the tps* drivers in our boards and cause
panics on boot.
v2 changes: Don't override platform data from DT in tps65910
ttynkkynen@ttynkkynen-lnx:~/upstream/kernel$ git bisect bad
c80f52847c50109ca248c22efbf71ff10553dca4 is the first bad commit
commit c80f52847c50109ca248c22efbf71ff10553dca4
Author: Linus Walleij <[email protected]>
Date: Mon May 13 22:18:21 2013 +0200
i2c: core: make it possible to match a pure device tree driver
This tries to address an issue found when writing an MFD driver
for the Nomadik STw481x PMICs: as the platform is using device
tree exclusively I want to specify the driver matching like
this:
static const struct of_device_id stw481x_match[] = {
{ .compatible = "st,stw4810", },
{ .compatible = "st,stw4811", },
{},
};
static struct i2c_driver stw481x_driver = {
.driver = {
.name = "stw481x",
.of_match_table = stw481x_match,
},
.probe = stw481x_probe,
.remove = stw481x_remove,
};
However that turns out not to be possible: the I2C probe code
is written so that the probe() call is always passed a match
from i2c_match_id() using non-devicetree matches.
This is probably why most devices using device tree for I2C
clients currently will pass no .of_match_table *at all* but
instead just use .id_table from struct i2c_driver to match
the device. As you realize that means that the whole idea with
compatible strings is discarded, and that is why we find strange
device tree I2C device compatible strings like "product" instead
of "vendor,product" as you could expect.
Let's figure out how to fix this before the mess spreads. This
patch will allow probeing devices with only an of_match_table
as per above, and will pass NULL as the second argument to the
probe() function. If the driver wants to deduce secondary info
from the struct of_device_id .data field, it has to call
of_match_device() on its own match table in the probe function
device tree probe path.
If drivers define both an .of_match_table *AND* a i2c_driver
.id_table, the .of_match_table will take precedence, just
as is done in the i2c_device_match() function in i2c-core.c.
I2C devices probed from device tree should subsequently be
fixed to handle the case where of_match_table() is
used (I think none of them do that today), and platforms should
fix their device trees to use compatible strings for I2C devices
instead of setting the name to Linux device driver names as is
done in multiple cases today.
Cc: Rob Herring <[email protected]>
Cc: Grant Likely <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
Signed-off-by: Wolfram Sang <[email protected]>
Tuomas Tynkkynen (2):
mfd: tps65910: Fix crash in i2c_driver .probe
regulator: tps62360: Fix crash in i2c_driver .probe
drivers/mfd/tps65910.c | 39 ++++++++++++++++++----------------
drivers/regulator/tps62360-regulator.c | 8 +++++--
2 files changed, 27 insertions(+), 20 deletions(-)
--
1.8.1.5
Commit "i2c: core: make it possible to match a pure device tree driver"
changed semantics of the i2c probing for device tree devices.
Device tree probed devices now get a NULL i2c_device_id pointer.
This caused kernel panics due to NULL dereference.
Moves the of_match_device call from tps65910_parse_dt to .probe to
allow the chip type to be detected from device tree but with the
device parameters coming from platform data.
Signed-off-by: Tuomas Tynkkynen <[email protected]>
---
v2: Don't overwrite platform data from DT
drivers/mfd/tps65910.c | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index d792772..61147b4 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -384,22 +384,13 @@ static struct of_device_id tps65910_of_match[] = {
MODULE_DEVICE_TABLE(of, tps65910_of_match);
static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
- int *chip_id)
+ int chip_id)
{
struct device_node *np = client->dev.of_node;
struct tps65910_board *board_info;
unsigned int prop;
- const struct of_device_id *match;
int ret = 0;
- match = of_match_device(tps65910_of_match, &client->dev);
- if (!match) {
- dev_err(&client->dev, "Failed to find matching dt id\n");
- return NULL;
- }
-
- *chip_id = (int)match->data;
-
board_info = devm_kzalloc(&client->dev, sizeof(*board_info),
GFP_KERNEL);
if (!board_info) {
@@ -410,13 +401,13 @@ static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
ret = of_property_read_u32(np, "ti,vmbch-threshold", &prop);
if (!ret)
board_info->vmbch_threshold = prop;
- else if (*chip_id == TPS65911)
+ else if (chip_id == TPS65911)
dev_warn(&client->dev, "VMBCH-Threshold not specified");
ret = of_property_read_u32(np, "ti,vmbch2-threshold", &prop);
if (!ret)
board_info->vmbch2_threshold = prop;
- else if (*chip_id == TPS65911)
+ else if (chip_id == TPS65911)
dev_warn(&client->dev, "VMBCH2-Threshold not specified");
prop = of_property_read_bool(np, "ti,en-ck32k-xtal");
@@ -432,7 +423,7 @@ static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
#else
static inline
struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
- int *chip_id)
+ int chip_id)
{
return NULL;
}
@@ -461,16 +452,28 @@ static int tps65910_i2c_probe(struct i2c_client *i2c,
struct tps65910_board *of_pmic_plat_data = NULL;
struct tps65910_platform_data *init_data;
int ret = 0;
- int chip_id = id->driver_data;
+ int chip_id = -1;
pmic_plat_data = dev_get_platdata(&i2c->dev);
- if (!pmic_plat_data && i2c->dev.of_node) {
- pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);
- of_pmic_plat_data = pmic_plat_data;
+ if (id) {
+ chip_id = id->driver_data;
+ } else if (i2c->dev.of_node) {
+ const struct of_device_id *match;
+ match = of_match_device(tps65910_of_match, &i2c->dev);
+ if (!match) {
+ dev_err(&i2c->dev, "Failed to find matching dt id\n");
+ return -EINVAL;
+ }
+ chip_id = (int)match->data;
+
+ if (!pmic_plat_data) {
+ pmic_plat_data = tps65910_parse_dt(i2c, chip_id);
+ of_pmic_plat_data = pmic_plat_data;
+ }
}
- if (!pmic_plat_data)
+ if (!pmic_plat_data || chip_id < 0)
return -EINVAL;
init_data = devm_kzalloc(&i2c->dev, sizeof(*init_data), GFP_KERNEL);
--
1.8.1.5
Commit "i2c: core: make it possible to match a pure device tree driver"
changed semantics of the i2c probing for device tree devices.
Device tree probed devices now get a NULL i2c_device_id pointer.
This caused kernel panics due to NULL dereference.
Tested-by: Stephen Warren <[email protected]>
Reviewed-by: Stephen Warren <[email protected]>
Signed-off-by: Tuomas Tynkkynen <[email protected]>
---
v2: Added Stephen's Tested-by & Reviewed-by
drivers/regulator/tps62360-regulator.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/tps62360-regulator.c b/drivers/regulator/tps62360-regulator.c
index 612919c..a490d5b 100644
--- a/drivers/regulator/tps62360-regulator.c
+++ b/drivers/regulator/tps62360-regulator.c
@@ -351,7 +351,6 @@ static int tps62360_probe(struct i2c_client *client,
int chip_id;
pdata = client->dev.platform_data;
- chip_id = id->driver_data;
if (client->dev.of_node) {
const struct of_device_id *match;
@@ -364,6 +363,11 @@ static int tps62360_probe(struct i2c_client *client,
chip_id = (int)match->data;
if (!pdata)
pdata = of_get_tps62360_platform_data(&client->dev);
+ } else if (id) {
+ chip_id = id->driver_data;
+ } else {
+ dev_err(&client->dev, "No device tree match or id table match found\n");
+ return -ENODEV;
}
if (!pdata) {
@@ -402,7 +406,7 @@ static int tps62360_probe(struct i2c_client *client,
return -ENODEV;
}
- tps->desc.name = id->name;
+ tps->desc.name = client->name;
tps->desc.id = 0;
tps->desc.ops = &tps62360_dcdc_ops;
tps->desc.type = REGULATOR_VOLTAGE;
--
1.8.1.5
On 06/18/2013 04:14 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.
>
> Moves the of_match_device call from tps65910_parse_dt to .probe to
> allow the chip type to be detected from device tree but with the
> device parameters coming from platform data.
Reviewed-by: Stephen Warren <[email protected]>
It's a pity that every driver that supports DT is going to need this
boiler-plate though. Perhaps the I2C core can acquire a follow-on patch
that does this for drivers in the future.
On Tue, Jun 18, 2013 at 01:14:39PM +0300, Tuomas Tynkkynen wrote:
> Hi,
>
> Latest linux-next head (next-20130617) seems to have some backwards-incompatible
> changes to the i2c core, which breaks the tps* drivers in our boards and cause
> panics on boot.
>
> v2 changes: Don't override platform data from DT in tps65910
Thanks for pointing out. I am going to revert the commit in hope for a
better solution.
So, this series is not needed.
On Tue, 18 Jun 2013, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.
>
> Moves the of_match_device call from tps65910_parse_dt to .probe to
> allow the chip type to be detected from device tree but with the
> device parameters coming from platform data.
>
> Signed-off-by: Tuomas Tynkkynen <[email protected]>
Applied with Stephen's Reviewed-by, thanks.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On Wed, Jun 19, 2013 at 09:18:59AM +0100, Lee Jones wrote:
> On Tue, 18 Jun 2013, Tuomas Tynkkynen wrote:
>
> > Commit "i2c: core: make it possible to match a pure device tree driver"
> > changed semantics of the i2c probing for device tree devices.
> > Device tree probed devices now get a NULL i2c_device_id pointer.
> > This caused kernel panics due to NULL dereference.
> >
> > Moves the of_match_device call from tps65910_parse_dt to .probe to
> > allow the chip type to be detected from device tree but with the
> > device parameters coming from platform data.
> >
> > Signed-off-by: Tuomas Tynkkynen <[email protected]>
>
> Applied with Stephen's Reviewed-by, thanks.
According to Wolfgang, this is not needed as he will revert the i2c
commits that are causing those crashes.
I would not take it for now.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
On Wed, 19 Jun 2013, Samuel Ortiz wrote:
> Hi Lee,
>
> On Wed, Jun 19, 2013 at 09:18:59AM +0100, Lee Jones wrote:
> > On Tue, 18 Jun 2013, Tuomas Tynkkynen wrote:
> >
> > > Commit "i2c: core: make it possible to match a pure device tree driver"
> > > changed semantics of the i2c probing for device tree devices.
> > > Device tree probed devices now get a NULL i2c_device_id pointer.
> > > This caused kernel panics due to NULL dereference.
> > >
> > > Moves the of_match_device call from tps65910_parse_dt to .probe to
> > > allow the chip type to be detected from device tree but with the
> > > device parameters coming from platform data.
> > >
> > > Signed-off-by: Tuomas Tynkkynen <[email protected]>
> >
> > Applied with Stephen's Reviewed-by, thanks.
> According to Wolfgang, this is not needed as he will revert the i2c
> commits that are causing those crashes.
> I would not take it for now.
Fair enough. Unapplied.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Jun 19, 2013 at 10:27:40AM +0200, Samuel Ortiz wrote:
> Hi Lee,
>
> On Wed, Jun 19, 2013 at 09:18:59AM +0100, Lee Jones wrote:
> > On Tue, 18 Jun 2013, Tuomas Tynkkynen wrote:
> >
> > > Commit "i2c: core: make it possible to match a pure device tree driver"
> > > changed semantics of the i2c probing for device tree devices.
> > > Device tree probed devices now get a NULL i2c_device_id pointer.
> > > This caused kernel panics due to NULL dereference.
> > >
> > > Moves the of_match_device call from tps65910_parse_dt to .probe to
> > > allow the chip type to be detected from device tree but with the
> > > device parameters coming from platform data.
> > >
> > > Signed-off-by: Tuomas Tynkkynen <[email protected]>
> >
> > Applied with Stephen's Reviewed-by, thanks.
> According to Wolfgang, this is not needed as he will revert the i2c
> commits that are causing those crashes.
> I would not take it for now.
"Wolfram", please ;) Otherwise correct.
Thanks.
Hi Wolfram,
On Wed, Jun 19, 2013 at 12:00:20PM +0200, Wolfram Sang wrote:
> > > Applied with Stephen's Reviewed-by, thanks.
> > According to Wolfgang, this is not needed as he will revert the i2c
> > commits that are causing those crashes.
> > I would not take it for now.
>
> "Wolfram", please ;) Otherwise correct.
Apologies, it seems I have issues with first names today...
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/