* 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
* Re: [PATCH] input: pxa27x_keypad: fix NULL pointer dereference
From: Mike Dunn @ 2013-09-18 14:28 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Marek Vasut, linux-input, Chao Xie, Robert Jarzmik
In-Reply-To: <20130918042423.GA13196@core.coreip.homeip.net>
On 09/17/2013 09:24 PM, Dmitry Torokhov wrote:
> On Tue, Sep 17, 2013 at 09:05:54PM -0700, Mike Dunn wrote:
>> On 09/16/2013 10:06 AM, Dmitry Torokhov wrote:
>>> On Mon, Sep 16, 2013 at 06:49:53PM +0200, Marek Vasut wrote:
>>>> Dear Mike Dunn,
>>>>
>>>>> 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>
>>>>> ---
>>>>> drivers/input/keyboard/pxa27x_keypad.c | 6 ++++--
>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/input/keyboard/pxa27x_keypad.c
>>>>> b/drivers/input/keyboard/pxa27x_keypad.c index 134c3b4..3b2a614 100644
>>>>> --- a/drivers/input/keyboard/pxa27x_keypad.c
>>>>> +++ b/drivers/input/keyboard/pxa27x_keypad.c
>>>>> @@ -795,8 +795,10 @@ static int pxa27x_keypad_probe(struct platform_device
>>>>> *pdev) goto failed_put_clk;
>>>>> }
>>>>>
>>>>> - if ((pdata->enable_rotary0 && keypad->rotary_rel_code[0] != -1) ||
>>>>> - (pdata->enable_rotary1 && keypad->rotary_rel_code[1] != -1)) {
>>>>> + if ((keypad->pdata->enable_rotary0 &&
>>>>> + keypad->rotary_rel_code[0] != -1) ||
>>>>> + (keypad->pdata->enable_rotary1 &&
>>>>> + keypad->rotary_rel_code[1] != -1)) {
>>>>> input_dev->evbit[0] |= BIT_MASK(EV_REL);
>>>>> }
>>>>
>>>> Nice find. Acked-by: Marek Vasut <marex@denx.de>
>>>
>>> Excellent booby trap. I would prefer if we explicitly did
>>>
>>> pdata = keypad->pdata;
>>>
>>> after calling the parse DT fucntion with a nice comment, because we
>>> somebody might want to rearrange the code and accidentially revert the
>>> checks to the original state.
>>
>>
>> Yes, that would have been better. Is someone picking this up? I'm not familir
>> with the input subsystem maintainer (sorry).
>
> That would be yours truly.
>
>> If this will be upstreamed in
>> someone's tree, I'll be glad to resubmit with this change.
>
> If you could resubmit that would be great - I do not have the hardware
> and I prefer applying patches that were tested, if possible.
OK, will do. Yes, I'm testing this on a palm treo 680 with matrix keys and one
direct key.
Thanks,
Mike
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Dmitry Torokhov @ 2013-09-18 14:15 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Zubair Lutfullah :, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <53771776-0d33-436d-9687-995ed0d6345d-2ueSQiBKiTY7tOexoI0I+QC/G2K4zDHf@public.gmane.org>
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.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Jonathan Cameron @ 2013-09-18 13:58 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: <1379503383-17086-3-git-send-email-zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
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.
>
> Continuous sampling starts when buffer is enabled.
> HW IRQ wakes worker thread that pushes samples to userspace.
> Sampling stops when buffer is disabled by userspace.
>
> Patil Rachna (TI) laid the ground work for ADC HW register access.
> Russ Dill (TI) fixed bugs in the driver relevant to FIFOs and IRQs.
>
> I fixed channel scanning so multiple ADC channels can be read
> simultaneously and pushed to userspace.
> Restructured the driver to fit IIO ABI.
> And added INDIO_BUFFER_HARDWARE mode.
>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Signed-off-by: Russ Dill <Russ.Dill-l0cyMroinI0@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Acked-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Very very nearly there. Couple of suggestions in-line.
(about 30 seconds work + testing ;)
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!
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.
Thanks,
Jonathan
> ---
> 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
> index a952538..b4b2ea0 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -28,12 +28,19 @@
> #include <linux/iio/driver.h>
>
> #include <linux/mfd/ti_am335x_tscadc.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>
> 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];
> };
>
> static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -56,8 +63,14 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
> return step_en;
> }
>
> -static void tiadc_step_config(struct tiadc_device *adc_dev)
> +static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
> {
> + return 1 << adc_dev->channel_step[chan];
> +}
> +
> +static void tiadc_step_config(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> unsigned int stepconfig;
> int i, steps;
>
> @@ -72,7 +85,11 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
> */
>
> steps = TOTAL_STEPS - adc_dev->channels;
> - stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
> + if (iio_buffer_enabled(indio_dev))
> + stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
> + | STEPCONFIG_MODE_SWCNT;
> + else
> + stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
>
> for (i = 0; i < adc_dev->channels; i++) {
> int chan;
> @@ -85,7 +102,177 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
> adc_dev->channel_step[i] = steps;
> steps++;
> }
> +}
> +
> +static irqreturn_t tiadc_irq_h(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + unsigned int status, config;
> + status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> +
> + /*
> + * ADC and touchscreen share the IRQ line.
> + * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
> + */
> + if (status & IRQENB_FIFO1OVRRUN) {
> + /* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
> + config = tiadc_readl(adc_dev, REG_CTRL);
> + config &= ~(CNTRLREG_TSCSSENB);
> + tiadc_writel(adc_dev, REG_CTRL, config);
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
> + | IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
> + tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
> + return IRQ_HANDLED;
> + } else if (status & IRQENB_FIFO1THRES) {
> + /* Disable irq and wake worker thread */
> + tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
I guess this is necessary given the sharing will make IRQF_ONESHOT
tricky. As disabling the source of the interrupts is nice and
easy here this will do the job.
> + return IRQ_WAKE_THREAD;
> + }
> +
> + return IRQ_NONE;
> +}
> +
> +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.
> + }
> + iio_push_to_buffers(indio_dev, (u8 *) data);
> + }
>
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
> + tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int i, fifo1count, read;
> +
> + tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
> + IRQENB_FIFO1OVRRUN |
> + IRQENB_FIFO1UNDRFLW));
> +
> + /* Flush FIFO. Needed in corner cases in simultaneous tsc/adc use */
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + for (i = 0; i < fifo1count; i++)
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> + return iio_sw_buffer_preenable(indio_dev);
> +}
> +
> +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;
> +
> + tiadc_step_config(indio_dev);
> + for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
> + enb |= (get_adc_step_bit(adc_dev, bit) << 1);
> + adc_dev->buffer_en_ch_steps = enb;
> +
> + am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
> +
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES
> + | IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
> + tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES
> + | IRQENB_FIFO1OVRRUN);
> +
> + return 0;
> +}
> +
> +static int tiadc_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int fifo1count, i, read;
> +
> + tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
> + IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW));
> + am335x_tsc_se_clr(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps);
> +
> + /* Flush FIFO of leftover data in the time it takes to disable adc */
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + for (i = 0; i < fifo1count; i++)
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> + return 0;
> +}
> +
> +static int tiadc_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + 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!)
> +
> + return 0;
> +}
^ permalink raw reply
* [PATCH] Input: i8042 - i8042_flush fix for a full 8042 buffer
From: Andrey Moiseev @ 2013-09-18 12:35 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: linux-input, linux-kernel, Andrey Moiseev
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.
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.
The fix:
i8042_flush should perform one more status check after
I8042_BUFFER_SIZE bytes have been read, and, if it succeeds,
indicate the error by returning -1. i8042_controller_check
should be changed appropriately.
Signed-off-by: Andrey Moiseev <o2g.org.ru@gmail.com>
---
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 78e4de4..cdda786 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -222,29 +222,32 @@ static int i8042_wait_write(void)
static int i8042_flush(void)
{
unsigned long flags;
unsigned char data, str;
int i = 0;
spin_lock_irqsave(&i8042_lock, flags);
- while (((str = i8042_read_status()) & I8042_STR_OBF) && (i < I8042_BUFFER_SIZE)) {
+ 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");
}
spin_unlock_irqrestore(&i8042_lock, flags);
+ if (i == I8042_BUFFER_SIZE + 1)
+ return -1;
+
return i;
}
/*
* i8042_command() executes a command on the i8042. It also sends the input
* parameter(s) of the commands to it, and receives the output value(s). The
* parameters are to be stored in the param array, and the output is placed
* into the same array. The number of the parameters and output values is
* encoded in bits 8-11 of the command number.
*/
@@ -849,11 +852,11 @@ static int __init i8042_check_aux(void)
static int i8042_controller_check(void)
{
- if (i8042_flush() == I8042_BUFFER_SIZE) {
+ if (i8042_flush() == -1) {
pr_err("No controller found\n");
return -ENODEV;
}
return 0;
}
--
1.8.4
^ permalink raw reply related
* [PATCH V10 0/2] iio: input: ti_am335x_adc: Add continuous sampling support
From: Zubair Lutfullah @ 2013-09-18 11:23 UTC (permalink / raw)
To: jic23, dmitry.torokhov
Cc: linux-iio, linux-input, linux-kernel, bigeasy, gregkh,
zubair.lutfullah
Hi,
These apply to togreg branch in iio.
Round 10 updates
- Removed devm_free as not needed
Round 9 updates
- Removed Trigger.
- Restructured everything for INDIO_BUFFERED_HARDWARE mode.
- Threaded IRQ with top/bottom halfs. Top half HW IRQ wakes
bottom half threaded worker that pushes samples to iio/userspace.
- Removed some of the patchwork to the driver that was irrelevant to the
continuous sampling patch.
Cleanup will be done separately once this goes through.
Hope these patches meet the mighty kernel standards :)
Zubair
Zubair Lutfullah (2):
input: ti_am335x_tsc: Enable shared IRQ for TSC
iio: ti_am335x_adc: Add continuous sampling support
drivers/iio/adc/ti_am335x_adc.c | 216 ++++++++++++++++++++++++++++-
drivers/input/touchscreen/ti_am335x_tsc.c | 12 +-
include/linux/mfd/ti_am335x_tscadc.h | 9 ++
3 files changed, 229 insertions(+), 8 deletions(-)
--
1.7.9.5
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Zubair Lutfullah : @ 2013-09-18 11:25 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Zubair Lutfullah :, Dmitry Torokhov,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <53771776-0d33-436d-9687-995ed0d6345d-2ueSQiBKiTY7tOexoI0I+QC/G2K4zDHf@public.gmane.org>
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;
...
> >> > +
> >> > +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
> >> Dmitry
>
> 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.
>
Thank-you for the feedback.
Updated and resent the series.
Zubair
> --
^ permalink raw reply
* [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Zubair Lutfullah @ 2013-09-18 11:23 UTC (permalink / raw)
To: jic23, dmitry.torokhov
Cc: linux-iio, linux-input, linux-kernel, bigeasy, gregkh,
zubair.lutfullah
In-Reply-To: <1379503383-17086-1-git-send-email-zubair.lutfullah@gmail.com>
Previously the driver had only one-shot reading functionality.
This patch adds continuous sampling support to the driver.
Continuous sampling starts when buffer is enabled.
HW IRQ wakes worker thread that pushes samples to userspace.
Sampling stops when buffer is disabled by userspace.
Patil Rachna (TI) laid the ground work for ADC HW register access.
Russ Dill (TI) fixed bugs in the driver relevant to FIFOs and IRQs.
I fixed channel scanning so multiple ADC channels can be read
simultaneously and pushed to userspace.
Restructured the driver to fit IIO ABI.
And added INDIO_BUFFER_HARDWARE mode.
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
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
index a952538..b4b2ea0 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -28,12 +28,19 @@
#include <linux/iio/driver.h>
#include <linux/mfd/ti_am335x_tscadc.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
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;
};
static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
@@ -56,8 +63,14 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
return step_en;
}
-static void tiadc_step_config(struct tiadc_device *adc_dev)
+static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
{
+ return 1 << adc_dev->channel_step[chan];
+}
+
+static void tiadc_step_config(struct iio_dev *indio_dev)
+{
+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
unsigned int stepconfig;
int i, steps;
@@ -72,7 +85,11 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
*/
steps = TOTAL_STEPS - adc_dev->channels;
- stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
+ if (iio_buffer_enabled(indio_dev))
+ stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
+ | STEPCONFIG_MODE_SWCNT;
+ else
+ stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
for (i = 0; i < adc_dev->channels; i++) {
int chan;
@@ -85,7 +102,177 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
adc_dev->channel_step[i] = steps;
steps++;
}
+}
+
+static irqreturn_t tiadc_irq_h(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
+ unsigned int status, config;
+ status = tiadc_readl(adc_dev, REG_IRQSTATUS);
+
+ /*
+ * ADC and touchscreen share the IRQ line.
+ * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
+ */
+ if (status & IRQENB_FIFO1OVRRUN) {
+ /* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
+ config = tiadc_readl(adc_dev, REG_CTRL);
+ config &= ~(CNTRLREG_TSCSSENB);
+ tiadc_writel(adc_dev, REG_CTRL, config);
+ tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
+ | IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
+ tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
+ return IRQ_HANDLED;
+ } else if (status & IRQENB_FIFO1THRES) {
+ /* Disable irq and wake worker thread */
+ tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
+ return IRQ_WAKE_THREAD;
+ }
+
+ return IRQ_NONE;
+}
+
+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;
+
+ 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;
+ }
+ iio_push_to_buffers(indio_dev, (u8 *) data);
+ }
+ tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
+ tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
+
+ return IRQ_HANDLED;
+}
+
+static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
+{
+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
+ int i, fifo1count, read;
+
+ tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
+ IRQENB_FIFO1OVRRUN |
+ IRQENB_FIFO1UNDRFLW));
+
+ /* Flush FIFO. Needed in corner cases in simultaneous tsc/adc use */
+ fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+ for (i = 0; i < fifo1count; i++)
+ read = tiadc_readl(adc_dev, REG_FIFO1);
+
+ return iio_sw_buffer_preenable(indio_dev);
+}
+
+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;
+
+ adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
+ if (adc_dev->data == NULL)
+ return -ENOMEM;
+
+ tiadc_step_config(indio_dev);
+ for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
+ enb |= (get_adc_step_bit(adc_dev, bit) << 1);
+ adc_dev->buffer_en_ch_steps = enb;
+
+ am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
+
+ tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES
+ | IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
+ tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES
+ | IRQENB_FIFO1OVRRUN);
+
+ return 0;
+}
+
+static int tiadc_buffer_predisable(struct iio_dev *indio_dev)
+{
+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
+ int fifo1count, i, read;
+
+ tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
+ IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW));
+ am335x_tsc_se_clr(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps);
+
+ /* Flush FIFO of leftover data in the time it takes to disable adc */
+ fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+ for (i = 0; i < fifo1count; i++)
+ read = tiadc_readl(adc_dev, REG_FIFO1);
+
+ return 0;
+}
+
+static int tiadc_buffer_postdisable(struct iio_dev *indio_dev)
+{
+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
+
+ tiadc_step_config(indio_dev);
+ kfree(adc_dev->data);
+
+ return 0;
+}
+
+static const struct iio_buffer_setup_ops tiadc_buffer_setup_ops = {
+ .preenable = &tiadc_buffer_preenable,
+ .postenable = &tiadc_buffer_postenable,
+ .predisable = &tiadc_buffer_predisable,
+ .postdisable = &tiadc_buffer_postdisable,
+};
+
+int tiadc_iio_buffered_hardware_setup(struct iio_dev *indio_dev,
+ irqreturn_t (*pollfunc_bh)(int irq, void *p),
+ irqreturn_t (*pollfunc_th)(int irq, void *p),
+ int irq,
+ unsigned long flags,
+ const struct iio_buffer_setup_ops *setup_ops)
+{
+ int ret;
+
+ indio_dev->buffer = iio_kfifo_allocate(indio_dev);
+ if (!indio_dev->buffer)
+ return -ENOMEM;
+
+ 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_kfifo_free;
+
+ return 0;
+
+error_kfifo_free:
+ iio_kfifo_free(indio_dev->buffer);
+ return ret;
+}
+
+static void tiadc_iio_buffered_hardware_remove(struct iio_dev *indio_dev)
+{
+ iio_kfifo_free(indio_dev->buffer);
+ iio_buffer_unregister(indio_dev);
}
static const char * const chan_name_ain[] = {
@@ -120,6 +307,7 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
chan->channel = adc_dev->channel_line[i];
chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
chan->datasheet_name = chan_name_ain[chan->channel];
+ chan->scan_index = i;
chan->scan_type.sign = 'u';
chan->scan_type.realbits = 12;
chan->scan_type.storagebits = 32;
@@ -147,6 +335,10 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
u32 step_en;
unsigned long timeout = jiffies + usecs_to_jiffies
(IDLE_TIMEOUT * adc_dev->channels);
+
+ if (iio_buffer_enabled(indio_dev))
+ return -EBUSY;
+
step_en = get_adc_step_mask(adc_dev);
am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
@@ -237,20 +429,33 @@ static int tiadc_probe(struct platform_device *pdev)
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &tiadc_info;
- tiadc_step_config(adc_dev);
+ tiadc_step_config(indio_dev);
+ tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
err = tiadc_channel_init(indio_dev, adc_dev->channels);
if (err < 0)
return err;
- err = iio_device_register(indio_dev);
+ err = tiadc_iio_buffered_hardware_setup(indio_dev,
+ &tiadc_worker_h,
+ &tiadc_irq_h,
+ adc_dev->mfd_tscadc->irq,
+ IRQF_SHARED,
+ &tiadc_buffer_setup_ops);
+
if (err)
goto err_free_channels;
+ err = iio_device_register(indio_dev);
+ if (err)
+ goto err_buffer_unregister;
+
platform_set_drvdata(pdev, indio_dev);
return 0;
+err_buffer_unregister:
+ tiadc_iio_buffered_hardware_remove(indio_dev);
err_free_channels:
tiadc_channels_remove(indio_dev);
return err;
@@ -263,6 +468,7 @@ static int tiadc_remove(struct platform_device *pdev)
u32 step_en;
iio_device_unregister(indio_dev);
+ tiadc_iio_buffered_hardware_remove(indio_dev);
tiadc_channels_remove(indio_dev);
step_en = get_adc_step_mask(adc_dev);
@@ -301,7 +507,7 @@ static int tiadc_resume(struct device *dev)
restore &= ~(CNTRLREG_POWERDOWN);
tiadc_writel(adc_dev, REG_CTRL, restore);
- tiadc_step_config(adc_dev);
+ tiadc_step_config(indio_dev);
return 0;
}
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index db1791b..7d98562 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -46,16 +46,24 @@
/* Step Enable */
#define STEPENB_MASK (0x1FFFF << 0)
#define STEPENB(val) ((val) << 0)
+#define ENB(val) (1 << (val))
+#define STPENB_STEPENB STEPENB(0x1FFFF)
+#define STPENB_STEPENB_TC STEPENB(0x1FFF)
/* IRQ enable */
#define IRQENB_HW_PEN BIT(0)
#define IRQENB_FIFO0THRES BIT(2)
+#define IRQENB_FIFO0OVRRUN BIT(3)
+#define IRQENB_FIFO0UNDRFLW BIT(4)
#define IRQENB_FIFO1THRES BIT(5)
+#define IRQENB_FIFO1OVRRUN BIT(6)
+#define IRQENB_FIFO1UNDRFLW BIT(7)
#define IRQENB_PENUP BIT(9)
/* Step Configuration */
#define STEPCONFIG_MODE_MASK (3 << 0)
#define STEPCONFIG_MODE(val) ((val) << 0)
+#define STEPCONFIG_MODE_SWCNT STEPCONFIG_MODE(1)
#define STEPCONFIG_MODE_HWSYNC STEPCONFIG_MODE(2)
#define STEPCONFIG_AVG_MASK (7 << 2)
#define STEPCONFIG_AVG(val) ((val) << 2)
@@ -124,6 +132,7 @@
#define MAX_CLK_DIV 7
#define TOTAL_STEPS 16
#define TOTAL_CHANNELS 8
+#define FIFO1_THRESHOLD 19
/*
* ADC runs at 3MHz, and it takes
--
1.7.9.5
^ permalink raw reply related
* [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC
From: Zubair Lutfullah @ 2013-09-18 11:23 UTC (permalink / raw)
To: jic23-KWPb1pKIrIJaa/9Udqfwiw,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1379503383-17086-1-git-send-email-zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Enable shared IRQ to allow ADC to share IRQ line from
parent MFD core. Only FIFO0 IRQs are for TSC and handled
on the TSC side.
Step mask would be updated from cached variable only previously.
In rare cases when both TSC and ADC are used, the cached
variable gets mixed up.
The step mask is written with the required mask every time.
Rachna Patil (TI) laid ground work for shared IRQ.
Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/input/touchscreen/ti_am335x_tsc.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..24e625c 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -52,6 +52,7 @@ struct titsc {
u32 config_inp[4];
u32 bit_xp, bit_xn, bit_yp, bit_yn;
u32 inp_xp, inp_xn, inp_yp, inp_yn;
+ u32 step_mask;
};
static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
@@ -196,7 +197,8 @@ static void titsc_step_config(struct titsc *ts_dev)
/* The steps1 … end and bit 0 for TS_Charge */
stepenable = (1 << (end_step + 2)) - 1;
- am335x_tsc_se_set(ts_dev->mfd_tscadc, stepenable);
+ ts_dev->step_mask = stepenable;
+ am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
}
static void titsc_read_coordinates(struct titsc *ts_dev,
@@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
unsigned int fsm;
status = titsc_readl(ts_dev, REG_IRQSTATUS);
+ /*
+ * ADC and touchscreen share the IRQ line.
+ * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
+ */
if (status & IRQENB_FIFO0THRES) {
titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
@@ -316,7 +322,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
if (irqclr) {
titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
- am335x_tsc_se_update(ts_dev->mfd_tscadc);
+ am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
return IRQ_HANDLED;
}
return IRQ_NONE;
@@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
}
err = request_irq(ts_dev->irq, titsc_irq,
- 0, pdev->dev.driver->name, ts_dev);
+ IRQF_SHARED, pdev->dev.driver->name, ts_dev);
if (err) {
dev_err(&pdev->dev, "failed to allocate irq.\n");
goto err_free_mem;
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Jonathan Cameron @ 2013-09-18 9:39 UTC (permalink / raw)
To: Zubair Lutfullah :, Dmitry Torokhov
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <20130918065406.GA13451-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
"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.
>
>Zubair
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH v2] input: don't call input_dev_release_keys() in resume
From: Oskar Andero @ 2013-09-18 9:10 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, linux-input@vger.kernel.org
Cc: Andersson, Björn, Lekanovic, Radovan, Makarov, Aleksej,
Dmitry Torokhov
In-Reply-To: <1374754154-7990-1-git-send-email-oskar.andero@sonymobile.com>
Hii Dmitry,
On 14:09 Thu 25 Jul , Oskar Andero wrote:
> From: Aleksej Makarov <aleksej.makarov@sonymobile.com>
>
> When waking up the platform by pressing a specific key, sending a
> release on that key makes it impossible to react on the event in
> user-space. This is fixed by moving the input_reset_device() call to
> resume instead.
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Reviewed-by: Radovan Lekanovic <radovan.lekanovic@sonymobile.com>
> Signed-off-by: Aleksej Makarov <aleksej.makarov@sonymobile.com>
> Signed-off-by: Oskar Andero <oskar.andero@sonymobile.com>
> ---
> drivers/input/input.c | 11 +----------
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index c044699..ee3ff16 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1676,22 +1676,13 @@ static int input_dev_suspend(struct device *dev)
> {
> struct input_dev *input_dev = to_input_dev(dev);
>
> - mutex_lock(&input_dev->mutex);
> -
> - if (input_dev->users)
> - input_dev_toggle(input_dev, false);
> -
> - mutex_unlock(&input_dev->mutex);
> + input_reset_device(input_dev);
>
> return 0;
> }
>
> static int input_dev_resume(struct device *dev)
> {
> - struct input_dev *input_dev = to_input_dev(dev);
> -
> - input_reset_device(input_dev);
> -
> return 0;
> }
Sorry for bugging you with this patch again! I realize that changes to
input.c is sensitive since it's a central part of the subsystem.
However, the problem of reading input events after wake-up remains.
Does the patch make sense or do you see any potential risks with it?
Thanks,
Oskar
^ permalink raw reply
* Re: [PATCH] add sur40 driver for Samsung SUR40 (aka MS Surface 2.0/Pixelsense)
From: Florian Echtler @ 2013-09-18 7:29 UTC (permalink / raw)
To: linux-input; +Cc: benjamin.tissoires, rydberg, dmitry.torokhov, dh.herrmann
In-Reply-To: <1378934763-17938-1-git-send-email-floe@butterbrot.org>
[-- Attachment #1: Type: text/plain, Size: 760 bytes --]
Hello everyone,
just curious: did anybody already have a chance to test this? JFYI, I
used a stock Ubuntu 12.04 32 bit on an external USB disk for
development. If you change the boot order in the SUR40's BIOS, there is
no need to modify anything on the pre-installed copy of Windows 7.
Best regards, Florian
On 11.09.2013 23:26, Florian Echtler wrote:
> 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>
> ---
[...]
--
SENT FROM MY DEC VT50 TERMINAL
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Zubair Lutfullah : @ 2013-09-18 6:54 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Zubair Lutfullah, jic23-KWPb1pKIrIJaa/9Udqfwiw,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <20130918042726.GB13196-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org>
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.
Zubair
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Dmitry Torokhov @ 2013-09-18 4:27 UTC (permalink / raw)
To: Zubair Lutfullah
Cc: jic23-KWPb1pKIrIJaa/9Udqfwiw, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <1379393047-11772-3-git-send-email-zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
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
^ permalink raw reply
* Re: [PATCH] input: pxa27x_keypad: fix NULL pointer dereference
From: Dmitry Torokhov @ 2013-09-18 4:24 UTC (permalink / raw)
To: Mike Dunn; +Cc: Marek Vasut, linux-input, Chao Xie, Robert Jarzmik
In-Reply-To: <523926A2.1010607@newsguy.com>
On Tue, Sep 17, 2013 at 09:05:54PM -0700, Mike Dunn wrote:
> On 09/16/2013 10:06 AM, Dmitry Torokhov wrote:
> > On Mon, Sep 16, 2013 at 06:49:53PM +0200, Marek Vasut wrote:
> >> Dear Mike Dunn,
> >>
> >>> 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>
> >>> ---
> >>> drivers/input/keyboard/pxa27x_keypad.c | 6 ++++--
> >>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/input/keyboard/pxa27x_keypad.c
> >>> b/drivers/input/keyboard/pxa27x_keypad.c index 134c3b4..3b2a614 100644
> >>> --- a/drivers/input/keyboard/pxa27x_keypad.c
> >>> +++ b/drivers/input/keyboard/pxa27x_keypad.c
> >>> @@ -795,8 +795,10 @@ static int pxa27x_keypad_probe(struct platform_device
> >>> *pdev) goto failed_put_clk;
> >>> }
> >>>
> >>> - if ((pdata->enable_rotary0 && keypad->rotary_rel_code[0] != -1) ||
> >>> - (pdata->enable_rotary1 && keypad->rotary_rel_code[1] != -1)) {
> >>> + if ((keypad->pdata->enable_rotary0 &&
> >>> + keypad->rotary_rel_code[0] != -1) ||
> >>> + (keypad->pdata->enable_rotary1 &&
> >>> + keypad->rotary_rel_code[1] != -1)) {
> >>> input_dev->evbit[0] |= BIT_MASK(EV_REL);
> >>> }
> >>
> >> Nice find. Acked-by: Marek Vasut <marex@denx.de>
> >
> > Excellent booby trap. I would prefer if we explicitly did
> >
> > pdata = keypad->pdata;
> >
> > after calling the parse DT fucntion with a nice comment, because we
> > somebody might want to rearrange the code and accidentially revert the
> > checks to the original state.
>
>
> Yes, that would have been better. Is someone picking this up? I'm not familir
> with the input subsystem maintainer (sorry).
That would be yours truly.
> If this will be upstreamed in
> someone's tree, I'll be glad to resubmit with this change.
If you could resubmit that would be great - I do not have the hardware
and I prefer applying patches that were tested, if possible.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: pxa27x_keypad: fix NULL pointer dereference
From: Mike Dunn @ 2013-09-18 4:05 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Marek Vasut, linux-input, Chao Xie, Robert Jarzmik
In-Reply-To: <20130916170617.GA20734@core.coreip.homeip.net>
On 09/16/2013 10:06 AM, Dmitry Torokhov wrote:
> On Mon, Sep 16, 2013 at 06:49:53PM +0200, Marek Vasut wrote:
>> Dear Mike Dunn,
>>
>>> 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>
>>> ---
>>> drivers/input/keyboard/pxa27x_keypad.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/input/keyboard/pxa27x_keypad.c
>>> b/drivers/input/keyboard/pxa27x_keypad.c index 134c3b4..3b2a614 100644
>>> --- a/drivers/input/keyboard/pxa27x_keypad.c
>>> +++ b/drivers/input/keyboard/pxa27x_keypad.c
>>> @@ -795,8 +795,10 @@ static int pxa27x_keypad_probe(struct platform_device
>>> *pdev) goto failed_put_clk;
>>> }
>>>
>>> - if ((pdata->enable_rotary0 && keypad->rotary_rel_code[0] != -1) ||
>>> - (pdata->enable_rotary1 && keypad->rotary_rel_code[1] != -1)) {
>>> + if ((keypad->pdata->enable_rotary0 &&
>>> + keypad->rotary_rel_code[0] != -1) ||
>>> + (keypad->pdata->enable_rotary1 &&
>>> + keypad->rotary_rel_code[1] != -1)) {
>>> input_dev->evbit[0] |= BIT_MASK(EV_REL);
>>> }
>>
>> Nice find. Acked-by: Marek Vasut <marex@denx.de>
>
> Excellent booby trap. I would prefer if we explicitly did
>
> pdata = keypad->pdata;
>
> after calling the parse DT fucntion with a nice comment, because we
> somebody might want to rearrange the code and accidentially revert the
> checks to the original state.
Yes, that would have been better. Is someone picking this up? I'm not familir
with the input subsystem maintainer (sorry). If this will be upstreamed in
someone's tree, I'll be glad to resubmit with this change. Or, if you prefer,
please feel free to shepherd this Dmitry.
Sorry for the delay.
Thanks,
Mike
^ permalink raw reply
* [PATCH 1/2] input: Return the number of bytes written so far on evdev write failure
From: Ryan Mallon @ 2013-09-17 22:55 UTC (permalink / raw)
To: dmitry.torokhov, rydberg; +Cc: carl, linux-input, linux-kernel, Ryan Mallon
If input_event_from_user() fails in evdev write() and at least one
event has been written successfully then return the number of bytes
written. If no events have been written, then the EFAULT error is
returned.
Signed-off-by: Ryan Mallon <rmallon@gmail.com>
---
drivers/input/evdev.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index d2b34fb..b0dec2ba 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -437,7 +437,8 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
while (retval + input_event_size() <= count) {
if (input_event_from_user(buffer + retval, &event)) {
- retval = -EFAULT;
+ if (retval == 0)
+ retval = -EFAULT;
goto out;
}
retval += input_event_size();
--
1.7.9.7
^ permalink raw reply related
* [PATCH 2/2] uinput: Support injecting multiple events in one write() call
From: Ryan Mallon @ 2013-09-17 22:55 UTC (permalink / raw)
To: dmitry.torokhov, rydberg; +Cc: carl, linux-input, linux-kernel, Ryan Mallon
In-Reply-To: <1379458544-6508-1-git-send-email-rmallon@gmail.com>
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.
Signed-off-by: Ryan Mallon <rmallon@gmail.com>
---
drivers/input/misc/uinput.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index a0a4bba..0247a8a 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -434,16 +434,24 @@ static ssize_t uinput_inject_event(struct uinput_device *udev,
const char __user *buffer, size_t count)
{
struct input_event ev;
+ size_t bytes = 0;
- if (count < input_event_size())
+ if (count != 0 && count < input_event_size())
return -EINVAL;
- if (input_event_from_user(buffer, &ev))
- return -EFAULT;
+ while (bytes + input_event_size() <= count) {
+ if (input_event_from_user(buffer + bytes, &ev)) {
+ if (!bytes)
+ bytes = -EFAULT;
+ goto out;
+ }
- input_event(udev->dev, ev.type, ev.code, ev.value);
+ input_event(udev->dev, ev.type, ev.code, ev.value);
+ bytes += input_event_size();
+ }
- return input_event_size();
+ out:
+ return bytes;
}
static ssize_t uinput_write(struct file *file, const char __user *buffer,
--
1.7.9.7
^ permalink raw reply related
* [PATCH V2 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: K. Y. Srinivasan @ 2013-09-17 23:26 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, olaf, apw, jasowang, dan.carpenter,
dmitry.torokhov, linux-input, vojtech
Cc: K. Y. Srinivasan
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
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/input/serio/Kconfig | 7 +
drivers/input/serio/Makefile | 1 +
drivers/input/serio/hyperv-keyboard.c | 404 +++++++++++++++++++++++++++++++++
3 files changed, 412 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/serio/hyperv-keyboard.c
diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index 1e691a3..f3996e7 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -267,4 +267,11 @@ config SERIO_OLPC_APSP
To compile this driver as a module, choose M here: the module will
be called olpc_apsp.
+config HYPERV_KEYBOARD
+ tristate "Microsoft Synthetic Keyboard driver"
+ depends on HYPERV
+ default HYPERV
+ help
+ Select this option to enable the Hyper-V Keyboard driver.
+
endif
diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
index 12298b1..815d874 100644
--- a/drivers/input/serio/Makefile
+++ b/drivers/input/serio/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_SERIO_ALTERA_PS2) += altera_ps2.o
obj-$(CONFIG_SERIO_ARC_PS2) += arc_ps2.o
obj-$(CONFIG_SERIO_APBPS2) += apbps2.o
obj-$(CONFIG_SERIO_OLPC_APSP) += olpc_apsp.o
+obj-$(CONFIG_HYPERV_KEYBOARD) += hyperv-keyboard.o
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
new file mode 100644
index 0000000..c327a18
--- /dev/null
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -0,0 +1,404 @@
+/*
+ * Copyright (c) 2013, Microsoft Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * 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>
+#include <linux/completion.h>
+#include <linux/hyperv.h>
+#include <linux/serio.h>
+#include <linux/slab.h>
+
+/*
+ * Current version 1.0
+ *
+ */
+#define SYNTH_KBD_VERSION_MAJOR 1
+#define SYNTH_KBD_VERSION_MINOR 0
+#define SYNTH_KBD_VERSION (SYNTH_KBD_VERSION_MINOR | \
+ (SYNTH_KBD_VERSION_MAJOR << 16))
+
+
+/*
+ * Message types in the synthetic input protocol
+ */
+enum synth_kbd_msg_type {
+ SYNTH_KBD_PROTOCOL_REQUEST = 1,
+ SYNTH_KBD_PROTOCOL_RESPONSE = 2,
+ SYNTH_KBD_EVENT = 3,
+ SYNTH_KBD_LED_INDICATORS = 4,
+};
+
+/*
+ * Basic message structures.
+ */
+struct synth_kbd_msg_hdr {
+ u32 type;
+};
+
+struct synth_kbd_msg {
+ struct synth_kbd_msg_hdr header;
+ char data[1]; /* Enclosed message */
+};
+
+union synth_kbd_version {
+ struct {
+ u16 minor_version;
+ u16 major_version;
+ };
+ u32 version;
+};
+
+/*
+ * Protocol messages
+ */
+struct synth_kbd_protocol_request {
+ struct synth_kbd_msg_hdr header;
+ union synth_kbd_version version_requested;
+};
+
+#define PROTOCOL_ACCEPTED BIT(0)
+struct synth_kbd_protocol_response {
+ struct synth_kbd_msg_hdr header;
+ __le32 proto_status;
+};
+
+#define IS_UNICODE BIT(0)
+#define IS_BREAK BIT(1)
+#define IS_E0 BIT(2)
+#define IS_E1 BIT(3)
+struct synth_kbd_keystroke {
+ struct synth_kbd_msg_hdr header;
+ u16 make_code;
+ u16 reserved0;
+ __le32 info; /* Additional information */
+};
+
+
+#define HK_MAXIMUM_MESSAGE_SIZE 256
+
+#define KBD_VSC_SEND_RING_BUFFER_SIZE (10 * PAGE_SIZE)
+#define KBD_VSC_RECV_RING_BUFFER_SIZE (10 * PAGE_SIZE)
+
+#define XTKBD_EMUL0 0xe0
+#define XTKBD_EMUL1 0xe1
+#define XTKBD_RELEASE 0x80
+
+
+/*
+ * Represents a keyboard device
+ */
+struct hv_kbd_dev {
+ struct hv_device *device;
+ 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;
+};
+
+
+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)
+{
+ struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
+ struct synth_kbd_keystroke *ks_msg;
+ u16 scan_code;
+ u32 info;
+
+ switch (msg->header.type) {
+ case SYNTH_KBD_PROTOCOL_RESPONSE:
+ /*
+ * Validate the information provided by the host.
+ * If the host is giving us a bogus packet,
+ * drop the packet (hoping the problem
+ * goes away).
+ */
+ if (msg_length < sizeof(struct synth_kbd_protocol_response)) {
+ pr_err("Illegal protocol response packet\n");
+ 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.
+ * If the host is giving us a bogus packet,
+ * drop the packet (hoping the problem
+ * goes away).
+ */
+ if (msg_length < sizeof(struct synth_kbd_keystroke)) {
+ pr_err("Illegal keyboard event packet\n");
+ 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);
+ break;
+ default:
+ pr_err("unhandled message type %d\n", msg->header.type);
+ }
+}
+
+static void hv_kbd_on_channel_callback(void *context)
+{
+ const int packet_size = 0x100;
+ int ret;
+ struct hv_device *device = context;
+ 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;
+
+ buffer = kmalloc(bufferlen, GFP_ATOMIC);
+ if (!buffer)
+ return;
+
+ while (1) {
+ ret = vmbus_recvpacket_raw(device->channel, buffer,
+ bufferlen, &bytes_recvd, &req_id);
+
+ switch (ret) {
+ 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;
+ }
+ break;
+ case -ENOBUFS:
+ kfree(buffer);
+ /* Handle large packet */
+ bufferlen = bytes_recvd;
+ buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
+ if (!buffer)
+ return;
+ break;
+ }
+ }
+}
+
+static int hv_kbd_connect_to_vsp(struct hv_device *device)
+{
+ int ret;
+ int t;
+ u32 proto_status;
+ struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
+ struct synth_kbd_protocol_request *request;
+ struct synth_kbd_protocol_response *response;
+
+ 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)
+ 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);
+ return -ENODEV;
+ }
+ return 0;
+}
+
+static int hv_kbd_probe(struct hv_device *device,
+ const struct hv_vmbus_device_id *dev_id)
+{
+ int ret;
+ struct hv_kbd_dev *kbd_dev;
+
+ 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;
+
+probe_connect_err:
+ vmbus_close(device->channel);
+
+probe_open_err:
+ hv_kbd_free_device(kbd_dev);
+
+ return ret;
+}
+
+static int hv_kbd_remove(struct hv_device *dev)
+{
+ struct hv_kbd_dev *kbd_dev = hv_get_drvdata(dev);
+
+ vmbus_close(dev->channel);
+ hv_kbd_free_device(kbd_dev);
+ return 0;
+}
+
+/*
+ * Keyboard GUID
+ * {f912ad6d-2b17-48ea-bd65-f927a61c7684}
+ */
+#define HV_KBD_GUID \
+ .guid = { \
+ 0x6d, 0xad, 0x12, 0xf9, 0x17, 0x2b, 0xea, 0x48, \
+ 0xbd, 0x65, 0xf9, 0x27, 0xa6, 0x1c, 0x76, 0x84 \
+ }
+
+static const struct hv_vmbus_device_id id_table[] = {
+ /* Keyboard guid */
+ { HV_KBD_GUID, },
+ { },
+};
+
+MODULE_DEVICE_TABLE(vmbus, id_table);
+
+static struct hv_driver hv_kbd_drv = {
+ .name = KBUILD_MODNAME,
+ .id_table = id_table,
+ .probe = hv_kbd_probe,
+ .remove = hv_kbd_remove,
+};
+
+static int __init hv_kbd_init(void)
+{
+ return vmbus_driver_register(&hv_kbd_drv);
+}
+
+static void __exit hv_kbd_exit(void)
+{
+ vmbus_driver_unregister(&hv_kbd_drv);
+}
+
+MODULE_LICENSE("GPL");
+module_init(hv_kbd_init);
+module_exit(hv_kbd_exit);
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH 1/4] Input: cobalt_btns - Remove redundant dev_set_drvdata
From: Sachin Kamat @ 2013-09-17 9:11 UTC (permalink / raw)
To: linux-input@vger.kernel.org; +Cc: Dmitry Torokhov, Sachin Kamat
In-Reply-To: <1378490888-5083-1-git-send-email-sachin.kamat@linaro.org>
Hi Dmitry,
Now that the merge window is closed, a gentle ping on this series :)
On 6 September 2013 23:38, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> Driver core sets the data to NULL upon release or probe
> failure. Hence explicit setting is not necessary.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> Cc: Yoichi Yuasa <yuasa@linux-mips.org>
> ---
> drivers/input/misc/cobalt_btns.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/misc/cobalt_btns.c b/drivers/input/misc/cobalt_btns.c
> index 4f77f87..b5d71d2 100644
> --- a/drivers/input/misc/cobalt_btns.c
> +++ b/drivers/input/misc/cobalt_btns.c
> @@ -131,7 +131,6 @@ static int cobalt_buttons_probe(struct platform_device *pdev)
> err_free_mem:
> input_free_polled_device(poll_dev);
> kfree(bdev);
> - dev_set_drvdata(&pdev->dev, NULL);
> return error;
> }
>
> @@ -144,7 +143,6 @@ static int cobalt_buttons_remove(struct platform_device *pdev)
> input_free_polled_device(bdev->poll_dev);
> iounmap(bdev->reg);
> kfree(bdev);
> - dev_set_drvdata(dev, NULL);
>
> return 0;
> }
> --
> 1.7.4.1
>
--
With warm regards,
Sachin
^ permalink raw reply
* [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Zubair Lutfullah @ 2013-09-17 4:44 UTC (permalink / raw)
To: jic23, dmitry.torokhov
Cc: linux-iio, linux-input, linux-kernel, bigeasy, gregkh,
zubair.lutfullah
In-Reply-To: <1379393047-11772-1-git-send-email-zubair.lutfullah@gmail.com>
Previously the driver had only one-shot reading functionality.
This patch adds continuous sampling support to the driver.
Continuous sampling starts when buffer is enabled.
HW IRQ wakes worker thread that pushes samples to userspace.
Sampling stops when buffer is disabled by userspace.
Patil Rachna (TI) laid the ground work for ADC HW register access.
Russ Dill (TI) fixed bugs in the driver relevant to FIFOs and IRQs.
I fixed channel scanning so multiple ADC channels can be read
simultaneously and pushed to userspace.
Restructured the driver to fit IIO ABI.
And added INDIO_BUFFER_HARDWARE mode.
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/iio/adc/ti_am335x_adc.c | 222 +++++++++++++++++++++++++++++++++-
include/linux/mfd/ti_am335x_tscadc.h | 9 ++
2 files changed, 226 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index a952538..fa916f3 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -28,12 +28,19 @@
#include <linux/iio/driver.h>
#include <linux/mfd/ti_am335x_tscadc.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
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;
};
static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
@@ -56,8 +63,14 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
return step_en;
}
-static void tiadc_step_config(struct tiadc_device *adc_dev)
+static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
{
+ return 1 << adc_dev->channel_step[chan];
+}
+
+static void tiadc_step_config(struct iio_dev *indio_dev)
+{
+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
unsigned int stepconfig;
int i, steps;
@@ -72,7 +85,11 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
*/
steps = TOTAL_STEPS - adc_dev->channels;
- stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
+ if (iio_buffer_enabled(indio_dev))
+ stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
+ | STEPCONFIG_MODE_SWCNT;
+ else
+ stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
for (i = 0; i < adc_dev->channels; i++) {
int chan;
@@ -85,7 +102,183 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
adc_dev->channel_step[i] = steps;
steps++;
}
+}
+
+static irqreturn_t tiadc_irq_h(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
+ unsigned int status, config;
+ status = tiadc_readl(adc_dev, REG_IRQSTATUS);
+
+ /*
+ * ADC and touchscreen share the IRQ line.
+ * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
+ */
+ if (status & IRQENB_FIFO1OVRRUN) {
+ /* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
+ config = tiadc_readl(adc_dev, REG_CTRL);
+ config &= ~(CNTRLREG_TSCSSENB);
+ tiadc_writel(adc_dev, REG_CTRL, config);
+ tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
+ | IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
+ tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
+ return IRQ_HANDLED;
+ } else if (status & IRQENB_FIFO1THRES) {
+ /* Disable irq and wake worker thread */
+ tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
+ return IRQ_WAKE_THREAD;
+ }
+
+ return IRQ_NONE;
+}
+
+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;
+
+ 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;
+ }
+ iio_push_to_buffers(indio_dev, (u8 *) data);
+ }
+
+ tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
+ tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
+ return IRQ_HANDLED;
+}
+
+static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
+{
+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
+ int i, fifo1count, read;
+
+ tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
+ IRQENB_FIFO1OVRRUN |
+ IRQENB_FIFO1UNDRFLW));
+
+ /* Flush FIFO. Needed in corner cases in simultaneous tsc/adc use */
+ fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+ for (i = 0; i < fifo1count; i++)
+ read = tiadc_readl(adc_dev, REG_FIFO1);
+
+ return iio_sw_buffer_preenable(indio_dev);
+}
+
+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;
+
+ adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
+ if (adc_dev->data == NULL)
+ return -ENOMEM;
+
+ tiadc_step_config(indio_dev);
+ for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
+ enb |= (get_adc_step_bit(adc_dev, bit) << 1);
+ adc_dev->buffer_en_ch_steps = enb;
+
+ am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
+
+ tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES
+ | IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
+ tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES
+ | IRQENB_FIFO1OVRRUN);
+
+ return 0;
+}
+
+static int tiadc_buffer_predisable(struct iio_dev *indio_dev)
+{
+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
+ int fifo1count, i, read;
+
+ tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
+ IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW));
+ am335x_tsc_se_clr(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps);
+
+ /* Flush FIFO of leftover data in the time it takes to disable adc */
+ fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+ for (i = 0; i < fifo1count; i++)
+ read = tiadc_readl(adc_dev, REG_FIFO1);
+
+ return 0;
+}
+
+static int tiadc_buffer_postdisable(struct iio_dev *indio_dev)
+{
+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
+
+ tiadc_step_config(indio_dev);
+ kfree(adc_dev->data);
+
+ return 0;
+}
+
+static const struct iio_buffer_setup_ops tiadc_buffer_setup_ops = {
+ .preenable = &tiadc_buffer_preenable,
+ .postenable = &tiadc_buffer_postenable,
+ .predisable = &tiadc_buffer_predisable,
+ .postdisable = &tiadc_buffer_postdisable,
+};
+
+int tiadc_iio_buffered_hardware_setup(struct iio_dev *indio_dev,
+ irqreturn_t (*pollfunc_bh)(int irq, void *p),
+ irqreturn_t (*pollfunc_th)(int irq, void *p),
+ int irq,
+ unsigned long flags,
+ const struct iio_buffer_setup_ops *setup_ops)
+{
+ int ret;
+
+ indio_dev->buffer = iio_kfifo_allocate(indio_dev);
+ if (!indio_dev->buffer)
+ return -ENOMEM;
+
+ 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);
+error_kfifo_free:
+ iio_kfifo_free(indio_dev->buffer);
+ return ret;
+}
+
+static void tiadc_iio_buffered_hardware_remove(struct iio_dev *indio_dev)
+{
+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
+
+ devm_free_irq(indio_dev->dev.parent,
+ adc_dev->mfd_tscadc->irq, indio_dev);
+ iio_kfifo_free(indio_dev->buffer);
+ iio_buffer_unregister(indio_dev);
}
static const char * const chan_name_ain[] = {
@@ -120,6 +313,7 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
chan->channel = adc_dev->channel_line[i];
chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
chan->datasheet_name = chan_name_ain[chan->channel];
+ chan->scan_index = i;
chan->scan_type.sign = 'u';
chan->scan_type.realbits = 12;
chan->scan_type.storagebits = 32;
@@ -147,6 +341,10 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
u32 step_en;
unsigned long timeout = jiffies + usecs_to_jiffies
(IDLE_TIMEOUT * adc_dev->channels);
+
+ if (iio_buffer_enabled(indio_dev))
+ return -EBUSY;
+
step_en = get_adc_step_mask(adc_dev);
am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
@@ -237,20 +435,33 @@ static int tiadc_probe(struct platform_device *pdev)
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &tiadc_info;
- tiadc_step_config(adc_dev);
+ tiadc_step_config(indio_dev);
+ tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
err = tiadc_channel_init(indio_dev, adc_dev->channels);
if (err < 0)
return err;
- err = iio_device_register(indio_dev);
+ err = tiadc_iio_buffered_hardware_setup(indio_dev,
+ &tiadc_worker_h,
+ &tiadc_irq_h,
+ adc_dev->mfd_tscadc->irq,
+ IRQF_SHARED,
+ &tiadc_buffer_setup_ops);
+
if (err)
goto err_free_channels;
+ err = iio_device_register(indio_dev);
+ if (err)
+ goto err_buffer_unregister;
+
platform_set_drvdata(pdev, indio_dev);
return 0;
+err_buffer_unregister:
+ tiadc_iio_buffered_hardware_remove(indio_dev);
err_free_channels:
tiadc_channels_remove(indio_dev);
return err;
@@ -263,6 +474,7 @@ static int tiadc_remove(struct platform_device *pdev)
u32 step_en;
iio_device_unregister(indio_dev);
+ tiadc_iio_buffered_hardware_remove(indio_dev);
tiadc_channels_remove(indio_dev);
step_en = get_adc_step_mask(adc_dev);
@@ -301,7 +513,7 @@ static int tiadc_resume(struct device *dev)
restore &= ~(CNTRLREG_POWERDOWN);
tiadc_writel(adc_dev, REG_CTRL, restore);
- tiadc_step_config(adc_dev);
+ tiadc_step_config(indio_dev);
return 0;
}
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index db1791b..7d98562 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -46,16 +46,24 @@
/* Step Enable */
#define STEPENB_MASK (0x1FFFF << 0)
#define STEPENB(val) ((val) << 0)
+#define ENB(val) (1 << (val))
+#define STPENB_STEPENB STEPENB(0x1FFFF)
+#define STPENB_STEPENB_TC STEPENB(0x1FFF)
/* IRQ enable */
#define IRQENB_HW_PEN BIT(0)
#define IRQENB_FIFO0THRES BIT(2)
+#define IRQENB_FIFO0OVRRUN BIT(3)
+#define IRQENB_FIFO0UNDRFLW BIT(4)
#define IRQENB_FIFO1THRES BIT(5)
+#define IRQENB_FIFO1OVRRUN BIT(6)
+#define IRQENB_FIFO1UNDRFLW BIT(7)
#define IRQENB_PENUP BIT(9)
/* Step Configuration */
#define STEPCONFIG_MODE_MASK (3 << 0)
#define STEPCONFIG_MODE(val) ((val) << 0)
+#define STEPCONFIG_MODE_SWCNT STEPCONFIG_MODE(1)
#define STEPCONFIG_MODE_HWSYNC STEPCONFIG_MODE(2)
#define STEPCONFIG_AVG_MASK (7 << 2)
#define STEPCONFIG_AVG(val) ((val) << 2)
@@ -124,6 +132,7 @@
#define MAX_CLK_DIV 7
#define TOTAL_STEPS 16
#define TOTAL_CHANNELS 8
+#define FIFO1_THRESHOLD 19
/*
* ADC runs at 3MHz, and it takes
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Jürgen Beisert @ 2013-09-17 7:33 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Dmitry Torokhov, Jonathan Cameron,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, marex-ynQEQJNshbs,
fabio.estevam-KZfg59tc24xl57MIdRCFDg,
jic23-KWPb1pKIrIJaa/9Udqfwiw,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20130916152846.GC12105-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org>
Hi Dmitry,
On Monday 16 September 2013 17:28:46 Dmitry Torokhov wrote:
> [...]
> > > static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
> > > @@ -641,6 +1056,7 @@ static void mxs_lradc_ts_unregister(struct
> > > mxs_lradc *lradc)
> > >
> > > cancel_work_sync(&lradc->ts_work);
> > >
> > > + mxs_lradc_disable_ts(lradc);
> > > input_unregister_device(lradc->ts_input);
> > > }
>
> This looks iffy... Normally you disable the device so that it does not
> generate more interrupts, and then cancel outstanding work(s), otherwise
> newly generated interrupts may cause more work to be scheduled. Or I
> missed some of the context and this is not a concern here?
This part gets removed in patch 6/6:
@@ -1054,8 +891,6 @@ static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
if (!lradc->use_touchscreen)
return;
- cancel_work_sync(&lradc->ts_work);
-
mxs_lradc_disable_ts(lradc);
input_unregister_device(lradc->ts_input);
}
But you are right, I should move this into patch 5/6.
Thanks
Regards,
Juergen
--
Pengutronix e.K. | Juergen Beisert |
Linux Solutions for Science and Industry | http://www.pengutronix.de/ |
^ permalink raw reply
* [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC
From: Zubair Lutfullah @ 2013-09-17 4:44 UTC (permalink / raw)
To: jic23, dmitry.torokhov
Cc: linux-iio, linux-input, linux-kernel, bigeasy, gregkh,
zubair.lutfullah
In-Reply-To: <1379393047-11772-1-git-send-email-zubair.lutfullah@gmail.com>
Enable shared IRQ to allow ADC to share IRQ line from
parent MFD core. Only FIFO0 IRQs are for TSC and handled
on the TSC side.
Step mask would be updated from cached variable only previously.
In rare cases when both TSC and ADC are used, the cached
variable gets mixed up.
The step mask is written with the required mask every time.
Rachna Patil (TI) laid ground work for shared IRQ.
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/ti_am335x_tsc.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..24e625c 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -52,6 +52,7 @@ struct titsc {
u32 config_inp[4];
u32 bit_xp, bit_xn, bit_yp, bit_yn;
u32 inp_xp, inp_xn, inp_yp, inp_yn;
+ u32 step_mask;
};
static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
@@ -196,7 +197,8 @@ static void titsc_step_config(struct titsc *ts_dev)
/* The steps1 … end and bit 0 for TS_Charge */
stepenable = (1 << (end_step + 2)) - 1;
- am335x_tsc_se_set(ts_dev->mfd_tscadc, stepenable);
+ ts_dev->step_mask = stepenable;
+ am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
}
static void titsc_read_coordinates(struct titsc *ts_dev,
@@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
unsigned int fsm;
status = titsc_readl(ts_dev, REG_IRQSTATUS);
+ /*
+ * ADC and touchscreen share the IRQ line.
+ * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
+ */
if (status & IRQENB_FIFO0THRES) {
titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
@@ -316,7 +322,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
if (irqclr) {
titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
- am335x_tsc_se_update(ts_dev->mfd_tscadc);
+ am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
return IRQ_HANDLED;
}
return IRQ_NONE;
@@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
}
err = request_irq(ts_dev->irq, titsc_irq,
- 0, pdev->dev.driver->name, ts_dev);
+ IRQF_SHARED, pdev->dev.driver->name, ts_dev);
if (err) {
dev_err(&pdev->dev, "failed to allocate irq.\n");
goto err_free_mem;
--
1.7.9.5
^ permalink raw reply related
* [PATCH V9 0/2] iio: input: ti_am335x_adc: Add continuous sampling support
From: Zubair Lutfullah @ 2013-09-17 4:44 UTC (permalink / raw)
To: jic23-KWPb1pKIrIJaa/9Udqfwiw,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w
Hi,
This applies to togreg branch in iio.
Round 9 updates
- Removed Trigger.
- Restructured everything for INDIO_BUFFERED_HARDWARE mode.
- Threaded IRQ with top/bottom halfs. Top half HW IRQ wakes
bottom half threaded worker that pushes samples to iio/userspace.
- Removed some of the patchwork to the driver that was irrelevant to the
continuous sampling patch.
Cleanup will be done separately once this goes through.
Hope these patches meet the mighty kernel standards :)
Zubair
Zubair Lutfullah (2):
input: ti_am335x_tsc: Enable shared IRQ for TSC
iio: ti_am335x_adc: Add continuous sampling support
drivers/iio/adc/ti_am335x_adc.c | 222 ++++++++++++++++++++++++++++-
drivers/input/touchscreen/ti_am335x_tsc.c | 12 +-
include/linux/mfd/ti_am335x_tscadc.h | 9 ++
3 files changed, 235 insertions(+), 8 deletions(-)
--
1.7.9.5
^ permalink raw reply
* [PATCH V2] Input: atmel_mxt_ts - update to devm_* API
From: Manish Badarkhe @ 2013-09-17 3:20 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: nick.dyer, dmitry.torokhov, josh.wu
Update the code to use devm_* API so that driver core will manage
resources.
Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
---
Changes since V1:
As per Dmitry's comment, avoid "devm_" operation on irq as it may cause
kernel crash while device is getting unbound and CCing Nick dryer.
:100644 100644 59aa240... d04dbed... M drivers/input/touchscreen/atmel_mxt_ts.c
drivers/input/touchscreen/atmel_mxt_ts.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 59aa240..d04dbed 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1139,12 +1139,12 @@ static int mxt_probe(struct i2c_client *client,
if (!pdata)
return -EINVAL;
- data = kzalloc(sizeof(struct mxt_data), GFP_KERNEL);
- input_dev = input_allocate_device();
+ data = devm_kzalloc(&client->dev, sizeof(struct mxt_data), GFP_KERNEL);
+ input_dev = devm_input_allocate_device(&client->dev);
if (!data || !input_dev) {
dev_err(&client->dev, "Failed to allocate memory\n");
error = -ENOMEM;
- goto err_free_mem;
+ goto err_out;
}
data->is_tp = pdata && pdata->is_tp;
@@ -1170,7 +1170,7 @@ static int mxt_probe(struct i2c_client *client,
error = mxt_initialize(data);
if (error)
- goto err_free_mem;
+ goto err_out;
__set_bit(EV_ABS, input_dev->evbit);
__set_bit(EV_KEY, input_dev->evbit);
@@ -1253,9 +1253,7 @@ err_free_irq:
free_irq(client->irq, data);
err_free_object:
kfree(data->object_table);
-err_free_mem:
- input_free_device(input_dev);
- kfree(data);
+err_out:
return error;
}
@@ -1267,7 +1265,6 @@ static int mxt_remove(struct i2c_client *client)
free_irq(data->irq, data);
input_unregister_device(data->input_dev);
kfree(data->object_table);
- kfree(data);
return 0;
}
--
1.7.10.4
^ 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