* Qualcomm PMIC8XXX keypad never worked?
From: Arnd Bergmann @ 2014-03-11 8:49 UTC (permalink / raw)
To: Stephen Boyd
Cc: Trilok Soni, Samuel Ortiz, Dmitry Torokhov, linux-kernel,
linux-arm-kernel, linux-input, Lee Jones, Anirudh Ghayal
Something in the MFD_PM8XXX driver must have changed recently that now allows
the KEYBOARD_PMIC8XXX driver to be enabled. Unfortunately, it does not build
because of a dependency on the <linux/mfd/pm8xxx/gpio.h> header file that
does not exist.
In order to keep 'make randconfig' working on my test box, I have disabled
this driver, but it would be nice of someone could have a look into fixing it.
Arnd
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -470,7 +470,7 @@ config KEYBOARD_PXA930_ROTARY
config KEYBOARD_PMIC8XXX
tristate "Qualcomm PMIC8XXX keypad support"
- depends on MFD_PM8XXX
+ depends on MFD_PM8XXX && BROKEN
select INPUT_MATRIXKMAP
help
Say Y here if you want to enable the driver for the PMIC8XXX
^ permalink raw reply
* Re: [PATCH V2] input: misc: da9063: OnKey driver
From: Lee Jones @ 2014-03-11 7:20 UTC (permalink / raw)
To: Opensource [Steve Twiss]
Cc: Dmitry Torokhov, Samuel Ortiz, David Dajun Chen, LKML-INPUT,
LKML-KERNEL, Mark Brown, Philipp Zabel
In-Reply-To: <201403101740.s2AHebta017785@swsrvapps-01.diasemi.com>
> From: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
>
> Add the OnKey driver for DA9063
>
> Plus some minor dependencies:
> - Addition of "ONKEY" name to OnKey IRQ resource structure;
> - Bool key_power platform data driver configuration option.
>
> Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
> ---
>
> drivers/input/misc/Kconfig | 10 ++
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/da9063-onkey.c | 209 +++++++++++++++++++++++++++++++++++++
> drivers/mfd/da9063-core.c | 1 +
> include/linux/mfd/da9063/pdata.h | 1 +
For the MFD parts:
Acked-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.
From: Christopher Heiny @ 2014-03-11 2:36 UTC (permalink / raw)
To: Courtney Cavin
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <20140310225750.GG18578@sonymobile.com>
On 03/10/2014 03:57 PM, Courtney Cavin wrote:
> On Mon, Mar 10, 2014 at 11:45:50PM +0100, Courtney Cavin wrote:
>> On Mon, Mar 10, 2014 at 11:33:06PM +0100, Christopher Heiny wrote:
>>> On 03/10/2014 07:46 AM, Courtney Cavin wrote:
>>>> On Sat, Mar 08, 2014 at 03:29:51AM +0100, Christopher Heiny wrote:
>>>>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>>>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>>>> Cc: Linux Walleij <linus.walleij@linaro.org>
>>>>> Cc: David Herrmann <dh.herrmann@gmail.com>
>>>>> Cc: Jiri Kosina <jkosina@suse.cz>
>>>>>
>>>>> ---
>>>>>
>>>>> drivers/input/rmi4/rmi_f01.c | 96 ++-----------------------------------
>>>>> drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
>>>>> 2 files changed, 114 insertions(+), 92 deletions(-)
>> [...]
>>>>
>>>> I might be missing something, but these seem like the only defines used
>>>> in the flash code. Why not keep these in the f01 driver, and export
>>>> a couple more functions, like rmi_f01_reset() and rmi_f01_set_sleep_mode() ?
>>>
>>> It seems better to me to have the information defined in a single place,
>>> rather than scattered hither and yon through the source files.
>>
>> Uh. Exactly? This is why I'm suggesting that you keep this information
>> isolated in the driver to which is directly related.
>>
>> Perhaps what you mean is that the regs/bits for the entire chip
>> functionality should be exposed in header files, so one can read/write
>> it from anywhere? That seems backwards to the idea of separating these
>> 'functions' out into drivers.
>
> Ah. Wait. I think there was some mis-communication on my part. What I
> should have said: Why not keep all of the defines in the driver, and
> export a couple more functions?
>
> My point is exactly yours. Keep the defines with the code. Expose
> what's needed.
I don't see any win to that approach. For example, you can hide the
sleep mode mask and the nosleep bit #defines in rmi_f01.c, but you've
got to add a new function with either a tri-state field for the nosleep
bit (set it, clear it, don't change it) which will require an additional
3 #defines and the new sleep mode which still needs the sleep mode
#defines (plus an additional one to indicate that you want to leave the
sleep mode as it was), or 3 new functions, once of which changes the
sleep mode, one of which sets/clears the nosleep bit, and one of which
does both. In either case, you still need the sleep mode #defines, so
some of the related stuff is in the header and some of it is in the .c
file. Now you wind up with more stuff in the .h file (at least twice as
much as you've removed) and you no longer have one stop shopping for the
F01_Ctrl0 #defines.
BTW - thanks very much for all of today's input. Even though we don't
necessarily agree with all of it, it's been helpful to go back and ask
"did we make the right decision?".
Chris
^ permalink raw reply
* Re: [PATCH 05/05] input synaptics-rmi4: Add reflash support
From: Christopher Heiny @ 2014-03-11 1:03 UTC (permalink / raw)
To: Courtney Cavin
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <20140310162440.GE18578@sonymobile.com>
On 03/10/2014 09:24 AM, Courtney Cavin wrote:
> On Sat, Mar 08, 2014 at 03:29:55AM +0100, Christopher Heiny wrote:
>> Add support for reflashing V5 bootloader firmwares into
>> RMI4 devices.
>
> Throughout this driver: I'm not sure of the name 'reflash'. Maybe just
> 'flash(ing)'?
That's a good point. We use the terms 'flash', 'reflash', and 'update'
pretty much interchangeably internally, and didn't realize it might
cause confusion. Using 'fw update' and similar terms would probably
eliminate that.
>
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>>
>> ---
>>
>> drivers/input/rmi4/Kconfig | 9 +
>> drivers/input/rmi4/Makefile | 1 +
>> drivers/input/rmi4/rmi_bus.c | 3 +
>> drivers/input/rmi4/rmi_driver.h | 11 +
>> drivers/input/rmi4/rmi_fw_update.c | 961 +++++++++++++++++++++++++++++++++++++
>> 5 files changed, 985 insertions(+)
>>
>> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
>> index d0c7b6e..9b88b6a 100644
>> --- a/drivers/input/rmi4/Kconfig
>> +++ b/drivers/input/rmi4/Kconfig
>> @@ -25,6 +25,15 @@ config RMI4_DEBUG
>>
>> If unsure, say N.
>>
>> +config RMI4_FWLIB
>
> Err. LIB?
It's an evolutionary vestige - originally this was a library module.
I'll change it as part of the change mentioned above.
>
>> + bool "RMI4 Firmware Update"
>> + depends on RMI4_CORE
>> + help
>> + Say Y here to enable in-kernel firmware update capability.
>> +
>> + Add support to the RMI4 core to enable reflashing of device
>> + firmware.
>
> Please provide more description here of what someone is supposed to do
> to utilize this support, and what it actually does. The term "update"
> here is generic enough to cause some confusion.
OK
>
>> +
>> config RMI4_I2C
>> tristate "RMI4 I2C Support"
>> depends on RMI4_CORE && I2C
>> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
>> index 5c6bad5..570ea80 100644
>> --- a/drivers/input/rmi4/Makefile
>> +++ b/drivers/input/rmi4/Makefile
>> @@ -1,5 +1,6 @@
>> obj-$(CONFIG_RMI4_CORE) += rmi_core.o
>> rmi_core-y := rmi_bus.o rmi_driver.o rmi_f01.o
>> +rmi_core-$(CONFIG_RMI4_FWLIB) += rmi_fw_update.o
>
> Ok. Now I'm thoroughly confused, and I haven't even gotten to the code
> yet. FWLIB => rmi_fw_update => rmi_reflash. What are we doing again?
See my first comment.
>
>>
>> # Function drivers
>> obj-$(CONFIG_RMI4_F11) += rmi_f11.o
>> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
>> index 6e0454a..3c93d08 100644
>> --- a/drivers/input/rmi4/rmi_bus.c
>> +++ b/drivers/input/rmi4/rmi_bus.c
>> @@ -117,6 +117,8 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
>> if (error)
>> goto err_put_device;
>>
>> + rmi_reflash_init(rmi_dev);
>> +
>> dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
>> pdata->sensor_name, dev_name(&rmi_dev->dev));
>>
>> @@ -139,6 +141,7 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
>> struct rmi_device *rmi_dev = xport->rmi_dev;
>>
>> device_del(&rmi_dev->dev);
>> + rmi_reflash_cleanup(rmi_dev);
>> rmi_physical_teardown_debugfs(rmi_dev);
>> put_device(&rmi_dev->dev);
>> }
>> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> [...]
>> +#ifdef CONFIG_RMI4_FWLIB
>> +void rmi_reflash_init(struct rmi_device *rmi_dev);
>> +void rmi_reflash_cleanup(struct rmi_device *rmi_dev);
>> +#else
>> +#define rmi_reflash_init(rmi_dev)
>> +#define rmi_reflash_cleanup(rmi_dev)
>
> Please use static inline functions here. It helps the compiler tell
> you when you do something wrong.
OK
>
>> +#endif
>>
>> #endif
>> diff --git a/drivers/input/rmi4/rmi_fw_update.c b/drivers/input/rmi4/rmi_fw_update.c
> [...]
>> +struct rmi_reflash_data {
>> + struct rmi_device *rmi_dev;
>> + bool force;
>> + ulong busy;
>> + char name_buf[RMI_NAME_BUFFER_SIZE];
>> + char *img_name;
>
> const?
OK
>
>> + struct pdt_entry f01_pdt;
>> + struct f01_basic_properties f01_props;
>> + u8 device_status;
>> + struct pdt_entry f34_pdt;
>> + u8 bootloader_id[2];
>> + struct rmi_f34_queries f34_queries;
>> + u16 f34_status_address;
>> + struct rmi_f34_control_status f34_controls;
>> + const u8 *firmware_data;
>> + const u8 *config_data;
>> + struct work_struct reflash_work;
>> +};
> [...]
>> +static int rmi_read_f01_status(struct rmi_reflash_data *data)
>> +{
>> + int retval;
>> +
>> + retval = rmi_read(data->rmi_dev, data->f01_pdt.data_base_addr,
>> + &data->device_status);
>> + if (retval)
>> + return retval;
>> +
>> + return 0;
>> +}
>
> This function is used one time, just calls rmi_read... There's no need
> to wrap this read.
OK, we'll unwrap this and the others.
>
> [...]
>> +static int rmi_wait_for_idle(struct rmi_reflash_data *data, int timeout_ms)
>> +{
>> + int timeout_count = ((timeout_ms * 1000) / RMI_MAX_SLEEP_TIME_US) + 1;
>> + int count = 0;
>> + struct rmi_f34_control_status *controls = &data->f34_controls;
>> + int retval;
>> +
>> + do {
>> + if (count || timeout_count == 1)
>> + usleep_range(RMI_MIN_SLEEP_TIME_US,
>> + RMI_MAX_SLEEP_TIME_US);
>> + retval = rmi_read_f34_controls(data);
>> + count++;
>> + if (retval)
>> + continue;
>> + else if (IS_IDLE(controls)) {
>> + if (dev_WARN_ONCE(&data->rmi_dev->dev,
>> + !data->f34_controls.program_enabled,
>> + "Reflash is idle but program_enabled bit isn't set.\n"
>> + ))
>> + /*
>> + * This works around a bug in certain device
>> + * firmwares, where the idle state is reached,
>> + * but the program_enabled bit is not yet set.
>> + */
>
> If it's a bug in devices, but it's ok to try again as a workaround, is
> there a good reason to print a stacktrace? Does the user care?
On some devices with this bug, you can generate a lot of log spam by
warning on each occurrence, possibly wrapping the log to the point where
any other information is lost on devices with limited buffer. I felt
the stack trace was a small overhead to pay.
>> + continue;
>> + return 0;
>> + }
>> + } while (count < timeout_count);
>> +
>> + dev_err(&data->rmi_dev->dev,
>> + "ERROR: Timeout waiting for idle status.\n");
>> + dev_err(&data->rmi_dev->dev, "Command: %#04x\n", controls->command);
>> + dev_err(&data->rmi_dev->dev, "Status: %#04x\n", controls->status);
>> + dev_err(&data->rmi_dev->dev, "Enabled: %d\n",
>> + controls->program_enabled);
>> + dev_err(&data->rmi_dev->dev, "Idle: %d\n", IS_IDLE(controls));
>> + return -ETIMEDOUT;
>> +}
> [...]
>> +static int rmi_write_f34_command(struct rmi_reflash_data *data, u8 command)
>> +{
>> + int retval;
>> + struct rmi_device *rmi_dev = data->rmi_dev;
>> +
>> + retval = rmi_write(rmi_dev, data->f34_status_address, command);
>> + if (retval < 0) {
>> + dev_err(&rmi_dev->dev,
>> + "Failed to write F34 command %#04x. Code: %d.\n",
>> + command, retval);
>> + return retval;
>> + }
>> +
>> + return 0;
>> +}
>
> This function is used three times, and calls one function without any
> wrapping magic. You could easily call rmi_write in each place.
>
> [...]
>> +static void rmi_reset_device(struct rmi_reflash_data *data)
>
> It really feels like this should have an error return.
I'm not sure that buys us anything. Regardless of whether we fail or
succeed, we need to execute the next step after this was called
(rediscover functions), and in the failure case we've already printed an
appropriate diagnostic message in this routine.
>
>> +{
>> + int retval;
>> + const struct rmi_device_platform_data *pdata =
>> + rmi_get_platform_data(data->rmi_dev);
>> +
>> + dev_dbg(&data->rmi_dev->dev, "Resetting...\n");
>> + retval = rmi_write(data->rmi_dev, data->f01_pdt.command_base_addr,
>> + RMI_F01_CMD_DEVICE_RESET);
>> + if (retval < 0)
>> + dev_warn(&data->rmi_dev->dev,
>> + "WARNING - post-flash reset failed, code: %d.\n",
>> + retval);
>> + msleep(pdata->reset_delay_ms ?: RMI_F01_DEFAULT_RESET_DELAY_MS);
>> + dev_dbg(&data->rmi_dev->dev, "Reset completed.\n");
>> +}
>
>> +static int rmi_write_firmware(struct rmi_reflash_data *data)
>> +{
>> + return rmi_write_blocks(data, (u8 *) data->firmware_data,
>> + data->f34_queries.fw_block_count, RMI_F34_WRITE_FW_BLOCK);
>> +}
>
> Called once.
>
>> +
>> +static int rmi_write_configuration(struct rmi_reflash_data *data)
>> +{
>> + return rmi_write_blocks(data, (u8 *) data->config_data,
>> + data->f34_queries.config_block_count,
>> + RMI_F34_WRITE_CONFIG_BLOCK);
>> +}
>
> Called once.
>
>> +
>> +static void rmi_reflash_firmware(struct rmi_reflash_data *data)
>
> Also feels like this should have an error return.
I'm not sure that buys us anything. Regardless of whether we fail or
succeed, we need to execute the next two steps after this is called
(reset the device and rediscover functions), and in the failure case
we've already printed an appropriate diagnostic message closer to the
actual point of failure.
>
> [...]
>> +static void rmi_fw_update(struct rmi_device *rmi_dev)
>
> Same.
Not sure it's needed here - if we get to the end of this, we're all done
and there's nothing else we can do if the update didn't work. We've
already printed any appropriate diagnostic messages closer to the point
of failure.
>
>> +{
> [...]
>> + retval = rmi_read_f34_queries(data);
>> + if (retval) {
>> + dev_err(&rmi_dev->dev, "F34 queries failed, code = %d.\n",
>> + retval);
>> + goto done;
>> + }
>> + if (data->img_name && strlen(data->img_name))
>
> data->img_name && data->img_name[0] ? No need to waste extra cycles.
That's tidy. Will apply this here and also as below.
>
>> + snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
>> + rmi_fw_name_format, data->img_name);
>> + else if (pdata->firmware_name && strlen(pdata->firmware_name))
>
> Same.
>
>> + snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
>> + rmi_fw_name_format, pdata->firmware_name);
>> + else {
>> + if (!strlen(data->f01_props.product_id)) {
>
> Same.
>
>> + dev_err(&rmi_dev->dev, "Product ID is missing or empty - will not reflash.\n");
>> + goto done;
>> + }
>> + snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
>> + rmi_fw_name_format, data->f01_props.product_id);
>> + }
> [...]
>> + if (rmi_go_nogo(data, header)) {
>> + dev_dbg(&rmi_dev->dev, "Go/NoGo said go.\n");
>> + rmi_free_function_list(rmi_dev);
>> + rmi_reflash_firmware(data);
>> + rmi_reset_device(data);
>> + rmi_driver_detect_functions(rmi_dev);
>> + } else
>> + dev_dbg(&rmi_dev->dev, "Go/NoGo said don't reflash.\n");
>
> Documentation/CodingStyle says:
> } else {
OK
> ...
> }
>
> [...]
>> +}
> [...]
>> +static ssize_t rmi_reflash_force_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct rmi_device *rmi_dev = to_rmi_device(dev);
>> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
>> + struct rmi_reflash_data *data = drv_data->reflash_data;
>> + int retval;
>> + unsigned long val;
>> +
>> + if (test_and_set_bit(0, &data->busy))
>> + return -EBUSY;
>> +
>> + retval = kstrtoul(buf, 10, &val);
>> + if (retval)
>> + count = retval;
>> + else
>> + data->force = !!val;
>
> Hrm. Perhaps '42' doesn't make sense here. Maybe add:
>
> else if (val > 1)
> count = -EINVAL;
OK.
>
>> +
>> + clear_bit(0, &data->busy);
>> +
>> + return count;
>> +}
>> +
>> +static ssize_t rmi_reflash_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct rmi_device *rmi_dev = to_rmi_device(dev);
>> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
>> + struct rmi_reflash_data *data = drv_data->reflash_data;
>> +
>> + return snprintf(buf, PAGE_SIZE, "%u\n", test_bit(0, &data->busy));
>> +}
>> +
>> +static ssize_t rmi_reflash_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int retval;
>> + unsigned long val;
>> + struct rmi_device *rmi_dev = to_rmi_device(dev);
>> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
>> + struct rmi_reflash_data *data = drv_data->reflash_data;
>> +
>> + retval = kstrtoul(buf, 10, &val);
>> + if (retval)
>> + return retval;
>> +
>> + if (test_and_set_bit(0, &data->busy))
>> + return -EBUSY;
>> +
>> + if (val)
>> + /*
>> + * TODO: Here we start a work thread to go do the reflash, but
>> + * maybe we can just use request_firmware_timeout().
>> + */
>
> Mmm.. Yes. It would make the lifecycle of this busy bit much more
> obvious. Otherwise perhaps call request_firmware_nowait() ? It already
> does this work queue ... uh ... work for you.
Yeah :-)
>
>> + schedule_work(&data->reflash_work);
>> + else
>> + clear_bit(0, &data->busy);
>> +
>> + return count;
>> +}
> [...]
>> +void rmi_reflash_init(struct rmi_device *rmi_dev)
>> +{
>> + int error;
>> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
>> + struct rmi_reflash_data *data;
>> +
>> + dev_dbg(&rmi_dev->dev, "%s called.\n", __func__);
>> +
>> + data = devm_kzalloc(&rmi_dev->dev, sizeof(struct rmi_reflash_data),
>> + GFP_KERNEL);
>
> The memory ownership here is odd. Maybe kzalloc, and just return that
> data?
That would work. I'll switch it to kzalloc/kfree.
>> +
>> + error = sysfs_create_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
>> + if (error) {
>> + dev_warn(&rmi_dev->dev, "Failed to create reflash sysfs attributes.\n");
>> + return;
>> + }
>> +
>> + INIT_WORK(&data->reflash_work, rmi_reflash_work);
>> + data->rmi_dev = rmi_dev;
>> + drv_data->reflash_data = data;
>> +}
>> +
>> +void rmi_reflash_cleanup(struct rmi_device *rmi_dev)
>> +{
>> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
>> + struct rmi_reflash_data *data = drv_data->reflash_data;
>> +
>> + sysfs_remove_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
>> + devm_kfree(&rmi_dev->dev, data);
>
> Who owns this memory again? devm_ doesn't seem right for this use-case.
See above.
^ permalink raw reply
* Re: [PATCH 04/05] input synaptics-rmi4: rmi_driver - Export some symbols and functions for use by reflash.
From: Christopher Heiny @ 2014-03-11 0:13 UTC (permalink / raw)
To: Courtney Cavin
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <20140310233445.GH18578@sonymobile.com>
On 03/10/2014 04:34 PM, Courtney Cavin wrote:
> On Mon, Mar 10, 2014 at 11:54:59PM +0100, Christopher Heiny wrote:
>> On 03/10/2014 08:04 AM, Courtney Cavin wrote:
>>> On Sat, Mar 08, 2014 at 03:29:54AM +0100, Christopher Heiny wrote:
>>>> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
>>> [...]
>>>> -int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
>>>> - u16 pdt_address);
>>>> +#define RMI_SCAN_CONTINUE 0
>>>> +#define RMI_SCAN_DONE 1
>>>> +
>>>> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
>>>> + int (*callback)(struct rmi_device *rmi_dev,
>>>> + void *ctx, const struct pdt_entry *entry));
>>>
>>> I don't really like this callback. The main reason for it is early
>>> abort of PDT scanning, right? It is really that beneficial?
>>
>> Well, the main reason for adding this that there are several places
>> where we perform a PDT scan, and they were proving vulnerable to
>> cut-and-paste errors and code drift. The boilerplate code of the
>> process of doing a PDT scan was also obscuring the actual purpose of
>> each PDT scan.
>>
>> Early abort of the PDT scan is also important - in some of the scans you
>> want to quit as soon as you've found the function(s) of interest, or if
>> you detect that the device is still in bootloader mode. Since there are
>> 256 possible RMI4 pages to scan, stopping early provides serious time
>> savings at boot time and during the reflash process. It also simplifies
>> the code when the device comes up in bootloader mode.
>
> Ok. I'm not entirely familiar with the whole paging thing, as it wasn't
> part of the spec when I was working with this stuff. Is it not true
> that you can cancel looking through pages when you encounter one with no
> functions? I guess it could be possible that there is a device with all
> 256 pages populated, where we could encounter a large enough time
> difference though. Bummer.
>
> Maybe we can do something like the following:
>
> struct rmi_pdt {
> u8 page;
> u8 offset;
> };
>
> int rmi_pdt_init(struct rmi_dev *dev, struct rmi_pdt *pdt);
> int rmi_pdt_next(struct rmi_dev *dev, struct rmi_pdt *pdt,
> struct rmi_pdt_entry *e);
>
> Where you can do:
>
> struct rmi_pdt_entry e;
> struct rmi_pdt pdt;
>
> rmi_pdt_init(dev, &pdt);
>
> while (!rmi_pdt_next(dev, &pdt, &e)) {
> do something; break when done
> }
>
> With this, you can drive the scanning directly from where you want the
> data, instead of playing with void pointers. It's also hard to use
> incorrectly, and easy to follow.
>
> What do you think?
Hmmmm. I'd like to noodle with it a bit and see what the resulting code
looks like. Can we split that off into a separate patch set from the
firmware update patches?
>
>>>> +int check_bootloader_mode(struct rmi_device *rmi_dev,
>>>> + const struct pdt_entry *pdt);
>>>
>>> This is a silly function name to put in a header. rmi_* perhaps?
>>
>> Yes, I noticed that too while preparing the patch, along with a lot of
>> other instances. I decided to do an overall namespace cleanup later,
>> and not piggyback it onto this particular patchset. I'll fix this one
>> if it's a blocking issue.
>
> I'd like it if we could fix this in this series, so we don't forget
> later.
OK.
^ permalink raw reply
* Re: [PATCH 04/05] input synaptics-rmi4: rmi_driver - Export some symbols and functions for use by reflash.
From: Courtney Cavin @ 2014-03-10 23:34 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <531E42C3.5000800@synaptics.com>
On Mon, Mar 10, 2014 at 11:54:59PM +0100, Christopher Heiny wrote:
> On 03/10/2014 08:04 AM, Courtney Cavin wrote:
> > On Sat, Mar 08, 2014 at 03:29:54AM +0100, Christopher Heiny wrote:
> >> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> > [...]
> >> -int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
> >> - u16 pdt_address);
> >> +#define RMI_SCAN_CONTINUE 0
> >> +#define RMI_SCAN_DONE 1
> >> +
> >> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
> >> + int (*callback)(struct rmi_device *rmi_dev,
> >> + void *ctx, const struct pdt_entry *entry));
> >
> > I don't really like this callback. The main reason for it is early
> > abort of PDT scanning, right? It is really that beneficial?
>
> Well, the main reason for adding this that there are several places
> where we perform a PDT scan, and they were proving vulnerable to
> cut-and-paste errors and code drift. The boilerplate code of the
> process of doing a PDT scan was also obscuring the actual purpose of
> each PDT scan.
>
> Early abort of the PDT scan is also important - in some of the scans you
> want to quit as soon as you've found the function(s) of interest, or if
> you detect that the device is still in bootloader mode. Since there are
> 256 possible RMI4 pages to scan, stopping early provides serious time
> savings at boot time and during the reflash process. It also simplifies
> the code when the device comes up in bootloader mode.
Ok. I'm not entirely familiar with the whole paging thing, as it wasn't
part of the spec when I was working with this stuff. Is it not true
that you can cancel looking through pages when you encounter one with no
functions? I guess it could be possible that there is a device with all
256 pages populated, where we could encounter a large enough time
difference though. Bummer.
Maybe we can do something like the following:
struct rmi_pdt {
u8 page;
u8 offset;
};
int rmi_pdt_init(struct rmi_dev *dev, struct rmi_pdt *pdt);
int rmi_pdt_next(struct rmi_dev *dev, struct rmi_pdt *pdt,
struct rmi_pdt_entry *e);
Where you can do:
struct rmi_pdt_entry e;
struct rmi_pdt pdt;
rmi_pdt_init(dev, &pdt);
while (!rmi_pdt_next(dev, &pdt, &e)) {
do something; break when done
}
With this, you can drive the scanning directly from where you want the
data, instead of playing with void pointers. It's also hard to use
incorrectly, and easy to follow.
What do you think?
> >> +int check_bootloader_mode(struct rmi_device *rmi_dev,
> >> + const struct pdt_entry *pdt);
> >
> > This is a silly function name to put in a header. rmi_* perhaps?
>
> Yes, I noticed that too while preparing the patch, along with a lot of
> other instances. I decided to do an overall namespace cleanup later,
> and not piggyback it onto this particular patchset. I'll fix this one
> if it's a blocking issue.
I'd like it if we could fix this in this series, so we don't forget
later.
-Courtney
^ permalink raw reply
* Re: [PATCH v2 7/8] HID: sony: Initialize the controller LEDs with the device ID value
From: Antonio Ospite @ 2014-03-10 22:59 UTC (permalink / raw)
To: Frank Praznik; +Cc: linux-input, jkosina, dh.herrmann
In-Reply-To: <1394145176-24174-8-git-send-email-frank.praznik@oh.rr.com>
On Thu, 6 Mar 2014 17:32:55 -0500
Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Use the device ID to initialize the Sixaxis and DualShock 4 controller LEDs to
> default values. The number or color of the controller is set relative to other
> connected Sony controllers.
>
You already know I am not a huge fan of this idea for the sixaxis and I
found another reason why: the Sixaxis requires the user to press the PS
button before it starts to actually send events and the all-blink
pattern is there to tell the user:
"Look, even if the device is connected, it isn't fully functional yet,
some action is required".
That's also why the BlueZ sixaxis plugin waits for events before
actually setting the LEDs via USB.
Furthermore I still seem to get the all-blink pattern even with the
patch applied, it seems to start _after_ the kernel driver set the
default as per your patch; do you also experience this?
And I still need a recent BLueZ with the sixaxis plugin to use the
controller via BT so I don't see the benefits of defaults over BT
either, but I am obviously biased.
That said, the approach used looks clean enough so I am not going to
oppose any further :)
Just please, if you can, test your changes in conjunction with the
BlueZ sixaxis plugin in order to make sure the two don't step on each
other toes too much.
Thanks,
Antonio
> Set the LED class brightness values to the initial values and add the new led to
> the array before calling led_classdev_register so that the correct brightness value
> shows up in the LED sysfs entry.
Also thanks for thinking about that last part, it's a nice thing to do.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>
> v2 uses the id assigned by the IDA allocator to set the default values.
>
> drivers/hid/hid-sony.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 13af58c..6de42b4 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -1081,6 +1081,52 @@ static int dualshock4_set_operational_bt(struct hid_device *hdev)
> HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> }
>
> +static void sixaxis_set_leds_from_id(int id, __u8 values[MAX_LEDS])
> +{
> + static const __u8 sixaxis_leds[10][4] = {
> + { 0x01, 0x00, 0x00, 0x00 },
> + { 0x00, 0x01, 0x00, 0x00 },
> + { 0x00, 0x00, 0x01, 0x00 },
> + { 0x00, 0x00, 0x00, 0x01 },
> + { 0x01, 0x00, 0x00, 0x01 },
> + { 0x00, 0x01, 0x00, 0x01 },
> + { 0x00, 0x00, 0x01, 0x01 },
> + { 0x01, 0x00, 0x01, 0x01 },
> + { 0x00, 0x01, 0x01, 0x01 },
> + { 0x01, 0x01, 0x01, 0x01 }
> + };
> +
> + BUG_ON(MAX_LEDS < ARRAY_SIZE(sixaxis_leds[0]));
> +
> + if (id < 0)
> + return;
> +
> + id %= 10;
> + memcpy(values, sixaxis_leds[id], sizeof(sixaxis_leds[id]));
> +}
> +
> +static void dualshock4_set_leds_from_id(int id, __u8 values[MAX_LEDS])
> +{
> + /* The first 4 color/index entries match what the PS4 assigns */
> + static const __u8 color_code[7][3] = {
> + /* Blue */ { 0x00, 0x00, 0x01 },
> + /* Red */ { 0x01, 0x00, 0x00 },
> + /* Green */ { 0x00, 0x01, 0x00 },
> + /* Pink */ { 0x02, 0x00, 0x01 },
> + /* Orange */ { 0x02, 0x01, 0x00 },
> + /* Teal */ { 0x00, 0x01, 0x01 },
> + /* White */ { 0x01, 0x01, 0x01 }
> + };
> +
> + BUG_ON(MAX_LEDS < ARRAY_SIZE(color_code[0]));
> +
> + if (id < 0)
> + return;
> +
> + id %= 7;
> + memcpy(values, color_code[id], sizeof(color_code[id]));
> +}
> +
> static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
> {
> struct list_head *report_list =
> @@ -1194,7 +1240,7 @@ static int sony_leds_init(struct sony_sc *sc)
> size_t name_len;
> const char *name_fmt;
> static const char * const color_str[] = { "red", "green", "blue" };
> - static const __u8 initial_values[MAX_LEDS] = { 0x00, 0x00, 0x00, 0x00 };
> + __u8 initial_values[MAX_LEDS] = { 0 };
>
> BUG_ON(!(sc->quirks & SONY_LED_SUPPORT));
>
> @@ -1208,12 +1254,14 @@ static int sony_leds_init(struct sony_sc *sc)
> if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 7))
> return -ENODEV;
> } else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
> + dualshock4_set_leds_from_id(sc->device_id, initial_values);
> sc->led_count = 3;
> max_brightness = 255;
> use_colors = 1;
> name_len = 0;
> name_fmt = "%s:%s";
> } else {
> + sixaxis_set_leds_from_id(sc->device_id, initial_values);
> sc->led_count = 4;
> max_brightness = 1;
> use_colors = 0;
> @@ -1248,19 +1296,20 @@ static int sony_leds_init(struct sony_sc *sc)
> else
> snprintf(name, name_sz, name_fmt, dev_name(&hdev->dev), n + 1);
> led->name = name;
> - led->brightness = 0;
> + led->brightness = initial_values[n];
> led->max_brightness = max_brightness;
> led->brightness_get = sony_led_get_brightness;
> led->brightness_set = sony_led_set_brightness;
>
> + sc->leds[n] = led;
> +
> ret = led_classdev_register(&hdev->dev, led);
> if (ret) {
> hid_err(hdev, "Failed to register LED %d\n", n);
> + sc->leds[n] = NULL;
> kfree(led);
> goto error_leds;
> }
> -
> - sc->leds[n] = led;
> }
>
> return ret;
> --
> 1.8.5.3
>
> --
> 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
--
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 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.
From: Courtney Cavin @ 2014-03-10 22:57 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <20140310224550.GF18578@sonymobile.com>
On Mon, Mar 10, 2014 at 11:45:50PM +0100, Courtney Cavin wrote:
> On Mon, Mar 10, 2014 at 11:33:06PM +0100, Christopher Heiny wrote:
> > On 03/10/2014 07:46 AM, Courtney Cavin wrote:
> > > On Sat, Mar 08, 2014 at 03:29:51AM +0100, Christopher Heiny wrote:
> > >> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> > >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > >> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > >> Cc: Linux Walleij <linus.walleij@linaro.org>
> > >> Cc: David Herrmann <dh.herrmann@gmail.com>
> > >> Cc: Jiri Kosina <jkosina@suse.cz>
> > >>
> > >> ---
> > >>
> > >> drivers/input/rmi4/rmi_f01.c | 96 ++-----------------------------------
> > >> drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
> > >> 2 files changed, 114 insertions(+), 92 deletions(-)
> [...]
> > >
> > > I might be missing something, but these seem like the only defines used
> > > in the flash code. Why not keep these in the f01 driver, and export
> > > a couple more functions, like rmi_f01_reset() and rmi_f01_set_sleep_mode() ?
> >
> > It seems better to me to have the information defined in a single place,
> > rather than scattered hither and yon through the source files.
>
> Uh. Exactly? This is why I'm suggesting that you keep this information
> isolated in the driver to which is directly related.
>
> Perhaps what you mean is that the regs/bits for the entire chip
> functionality should be exposed in header files, so one can read/write
> it from anywhere? That seems backwards to the idea of separating these
> 'functions' out into drivers.
Ah. Wait. I think there was some mis-communication on my part. What I
should have said: Why not keep all of the defines in the driver, and
export a couple more functions?
My point is exactly yours. Keep the defines with the code. Expose
what's needed.
-Courtney
^ permalink raw reply
* Re: [PATCH 04/05] input synaptics-rmi4: rmi_driver - Export some symbols and functions for use by reflash.
From: Christopher Heiny @ 2014-03-10 22:54 UTC (permalink / raw)
To: Courtney Cavin
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <20140310150448.GD18578@sonymobile.com>
On 03/10/2014 08:04 AM, Courtney Cavin wrote:
> On Sat, Mar 08, 2014 at 03:29:54AM +0100, Christopher Heiny wrote:
>> Reflash functionality will need to unload the existing functions and
>> rescan the PDT before starting reflash; then reload the functions
>> afterwards.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_driver.c | 165 ++++++++++++++++++++++------------------
>> drivers/input/rmi4/rmi_driver.h | 22 +++---
>> 2 files changed, 101 insertions(+), 86 deletions(-)
> [...]
>> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> [...]
>> -int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
>> - u16 pdt_address);
>> +#define RMI_SCAN_CONTINUE 0
>> +#define RMI_SCAN_DONE 1
>> +
>> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
>> + int (*callback)(struct rmi_device *rmi_dev,
>> + void *ctx, const struct pdt_entry *entry));
>
> I don't really like this callback. The main reason for it is early
> abort of PDT scanning, right? It is really that beneficial?
Well, the main reason for adding this that there are several places
where we perform a PDT scan, and they were proving vulnerable to
cut-and-paste errors and code drift. The boilerplate code of the
process of doing a PDT scan was also obscuring the actual purpose of
each PDT scan.
Early abort of the PDT scan is also important - in some of the scans you
want to quit as soon as you've found the function(s) of interest, or if
you detect that the device is still in bootloader mode. Since there are
256 possible RMI4 pages to scan, stopping early provides serious time
savings at boot time and during the reflash process. It also simplifies
the code when the device comes up in bootloader mode.
>>
>> bool rmi_is_physical_driver(struct device_driver *);
>> int rmi_register_physical_driver(void);
>> @@ -113,4 +109,10 @@ void rmi_unregister_physical_driver(void);
>> int rmi_register_f01_handler(void);
>> void rmi_unregister_f01_handler(void);
>>
>> +int check_bootloader_mode(struct rmi_device *rmi_dev,
>> + const struct pdt_entry *pdt);
>
> This is a silly function name to put in a header. rmi_* perhaps?
Yes, I noticed that too while preparing the patch, along with a lot of
other instances. I decided to do an overall namespace cleanup later,
and not piggyback it onto this particular patchset. I'll fix this one
if it's a blocking issue.
>
>> +void rmi_free_function_list(struct rmi_device *rmi_dev);
>> +int rmi_driver_detect_functions(struct rmi_device *rmi_dev);
>> +
>> +
>> #endif
>
> -Courtney
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.
From: Courtney Cavin @ 2014-03-10 22:45 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <531E3DA2.5090705@synaptics.com>
On Mon, Mar 10, 2014 at 11:33:06PM +0100, Christopher Heiny wrote:
> On 03/10/2014 07:46 AM, Courtney Cavin wrote:
> > On Sat, Mar 08, 2014 at 03:29:51AM +0100, Christopher Heiny wrote:
> >> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >> Cc: Linux Walleij <linus.walleij@linaro.org>
> >> Cc: David Herrmann <dh.herrmann@gmail.com>
> >> Cc: Jiri Kosina <jkosina@suse.cz>
> >>
> >> ---
> >>
> >> drivers/input/rmi4/rmi_f01.c | 96 ++-----------------------------------
> >> drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 114 insertions(+), 92 deletions(-)
[...]
> >
> > I might be missing something, but these seem like the only defines used
> > in the flash code. Why not keep these in the f01 driver, and export
> > a couple more functions, like rmi_f01_reset() and rmi_f01_set_sleep_mode() ?
>
> It seems better to me to have the information defined in a single place,
> rather than scattered hither and yon through the source files.
Uh. Exactly? This is why I'm suggesting that you keep this information
isolated in the driver to which is directly related.
Perhaps what you mean is that the regs/bits for the entire chip
functionality should be exposed in header files, so one can read/write
it from anywhere? That seems backwards to the idea of separating these
'functions' out into drivers.
-Courtney
^ permalink raw reply
* Re: [PATCH 03/05] input synaptics-rmi4: rmi_f01 - Fix a comment and add a diagnostic output message.
From: Christopher Heiny @ 2014-03-10 22:37 UTC (permalink / raw)
To: Courtney Cavin
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <20140310145158.GC18578@sonymobile.com>
On 03/10/2014 07:51 AM, Courtney Cavin wrote:
> On Sat, Mar 08, 2014 at 03:29:53AM +0100, Christopher Heiny wrote:
>> In debugging certain touch sensor failures, it's useful to know
>> whether the device is stuck in bootloader, so print a message
>> to that effect.
>>
>> Also, point to the actual location of the defs for the F01 CTRL0
>> bitfields.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_f01.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
>> index 8504865..a078d7d 100644
>> --- a/drivers/input/rmi4/rmi_f01.c
>> +++ b/drivers/input/rmi4/rmi_f01.c
>> @@ -16,7 +16,7 @@
>> #include "rmi_f01.h"
>>
>> /**
>> - * @ctrl0 - see the bit definitions above.
>> + * @ctrl0 - see the bit definitions in rmi_f01.h.
>> * @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
>> @@ -415,6 +415,13 @@ static int rmi_f01_probe(struct rmi_function *fn)
>> return error;
>> }
>>
>> + driver_data->f01_bootloader_mode =
>> + RMI_F01_STATUS_BOOTLOADER(device_status);
>> + if (driver_data->f01_bootloader_mode)
>> + dev_warn(&rmi_dev->dev,
>> + "WARNING: RMI4 device is in bootloader mode!\n");
>> +
>> +
>
> The logic here is a bit odd. Would it make sense to put this warning in
> the if condition below? IIRC you can't have a configured device while
> in bootloader mode.
Actually, you can write the configured bit (thus clearing the
unconfigured bit) at any time, whether you're in bootloader mode or not.
>
>> if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
>> dev_err(&fn->dev,
>> "Device was reset during configuration process, status: %#02x!\n",
> -Courtney
> --
> 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
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.
From: Christopher Heiny @ 2014-03-10 22:33 UTC (permalink / raw)
To: Courtney Cavin
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <20140310144658.GB18578@sonymobile.com>
On 03/10/2014 07:46 AM, Courtney Cavin wrote:
> On Sat, Mar 08, 2014 at 03:29:51AM +0100, Christopher Heiny wrote:
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_f01.c | 96 ++-----------------------------------
>> drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 114 insertions(+), 92 deletions(-)
>
> $SUBJECT is 83 characters long. Please be more brief and provide a
> description.
OK.
>
> [...]
>> +#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)
>> +
>
> I might be missing something, but these seem like the only defines used
> in the flash code. Why not keep these in the f01 driver, and export
> a couple more functions, like rmi_f01_reset() and rmi_f01_set_sleep_mode() ?
It seems better to me to have the information defined in a single place,
rather than scattered hither and yon through the source files.
^ permalink raw reply
* Re: [PATCH v2 6/8] HID: sony: Add an IDA allocator to assign unique device ids
From: Antonio Ospite @ 2014-03-10 22:25 UTC (permalink / raw)
To: Frank Praznik; +Cc: linux-input, jkosina, dh.herrmann
In-Reply-To: <1394145176-24174-7-git-send-email-frank.praznik@oh.rr.com>
Hi Frank,
On Thu, 6 Mar 2014 17:32:54 -0500
Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Add an IDA id allocator to assign unique, sequential device ids to Sixaxis and
> DualShock 4 controllers.
>
> Use explicit module init and exit functions since the IDA allocator must be
> manually destroyed when the module is unloaded.
>
> Use the device id as the unique number for the battery identification string.
>
Have you thought about using the bdaddr as the battery id?
I think that decoupling led numbers (from the following patch) and
battery ids would be saner. For instance in a scenario when userspace
decided that the _second_ sixaxis has LEDs saying "controller
3" (because of different kind of joypads, remember?) we would have
battery still saying "2" because the battery id is assigned at probe
time while LEDs can change at any time. This mismatch may become
confusing.
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
> drivers/hid/hid-sony.c | 70 +++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index a9bcfbe..13af58c 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -33,6 +33,7 @@
> #include <linux/power_supply.h>
> #include <linux/spinlock.h>
> #include <linux/list.h>
> +#include <linux/idr.h>
> #include <linux/input/mt.h>
>
> #include "hid-ids.h"
> @@ -749,6 +750,7 @@ union sixaxis_output_report_01 {
>
> static spinlock_t sony_dev_list_lock;
> static LIST_HEAD(sony_device_list);
> +static DEFINE_IDA(sony_device_id_allocator);
>
> struct sony_sc {
> spinlock_t lock;
> @@ -758,6 +760,7 @@ struct sony_sc {
> unsigned long quirks;
> struct work_struct state_worker;
> struct power_supply battery;
> + int device_id;
>
> #ifdef CONFIG_SONY_FF
> __u8 left;
> @@ -1413,8 +1416,6 @@ static int sony_battery_get_property(struct power_supply *psy,
>
> static int sony_battery_probe(struct sony_sc *sc)
> {
> - static atomic_t power_id_seq = ATOMIC_INIT(0);
> - unsigned long power_id;
> struct hid_device *hdev = sc->hdev;
> int ret;
>
> @@ -1424,15 +1425,13 @@ static int sony_battery_probe(struct sony_sc *sc)
> */
> sc->battery_capacity = 100;
>
> - power_id = (unsigned long)atomic_inc_return(&power_id_seq);
> -
> sc->battery.properties = sony_battery_props;
> sc->battery.num_properties = ARRAY_SIZE(sony_battery_props);
> sc->battery.get_property = sony_battery_get_property;
> sc->battery.type = POWER_SUPPLY_TYPE_BATTERY;
> sc->battery.use_for_apm = 0;
> - sc->battery.name = kasprintf(GFP_KERNEL, "sony_controller_battery_%lu",
> - power_id);
> + sc->battery.name = kasprintf(GFP_KERNEL, "sony_controller_battery_%i",
> + sc->device_id);
> if (!sc->battery.name)
> return -ENOMEM;
>
> @@ -1607,6 +1606,38 @@ static int sony_check_add(struct sony_sc *sc)
> return sony_check_add_dev_list(sc);
> }
>
> +static int sony_set_device_id(struct sony_sc *sc)
> +{
> + int ret;
> +
> + /*
> + * Only DualShock 4 or Sixaxis controller get an id.
> + * All others are set to -1.
> + */
> + if ((sc->quirks & SIXAXIS_CONTROLLER) ||
> + (sc->quirks & DUALSHOCK4_CONTROLLER)) {
> + ret = ida_simple_get(&sony_device_id_allocator, 0, 0,
> + GFP_KERNEL);
> + if (ret < 0) {
> + sc->device_id = -1;
> + return ret;
> + }
> + sc->device_id = ret;
> + } else {
> + sc->device_id = -1;
> + }
> +
> + return 0;
> +}
> +
> +static void sony_release_device_id(struct sony_sc *sc)
> +{
> + if (sc->device_id >= 0) {
> + ida_simple_remove(&sony_device_id_allocator, sc->device_id);
> + sc->device_id = -1;
> + }
> +}
> +
> static inline void sony_init_work(struct sony_sc *sc,
> void(*worker)(struct work_struct *))
> {
> @@ -1658,6 +1689,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> return ret;
> }
>
> + ret = sony_set_device_id(sc);
> + if (ret < 0) {
> + hid_err(hdev, "failed to allocate the device id\n");
> + goto err_stop;
> + }
> +
> if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> /*
> * The Sony Sixaxis does not handle HID Output Reports on the
> @@ -1749,6 +1786,7 @@ err_stop:
> sony_battery_remove(sc);
> sony_cancel_work_sync(sc);
> sony_remove_dev_list(sc);
> + sony_release_device_id(sc);
> hid_hw_stop(hdev);
> return ret;
> }
> @@ -1769,6 +1807,8 @@ static void sony_remove(struct hid_device *hdev)
>
> sony_remove_dev_list(sc);
>
> + sony_release_device_id(sc);
> +
> hid_hw_stop(hdev);
> }
>
> @@ -1813,6 +1853,22 @@ static struct hid_driver sony_driver = {
> .report_fixup = sony_report_fixup,
> .raw_event = sony_raw_event
> };
> -module_hid_driver(sony_driver);
> +
> +static int __init sony_init(void)
> +{
> + dbg_hid("Sony:%s\n", __func__);
> +
> + return hid_register_driver(&sony_driver);
> +}
> +
> +static void __exit sony_exit(void)
> +{
> + dbg_hid("Sony:%s\n", __func__);
> +
> + ida_destroy(&sony_device_id_allocator);
> + hid_unregister_driver(&sony_driver);
> +}
> +module_init(sony_init);
> +module_exit(sony_exit);
>
> MODULE_LICENSE("GPL");
> --
> 1.8.5.3
>
> --
> 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
>
--
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 v3 3/4] HID: sony: do not rely on hid_output_raw_report
From: Antonio Ospite @ 2014-03-10 21:49 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, David Herrmann, David Barksdale,
linux-input, linux-kernel
In-Reply-To: <20140310123253.0b728251617d599b852ac23a@ao2.it>
On Mon, 10 Mar 2014 12:32:53 +0100
Antonio Ospite <ao2@ao2.it> wrote:
> On Sat, 8 Mar 2014 22:52:42 -0500
> Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>
> > hid_out_raw_report is going to be obsoleted as it is not part of the
> > unified HID low level transport documentation
> > (Documentation/hid/hid-transport.txt)
> >
> > To do so, we need to introduce two new quirks:
> > * HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP: this quirks prevents the
> > transport driver to use the interrupt channel to send output report
> > (and thus force to use HID_REQ_SET_REPORT command)
> > * HID_QUIRK_SKIP_OUTPUT_REPORT_ID: this one forces usbhid to not
> > include the report ID in the buffer it sends to the device through
> > HID_REQ_SET_REPORT in case of an output report
> >
> > This also fixes a regression introduced in commit 3a75b24949a8
> > (HID: hidraw: replace hid_output_raw_report() calls by appropriates ones).
> > The hidraw API was not able to communicate with the PS3 SixAxis
> > controllers in USB mode.
> >
> > Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
>
> I confirm that using hidraw via USB works again now, thanks, so the
> patch is OK:
>
> Tested-by: Antonio Ospite <ao2@ao2.it>
>
> However I still get the failure with hidraw via BT, I am going to look
> into that.
>
OK, it's all good even with hidraw via BT.
I was compiling the wrong branch which didn't contain the changes from
for-3.15/hid-core-ll-transport-cleanup. Sorry for the noise.
> > ---
> > changes in v3:
> > - no changes
> >
> > changes in v2:
> > - removed usb.h include
> > - renamed HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP
> > - fix typo
> >
> >
> > drivers/hid/hid-sony.c | 60 ++++++++++---------------------------------
> > drivers/hid/hidraw.c | 3 ++-
> > drivers/hid/usbhid/hid-core.c | 7 ++++-
> > include/linux/hid.h | 2 ++
> > 4 files changed, 24 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> > index b5fe65e..4884bb5 100644
> > --- a/drivers/hid/hid-sony.c
> > +++ b/drivers/hid/hid-sony.c
> > @@ -29,7 +29,6 @@
> > #include <linux/hid.h>
> > #include <linux/module.h>
> > #include <linux/slab.h>
> > -#include <linux/usb.h>
> > #include <linux/leds.h>
> > #include <linux/power_supply.h>
> > #include <linux/spinlock.h>
> > @@ -1007,45 +1006,6 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
> > }
> >
> > /*
> > - * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
> > - * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
> > - * so we need to override that forcing HID Output Reports on the Control EP.
> > - *
> > - * There is also another issue about HID Output Reports via USB, the Sixaxis
> > - * does not want the report_id as part of the data packet, so we have to
> > - * discard buf[0] when sending the actual control message, even for numbered
> > - * reports, humpf!
> > - */
> > -static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
> > - size_t count, unsigned char report_type)
> > -{
> > - struct usb_interface *intf = to_usb_interface(hid->dev.parent);
> > - struct usb_device *dev = interface_to_usbdev(intf);
> > - struct usb_host_interface *interface = intf->cur_altsetting;
> > - int report_id = buf[0];
> > - int ret;
> > -
> > - if (report_type == HID_OUTPUT_REPORT) {
> > - /* Don't send the Report ID */
> > - buf++;
> > - count--;
> > - }
> > -
> > - ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> > - HID_REQ_SET_REPORT,
> > - USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> > - ((report_type + 1) << 8) | report_id,
> > - interface->desc.bInterfaceNumber, buf, count,
> > - USB_CTRL_SET_TIMEOUT);
> > -
> > - /* Count also the Report ID, in case of an Output report. */
> > - if (ret > 0 && report_type == HID_OUTPUT_REPORT)
> > - ret++;
> > -
> > - return ret;
> > -}
> > -
> > -/*
> > * Sending HID_REQ_GET_REPORT changes the operation mode of the ps3 controller
> > * to "operational". Without this, the ps3 controller will not report any
> > * events.
> > @@ -1305,11 +1265,8 @@ static void sixaxis_state_worker(struct work_struct *work)
> > buf[10] |= sc->led_state[2] << 3;
> > buf[10] |= sc->led_state[3] << 4;
> >
> > - if (sc->quirks & SIXAXIS_CONTROLLER_USB)
> > - hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
> > - else
> > - hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf),
> > - HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> > + hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf), HID_OUTPUT_REPORT,
> > + HID_REQ_SET_REPORT);
> > }
> >
> > static void dualshock4_state_worker(struct work_struct *work)
> > @@ -1659,7 +1616,18 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > }
> >
> > if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> > - hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
> > + /*
> > + * The Sony Sixaxis does not handle HID Output Reports on the
> > + * Interrupt EP like it could, so we need to force HID Output
> > + * Reports to use HID_REQ_SET_REPORT on the Control EP.
> > + *
> > + * There is also another issue about HID Output Reports via USB,
> > + * the Sixaxis does not want the report_id as part of the data
> > + * packet, so we have to discard buf[0] when sending the actual
> > + * control message, even for numbered reports, humpf!
> > + */
> > + hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP;
> > + hdev->quirks |= HID_QUIRK_SKIP_OUTPUT_REPORT_ID;
> > ret = sixaxis_set_operational_usb(hdev);
> > sc->worker_initialized = 1;
> > INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> > diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> > index 2cc484c..ffa648c 100644
> > --- a/drivers/hid/hidraw.c
> > +++ b/drivers/hid/hidraw.c
> > @@ -149,7 +149,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
> > goto out_free;
> > }
> >
> > - if (report_type == HID_OUTPUT_REPORT) {
> > + if ((report_type == HID_OUTPUT_REPORT) &&
> > + !(dev->quirks & HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP)) {
> > ret = hid_hw_output_report(dev, buf, count);
> > /*
> > * compatibility with old implementation of USB-HID and I2C-HID:
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index 0d1d875..3bc7cad 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -894,7 +894,12 @@ static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
> > int ret, skipped_report_id = 0;
> >
> > /* Byte 0 is the report number. Report data starts at byte 1.*/
> > - buf[0] = reportnum;
> > + if ((rtype == HID_OUTPUT_REPORT) &&
> > + (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORT_ID))
> > + buf[0] = 0;
> > + else
> > + buf[0] = reportnum;
> > +
> > if (buf[0] == 0x0) {
> > /* Don't send the Report ID */
> > buf++;
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index e224516..2cd7174 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -290,6 +290,8 @@ struct hid_item {
> > #define HID_QUIRK_NO_EMPTY_INPUT 0x00000100
> > #define HID_QUIRK_NO_INIT_INPUT_REPORTS 0x00000200
> > #define HID_QUIRK_SKIP_OUTPUT_REPORTS 0x00010000
> > +#define HID_QUIRK_SKIP_OUTPUT_REPORT_ID 0x00020000
> > +#define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP 0x00040000
> > #define HID_QUIRK_FULLSPEED_INTERVAL 0x10000000
> > #define HID_QUIRK_NO_INIT_REPORTS 0x20000000
> > #define HID_QUIRK_NO_IGNORE 0x40000000
> > --
> > 1.8.5.3
> >
>
>
> --
> 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?
> --
> 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
>
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH 2/2] Surface Pro 2 USB composite device handling
From: Benjamin Tissoires @ 2014-03-10 20:23 UTC (permalink / raw)
To: Derya; +Cc: linux-input, Jiri Kosina
In-Reply-To: <531CAB67.4020202@yahoo.de>
On Mar 09 2014 or thereabouts, Derya wrote:
> Surface Pro 2 USB composite device handling
>
> (device changes it product id with attached keyboard cover)
> - Adds MS Surface Pro 2's composite device ids, with and without attached
> keyboard, to the hid-ids
> - Ensures to load hid-core instead of hid-multitouch for the keyboard covers
> - Adds HID_QUIRK_NO_INIT_INPUT_REPORTS to prevent USB submit urb failure
> during keyboard cover (de)attaching
>
> Signed-off-by: Derya <derya.kiran@yahoo.de>
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
> ---
> drivers/hid/hid-core.c | 9 +++++++++
> drivers/hid/hid-ids.h | 3 +++
> drivers/hid/usbhid/hid-quirks.c | 3 +++
> 3 files changed, 15 insertions(+)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index bb5c494..f4e4820 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -775,6 +775,15 @@ static int hid_scan_report(struct hid_device *hid)
> if ((parser->scan_flags & HID_SCAN_FLAG_MT_WIN_8) &&
> (hid->group == HID_GROUP_MULTITOUCH))
> hid->group = HID_GROUP_MULTITOUCH_WIN_8;
> +
> + /*
> + * Handle vendor specific handlings
> + */
> + if ((hid->vendor == USB_VENDOR_ID_MICROSOFT) &&
> + (hid->product == USB_DEVICE_ID_MS_TYPE_COVER_2 ||
> + hid->product == USB_DEVICE_ID_MS_TOUCH_COVER_2) &&
> + (hid->group == HID_GROUP_MULTITOUCH))
> + hid->group = HID_GROUP_GENERIC;
>
> vfree(parser);
> return 0;
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 07cd28c..f61c0d8 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -624,6 +624,9 @@
> #define USB_DEVICE_ID_MS_PRESENTER_8K_USB 0x0713
> #define USB_DEVICE_ID_MS_DIGITAL_MEDIA_3K 0x0730
> #define USB_DEVICE_ID_MS_COMFORT_MOUSE_4500 0x076c
> +#define USB_DEVICE_ID_MS_SURFACE_PRO_2 0x0799
> +#define USB_DEVICE_ID_MS_TOUCH_COVER_2 0x07a7
> +#define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9
>
> #define USB_VENDOR_ID_MOJO 0x8282
> #define USB_DEVICE_ID_RETRO_ADAPTER 0x3201
> diff --git a/drivers/hid/usbhid/hid-quirks.c
> b/drivers/hid/usbhid/hid-quirks.c
> index dbd8387..7c4f86e 100644
> --- a/drivers/hid/usbhid/hid-quirks.c
> +++ b/drivers/hid/usbhid/hid-quirks.c
> @@ -73,6 +73,9 @@ static const struct hid_blacklist {
> { USB_VENDOR_ID_FORMOSA, USB_DEVICE_ID_FORMOSA_IR_RECEIVER,
> HID_QUIRK_NO_INIT_REPORTS },
> { USB_VENDOR_ID_FREESCALE, USB_DEVICE_ID_FREESCALE_MX28,
> HID_QUIRK_NOGET },
> { USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS, HID_QUIRK_NOGET },
> + { USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_SURFACE_PRO_2,
> HID_QUIRK_NO_INIT_INPUT_REPORTS },
> + { USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_2,
> HID_QUIRK_NO_INIT_INPUT_REPORTS },
> + { USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TOUCH_COVER_2,
> HID_QUIRK_NO_INIT_INPUT_REPORTS },
> { USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GX680R_LED_PANEL,
> HID_QUIRK_NO_INIT_REPORTS },
> { USB_VENDOR_ID_NEXIO, USB_DEVICE_ID_NEXIO_MULTITOUCH_PTI0750,
> HID_QUIRK_NO_INIT_REPORTS },
> { USB_VENDOR_ID_NOVATEK, USB_DEVICE_ID_NOVATEK_MOUSE,
> HID_QUIRK_NO_INIT_REPORTS },
> --
> 1.8.3.2
>
^ permalink raw reply
* Re: [PATCH 1/2] Revert "HID: microsoft: Add ID's for Surface Type/Touch, Cover 2"
From: Benjamin Tissoires @ 2014-03-10 20:22 UTC (permalink / raw)
To: Derya; +Cc: linux-input, Jiri Kosina
In-Reply-To: <531CAA97.4020802@yahoo.de>
On Mar 09 2014 or thereabouts, Derya wrote:
> This reverts commit 117309c51dca42121f70cacec801511b76acf75c.
>
> The MS Surface Pro 2 has an USB composite device with 3 interfaces
> - interface 0 - sensor hub
> - interface 1 - wacom digitizer
> - interface 2 - the keyboard cover, if one is attached
> This USB composite device changes it product id dependent on if and which
> keyboard cover is attached. Adding the covers to hid_have_special_driver
> prevents loading the right hid drivers for the other two interfaces, all 3
> get loaded with hid-microsoft. We don't even need hid-microsoft for the
> keyboards. We have to revert this to load the right hid modules for each
> interface.
>
> Signed-off-by: Derya <derya.kiran@yahoo.de>
Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Hoewever, Derya, you should reconsider re-sending the whole series
through "git send-email" or configuring your e-mail client according to
Documentation/email-clients.txt . This series has the lines wrapped and
tabs are converted to spaces, which means that Jiri ca not take the
patch without reformatting them to his tree.
Thanks,
Benjamin
> ---
> drivers/hid/hid-core.c | 2 --
> drivers/hid/hid-ids.h | 2 --
> drivers/hid/hid-microsoft.c | 4 ----
> 3 files changed, 8 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index cc32a6f..bb5c494 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1780,8 +1780,6 @@ static const struct hid_device_id
> hid_have_special_driver[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_PRESENTER_8K_USB) },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_DIGITAL_MEDIA_3K) },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_WIRELESS_OPTICAL_DESKTOP_3_0) },
> - { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_TYPE_COVER_2) },
> - { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_TOUCH_COVER_2) },
> { HID_USB_DEVICE(USB_VENDOR_ID_MONTEREY, USB_DEVICE_ID_GENIUS_KB29E) },
> { HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, USB_DEVICE_ID_NTRIG_TOUCH_SCREEN)
> },
> { HID_USB_DEVICE(USB_VENDOR_ID_NTRIG,
> USB_DEVICE_ID_NTRIG_TOUCH_SCREEN_1) },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 22f28d6..07cd28c 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -624,8 +624,6 @@
> #define USB_DEVICE_ID_MS_PRESENTER_8K_USB 0x0713
> #define USB_DEVICE_ID_MS_DIGITAL_MEDIA_3K 0x0730
> #define USB_DEVICE_ID_MS_COMFORT_MOUSE_4500 0x076c
> -#define USB_DEVICE_ID_MS_TOUCH_COVER_2 0x07a7
> -#define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9
>
> #define USB_VENDOR_ID_MOJO 0x8282
> #define USB_DEVICE_ID_RETRO_ADAPTER 0x3201
> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index 404a3a8..c6ef6ee 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -208,10 +208,6 @@ static const struct hid_device_id ms_devices[] = {
> .driver_data = MS_NOGET },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_COMFORT_MOUSE_4500),
> .driver_data = MS_DUPLICATE_USAGES },
> - { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_TYPE_COVER_2),
> - .driver_data = 0 },
> - { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_TOUCH_COVER_2),
> - .driver_data = 0 },
>
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_PRESENTER_8K_BT),
> .driver_data = MS_PRESENTER },
> --
> 1.8.3.2
>
^ permalink raw reply
* [PATCH V2] input: misc: da9063: OnKey driver
From: Opensource [Steve Twiss] @ 2014-03-10 17:24 UTC (permalink / raw)
To: Dmitry Torokhov, Lee Jones, Samuel Ortiz
Cc: David Dajun Chen, LKML-INPUT, LKML-KERNEL, Mark Brown,
Philipp Zabel
From: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
Add the OnKey driver for DA9063
Plus some minor dependencies:
- Addition of "ONKEY" name to OnKey IRQ resource structure;
- Bool key_power platform data driver configuration option.
Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
---
Checks performed with linux-next/next-20140307/scripts/checkpatch.pl
da9063-core.c total: 0 errors, 0 warnings, 189 lines checked
pdata.h total: 0 errors, 0 warnings, 112 lines checked
da9063-onkey.c total: 0 errors, 0 warnings, 209 lines checked
Kconfig total: 0 errors, 11 warnings, 679 lines checked
Makefile total: 0 errors, 0 warnings, 66 lines checked
Hi Lee,
I have added the minor changes as part of the main DA9063 OnKey patch
as requested.
The majority of this patch is DA9063 OnKey driver.
The other changes to the files drivers/mfd/da9063-core.c and
include/linux/mfd/da9063/pdata.h are dependencies required to support
the OnKey driver. The changes are made up of the following:
- An addition of a name field "ONKEY" to the properties of the
the OnKey IORESOURCE_IRQ resource structure (part of the mfd_cell
Onkey resource).
- A bool key_power variable which will be passed to the onkey
driver and which will allow KEY_POWER support to be turned on/off
as a driver configuration option.
Hi Dmitry,
Thank you for your previous response to my RFC.
Here are my changes. I have implemented your suggestions. Please find my
explanations below.
Changes made to this driver since previous RFC V1:
- Several alterations to the previous patch have been made according
to the comments provided by Dmitry Torokhov in the previous RFC V1
e-mail thread, see here:
http://www.spinics.net/lists/linux-input/msg30171.html
- The use of booleans for boolean data (true/false);
- The addition of an extra input_sync() in between the two calls to
input_report_key(KEY_SLEEP);
- The removal of call to dev_err() because it was unnecessary;
- A fix for the race condition during driver remove() function:
"nothing stops IRQ from firing again and rescheduling the work item".
Also, the addition of a work cancelling function during the error
path of the driver probe() after the interrupt has been registered.
The error path of the probe() function has been refactored slightly
using goto statements to make things clearer.
This fix also required a change to the way the interrupt was
registered: probe() is now using request_threaded_irq() instead of
the devm_ equivalent so that an explicit call to free_irq() can be
done before any calls to cancel_delayed_work_sync() are made;
- A clarification to the way the da9063_poll_on() function should
handle the I2C failure error case.
The key report for KEY_POWER is now only made if there has been a
fully a successful update to the DA9063_NONKEY bit in the IRQ
mask. Otherwise it will continue to re-poll until the onkey's
IRQ mask has been modified successfully.
These patches apply against kernel linux-next next-20140307
Regards,
Steve Twiss, Dialog Semiconductor Ltd.
drivers/input/misc/Kconfig | 10 ++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/da9063-onkey.c | 209 +++++++++++++++++++++++++++++++++++++
drivers/mfd/da9063-core.c | 1 +
include/linux/mfd/da9063/pdata.h | 1 +
5 files changed, 222 insertions(+)
create mode 100644 drivers/input/misc/da9063-onkey.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 762e6d2..3deb008 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -522,6 +522,16 @@ config INPUT_DA9055_ONKEY
To compile this driver as a module, choose M here: the module
will be called da9055_onkey.
+config INPUT_DA9063_ONKEY
+ tristate "Dialog DA9063 OnKey"
+ depends on MFD_DA9063
+ help
+ Support the ONKEY of Dialog DA9063 Power Management IC as an
+ input device reporting power button statue.
+
+ To compile this driver as a module, choose M here: the module
+ will be called da9063-onkey.
+
config INPUT_DM355EVM
tristate "TI DaVinci DM355 EVM Keypad and IR Remote"
depends on MFD_DM355EVM_MSP
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index cda71fc..f40caa7 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_INPUT_CMA3000_I2C) += cma3000_d0x_i2c.o
obj-$(CONFIG_INPUT_COBALT_BTNS) += cobalt_btns.o
obj-$(CONFIG_INPUT_DA9052_ONKEY) += da9052_onkey.o
obj-$(CONFIG_INPUT_DA9055_ONKEY) += da9055_onkey.o
+obj-$(CONFIG_INPUT_DA9063_ONKEY) += da9063-onkey.o
obj-$(CONFIG_INPUT_DM355EVM) += dm355evm_keys.o
obj-$(CONFIG_INPUT_GP2A) += gp2ap002a00f.o
obj-$(CONFIG_INPUT_GPIO_BEEPER) += gpio-beeper.o
diff --git a/drivers/input/misc/da9063-onkey.c b/drivers/input/misc/da9063-onkey.c
new file mode 100644
index 0000000..ce08954
--- /dev/null
+++ b/drivers/input/misc/da9063-onkey.c
@@ -0,0 +1,209 @@
+/* da9063-onkey.c - Onkey device driver for DA9063
+ * Copyright (C) 2013 Dialog Semiconductor Ltd.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Library General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/workqueue.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/da9063/core.h>
+#include <linux/mfd/da9063/pdata.h>
+#include <linux/mfd/da9063/registers.h>
+
+struct da9063_onkey {
+ struct da9063 *hw;
+ struct delayed_work work;
+ struct input_dev *input;
+ int irq;
+ bool key_power;
+};
+
+static void da9063_poll_on(struct work_struct *work)
+{
+ struct da9063_onkey *onkey = container_of(work, struct da9063_onkey,
+ work.work);
+ unsigned int val;
+ bool poll = true;
+ int ret;
+
+ /* poll to see when the pin is released */
+ ret = regmap_read(onkey->hw->regmap, DA9063_REG_STATUS_A, &val);
+ if (ret < 0) {
+ dev_err(&onkey->input->dev,
+ "Failed to read ON status: %d\n", ret);
+ goto err_poll;
+ }
+
+ if (!(val & DA9063_NONKEY)) {
+ ret = regmap_update_bits(onkey->hw->regmap,
+ DA9063_REG_CONTROL_B,
+ DA9063_NONKEY_LOCK, 0);
+ if (ret < 0) {
+ dev_err(&onkey->input->dev,
+ "Failed to reset the Key Delay %d\n", ret);
+ goto err_poll;
+ }
+
+ /* unmask the onkey interrupt again */
+ ret = regmap_update_bits(onkey->hw->regmap,
+ DA9063_REG_IRQ_MASK_A,
+ DA9063_NONKEY, 0);
+ if (ret < 0) {
+ dev_err(&onkey->input->dev,
+ "Failed to unmask the onkey IRQ: %d\n", ret);
+ goto err_poll;
+ }
+
+ input_report_key(onkey->input, KEY_POWER, 0);
+ input_sync(onkey->input);
+
+ poll = false;
+ }
+
+err_poll:
+ if (poll)
+ schedule_delayed_work(&onkey->work, 50);
+}
+
+static irqreturn_t da9063_onkey_irq_handler(int irq, void *data)
+{
+ struct da9063_onkey *onkey = data;
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(onkey->hw->regmap, DA9063_REG_STATUS_A, &val);
+ if (onkey->key_power && (ret >= 0) && (val & DA9063_NONKEY)) {
+ ret = regmap_update_bits(onkey->hw->regmap,
+ DA9063_REG_IRQ_MASK_A,
+ DA9063_NONKEY, 1);
+ if (ret < 0)
+ dev_err(&onkey->input->dev,
+ "Failed to mask the onkey IRQ: %d\n", ret);
+
+ input_report_key(onkey->input, KEY_POWER, 1);
+ input_sync(onkey->input);
+
+ schedule_delayed_work(&onkey->work, 0);
+ dev_dbg(&onkey->input->dev, "KEY_POWER pressed.\n");
+ } else {
+ input_report_key(onkey->input, KEY_SLEEP, 1);
+ input_sync(onkey->input);
+ input_report_key(onkey->input, KEY_SLEEP, 0);
+ input_sync(onkey->input);
+ dev_dbg(&onkey->input->dev, "KEY_SLEEP pressed.\n");
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int da9063_onkey_probe(struct platform_device *pdev)
+{
+ struct da9063 *da9063 = dev_get_drvdata(pdev->dev.parent);
+ struct da9063_pdata *pdata = dev_get_platdata(da9063->dev);
+ struct da9063_onkey *onkey;
+ bool kp_tmp = true;
+ int ret = 0;
+
+ if (pdata)
+ kp_tmp = pdata->key_power;
+
+ onkey = devm_kzalloc(&pdev->dev, sizeof(struct da9063_onkey),
+ GFP_KERNEL);
+ if (!onkey) {
+ dev_err(&pdev->dev, "Failed to allocate memory.\n");
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ INIT_DELAYED_WORK(&onkey->work, da9063_poll_on);
+
+ onkey->input = devm_input_allocate_device(&pdev->dev);
+ if (!onkey->input) {
+ dev_err(&pdev->dev, "Failed to allocated input device.\n");
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ ret = platform_get_irq_byname(pdev, "ONKEY");
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Failed to get platform IRQ.\n");
+ goto err;
+ }
+ onkey->irq = ret;
+
+ ret = request_threaded_irq(onkey->irq, NULL,
+ da9063_onkey_irq_handler,
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+ "ONKEY", onkey);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to request input device IRQ.\n");
+ goto err;
+ }
+
+ onkey->hw = da9063;
+ onkey->key_power = kp_tmp;
+ onkey->input->evbit[0] = BIT_MASK(EV_KEY);
+ onkey->input->name = DA9063_DRVNAME_ONKEY;
+ onkey->input->phys = DA9063_DRVNAME_ONKEY "/input0";
+ onkey->input->dev.parent = &pdev->dev;
+
+ if (onkey->key_power)
+ input_set_capability(onkey->input, EV_KEY, KEY_POWER);
+ input_set_capability(onkey->input, EV_KEY, KEY_SLEEP);
+
+ ret = input_register_device(onkey->input);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to register input device.\n");
+ goto err_irq;
+ }
+
+ platform_set_drvdata(pdev, onkey);
+ return 0;
+
+err_irq:
+ free_irq(onkey->irq, onkey);
+ cancel_delayed_work_sync(&onkey->work);
+err:
+ return ret;
+}
+
+static int da9063_onkey_remove(struct platform_device *pdev)
+{
+ struct da9063_onkey *onkey = platform_get_drvdata(pdev);
+ free_irq(onkey->irq, onkey);
+ cancel_delayed_work_sync(&onkey->work);
+ input_unregister_device(onkey->input);
+ return 0;
+}
+
+static struct platform_driver da9063_onkey_driver = {
+ .probe = da9063_onkey_probe,
+ .remove = da9063_onkey_remove,
+ .driver = {
+ .name = DA9063_DRVNAME_ONKEY,
+ .owner = THIS_MODULE,
+ },
+};
+
+module_platform_driver(da9063_onkey_driver);
+
+MODULE_AUTHOR("S Twiss <stwiss.opensource@diasemi.com>");
+MODULE_DESCRIPTION("Onkey device driver for Dialog DA9063");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DA9063_DRVNAME_ONKEY);
diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
index e70ae31..b410a14 100644
--- a/drivers/mfd/da9063-core.c
+++ b/drivers/mfd/da9063-core.c
@@ -60,6 +60,7 @@ static struct resource da9063_rtc_resources[] = {
static struct resource da9063_onkey_resources[] = {
{
+ .name = "ONKEY",
.start = DA9063_IRQ_ONKEY,
.end = DA9063_IRQ_ONKEY,
.flags = IORESOURCE_IRQ,
diff --git a/include/linux/mfd/da9063/pdata.h b/include/linux/mfd/da9063/pdata.h
index 95c8742..612383b 100644
--- a/include/linux/mfd/da9063/pdata.h
+++ b/include/linux/mfd/da9063/pdata.h
@@ -103,6 +103,7 @@ struct da9063;
struct da9063_pdata {
int (*init)(struct da9063 *da9063);
int irq_base;
+ bool key_power;
unsigned flags;
struct da9063_regulators_pdata *regulators_pdata;
struct led_platform_data *leds_pdata;
--
end-of-patch for PATCH V2
^ permalink raw reply related
* RE: [PATCH V1 1/2] mfd: da9063: Linkages for ONKEY support in core files
From: Opensource [Steve Twiss] @ 2014-03-10 17:16 UTC (permalink / raw)
To: Lee Jones
Cc: Dmitry Torokhov, Samuel Ortiz, David Dajun Chen, LKML-INPUT,
LKML-KERNEL, Mark Brown, Philipp Zabel
In-Reply-To: <20140310164026.GB16697@lee--X1>
On 10 March 2014 16:40, Lee Jones wrote:
>> From: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
>>
>> This change adds the name field "ONKEY" to the properties of the
>> the OnKey IORESOURCE_IRQ resource structure (part of the mfd_cell
>> Onkey resource).
>>
>> The addition of bool key_power which will be passed to the onkey
>> driver and allow KEY_POWER support to be turned on/off as a
>> driver configuration option.
>>
>>
>> Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
>> ---
>> Checks performed with linux-next/next-20140307/scripts/checkpatch.pl
>> da9063-core.c total: 0 errors, 0 warnings, 189 lines checked
>> pdata.h total: 0 errors, 0 warnings, 112 lines checked
>>
>> This change is a dependency for the new DA9063 OnKey driver which
>> forms the remainder of this patch set.
>> This patch applies against kernel linux-next next-20140307
>>
>> Regards,
>> Steve Twiss, Dialog Semiconductor Ltd.
>>
>>
>>
>> drivers/mfd/da9063-core.c | 1 +
>> include/linux/mfd/da9063/pdata.h | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
>> index e70ae31..b410a14 100644
>> --- a/drivers/mfd/da9063-core.c
>> +++ b/drivers/mfd/da9063-core.c
>> @@ -60,6 +60,7 @@ static struct resource da9063_rtc_resources[] = {
>>
>> static struct resource da9063_onkey_resources[] = {
>> {
>> + .name = "ONKEY",
>> .start = DA9063_IRQ_ONKEY,
>> .end = DA9063_IRQ_ONKEY,
>> .flags = IORESOURCE_IRQ,
>> diff --git a/include/linux/mfd/da9063/pdata.h
>b/include/linux/mfd/da9063/pdata.h
>> index 95c8742..612383b 100644
>> --- a/include/linux/mfd/da9063/pdata.h
>> +++ b/include/linux/mfd/da9063/pdata.h
>> @@ -103,6 +103,7 @@ struct da9063;
>> struct da9063_pdata {
>> int (*init)(struct da9063 *da9063);
>> int irq_base;
>> + bool key_power;
>> unsigned flags;
>> struct da9063_regulators_pdata *regulators_pdata;
>> struct led_platform_data *leds_pdata;
>
>I'd like this change to be added by the first patch which makes use of
>it.
>
No problem,
I'll resent it as PATCH V2...
Regards,
Steve
^ permalink raw reply
* Re: [PATCH V1 1/2] mfd: da9063: Linkages for ONKEY support in core files
From: Lee Jones @ 2014-03-10 16:40 UTC (permalink / raw)
To: Opensource [Steve Twiss]
Cc: Dmitry Torokhov, Samuel Ortiz, David Dajun Chen, LKML-INPUT,
LKML-KERNEL, Mark Brown, Philipp Zabel
In-Reply-To: <f5a169e00565134fa856a3daa615b17253ce844e.1394468644.git.stwiss.opensource@diasemi.com>
> From: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
>
> This change adds the name field "ONKEY" to the properties of the
> the OnKey IORESOURCE_IRQ resource structure (part of the mfd_cell
> Onkey resource).
>
> The addition of bool key_power which will be passed to the onkey
> driver and allow KEY_POWER support to be turned on/off as a
> driver configuration option.
>
>
> Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
> ---
> Checks performed with linux-next/next-20140307/scripts/checkpatch.pl
> da9063-core.c total: 0 errors, 0 warnings, 189 lines checked
> pdata.h total: 0 errors, 0 warnings, 112 lines checked
>
> This change is a dependency for the new DA9063 OnKey driver which
> forms the remainder of this patch set.
> This patch applies against kernel linux-next next-20140307
>
> Regards,
> Steve Twiss, Dialog Semiconductor Ltd.
>
>
>
> drivers/mfd/da9063-core.c | 1 +
> include/linux/mfd/da9063/pdata.h | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index e70ae31..b410a14 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -60,6 +60,7 @@ static struct resource da9063_rtc_resources[] = {
>
> static struct resource da9063_onkey_resources[] = {
> {
> + .name = "ONKEY",
> .start = DA9063_IRQ_ONKEY,
> .end = DA9063_IRQ_ONKEY,
> .flags = IORESOURCE_IRQ,
> diff --git a/include/linux/mfd/da9063/pdata.h b/include/linux/mfd/da9063/pdata.h
> index 95c8742..612383b 100644
> --- a/include/linux/mfd/da9063/pdata.h
> +++ b/include/linux/mfd/da9063/pdata.h
> @@ -103,6 +103,7 @@ struct da9063;
> struct da9063_pdata {
> int (*init)(struct da9063 *da9063);
> int irq_base;
> + bool key_power;
> unsigned flags;
> struct da9063_regulators_pdata *regulators_pdata;
> struct led_platform_data *leds_pdata;
I'd like this change to be added by the first patch which makes use of
it.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v2 4/8] Input: pixcir_i2c_ts: Use Type-B Multi-Touch protocol
From: Felipe Balbi @ 2014-03-10 16:37 UTC (permalink / raw)
To: Roger Quadros
Cc: Henrik Rydberg, dmitry.torokhov, jcbian, balbi, dmurphy,
mugunthanvnm, linux-input, linux-kernel, devicetree
In-Reply-To: <531D7E66.1070300@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]
On Mon, Mar 10, 2014 at 10:57:10AM +0200, Roger Quadros wrote:
> Hi Henrik,
>
> On 03/08/2014 05:11 PM, Henrik Rydberg wrote:
> > Hi Roger,
> >
> > the MT implementation seems mostly fine, just one curiosity:
> >
> >> static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
> >> {
> >> struct pixcir_i2c_ts_data *tsdata = dev_id;
> >> const struct pixcir_ts_platform_data *pdata = tsdata->chip;
> >> + struct pixcir_report_data report;
> >>
> >> while (!tsdata->exiting) {
> >> - pixcir_ts_poscheck(tsdata);
> >> -
> >> - if (gpio_get_value(pdata->gpio_attb))
> >> + /* parse packet */
> >> + pixcir_ts_parse(tsdata, &report);
> >> +
> >> + /* report it */
> >> + pixcir_ts_report(tsdata, &report);
> >> +
> >> + if (gpio_get_value(pdata->gpio_attb)) {
> >> + if (report.num_touches) {
> >> + /*
> >> + * Last report with no finger up?
> >> + * Do it now then.
> >> + */
> >> + input_mt_sync_frame(tsdata->input);
> >> + input_sync(tsdata->input);
> >
> > Why is this special handling needed?
>
> This is needed because the controller doesn't always report when all fingers
> have left the screen. e.g. report might contain 3 fingers touched and then
> gpio_attb line is de-asserted. There's no report with 0 fingers touched even
> if the user's fingers have left the screen. So we never detect a BUTTON_UP.
>
> Without this s/w workaround we observe side effects like buttons being pressed
> but not released. To me it looks like a bug in the controller.
the other way would be to *also* use IRQF_TRIGGER_RISING, then you get
an IRQ when fingers leave the screen. No ?
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
From: Lee Jones @ 2014-03-10 16:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Gabriel FERNANDEZ, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
In-Reply-To: <20140310155824.GB29054@core.coreip.homeip.net>
> > > > > This patch adds ST Keyscan driver to use the keypad hw a subset
> > > > > of ST boards provide. Specific board setup will be put in the
> > > > > given dt.
> > > > >
> > > > > Signed-off-by: Giuseppe Condorelli <giuseppe.condorelli@st.com>
> > > > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> > > >
> > > > Are you sure these are in the correct order?
> > > >
> > > > What is the history of this commit?
> > > >
> > > > > ---
> > > > > .../devicetree/bindings/input/st-keypad.txt | 50 ++++
> > > >
> > > > This should be submitted as a seperate patch.
> > >
> > > Why do we have such requirement? To me it would make more sense to add
> > > binding documentation in the same commit as the code that uses these
> > > bindings.
> >
> > I'm inclined to agree with you and that's actually how we used to do
> > it, but a decision was made by the DT guys at one of the Kernel
> > Summits to submit Documentation as a separate patch.
>
> Do you have background for this decision? To me it is akin splitting
> header file into a separate patch.
The discussion/decision was verbal unfortunately.
I don't really mind either way. I'm just attempting to enforce the
decisions that were made by the forces-that-be. Perhaps ping
devicetree@vger.kernel.org with me in CC for more clarification if
required.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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
* [PATCH V1 0/2] da9063: Support for OnKey
From: Opensource [Steve Twiss] @ 2014-03-10 16:24 UTC (permalink / raw)
To: Dmitry Torokhov, Lee Jones, Samuel Ortiz
Cc: David Dajun Chen, LKML-INPUT, LKML-KERNEL, Mark Brown,
Philipp Zabel
From: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
This patch series adds support for the DA9063 OnKey.
It has been split into two parts because the OnKey driver
requires a minor alteration to the da9063-core.c file and
the platform data header file. These parts have been
separated out.
- Add a new OnKey driver for DA9063;
- Minor addition "ONKEY" name to OnKey IRQ resource structure;
Thank you,
Steve Twiss, Dialog Semiconductor Ltd.
S Twiss (2):
mfd: da9063: Linkages for ONKEY support in core files
input: misc: da9063: OnKey driver
drivers/input/misc/Kconfig | 10 ++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/da9063-onkey.c | 209 ++++++++++++++++++++++++++++++++++++++
drivers/mfd/da9063-core.c | 1 +
include/linux/mfd/da9063/pdata.h | 1 +
5 files changed, 222 insertions(+)
create mode 100644 drivers/input/misc/da9063-onkey.c
--
end-of-patch for PATCH V1
^ permalink raw reply
* [PATCH V1 1/2] mfd: da9063: Linkages for ONKEY support in core files
From: Opensource [Steve Twiss] @ 2014-03-10 16:24 UTC (permalink / raw)
To: Dmitry Torokhov, Lee Jones, Samuel Ortiz
Cc: David Dajun Chen, LKML-INPUT, LKML-KERNEL, Mark Brown,
Philipp Zabel
In-Reply-To: <cover.1394468644.git.stwiss.opensource@diasemi.com>
From: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
This change adds the name field "ONKEY" to the properties of the
the OnKey IORESOURCE_IRQ resource structure (part of the mfd_cell
Onkey resource).
The addition of bool key_power which will be passed to the onkey
driver and allow KEY_POWER support to be turned on/off as a
driver configuration option.
Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
---
Checks performed with linux-next/next-20140307/scripts/checkpatch.pl
da9063-core.c total: 0 errors, 0 warnings, 189 lines checked
pdata.h total: 0 errors, 0 warnings, 112 lines checked
This change is a dependency for the new DA9063 OnKey driver which
forms the remainder of this patch set.
This patch applies against kernel linux-next next-20140307
Regards,
Steve Twiss, Dialog Semiconductor Ltd.
drivers/mfd/da9063-core.c | 1 +
include/linux/mfd/da9063/pdata.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
index e70ae31..b410a14 100644
--- a/drivers/mfd/da9063-core.c
+++ b/drivers/mfd/da9063-core.c
@@ -60,6 +60,7 @@ static struct resource da9063_rtc_resources[] = {
static struct resource da9063_onkey_resources[] = {
{
+ .name = "ONKEY",
.start = DA9063_IRQ_ONKEY,
.end = DA9063_IRQ_ONKEY,
.flags = IORESOURCE_IRQ,
diff --git a/include/linux/mfd/da9063/pdata.h b/include/linux/mfd/da9063/pdata.h
index 95c8742..612383b 100644
--- a/include/linux/mfd/da9063/pdata.h
+++ b/include/linux/mfd/da9063/pdata.h
@@ -103,6 +103,7 @@ struct da9063;
struct da9063_pdata {
int (*init)(struct da9063 *da9063);
int irq_base;
+ bool key_power;
unsigned flags;
struct da9063_regulators_pdata *regulators_pdata;
struct led_platform_data *leds_pdata;
--
end-of-patch for PATCH V1
^ permalink raw reply related
* [PATCH V1 2/2] input: misc: da9063: OnKey driver
From: Opensource [Steve Twiss] @ 2014-03-10 16:24 UTC (permalink / raw)
To: Dmitry Torokhov, Lee Jones, Samuel Ortiz
Cc: David Dajun Chen, LKML-INPUT, LKML-KERNEL, Mark Brown,
Philipp Zabel
In-Reply-To: <cover.1394468644.git.stwiss.opensource@diasemi.com>
From: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
Add the OnKey driver for DA9063.
Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
---
Checks performed with linux-next/next-20140307/scripts/checkpatch.pl
da9063-onkey.c total: 0 errors, 0 warnings, 209 lines checked
Kconfig total: 0 errors, 11 warnings, 679 lines checked
Makefile total: 0 errors, 0 warnings, 66 lines checked
Hi Dmitry,
Thank you for your previous response to my RFC.
Here are my changes. I have implemented your suggestions. Please find my
explanations below.
Changes made to this driver since previous RFC V1:
- Several alterations to the previous patch have been made according
to the comments provided by Dmitry Torokhov in the previous RFC V1
e-mail thread, see here:
http://www.spinics.net/lists/linux-input/msg30171.html
- The use of booleans for boolean data (true/false);
- The addition of an extra input_sync() in between the two calls to
input_report_key(KEY_SLEEP);
- The removal of call to dev_err() because it was unnecessary;
- A fix for the race condition during driver remove() function:
"nothing stops IRQ from firing again and rescheduling the work item".
Also, the addition of a work cancelling function during the error
path of the driver probe() after the interrupt has been registered.
The error path of the probe() function has been refactored slightly
using goto statements to make things clearer.
This fix also required a change to the way the interrupt was
registered: probe() is now using request_threaded_irq() instead of
the devm_ equivalent so that an explicit call to free_irq() can be
done before any calls to cancel_delayed_work_sync() are made;
- A clarification to the way the da9063_poll_on() function should
handle the I2C failure error case.
The key report for KEY_POWER is now only made if there has been a
fully a successful update to the DA9063_NONKEY bit in the IRQ
mask. Otherwise it will continue to re-poll until the onkey's
IRQ mask has been modified successfully.
Dependencies:
This driver makes use of the name field "ONKEY" as part of the
function call: platform_get_irq_byname();
This driver is therefore dependent on this change to the name field
in the properties of the the OnKey IORESOURCE_IRQ resource structure
(part of the mfd_cell Onkey resource inside da9063-core.c). This
change comes as part of this patch set.
This patch applies against kernel version linux-next next-20140307
Regards,
Steve Twiss, Dialog Semiconductor Ltd.
drivers/input/misc/Kconfig | 10 ++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/da9063-onkey.c | 209 ++++++++++++++++++++++++++++++++++++++
3 files changed, 220 insertions(+)
create mode 100644 drivers/input/misc/da9063-onkey.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 762e6d2..3deb008 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -522,6 +522,16 @@ config INPUT_DA9055_ONKEY
To compile this driver as a module, choose M here: the module
will be called da9055_onkey.
+config INPUT_DA9063_ONKEY
+ tristate "Dialog DA9063 OnKey"
+ depends on MFD_DA9063
+ help
+ Support the ONKEY of Dialog DA9063 Power Management IC as an
+ input device reporting power button statue.
+
+ To compile this driver as a module, choose M here: the module
+ will be called da9063-onkey.
+
config INPUT_DM355EVM
tristate "TI DaVinci DM355 EVM Keypad and IR Remote"
depends on MFD_DM355EVM_MSP
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index cda71fc..f40caa7 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_INPUT_CMA3000_I2C) += cma3000_d0x_i2c.o
obj-$(CONFIG_INPUT_COBALT_BTNS) += cobalt_btns.o
obj-$(CONFIG_INPUT_DA9052_ONKEY) += da9052_onkey.o
obj-$(CONFIG_INPUT_DA9055_ONKEY) += da9055_onkey.o
+obj-$(CONFIG_INPUT_DA9063_ONKEY) += da9063-onkey.o
obj-$(CONFIG_INPUT_DM355EVM) += dm355evm_keys.o
obj-$(CONFIG_INPUT_GP2A) += gp2ap002a00f.o
obj-$(CONFIG_INPUT_GPIO_BEEPER) += gpio-beeper.o
diff --git a/drivers/input/misc/da9063-onkey.c b/drivers/input/misc/da9063-onkey.c
new file mode 100644
index 0000000..ce08954
--- /dev/null
+++ b/drivers/input/misc/da9063-onkey.c
@@ -0,0 +1,209 @@
+/* da9063-onkey.c - Onkey device driver for DA9063
+ * Copyright (C) 2013 Dialog Semiconductor Ltd.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Library General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/workqueue.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/da9063/core.h>
+#include <linux/mfd/da9063/pdata.h>
+#include <linux/mfd/da9063/registers.h>
+
+struct da9063_onkey {
+ struct da9063 *hw;
+ struct delayed_work work;
+ struct input_dev *input;
+ int irq;
+ bool key_power;
+};
+
+static void da9063_poll_on(struct work_struct *work)
+{
+ struct da9063_onkey *onkey = container_of(work, struct da9063_onkey,
+ work.work);
+ unsigned int val;
+ bool poll = true;
+ int ret;
+
+ /* poll to see when the pin is released */
+ ret = regmap_read(onkey->hw->regmap, DA9063_REG_STATUS_A, &val);
+ if (ret < 0) {
+ dev_err(&onkey->input->dev,
+ "Failed to read ON status: %d\n", ret);
+ goto err_poll;
+ }
+
+ if (!(val & DA9063_NONKEY)) {
+ ret = regmap_update_bits(onkey->hw->regmap,
+ DA9063_REG_CONTROL_B,
+ DA9063_NONKEY_LOCK, 0);
+ if (ret < 0) {
+ dev_err(&onkey->input->dev,
+ "Failed to reset the Key Delay %d\n", ret);
+ goto err_poll;
+ }
+
+ /* unmask the onkey interrupt again */
+ ret = regmap_update_bits(onkey->hw->regmap,
+ DA9063_REG_IRQ_MASK_A,
+ DA9063_NONKEY, 0);
+ if (ret < 0) {
+ dev_err(&onkey->input->dev,
+ "Failed to unmask the onkey IRQ: %d\n", ret);
+ goto err_poll;
+ }
+
+ input_report_key(onkey->input, KEY_POWER, 0);
+ input_sync(onkey->input);
+
+ poll = false;
+ }
+
+err_poll:
+ if (poll)
+ schedule_delayed_work(&onkey->work, 50);
+}
+
+static irqreturn_t da9063_onkey_irq_handler(int irq, void *data)
+{
+ struct da9063_onkey *onkey = data;
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(onkey->hw->regmap, DA9063_REG_STATUS_A, &val);
+ if (onkey->key_power && (ret >= 0) && (val & DA9063_NONKEY)) {
+ ret = regmap_update_bits(onkey->hw->regmap,
+ DA9063_REG_IRQ_MASK_A,
+ DA9063_NONKEY, 1);
+ if (ret < 0)
+ dev_err(&onkey->input->dev,
+ "Failed to mask the onkey IRQ: %d\n", ret);
+
+ input_report_key(onkey->input, KEY_POWER, 1);
+ input_sync(onkey->input);
+
+ schedule_delayed_work(&onkey->work, 0);
+ dev_dbg(&onkey->input->dev, "KEY_POWER pressed.\n");
+ } else {
+ input_report_key(onkey->input, KEY_SLEEP, 1);
+ input_sync(onkey->input);
+ input_report_key(onkey->input, KEY_SLEEP, 0);
+ input_sync(onkey->input);
+ dev_dbg(&onkey->input->dev, "KEY_SLEEP pressed.\n");
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int da9063_onkey_probe(struct platform_device *pdev)
+{
+ struct da9063 *da9063 = dev_get_drvdata(pdev->dev.parent);
+ struct da9063_pdata *pdata = dev_get_platdata(da9063->dev);
+ struct da9063_onkey *onkey;
+ bool kp_tmp = true;
+ int ret = 0;
+
+ if (pdata)
+ kp_tmp = pdata->key_power;
+
+ onkey = devm_kzalloc(&pdev->dev, sizeof(struct da9063_onkey),
+ GFP_KERNEL);
+ if (!onkey) {
+ dev_err(&pdev->dev, "Failed to allocate memory.\n");
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ INIT_DELAYED_WORK(&onkey->work, da9063_poll_on);
+
+ onkey->input = devm_input_allocate_device(&pdev->dev);
+ if (!onkey->input) {
+ dev_err(&pdev->dev, "Failed to allocated input device.\n");
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ ret = platform_get_irq_byname(pdev, "ONKEY");
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Failed to get platform IRQ.\n");
+ goto err;
+ }
+ onkey->irq = ret;
+
+ ret = request_threaded_irq(onkey->irq, NULL,
+ da9063_onkey_irq_handler,
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+ "ONKEY", onkey);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to request input device IRQ.\n");
+ goto err;
+ }
+
+ onkey->hw = da9063;
+ onkey->key_power = kp_tmp;
+ onkey->input->evbit[0] = BIT_MASK(EV_KEY);
+ onkey->input->name = DA9063_DRVNAME_ONKEY;
+ onkey->input->phys = DA9063_DRVNAME_ONKEY "/input0";
+ onkey->input->dev.parent = &pdev->dev;
+
+ if (onkey->key_power)
+ input_set_capability(onkey->input, EV_KEY, KEY_POWER);
+ input_set_capability(onkey->input, EV_KEY, KEY_SLEEP);
+
+ ret = input_register_device(onkey->input);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to register input device.\n");
+ goto err_irq;
+ }
+
+ platform_set_drvdata(pdev, onkey);
+ return 0;
+
+err_irq:
+ free_irq(onkey->irq, onkey);
+ cancel_delayed_work_sync(&onkey->work);
+err:
+ return ret;
+}
+
+static int da9063_onkey_remove(struct platform_device *pdev)
+{
+ struct da9063_onkey *onkey = platform_get_drvdata(pdev);
+ free_irq(onkey->irq, onkey);
+ cancel_delayed_work_sync(&onkey->work);
+ input_unregister_device(onkey->input);
+ return 0;
+}
+
+static struct platform_driver da9063_onkey_driver = {
+ .probe = da9063_onkey_probe,
+ .remove = da9063_onkey_remove,
+ .driver = {
+ .name = DA9063_DRVNAME_ONKEY,
+ .owner = THIS_MODULE,
+ },
+};
+
+module_platform_driver(da9063_onkey_driver);
+
+MODULE_AUTHOR("S Twiss <stwiss.opensource@diasemi.com>");
+MODULE_DESCRIPTION("Onkey device driver for Dialog DA9063");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DA9063_DRVNAME_ONKEY);
--
end-of-patch for PATCH V1
^ permalink raw reply related
* Re: [PATCH 05/05] input synaptics-rmi4: Add reflash support
From: Courtney Cavin @ 2014-03-10 16:24 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <1394245795-17347-5-git-send-email-cheiny@synaptics.com>
On Sat, Mar 08, 2014 at 03:29:55AM +0100, Christopher Heiny wrote:
> Add support for reflashing V5 bootloader firmwares into
> RMI4 devices.
Throughout this driver: I'm not sure of the name 'reflash'. Maybe just
'flash(ing)'?
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
>
> ---
>
> drivers/input/rmi4/Kconfig | 9 +
> drivers/input/rmi4/Makefile | 1 +
> drivers/input/rmi4/rmi_bus.c | 3 +
> drivers/input/rmi4/rmi_driver.h | 11 +
> drivers/input/rmi4/rmi_fw_update.c | 961 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 985 insertions(+)
>
> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
> index d0c7b6e..9b88b6a 100644
> --- a/drivers/input/rmi4/Kconfig
> +++ b/drivers/input/rmi4/Kconfig
> @@ -25,6 +25,15 @@ config RMI4_DEBUG
>
> If unsure, say N.
>
> +config RMI4_FWLIB
Err. LIB?
> + bool "RMI4 Firmware Update"
> + depends on RMI4_CORE
> + help
> + Say Y here to enable in-kernel firmware update capability.
> +
> + Add support to the RMI4 core to enable reflashing of device
> + firmware.
Please provide more description here of what someone is supposed to do
to utilize this support, and what it actually does. The term "update"
here is generic enough to cause some confusion.
> +
> config RMI4_I2C
> tristate "RMI4 I2C Support"
> depends on RMI4_CORE && I2C
> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
> index 5c6bad5..570ea80 100644
> --- a/drivers/input/rmi4/Makefile
> +++ b/drivers/input/rmi4/Makefile
> @@ -1,5 +1,6 @@
> obj-$(CONFIG_RMI4_CORE) += rmi_core.o
> rmi_core-y := rmi_bus.o rmi_driver.o rmi_f01.o
> +rmi_core-$(CONFIG_RMI4_FWLIB) += rmi_fw_update.o
Ok. Now I'm thoroughly confused, and I haven't even gotten to the code
yet. FWLIB => rmi_fw_update => rmi_reflash. What are we doing again?
>
> # Function drivers
> obj-$(CONFIG_RMI4_F11) += rmi_f11.o
> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> index 6e0454a..3c93d08 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
> @@ -117,6 +117,8 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
> if (error)
> goto err_put_device;
>
> + rmi_reflash_init(rmi_dev);
> +
> dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
> pdata->sensor_name, dev_name(&rmi_dev->dev));
>
> @@ -139,6 +141,7 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
> struct rmi_device *rmi_dev = xport->rmi_dev;
>
> device_del(&rmi_dev->dev);
> + rmi_reflash_cleanup(rmi_dev);
> rmi_physical_teardown_debugfs(rmi_dev);
> put_device(&rmi_dev->dev);
> }
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
[...]
> +#ifdef CONFIG_RMI4_FWLIB
> +void rmi_reflash_init(struct rmi_device *rmi_dev);
> +void rmi_reflash_cleanup(struct rmi_device *rmi_dev);
> +#else
> +#define rmi_reflash_init(rmi_dev)
> +#define rmi_reflash_cleanup(rmi_dev)
Please use static inline functions here. It helps the compiler tell
you when you do something wrong.
> +#endif
>
> #endif
> diff --git a/drivers/input/rmi4/rmi_fw_update.c b/drivers/input/rmi4/rmi_fw_update.c
[...]
> +struct rmi_reflash_data {
> + struct rmi_device *rmi_dev;
> + bool force;
> + ulong busy;
> + char name_buf[RMI_NAME_BUFFER_SIZE];
> + char *img_name;
const?
> + struct pdt_entry f01_pdt;
> + struct f01_basic_properties f01_props;
> + u8 device_status;
> + struct pdt_entry f34_pdt;
> + u8 bootloader_id[2];
> + struct rmi_f34_queries f34_queries;
> + u16 f34_status_address;
> + struct rmi_f34_control_status f34_controls;
> + const u8 *firmware_data;
> + const u8 *config_data;
> + struct work_struct reflash_work;
> +};
[...]
> +static int rmi_read_f01_status(struct rmi_reflash_data *data)
> +{
> + int retval;
> +
> + retval = rmi_read(data->rmi_dev, data->f01_pdt.data_base_addr,
> + &data->device_status);
> + if (retval)
> + return retval;
> +
> + return 0;
> +}
This function is used one time, just calls rmi_read... There's no need
to wrap this read.
[...]
> +static int rmi_wait_for_idle(struct rmi_reflash_data *data, int timeout_ms)
> +{
> + int timeout_count = ((timeout_ms * 1000) / RMI_MAX_SLEEP_TIME_US) + 1;
> + int count = 0;
> + struct rmi_f34_control_status *controls = &data->f34_controls;
> + int retval;
> +
> + do {
> + if (count || timeout_count == 1)
> + usleep_range(RMI_MIN_SLEEP_TIME_US,
> + RMI_MAX_SLEEP_TIME_US);
> + retval = rmi_read_f34_controls(data);
> + count++;
> + if (retval)
> + continue;
> + else if (IS_IDLE(controls)) {
> + if (dev_WARN_ONCE(&data->rmi_dev->dev,
> + !data->f34_controls.program_enabled,
> + "Reflash is idle but program_enabled bit isn't set.\n"
> + ))
> + /*
> + * This works around a bug in certain device
> + * firmwares, where the idle state is reached,
> + * but the program_enabled bit is not yet set.
> + */
If it's a bug in devices, but it's ok to try again as a workaround, is
there a good reason to print a stacktrace? Does the user care?
> + continue;
> + return 0;
> + }
> + } while (count < timeout_count);
> +
> + dev_err(&data->rmi_dev->dev,
> + "ERROR: Timeout waiting for idle status.\n");
> + dev_err(&data->rmi_dev->dev, "Command: %#04x\n", controls->command);
> + dev_err(&data->rmi_dev->dev, "Status: %#04x\n", controls->status);
> + dev_err(&data->rmi_dev->dev, "Enabled: %d\n",
> + controls->program_enabled);
> + dev_err(&data->rmi_dev->dev, "Idle: %d\n", IS_IDLE(controls));
> + return -ETIMEDOUT;
> +}
[...]
> +static int rmi_write_f34_command(struct rmi_reflash_data *data, u8 command)
> +{
> + int retval;
> + struct rmi_device *rmi_dev = data->rmi_dev;
> +
> + retval = rmi_write(rmi_dev, data->f34_status_address, command);
> + if (retval < 0) {
> + dev_err(&rmi_dev->dev,
> + "Failed to write F34 command %#04x. Code: %d.\n",
> + command, retval);
> + return retval;
> + }
> +
> + return 0;
> +}
This function is used three times, and calls one function without any
wrapping magic. You could easily call rmi_write in each place.
[...]
> +static void rmi_reset_device(struct rmi_reflash_data *data)
It really feels like this should have an error return.
> +{
> + int retval;
> + const struct rmi_device_platform_data *pdata =
> + rmi_get_platform_data(data->rmi_dev);
> +
> + dev_dbg(&data->rmi_dev->dev, "Resetting...\n");
> + retval = rmi_write(data->rmi_dev, data->f01_pdt.command_base_addr,
> + RMI_F01_CMD_DEVICE_RESET);
> + if (retval < 0)
> + dev_warn(&data->rmi_dev->dev,
> + "WARNING - post-flash reset failed, code: %d.\n",
> + retval);
> + msleep(pdata->reset_delay_ms ?: RMI_F01_DEFAULT_RESET_DELAY_MS);
> + dev_dbg(&data->rmi_dev->dev, "Reset completed.\n");
> +}
> +static int rmi_write_firmware(struct rmi_reflash_data *data)
> +{
> + return rmi_write_blocks(data, (u8 *) data->firmware_data,
> + data->f34_queries.fw_block_count, RMI_F34_WRITE_FW_BLOCK);
> +}
Called once.
> +
> +static int rmi_write_configuration(struct rmi_reflash_data *data)
> +{
> + return rmi_write_blocks(data, (u8 *) data->config_data,
> + data->f34_queries.config_block_count,
> + RMI_F34_WRITE_CONFIG_BLOCK);
> +}
Called once.
> +
> +static void rmi_reflash_firmware(struct rmi_reflash_data *data)
Also feels like this should have an error return.
[...]
> +static void rmi_fw_update(struct rmi_device *rmi_dev)
Same.
> +{
[...]
> + retval = rmi_read_f34_queries(data);
> + if (retval) {
> + dev_err(&rmi_dev->dev, "F34 queries failed, code = %d.\n",
> + retval);
> + goto done;
> + }
> + if (data->img_name && strlen(data->img_name))
data->img_name && data->img_name[0] ? No need to waste extra cycles.
> + snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
> + rmi_fw_name_format, data->img_name);
> + else if (pdata->firmware_name && strlen(pdata->firmware_name))
Same.
> + snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
> + rmi_fw_name_format, pdata->firmware_name);
> + else {
> + if (!strlen(data->f01_props.product_id)) {
Same.
> + dev_err(&rmi_dev->dev, "Product ID is missing or empty - will not reflash.\n");
> + goto done;
> + }
> + snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
> + rmi_fw_name_format, data->f01_props.product_id);
> + }
[...]
> + if (rmi_go_nogo(data, header)) {
> + dev_dbg(&rmi_dev->dev, "Go/NoGo said go.\n");
> + rmi_free_function_list(rmi_dev);
> + rmi_reflash_firmware(data);
> + rmi_reset_device(data);
> + rmi_driver_detect_functions(rmi_dev);
> + } else
> + dev_dbg(&rmi_dev->dev, "Go/NoGo said don't reflash.\n");
Documentation/CodingStyle says:
} else {
...
}
[...]
> +}
[...]
> +static ssize_t rmi_reflash_force_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct rmi_device *rmi_dev = to_rmi_device(dev);
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + struct rmi_reflash_data *data = drv_data->reflash_data;
> + int retval;
> + unsigned long val;
> +
> + if (test_and_set_bit(0, &data->busy))
> + return -EBUSY;
> +
> + retval = kstrtoul(buf, 10, &val);
> + if (retval)
> + count = retval;
> + else
> + data->force = !!val;
Hrm. Perhaps '42' doesn't make sense here. Maybe add:
else if (val > 1)
count = -EINVAL;
> +
> + clear_bit(0, &data->busy);
> +
> + return count;
> +}
> +
> +static ssize_t rmi_reflash_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct rmi_device *rmi_dev = to_rmi_device(dev);
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + struct rmi_reflash_data *data = drv_data->reflash_data;
> +
> + return snprintf(buf, PAGE_SIZE, "%u\n", test_bit(0, &data->busy));
> +}
> +
> +static ssize_t rmi_reflash_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int retval;
> + unsigned long val;
> + struct rmi_device *rmi_dev = to_rmi_device(dev);
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + struct rmi_reflash_data *data = drv_data->reflash_data;
> +
> + retval = kstrtoul(buf, 10, &val);
> + if (retval)
> + return retval;
> +
> + if (test_and_set_bit(0, &data->busy))
> + return -EBUSY;
> +
> + if (val)
> + /*
> + * TODO: Here we start a work thread to go do the reflash, but
> + * maybe we can just use request_firmware_timeout().
> + */
Mmm.. Yes. It would make the lifecycle of this busy bit much more
obvious. Otherwise perhaps call request_firmware_nowait() ? It already
does this work queue ... uh ... work for you.
> + schedule_work(&data->reflash_work);
> + else
> + clear_bit(0, &data->busy);
> +
> + return count;
> +}
[...]
> +void rmi_reflash_init(struct rmi_device *rmi_dev)
> +{
> + int error;
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + struct rmi_reflash_data *data;
> +
> + dev_dbg(&rmi_dev->dev, "%s called.\n", __func__);
> +
> + data = devm_kzalloc(&rmi_dev->dev, sizeof(struct rmi_reflash_data),
> + GFP_KERNEL);
The memory ownership here is odd. Maybe kzalloc, and just return that
data?
> +
> + error = sysfs_create_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
> + if (error) {
> + dev_warn(&rmi_dev->dev, "Failed to create reflash sysfs attributes.\n");
> + return;
> + }
> +
> + INIT_WORK(&data->reflash_work, rmi_reflash_work);
> + data->rmi_dev = rmi_dev;
> + drv_data->reflash_data = data;
> +}
> +
> +void rmi_reflash_cleanup(struct rmi_device *rmi_dev)
> +{
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + struct rmi_reflash_data *data = drv_data->reflash_data;
> +
> + sysfs_remove_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
> + devm_kfree(&rmi_dev->dev, data);
Who owns this memory again? devm_ doesn't seem right for this use-case.
> +}
-Courtney
^ 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