* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Jonathan Cameron @ 2013-09-19 5:41 UTC (permalink / raw)
To: Zubair Lutfullah :
Cc: jic23-KWPb1pKIrIJaa/9Udqfwiw,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <20130919052419.GB4363-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
"Zubair Lutfullah :" <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>On Wed, Sep 18, 2013 at 02:58:47PM +0100, Jonathan Cameron wrote:
>> On 18/09/13 12:23, Zubair Lutfullah wrote:
>> >Previously the driver had only one-shot reading functionality.
>> >This patch adds continuous sampling support to the driver.
>> >
>...
>>
>> Very very nearly there. Couple of suggestions in-line.
>> (about 30 seconds work + testing ;)
>
>Thank-you so much. Makes me want to put in more work and finish it :)
>
>>
>> I'm still unsure of why we need 32bit storage in the fifo
>> for the data. I've proposed the changes I'd make to put it in 16 bit
>> storage inline. The fact that the device is working in 32bits
>> is irrelevant given we only have a 12 bit number coming out and
>> it is in 12 least significant bits. I guess there might be a tiny
>> cost in doing the conversion, but you kfifo will be half the size
>> as a result and that seems more likely to a worthwhile gain!
>>
>
>I understand and will make the changes.
>
>> Out of interest, are you testing this with generic_buffer.c?
>> If so, what changes were necessary? Given this driver will not
>> have a trigger it would be nice to update that example code to handle
>> that case if it doesn't already.
>
>I simply remove the lines like goto err_trigger etc. :p
>Sneaky but works. I wasn't sure how to make the code understand its a
>INDIO_BUFFER_HARDWARE..
>But this is a separate discussion..
>
>>
>>
>> >---
>> > drivers/iio/adc/ti_am335x_adc.c | 216
>+++++++++++++++++++++++++++++++++-
>> > include/linux/mfd/ti_am335x_tscadc.h | 9 ++
>> > 2 files changed, 220 insertions(+), 5 deletions(-)
>> >
>> >diff --git a/drivers/iio/adc/ti_am335x_adc.c
>b/drivers/iio/adc/ti_am335x_adc.c
>...
>> >
>> > struct tiadc_device {
>> > struct ti_tscadc_dev *mfd_tscadc;
>> > int channels;
>> > u8 channel_line[8];
>> > u8 channel_step[8];
>> >+ int buffer_en_ch_steps;
>> >+ u32 *data;
>> u16 *data;
>>
>> Also it might actually save memory to simply have a fixed size array
>> of the maximum size used here and avoid the extra allocations for
>> the cost
>> of 16 bytes in here.
>>
>> Hence,
>>
>> u16 data[8];
>> > };
>
>Why data[8]? The length is dynamic.
>This would be valid for the usual one sample per trigger case.
>But here its continuous sampling and the hardware pushes samples
>*quickly*
>Dynamic allocation is needed.
As far as I can see you pull one set of channels into data[] then push that out to the kfifo.
That is repeated lots of times but only up to 8 entries are ever used in this array. If not what is the maximum scanbytes can be?
>
>
>...
>
>> >+static irqreturn_t tiadc_worker_h(int irq, void *private)
>> >+{
>> >+ struct iio_dev *indio_dev = private;
>> >+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+ int i, k, fifo1count, read;
>> >+ u32 *data = adc_dev->data;
>> u16* data = adc_dev->data;
>> >+
>> >+ fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
>> >+ for (k = 0; k < fifo1count; k = k + i) {
>> >+ for (i = 0; i < (indio_dev->scan_bytes)/4; i++) {
>> >+ read = tiadc_readl(adc_dev, REG_FIFO1);
>> >+ data[i] = read & FIFOREAD_DATA_MASK;
i is only ever up to scanbytes / 4.
Hence max of 8 I think?
Now there is a good argument for adding some bulk filling code for the buffers but that is not happening here.
>> //This is a 12 bit number after the mask so will fit just fine into
>16 bits.
>
>Indeed
>...
>> >+
>> >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
>> >+{
>> >+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+ struct iio_buffer *buffer = indio_dev->buffer;
>> >+ unsigned int enb = 0;
>> >+ u8 bit;
>> >+
>> (can drop this if doing the array with adc_dev as suggested above)
>> >+ adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
>> >+ if (adc_dev->data == NULL)
>> >+ return -ENOMEM;
>
>As explained previously. Large array.. This is needed..
>
>...
>> >+{
>> >+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+
>> >+ tiadc_step_config(indio_dev);
>> (can drop this if doing the array withing adc_dev as suggested above)
>> >+ kfree(adc_dev->data);
>> This is also missbalanced with the preenable (which is true of quite
>> a few drivers - one day I'll clean those up!)
>
>Misbalanced? :s
>
>> >+
>> >+ return 0;
>> >+}
>>
>
>Thanks for the review and feedback.
>I'll resend the patches with 16 bit everything and dynamic allocation.
>
>Zubair
>>
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Jonathan Cameron @ 2013-09-19 5:33 UTC (permalink / raw)
To: Zubair Lutfullah :
Cc: Dmitry Torokhov, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <20130919051611.GA4363-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
"Zubair Lutfullah :" <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>On Wed, Sep 18, 2013 at 06:05:12PM +0100, Jonathan Cameron wrote:
>>
>> >> >However in this case such conversion us dangerous. With all but
>IRQ
>> >> >resource managed by the traditional methods they will be released
>> >first
>> >> >with IRQ handler deregistered very last. Therefore if device is
>not
>> >> >properly quiesced IRQ raised during driver unbinding is likely to
>> >> >result
>> >> >in kernel oops.
>> >> >
>> >> >IOW devm_request_irq() is very often evil (it is still useful if
>> >_all_
>> >> >your resources are managed by devm_*).
>> >> >
>> >> >In case of your driver I'd recommend switching to
>> >> >request_irq()/free_irq() instead.
>> >> >
>> >> >Thanks.
>> >>
>> >> Pretty much all resources are devm managed in here
>> >>
>> >>
>>
>>https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/adc/ti_am335x_adc.c?h=togreg&id=7a1aeba7ed0d5a1e83fd5a8ee2a2869430d40347
>> >
>> >
>> >So we are guaranteed that that new kfifo that is being allocated
>just
>> >before we requesting IRQ and will be freed way before we free the
>IRQ
>> >will not be used by the IOTQ handler?
>> >
>> >Thanks.
>> Good point. I forgot about that. The source of interrupts 'should'
>be disabled before the kfifo is freed but I guess perhaps better to
>play it safe. Would take a fair bit of review to be sure that is not
>going to cause grief.
>>
>> A few more devm handlers need writing before it is truly useful here.
>>
>> Thanks for pointing this out.
>>
>> J
>
>If I understand the conclusion correctly, no devm here?
No devm for the irq. The rest are fine.
>
>Zubair
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Zubair Lutfullah : @ 2013-09-19 5:24 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Zubair Lutfullah, jic23-KWPb1pKIrIJaa/9Udqfwiw,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <5239B197.2040807-tko9wxEg+fIOOJlXag/Snyp2UmYkHbXO@public.gmane.org>
On Wed, Sep 18, 2013 at 02:58:47PM +0100, Jonathan Cameron wrote:
> On 18/09/13 12:23, Zubair Lutfullah wrote:
> >Previously the driver had only one-shot reading functionality.
> >This patch adds continuous sampling support to the driver.
> >
...
>
> Very very nearly there. Couple of suggestions in-line.
> (about 30 seconds work + testing ;)
Thank-you so much. Makes me want to put in more work and finish it :)
>
> I'm still unsure of why we need 32bit storage in the fifo
> for the data. I've proposed the changes I'd make to put it in 16 bit
> storage inline. The fact that the device is working in 32bits
> is irrelevant given we only have a 12 bit number coming out and
> it is in 12 least significant bits. I guess there might be a tiny
> cost in doing the conversion, but you kfifo will be half the size
> as a result and that seems more likely to a worthwhile gain!
>
I understand and will make the changes.
> Out of interest, are you testing this with generic_buffer.c?
> If so, what changes were necessary? Given this driver will not
> have a trigger it would be nice to update that example code to handle
> that case if it doesn't already.
I simply remove the lines like goto err_trigger etc. :p
Sneaky but works. I wasn't sure how to make the code understand its a
INDIO_BUFFER_HARDWARE..
But this is a separate discussion..
>
>
> >---
> > drivers/iio/adc/ti_am335x_adc.c | 216 +++++++++++++++++++++++++++++++++-
> > include/linux/mfd/ti_am335x_tscadc.h | 9 ++
> > 2 files changed, 220 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
...
> >
> > struct tiadc_device {
> > struct ti_tscadc_dev *mfd_tscadc;
> > int channels;
> > u8 channel_line[8];
> > u8 channel_step[8];
> >+ int buffer_en_ch_steps;
> >+ u32 *data;
> u16 *data;
>
> Also it might actually save memory to simply have a fixed size array
> of the maximum size used here and avoid the extra allocations for
> the cost
> of 16 bytes in here.
>
> Hence,
>
> u16 data[8];
> > };
Why data[8]? The length is dynamic.
This would be valid for the usual one sample per trigger case.
But here its continuous sampling and the hardware pushes samples
*quickly*
Dynamic allocation is needed.
...
> >+static irqreturn_t tiadc_worker_h(int irq, void *private)
> >+{
> >+ struct iio_dev *indio_dev = private;
> >+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+ int i, k, fifo1count, read;
> >+ u32 *data = adc_dev->data;
> u16* data = adc_dev->data;
> >+
> >+ fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> >+ for (k = 0; k < fifo1count; k = k + i) {
> >+ for (i = 0; i < (indio_dev->scan_bytes)/4; i++) {
> >+ read = tiadc_readl(adc_dev, REG_FIFO1);
> >+ data[i] = read & FIFOREAD_DATA_MASK;
> //This is a 12 bit number after the mask so will fit just fine into 16 bits.
Indeed
...
> >+
> >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
> >+{
> >+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+ struct iio_buffer *buffer = indio_dev->buffer;
> >+ unsigned int enb = 0;
> >+ u8 bit;
> >+
> (can drop this if doing the array with adc_dev as suggested above)
> >+ adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> >+ if (adc_dev->data == NULL)
> >+ return -ENOMEM;
As explained previously. Large array.. This is needed..
...
> >+{
> >+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+
> >+ tiadc_step_config(indio_dev);
> (can drop this if doing the array withing adc_dev as suggested above)
> >+ kfree(adc_dev->data);
> This is also missbalanced with the preenable (which is true of quite
> a few drivers - one day I'll clean those up!)
Misbalanced? :s
> >+
> >+ return 0;
> >+}
>
Thanks for the review and feedback.
I'll resend the patches with 16 bit everything and dynamic allocation.
Zubair
>
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Zubair Lutfullah : @ 2013-09-19 5:16 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Zubair Lutfullah :, linux-iio, linux-input,
linux-kernel, bigeasy, gregkh
In-Reply-To: <c141b8a3-1e5d-406c-bcc4-ef0abec56192@email.android.com>
On Wed, Sep 18, 2013 at 06:05:12PM +0100, Jonathan Cameron wrote:
>
> >> >However in this case such conversion us dangerous. With all but IRQ
> >> >resource managed by the traditional methods they will be released
> >first
> >> >with IRQ handler deregistered very last. Therefore if device is not
> >> >properly quiesced IRQ raised during driver unbinding is likely to
> >> >result
> >> >in kernel oops.
> >> >
> >> >IOW devm_request_irq() is very often evil (it is still useful if
> >_all_
> >> >your resources are managed by devm_*).
> >> >
> >> >In case of your driver I'd recommend switching to
> >> >request_irq()/free_irq() instead.
> >> >
> >> >Thanks.
> >>
> >> Pretty much all resources are devm managed in here
> >>
> >>
> >https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/adc/ti_am335x_adc.c?h=togreg&id=7a1aeba7ed0d5a1e83fd5a8ee2a2869430d40347
> >
> >
> >So we are guaranteed that that new kfifo that is being allocated just
> >before we requesting IRQ and will be freed way before we free the IRQ
> >will not be used by the IOTQ handler?
> >
> >Thanks.
> Good point. I forgot about that. The source of interrupts 'should' be disabled before the kfifo is freed but I guess perhaps better to play it safe. Would take a fair bit of review to be sure that is not going to cause grief.
>
> A few more devm handlers need writing before it is truly useful here.
>
> Thanks for pointing this out.
>
> J
If I understand the conclusion correctly, no devm here?
Zubair
^ permalink raw reply
* RE: [PATCH V2 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-18 23:27 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, olaf@aepfle.de, apw@canonical.com,
jasowang@redhat.com, dan.carpenter@oracle.com,
linux-input@vger.kernel.org, vojtech@suse.cz
In-Reply-To: <20130918210107.GA32320@core.coreip.homeip.net>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Wednesday, September 18, 2013 2:01 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; dan.carpenter@oracle.com; linux-
> input@vger.kernel.org; vojtech@suse.cz
> Subject: Re: [PATCH V2 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> Hi K.Y.,
>
> On Tue, Sep 17, 2013 at 04:26:58PM -0700, K. Y. Srinivasan wrote:
> > Add a new driver to support synthetic keyboard. On the next generation
> > Hyper-V guest firmware, many legacy devices will not be emulated and this
> > driver will be required.
> >
> > I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> > details of the AT keyboard driver. I would also like to thank
> > Dan Carpenter <dan.carpenter@oracle.com> and
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> for their detailed review of
> this
> > driver.
> >
> > I have addressed all the comments of Dan and Dmitry in this version of
> > the patch
>
> This looks much better. Could you tell me if the patch below (on top of
> yours) still works?
>
> Thanks.
Thank you. The code looks much better now. You forgot to initialize the port_data and
after I fixed that everything seems to work as it did before: Here is the patch I used:
> -----Original Message-----
> From: K. Y. Srinivasan [mailto:kys@microsoft.com]
> Sent: Wednesday, September 18, 2013 4:50 PM
> To: KY Srinivasan
> Subject: [PATCH 1/1] Drivers: input: serio: hyper-V: Initialize the port data
> correctly
>
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> drivers/input/serio/hyperv-keyboard.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-
> keyboard.c
> index 401fbdd..aff4152 100644
> --- a/drivers/input/serio/hyperv-keyboard.c
> +++ b/drivers/input/serio/hyperv-keyboard.c
> @@ -351,6 +351,7 @@ static int hv_kbd_probe(struct hv_device *hv_dev,
>
> hv_serio->dev.parent = &hv_dev->device;
> hv_serio->id.type = SERIO_8042_XL;
> + hv_serio->port_data = kbd_dev;
> strlcpy(hv_serio->name, dev_name(&hv_dev->device),
> sizeof(hv_serio->name));
> strlcpy(hv_serio->phys, dev_name(&hv_dev->device),
> --
> 1.7.4.1
Once again; thank you for all your help.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 2/2] uinput: Support injecting multiple events in one write() call
From: Dmitry Torokhov @ 2013-09-18 22:15 UTC (permalink / raw)
To: Ryan Mallon; +Cc: rydberg, carl, linux-input, linux-kernel
In-Reply-To: <523A1CCB.3090201@gmail.com>
Ryan Mallon <rmallon@gmail.com> wrote:
>On 19/09/13 05:48, Dmitry Torokhov wrote:
>
>> Hi Ryan,
>>
>> On Wed, Sep 18, 2013 at 08:55:44AM +1000, Ryan Mallon wrote:
>>> Rework the code in uinput_inject_event so that it matches the code
>in
>>> evdev_write and allows injecting more than one event, or zero
>events.
>>
>> After some thinking I went back to the original version of your
>patch.
>> For justification see 46f49b7a223ac7493e7cf619fb583d11edefc2c2:
>>
>> "When copy_to/from_user fails in the middle of transfer we should not
>> report to the user that read/write partially succeeded but rather
>> report -EFAULT right away, so that application will know that it got
>> its buffers all wrong.
>>
>> If application messed up its buffers we can't trust the data fetched
>> from userspace and successfully written to the device or if data read
>> from the device and transferred to userspace ended up where
>application
>> expected it to end."
>
>
>Okay, so patch 1 is obviously dropped. Do you want me to resend a fixed
>
>version of this one, or have you already modified it?
I took your original version - is does the right thing.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] uinput: Support injecting multiple events in one write() call
From: Ryan Mallon @ 2013-09-18 21:36 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: rydberg, carl, linux-input, linux-kernel
In-Reply-To: <20130918194806.GA26085@core.coreip.homeip.net>
On 19/09/13 05:48, Dmitry Torokhov wrote:
> Hi Ryan,
>
> On Wed, Sep 18, 2013 at 08:55:44AM +1000, Ryan Mallon wrote:
>> Rework the code in uinput_inject_event so that it matches the code in
>> evdev_write and allows injecting more than one event, or zero events.
>
> After some thinking I went back to the original version of your patch.
> For justification see 46f49b7a223ac7493e7cf619fb583d11edefc2c2:
>
> "When copy_to/from_user fails in the middle of transfer we should not
> report to the user that read/write partially succeeded but rather
> report -EFAULT right away, so that application will know that it got
> its buffers all wrong.
>
> If application messed up its buffers we can't trust the data fetched
> from userspace and successfully written to the device or if data read
> from the device and transferred to userspace ended up where application
> expected it to end."
Okay, so patch 1 is obviously dropped. Do you want me to resend a fixed
version of this one, or have you already modified it?
Thanks,
~Ryan
^ permalink raw reply
* Re: [PATCH V2 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dmitry Torokhov @ 2013-09-18 21:01 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: olaf, gregkh, jasowang, linux-kernel, vojtech, linux-input, apw,
devel, dan.carpenter
In-Reply-To: <1379460418-1523-1-git-send-email-kys@microsoft.com>
Hi K.Y.,
On Tue, Sep 17, 2013 at 04:26:58PM -0700, K. Y. Srinivasan wrote:
> Add a new driver to support synthetic keyboard. On the next generation
> Hyper-V guest firmware, many legacy devices will not be emulated and this
> driver will be required.
>
> I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> details of the AT keyboard driver. I would also like to thank
> Dan Carpenter <dan.carpenter@oracle.com> and
> Dmitry Torokhov <dmitry.torokhov@gmail.com> for their detailed review of this
> driver.
>
> I have addressed all the comments of Dan and Dmitry in this version of
> the patch
This looks much better. Could you tell me if the patch below (on top of
yours) still works?
Thanks.
--
Dmitry
Input: hyperv-keyboard - misc changes
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/serio/Kconfig | 3
drivers/input/serio/hyperv-keyboard.c | 378 ++++++++++++++++++---------------
2 files changed, 208 insertions(+), 173 deletions(-)
diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index f3996e7..a5f342e 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -274,4 +274,7 @@ config HYPERV_KEYBOARD
help
Select this option to enable the Hyper-V Keyboard driver.
+ To compile this driver as a module, choose M here: the module will
+ be called hyperv_keyboard.
+
endif
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
index c327a18..401fbdd 100644
--- a/drivers/input/serio/hyperv-keyboard.c
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -10,6 +10,7 @@
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
* more details.
*/
+
#include <linux/init.h>
#include <linux/module.h>
#include <linux/device.h>
@@ -42,20 +43,16 @@ enum synth_kbd_msg_type {
* Basic message structures.
*/
struct synth_kbd_msg_hdr {
- u32 type;
+ __le32 type;
};
struct synth_kbd_msg {
struct synth_kbd_msg_hdr header;
- char data[1]; /* Enclosed message */
+ char data[]; /* Enclosed message */
};
union synth_kbd_version {
- struct {
- u16 minor_version;
- u16 major_version;
- };
- u32 version;
+ __le32 version;
};
/*
@@ -78,8 +75,8 @@ struct synth_kbd_protocol_response {
#define IS_E1 BIT(3)
struct synth_kbd_keystroke {
struct synth_kbd_msg_hdr header;
- u16 make_code;
- u16 reserved0;
+ __le16 make_code;
+ __le16 reserved0;
__le32 info; /* Additional information */
};
@@ -98,58 +95,27 @@ struct synth_kbd_keystroke {
* Represents a keyboard device
*/
struct hv_kbd_dev {
- struct hv_device *device;
+ struct hv_device *hv_dev;
+ struct serio *hv_serio;
struct synth_kbd_protocol_request protocol_req;
struct synth_kbd_protocol_response protocol_resp;
/* Synchronize the request/response if needed */
- struct completion wait_event;
- struct serio *hv_serio;
+ struct completion wait_event;
+ spinlock_t lock; /* protects 'started' field */
+ bool started;
};
-
-static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
-{
- struct hv_kbd_dev *kbd_dev;
- struct serio *hv_serio;
-
- kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
- if (!kbd_dev)
- return NULL;
- hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
- if (hv_serio == NULL) {
- kfree(kbd_dev);
- return NULL;
- }
- hv_serio->id.type = SERIO_8042_XL;
- strlcpy(hv_serio->name, dev_name(&device->device),
- sizeof(hv_serio->name));
- strlcpy(hv_serio->phys, dev_name(&device->device),
- sizeof(hv_serio->phys));
- hv_serio->dev.parent = &device->device;
- kbd_dev->device = device;
- kbd_dev->hv_serio = hv_serio;
- hv_set_drvdata(device, kbd_dev);
- init_completion(&kbd_dev->wait_event);
-
- return kbd_dev;
-}
-
-static void hv_kbd_free_device(struct hv_kbd_dev *device)
-{
- serio_unregister_port(device->hv_serio);
- hv_set_drvdata(device->device, NULL);
- kfree(device);
-}
-
-static void hv_kbd_on_receive(struct hv_device *device,
- struct synth_kbd_msg *msg, u32 msg_length)
+static void hv_kbd_on_receive(struct hv_device *hv_dev,
+ struct synth_kbd_msg *msg, u32 msg_length)
{
- struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
+ struct hv_kbd_dev *kbd_dev = hv_get_drvdata(hv_dev);
struct synth_kbd_keystroke *ks_msg;
- u16 scan_code;
+ unsigned long flags;
+ u32 msg_type = __le32_to_cpu(msg->header.type);
u32 info;
+ u16 scan_code;
- switch (msg->header.type) {
+ switch (msg_type) {
case SYNTH_KBD_PROTOCOL_RESPONSE:
/*
* Validate the information provided by the host.
@@ -158,13 +124,17 @@ static void hv_kbd_on_receive(struct hv_device *device,
* goes away).
*/
if (msg_length < sizeof(struct synth_kbd_protocol_response)) {
- pr_err("Illegal protocol response packet\n");
+ dev_err(&hv_dev->device,
+ "Illegal protocol response packet (len: %d)\n",
+ msg_length);
break;
}
+
memcpy(&kbd_dev->protocol_resp, msg,
sizeof(struct synth_kbd_protocol_response));
complete(&kbd_dev->wait_event);
break;
+
case SYNTH_KBD_EVENT:
/*
* Validate the information provided by the host.
@@ -173,105 +143,122 @@ static void hv_kbd_on_receive(struct hv_device *device,
* goes away).
*/
if (msg_length < sizeof(struct synth_kbd_keystroke)) {
- pr_err("Illegal keyboard event packet\n");
+ dev_err(&hv_dev->device,
+ "Illegal keyboard event packet (len: %d)\n",
+ msg_length);
break;
}
+
ks_msg = (struct synth_kbd_keystroke *)msg;
- scan_code = ks_msg->make_code;
info = __le32_to_cpu(ks_msg->info);
/*
* Inject the information through the serio interrupt.
*/
- if (info & IS_E0)
- serio_interrupt(kbd_dev->hv_serio, XTKBD_EMUL0, 0);
- serio_interrupt(kbd_dev->hv_serio,
- scan_code | ((info & IS_BREAK) ?
- XTKBD_RELEASE : 0),
- 0);
+ spin_lock_irqsave(&kbd_dev->lock, flags);
+ if (kbd_dev->started) {
+ if (info & IS_E0)
+ serio_interrupt(kbd_dev->hv_serio,
+ XTKBD_EMUL0, 0);
+
+ scan_code = __le16_to_cpu(ks_msg->make_code);
+ if (info & IS_BREAK)
+ scan_code |= XTKBD_RELEASE;
+
+ serio_interrupt(kbd_dev->hv_serio, scan_code, 0);
+ }
+ spin_unlock_irqrestore(&kbd_dev->lock, flags);
+ break;
+
+ default:
+ dev_err(&hv_dev->device,
+ "unhandled message type %d\n", msg_type);
+ }
+}
+
+static void hv_kbd_handle_received_packet(struct hv_device *hv_dev,
+ struct vmpacket_descriptor *desc,
+ u32 bytes_recvd,
+ u64 req_id)
+{
+ struct synth_kbd_msg *msg;
+ u32 msg_sz;
+
+ switch (desc->type) {
+ case VM_PKT_COMP:
+ break;
+
+ case VM_PKT_DATA_INBAND:
+ /*
+ * We have a packet that has "inband" data. The API used
+ * for retrieving the packet guarantees that the complete
+ * packet is read. So, minimally, we should be able to
+ * parse the payload header safely (assuming that the host
+ * can be trusted. Trusting the host seems to be a
+ * reasonable assumption because in a virtualized
+ * environment there is not whole lot you can do if you
+ * don't trust the host.
+ *
+ * Nonetheless, let us validate if the host can be trusted
+ * (in a trivial way). The interesting aspect of this
+ * validation is how do you recover if we discover that the
+ * host is not to be trusted? Simply dropping the packet, I
+ * don't think is an appropriate recovery. In the interest
+ * of failing fast, it may be better to crash the guest.
+ * For now, I will just drop the packet!
+ */
+
+ msg_sz = bytes_recvd - (desc->offset8 << 3);
+ if (msg_sz <= sizeof(struct synth_kbd_msg_hdr)) {
+ /*
+ * Drop the packet and hope
+ * the problem magically goes away.
+ */
+ dev_err(&hv_dev->device,
+ "Illegal packet (type: %d, tid: %llx, size: %d)\n",
+ desc->type, req_id, msg_sz);
+ break;
+ }
+
+ msg = (void *)desc + (desc->offset8 << 3);
+ hv_kbd_on_receive(hv_dev, msg, msg_sz);
break;
+
default:
- pr_err("unhandled message type %d\n", msg->header.type);
+ dev_err(&hv_dev->device,
+ "unhandled packet type %d, tid %llx len %d\n",
+ desc->type, req_id, bytes_recvd);
+ break;
}
}
static void hv_kbd_on_channel_callback(void *context)
{
- const int packet_size = 0x100;
- int ret;
- struct hv_device *device = context;
+ struct hv_device *hv_dev = context;
+ void *buffer;
+ int bufferlen = 0x100; /* Start with sensible size */
u32 bytes_recvd;
u64 req_id;
- struct vmpacket_descriptor *desc;
- struct synth_kbd_msg *msg;
- int hdr_sz = sizeof(struct synth_kbd_msg);
- int msg_sz;
- unsigned char *buffer;
- int bufferlen = packet_size;
+ int error;
buffer = kmalloc(bufferlen, GFP_ATOMIC);
if (!buffer)
return;
while (1) {
- ret = vmbus_recvpacket_raw(device->channel, buffer,
- bufferlen, &bytes_recvd, &req_id);
-
- switch (ret) {
+ error = vmbus_recvpacket_raw(hv_dev->channel, buffer, bufferlen,
+ &bytes_recvd, &req_id);
+ switch (error) {
case 0:
if (bytes_recvd == 0) {
kfree(buffer);
return;
}
- desc = (struct vmpacket_descriptor *)buffer;
- switch (desc->type) {
- case VM_PKT_COMP:
- break;
- case VM_PKT_DATA_INBAND:
- /*
- * We have a packet that has "inband"
- * data. The API used for retrieving the
- * packet guarantees that the complete
- * packet is read. So, minimally, we should
- * be able to parse the payload header
- * safely (assuming that the host can be
- * trusted. Trusting the host seems to be a
- * reasonable assumption because in a
- * virtualized environment there is not whole
- * lot you can do if you don't trust the host.
- *
- * Nonetheless, let us validate if the host can
- * be trusted (in a trivial way).
- * The intresting aspect of this
- * validation is how do you recover if we
- * discover that the host is not to be
- * trusted? Simply dropping the packet, I
- * don't think is an appropriate recovery.
- * In the interest of failing fast, it may
- * be better to crash the guest. For now,
- * I will just drop the packet!
- */
- msg_sz = bytes_recvd - (desc->offset8 << 3);
- if (msg_sz < hdr_sz) {
- /*
- * Drop the packet and hope
- * the problem magically goes away.
- */
- pr_err("Illegal packet\n");
- break;
- }
-
- msg = (struct synth_kbd_msg *)
- ((unsigned long)desc +
- (desc->offset8 << 3));
- hv_kbd_on_receive(device, msg, (u32)msg_sz);
- break;
- default:
- pr_err("unhandled packet type %d, tid %llx len %d\n",
- desc->type, req_id, bytes_recvd);
- break;
- }
+
+ hv_kbd_handle_received_packet(hv_dev, buffer,
+ bytes_recvd, req_id);
break;
+
case -ENOBUFS:
kfree(buffer);
/* Handle large packet */
@@ -284,83 +271,128 @@ static void hv_kbd_on_channel_callback(void *context)
}
}
-static int hv_kbd_connect_to_vsp(struct hv_device *device)
+static int hv_kbd_connect_to_vsp(struct hv_device *hv_dev)
{
- int ret;
- int t;
- u32 proto_status;
- struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
+ struct hv_kbd_dev *kbd_dev = hv_get_drvdata(hv_dev);
struct synth_kbd_protocol_request *request;
struct synth_kbd_protocol_response *response;
+ u32 proto_status;
+ int error;
request = &kbd_dev->protocol_req;
memset(request, 0, sizeof(struct synth_kbd_protocol_request));
- request->header.type = SYNTH_KBD_PROTOCOL_REQUEST;
- request->version_requested.version = SYNTH_KBD_VERSION;
-
- ret = vmbus_sendpacket(device->channel, request,
- sizeof(struct synth_kbd_protocol_request),
- (unsigned long)request,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
- if (ret)
- return ret;
-
- t = wait_for_completion_timeout(&kbd_dev->wait_event, 10 * HZ);
- if (!t)
+ request->header.type = cpu_to_le32(SYNTH_KBD_PROTOCOL_REQUEST);
+ request->version_requested.version = cpu_to_le32(SYNTH_KBD_VERSION);
+
+ error = vmbus_sendpacket(hv_dev->channel, request,
+ sizeof(struct synth_kbd_protocol_request),
+ (unsigned long)request,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ if (error)
+ return error;
+
+ if (!wait_for_completion_timeout(&kbd_dev->wait_event, 10 * HZ))
return -ETIMEDOUT;
response = &kbd_dev->protocol_resp;
proto_status = __le32_to_cpu(response->proto_status);
if (!(proto_status & PROTOCOL_ACCEPTED)) {
- pr_err("synth_kbd protocol request failed (version %d)\n",
- SYNTH_KBD_VERSION);
+ dev_err(&hv_dev->device,
+ "synth_kbd protocol request failed (version %d)\n",
+ SYNTH_KBD_VERSION);
return -ENODEV;
}
+
+ return 0;
+}
+
+static int hv_kbd_start(struct serio *serio)
+{
+ struct hv_kbd_dev *kbd_dev = serio->port_data;
+ unsigned long flags;
+
+ spin_lock_irqsave(&kbd_dev->lock, flags);
+ kbd_dev->started = true;
+ spin_unlock_irqrestore(&kbd_dev->lock, flags);
+
return 0;
}
-static int hv_kbd_probe(struct hv_device *device,
+static void hv_kbd_stop(struct serio *serio)
+{
+ struct hv_kbd_dev *kbd_dev = serio->port_data;
+ unsigned long flags;
+
+ spin_lock_irqsave(&kbd_dev->lock, flags);
+ kbd_dev->started = false;
+ spin_unlock_irqrestore(&kbd_dev->lock, flags);
+}
+
+static int hv_kbd_probe(struct hv_device *hv_dev,
const struct hv_vmbus_device_id *dev_id)
{
- int ret;
struct hv_kbd_dev *kbd_dev;
+ struct serio *hv_serio;
+ int error;
- kbd_dev = hv_kbd_alloc_device(device);
- if (!kbd_dev)
- return -ENOMEM;
- ret = vmbus_open(device->channel,
- KBD_VSC_SEND_RING_BUFFER_SIZE,
- KBD_VSC_RECV_RING_BUFFER_SIZE,
- NULL,
- 0,
- hv_kbd_on_channel_callback,
- device
- );
-
- if (ret)
- goto probe_open_err;
- ret = hv_kbd_connect_to_vsp(device);
- if (ret)
- goto probe_connect_err;
- serio_register_port(kbd_dev->hv_serio);
- return ret;
+ kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
+ hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
+ if (!kbd_dev || !hv_serio) {
+ error = -ENOMEM;
+ goto err_free_mem;
+ }
-probe_connect_err:
- vmbus_close(device->channel);
+ kbd_dev->hv_dev = hv_dev;
+ kbd_dev->hv_serio = hv_serio;
+ spin_lock_init(&kbd_dev->lock);
+ init_completion(&kbd_dev->wait_event);
+ hv_set_drvdata(hv_dev, kbd_dev);
-probe_open_err:
- hv_kbd_free_device(kbd_dev);
+ hv_serio->dev.parent = &hv_dev->device;
+ hv_serio->id.type = SERIO_8042_XL;
+ strlcpy(hv_serio->name, dev_name(&hv_dev->device),
+ sizeof(hv_serio->name));
+ strlcpy(hv_serio->phys, dev_name(&hv_dev->device),
+ sizeof(hv_serio->phys));
+
+ hv_serio->start = hv_kbd_start;
+ hv_serio->stop = hv_kbd_stop;
+
+ error = vmbus_open(hv_dev->channel,
+ KBD_VSC_SEND_RING_BUFFER_SIZE,
+ KBD_VSC_RECV_RING_BUFFER_SIZE,
+ NULL, 0,
+ hv_kbd_on_channel_callback,
+ hv_dev);
+ if (error)
+ goto err_free_mem;
+
+ error = hv_kbd_connect_to_vsp(hv_dev);
+ if (error)
+ goto err_close_vmbus;
- return ret;
+ serio_register_port(kbd_dev->hv_serio);
+ return 0;
+
+err_close_vmbus:
+ vmbus_close(hv_dev->channel);
+err_free_mem:
+ kfree(hv_serio);
+ kfree(kbd_dev);
+ return error;
}
-static int hv_kbd_remove(struct hv_device *dev)
+static int hv_kbd_remove(struct hv_device *hv_dev)
{
- struct hv_kbd_dev *kbd_dev = hv_get_drvdata(dev);
+ struct hv_kbd_dev *kbd_dev = hv_get_drvdata(hv_dev);
+
+ serio_unregister_port(kbd_dev->hv_serio);
+ vmbus_close(hv_dev->channel);
+ kfree(kbd_dev);
+
+ hv_set_drvdata(hv_dev, NULL);
- vmbus_close(dev->channel);
- hv_kbd_free_device(kbd_dev);
return 0;
}
^ permalink raw reply related
* Re: [PATCH 1/3] HID: Delay opening HID device
From: Jonathan Cameron @ 2013-09-18 20:39 UTC (permalink / raw)
To: Srinivas Pandruvada
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, jkosina-AlSwsSmVLrQ,
holler-SXC+2es9fhnfWeYVQQPykw
In-Reply-To: <1379524399-16995-1-git-send-email-srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On 09/18/13 18:13, Srinivas Pandruvada wrote:
> Don't call hid_open_device till there is actually an user. This saves
> power by not opening underlying transport for HID. Also close device
> if there are no active mfd client using HID sensor hub.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
All look fine to me. Ideally I'd like an Ack from Jiri to cover the bit
under drivers/hid/ before taking it through IIO.
Jonathan
> ---
> drivers/hid/hid-sensor-hub.c | 45 ++++++++++++++++++++++++++++++++----------
> include/linux/hid-sensor-hub.h | 18 +++++++++++++++++
> 2 files changed, 53 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 10e1581..88fc5ae 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -465,6 +465,39 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
> return 1;
> }
>
> +int sensor_hub_device_open(struct hid_sensor_hub_device *hsdev)
> +{
> + int ret = 0;
> + struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> +
> + mutex_lock(&data->mutex);
> + if (!hsdev->ref_cnt) {
> + ret = hid_hw_open(hsdev->hdev);
> + if (ret) {
> + hid_err(hsdev->hdev, "failed to open hid device\n");
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> + }
> + hsdev->ref_cnt++;
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(sensor_hub_device_open);
> +
> +void sensor_hub_device_close(struct hid_sensor_hub_device *hsdev)
> +{
> + struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> +
> + mutex_lock(&data->mutex);
> + hsdev->ref_cnt--;
> + if (!hsdev->ref_cnt)
> + hid_hw_close(hsdev->hdev);
> + mutex_unlock(&data->mutex);
> +}
> +EXPORT_SYMBOL_GPL(sensor_hub_device_close);
> +
> static int sensor_hub_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
> {
> @@ -506,12 +539,6 @@ static int sensor_hub_probe(struct hid_device *hdev,
> hid_err(hdev, "hw start failed\n");
> return ret;
> }
> - ret = hid_hw_open(hdev);
> - if (ret) {
> - hid_err(hdev, "failed to open input interrupt pipe\n");
> - goto err_stop_hw;
> - }
> -
> INIT_LIST_HEAD(&sd->dyn_callback_list);
> sd->hid_sensor_client_cnt = 0;
> report_enum = &hdev->report_enum[HID_INPUT_REPORT];
> @@ -520,7 +547,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
> if (dev_cnt > HID_MAX_PHY_DEVICES) {
> hid_err(hdev, "Invalid Physical device count\n");
> ret = -EINVAL;
> - goto err_close;
> + goto err_stop_hw;
> }
> sd->hid_sensor_hub_client_devs = kzalloc(dev_cnt *
> sizeof(struct mfd_cell),
> @@ -528,7 +555,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
> if (sd->hid_sensor_hub_client_devs == NULL) {
> hid_err(hdev, "Failed to allocate memory for mfd cells\n");
> ret = -ENOMEM;
> - goto err_close;
> + goto err_stop_hw;
> }
> list_for_each_entry(report, &report_enum->report_list, list) {
> hid_dbg(hdev, "Report id:%x\n", report->id);
> @@ -565,8 +592,6 @@ err_free_names:
> for (i = 0; i < sd->hid_sensor_client_cnt ; ++i)
> kfree(sd->hid_sensor_hub_client_devs[i].name);
> kfree(sd->hid_sensor_hub_client_devs);
> -err_close:
> - hid_hw_close(hdev);
> err_stop_hw:
> hid_hw_stop(hdev);
>
> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
> index 32ba451..a265af2 100644
> --- a/include/linux/hid-sensor-hub.h
> +++ b/include/linux/hid-sensor-hub.h
> @@ -47,11 +47,13 @@ struct hid_sensor_hub_attribute_info {
> * @hdev: Stores the hid instance.
> * @vendor_id: Vendor id of hub device.
> * @product_id: Product id of hub device.
> + * @ref_cnt: Number of MFD clients have opened this device
> */
> struct hid_sensor_hub_device {
> struct hid_device *hdev;
> u32 vendor_id;
> u32 product_id;
> + int ref_cnt;
> };
>
> /**
> @@ -74,6 +76,22 @@ struct hid_sensor_hub_callbacks {
> void *priv);
> };
>
> +/**
> +* sensor_hub_device_open() - Open hub device
> +* @hsdev: Hub device instance.
> +*
> +* Used to open hid device for sensor hub.
> +*/
> +int sensor_hub_device_open(struct hid_sensor_hub_device *hsdev);
> +
> +/**
> +* sensor_hub_device_clode() - Close hub device
> +* @hsdev: Hub device instance.
> +*
> +* Used to clode hid device for sensor hub.
> +*/
> +void sensor_hub_device_close(struct hid_sensor_hub_device *hsdev);
> +
> /* Registration functions */
>
> /**
>
^ permalink raw reply
* Re: [PATCH] add sur40 driver for Samsung SUR40 (aka MS Surface 2.0/Pixelsense)
From: Dmitry Torokhov @ 2013-09-18 19:49 UTC (permalink / raw)
To: Florian Echtler; +Cc: linux-input, benjamin.tissoires, rydberg, dh.herrmann
In-Reply-To: <1378934763-17938-1-git-send-email-floe@butterbrot.org>
Hi Florian,
On Wed, Sep 11, 2013 at 11:26:03PM +0200, Florian Echtler wrote:
> +
> + /* allocate memory for our device state and initialize it */
> + sur40 = kzalloc(sizeof(struct sur40_state), GFP_KERNEL);
> + poll_dev = input_allocate_polled_device();
> + if (!sur40 || !poll_dev)
> + return -ENOMEM;
You are leaking memory here.
> +
> + /* setup polled input device control struct */
> + poll_dev->private = sur40;
> + poll_dev->poll_interval = POLL_INTERVAL;
> + poll_dev->open = sur40_open;
> + poll_dev->poll = sur40_poll;
> + poll_dev->close = sur40_close;
> +
> + /* setup regular input device struct */
> + sur40_input_setup(poll_dev->input);
> +
> + poll_dev->input->name = "Samsung SUR40";
> + usb_to_input_id(usbdev, &(poll_dev->input->id));
> + usb_make_path(usbdev, sur40->phys, sizeof(sur40->phys));
> + strlcat(sur40->phys, "/input0", sizeof(sur40->phys));
> + poll_dev->input->phys = sur40->phys;
> +
> + sur40->usbdev = usbdev;
> + sur40->input = poll_dev;
> +
> + /* use the bulk-in endpoint tested above */
> + sur40->bulk_in_size = le16_to_cpu(endpoint->wMaxPacketSize);
> + sur40->bulk_in_epaddr = endpoint->bEndpointAddress;
> + sur40->bulk_in_buffer = kmalloc(2 * sur40->bulk_in_size, GFP_KERNEL);
> + if (!sur40->bulk_in_buffer) {
> + dev_err(&interface->dev, "Unable to allocate input buffer.");
> + sur40_delete(sur40);
> + return -ENOMEM;
> + }
> +
> + result = input_register_polled_device(poll_dev);
> + if (result) {
> + dev_err(&interface->dev,
> + "Unable to register polled input device.");
> + sur40_delete(sur40);
> + return result;
> + }
> +
> + /* we can register the device now, as it is ready */
> + usb_set_intfdata(interface, sur40);
> + dev_info(&interface->dev, "%s now attached\n", DRIVER_DESC);
dev_dbg() if you must.
> +
> + return 0;
> +}
> +
> +/* unregister device & clean up */
> +static void sur40_disconnect(struct usb_interface *interface)
> +{
> + struct sur40_state *sur40 = usb_get_intfdata(interface);
> +
> + input_unregister_polled_device(sur40->input);
> +
> + usb_set_intfdata(interface, NULL);
> +
> + sur40_delete(sur40);
> +
> + dev_info(&interface->dev, "%s now disconnected\n", DRIVER_DESC);
Here as well.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] uinput: Support injecting multiple events in one write() call
From: Dmitry Torokhov @ 2013-09-18 19:48 UTC (permalink / raw)
To: Ryan Mallon; +Cc: rydberg, carl, linux-input, linux-kernel
In-Reply-To: <1379458544-6508-2-git-send-email-rmallon@gmail.com>
Hi Ryan,
On Wed, Sep 18, 2013 at 08:55:44AM +1000, Ryan Mallon wrote:
> Rework the code in uinput_inject_event so that it matches the code in
> evdev_write and allows injecting more than one event, or zero events.
After some thinking I went back to the original version of your patch.
For justification see 46f49b7a223ac7493e7cf619fb583d11edefc2c2:
"When copy_to/from_user fails in the middle of transfer we should not
report to the user that read/write partially succeeded but rather
report -EFAULT right away, so that application will know that it got
its buffers all wrong.
If application messed up its buffers we can't trust the data fetched
from userspace and successfully written to the device or if data read
from the device and transferred to userspace ended up where application
expected it to end."
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] add sur40 driver for Samsung SUR40 (aka MS Surface 2.0/Pixelsense)
From: Henrik Rydberg @ 2013-09-18 19:38 UTC (permalink / raw)
To: Florian Echtler
Cc: linux-input, benjamin.tissoires, dmitry.torokhov, dh.herrmann
In-Reply-To: <1378934763-17938-1-git-send-email-floe@butterbrot.org>
Hi Florian,
> This patch adds support for the built-in multitouch sensor in the Samsung
> SUR40 touchscreen device, also known as Microsoft Surface 2.0 or Microsoft
> Pixelsense. Support for raw video output from the sensor as well as the
> accelerometer will be added in a later patch.
>
> Signed-off-by: Florian Echtler <floe@butterbrot.org>
> ---
Thanks for the driver, this is excellent work. Please find some nit-picks inline.
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 515cfe7..99aaf10 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -876,6 +876,16 @@ config TOUCHSCREEN_STMPE
> To compile this driver as a module, choose M here: the
> module will be called stmpe-ts.
>
> +config TOUCHSCREEN_SUR40
> + tristate "Samsung SUR40 (Surface 2.0/PixelSense) touchscreen"
> + depends on USB
> + help
> + Say Y here if you want support for the Samsung SUR40 touchscreen
> + (also known as Microsoft Surface 2.0 or Microsoft PixelSense).
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sur40.
> +
> config TOUCHSCREEN_TPS6507X
> tristate "TPS6507x based touchscreens"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 6bfbeab..b63a25d 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_TOUCHSCREEN_PIXCIR) += pixcir_i2c_ts.o
> obj-$(CONFIG_TOUCHSCREEN_S3C2410) += s3c2410_ts.o
> obj-$(CONFIG_TOUCHSCREEN_ST1232) += st1232.o
> obj-$(CONFIG_TOUCHSCREEN_STMPE) += stmpe-ts.o
> +obj-$(CONFIG_TOUCHSCREEN_SUR40) += sur40.o
> obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC) += ti_am335x_tsc.o
> obj-$(CONFIG_TOUCHSCREEN_TNETV107X) += tnetv107x-ts.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213) += touchit213.o
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> new file mode 100644
> index 0000000..45f6cf4
> --- /dev/null
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -0,0 +1,444 @@
> +/*
> + Surface2.0/SUR40/PixelSense input driver v0.0.1
Versions are best handled in git, so you could simply drop that information here.
> +
> + This program is free software; you can redistribute it and/or
> + modify it under the terms of the GNU General Public License as
> + published by the Free Software Foundation; either version 2 of
> + the License, or (at your option) any later version.
> +
> + Copyright (c) 2013 by Florian 'floe' Echtler <floe@butterbrot.org>
> +
> + Derived from the USB Skeleton driver 1.1,
> + Copyright (c) 2003 Greg Kroah-Hartman (greg@kroah.com)
> +
> + and from the Apple USB BCM5974 multitouch driver,
> + Copyright (c) 2008 Henrik Rydberg (rydberg@euromail.se)
> +
> + and from the generic hid-multitouch driver,
> + Copyright (c) 2010-2012 Stephane Chatty <chatty@enac.fr>
> +*/
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/completion.h>
> +#include <linux/uaccess.h>
> +#include <linux/usb.h>
> +#include <linux/printk.h>
> +#include <linux/input-polldev.h>
> +#include <linux/input/mt.h>
> +#include <linux/usb/input.h>
> +
> +/* read 512 bytes from endpoint 0x86 -> get header + blobs */
> +struct sur40_header {
> +
> + uint16_t type; /* always 0x0001 */
> + uint16_t count; /* count of blobs (if 0: continue prev. packet) */
> +
> + uint32_t packet_id;
> +
> + uint32_t timestamp; /* milliseconds (inc. by 16 or 17 each frame) */
> + uint32_t unknown; /* "epoch?" always 02/03 00 00 00 */
> +};
Since you read these structs directly from the device, it would be
good to see the endianess explicitly. It probably also means this will
only work on some architectures... ;-) Also, you may want to use the
__packed attribute.
> +
> +struct sur40_blob {
> +
> + uint16_t blob_id;
> +
> + uint8_t action; /* 0x02 = enter/exit, 0x03 = update (?) */
> + uint8_t unknown; /* always 0x01 or 0x02 (no idea what this is?) */
> +
> + uint16_t bb_pos_x; /* upper left corner of bounding box */
> + uint16_t bb_pos_y;
> +
> + uint16_t bb_size_x; /* size of bounding box */
> + uint16_t bb_size_y;
> +
> + uint16_t pos_x; /* finger tip position */
> + uint16_t pos_y;
> +
> + uint16_t ctr_x; /* centroid position */
> + uint16_t ctr_y;
> +
> + uint16_t axis_x; /* somehow related to major/minor axis, mostly: */
> + uint16_t axis_y; /* axis_x == bb_size_y && axis_y == bb_size_x */
> +
> + float angle; /* orientation in radians relative to x axis */
float is unusual in the kernel - is it actually ieee 754 encoded?
> + uint32_t area; /* size in pixels/pressure (?) */
> +
> + uint8_t padding[32];
> +};
Same here. Also, you could add a struct like this:
struct sur40_data {
struct sur40_header header;
struct sur40_blob blobs[];
};
which should make conversion easier later on.
> +
> +/* read 8 bytes using control message 0xc0,0xb1,0x00,0x00 */
> +struct sur40_sensors {
> + uint16_t temp;
> + uint16_t acc_x;
> + uint16_t acc_y;
> + uint16_t acc_z;
> +};
Hmm, seems to be unused?
> +
> +/* version information */
> +#define DRIVER_VERSION "0.0.1"
This one can be skipped.
> +#define DRIVER_SHORT "sur40"
> +#define DRIVER_AUTHOR "Florian 'floe' Echtler <floe@butterbrot.org>"
> +#define DRIVER_DESC "Surface2.0/SUR40/PixelSense input driver"
> +
> +/* vendor and device IDs */
> +#define ID_MICROSOFT 0x045e
> +#define ID_SUR40 0x0775
> +
> +/* sensor resolution */
> +#define SENSOR_RES_X 1920
> +#define SENSOR_RES_Y 1080
> +
> +/* touch data endpoint */
> +#define TOUCH_ENDPOINT 0x86
> +
> +/* polling interval (ms) */
> +#define POLL_INTERVAL 10
Interesting that 100 Hz is enough here - is it the limit of the
device, or simply picked out of convenience?
> +
> +/* maximum number of contacts FIXME: this is a guess? */
> +#define MAX_CONTACTS 64
> +
> +/* device ID table */
> +static const struct usb_device_id sur40_table[] = {
> + {USB_DEVICE(ID_MICROSOFT, ID_SUR40)}, /* Samsung SUR40 */
> + {} /* terminating null entry */
> +};
> +
> +/* control commands */
> +#define SUR40_GET_VERSION 0xb0 /* 12 bytes string */
> +#define SUR40_UNKNOWN1 0xb3 /* 5 bytes */
> +#define SUR40_UNKNOWN2 0xc1 /* 24 bytes */
> +
> +#define SUR40_GET_STATE 0xc5 /* 4 bytes state (?) */
> +#define SUR40_GET_SENSORS 0xb1 /* 8 bytes sensors */
> +
> +/*
> + * Note: a earlier, non-public version of this driver used USB_RECIP_ENDPOINT
> + * here by mistake which is very likely to have corrupted the firmware EEPROM
> + * on two separate SUR40 devices. Thanks to Alan Stern who spotted this bug.
> + * Should you ever run into a similar problem, the background story to this
> + * incident and instructions on how to fix the corrupted EEPROM are available
> + * at https://floe.butterbrot.org/matrix/hacking/surface/brick.html
> +*/
> +#define sur40_command(dev, command, index, buffer, size) \
> + usb_control_msg(dev->usbdev, usb_rcvctrlpipe(dev->usbdev, 0), \
> + command, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN, 0x00, \
> + index, buffer, size, 1000)
Good to know. ;-)
> +
> +MODULE_DEVICE_TABLE(usb, sur40_table);
> +
> +/* structure to hold all of our device specific stuff */
> +struct sur40_state {
> +
> + struct usb_device *usbdev; /* save the usb device pointer */
> + struct input_polled_dev *input; /* struct for polled input device */
> +
> + unsigned char *bulk_in_buffer; /* the buffer to receive data */
Could be (struct sur40 *) instead, as suggested above.
> + size_t bulk_in_size; /* the maximum bulk packet size */
> + __u8 bulk_in_epaddr; /* address of the bulk in endpoint */
> +
> + char phys[64]; /* buffer for phys name */
> +};
> +
> +/* debug helper macro */
> +#define get_dev(x) (&(x->usbdev->dev))
> +
> +/* initialization routine, called from sur40_open */
> +static int sur40_init(struct sur40_state *dev)
> +{
> + int result;
> + __u8 buffer[24];
> +
> + /* stupidly replay the original MS driver init sequence */
> + result = sur40_command(dev, SUR40_GET_VERSION, 0x00, buffer, 12);
> + if (result < 0)
> + return result;
> +
> + result = sur40_command(dev, SUR40_GET_VERSION, 0x01, buffer, 12);
> + if (result < 0)
> + return result;
> +
> + result = sur40_command(dev, SUR40_GET_VERSION, 0x02, buffer, 12);
> + if (result < 0)
> + return result;
> +
> + result = sur40_command(dev, SUR40_UNKNOWN2, 0x00, buffer, 24);
> + if (result < 0)
> + return result;
> +
> + result = sur40_command(dev, SUR40_UNKNOWN1, 0x00, buffer, 5);
> + if (result < 0)
> + return result;
> +
> + result = sur40_command(dev, SUR40_GET_VERSION, 0x03, buffer, 12);
> +
> + /* discard the result buffer - no known data inside except
> + some version strings, maybe extract these sometime.. */
> +
> + return result;
> +}
> +
> +/*
> + * callback routines from input_polled_dev
> +*/
> +
> +/* enable the device, polling will now start */
> +void sur40_open(struct input_polled_dev *polldev)
> +{
> + struct sur40_state *sur40 = polldev->private;
> + dev_dbg(get_dev(sur40), "open\n");
> + sur40_init(sur40);
> +}
> +
> +/* disable device, polling has stopped */
> +void sur40_close(struct input_polled_dev *polldev)
> +{
> + /* no known way to stop the device, except to stop polling */
> + struct sur40_state *sur40 = polldev->private;
> + dev_dbg(get_dev(sur40), "close\n");
> +}
> +
> +/*
> + * this function is called when a whole contact has been processed,
> + * so that it can assign it to a slot and store the data there
> + */
> +static void report_blob(struct sur40_blob *blob, struct input_dev *input)
> +{
> + int wide, major, minor;
> +
> + int slotnum = input_mt_get_slot_by_key(input, blob->blob_id);
> + if (slotnum < 0 || slotnum >= MAX_CONTACTS)
> + return;
> +
> + input_mt_slot(input, slotnum);
> + input_mt_report_slot_state(input, MT_TOOL_FINGER, 1);
> + wide = (blob->bb_size_x > blob->bb_size_y);
> + major = max(blob->bb_size_x, blob->bb_size_y);
> + minor = min(blob->bb_size_x, blob->bb_size_y);
> +
> + input_event(input, EV_ABS, ABS_MT_POSITION_X, blob->pos_x);
> + input_event(input, EV_ABS, ABS_MT_POSITION_Y, blob->pos_y);
> + input_event(input, EV_ABS, ABS_MT_TOOL_X, blob->ctr_x);
> + input_event(input, EV_ABS, ABS_MT_TOOL_Y, blob->ctr_y);
> + input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
> + input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
> + input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
> +}
You mention there is an angle in the data, but I take it it is not (yet) useful?
> +
> +/* core function: poll for new input data */
> +void sur40_poll(struct input_polled_dev *polldev)
> +{
> +
> + struct sur40_state *sur40 = polldev->private;
> + struct input_dev *input = polldev->input;
> + int result, bulk_read, need_blobs, packet_blobs, i;
> + uint32_t packet_id;
> +
> + struct sur40_header *header =
> + (struct sur40_header *)(sur40->bulk_in_buffer);
> + struct sur40_blob *inblob =
> + (struct sur40_blob *)(sur40->bulk_in_buffer +
> + sizeof(struct sur40_header));
struct sur40_data * ...
> +
> + need_blobs = -1;
> +
> + dev_dbg(get_dev(sur40), "poll\n");
> +
> + do {
> +
> + /* perform a blocking bulk read to get data from the device */
> + result = usb_bulk_msg(sur40->usbdev,
> + usb_rcvbulkpipe(sur40->usbdev, sur40->bulk_in_epaddr),
> + sur40->bulk_in_buffer, 512, &bulk_read, 1000);
> +
> + dev_dbg(get_dev(sur40), "received %d bytes\n", bulk_read);
> +
> + if (result < 0) {
> + dev_err(get_dev(sur40), "error in usb_bulk_read\n");
> + return;
> + }
> +
> + result = bulk_read - sizeof(struct sur40_header);
> +
> + if (result % sizeof(struct sur40_blob) != 0) {
> + dev_err(get_dev(sur40), "transfer size mismatch\n");
> + return;
> + }
> +
> + /* first packet? */
> + if (need_blobs == -1) {
> + need_blobs = header->count;
> + dev_dbg(get_dev(sur40), "need %d blobs\n", need_blobs);
> + packet_id = header->packet_id;
> + }
> +
> + /* sanity check. when video data is also being retrieved, the
> + * packet ID will usually increase in the middle of a series
> + * instead of at the end. */
> + if (packet_id != header->packet_id)
> + dev_warn(get_dev(sur40), "packet ID mismatch\n");
> +
> + packet_blobs = result / sizeof(struct sur40_blob);
> + dev_dbg(get_dev(sur40), "received %d blobs\n", packet_blobs);
> +
> + /* packets always contain at least 4 blobs, even if empty */
> + if (packet_blobs > need_blobs)
> + packet_blobs = need_blobs;
> +
> + for (i = 0; i < packet_blobs; i++) {
> + need_blobs--;
> + dev_dbg(get_dev(sur40), "processing blob\n");
> + report_blob(&(inblob[i]), input);
> + }
> +
> + } while (need_blobs > 0);
> +
> + input_mt_sync_frame(input);
> + input_sync(input);
> +}
> +
> +/*
> + * housekeeping routines
> +*/
> +
> +/* initialize input device parameters */
> +static void sur40_input_setup(struct input_dev *input_dev)
> +{
> + set_bit(EV_KEY, input_dev->evbit);
> + set_bit(EV_SYN, input_dev->evbit);
> + set_bit(EV_ABS, input_dev->evbit);
> +
> + input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> + 0, SENSOR_RES_X, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> + 0, SENSOR_RES_Y, 0, 0);
> +
> + input_set_abs_params(input_dev, ABS_MT_TOOL_X,
> + 0, SENSOR_RES_X, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_TOOL_Y,
> + 0, SENSOR_RES_Y, 0, 0);
> +
> + /* max value unknown, but major/minor axis
> + * can never be larger than screen */
> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR,
> + 0, SENSOR_RES_X, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR,
> + 0, SENSOR_RES_Y, 0, 0);
> +
> + input_set_abs_params(input_dev, ABS_MT_ORIENTATION, 0, 1, 0, 0);
> +
> + input_mt_init_slots(input_dev, MAX_CONTACTS,
> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> +}
> +
> +/* clean up all allocated buffers/structs */
> +static inline void sur40_delete(struct sur40_state *sur40)
> +{
> + input_free_polled_device(sur40->input);
> + kfree(sur40->bulk_in_buffer);
> + kfree(sur40);
> +}
> +
> +/* check candidate USB interface */
> +static int sur40_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct usb_device *usbdev = interface_to_usbdev(interface);
> + struct sur40_state *sur40;
> + struct usb_host_interface *iface_desc;
> + struct usb_endpoint_descriptor *endpoint;
> + struct input_polled_dev *poll_dev;
> + int result;
> +
> + /* check if we really have the right interface */
> + iface_desc = &interface->altsetting[0];
> + if (iface_desc->desc.bInterfaceClass != 0xFF)
> + return -ENODEV;
> +
> + /* use endpoint #4 (0x86) */
> + endpoint = &iface_desc->endpoint[4].desc;
> + if (endpoint->bEndpointAddress != TOUCH_ENDPOINT)
> + return -ENODEV;
> +
> + /* allocate memory for our device state and initialize it */
> + sur40 = kzalloc(sizeof(struct sur40_state), GFP_KERNEL);
> + poll_dev = input_allocate_polled_device();
> + if (!sur40 || !poll_dev)
> + return -ENOMEM;
> +
> + /* setup polled input device control struct */
> + poll_dev->private = sur40;
> + poll_dev->poll_interval = POLL_INTERVAL;
> + poll_dev->open = sur40_open;
> + poll_dev->poll = sur40_poll;
> + poll_dev->close = sur40_close;
> +
> + /* setup regular input device struct */
> + sur40_input_setup(poll_dev->input);
> +
> + poll_dev->input->name = "Samsung SUR40";
> + usb_to_input_id(usbdev, &(poll_dev->input->id));
> + usb_make_path(usbdev, sur40->phys, sizeof(sur40->phys));
> + strlcat(sur40->phys, "/input0", sizeof(sur40->phys));
> + poll_dev->input->phys = sur40->phys;
> +
> + sur40->usbdev = usbdev;
> + sur40->input = poll_dev;
> +
> + /* use the bulk-in endpoint tested above */
> + sur40->bulk_in_size = le16_to_cpu(endpoint->wMaxPacketSize);
> + sur40->bulk_in_epaddr = endpoint->bEndpointAddress;
> + sur40->bulk_in_buffer = kmalloc(2 * sur40->bulk_in_size, GFP_KERNEL);
The factor of two looks a bit cryptic? There also seem to be functions
to extract the packet size nowadays, if one wants to get fancy.
> + if (!sur40->bulk_in_buffer) {
> + dev_err(&interface->dev, "Unable to allocate input buffer.");
> + sur40_delete(sur40);
> + return -ENOMEM;
> + }
> +
> + result = input_register_polled_device(poll_dev);
> + if (result) {
> + dev_err(&interface->dev,
> + "Unable to register polled input device.");
> + sur40_delete(sur40);
> + return result;
> + }
> +
> + /* we can register the device now, as it is ready */
> + usb_set_intfdata(interface, sur40);
> + dev_info(&interface->dev, "%s now attached\n", DRIVER_DESC);
> +
> + return 0;
> +}
> +
> +/* unregister device & clean up */
> +static void sur40_disconnect(struct usb_interface *interface)
> +{
> + struct sur40_state *sur40 = usb_get_intfdata(interface);
> +
> + input_unregister_polled_device(sur40->input);
> +
> + usb_set_intfdata(interface, NULL);
> +
> + sur40_delete(sur40);
> +
> + dev_info(&interface->dev, "%s now disconnected\n", DRIVER_DESC);
> +}
> +
> +/* usb specific object needed to register this driver with the usb subsystem */
> +static struct usb_driver sur40_driver = {
> + .name = DRIVER_SHORT,
> + .probe = sur40_probe,
> + .disconnect = sur40_disconnect,
> + .id_table = sur40_table,
> +};
> +
> +module_usb_driver(sur40_driver);
> +
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
Thanks!
Henrik
^ permalink raw reply
* Re: [11/51] Input: atmel_mxt_ts - Implement CRC check for configuration data
From: Martin Fuzzey @ 2013-09-18 16:59 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Daniel Kurtz, Henrik Rydberg, Joonyoung Shim,
Alan Bowens, linux-input, linux-kernel, Peter Meerwald,
Benson Leung, Olof Johansson
In-Reply-To: <1372337366-9286-12-git-send-email-nick.dyer@itdev.co.uk>
On 27/06/13 14:48, Nick Dyer wrote:
> The configuration is stored in NVRAM on the maXTouch chip. When the device is
> reset it reports a CRC of the stored configuration values. Therefore it isn't
> necessary to send the configuration on each probe - we can check the CRC
> matches and avoid a timeconsuming backup/reset cycle.
>
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> Acked-by: Benson Leung <bleung@chromium.org>
> -static unsigned mxt_extract_T6_csum(const u8 *csum)
> +static u16 mxt_extract_T6_csum(const u8 *csum)
> {
> return csum[0] | (csum[1] << 8) | (csum[2] << 16);
> }
Shouldn't this be u32?
It's losing data causing the checksums not to match for me.
^ permalink raw reply
* [PATCH 3/3] HID RTC: Open sensor hub open close
From: Srinivas Pandruvada @ 2013-09-18 17:13 UTC (permalink / raw)
To: linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA
Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A, jkosina-AlSwsSmVLrQ,
holler-SXC+2es9fhnfWeYVQQPykw, Srinivas Pandruvada
In-Reply-To: <1379524399-16995-1-git-send-email-srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Open sensor hub when module is loaded and close when module is removed.
This helps saving power by opening HID transport only when there is an
user.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Acked-by: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
---
drivers/rtc/rtc-hid-sensor-time.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index 7273b01..1fe170c 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -279,15 +279,28 @@ static int hid_time_probe(struct platform_device *pdev)
return ret;
}
+ ret = sensor_hub_device_open(hsdev);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to open sensor hub device!\n");
+ goto err_open;
+ }
+
time_state->rtc = devm_rtc_device_register(&pdev->dev,
"hid-sensor-time", &hid_time_rtc_ops,
THIS_MODULE);
if (IS_ERR(time_state->rtc)) {
dev_err(&pdev->dev, "rtc device register failed!\n");
- return PTR_ERR(time_state->rtc);
+ ret = PTR_ERR(time_state->rtc);
+ goto err_rtc;
}
+ return 0;
+
+err_rtc:
+ sensor_hub_device_close(hsdev);
+err_open:
+ sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
return ret;
}
@@ -295,6 +308,7 @@ static int hid_time_remove(struct platform_device *pdev)
{
struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
+ sensor_hub_device_close(hsdev);
sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
return 0;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/3] IIO: call sensor hub open close function
From: Srinivas Pandruvada @ 2013-09-18 17:13 UTC (permalink / raw)
To: linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA
Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A, jkosina-AlSwsSmVLrQ,
holler-SXC+2es9fhnfWeYVQQPykw, Srinivas Pandruvada
In-Reply-To: <1379524399-16995-1-git-send-email-srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Call hid_sensor_hub_device_open when user space opens device and call
hid_sensor_hub_device_close when device is closed. This helps in
saving power.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index 87419c4..b6e77e0 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -34,6 +34,12 @@ static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
struct hid_sensor_common *st = iio_trigger_get_drvdata(trig);
int state_val;
+ if (state) {
+ if (sensor_hub_device_open(st->hsdev))
+ return -EIO;
+ } else
+ sensor_hub_device_close(st->hsdev);
+
state_val = state ? 1 : 0;
if (IS_ENABLED(CONFIG_HID_SENSOR_ENUM_BASE_QUIRKS))
++state_val;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 1/3] HID: Delay opening HID device
From: Srinivas Pandruvada @ 2013-09-18 17:13 UTC (permalink / raw)
To: linux-input, linux-iio; +Cc: jic23, jkosina, holler, Srinivas Pandruvada
Don't call hid_open_device till there is actually an user. This saves
power by not opening underlying transport for HID. Also close device
if there are no active mfd client using HID sensor hub.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/hid/hid-sensor-hub.c | 45 ++++++++++++++++++++++++++++++++----------
include/linux/hid-sensor-hub.h | 18 +++++++++++++++++
2 files changed, 53 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 10e1581..88fc5ae 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -465,6 +465,39 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
return 1;
}
+int sensor_hub_device_open(struct hid_sensor_hub_device *hsdev)
+{
+ int ret = 0;
+ struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
+
+ mutex_lock(&data->mutex);
+ if (!hsdev->ref_cnt) {
+ ret = hid_hw_open(hsdev->hdev);
+ if (ret) {
+ hid_err(hsdev->hdev, "failed to open hid device\n");
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+ }
+ hsdev->ref_cnt++;
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sensor_hub_device_open);
+
+void sensor_hub_device_close(struct hid_sensor_hub_device *hsdev)
+{
+ struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
+
+ mutex_lock(&data->mutex);
+ hsdev->ref_cnt--;
+ if (!hsdev->ref_cnt)
+ hid_hw_close(hsdev->hdev);
+ mutex_unlock(&data->mutex);
+}
+EXPORT_SYMBOL_GPL(sensor_hub_device_close);
+
static int sensor_hub_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
@@ -506,12 +539,6 @@ static int sensor_hub_probe(struct hid_device *hdev,
hid_err(hdev, "hw start failed\n");
return ret;
}
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev, "failed to open input interrupt pipe\n");
- goto err_stop_hw;
- }
-
INIT_LIST_HEAD(&sd->dyn_callback_list);
sd->hid_sensor_client_cnt = 0;
report_enum = &hdev->report_enum[HID_INPUT_REPORT];
@@ -520,7 +547,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
if (dev_cnt > HID_MAX_PHY_DEVICES) {
hid_err(hdev, "Invalid Physical device count\n");
ret = -EINVAL;
- goto err_close;
+ goto err_stop_hw;
}
sd->hid_sensor_hub_client_devs = kzalloc(dev_cnt *
sizeof(struct mfd_cell),
@@ -528,7 +555,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
if (sd->hid_sensor_hub_client_devs == NULL) {
hid_err(hdev, "Failed to allocate memory for mfd cells\n");
ret = -ENOMEM;
- goto err_close;
+ goto err_stop_hw;
}
list_for_each_entry(report, &report_enum->report_list, list) {
hid_dbg(hdev, "Report id:%x\n", report->id);
@@ -565,8 +592,6 @@ err_free_names:
for (i = 0; i < sd->hid_sensor_client_cnt ; ++i)
kfree(sd->hid_sensor_hub_client_devs[i].name);
kfree(sd->hid_sensor_hub_client_devs);
-err_close:
- hid_hw_close(hdev);
err_stop_hw:
hid_hw_stop(hdev);
diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
index 32ba451..a265af2 100644
--- a/include/linux/hid-sensor-hub.h
+++ b/include/linux/hid-sensor-hub.h
@@ -47,11 +47,13 @@ struct hid_sensor_hub_attribute_info {
* @hdev: Stores the hid instance.
* @vendor_id: Vendor id of hub device.
* @product_id: Product id of hub device.
+ * @ref_cnt: Number of MFD clients have opened this device
*/
struct hid_sensor_hub_device {
struct hid_device *hdev;
u32 vendor_id;
u32 product_id;
+ int ref_cnt;
};
/**
@@ -74,6 +76,22 @@ struct hid_sensor_hub_callbacks {
void *priv);
};
+/**
+* sensor_hub_device_open() - Open hub device
+* @hsdev: Hub device instance.
+*
+* Used to open hid device for sensor hub.
+*/
+int sensor_hub_device_open(struct hid_sensor_hub_device *hsdev);
+
+/**
+* sensor_hub_device_clode() - Close hub device
+* @hsdev: Hub device instance.
+*
+* Used to clode hid device for sensor hub.
+*/
+void sensor_hub_device_close(struct hid_sensor_hub_device *hsdev);
+
/* Registration functions */
/**
--
1.8.3.1
^ permalink raw reply related
* Re: [12/51] Input: atmel_mxt_ts - Download device config using firmware loader
From: Martin Fuzzey @ 2013-09-18 17:08 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Daniel Kurtz, Henrik Rydberg, Joonyoung Shim,
Alan Bowens, linux-input, linux-kernel, Peter Meerwald,
Benson Leung, Olof Johansson
In-Reply-To: <1372337366-9286-13-git-send-email-nick.dyer@itdev.co.uk>
On 27/06/13 14:48, Nick Dyer wrote:
> The existing implementation which encodes the configuration as a binary blob
> in platform data is unsatisfactory since it requires a kernel recompile for
> the configuration to be changed, and it doesn't deal well with firmware
> changes that move values around on the chip.
>
> Atmel define an ASCII format for the configuration which can be exported from
> their tools. This patch implements a parser for that format which loads the
> configuration via the firmware loader and sends it to the MXT chip.
I am using the mxt-app tool (v1.13) from
git://github.com/atmel-maxtouch/obp-utils.git
However when an existing config is dumped using that tool's --save
option the
config CRC is always zero.
That means that with this patch the config is loaded every time unless the
dumped file is manually tweaked to set the correct CRC.
>
> ---
> - dev_dbg(dev, "No cfg data defined, skipping reg init\n");
> + ret = request_firmware(&cfg, MXT_CFG_NAME, dev);
>
When building the driver into the kernel this just hangs for 60 seconds
then fails.
Wouldn't it be better to use request_firmware_nowait() ?
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Jonathan Cameron @ 2013-09-18 17:05 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Zubair Lutfullah :, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <20130918162442.GA14803-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org>
Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>On Wed, Sep 18, 2013 at 05:12:02PM +0100, Jonathan Cameron wrote:
>>
>>
>> Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >On Wed, Sep 18, 2013 at 10:39:42AM +0100, Jonathan Cameron wrote:
>> >>
>> >>
>> >> "Zubair Lutfullah :" <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> >On Tue, Sep 17, 2013 at 09:27:27PM -0700, Dmitry Torokhov wrote:
>> >> >> Hi Zubair,
>> >> >>
>> >> >> On Tue, Sep 17, 2013 at 09:44:07AM +0500, Zubair Lutfullah
>wrote:
>> >> >> > +
>> >> >> > + ret = devm_request_threaded_irq(indio_dev->dev.parent,
>> >> >> > + irq,
>> >> >> > + pollfunc_th, pollfunc_bh,
>> >> >> > + flags, indio_dev->name,
>> >> >> > + indio_dev);
>> >> >> > + if (ret)
>> >> >> > + goto error_kfifo_free;
>> >> >> > +
>> >> >> > + indio_dev->setup_ops = setup_ops;
>> >> >> > + indio_dev->modes |= INDIO_BUFFER_HARDWARE;
>> >> >> > +
>> >> >> > + ret = iio_buffer_register(indio_dev,
>> >> >> > + indio_dev->channels,
>> >> >> > + indio_dev->num_channels);
>> >> >> > + if (ret)
>> >> >> > + goto error_free_irq;
>> >> >> > +
>> >> >> > + return 0;
>> >> >> > +
>> >> >> > +error_free_irq:
>> >> >> > + devm_free_irq(indio_dev->dev.parent, irq, indio_dev);
>> >> >>
>> >> >> What is the point of using devm_* here if you are doing
>explicit
>> >> >> management of the resource anyway (you explicitly release it in
>> >all
>> >> >code
>> >> >> paths)?
>> >> >>
>> >> >> Thanks.
>> >> >>
>> >> >> --
>> >> >> Dmitry
>> >> >
>> >> >I admit I am unaware at the moment about how it works.
>> >> >
>> >> >I use devm and simply ignore the error path?
>> >> >
>> >> >The devm function header description said something about using
>> >> >devm_free when freeing. And this is the way I am used to seeing
>> >> >error handling.
>> >>
>> >
>> >> The devm interfaces ensure this is all cleaned when the device is
>> >> removed thus avoiding the need to free the stuff explicitly.
>Device
>> >> will get freed on deliberate remove and on an error from probe.
>Hence
>> >> you can drop all calls to devm free. The devm free functions are
>only
>> >> needed if you wish to free in order to reallocate. This might
>happen
>> >> if you want to change a buffer size for instance.
>> >
>> >However in this case such conversion us dangerous. With all but IRQ
>> >resource managed by the traditional methods they will be released
>first
>> >with IRQ handler deregistered very last. Therefore if device is not
>> >properly quiesced IRQ raised during driver unbinding is likely to
>> >result
>> >in kernel oops.
>> >
>> >IOW devm_request_irq() is very often evil (it is still useful if
>_all_
>> >your resources are managed by devm_*).
>> >
>> >In case of your driver I'd recommend switching to
>> >request_irq()/free_irq() instead.
>> >
>> >Thanks.
>>
>> Pretty much all resources are devm managed in here
>>
>>
>https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/adc/ti_am335x_adc.c?h=togreg&id=7a1aeba7ed0d5a1e83fd5a8ee2a2869430d40347
>
>
>So we are guaranteed that that new kfifo that is being allocated just
>before we requesting IRQ and will be freed way before we free the IRQ
>will not be used by the IOTQ handler?
>
>Thanks.
Good point. I forgot about that. The source of interrupts 'should' be disabled before the kfifo is freed but I guess perhaps better to play it safe. Would take a fair bit of review to be sure that is not going to cause grief.
A few more devm handlers need writing before it is truly useful here.
Thanks for pointing this out.
J
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH 1/3] HID: Delay opening HID device
From: Srinivas Pandruvada @ 2013-09-18 16:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, jkosina-AlSwsSmVLrQ,
holler-SXC+2es9fhnfWeYVQQPykw
In-Reply-To: <523588DE.1000603-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On 09/15/2013 03:15 AM, Jonathan Cameron wrote:
> On 09/10/13 21:03, Srinivas Pandruvada wrote:
>> Don't call hid_open_device till there is actually an user. This saves
>> power by not opening underlying transport for HID. Also close device
>> if there are no active mfd client using HID sensor hub.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Buglet inline.
>> ---
>> drivers/hid/hid-sensor-hub.c | 45 ++++++++++++++++++++++++++++++++----------
>> include/linux/hid-sensor-hub.h | 18 +++++++++++++++++
>> 2 files changed, 53 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
>> index 10e1581..8effdee 100644
>> --- a/drivers/hid/hid-sensor-hub.c
>> +++ b/drivers/hid/hid-sensor-hub.c
>> @@ -465,6 +465,39 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
>> return 1;
>> }
>>
>> +int sensor_hub_device_open(struct hid_sensor_hub_device *hsdev)
>> +{
>> + int ret;
>> + struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
>> +
>> + mutex_lock(&data->mutex);
>> + if (!hsdev->ref_cnt) {
>> + ret = hid_hw_open(hsdev->hdev);
>> + if (ret) {
>> + hid_err(hsdev->hdev, "failed to open hid device\n");
>> + mutex_unlock(&data->mutex);
>> + return ret;
>> + }
>> + }
>> + hsdev->ref_cnt++;
>> + mutex_unlock(&data->mutex);
>> +
>> + return ret;
> Not initialized.
Good catch.
>> +}
>> +EXPORT_SYMBOL_GPL(sensor_hub_device_open);
>> +
>> +void sensor_hub_device_close(struct hid_sensor_hub_device *hsdev)
>> +{
>> + struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
>> +
>> + mutex_lock(&data->mutex);
>> + hsdev->ref_cnt--;
>> + if (!hsdev->ref_cnt)
>> + hid_hw_close(hsdev->hdev);
>> + mutex_unlock(&data->mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(sensor_hub_device_close);
>> +
>> static int sensor_hub_probe(struct hid_device *hdev,
>> const struct hid_device_id *id)
>> {
>> @@ -506,12 +539,6 @@ static int sensor_hub_probe(struct hid_device *hdev,
>> hid_err(hdev, "hw start failed\n");
>> return ret;
>> }
>> - ret = hid_hw_open(hdev);
>> - if (ret) {
>> - hid_err(hdev, "failed to open input interrupt pipe\n");
>> - goto err_stop_hw;
>> - }
>> -
>> INIT_LIST_HEAD(&sd->dyn_callback_list);
>> sd->hid_sensor_client_cnt = 0;
>> report_enum = &hdev->report_enum[HID_INPUT_REPORT];
>> @@ -520,7 +547,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
>> if (dev_cnt > HID_MAX_PHY_DEVICES) {
>> hid_err(hdev, "Invalid Physical device count\n");
>> ret = -EINVAL;
>> - goto err_close;
>> + goto err_stop_hw;
>> }
>> sd->hid_sensor_hub_client_devs = kzalloc(dev_cnt *
>> sizeof(struct mfd_cell),
>> @@ -528,7 +555,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
>> if (sd->hid_sensor_hub_client_devs == NULL) {
>> hid_err(hdev, "Failed to allocate memory for mfd cells\n");
>> ret = -ENOMEM;
>> - goto err_close;
>> + goto err_stop_hw;
>> }
>> list_for_each_entry(report, &report_enum->report_list, list) {
>> hid_dbg(hdev, "Report id:%x\n", report->id);
>> @@ -565,8 +592,6 @@ err_free_names:
>> for (i = 0; i < sd->hid_sensor_client_cnt ; ++i)
>> kfree(sd->hid_sensor_hub_client_devs[i].name);
>> kfree(sd->hid_sensor_hub_client_devs);
>> -err_close:
>> - hid_hw_close(hdev);
>> err_stop_hw:
>> hid_hw_stop(hdev);
>>
>> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
>> index 32ba451..a265af2 100644
>> --- a/include/linux/hid-sensor-hub.h
>> +++ b/include/linux/hid-sensor-hub.h
>> @@ -47,11 +47,13 @@ struct hid_sensor_hub_attribute_info {
>> * @hdev: Stores the hid instance.
>> * @vendor_id: Vendor id of hub device.
>> * @product_id: Product id of hub device.
>> + * @ref_cnt: Number of MFD clients have opened this device
>> */
>> struct hid_sensor_hub_device {
>> struct hid_device *hdev;
>> u32 vendor_id;
>> u32 product_id;
>> + int ref_cnt;
>> };
>>
>> /**
>> @@ -74,6 +76,22 @@ struct hid_sensor_hub_callbacks {
>> void *priv);
>> };
>>
>> +/**
>> +* sensor_hub_device_open() - Open hub device
>> +* @hsdev: Hub device instance.
>> +*
>> +* Used to open hid device for sensor hub.
>> +*/
>> +int sensor_hub_device_open(struct hid_sensor_hub_device *hsdev);
>> +
>> +/**
>> +* sensor_hub_device_clode() - Close hub device
>> +* @hsdev: Hub device instance.
>> +*
>> +* Used to clode hid device for sensor hub.
>> +*/
>> +void sensor_hub_device_close(struct hid_sensor_hub_device *hsdev);
>> +
>> /* Registration functions */
>>
>> /**
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Thanks,
Srinivas
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Dmitry Torokhov @ 2013-09-18 16:24 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Zubair Lutfullah :, linux-iio, linux-input, linux-kernel, bigeasy,
gregkh
In-Reply-To: <b37c9a0e-36ed-49bf-bb68-453c7b07b437@email.android.com>
On Wed, Sep 18, 2013 at 05:12:02PM +0100, Jonathan Cameron wrote:
>
>
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >On Wed, Sep 18, 2013 at 10:39:42AM +0100, Jonathan Cameron wrote:
> >>
> >>
> >> "Zubair Lutfullah :" <zubair.lutfullah@gmail.com> wrote:
> >> >On Tue, Sep 17, 2013 at 09:27:27PM -0700, Dmitry Torokhov wrote:
> >> >> Hi Zubair,
> >> >>
> >> >> On Tue, Sep 17, 2013 at 09:44:07AM +0500, Zubair Lutfullah wrote:
> >> >> > +
> >> >> > + ret = devm_request_threaded_irq(indio_dev->dev.parent,
> >> >> > + irq,
> >> >> > + pollfunc_th, pollfunc_bh,
> >> >> > + flags, indio_dev->name,
> >> >> > + indio_dev);
> >> >> > + if (ret)
> >> >> > + goto error_kfifo_free;
> >> >> > +
> >> >> > + indio_dev->setup_ops = setup_ops;
> >> >> > + indio_dev->modes |= INDIO_BUFFER_HARDWARE;
> >> >> > +
> >> >> > + ret = iio_buffer_register(indio_dev,
> >> >> > + indio_dev->channels,
> >> >> > + indio_dev->num_channels);
> >> >> > + if (ret)
> >> >> > + goto error_free_irq;
> >> >> > +
> >> >> > + return 0;
> >> >> > +
> >> >> > +error_free_irq:
> >> >> > + devm_free_irq(indio_dev->dev.parent, irq, indio_dev);
> >> >>
> >> >> What is the point of using devm_* here if you are doing explicit
> >> >> management of the resource anyway (you explicitly release it in
> >all
> >> >code
> >> >> paths)?
> >> >>
> >> >> Thanks.
> >> >>
> >> >> --
> >> >> Dmitry
> >> >
> >> >I admit I am unaware at the moment about how it works.
> >> >
> >> >I use devm and simply ignore the error path?
> >> >
> >> >The devm function header description said something about using
> >> >devm_free when freeing. And this is the way I am used to seeing
> >> >error handling.
> >>
> >
> >> The devm interfaces ensure this is all cleaned when the device is
> >> removed thus avoiding the need to free the stuff explicitly. Device
> >> will get freed on deliberate remove and on an error from probe. Hence
> >> you can drop all calls to devm free. The devm free functions are only
> >> needed if you wish to free in order to reallocate. This might happen
> >> if you want to change a buffer size for instance.
> >
> >However in this case such conversion us dangerous. With all but IRQ
> >resource managed by the traditional methods they will be released first
> >with IRQ handler deregistered very last. Therefore if device is not
> >properly quiesced IRQ raised during driver unbinding is likely to
> >result
> >in kernel oops.
> >
> >IOW devm_request_irq() is very often evil (it is still useful if _all_
> >your resources are managed by devm_*).
> >
> >In case of your driver I'd recommend switching to
> >request_irq()/free_irq() instead.
> >
> >Thanks.
>
> Pretty much all resources are devm managed in here
>
> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/adc/ti_am335x_adc.c?h=togreg&id=7a1aeba7ed0d5a1e83fd5a8ee2a2869430d40347
So we are guaranteed that that new kfifo that is being allocated just
before we requesting IRQ and will be freed way before we free the IRQ
will not be used by the IOTQ handler?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Jonathan Cameron @ 2013-09-18 16:12 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Zubair Lutfullah :, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <20130918141533.GA16424-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org>
Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>On Wed, Sep 18, 2013 at 10:39:42AM +0100, Jonathan Cameron wrote:
>>
>>
>> "Zubair Lutfullah :" <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >On Tue, Sep 17, 2013 at 09:27:27PM -0700, Dmitry Torokhov wrote:
>> >> Hi Zubair,
>> >>
>> >> On Tue, Sep 17, 2013 at 09:44:07AM +0500, Zubair Lutfullah wrote:
>> >> > +
>> >> > + ret = devm_request_threaded_irq(indio_dev->dev.parent,
>> >> > + irq,
>> >> > + pollfunc_th, pollfunc_bh,
>> >> > + flags, indio_dev->name,
>> >> > + indio_dev);
>> >> > + if (ret)
>> >> > + goto error_kfifo_free;
>> >> > +
>> >> > + indio_dev->setup_ops = setup_ops;
>> >> > + indio_dev->modes |= INDIO_BUFFER_HARDWARE;
>> >> > +
>> >> > + ret = iio_buffer_register(indio_dev,
>> >> > + indio_dev->channels,
>> >> > + indio_dev->num_channels);
>> >> > + if (ret)
>> >> > + goto error_free_irq;
>> >> > +
>> >> > + return 0;
>> >> > +
>> >> > +error_free_irq:
>> >> > + devm_free_irq(indio_dev->dev.parent, irq, indio_dev);
>> >>
>> >> What is the point of using devm_* here if you are doing explicit
>> >> management of the resource anyway (you explicitly release it in
>all
>> >code
>> >> paths)?
>> >>
>> >> Thanks.
>> >>
>> >> --
>> >> Dmitry
>> >
>> >I admit I am unaware at the moment about how it works.
>> >
>> >I use devm and simply ignore the error path?
>> >
>> >The devm function header description said something about using
>> >devm_free when freeing. And this is the way I am used to seeing
>> >error handling.
>>
>
>> The devm interfaces ensure this is all cleaned when the device is
>> removed thus avoiding the need to free the stuff explicitly. Device
>> will get freed on deliberate remove and on an error from probe. Hence
>> you can drop all calls to devm free. The devm free functions are only
>> needed if you wish to free in order to reallocate. This might happen
>> if you want to change a buffer size for instance.
>
>However in this case such conversion us dangerous. With all but IRQ
>resource managed by the traditional methods they will be released first
>with IRQ handler deregistered very last. Therefore if device is not
>properly quiesced IRQ raised during driver unbinding is likely to
>result
>in kernel oops.
>
>IOW devm_request_irq() is very often evil (it is still useful if _all_
>your resources are managed by devm_*).
>
>In case of your driver I'd recommend switching to
>request_irq()/free_irq() instead.
>
>Thanks.
Pretty much all resources are devm managed in here
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/adc/ti_am335x_adc.c?h=togreg&id=7a1aeba7ed0d5a1e83fd5a8ee2a2869430d40347
There is a left over calloc which could be converted but that's it.
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH] Input: i8042 - i8042_flush fix for a full 8042 buffer
From: Andrey Moiseev @ 2013-09-18 15:20 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <20130918144420.GB16424@core.coreip.homeip.net>
Nice. That should be of no interest for the outside world how much
junk 8042 contained, so that's better.
On Wed, Sep 18, 2013 at 6:44 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Andrey,
>
> On Wed, Sep 18, 2013 at 04:35:56PM +0400, Andrey Moiseev wrote:
>> When 8042 internal data buffer is full, the driver
>> erroneously decides that the controller is not present.
>>
>> I've already sent this 2 weeks ago, but that message received no comments.
>>
>
> Sorry about the delay. How about we rework it a bit and make flush
> return success/error instead of number of bytes read, like in the patch
> below?
>
> Thanks.
>
> --
> Dmitry
>
> Input: i8042 - i8042_flush fix for a full 8042 buffer
>
> From: Andrey Moiseev <o2g.org.ru@gmail.com>
>
> When 8042 internal data buffer is full, the driver
> erroneously decides that the controller is not present.
>
> i8042_flush returns the number of flushed bytes, which is
> in 0 - I8042_BUFFER_SIZE range inclusive. Therefore, i8042_flush
> has no way to indicate an error. Moreover i8042_controller_check
> takes initially full buffer (i8042_flush returned
> I8042_BUFFER_SIZE) as a sign of absence of the controller.
>
> Let's change i8042 to return success/error instead and make sure
> we do not return error prematurely.
>
> Signed-off-by: Andrey Moiseev <o2g.org.ru@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/serio/i8042.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index 78e4de4..52c9ebf 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -223,21 +223,26 @@ static int i8042_flush(void)
> {
> unsigned long flags;
> unsigned char data, str;
> - int i = 0;
> + int count = 0;
> + int retval = 0;
>
> spin_lock_irqsave(&i8042_lock, flags);
>
> - while (((str = i8042_read_status()) & I8042_STR_OBF) && (i < I8042_BUFFER_SIZE)) {
> - udelay(50);
> - data = i8042_read_data();
> - i++;
> - dbg("%02x <- i8042 (flush, %s)\n",
> - data, str & I8042_STR_AUXDATA ? "aux" : "kbd");
> + while ((str = i8042_read_status()) & I8042_STR_OBF) {
> + if (count++ < I8042_BUFFER_SIZE) {
> + udelay(50);
> + data = i8042_read_data();
> + dbg("%02x <- i8042 (flush, %s)\n",
> + data, str & I8042_STR_AUXDATA ? "aux" : "kbd");
> + } else {
> + retval = -EIO;
> + break;
> + }
> }
>
> spin_unlock_irqrestore(&i8042_lock, flags);
>
> - return i;
> + return retval;
> }
>
> /*
> @@ -849,7 +854,7 @@ static int __init i8042_check_aux(void)
>
> static int i8042_controller_check(void)
> {
> - if (i8042_flush() == I8042_BUFFER_SIZE) {
> + if (i8042_flush()) {
> pr_err("No controller found\n");
> return -ENODEV;
> }
>
^ permalink raw reply
* Re: [PATCH v2] input: pxa27x_keypad: fix NULL pointer dereference
From: Dmitry Torokhov @ 2013-09-18 15:17 UTC (permalink / raw)
To: Mike Dunn; +Cc: linux-input, Chao Xie, Robert Jarzmik, Marek Vasut
In-Reply-To: <1379516624-3789-1-git-send-email-mikedunn@newsguy.com>
On Wed, Sep 18, 2013 at 08:03:44AM -0700, Mike Dunn wrote:
> A NULL pointer dereference exception occurs in the driver probe function when
> device tree is used. The pdata pointer will be NULL in this case, but the code
> dereferences it in all cases. When device tree is used, a platform data
> structure is allocated and initialized, and in all cases this pointer is copied
> to the driver's private data, so the variable being tested should be accessed
> through the driver's private data structure.
>
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
Applied, thank you.
> ---
> Changelog:
> v2:
> Initialize pdata for DT case, instead of referencing through keypad struct.
>
> drivers/input/keyboard/pxa27x_keypad.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/keyboard/pxa27x_keypad.c b/drivers/input/keyboard/pxa27x_keypad.c
> index 134c3b4..d02bdb2 100644
> --- a/drivers/input/keyboard/pxa27x_keypad.c
> +++ b/drivers/input/keyboard/pxa27x_keypad.c
> @@ -788,8 +788,10 @@ static int pxa27x_keypad_probe(struct platform_device *pdev)
>
> if (pdata)
> error = pxa27x_keypad_build_keycode(keypad);
> - else
> + else {
> error = pxa27x_keypad_build_keycode_from_dt(keypad);
> + pdata = keypad->pdata;
> + }
> if (error) {
> dev_err(&pdev->dev, "failed to build keycode\n");
> goto failed_put_clk;
> --
> 1.8.1.5
>
--
Dmitry
^ permalink raw reply
* [PATCH v2] input: pxa27x_keypad: fix NULL pointer dereference
From: Mike Dunn @ 2013-09-18 15:03 UTC (permalink / raw)
To: linux-input
Cc: Mike Dunn, Dmitry Torokhov, Chao Xie, Robert Jarzmik, Marek Vasut
A NULL pointer dereference exception occurs in the driver probe function when
device tree is used. The pdata pointer will be NULL in this case, but the code
dereferences it in all cases. When device tree is used, a platform data
structure is allocated and initialized, and in all cases this pointer is copied
to the driver's private data, so the variable being tested should be accessed
through the driver's private data structure.
Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
Changelog:
v2:
Initialize pdata for DT case, instead of referencing through keypad struct.
drivers/input/keyboard/pxa27x_keypad.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/pxa27x_keypad.c b/drivers/input/keyboard/pxa27x_keypad.c
index 134c3b4..d02bdb2 100644
--- a/drivers/input/keyboard/pxa27x_keypad.c
+++ b/drivers/input/keyboard/pxa27x_keypad.c
@@ -788,8 +788,10 @@ static int pxa27x_keypad_probe(struct platform_device *pdev)
if (pdata)
error = pxa27x_keypad_build_keycode(keypad);
- else
+ else {
error = pxa27x_keypad_build_keycode_from_dt(keypad);
+ pdata = keypad->pdata;
+ }
if (error) {
dev_err(&pdev->dev, "failed to build keycode\n");
goto failed_put_clk;
--
1.8.1.5
^ permalink raw reply related
* Re: [PATCH] Input: i8042 - i8042_flush fix for a full 8042 buffer
From: Dmitry Torokhov @ 2013-09-18 14:44 UTC (permalink / raw)
To: Andrey Moiseev; +Cc: linux-input, linux-kernel
In-Reply-To: <52399E2C.7000805@gmail.com>
Hi Andrey,
On Wed, Sep 18, 2013 at 04:35:56PM +0400, Andrey Moiseev wrote:
> When 8042 internal data buffer is full, the driver
> erroneously decides that the controller is not present.
>
> I've already sent this 2 weeks ago, but that message received no comments.
>
Sorry about the delay. How about we rework it a bit and make flush
return success/error instead of number of bytes read, like in the patch
below?
Thanks.
--
Dmitry
Input: i8042 - i8042_flush fix for a full 8042 buffer
From: Andrey Moiseev <o2g.org.ru@gmail.com>
When 8042 internal data buffer is full, the driver
erroneously decides that the controller is not present.
i8042_flush returns the number of flushed bytes, which is
in 0 - I8042_BUFFER_SIZE range inclusive. Therefore, i8042_flush
has no way to indicate an error. Moreover i8042_controller_check
takes initially full buffer (i8042_flush returned
I8042_BUFFER_SIZE) as a sign of absence of the controller.
Let's change i8042 to return success/error instead and make sure
we do not return error prematurely.
Signed-off-by: Andrey Moiseev <o2g.org.ru@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/serio/i8042.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 78e4de4..52c9ebf 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -223,21 +223,26 @@ static int i8042_flush(void)
{
unsigned long flags;
unsigned char data, str;
- int i = 0;
+ int count = 0;
+ int retval = 0;
spin_lock_irqsave(&i8042_lock, flags);
- while (((str = i8042_read_status()) & I8042_STR_OBF) && (i < I8042_BUFFER_SIZE)) {
- udelay(50);
- data = i8042_read_data();
- i++;
- dbg("%02x <- i8042 (flush, %s)\n",
- data, str & I8042_STR_AUXDATA ? "aux" : "kbd");
+ while ((str = i8042_read_status()) & I8042_STR_OBF) {
+ if (count++ < I8042_BUFFER_SIZE) {
+ udelay(50);
+ data = i8042_read_data();
+ dbg("%02x <- i8042 (flush, %s)\n",
+ data, str & I8042_STR_AUXDATA ? "aux" : "kbd");
+ } else {
+ retval = -EIO;
+ break;
+ }
}
spin_unlock_irqrestore(&i8042_lock, flags);
- return i;
+ return retval;
}
/*
@@ -849,7 +854,7 @@ static int __init i8042_check_aux(void)
static int i8042_controller_check(void)
{
- if (i8042_flush() == I8042_BUFFER_SIZE) {
+ if (i8042_flush()) {
pr_err("No controller found\n");
return -ENODEV;
}
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox