| Message ID | 1390521623-6491-6-git-send-email-courtney.cavin@sonymobile.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/23/2014 04:00 PM, Courtney Cavin wrote: > Since all the configuration needed for an irq can be provided in other > ways, remove all gpio->irq functionality. This cleans up the code quite > a bit. In certain diagnostic modes, we need to be able to release the IRQ so the GPIO can be monitored from userspace. This patch removes that capability. > This also gets rid of polling functionality, as this should be done > elsewhere if absolutely needed. As mentioned in previous patch discussions, the polling functionality is quite useful during new system integration, so we need to retain that. If you have a proposal for where else it could be done, we'd certainly entertain a patch that implements that. Thanks, Chris > > Cc: Christopher Heiny <cheiny@synaptics.com> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> > --- > drivers/input/rmi4/rmi_driver.c | 143 +++++----------------------------------- > drivers/input/rmi4/rmi_driver.h | 7 -- > drivers/input/rmi4/rmi_i2c.c | 24 +------ > include/linux/rmi.h | 31 +-------- > 4 files changed, 20 insertions(+), 185 deletions(-) > > diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c > index 5fb582c..780742f 100644 > --- a/drivers/input/rmi4/rmi_driver.c > +++ b/drivers/input/rmi4/rmi_driver.c > @@ -20,7 +20,6 @@ > #include <linux/delay.h> > #include <linux/device.h> > #include <linux/fs.h> > -#include <linux/gpio.h> > #include <linux/kconfig.h> > #include <linux/list.h> > #include <linux/module.h> > @@ -50,74 +49,17 @@ static irqreturn_t rmi_irq_thread(int irq, void *p) > struct rmi_transport_dev *xport = p; > struct rmi_device *rmi_dev = xport->rmi_dev; > struct rmi_driver *driver = rmi_dev->driver; > - struct rmi_device_platform_data *pdata = xport->dev->platform_data; > struct rmi_driver_data *data; > > data = dev_get_drvdata(&rmi_dev->dev); > - > - if (IRQ_DEBUG(data)) > - dev_dbg(xport->dev, "ATTN gpio, value: %d.\n", > - gpio_get_value(pdata->attn_gpio)); > - > - if (gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity) { > - data->attn_count++; > - if (driver && driver->irq_handler && rmi_dev) > - driver->irq_handler(rmi_dev, irq); > - } > + if (driver && driver->irq_handler && rmi_dev) > + driver->irq_handler(rmi_dev, irq); > > return IRQ_HANDLED; > } > > static int process_interrupt_requests(struct rmi_device *rmi_dev); > > -static void rmi_poll_work(struct work_struct *work) > -{ > - struct rmi_driver_data *data = > - container_of(work, struct rmi_driver_data, poll_work); > - struct rmi_device *rmi_dev = data->rmi_dev; > - > - process_interrupt_requests(rmi_dev); > -} > - > -/* > - * This is the timer function for polling - it simply has to schedule work > - * and restart the timer. > - */ > -static enum hrtimer_restart rmi_poll_timer(struct hrtimer *timer) > -{ > - struct rmi_driver_data *data = > - container_of(timer, struct rmi_driver_data, poll_timer); > - > - if (!data->enabled) > - return HRTIMER_NORESTART; > - if (!work_pending(&data->poll_work)) > - schedule_work(&data->poll_work); > - hrtimer_start(&data->poll_timer, data->poll_interval, HRTIMER_MODE_REL); > - return HRTIMER_NORESTART; > -} > - > -static int enable_polling(struct rmi_device *rmi_dev) > -{ > - struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); > - > - dev_dbg(&rmi_dev->dev, "Polling enabled.\n"); > - INIT_WORK(&data->poll_work, rmi_poll_work); > - hrtimer_init(&data->poll_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > - data->poll_timer.function = rmi_poll_timer; > - hrtimer_start(&data->poll_timer, data->poll_interval, HRTIMER_MODE_REL); > - > - return 0; > -} > - > -static void disable_polling(struct rmi_device *rmi_dev) > -{ > - struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); > - > - dev_dbg(&rmi_dev->dev, "Polling disabled.\n"); > - hrtimer_cancel(&data->poll_timer); > - cancel_work_sync(&data->poll_work); > -} > - > static void disable_sensor(struct rmi_device *rmi_dev) > { > struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); > @@ -125,9 +67,6 @@ static void disable_sensor(struct rmi_device *rmi_dev) > if (!data->enabled) > return; > > - if (!data->irq) > - disable_polling(rmi_dev); > - > if (rmi_dev->xport->ops->disable_device) > rmi_dev->xport->ops->disable_device(rmi_dev->xport); > > @@ -155,20 +94,14 @@ static int enable_sensor(struct rmi_device *rmi_dev) > } > > xport = rmi_dev->xport; > - if (data->irq) { > - retval = request_threaded_irq(data->irq, > - xport->hard_irq ? xport->hard_irq : NULL, > - xport->irq_thread ? > - xport->irq_thread : rmi_irq_thread, > - data->irq_flags, > - dev_name(&rmi_dev->dev), xport); > - if (retval) > - return retval; > - } else { > - retval = enable_polling(rmi_dev); > - if (retval < 0) > - return retval; > - } > + retval = request_threaded_irq(data->irq, > + xport->hard_irq ? xport->hard_irq : NULL, > + xport->irq_thread ? > + xport->irq_thread : rmi_irq_thread, > + IRQF_ONESHOT, > + dev_name(&rmi_dev->dev), xport); > + if (retval) > + return retval; > > data->enabled = true; > > @@ -751,16 +684,9 @@ static SIMPLE_DEV_PM_OPS(rmi_driver_pm, rmi_driver_suspend, rmi_driver_resume); > static int rmi_driver_remove(struct device *dev) > { > struct rmi_device *rmi_dev = to_rmi_device(dev); > - const struct rmi_device_platform_data *pdata = > - to_rmi_platform_data(rmi_dev); > - const struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); > - > disable_sensor(rmi_dev); > rmi_free_function_list(rmi_dev); > > - if (data->gpio_held) > - gpio_free(pdata->attn_gpio); > - > return 0; > } > > @@ -895,51 +821,12 @@ static int rmi_driver_probe(struct device *dev) > mutex_init(&data->suspend_mutex); > } > > - if (gpio_is_valid(pdata->attn_gpio)) { > - static const char GPIO_LABEL[] = "attn"; > - unsigned long gpio_flags = GPIOF_DIR_IN; > - > - data->irq = gpio_to_irq(pdata->attn_gpio); > - if (pdata->level_triggered) { > - data->irq_flags = IRQF_ONESHOT | > - ((pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH) > - ? IRQF_TRIGGER_HIGH : IRQF_TRIGGER_LOW); > - } else { > - data->irq_flags = > - (pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH) > - ? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING; > - } > + data->irq = pdata->irq; > + if (data->irq < 0) { > + dev_err(dev, "Failed to get attn IRQ.\n"); > + retval = data->irq; > + goto err_free_data; > > - if (IS_ENABLED(CONFIG_RMI4_DEV)) > - gpio_flags |= GPIOF_EXPORT; > - > - retval = gpio_request_one(pdata->attn_gpio, gpio_flags, > - GPIO_LABEL); > - if (retval) { > - dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code=%d.\n", > - pdata->attn_gpio, retval); > - retval = 0; > - } else { > - dev_info(dev, "Obtained ATTN gpio %d.\n", > - pdata->attn_gpio); > - data->gpio_held = true; > - if (IS_ENABLED(CONFIG_RMI4_DEV)) { > - retval = gpio_export_link(dev, > - GPIO_LABEL, pdata->attn_gpio); > - if (retval) { > - dev_warn(dev, > - "WARNING: Failed to symlink ATTN gpio!\n"); > - retval = 0; > - } else { > - dev_info(dev, "Exported ATTN gpio %d.", > - pdata->attn_gpio); > - } > - } > - } > - } else { > - data->poll_interval = ktime_set(0, > - (pdata->poll_interval_ms ? pdata->poll_interval_ms : > - DEFAULT_POLL_INTERVAL_MS) * 1000 * 1000); > } > > if (data->f01_container->dev.driver) { > diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h > index 4f44a54..aef5521 100644 > --- a/drivers/input/rmi4/rmi_driver.h > +++ b/drivers/input/rmi4/rmi_driver.h > @@ -39,9 +39,7 @@ struct rmi_driver_data { > > u32 attn_count; > u32 irq_debug; /* Should be bool, but debugfs wants u32 */ > - bool gpio_held; > int irq; > - int irq_flags; > int num_of_irq_regs; > int irq_count; > unsigned long *irq_status; > @@ -50,11 +48,6 @@ struct rmi_driver_data { > bool irq_stored; > struct mutex irq_mutex; > > - /* Following are used when polling. */ > - struct hrtimer poll_timer; > - struct work_struct poll_work; > - ktime_t poll_interval; > - > struct mutex pdt_mutex; > u8 pdt_props; > u8 bsr; > diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c > index 910f05c..aebf974 100644 > --- a/drivers/input/rmi4/rmi_i2c.c > +++ b/drivers/input/rmi4/rmi_i2c.c > @@ -196,8 +196,7 @@ static int rmi_i2c_probe(struct i2c_client *client, > return -EINVAL; > } > > - dev_dbg(&client->dev, "Probing %#02x (GPIO %d).\n", > - client->addr, pdata->attn_gpio); > + dev_dbg(&client->dev, "Probing %#02x.\n", client->addr); > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > dev_err(&client->dev, > @@ -205,15 +204,6 @@ static int rmi_i2c_probe(struct i2c_client *client, > return -ENODEV; > } > > - if (pdata->gpio_config) { > - retval = pdata->gpio_config(pdata->gpio_data, true); > - if (retval < 0) { > - dev_err(&client->dev, "Failed to configure GPIOs, code: %d.\n", > - retval); > - return retval; > - } > - } > - > rmi_i2c = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_xport), > GFP_KERNEL); > if (!rmi_i2c) > @@ -240,7 +230,7 @@ static int rmi_i2c_probe(struct i2c_client *client, > if (retval) { > dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n", > client->addr); > - goto err_gpio; > + goto err; > } > > i2c_set_clientdata(client, rmi_i2c); > @@ -249,24 +239,16 @@ static int rmi_i2c_probe(struct i2c_client *client, > client->addr); > return 0; > > -err_gpio: > - if (pdata->gpio_config) > - pdata->gpio_config(pdata->gpio_data, false); > - > +err: > return retval; > } > > static int rmi_i2c_remove(struct i2c_client *client) > { > - const struct rmi_device_platform_data *pdata = > - dev_get_platdata(&client->dev); > struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client); > > rmi_unregister_transport_device(&rmi_i2c->xport); > > - if (pdata->gpio_config) > - pdata->gpio_config(pdata->gpio_data, false); > - > return 0; > } > > diff --git a/include/linux/rmi.h b/include/linux/rmi.h > index 65b59b5..326e741 100644 > --- a/include/linux/rmi.h > +++ b/include/linux/rmi.h > @@ -23,11 +23,6 @@ > #include <linux/wait.h> > #include <linux/debugfs.h> > > -enum rmi_attn_polarity { > - RMI_ATTN_ACTIVE_LOW = 0, > - RMI_ATTN_ACTIVE_HIGH = 1 > -}; > - > /** > * struct rmi_f11_axis_alignment - target axis alignment > * @swap_axes: set to TRUE if desired to swap x- and y-axis > @@ -194,25 +189,10 @@ struct rmi_device_platform_data_spi { > /** > * struct rmi_device_platform_data - system specific configuration info. > * > + * @irq - attention IRQ > * @firmware_name - if specified will override default firmware name, > * for reflashing. > * > - * @attn_gpio - the index of a GPIO that will be used to provide the ATTN > - * interrupt from the touch sensor. > - * @attn_polarity - indicates whether ATTN is active high or low. > - * @level_triggered - by default, the driver uses edge triggered interrupts. > - * However, this can cause problems with suspend/resume on some platforms. In > - * that case, set this to 1 to use level triggered interrupts. > - * @gpio_config - a routine that will be called when the driver is loaded to > - * perform any platform specific GPIO configuration, and when it is unloaded > - * for GPIO de-configuration. This is typically used to configure the ATTN > - * GPIO and the I2C or SPI pins, if necessary. > - * @gpio_data - platform specific data to be passed to the GPIO configuration > - * function. > - * > - * @poll_interval_ms - the time in milliseconds between reads of the interrupt > - * status register. This is ignored if attn_gpio is non-zero. > - * > * @reset_delay_ms - after issuing a reset command to the touch sensor, the > * driver waits a few milliseconds to give the firmware a chance to > * to re-initialize. You can override the default wait period here. > @@ -245,14 +225,7 @@ struct rmi_device_platform_data_spi { > * functions. > */ > struct rmi_device_platform_data { > - int attn_gpio; > - enum rmi_attn_polarity attn_polarity; > - bool level_triggered; > - void *gpio_data; > - int (*gpio_config)(void *gpio_data, bool configure); > - > - int poll_interval_ms; > - > + int irq; > int reset_delay_ms; > > struct rmi_device_platform_data_spi spi_data; >
On Wed, Feb 05, 2014 at 12:08:35AM +0100, Christopher Heiny wrote: > On 01/23/2014 04:00 PM, Courtney Cavin wrote: > > Since all the configuration needed for an irq can be provided in other > > ways, remove all gpio->irq functionality. This cleans up the code quite > > a bit. > > In certain diagnostic modes, we need to be able to release the IRQ so > the GPIO can be monitored from userspace. This patch removes that > capability. > Polling a GPIO from userspace is poor design regardless of the use-case, if you ask me. It certainly doesn't motivate the extra gpio<->IRQ code. > > This also gets rid of polling functionality, as this should be done > > elsewhere if absolutely needed. > > As mentioned in previous patch discussions, the polling functionality is > quite useful during new system integration, so we need to retain that. > If you have a proposal for where else it could be done, we'd certainly > entertain a patch that implements that. Do you actually have systems that have these hooked up to GPIOs without IRQ support? Regardless, if this is the case, implementing a GPIO polling IRQ chip should be possible, if extremely ugly. Linus may have some comments in this area, though. Linus? > > Thanks, > Chris > > > > > Cc: Christopher Heiny <cheiny@synaptics.com> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> > > --- > > drivers/input/rmi4/rmi_driver.c | 143 +++++----------------------------------- > > drivers/input/rmi4/rmi_driver.h | 7 -- > > drivers/input/rmi4/rmi_i2c.c | 24 +------ > > include/linux/rmi.h | 31 +-------- > > 4 files changed, 20 insertions(+), 185 deletions(-) > > > > diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c > > index 5fb582c..780742f 100644 > > --- a/drivers/input/rmi4/rmi_driver.c > > +++ b/drivers/input/rmi4/rmi_driver.c > > @@ -20,7 +20,6 @@ > > #include <linux/delay.h> > > #include <linux/device.h> > > #include <linux/fs.h> > > -#include <linux/gpio.h> > > #include <linux/kconfig.h> > > #include <linux/list.h> > > #include <linux/module.h> > > @@ -50,74 +49,17 @@ static irqreturn_t rmi_irq_thread(int irq, void *p) > > struct rmi_transport_dev *xport = p; > > struct rmi_device *rmi_dev = xport->rmi_dev; > > struct rmi_driver *driver = rmi_dev->driver; > > - struct rmi_device_platform_data *pdata = xport->dev->platform_data; > > struct rmi_driver_data *data; > > > > data = dev_get_drvdata(&rmi_dev->dev); > > - > > - if (IRQ_DEBUG(data)) > > - dev_dbg(xport->dev, "ATTN gpio, value: %d.\n", > > - gpio_get_value(pdata->attn_gpio)); > > - > > - if (gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity) { > > - data->attn_count++; > > - if (driver && driver->irq_handler && rmi_dev) > > - driver->irq_handler(rmi_dev, irq); > > - } > > + if (driver && driver->irq_handler && rmi_dev) > > + driver->irq_handler(rmi_dev, irq); > > > > return IRQ_HANDLED; > > } > > > > static int process_interrupt_requests(struct rmi_device *rmi_dev); > > > > -static void rmi_poll_work(struct work_struct *work) > > -{ > > - struct rmi_driver_data *data = > > - container_of(work, struct rmi_driver_data, poll_work); > > - struct rmi_device *rmi_dev = data->rmi_dev; > > - > > - process_interrupt_requests(rmi_dev); > > -} > > - > > -/* > > - * This is the timer function for polling - it simply has to schedule work > > - * and restart the timer. > > - */ > > -static enum hrtimer_restart rmi_poll_timer(struct hrtimer *timer) > > -{ > > - struct rmi_driver_data *data = > > - container_of(timer, struct rmi_driver_data, poll_timer); > > - > > - if (!data->enabled) > > - return HRTIMER_NORESTART; > > - if (!work_pending(&data->poll_work)) > > - schedule_work(&data->poll_work); > > - hrtimer_start(&data->poll_timer, data->poll_interval, HRTIMER_MODE_REL); > > - return HRTIMER_NORESTART; > > -} > > - > > -static int enable_polling(struct rmi_device *rmi_dev) > > -{ > > - struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); > > - > > - dev_dbg(&rmi_dev->dev, "Polling enabled.\n"); > > - INIT_WORK(&data->poll_work, rmi_poll_work); > > - hrtimer_init(&data->poll_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > > - data->poll_timer.function = rmi_poll_timer; > > - hrtimer_start(&data->poll_timer, data->poll_interval, HRTIMER_MODE_REL); > > - > > - return 0; > > -} > > - > > -static void disable_polling(struct rmi_device *rmi_dev) > > -{ > > - struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); > > - > > - dev_dbg(&rmi_dev->dev, "Polling disabled.\n"); > > - hrtimer_cancel(&data->poll_timer); > > - cancel_work_sync(&data->poll_work); > > -} > > - > > static void disable_sensor(struct rmi_device *rmi_dev) > > { > > struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); > > @@ -125,9 +67,6 @@ static void disable_sensor(struct rmi_device *rmi_dev) > > if (!data->enabled) > > return; > > > > - if (!data->irq) > > - disable_polling(rmi_dev); > > - > > if (rmi_dev->xport->ops->disable_device) > > rmi_dev->xport->ops->disable_device(rmi_dev->xport); > > > > @@ -155,20 +94,14 @@ static int enable_sensor(struct rmi_device *rmi_dev) > > } > > > > xport = rmi_dev->xport; > > - if (data->irq) { > > - retval = request_threaded_irq(data->irq, > > - xport->hard_irq ? xport->hard_irq : NULL, > > - xport->irq_thread ? > > - xport->irq_thread : rmi_irq_thread, > > - data->irq_flags, > > - dev_name(&rmi_dev->dev), xport); > > - if (retval) > > - return retval; > > - } else { > > - retval = enable_polling(rmi_dev); > > - if (retval < 0) > > - return retval; > > - } > > + retval = request_threaded_irq(data->irq, > > + xport->hard_irq ? xport->hard_irq : NULL, > > + xport->irq_thread ? > > + xport->irq_thread : rmi_irq_thread, > > + IRQF_ONESHOT, > > + dev_name(&rmi_dev->dev), xport); > > + if (retval) > > + return retval; > > > > data->enabled = true; > > > > @@ -751,16 +684,9 @@ static SIMPLE_DEV_PM_OPS(rmi_driver_pm, rmi_driver_suspend, rmi_driver_resume); > > static int rmi_driver_remove(struct device *dev) > > { > > struct rmi_device *rmi_dev = to_rmi_device(dev); > > - const struct rmi_device_platform_data *pdata = > > - to_rmi_platform_data(rmi_dev); > > - const struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); > > - > > disable_sensor(rmi_dev); > > rmi_free_function_list(rmi_dev); > > > > - if (data->gpio_held) > > - gpio_free(pdata->attn_gpio); > > - > > return 0; > > } > > > > @@ -895,51 +821,12 @@ static int rmi_driver_probe(struct device *dev) > > mutex_init(&data->suspend_mutex); > > } > > > > - if (gpio_is_valid(pdata->attn_gpio)) { > > - static const char GPIO_LABEL[] = "attn"; > > - unsigned long gpio_flags = GPIOF_DIR_IN; > > - > > - data->irq = gpio_to_irq(pdata->attn_gpio); > > - if (pdata->level_triggered) { > > - data->irq_flags = IRQF_ONESHOT | > > - ((pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH) > > - ? IRQF_TRIGGER_HIGH : IRQF_TRIGGER_LOW); > > - } else { > > - data->irq_flags = > > - (pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH) > > - ? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING; > > - } > > + data->irq = pdata->irq; > > + if (data->irq < 0) { > > + dev_err(dev, "Failed to get attn IRQ.\n"); > > + retval = data->irq; > > + goto err_free_data; > > > > - if (IS_ENABLED(CONFIG_RMI4_DEV)) > > - gpio_flags |= GPIOF_EXPORT; > > - > > - retval = gpio_request_one(pdata->attn_gpio, gpio_flags, > > - GPIO_LABEL); > > - if (retval) { > > - dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code=%d.\n", > > - pdata->attn_gpio, retval); > > - retval = 0; > > - } else { > > - dev_info(dev, "Obtained ATTN gpio %d.\n", > > - pdata->attn_gpio); > > - data->gpio_held = true; > > - if (IS_ENABLED(CONFIG_RMI4_DEV)) { > > - retval = gpio_export_link(dev, > > - GPIO_LABEL, pdata->attn_gpio); > > - if (retval) { > > - dev_warn(dev, > > - "WARNING: Failed to symlink ATTN gpio!\n"); > > - retval = 0; > > - } else { > > - dev_info(dev, "Exported ATTN gpio %d.", > > - pdata->attn_gpio); > > - } > > - } > > - } > > - } else { > > - data->poll_interval = ktime_set(0, > > - (pdata->poll_interval_ms ? pdata->poll_interval_ms : > > - DEFAULT_POLL_INTERVAL_MS) * 1000 * 1000); > > } > > > > if (data->f01_container->dev.driver) { > > diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h > > index 4f44a54..aef5521 100644 > > --- a/drivers/input/rmi4/rmi_driver.h > > +++ b/drivers/input/rmi4/rmi_driver.h > > @@ -39,9 +39,7 @@ struct rmi_driver_data { > > > > u32 attn_count; > > u32 irq_debug; /* Should be bool, but debugfs wants u32 */ > > - bool gpio_held; > > int irq; > > - int irq_flags; > > int num_of_irq_regs; > > int irq_count; > > unsigned long *irq_status; > > @@ -50,11 +48,6 @@ struct rmi_driver_data { > > bool irq_stored; > > struct mutex irq_mutex; > > > > - /* Following are used when polling. */ > > - struct hrtimer poll_timer; > > - struct work_struct poll_work; > > - ktime_t poll_interval; > > - > > struct mutex pdt_mutex; > > u8 pdt_props; > > u8 bsr; > > diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c > > index 910f05c..aebf974 100644 > > --- a/drivers/input/rmi4/rmi_i2c.c > > +++ b/drivers/input/rmi4/rmi_i2c.c > > @@ -196,8 +196,7 @@ static int rmi_i2c_probe(struct i2c_client *client, > > return -EINVAL; > > } > > > > - dev_dbg(&client->dev, "Probing %#02x (GPIO %d).\n", > > - client->addr, pdata->attn_gpio); > > + dev_dbg(&client->dev, "Probing %#02x.\n", client->addr); > > > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > > dev_err(&client->dev, > > @@ -205,15 +204,6 @@ static int rmi_i2c_probe(struct i2c_client *client, > > return -ENODEV; > > } > > > > - if (pdata->gpio_config) { > > - retval = pdata->gpio_config(pdata->gpio_data, true); > > - if (retval < 0) { > > - dev_err(&client->dev, "Failed to configure GPIOs, code: %d.\n", > > - retval); > > - return retval; > > - } > > - } > > - > > rmi_i2c = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_xport), > > GFP_KERNEL); > > if (!rmi_i2c) > > @@ -240,7 +230,7 @@ static int rmi_i2c_probe(struct i2c_client *client, > > if (retval) { > > dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n", > > client->addr); > > - goto err_gpio; > > + goto err; > > } > > > > i2c_set_clientdata(client, rmi_i2c); > > @@ -249,24 +239,16 @@ static int rmi_i2c_probe(struct i2c_client *client, > > client->addr); > > return 0; > > > > -err_gpio: > > - if (pdata->gpio_config) > > - pdata->gpio_config(pdata->gpio_data, false); > > - > > +err: > > return retval; > > } > > > > static int rmi_i2c_remove(struct i2c_client *client) > > { > > - const struct rmi_device_platform_data *pdata = > > - dev_get_platdata(&client->dev); > > struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client); > > > > rmi_unregister_transport_device(&rmi_i2c->xport); > > > > - if (pdata->gpio_config) > > - pdata->gpio_config(pdata->gpio_data, false); > > - > > return 0; > > } > > > > diff --git a/include/linux/rmi.h b/include/linux/rmi.h > > index 65b59b5..326e741 100644 > > --- a/include/linux/rmi.h > > +++ b/include/linux/rmi.h > > @@ -23,11 +23,6 @@ > > #include <linux/wait.h> > > #include <linux/debugfs.h> > > > > -enum rmi_attn_polarity { > > - RMI_ATTN_ACTIVE_LOW = 0, > > - RMI_ATTN_ACTIVE_HIGH = 1 > > -}; > > - > > /** > > * struct rmi_f11_axis_alignment - target axis alignment > > * @swap_axes: set to TRUE if desired to swap x- and y-axis > > @@ -194,25 +189,10 @@ struct rmi_device_platform_data_spi { > > /** > > * struct rmi_device_platform_data - system specific configuration info. > > * > > + * @irq - attention IRQ > > * @firmware_name - if specified will override default firmware name, > > * for reflashing. > > * > > - * @attn_gpio - the index of a GPIO that will be used to provide the ATTN > > - * interrupt from the touch sensor. > > - * @attn_polarity - indicates whether ATTN is active high or low. > > - * @level_triggered - by default, the driver uses edge triggered interrupts. > > - * However, this can cause problems with suspend/resume on some platforms. In > > - * that case, set this to 1 to use level triggered interrupts. > > - * @gpio_config - a routine that will be called when the driver is loaded to > > - * perform any platform specific GPIO configuration, and when it is unloaded > > - * for GPIO de-configuration. This is typically used to configure the ATTN > > - * GPIO and the I2C or SPI pins, if necessary. > > - * @gpio_data - platform specific data to be passed to the GPIO configuration > > - * function. > > - * > > - * @poll_interval_ms - the time in milliseconds between reads of the interrupt > > - * status register. This is ignored if attn_gpio is non-zero. > > - * > > * @reset_delay_ms - after issuing a reset command to the touch sensor, the > > * driver waits a few milliseconds to give the firmware a chance to > > * to re-initialize. You can override the default wait period here. > > @@ -245,14 +225,7 @@ struct rmi_device_platform_data_spi { > > * functions. > > */ > > struct rmi_device_platform_data { > > - int attn_gpio; > > - enum rmi_attn_polarity attn_polarity; > > - bool level_triggered; > > - void *gpio_data; > > - int (*gpio_config)(void *gpio_data, bool configure); > > - > > - int poll_interval_ms; > > - > > + int irq; > > int reset_delay_ms; > > > > struct rmi_device_platform_data_spi spi_data; > > -- 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
On Wed, Feb 5, 2014 at 3:31 AM, Courtney Cavin <courtney.cavin@sonymobile.com> wrote: > On Wed, Feb 05, 2014 at 12:08:35AM +0100, Christopher Heiny wrote: >> On 01/23/2014 04:00 PM, Courtney Cavin wrote: >> > Since all the configuration needed for an irq can be provided in other >> > ways, remove all gpio->irq functionality. This cleans up the code quite >> > a bit. >> >> In certain diagnostic modes, we need to be able to release the IRQ so >> the GPIO can be monitored from userspace. This patch removes that >> capability. > > Polling a GPIO from userspace is poor design regardless of the use-case, if > you ask me. It certainly doesn't motivate the extra gpio<->IRQ code. I think Courtney is right here, if you want this kind of stuff for debugging it should be debug-add-on-patches, not part of the normal execution path for the driver. >> As mentioned in previous patch discussions, the polling functionality is >> quite useful during new system integration, so we need to retain that. >> If you have a proposal for where else it could be done, we'd certainly >> entertain a patch that implements that. > > Do you actually have systems that have these hooked up to GPIOs without > IRQ support? > > Regardless, if this is the case, implementing a GPIO polling IRQ chip > should be possible, if extremely ugly. OMG that would be quite horrible. > Linus may have some comments in this area, though. Linus? A driver may rely on polling but a driver may not rely on polling done from userspace. Yours, Linus Walleij -- 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
On 02/04/2014 06:31 PM, Courtney Cavin wrote: > On Wed, Feb 05, 2014 at 12:08:35AM +0100, Christopher Heiny wrote: >> On 01/23/2014 04:00 PM, Courtney Cavin wrote: >>> Since all the configuration needed for an irq can be provided in other >>> ways, remove all gpio->irq functionality. This cleans up the code quite >>> a bit. >> >> In certain diagnostic modes, we need to be able to release the IRQ so >> the GPIO can be monitored from userspace. This patch removes that >> capability. >> > > Polling a GPIO from userspace is poor design regardless of the use-case, if > you ask me. It certainly doesn't motivate the extra gpio<->IRQ code. Fortunately, neither the in-driver polling implementation nor the user space diagnostic applications are implemented that way. >>> This also gets rid of polling functionality, as this should be done >>> elsewhere if absolutely needed. >> >> As mentioned in previous patch discussions, the polling functionality is >> quite useful during new system integration, so we need to retain that. >> If you have a proposal for where else it could be done, we'd certainly >> entertain a patch that implements that. > > Do you actually have systems that have these hooked up to GPIOs without > IRQ support? The right question would be "What are some situations where this polling functionality would be needed?" As I mentioned, this is usually needed during new system integration - that is, when trying to bring up the driver on a new or substantially modified platform. You might not know where the ATTN GPIO is not known (either by omission or an error in the specifications) at the time it is initially brought up, or you might have a prototype board has a layout error that leaves the ATTN GPIO unconnected. You'd be surprised at how often these situations occur (twice so far this week, f'rinstance). Rather than just telling the engineers "Hey, go play MineSweeper for a few days (or weeks) while someone tracks down the specs (or spins the PCB)", having in-driver polling support lets you bring up the sensor right away. > Regardless, if this is the case, implementing a GPIO polling IRQ chip > should be possible, if extremely ugly. > > Linus may have some comments in this area, though. Linus? > >> >> Thanks, >> Chris >> >>> >>> Cc: Christopher Heiny <cheiny@synaptics.com> >>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >>> Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> >>> --- >>> drivers/input/rmi4/rmi_driver.c | 143 +++++----------------------------------- >>> drivers/input/rmi4/rmi_driver.h | 7 -- >>> drivers/input/rmi4/rmi_i2c.c | 24 +------ >>> include/linux/rmi.h | 31 +-------- >>> 4 files changed, 20 insertions(+), 185 deletions(-) >>> >>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c >>> index 5fb582c..780742f 100644 >>> --- a/drivers/input/rmi4/rmi_driver.c >>> +++ b/drivers/input/rmi4/rmi_driver.c >>> @@ -20,7 +20,6 @@ >>> #include <linux/delay.h> >>> #include <linux/device.h> >>> #include <linux/fs.h> >>> -#include <linux/gpio.h> >>> #include <linux/kconfig.h> >>> #include <linux/list.h> >>> #include <linux/module.h> >>> @@ -50,74 +49,17 @@ static irqreturn_t rmi_irq_thread(int irq, void *p) >>> struct rmi_transport_dev *xport = p; >>> struct rmi_device *rmi_dev = xport->rmi_dev; >>> struct rmi_driver *driver = rmi_dev->driver; >>> - struct rmi_device_platform_data *pdata = xport->dev->platform_data; >>> struct rmi_driver_data *data; >>> >>> data = dev_get_drvdata(&rmi_dev->dev); >>> - >>> - if (IRQ_DEBUG(data)) >>> - dev_dbg(xport->dev, "ATTN gpio, value: %d.\n", >>> - gpio_get_value(pdata->attn_gpio)); >>> - >>> - if (gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity) { >>> - data->attn_count++; >>> - if (driver && driver->irq_handler && rmi_dev) >>> - driver->irq_handler(rmi_dev, irq); >>> - } >>> + if (driver && driver->irq_handler && rmi_dev) >>> + driver->irq_handler(rmi_dev, irq); >>> >>> return IRQ_HANDLED; >>> } >>> >>> static int process_interrupt_requests(struct rmi_device *rmi_dev); >>> >>> -static void rmi_poll_work(struct work_struct *work) >>> -{ >>> - struct rmi_driver_data *data = >>> - container_of(work, struct rmi_driver_data, poll_work); >>> - struct rmi_device *rmi_dev = data->rmi_dev; >>> - >>> - process_interrupt_requests(rmi_dev); >>> -} >>> - >>> -/* >>> - * This is the timer function for polling - it simply has to schedule work >>> - * and restart the timer. >>> - */ >>> -static enum hrtimer_restart rmi_poll_timer(struct hrtimer *timer) >>> -{ >>> - struct rmi_driver_data *data = >>> - container_of(timer, struct rmi_driver_data, poll_timer); >>> - >>> - if (!data->enabled) >>> - return HRTIMER_NORESTART; >>> - if (!work_pending(&data->poll_work)) >>> - schedule_work(&data->poll_work); >>> - hrtimer_start(&data->poll_timer, data->poll_interval, HRTIMER_MODE_REL); >>> - return HRTIMER_NORESTART; >>> -} >>> - >>> -static int enable_polling(struct rmi_device *rmi_dev) >>> -{ >>> - struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); >>> - >>> - dev_dbg(&rmi_dev->dev, "Polling enabled.\n"); >>> - INIT_WORK(&data->poll_work, rmi_poll_work); >>> - hrtimer_init(&data->poll_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); >>> - data->poll_timer.function = rmi_poll_timer; >>> - hrtimer_start(&data->poll_timer, data->poll_interval, HRTIMER_MODE_REL); >>> - >>> - return 0; >>> -} >>> - >>> -static void disable_polling(struct rmi_device *rmi_dev) >>> -{ >>> - struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); >>> - >>> - dev_dbg(&rmi_dev->dev, "Polling disabled.\n"); >>> - hrtimer_cancel(&data->poll_timer); >>> - cancel_work_sync(&data->poll_work); >>> -} >>> - >>> static void disable_sensor(struct rmi_device *rmi_dev) >>> { >>> struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); >>> @@ -125,9 +67,6 @@ static void disable_sensor(struct rmi_device *rmi_dev) >>> if (!data->enabled) >>> return; >>> >>> - if (!data->irq) >>> - disable_polling(rmi_dev); >>> - >>> if (rmi_dev->xport->ops->disable_device) >>> rmi_dev->xport->ops->disable_device(rmi_dev->xport); >>> >>> @@ -155,20 +94,14 @@ static int enable_sensor(struct rmi_device *rmi_dev) >>> } >>> >>> xport = rmi_dev->xport; >>> - if (data->irq) { >>> - retval = request_threaded_irq(data->irq, >>> - xport->hard_irq ? xport->hard_irq : NULL, >>> - xport->irq_thread ? >>> - xport->irq_thread : rmi_irq_thread, >>> - data->irq_flags, >>> - dev_name(&rmi_dev->dev), xport); >>> - if (retval) >>> - return retval; >>> - } else { >>> - retval = enable_polling(rmi_dev); >>> - if (retval < 0) >>> - return retval; >>> - } >>> + retval = request_threaded_irq(data->irq, >>> + xport->hard_irq ? xport->hard_irq : NULL, >>> + xport->irq_thread ? >>> + xport->irq_thread : rmi_irq_thread, >>> + IRQF_ONESHOT, >>> + dev_name(&rmi_dev->dev), xport); >>> + if (retval) >>> + return retval; >>> >>> data->enabled = true; >>> >>> @@ -751,16 +684,9 @@ static SIMPLE_DEV_PM_OPS(rmi_driver_pm, rmi_driver_suspend, rmi_driver_resume); >>> static int rmi_driver_remove(struct device *dev) >>> { >>> struct rmi_device *rmi_dev = to_rmi_device(dev); >>> - const struct rmi_device_platform_data *pdata = >>> - to_rmi_platform_data(rmi_dev); >>> - const struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); >>> - >>> disable_sensor(rmi_dev); >>> rmi_free_function_list(rmi_dev); >>> >>> - if (data->gpio_held) >>> - gpio_free(pdata->attn_gpio); >>> - >>> return 0; >>> } >>> >>> @@ -895,51 +821,12 @@ static int rmi_driver_probe(struct device *dev) >>> mutex_init(&data->suspend_mutex); >>> } >>> >>> - if (gpio_is_valid(pdata->attn_gpio)) { >>> - static const char GPIO_LABEL[] = "attn"; >>> - unsigned long gpio_flags = GPIOF_DIR_IN; >>> - >>> - data->irq = gpio_to_irq(pdata->attn_gpio); >>> - if (pdata->level_triggered) { >>> - data->irq_flags = IRQF_ONESHOT | >>> - ((pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH) >>> - ? IRQF_TRIGGER_HIGH : IRQF_TRIGGER_LOW); >>> - } else { >>> - data->irq_flags = >>> - (pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH) >>> - ? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING; >>> - } >>> + data->irq = pdata->irq; >>> + if (data->irq < 0) { >>> + dev_err(dev, "Failed to get attn IRQ.\n"); >>> + retval = data->irq; >>> + goto err_free_data; >>> >>> - if (IS_ENABLED(CONFIG_RMI4_DEV)) >>> - gpio_flags |= GPIOF_EXPORT; >>> - >>> - retval = gpio_request_one(pdata->attn_gpio, gpio_flags, >>> - GPIO_LABEL); >>> - if (retval) { >>> - dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code=%d.\n", >>> - pdata->attn_gpio, retval); >>> - retval = 0; >>> - } else { >>> - dev_info(dev, "Obtained ATTN gpio %d.\n", >>> - pdata->attn_gpio); >>> - data->gpio_held = true; >>> - if (IS_ENABLED(CONFIG_RMI4_DEV)) { >>> - retval = gpio_export_link(dev, >>> - GPIO_LABEL, pdata->attn_gpio); >>> - if (retval) { >>> - dev_warn(dev, >>> - "WARNING: Failed to symlink ATTN gpio!\n"); >>> - retval = 0; >>> - } else { >>> - dev_info(dev, "Exported ATTN gpio %d.", >>> - pdata->attn_gpio); >>> - } >>> - } >>> - } >>> - } else { >>> - data->poll_interval = ktime_set(0, >>> - (pdata->poll_interval_ms ? pdata->poll_interval_ms : >>> - DEFAULT_POLL_INTERVAL_MS) * 1000 * 1000); >>> } >>> >>> if (data->f01_container->dev.driver) { >>> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h >>> index 4f44a54..aef5521 100644 >>> --- a/drivers/input/rmi4/rmi_driver.h >>> +++ b/drivers/input/rmi4/rmi_driver.h >>> @@ -39,9 +39,7 @@ struct rmi_driver_data { >>> >>> u32 attn_count; >>> u32 irq_debug; /* Should be bool, but debugfs wants u32 */ >>> - bool gpio_held; >>> int irq; >>> - int irq_flags; >>> int num_of_irq_regs; >>> int irq_count; >>> unsigned long *irq_status; >>> @@ -50,11 +48,6 @@ struct rmi_driver_data { >>> bool irq_stored; >>> struct mutex irq_mutex; >>> >>> - /* Following are used when polling. */ >>> - struct hrtimer poll_timer; >>> - struct work_struct poll_work; >>> - ktime_t poll_interval; >>> - >>> struct mutex pdt_mutex; >>> u8 pdt_props; >>> u8 bsr; >>> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c >>> index 910f05c..aebf974 100644 >>> --- a/drivers/input/rmi4/rmi_i2c.c >>> +++ b/drivers/input/rmi4/rmi_i2c.c >>> @@ -196,8 +196,7 @@ static int rmi_i2c_probe(struct i2c_client *client, >>> return -EINVAL; >>> } >>> >>> - dev_dbg(&client->dev, "Probing %#02x (GPIO %d).\n", >>> - client->addr, pdata->attn_gpio); >>> + dev_dbg(&client->dev, "Probing %#02x.\n", client->addr); >>> >>> if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { >>> dev_err(&client->dev, >>> @@ -205,15 +204,6 @@ static int rmi_i2c_probe(struct i2c_client *client, >>> return -ENODEV; >>> } >>> >>> - if (pdata->gpio_config) { >>> - retval = pdata->gpio_config(pdata->gpio_data, true); >>> - if (retval < 0) { >>> - dev_err(&client->dev, "Failed to configure GPIOs, code: %d.\n", >>> - retval); >>> - return retval; >>> - } >>> - } >>> - >>> rmi_i2c = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_xport), >>> GFP_KERNEL); >>> if (!rmi_i2c) >>> @@ -240,7 +230,7 @@ static int rmi_i2c_probe(struct i2c_client *client, >>> if (retval) { >>> dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n", >>> client->addr); >>> - goto err_gpio; >>> + goto err; >>> } >>> >>> i2c_set_clientdata(client, rmi_i2c); >>> @@ -249,24 +239,16 @@ static int rmi_i2c_probe(struct i2c_client *client, >>> client->addr); >>> return 0; >>> >>> -err_gpio: >>> - if (pdata->gpio_config) >>> - pdata->gpio_config(pdata->gpio_data, false); >>> - >>> +err: >>> return retval; >>> } >>> >>> static int rmi_i2c_remove(struct i2c_client *client) >>> { >>> - const struct rmi_device_platform_data *pdata = >>> - dev_get_platdata(&client->dev); >>> struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client); >>> >>> rmi_unregister_transport_device(&rmi_i2c->xport); >>> >>> - if (pdata->gpio_config) >>> - pdata->gpio_config(pdata->gpio_data, false); >>> - >>> return 0; >>> } >>> >>> diff --git a/include/linux/rmi.h b/include/linux/rmi.h >>> index 65b59b5..326e741 100644 >>> --- a/include/linux/rmi.h >>> +++ b/include/linux/rmi.h >>> @@ -23,11 +23,6 @@ >>> #include <linux/wait.h> >>> #include <linux/debugfs.h> >>> >>> -enum rmi_attn_polarity { >>> - RMI_ATTN_ACTIVE_LOW = 0, >>> - RMI_ATTN_ACTIVE_HIGH = 1 >>> -}; >>> - >>> /** >>> * struct rmi_f11_axis_alignment - target axis alignment >>> * @swap_axes: set to TRUE if desired to swap x- and y-axis >>> @@ -194,25 +189,10 @@ struct rmi_device_platform_data_spi { >>> /** >>> * struct rmi_device_platform_data - system specific configuration info. >>> * >>> + * @irq - attention IRQ >>> * @firmware_name - if specified will override default firmware name, >>> * for reflashing. >>> * >>> - * @attn_gpio - the index of a GPIO that will be used to provide the ATTN >>> - * interrupt from the touch sensor. >>> - * @attn_polarity - indicates whether ATTN is active high or low. >>> - * @level_triggered - by default, the driver uses edge triggered interrupts. >>> - * However, this can cause problems with suspend/resume on some platforms. In >>> - * that case, set this to 1 to use level triggered interrupts. >>> - * @gpio_config - a routine that will be called when the driver is loaded to >>> - * perform any platform specific GPIO configuration, and when it is unloaded >>> - * for GPIO de-configuration. This is typically used to configure the ATTN >>> - * GPIO and the I2C or SPI pins, if necessary. >>> - * @gpio_data - platform specific data to be passed to the GPIO configuration >>> - * function. >>> - * >>> - * @poll_interval_ms - the time in milliseconds between reads of the interrupt >>> - * status register. This is ignored if attn_gpio is non-zero. >>> - * >>> * @reset_delay_ms - after issuing a reset command to the touch sensor, the >>> * driver waits a few milliseconds to give the firmware a chance to >>> * to re-initialize. You can override the default wait period here. >>> @@ -245,14 +225,7 @@ struct rmi_device_platform_data_spi { >>> * functions. >>> */ >>> struct rmi_device_platform_data { >>> - int attn_gpio; >>> - enum rmi_attn_polarity attn_polarity; >>> - bool level_triggered; >>> - void *gpio_data; >>> - int (*gpio_config)(void *gpio_data, bool configure); >>> - >>> - int poll_interval_ms; >>> - >>> + int irq; >>> int reset_delay_ms; >>> >>> struct rmi_device_platform_data_spi spi_data; >>> -- 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
On 02/06/2014 01:28 AM, Linus Walleij wrote: > On Wed, Feb 5, 2014 at 3:31 AM, Courtney Cavin > <courtney.cavin@sonymobile.com> wrote: >> >On Wed, Feb 05, 2014 at 12:08:35AM +0100, Christopher Heiny wrote: >>> >>On 01/23/2014 04:00 PM, Courtney Cavin wrote: >>>> >> >Since all the configuration needed for an irq can be provided in other >>>> >> >ways, remove all gpio->irq functionality. This cleans up the code quite >>>> >> >a bit. >>> >> >>> >>In certain diagnostic modes, we need to be able to release the IRQ so >>> >>the GPIO can be monitored from userspace. This patch removes that >>> >>capability. >> > >> >Polling a GPIO from userspace is poor design regardless of the use-case, if >> >you ask me. It certainly doesn't motivate the extra gpio<->IRQ code. > > I think Courtney is right here, if you want this kind of stuff for debugging > it should be debug-add-on-patches, not part of the normal execution > path for the driver. I'm OK with replacing the current polling implementation with an add-on-patch, as long as the support for the debug-add-on-patches doesn't get torn out of the driver because "hey, I don't see what this is used for in the core driver code, so it must be unnecessary". >>> >>As mentioned in previous patch discussions, the polling functionality is >>> >>quite useful during new system integration, so we need to retain that. >>> >>If you have a proposal for where else it could be done, we'd certainly >>> >>entertain a patch that implements that. >> > >> >Do you actually have systems that have these hooked up to GPIOs without >> >IRQ support? >> > >> >Regardless, if this is the case, implementing a GPIO polling IRQ chip >> >should be possible, if extremely ugly. > OMG that would be quite horrible. > >> >Linus may have some comments in this area, though. Linus? > > A driver may rely on polling but a driver may not rely on polling > done from userspace. Fortunately, the driver doesn't rely on polling from user space. -- 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
On Thu, Feb 06, 2014 at 09:05:09PM +0100, Christopher Heiny wrote: > On 02/06/2014 01:28 AM, Linus Walleij wrote: > > On Wed, Feb 5, 2014 at 3:31 AM, Courtney Cavin > > <courtney.cavin@sonymobile.com> wrote: > >> >On Wed, Feb 05, 2014 at 12:08:35AM +0100, Christopher Heiny wrote: > >>> >>On 01/23/2014 04:00 PM, Courtney Cavin wrote: > >>>> >> >Since all the configuration needed for an irq can be provided in other > >>>> >> >ways, remove all gpio->irq functionality. This cleans up the code quite > >>>> >> >a bit. > >>> >> > >>> >>In certain diagnostic modes, we need to be able to release the IRQ so > >>> >>the GPIO can be monitored from userspace. This patch removes that > >>> >>capability. > >> > > >> >Polling a GPIO from userspace is poor design regardless of the use-case, if > >> >you ask me. It certainly doesn't motivate the extra gpio<->IRQ code. > > > > I think Courtney is right here, if you want this kind of stuff for debugging > > it should be debug-add-on-patches, not part of the normal execution > > path for the driver. > > I'm OK with replacing the current polling implementation with an > add-on-patch, as long as the support for the debug-add-on-patches > doesn't get torn out of the driver because "hey, I don't see what this > is used for in the core driver code, so it must be unnecessary". An add-on patch shouldn't force dependencies on the code which it patches. Thus, the code shouldn't need support for debug add-ons. Besides, try "hey, this isn't used in the core driver code, so it is unnecessary." This whole "there's another tree which depends on things staying the way they are," isn't conducive to actually making changes to improve the driver. > >>> >>As mentioned in previous patch discussions, the polling functionality is > >>> >>quite useful during new system integration, so we need to retain that. > >>> >>If you have a proposal for where else it could be done, we'd certainly > >>> >>entertain a patch that implements that. > >> > > >> >Do you actually have systems that have these hooked up to GPIOs without > >> >IRQ support? > >> > > >> >Regardless, if this is the case, implementing a GPIO polling IRQ chip > >> >should be possible, if extremely ugly. > > OMG that would be quite horrible. > > > >> >Linus may have some comments in this area, though. Linus? > > > > A driver may rely on polling but a driver may not rely on polling > > done from userspace. > > Fortunately, the driver doesn't rely on polling from user space. > -- > 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 -- 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
On Thu, Feb 06, 2014 at 09:05:05PM +0100, Christopher Heiny wrote: > On 02/04/2014 06:31 PM, Courtney Cavin wrote: > > On Wed, Feb 05, 2014 at 12:08:35AM +0100, Christopher Heiny wrote: > >> On 01/23/2014 04:00 PM, Courtney Cavin wrote: > >>> Since all the configuration needed for an irq can be provided in other > >>> ways, remove all gpio->irq functionality. This cleans up the code quite > >>> a bit. > >> > >> In certain diagnostic modes, we need to be able to release the IRQ so > >> the GPIO can be monitored from userspace. This patch removes that > >> capability. > >> > > > > Polling a GPIO from userspace is poor design regardless of the use-case, if > > you ask me. It certainly doesn't motivate the extra gpio<->IRQ code. > > Fortunately, neither the in-driver polling implementation nor the user > space diagnostic applications are implemented that way. I'm missing something here then. If the userspace diagnostic applications do not poll the GPIO, then what exactly is the purpose of them accessing the GPIO at all? -- 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
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c index 5fb582c..780742f 100644 --- a/drivers/input/rmi4/rmi_driver.c +++ b/drivers/input/rmi4/rmi_driver.c @@ -20,7 +20,6 @@ #include <linux/delay.h> #include <linux/device.h> #include <linux/fs.h> -#include <linux/gpio.h> #include <linux/kconfig.h> #include <linux/list.h> #include <linux/module.h> @@ -50,74 +49,17 @@ static irqreturn_t rmi_irq_thread(int irq, void *p) struct rmi_transport_dev *xport = p; struct rmi_device *rmi_dev = xport->rmi_dev; struct rmi_driver *driver = rmi_dev->driver; - struct rmi_device_platform_data *pdata = xport->dev->platform_data; struct rmi_driver_data *data; data = dev_get_drvdata(&rmi_dev->dev); - - if (IRQ_DEBUG(data)) - dev_dbg(xport->dev, "ATTN gpio, value: %d.\n", - gpio_get_value(pdata->attn_gpio)); - - if (gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity) { - data->attn_count++; - if (driver && driver->irq_handler && rmi_dev) - driver->irq_handler(rmi_dev, irq); - } + if (driver && driver->irq_handler && rmi_dev) + driver->irq_handler(rmi_dev, irq); return IRQ_HANDLED; } static int process_interrupt_requests(struct rmi_device *rmi_dev); -static void rmi_poll_work(struct work_struct *work) -{ - struct rmi_driver_data *data = - container_of(work, struct rmi_driver_data, poll_work); - struct rmi_device *rmi_dev = data->rmi_dev; - - process_interrupt_requests(rmi_dev); -} - -/* - * This is the timer function for polling - it simply has to schedule work - * and restart the timer. - */ -static enum hrtimer_restart rmi_poll_timer(struct hrtimer *timer) -{ - struct rmi_driver_data *data = - container_of(timer, struct rmi_driver_data, poll_timer); - - if (!data->enabled) - return HRTIMER_NORESTART; - if (!work_pending(&data->poll_work)) - schedule_work(&data->poll_work); - hrtimer_start(&data->poll_timer, data->poll_interval, HRTIMER_MODE_REL); - return HRTIMER_NORESTART; -} - -static int enable_polling(struct rmi_device *rmi_dev) -{ - struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); - - dev_dbg(&rmi_dev->dev, "Polling enabled.\n"); - INIT_WORK(&data->poll_work, rmi_poll_work); - hrtimer_init(&data->poll_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); - data->poll_timer.function = rmi_poll_timer; - hrtimer_start(&data->poll_timer, data->poll_interval, HRTIMER_MODE_REL); - - return 0; -} - -static void disable_polling(struct rmi_device *rmi_dev) -{ - struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); - - dev_dbg(&rmi_dev->dev, "Polling disabled.\n"); - hrtimer_cancel(&data->poll_timer); - cancel_work_sync(&data->poll_work); -} - static void disable_sensor(struct rmi_device *rmi_dev) { struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); @@ -125,9 +67,6 @@ static void disable_sensor(struct rmi_device *rmi_dev) if (!data->enabled) return; - if (!data->irq) - disable_polling(rmi_dev); - if (rmi_dev->xport->ops->disable_device) rmi_dev->xport->ops->disable_device(rmi_dev->xport); @@ -155,20 +94,14 @@ static int enable_sensor(struct rmi_device *rmi_dev) } xport = rmi_dev->xport; - if (data->irq) { - retval = request_threaded_irq(data->irq, - xport->hard_irq ? xport->hard_irq : NULL, - xport->irq_thread ? - xport->irq_thread : rmi_irq_thread, - data->irq_flags, - dev_name(&rmi_dev->dev), xport); - if (retval) - return retval; - } else { - retval = enable_polling(rmi_dev); - if (retval < 0) - return retval; - } + retval = request_threaded_irq(data->irq, + xport->hard_irq ? xport->hard_irq : NULL, + xport->irq_thread ? + xport->irq_thread : rmi_irq_thread, + IRQF_ONESHOT, + dev_name(&rmi_dev->dev), xport); + if (retval) + return retval; data->enabled = true; @@ -751,16 +684,9 @@ static SIMPLE_DEV_PM_OPS(rmi_driver_pm, rmi_driver_suspend, rmi_driver_resume); static int rmi_driver_remove(struct device *dev) { struct rmi_device *rmi_dev = to_rmi_device(dev); - const struct rmi_device_platform_data *pdata = - to_rmi_platform_data(rmi_dev); - const struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); - disable_sensor(rmi_dev); rmi_free_function_list(rmi_dev); - if (data->gpio_held) - gpio_free(pdata->attn_gpio); - return 0; } @@ -895,51 +821,12 @@ static int rmi_driver_probe(struct device *dev) mutex_init(&data->suspend_mutex); } - if (gpio_is_valid(pdata->attn_gpio)) { - static const char GPIO_LABEL[] = "attn"; - unsigned long gpio_flags = GPIOF_DIR_IN; - - data->irq = gpio_to_irq(pdata->attn_gpio); - if (pdata->level_triggered) { - data->irq_flags = IRQF_ONESHOT | - ((pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH) - ? IRQF_TRIGGER_HIGH : IRQF_TRIGGER_LOW); - } else { - data->irq_flags = - (pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH) - ? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING; - } + data->irq = pdata->irq; + if (data->irq < 0) { + dev_err(dev, "Failed to get attn IRQ.\n"); + retval = data->irq; + goto err_free_data; - if (IS_ENABLED(CONFIG_RMI4_DEV)) - gpio_flags |= GPIOF_EXPORT; - - retval = gpio_request_one(pdata->attn_gpio, gpio_flags, - GPIO_LABEL); - if (retval) { - dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code=%d.\n", - pdata->attn_gpio, retval); - retval = 0; - } else { - dev_info(dev, "Obtained ATTN gpio %d.\n", - pdata->attn_gpio); - data->gpio_held = true; - if (IS_ENABLED(CONFIG_RMI4_DEV)) { - retval = gpio_export_link(dev, - GPIO_LABEL, pdata->attn_gpio); - if (retval) { - dev_warn(dev, - "WARNING: Failed to symlink ATTN gpio!\n"); - retval = 0; - } else { - dev_info(dev, "Exported ATTN gpio %d.", - pdata->attn_gpio); - } - } - } - } else { - data->poll_interval = ktime_set(0, - (pdata->poll_interval_ms ? pdata->poll_interval_ms : - DEFAULT_POLL_INTERVAL_MS) * 1000 * 1000); } if (data->f01_container->dev.driver) { diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h index 4f44a54..aef5521 100644 --- a/drivers/input/rmi4/rmi_driver.h +++ b/drivers/input/rmi4/rmi_driver.h @@ -39,9 +39,7 @@ struct rmi_driver_data { u32 attn_count; u32 irq_debug; /* Should be bool, but debugfs wants u32 */ - bool gpio_held; int irq; - int irq_flags; int num_of_irq_regs; int irq_count; unsigned long *irq_status; @@ -50,11 +48,6 @@ struct rmi_driver_data { bool irq_stored; struct mutex irq_mutex; - /* Following are used when polling. */ - struct hrtimer poll_timer; - struct work_struct poll_work; - ktime_t poll_interval; - struct mutex pdt_mutex; u8 pdt_props; u8 bsr; diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c index 910f05c..aebf974 100644 --- a/drivers/input/rmi4/rmi_i2c.c +++ b/drivers/input/rmi4/rmi_i2c.c @@ -196,8 +196,7 @@ static int rmi_i2c_probe(struct i2c_client *client, return -EINVAL; } - dev_dbg(&client->dev, "Probing %#02x (GPIO %d).\n", - client->addr, pdata->attn_gpio); + dev_dbg(&client->dev, "Probing %#02x.\n", client->addr); if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { dev_err(&client->dev, @@ -205,15 +204,6 @@ static int rmi_i2c_probe(struct i2c_client *client, return -ENODEV; } - if (pdata->gpio_config) { - retval = pdata->gpio_config(pdata->gpio_data, true); - if (retval < 0) { - dev_err(&client->dev, "Failed to configure GPIOs, code: %d.\n", - retval); - return retval; - } - } - rmi_i2c = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_xport), GFP_KERNEL); if (!rmi_i2c) @@ -240,7 +230,7 @@ static int rmi_i2c_probe(struct i2c_client *client, if (retval) { dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n", client->addr); - goto err_gpio; + goto err; } i2c_set_clientdata(client, rmi_i2c); @@ -249,24 +239,16 @@ static int rmi_i2c_probe(struct i2c_client *client, client->addr); return 0; -err_gpio: - if (pdata->gpio_config) - pdata->gpio_config(pdata->gpio_data, false); - +err: return retval; } static int rmi_i2c_remove(struct i2c_client *client) { - const struct rmi_device_platform_data *pdata = - dev_get_platdata(&client->dev); struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client); rmi_unregister_transport_device(&rmi_i2c->xport); - if (pdata->gpio_config) - pdata->gpio_config(pdata->gpio_data, false); - return 0; } diff --git a/include/linux/rmi.h b/include/linux/rmi.h index 65b59b5..326e741 100644 --- a/include/linux/rmi.h +++ b/include/linux/rmi.h @@ -23,11 +23,6 @@ #include <linux/wait.h> #include <linux/debugfs.h> -enum rmi_attn_polarity { - RMI_ATTN_ACTIVE_LOW = 0, - RMI_ATTN_ACTIVE_HIGH = 1 -}; - /** * struct rmi_f11_axis_alignment - target axis alignment * @swap_axes: set to TRUE if desired to swap x- and y-axis @@ -194,25 +189,10 @@ struct rmi_device_platform_data_spi { /** * struct rmi_device_platform_data - system specific configuration info. * + * @irq - attention IRQ * @firmware_name - if specified will override default firmware name, * for reflashing. * - * @attn_gpio - the index of a GPIO that will be used to provide the ATTN - * interrupt from the touch sensor. - * @attn_polarity - indicates whether ATTN is active high or low. - * @level_triggered - by default, the driver uses edge triggered interrupts. - * However, this can cause problems with suspend/resume on some platforms. In - * that case, set this to 1 to use level triggered interrupts. - * @gpio_config - a routine that will be called when the driver is loaded to - * perform any platform specific GPIO configuration, and when it is unloaded - * for GPIO de-configuration. This is typically used to configure the ATTN - * GPIO and the I2C or SPI pins, if necessary. - * @gpio_data - platform specific data to be passed to the GPIO configuration - * function. - * - * @poll_interval_ms - the time in milliseconds between reads of the interrupt - * status register. This is ignored if attn_gpio is non-zero. - * * @reset_delay_ms - after issuing a reset command to the touch sensor, the * driver waits a few milliseconds to give the firmware a chance to * to re-initialize. You can override the default wait period here. @@ -245,14 +225,7 @@ struct rmi_device_platform_data_spi { * functions. */ struct rmi_device_platform_data { - int attn_gpio; - enum rmi_attn_polarity attn_polarity; - bool level_triggered; - void *gpio_data; - int (*gpio_config)(void *gpio_data, bool configure); - - int poll_interval_ms; - + int irq; int reset_delay_ms; struct rmi_device_platform_data_spi spi_data;
Since all the configuration needed for an irq can be provided in other ways, remove all gpio->irq functionality. This cleans up the code quite a bit. This also gets rid of polling functionality, as this should be done elsewhere if absolutely needed. Cc: Christopher Heiny <cheiny@synaptics.com> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> --- drivers/input/rmi4/rmi_driver.c | 143 +++++----------------------------------- drivers/input/rmi4/rmi_driver.h | 7 -- drivers/input/rmi4/rmi_i2c.c | 24 +------ include/linux/rmi.h | 31 +-------- 4 files changed, 20 insertions(+), 185 deletions(-)