Re: [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 23 November 2012 21:13, Lee Jones wrote: > No, in DT devices named as part of the hiearchy, so you'd have: > > soc-u9500/i2c@80004000/stmpe1601@40/stmpe_keypad > soc-u9500/i2c@80004000/stmpe1601@41/stmpe_keypad > ... etc Obviously. How could i miss this naming :( Okay, so i need to pass -1 and that would be enough. Right? -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Fri, 23 Nov 2012, Viresh Kumar wrote: > On 23 November 2012 15:06, Lee Jones wrote: > > On Fri, 23 Nov 2012, Viresh Kumar wrote: > >> pdev = platform_device_alloc(cell->name, id + cell->id); > >> > >> This is required when we have multiple instances of MFD device present > >> on board. How do you want me to handle this ? > > > > There are lots of examples of this already. I have to leave something > > to the imagination, or I'll be requesting a cut of your salary. :D > > My manager already reduced my salary by 20% after reading this mail :( Ah, Good. Tell your manager I'll send my offshore bank details over soon. :) > Ok, this is what my understanding of whole this is. Platform devices are > named like: > - pdev-name: if id passed in pdev.id is -1 > - pdev-name.0[1|2|...]: if id passed is 0[1|2|...] > - pdev-name.: if id passed is -2 > > Now, we don't declare cell->id fields and they are currently zero and so > value is passed from pdata->id field. So, for example with multiple instances > of stmpe on a board, we have: > > - stmpe-0: //Name just for reference... >- stmpe-gpio.0 >- stmpe-ts.0 > - stmpe-1: >- stmpe-gpio.1 >- stmpe-ts.1 > - stmpe-2: >- stmpe-gpio.2 >- stmpe-ts.2 > > I main idea is to distinguish various instances of sub modules, like > stmpe-gpio. > And this works well with non-DT support we have currently. Yes, when !DT, then passing ID is no problem. > With DT, i am not sure how should we pass id field to mfd_add_devices(). If > we pass it -1, then multiple instances will have same name: "stmpe-gpio" No, in DT devices named as part of the hiearchy, so you'd have: soc-u9500/i2c@80004000/stmpe1601@40/stmpe_keypad soc-u9500/i2c@80004000/stmpe1601@41/stmpe_keypad ... etc The only time we need to be concerned is if one stmpe device can handle more than one keypad, gpio controller, etc. Which I don't think is the case. > Sorry, for my lack of knowledge. Don't send another mail with salary cut > suggestion as that will make it -40% in total ;) Ah, I promise I'll spend it wisely. :) -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 23 November 2012 15:06, Lee Jones wrote: > On Fri, 23 Nov 2012, Viresh Kumar wrote: >> pdev = platform_device_alloc(cell->name, id + cell->id); >> >> This is required when we have multiple instances of MFD device present >> on board. How do you want me to handle this ? > > There are lots of examples of this already. I have to leave something > to the imagination, or I'll be requesting a cut of your salary. :D My manager already reduced my salary by 20% after reading this mail :( Ok, this is what my understanding of whole this is. Platform devices are named like: - pdev-name: if id passed in pdev.id is -1 - pdev-name.0[1|2|...]: if id passed is 0[1|2|...] - pdev-name.: if id passed is -2 Now, we don't declare cell->id fields and they are currently zero and so value is passed from pdata->id field. So, for example with multiple instances of stmpe on a board, we have: - stmpe-0: //Name just for reference... - stmpe-gpio.0 - stmpe-ts.0 - stmpe-1: - stmpe-gpio.1 - stmpe-ts.1 - stmpe-2: - stmpe-gpio.2 - stmpe-ts.2 I main idea is to distinguish various instances of sub modules, like stmpe-gpio. And this works well with non-DT support we have currently. With DT, i am not sure how should we pass id field to mfd_add_devices(). If we pass it -1, then multiple instances will have same name: "stmpe-gpio" Sorry, for my lack of knowledge. Don't send another mail with salary cut suggestion as that will make it -40% in total ;) Thanks for reviewing :) -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Fri, 23 Nov 2012, Viresh Kumar wrote: > On 22 November 2012 16:54, Lee Jones wrote: > >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt > >> b/Documentation/devicetree/bindings/mfd/stmpe.txt > >> +Optional properties: > >> +- irq-trigger: IRQ trigger to use for the interrupt to the host > >> +- irq-invert-polarity: bool, IRQ line is connected with reversed polarity > > > > Are these new? > > > > When adding new bindings, ask yourself: > > I was trying to get information of these two bindings from other sources, > but couldn't find. The only other place can be the interrupts field, right? Bingo. Continue along this thread. > Because interrupts field depends on the interrupt controller, which can > be a wide variety of controllers in our case, we can't use that. Also, > the cells of interrupts field will have something that is required to be > programmed in the interrupt controller and not in the user of IC. > > But here we are talking about programming stmpe and so these bindings > are very much required in stmpe only. Not true. Hint: `git grep triggered -- Documentation/devicetree/` -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Fri, 23 Nov 2012, Viresh Kumar wrote:
> On 22 November 2012 16:54, Lee Jones wrote:
> >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt
> >> b/Documentation/devicetree/bindings/mfd/stmpe.txt
> >> stmpe1601: stmpe1601@40 {
>
> >> + id = <0>;
> >
> > Don't do this. Device IDs are Linux specific.
>
> Hi Lee,
>
> This is id of the mfd device that we need to pass to mfd_add_device()
> and is used in following:
MFD devices are Linux specific, whereas DT is cross-platform. Thus
you can't put it in the DTS(I) files.
> pdev = platform_device_alloc(cell->name, id + cell->id);
>
> This is required when we have multiple instances of MFD device present
> on board. How do you want me to handle this ?
There are lots of examples of this already. I have to leave something
to the imagination, or I'll be requesting a cut of your salary. :D
--
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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Fri, 23 Nov 2012, Shiraz Hashim wrote: > On Thu, Nov 22, 2012 at 10:31:48PM +0530, Viresh Kumar wrote: > > On 22 November 2012 21:16, Lee Jones wrote: > > > The STMPE GPIO controller can't be used by Device Tree yet in > > > any case, because it doesn't have an IRQ domain. This is > > > compulsory, or it won't work. Have you tried to test this > > > functionality yet? > > > > I don't have SPEAr board to test it anymore. I have moved out of > > ST now and working in linaro as ARM asignee. Just pushing these > > as an part time activity. > > > > Though ST guys would have tested stmpe, but stmpe-gpio, i am not > > sure about. > > Let me bring some more information here. I totally understand > Jones concerns, but the way stmpe (and may be other mfd devices) > are handled is this that the parent block (i.e. stmpe) decides on > the variants (say by probing device itself) and then prepares > associated data for the (probed) variant and creates a platform > device for the same. I realise this, but now we're using Device Tree, there's no need to stuff pdata in the parent driver now. It's better that the child devices are self sufficient. > For the interrupts case also, it is stmpe which registers the > irq domain. This is because, stmpe driver probes variant and > populates its platform data and stmpe-gpio may not be aware of the > variant it serves. At the same time, it (stmpe) needs few of the > (virtual) interrupts for its internal purpose also. I know. I wrote the IRQ domain for STMPE. ;) STMPE needs its own one too, which I will work on now. STMPE-GPIO doesn't need to be aware of anything, the device which wishes to use its GPIOs/IRQs will reference it from Device Tree in the manor previously explained. > Hence stmpe passes irq_base to the stmpe-gpio driver while > allocating and registering irq domain by itself. Not anymore it doesn't. There are no irq_base:s with DT. All IRQs are dynamic, hence why STMPE requires its own domain to play with. I can fix that. > With this approach we have tested the functionality on SPEAr > platform. You'll need to test it again with the new DT approach too. :) -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Thu, 22 Nov 2012, Viresh Kumar wrote: > On 22 November 2012 21:16, Lee Jones wrote: > >> >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt > >> >> b/Documentation/devicetree/bindings/mfd/stmpe.txt > >> >> +- irq-over-gpio: bool, true if gpio is used to get irq > >> >> +- irq-gpios: gpio number over which irq will be requested (significant > >> >> only if > >> >> + irq-over-gpio is true) > >> > > >> > You don't need these. Use gpio_to_irq() instead. > >> > >> I am passing gpio numbers here and am doing gpio_to_irq() in driver. > >> Didn't get this one :( > > > > For a start you have 'irq-over-gpio' in the binding document and Device Tree > > and 'irq_over_gpio' in the code. Has it even been tested? > > > > GPIOs are used as IRQ lines in many other previously DT:ed drivers. Take a > > look to see how they are handled without adding unnecessary DT bindings. > > I already knew it, should have picked that up. :( > > >> stmpe is an interrupt controller for the IP's which are present inside > >> it: gpio, adc. > >> But interrupt lines for them are managed by stmpe driver internally. So > >> should > >> we really add interrupt-controller for it? > > > > You can't manage IRQ lines internally, you have to go through > > the IRQ subsystem. When you request an IRQ via device tree you > > will do so like this: > > By that i meant, there is no external node which would have stmpe as > interrupt controller. Because all of them would be its child node. > > This is guaranteed because stmpe is an external device is present on board. > So, it will have its entry in board dts file, and so wouldn't be > scattered in different > files. It doesn't matter how it's wired up. If another node references it as it's IRQ controller you have to declare it as one using the interrupt-controller binding. > > The STMPE GPIO controller can't be used by Device Tree yet in any case, > > because it doesn't have an IRQ domain. This is compulsory, or it won't > > work. Have you tried to test this functionality yet? > > I don't have SPEAr board to test it anymore. I have moved out of ST now and > working in linaro as ARM asignee. Just pushing these as an part time activity. Surely you can't push patches which haven't been tested?! Try to get yourself some hardware. Does anyone near you have an HREF? In the mean-time, I will write you an IRQ domain. > Though ST guys would have tested stmpe, but stmpe-gpio, i am not sure about. It needs to be tested before being accpeted. > > I didn't go through them, but are you sure that: > > > > 1. Can I do without them? > >1.1 Can I derive the configuration from other things? > >2.2 Are they _really_ required, or am I just blindly copying platform > > data? > > 2. Does a similar binding already exist? > > 3. Can other drivers make use of them? > >3.1 If so, create a generic binding > >3.2 If not, prepend the binding with "," > > I will go through them again. Thanks. -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Thu, 22 Nov 2012, Viresh Kumar wrote: On 22 November 2012 21:16, Lee Jones lee.jo...@linaro.org wrote: diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt +- irq-over-gpio: bool, true if gpio is used to get irq +- irq-gpios: gpio number over which irq will be requested (significant only if + irq-over-gpio is true) You don't need these. Use gpio_to_irq() instead. I am passing gpio numbers here and am doing gpio_to_irq() in driver. Didn't get this one :( For a start you have 'irq-over-gpio' in the binding document and Device Tree and 'irq_over_gpio' in the code. Has it even been tested? GPIOs are used as IRQ lines in many other previously DT:ed drivers. Take a look to see how they are handled without adding unnecessary DT bindings. I already knew it, should have picked that up. :( stmpe is an interrupt controller for the IP's which are present inside it: gpio, adc. But interrupt lines for them are managed by stmpe driver internally. So should we really add interrupt-controller for it? You can't manage IRQ lines internally, you have to go through the IRQ subsystem. When you request an IRQ via device tree you will do so like this: By that i meant, there is no external node which would have stmpe as interrupt controller. Because all of them would be its child node. This is guaranteed because stmpe is an external device is present on board. So, it will have its entry in board dts file, and so wouldn't be scattered in different files. It doesn't matter how it's wired up. If another node references it as it's IRQ controller you have to declare it as one using the interrupt-controller binding. The STMPE GPIO controller can't be used by Device Tree yet in any case, because it doesn't have an IRQ domain. This is compulsory, or it won't work. Have you tried to test this functionality yet? I don't have SPEAr board to test it anymore. I have moved out of ST now and working in linaro as ARM asignee. Just pushing these as an part time activity. Surely you can't push patches which haven't been tested?! Try to get yourself some hardware. Does anyone near you have an HREF? In the mean-time, I will write you an IRQ domain. Though ST guys would have tested stmpe, but stmpe-gpio, i am not sure about. It needs to be tested before being accpeted. I didn't go through them, but are you sure that: 1. Can I do without them? 1.1 Can I derive the configuration from other things? 2.2 Are they _really_ required, or am I just blindly copying platform data? 2. Does a similar binding already exist? 3. Can other drivers make use of them? 3.1 If so, create a generic binding 3.2 If not, prepend the binding with vendor, I will go through them again. Thanks. -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Fri, 23 Nov 2012, Shiraz Hashim wrote: On Thu, Nov 22, 2012 at 10:31:48PM +0530, Viresh Kumar wrote: On 22 November 2012 21:16, Lee Jones lee.jo...@linaro.org wrote: The STMPE GPIO controller can't be used by Device Tree yet in any case, because it doesn't have an IRQ domain. This is compulsory, or it won't work. Have you tried to test this functionality yet? I don't have SPEAr board to test it anymore. I have moved out of ST now and working in linaro as ARM asignee. Just pushing these as an part time activity. Though ST guys would have tested stmpe, but stmpe-gpio, i am not sure about. Let me bring some more information here. I totally understand Jones concerns, but the way stmpe (and may be other mfd devices) are handled is this that the parent block (i.e. stmpe) decides on the variants (say by probing device itself) and then prepares associated data for the (probed) variant and creates a platform device for the same. I realise this, but now we're using Device Tree, there's no need to stuff pdata in the parent driver now. It's better that the child devices are self sufficient. For the interrupts case also, it is stmpe which registers the irq domain. This is because, stmpe driver probes variant and populates its platform data and stmpe-gpio may not be aware of the variant it serves. At the same time, it (stmpe) needs few of the (virtual) interrupts for its internal purpose also. I know. I wrote the IRQ domain for STMPE. ;) STMPE needs its own one too, which I will work on now. STMPE-GPIO doesn't need to be aware of anything, the device which wishes to use its GPIOs/IRQs will reference it from Device Tree in the manor previously explained. Hence stmpe passes irq_base to the stmpe-gpio driver while allocating and registering irq domain by itself. Not anymore it doesn't. There are no irq_base:s with DT. All IRQs are dynamic, hence why STMPE requires its own domain to play with. I can fix that. With this approach we have tested the functionality on SPEAr platform. You'll need to test it again with the new DT approach too. :) -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Fri, 23 Nov 2012, Viresh Kumar wrote:
On 22 November 2012 16:54, Lee Jones lee.jo...@linaro.org wrote:
diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt
b/Documentation/devicetree/bindings/mfd/stmpe.txt
stmpe1601: stmpe1601@40 {
+ id = 0;
Don't do this. Device IDs are Linux specific.
Hi Lee,
This is id of the mfd device that we need to pass to mfd_add_device()
and is used in following:
MFD devices are Linux specific, whereas DT is cross-platform. Thus
you can't put it in the DTS(I) files.
pdev = platform_device_alloc(cell-name, id + cell-id);
This is required when we have multiple instances of MFD device present
on board. How do you want me to handle this ?
There are lots of examples of this already. I have to leave something
to the imagination, or I'll be requesting a cut of your salary. :D
--
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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Fri, 23 Nov 2012, Viresh Kumar wrote: On 22 November 2012 16:54, Lee Jones lee.jo...@linaro.org wrote: diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt +Optional properties: +- irq-trigger: IRQ trigger to use for the interrupt to the host +- irq-invert-polarity: bool, IRQ line is connected with reversed polarity Are these new? When adding new bindings, ask yourself: I was trying to get information of these two bindings from other sources, but couldn't find. The only other place can be the interrupts field, right? Bingo. Continue along this thread. Because interrupts field depends on the interrupt controller, which can be a wide variety of controllers in our case, we can't use that. Also, the cells of interrupts field will have something that is required to be programmed in the interrupt controller and not in the user of IC. But here we are talking about programming stmpe and so these bindings are very much required in stmpe only. Not true. Hint: `git grep triggered -- Documentation/devicetree/` -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 23 November 2012 15:06, Lee Jones lee.jo...@linaro.org wrote: On Fri, 23 Nov 2012, Viresh Kumar wrote: pdev = platform_device_alloc(cell-name, id + cell-id); This is required when we have multiple instances of MFD device present on board. How do you want me to handle this ? There are lots of examples of this already. I have to leave something to the imagination, or I'll be requesting a cut of your salary. :D My manager already reduced my salary by 20% after reading this mail :( Ok, this is what my understanding of whole this is. Platform devices are named like: - pdev-name: if id passed in pdev.id is -1 - pdev-name.0[1|2|...]: if id passed is 0[1|2|...] - pdev-name.dynamically allocated by kernel: if id passed is -2 Now, we don't declare cell-id fields and they are currently zero and so value is passed from pdata-id field. So, for example with multiple instances of stmpe on a board, we have: - stmpe-0: //Name just for reference... - stmpe-gpio.0 - stmpe-ts.0 - stmpe-1: - stmpe-gpio.1 - stmpe-ts.1 - stmpe-2: - stmpe-gpio.2 - stmpe-ts.2 I main idea is to distinguish various instances of sub modules, like stmpe-gpio. And this works well with non-DT support we have currently. With DT, i am not sure how should we pass id field to mfd_add_devices(). If we pass it -1, then multiple instances will have same name: stmpe-gpio Sorry, for my lack of knowledge. Don't send another mail with salary cut suggestion as that will make it -40% in total ;) Thanks for reviewing :) -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Fri, 23 Nov 2012, Viresh Kumar wrote: On 23 November 2012 15:06, Lee Jones lee.jo...@linaro.org wrote: On Fri, 23 Nov 2012, Viresh Kumar wrote: pdev = platform_device_alloc(cell-name, id + cell-id); This is required when we have multiple instances of MFD device present on board. How do you want me to handle this ? There are lots of examples of this already. I have to leave something to the imagination, or I'll be requesting a cut of your salary. :D My manager already reduced my salary by 20% after reading this mail :( Ah, Good. Tell your manager I'll send my offshore bank details over soon. :) Ok, this is what my understanding of whole this is. Platform devices are named like: - pdev-name: if id passed in pdev.id is -1 - pdev-name.0[1|2|...]: if id passed is 0[1|2|...] - pdev-name.dynamically allocated by kernel: if id passed is -2 Now, we don't declare cell-id fields and they are currently zero and so value is passed from pdata-id field. So, for example with multiple instances of stmpe on a board, we have: - stmpe-0: //Name just for reference... - stmpe-gpio.0 - stmpe-ts.0 - stmpe-1: - stmpe-gpio.1 - stmpe-ts.1 - stmpe-2: - stmpe-gpio.2 - stmpe-ts.2 I main idea is to distinguish various instances of sub modules, like stmpe-gpio. And this works well with non-DT support we have currently. Yes, when !DT, then passing ID is no problem. With DT, i am not sure how should we pass id field to mfd_add_devices(). If we pass it -1, then multiple instances will have same name: stmpe-gpio No, in DT devices named as part of the hiearchy, so you'd have: soc-u9500/i2c@80004000/stmpe1601@40/stmpe_keypad soc-u9500/i2c@80004000/stmpe1601@41/stmpe_keypad ... etc The only time we need to be concerned is if one stmpe device can handle more than one keypad, gpio controller, etc. Which I don't think is the case. Sorry, for my lack of knowledge. Don't send another mail with salary cut suggestion as that will make it -40% in total ;) Ah, I promise I'll spend it wisely. :) -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 23 November 2012 21:13, Lee Jones lee.jo...@linaro.org wrote: No, in DT devices named as part of the hiearchy, so you'd have: soc-u9500/i2c@80004000/stmpe1601@40/stmpe_keypad soc-u9500/i2c@80004000/stmpe1601@41/stmpe_keypad ... etc Obviously. How could i miss this naming :( Okay, so i need to pass -1 and that would be enough. Right? -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 22 November 2012 16:54, Lee Jones wrote:
>> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt
>> b/Documentation/devicetree/bindings/mfd/stmpe.txt
>> stmpe1601: stmpe1601@40 {
>> + id = <0>;
>
> Don't do this. Device IDs are Linux specific.
Hi Lee,
This is id of the mfd device that we need to pass to mfd_add_device()
and is used in following:
pdev = platform_device_alloc(cell->name, id + cell->id);
This is required when we have multiple instances of MFD device present
on board. How do you want me to handle this ?
--
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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Thu, Nov 22, 2012 at 10:31:48PM +0530, Viresh Kumar wrote: > On 22 November 2012 21:16, Lee Jones wrote: > > The STMPE GPIO controller can't be used by Device Tree yet in > > any case, because it doesn't have an IRQ domain. This is > > compulsory, or it won't work. Have you tried to test this > > functionality yet? > > I don't have SPEAr board to test it anymore. I have moved out of > ST now and working in linaro as ARM asignee. Just pushing these > as an part time activity. > > Though ST guys would have tested stmpe, but stmpe-gpio, i am not > sure about. Let me bring some more information here. I totally understand Jones concerns, but the way stmpe (and may be other mfd devices) are handled is this that the parent block (i.e. stmpe) decides on the variants (say by probing device itself) and then prepares associated data for the (probed) variant and creates a platform device for the same. For the interrupts case also, it is stmpe which registers the irq domain. This is because, stmpe driver probes variant and populates its platform data and stmpe-gpio may not be aware of the variant it serves. At the same time, it (stmpe) needs few of the (virtual) interrupts for its internal purpose also. Hence stmpe passes irq_base to the stmpe-gpio driver while allocating and registering irq domain by itself. With this approach we have tested the functionality on SPEAr platform. -- regards Shiraz -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 22 November 2012 16:54, Lee Jones wrote:
> Big fat NACK.
>
> You've just overwritten the current implementation with your own.
> Please take time to understand the mechanisms in place before
> you submit any changes or additions to it.
:)
My fault. Comments on all overwritten stuff accepted
>> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt
>> b/Documentation/devicetree/bindings/mfd/stmpe.txt
>> +- irq-over-gpio: bool, true if gpio is used to get irq
>> +- irq-gpios: gpio number over which irq will be requested (significant only
>> if
>> + irq-over-gpio is true)
>
> You don't need these. Use gpio_to_irq() instead.
I am passing gpio numbers here and am doing gpio_to_irq() in driver.
Didn't get this one :(
>> Optional properties:
>> - - interrupts : The interrupt outputs from the controller
>> - - interrupt-controller : Marks the device node as an interrupt
>> controller
>> - - interrupt-parent : Specifies which IRQ controller we're
>> connected to
>> - - i2c-client-wake : Marks the input device as wakable
>> - - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128,
>> 256, 512 and 1024
>
> And you've removed these why?
No. They are readjusted...
One thing removed is interrupt-controller. I had a doubt on this.
stmpe, by itself doesn't give any interrupt lines to SoC to freely use
them. Instead
gpio controller driver part of it does. And so adding
interrupt-controller for that is
the right option.
stmpe is an interrupt controller for the IP's which are present inside
it: gpio, adc.
But interrupt lines for them are managed by stmpe driver internally. So should
we really add interrupt-controller for it?
>> +- keypad,scan-count: number of key scanning cycles to confirm key data.
>> Maximum
>> + is STMPE_KEYPAD_MAX_SCAN_COUNT.
>> +- keypad,debounce-ms: debounce interval, in ms. Maximum is
>> + STMPE_KEYPAD_MAX_DEBOUNCE.
>> +- keypad,no-autorepeat: bool, disable key autorepeat
>
> See "When adding new bindings, ask yourself" above.
Yes, these are required. This is part of platform data it expects.
>> +stmpe-ts:
>> +---
> See "When adding new bindings, ask yourself" above.
Same. Can you explicitly point out, which bindings you didn't like.
>> +spi@e010 {
>
> This shouldn't be a child of the SPI device becuase it uses SPI.
>
> Drivers use clocks, regulators, IRQ controller too, but they're
> not children of those devices.
Yes.
>> + reg = <0>;
>
> You have reg twice here. Also reg should never be '0'.
For SPI, there are chip selects and there is no reg offset.
>> + stmpe610-ts {
>> + compatible = "stmpe,ts";
>> + ts,sample-time = <4>;
>> + ts,mod-12b = <1>;
>> + ts,ref-sel = <0>;
>> + ts,adc-freq = <1>;
>> + ts,ave-ctrl = <1>;
>> + ts,touch-det-delay = <2>;
>> + ts,settling = <2>;
>> + ts,fraction-z = <7>;
>> + ts,i-drive = <1>;
>
> Wow! See "When adding new bindings, ask yourself" above.
:)
They are required. I didn't get your point, sorry.
>> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
>> +static struct stmpe_keypad_platform_data *
>> +get_keyboard_pdata_dt(struct device *dev, struct device_node *np)
>> +{
>> + struct stmpe_keypad_platform_data *pdata;
>> + u32 val;
>> +
>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata) {
>> + dev_warn(dev, "stmpe keypad kzalloc fail\n");
>> + return NULL;
>> + }
>> +
>> + if (!of_property_read_u32(np, "keypad,scan-count", ))
>> + pdata->scan_count = val;
>> + if (!of_property_read_u32(np, "keypad,debounce-ms", ))
>> + pdata->debounce_ms = val;
>> + if (of_property_read_bool(np, "keypad,no-autorepeat"))
>> + pdata->no_autorepeat = true;
>
> Why are you (re)adding these here? Have you even looked in the driver?
Because i wanted to keep all DT stuff together. Obviously i have seen keypad
driver earlier :)
I am not setting pdata of stmpe here, but pdata of keypad.
>> +static struct stmpe_gpio_platform_data *get_gpio_pdata_dt(struct device
>> *dev,
>> + struct device_node *np)
>> +{
>> + struct stmpe_gpio_platform_data *pdata;
>> + u32 val;
>> +
>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata) {
>> + dev_warn(dev, "stmpe gpio kzalloc fail\n");
>> + return NULL;
>> + }
>> +
>> + if (!of_property_read_u32(np, "gpio,norequest-mask", ))
>> + pdata->norequest_mask = val;
>> +
>> + /* assign gpio numbers dynamically */
>> + pdata->gpio_base = -1;
>> +
>> + return pdata;
>> +}
>
> Is this function really required? Even if is is, should it live here
> or in the STMPE driver?
As said earlier, either i can
Re: [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 22 November 2012 21:16, Lee Jones wrote: >> >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt >> >> b/Documentation/devicetree/bindings/mfd/stmpe.txt >> >> +- irq-over-gpio: bool, true if gpio is used to get irq >> >> +- irq-gpios: gpio number over which irq will be requested (significant >> >> only if >> >> + irq-over-gpio is true) >> > >> > You don't need these. Use gpio_to_irq() instead. >> >> I am passing gpio numbers here and am doing gpio_to_irq() in driver. >> Didn't get this one :( > > For a start you have 'irq-over-gpio' in the binding document and Device Tree > and 'irq_over_gpio' in the code. Has it even been tested? > > GPIOs are used as IRQ lines in many other previously DT:ed drivers. Take a > look to see how they are handled without adding unnecessary DT bindings. I already knew it, should have picked that up. :( >> stmpe is an interrupt controller for the IP's which are present inside >> it: gpio, adc. >> But interrupt lines for them are managed by stmpe driver internally. So >> should >> we really add interrupt-controller for it? > > You can't manage IRQ lines internally, you have to go through > the IRQ subsystem. When you request an IRQ via device tree you > will do so like this: By that i meant, there is no external node which would have stmpe as interrupt controller. Because all of them would be its child node. This is guaranteed because stmpe is an external device is present on board. So, it will have its entry in board dts file, and so wouldn't be scattered in different files. > The STMPE GPIO controller can't be used by Device Tree yet in any case, > because it doesn't have an IRQ domain. This is compulsory, or it won't > work. Have you tried to test this functionality yet? I don't have SPEAr board to test it anymore. I have moved out of ST now and working in linaro as ARM asignee. Just pushing these as an part time activity. Though ST guys would have tested stmpe, but stmpe-gpio, i am not sure about. > Okay, I've just had a look at my tested bindings. > > We already have these: > > debounce-interval /* This is a generic binding */ > st,scan-count /* These are vendor specific */ > st,no-autorepeat/*" */ > > Vendor specific bindings should be ",", rather than > "," Will take care of these. >> >> + reg = <0>; >> > >> > You have reg twice here. Also reg should never be '0'. >> >> For SPI, there are chip selects and there is no reg offset. > > I understand the addressing issues, but you have 'reg' twice. > > Which one should be used? I haven't replied on that, because it was accepted. Only one definition should be there for reg. > I didn't go through them, but are you sure that: > > 1. Can I do without them? >1.1 Can I derive the configuration from other things? >2.2 Are they _really_ required, or am I just blindly copying platform data? > 2. Does a similar binding already exist? > 3. Can other drivers make use of them? >3.1 If so, create a generic binding >3.2 If not, prepend the binding with "," I will go through them again. >> >> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c >> Because i wanted to keep all DT stuff together. Obviously i have seen keypad >> driver earlier :) > > If you had, you'd realise that these bindings already exist. ;) Ok. I had seen it during my V1 and missed these bindings. will fix them now. -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
> From: Vipul Kumar Samar > > This patch extends existing DT support for stmpe devices. Bindings are > mentioned > in binding document. > > Signed-off-by: Vipul Kumar Samar > Signed-off-by: Viresh Kumar > --- > V1->V2: > -- > - Partial DT support was already there, which i missed earlier. > - Now, this patch is an enhancement of existing DT support. Big fat NACK. You've just overwritten the current implementation with your own. Please take time to understand the mechanisms in place before you submit any changes or additions to it. > Documentation/devicetree/bindings/mfd/stmpe.txt | 127 ++-- > drivers/mfd/stmpe-i2c.c | 23 ++- > drivers/mfd/stmpe-spi.c | 15 ++ > drivers/mfd/stmpe.c | 264 > +--- > 4 files changed, 384 insertions(+), 45 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt > b/Documentation/devicetree/bindings/mfd/stmpe.txt > index 8f0aeda..44aebf3 100644 > --- a/Documentation/devicetree/bindings/mfd/stmpe.txt > +++ b/Documentation/devicetree/bindings/mfd/stmpe.txt > @@ -1,25 +1,124 @@ > -* STMPE Multi-Functional Device > +ST Microelectronics STMPE Multi-Functional Device > +- > > +STMPE is an MFD device which may expose following inbuilt devices: gpio, > keypad, > +touchscreen, adc, pwm, rotator. > + > +stmpe: > +- > Required properties: > - - compatible : "st,stmpe[811|1601|2401|2403]" > - - reg : I2C address of the device > +- compatible: Must be one of: "st,stmpe610", "st,stmpe801", "st,stmpe811", > + "st,stmpe1601", "st,stmpe2401", "st,stmpe2403", This is messy. Use the current format, just add the new versions. > +- id: device id to distinguish between multiple STMPEs on the same board > +- reg: I2C/SPI address of the device Why is this better? Can't you just add "/SPI" to the current string? > +Optional properties: > +- irq-trigger: IRQ trigger to use for the interrupt to the host > +- irq-invert-polarity: bool, IRQ line is connected with reversed polarity Are these new? When adding new bindings, ask yourself: 1. Can I do without them? 1.1 Can I derive the configuration from other things? 2.2 Are they _really_ required, or am I just blindly copying platform data? 2. Does a similar binding already exist? 3. Can other drivers make use of them? 3.1 If so, create a generic binding 3.2 If not, prepend the binding with "," > +- autosleep-timeout: inactivity timeout in milliseconds for autosleep. Valid > + entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024 > +- interrupts: interrupt number of the device, if interrupt is not via gpio > +- interrupt-parent: Specifies which IRQ controller we're connected to > +- irq-over-gpio: bool, true if gpio is used to get irq > +- irq-gpios: gpio number over which irq will be requested (significant only > if What was wrong with the old format (below)? Now it's messy and hard to read. Only provide changes which are _better_. > + irq-over-gpio is true) You don't need these. Use gpio_to_irq() instead. > +- i2c-client-wake: Marks the input device as wakable > +stmpe-keypad: > +-- > +Required properties in addition to those specified by the shared > matrix-keyboard > +bindings: > +- compatible: Must be "stmpe,keypad" That's not how the current implementation works. All you have to do is add child nodes with the names: stmpe_gpio stmpe_keypad stmpe_touchscreen stmpe_adc > Optional properties: > - - interrupts : The interrupt outputs from the controller > - - interrupt-controller : Marks the device node as an interrupt > controller > - - interrupt-parent : Specifies which IRQ controller we're > connected to > - - i2c-client-wake : Marks the input device as wakable > - - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128, > 256, 512 and 1024 And you've removed these why? > +- keypad,scan-count: number of key scanning cycles to confirm key data. > Maximum > + is STMPE_KEYPAD_MAX_SCAN_COUNT. > +- keypad,debounce-ms: debounce interval, in ms. Maximum is > + STMPE_KEYPAD_MAX_DEBOUNCE. > +- keypad,no-autorepeat: bool, disable key autorepeat See "When adding new bindings, ask yourself" above. > +stmpe-gpio: > +--- > +Required properties: > +- compatible: Must be "stmpe,gpio" > + > +Optional properties: > +- norequest-mask: bitmask specifying which GPIOs should _not_ be requestable > due > + to different usage (e.g. touch, keypad) STMPE_GPIO_NOREQ_* macros can be > used > + here. > + > +stmpe-ts: > +--- > +Required properties: > +- compatible: Must be "stmpe,ts" > + > +Optional properties: > +- sample-time: ADC converstion time in number of clock. (0 -> 36 clocks, 1 > -> > + 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks, 5 -> 96 clocks, > 6 > + ->
Re: [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 22 November 2012 16:54, Lee Jones wrote: >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt >> b/Documentation/devicetree/bindings/mfd/stmpe.txt >> +Optional properties: >> +- irq-trigger: IRQ trigger to use for the interrupt to the host >> +- irq-invert-polarity: bool, IRQ line is connected with reversed polarity > > Are these new? > > When adding new bindings, ask yourself: I was trying to get information of these two bindings from other sources, but couldn't find. The only other place can be the interrupts field, right? Because interrupts field depends on the interrupt controller, which can be a wide variety of controllers in our case, we can't use that. Also, the cells of interrupts field will have something that is required to be programmed in the interrupt controller and not in the user of IC. But here we are talking about programming stmpe and so these bindings are very much required in stmpe only. Comments?? -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
> > Big fat NACK.
> >
> > You've just overwritten the current implementation with your own.
> > Please take time to understand the mechanisms in place before
> > you submit any changes or additions to it.
>
> :)
>
> My fault. Comments on all overwritten stuff accepted
Okay, great.
> >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt
> >> b/Documentation/devicetree/bindings/mfd/stmpe.txt
> >> +- irq-over-gpio: bool, true if gpio is used to get irq
> >> +- irq-gpios: gpio number over which irq will be requested (significant
> >> only if
> >> + irq-over-gpio is true)
> >
> > You don't need these. Use gpio_to_irq() instead.
>
> I am passing gpio numbers here and am doing gpio_to_irq() in driver.
> Didn't get this one :(
For a start you have 'irq-over-gpio' in the binding document and Device Tree
and 'irq_over_gpio' in the code. Has it even been tested?
GPIOs are used as IRQ lines in many other previously DT:ed drivers. Take a
look to see how they are handled without adding unnecessary DT bindings.
> >> Optional properties:
> >> - - interrupts : The interrupt outputs from the
> >> controller
> >> - - interrupt-controller : Marks the device node as an interrupt
> >> controller
> >> - - interrupt-parent : Specifies which IRQ controller we're
> >> connected to
> >> - - i2c-client-wake : Marks the input device as wakable
> >> - - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128,
> >> 256, 512 and 1024
> >
> > And you've removed these why?
>
> No. They are readjusted...
Please don't readjust them.
They are clear and easily readable in their current state. In the new
implementation they are messy and hard to decipher.
> One thing removed is interrupt-controller. I had a doubt on this.
> stmpe, by itself doesn't give any interrupt lines to SoC to freely use
> them. Instead
> gpio controller driver part of it does. And so adding
> interrupt-controller for that is
> the right option.
That's fine, I don't have an issue with that.
> stmpe is an interrupt controller for the IP's which are present inside
> it: gpio, adc.
> But interrupt lines for them are managed by stmpe driver internally. So should
> we really add interrupt-controller for it?
You can't manage IRQ lines internally, you have to go through
the IRQ subsystem. When you request an IRQ via device tree you
will do so like this:
interrupts = <25 0x1>;
interrupt-parent = <>;
In this example the stmpe-gpio node must advertise itself using
the interrupt-controller property.
The STMPE GPIO controller can't be used by Device Tree yet in any case,
because it doesn't have an IRQ domain. This is compulsory, or it won't
work. Have you tried to test this functionality yet?
> >> +- keypad,scan-count: number of key scanning cycles to confirm key data.
> >> Maximum
> >> + is STMPE_KEYPAD_MAX_SCAN_COUNT.
> >> +- keypad,debounce-ms: debounce interval, in ms. Maximum is
> >> + STMPE_KEYPAD_MAX_DEBOUNCE.
> >> +- keypad,no-autorepeat: bool, disable key autorepeat
> >
> > See "When adding new bindings, ask yourself" above.
>
> Yes, these are required. This is part of platform data it expects.
Okay, I've just had a look at my tested bindings.
We already have these:
debounce-interval /* This is a generic binding */
st,scan-count /* These are vendor specific */
st,no-autorepeat/*" */
Vendor specific bindings should be ",", rather than
","
> >> + reg = <0>;
> >
> > You have reg twice here. Also reg should never be '0'.
>
> For SPI, there are chip selects and there is no reg offset.
I understand the addressing issues, but you have 'reg' twice.
Which one should be used?
> >> + stmpe610-ts {
> >> + compatible = "stmpe,ts";
> >> + ts,sample-time = <4>;
> >> + ts,mod-12b = <1>;
> >> + ts,ref-sel = <0>;
> >> + ts,adc-freq = <1>;
> >> + ts,ave-ctrl = <1>;
> >> + ts,touch-det-delay = <2>;
> >> + ts,settling = <2>;
> >> + ts,fraction-z = <7>;
> >> + ts,i-drive = <1>;
> >
> > Wow! See "When adding new bindings, ask yourself" above.
>
> :)
> They are required. I didn't get your point, sorry.
I didn't go through them, but are you sure that:
1. Can I do without them?
1.1 Can I derive the configuration from other things?
2.2 Are they _really_ required, or am I just blindly copying platform data?
2. Does a similar binding already exist?
3. Can other drivers make use of them?
3.1 If so, create a generic binding
3.2 If not, prepend the binding with ","
There looks like a lot there. So each of those values vary, or can
any of them be hard-coded? Just because they're in platform code,
it doesn't mean they should be in the DT binding. A lot of platform
code is actually crap.
> >> diff --git
[PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
From: Vipul Kumar Samar This patch extends existing DT support for stmpe devices. Bindings are mentioned in binding document. Signed-off-by: Vipul Kumar Samar Signed-off-by: Viresh Kumar --- V1->V2: -- - Partial DT support was already there, which i missed earlier. - Now, this patch is an enhancement of existing DT support. Documentation/devicetree/bindings/mfd/stmpe.txt | 127 ++-- drivers/mfd/stmpe-i2c.c | 23 ++- drivers/mfd/stmpe-spi.c | 15 ++ drivers/mfd/stmpe.c | 264 +--- 4 files changed, 384 insertions(+), 45 deletions(-) diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt index 8f0aeda..44aebf3 100644 --- a/Documentation/devicetree/bindings/mfd/stmpe.txt +++ b/Documentation/devicetree/bindings/mfd/stmpe.txt @@ -1,25 +1,124 @@ -* STMPE Multi-Functional Device +ST Microelectronics STMPE Multi-Functional Device +- +STMPE is an MFD device which may expose following inbuilt devices: gpio, keypad, +touchscreen, adc, pwm, rotator. + +stmpe: +- Required properties: - - compatible : "st,stmpe[811|1601|2401|2403]" - - reg : I2C address of the device +- compatible: Must be one of: "st,stmpe610", "st,stmpe801", "st,stmpe811", + "st,stmpe1601", "st,stmpe2401", "st,stmpe2403", +- id: device id to distinguish between multiple STMPEs on the same board +- reg: I2C/SPI address of the device + +Optional properties: +- irq-trigger: IRQ trigger to use for the interrupt to the host +- irq-invert-polarity: bool, IRQ line is connected with reversed polarity +- autosleep-timeout: inactivity timeout in milliseconds for autosleep. Valid + entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024 +- interrupts: interrupt number of the device, if interrupt is not via gpio +- interrupt-parent: Specifies which IRQ controller we're connected to +- irq-over-gpio: bool, true if gpio is used to get irq +- irq-gpios: gpio number over which irq will be requested (significant only if + irq-over-gpio is true) +- i2c-client-wake: Marks the input device as wakable + +stmpe-keypad: +-- +Required properties in addition to those specified by the shared matrix-keyboard +bindings: +- compatible: Must be "stmpe,keypad" Optional properties: - - interrupts : The interrupt outputs from the controller - - interrupt-controller : Marks the device node as an interrupt controller - - interrupt-parent : Specifies which IRQ controller we're connected to - - i2c-client-wake : Marks the input device as wakable - - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024 +- keypad,scan-count: number of key scanning cycles to confirm key data. Maximum + is STMPE_KEYPAD_MAX_SCAN_COUNT. +- keypad,debounce-ms: debounce interval, in ms. Maximum is + STMPE_KEYPAD_MAX_DEBOUNCE. +- keypad,no-autorepeat: bool, disable key autorepeat + +stmpe-gpio: +--- +Required properties: +- compatible: Must be "stmpe,gpio" + +Optional properties: +- norequest-mask: bitmask specifying which GPIOs should _not_ be requestable due + to different usage (e.g. touch, keypad) STMPE_GPIO_NOREQ_* macros can be used + here. + +stmpe-ts: +--- +Required properties: +- compatible: Must be "stmpe,ts" + +Optional properties: +- sample-time: ADC converstion time in number of clock. (0 -> 36 clocks, 1 -> + 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks, 5 -> 96 clocks, 6 + -> 144 clocks), recommended is 4. +- mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC) +- ref-sel: ADC reference source (0 -> internal reference, 1 -> external + reference) +- adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 -> 6.5 MHz) +- ave-ctrl: Sample average control (0 -> 1 sample, 1 -> 2 samples, 2 -> 4 + samples, 3 -> 8 samples) +- touch-det-delay: Touch detect interrupt delay (0 -> 10 us, 1 -> 50 us, 2 -> + 100 us, 3 -> 500 us, 4-> 1 ms, 5 -> 5 ms, 6 -> 10 ms, 7 -> 50 ms) recommended + is 3 +- settling: Panel driver settling time (0 -> 10 us, 1 -> 100 us, 2 -> 500 us, 3 + -> 1 ms, 4 -> 5 ms, 5 -> 10 ms, 6 for 50 ms, 7 -> 100 ms) recommended is 2 +- fraction-z: Length of the fractional part in z (fraction-z ([0..7]) = Count of + the fractional part) recommended is 7 +- i-drive: current limit value of the touchscreen drivers (0 -> 20 mA typical 35 + mA max, 1 -> 50 mA typical 80 mA max) + +stmpe-adc: +--- +Required properties: +- compatible: Must be "stmpe,adc" + +stmpe-pwm: +--- +Required properties: +- compatible: Must be "stmpe,pwm" + +stmpe-rotator: +--- +Required properties: +- compatible: Must be "stmpe,rotator" Example: +--- +stmpe can be present over i2c or spi bus, so its node would be added as child +node of either of spi or i2c device. +
[PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
From: Vipul Kumar Samar vipulkumar.sa...@st.com This patch extends existing DT support for stmpe devices. Bindings are mentioned in binding document. Signed-off-by: Vipul Kumar Samar vipulkumar.sa...@st.com Signed-off-by: Viresh Kumar viresh.ku...@linaro.org --- V1-V2: -- - Partial DT support was already there, which i missed earlier. - Now, this patch is an enhancement of existing DT support. Documentation/devicetree/bindings/mfd/stmpe.txt | 127 ++-- drivers/mfd/stmpe-i2c.c | 23 ++- drivers/mfd/stmpe-spi.c | 15 ++ drivers/mfd/stmpe.c | 264 +--- 4 files changed, 384 insertions(+), 45 deletions(-) diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt index 8f0aeda..44aebf3 100644 --- a/Documentation/devicetree/bindings/mfd/stmpe.txt +++ b/Documentation/devicetree/bindings/mfd/stmpe.txt @@ -1,25 +1,124 @@ -* STMPE Multi-Functional Device +ST Microelectronics STMPE Multi-Functional Device +- +STMPE is an MFD device which may expose following inbuilt devices: gpio, keypad, +touchscreen, adc, pwm, rotator. + +stmpe: +- Required properties: - - compatible : st,stmpe[811|1601|2401|2403] - - reg : I2C address of the device +- compatible: Must be one of: st,stmpe610, st,stmpe801, st,stmpe811, + st,stmpe1601, st,stmpe2401, st,stmpe2403, +- id: device id to distinguish between multiple STMPEs on the same board +- reg: I2C/SPI address of the device + +Optional properties: +- irq-trigger: IRQ trigger to use for the interrupt to the host +- irq-invert-polarity: bool, IRQ line is connected with reversed polarity +- autosleep-timeout: inactivity timeout in milliseconds for autosleep. Valid + entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024 +- interrupts: interrupt number of the device, if interrupt is not via gpio +- interrupt-parent: Specifies which IRQ controller we're connected to +- irq-over-gpio: bool, true if gpio is used to get irq +- irq-gpios: gpio number over which irq will be requested (significant only if + irq-over-gpio is true) +- i2c-client-wake: Marks the input device as wakable + +stmpe-keypad: +-- +Required properties in addition to those specified by the shared matrix-keyboard +bindings: +- compatible: Must be stmpe,keypad Optional properties: - - interrupts : The interrupt outputs from the controller - - interrupt-controller : Marks the device node as an interrupt controller - - interrupt-parent : Specifies which IRQ controller we're connected to - - i2c-client-wake : Marks the input device as wakable - - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024 +- keypad,scan-count: number of key scanning cycles to confirm key data. Maximum + is STMPE_KEYPAD_MAX_SCAN_COUNT. +- keypad,debounce-ms: debounce interval, in ms. Maximum is + STMPE_KEYPAD_MAX_DEBOUNCE. +- keypad,no-autorepeat: bool, disable key autorepeat + +stmpe-gpio: +--- +Required properties: +- compatible: Must be stmpe,gpio + +Optional properties: +- norequest-mask: bitmask specifying which GPIOs should _not_ be requestable due + to different usage (e.g. touch, keypad) STMPE_GPIO_NOREQ_* macros can be used + here. + +stmpe-ts: +--- +Required properties: +- compatible: Must be stmpe,ts + +Optional properties: +- sample-time: ADC converstion time in number of clock. (0 - 36 clocks, 1 - + 44 clocks, 2 - 56 clocks, 3 - 64 clocks, 4 - 80 clocks, 5 - 96 clocks, 6 + - 144 clocks), recommended is 4. +- mod-12b: ADC Bit mode (0 - 10bit ADC, 1 - 12bit ADC) +- ref-sel: ADC reference source (0 - internal reference, 1 - external + reference) +- adc-freq: ADC Clock speed (0 - 1.625 MHz, 1 - 3.25 MHz, 2 || 3 - 6.5 MHz) +- ave-ctrl: Sample average control (0 - 1 sample, 1 - 2 samples, 2 - 4 + samples, 3 - 8 samples) +- touch-det-delay: Touch detect interrupt delay (0 - 10 us, 1 - 50 us, 2 - + 100 us, 3 - 500 us, 4- 1 ms, 5 - 5 ms, 6 - 10 ms, 7 - 50 ms) recommended + is 3 +- settling: Panel driver settling time (0 - 10 us, 1 - 100 us, 2 - 500 us, 3 + - 1 ms, 4 - 5 ms, 5 - 10 ms, 6 for 50 ms, 7 - 100 ms) recommended is 2 +- fraction-z: Length of the fractional part in z (fraction-z ([0..7]) = Count of + the fractional part) recommended is 7 +- i-drive: current limit value of the touchscreen drivers (0 - 20 mA typical 35 + mA max, 1 - 50 mA typical 80 mA max) + +stmpe-adc: +--- +Required properties: +- compatible: Must be stmpe,adc + +stmpe-pwm: +--- +Required properties: +- compatible: Must be stmpe,pwm + +stmpe-rotator: +--- +Required properties: +- compatible: Must be stmpe,rotator Example: +--- +stmpe can be present over i2c or spi bus, so its node would be added as child +node of either of spi or i2c device. +
Re: [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
Big fat NACK.
You've just overwritten the current implementation with your own.
Please take time to understand the mechanisms in place before
you submit any changes or additions to it.
:)
My fault. Comments on all overwritten stuff accepted
Okay, great.
diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt
b/Documentation/devicetree/bindings/mfd/stmpe.txt
+- irq-over-gpio: bool, true if gpio is used to get irq
+- irq-gpios: gpio number over which irq will be requested (significant
only if
+ irq-over-gpio is true)
You don't need these. Use gpio_to_irq() instead.
I am passing gpio numbers here and am doing gpio_to_irq() in driver.
Didn't get this one :(
For a start you have 'irq-over-gpio' in the binding document and Device Tree
and 'irq_over_gpio' in the code. Has it even been tested?
GPIOs are used as IRQ lines in many other previously DT:ed drivers. Take a
look to see how they are handled without adding unnecessary DT bindings.
Optional properties:
- - interrupts : The interrupt outputs from the
controller
- - interrupt-controller : Marks the device node as an interrupt
controller
- - interrupt-parent : Specifies which IRQ controller we're
connected to
- - i2c-client-wake : Marks the input device as wakable
- - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128,
256, 512 and 1024
And you've removed these why?
No. They are readjusted...
Please don't readjust them.
They are clear and easily readable in their current state. In the new
implementation they are messy and hard to decipher.
One thing removed is interrupt-controller. I had a doubt on this.
stmpe, by itself doesn't give any interrupt lines to SoC to freely use
them. Instead
gpio controller driver part of it does. And so adding
interrupt-controller for that is
the right option.
That's fine, I don't have an issue with that.
stmpe is an interrupt controller for the IP's which are present inside
it: gpio, adc.
But interrupt lines for them are managed by stmpe driver internally. So should
we really add interrupt-controller for it?
You can't manage IRQ lines internally, you have to go through
the IRQ subsystem. When you request an IRQ via device tree you
will do so like this:
interrupts = 25 0x1;
interrupt-parent = stmpe-gpio;
In this example the stmpe-gpio node must advertise itself using
the interrupt-controller property.
The STMPE GPIO controller can't be used by Device Tree yet in any case,
because it doesn't have an IRQ domain. This is compulsory, or it won't
work. Have you tried to test this functionality yet?
+- keypad,scan-count: number of key scanning cycles to confirm key data.
Maximum
+ is STMPE_KEYPAD_MAX_SCAN_COUNT.
+- keypad,debounce-ms: debounce interval, in ms. Maximum is
+ STMPE_KEYPAD_MAX_DEBOUNCE.
+- keypad,no-autorepeat: bool, disable key autorepeat
See When adding new bindings, ask yourself above.
Yes, these are required. This is part of platform data it expects.
Okay, I've just had a look at my tested bindings.
We already have these:
debounce-interval /* This is a generic binding */
st,scan-count /* These are vendor specific */
st,no-autorepeat/* */
Vendor specific bindings should be vendor,binding, rather than
type_of_driver,binding
+ reg = 0;
You have reg twice here. Also reg should never be '0'.
For SPI, there are chip selects and there is no reg offset.
I understand the addressing issues, but you have 'reg' twice.
Which one should be used?
+ stmpe610-ts {
+ compatible = stmpe,ts;
+ ts,sample-time = 4;
+ ts,mod-12b = 1;
+ ts,ref-sel = 0;
+ ts,adc-freq = 1;
+ ts,ave-ctrl = 1;
+ ts,touch-det-delay = 2;
+ ts,settling = 2;
+ ts,fraction-z = 7;
+ ts,i-drive = 1;
Wow! See When adding new bindings, ask yourself above.
:)
They are required. I didn't get your point, sorry.
I didn't go through them, but are you sure that:
1. Can I do without them?
1.1 Can I derive the configuration from other things?
2.2 Are they _really_ required, or am I just blindly copying platform data?
2. Does a similar binding already exist?
3. Can other drivers make use of them?
3.1 If so, create a generic binding
3.2 If not, prepend the binding with vendor,
There looks like a lot there. So each of those values vary, or can
any of them be hard-coded? Just because they're in platform code,
it doesn't mean they should be in the DT binding. A lot of platform
code is actually crap.
diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
+static struct stmpe_keypad_platform_data *
+get_keyboard_pdata_dt(struct device *dev, struct device_node *np)
Re: [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 22 November 2012 16:54, Lee Jones lee.jo...@linaro.org wrote: diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt +Optional properties: +- irq-trigger: IRQ trigger to use for the interrupt to the host +- irq-invert-polarity: bool, IRQ line is connected with reversed polarity Are these new? When adding new bindings, ask yourself: I was trying to get information of these two bindings from other sources, but couldn't find. The only other place can be the interrupts field, right? Because interrupts field depends on the interrupt controller, which can be a wide variety of controllers in our case, we can't use that. Also, the cells of interrupts field will have something that is required to be programmed in the interrupt controller and not in the user of IC. But here we are talking about programming stmpe and so these bindings are very much required in stmpe only. Comments?? -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
From: Vipul Kumar Samar vipulkumar.sa...@st.com This patch extends existing DT support for stmpe devices. Bindings are mentioned in binding document. Signed-off-by: Vipul Kumar Samar vipulkumar.sa...@st.com Signed-off-by: Viresh Kumar viresh.ku...@linaro.org --- V1-V2: -- - Partial DT support was already there, which i missed earlier. - Now, this patch is an enhancement of existing DT support. Big fat NACK. You've just overwritten the current implementation with your own. Please take time to understand the mechanisms in place before you submit any changes or additions to it. Documentation/devicetree/bindings/mfd/stmpe.txt | 127 ++-- drivers/mfd/stmpe-i2c.c | 23 ++- drivers/mfd/stmpe-spi.c | 15 ++ drivers/mfd/stmpe.c | 264 +--- 4 files changed, 384 insertions(+), 45 deletions(-) diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt index 8f0aeda..44aebf3 100644 --- a/Documentation/devicetree/bindings/mfd/stmpe.txt +++ b/Documentation/devicetree/bindings/mfd/stmpe.txt @@ -1,25 +1,124 @@ -* STMPE Multi-Functional Device +ST Microelectronics STMPE Multi-Functional Device +- +STMPE is an MFD device which may expose following inbuilt devices: gpio, keypad, +touchscreen, adc, pwm, rotator. + +stmpe: +- Required properties: - - compatible : st,stmpe[811|1601|2401|2403] - - reg : I2C address of the device +- compatible: Must be one of: st,stmpe610, st,stmpe801, st,stmpe811, + st,stmpe1601, st,stmpe2401, st,stmpe2403, This is messy. Use the current format, just add the new versions. +- id: device id to distinguish between multiple STMPEs on the same board +- reg: I2C/SPI address of the device Why is this better? Can't you just add /SPI to the current string? +Optional properties: +- irq-trigger: IRQ trigger to use for the interrupt to the host +- irq-invert-polarity: bool, IRQ line is connected with reversed polarity Are these new? When adding new bindings, ask yourself: 1. Can I do without them? 1.1 Can I derive the configuration from other things? 2.2 Are they _really_ required, or am I just blindly copying platform data? 2. Does a similar binding already exist? 3. Can other drivers make use of them? 3.1 If so, create a generic binding 3.2 If not, prepend the binding with vendor, +- autosleep-timeout: inactivity timeout in milliseconds for autosleep. Valid + entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024 +- interrupts: interrupt number of the device, if interrupt is not via gpio +- interrupt-parent: Specifies which IRQ controller we're connected to +- irq-over-gpio: bool, true if gpio is used to get irq +- irq-gpios: gpio number over which irq will be requested (significant only if What was wrong with the old format (below)? Now it's messy and hard to read. Only provide changes which are _better_. + irq-over-gpio is true) You don't need these. Use gpio_to_irq() instead. +- i2c-client-wake: Marks the input device as wakable +stmpe-keypad: +-- +Required properties in addition to those specified by the shared matrix-keyboard +bindings: +- compatible: Must be stmpe,keypad That's not how the current implementation works. All you have to do is add child nodes with the names: stmpe_gpio stmpe_keypad stmpe_touchscreen stmpe_adc Optional properties: - - interrupts : The interrupt outputs from the controller - - interrupt-controller : Marks the device node as an interrupt controller - - interrupt-parent : Specifies which IRQ controller we're connected to - - i2c-client-wake : Marks the input device as wakable - - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024 And you've removed these why? +- keypad,scan-count: number of key scanning cycles to confirm key data. Maximum + is STMPE_KEYPAD_MAX_SCAN_COUNT. +- keypad,debounce-ms: debounce interval, in ms. Maximum is + STMPE_KEYPAD_MAX_DEBOUNCE. +- keypad,no-autorepeat: bool, disable key autorepeat See When adding new bindings, ask yourself above. +stmpe-gpio: +--- +Required properties: +- compatible: Must be stmpe,gpio + +Optional properties: +- norequest-mask: bitmask specifying which GPIOs should _not_ be requestable due + to different usage (e.g. touch, keypad) STMPE_GPIO_NOREQ_* macros can be used + here. + +stmpe-ts: +--- +Required properties: +- compatible: Must be stmpe,ts + +Optional properties: +- sample-time: ADC converstion time in number of clock. (0 - 36 clocks, 1 - + 44 clocks, 2 - 56 clocks, 3 - 64 clocks, 4 - 80 clocks, 5 - 96 clocks, 6 + - 144 clocks), recommended is 4. +- mod-12b: ADC Bit mode (0
Re: [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 22 November 2012 21:16, Lee Jones lee.jo...@linaro.org wrote: diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt +- irq-over-gpio: bool, true if gpio is used to get irq +- irq-gpios: gpio number over which irq will be requested (significant only if + irq-over-gpio is true) You don't need these. Use gpio_to_irq() instead. I am passing gpio numbers here and am doing gpio_to_irq() in driver. Didn't get this one :( For a start you have 'irq-over-gpio' in the binding document and Device Tree and 'irq_over_gpio' in the code. Has it even been tested? GPIOs are used as IRQ lines in many other previously DT:ed drivers. Take a look to see how they are handled without adding unnecessary DT bindings. I already knew it, should have picked that up. :( stmpe is an interrupt controller for the IP's which are present inside it: gpio, adc. But interrupt lines for them are managed by stmpe driver internally. So should we really add interrupt-controller for it? You can't manage IRQ lines internally, you have to go through the IRQ subsystem. When you request an IRQ via device tree you will do so like this: By that i meant, there is no external node which would have stmpe as interrupt controller. Because all of them would be its child node. This is guaranteed because stmpe is an external device is present on board. So, it will have its entry in board dts file, and so wouldn't be scattered in different files. The STMPE GPIO controller can't be used by Device Tree yet in any case, because it doesn't have an IRQ domain. This is compulsory, or it won't work. Have you tried to test this functionality yet? I don't have SPEAr board to test it anymore. I have moved out of ST now and working in linaro as ARM asignee. Just pushing these as an part time activity. Though ST guys would have tested stmpe, but stmpe-gpio, i am not sure about. Okay, I've just had a look at my tested bindings. We already have these: debounce-interval /* This is a generic binding */ st,scan-count /* These are vendor specific */ st,no-autorepeat/* */ Vendor specific bindings should be vendor,binding, rather than type_of_driver,binding Will take care of these. + reg = 0; You have reg twice here. Also reg should never be '0'. For SPI, there are chip selects and there is no reg offset. I understand the addressing issues, but you have 'reg' twice. Which one should be used? I haven't replied on that, because it was accepted. Only one definition should be there for reg. I didn't go through them, but are you sure that: 1. Can I do without them? 1.1 Can I derive the configuration from other things? 2.2 Are they _really_ required, or am I just blindly copying platform data? 2. Does a similar binding already exist? 3. Can other drivers make use of them? 3.1 If so, create a generic binding 3.2 If not, prepend the binding with vendor, I will go through them again. diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c Because i wanted to keep all DT stuff together. Obviously i have seen keypad driver earlier :) If you had, you'd realise that these bindings already exist. ;) Ok. I had seen it during my V1 and missed these bindings. will fix them now. -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 22 November 2012 16:54, Lee Jones lee.jo...@linaro.org wrote:
Big fat NACK.
You've just overwritten the current implementation with your own.
Please take time to understand the mechanisms in place before
you submit any changes or additions to it.
:)
My fault. Comments on all overwritten stuff accepted
diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt
b/Documentation/devicetree/bindings/mfd/stmpe.txt
+- irq-over-gpio: bool, true if gpio is used to get irq
+- irq-gpios: gpio number over which irq will be requested (significant only
if
+ irq-over-gpio is true)
You don't need these. Use gpio_to_irq() instead.
I am passing gpio numbers here and am doing gpio_to_irq() in driver.
Didn't get this one :(
Optional properties:
- - interrupts : The interrupt outputs from the controller
- - interrupt-controller : Marks the device node as an interrupt
controller
- - interrupt-parent : Specifies which IRQ controller we're
connected to
- - i2c-client-wake : Marks the input device as wakable
- - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128,
256, 512 and 1024
And you've removed these why?
No. They are readjusted...
One thing removed is interrupt-controller. I had a doubt on this.
stmpe, by itself doesn't give any interrupt lines to SoC to freely use
them. Instead
gpio controller driver part of it does. And so adding
interrupt-controller for that is
the right option.
stmpe is an interrupt controller for the IP's which are present inside
it: gpio, adc.
But interrupt lines for them are managed by stmpe driver internally. So should
we really add interrupt-controller for it?
+- keypad,scan-count: number of key scanning cycles to confirm key data.
Maximum
+ is STMPE_KEYPAD_MAX_SCAN_COUNT.
+- keypad,debounce-ms: debounce interval, in ms. Maximum is
+ STMPE_KEYPAD_MAX_DEBOUNCE.
+- keypad,no-autorepeat: bool, disable key autorepeat
See When adding new bindings, ask yourself above.
Yes, these are required. This is part of platform data it expects.
+stmpe-ts:
+---
See When adding new bindings, ask yourself above.
Same. Can you explicitly point out, which bindings you didn't like.
+spi@e010 {
This shouldn't be a child of the SPI device becuase it uses SPI.
Drivers use clocks, regulators, IRQ controller too, but they're
not children of those devices.
Yes.
+ reg = 0;
You have reg twice here. Also reg should never be '0'.
For SPI, there are chip selects and there is no reg offset.
+ stmpe610-ts {
+ compatible = stmpe,ts;
+ ts,sample-time = 4;
+ ts,mod-12b = 1;
+ ts,ref-sel = 0;
+ ts,adc-freq = 1;
+ ts,ave-ctrl = 1;
+ ts,touch-det-delay = 2;
+ ts,settling = 2;
+ ts,fraction-z = 7;
+ ts,i-drive = 1;
Wow! See When adding new bindings, ask yourself above.
:)
They are required. I didn't get your point, sorry.
diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
+static struct stmpe_keypad_platform_data *
+get_keyboard_pdata_dt(struct device *dev, struct device_node *np)
+{
+ struct stmpe_keypad_platform_data *pdata;
+ u32 val;
+
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_warn(dev, stmpe keypad kzalloc fail\n);
+ return NULL;
+ }
+
+ if (!of_property_read_u32(np, keypad,scan-count, val))
+ pdata-scan_count = val;
+ if (!of_property_read_u32(np, keypad,debounce-ms, val))
+ pdata-debounce_ms = val;
+ if (of_property_read_bool(np, keypad,no-autorepeat))
+ pdata-no_autorepeat = true;
Why are you (re)adding these here? Have you even looked in the driver?
Because i wanted to keep all DT stuff together. Obviously i have seen keypad
driver earlier :)
I am not setting pdata of stmpe here, but pdata of keypad.
+static struct stmpe_gpio_platform_data *get_gpio_pdata_dt(struct device
*dev,
+ struct device_node *np)
+{
+ struct stmpe_gpio_platform_data *pdata;
+ u32 val;
+
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_warn(dev, stmpe gpio kzalloc fail\n);
+ return NULL;
+ }
+
+ if (!of_property_read_u32(np, gpio,norequest-mask, val))
+ pdata-norequest_mask = val;
+
+ /* assign gpio numbers dynamically */
+ pdata-gpio_base = -1;
+
+ return pdata;
+}
Is this function really required? Even if is is, should it live here
or in the STMPE driver?
As said earlier, either i can do DT parsing of sub-modules of stmpe in
their specific files or in stmpe driver itself. Because currently platform
data of those sub-modules is passed from stmpe, i kept them here only.
stmpe
Re: [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On Thu, Nov 22, 2012 at 10:31:48PM +0530, Viresh Kumar wrote: On 22 November 2012 21:16, Lee Jones lee.jo...@linaro.org wrote: The STMPE GPIO controller can't be used by Device Tree yet in any case, because it doesn't have an IRQ domain. This is compulsory, or it won't work. Have you tried to test this functionality yet? I don't have SPEAr board to test it anymore. I have moved out of ST now and working in linaro as ARM asignee. Just pushing these as an part time activity. Though ST guys would have tested stmpe, but stmpe-gpio, i am not sure about. Let me bring some more information here. I totally understand Jones concerns, but the way stmpe (and may be other mfd devices) are handled is this that the parent block (i.e. stmpe) decides on the variants (say by probing device itself) and then prepares associated data for the (probed) variant and creates a platform device for the same. For the interrupts case also, it is stmpe which registers the irq domain. This is because, stmpe driver probes variant and populates its platform data and stmpe-gpio may not be aware of the variant it serves. At the same time, it (stmpe) needs few of the (virtual) interrupts for its internal purpose also. Hence stmpe passes irq_base to the stmpe-gpio driver while allocating and registering irq domain by itself. With this approach we have tested the functionality on SPEAr platform. -- regards Shiraz -- 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 V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
On 22 November 2012 16:54, Lee Jones lee.jo...@linaro.org wrote:
diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt
b/Documentation/devicetree/bindings/mfd/stmpe.txt
stmpe1601: stmpe1601@40 {
+ id = 0;
Don't do this. Device IDs are Linux specific.
Hi Lee,
This is id of the mfd device that we need to pass to mfd_add_device()
and is used in following:
pdev = platform_device_alloc(cell-name, id + cell-id);
This is required when we have multiple instances of MFD device present
on board. How do you want me to handle this ?
--
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/
