* Re: [RFC PATCH] Input: evdev - drop redundant list-locking
From: Dmitry Torokhov @ 2014-07-20 18:54 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Henrik Rydberg
In-Reply-To: <1405882092-7005-1-git-send-email-dh.herrmann@gmail.com>
On Sun, Jul 20, 2014 at 08:48:12PM +0200, David Herrmann wrote:
> evdev->client_list is rcu-protected. There is no need to have a
> separate spinlock just for the list. Either one is good enough, so lets
> drop the spinlock.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> Hi
>
> I stumbled across this one when doing some evdev reviews. Maybe I'm missing
> something obvious and I should stop coding on Sundays. But the RCU-protection
> should be enough here, right?
RCU protection is for traversing list only, writes (as is adding and removing
elements from client_list) still have to be mutually exclusive.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RFC PATCH] Input: evdev - drop redundant list-locking
From: David Herrmann @ 2014-07-20 19:00 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: open list:HID CORE LAYER, Henrik Rydberg
In-Reply-To: <20140720185406.GA4446@core.coreip.homeip.net>
Hi
On Sun, Jul 20, 2014 at 8:54 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Sun, Jul 20, 2014 at 08:48:12PM +0200, David Herrmann wrote:
>> evdev->client_list is rcu-protected. There is no need to have a
>> separate spinlock just for the list. Either one is good enough, so lets
>> drop the spinlock.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> Hi
>>
>> I stumbled across this one when doing some evdev reviews. Maybe I'm missing
>> something obvious and I should stop coding on Sundays. But the RCU-protection
>> should be enough here, right?
>
> RCU protection is for traversing list only, writes (as is adding and removing
> elements from client_list) still have to be mutually exclusive.
Oh, you mean to protect against concurrent writes? Right, but we could
just use evdev->mutex for that. I mean all paths that call
attach_client() or detach_client() already lock evdev->mutex at some
point. It would allow us to get rid of the lock.
Thanks
David
^ permalink raw reply
* Re: [RFC PATCH] Input: evdev - drop redundant list-locking
From: Dmitry Torokhov @ 2014-07-20 19:53 UTC (permalink / raw)
To: David Herrmann; +Cc: open list:HID CORE LAYER, Henrik Rydberg
In-Reply-To: <CANq1E4RRV7A3dAJgeNCV72Q_sUz7mhbSPFLP+f3J+K2YVd4_0w@mail.gmail.com>
On Sun, Jul 20, 2014 at 09:00:02PM +0200, David Herrmann wrote:
> Hi
>
> On Sun, Jul 20, 2014 at 8:54 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Sun, Jul 20, 2014 at 08:48:12PM +0200, David Herrmann wrote:
> >> evdev->client_list is rcu-protected. There is no need to have a
> >> separate spinlock just for the list. Either one is good enough, so lets
> >> drop the spinlock.
> >>
> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >> ---
> >> Hi
> >>
> >> I stumbled across this one when doing some evdev reviews. Maybe I'm missing
> >> something obvious and I should stop coding on Sundays. But the RCU-protection
> >> should be enough here, right?
> >
> > RCU protection is for traversing list only, writes (as is adding and removing
> > elements from client_list) still have to be mutually exclusive.
>
> Oh, you mean to protect against concurrent writes? Right, but we could
> just use evdev->mutex for that. I mean all paths that call
> attach_client() or detach_client() already lock evdev->mutex at some
> point. It would allow us to get rid of the lock.
Right, we probably could do it by pulling taking/releasing evdev->mutex into
evdev_pen and evdev_release.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH v2] Input: evdev - drop redundant list-locking
From: David Herrmann @ 2014-07-20 20:03 UTC (permalink / raw)
To: linux-input; +Cc: Dmitry Torokhov, Henrik Rydberg, David Herrmann
In-Reply-To: <1405882092-7005-1-git-send-email-dh.herrmann@gmail.com>
evdev->client_list is rcu-protected. We need the client_lock only to
protect against concurrent writes. However, all paths that access
client_list already lock evdev->mutex. Therefore, drop client_lock and use
evdev->mutex as list-protection.
This also drops several helper functions that are called only once. Most
of them are fairly trivial so there's little reason to extract them. This
is needed to get better control over evdev->mutex locking.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
v2:
- use evdev->mutex to protect against concurrent writes
- inline most of the helper functions
drivers/input/evdev.c | 126 ++++++++++++++++----------------------------------
1 file changed, 40 insertions(+), 86 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 7a25a7a..406acde 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -34,7 +34,6 @@ struct evdev {
wait_queue_head_t wait;
struct evdev_client __rcu *grab;
struct list_head client_list;
- spinlock_t client_lock; /* protects client_list */
struct mutex mutex;
struct device dev;
struct cdev cdev;
@@ -430,69 +429,6 @@ static int evdev_ungrab(struct evdev *evdev, struct evdev_client *client)
return 0;
}
-static void evdev_attach_client(struct evdev *evdev,
- struct evdev_client *client)
-{
- spin_lock(&evdev->client_lock);
- list_add_tail_rcu(&client->node, &evdev->client_list);
- spin_unlock(&evdev->client_lock);
-}
-
-static void evdev_detach_client(struct evdev *evdev,
- struct evdev_client *client)
-{
- spin_lock(&evdev->client_lock);
- list_del_rcu(&client->node);
- spin_unlock(&evdev->client_lock);
- synchronize_rcu();
-}
-
-static int evdev_open_device(struct evdev *evdev)
-{
- int retval;
-
- retval = mutex_lock_interruptible(&evdev->mutex);
- if (retval)
- return retval;
-
- if (!evdev->exist)
- retval = -ENODEV;
- else if (!evdev->open++) {
- retval = input_open_device(&evdev->handle);
- if (retval)
- evdev->open--;
- }
-
- mutex_unlock(&evdev->mutex);
- return retval;
-}
-
-static void evdev_close_device(struct evdev *evdev)
-{
- mutex_lock(&evdev->mutex);
-
- if (evdev->exist && !--evdev->open)
- input_close_device(&evdev->handle);
-
- mutex_unlock(&evdev->mutex);
-}
-
-/*
- * Wake up users waiting for IO so they can disconnect from
- * dead device.
- */
-static void evdev_hangup(struct evdev *evdev)
-{
- struct evdev_client *client;
-
- spin_lock(&evdev->client_lock);
- list_for_each_entry(client, &evdev->client_list, node)
- kill_fasync(&client->fasync, SIGIO, POLL_HUP);
- spin_unlock(&evdev->client_lock);
-
- wake_up_interruptible(&evdev->wait);
-}
-
static int evdev_release(struct inode *inode, struct file *file)
{
struct evdev_client *client = file->private_data;
@@ -501,9 +437,12 @@ static int evdev_release(struct inode *inode, struct file *file)
mutex_lock(&evdev->mutex);
evdev_ungrab(evdev, client);
+ list_del_rcu(&client->node);
+ if (evdev->exist && !--evdev->open)
+ input_close_device(&evdev->handle);
mutex_unlock(&evdev->mutex);
- evdev_detach_client(evdev, client);
+ synchronize_rcu();
for (i = 0; i < EV_CNT; ++i)
kfree(client->evmasks[i]);
@@ -513,8 +452,6 @@ static int evdev_release(struct inode *inode, struct file *file)
else
kfree(client);
- evdev_close_device(evdev);
-
return 0;
}
@@ -545,19 +482,38 @@ static int evdev_open(struct inode *inode, struct file *file)
client->bufsize = bufsize;
spin_lock_init(&client->buffer_lock);
client->evdev = evdev;
- evdev_attach_client(evdev, client);
- error = evdev_open_device(evdev);
+ error = mutex_lock_interruptible(&evdev->mutex);
if (error)
- goto err_free_client;
+ goto err_free;
+
+ list_add_tail_rcu(&client->node, &evdev->client_list);
+
+ if (!evdev->exist) {
+ error = -ENODEV;
+ goto err_detach;
+ }
+
+ if (!evdev->open++) {
+ error = input_open_device(&evdev->handle);
+ if (error) {
+ evdev->open--;
+ goto err_detach;
+ }
+ }
+
+ mutex_unlock(&evdev->mutex);
file->private_data = client;
nonseekable_open(inode, file);
return 0;
- err_free_client:
- evdev_detach_client(evdev, client);
+err_detach:
+ list_del_rcu(&client->node);
+ mutex_unlock(&evdev->mutex);
+ synchronize_rcu();
+err_free:
kfree(client);
return error;
}
@@ -1384,24 +1340,23 @@ static const struct file_operations evdev_fops = {
.llseek = no_llseek,
};
-/*
- * Mark device non-existent. This disables writes, ioctls and
- * prevents new users from opening the device. Already posted
- * blocking reads will stay, however new ones will fail.
- */
-static void evdev_mark_dead(struct evdev *evdev)
+static void evdev_cleanup(struct evdev *evdev)
{
+ struct input_handle *handle = &evdev->handle;
+ struct evdev_client *client;
+
+ /*
+ * Mark device non-existent to disable writes, ioctls and new users.
+ * Then wake up running users that wait for I/O so they can disconnect
+ * from the dead device.
+ */
mutex_lock(&evdev->mutex);
evdev->exist = false;
+ list_for_each_entry(client, &evdev->client_list, node)
+ kill_fasync(&client->fasync, SIGIO, POLL_HUP);
mutex_unlock(&evdev->mutex);
-}
-static void evdev_cleanup(struct evdev *evdev)
-{
- struct input_handle *handle = &evdev->handle;
-
- evdev_mark_dead(evdev);
- evdev_hangup(evdev);
+ wake_up_interruptible(&evdev->wait);
cdev_del(&evdev->cdev);
@@ -1438,7 +1393,6 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev,
}
INIT_LIST_HEAD(&evdev->client_list);
- spin_lock_init(&evdev->client_lock);
mutex_init(&evdev->mutex);
init_waitqueue_head(&evdev->wait);
evdev->exist = true;
--
2.0.2
^ permalink raw reply related
* Re: [PATCH] staging: input: sunkbd.c: Fix coding style
From: Dmitry Torokhov @ 2014-07-20 20:19 UTC (permalink / raw)
To: Joe Perches
Cc: Bivolaru Catalin, kernel-janitors, linux-input, linux-kernel,
paul.gortmaker
In-Reply-To: <1405871574.5135.24.camel@joe-AO725>
On Sun, Jul 20, 2014 at 08:52:54AM -0700, Joe Perches wrote:
> On Sun, 2014-07-20 at 15:01 +0300, Bivolaru Catalin wrote:
> > Fix coding style with regard to missing spaces after , and the column limit
> > that was violated after the first change.
>
> Some checkpatch errors should be ignored.
>
> These are some of them.
>
> This change doesn't make it easier to read
> or to find any array entry.
>
> It's currently in groups of 16 (0x10).
>
> This change would make it very difficult
> to find any particular entry.
>
> It might make some sense to change the last
> line to add a trailing " 0, 0" entry as
> the compiler does to make it a bit clearer
> the table has 128 entries.
I'd rather not change a thing in this table - it has been working as is
for many years.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
From: Dmitry Torokhov @ 2014-07-20 20:28 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Arnd Bergmann, linux-arm-kernel, Chanwoo Choi, ch.naveen,
mark.rutland, devicetree, kgene.kim, pawel.moll, ijc+devicetree,
linux-iio, t.figa, rdunlap, linux-doc, linux-kernel,
linux-samsung-soc, kyungmin.park, robh+dt, galak, heiko.stuebner,
Ben Dooks, linux-input
In-Reply-To: <53CBC969.4090704@kernel.org>
On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
> On 20/07/14 14:49, Jonathan Cameron wrote:
> >On 18/07/14 20:29, Arnd Bergmann wrote:
> >>This adds support for the touchscreen on Samsung s3c64xx.
> >>The driver is completely untested but shows roughly how
> >>it could be done, following the example of the at91 driver.
> >>
> >Hi Arnd,
> Cc'd linux-input and Dmitry.
> >
> >>Open questions include:
> >>
> >>- compared to the old plat-samsung/adc driver, there is
> >> no support for prioritizing ts over other clients, nor
> >> for oversampling. From my reading of the code, the
> >> priorities didn't actually have any effect at all, but
> >> the oversampling might be needed. Maybe the original
> >> authors have some insight.
> >>
> >>- I simply register the input device from the adc driver
> >> itself, as the at91 code does. The driver also supports
> >> sub-nodes, but I don't understand how they are meant
> >> to be used, so using those might be better.
> >So, the alternative you are (I think) referring to is to use
> >the buffered in kernel client interface. That way a separate
> >touch screen driver can use the output channels provided by IIO
> >in much the same way you might use a regulator or similar.
> >Note that whilst this is similar to the simple polled interface
> >used for things like the iio_hwmon driver, the data flow is
> >quite different (clearly the polled interfce would be
> >inappropriate here).
> >
> >Whilst we've discussed it in the past for touch screen drivers
> >like this, usually the hardware isn't generic enough to be
> >of any real use if not being used as a touch screen. As such
> >it's often simpler to just have the support directly in the
> >driver (as you've observed the at91 driver does this).
> >
> >Whilst the interface has been there a while, it's not really had
> >all that much use. The original target was the simpler case
> >of 3D accelerometer where we have a generic iio to input
> >bridge driver. Time constraints meant that I haven't yet actually
> >formally submitted the input side of this. Whilst there are lots
> >of other things that can use this interface, right now nothing
> >actually does so.
> >
> >>
> >>- The new exynos_read_s3c64xx_ts() function is intentionally
> >> very similar to the existing exynos_read_raw() functions.
> >> It should probably be changed, either by merging the two
> >> into one, or by simplifying the exynos_read_s3c64xx_ts()
> >> function. This depends a bit on the answers to the questions
> >> above.
> >I'd be tempted to not bother keeping them that similar. It's
> >not a generic IIO channel so simplify it where possible.
> >>
> >>- We probably need to add support for platform_data as well,
> >> I've skipped this so far.
> >>
> >>- Is anybody able to debug this driver on real hardware?
> >> While it's possible that it actually works, it's more
> >> likely that I made a few subtle mistakes.
> >>
> >>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >Looks pretty good to me. A few symantic bits and pieces and
> >one bug spotted. Short and sweet.
> >>
> >>diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> >>index e1b74828f413..4329bf3c3326 100644
> >>--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> >>+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> >>@@ -41,6 +41,10 @@ Required properties:
> >> and compatible ADC block)
> >> - vdd-supply VDD input supply.
> >>
> >>+Optional properties:
> >>+- has-touchscreen: If present, indicates that a touchscreen is
> >>+ connected an usable.
> >>+
> >> Note: child nodes can be added for auto probing from device tree.
> >>
> >> Example: adding device info in dtsi file
> >>diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> >>index 5f95638513d2..cf1d9f3e2492 100644
> >>--- a/drivers/iio/adc/exynos_adc.c
> >>+++ b/drivers/iio/adc/exynos_adc.c
> >>@@ -34,6 +34,7 @@
> >> #include <linux/regulator/consumer.h>
> >> #include <linux/of_platform.h>
> >> #include <linux/err.h>
> >>+#include <linux/input.h>
> >Might want to make the input side optional at compile time...
> >I supose the existing parts are unlikely to be used much in headless
> >devices, but you never know. Maybe we just leave this until someone
> >shouts they want to be able to avoid compiling it in.
> >>
> >> #include <linux/iio/iio.h>
> >> #include <linux/iio/machine.h>
> >>@@ -103,6 +104,7 @@
> >>
> >> /* Bit definitions common for ADC_V1 and ADC_V2 */
> >> #define ADC_CON_EN_START (1u << 0)
> >>+#define ADC_DATX_PRESSED (1u << 15)
> >> #define ADC_DATX_MASK 0xFFF
> >>
> >> #define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
> >>@@ -110,16 +112,20 @@
> >> struct exynos_adc {
> >> struct exynos_adc_data *data;
> >> struct device *dev;
> >>+ struct input_dev *input;
> >> void __iomem *regs;
> >> void __iomem *enable_reg;
> >> struct clk *clk;
> >> struct clk *sclk;
> >> unsigned int irq;
> >>+ unsigned int tsirq;
> >> struct regulator *vdd;
> >>
> >> struct completion completion;
> >>
> >>+ bool read_ts;
> >> u32 value;
> >>+ u32 value2;
> >As I state further down, I'd rather keep a little
> >clear of the naming used in IIO for bits that aren't
> >going through IIO (less confusing!). Maybe just
> >have
> > u32 x, y;
> >> unsigned int version;
> >> };
> >>
> >>@@ -390,12 +396,61 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> >> return ret;
> >> }
> >>
> >>+static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
> >>+ struct iio_chan_spec const *chan,
> >>+ int *val,
> >>+ int *val2,
> >>+ long mask)
> >>+{
> >>+ struct exynos_adc *info = iio_priv(indio_dev);
> >>+ unsigned long timeout;
> >>+ int ret;
> >>+
> >>+ if (mask != IIO_CHAN_INFO_RAW)
> >>+ return -EINVAL;
> >>+
> >>+ mutex_lock(&indio_dev->mlock);
> >>+ info->read_ts = 1;
> >>+
> >>+ reinit_completion(&info->completion);
> >>+
> >>+ writel(ADC_S3C2410_TSC_PULL_UP_DISABLE | ADC_TSC_AUTOPST,
> >>+ ADC_V1_TSC(info->regs));
> >>+
> >>+ /* Select the ts channel to be used and Trigger conversion */
> >>+ info->data->start_conv(info, 0);
> >0 is a rather magic value. A define perhaps?
> >>+
> >>+ timeout = wait_for_completion_timeout
> >>+ (&info->completion, EXYNOS_ADC_TIMEOUT);
> >>+ if (timeout == 0) {
> >>+ dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
> >>+ if (info->data->init_hw)
> >>+ info->data->init_hw(info);
> >>+ ret = -ETIMEDOUT;
> >>+ } else {
> >>+ *val = info->value;
> >>+ *val2 = info->value2;
> >This is definitely abuse as those two values are not intended for
> >different values. If you want to do this please use different naming
> >and don't try to fiddle it into the IIO read raw framework.
> >As you've suggested above, better to simplify this code and drop the
> >bits cloned from the other handler.
> >>+ ret = IIO_VAL_INT;
> >>+ }
> >>+
> >>+ info->read_ts = 0;
> >>+ mutex_unlock(&indio_dev->mlock);
> >>+
> >>+ return ret;
> >>+}
> >>+
> >> static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> >> {
> >> struct exynos_adc *info = (struct exynos_adc *)dev_id;
> >>
> >> /* Read value */
> >>- info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >>+ if (info->read_ts) {
> >>+ info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >>+ info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
> >ADC_DATY_MASK would be more obviously correct.
> >
> >>+ writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
> >Perhaps the above is cryptic enough to warrant a comment?
> >>+ } else {
> >>+ info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >>+ }
> >>
> >> /* clear irq */
> >> if (info->data->clear_irq)
> >>@@ -406,6 +461,46 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> >> return IRQ_HANDLED;
> >> }
> >>
> >>+/*
> >>+ * Here we (ab)use a threaded interrupt handler to stay running
> >>+ * for as long as the touchscreen remains pressed, we report
> >>+ * a new event with the latest data and then sleep until the
> >>+ * next timer tick. This mirrors the behavior of the old
> >>+ * driver, with much less code.
> >>+ */
> >>+static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> >>+{
> >>+ struct exynos_adc *info = dev_id;
> >>+ struct iio_dev *dev = dev_get_drvdata(info->dev);
> >>+ u32 x, y;
> >>+ bool pressed;
> >>+ int ret;
> >>+
> >>+ do {
> >>+ ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> >= exynos
> >>+ if (ret == -ETIMEDOUT)
> >>+ break;
> >>+
> >>+ pressed = x & y & ADC_DATX_PRESSED;
> >>+ if (!pressed)
> >>+ break;
> >>+
> >>+ input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
> >>+ input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
> >>+ input_report_key(info->input, BTN_TOUCH, 1);
> >>+ input_sync(info->input);
> >>+
> >>+ msleep(1);
> >>+ } while (1);
It would be nice to actually close the device even if someone is
touching screen. Please implement open/close methods and have them set a
flag that you would check here.
> >>+
> >>+ input_report_key(info->input, BTN_TOUCH, 0);
> >>+ input_sync(info->input);
> >>+
> >>+ writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
> >>+
> >>+ return IRQ_HANDLED;
> >>+}
> >>+
> >> static int exynos_adc_reg_access(struct iio_dev *indio_dev,
> >> unsigned reg, unsigned writeval,
> >> unsigned *readval)
> >>@@ -457,12 +552,57 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
> >> return 0;
> >> }
> >>
> >>+static int exynos_adc_ts_init(struct exynos_adc *info)
> >>+{
> >>+ int ret;
> >>+
> >>+ info->input = input_allocate_device();
> >>+ if (!info->input)
> >>+ return -ENOMEM;
> >>+
> >>+ info->input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> >>+ info->input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> >>+
> >>+ input_set_abs_params(info->input, ABS_X, 0, 0x3FF, 0, 0);
> >>+ input_set_abs_params(info->input, ABS_Y, 0, 0x3FF, 0, 0);
> >>+
> >>+ /* data from s3c2410_ts driver */
> >>+ info->input->name = "S3C24xx TouchScreen";
> >>+ info->input->id.bustype = BUS_HOST;
> >>+ info->input->id.vendor = 0xDEAD;
> >>+ info->input->id.product = 0xBEEF;
You do not need to fill these entries with fake data.
> >>+ info->input->id.version = 0x0200;
> >>+
> >>+ ret = input_register_device(info->input);
> >>+ if (ret) {
> >>+ input_free_device(info->input);
> >>+ goto err;
> >>+ }
> >>+
> >>+ if (info->tsirq > 0)
> >>+ ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
> >>+ 0, "touchscreen", info);
> >info->tsirq
> >(that had me really confused for a moment ;)
> >Also, perhaps a more specific name. touchscreen_updown or similar as the
> >main interrupt is also used during touchscreen operation.
> >>+ if (ret < 0) {
> >>+ dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
> >>+ info->irq);
> >>+ goto err_input;
> >>+ }
> >>+
> >>+ return 0;
> >>+
> >>+err_input:
> >>+ input_unregister_device(info->input);
> >>+err:
> >>+ return ret;
> >>+}
> >>+
> >> static int exynos_adc_probe(struct platform_device *pdev)
> >> {
> >> struct exynos_adc *info = NULL;
> >> struct device_node *np = pdev->dev.of_node;
> >> struct iio_dev *indio_dev = NULL;
> >> struct resource *mem;
> >>+ bool has_ts;
> >> int ret = -ENODEV;
> >> int irq;
> >>
> >>@@ -498,8 +638,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
> >> dev_err(&pdev->dev, "no irq resource?\n");
> >> return irq;
> >> }
> >>-
> >> info->irq = irq;
> >>+
> >>+ irq = platform_get_irq(pdev, 1);
> >>+ if (irq == -EPROBE_DEFER)
> >>+ return irq;
> >>+
> >>+ info->tsirq = irq;
> >>+
> >> info->dev = &pdev->dev;
> >>
> >> init_completion(&info->completion);
> >>@@ -565,6 +711,12 @@ static int exynos_adc_probe(struct platform_device *pdev)
> >> if (info->data->init_hw)
> >> info->data->init_hw(info);
> >>
> >>+ has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
> >>+ if (has_ts)
> >>+ ret = exynos_adc_ts_init(info);
> >>+ if (ret)
> >>+ goto err_iio;
> >>+
> >> ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
> >> if (ret < 0) {
> >> dev_err(&pdev->dev, "failed adding child nodes\n");
> >>@@ -576,6 +728,11 @@ static int exynos_adc_probe(struct platform_device *pdev)
> >> err_of_populate:
> >> device_for_each_child(&indio_dev->dev, NULL,
> >> exynos_adc_remove_devices);
> >>+ if (has_ts) {
> >>+ input_unregister_device(info->input);
> >>+ free_irq(info->tsirq, info);
Are we sure that device is quiesced here and interrupt won't arrive
between input_unregister_device() and free_irq()? I guess if you
properly implement open/close it won't matter.
> >>+ }
> >>+err_iio:
> >> iio_device_unregister(indio_dev);
> >> err_irq:
> >> free_irq(info->irq, info);
> >>@@ -595,9 +752,12 @@ static int exynos_adc_remove(struct platform_device *pdev)
> >> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >> struct exynos_adc *info = iio_priv(indio_dev);
> >>
> >>+ input_free_device(info->input);
Should be unregister, not free.
> >> device_for_each_child(&indio_dev->dev, NULL,
> >> exynos_adc_remove_devices);
> >> iio_device_unregister(indio_dev);
> >>+ if (info->tsirq > 0)
> >>+ free_irq(info->tsirq, info);
> >> free_irq(info->irq, info);
> >> if (info->data->exit_hw)
> >> info->data->exit_hw(info);
> >>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v5] Input: Add driver for Microchip's CAP1106
From: Dmitry Torokhov @ 2014-07-20 20:42 UTC (permalink / raw)
To: Daniel Mack; +Cc: linux-input, broonie, dh.herrmann, devicetree, mark.rutland
In-Reply-To: <1405423859-16121-1-git-send-email-zonque@gmail.com>
On Tue, Jul 15, 2014 at 01:30:59PM +0200, Daniel Mack wrote:
> This patch adds a driver for Microchips CAP1106, an I2C driven, 6-channel
> capacitive touch sensor.
>
> For now, only the capacitive buttons are supported, and no specific
> settings that can be tweaked for individual channels, except for the
> device-wide sensitivity gain. The defaults seem to work just fine out of
> the box, so I'll leave configurable parameters for someone who's in need
> of them and who can actually measure the impact. All registers are
> prepared, however. Many of them are just not used for now.
>
> The implementation does not make any attempt to be compatible to platform
> data driven boards, but fully depends on CONFIG_OF.
>
> Power management functions are also left for volounteers with the ability
> to actually test them.
>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/1] input: remove unnecessary break after goto
From: Dmitry Torokhov @ 2014-07-20 20:43 UTC (permalink / raw)
To: Fabian Frederick; +Cc: linux-kernel, Paul Gortmaker, linux-input
In-Reply-To: <1404837456-7321-1-git-send-email-fabf@skynet.be>
On Tue, Jul 08, 2014 at 06:37:36PM +0200, Fabian Frederick wrote:
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: linux-input@vger.kernel.org
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
Applied, thank you.
> ---
> drivers/input/misc/keyspan_remote.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/input/misc/keyspan_remote.c b/drivers/input/misc/keyspan_remote.c
> index 01f3b5b..a3fe4a9 100644
> --- a/drivers/input/misc/keyspan_remote.c
> +++ b/drivers/input/misc/keyspan_remote.c
> @@ -392,7 +392,6 @@ static void keyspan_irq_recv(struct urb *urb)
>
> default:
> goto resubmit;
> - break;
> }
>
> if (debug)
> --
> 1.8.4.5
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: st-keyscan - Fix 'defined but not used' compiler warnings
From: Dmitry Torokhov @ 2014-07-20 20:43 UTC (permalink / raw)
To: Tobias Klauser; +Cc: Geert Uytterhoeven, linux-input
In-Reply-To: <1404809278-2604-1-git-send-email-tklauser@distanz.ch>
On Tue, Jul 08, 2014 at 10:47:58AM +0200, Tobias Klauser wrote:
> Add #ifdef CONFIG_PM_SLEEP around keyscan_supend() and keyscan_resume() to fix
> the following compiler warnings occuring if CONFIG_PM_SLEEP is unset:
>
> + /scratch/kisskb/src/drivers/input/keyboard/st-keyscan.c: warning: 'keyscan_resume' defined but not used [-Wunused-function]: => 235:12
> + /scratch/kisskb/src/drivers/input/keyboard/st-keyscan.c: warning: 'keyscan_suspend' defined but not used [-Wunused-function]: => 218:12
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Link: https://lkml.org/lkml/2014/7/8/109
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Applied, thank you.
> ---
> drivers/input/keyboard/st-keyscan.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c
> index 758b487..de7be4f 100644
> --- a/drivers/input/keyboard/st-keyscan.c
> +++ b/drivers/input/keyboard/st-keyscan.c
> @@ -215,6 +215,7 @@ static int keyscan_probe(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> static int keyscan_suspend(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> @@ -249,6 +250,7 @@ static int keyscan_resume(struct device *dev)
> mutex_unlock(&input->mutex);
> return retval;
> }
> +#endif
>
> static SIMPLE_DEV_PM_OPS(keyscan_dev_pm_ops, keyscan_suspend, keyscan_resume);
>
> --
> 2.0.1
>
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH RESEND 2/5] Input: uinput - uinput_validate_absbits() cleanup
From: Dmitry Torokhov @ 2014-07-21 0:34 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Peter Hutterer, Dmitry Torokhov
In-Reply-To: <1405775445-4454-3-git-send-email-dh.herrmann@gmail.com>
On Sat, Jul 19, 2014 at 03:10:42PM +0200, David Herrmann wrote:
> This moves basic checks and setup from uinput_setup_device() into
> uinput_validate_absbits() to make it easier to use. This way, we can call
> it from other places without copying the boilerplate code.
>
> Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Applied, thank you.
> ---
> drivers/input/misc/uinput.c | 40 +++++++++++++++++++++-------------------
> 1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 8569362..615324c 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -311,7 +311,13 @@ static int uinput_open(struct inode *inode, struct file *file)
> static int uinput_validate_absbits(struct input_dev *dev)
> {
> unsigned int cnt;
> - int retval = 0;
> + int nslot;
> +
> + if (!test_bit(EV_ABS, dev->evbit))
> + return 0;
> +
> + /* check if absmin/absmax/absfuzz/absflat are filled as
> + * told in Documentation/input/input-programming.txt */
>
> for (cnt = 0; cnt < ABS_CNT; cnt++) {
> int min, max;
> @@ -327,8 +333,7 @@ static int uinput_validate_absbits(struct input_dev *dev)
> UINPUT_NAME, cnt,
> input_abs_get_min(dev, cnt),
> input_abs_get_max(dev, cnt));
> - retval = -EINVAL;
> - break;
> + return -EINVAL;
> }
>
> if (input_abs_get_flat(dev, cnt) >
> @@ -340,11 +345,18 @@ static int uinput_validate_absbits(struct input_dev *dev)
> input_abs_get_flat(dev, cnt),
> input_abs_get_min(dev, cnt),
> input_abs_get_max(dev, cnt));
> - retval = -EINVAL;
> - break;
> + return -EINVAL;
> }
> }
> - return retval;
> +
> + if (test_bit(ABS_MT_SLOT, dev->absbit)) {
> + nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
> + input_mt_init_slots(dev, nslot, 0);
> + } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
> + input_set_events_per_packet(dev, 60);
> + }
> +
> + return 0;
> }
>
> static int uinput_allocate_device(struct uinput_device *udev)
> @@ -410,19 +422,9 @@ static int uinput_setup_device(struct uinput_device *udev,
> input_abs_set_flat(dev, i, user_dev->absflat[i]);
> }
>
> - /* check if absmin/absmax/absfuzz/absflat are filled as
> - * told in Documentation/input/input-programming.txt */
> - if (test_bit(EV_ABS, dev->evbit)) {
> - retval = uinput_validate_absbits(dev);
> - if (retval < 0)
> - goto exit;
> - if (test_bit(ABS_MT_SLOT, dev->absbit)) {
> - int nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
> - input_mt_init_slots(dev, nslot, 0);
> - } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
> - input_set_events_per_packet(dev, 60);
> - }
> - }
> + retval = uinput_validate_absbits(dev);
> + if (retval < 0)
> + goto exit;
>
> udev->state = UIST_SETUP_COMPLETE;
> retval = count;
> --
> 2.0.2
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH RESEND 3/5] Input: uinput - add UI_GET_VERSION ioctl
From: Dmitry Torokhov @ 2014-07-21 0:34 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Peter Hutterer, Dmitry Torokhov
In-Reply-To: <1405775445-4454-4-git-send-email-dh.herrmann@gmail.com>
On Sat, Jul 19, 2014 at 03:10:43PM +0200, David Herrmann wrote:
> This ioctl is the counterpart to EVIOCGVERSION and returns the
> uinput-version the kernel was compiled with.
>
> Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Applied, thank you.
> ---
> drivers/input/misc/uinput.c | 6 ++++++
> include/uapi/linux/uinput.h | 9 +++++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 615324c..a2a3895 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -722,6 +722,12 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> }
>
> switch (cmd) {
> + case UI_GET_VERSION:
> + if (put_user(UINPUT_VERSION,
> + (unsigned int __user*)p))
> + retval = -EFAULT;
> + goto out;
> +
> case UI_DEV_CREATE:
> retval = uinput_create_device(udev);
> goto out;
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index 0389b48..19339cf 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -84,6 +84,15 @@ struct uinput_ff_erase {
> */
> #define UI_GET_SYSNAME(len) _IOC(_IOC_READ, UINPUT_IOCTL_BASE, 300, len)
>
> +/**
> + * UI_GET_VERSION - Return uinput version of the kernel
> + *
> + * This writes the kernel's uinput version into the integer pointed to by the
> + * ioctl argument. The uinput-version is hard-coded in the kernel and
> + * independent of the uinput device.
> + */
> +#define UI_GET_VERSION _IOR(UINPUT_IOCTL_BASE, 301, unsigned int)
> +
> /*
> * To write a force-feedback-capable driver, the upload_effect
> * and erase_effect callbacks in input_dev must be implemented.
> --
> 2.0.2
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH RESEND 0/5] Evdev Extensions
From: Dmitry Torokhov @ 2014-07-21 0:37 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Peter Hutterer
In-Reply-To: <1405775445-4454-1-git-send-email-dh.herrmann@gmail.com>
Hi David,
On Sat, Jul 19, 2014 at 03:10:40PM +0200, David Herrmann wrote:
> Hi Dmitry
>
> I posted all these ~2 months ago, but haven't seen any comments from you. Seeing
> that you switched jobs (congratulations, btw!),
Thanks :)
>I guess you were quite busy the
> last few weeks. Hence, here's a resend of all the evdev changes squashed into a
> single series. They're also available in my fdo repo [1] in case you wanna fetch
> them directly.
>
> No big changes except for a single "udev->state == UIST_CREATED" fix spotted by
> Peter.
>
> The last patch is still RFC, so please omit it in case you apply the patches!
>
> All patches but the last are tested via libevdev and run fine here since several
> months.
OK, I am looking at/applying them.
BTW, please continue using my @gmail.com address for upstream input
stuff and @google if it is something corporate related.
Thanks!
--
Dmitry
^ permalink raw reply
* Re: [PATCH RESEND 4/5] Input: uinput - add new UINPUT_DEV_SETUP ioctl
From: Dmitry Torokhov @ 2014-07-21 1:01 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Peter Hutterer, Dmitry Torokhov
In-Reply-To: <1405775445-4454-5-git-send-email-dh.herrmann@gmail.com>
Hi David,
On Sat, Jul 19, 2014 at 03:10:44PM +0200, David Herrmann wrote:
> This adds a new ioctl UINPUT_DEV_SETUP that replaces the old device setup
> method (by write()'ing "struct uinput_user_dev" to the node). The old
> method is not easily extendable and requires huge payloads. Furthermore,
> overloading write() without properly versioned objects is error-prone.
>
> Therefore, we introduce a new ioctl to replace the old method. The ioctl
> supports all features of the old method, plus a "resolution" field for
> absinfo. Furthermore, it's properly forward-compatible to new ABS codes
> and a growing "struct input_absinfo" structure.
>
> The ioctl also allows user-space to skip unknown axes if not set. The
> payload-size can now be specified by the caller. There is no need to copy
> the whole array temporarily into the kernel, but instead we can iterate
> over it and copy each value manually.
>
> Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
> include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
> 2 files changed, 118 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index a2a3895..0f45595 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -371,8 +371,67 @@ static int uinput_allocate_device(struct uinput_device *udev)
> return 0;
> }
>
> -static int uinput_setup_device(struct uinput_device *udev,
> - const char __user *buffer, size_t count)
> +static int uinput_dev_setup(struct uinput_device *udev,
> + struct uinput_setup __user *arg)
> +{
> + struct uinput_setup setup;
> + struct input_dev *dev;
> + int i, retval;
> +
> + if (udev->state == UIST_CREATED)
> + return -EINVAL;
> + if (copy_from_user(&setup, arg, sizeof(setup)))
> + return -EFAULT;
> + if (!setup.name[0])
> + return -EINVAL;
> + /* So far we only support the original "struct input_absinfo", but be
> + * forward compatible and allow larger payloads. */
> + if (setup.absinfo_size < sizeof(struct input_absinfo))
> + return -EINVAL;
No, we can not do this, as it breaks backward compatibility (the most
important one!). If we were to increase size of in-kernel input_absinfo
in let's say 3.20, userspace compiled against older kernel headers
(but using the new ioctl available let's say since 3.16 - don't hold me
to the numbers ;) ), would break since it wold start tripping on thi
check.
The proper way to handle it is to convert "old" absinfo into new one,
applying as much as we can.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] Input: cap1106 - allow changing key mapping from userspace
From: Dmitry Torokhov @ 2014-07-21 1:20 UTC (permalink / raw)
To: Daniel Mack; +Cc: linux-input, linux-kernel
Wire up support for EVIOC{G|S}KEYCODE to allow users change key mappings
from userspace.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
Just compiled, not tested.
drivers/input/keyboard/cap1106.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/cap1106.c b/drivers/input/keyboard/cap1106.c
index f7d7a0d..180b184 100644
--- a/drivers/input/keyboard/cap1106.c
+++ b/drivers/input/keyboard/cap1106.c
@@ -64,7 +64,7 @@ struct cap1106_priv {
struct input_dev *idev;
/* config */
- unsigned int keycodes[CAP1106_NUM_CHN];
+ unsigned short keycodes[CAP1106_NUM_CHN];
};
static const struct reg_default cap1106_reg_defaults[] = {
@@ -272,6 +272,12 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
for (i = 0; i < CAP1106_NUM_CHN; i++)
__set_bit(priv->keycodes[i], priv->idev->keybit);
+ __clear_bit(KEY_RESERVED, priv->idev->keybit);
+
+ priv->idev->keycode = priv->keycodes;
+ priv->idev->keycodesize = sizeof(priv->keycodes[0]);
+ priv->idev->keycodemax = ARRAY_SIZE(priv->keycodes);
+
priv->idev->id.vendor = CAP1106_MANUFACTURER_ID;
priv->idev->id.product = CAP1106_PRODUCT_ID;
priv->idev->id.version = rev;
--
2.0.0.526.g5318336
--
Dmitry
^ permalink raw reply related
* Re: [PATCH RESEND 4/5] Input: uinput - add new UINPUT_DEV_SETUP ioctl
From: David Herrmann @ 2014-07-21 6:22 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: open list:HID CORE LAYER, Peter Hutterer
In-Reply-To: <20140721010110.GD22552@core.coreip.homeip.net>
Hi
On Mon, Jul 21, 2014 at 3:01 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi David,
>
> On Sat, Jul 19, 2014 at 03:10:44PM +0200, David Herrmann wrote:
>> This adds a new ioctl UINPUT_DEV_SETUP that replaces the old device setup
>> method (by write()'ing "struct uinput_user_dev" to the node). The old
>> method is not easily extendable and requires huge payloads. Furthermore,
>> overloading write() without properly versioned objects is error-prone.
>>
>> Therefore, we introduce a new ioctl to replace the old method. The ioctl
>> supports all features of the old method, plus a "resolution" field for
>> absinfo. Furthermore, it's properly forward-compatible to new ABS codes
>> and a growing "struct input_absinfo" structure.
>>
>> The ioctl also allows user-space to skip unknown axes if not set. The
>> payload-size can now be specified by the caller. There is no need to copy
>> the whole array temporarily into the kernel, but instead we can iterate
>> over it and copy each value manually.
>>
>> Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
>> include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
>> 2 files changed, 118 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> index a2a3895..0f45595 100644
>> --- a/drivers/input/misc/uinput.c
>> +++ b/drivers/input/misc/uinput.c
>> @@ -371,8 +371,67 @@ static int uinput_allocate_device(struct uinput_device *udev)
>> return 0;
>> }
>>
>> -static int uinput_setup_device(struct uinput_device *udev,
>> - const char __user *buffer, size_t count)
>> +static int uinput_dev_setup(struct uinput_device *udev,
>> + struct uinput_setup __user *arg)
>> +{
>> + struct uinput_setup setup;
>> + struct input_dev *dev;
>> + int i, retval;
>> +
>> + if (udev->state == UIST_CREATED)
>> + return -EINVAL;
>> + if (copy_from_user(&setup, arg, sizeof(setup)))
>> + return -EFAULT;
>> + if (!setup.name[0])
>> + return -EINVAL;
>> + /* So far we only support the original "struct input_absinfo", but be
>> + * forward compatible and allow larger payloads. */
>> + if (setup.absinfo_size < sizeof(struct input_absinfo))
>> + return -EINVAL;
>
> No, we can not do this, as it breaks backward compatibility (the most
> important one!). If we were to increase size of in-kernel input_absinfo
> in let's say 3.20, userspace compiled against older kernel headers
> (but using the new ioctl available let's say since 3.16 - don't hold me
> to the numbers ;) ), would break since it wold start tripping on thi
> check.
>
> The proper way to handle it is to convert "old" absinfo into new one,
> applying as much as we can.
I know, but there is no "old absinfo". Once we extend "struct absinfo"
I expect this code to change to:
{
/* initially supported absinfo had size 24 */
if (setup.absinfo_size < 24)
return -EINVAL;
/* ...pseudo code... */
memset(&absinfo, 0, sizeof(absinfo));
memcpy(&absinfo, sth->absinfo, MIN(setup.absinfo_size,
sizeof(absinfo)));
}
This allows you to use this ioctl with old absinfo objects. I can
change the current code to this already, if you want? I tried to avoid
it, because a memset() is not neccessarily an appropriate way to
initialize unset fields.
I cal also add support for "absinfo" without the "resolution" field,
which I think is the only field that wasn't available in the initial
structure.
Let me know which way you prefer.
Thanks
David
^ permalink raw reply
* Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
From: Arnd Bergmann @ 2014-07-21 10:23 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jonathan Cameron, linux-arm-kernel, Chanwoo Choi, ch.naveen,
mark.rutland, devicetree, kgene.kim, pawel.moll, ijc+devicetree,
linux-iio, t.figa, rdunlap, linux-doc, linux-kernel,
linux-samsung-soc, kyungmin.park, robh+dt, galak, heiko.stuebner,
Ben Dooks, linux-input
In-Reply-To: <20140720202842.GB18347@core.coreip.homeip.net>
On Sunday 20 July 2014 13:28:42 Dmitry Torokhov wrote:
> On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
> > >>+
> > >>+ do {
> > >>+ ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> > >= exynos
> > >>+ if (ret == -ETIMEDOUT)
> > >>+ break;
> > >>+
> > >>+ pressed = x & y & ADC_DATX_PRESSED;
> > >>+ if (!pressed)
> > >>+ break;
> > >>+
> > >>+ input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
> > >>+ input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
> > >>+ input_report_key(info->input, BTN_TOUCH, 1);
> > >>+ input_sync(info->input);
> > >>+
> > >>+ msleep(1);
> > >>+ } while (1);
>
> It would be nice to actually close the device even if someone is
> touching screen. Please implement open/close methods and have them set a
> flag that you would check here.
Ok. I think it's even better to move the request_irq() into the open function,
which will avoid the flag and defer the error handling into the actual opening,
as well as syncing the running irq with the close function.
> > >>+ /* data from s3c2410_ts driver */
> > >>+ info->input->name = "S3C24xx TouchScreen";
> > >>+ info->input->id.bustype = BUS_HOST;
> > >>+ info->input->id.vendor = 0xDEAD;
> > >>+ info->input->id.product = 0xBEEF;
>
> You do not need to fill these entries with fake data.
Ok, I wondered about this, but didn't want to change too much from
the old driver (I changed the version number).
> > >>+ info->input->id.version = 0x0200;
Do I need this?
> > >>@@ -576,6 +728,11 @@ static int exynos_adc_probe(struct platform_device *pdev)
> > >> err_of_populate:
> > >> device_for_each_child(&indio_dev->dev, NULL,
> > >> exynos_adc_remove_devices);
> > >>+ if (has_ts) {
> > >>+ input_unregister_device(info->input);
> > >>+ free_irq(info->tsirq, info);
>
> Are we sure that device is quiesced here and interrupt won't arrive
> between input_unregister_device() and free_irq()? I guess if you
> properly implement open/close it won't matter.
Should be fixed now.
> > >>+err_iio:
> > >> iio_device_unregister(indio_dev);
> > >> err_irq:
> > >> free_irq(info->irq, info);
> > >>@@ -595,9 +752,12 @@ static int exynos_adc_remove(struct platform_device *pdev)
> > >> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > >> struct exynos_adc *info = iio_priv(indio_dev);
> > >>
> > >>+ input_free_device(info->input);
>
> Should be unregister, not free.
Ok.
Thanks a lot for the review! I'll send out the new version after some build testing.
Arnd
^ permalink raw reply
* Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
From: Arnd Bergmann @ 2014-07-21 10:26 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jonathan Cameron, linux-arm-kernel, Chanwoo Choi, ch.naveen,
mark.rutland, devicetree, kgene.kim, pawel.moll, ijc+devicetree,
linux-iio, t.figa, rdunlap, linux-doc, linux-kernel,
linux-samsung-soc, kyungmin.park, robh+dt, galak, heiko.stuebner,
Ben Dooks, linux-input
In-Reply-To: <5158672.dMAnnUQc7Z@wuerfel>
On Monday 21 July 2014 12:23:58 Arnd Bergmann wrote:
>
> Thanks a lot for the review! I'll send out the new version after some build testing.
Here are the changes I did to the adc driver based on the review comments.
I'll start a new thread for the updated version that includes the changes.
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 3d2caea05977..823d7ebc7f52 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -67,6 +67,9 @@
#define ADC_S3C2410_CON_SELMUX(x) (((x)&0x7)<<3)
+/* touch screen always uses channel 0 */
+#define ADC_S3C2410_MUX_TS 0
+
/* ADCTSC Register Bits */
#define ADC_S3C2443_TSC_UD_SEN (1u << 8)
#define ADC_S3C2410_TSC_YM_SEN (1u << 7)
@@ -106,6 +109,7 @@
#define ADC_CON_EN_START (1u << 0)
#define ADC_DATX_PRESSED (1u << 15)
#define ADC_DATX_MASK 0xFFF
+#define ADC_DATY_MASK 0xFFF
#define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
@@ -123,10 +127,12 @@ struct exynos_adc {
struct completion completion;
- bool read_ts;
u32 value;
- u32 value2;
unsigned int version;
+
+ bool read_ts;
+ u32 ts_x;
+ u32 ts_y;
};
struct exynos_adc_data {
@@ -396,21 +402,14 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
return ret;
}
-static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
- struct iio_chan_spec const *chan,
- int *val,
- int *val2,
- long mask)
+static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev, int *x, int *y)
{
struct exynos_adc *info = iio_priv(indio_dev);
unsigned long timeout;
int ret;
- if (mask != IIO_CHAN_INFO_RAW)
- return -EINVAL;
-
mutex_lock(&indio_dev->mlock);
- info->read_ts = 1;
+ info->read_ts = true;
reinit_completion(&info->completion);
@@ -418,7 +417,7 @@ static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
ADC_V1_TSC(info->regs));
/* Select the ts channel to be used and Trigger conversion */
- info->data->start_conv(info, 0);
+ info->data->start_conv(info, ADC_S3C2410_MUX_TS);
timeout = wait_for_completion_timeout
(&info->completion, EXYNOS_ADC_TIMEOUT);
@@ -428,12 +427,12 @@ static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
info->data->init_hw(info);
ret = -ETIMEDOUT;
} else {
- *val = info->value;
- *val2 = info->value2;
- ret = IIO_VAL_INT;
+ *x = info->ts_x;
+ *y = info->ts_y;
+ ret = 0;
}
- info->read_ts = 0;
+ info->read_ts = false;
mutex_unlock(&indio_dev->mlock);
return ret;
@@ -445,8 +444,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
/* Read value */
if (info->read_ts) {
- info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
- info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
+ info->ts_x = readl(ADC_V1_DATX(info->regs));
+ info->ts_y = readl(ADC_V1_DATY(info->regs));
writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
} else {
info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
@@ -477,7 +476,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
int ret;
do {
- ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
+ ret = exynos_read_s3c64xx_ts(dev, &x, &y);
if (ret == -ETIMEDOUT)
break;
@@ -486,11 +485,15 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
break;
input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
- input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
+ input_report_abs(info->input, ABS_Y, y & ADC_DATY_MASK);
input_report_key(info->input, BTN_TOUCH, 1);
input_sync(info->input);
msleep(1);
+
+ /* device may have been closed while touched */
+ if (!info->input->users)
+ return IRQ_HANDLED;
} while (1);
input_report_key(info->input, BTN_TOUCH, 0);
@@ -552,10 +555,28 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
return 0;
}
+static int exynos_adc_ts_open(struct input_dev *dev)
+{
+ struct exynos_adc *info = input_get_drvdata(dev);
+
+ return request_threaded_irq(info->tsirq, NULL, exynos_ts_isr,
+ 0, "touchscreen", info);
+}
+
+static void exynos_adc_ts_close(struct input_dev *dev)
+{
+ struct exynos_adc *info = input_get_drvdata(dev);
+
+ free_irq(info->tsirq, info);
+}
+
static int exynos_adc_ts_init(struct exynos_adc *info)
{
int ret;
+ if (info->tsirq <= 0)
+ return -ENODEV;
+
info->input = input_allocate_device();
if (!info->input)
return -ENOMEM;
@@ -566,33 +587,17 @@ static int exynos_adc_ts_init(struct exynos_adc *info)
input_set_abs_params(info->input, ABS_X, 0, 0x3FF, 0, 0);
input_set_abs_params(info->input, ABS_Y, 0, 0x3FF, 0, 0);
- /* data from s3c2410_ts driver */
info->input->name = "S3C24xx TouchScreen";
info->input->id.bustype = BUS_HOST;
- info->input->id.vendor = 0xDEAD;
- info->input->id.product = 0xBEEF;
- info->input->id.version = 0x0200;
+ info->input->open = exynos_adc_ts_open;
+ info->input->close = exynos_adc_ts_close;
+
+ input_set_drvdata(info->input, info);
ret = input_register_device(info->input);
- if (ret) {
+ if (ret)
input_free_device(info->input);
- goto err;
- }
-
- if (info->tsirq > 0)
- ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
- 0, "touchscreen", info);
- if (ret < 0) {
- dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
- info->irq);
- goto err_input;
- }
-
- return 0;
-err_input:
- input_unregister_device(info->input);
-err:
return ret;
}
@@ -602,7 +607,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
struct device_node *np = pdev->dev.of_node;
struct iio_dev *indio_dev = NULL;
struct resource *mem;
- bool has_ts;
+ bool has_ts = false;
int ret = -ENODEV;
int irq;
@@ -711,7 +716,10 @@ static int exynos_adc_probe(struct platform_device *pdev)
if (info->data->init_hw)
info->data->init_hw(info);
- has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
+ /* leave out any TS related code if unreachable */
+ if (IS_BUILTIN(CONFIG_INPUT) ||
+ (IS_MODULE(CONFIG_INPUT) && config_enabled(MODULE)))
+ has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
if (has_ts)
ret = exynos_adc_ts_init(info);
if (ret)
@@ -752,7 +760,7 @@ static int exynos_adc_remove(struct platform_device *pdev)
struct iio_dev *indio_dev = platform_get_drvdata(pdev);
struct exynos_adc *info = iio_priv(indio_dev);
- input_free_device(info->input);
+ input_unregister_device(info->input);
device_for_each_child(&indio_dev->dev, NULL,
exynos_adc_remove_devices);
iio_device_unregister(indio_dev);
^ permalink raw reply related
* Re: [PATCH v4 1/4] Rename hid-lenovo-tpkbd to hid-lenovo
From: Antonio Ospite @ 2014-07-21 13:55 UTC (permalink / raw)
To: Jamie Lentin; +Cc: Jiri Kosina, Hans de Goede, linux-input, linux-kernel
In-Reply-To: <alpine.DEB.2.02.1407181016170.17078@marmot.wormnet.eu>
On Fri, 18 Jul 2014 10:35:35 +0100 (BST)
Jamie Lentin <jm@lentin.co.uk> wrote:
> On Mon, 14 Jul 2014, Antonio Ospite wrote:
>
> > On Sun, 13 Jul 2014 08:24:19 +0100
> > Jamie Lentin <jm@lentin.co.uk> wrote:
> >
> >> Rename module and all functions within so we can add support for other
> >> keyboards in the same file. Rename the _tp postfix to _tpkbd, to
> >> signify functions relevant to the TP USB keyboard.
> >>
> >> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> >
> > Reviewed-by: Antonio Ospite <ao2@ao2.it>
> >
> > Patch looks good to me, easier to validate now :)
> >
> > A few ideas below for some possible _future_ cleanups, but no reason to
> > hold this one any longer IMHO.
> >
> > Ciao,
> > Antonio
> >
> >> ---
> >> ...er-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} | 0
> >> drivers/hid/Kconfig | 14 +-
> >> drivers/hid/Makefile | 2 +-
> >> drivers/hid/hid-core.c | 2 +-
> >> drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} | 185 +++++++++++----------
> >> 5 files changed, 102 insertions(+), 101 deletions(-)
> >> rename Documentation/ABI/testing/{sysfs-driver-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} (100%)
> >> rename drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} (64%)
> >>
[...]
> >> -static int tpkbd_features_set(struct hid_device *hdev)
> >> +static int lenovo_features_set_tpkbd(struct hid_device *hdev)
> >> {
> >> struct hid_report *report;
> >> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> >> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
> >
> >
> > I think the word "pointer" here is misleading, sometimes in other parts
> > of the driver it is used as a synonym of trackpoint, but
> > lenovo_drvdata_tpkbd also contains data about leds, so here it looks
> > like it refers to C pointers. I'd call the variable tpkbd_data or
> > tpkbd_drvdata, throughout the driver. What do you think?
>
> Yes, the rationale behind the name really isn't obvious but all meanings
> are sorta correct, so it's not that bad :P
>
> I like tpkbd_drvdata better though, and can correct it in the compact
> keyboard code too. I wanted to make the variable names match, but didn't
> get around to it, so will do it next time around.
>
IMHO you can very well use data_pointer for now so the names are
consistent with the current ones; a cleanup can be done later to make
the naming more straightforward too.
> > And the driver could standardize on the word "trackpoint" where
> > appropriate in order to eliminate any ambiguity.
>
> The key other place I can think of that this happens is for the USB
> compact keyboard, for it's HID_TYPE_USBMOUSE half (versus the keyboard
> half). Saying trackpoint there is a mis-truth, since it's also where all
> the extra keys hide. But I'll have a look through anyway.
>
I meant: where "pointer" means trackpoint let's just use "trackpoint",
where pointer means "not keyboard" (like in data_pointer apparently)
something else can be used if "trackpoint" is too specific.
But again, this is cleanup: it'd be nice to have it eventually but not
worth blocking this patchset.
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
From: Dmitry Torokhov @ 2014-07-21 14:44 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jonathan Cameron, linux-arm-kernel, Chanwoo Choi, ch.naveen,
mark.rutland, devicetree, kgene.kim, pawel.moll, ijc+devicetree,
linux-iio, t.figa, rdunlap, linux-doc, linux-kernel,
linux-samsung-soc, kyungmin.park, robh+dt, galak, heiko.stuebner,
Ben Dooks, linux-input
In-Reply-To: <5158672.dMAnnUQc7Z@wuerfel>
On Mon, Jul 21, 2014 at 12:23:58PM +0200, Arnd Bergmann wrote:
> On Sunday 20 July 2014 13:28:42 Dmitry Torokhov wrote:
> > On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
> > > >>+
> > > >>+ do {
> > > >>+ ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> > > >= exynos
> > > >>+ if (ret == -ETIMEDOUT)
> > > >>+ break;
> > > >>+
> > > >>+ pressed = x & y & ADC_DATX_PRESSED;
> > > >>+ if (!pressed)
> > > >>+ break;
> > > >>+
> > > >>+ input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
> > > >>+ input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
> > > >>+ input_report_key(info->input, BTN_TOUCH, 1);
> > > >>+ input_sync(info->input);
> > > >>+
> > > >>+ msleep(1);
> > > >>+ } while (1);
> >
> > It would be nice to actually close the device even if someone is
> > touching screen. Please implement open/close methods and have them set a
> > flag that you would check here.
>
> Ok. I think it's even better to move the request_irq() into the open function,
> which will avoid the flag and defer the error handling into the actual opening,
> as well as syncing the running irq with the close function.
I do not quite like acquiring resources needed in open. I think drivers should
do all resource acquisition in probe() and leave open()/close() to
activate/quiesce devices.
>
> > > >>+ /* data from s3c2410_ts driver */
> > > >>+ info->input->name = "S3C24xx TouchScreen";
> > > >>+ info->input->id.bustype = BUS_HOST;
> > > >>+ info->input->id.vendor = 0xDEAD;
> > > >>+ info->input->id.product = 0xBEEF;
> >
> > You do not need to fill these entries with fake data.
>
> Ok, I wondered about this, but didn't want to change too much from
> the old driver (I changed the version number).
>
> > > >>+ info->input->id.version = 0x0200;
>
> Do I need this?
Not really.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
From: Arnd Bergmann @ 2014-07-21 15:11 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, devicetree, kgene.kim, pawel.moll, ijc+devicetree,
linux-iio, t.figa, Dmitry Torokhov, linux-doc, linux-kernel,
Jonathan Cameron, Chanwoo Choi, kyungmin.park, robh+dt, rdunlap,
Ben Dooks, galak, ch.naveen, linux-input, linux-samsung-soc,
heiko.stuebner
In-Reply-To: <20140721144442.GB9112@core.coreip.homeip.net>
On Monday 21 July 2014 07:44:42 Dmitry Torokhov wrote:
> > >
> > > It would be nice to actually close the device even if someone is
> > > touching screen. Please implement open/close methods and have them set a
> > > flag that you would check here.
> >
> > Ok. I think it's even better to move the request_irq() into the open function,
> > which will avoid the flag and defer the error handling into the actual opening,
> > as well as syncing the running irq with the close function.
>
> I do not quite like acquiring resources needed in open. I think drivers should
> do all resource acquisition in probe() and leave open()/close() to
> activate/quiesce devices.
Ok, I'll move it back then. I'm not sure what I'm supposed to do
in open/close then. Isn't it enough to check info->input->users
like this?
static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
{
struct exynos_adc *info = dev_id;
struct iio_dev *dev = dev_get_drvdata(info->dev);
u32 x, y;
bool pressed;
int ret;
while (info->input->users) {
ret = exynos_read_s3c64xx_ts(dev, &x, &y);
if (ret == -ETIMEDOUT)
break;
pressed = x & y & ADC_DATX_PRESSED;
if (!pressed) {
input_report_key(info->input, BTN_TOUCH, 0);
input_sync(info->input);
break;
}
input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
input_report_abs(info->input, ABS_Y, y & ADC_DATY_MASK);
input_report_key(info->input, BTN_TOUCH, 1);
input_sync(info->input);
msleep(1);
};
writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
return IRQ_HANDLED;
}
I could do enable_irq()/disable_irq(), but that leaves a small
race at startup where we request the irq line and then immediately
disable it again.
Arnd
^ permalink raw reply
* [PATCH RESEND] input: zforce: add regulator handling
From: Heiko Stübner @ 2014-07-21 15:20 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Henrik Rydberg, linux-input, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, devicetree
In-Reply-To: <2587208.XYbbI8xJiH@phil>
From: Heiko Stuebner <heiko.stuebner@bq.com>
It's possible that the controller has an individually switchable power supply.
Therefore add support to control a supplying regulator.
As this is not always the case, the regulator is requested as optional.
Signed-off-by: Heiko Stuebner <heiko.stuebner@bq.com>
---
.../bindings/input/touchscreen/zforce_ts.txt | 4 +++
drivers/input/touchscreen/zforce_ts.c | 30 ++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
index 2faf1f1..80c37df 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
@@ -9,6 +9,9 @@ Required properties:
- x-size: horizontal resolution of touchscreen
- y-size: vertical resolution of touchscreen
+Optional properties:
+- vdd-supply: Regulator controlling the controller supply
+
Example:
i2c@00000000 {
@@ -18,6 +21,7 @@ Example:
compatible = "neonode,zforce";
reg = <0x50>;
interrupts = <2 0>;
+ vdd-supply = <®_zforce_vdd>;
gpios = <&gpio5 6 0>, /* INT */
<&gpio5 9 0>; /* RST */
diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
index 01d30ce..39ca962 100644
--- a/drivers/input/touchscreen/zforce_ts.c
+++ b/drivers/input/touchscreen/zforce_ts.c
@@ -29,6 +29,8 @@
#include <linux/sysfs.h>
#include <linux/input/mt.h>
#include <linux/platform_data/zforce_ts.h>
+#include <linux/regulator/consumer.h>
+#include <linux/delay.h>
#include <linux/of.h>
#include <linux/of_gpio.h>
@@ -117,6 +119,8 @@ struct zforce_ts {
const struct zforce_ts_platdata *pdata;
char phys[32];
+ struct regulator *reg_vdd;
+
bool suspending;
bool suspended;
bool boot_complete;
@@ -690,6 +694,11 @@ static void zforce_reset(void *data)
struct zforce_ts *ts = data;
gpio_set_value(ts->pdata->gpio_rst, 0);
+
+ udelay(10);
+
+ if (!IS_ERR(ts->reg_vdd))
+ regulator_disable(ts->reg_vdd);
}
static struct zforce_ts_platdata *zforce_parse_dt(struct device *dev)
@@ -765,10 +774,31 @@ static int zforce_probe(struct i2c_client *client,
return ret;
}
+ ts->reg_vdd = devm_regulator_get_optional(&client->dev, "vdd");
+ if (IS_ERR(ts->reg_vdd) && PTR_ERR(ts->reg_vdd) == -EPROBE_DEFER)
+ return PTR_ERR(ts->reg_vdd);
+
+ if (!IS_ERR(ts->reg_vdd)) {
+ ret = regulator_enable(ts->reg_vdd);
+ if (ret)
+ return ret;
+
+ /*
+ * according to datasheet add 100us grace time after regular
+ * regulator enable delay.
+ */
+ udelay(100);
+ }
+
ret = devm_add_action(&client->dev, zforce_reset, ts);
if (ret) {
dev_err(&client->dev, "failed to register reset action, %d\n",
ret);
+
+ /* hereafter the regulator will be disabled by the action */
+ if (!IS_ERR(ts->reg_vdd))
+ regulator_disable(ts->reg_vdd);
+
return ret;
}
--
1.9.0
^ permalink raw reply related
* Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
From: Dmitry Torokhov @ 2014-07-21 16:19 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arm-kernel, mark.rutland, devicetree, kgene.kim, pawel.moll,
ijc+devicetree, linux-iio, t.figa, rdunlap, linux-doc,
linux-kernel, robh+dt, Chanwoo Choi, kyungmin.park,
linux-samsung-soc, Jonathan Cameron, Ben Dooks, galak, ch.naveen,
linux-input, heiko.stuebner
In-Reply-To: <6569004.FG0IRy8WyL@wuerfel>
On Mon, Jul 21, 2014 at 05:11:27PM +0200, Arnd Bergmann wrote:
> On Monday 21 July 2014 07:44:42 Dmitry Torokhov wrote:
> > > >
> > > > It would be nice to actually close the device even if someone is
> > > > touching screen. Please implement open/close methods and have them set a
> > > > flag that you would check here.
> > >
> > > Ok. I think it's even better to move the request_irq() into the open function,
> > > which will avoid the flag and defer the error handling into the actual opening,
> > > as well as syncing the running irq with the close function.
> >
> > I do not quite like acquiring resources needed in open. I think drivers should
> > do all resource acquisition in probe() and leave open()/close() to
> > activate/quiesce devices.
>
> Ok, I'll move it back then. I'm not sure what I'm supposed to do
> in open/close then. Isn't it enough to check info->input->users
> like this?
>
> static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> {
> struct exynos_adc *info = dev_id;
> struct iio_dev *dev = dev_get_drvdata(info->dev);
> u32 x, y;
> bool pressed;
> int ret;
>
> while (info->input->users) {
> ret = exynos_read_s3c64xx_ts(dev, &x, &y);
> if (ret == -ETIMEDOUT)
> break;
>
> pressed = x & y & ADC_DATX_PRESSED;
> if (!pressed) {
> input_report_key(info->input, BTN_TOUCH, 0);
> input_sync(info->input);
> break;
> }
>
> input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
> input_report_abs(info->input, ABS_Y, y & ADC_DATY_MASK);
> input_report_key(info->input, BTN_TOUCH, 1);
> input_sync(info->input);
>
> msleep(1);
> };
>
> writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
>
> return IRQ_HANDLED;
> }
>
> I could do enable_irq()/disable_irq(), but that leaves a small
> race at startup where we request the irq line and then immediately
> disable it again.
I think the above (or a separate flag in driver structure) coupled with
enable/disable IRQ is fine - input core can deal with calls to input_report_*
on devices that have been properly allocated but have not been registered yet.
drivers/input/keyboard/samsung-keypad.c does similar handling.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH RESEND] input: zforce: add regulator handling
From: Dmitry Torokhov @ 2014-07-21 17:42 UTC (permalink / raw)
To: Heiko Stübner
Cc: Henrik Rydberg, linux-input, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, devicetree
In-Reply-To: <2184295.fEH4OJRW3F@diego>
On Mon, Jul 21, 2014 at 05:20:11PM +0200, Heiko Stübner wrote:
> From: Heiko Stuebner <heiko.stuebner@bq.com>
>
> It's possible that the controller has an individually switchable power supply.
> Therefore add support to control a supplying regulator.
>
> As this is not always the case, the regulator is requested as optional.
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@bq.com>
Applied (with minimal edits to avoid repeated IS_ERR/PTR_ERR), thank
you.
> ---
> .../bindings/input/touchscreen/zforce_ts.txt | 4 +++
> drivers/input/touchscreen/zforce_ts.c | 30 ++++++++++++++++++++++
> 2 files changed, 34 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> index 2faf1f1..80c37df 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> @@ -9,6 +9,9 @@ Required properties:
> - x-size: horizontal resolution of touchscreen
> - y-size: vertical resolution of touchscreen
>
> +Optional properties:
> +- vdd-supply: Regulator controlling the controller supply
> +
> Example:
>
> i2c@00000000 {
> @@ -18,6 +21,7 @@ Example:
> compatible = "neonode,zforce";
> reg = <0x50>;
> interrupts = <2 0>;
> + vdd-supply = <®_zforce_vdd>;
>
> gpios = <&gpio5 6 0>, /* INT */
> <&gpio5 9 0>; /* RST */
> diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
> index 01d30ce..39ca962 100644
> --- a/drivers/input/touchscreen/zforce_ts.c
> +++ b/drivers/input/touchscreen/zforce_ts.c
> @@ -29,6 +29,8 @@
> #include <linux/sysfs.h>
> #include <linux/input/mt.h>
> #include <linux/platform_data/zforce_ts.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/delay.h>
> #include <linux/of.h>
> #include <linux/of_gpio.h>
>
> @@ -117,6 +119,8 @@ struct zforce_ts {
> const struct zforce_ts_platdata *pdata;
> char phys[32];
>
> + struct regulator *reg_vdd;
> +
> bool suspending;
> bool suspended;
> bool boot_complete;
> @@ -690,6 +694,11 @@ static void zforce_reset(void *data)
> struct zforce_ts *ts = data;
>
> gpio_set_value(ts->pdata->gpio_rst, 0);
> +
> + udelay(10);
> +
> + if (!IS_ERR(ts->reg_vdd))
> + regulator_disable(ts->reg_vdd);
> }
>
> static struct zforce_ts_platdata *zforce_parse_dt(struct device *dev)
> @@ -765,10 +774,31 @@ static int zforce_probe(struct i2c_client *client,
> return ret;
> }
>
> + ts->reg_vdd = devm_regulator_get_optional(&client->dev, "vdd");
> + if (IS_ERR(ts->reg_vdd) && PTR_ERR(ts->reg_vdd) == -EPROBE_DEFER)
> + return PTR_ERR(ts->reg_vdd);
> +
> + if (!IS_ERR(ts->reg_vdd)) {
> + ret = regulator_enable(ts->reg_vdd);
> + if (ret)
> + return ret;
> +
> + /*
> + * according to datasheet add 100us grace time after regular
> + * regulator enable delay.
> + */
> + udelay(100);
> + }
> +
> ret = devm_add_action(&client->dev, zforce_reset, ts);
> if (ret) {
> dev_err(&client->dev, "failed to register reset action, %d\n",
> ret);
> +
> + /* hereafter the regulator will be disabled by the action */
> + if (!IS_ERR(ts->reg_vdd))
> + regulator_disable(ts->reg_vdd);
> +
> return ret;
> }
>
> --
> 1.9.0
>
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] input: Add support for Wacom protocol 4 serial tablets
From: Dmitry Torokhov @ 2014-07-21 17:56 UTC (permalink / raw)
To: Hans de Goede; +Cc: Julian Squires, linux-input
In-Reply-To: <1405867583-3321-1-git-send-email-hdegoede@redhat.com>
On Sun, Jul 20, 2014 at 04:46:23PM +0200, Hans de Goede wrote:
> Recent version of xf86-input-wacom no longer support directly accessing
> serial tablets. Instead xf86-input-wacom now expects all wacom tablets to
> be driven by the kernel and to show up as evdev devices.
>
> This has caused old serial Wacom tablets to stop working for people who still
> have such tablets. Julian Squires has written a serio input driver to fix this:
> https://github.com/tokenrove/wacom-serial-iv
>
> This is a cleaned up version of this driver with improved Graphire support
> (I own an old Graphire myself).
>
> Signed-off-by: Julian Squires <julian@cipht.net>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applied (with minor edits - mostly formatting), thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH RESEND 4/5] Input: uinput - add new UINPUT_DEV_SETUP ioctl
From: Dmitry Torokhov @ 2014-07-21 20:11 UTC (permalink / raw)
To: David Herrmann; +Cc: open list:HID CORE LAYER, Peter Hutterer
In-Reply-To: <CANq1E4S0=h=PccuHVAFPMgdMvGWyBLitpaeJAkZ--PmuTT0xxg@mail.gmail.com>
On Mon, Jul 21, 2014 at 08:22:09AM +0200, David Herrmann wrote:
> Hi
>
> On Mon, Jul 21, 2014 at 3:01 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi David,
> >
> > On Sat, Jul 19, 2014 at 03:10:44PM +0200, David Herrmann wrote:
> >> This adds a new ioctl UINPUT_DEV_SETUP that replaces the old device setup
> >> method (by write()'ing "struct uinput_user_dev" to the node). The old
> >> method is not easily extendable and requires huge payloads. Furthermore,
> >> overloading write() without properly versioned objects is error-prone.
> >>
> >> Therefore, we introduce a new ioctl to replace the old method. The ioctl
> >> supports all features of the old method, plus a "resolution" field for
> >> absinfo. Furthermore, it's properly forward-compatible to new ABS codes
> >> and a growing "struct input_absinfo" structure.
> >>
> >> The ioctl also allows user-space to skip unknown axes if not set. The
> >> payload-size can now be specified by the caller. There is no need to copy
> >> the whole array temporarily into the kernel, but instead we can iterate
> >> over it and copy each value manually.
> >>
> >> Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >> ---
> >> drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
> >> include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
> >> 2 files changed, 118 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> >> index a2a3895..0f45595 100644
> >> --- a/drivers/input/misc/uinput.c
> >> +++ b/drivers/input/misc/uinput.c
> >> @@ -371,8 +371,67 @@ static int uinput_allocate_device(struct uinput_device *udev)
> >> return 0;
> >> }
> >>
> >> -static int uinput_setup_device(struct uinput_device *udev,
> >> - const char __user *buffer, size_t count)
> >> +static int uinput_dev_setup(struct uinput_device *udev,
> >> + struct uinput_setup __user *arg)
> >> +{
> >> + struct uinput_setup setup;
> >> + struct input_dev *dev;
> >> + int i, retval;
> >> +
> >> + if (udev->state == UIST_CREATED)
> >> + return -EINVAL;
> >> + if (copy_from_user(&setup, arg, sizeof(setup)))
> >> + return -EFAULT;
> >> + if (!setup.name[0])
> >> + return -EINVAL;
> >> + /* So far we only support the original "struct input_absinfo", but be
> >> + * forward compatible and allow larger payloads. */
> >> + if (setup.absinfo_size < sizeof(struct input_absinfo))
> >> + return -EINVAL;
> >
> > No, we can not do this, as it breaks backward compatibility (the most
> > important one!). If we were to increase size of in-kernel input_absinfo
> > in let's say 3.20, userspace compiled against older kernel headers
> > (but using the new ioctl available let's say since 3.16 - don't hold me
> > to the numbers ;) ), would break since it wold start tripping on thi
> > check.
> >
> > The proper way to handle it is to convert "old" absinfo into new one,
> > applying as much as we can.
>
> I know, but there is no "old absinfo". Once we extend "struct absinfo"
> I expect this code to change to:
>
> {
> /* initially supported absinfo had size 24 */
> if (setup.absinfo_size < 24)
> return -EINVAL;
>
> /* ...pseudo code... */
> memset(&absinfo, 0, sizeof(absinfo));
> memcpy(&absinfo, sth->absinfo, MIN(setup.absinfo_size,
> sizeof(absinfo)));
> }
>
> This allows you to use this ioctl with old absinfo objects. I can
> change the current code to this already, if you want? I tried to avoid
> it, because a memset() is not neccessarily an appropriate way to
> initialize unset fields.
> I cal also add support for "absinfo" without the "resolution" field,
> which I think is the only field that wasn't available in the initial
> structure.
I think for now I would do:
/*
* Whoever is changing struct input_absinfo will have to take
* care of backwards compatibility.
*/
BUILD_BUG_ON(sizeof(struct input_absinfo)) != 24);
if (setup.absinfo_size != sizeof(struct input_absinfo))
return -EINVAL;
...
and later when we detect setup.absinfo_size < sizeof(struct
input_absinfo) we'll have to take care about backwards compatibility. We
do not need to take care of forward compatibility as we do not know if
userspace will be satisfied with partial results or not and newer
userspace can deal with proper handling of older kernels.
By the way, I realize I do not like the new IOCTL as it is - it's too
big and would be hard to extend if we want to change items other than
absinfo. Why don't we create UI_DEV_SETUP that only sets name, id, and
number of effects, and add UI_SET_ABSAXIS that would take:
struct uinput_abs_setup {
__u16 code; /* axis code */
/* __u16 filler; */
struct input_absinfo absinfo;
}
By the way, while you are hacking on uinput can we also fix formatting
style of switch/case and switch printk() to pr_debug() and friends? I'd
do it myself but do not want to step on your toes.
Thanks!
--
Dmitry
^ permalink raw reply
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