Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 6 Dec 2012 08:06:20 +0530, Viresh Kumar wrote: > On 6 December 2012 04:12, Grant Likely wrote: > > On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar > > wrote: > >> This first tries to match the table my patch added, _BUT_ the string will > >> never match as we had "st,stmpe810" in table and "stmpe810" in dev. > > > > of_driver_match_device() matches against the compatible list in > > dev->of_node, not against the device name. So, if the compatible > > property has a string that is in the table, then it really should match > > against it. > > Grant, but isn't it true that the final device's name would not be the DT > way of names? It would simply be "stmpe810" for us and so if we have > multiple instances of stmpe on a board, we need to distinguish them > ourselves? One way for that was passing id for these instances, which > would finally be used, when we create platform devices for sub-modules > of stmpe (gpio, keypad, ts, etc). of_modalias_node() is based on a *heruistic*. It is a best-effort attempt to convert the node's compatible lists into a string that will match against an existing driver. In the simple case it works because historically i2c has used the chip name for the driver name. We get lucky and a lot of drivers will work with DT without changes. However, it is in no way guaranteed. Sometimes the strings won't line up or a certain silicon vendor will have an extra errata or feature. In that case it makes sense to use a DT match table that can parse the entries in the compatible list. Or a driver can call of_ helper functions. For example, it might call of_alias_get_id() to figure out which device id it needs. That's why the full DT parsing exists. It's the fallback when the simple heuristic fails. Only use it when you need to. g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 6 Dec 2012 07:28:22 +0530, Viresh Kumar wrote:
> First of all, thanks for explaining :)
>
> On 6 December 2012 04:12, Grant Likely wrote:
> > On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar
> > wrote:
> >> This first tries to match the table my patch added, _BUT_ the string will
> >> never match as we had "st,stmpe810" in table and "stmpe810" in dev.
> >
> > of_driver_match_device() matches against the compatible list in
> > dev->of_node, not against the device name. So, if the compatible
> > property has a string that is in the table, then it really should match
> > against it.
>
> How could i misread it? Yes you are correct.
>
> >> static int i2c_device_probe(struct device *dev)
> >> {
> >> .
> >> status = driver->probe(client, i2c_match_id(driver->id_table,
> >> client));
> >
> > Here things are a bit wonky. Even if matched against the table, it is
>
> table means of_device_id table ?
yes
>
> > possible that it also matches against i2c_match_id() and that data is
> > passed to the driver.
>
> It is a possibility or guarantee ? And so whatever device name we got from
> modalias routine, should match with the names in driver->id_table.
possibility. If the generated alias name does match something in
driver->id_table, then that pointer will be returned here.
> > But regardless, it is the responsiblity of the probe function to go and
> > look if of_driver_match_device() matches against anything if it cares
> > about the of_match_table entries (for instance, if there is extra data
> > attached).
>
> Ok, so filling .data field in of_device_id[] is not required for our case as
> we aren't doing anything special in our drivers.
As long as things are simple, correct.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 6 Dec 2012 07:28:22 +0530, Viresh Kumar viresh.ku...@linaro.org wrote:
First of all, thanks for explaining :)
On 6 December 2012 04:12, Grant Likely grant.lik...@secretlab.ca wrote:
On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar viresh.ku...@linaro.org
wrote:
This first tries to match the table my patch added, _BUT_ the string will
never match as we had st,stmpe810 in table and stmpe810 in dev.
of_driver_match_device() matches against the compatible list in
dev-of_node, not against the device name. So, if the compatible
property has a string that is in the table, then it really should match
against it.
How could i misread it? Yes you are correct.
static int i2c_device_probe(struct device *dev)
{
.
status = driver-probe(client, i2c_match_id(driver-id_table,
client));
Here things are a bit wonky. Even if matched against the table, it is
table means of_device_id table ?
yes
possible that it also matches against i2c_match_id() and that data is
passed to the driver.
It is a possibility or guarantee ? And so whatever device name we got from
modalias routine, should match with the names in driver-id_table.
possibility. If the generated alias name does match something in
driver-id_table, then that pointer will be returned here.
But regardless, it is the responsiblity of the probe function to go and
look if of_driver_match_device() matches against anything if it cares
about the of_match_table entries (for instance, if there is extra data
attached).
Ok, so filling .data field in of_device_id[] is not required for our case as
we aren't doing anything special in our drivers.
As long as things are simple, correct.
g.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 6 Dec 2012 08:06:20 +0530, Viresh Kumar viresh.ku...@linaro.org wrote: On 6 December 2012 04:12, Grant Likely grant.lik...@secretlab.ca wrote: On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar viresh.ku...@linaro.org wrote: This first tries to match the table my patch added, _BUT_ the string will never match as we had st,stmpe810 in table and stmpe810 in dev. of_driver_match_device() matches against the compatible list in dev-of_node, not against the device name. So, if the compatible property has a string that is in the table, then it really should match against it. Grant, but isn't it true that the final device's name would not be the DT way of names? It would simply be stmpe810 for us and so if we have multiple instances of stmpe on a board, we need to distinguish them ourselves? One way for that was passing id for these instances, which would finally be used, when we create platform devices for sub-modules of stmpe (gpio, keypad, ts, etc). of_modalias_node() is based on a *heruistic*. It is a best-effort attempt to convert the node's compatible lists into a string that will match against an existing driver. In the simple case it works because historically i2c has used the chip name for the driver name. We get lucky and a lot of drivers will work with DT without changes. However, it is in no way guaranteed. Sometimes the strings won't line up or a certain silicon vendor will have an extra errata or feature. In that case it makes sense to use a DT match table that can parse the entries in the compatible list. Or a driver can call of_ helper functions. For example, it might call of_alias_get_id() to figure out which device id it needs. That's why the full DT parsing exists. It's the fallback when the simple heuristic fails. Only use it when you need to. g. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 06 Dec 2012, Viresh Kumar wrote: > On 6 December 2012 16:42, Lee Jones wrote: > > I thought we'd be over this? The 'ID' will be represented by the > > address of the chip i.e. stmpe1601@40, where '40' will be > > distinguishing factor? > > I haven't tested it but i thought we are getting i2c device name from > modalias() fn running on "st,stmpe1601" and so would give us "stmpe1601". > > Now, i went back to your earlier mail and checked device name: > stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212 > > It came from DT :) > > So, probably no more issues of id's. Will resend my patch, after getting > rid of tables. Now we're talking! -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 6 December 2012 16:42, Lee Jones wrote: > I thought we'd be over this? The 'ID' will be represented by the > address of the chip i.e. stmpe1601@40, where '40' will be > distinguishing factor? I haven't tested it but i thought we are getting i2c device name from modalias() fn running on "st,stmpe1601" and so would give us "stmpe1601". Now, i went back to your earlier mail and checked device name: stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212 It came from DT :) So, probably no more issues of id's. Will resend my patch, after getting rid of tables. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 06 Dec 2012, Viresh Kumar wrote: > On 6 December 2012 16:05, Lee Jones wrote: > >> > Or you could not put unnecessary bindings into the Device Tree > >> > by putting two and two together and realise that using the table > >> > is the correct thing to do instead. This actually gives reason > >> > to you previous patch, but should probably be fixed-up into it > >> > so it has some proper meaning/purpose. ;) > >> > >> Couldn't understand this one :( > > > > Really? > > I tried again, but couldn't get it :( > > > Let's break it down - what do you need "stmpe,id" for? > > To distinguish two instances of of stmpe811 (for instance) on a board. > Names of stmpe sub-modules would contain .0, .1, if we have an id > passed to it. Passing -1 would create same names for both gpio blocks > which belonged to different stmpe811's. I thought we'd be over this? The 'ID' will be represented by the address of the chip i.e. stmpe1601@40, where '40' will be distinguishing factor? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 6 December 2012 16:05, Lee Jones wrote: >> > Or you could not put unnecessary bindings into the Device Tree >> > by putting two and two together and realise that using the table >> > is the correct thing to do instead. This actually gives reason >> > to you previous patch, but should probably be fixed-up into it >> > so it has some proper meaning/purpose. ;) >> >> Couldn't understand this one :( > > Really? I tried again, but couldn't get it :( > Let's break it down - what do you need "stmpe,id" for? To distinguish two instances of of stmpe811 (for instance) on a board. Names of stmpe sub-modules would contain .0, .1, if we have an id passed to it. Passing -1 would create same names for both gpio blocks which belonged to different stmpe811's. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 06 Dec 2012, Viresh Kumar wrote: > On 6 December 2012 15:41, Lee Jones wrote: > > So then I'm back to my original question, why? > > > > What is it used for? What difference does it make? > > > > I could understand if the .data attribute was used in the driver > > to make vital decisions based on STMPE version, but it's not. So > > why are we burdening the driver with unused code that's not > > required? Other than to get your mainlined patch-count up of > > course? This has an air of "placing header files in alphabetical > > order" about it. ;) > > The count would still be the same as some part of this patch will > go :) > > I said that because of Grant's comment: "An of_match_table is the > robust way of identifying specific devices and allows for matching > against any entry in the compatible list." > > So, thought its better we keep it. Only if you're going to use it, or else it's just unused cruft. > Now, the problem is, with this new table we will bind device and > driver based on of_device_id table and probe it using device_id > table. Ahh.. that's broken. Maybe yes, can remove it unless we > have a real need of it. Broken or otherwise, if it's unused, it's cruft. :) > >> By chance > >> our non-DT and DT tables had a difference of "st," only in the name > >> of instances and so it worked without tables. Otherwise it couldn't > >> have worked. > >> > >> Over that, i am looking to bring the "stmpe,id" binding back again (unless > >> you have a better option), as device name is not coming from DT currently, > >> which we discussed earlier. > > > > Or you could not put unnecessary bindings into the Device Tree > > by putting two and two together and realise that using the table > > is the correct thing to do instead. This actually gives reason > > to you previous patch, but should probably be fixed-up into it > > so it has some proper meaning/purpose. ;) > > Couldn't understand this one :( Really? Let's break it down - what do you need "stmpe,id" for? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 6 December 2012 15:20, Lee Jones wrote: >> > But regardless, it is the responsiblity of the probe function to go and >> > look if of_driver_match_device() matches against anything if it cares >> > about the of_match_table entries (for instance, if there is extra data >> > attached). >> >> Ok, so filling .data field in of_device_id[] is not required for our case as >> we aren't doing anything special in our drivers. > > This is exactly my point, and the reason I bought it up in the > first place. Normally when you specify an ID table and populate > the .data attribute, you parse for it in the code and then cast > it back to some kind of useful data. However, you're not doing > that, which is precisely why I wondered if the table was > necessary at all. In all my testing, the DT portion worked and > the correct STMPE chip was identified without it. Probably Vipul (Author of this patch), copied it from existing i2c/spi clients, which have also added this blindly :) > So, are you adding the table for good reason, or because you > think it's the right thing to do? I would be keeping the table as that's the right thing to do. By chance our non-DT and DT tables had a difference of "st," only in the name of instances and so it worked without tables. Otherwise it couldn't have worked. Over that, i am looking to bring the "stmpe,id" binding back again (unless you have a better option), as device name is not coming from DT currently, which we discussed earlier. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 6 December 2012 15:41, Lee Jones wrote: > So then I'm back to my original question, why? > > What is it used for? What difference does it make? > > I could understand if the .data attribute was used in the driver > to make vital decisions based on STMPE version, but it's not. So > why are we burdening the driver with unused code that's not > required? Other than to get your mainlined patch-count up of > course? This has an air of "placing header files in alphabetical > order" about it. ;) The count would still be the same as some part of this patch will go :) I said that because of Grant's comment: "An of_match_table is the robust way of identifying specific devices and allows for matching against any entry in the compatible list." So, thought its better we keep it. Now, the problem is, with this new table we will bind device and driver based on of_device_id table and probe it using device_id table. Ahh.. that's broken. Maybe yes, can remove it unless we have a real need of it. >> By chance >> our non-DT and DT tables had a difference of "st," only in the name >> of instances and so it worked without tables. Otherwise it couldn't >> have worked. >> >> Over that, i am looking to bring the "stmpe,id" binding back again (unless >> you have a better option), as device name is not coming from DT currently, >> which we discussed earlier. > > Or you could not put unnecessary bindings into the Device Tree > by putting two and two together and realise that using the table > is the correct thing to do instead. This actually gives reason > to you previous patch, but should probably be fixed-up into it > so it has some proper meaning/purpose. ;) Couldn't understand this one :( -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 06 Dec 2012, Viresh Kumar wrote: > On 6 December 2012 15:20, Lee Jones wrote: > >> > But regardless, it is the responsiblity of the probe function to go and > >> > look if of_driver_match_device() matches against anything if it cares > >> > about the of_match_table entries (for instance, if there is extra data > >> > attached). > >> > >> Ok, so filling .data field in of_device_id[] is not required for our case > >> as > >> we aren't doing anything special in our drivers. > > > > This is exactly my point, and the reason I bought it up in the > > first place. Normally when you specify an ID table and populate > > the .data attribute, you parse for it in the code and then cast > > it back to some kind of useful data. However, you're not doing > > that, which is precisely why I wondered if the table was > > necessary at all. In all my testing, the DT portion worked and > > the correct STMPE chip was identified without it. > > Probably Vipul (Author of this patch), copied it from existing i2c/spi > clients, which have also added this blindly :) > > > So, are you adding the table for good reason, or because you > > think it's the right thing to do? > > I would be keeping the table as that's the right thing to do. So then I'm back to my original question, why? What is it used for? What difference does it make? I could understand if the .data attribute was used in the driver to make vital decisions based on STMPE version, but it's not. So why are we burdening the driver with unused code that's not required? Other than to get your mainlined patch-count up of course? This has an air of "placing header files in alphabetical order" about it. ;) > By chance > our non-DT and DT tables had a difference of "st," only in the name > of instances and so it worked without tables. Otherwise it couldn't > have worked. > > Over that, i am looking to bring the "stmpe,id" binding back again (unless > you have a better option), as device name is not coming from DT currently, > which we discussed earlier. Or you could not put unnecessary bindings into the Device Tree by putting two and two together and realise that using the table is the correct thing to do instead. This actually gives reason to you previous patch, but should probably be fixed-up into it so it has some proper meaning/purpose. ;) -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
> > But regardless, it is the responsiblity of the probe function to go and > > look if of_driver_match_device() matches against anything if it cares > > about the of_match_table entries (for instance, if there is extra data > > attached). > > Ok, so filling .data field in of_device_id[] is not required for our case as > we aren't doing anything special in our drivers. This is exactly my point, and the reason I bought it up in the first place. Normally when you specify an ID table and populate the .data attribute, you parse for it in the code and then cast it back to some kind of useful data. However, you're not doing that, which is precisely why I wondered if the table was necessary at all. In all my testing, the DT portion worked and the correct STMPE chip was identified without it. So, are you adding the table for good reason, or because you think it's the right thing to do? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
But regardless, it is the responsiblity of the probe function to go and look if of_driver_match_device() matches against anything if it cares about the of_match_table entries (for instance, if there is extra data attached). Ok, so filling .data field in of_device_id[] is not required for our case as we aren't doing anything special in our drivers. This is exactly my point, and the reason I bought it up in the first place. Normally when you specify an ID table and populate the .data attribute, you parse for it in the code and then cast it back to some kind of useful data. However, you're not doing that, which is precisely why I wondered if the table was necessary at all. In all my testing, the DT portion worked and the correct STMPE chip was identified without it. So, are you adding the table for good reason, or because you think it's the right thing to do? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 06 Dec 2012, Viresh Kumar wrote: On 6 December 2012 15:20, Lee Jones lee.jo...@linaro.org wrote: But regardless, it is the responsiblity of the probe function to go and look if of_driver_match_device() matches against anything if it cares about the of_match_table entries (for instance, if there is extra data attached). Ok, so filling .data field in of_device_id[] is not required for our case as we aren't doing anything special in our drivers. This is exactly my point, and the reason I bought it up in the first place. Normally when you specify an ID table and populate the .data attribute, you parse for it in the code and then cast it back to some kind of useful data. However, you're not doing that, which is precisely why I wondered if the table was necessary at all. In all my testing, the DT portion worked and the correct STMPE chip was identified without it. Probably Vipul (Author of this patch), copied it from existing i2c/spi clients, which have also added this blindly :) So, are you adding the table for good reason, or because you think it's the right thing to do? I would be keeping the table as that's the right thing to do. So then I'm back to my original question, why? What is it used for? What difference does it make? I could understand if the .data attribute was used in the driver to make vital decisions based on STMPE version, but it's not. So why are we burdening the driver with unused code that's not required? Other than to get your mainlined patch-count up of course? This has an air of placing header files in alphabetical order about it. ;) By chance our non-DT and DT tables had a difference of st, only in the name of instances and so it worked without tables. Otherwise it couldn't have worked. Over that, i am looking to bring the stmpe,id binding back again (unless you have a better option), as device name is not coming from DT currently, which we discussed earlier. Or you could not put unnecessary bindings into the Device Tree by putting two and two together and realise that using the table is the correct thing to do instead. This actually gives reason to you previous patch, but should probably be fixed-up into it so it has some proper meaning/purpose. ;) -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 6 December 2012 15:41, Lee Jones lee.jo...@linaro.org wrote: So then I'm back to my original question, why? What is it used for? What difference does it make? I could understand if the .data attribute was used in the driver to make vital decisions based on STMPE version, but it's not. So why are we burdening the driver with unused code that's not required? Other than to get your mainlined patch-count up of course? This has an air of placing header files in alphabetical order about it. ;) The count would still be the same as some part of this patch will go :) I said that because of Grant's comment: An of_match_table is the robust way of identifying specific devices and allows for matching against any entry in the compatible list. So, thought its better we keep it. Now, the problem is, with this new table we will bind device and driver based on of_device_id table and probe it using device_id table. Ahh.. that's broken. Maybe yes, can remove it unless we have a real need of it. By chance our non-DT and DT tables had a difference of st, only in the name of instances and so it worked without tables. Otherwise it couldn't have worked. Over that, i am looking to bring the stmpe,id binding back again (unless you have a better option), as device name is not coming from DT currently, which we discussed earlier. Or you could not put unnecessary bindings into the Device Tree by putting two and two together and realise that using the table is the correct thing to do instead. This actually gives reason to you previous patch, but should probably be fixed-up into it so it has some proper meaning/purpose. ;) Couldn't understand this one :( -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 6 December 2012 15:20, Lee Jones lee.jo...@linaro.org wrote: But regardless, it is the responsiblity of the probe function to go and look if of_driver_match_device() matches against anything if it cares about the of_match_table entries (for instance, if there is extra data attached). Ok, so filling .data field in of_device_id[] is not required for our case as we aren't doing anything special in our drivers. This is exactly my point, and the reason I bought it up in the first place. Normally when you specify an ID table and populate the .data attribute, you parse for it in the code and then cast it back to some kind of useful data. However, you're not doing that, which is precisely why I wondered if the table was necessary at all. In all my testing, the DT portion worked and the correct STMPE chip was identified without it. Probably Vipul (Author of this patch), copied it from existing i2c/spi clients, which have also added this blindly :) So, are you adding the table for good reason, or because you think it's the right thing to do? I would be keeping the table as that's the right thing to do. By chance our non-DT and DT tables had a difference of st, only in the name of instances and so it worked without tables. Otherwise it couldn't have worked. Over that, i am looking to bring the stmpe,id binding back again (unless you have a better option), as device name is not coming from DT currently, which we discussed earlier. -- viresh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 06 Dec 2012, Viresh Kumar wrote: On 6 December 2012 15:41, Lee Jones lee.jo...@linaro.org wrote: So then I'm back to my original question, why? What is it used for? What difference does it make? I could understand if the .data attribute was used in the driver to make vital decisions based on STMPE version, but it's not. So why are we burdening the driver with unused code that's not required? Other than to get your mainlined patch-count up of course? This has an air of placing header files in alphabetical order about it. ;) The count would still be the same as some part of this patch will go :) I said that because of Grant's comment: An of_match_table is the robust way of identifying specific devices and allows for matching against any entry in the compatible list. So, thought its better we keep it. Only if you're going to use it, or else it's just unused cruft. Now, the problem is, with this new table we will bind device and driver based on of_device_id table and probe it using device_id table. Ahh.. that's broken. Maybe yes, can remove it unless we have a real need of it. Broken or otherwise, if it's unused, it's cruft. :) By chance our non-DT and DT tables had a difference of st, only in the name of instances and so it worked without tables. Otherwise it couldn't have worked. Over that, i am looking to bring the stmpe,id binding back again (unless you have a better option), as device name is not coming from DT currently, which we discussed earlier. Or you could not put unnecessary bindings into the Device Tree by putting two and two together and realise that using the table is the correct thing to do instead. This actually gives reason to you previous patch, but should probably be fixed-up into it so it has some proper meaning/purpose. ;) Couldn't understand this one :( Really? Let's break it down - what do you need stmpe,id for? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 6 December 2012 16:05, Lee Jones lee.jo...@linaro.org wrote: Or you could not put unnecessary bindings into the Device Tree by putting two and two together and realise that using the table is the correct thing to do instead. This actually gives reason to you previous patch, but should probably be fixed-up into it so it has some proper meaning/purpose. ;) Couldn't understand this one :( Really? I tried again, but couldn't get it :( Let's break it down - what do you need stmpe,id for? To distinguish two instances of of stmpe811 (for instance) on a board. Names of stmpe sub-modules would contain .0, .1, if we have an id passed to it. Passing -1 would create same names for both gpio blocks which belonged to different stmpe811's. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 06 Dec 2012, Viresh Kumar wrote: On 6 December 2012 16:05, Lee Jones lee.jo...@linaro.org wrote: Or you could not put unnecessary bindings into the Device Tree by putting two and two together and realise that using the table is the correct thing to do instead. This actually gives reason to you previous patch, but should probably be fixed-up into it so it has some proper meaning/purpose. ;) Couldn't understand this one :( Really? I tried again, but couldn't get it :( Let's break it down - what do you need stmpe,id for? To distinguish two instances of of stmpe811 (for instance) on a board. Names of stmpe sub-modules would contain .0, .1, if we have an id passed to it. Passing -1 would create same names for both gpio blocks which belonged to different stmpe811's. I thought we'd be over this? The 'ID' will be represented by the address of the chip i.e. stmpe1601@40, where '40' will be distinguishing factor? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 6 December 2012 16:42, Lee Jones lee.jo...@linaro.org wrote: I thought we'd be over this? The 'ID' will be represented by the address of the chip i.e. stmpe1601@40, where '40' will be distinguishing factor? I haven't tested it but i thought we are getting i2c device name from modalias() fn running on st,stmpe1601 and so would give us stmpe1601. Now, i went back to your earlier mail and checked device name: stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212 It came from DT :) So, probably no more issues of id's. Will resend my patch, after getting rid of tables. -- viresh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Thu, 06 Dec 2012, Viresh Kumar wrote: On 6 December 2012 16:42, Lee Jones lee.jo...@linaro.org wrote: I thought we'd be over this? The 'ID' will be represented by the address of the chip i.e. stmpe1601@40, where '40' will be distinguishing factor? I haven't tested it but i thought we are getting i2c device name from modalias() fn running on st,stmpe1601 and so would give us stmpe1601. Now, i went back to your earlier mail and checked device name: stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212 It came from DT :) So, probably no more issues of id's. Will resend my patch, after getting rid of tables. Now we're talking! -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 6 December 2012 04:12, Grant Likely wrote: > On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar > wrote: >> This first tries to match the table my patch added, _BUT_ the string will >> never match as we had "st,stmpe810" in table and "stmpe810" in dev. > > of_driver_match_device() matches against the compatible list in > dev->of_node, not against the device name. So, if the compatible > property has a string that is in the table, then it really should match > against it. Grant, but isn't it true that the final device's name would not be the DT way of names? It would simply be "stmpe810" for us and so if we have multiple instances of stmpe on a board, we need to distinguish them ourselves? One way for that was passing id for these instances, which would finally be used, when we create platform devices for sub-modules of stmpe (gpio, keypad, ts, etc). -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
First of all, thanks for explaining :)
On 6 December 2012 04:12, Grant Likely wrote:
> On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar
> wrote:
>> This first tries to match the table my patch added, _BUT_ the string will
>> never match as we had "st,stmpe810" in table and "stmpe810" in dev.
>
> of_driver_match_device() matches against the compatible list in
> dev->of_node, not against the device name. So, if the compatible
> property has a string that is in the table, then it really should match
> against it.
How could i misread it? Yes you are correct.
>> static int i2c_device_probe(struct device *dev)
>> {
>> .
>> status = driver->probe(client, i2c_match_id(driver->id_table, client));
>
> Here things are a bit wonky. Even if matched against the table, it is
table means of_device_id table ?
> possible that it also matches against i2c_match_id() and that data is
> passed to the driver.
It is a possibility or guarantee ? And so whatever device name we got from
modalias routine, should match with the names in driver->id_table.
> But regardless, it is the responsiblity of the probe function to go and
> look if of_driver_match_device() matches against anything if it cares
> about the of_match_table entries (for instance, if there is extra data
> attached).
Ok, so filling .data field in of_device_id[] is not required for our case as
we aren't doing anything special in our drivers.
>> Which will again match the legacy table to find correct struct i2c_device_id
>> *id
>> to pass to probe().
>>
>> So, the final question: WTF is of_match_table for?
>
> A bit of history is valuable here. The whole of_modalias_node() thing is
> really just a best-effort heuristic for figuring out which driver
> *might* work against a device described in the device tree. It won't
> work in all circumstances (and it was created at a time when there was
> resistance to adding DT knowledge to drivers). An of_match_table is the
> robust way of identifying specific devices and allows for matching
> against any entry in the compatible list.
Got it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar wrote:
> On 30 November 2012 21:15, Lee Jones wrote:
> > But ... I don't see how the changes in the -i2c and -spi files
> > are of benefit either. When I boot without the ID table I still
> > get "stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212".
> >
> > What is it that actually uses the IDs?
> >
> > Perhaps Viresh can shine some light on the matter?
>
> As you can see, i wasn't the author of this patch and when you asked
> this question, i didn't had an answer to it. I went through code and
> formed some theory/story :) .
>
> @Grant: i need your help to check if my theory is correct or not. Question
> is about adding below code in any i2c client driver:
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id stmpe_dt_ids[] = {
> + { .compatible = "st,stmpe610", .data = _i2c_id[0], },
> + { .compatible = "st,stmpe801", .data = _i2c_id[1], },
> + { .compatible = "st,stmpe811", .data = _i2c_id[2], },
> + { .compatible = "st,stmpe1601", .data = _i2c_id[3], },
> + { .compatible = "st,stmpe2401", .data = _i2c_id[4], },
> + { .compatible = "st,stmpe2403", .data = _i2c_id[5], },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
> +#endif
> +
> static struct i2c_driver stmpe_i2c_driver = {
> .driver = {
> .name = "stmpe-i2c",
> @@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = {
> #ifdef CONFIG_PM
> .pm = _dev_pm_ops,
> #endif
> + .of_match_table = of_match_ptr(stmpe_dt_ids),
>
> So, what is the use of this table when we already have i2c_driver.id_table
> populated.
>
> This is my theory:
> -
> Adapter drivers supporting DT will call:
> of_i2c_register_devices()
> {
> for_each_child_of_node(adap->dev.of_node, node) {
> if (of_modalias_node(node, info.type, sizeof(info.type)) < 0)
> error condition
>
> ...
> result = i2c_new_device(adap, );
>
> ...
> }
>
> of_modalias_node(): expects compatible in child node, i.e. stmpe node in our
> case. If it is not there, then that node is skipped. then it copies
> string after ','
> to info.type. So, for us only "stmpe810" out of "st,stmpe810" is copied.
>
> Now this name, i.e. "stmpe810" is copied as client.name in i2c_new_device()
> and device_register() is called, which will eventually call:
>
> i2c_device_match()
> {
> /* Attempt an OF style match */
> if (of_driver_match_device(dev, 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;
> }
>
> This first tries to match the table my patch added, _BUT_ the string will
> never match as we had "st,stmpe810" in table and "stmpe810" in dev.
of_driver_match_device() matches against the compatible list in
dev->of_node, not against the device name. So, if the compatible
property has a string that is in the table, then it really should match
against it.
>
> So, we fall back to i2c_match_id(), which will match it against
> i2c_driver.id_table present in driver, which has entry for "stmpe810" and
> so strings matched.
>
> @Lee: This is what happened in your case. :)
>
> So, whether its DT or non DT, true is returned from here if something
> matched.
>
> Later on, this will be called:
>
> static int i2c_device_probe(struct device *dev)
> {
> .
> status = driver->probe(client, i2c_match_id(driver->id_table, client));
Here things are a bit wonky. Even if matched against the table, it is
possible that it also matches against i2c_match_id() and that data is
passed to the driver.
But regardless, it is the responsiblity of the probe function to go and
look if of_driver_match_device() matches against anything if it cares
about the of_match_table entries (for instance, if there is extra data
attached).
>
> }
>
> Which will again match the legacy table to find correct struct i2c_device_id
> *id
> to pass to probe().
>
> So, the final question: WTF is of_match_table for?
A bit of history is valuable here. The whole of_modalias_node() thing is
really just a best-effort heuristic for figuring out which driver
*might* work against a device described in the device tree. It won't
work in all circumstances (and it was created at a time when there was
resistance to adding DT knowledge to drivers). An of_match_table is the
robust way of identifying specific devices and allows for matching
against any entry in the compatible list.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 5 December 2012 18:49, Lee Jones wrote: >> Ping!!! > > Documentation/development-process/2.Process: > > - Avoid top-posting (the practice of putting your answer above the quoted > text you are responding to). It makes your response harder to read and > makes a poor impression. Yes, i know that. Did that in hurry, as it was just an ping and wasn't answering any question. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
> Ping!!!
Documentation/development-process/2.Process:
- Avoid top-posting (the practice of putting your answer above the quoted
text you are responding to). It makes your response harder to read and
makes a poor impression.
:)
> On 1 December 2012 00:33, Viresh Kumar wrote:
> > On 30 November 2012 21:15, Lee Jones wrote:
> >> But ... I don't see how the changes in the -i2c and -spi files
> >> are of benefit either. When I boot without the ID table I still
> >> get "stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212".
> >>
> >> What is it that actually uses the IDs?
> >>
> >> Perhaps Viresh can shine some light on the matter?
> >
> > As you can see, i wasn't the author of this patch and when you asked
> > this question, i didn't had an answer to it. I went through code and
> > formed some theory/story :) .
> >
> > @Grant: i need your help to check if my theory is correct or not. Question
> > is about adding below code in any i2c client driver:
> >
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id stmpe_dt_ids[] = {
> > + { .compatible = "st,stmpe610", .data = _i2c_id[0], },
> > + { .compatible = "st,stmpe801", .data = _i2c_id[1], },
> > + { .compatible = "st,stmpe811", .data = _i2c_id[2], },
> > + { .compatible = "st,stmpe1601", .data = _i2c_id[3], },
> > + { .compatible = "st,stmpe2401", .data = _i2c_id[4], },
> > + { .compatible = "st,stmpe2403", .data = _i2c_id[5], },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
> > +#endif
> > +
> > static struct i2c_driver stmpe_i2c_driver = {
> > .driver = {
> > .name = "stmpe-i2c",
> > @@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = {
> > #ifdef CONFIG_PM
> > .pm = _dev_pm_ops,
> > #endif
> > + .of_match_table = of_match_ptr(stmpe_dt_ids),
> >
> > So, what is the use of this table when we already have i2c_driver.id_table
> > populated.
> >
> > This is my theory:
> > -
> > Adapter drivers supporting DT will call:
> > of_i2c_register_devices()
> > {
> > for_each_child_of_node(adap->dev.of_node, node) {
> > if (of_modalias_node(node, info.type, sizeof(info.type)) <
> > 0)
> > error condition
> >
> > ...
> > result = i2c_new_device(adap, );
> >
> > ...
> > }
> >
> > of_modalias_node(): expects compatible in child node, i.e. stmpe node in our
> > case. If it is not there, then that node is skipped. then it copies
> > string after ','
> > to info.type. So, for us only "stmpe810" out of "st,stmpe810" is copied.
> >
> > Now this name, i.e. "stmpe810" is copied as client.name in i2c_new_device()
> > and device_register() is called, which will eventually call:
> >
> > i2c_device_match()
> > {
> > /* Attempt an OF style match */
> > if (of_driver_match_device(dev, 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;
> > }
> >
> > This first tries to match the table my patch added, _BUT_ the string will
> > never match as we had "st,stmpe810" in table and "stmpe810" in dev.
> >
> > So, we fall back to i2c_match_id(), which will match it against
> > i2c_driver.id_table present in driver, which has entry for "stmpe810" and
> > so strings matched.
> >
> > @Lee: This is what happened in your case. :)
> >
> > So, whether its DT or non DT, true is returned from here if something
> > matched.
> >
> > Later on, this will be called:
> >
> > static int i2c_device_probe(struct device *dev)
> > {
> > .
> > status = driver->probe(client, i2c_match_id(driver->id_table,
> > client));
> >
> > }
> >
> > Which will again match the legacy table to find correct struct
> > i2c_device_id *id
> > to pass to probe().
> >
> > So, the final question: WTF is of_match_table for?
> >
> > Then i thought maybe it is used when we don't have child nodes inside
> > parent,
> > something related to the phandle way ? I grep'd i2c in arch/arm/boot/dts and
> > couldn't find anything of that sort, the way i2c clients are added is:
> >
> > in dtsi file:
> >
> > i2c0: i2c@address {
> > ...
> > }
> >
> > in dts file:
> > {
> > stmpe810 {
> >
> > }
> > }
> >
> > which i believe will be taken care by dtc and will fold client nodes
> > as child nodes
> > of i2c0.
> >
> > @Grant: Please throw some light here :)
> >
> > --
> > viresh
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
Ping!!!
On 1 December 2012 00:33, Viresh Kumar wrote:
> On 30 November 2012 21:15, Lee Jones wrote:
>> But ... I don't see how the changes in the -i2c and -spi files
>> are of benefit either. When I boot without the ID table I still
>> get "stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212".
>>
>> What is it that actually uses the IDs?
>>
>> Perhaps Viresh can shine some light on the matter?
>
> As you can see, i wasn't the author of this patch and when you asked
> this question, i didn't had an answer to it. I went through code and
> formed some theory/story :) .
>
> @Grant: i need your help to check if my theory is correct or not. Question
> is about adding below code in any i2c client driver:
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id stmpe_dt_ids[] = {
> + { .compatible = "st,stmpe610", .data = _i2c_id[0], },
> + { .compatible = "st,stmpe801", .data = _i2c_id[1], },
> + { .compatible = "st,stmpe811", .data = _i2c_id[2], },
> + { .compatible = "st,stmpe1601", .data = _i2c_id[3], },
> + { .compatible = "st,stmpe2401", .data = _i2c_id[4], },
> + { .compatible = "st,stmpe2403", .data = _i2c_id[5], },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
> +#endif
> +
> static struct i2c_driver stmpe_i2c_driver = {
> .driver = {
> .name = "stmpe-i2c",
> @@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = {
> #ifdef CONFIG_PM
> .pm = _dev_pm_ops,
> #endif
> + .of_match_table = of_match_ptr(stmpe_dt_ids),
>
> So, what is the use of this table when we already have i2c_driver.id_table
> populated.
>
> This is my theory:
> -
> Adapter drivers supporting DT will call:
> of_i2c_register_devices()
> {
> for_each_child_of_node(adap->dev.of_node, node) {
> if (of_modalias_node(node, info.type, sizeof(info.type)) < 0)
> error condition
>
> ...
> result = i2c_new_device(adap, );
>
> ...
> }
>
> of_modalias_node(): expects compatible in child node, i.e. stmpe node in our
> case. If it is not there, then that node is skipped. then it copies
> string after ','
> to info.type. So, for us only "stmpe810" out of "st,stmpe810" is copied.
>
> Now this name, i.e. "stmpe810" is copied as client.name in i2c_new_device()
> and device_register() is called, which will eventually call:
>
> i2c_device_match()
> {
> /* Attempt an OF style match */
> if (of_driver_match_device(dev, 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;
> }
>
> This first tries to match the table my patch added, _BUT_ the string will
> never match as we had "st,stmpe810" in table and "stmpe810" in dev.
>
> So, we fall back to i2c_match_id(), which will match it against
> i2c_driver.id_table present in driver, which has entry for "stmpe810" and
> so strings matched.
>
> @Lee: This is what happened in your case. :)
>
> So, whether its DT or non DT, true is returned from here if something
> matched.
>
> Later on, this will be called:
>
> static int i2c_device_probe(struct device *dev)
> {
> .
> status = driver->probe(client, i2c_match_id(driver->id_table,
> client));
>
> }
>
> Which will again match the legacy table to find correct struct i2c_device_id
> *id
> to pass to probe().
>
> So, the final question: WTF is of_match_table for?
>
> Then i thought maybe it is used when we don't have child nodes inside parent,
> something related to the phandle way ? I grep'd i2c in arch/arm/boot/dts and
> couldn't find anything of that sort, the way i2c clients are added is:
>
> in dtsi file:
>
> i2c0: i2c@address {
> ...
> }
>
> in dts file:
> {
> stmpe810 {
>
> }
> }
>
> which i believe will be taken care by dtc and will fold client nodes
> as child nodes
> of i2c0.
>
> @Grant: Please throw some light here :)
>
> --
> viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
Ping!!!
On 1 December 2012 00:33, Viresh Kumar viresh.ku...@linaro.org wrote:
On 30 November 2012 21:15, Lee Jones lee.jo...@linaro.org wrote:
But ... I don't see how the changes in the -i2c and -spi files
are of benefit either. When I boot without the ID table I still
get stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212.
What is it that actually uses the IDs?
Perhaps Viresh can shine some light on the matter?
As you can see, i wasn't the author of this patch and when you asked
this question, i didn't had an answer to it. I went through code and
formed some theory/story :) .
@Grant: i need your help to check if my theory is correct or not. Question
is about adding below code in any i2c client driver:
+#ifdef CONFIG_OF
+static const struct of_device_id stmpe_dt_ids[] = {
+ { .compatible = st,stmpe610, .data = stmpe_i2c_id[0], },
+ { .compatible = st,stmpe801, .data = stmpe_i2c_id[1], },
+ { .compatible = st,stmpe811, .data = stmpe_i2c_id[2], },
+ { .compatible = st,stmpe1601, .data = stmpe_i2c_id[3], },
+ { .compatible = st,stmpe2401, .data = stmpe_i2c_id[4], },
+ { .compatible = st,stmpe2403, .data = stmpe_i2c_id[5], },
+ { }
+};
+MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
+#endif
+
static struct i2c_driver stmpe_i2c_driver = {
.driver = {
.name = stmpe-i2c,
@@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = {
#ifdef CONFIG_PM
.pm = stmpe_dev_pm_ops,
#endif
+ .of_match_table = of_match_ptr(stmpe_dt_ids),
So, what is the use of this table when we already have i2c_driver.id_table
populated.
This is my theory:
-
Adapter drivers supporting DT will call:
of_i2c_register_devices()
{
for_each_child_of_node(adap-dev.of_node, node) {
if (of_modalias_node(node, info.type, sizeof(info.type)) 0)
error condition
...
result = i2c_new_device(adap, info);
...
}
of_modalias_node(): expects compatible in child node, i.e. stmpe node in our
case. If it is not there, then that node is skipped. then it copies
string after ','
to info.type. So, for us only stmpe810 out of st,stmpe810 is copied.
Now this name, i.e. stmpe810 is copied as client.name in i2c_new_device()
and device_register() is called, which will eventually call:
i2c_device_match()
{
/* Attempt an OF style match */
if (of_driver_match_device(dev, 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;
}
This first tries to match the table my patch added, _BUT_ the string will
never match as we had st,stmpe810 in table and stmpe810 in dev.
So, we fall back to i2c_match_id(), which will match it against
i2c_driver.id_table present in driver, which has entry for stmpe810 and
so strings matched.
@Lee: This is what happened in your case. :)
So, whether its DT or non DT, true is returned from here if something
matched.
Later on, this will be called:
static int i2c_device_probe(struct device *dev)
{
.
status = driver-probe(client, i2c_match_id(driver-id_table,
client));
}
Which will again match the legacy table to find correct struct i2c_device_id
*id
to pass to probe().
So, the final question: WTF is of_match_table for?
Then i thought maybe it is used when we don't have child nodes inside parent,
something related to the phandle way ? I grep'd i2c in arch/arm/boot/dts and
couldn't find anything of that sort, the way i2c clients are added is:
in dtsi file:
i2c0: i2c@address {
...
}
in dts file:
i2c0 {
stmpe810 {
}
}
which i believe will be taken care by dtc and will fold client nodes
as child nodes
of i2c0.
@Grant: Please throw some light here :)
--
viresh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
Ping!!!
Documentation/development-process/2.Process:
- Avoid top-posting (the practice of putting your answer above the quoted
text you are responding to). It makes your response harder to read and
makes a poor impression.
:)
On 1 December 2012 00:33, Viresh Kumar viresh.ku...@linaro.org wrote:
On 30 November 2012 21:15, Lee Jones lee.jo...@linaro.org wrote:
But ... I don't see how the changes in the -i2c and -spi files
are of benefit either. When I boot without the ID table I still
get stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212.
What is it that actually uses the IDs?
Perhaps Viresh can shine some light on the matter?
As you can see, i wasn't the author of this patch and when you asked
this question, i didn't had an answer to it. I went through code and
formed some theory/story :) .
@Grant: i need your help to check if my theory is correct or not. Question
is about adding below code in any i2c client driver:
+#ifdef CONFIG_OF
+static const struct of_device_id stmpe_dt_ids[] = {
+ { .compatible = st,stmpe610, .data = stmpe_i2c_id[0], },
+ { .compatible = st,stmpe801, .data = stmpe_i2c_id[1], },
+ { .compatible = st,stmpe811, .data = stmpe_i2c_id[2], },
+ { .compatible = st,stmpe1601, .data = stmpe_i2c_id[3], },
+ { .compatible = st,stmpe2401, .data = stmpe_i2c_id[4], },
+ { .compatible = st,stmpe2403, .data = stmpe_i2c_id[5], },
+ { }
+};
+MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
+#endif
+
static struct i2c_driver stmpe_i2c_driver = {
.driver = {
.name = stmpe-i2c,
@@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = {
#ifdef CONFIG_PM
.pm = stmpe_dev_pm_ops,
#endif
+ .of_match_table = of_match_ptr(stmpe_dt_ids),
So, what is the use of this table when we already have i2c_driver.id_table
populated.
This is my theory:
-
Adapter drivers supporting DT will call:
of_i2c_register_devices()
{
for_each_child_of_node(adap-dev.of_node, node) {
if (of_modalias_node(node, info.type, sizeof(info.type))
0)
error condition
...
result = i2c_new_device(adap, info);
...
}
of_modalias_node(): expects compatible in child node, i.e. stmpe node in our
case. If it is not there, then that node is skipped. then it copies
string after ','
to info.type. So, for us only stmpe810 out of st,stmpe810 is copied.
Now this name, i.e. stmpe810 is copied as client.name in i2c_new_device()
and device_register() is called, which will eventually call:
i2c_device_match()
{
/* Attempt an OF style match */
if (of_driver_match_device(dev, 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;
}
This first tries to match the table my patch added, _BUT_ the string will
never match as we had st,stmpe810 in table and stmpe810 in dev.
So, we fall back to i2c_match_id(), which will match it against
i2c_driver.id_table present in driver, which has entry for stmpe810 and
so strings matched.
@Lee: This is what happened in your case. :)
So, whether its DT or non DT, true is returned from here if something
matched.
Later on, this will be called:
static int i2c_device_probe(struct device *dev)
{
.
status = driver-probe(client, i2c_match_id(driver-id_table,
client));
}
Which will again match the legacy table to find correct struct
i2c_device_id *id
to pass to probe().
So, the final question: WTF is of_match_table for?
Then i thought maybe it is used when we don't have child nodes inside
parent,
something related to the phandle way ? I grep'd i2c in arch/arm/boot/dts and
couldn't find anything of that sort, the way i2c clients are added is:
in dtsi file:
i2c0: i2c@address {
...
}
in dts file:
i2c0 {
stmpe810 {
}
}
which i believe will be taken care by dtc and will fold client nodes
as child nodes
of i2c0.
@Grant: Please throw some light here :)
--
viresh
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 5 December 2012 18:49, Lee Jones lee.jo...@linaro.org wrote: Ping!!! Documentation/development-process/2.Process: - Avoid top-posting (the practice of putting your answer above the quoted text you are responding to). It makes your response harder to read and makes a poor impression. Yes, i know that. Did that in hurry, as it was just an ping and wasn't answering any question. -- viresh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar viresh.ku...@linaro.org wrote:
On 30 November 2012 21:15, Lee Jones lee.jo...@linaro.org wrote:
But ... I don't see how the changes in the -i2c and -spi files
are of benefit either. When I boot without the ID table I still
get stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212.
What is it that actually uses the IDs?
Perhaps Viresh can shine some light on the matter?
As you can see, i wasn't the author of this patch and when you asked
this question, i didn't had an answer to it. I went through code and
formed some theory/story :) .
@Grant: i need your help to check if my theory is correct or not. Question
is about adding below code in any i2c client driver:
+#ifdef CONFIG_OF
+static const struct of_device_id stmpe_dt_ids[] = {
+ { .compatible = st,stmpe610, .data = stmpe_i2c_id[0], },
+ { .compatible = st,stmpe801, .data = stmpe_i2c_id[1], },
+ { .compatible = st,stmpe811, .data = stmpe_i2c_id[2], },
+ { .compatible = st,stmpe1601, .data = stmpe_i2c_id[3], },
+ { .compatible = st,stmpe2401, .data = stmpe_i2c_id[4], },
+ { .compatible = st,stmpe2403, .data = stmpe_i2c_id[5], },
+ { }
+};
+MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
+#endif
+
static struct i2c_driver stmpe_i2c_driver = {
.driver = {
.name = stmpe-i2c,
@@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = {
#ifdef CONFIG_PM
.pm = stmpe_dev_pm_ops,
#endif
+ .of_match_table = of_match_ptr(stmpe_dt_ids),
So, what is the use of this table when we already have i2c_driver.id_table
populated.
This is my theory:
-
Adapter drivers supporting DT will call:
of_i2c_register_devices()
{
for_each_child_of_node(adap-dev.of_node, node) {
if (of_modalias_node(node, info.type, sizeof(info.type)) 0)
error condition
...
result = i2c_new_device(adap, info);
...
}
of_modalias_node(): expects compatible in child node, i.e. stmpe node in our
case. If it is not there, then that node is skipped. then it copies
string after ','
to info.type. So, for us only stmpe810 out of st,stmpe810 is copied.
Now this name, i.e. stmpe810 is copied as client.name in i2c_new_device()
and device_register() is called, which will eventually call:
i2c_device_match()
{
/* Attempt an OF style match */
if (of_driver_match_device(dev, 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;
}
This first tries to match the table my patch added, _BUT_ the string will
never match as we had st,stmpe810 in table and stmpe810 in dev.
of_driver_match_device() matches against the compatible list in
dev-of_node, not against the device name. So, if the compatible
property has a string that is in the table, then it really should match
against it.
So, we fall back to i2c_match_id(), which will match it against
i2c_driver.id_table present in driver, which has entry for stmpe810 and
so strings matched.
@Lee: This is what happened in your case. :)
So, whether its DT or non DT, true is returned from here if something
matched.
Later on, this will be called:
static int i2c_device_probe(struct device *dev)
{
.
status = driver-probe(client, i2c_match_id(driver-id_table, client));
Here things are a bit wonky. Even if matched against the table, it is
possible that it also matches against i2c_match_id() and that data is
passed to the driver.
But regardless, it is the responsiblity of the probe function to go and
look if of_driver_match_device() matches against anything if it cares
about the of_match_table entries (for instance, if there is extra data
attached).
}
Which will again match the legacy table to find correct struct i2c_device_id
*id
to pass to probe().
So, the final question: WTF is of_match_table for?
A bit of history is valuable here. The whole of_modalias_node() thing is
really just a best-effort heuristic for figuring out which driver
*might* work against a device described in the device tree. It won't
work in all circumstances (and it was created at a time when there was
resistance to adding DT knowledge to drivers). An of_match_table is the
robust way of identifying specific devices and allows for matching
against any entry in the compatible list.
g.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
First of all, thanks for explaining :)
On 6 December 2012 04:12, Grant Likely grant.lik...@secretlab.ca wrote:
On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar viresh.ku...@linaro.org
wrote:
This first tries to match the table my patch added, _BUT_ the string will
never match as we had st,stmpe810 in table and stmpe810 in dev.
of_driver_match_device() matches against the compatible list in
dev-of_node, not against the device name. So, if the compatible
property has a string that is in the table, then it really should match
against it.
How could i misread it? Yes you are correct.
static int i2c_device_probe(struct device *dev)
{
.
status = driver-probe(client, i2c_match_id(driver-id_table, client));
Here things are a bit wonky. Even if matched against the table, it is
table means of_device_id table ?
possible that it also matches against i2c_match_id() and that data is
passed to the driver.
It is a possibility or guarantee ? And so whatever device name we got from
modalias routine, should match with the names in driver-id_table.
But regardless, it is the responsiblity of the probe function to go and
look if of_driver_match_device() matches against anything if it cares
about the of_match_table entries (for instance, if there is extra data
attached).
Ok, so filling .data field in of_device_id[] is not required for our case as
we aren't doing anything special in our drivers.
Which will again match the legacy table to find correct struct i2c_device_id
*id
to pass to probe().
So, the final question: WTF is of_match_table for?
A bit of history is valuable here. The whole of_modalias_node() thing is
really just a best-effort heuristic for figuring out which driver
*might* work against a device described in the device tree. It won't
work in all circumstances (and it was created at a time when there was
resistance to adding DT knowledge to drivers). An of_match_table is the
robust way of identifying specific devices and allows for matching
against any entry in the compatible list.
Got it.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 6 December 2012 04:12, Grant Likely grant.lik...@secretlab.ca wrote: On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar viresh.ku...@linaro.org wrote: This first tries to match the table my patch added, _BUT_ the string will never match as we had st,stmpe810 in table and stmpe810 in dev. of_driver_match_device() matches against the compatible list in dev-of_node, not against the device name. So, if the compatible property has a string that is in the table, then it really should match against it. Grant, but isn't it true that the final device's name would not be the DT way of names? It would simply be stmpe810 for us and so if we have multiple instances of stmpe on a board, we need to distinguish them ourselves? One way for that was passing id for these instances, which would finally be used, when we create platform devices for sub-modules of stmpe (gpio, keypad, ts, etc). -- viresh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 30 November 2012 21:15, Lee Jones wrote:
> But ... I don't see how the changes in the -i2c and -spi files
> are of benefit either. When I boot without the ID table I still
> get "stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212".
>
> What is it that actually uses the IDs?
>
> Perhaps Viresh can shine some light on the matter?
As you can see, i wasn't the author of this patch and when you asked
this question, i didn't had an answer to it. I went through code and
formed some theory/story :) .
@Grant: i need your help to check if my theory is correct or not. Question
is about adding below code in any i2c client driver:
+#ifdef CONFIG_OF
+static const struct of_device_id stmpe_dt_ids[] = {
+ { .compatible = "st,stmpe610", .data = _i2c_id[0], },
+ { .compatible = "st,stmpe801", .data = _i2c_id[1], },
+ { .compatible = "st,stmpe811", .data = _i2c_id[2], },
+ { .compatible = "st,stmpe1601", .data = _i2c_id[3], },
+ { .compatible = "st,stmpe2401", .data = _i2c_id[4], },
+ { .compatible = "st,stmpe2403", .data = _i2c_id[5], },
+ { }
+};
+MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
+#endif
+
static struct i2c_driver stmpe_i2c_driver = {
.driver = {
.name = "stmpe-i2c",
@@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = {
#ifdef CONFIG_PM
.pm = _dev_pm_ops,
#endif
+ .of_match_table = of_match_ptr(stmpe_dt_ids),
So, what is the use of this table when we already have i2c_driver.id_table
populated.
This is my theory:
-
Adapter drivers supporting DT will call:
of_i2c_register_devices()
{
for_each_child_of_node(adap->dev.of_node, node) {
if (of_modalias_node(node, info.type, sizeof(info.type)) < 0)
error condition
...
result = i2c_new_device(adap, );
...
}
of_modalias_node(): expects compatible in child node, i.e. stmpe node in our
case. If it is not there, then that node is skipped. then it copies
string after ','
to info.type. So, for us only "stmpe810" out of "st,stmpe810" is copied.
Now this name, i.e. "stmpe810" is copied as client.name in i2c_new_device()
and device_register() is called, which will eventually call:
i2c_device_match()
{
/* Attempt an OF style match */
if (of_driver_match_device(dev, 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;
}
This first tries to match the table my patch added, _BUT_ the string will
never match as we had "st,stmpe810" in table and "stmpe810" in dev.
So, we fall back to i2c_match_id(), which will match it against
i2c_driver.id_table present in driver, which has entry for "stmpe810" and
so strings matched.
@Lee: This is what happened in your case. :)
So, whether its DT or non DT, true is returned from here if something
matched.
Later on, this will be called:
static int i2c_device_probe(struct device *dev)
{
.
status = driver->probe(client, i2c_match_id(driver->id_table, client));
}
Which will again match the legacy table to find correct struct i2c_device_id *id
to pass to probe().
So, the final question: WTF is of_match_table for?
Then i thought maybe it is used when we don't have child nodes inside parent,
something related to the phandle way ? I grep'd i2c in arch/arm/boot/dts and
couldn't find anything of that sort, the way i2c clients are added is:
in dtsi file:
i2c0: i2c@address {
...
}
in dts file:
{
stmpe810 {
}
}
which i believe will be taken care by dtc and will fold client nodes
as child nodes
of i2c0.
@Grant: Please throw some light here :)
--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Fri, 30 Nov 2012, Samuel Ortiz wrote: > Hi Viresh, Lee, > > On Thu, Nov 29, 2012 at 08:10:18PM +0530, Viresh Kumar wrote: > > From: Vipul Kumar Samar > > > > This patch extends existing DT support for stmpe devices. This updates: > > - DT support from stmpe SPI and I2C drivers > > - missing header files in stmpe.c > > - stmpe_of_probe() with pwm, rotator and new bindings. > > - Bindings are updated in binding document. > > > > Signed-off-by: Vipul Kumar Samar > > Signed-off-by: Viresh Kumar > > --- > > V4->V5: > > -- > > - 2/3 and 3/3 merged. > > - irq_trigger is kept same for non-DT booti. > > > > @Lee: I haven't added your Acked-by, because this differs from your Acked > > version. > Lee, are you ok with this one ? Could you give it a test ? Okay, I've tested it, and it doesn't break anything. But ... I don't see how the changes in the -i2c and -spi files are of benefit either. When I boot without the ID table I still get "stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212". What is it that actually uses the IDs? Perhaps Viresh can shine some light on the matter? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Fri, 30 Nov 2012, Viresh Kumar wrote: > On 30 November 2012 18:15, Lee Jones wrote: > > The patch doesn't apply for me - does it for you? > > > > Viresh, what's it based on? > > Because this was applied 2 days back by Samuel, and i didn't > fetch it again yesterday: > > commit 20d5c7defc228cdaeff3ce3442f3a4e86af293c1 > Author: Randy Dunlap > Date: Mon Nov 12 09:20:49 2012 -0800 > > Its a simple conflict to fix, as this is the only place of conflict. Eh? You don't know what my tree is based on. ;) What does this patch apply on top of? Which -rc? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 30 November 2012 18:15, Lee Jones wrote:
> The patch doesn't apply for me - does it for you?
>
> Viresh, what's it based on?
Because this was applied 2 days back by Samuel, and i didn't
fetch it again yesterday:
commit 20d5c7defc228cdaeff3ce3442f3a4e86af293c1
Author: Randy Dunlap
Date: Mon Nov 12 09:20:49 2012 -0800
mfd: Fix stmpe.c build when OF is not enabled
Fix build errors when CONFIG_OF is not enabled by including
(needs to be added in any case).
An alternative fix could be to make the driver depend on OF.
drivers/mfd/stmpe.c:1025:2: error: implicit declaration of
function 'of_property_read_u32'
drivers/mfd/stmpe.c:1030:2: error: implicit declaration of
function 'for_each_child_of_node'
drivers/mfd/stmpe.c:1030:36: error: expected ';' before '{' token
Signed-off-by: Randy Dunlap
Acked-by: Linus Walleij
Signed-off-by: Samuel Ortiz
---
drivers/mfd/stmpe.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index 0061d1b..f9f7de7 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
Its a simple conflict to fix, as this is the only place of conflict.
--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Fri, 30 Nov 2012, Samuel Ortiz wrote: > Hi Viresh, Lee, > > On Thu, Nov 29, 2012 at 08:10:18PM +0530, Viresh Kumar wrote: > > From: Vipul Kumar Samar > > > > This patch extends existing DT support for stmpe devices. This updates: > > - DT support from stmpe SPI and I2C drivers > > - missing header files in stmpe.c > > - stmpe_of_probe() with pwm, rotator and new bindings. > > - Bindings are updated in binding document. > > > > Signed-off-by: Vipul Kumar Samar > > Signed-off-by: Viresh Kumar > > --- > > V4->V5: > > -- > > - 2/3 and 3/3 merged. > > - irq_trigger is kept same for non-DT booti. > > > > @Lee: I haven't added your Acked-by, because this differs from your Acked > > version. > Lee, are you ok with this one ? Could you give it a test ? The patch doesn't apply for me - does it for you? Viresh, what's it based on? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
Hi Viresh, Lee, On Thu, Nov 29, 2012 at 08:10:18PM +0530, Viresh Kumar wrote: > From: Vipul Kumar Samar > > This patch extends existing DT support for stmpe devices. This updates: > - DT support from stmpe SPI and I2C drivers > - missing header files in stmpe.c > - stmpe_of_probe() with pwm, rotator and new bindings. > - Bindings are updated in binding document. > > Signed-off-by: Vipul Kumar Samar > Signed-off-by: Viresh Kumar > --- > V4->V5: > -- > - 2/3 and 3/3 merged. > - irq_trigger is kept same for non-DT booti. > > @Lee: I haven't added your Acked-by, because this differs from your Acked > version. Lee, are you ok with this one ? Could you give it a test ? Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
Hi Viresh, Lee, On Thu, Nov 29, 2012 at 08:10:18PM +0530, Viresh Kumar wrote: From: Vipul Kumar Samar vipulkumar.sa...@st.com This patch extends existing DT support for stmpe devices. This updates: - DT support from stmpe SPI and I2C drivers - missing header files in stmpe.c - stmpe_of_probe() with pwm, rotator and new bindings. - Bindings are updated in binding document. Signed-off-by: Vipul Kumar Samar vipulkumar.sa...@st.com Signed-off-by: Viresh Kumar viresh.ku...@linaro.org --- V4-V5: -- - 2/3 and 3/3 merged. - irq_trigger is kept same for non-DT booti. @Lee: I haven't added your Acked-by, because this differs from your Acked version. Lee, are you ok with this one ? Could you give it a test ? Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Fri, 30 Nov 2012, Samuel Ortiz wrote: Hi Viresh, Lee, On Thu, Nov 29, 2012 at 08:10:18PM +0530, Viresh Kumar wrote: From: Vipul Kumar Samar vipulkumar.sa...@st.com This patch extends existing DT support for stmpe devices. This updates: - DT support from stmpe SPI and I2C drivers - missing header files in stmpe.c - stmpe_of_probe() with pwm, rotator and new bindings. - Bindings are updated in binding document. Signed-off-by: Vipul Kumar Samar vipulkumar.sa...@st.com Signed-off-by: Viresh Kumar viresh.ku...@linaro.org --- V4-V5: -- - 2/3 and 3/3 merged. - irq_trigger is kept same for non-DT booti. @Lee: I haven't added your Acked-by, because this differs from your Acked version. Lee, are you ok with this one ? Could you give it a test ? The patch doesn't apply for me - does it for you? Viresh, what's it based on? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 30 November 2012 18:15, Lee Jones lee.jo...@linaro.org wrote:
The patch doesn't apply for me - does it for you?
Viresh, what's it based on?
Because this was applied 2 days back by Samuel, and i didn't
fetch it again yesterday:
commit 20d5c7defc228cdaeff3ce3442f3a4e86af293c1
Author: Randy Dunlap rdun...@infradead.org
Date: Mon Nov 12 09:20:49 2012 -0800
mfd: Fix stmpe.c build when OF is not enabled
Fix build errors when CONFIG_OF is not enabled by including
linux/of.h (needs to be added in any case).
An alternative fix could be to make the driver depend on OF.
drivers/mfd/stmpe.c:1025:2: error: implicit declaration of
function 'of_property_read_u32'
drivers/mfd/stmpe.c:1030:2: error: implicit declaration of
function 'for_each_child_of_node'
drivers/mfd/stmpe.c:1030:36: error: expected ';' before '{' token
Signed-off-by: Randy Dunlap rdun...@infradead.org
Acked-by: Linus Walleij linus.wall...@linaro.org
Signed-off-by: Samuel Ortiz sa...@linux.intel.com
---
drivers/mfd/stmpe.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index 0061d1b..f9f7de7 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -13,6 +13,7 @@
#include linux/interrupt.h
#include linux/irq.h
#include linux/irqdomain.h
+#include linux/of.h
Its a simple conflict to fix, as this is the only place of conflict.
--
viresh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Fri, 30 Nov 2012, Viresh Kumar wrote: On 30 November 2012 18:15, Lee Jones lee.jo...@linaro.org wrote: The patch doesn't apply for me - does it for you? Viresh, what's it based on? Because this was applied 2 days back by Samuel, and i didn't fetch it again yesterday: commit 20d5c7defc228cdaeff3ce3442f3a4e86af293c1 Author: Randy Dunlap rdun...@infradead.org Date: Mon Nov 12 09:20:49 2012 -0800 Its a simple conflict to fix, as this is the only place of conflict. Eh? You don't know what my tree is based on. ;) What does this patch apply on top of? Which -rc? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On Fri, 30 Nov 2012, Samuel Ortiz wrote: Hi Viresh, Lee, On Thu, Nov 29, 2012 at 08:10:18PM +0530, Viresh Kumar wrote: From: Vipul Kumar Samar vipulkumar.sa...@st.com This patch extends existing DT support for stmpe devices. This updates: - DT support from stmpe SPI and I2C drivers - missing header files in stmpe.c - stmpe_of_probe() with pwm, rotator and new bindings. - Bindings are updated in binding document. Signed-off-by: Vipul Kumar Samar vipulkumar.sa...@st.com Signed-off-by: Viresh Kumar viresh.ku...@linaro.org --- V4-V5: -- - 2/3 and 3/3 merged. - irq_trigger is kept same for non-DT booti. @Lee: I haven't added your Acked-by, because this differs from your Acked version. Lee, are you ok with this one ? Could you give it a test ? Okay, I've tested it, and it doesn't break anything. But ... I don't see how the changes in the -i2c and -spi files are of benefit either. When I boot without the ID table I still get stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212. What is it that actually uses the IDs? Perhaps Viresh can shine some light on the matter? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver
On 30 November 2012 21:15, Lee Jones lee.jo...@linaro.org wrote:
But ... I don't see how the changes in the -i2c and -spi files
are of benefit either. When I boot without the ID table I still
get stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212.
What is it that actually uses the IDs?
Perhaps Viresh can shine some light on the matter?
As you can see, i wasn't the author of this patch and when you asked
this question, i didn't had an answer to it. I went through code and
formed some theory/story :) .
@Grant: i need your help to check if my theory is correct or not. Question
is about adding below code in any i2c client driver:
+#ifdef CONFIG_OF
+static const struct of_device_id stmpe_dt_ids[] = {
+ { .compatible = st,stmpe610, .data = stmpe_i2c_id[0], },
+ { .compatible = st,stmpe801, .data = stmpe_i2c_id[1], },
+ { .compatible = st,stmpe811, .data = stmpe_i2c_id[2], },
+ { .compatible = st,stmpe1601, .data = stmpe_i2c_id[3], },
+ { .compatible = st,stmpe2401, .data = stmpe_i2c_id[4], },
+ { .compatible = st,stmpe2403, .data = stmpe_i2c_id[5], },
+ { }
+};
+MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
+#endif
+
static struct i2c_driver stmpe_i2c_driver = {
.driver = {
.name = stmpe-i2c,
@@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = {
#ifdef CONFIG_PM
.pm = stmpe_dev_pm_ops,
#endif
+ .of_match_table = of_match_ptr(stmpe_dt_ids),
So, what is the use of this table when we already have i2c_driver.id_table
populated.
This is my theory:
-
Adapter drivers supporting DT will call:
of_i2c_register_devices()
{
for_each_child_of_node(adap-dev.of_node, node) {
if (of_modalias_node(node, info.type, sizeof(info.type)) 0)
error condition
...
result = i2c_new_device(adap, info);
...
}
of_modalias_node(): expects compatible in child node, i.e. stmpe node in our
case. If it is not there, then that node is skipped. then it copies
string after ','
to info.type. So, for us only stmpe810 out of st,stmpe810 is copied.
Now this name, i.e. stmpe810 is copied as client.name in i2c_new_device()
and device_register() is called, which will eventually call:
i2c_device_match()
{
/* Attempt an OF style match */
if (of_driver_match_device(dev, 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;
}
This first tries to match the table my patch added, _BUT_ the string will
never match as we had st,stmpe810 in table and stmpe810 in dev.
So, we fall back to i2c_match_id(), which will match it against
i2c_driver.id_table present in driver, which has entry for stmpe810 and
so strings matched.
@Lee: This is what happened in your case. :)
So, whether its DT or non DT, true is returned from here if something
matched.
Later on, this will be called:
static int i2c_device_probe(struct device *dev)
{
.
status = driver-probe(client, i2c_match_id(driver-id_table, client));
}
Which will again match the legacy table to find correct struct i2c_device_id *id
to pass to probe().
So, the final question: WTF is of_match_table for?
Then i thought maybe it is used when we don't have child nodes inside parent,
something related to the phandle way ? I grep'd i2c in arch/arm/boot/dts and
couldn't find anything of that sort, the way i2c clients are added is:
in dtsi file:
i2c0: i2c@address {
...
}
in dts file:
i2c0 {
stmpe810 {
}
}
which i believe will be taken care by dtc and will fold client nodes
as child nodes
of i2c0.
@Grant: Please throw some light here :)
--
viresh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
