* [PATCH 2/2] input: egalax: report end state in suspend callback
From: Felipe F. Tonello @ 2013-12-31 2:06 UTC (permalink / raw)
To: linux-input
Cc: Felipe F. Tonello, linux-kernel, Henrik Rydberg, Dmitry Torokhov
In-Reply-To: <1388455601-17033-1-git-send-email-eu@felipetonello.com>
From: "Felipe F. Tonello" <eu@felipetonello.com>
Make sure user-space will receive the touch end event even if the device
driver suspends while touchscreen is still active (with fingers touching it).
Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
drivers/input/touchscreen/egalax_ts.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c
index 054d225..9cf4cdb 100644
--- a/drivers/input/touchscreen/egalax_ts.c
+++ b/drivers/input/touchscreen/egalax_ts.c
@@ -247,8 +247,13 @@ static int egalax_ts_suspend(struct device *dev)
0x3, 0x6, 0xa, 0x3, 0x36, 0x3f, 0x2, 0, 0, 0
};
struct i2c_client *client = to_i2c_client(dev);
+ struct egalax_ts *ts = i2c_get_clientdata(client);
int ret;
+ input_mt_report_end_state(ts->input_dev);
+ input_mt_report_pointer_emulation(ts->input_dev, true);
+ input_sync(ts->input_dev);
+
ret = i2c_master_send(client, suspend_cmd, MAX_I2C_DATA_LEN);
return ret > 0 ? 0 : ret;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH V5] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
From: Christopher Heiny @ 2013-12-31 2:05 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires
This patch fixes some bugs in handling of the RMI4 attention line GPIO.
1) in enable_sensor(), eliminate the complicated check on ATTN and just
call process_interrupt_requests(). This will have minimal overhead if ATTN
is not asserted, and clears the state of the RMI4 device in any case.
2) Correctly free the GPIO in rmi_driver_remove().
3) in rmi_driver_probe()
- declare the name of the attention gpio (GPIO_LABEL)
- use gpio_request_one() to get the gpio and export it.
- simplify (somewhat) conditional gpio acquisition logic and combine
with interrupt setup
4) use gpio_is_valid() instead of comparing to 0.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/rmi4/rmi_driver.c | 66 ++++++++++++++++++++++++-----------------
drivers/input/rmi4/rmi_driver.h | 1 +
2 files changed, 40 insertions(+), 27 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 2ae9af9..420ef56 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -140,7 +140,6 @@ static int enable_sensor(struct rmi_device *rmi_dev)
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
struct rmi_transport_dev *xport;
int retval = 0;
- struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
if (data->enabled)
return 0;
@@ -169,11 +168,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
data->enabled = true;
- if (!pdata->level_triggered &&
- gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
- retval = process_interrupt_requests(rmi_dev);
-
- return retval;
+ return process_interrupt_requests(rmi_dev);
}
static void rmi_free_function_list(struct rmi_device *rmi_dev)
@@ -800,10 +795,16 @@ 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;
}
@@ -815,6 +816,7 @@ static int rmi_driver_probe(struct device *dev)
struct rmi_device_platform_data *pdata;
int retval = 0;
struct rmi_device *rmi_dev;
+ static const char GPIO_LABEL[] = "attn";
dev_dbg(dev, "%s: Starting probe.\n", __func__);
@@ -937,7 +939,9 @@ static int rmi_driver_probe(struct device *dev)
mutex_init(&data->suspend_mutex);
}
- if (pdata->attn_gpio) {
+ if (gpio_is_valid(pdata->attn_gpio)) {
+ ulong gpio_flags = GPIOF_DIR_IN;
+
data->irq = gpio_to_irq(pdata->attn_gpio);
if (pdata->level_triggered) {
data->irq_flags = IRQF_ONESHOT |
@@ -948,33 +952,41 @@ static int rmi_driver_probe(struct device *dev)
(pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH)
? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
}
- } 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) {
- /* Driver already bound, so enable ATTN now. */
- enable_sensor(rmi_dev);
- }
- if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
- retval = gpio_export(pdata->attn_gpio, false);
+ 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 export ATTN gpio!\n");
+ dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code=%d.\n",
+ pdata->attn_gpio, retval);
retval = 0;
} else {
- retval = gpio_export_link(dev,
- "attn", 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.",
+ 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) {
+ /* Driver already bound, so enable ATTN now. */
+ enable_sensor(rmi_dev);
}
return 0;
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index 0d57700..df9ddd8 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -41,6 +41,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;
^ permalink raw reply related
* Re: [PATCH V4] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
From: Christopher Heiny @ 2013-12-31 1:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <20131228023612.GE14188@core.coreip.homeip.net>
On 12/27/2013 06:36 PM, Dmitry Torokhov wrote:
> On Fri, Dec 27, 2013 at 05:26:44PM -0800, Christopher Heiny wrote:
>> This patch fixes some bugs in handling of the RMI4 attention line GPIO.
>>
>> 1) in enable_sensor(), eliminate the complicated check on ATTN and just
>> call process_interrupt_requests(). This will have minimal overhead if ATTN
>> is not asserted, and clears the state of the RMI4 device in any case.
>>
>> 2) Correctly free the GPIO in rmi_driver_remove().
>>
>> 3) in rmi_driver_probe()
>> - declare the name of the attention gpio (GPIO_LABEL)
>> - use gpio_request_one() to get the gpio and export it.
>> - simplify (somewhat) conditional gpio acquisition logic and combine
>> with interrupt setup
>>
>> 4) use gpio_is_valid() instead of comparing to 0.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> drivers/input/rmi4/rmi_driver.c: In function ‘rmi_driver_probe’:
> drivers/input/rmi4/rmi_driver.c:920:8: error: ‘struct rmi_driver_data’
> has no member named ‘gpio_held’
> data->gpio_held = true;
>
> You forgot to include header file changes...
Oh, $%^&*(!
>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_driver.c | 64 ++++++++++++++++++++++++-----------------
>> 1 file changed, 38 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>> index 2ae9af9..766954f 100644
>> --- a/drivers/input/rmi4/rmi_driver.c
>> +++ b/drivers/input/rmi4/rmi_driver.c
>> @@ -140,7 +140,6 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>> struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
>> struct rmi_transport_dev *xport;
>> int retval = 0;
>> - struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
>>
>> if (data->enabled)
>> return 0;
>> @@ -169,11 +168,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>>
>> data->enabled = true;
>>
>> - if (!pdata->level_triggered &&
>> - gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
>> - retval = process_interrupt_requests(rmi_dev);
>> -
>> - return retval;
>> + return process_interrupt_requests(rmi_dev);
>> }
>>
>> static void rmi_free_function_list(struct rmi_device *rmi_dev)
>> @@ -800,13 +795,21 @@ 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 (gpio_is_valid(pdata->attn_gpio) && data->gpio_held)
>> + gpio_free(pdata->attn_gpio);
>
> You only need to check data->gpio_held here...
Good point.
>
>> +
>> return 0;
>> }
>>
>> +static const char GPIO_LABEL[] = "attn";
>> +
>
> While you are updating paatch could you move it in rmi_driver_probe()
> under if (gpio_is_valid()).
Can do.
>
> By the way, maybe we should have platform supply gpio name or, in its
> absence, generate unique gpio name, so that we could have several RMI
> devices be present in a box?
>
> That can be a followup patch at a later time.
Hmmm. I think it'd be better to name it as attn0, attn1, and so on.
But as you say, we can address that at a later time.
>> static int rmi_driver_probe(struct device *dev)
>> {
>> struct rmi_driver *rmi_driver;
>> @@ -937,7 +940,9 @@ static int rmi_driver_probe(struct device *dev)
>> mutex_init(&data->suspend_mutex);
>> }
>>
>> - if (pdata->attn_gpio) {
>> + if (gpio_is_valid(pdata->attn_gpio)) {
>> + ulong gpio_flags = GPIOF_DIR_IN;
>> +
>> data->irq = gpio_to_irq(pdata->attn_gpio);
>> if (pdata->level_triggered) {
>> data->irq_flags = IRQF_ONESHOT |
>> @@ -948,6 +953,32 @@ static int rmi_driver_probe(struct device *dev)
>> (pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH)
>> ? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
>> }
>> +
>> + 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
>
> } else {
>
>> data->poll_interval = ktime_set(0,
>> (pdata->poll_interval_ms ? pdata->poll_interval_ms :
>
>
> Another thing I was wondering - polling support is really unusable for
> device in production (battery gets killed) so maybe it should be removed
> altogether?
You're right that polling is not good in shipping kernels, but the
polling capability is extensively used when bringing up new hardware or
initially porting the driver onto existing hardware. I suppose we could
make the polling capability contingent on CONFIG_RMI4_DEBUG.
>
>> @@ -958,25 +989,6 @@ static int rmi_driver_probe(struct device *dev)
>> enable_sensor(rmi_dev);
>> }
>>
>> - if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
>> - retval = gpio_export(pdata->attn_gpio, false);
>> - if (retval) {
>> - dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
>> - retval = 0;
>> - } else {
>> - retval = gpio_export_link(dev,
>> - "attn", 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);
>> - }
>> - }
>> - }
>> -
>> return 0;
>>
>> err_free_data:
>
> Thanks!
--
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 03/04] input synaptics-rmi4: RMI4 F01 device control
From: Christopher Heiny @ 2013-12-31 0:26 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij
In-Reply-To: <20131230003302.GA11528@core.coreip.homeip.net>
On 12/29/2013 04:33 PM, Dmitry Torokhov wrote:
> Hi Chris,
>
> On Wed, Nov 13, 2013 at 03:39:31PM -0800, Christopher Heiny wrote:
>> * eliminate packed struct bitfield definitions.
>>
>> * removes sysfs/debugfs from the core functionality.
>>
>> * refactors register definitions into rmi_f01.h for use by external modules
>> implementing sysfs/debugfs control and debug functions.
>>
>> * adds query register parsing to extract the touch sensor firmare build ID.
>
> I know you are resubmitting this piecemeal but I decided I would provide
> some comments on these patches anyways...
Thanks - we appreciate the input a lot!
>
>>
>> +static void get_board_and_rev(struct rmi_function *fn,
>> + struct rmi_driver_data *driver_data)
>> +{
>> + struct f01_data *data = fn->data;
>> + int retval;
>> + int board = 0, rev = 0;
>> + int i;
>> + static const char * const pattern[] = {
>> + "tm%4d-%d", "s%4d-%d", "s%4d-ver%1d", "s%4d_ver%1d"};
>> + u8 product_id[RMI_PRODUCT_ID_LENGTH+1];
>> +
>> + for (i = 0; i < strlen(data->product_id); i++)
>> + product_id[i] = tolower(data->product_id[i]);
>> + product_id[i] = '\0';
>> +
>> + for (i = 0; i < ARRAY_SIZE(pattern); i++) {
>> + retval = sscanf(product_id, pattern[i], &board, &rev);
>> + if (retval)
>> + break;
>
> I think you want to rest of retval == 2 here to make sure that both
> board and revision have been parsed.
>
> I wonder though, do you really need to parse this in kernel? Where is
> this data being used?
This data was used in setting some of the input subsystem parameters in
some code that got excised. This function is unlikely to return in
future patches.
>
>> + }
>> +
>> + /* save board and rev data in the rmi_driver_data */
>> + driver_data->board = board;
>> + driver_data->rev = rev;
>> + dev_dbg(&fn->dev, "From product ID %s, set board: %d rev: %d\n",
>> + product_id, driver_data->board, driver_data->rev);
>> +}
>> +
>> +#define PACKAGE_ID_BYTES 4
>> +#define BUILD_ID_BYTES 3
>> +
>> static int rmi_f01_initialize(struct rmi_function *fn)
>> {
>> u8 temp;
>> + int i;
>> int error;
>> - u16 ctrl_base_addr;
>> + u16 query_addr = fn->fd.query_base_addr;
>> + u16 prod_info_addr;
>> + u8 info_buf[4];
>> + u16 ctrl_base_addr = fn->fd.control_base_addr;
>> struct rmi_device *rmi_dev = fn->rmi_dev;
>> struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
>> struct f01_data *data = fn->data;
>> struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
>> u8 basic_query[RMI_F01_BASIC_QUERY_LEN];
>> + struct f01_basic_properties *props = &data->properties;
>>
>> mutex_init(&data->control_mutex);
>>
>> - /*
>> - * Set the configured bit and (optionally) other important stuff
>> - * in the device control register.
>> - */
>> - ctrl_base_addr = fn->fd.control_base_addr;
>> + /* Set the configured bit and (optionally) other important stuff
>> + * in the device control register. */
>
> Please use the following style for multi-line comments:
>
> /*
> * This is a multi-line
> * comment.
> */
Will do.
>
>> error = rmi_read_block(rmi_dev, fn->fd.control_base_addr,
>> &data->device_control.ctrl0,
>> sizeof(data->device_control.ctrl0));
>> @@ -978,8 +110,7 @@ static int rmi_f01_initialize(struct rmi_function *fn)
>> break;
>> }
>>
>> - /*
>> - * Sleep mode might be set as a hangover from a system crash or
>> + /* Sleep mode might be set as a hangover from a system crash or
>> * reboot without power cycle. If so, clear it so the sensor
>> * is certain to function.
>> */
>> @@ -990,11 +121,16 @@ static int rmi_f01_initialize(struct rmi_function *fn)
>> data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
>> }
>>
>> + /* Set this to indicate that we've initialized the sensor. This will
>> + * CLEAR the unconfigured bit in the status registers. If we ever
>> + * see unconfigured become set again, we'll know that the sensor has
>> + * reset for some reason.
>> + */
>> data->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
>>
>> error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
>> - &data->device_control.ctrl0,
>> - sizeof(data->device_control.ctrl0));
>> + &data->device_control.ctrl0,
>> + sizeof(data->device_control.ctrl0));
>
> The old code had arguments aligned perfectly, why change that?
I think it's an artifact of some code refactoring that was later backed out.
>
>> if (error < 0) {
>> dev_err(&fn->dev, "Failed to write F01 control.\n");
>> return error;
>> @@ -1006,14 +142,12 @@ static int rmi_f01_initialize(struct rmi_function *fn)
>>
>> data->interrupt_enable_addr = ctrl_base_addr;
>> error = rmi_read_block(rmi_dev, ctrl_base_addr,
>> - data->device_control.interrupt_enable,
>> - sizeof(u8) * (data->num_of_irq_regs));
>> + data->device_control.interrupt_enable,
>> + sizeof(u8)*(data->num_of_irq_regs));
>
> Same here. Also please keep spaces around operators.
Will do. I'm surprised checkpatch didn't complain about that in this case.
>
>> if (error < 0) {
>> - dev_err(&fn->dev,
>> - "Failed to read F01 control interrupt enable register.\n");
>> + dev_err(&fn->dev, "Failed to read F01 control interrupt enable register.\n");
>> goto error_exit;
>> }
>> -
>> ctrl_base_addr += data->num_of_irq_regs;
>>
>> /* dummy read in order to clear irqs */
>> @@ -1023,43 +157,226 @@ static int rmi_f01_initialize(struct rmi_function *fn)
>> return error;
>> }
>>
>> + /* read queries */
>> error = rmi_read_block(rmi_dev, fn->fd.query_base_addr,
>> - basic_query, sizeof(basic_query));
>> + basic_query, sizeof(basic_query));
>> if (error < 0) {
>> dev_err(&fn->dev, "Failed to read device query registers.\n");
>> return error;
>> }
>>
>> /* Now parse what we got */
>> - data->properties.manufacturer_id = basic_query[0];
>> + props->manufacturer_id = basic_query[0];
>>
>> - data->properties.has_lts = basic_query[1] & RMI_F01_QRY1_HAS_LTS;
>> - data->properties.has_adjustable_doze =
>> + props->has_lts = basic_query[1] & RMI_F01_QRY1_HAS_LTS;
>> + props->has_sensor_id =
>> + !!(basic_query[1] & RMI_F01_QRY1_HAS_SENSOR_ID);
>
> I believe compiler will do the proper conversion to boolean for you
> since the target of assignment is boolean.
OK.
>
>> + props->has_adjustable_doze =
>> basic_query[1] & RMI_F01_QRY1_HAS_ADJ_DOZE;
>> - data->properties.has_adjustable_doze_holdoff =
>> + props->has_adjustable_doze_holdoff =
>> basic_query[1] & RMI_F01_QRY1_HAS_ADJ_DOZE_HOFF;
>> + props->has_query42 = basic_query[1] & RMI_F01_QRY1_HAS_PROPS_2;
>> +
>> + snprintf(props->dom, sizeof(props->dom), "20%02d/%02d/%02d",
>> + basic_query[5] & RMI_F01_QRY5_YEAR_MASK,
>> + basic_query[6] & RMI_F01_QRY6_MONTH_MASK,
>> + basic_query[7] & RMI_F01_QRY7_DAY_MASK);
>> +
>> + memcpy(props->product_id, &basic_query[11], RMI_PRODUCT_ID_LENGTH);
>> + props->product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
>> + query_addr += 11;
>> +
>> + error = rmi_read_block(rmi_dev, query_addr, data->product_id,
>> + RMI_PRODUCT_ID_LENGTH);
>> + if (error < 0) {
>> + dev_err(&fn->dev, "Failed to read product ID.\n");
>> + return error;
>> + }
>> + data->product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
>> + get_board_and_rev(fn, driver_data);
>> + dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s, date: %s\n",
>> + props->manufacturer_id == 1 ?
>> + "synaptics" : "unknown", data->product_id, props->dom);
>
> Capitalize your company name?
OK.
[snip]
>>
>> - data->properties.productinfo =
>> - ((basic_query[2] & RMI_F01_QRY2_PRODINFO_MASK) << 7) |
>> - (basic_query[3] & RMI_F01_QRY2_PRODINFO_MASK);
>> + /* The firmware build id (if present) is similarly overlaid on product
>> + * ID registers. Go back again and read that data.
>> + */
>> + if (props->has_build_id_query) {
>> + error = rmi_read_block(rmi_dev, prod_info_addr, info_buf,
>> + BUILD_ID_BYTES);
>> + if (error < 0)
>> + dev_warn(&fn->dev, "Failed to read FW build ID.\n");
>> + else {
>> + u16 *val = (u16 *)info_buf;
>> + data->build_id = le16_to_cpu(*val);
>
> Did you try that with sparse? I do not think it will be happy here...
> Something like
>
> data->build_id = le16_to_cpup((__le16 *)info_buf);
Hmmmm. I didn't see any sparse complaints originally, but maybe we
didn't have the right flags set. We'll check on this.
>
>> + data->build_id += info_buf[2] * 65536;
>> + dev_info(&fn->dev, "FW build ID: %#08x (%u).\n",
>> + data->build_id, data->build_id);
>> + }
>> + }
>>
[snip]
>> +
>> +/*
>> + *
>> + * @serialization - 7 bytes of device serialization data. The meaning of
>> + * these bytes varies from product to product, consult your product spec sheet.
>> + */
>> +struct f01_data {
>> + struct f01_device_control device_control;
>> + struct mutex control_mutex;
>> +
>> + u8 device_status;
>> +
>> + struct f01_basic_properties properties;
>> + u8 serialization[F01_SERIALIZATION_SIZE];
>> + u8 product_id[RMI_PRODUCT_ID_LENGTH+1];
>> +
>> + u16 package_id;
>> + u16 package_rev;
>> + u32 build_id;
>> +
>> + u16 interrupt_enable_addr;
>> + u16 doze_interval_addr;
>> + u16 wakeup_threshold_addr;
>> + u16 doze_holdoff_addr;
>> +
>> + int irq_count;
>> + int num_of_irq_regs;
>> +
>> +#ifdef CONFIG_PM
>
> I think you want CONFIG_PM_SLEEP here to mirror implementation of
> susped/resume methods.
OK.
^ permalink raw reply
* Advise to invest in your Country
From: nkoffi396 @ 2013-12-30 18:05 UTC (permalink / raw)
To: linux-input
Dear Friend,
I need someone in your Country I want to invest 30MillionEuros. For more info send me your telephone number for conversation to enable us have a concrete discussion and meeting.
Best Regards
Mr.Norbert Koffi
^ permalink raw reply
* Re: [PATCH] Input: gamepad - use independent axes for analog D-Pad buttons
From: Dmitry Torokhov @ 2013-12-30 18:51 UTC (permalink / raw)
To: Antonio Ospite; +Cc: linux-input, David Herrmann, Jiri Kosina
In-Reply-To: <20131230122020.ebaaefb09375e371c2178b38@studenti.unina.it>
On Mon, Dec 30, 2013 at 12:20:20PM +0100, Antonio Ospite wrote:
> On Sun, 29 Dec 2013 16:52:09 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> > Hi Antonio,
> >
> > On Mon, Dec 23, 2013 at 05:17:43PM +0100, Antonio Ospite wrote:
> > > Model this part of the API after the Sony PlayStation 3 Controller which
> > > exposes independent analog values for each one of the D-Pad buttons.
> > >
> > > The PS3 programming API psl1ght also maps the analog D-Pad buttons
> > > individually.
> >
> > Hmm, even though the hardware is capable of producing independent analog
> > values does are they really useful? Looking at my PS3 controller I do
> > not think users will be pressing up/down and left/right dpad buttons at
> > the same time.
> >
>
> I must agree it's unlikely, while still possible.
>
> > I'd rather keep using ABS_HAT0X/Y unless there is really good reason for
> > introducing new events.
> >
>
> Having analog D-Pad values reported independently was proposed for these
> reasons:
>
> - it matches _some_ existing hardware closely (that was the main
> reason TBH, to simplify the drivers);
>
> - it happens to follow what it's being done for D-Pad digital buttons,
> they are also reported independently.
>
> However if some other hardware reported D-Pad axis combined already
> then I agree that using something like ABS_HAT0X/Y is safer, I see
> decomposing HORIZ/VERT into the separate LEFT/RIGHT and UP/DOWN to be
> less intuitive (not harder tho).
>
> Another doubt: David, in the other email you mentioned we could use
> ABS_DPAD_HORIZ/VERT, any particular reason to introduce them in place
> of ABS_HAT0X/Y?
I think xpad driver mapping (while in no way perfect) uses HAT0 for that.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: gamepad - use independent axes for analog D-Pad buttons
From: Dmitry Torokhov @ 2013-12-30 18:50 UTC (permalink / raw)
To: David Herrmann; +Cc: Antonio Ospite, open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <CANq1E4TQJs_pqzL8u_zTneDg6_4Bt_s5DBaCJ1ei+mvhVxpgLQ@mail.gmail.com>
On Mon, Dec 30, 2013 at 12:44:21PM +0100, David Herrmann wrote:
> Hi
>
> On Mon, Dec 30, 2013 at 12:20 PM, Antonio Ospite
> <ospite@studenti.unina.it> wrote:
> > On Sun, 29 Dec 2013 16:52:09 -0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >
> >> Hi Antonio,
> >>
> >> On Mon, Dec 23, 2013 at 05:17:43PM +0100, Antonio Ospite wrote:
> >> > Model this part of the API after the Sony PlayStation 3 Controller which
> >> > exposes independent analog values for each one of the D-Pad buttons.
> >> >
> >> > The PS3 programming API psl1ght also maps the analog D-Pad buttons
> >> > individually.
> >>
> >> Hmm, even though the hardware is capable of producing independent analog
> >> values does are they really useful? Looking at my PS3 controller I do
> >> not think users will be pressing up/down and left/right dpad buttons at
> >> the same time.
> >>
> >
> > I must agree it's unlikely, while still possible.
> >
> >> I'd rather keep using ABS_HAT0X/Y unless there is really good reason for
> >> introducing new events.
> >>
> >
> > Having analog D-Pad values reported independently was proposed for these
> > reasons:
> >
> > - it matches _some_ existing hardware closely (that was the main
> > reason TBH, to simplify the drivers);
> >
> > - it happens to follow what it's being done for D-Pad digital buttons,
> > they are also reported independently.
> >
> > However if some other hardware reported D-Pad axis combined already
> > then I agree that using something like ABS_HAT0X/Y is safer, I see
> > decomposing HORIZ/VERT into the separate LEFT/RIGHT and UP/DOWN to be
> > less intuitive (not harder tho).
> >
> > Another doubt: David, in the other email you mentioned we could use
> > ABS_DPAD_HORIZ/VERT, any particular reason to introduce them in place
> > of ABS_HAT0X/Y?
>
> A "HAT" is an axis on the top of a joystick, nothing else. It seems
> troublesome to overload the definition (which we did for many years).
> Device-detection based on advertised ABS-bits gets pretty hard if we
> reuse ABS-bits for different hardware. The most annoying example of
> what happens is accelerometers being misdetected by Xorg as
> mouse-input because they reuse ABS_X/Y.
But they are good as joysticks (also ABS_X/ABS_Y). I do not think having
all unique events for all device types is feasible. Can we provide hints
(via properties) to lessen ambiguity, like we do with direct/indirect
pointers for touchscreens/tablets?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: kernel panic on gpio-keys
From: Dmitry Torokhov @ 2013-12-30 18:42 UTC (permalink / raw)
To: Paul Cercueil; +Cc: linux-input, Henrik Rydberg
In-Reply-To: <52C18D88.9070707@gmail.com>
On Mon, Dec 30, 2013 at 04:13:12PM +0100, Paul Cercueil wrote:
> On 27/12/2013 02:56, Dmitry Torokhov wrote:
> >On Tue, Dec 17, 2013 at 04:17:34PM +0100, Paul Cercueil wrote:
> >>On 14/12/2013 10:39, Dmitry Torokhov wrote:
> >>>On Wed, Dec 11, 2013 at 08:17:29PM +0100, Paul Cercueil wrote:
> >>>>Hi there,
> >>>>
> >>>>I am trying to use the gpio-keys driver to inject joystick events.
> >>>>There seems to be some basic support of it, looking at <linux/gpio_keys.h>.
> >>>>
> >>>>However, registering the following will trigger a kernel panic in
> >>>>the kernel:
> >>>>
> >>>>static struct gpio_keys_button my_buttons[] {
> >>>> {
> >>>> .gpio = GPIO_FOO,
> >>>> .type = EV_ABS,
> >>>> .code = ABS_HAT0X,
> >>>> .value = 1,
> >>>> },
> >>>>};
> >>>>
> >>>>(tested on kernel 3.12).
> >>>>
> >>>>I don't know well the input subsystem, so I have no idea of what is
> >>>>going wrong. Could anybody try to at least reproduce the issue?
> >>>
> >>>It woudl be helpful if you poster the stack trace from panic so we'd
> >>>have an idea where the fault happens.
> >>>
> >>>Thanks.
> >>>
> >>
> >>Here is the crash log I get: http://pastebin.com/FzTTGxsR
> >>(I did put it on pastebin because it's huge, 200+ lines).
> >>
> >>The first OOPS happen as soon as the GPIO button is pressed; the
> >>other ones seem to happen recursively. I included only a part of the
> >>log I get, as the OOPSes continue to flow until the watchdog kicks
> >>in.
> >
> >Hmm, I have an idea: this driver is one of few that does not use
> >input_set_abs_info() and this does not allocate memory for absinfo data
> >that input core uses to handle absolute events.
> >
> >Does the patch below work for you?
> >
> >Thanks.
> >
>
> Hi,
>
> The patch works just fine, thank you!
Thanks Paul.
--
Dmitry
^ permalink raw reply
* Re: kernel panic on gpio-keys
From: Paul Cercueil @ 2013-12-30 15:13 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Henrik Rydberg
In-Reply-To: <20131227015658.GA20756@core.coreip.homeip.net>
On 27/12/2013 02:56, Dmitry Torokhov wrote:
> On Tue, Dec 17, 2013 at 04:17:34PM +0100, Paul Cercueil wrote:
>> On 14/12/2013 10:39, Dmitry Torokhov wrote:
>>> On Wed, Dec 11, 2013 at 08:17:29PM +0100, Paul Cercueil wrote:
>>>> Hi there,
>>>>
>>>> I am trying to use the gpio-keys driver to inject joystick events.
>>>> There seems to be some basic support of it, looking at <linux/gpio_keys.h>.
>>>>
>>>> However, registering the following will trigger a kernel panic in
>>>> the kernel:
>>>>
>>>> static struct gpio_keys_button my_buttons[] {
>>>> {
>>>> .gpio = GPIO_FOO,
>>>> .type = EV_ABS,
>>>> .code = ABS_HAT0X,
>>>> .value = 1,
>>>> },
>>>> };
>>>>
>>>> (tested on kernel 3.12).
>>>>
>>>> I don't know well the input subsystem, so I have no idea of what is
>>>> going wrong. Could anybody try to at least reproduce the issue?
>>>
>>> It woudl be helpful if you poster the stack trace from panic so we'd
>>> have an idea where the fault happens.
>>>
>>> Thanks.
>>>
>>
>> Here is the crash log I get: http://pastebin.com/FzTTGxsR
>> (I did put it on pastebin because it's huge, 200+ lines).
>>
>> The first OOPS happen as soon as the GPIO button is pressed; the
>> other ones seem to happen recursively. I included only a part of the
>> log I get, as the OOPSes continue to flow until the watchdog kicks
>> in.
>
> Hmm, I have an idea: this driver is one of few that does not use
> input_set_abs_info() and this does not allocate memory for absinfo data
> that input core uses to handle absolute events.
>
> Does the patch below work for you?
>
> Thanks.
>
Hi,
The patch works just fine, thank you!
^ permalink raw reply
* Re: hello, please help me check this potential bug, thanks.
From: Lars-Peter Clausen @ 2013-12-30 13:45 UTC (permalink / raw)
To: Huqiu Liu; +Cc: yinj2010, Linux Input
In-Reply-To: <52C11B36.167D70.09200@tsinghua.edu.cn>
Added linux-input to Cc
On 12/30/2013 08:05 AM, Huqiu Liu wrote:
> Dear Lars-Peter Clausen,
> I'm very sorry to trouble you. Recently I checked lots of drivers and found the
> following potential bug:
> 1) In the file _/drivers\input\misc\pwm-beeper.c/_, the function
> *input_allocate_device * is called by the function *pwm_beeper_probe*. While the
> function *pwm_beeper_remove * does not call the function *input_free_device * to
> release the acquired resources when removing the device. Generelly the function
> *input_free_device * and *input_allocate_device * should be used in pairs, and
> more than *109 * drivers have called the function *input_free_device * to
> release the resources which are acquired by the function *input_allocate_device*.
> The acqured resources are ignored to be released when removing the device, which
> would cause memory leaks and other potential problems. Could you help me to
> check or confirm this bug please? Thank you very much. I'm looking forward to
> your reply.
I think the code is corect. See the input_allocate_device() documentation:
* input_allocate_device - allocate memory for new input device
*
* Returns prepared struct input_dev or %NULL.
*
* NOTE: Use input_free_device() to free devices that have not been
* registered; input_unregister_device() should be used for already
* registered devices.
- Lars
^ permalink raw reply
* Re: [PATCH] Input: gamepad - use independent axes for analog D-Pad buttons
From: David Herrmann @ 2013-12-30 11:44 UTC (permalink / raw)
To: Antonio Ospite; +Cc: Dmitry Torokhov, open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <20131230122020.ebaaefb09375e371c2178b38@studenti.unina.it>
Hi
On Mon, Dec 30, 2013 at 12:20 PM, Antonio Ospite
<ospite@studenti.unina.it> wrote:
> On Sun, 29 Dec 2013 16:52:09 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
>> Hi Antonio,
>>
>> On Mon, Dec 23, 2013 at 05:17:43PM +0100, Antonio Ospite wrote:
>> > Model this part of the API after the Sony PlayStation 3 Controller which
>> > exposes independent analog values for each one of the D-Pad buttons.
>> >
>> > The PS3 programming API psl1ght also maps the analog D-Pad buttons
>> > individually.
>>
>> Hmm, even though the hardware is capable of producing independent analog
>> values does are they really useful? Looking at my PS3 controller I do
>> not think users will be pressing up/down and left/right dpad buttons at
>> the same time.
>>
>
> I must agree it's unlikely, while still possible.
>
>> I'd rather keep using ABS_HAT0X/Y unless there is really good reason for
>> introducing new events.
>>
>
> Having analog D-Pad values reported independently was proposed for these
> reasons:
>
> - it matches _some_ existing hardware closely (that was the main
> reason TBH, to simplify the drivers);
>
> - it happens to follow what it's being done for D-Pad digital buttons,
> they are also reported independently.
>
> However if some other hardware reported D-Pad axis combined already
> then I agree that using something like ABS_HAT0X/Y is safer, I see
> decomposing HORIZ/VERT into the separate LEFT/RIGHT and UP/DOWN to be
> less intuitive (not harder tho).
>
> Another doubt: David, in the other email you mentioned we could use
> ABS_DPAD_HORIZ/VERT, any particular reason to introduce them in place
> of ABS_HAT0X/Y?
A "HAT" is an axis on the top of a joystick, nothing else. It seems
troublesome to overload the definition (which we did for many years).
Device-detection based on advertised ABS-bits gets pretty hard if we
reuse ABS-bits for different hardware. The most annoying example of
what happens is accelerometers being misdetected by Xorg as
mouse-input because they reuse ABS_X/Y.
Thanks
David
^ permalink raw reply
* Re: [PATCH] Input: gamepad - use independent axes for analog D-Pad buttons
From: Antonio Ospite @ 2013-12-30 11:20 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, David Herrmann, Jiri Kosina
In-Reply-To: <20131230005209.GC11528@core.coreip.homeip.net>
On Sun, 29 Dec 2013 16:52:09 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Antonio,
>
> On Mon, Dec 23, 2013 at 05:17:43PM +0100, Antonio Ospite wrote:
> > Model this part of the API after the Sony PlayStation 3 Controller which
> > exposes independent analog values for each one of the D-Pad buttons.
> >
> > The PS3 programming API psl1ght also maps the analog D-Pad buttons
> > individually.
>
> Hmm, even though the hardware is capable of producing independent analog
> values does are they really useful? Looking at my PS3 controller I do
> not think users will be pressing up/down and left/right dpad buttons at
> the same time.
>
I must agree it's unlikely, while still possible.
> I'd rather keep using ABS_HAT0X/Y unless there is really good reason for
> introducing new events.
>
Having analog D-Pad values reported independently was proposed for these
reasons:
- it matches _some_ existing hardware closely (that was the main
reason TBH, to simplify the drivers);
- it happens to follow what it's being done for D-Pad digital buttons,
they are also reported independently.
However if some other hardware reported D-Pad axis combined already
then I agree that using something like ABS_HAT0X/Y is safer, I see
decomposing HORIZ/VERT into the separate LEFT/RIGHT and UP/DOWN to be
less intuitive (not harder tho).
Another doubt: David, in the other email you mentioned we could use
ABS_DPAD_HORIZ/VERT, any particular reason to introduce them in place
of ABS_HAT0X/Y?
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] Input: gamepad - use independent axes for analog D-Pad buttons
From: Dmitry Torokhov @ 2013-12-30 0:52 UTC (permalink / raw)
To: Antonio Ospite; +Cc: linux-input, David Herrmann, Jiri Kosina
In-Reply-To: <1387815463-29345-1-git-send-email-ospite@studenti.unina.it>
Hi Antonio,
On Mon, Dec 23, 2013 at 05:17:43PM +0100, Antonio Ospite wrote:
> Model this part of the API after the Sony PlayStation 3 Controller which
> exposes independent analog values for each one of the D-Pad buttons.
>
> The PS3 programming API psl1ght also maps the analog D-Pad buttons
> individually.
Hmm, even though the hardware is capable of producing independent analog
values does are they really useful? Looking at my PS3 controller I do
not think users will be pressing up/down and left/right dpad buttons at
the same time.
I'd rather keep using ABS_HAT0X/Y unless there is really good reason for
introducing new events.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: egalax: Send touch end event when is about to suspend
From: Dmitry Torokhov @ 2013-12-30 0:36 UTC (permalink / raw)
To: Felipe Ferreri Tonello
Cc: linux-kernel, linux-input, Andy Shevchenko, Sachin Kamat,
Heiko Abraham
In-Reply-To: <52AF90B6.1000204@felipetonello.com>
Hi Felipe,
On Mon, Dec 16, 2013 at 03:45:58PM -0800, Felipe Ferreri Tonello wrote:
> Hi Dmitry,
>
> On 12/15/2013 02:40 AM, Dmitry Torokhov wrote:
> >Hi Felipe,
> >
> >On Tue, Dec 10, 2013 at 04:03:16PM -0800, Felipe F. Tonello wrote:
> >>From: "Felipe F. Tonello" <eu@felipetonello.com>
> >>
> >>This is useful to report for users of this device that don't know anything
> >>about the suspension of the device. So users will receive a touch end event
> >>when the device is about to suspend, making it more user friendly.
> >>
> >>One example of users is the X Server with the evdev input driver. This patch
> >>make sure that the X server will propagate a touch end event to its windows.
> >>
> >
> >Hmm, I would argue we need to do this in input core, similarly to what
> >we do for the keys, so that all drivers would benefit from the change.
> >
> >Thanks.
>
> I agree with you. I didn't do in this case because previous patches
> that I had to change core functionality were declined due the fact
> it's hard to maintain backwards compatibility.
>
> Do you recommend me something in this case?
I am not sure why your other patches were declined, but I think in this
particular case it makes sense to move this functionality into input
core as many multitouch drivers would need it.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 03/04] input synaptics-rmi4: RMI4 F01 device control
From: Dmitry Torokhov @ 2013-12-30 0:33 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij
In-Reply-To: <1384385972-1686-4-git-send-email-cheiny@synaptics.com>
Hi Chris,
On Wed, Nov 13, 2013 at 03:39:31PM -0800, Christopher Heiny wrote:
> * eliminate packed struct bitfield definitions.
>
> * removes sysfs/debugfs from the core functionality.
>
> * refactors register definitions into rmi_f01.h for use by external modules
> implementing sysfs/debugfs control and debug functions.
>
> * adds query register parsing to extract the touch sensor firmare build ID.
I know you are resubmitting this piecemeal but I decided I would provide
some comments on these patches anyways...
>
> +static void get_board_and_rev(struct rmi_function *fn,
> + struct rmi_driver_data *driver_data)
> +{
> + struct f01_data *data = fn->data;
> + int retval;
> + int board = 0, rev = 0;
> + int i;
> + static const char * const pattern[] = {
> + "tm%4d-%d", "s%4d-%d", "s%4d-ver%1d", "s%4d_ver%1d"};
> + u8 product_id[RMI_PRODUCT_ID_LENGTH+1];
> +
> + for (i = 0; i < strlen(data->product_id); i++)
> + product_id[i] = tolower(data->product_id[i]);
> + product_id[i] = '\0';
> +
> + for (i = 0; i < ARRAY_SIZE(pattern); i++) {
> + retval = sscanf(product_id, pattern[i], &board, &rev);
> + if (retval)
> + break;
I think you want to rest of retval == 2 here to make sure that both
board and revision have been parsed.
I wonder though, do you really need to parse this in kernel? Where is
this data being used?
> + }
> +
> + /* save board and rev data in the rmi_driver_data */
> + driver_data->board = board;
> + driver_data->rev = rev;
> + dev_dbg(&fn->dev, "From product ID %s, set board: %d rev: %d\n",
> + product_id, driver_data->board, driver_data->rev);
> +}
> +
> +#define PACKAGE_ID_BYTES 4
> +#define BUILD_ID_BYTES 3
> +
> static int rmi_f01_initialize(struct rmi_function *fn)
> {
> u8 temp;
> + int i;
> int error;
> - u16 ctrl_base_addr;
> + u16 query_addr = fn->fd.query_base_addr;
> + u16 prod_info_addr;
> + u8 info_buf[4];
> + u16 ctrl_base_addr = fn->fd.control_base_addr;
> struct rmi_device *rmi_dev = fn->rmi_dev;
> struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> struct f01_data *data = fn->data;
> struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
> u8 basic_query[RMI_F01_BASIC_QUERY_LEN];
> + struct f01_basic_properties *props = &data->properties;
>
> mutex_init(&data->control_mutex);
>
> - /*
> - * Set the configured bit and (optionally) other important stuff
> - * in the device control register.
> - */
> - ctrl_base_addr = fn->fd.control_base_addr;
> + /* Set the configured bit and (optionally) other important stuff
> + * in the device control register. */
Please use the following style for multi-line comments:
/*
* This is a multi-line
* comment.
*/
> error = rmi_read_block(rmi_dev, fn->fd.control_base_addr,
> &data->device_control.ctrl0,
> sizeof(data->device_control.ctrl0));
> @@ -978,8 +110,7 @@ static int rmi_f01_initialize(struct rmi_function *fn)
> break;
> }
>
> - /*
> - * Sleep mode might be set as a hangover from a system crash or
> + /* Sleep mode might be set as a hangover from a system crash or
> * reboot without power cycle. If so, clear it so the sensor
> * is certain to function.
> */
> @@ -990,11 +121,16 @@ static int rmi_f01_initialize(struct rmi_function *fn)
> data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
> }
>
> + /* Set this to indicate that we've initialized the sensor. This will
> + * CLEAR the unconfigured bit in the status registers. If we ever
> + * see unconfigured become set again, we'll know that the sensor has
> + * reset for some reason.
> + */
> data->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
>
> error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
> - &data->device_control.ctrl0,
> - sizeof(data->device_control.ctrl0));
> + &data->device_control.ctrl0,
> + sizeof(data->device_control.ctrl0));
The old code had arguments aligned perfectly, why change that?
> if (error < 0) {
> dev_err(&fn->dev, "Failed to write F01 control.\n");
> return error;
> @@ -1006,14 +142,12 @@ static int rmi_f01_initialize(struct rmi_function *fn)
>
> data->interrupt_enable_addr = ctrl_base_addr;
> error = rmi_read_block(rmi_dev, ctrl_base_addr,
> - data->device_control.interrupt_enable,
> - sizeof(u8) * (data->num_of_irq_regs));
> + data->device_control.interrupt_enable,
> + sizeof(u8)*(data->num_of_irq_regs));
Same here. Also please keep spaces around operators.
> if (error < 0) {
> - dev_err(&fn->dev,
> - "Failed to read F01 control interrupt enable register.\n");
> + dev_err(&fn->dev, "Failed to read F01 control interrupt enable register.\n");
> goto error_exit;
> }
> -
> ctrl_base_addr += data->num_of_irq_regs;
>
> /* dummy read in order to clear irqs */
> @@ -1023,43 +157,226 @@ static int rmi_f01_initialize(struct rmi_function *fn)
> return error;
> }
>
> + /* read queries */
> error = rmi_read_block(rmi_dev, fn->fd.query_base_addr,
> - basic_query, sizeof(basic_query));
> + basic_query, sizeof(basic_query));
> if (error < 0) {
> dev_err(&fn->dev, "Failed to read device query registers.\n");
> return error;
> }
>
> /* Now parse what we got */
> - data->properties.manufacturer_id = basic_query[0];
> + props->manufacturer_id = basic_query[0];
>
> - data->properties.has_lts = basic_query[1] & RMI_F01_QRY1_HAS_LTS;
> - data->properties.has_adjustable_doze =
> + props->has_lts = basic_query[1] & RMI_F01_QRY1_HAS_LTS;
> + props->has_sensor_id =
> + !!(basic_query[1] & RMI_F01_QRY1_HAS_SENSOR_ID);
I believe compiler will do the proper conversion to boolean for you
since the target of assignment is boolean.
> + props->has_adjustable_doze =
> basic_query[1] & RMI_F01_QRY1_HAS_ADJ_DOZE;
> - data->properties.has_adjustable_doze_holdoff =
> + props->has_adjustable_doze_holdoff =
> basic_query[1] & RMI_F01_QRY1_HAS_ADJ_DOZE_HOFF;
> + props->has_query42 = basic_query[1] & RMI_F01_QRY1_HAS_PROPS_2;
> +
> + snprintf(props->dom, sizeof(props->dom), "20%02d/%02d/%02d",
> + basic_query[5] & RMI_F01_QRY5_YEAR_MASK,
> + basic_query[6] & RMI_F01_QRY6_MONTH_MASK,
> + basic_query[7] & RMI_F01_QRY7_DAY_MASK);
> +
> + memcpy(props->product_id, &basic_query[11], RMI_PRODUCT_ID_LENGTH);
> + props->product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
> + query_addr += 11;
> +
> + error = rmi_read_block(rmi_dev, query_addr, data->product_id,
> + RMI_PRODUCT_ID_LENGTH);
> + if (error < 0) {
> + dev_err(&fn->dev, "Failed to read product ID.\n");
> + return error;
> + }
> + data->product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
> + get_board_and_rev(fn, driver_data);
> + dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s, date: %s\n",
> + props->manufacturer_id == 1 ?
> + "synaptics" : "unknown", data->product_id, props->dom);
Capitalize your company name?
> +
> + /* We'll come back and use this later, depending on some other query
> + * bits.
> + */
> + prod_info_addr = query_addr + 6;
> +
> + query_addr += RMI_PRODUCT_ID_LENGTH;
> + if (props->has_lts) {
> + error = rmi_read(rmi_dev, query_addr, info_buf);
> + if (error < 0) {
> + dev_err(&fn->dev, "Failed to read LTS info.\n");
> + return error;
> + }
> + props->slave_asic_rows = info_buf[0] &
> + RMI_F01_QRY21_SLAVE_ROWS_MASK;
> + props->slave_asic_columns = (info_buf[1] &
> + RMI_F01_QRY21_SLAVE_COLUMNS_MASK) >> 3;
> + query_addr++;
> + }
> +
> + if (props->has_sensor_id) {
> + error = rmi_read(rmi_dev, query_addr, &props->sensor_id);
> + if (error < 0) {
> + dev_err(&fn->dev, "Failed to read sensor ID.\n");
> + return error;
> + }
> + query_addr++;
> + }
> +
> + /* Maybe skip a block of undefined LTS registers. */
> + if (props->has_lts)
> + query_addr += RMI_F01_LTS_RESERVED_SIZE;
> +
> + if (props->has_query42) {
> + error = rmi_read(rmi_dev, query_addr, info_buf);
> + if (error < 0) {
> + dev_err(&fn->dev, "Failed to read additional properties.\n");
> + return error;
> + }
> + props->has_ds4_queries = info_buf[0] &
> + RMI_F01_QRY42_DS4_QUERIES;
> + props->has_multi_physical = info_buf[0] &
> + RMI_F01_QRY42_MULTI_PHYS;
> + props->has_guest = info_buf[0] & RMI_F01_QRY42_GUEST;
> + props->has_swr = info_buf[0] & RMI_F01_QRY42_SWR;
> + props->has_nominal_report_rate = info_buf[0] &
> + RMI_F01_QRY42_NOMINAL_REPORT;
> + props->has_recalibration_interval = info_buf[0] &
> + RMI_F01_QRY42_RECAL_INTERVAL;
> + query_addr++;
> + }
> +
> + if (props->has_ds4_queries) {
> + error = rmi_read(rmi_dev, query_addr, &props->ds4_query_length);
> + if (error < 0) {
> + dev_err(&fn->dev, "Failed to read DS4 query length size.\n");
> + return error;
> + }
> + query_addr++;
> + }
>
> - snprintf(data->properties.dom, sizeof(data->properties.dom),
> - "20%02x%02x%02x",
> - basic_query[5] & RMI_F01_QRY5_YEAR_MASK,
> - basic_query[6] & RMI_F01_QRY6_MONTH_MASK,
> - basic_query[7] & RMI_F01_QRY7_DAY_MASK);
> + for (i = 1; i <= props->ds4_query_length; i++) {
> + u8 val;
> + error = rmi_read(rmi_dev, query_addr, &val);
> + query_addr++;
> + if (error < 0) {
> + dev_err(&fn->dev, "Failed to read F01_RMI_QUERY43.%02d, code: %d.\n",
> + i, error);
> + continue;
> + }
> + switch (i) {
> + case 1:
> + props->has_package_id_query = val &
> + RMI_F01_QRY43_01_PACKAGE_ID;
> + props->has_build_id_query = val &
> + RMI_F01_QRY43_01_BUILD_ID;
> + props->has_reset_query = val & RMI_F01_QRY43_01_RESET;
> + props->has_maskrev_query = val &
> + RMI_F01_QRY43_01_PACKAGE_ID;
> + break;
> + case 2:
> + props->has_i2c_control = val & RMI_F01_QRY43_02_I2C_CTL;
> + props->has_spi_control = val & RMI_F01_QRY43_02_SPI_CTL;
> + props->has_attn_control = val &
> + RMI_F01_QRY43_02_ATTN_CTL;
> + props->has_win8_vendor_info = val &
> + RMI_F01_QRY43_02_WIN8;
> + props->has_timestamp = val & RMI_F01_QRY43_02_TIMESTAMP;
> + break;
> + case 3:
> + props->has_tool_id_query = val &
> + RMI_F01_QRY43_03_TOOL_ID;
> + props->has_fw_revision_query = val &
> + RMI_F01_QRY43_03_FW_REVISION;
> + break;
> + default:
> + dev_warn(&fn->dev, "No handling for F01_RMI_QUERY43.%02d.\n",
> + i);
> + }
> + }
>
> - memcpy(data->properties.product_id, &basic_query[11],
> - RMI_PRODUCT_ID_LENGTH);
> - data->properties.product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
> + /* If present, the ASIC package ID registers are overlaid on the
> + * product ID. Go back to the right address (saved previously) and
> + * read them.
> + */
> + if (props->has_package_id_query) {
> + error = rmi_read_block(rmi_dev, prod_info_addr, info_buf,
> + PACKAGE_ID_BYTES);
> + if (error < 0)
> + dev_warn(&fn->dev, "Failed to read package ID.\n");
> + else {
> + u16 *val = (u16 *)info_buf;
> + data->package_id = le16_to_cpu(*val);
> + val = (u16 *)(info_buf + 2);
> + data->package_rev = le16_to_cpu(*val);
> + }
> + }
> + prod_info_addr++;
>
> - data->properties.productinfo =
> - ((basic_query[2] & RMI_F01_QRY2_PRODINFO_MASK) << 7) |
> - (basic_query[3] & RMI_F01_QRY2_PRODINFO_MASK);
> + /* The firmware build id (if present) is similarly overlaid on product
> + * ID registers. Go back again and read that data.
> + */
> + if (props->has_build_id_query) {
> + error = rmi_read_block(rmi_dev, prod_info_addr, info_buf,
> + BUILD_ID_BYTES);
> + if (error < 0)
> + dev_warn(&fn->dev, "Failed to read FW build ID.\n");
> + else {
> + u16 *val = (u16 *)info_buf;
> + data->build_id = le16_to_cpu(*val);
Did you try that with sparse? I do not think it will be happy here...
Something like
data->build_id = le16_to_cpup((__le16 *)info_buf);
> + data->build_id += info_buf[2] * 65536;
> + dev_info(&fn->dev, "FW build ID: %#08x (%u).\n",
> + data->build_id, data->build_id);
> + }
> + }
>
> - dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s\n",
> - data->properties.manufacturer_id == 1 ?
> - "synaptics" : "unknown",
> - data->properties.product_id);
> + if (props->has_reset_query) {
> + u8 val;
> + error = rmi_read(rmi_dev, query_addr, &val);
> + query_addr++;
> + if (error < 0)
> + dev_warn(&fn->dev, "Failed to read F01_RMI_QUERY44, code: %d.\n",
> + error);
> + else {
> + props->reset_enabled = val & RMI_F01_QRY44_RST_ENABLED;
> + props->reset_polarity = val &
> + RMI_F01_QRY44_RST_POLARITY;
> + props->pullup_enabled = val &
> + RMI_F01_QRY44_PULLUP_ENABLED;
> + props->reset_pin = (val &
> + RMI_F01_QRY44_RST_PIN_MASK) >> 4;
> + }
> + }
> +
> + if (props->has_tool_id_query) {
> + error = rmi_read_block(rmi_dev, query_addr, props->tool_id,
> + RMI_TOOL_ID_LENGTH);
> + if (error < 0)
> + dev_warn(&fn->dev, "Failed to read F01_RMI_QUERY45, code: %d.\n",
> + error);
> + /* This is a so-called "packet register", so address map
> + * increments only by one. */
> + query_addr++;
> + props->tool_id[RMI_TOOL_ID_LENGTH] = '\0';
> + }
> +
> + if (props->has_fw_revision_query) {
> + error = rmi_read_block(rmi_dev, query_addr, props->fw_revision,
> + RMI_FW_REVISION_LENGTH);
> + if (error < 0)
> + dev_warn(&fn->dev, "Failed to read F01_RMI_QUERY46, code: %d.\n",
> + error);
> + /* This is a so-called "packet register", so address map
> + * increments only by one. */
> + query_addr++;
> + props->tool_id[RMI_FW_REVISION_LENGTH] = '\0';
> + }
>
> /* read control register */
> - if (data->properties.has_adjustable_doze) {
> + if (props->has_adjustable_doze) {
> data->doze_interval_addr = ctrl_base_addr;
> ctrl_base_addr++;
>
> @@ -1103,7 +420,7 @@ static int rmi_f01_initialize(struct rmi_function *fn)
> }
> }
>
> - if (data->properties.has_adjustable_doze_holdoff) {
> + if (props->has_adjustable_doze_holdoff) {
> data->doze_holdoff_addr = ctrl_base_addr;
> ctrl_base_addr++;
>
> @@ -1133,27 +450,20 @@ static int rmi_f01_initialize(struct rmi_function *fn)
> goto error_exit;
> }
>
> - driver_data->f01_bootloader_mode =
> - RMI_F01_STATUS_BOOTLOADER(data->device_status);
> - if (driver_data->f01_bootloader_mode)
> - dev_warn(&rmi_dev->dev,
> - "WARNING: RMI4 device is in bootloader mode!\n");
> -
> -
> if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
> - dev_err(&fn->dev,
> - "Device was reset during configuration process, status: %#02x!\n",
> - RMI_F01_STATUS_CODE(data->device_status));
> + dev_err(&fn->dev, "Device reset during configuration process, status: %#02x!\n",
> + RMI_F01_STATUS_CODE(data->device_status));
> error = -EINVAL;
> goto error_exit;
> }
>
> - error = setup_debugfs(fn);
> - if (error)
> - dev_warn(&fn->dev, "Failed to setup debugfs, error: %d.\n",
> - error);
> + driver_data->f01_bootloader_mode =
> + RMI_F01_STATUS_BOOTLOADER(data->device_status);
> + if (RMI_F01_STATUS_BOOTLOADER(data->device_status))
> + dev_warn(&rmi_dev->dev,
> + "WARNING: RMI4 device is in bootloader mode!\n");
>
> - return 0;
> + return error;
>
> error_exit:
> kfree(data);
> @@ -1166,36 +476,33 @@ static int rmi_f01_config(struct rmi_function *fn)
> int retval;
>
> retval = rmi_write_block(fn->rmi_dev, fn->fd.control_base_addr,
> - &data->device_control.ctrl0,
> - sizeof(data->device_control.ctrl0));
> + &data->device_control.ctrl0,
> + sizeof(data->device_control.ctrl0));
> if (retval < 0) {
> dev_err(&fn->dev, "Failed to write device_control.reg.\n");
> return retval;
> }
>
> retval = rmi_write_block(fn->rmi_dev, data->interrupt_enable_addr,
> - data->device_control.interrupt_enable,
> - sizeof(u8) * data->num_of_irq_regs);
> + data->device_control.interrupt_enable,
> + sizeof(u8)*(data->num_of_irq_regs));
>
> if (retval < 0) {
> dev_err(&fn->dev, "Failed to write interrupt enable.\n");
> return retval;
> }
> - if (data->properties.has_lts) {
> - retval = rmi_write_block(fn->rmi_dev, data->doze_interval_addr,
> - &data->device_control.doze_interval,
> - sizeof(u8));
> +
> + if (data->properties.has_adjustable_doze) {
> + retval = rmi_write(fn->rmi_dev,
> + data->doze_interval_addr,
> + data->device_control.doze_interval);
> if (retval < 0) {
> dev_err(&fn->dev, "Failed to write doze interval.\n");
> return retval;
> }
> - }
> -
> - if (data->properties.has_adjustable_doze) {
> - retval = rmi_write_block(fn->rmi_dev,
> - data->wakeup_threshold_addr,
> - &data->device_control.wakeup_threshold,
> - sizeof(u8));
> + retval = rmi_write(
> + fn->rmi_dev, data->wakeup_threshold_addr,
> + data->device_control.wakeup_threshold);
> if (retval < 0) {
> dev_err(&fn->dev, "Failed to write wakeup threshold.\n");
> return retval;
> @@ -1203,9 +510,9 @@ static int rmi_f01_config(struct rmi_function *fn)
> }
>
> if (data->properties.has_adjustable_doze_holdoff) {
> - retval = rmi_write_block(fn->rmi_dev, data->doze_holdoff_addr,
> - &data->device_control.doze_holdoff,
> - sizeof(u8));
> + retval = rmi_write(fn->rmi_dev,
> + data->doze_holdoff_addr,
> + data->device_control.doze_holdoff);
> if (retval < 0) {
> dev_err(&fn->dev, "Failed to write doze holdoff.\n");
> return retval;
> @@ -1221,51 +528,40 @@ static int rmi_f01_probe(struct rmi_function *fn)
> int error;
>
> error = rmi_f01_alloc_memory(fn, driver_data->num_of_irq_regs);
> - if (error)
> + if (error < 0)
> return error;
>
> error = rmi_f01_initialize(fn);
> - if (error)
> - return error;
> -
> - error = sysfs_create_group(&fn->dev.kobj, &rmi_fn_01_attr_group);
> - if (error)
> + if (error < 0)
> return error;
>
> return 0;
> }
>
> -static void rmi_f01_remove(struct rmi_function *fn)
> -{
> - teardown_debugfs(fn->data);
> - sysfs_remove_group(&fn->dev.kobj, &rmi_fn_01_attr_group);
> -}
> -
> #ifdef CONFIG_PM_SLEEP
> static int rmi_f01_suspend(struct device *dev)
> {
> struct rmi_function *fn = to_rmi_function(dev);
> struct rmi_device *rmi_dev = fn->rmi_dev;
> struct f01_data *data = fn->data;
> - int error;
> + int error = 0;
>
> data->old_nosleep = data->device_control.ctrl0 &
> - RMI_F01_CRTL0_NOSLEEP_BIT;
> + RMI_F01_CRTL0_NOSLEEP_BIT;
> data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
>
> data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
> data->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
>
> error = rmi_write_block(rmi_dev,
> - fn->fd.control_base_addr,
> - &data->device_control.ctrl0,
> - sizeof(data->device_control.ctrl0));
> + fn->fd.control_base_addr,
> + &data->device_control.ctrl0,
> + sizeof(data->device_control.ctrl0));
> if (error < 0) {
> dev_err(&fn->dev, "Failed to write sleep mode. Code: %d.\n",
> error);
> if (data->old_nosleep)
> - data->device_control.ctrl0 |=
> - RMI_F01_CRTL0_NOSLEEP_BIT;
> + data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
> data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
> data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
> return error;
> @@ -1289,7 +585,7 @@ static int rmi_f01_resume(struct device *dev)
>
> error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
> &data->device_control.ctrl0,
> - sizeof(data->device_control.ctrl0));
> + sizeof(data->device_control.ctrl0));
> if (error < 0) {
> dev_err(&fn->dev,
> "Failed to restore normal operation. Code: %d.\n",
> @@ -1304,7 +600,7 @@ static int rmi_f01_resume(struct device *dev)
> static SIMPLE_DEV_PM_OPS(rmi_f01_pm_ops, rmi_f01_suspend, rmi_f01_resume);
>
> static int rmi_f01_attention(struct rmi_function *fn,
> - unsigned long *irq_bits)
> + unsigned long *irq_bits)
> {
> struct rmi_device *rmi_dev = fn->rmi_dev;
> struct f01_data *data = fn->data;
> @@ -1317,7 +613,6 @@ static int rmi_f01_attention(struct rmi_function *fn,
> retval);
> return retval;
> }
> -
> if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
> dev_warn(&fn->dev, "Device reset detected.\n");
> retval = rmi_dev->driver->reset_handler(rmi_dev);
> @@ -1327,29 +622,18 @@ static int rmi_f01_attention(struct rmi_function *fn,
> return 0;
> }
>
> -static struct rmi_function_handler rmi_f01_handler = {
> +struct rmi_function_driver rmi_f01_driver = {
> .driver = {
> .name = "rmi_f01",
> .pm = &rmi_f01_pm_ops,
> /*
> - * Do not allow user unbinding F01 as it is critical
> + * Do not allow user unbinding of F01 as it is a critical
> * function.
> */
> .suppress_bind_attrs = true,
> },
> - .func = 0x01,
> - .probe = rmi_f01_probe,
> - .remove = rmi_f01_remove,
> - .config = rmi_f01_config,
> - .attention = rmi_f01_attention,
> + .func = FUNCTION_NUMBER,
> + .probe = rmi_f01_probe,
> + .config = rmi_f01_config,
> + .attention = rmi_f01_attention,
> };
> -
> -int __init rmi_register_f01_handler(void)
> -{
> - return rmi_register_function_handler(&rmi_f01_handler);
> -}
> -
> -void rmi_unregister_f01_handler(void)
> -{
> - rmi_unregister_function_handler(&rmi_f01_handler);
> -}
> diff --git a/drivers/input/rmi4/rmi_f01.h b/drivers/input/rmi4/rmi_f01.h
> new file mode 100644
> index 0000000..bfd0dcf
> --- /dev/null
> +++ b/drivers/input/rmi4/rmi_f01.h
> @@ -0,0 +1,269 @@
> +/*
> + * Copyright (c) 2012 Synaptics Incorporated
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +
> +#ifndef _RMI_F01_H
> +#define _RMI_F01_H
> +
> +#define RMI_PRODUCT_ID_LENGTH 10
> +
> +#define RMI_DATE_CODE_LENGTH 3
> +
> +/* Force a firmware reset of the sensor */
> +#define RMI_F01_CMD_DEVICE_RESET 1
> +
> +#define F01_SERIALIZATION_SIZE 7
> +
> +/* Various F01_RMI_QueryX bits */
> +
> +#define RMI_F01_QRY1_CUSTOM_MAP (1 << 0)
> +#define RMI_F01_QRY1_NON_COMPLIANT (1 << 1)
> +#define RMI_F01_QRY1_HAS_LTS (1 << 2)
> +#define RMI_F01_QRY1_HAS_SENSOR_ID (1 << 3)
> +#define RMI_F01_QRY1_HAS_CHARGER_INP (1 << 4)
> +#define RMI_F01_QRY1_HAS_ADJ_DOZE (1 << 5)
> +#define RMI_F01_QRY1_HAS_ADJ_DOZE_HOFF (1 << 6)
> +#define RMI_F01_QRY1_HAS_PROPS_2 (1 << 7)
> +
> +#define RMI_F01_QRY5_YEAR_MASK 0x1f
> +#define RMI_F01_QRY6_MONTH_MASK 0x0f
> +#define RMI_F01_QRY7_DAY_MASK 0x1f
> +
> +#define RMI_F01_QRY2_PRODINFO_MASK 0x7f
> +
> +#define RMI_F01_BASIC_QUERY_LEN 21 /* From Query 00 through 20 */
> +
> +#define RMI_F01_QRY21_SLAVE_ROWS_MASK 0x07
> +#define RMI_F01_QRY21_SLAVE_COLUMNS_MASK 0x38
> +
> +#define RMI_F01_LTS_RESERVED_SIZE 19
> +
> +#define RMI_F01_QRY42_DS4_QUERIES (1 << 0)
> +#define RMI_F01_QRY42_MULTI_PHYS (1 << 1)
> +#define RMI_F01_QRY42_GUEST (1 << 2)
> +#define RMI_F01_QRY42_SWR (1 << 3)
> +#define RMI_F01_QRY42_NOMINAL_REPORT (1 << 4)
> +#define RMI_F01_QRY42_RECAL_INTERVAL (1 << 5)
> +
> +#define RMI_F01_QRY43_01_PACKAGE_ID (1 << 0)
> +#define RMI_F01_QRY43_01_BUILD_ID (1 << 1)
> +#define RMI_F01_QRY43_01_RESET (1 << 2)
> +#define RMI_F01_QRY43_01_MASK_REV (1 << 3)
> +
> +#define RMI_F01_QRY43_02_I2C_CTL (1 << 0)
> +#define RMI_F01_QRY43_02_SPI_CTL (1 << 1)
> +#define RMI_F01_QRY43_02_ATTN_CTL (1 << 2)
> +#define RMI_F01_QRY43_02_WIN8 (1 << 3)
> +#define RMI_F01_QRY43_02_TIMESTAMP (1 << 4)
> +
> +#define RMI_F01_QRY43_03_TOOL_ID (1 << 0)
> +#define RMI_F01_QRY43_03_FW_REVISION (1 << 1)
> +
> +#define RMI_F01_QRY44_RST_ENABLED (1 << 0)
> +#define RMI_F01_QRY44_RST_POLARITY (1 << 1)
> +#define RMI_F01_QRY44_PULLUP_ENABLED (1 << 2)
> +#define RMI_F01_QRY44_RST_PIN_MASK 0xF0
> +
> +#define RMI_TOOL_ID_LENGTH 16
> +#define RMI_FW_REVISION_LENGTH 16
> +
> +struct f01_basic_properties {
> + u8 manufacturer_id;
> + bool has_lts;
> + bool has_sensor_id;
> + bool has_adjustable_doze;
> + bool has_adjustable_doze_holdoff;
> + bool has_query42;
> + char dom[11]; /* YYYY/MM/DD + '\0' */
> + u8 product_id[RMI_PRODUCT_ID_LENGTH + 1];
> + u16 productinfo;
> +
> + /* These are meaningful only if has_lts is true. */
> + u8 slave_asic_rows;
> + u8 slave_asic_columns;
> +
> + /* This is meaningful only if has_sensor_id is true. */
> + u8 sensor_id;
> +
> + /* These are meaningful only if has_query42 is true. */
> + bool has_ds4_queries;
> + bool has_multi_physical;
> + bool has_guest;
> + bool has_swr;
> + bool has_nominal_report_rate;
> + bool has_recalibration_interval;
> +
> + /* Tells how many of the Query43.xx registers are present.
> + */
> + u8 ds4_query_length;
> +
> + /* Query 43.1 */
> + bool has_package_id_query;
> + bool has_build_id_query;
> + bool has_reset_query;
> + bool has_maskrev_query;
> +
> + /* Query 43.2 */
> + bool has_i2c_control;
> + bool has_spi_control;
> + bool has_attn_control;
> + bool has_win8_vendor_info;
> + bool has_timestamp;
> +
> + /* Query 43.3 */
> + bool has_tool_id_query;
> + bool has_fw_revision_query;
> +
> + /* Query 44 */
> + bool reset_enabled;
> + bool reset_polarity;
> + bool pullup_enabled;
> + u8 reset_pin;
> +
> + /* Query 45 */
> + char tool_id[RMI_TOOL_ID_LENGTH + 1];
> +
> + /* Query 46 */
> + char fw_revision[RMI_FW_REVISION_LENGTH + 1];
> +};
> +
> +/** The status code field reports the most recent device status event.
> + * @no_error - should be self explanatory.
> + * @reset_occurred - no other event was seen since the last reset.
> + * @invalid_config - general device configuration has a problem.
> + * @device_failure - general device hardware failure.
> + * @config_crc - configuration failed memory self check.
> + * @firmware_crc - firmware failed memory self check.
> + * @crc_in_progress - bootloader is currently testing config and fw areas.
> + */
> +enum rmi_device_status {
> + no_error = 0x00,
> + reset_occurred = 0x01,
> + invalid_config = 0x02,
> + device_failure = 0x03,
> + config_crc = 0x04,
> + firmware_crc = 0x05,
> + crc_in_progress = 0x06
> +};
> +
> +
> +/* F01 device status bits */
> +
> +/* Most recent device status event */
> +#define RMI_F01_STATUS_CODE(status) ((status) & 0x0f)
> +/* Indicates that flash programming is enabled (bootloader mode). */
> +#define RMI_F01_STATUS_BOOTLOADER(status) (!!((status) & 0x40))
> +/* The device has lost its configuration for some reason. */
> +#define RMI_F01_STATUS_UNCONFIGURED(status) (!!((status) & 0x80))
> +
> +
> +
> +/* Control register bits */
> +
> +/*
> +* Sleep mode controls power management on the device and affects all
> +* functions of the device.
> +*/
> +#define RMI_F01_CTRL0_SLEEP_MODE_MASK 0x03
> +
> +#define RMI_SLEEP_MODE_NORMAL 0x00
> +#define RMI_SLEEP_MODE_SENSOR_SLEEP 0x01
> +#define RMI_SLEEP_MODE_RESERVED0 0x02
> +#define RMI_SLEEP_MODE_RESERVED1 0x03
> +
> +#define RMI_IS_VALID_SLEEPMODE(mode) \
> +(mode >= RMI_SLEEP_MODE_NORMAL && mode <= RMI_SLEEP_MODE_RESERVED1)
> +
> +/*
> + * This bit disables whatever sleep mode may be selected by the sleep_mode
> + * field and forces the device to run at full power without sleeping.
> + */
> +#define RMI_F01_CRTL0_NOSLEEP_BIT (1 << 2)
> +
> +/*
> + * When this bit is set, the touch controller employs a noise-filtering
> + * algorithm designed for use with a connected battery charger.
> + */
> +#define RMI_F01_CRTL0_CHARGER_BIT (1 << 5)
> +
> +/*
> + * Sets the report rate for the device. The effect of this setting is
> + * highly product dependent. Check the spec sheet for your particular
> + * touch sensor.
> + */
> +#define RMI_F01_CRTL0_REPORTRATE_BIT (1 << 6)
> +
> +/*
> + * Written by the host as an indicator that the device has been
> + * successfully configured.
> + */
> +#define RMI_F01_CRTL0_CONFIGURED_BIT (1 << 7)
> +
> +/**
> + * @ctrl0 - see documentation in rmi_f01.h.
> + * @interrupt_enable - A mask of per-function interrupts on the touch sensor.
> + * @doze_interval - controls the interval between checks for finger presence
> + * when the touch sensor is in doze mode, in units of 10ms.
> + * @wakeup_threshold - controls the capacitance threshold at which the touch
> + * sensor will decide to wake up from that low power state.
> + * @doze_holdoff - controls how long the touch sensor waits after the last
> + * finger lifts before entering the doze state, in units of 100ms.
> + */
> +struct f01_device_control {
> + u8 ctrl0;
> + u8 *interrupt_enable;
> + u8 doze_interval;
> + u8 wakeup_threshold;
> + u8 doze_holdoff;
> +};
> +
> +
> +/*
> + *
> + * @serialization - 7 bytes of device serialization data. The meaning of
> + * these bytes varies from product to product, consult your product spec sheet.
> + */
> +struct f01_data {
> + struct f01_device_control device_control;
> + struct mutex control_mutex;
> +
> + u8 device_status;
> +
> + struct f01_basic_properties properties;
> + u8 serialization[F01_SERIALIZATION_SIZE];
> + u8 product_id[RMI_PRODUCT_ID_LENGTH+1];
> +
> + u16 package_id;
> + u16 package_rev;
> + u32 build_id;
> +
> + u16 interrupt_enable_addr;
> + u16 doze_interval_addr;
> + u16 wakeup_threshold_addr;
> + u16 doze_holdoff_addr;
> +
> + int irq_count;
> + int num_of_irq_regs;
> +
> +#ifdef CONFIG_PM
I think you want CONFIG_PM_SLEEP here to mirror implementation of
susped/resume methods.
> + bool suspended;
> + bool old_nosleep;
> +#endif
> +};
> +
> +#endif
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH 12/25] HID: sony: fix error return code
From: Julia Lawall @ 2013-12-29 22:47 UTC (permalink / raw)
To: Jiri Kosina; +Cc: kernel-janitors, linux-input, linux-kernel
In-Reply-To: <1388357260-4843-1-git-send-email-Julia.Lawall@lip6.fr>
From: Julia Lawall <Julia.Lawall@lip6.fr>
Currently the return variable ret is always 0. Set it to other values in
error cases, as used in the direct return.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
Not tested.
drivers/hid/hid-sony.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index b60bc38..f57ab5e 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -585,6 +585,7 @@ static int sony_leds_init(struct hid_device *hdev)
led = kzalloc(sizeof(struct led_classdev) + name_sz, GFP_KERNEL);
if (!led) {
hid_err(hdev, "Couldn't allocate memory for LED %d\n", n);
+ ret = -ENOMEM;
goto error_leds;
}
@@ -596,7 +597,8 @@ static int sony_leds_init(struct hid_device *hdev)
led->brightness_get = sony_led_get_brightness;
led->brightness_set = sony_led_set_brightness;
- if (led_classdev_register(&hdev->dev, led)) {
+ ret = led_classdev_register(&hdev->dev, led);
+ if (ret) {
hid_err(hdev, "Failed to register LED %d\n", n);
kfree(led);
goto error_leds;
^ permalink raw reply related
* [PATCH 0/25] fix error return code
From: Julia Lawall @ 2013-12-29 22:47 UTC (permalink / raw)
To: linux-input-u79uwXL29TY76Z2rM5mHXA
Cc: cbe-oss-dev-uLR06cmDAlY/bJ5BZ2RsiQ,
davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
linux-pcmcia-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
rtc-linux-/JYPxA39Uh5TLH3MbocFFw,
linux-scsi-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-media-u79uwXL29TY76Z2rM5mHXA
These patches fix cases where the return variable is not set to an error
code in an error case.
^ permalink raw reply
* Re: [PATCH V4] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
From: Dmitry Torokhov @ 2013-12-28 2:36 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <1388194004-32253-1-git-send-email-cheiny@synaptics.com>
On Fri, Dec 27, 2013 at 05:26:44PM -0800, Christopher Heiny wrote:
> This patch fixes some bugs in handling of the RMI4 attention line GPIO.
>
> 1) in enable_sensor(), eliminate the complicated check on ATTN and just
> call process_interrupt_requests(). This will have minimal overhead if ATTN
> is not asserted, and clears the state of the RMI4 device in any case.
>
> 2) Correctly free the GPIO in rmi_driver_remove().
>
> 3) in rmi_driver_probe()
> - declare the name of the attention gpio (GPIO_LABEL)
> - use gpio_request_one() to get the gpio and export it.
> - simplify (somewhat) conditional gpio acquisition logic and combine
> with interrupt setup
>
> 4) use gpio_is_valid() instead of comparing to 0.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
drivers/input/rmi4/rmi_driver.c: In function ‘rmi_driver_probe’:
drivers/input/rmi4/rmi_driver.c:920:8: error: ‘struct rmi_driver_data’
has no member named ‘gpio_held’
data->gpio_held = true;
You forgot to include header file changes...
>
> ---
>
> drivers/input/rmi4/rmi_driver.c | 64 ++++++++++++++++++++++++-----------------
> 1 file changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 2ae9af9..766954f 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -140,7 +140,6 @@ static int enable_sensor(struct rmi_device *rmi_dev)
> struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> struct rmi_transport_dev *xport;
> int retval = 0;
> - struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
>
> if (data->enabled)
> return 0;
> @@ -169,11 +168,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>
> data->enabled = true;
>
> - if (!pdata->level_triggered &&
> - gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
> - retval = process_interrupt_requests(rmi_dev);
> -
> - return retval;
> + return process_interrupt_requests(rmi_dev);
> }
>
> static void rmi_free_function_list(struct rmi_device *rmi_dev)
> @@ -800,13 +795,21 @@ 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 (gpio_is_valid(pdata->attn_gpio) && data->gpio_held)
> + gpio_free(pdata->attn_gpio);
You only need to check data->gpio_held here...
> +
> return 0;
> }
>
> +static const char GPIO_LABEL[] = "attn";
> +
While you are updating paatch could you move it in rmi_driver_probe()
under if (gpio_is_valid()).
By the way, maybe we should have platform supply gpio name or, in its
absence, generate unique gpio name, so that we could have several RMI
devices be present in a box?
That can be a followup patch at a later time.
> static int rmi_driver_probe(struct device *dev)
> {
> struct rmi_driver *rmi_driver;
> @@ -937,7 +940,9 @@ static int rmi_driver_probe(struct device *dev)
> mutex_init(&data->suspend_mutex);
> }
>
> - if (pdata->attn_gpio) {
> + if (gpio_is_valid(pdata->attn_gpio)) {
> + ulong gpio_flags = GPIOF_DIR_IN;
> +
> data->irq = gpio_to_irq(pdata->attn_gpio);
> if (pdata->level_triggered) {
> data->irq_flags = IRQF_ONESHOT |
> @@ -948,6 +953,32 @@ static int rmi_driver_probe(struct device *dev)
> (pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH)
> ? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
> }
> +
> + 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
} else {
> data->poll_interval = ktime_set(0,
> (pdata->poll_interval_ms ? pdata->poll_interval_ms :
Another thing I was wondering - polling support is really unusable for
device in production (battery gets killed) so maybe it should be removed
altogether?
> @@ -958,25 +989,6 @@ static int rmi_driver_probe(struct device *dev)
> enable_sensor(rmi_dev);
> }
>
> - if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
> - retval = gpio_export(pdata->attn_gpio, false);
> - if (retval) {
> - dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
> - retval = 0;
> - } else {
> - retval = gpio_export_link(dev,
> - "attn", 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);
> - }
> - }
> - }
> -
> return 0;
>
> err_free_data:
Thanks!
--
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] input synaptics-rmi4: Delete some obsolete code
From: Dmitry Torokhov @ 2013-12-28 2:24 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <1388195887-26503-1-git-send-email-cheiny@synaptics.com>
On Fri, Dec 27, 2013 at 05:58:07PM -0800, Christopher Heiny wrote:
> The answer to the question "is this crap needed with F01 always present?" is
> "no, it's not". Also delete obsolete [suspend|resume]_one_device.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Applied, thank you.
>
> ---
>
> drivers/input/rmi4/rmi_driver.c | 48 -----------------------------------------
> 1 file changed, 48 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 2ae9af9..a4e5236 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -686,39 +686,6 @@ error_exit:
> return retval;
> }
>
> -#if 0
> -// XXX is this crap needed with F01 always present?
> -static int f01_notifier_call(struct notifier_block *nb,
> - unsigned long action, void *data)
> -{
> - struct device *dev = data;
> - struct rmi_function *fn;
> -
> - if (!rmi_is_function_device(dev))
> - return 0;
> -
> - fn = to_rmi_function(dev);
> - if (fn->fd.function_number != 0x01)
> - return 0;
> -
> - switch (action) {
> - case BUS_NOTIFY_BOUND_DRIVER:
> - dev_dbg(dev, "%s: F01 driver bound.\n", __func__);
> - enable_sensor(fn->rmi_dev);
> - break;
> - case BUS_NOTIFY_UNBIND_DRIVER:
> - dev_dbg(dev, "%s: F01 driver going away.\n", __func__);
> - disable_sensor(fn->rmi_dev);
> - break;
> - }
> - return 0;
> -}
> -
> -static struct notifier_block rmi_bus_notifier = {
> - .notifier_call = f01_notifier_call,
> -};
> -#endif
> -
> #ifdef CONFIG_PM_SLEEP
> static int rmi_driver_suspend(struct device *dev)
> {
> @@ -738,13 +705,6 @@ static int rmi_driver_suspend(struct device *dev)
>
> disable_sensor(rmi_dev);
>
> -#if 0
> - /** Do it backwards so F01 comes last. */
> - list_for_each_entry_reverse(entry, &data->function_list, node)
> - if (suspend_one_device(entry) < 0)
> - goto exit;
> -#endif
> -
> if (data->post_suspend)
> retval = data->post_suspend(data->pm_data);
>
> @@ -768,14 +728,6 @@ static int rmi_driver_resume(struct device *dev)
> goto exit;
> }
>
> -#if 0
> - /** Do it forwards, so F01 comes first. */
> - list_for_each_entry(entry, &data->function_list, node) {
> - if (resume_one_device(entry) < 0)
> - goto exit;
> - }
> -#endif
> -
> retval = enable_sensor(rmi_dev);
> if (retval)
> goto exit;
--
Dmitry
^ permalink raw reply
* [PATCH] input synaptics-rmi4: Delete some obsolete code
From: Christopher Heiny @ 2013-12-28 1:58 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires
The answer to the question "is this crap needed with F01 always present?" is
"no, it's not". Also delete obsolete [suspend|resume]_one_device.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/rmi4/rmi_driver.c | 48 -----------------------------------------
1 file changed, 48 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 2ae9af9..a4e5236 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -686,39 +686,6 @@ error_exit:
return retval;
}
-#if 0
-// XXX is this crap needed with F01 always present?
-static int f01_notifier_call(struct notifier_block *nb,
- unsigned long action, void *data)
-{
- struct device *dev = data;
- struct rmi_function *fn;
-
- if (!rmi_is_function_device(dev))
- return 0;
-
- fn = to_rmi_function(dev);
- if (fn->fd.function_number != 0x01)
- return 0;
-
- switch (action) {
- case BUS_NOTIFY_BOUND_DRIVER:
- dev_dbg(dev, "%s: F01 driver bound.\n", __func__);
- enable_sensor(fn->rmi_dev);
- break;
- case BUS_NOTIFY_UNBIND_DRIVER:
- dev_dbg(dev, "%s: F01 driver going away.\n", __func__);
- disable_sensor(fn->rmi_dev);
- break;
- }
- return 0;
-}
-
-static struct notifier_block rmi_bus_notifier = {
- .notifier_call = f01_notifier_call,
-};
-#endif
-
#ifdef CONFIG_PM_SLEEP
static int rmi_driver_suspend(struct device *dev)
{
@@ -738,13 +705,6 @@ static int rmi_driver_suspend(struct device *dev)
disable_sensor(rmi_dev);
-#if 0
- /** Do it backwards so F01 comes last. */
- list_for_each_entry_reverse(entry, &data->function_list, node)
- if (suspend_one_device(entry) < 0)
- goto exit;
-#endif
-
if (data->post_suspend)
retval = data->post_suspend(data->pm_data);
@@ -768,14 +728,6 @@ static int rmi_driver_resume(struct device *dev)
goto exit;
}
-#if 0
- /** Do it forwards, so F01 comes first. */
- list_for_each_entry(entry, &data->function_list, node) {
- if (resume_one_device(entry) < 0)
- goto exit;
- }
-#endif
-
retval = enable_sensor(rmi_dev);
if (retval)
goto exit;
^ permalink raw reply related
* [PATCH V4] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
From: Christopher Heiny @ 2013-12-28 1:26 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires
This patch fixes some bugs in handling of the RMI4 attention line GPIO.
1) in enable_sensor(), eliminate the complicated check on ATTN and just
call process_interrupt_requests(). This will have minimal overhead if ATTN
is not asserted, and clears the state of the RMI4 device in any case.
2) Correctly free the GPIO in rmi_driver_remove().
3) in rmi_driver_probe()
- declare the name of the attention gpio (GPIO_LABEL)
- use gpio_request_one() to get the gpio and export it.
- simplify (somewhat) conditional gpio acquisition logic and combine
with interrupt setup
4) use gpio_is_valid() instead of comparing to 0.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/rmi4/rmi_driver.c | 64 ++++++++++++++++++++++++-----------------
1 file changed, 38 insertions(+), 26 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 2ae9af9..766954f 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -140,7 +140,6 @@ static int enable_sensor(struct rmi_device *rmi_dev)
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
struct rmi_transport_dev *xport;
int retval = 0;
- struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
if (data->enabled)
return 0;
@@ -169,11 +168,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
data->enabled = true;
- if (!pdata->level_triggered &&
- gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
- retval = process_interrupt_requests(rmi_dev);
-
- return retval;
+ return process_interrupt_requests(rmi_dev);
}
static void rmi_free_function_list(struct rmi_device *rmi_dev)
@@ -800,13 +795,21 @@ 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 (gpio_is_valid(pdata->attn_gpio) && data->gpio_held)
+ gpio_free(pdata->attn_gpio);
+
return 0;
}
+static const char GPIO_LABEL[] = "attn";
+
static int rmi_driver_probe(struct device *dev)
{
struct rmi_driver *rmi_driver;
@@ -937,7 +940,9 @@ static int rmi_driver_probe(struct device *dev)
mutex_init(&data->suspend_mutex);
}
- if (pdata->attn_gpio) {
+ if (gpio_is_valid(pdata->attn_gpio)) {
+ ulong gpio_flags = GPIOF_DIR_IN;
+
data->irq = gpio_to_irq(pdata->attn_gpio);
if (pdata->level_triggered) {
data->irq_flags = IRQF_ONESHOT |
@@ -948,6 +953,32 @@ static int rmi_driver_probe(struct device *dev)
(pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH)
? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
}
+
+ 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 :
@@ -958,25 +989,6 @@ static int rmi_driver_probe(struct device *dev)
enable_sensor(rmi_dev);
}
- if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
- retval = gpio_export(pdata->attn_gpio, false);
- if (retval) {
- dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
- retval = 0;
- } else {
- retval = gpio_export_link(dev,
- "attn", 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);
- }
- }
- }
-
return 0;
err_free_data:
^ permalink raw reply related
* Re: [PATCH 2/2] Input: keypad-omap - Cleanup header file
From: Dmitry Torokhov @ 2013-12-28 1:15 UTC (permalink / raw)
To: Sachin Kamat; +Cc: linux-input, patches
In-Reply-To: <1388145788-6253-2-git-send-email-sachin.kamat@linaro.org>
On Fri, Dec 27, 2013 at 05:33:08PM +0530, Sachin Kamat wrote:
> Commit 2203747c9771 ("ARM: omap: move platform_data definitions")
> moved the file to the current location but forgot to remove the pointer
> to its previous location. Clean it up. While at it also change the header
> file protection macros appropriately.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
Applied both, thanks Sachin.
> ---
> include/linux/platform_data/keypad-omap.h | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/platform_data/keypad-omap.h b/include/linux/platform_data/keypad-omap.h
> index a6b21eddb212..c3a3abae98f0 100644
> --- a/include/linux/platform_data/keypad-omap.h
> +++ b/include/linux/platform_data/keypad-omap.h
> @@ -1,14 +1,12 @@
> /*
> - * arch/arm/plat-omap/include/mach/keypad.h
> - *
> * Copyright (C) 2006 Komal Shah <komal_shah802003@yahoo.com>
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> * published by the Free Software Foundation.
> */
> -#ifndef ASMARM_ARCH_KEYPAD_H
> -#define ASMARM_ARCH_KEYPAD_H
> +#ifndef __KEYPAD_OMAP_H
> +#define __KEYPAD_OMAP_H
>
> #ifndef CONFIG_ARCH_OMAP1
> #warning Please update the board to use matrix-keypad driver
> --
> 1.7.9.5
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 2/5] input: sun4i-ts: Add support for temperature sensor
From: Dmitry Torokhov @ 2013-12-28 1:13 UTC (permalink / raw)
To: Hans de Goede
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA, LM Sensors, Maxime Ripard,
Corentin LABBE, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <1388156399-29677-3-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Fri, Dec 27, 2013 at 03:59:56PM +0100, Hans de Goede wrote:
>
> +Optional properties:
> + - ts-attached: boolean set this to tell the driver that an actual touchscreen
> + is attached and that it should register an input device,
> + without this it only registers the builtin temperate sensor
I believe the guidance is to avoid describing kernel behavior in DT
property so I think it should say:
- ts-attached: boolean indicating that an actual touchscreen is
attached to the controller.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 2/5] input: sun4i-ts: Add support for temperature sensor
From: Dmitry Torokhov @ 2013-12-28 1:10 UTC (permalink / raw)
To: Hans de Goede
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA, LM Sensors, Maxime Ripard,
Corentin LABBE, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <1388156399-29677-3-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Hi Hans,
On Fri, Dec 27, 2013 at 03:59:56PM +0100, Hans de Goede wrote:
> +
> + if (!ts->input)
> + goto leave;
> +
> if (reg_val & FIFO_DATA_PENDING) {
> x = readl(ts->base + TP_DATA);
> y = readl(ts->base + TP_DATA);
> @@ -140,6 +151,7 @@ static irqreturn_t sun4i_ts_irq(int irq, void *dev_id)
> input_sync(ts->input);
> }
>
> +leave:
Can we please not introduce gotos where they are not needed? If you
concerned about extra indent just split touchscreen handling into a
separate function and then do
if (ts->input)
sun4i_ts_handle_touchscreen_data(...);
> writel(reg_val, ts->base + TP_INT_FIFOS);
>
> return IRQ_HANDLED;
> @@ -149,9 +161,9 @@ static int sun4i_ts_open(struct input_dev *dev)
> {
> struct sun4i_ts_data *ts = input_get_drvdata(dev);
>
> - /* Flush, set trig level to 1, enable data and up irqs */
> - writel(DATA_IRQ_EN(1) | FIFO_TRIG(1) |FIFO_FLUSH(1) | TP_UP_IRQ_EN(1),
> - ts->base + TP_INT_FIFOC);
> + /* Flush, set trig level to 1, enable temp, data and up irqs */
> + writel(TEMP_IRQ_EN(1) | DATA_IRQ_EN(1) | FIFO_TRIG(1) | FIFO_FLUSH(1) |
> + TP_UP_IRQ_EN(1), ts->base + TP_INT_FIFOC);
>
> return 0;
> }
> @@ -160,40 +172,76 @@ static void sun4i_ts_close(struct input_dev *dev)
> {
> struct sun4i_ts_data *ts = input_get_drvdata(dev);
>
> - /* Deactivate all IRQs */
> - writel(0, ts->base + TP_INT_FIFOC);
> + /* Deactivate all input IRQs */
> + writel(TEMP_IRQ_EN(1), ts->base + TP_INT_FIFOC);
> synchronize_irq(ts->irq);
Hmm, it would be nice we touchscreen methods only affected touchscreen
functionality. Can you read current state and adjust it as needed
instead of clobbering it? Then you could do away with remove() method
altogether.
> }
>
> +static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
> + char *buf)
> +{
> + struct sun4i_ts_data *ts = dev_get_drvdata(dev);
> +
> + /* No temp_data until the first irq */
> + if (ts->temp_data == -1)
> + return -EAGAIN;
> +
> + return sprintf(buf, "%d\n", (ts->temp_data - 1447) * 100);
> +}
> +
> +static ssize_t show_temp_label(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + return sprintf(buf, "SoC temperature\n");
> +}
> +
> +static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL);
> +static DEVICE_ATTR(temp1_label, S_IRUGO, show_temp_label, NULL);
> +
> +static struct attribute *sun4i_ts_attrs[] = {
> + &dev_attr_temp1_input.attr,
> + &dev_attr_temp1_label.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(sun4i_ts);
> +
> static int sun4i_ts_probe(struct platform_device *pdev)
> {
> struct sun4i_ts_data *ts;
> struct device *dev = &pdev->dev;
> + struct device_node *np =dev->of_node;
> + struct device *hwmon;
> int ret;
> + bool ts_attached;
> +
> + ts_attached = of_property_read_bool(np, "ts-attached");
>
> ts = devm_kzalloc(dev, sizeof(struct sun4i_ts_data), GFP_KERNEL);
> if (!ts)
> return -ENOMEM;
>
> ts->dev = dev;
> -
> - ts->input = devm_input_allocate_device(dev);
> - if (!ts->input)
> - return -ENOMEM;
> -
> - ts->input->name = pdev->name;
> - ts->input->phys = "sun4i_ts/input0";
> - ts->input->open = sun4i_ts_open;
> - ts->input->close = sun4i_ts_close;
> - ts->input->id.bustype = BUS_HOST;
> - ts->input->id.vendor = 0x0001;
> - ts->input->id.product = 0x0001;
> - ts->input->id.version = 0x0100;
> - ts->input->evbit[0] = BIT(EV_SYN) | BIT(EV_KEY) | BIT(EV_ABS);
> - set_bit(BTN_TOUCH, ts->input->keybit);
> - input_set_abs_params(ts->input, ABS_X, 0, 4095, 0, 0);
> - input_set_abs_params(ts->input, ABS_Y, 0, 4095, 0, 0);
> - input_set_drvdata(ts->input, ts);
> + ts->temp_data = -1;
> +
> + if (ts_attached) {
> + ts->input = devm_input_allocate_device(dev);
> + if (!ts->input)
> + return -ENOMEM;
> +
> + ts->input->name = pdev->name;
> + ts->input->phys = "sun4i_ts/input0";
> + ts->input->open = sun4i_ts_open;
> + ts->input->close = sun4i_ts_close;
> + ts->input->id.bustype = BUS_HOST;
> + ts->input->id.vendor = 0x0001;
> + ts->input->id.product = 0x0001;
> + ts->input->id.version = 0x0100;
> + ts->input->evbit[0] = BIT(EV_SYN) | BIT(EV_KEY) | BIT(EV_ABS);
> + set_bit(BTN_TOUCH, ts->input->keybit);
> + input_set_abs_params(ts->input, ABS_X, 0, 4095, 0, 0);
> + input_set_abs_params(ts->input, ABS_Y, 0, 4095, 0, 0);
> + input_set_drvdata(ts->input, ts);
> + }
>
> ts->base = devm_ioremap_resource(dev,
> platform_get_resource(pdev, IORESOURCE_MEM, 0));
> @@ -232,14 +280,42 @@ static int sun4i_ts_probe(struct platform_device *pdev)
> writel(STYLUS_UP_DEBOUN(5) | STYLUS_UP_DEBOUN_EN(1) | TP_MODE_EN(1),
> ts->base + TP_CTRL1);
>
> - ret = input_register_device(ts->input);
> - if (ret)
> - return ret;
> + hwmon = devm_hwmon_device_register_with_groups(ts->dev, "sun4i_ts",
> + ts, sun4i_ts_groups);
> + if (IS_ERR(hwmon)) {
> + return PTR_ERR(hwmon);
> + }
> +
> + writel(TEMP_IRQ_EN(1), ts->base + TP_INT_FIFOC);
> +
> + if (ts_attached) {
> + ret = input_register_device(ts->input);
> + if (ret) {
> + writel(0, ts->base + TP_INT_FIFOC);
> + synchronize_irq(ts->irq);
Given that we now can't avoid freeing irq before freeing ts memory or
input device I think you can do away with synchronize_irq() here and
elsewhere as freeing irq is guaranteed to wait for current interrupt to
complete.
> + return ret;
> + }
> + }
>
> platform_set_drvdata(pdev, ts);
> return 0;
> }
>
> +static int sun4i_ts_remove(struct platform_device *pdev)
> +{
> + struct sun4i_ts_data *ts = platform_get_drvdata(pdev);
> +
> + /* Explicit unregister to avoid open/close changing the imask later */
> + if (ts->input)
> + input_unregister_device(ts->input);
> +
> + /* Deactivate all IRQs */
> + writel(0, ts->base + TP_INT_FIFOC);
> + synchronize_irq(ts->irq);
> +
> + return 0;
> +}
> +
> static const struct of_device_id sun4i_ts_of_match[] = {
> { .compatible = "allwinner,sun4i-ts", },
> { /* sentinel */ }
> @@ -253,6 +329,7 @@ static struct platform_driver sun4i_ts_driver = {
> .of_match_table = of_match_ptr(sun4i_ts_of_match),
> },
> .probe = sun4i_ts_probe,
> + .remove = sun4i_ts_remove,
> };
>
> module_platform_driver(sun4i_ts_driver);
> --
> 1.8.4.2
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
From: Christopher Heiny @ 2013-12-27 23:43 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <20131226223718.GC18562@core.coreip.homeip.net>
On 12/26/2013 02:37 PM, Dmitry Torokhov wrote:
> Hi Chris,
>
> On Mon, Dec 23, 2013 at 06:44:19PM -0800, Christopher Heiny wrote:
>> This patch fixes some bugs in handling of the RMI4 attention line GPIO.
>>
>> 1) in enable_sensor(), eliminate the complicated check on ATTN and just
>> call process_interrupt_requests(). This will have minimal overhead if ATTN
>> is not asserted, and clears the state of the RMI4 device in any case.
>>
>> 2) Correctly free the GPIO in rmi_driver_remove().
>>
>> 3) in rmi_driver_probe()
>> - declare the name of the attention gpio (GPIO_LABEL)
>> - use gpio_request_one() to get the gpio and export it.
>> - simplify conditional gpio acquisition logic and combine with interrupt
>> setup
>>
>> 4) use gpio_is_valid() instead of comparing to 0.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_driver.c | 43 ++++++++++++++++++++++-------------------
>> 1 file changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>> index a30c7d3..9b02358 100644
>> --- a/drivers/input/rmi4/rmi_driver.c
>> +++ b/drivers/input/rmi4/rmi_driver.c
>> @@ -140,7 +140,6 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>> struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
>> struct rmi_phys_device *rmi_phys;
>> int retval = 0;
>> - struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
>>
>> if (data->enabled)
>> return 0;
>> @@ -169,11 +168,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>>
>> data->enabled = true;
>>
>> - if (!pdata->level_triggered &&
>> - gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
>> - retval = process_interrupt_requests(rmi_dev);
>> -
>> - return retval;
>> + return process_interrupt_requests(rmi_dev);
>> }
>>
>> static void rmi_free_function_list(struct rmi_device *rmi_dev)
>> @@ -800,13 +795,20 @@ 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);
>>
>> disable_sensor(rmi_dev);
>> rmi_free_function_list(rmi_dev);
>>
>> + if (gpio_is_valid(pdata->attn_gpio))
>> + gpio_free(pdata->attn_gpio);
>
> It looks like you let driver registration to continue even if GPIO
> request fails. You probably need to introduce a flag indicating whether
> you successfully requested gpio or not. Or you can treat failure to
> acquire gpio as fatal.
In testing, the driver continues to work even if the GPIO is not
acquired, but some diagnostic features may not be available, so we treat
the failure to acquire as a warning. I'll add a flag to indicate
whether it was acquired or not.
>
>> +
>> return 0;
>> }
>>
>> +static const char GPIO_LABEL[] = "attn";
>> +
>> static int rmi_driver_probe(struct device *dev)
>> {
>> struct rmi_driver *rmi_driver;
>> @@ -937,7 +939,7 @@ static int rmi_driver_probe(struct device *dev)
>> mutex_init(&data->suspend_mutex);
>> }
>>
>> - if (pdata->attn_gpio) {
>> + if (gpio_is_valid(pdata->attn_gpio)) {
>> data->irq = gpio_to_irq(pdata->attn_gpio);
>> if (pdata->level_triggered) {
>> data->irq_flags = IRQF_ONESHOT |
>> @@ -948,24 +950,17 @@ static int rmi_driver_probe(struct device *dev)
>> (pdata->attn_polarity == RMI_ATTN_ACTIVE_HIGH)
>> ? IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
>> }
>> - } 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) {
>> - /* Driver already bound, so enable ATTN now. */
>> - enable_sensor(rmi_dev);
>> - }
>>
>> - if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
>
> Do you really need to export gpio if you do not have RMI4_DEV option?
Not really, but it doesn't hurt to do so. I was trying to simplify the
driver logic, but I guess that went too far. I'll re-add the check.
[snip]
^ 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