* Re: [PATCH 00/15] atmel_mxt_ts - device tree, bootloader, etc
From: Nick Dyer @ 2014-07-07 11:38 UTC (permalink / raw)
To: Sekhar Nori, Dmitry Torokhov
Cc: Yufeng Shen, Daniel Kurtz, Henrik Rydberg, Joonyoung Shim,
Alan Bowens, linux-input, linux-kernel, Peter Meerwald,
Benson Leung, Olof Johansson
In-Reply-To: <53BA82A4.5070104@ti.com>
On 07/07/14 12:21, Sekhar Nori wrote:
> I was unable to get the touchscreen working on my board after applying
> just these patches. It does work correctly with your for-next branch so
> I guess I need to wait for you to post the rest of your patches too.
>
> Here are the relevant messages at boot. Full boot log is available here
> (in case you want to have a look): http://paste.ubuntu.com/7759703/
>
> [ 2.315717] atmel_mxt_ts 0-004a: Direct firmware load failed with error -2
> [ 2.322949] atmel_mxt_ts 0-004a: Falling back to user helper
> [ 5.934924] atmel_mxt_ts 0-004a: Wait for completion timed out.
> [ 5.941237] atmel_mxt_ts 0-004a: Warning: Info CRC error - device=0x000000 file=0x8EE45C
> [ 7.294769] atmel_mxt_ts 0-004a: Wait for completion timed out.
> [ 7.300976] atmel_mxt_ts 0-004a: Resetting chip
> [ 10.574729] atmel_mxt_ts 0-004a: Wait for completion timed out.
> [ 10.581010] atmel_mxt_ts 0-004a: Error -110 updating config
> [ 10.626788] atmel_mxt_ts 0-004a: Family: 128 Variant: 1 Firmware V1.6.AB Objects: 17
>
> One key difference is that these patches try to load the config at
> probe where as with your -next branch that is avoided in the DT case.
> This is also missing the new update_cfg sysfs interface (which I guess
> you will post as follow-on patches).
>
> The wait_for_completion() times out because the interrupt never
> arrives. Even later when testing using evtest, I do not see interrupts
> coming. There are only two interrupts that arrive during boot and it
> stays that way. There is something going on with the way interrupts are
> handled. I havent debugged further yet. This problem is not there with
> your for-next branch.
Thanks for trying this, apologies that it didn't work.
Looking at it, I've a feeling that this is the solution for the issue you see:
https://github.com/ndyer/linux/commit/4e8f8d56361b09a2a
Although, the fact that it says the device is reporting the Info CRC to be
0x000000 is rather odd, as well.
It would be useful if you could turn on all the debug in the driver and
re-test (#define DEBUG 1 at the top). However, if you don't have time, I
will try and reproduce on my test system.
cheers
Nick
^ permalink raw reply
* Re: [PATCH] HID: cp2112: add I2C mode
From: Benjamin Tissoires @ 2014-07-07 13:57 UTC (permalink / raw)
To: Antonio Borneo
Cc: Jiri Kosina, linux-input, David Barksdale,
linux-kernel@vger.kernel.org
In-Reply-To: <1404022473-2862-1-git-send-email-borneo.antonio@gmail.com>
Hi Antonio,
On Sun, Jun 29, 2014 at 2:14 AM, Antonio Borneo
<borneo.antonio@gmail.com> wrote:
> cp2112 supports single I2C read/write transactions.
> It can't combine I2C transactions.
>
> Add master_xfer, using similar code flow as for smbus_xfer.
Thanks for taking the time to implement it. I wanted to add this
functionality, but did not found the time to finalize/submit the
patches.
So here is a review.
>
> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> ---
> drivers/hid/hid-cp2112.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 101 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 3952d90..1260a8a 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -429,6 +429,104 @@ static int cp2112_write_req(void *buf, u8 slave_address, u8 command, u8 *data,
> return data_length + 4;
> }
>
> +static int cp2112_i2c_write_req(void *buf, u8 slave_address, u8 *data,
> + u8 data_length)
> +{
> + struct cp2112_write_req_report *report = buf;
> +
> + if (data_length > sizeof(report->data))
> + return -EINVAL;
> +
> + report->report = CP2112_DATA_WRITE_REQUEST;
> + report->slave_address = slave_address << 1;
> + report->length = data_length;
> + memcpy(report->data, data, data_length);
> + return data_length + 3;
> +}
> +
> +static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
> + struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data;
> + struct hid_device *hdev = dev->hdev;
> + u8 buf[64];
> + ssize_t count;
> + unsigned int retries;
> + int ret;
> +
> + hid_dbg(hdev, "I2C %d messages\n", num);
> +
> + if (num != 1) {
> + hid_err(hdev,
> + "Multi-message I2C transactions not supported\n");
This is a shame. You can just externalize the following block (in pseudo code):
{ - read/write_req
- hid_output
- xfer_status
- read}
and then just use a for loop to call this sequence for each incoming message.
> + return -EOPNOTSUPP;
> + }
> +
> + if (msgs->flags & I2C_M_RD)
> + count = cp2112_read_req(buf, msgs->addr, msgs->len);
> + else
> + count = cp2112_i2c_write_req(buf, msgs->addr, msgs->buf,
> + msgs->len);
> +
> + if (count < 0)
> + return count;
> +
> + ret = hid_hw_power(hdev, PM_HINT_FULLON);
> + if (ret < 0) {
> + hid_err(hdev, "power management error: %d\n", ret);
> + return ret;
> + }
> +
> + ret = cp2112_hid_output(hdev, buf, count, HID_OUTPUT_REPORT);
> + if (ret < 0) {
> + hid_warn(hdev, "Error starting transaction: %d\n", ret);
> + goto power_normal;
> + }
> +
> + for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) {
> + ret = cp2112_xfer_status(dev);
> + if (-EBUSY == ret)
> + continue;
> + if (ret < 0)
> + goto power_normal;
> + break;
> + }
> +
> + if (XFER_STATUS_RETRIES <= retries) {
> + hid_warn(hdev, "Transfer timed out, cancelling.\n");
> + buf[0] = CP2112_CANCEL_TRANSFER;
> + buf[1] = 0x01;
> +
> + ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT);
> + if (ret < 0)
> + hid_warn(hdev, "Error cancelling transaction: %d\n",
> + ret);
> +
> + ret = -ETIMEDOUT;
> + goto power_normal;
> + }
> +
> + if (!(msgs->flags & I2C_M_RD)) {
> + ret = 0;
> + goto power_normal;
> + }
> +
> + ret = cp2112_read(dev, msgs->buf, msgs->len);
> + if (ret < 0)
> + goto power_normal;
> + if (ret != msgs->len) {
> + hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len);
> + ret = -EIO;
> + goto power_normal;
> + }
> +
> + ret = 0;
ret = num;
> +power_normal:
> + hid_hw_power(hdev, PM_HINT_NORMAL);
> + hid_dbg(hdev, "I2C transfer finished: %d\n", ret);
> + return (ret) ? ret : 1;
and "return ret;"
> +}
> +
> static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
> unsigned short flags, char read_write, u8 command,
> int size, union i2c_smbus_data *data)
> @@ -595,7 +693,8 @@ power_normal:
>
> static u32 cp2112_functionality(struct i2c_adapter *adap)
> {
> - return I2C_FUNC_SMBUS_BYTE |
> + return I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_BYTE |
> I2C_FUNC_SMBUS_BYTE_DATA |
> I2C_FUNC_SMBUS_WORD_DATA |
> I2C_FUNC_SMBUS_BLOCK_DATA |
> @@ -605,6 +704,7 @@ static u32 cp2112_functionality(struct i2c_adapter *adap)
> }
>
> static const struct i2c_algorithm smbus_algorithm = {
> + .master_xfer = cp2112_i2c_xfer,
> .smbus_xfer = cp2112_xfer,
> .functionality = cp2112_functionality,
> };
> --
> 2.0.1
>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] HID: cp2112: fix gpio value in gpio_direction_output
From: Benjamin Tissoires @ 2014-07-07 14:03 UTC (permalink / raw)
To: Antonio Borneo
Cc: Jiri Kosina, linux-input, David Barksdale,
linux-kernel@vger.kernel.org
In-Reply-To: <1404022428-2816-1-git-send-email-borneo.antonio@gmail.com>
On Sun, Jun 29, 2014 at 2:13 AM, Antonio Borneo
<borneo.antonio@gmail.com> wrote:
> CP2112 does not offer an atomic method to set both gpio
> direction and value.
> Also it does not permit to set gpio value before putting
> gpio in output. In fact, accordingly to Silicon Labs
> AN495, Rev. 0.2, cpt. 4.4, the HID report to set gpio
> values "does not affect any pins that are not configured
> as outputs".
>
> This is confirmed on evaluation board CP2112-EK.
> With current driver, after execute:
> echo in > /sys/class/gpio/gpio248/direction
> echo low > /sys/class/gpio/gpio248/direction
> gpio output is still high. Only after a following
> echo low > /sys/class/gpio/gpio248/direction
> gpio output gets low.
>
> Fix driver by changing order of operations; first set
> direction then set value.
>
> The drawback of this new sequence is that we can have
> a pulse on gpio pin when direction is changed from
> input to output-low, but this cannot be avoided on
> current CP2112.
In this case, does keeping the first cp2112_gpio_set() before setting
the output direction prevents the pulse?
If so, then you can just keep the current code, and simply add the
cp2112_gpio_set() at the end of cp2112_gpio_direction_output().
In both case, this is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
>
> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> ---
> drivers/hid/hid-cp2112.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 56be85a..3952d90 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -240,8 +240,6 @@ static int cp2112_gpio_direction_output(struct gpio_chip *chip,
> u8 buf[5];
> int ret;
>
> - cp2112_gpio_set(chip, offset, value);
> -
> ret = hid_hw_raw_request(hdev, CP2112_GPIO_CONFIG, buf,
> sizeof(buf), HID_FEATURE_REPORT,
> HID_REQ_GET_REPORT);
> @@ -260,6 +258,12 @@ static int cp2112_gpio_direction_output(struct gpio_chip *chip,
> return ret;
> }
>
> + /*
> + * Set gpio value when output direction is already set,
> + * as specified in AN495, Rev. 0.2, cpt. 4.4
> + */
> + cp2112_gpio_set(chip, offset, value);
> +
> return 0;
> }
>
> --
> 2.0.1
>
> --
> 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] HID: cp2112: fix gpio value in gpio_direction_output
From: Jiri Kosina @ 2014-07-07 15:07 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Antonio Borneo, linux-input, David Barksdale,
linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=GM9FqZJwJF0G41E9oWEjcWDeavd=kfZ9qcXvoAkmUdpA@mail.gmail.com>
On Mon, 7 Jul 2014, Benjamin Tissoires wrote:
> > CP2112 does not offer an atomic method to set both gpio
> > direction and value.
> > Also it does not permit to set gpio value before putting
> > gpio in output. In fact, accordingly to Silicon Labs
> > AN495, Rev. 0.2, cpt. 4.4, the HID report to set gpio
> > values "does not affect any pins that are not configured
> > as outputs".
> >
> > This is confirmed on evaluation board CP2112-EK.
> > With current driver, after execute:
> > echo in > /sys/class/gpio/gpio248/direction
> > echo low > /sys/class/gpio/gpio248/direction
> > gpio output is still high. Only after a following
> > echo low > /sys/class/gpio/gpio248/direction
> > gpio output gets low.
> >
> > Fix driver by changing order of operations; first set
> > direction then set value.
> >
> > The drawback of this new sequence is that we can have
> > a pulse on gpio pin when direction is changed from
> > input to output-low, but this cannot be avoided on
> > current CP2112.
>
> In this case, does keeping the first cp2112_gpio_set() before setting
> the output direction prevents the pulse?
> If so, then you can just keep the current code, and simply add the
> cp2112_gpio_set() at the end of cp2112_gpio_direction_output().
>
> In both case, this is:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
I am queuing this for 3.17, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: cp2112: add I2C mode
From: Antonio Borneo @ 2014-07-07 15:32 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, linux-input, David Barksdale,
linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=G-xqc5J8J8ndfL2d8SY3K4+d80LNQX7cgfUeA3ap=oYA@mail.gmail.com>
On Mon, Jul 7, 2014 at 9:57 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> Hi Antonio,
>
> On Sun, Jun 29, 2014 at 2:14 AM, Antonio Borneo
> <borneo.antonio@gmail.com> wrote:
>> cp2112 supports single I2C read/write transactions.
>> It can't combine I2C transactions.
>>
>> Add master_xfer, using similar code flow as for smbus_xfer.
>
> Thanks for taking the time to implement it. I wanted to add this
> functionality, but did not found the time to finalize/submit the
> patches.
> So here is a review.
Ciao Benjamin,
thanks for the review.
My comments inline below
>>
>> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
>> ---
>> drivers/hid/hid-cp2112.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 101 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
>> index 3952d90..1260a8a 100644
>> --- a/drivers/hid/hid-cp2112.c
>> +++ b/drivers/hid/hid-cp2112.c
>> @@ -429,6 +429,104 @@ static int cp2112_write_req(void *buf, u8 slave_address, u8 command, u8 *data,
>> return data_length + 4;
>> }
>>
>> +static int cp2112_i2c_write_req(void *buf, u8 slave_address, u8 *data,
>> + u8 data_length)
>> +{
>> + struct cp2112_write_req_report *report = buf;
>> +
>> + if (data_length > sizeof(report->data))
>> + return -EINVAL;
>> +
>> + report->report = CP2112_DATA_WRITE_REQUEST;
>> + report->slave_address = slave_address << 1;
>> + report->length = data_length;
>> + memcpy(report->data, data, data_length);
>> + return data_length + 3;
>> +}
>> +
>> +static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>> + int num)
>> +{
>> + struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data;
>> + struct hid_device *hdev = dev->hdev;
>> + u8 buf[64];
>> + ssize_t count;
>> + unsigned int retries;
>> + int ret;
>> +
>> + hid_dbg(hdev, "I2C %d messages\n", num);
>> +
>> + if (num != 1) {
>> + hid_err(hdev,
>> + "Multi-message I2C transactions not supported\n");
>
> This is a shame. You can just externalize the following block (in pseudo code):
> { - read/write_req
> - hid_output
> - xfer_status
> - read}
>
> and then just use a for loop to call this sequence for each incoming message.
The parameter "num" is for sending a "combined I2C transaction" in which
a series of messages is sent "without" stop-bits between the messages.
You can get full description searching for "i2c_transfer" in
- Documentation/i2c/writing-clients
- Documentation/i2c/i2c-protocol
Since CP2112 does not support this feature, I return EOPNOTSUPP as is done in
drivers/i2c/busses/i2c-powermac.c
If I implement the loop, as you suggest, CP2112 will just send a
sequence of independent messages, each with its own stop bit.
Probably the error message could be more explicit by mentioning the
word "combined".
I lazily copied the same message from i2c-powermac think it's clear enough.
>
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + if (msgs->flags & I2C_M_RD)
>> + count = cp2112_read_req(buf, msgs->addr, msgs->len);
>> + else
>> + count = cp2112_i2c_write_req(buf, msgs->addr, msgs->buf,
>> + msgs->len);
>> +
>> + if (count < 0)
>> + return count;
>> +
>> + ret = hid_hw_power(hdev, PM_HINT_FULLON);
>> + if (ret < 0) {
>> + hid_err(hdev, "power management error: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = cp2112_hid_output(hdev, buf, count, HID_OUTPUT_REPORT);
>> + if (ret < 0) {
>> + hid_warn(hdev, "Error starting transaction: %d\n", ret);
>> + goto power_normal;
>> + }
>> +
>> + for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) {
>> + ret = cp2112_xfer_status(dev);
>> + if (-EBUSY == ret)
>> + continue;
>> + if (ret < 0)
>> + goto power_normal;
>> + break;
>> + }
>> +
>> + if (XFER_STATUS_RETRIES <= retries) {
>> + hid_warn(hdev, "Transfer timed out, cancelling.\n");
>> + buf[0] = CP2112_CANCEL_TRANSFER;
>> + buf[1] = 0x01;
>> +
>> + ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT);
>> + if (ret < 0)
>> + hid_warn(hdev, "Error cancelling transaction: %d\n",
>> + ret);
>> +
>> + ret = -ETIMEDOUT;
>> + goto power_normal;
>> + }
>> +
>> + if (!(msgs->flags & I2C_M_RD)) {
>> + ret = 0;
>> + goto power_normal;
>> + }
>> +
>> + ret = cp2112_read(dev, msgs->buf, msgs->len);
>> + if (ret < 0)
>> + goto power_normal;
>> + if (ret != msgs->len) {
>> + hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len);
>> + ret = -EIO;
>> + goto power_normal;
>> + }
>> +
>> + ret = 0;
>
> ret = num;
>
>> +power_normal:
>> + hid_hw_power(hdev, PM_HINT_NORMAL);
>> + hid_dbg(hdev, "I2C transfer finished: %d\n", ret);
>> + return (ret) ? ret : 1;
>
> and "return ret;"
Not sure I catch what you mean, here. Please confirm my understanding.
>From a comment inside include/linux/i2c.h
/* master_xfer should return the number of messages successfully
processed, or a negative value on error */
In the code above I set "ret" to an error number or to zero.
Then return the error number or the number of messages (equal to 1, as
explained above).
Do you prefer I set "ret" directly to 1 instead of 0?
I can send a V2 for this.
In this case I would not change the debug message above that prints
"ret" and I will just print the current value for "ret".
Best Regards,
Antonio
>
>> +}
>> +
>> static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
>> unsigned short flags, char read_write, u8 command,
>> int size, union i2c_smbus_data *data)
>> @@ -595,7 +693,8 @@ power_normal:
>>
>> static u32 cp2112_functionality(struct i2c_adapter *adap)
>> {
>> - return I2C_FUNC_SMBUS_BYTE |
>> + return I2C_FUNC_I2C |
>> + I2C_FUNC_SMBUS_BYTE |
>> I2C_FUNC_SMBUS_BYTE_DATA |
>> I2C_FUNC_SMBUS_WORD_DATA |
>> I2C_FUNC_SMBUS_BLOCK_DATA |
>> @@ -605,6 +704,7 @@ static u32 cp2112_functionality(struct i2c_adapter *adap)
>> }
>>
>> static const struct i2c_algorithm smbus_algorithm = {
>> + .master_xfer = cp2112_i2c_xfer,
>> .smbus_xfer = cp2112_xfer,
>> .functionality = cp2112_functionality,
>> };
>> --
>> 2.0.1
>>
>
> Cheers,
> Benjamin
^ permalink raw reply
* Re: [PATCH] HID: cp2112: add I2C mode
From: Benjamin Tissoires @ 2014-07-07 15:52 UTC (permalink / raw)
To: Antonio Borneo
Cc: Jiri Kosina, linux-input, David Barksdale,
linux-kernel@vger.kernel.org
In-Reply-To: <CAAj6DX2fEqk-gBeismE2+9hbKE7ofES1p_a78trGKzaBjz_2Ow@mail.gmail.com>
On Mon, Jul 7, 2014 at 11:32 AM, Antonio Borneo
<borneo.antonio@gmail.com> wrote:
> On Mon, Jul 7, 2014 at 9:57 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> Hi Antonio,
>>
>> On Sun, Jun 29, 2014 at 2:14 AM, Antonio Borneo
>> <borneo.antonio@gmail.com> wrote:
>>> cp2112 supports single I2C read/write transactions.
>>> It can't combine I2C transactions.
>>>
>>> Add master_xfer, using similar code flow as for smbus_xfer.
>>
>> Thanks for taking the time to implement it. I wanted to add this
>> functionality, but did not found the time to finalize/submit the
>> patches.
>> So here is a review.
>
> Ciao Benjamin,
>
> thanks for the review.
> My comments inline below
>
>>>
>>> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
>>> ---
>>> drivers/hid/hid-cp2112.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 101 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
>>> index 3952d90..1260a8a 100644
>>> --- a/drivers/hid/hid-cp2112.c
>>> +++ b/drivers/hid/hid-cp2112.c
>>> @@ -429,6 +429,104 @@ static int cp2112_write_req(void *buf, u8 slave_address, u8 command, u8 *data,
>>> return data_length + 4;
>>> }
>>>
>>> +static int cp2112_i2c_write_req(void *buf, u8 slave_address, u8 *data,
>>> + u8 data_length)
>>> +{
>>> + struct cp2112_write_req_report *report = buf;
>>> +
>>> + if (data_length > sizeof(report->data))
>>> + return -EINVAL;
>>> +
>>> + report->report = CP2112_DATA_WRITE_REQUEST;
>>> + report->slave_address = slave_address << 1;
>>> + report->length = data_length;
>>> + memcpy(report->data, data, data_length);
>>> + return data_length + 3;
>>> +}
>>> +
>>> +static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>>> + int num)
>>> +{
>>> + struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data;
>>> + struct hid_device *hdev = dev->hdev;
>>> + u8 buf[64];
>>> + ssize_t count;
>>> + unsigned int retries;
>>> + int ret;
>>> +
>>> + hid_dbg(hdev, "I2C %d messages\n", num);
>>> +
>>> + if (num != 1) {
>>> + hid_err(hdev,
>>> + "Multi-message I2C transactions not supported\n");
>>
>> This is a shame. You can just externalize the following block (in pseudo code):
>> { - read/write_req
>> - hid_output
>> - xfer_status
>> - read}
>>
>> and then just use a for loop to call this sequence for each incoming message.
>
> The parameter "num" is for sending a "combined I2C transaction" in which
> a series of messages is sent "without" stop-bits between the messages.
> You can get full description searching for "i2c_transfer" in
> - Documentation/i2c/writing-clients
> - Documentation/i2c/i2c-protocol
Alright, this convince me.
> Since CP2112 does not support this feature, I return EOPNOTSUPP as is done in
> drivers/i2c/busses/i2c-powermac.c
>
> If I implement the loop, as you suggest, CP2112 will just send a
> sequence of independent messages, each with its own stop bit.
>
> Probably the error message could be more explicit by mentioning the
> word "combined".
> I lazily copied the same message from i2c-powermac think it's clear enough.
No, I think the error message is fine. I just thought that we could
"emulate" the combined transaction protocol by sending a bunch of
single transactions. Given that the protocol is explicit regarding the
stop, it is better not to emulate it and use the implementation you
are proposing.
>
>>
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + if (msgs->flags & I2C_M_RD)
>>> + count = cp2112_read_req(buf, msgs->addr, msgs->len);
>>> + else
>>> + count = cp2112_i2c_write_req(buf, msgs->addr, msgs->buf,
>>> + msgs->len);
>>> +
>>> + if (count < 0)
>>> + return count;
>>> +
>>> + ret = hid_hw_power(hdev, PM_HINT_FULLON);
>>> + if (ret < 0) {
>>> + hid_err(hdev, "power management error: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + ret = cp2112_hid_output(hdev, buf, count, HID_OUTPUT_REPORT);
>>> + if (ret < 0) {
>>> + hid_warn(hdev, "Error starting transaction: %d\n", ret);
>>> + goto power_normal;
>>> + }
>>> +
>>> + for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) {
>>> + ret = cp2112_xfer_status(dev);
>>> + if (-EBUSY == ret)
>>> + continue;
>>> + if (ret < 0)
>>> + goto power_normal;
>>> + break;
>>> + }
>>> +
>>> + if (XFER_STATUS_RETRIES <= retries) {
>>> + hid_warn(hdev, "Transfer timed out, cancelling.\n");
>>> + buf[0] = CP2112_CANCEL_TRANSFER;
>>> + buf[1] = 0x01;
>>> +
>>> + ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT);
>>> + if (ret < 0)
>>> + hid_warn(hdev, "Error cancelling transaction: %d\n",
>>> + ret);
>>> +
>>> + ret = -ETIMEDOUT;
>>> + goto power_normal;
>>> + }
>>> +
>>> + if (!(msgs->flags & I2C_M_RD)) {
>>> + ret = 0;
>>> + goto power_normal;
>>> + }
>>> +
>>> + ret = cp2112_read(dev, msgs->buf, msgs->len);
>>> + if (ret < 0)
>>> + goto power_normal;
>>> + if (ret != msgs->len) {
>>> + hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len);
>>> + ret = -EIO;
>>> + goto power_normal;
>>> + }
>>> +
>>> + ret = 0;
>>
>> ret = num;
>>
>>> +power_normal:
>>> + hid_hw_power(hdev, PM_HINT_NORMAL);
>>> + hid_dbg(hdev, "I2C transfer finished: %d\n", ret);
>>> + return (ret) ? ret : 1;
>>
>> and "return ret;"
>
> Not sure I catch what you mean, here. Please confirm my understanding.
>
> From a comment inside include/linux/i2c.h
> /* master_xfer should return the number of messages successfully
> processed, or a negative value on error */
>
> In the code above I set "ret" to an error number or to zero.
> Then return the error number or the number of messages (equal to 1, as
> explained above).
>
> Do you prefer I set "ret" directly to 1 instead of 0?
Yep, just set ret to 1 instead of 0 (both at the end and right after
the I2C_M_RD test). Then, ret will contain exactly either the number
of messages processed, or the error.
>
> I can send a V2 for this.
> In this case I would not change the debug message above that prints
> "ret" and I will just print the current value for "ret".
Yes, IMO, that makes more sense to print the exit value instead of
changing it after.
If you just do this small nitpicks in the V2, you can add my
"Reviewed-by: Benjamin TIssoires <benjamin.tissoires@redhat.com>"
directly in your submission.
Cheers,
Benjamin
>
> Best Regards,
> Antonio
>
>>
>>> +}
>>> +
>>> static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
>>> unsigned short flags, char read_write, u8 command,
>>> int size, union i2c_smbus_data *data)
>>> @@ -595,7 +693,8 @@ power_normal:
>>>
>>> static u32 cp2112_functionality(struct i2c_adapter *adap)
>>> {
>>> - return I2C_FUNC_SMBUS_BYTE |
>>> + return I2C_FUNC_I2C |
>>> + I2C_FUNC_SMBUS_BYTE |
>>> I2C_FUNC_SMBUS_BYTE_DATA |
>>> I2C_FUNC_SMBUS_WORD_DATA |
>>> I2C_FUNC_SMBUS_BLOCK_DATA |
>>> @@ -605,6 +704,7 @@ static u32 cp2112_functionality(struct i2c_adapter *adap)
>>> }
>>>
>>> static const struct i2c_algorithm smbus_algorithm = {
>>> + .master_xfer = cp2112_i2c_xfer,
>>> .smbus_xfer = cp2112_xfer,
>>> .functionality = cp2112_functionality,
>>> };
>>> --
>>> 2.0.1
>>>
>>
>> Cheers,
>> Benjamin
^ permalink raw reply
* Re: [PATCH] HID: cp2112: fix gpio value in gpio_direction_output
From: Antonio Borneo @ 2014-07-07 16:01 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, linux-input, David Barksdale,
linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=GM9FqZJwJF0G41E9oWEjcWDeavd=kfZ9qcXvoAkmUdpA@mail.gmail.com>
On Mon, Jul 7, 2014 at 10:03 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Sun, Jun 29, 2014 at 2:13 AM, Antonio Borneo
> <borneo.antonio@gmail.com> wrote:
>> CP2112 does not offer an atomic method to set both gpio
>> direction and value.
>> Also it does not permit to set gpio value before putting
>> gpio in output. In fact, accordingly to Silicon Labs
>> AN495, Rev. 0.2, cpt. 4.4, the HID report to set gpio
>> values "does not affect any pins that are not configured
>> as outputs".
>>
>> This is confirmed on evaluation board CP2112-EK.
>> With current driver, after execute:
>> echo in > /sys/class/gpio/gpio248/direction
>> echo low > /sys/class/gpio/gpio248/direction
>> gpio output is still high. Only after a following
>> echo low > /sys/class/gpio/gpio248/direction
>> gpio output gets low.
>>
>> Fix driver by changing order of operations; first set
>> direction then set value.
>>
>> The drawback of this new sequence is that we can have
>> a pulse on gpio pin when direction is changed from
>> input to output-low, but this cannot be avoided on
>> current CP2112.
>
> In this case, does keeping the first cp2112_gpio_set() before setting
> the output direction prevents the pulse?
> If so, then you can just keep the current code, and simply add the
> cp2112_gpio_set() at the end of cp2112_gpio_direction_output().
Hi Benjamin,
unfortunately this would not prevent the pulse.
In fact:
a) if gpio is already output
- the first cp2112_gpio_set() will set the value
- then cp2112_gpio_direction_output() would be redundant
- finally the second cp2112_gpio_set() would be redundant too.
b) if gpio is input
- the first cp2112_gpio_set() would be ignored, as explained in AN495
- then cp2112_gpio_direction_output() would put gpio in output.
Accordingly to my experiments the value is set high
- finally the second cp2112_gpio_set() would set the proper value. If
value is different from what is set by cp2112_gpio_direction_output()
we get a pulse.
That's why I removed the first cp2112_gpio_set() or, better, I moved
it after setting gpio direction.
Thanks for the review.
Antonio
>
> In both case, this is:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Cheers,
> Benjamin
>
>>
>> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
>> ---
>> drivers/hid/hid-cp2112.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
>> index 56be85a..3952d90 100644
>> --- a/drivers/hid/hid-cp2112.c
>> +++ b/drivers/hid/hid-cp2112.c
>> @@ -240,8 +240,6 @@ static int cp2112_gpio_direction_output(struct gpio_chip *chip,
>> u8 buf[5];
>> int ret;
>>
>> - cp2112_gpio_set(chip, offset, value);
>> -
>> ret = hid_hw_raw_request(hdev, CP2112_GPIO_CONFIG, buf,
>> sizeof(buf), HID_FEATURE_REPORT,
>> HID_REQ_GET_REPORT);
>> @@ -260,6 +258,12 @@ static int cp2112_gpio_direction_output(struct gpio_chip *chip,
>> return ret;
>> }
>>
>> + /*
>> + * Set gpio value when output direction is already set,
>> + * as specified in AN495, Rev. 0.2, cpt. 4.4
>> + */
>> + cp2112_gpio_set(chip, offset, value);
>> +
>> return 0;
>> }
>>
>> --
>> 2.0.1
>>
>> --
>> 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 v2] HID: cp2112: add I2C mode
From: Antonio Borneo @ 2014-07-07 23:25 UTC (permalink / raw)
To: Jiri Kosina, linux-input
Cc: David Barksdale, linux-kernel, Benjamin Tissoires, Antonio Borneo
In-Reply-To: <CAN+gG=F2wyFw0mR2RxSEz_RNPp3a+zr0R3an38NNU4NVDSYFMQ@mail.gmail.com>
cp2112 supports single I2C read/write transactions.
It can't combine I2C transactions.
Add master_xfer, using similar code flow as for smbus_xfer.
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
ChangeLog v1->v2:
- In cp2112_i2c_xfer() set variable "ret" directly with the
expected return value: either an error number or the number
of transferred messages.
---
drivers/hid/hid-cp2112.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 102 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 3952d90..a822db5 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -429,6 +429,105 @@ static int cp2112_write_req(void *buf, u8 slave_address, u8 command, u8 *data,
return data_length + 4;
}
+static int cp2112_i2c_write_req(void *buf, u8 slave_address, u8 *data,
+ u8 data_length)
+{
+ struct cp2112_write_req_report *report = buf;
+
+ if (data_length > sizeof(report->data))
+ return -EINVAL;
+
+ report->report = CP2112_DATA_WRITE_REQUEST;
+ report->slave_address = slave_address << 1;
+ report->length = data_length;
+ memcpy(report->data, data, data_length);
+ return data_length + 3;
+}
+
+static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+ int num)
+{
+ struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data;
+ struct hid_device *hdev = dev->hdev;
+ u8 buf[64];
+ ssize_t count;
+ unsigned int retries;
+ int ret;
+
+ hid_dbg(hdev, "I2C %d messages\n", num);
+
+ if (num != 1) {
+ hid_err(hdev,
+ "Multi-message I2C transactions not supported\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (msgs->flags & I2C_M_RD)
+ count = cp2112_read_req(buf, msgs->addr, msgs->len);
+ else
+ count = cp2112_i2c_write_req(buf, msgs->addr, msgs->buf,
+ msgs->len);
+
+ if (count < 0)
+ return count;
+
+ ret = hid_hw_power(hdev, PM_HINT_FULLON);
+ if (ret < 0) {
+ hid_err(hdev, "power management error: %d\n", ret);
+ return ret;
+ }
+
+ ret = cp2112_hid_output(hdev, buf, count, HID_OUTPUT_REPORT);
+ if (ret < 0) {
+ hid_warn(hdev, "Error starting transaction: %d\n", ret);
+ goto power_normal;
+ }
+
+ for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) {
+ ret = cp2112_xfer_status(dev);
+ if (-EBUSY == ret)
+ continue;
+ if (ret < 0)
+ goto power_normal;
+ break;
+ }
+
+ if (XFER_STATUS_RETRIES <= retries) {
+ hid_warn(hdev, "Transfer timed out, cancelling.\n");
+ buf[0] = CP2112_CANCEL_TRANSFER;
+ buf[1] = 0x01;
+
+ ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT);
+ if (ret < 0)
+ hid_warn(hdev, "Error cancelling transaction: %d\n",
+ ret);
+
+ ret = -ETIMEDOUT;
+ goto power_normal;
+ }
+
+ if (!(msgs->flags & I2C_M_RD))
+ goto finish;
+
+ ret = cp2112_read(dev, msgs->buf, msgs->len);
+ if (ret < 0)
+ goto power_normal;
+ if (ret != msgs->len) {
+ hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len);
+ ret = -EIO;
+ goto power_normal;
+ }
+
+finish:
+ /* return the number of transferred messages */
+ ret = 1;
+
+power_normal:
+ hid_hw_power(hdev, PM_HINT_NORMAL);
+ hid_dbg(hdev, "I2C transfer finished: %d\n", ret);
+ return ret;
+}
+
static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
unsigned short flags, char read_write, u8 command,
int size, union i2c_smbus_data *data)
@@ -595,7 +694,8 @@ power_normal:
static u32 cp2112_functionality(struct i2c_adapter *adap)
{
- return I2C_FUNC_SMBUS_BYTE |
+ return I2C_FUNC_I2C |
+ I2C_FUNC_SMBUS_BYTE |
I2C_FUNC_SMBUS_BYTE_DATA |
I2C_FUNC_SMBUS_WORD_DATA |
I2C_FUNC_SMBUS_BLOCK_DATA |
@@ -605,6 +705,7 @@ static u32 cp2112_functionality(struct i2c_adapter *adap)
}
static const struct i2c_algorithm smbus_algorithm = {
+ .master_xfer = cp2112_i2c_xfer,
.smbus_xfer = cp2112_xfer,
.functionality = cp2112_functionality,
};
--
2.0.1
^ permalink raw reply related
* (unknown)
From: Ms Teresa Au @ 2014-07-06 11:42 UTC (permalink / raw)
--
please can you help me re-profile fund? 100% legit contact me personal
email: Teresa_Au@yeah.net
^ permalink raw reply
* [PATCH] Input: st-keyscan - Fix 'defined but not used' compiler warnings
From: Tobias Klauser @ 2014-07-08 8:47 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Geert Uytterhoeven, linux-input
Add #ifdef CONFIG_PM_SLEEP around keyscan_supend() and keyscan_resume() to fix
the following compiler warnings occuring if CONFIG_PM_SLEEP is unset:
+ /scratch/kisskb/src/drivers/input/keyboard/st-keyscan.c: warning: 'keyscan_resume' defined but not used [-Wunused-function]: => 235:12
+ /scratch/kisskb/src/drivers/input/keyboard/st-keyscan.c: warning: 'keyscan_suspend' defined but not used [-Wunused-function]: => 218:12
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Link: https://lkml.org/lkml/2014/7/8/109
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
drivers/input/keyboard/st-keyscan.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c
index 758b487..de7be4f 100644
--- a/drivers/input/keyboard/st-keyscan.c
+++ b/drivers/input/keyboard/st-keyscan.c
@@ -215,6 +215,7 @@ static int keyscan_probe(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_PM_SLEEP
static int keyscan_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -249,6 +250,7 @@ static int keyscan_resume(struct device *dev)
mutex_unlock(&input->mutex);
return retval;
}
+#endif
static SIMPLE_DEV_PM_OPS(keyscan_dev_pm_ops, keyscan_suspend, keyscan_resume);
--
2.0.1
^ permalink raw reply related
* Re: [PATCH] Input: s3c2410_ts: Move to clk_prepare_enable/clk_disable_unprepare
From: Tomasz Figa @ 2014-07-08 11:50 UTC (permalink / raw)
To: Vasily Khoruzhick, Ben Dooks, Kukjin Kim, Dmitry Torokhov,
linux-arm-kernel, linux-samsung-soc, linux-input
In-Reply-To: <1404155377-2862-1-git-send-email-anarsoul@gmail.com>
Hi Vasily,
On 30.06.2014 21:09, Vasily Khoruzhick wrote:
> Use clk_prepare_enable/clk_disable_unprepare to make the driver
> work properly with common clock framework.
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> drivers/input/touchscreen/s3c2410_ts.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
--
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 00/15] atmel_mxt_ts - device tree, bootloader, etc
From: Sekhar Nori @ 2014-07-08 12:28 UTC (permalink / raw)
To: Nick Dyer, Dmitry Torokhov
Cc: Yufeng Shen, Daniel Kurtz, Henrik Rydberg, Joonyoung Shim,
Alan Bowens, linux-input, linux-kernel, Peter Meerwald,
Benson Leung, Olof Johansson
In-Reply-To: <53BA86AB.8050603@itdev.co.uk>
On Monday 07 July 2014 05:08 PM, Nick Dyer wrote:
> On 07/07/14 12:21, Sekhar Nori wrote:
>> I was unable to get the touchscreen working on my board after applying
>> just these patches. It does work correctly with your for-next branch so
>> I guess I need to wait for you to post the rest of your patches too.
>>
>> Here are the relevant messages at boot. Full boot log is available here
>> (in case you want to have a look): http://paste.ubuntu.com/7759703/
>>
>> [ 2.315717] atmel_mxt_ts 0-004a: Direct firmware load failed with error -2
>> [ 2.322949] atmel_mxt_ts 0-004a: Falling back to user helper
>> [ 5.934924] atmel_mxt_ts 0-004a: Wait for completion timed out.
>> [ 5.941237] atmel_mxt_ts 0-004a: Warning: Info CRC error - device=0x000000 file=0x8EE45C
>> [ 7.294769] atmel_mxt_ts 0-004a: Wait for completion timed out.
>> [ 7.300976] atmel_mxt_ts 0-004a: Resetting chip
>> [ 10.574729] atmel_mxt_ts 0-004a: Wait for completion timed out.
>> [ 10.581010] atmel_mxt_ts 0-004a: Error -110 updating config
>> [ 10.626788] atmel_mxt_ts 0-004a: Family: 128 Variant: 1 Firmware V1.6.AB Objects: 17
>>
>> One key difference is that these patches try to load the config at
>> probe where as with your -next branch that is avoided in the DT case.
>> This is also missing the new update_cfg sysfs interface (which I guess
>> you will post as follow-on patches).
>>
>> The wait_for_completion() times out because the interrupt never
>> arrives. Even later when testing using evtest, I do not see interrupts
>> coming. There are only two interrupts that arrive during boot and it
>> stays that way. There is something going on with the way interrupts are
>> handled. I havent debugged further yet. This problem is not there with
>> your for-next branch.
>
> Thanks for trying this, apologies that it didn't work.
>
> Looking at it, I've a feeling that this is the solution for the issue you see:
> https://github.com/ndyer/linux/commit/4e8f8d56361b09a2a
Okay, I rebased that patch onto this series, but it did not solve the
problem.
>
> Although, the fact that it says the device is reporting the Info CRC to be
> 0x000000 is rather odd, as well.
Thats because the device was not configured at factory (I was using the
board for the first time). Once I run the kernel from your for-next branch,
this messages disappear since I guess the configuration is successfully
flashed by that kernel.
> It would be useful if you could turn on all the debug in the driver and
> re-test (#define DEBUG 1 at the top). However, if you don't have time, I
> will try and reproduce on my test system.
Here are the messages I see:
[ 2.257611] atmel_mxt_ts 0-004a: T5 Start:242 Size:9 Instances:1 Report IDs:0-0
[ 2.265261] atmel_mxt_ts 0-004a: T6 Start:251 Size:6 Instances:1 Report IDs:1-1
[ 2.272966] atmel_mxt_ts 0-004a: T38 Start:257 Size:8 Instances:1 Report IDs:0-0
[ 2.280726] atmel_mxt_ts 0-004a: T7 Start:265 Size:3 Instances:1 Report IDs:0-0
[ 2.288389] atmel_mxt_ts 0-004a: T8 Start:268 Size:8 Instances:1 Report IDs:0-0
[ 2.296045] atmel_mxt_ts 0-004a: T9 Start:276 Size:31 Instances:1 Report IDs:2-11
[ 2.303905] atmel_mxt_ts 0-004a: T15 Start:307 Size:11 Instances:1 Report IDs:12-12
[ 2.311935] atmel_mxt_ts 0-004a: T18 Start:318 Size:2 Instances:1 Report IDs:0-0
[ 2.319695] atmel_mxt_ts 0-004a: T19 Start:320 Size:16 Instances:1 Report IDs:13-13
[ 2.327723] atmel_mxt_ts 0-004a: T20 Start:336 Size:12 Instances:1 Report IDs:14-14
[ 2.335732] atmel_mxt_ts 0-004a: T22 Start:348 Size:17 Instances:1 Report IDs:15-15
[ 2.343761] atmel_mxt_ts 0-004a: T23 Start:365 Size:15 Instances:1 Report IDs:16-16
[ 2.351789] atmel_mxt_ts 0-004a: T24 Start:380 Size:19 Instances:1 Report IDs:17-20
[ 2.359812] atmel_mxt_ts 0-004a: T25 Start:399 Size:14 Instances:1 Report IDs:21-21
[ 2.367844] atmel_mxt_ts 0-004a: T27 Start:413 Size:7 Instances:1 Report IDs:22-22
[ 2.375763] atmel_mxt_ts 0-004a: T28 Start:420 Size:6 Instances:1 Report IDs:23-23
[ 2.383705] atmel_mxt_ts 0-004a: T37 Start:112 Size:130 Instances:1 Report IDs:0-0
[ 2.401277] atmel_mxt_ts 0-004a: T6 Config Checksum: 0x6BC1AB
[ 2.407301] atmel_mxt_ts 0-004a: T6 Status 0x90 RESET CAL
[ 2.413026] atmel_mxt_ts 0-004a: T6 Status 0x00 OK
[ 2.424854] atmel_mxt_ts 0-004a: Direct firmware load failed with error -2
[ 2.462113] atmel_mxt_ts 0-004a: Falling back to user helper
[ 5.698524] atmel_mxt_ts 0-004a: Initialized power cfg: ACTV 255, IDLE 32
[ 5.712014] atmel_mxt_ts 0-004a: Touchscreen size X799Y479
[ 5.742180] atmel_mxt_ts 0-004a: Family: 128 Variant: 1 Firmware V1.6.AB Objects: 17
Full testing log here: http://paste.ubuntu.com/7765352/
Thanks,
Sekhar
^ permalink raw reply
* Re: [PATCH 0/2] synaptics_usb trackpoint cursor movement improvements
From: Rasmus Villemoes @ 2014-07-08 12:43 UTC (permalink / raw)
To: Andrew Deason, Dmitry Torokhov; +Cc: linux-input
In-Reply-To: <20130915160134.cd2eb260d8a5703a78ebfe84@dson.org>
Andrew Deason <adeason@dson.org> writes:
> Also, I've been testing these changes with my own devices just by
> myself; I would welcome feedback from others with similar devices if any
> of them see this. It's difficult to objectively test how the trackpoint
> "feel"s with different parameters.
Hi Andrew,
I ran into the same issue after upgrading to Ubuntu 14.04 (with kernel
3.13); found <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=682413>,
and now finally got around to compile a kernel with your patches [1],[2]
applied. I can confirm that the "feel" of the trackpoint is back to
normal.
Also, the various workarounds mentioned at
http://unix.stackexchange.com/questions/43430 do not work nearly as good
(and of course, it would be much better if the trackpoint "just worked").
[0] http://thread.gmane.org/gmane.linux.kernel.input/31934
[1] http://thread.gmane.org/gmane.linux.kernel.input/31935
[2] http://thread.gmane.org/gmane.linux.kernel.input/31936
I have no idea whether those patches are the "right" fix, but FWIW, you
can add 'Tested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>'.
Thanks,
Rasmus
^ permalink raw reply
* [PATCH 1/1] input: remove unnecessary break after goto
From: Fabian Frederick @ 2014-07-08 16:37 UTC (permalink / raw)
To: linux-kernel
Cc: Fabian Frederick, Dmitry Torokhov, Paul Gortmaker, linux-input
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: linux-input@vger.kernel.org
Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
drivers/input/misc/keyspan_remote.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/input/misc/keyspan_remote.c b/drivers/input/misc/keyspan_remote.c
index 01f3b5b..a3fe4a9 100644
--- a/drivers/input/misc/keyspan_remote.c
+++ b/drivers/input/misc/keyspan_remote.c
@@ -392,7 +392,6 @@ static void keyspan_irq_recv(struct urb *urb)
default:
goto resubmit;
- break;
}
if (debug)
--
1.8.4.5
^ permalink raw reply related
* Re: [PATCH] soc_button_array: fix the issue that button device can't be enumerated since 3.16-rc1
From: Rafael J. Wysocki @ 2014-07-08 20:50 UTC (permalink / raw)
To: Zhang Rui; +Cc: dmitry.torokhov, yao.jin, linux-input
In-Reply-To: <1404309713.8366.94.camel@rzhang1-toshiba>
On Wednesday, July 02, 2014 10:01:53 PM Zhang Rui wrote:
> From c2ee1886ba230d9d93d2ea2f350b1dc1a2d5ead5 Mon Sep 17 00:00:00 2001
> From: Jin Yao <yao.jin@linux.intel.com>
> Date: Thu, 26 Jun 2014 10:26:44 +0800
> Subject: [PATCH] soc_button_array: fix the issue that button device can't be
> enumerated since 3.16-rc1
Hi Rui,
For 3.16 I'm afraid we need to add the missing device ID to the PNP list.
It is too late to do the conversion at this point IMO and we can do it later.
> ACPI device enumeration mechanism changed a lot since 3.16-rc1.
> ACPI device objects with _HID will be enumerated to platform bus by default.
> For the existing PNP drivers that probe the PNPACPI devices, the device ids
> are listed explicitly in drivers/acpi/acpi_pnp.c. And ACPI folks will
> continue their effort on shrinking this id list by converting the PNP drivers
> to platform drivers.
> But unfortunately, soc_button_array device id, aka, "PNP0C40", is missing
> during this transition.
>
> Thus soc_button_array driver fails to probe any device since 3.16-rc1 because
> the device is enumerated to platform bus instead.
>
> A simply fix is to add device id PNP0C40 back to drivers/acpi/acpi_pnp.c,
> but given that the soc_button_array driver will be converted to platform
> driver sooner or later, and the id will be removed from the list again,
> it is better to fix the problem in one time.
>
> This patch fixes the problem by converting the soc_button_driver
> from PNP bus to platform bus.
>
> Signed-off-by: Jin Yao <yao.jin@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
> drivers/input/misc/soc_button_array.c | 60 ++++++++++++++++++-----------------
> 1 file changed, 31 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
> index 5a6334b..16ca162 100644
> --- a/drivers/input/misc/soc_button_array.c
> +++ b/drivers/input/misc/soc_button_array.c
> @@ -18,7 +18,7 @@
> #include <linux/gpio/consumer.h>
> #include <linux/gpio_keys.h>
> #include <linux/platform_device.h>
> -#include <linux/pnp.h>
> +#include <linux/acpi.h>
>
> /*
> * Definition of buttons on the tablet. The ACPI index of each button
> @@ -67,7 +67,7 @@ static int soc_button_lookup_gpio(struct device *dev, int acpi_index)
> }
>
> static struct platform_device *
> -soc_button_device_create(struct pnp_dev *pdev,
> +soc_button_device_create(struct platform_device *pdev,
> const struct soc_button_info *button_info,
> bool autorepeat)
> {
> @@ -135,30 +135,40 @@ soc_button_device_create(struct pnp_dev *pdev,
> return ERR_PTR(error);
> }
>
> -static void soc_button_remove(struct pnp_dev *pdev)
> +static int soc_button_remove(struct platform_device *pdev)
> {
> - struct soc_button_data *priv = pnp_get_drvdata(pdev);
> + struct soc_button_data *priv = platform_get_drvdata(pdev);
> +
> int i;
>
> for (i = 0; i < BUTTON_TYPES; i++)
> if (priv->children[i])
> platform_device_unregister(priv->children[i]);
> +
> + return 0;
> }
>
> -static int soc_button_pnp_probe(struct pnp_dev *pdev,
> - const struct pnp_device_id *id)
> +static int soc_button_probe(struct platform_device *pdev)
> {
> - const struct soc_button_info *button_info = (void *)id->driver_data;
> + struct device *dev = &pdev->dev;
> + const struct acpi_device_id *id;
> + struct soc_button_info *button_info;
> struct soc_button_data *priv;
> struct platform_device *pd;
> int i;
> int error;
>
> + id = acpi_match_device(dev->driver->acpi_match_table, dev);
> + if (!id)
> + return -ENODEV;
> +
> + button_info = (struct soc_button_info *)id->driver_data;
> +
> priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
>
> - pnp_set_drvdata(pdev, priv);
> + platform_set_drvdata(pdev, priv);
>
> for (i = 0; i < BUTTON_TYPES; i++) {
> pd = soc_button_device_create(pdev, button_info, i == 0);
> @@ -189,30 +199,22 @@ static struct soc_button_info soc_button_PNP0C40[] = {
> { }
> };
>
> -static const struct pnp_device_id soc_button_pnp_match[] = {
> - { .id = "PNP0C40", .driver_data = (long)soc_button_PNP0C40 },
> - { .id = "" }
> +static const struct acpi_device_id soc_button_acpi_match[] = {
> + { "PNP0C40", (unsigned long)soc_button_PNP0C40 },
> + { }
> };
> -MODULE_DEVICE_TABLE(pnp, soc_button_pnp_match);
>
> -static struct pnp_driver soc_button_pnp_driver = {
> - .name = KBUILD_MODNAME,
> - .id_table = soc_button_pnp_match,
> - .probe = soc_button_pnp_probe,
> +MODULE_DEVICE_TABLE(acpi, soc_button_acpi_match);
> +
> +static struct platform_driver soc_button_driver = {
> + .probe = soc_button_probe,
> .remove = soc_button_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .owner = THIS_MODULE,
> + .acpi_match_table = ACPI_PTR(soc_button_acpi_match),
> + },
> };
> -
> -static int __init soc_button_init(void)
> -{
> - return pnp_register_driver(&soc_button_pnp_driver);
> -}
> -
> -static void __exit soc_button_exit(void)
> -{
> - pnp_unregister_driver(&soc_button_pnp_driver);
> -}
> -
> -module_init(soc_button_init);
> -module_exit(soc_button_exit);
> +module_platform_driver(soc_button_driver);
>
> MODULE_LICENSE("GPL");
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Rafael J. Wysocki @ 2014-07-08 20:52 UTC (permalink / raw)
To: Li, Aubrey
Cc: dmitry.torokhov@gmail.com, linux-input@vger.kernel.org, LKML,
One Thousand Gnomes
In-Reply-To: <53A2340D.9030503@linux.intel.com>
On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> When the wakeup attribute is set, the GPIO button is capable of
> waking up the system from sleep states, including the "freeze"
> sleep state. For that to work, its driver needs to pass the
> IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> interrupt will be disabled by suspend_device_irqs() and the
> system won't be woken up by it from the "freeze" sleep state.
>
> The suspend_device_irqs() routine is a workaround for drivers
> that mishandle interrupts triggered when the devices handled
> by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> all drivers that don't have that problem.
>
> The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
>
> Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
OK
Due to the lack of response (ie. no objections) and because the issue
addressed by this patch is real, I'm queuing it up as a PM-related fix
for 3.17.
Rafael
> ---
> drivers/input/keyboard/gpio_keys.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 8c98e97..b046dc6 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -527,6 +527,14 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
> if (!button->can_disable)
> irqflags |= IRQF_SHARED;
>
> + /*
> + * If platform has specified the wakeup attribute of the button,
> + * we add IRQF_NO_SUSPEND to desc->action->flag. So the wake up
> + * interrupt will not be disabled in suspend_device_irqs
> + */
> + if (button->wakeup)
> + irqflags |= IRQF_NO_SUSPEND;
> +
> error = devm_request_any_context_irq(&pdev->dev, bdata->irq,
> isr, irqflags, desc, bdata);
> if (error < 0) {
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Dmitry Torokhov @ 2014-07-08 20:45 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Li, Aubrey, linux-input@vger.kernel.org, LKML,
One Thousand Gnomes
In-Reply-To: <13019301.98tkdHyOVf@vostro.rjw.lan>
On Tue, Jul 08, 2014 at 10:52:52PM +0200, Rafael J. Wysocki wrote:
> On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> > When the wakeup attribute is set, the GPIO button is capable of
> > waking up the system from sleep states, including the "freeze"
> > sleep state. For that to work, its driver needs to pass the
> > IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> > interrupt will be disabled by suspend_device_irqs() and the
> > system won't be woken up by it from the "freeze" sleep state.
> >
> > The suspend_device_irqs() routine is a workaround for drivers
> > that mishandle interrupts triggered when the devices handled
> > by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> > all drivers that don't have that problem.
> >
> > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> >
> > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> OK
>
> Due to the lack of response (ie. no objections) and because the issue
> addressed by this patch is real, I'm queuing it up as a PM-related fix
> for 3.17.
Please do not. The response is till the same: board code should make sure
that enable_irq_wake() does the right thing and keeps interrupts enabled.
It is wrong to patch drivers for this.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Rafael J. Wysocki @ 2014-07-08 21:06 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Li, Aubrey, linux-input@vger.kernel.org, LKML,
One Thousand Gnomes
In-Reply-To: <20140708204530.GA23908@core.coreip.homeip.net>
On Tuesday, July 08, 2014 01:45:30 PM Dmitry Torokhov wrote:
> On Tue, Jul 08, 2014 at 10:52:52PM +0200, Rafael J. Wysocki wrote:
> > On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> > > When the wakeup attribute is set, the GPIO button is capable of
> > > waking up the system from sleep states, including the "freeze"
> > > sleep state. For that to work, its driver needs to pass the
> > > IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> > > interrupt will be disabled by suspend_device_irqs() and the
> > > system won't be woken up by it from the "freeze" sleep state.
> > >
> > > The suspend_device_irqs() routine is a workaround for drivers
> > > that mishandle interrupts triggered when the devices handled
> > > by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> > > all drivers that don't have that problem.
> > >
> > > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> > >
> > > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > OK
> >
> > Due to the lack of response (ie. no objections) and because the issue
> > addressed by this patch is real, I'm queuing it up as a PM-related fix
> > for 3.17.
>
> Please do not. The response is till the same: board code should make sure
> that enable_irq_wake() does the right thing and keeps interrupts enabled.
Which board code? That's nothing like that for the platforms in question.
> It is wrong to patch drivers for this.
Why is it? Only drivers know if they can handle incoming interrupts after
having suspended their devices.
Rafael
^ permalink raw reply
* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Dmitry Torokhov @ 2014-07-08 21:12 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Li, Aubrey, linux-input@vger.kernel.org, LKML,
One Thousand Gnomes
In-Reply-To: <2962778.H1VBlZAtS4@vostro.rjw.lan>
On Tue, Jul 08, 2014 at 11:06:17PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 08, 2014 01:45:30 PM Dmitry Torokhov wrote:
> > On Tue, Jul 08, 2014 at 10:52:52PM +0200, Rafael J. Wysocki wrote:
> > > On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> > > > When the wakeup attribute is set, the GPIO button is capable of
> > > > waking up the system from sleep states, including the "freeze"
> > > > sleep state. For that to work, its driver needs to pass the
> > > > IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> > > > interrupt will be disabled by suspend_device_irqs() and the
> > > > system won't be woken up by it from the "freeze" sleep state.
> > > >
> > > > The suspend_device_irqs() routine is a workaround for drivers
> > > > that mishandle interrupts triggered when the devices handled
> > > > by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> > > > all drivers that don't have that problem.
> > > >
> > > > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> > > >
> > > > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > OK
> > >
> > > Due to the lack of response (ie. no objections) and because the issue
> > > addressed by this patch is real, I'm queuing it up as a PM-related fix
> > > for 3.17.
> >
> > Please do not. The response is till the same: board code should make sure
> > that enable_irq_wake() does the right thing and keeps interrupts enabled.
>
> Which board code? That's nothing like that for the platforms in question.
Then it needs to be written.
>
> > It is wrong to patch drivers for this.
>
> Why is it? Only drivers know if they can handle incoming interrupts after
> having suspended their devices.
The driver correctly used enable_irq_wake() to indicate that interrupt should
be a wakeup source, the now the core/board needs to make sure the interrupt
gets delivered to the driver properly. We should not be patching every driver
that uses enable_irq_wake() with IRQF_NO_SUSPEND. If you look at the earlier
patch discussion Tegra folks managed to implement this behavior just fine.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Rafael J. Wysocki @ 2014-07-08 21:47 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Li, Aubrey, linux-input@vger.kernel.org, LKML,
One Thousand Gnomes, Linux PM list
In-Reply-To: <20140708211259.GA26792@core.coreip.homeip.net>
On Tuesday, July 08, 2014 02:12:59 PM Dmitry Torokhov wrote:
> On Tue, Jul 08, 2014 at 11:06:17PM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, July 08, 2014 01:45:30 PM Dmitry Torokhov wrote:
> > > On Tue, Jul 08, 2014 at 10:52:52PM +0200, Rafael J. Wysocki wrote:
> > > > On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> > > > > When the wakeup attribute is set, the GPIO button is capable of
> > > > > waking up the system from sleep states, including the "freeze"
> > > > > sleep state. For that to work, its driver needs to pass the
> > > > > IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> > > > > interrupt will be disabled by suspend_device_irqs() and the
> > > > > system won't be woken up by it from the "freeze" sleep state.
> > > > >
> > > > > The suspend_device_irqs() routine is a workaround for drivers
> > > > > that mishandle interrupts triggered when the devices handled
> > > > > by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> > > > > all drivers that don't have that problem.
> > > > >
> > > > > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> > > > >
> > > > > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> > > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > OK
> > > >
> > > > Due to the lack of response (ie. no objections) and because the issue
> > > > addressed by this patch is real, I'm queuing it up as a PM-related fix
> > > > for 3.17.
> > >
> > > Please do not. The response is till the same: board code should make sure
> > > that enable_irq_wake() does the right thing and keeps interrupts enabled.
> >
> > Which board code? That's nothing like that for the platforms in question.
>
> Then it needs to be written.
Well, excuse me, but I don't get it. Why would I need to write any board code
for an ACPI-based system?
> > > It is wrong to patch drivers for this.
> >
> > Why is it? Only drivers know if they can handle incoming interrupts after
> > having suspended their devices.
>
> The driver correctly used enable_irq_wake() to indicate that interrupt should
> be a wakeup source, the now the core/board needs to make sure the interrupt
> gets delivered to the driver properly. We should not be patching every driver
> that uses enable_irq_wake() with IRQF_NO_SUSPEND.
Interrupts that can wake up from the "freeze" sleep state need not be set up
with enable_irq_wake() and the flag doesn't say "this is a wakeup interrupt".
It says "do not suspend this interrupt, I can handle it after the device has
been suspended" (as I said).
And if it is OK for a driver to set IRQF_SHARED, it is equally OK for it to
set IRQF_NO_SUSPEND, because, in fact, those two flags are related.
> If you look at the earlier patch discussion Tegra folks managed to implement
> this behavior just fine.
I'm not sure whose idea it was that IRQF_NO_SUSPEND was not to be set by drivers,
but it is not a correct one. I know why suspend_device_irqs() was introduced
and I'm telling you this has nothing to do with setting up the IRQ chip to do
system wakeup.
And please grep for IRQF_NO_SUSPEND to see how drivers generally use it.
Rafael
^ permalink raw reply
* Re: [PATCH 1/2 v2] lib: int_sqrt: add int64_sqrt routine
From: Andrew Morton @ 2014-07-08 21:55 UTC (permalink / raw)
To: Barry Song
Cc: dmitry.torokhov, dtor, linux-input, workgroup.linux, Yibo Cai,
Peter Meerwald, Barry Song
In-Reply-To: <1404390870-24062-1-git-send-email-21cnbao@gmail.com>
On Thu, 3 Jul 2014 20:34:30 +0800 Barry Song <21cnbao@gmail.com> wrote:
> Get square root of a 64-bit digit on 32bit platforms. CSR SiRFSoC
> touchscreen driver needs it.
Looks OK to me. Can this be carried in whatever tree holds the "CSR
SiRFSoC touchscreen driver"?
^ permalink raw reply
* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Dmitry Torokhov @ 2014-07-08 22:11 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Li, Aubrey, linux-input@vger.kernel.org, LKML,
One Thousand Gnomes, Linux PM list
In-Reply-To: <1489151.dWxTTZlWzH@vostro.rjw.lan>
On Tue, Jul 08, 2014 at 11:47:01PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 08, 2014 02:12:59 PM Dmitry Torokhov wrote:
> > On Tue, Jul 08, 2014 at 11:06:17PM +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, July 08, 2014 01:45:30 PM Dmitry Torokhov wrote:
> > > > On Tue, Jul 08, 2014 at 10:52:52PM +0200, Rafael J. Wysocki wrote:
> > > > > On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> > > > > > When the wakeup attribute is set, the GPIO button is capable of
> > > > > > waking up the system from sleep states, including the "freeze"
> > > > > > sleep state. For that to work, its driver needs to pass the
> > > > > > IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> > > > > > interrupt will be disabled by suspend_device_irqs() and the
> > > > > > system won't be woken up by it from the "freeze" sleep state.
> > > > > >
> > > > > > The suspend_device_irqs() routine is a workaround for drivers
> > > > > > that mishandle interrupts triggered when the devices handled
> > > > > > by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> > > > > > all drivers that don't have that problem.
> > > > > >
> > > > > > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> > > > > >
> > > > > > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> > > > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > > OK
> > > > >
> > > > > Due to the lack of response (ie. no objections) and because the issue
> > > > > addressed by this patch is real, I'm queuing it up as a PM-related fix
> > > > > for 3.17.
> > > >
> > > > Please do not. The response is till the same: board code should make sure
> > > > that enable_irq_wake() does the right thing and keeps interrupts enabled.
> > >
> > > Which board code? That's nothing like that for the platforms in question.
> >
> > Then it needs to be written.
>
> Well, excuse me, but I don't get it. Why would I need to write any board code
> for an ACPI-based system?
Why would not you? What is the difference between ACPI systems and all
other systems? If ACPI-based systems need certain behavior they need to
implement it, the same as DT-based systems or custom boards.
>
> > > > It is wrong to patch drivers for this.
> > >
> > > Why is it? Only drivers know if they can handle incoming interrupts after
> > > having suspended their devices.
> >
> > The driver correctly used enable_irq_wake() to indicate that interrupt should
> > be a wakeup source, the now the core/board needs to make sure the interrupt
> > gets delivered to the driver properly. We should not be patching every driver
> > that uses enable_irq_wake() with IRQF_NO_SUSPEND.
>
> Interrupts that can wake up from the "freeze" sleep state need not be set up
> with enable_irq_wake() and the flag doesn't say "this is a wakeup interrupt".
> It says "do not suspend this interrupt, I can handle it after the device has
> been suspended" (as I said).
And the driver does not really care about this and whether the sleep
state is suspend or freeze or something else, it is your platform that
cares and has certain limitations that require interrupts to be not
suspended in certain cases.
>From the driver POV it says that the device can be a waekup source
(again it does not care about details as to which sleep state we woudl
be waking from) and it expects the PM core to handle the things
properly. If certain sleep state requires interrupts to be kept on then
PM core should make them as such, not driver.
>
> And if it is OK for a driver to set IRQF_SHARED, it is equally OK for it to
> set IRQF_NO_SUSPEND, because, in fact, those two flags are related.
Are you proposing for IRQ core to automatically set IRQF_NO_SUSPEND for
IRQF_SHARED interrupts? That wold be fine with me.
>
> > If you look at the earlier patch discussion Tegra folks managed to implement
> > this behavior just fine.
>
> I'm not sure whose idea it was that IRQF_NO_SUSPEND was not to be set by drivers,
> but it is not a correct one. I know why suspend_device_irqs() was introduced
> and I'm telling you this has nothing to do with setting up the IRQ chip to do
> system wakeup.
I do not believe I asked why suspend_device_irqs() was introduced.
>
> And please grep for IRQF_NO_SUSPEND to see how drivers generally use it.
I see that just handful of them use IRQF_NO_SUSPEND (not sure how many
are actully required), I see that a lot more drivers use
enable_irq_wake() and do not bother setting IRQF_NO_SUSPEND.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2 v2] lib: int_sqrt: add int64_sqrt routine
From: Dmitry Torokhov @ 2014-07-08 22:12 UTC (permalink / raw)
To: Andrew Morton
Cc: Barry Song, linux-input, workgroup.linux, Yibo Cai,
Peter Meerwald, Barry Song
In-Reply-To: <20140708145501.bf6efdebd4991fec5422d0a1@linux-foundation.org>
On Tue, Jul 08, 2014 at 02:55:01PM -0700, Andrew Morton wrote:
> On Thu, 3 Jul 2014 20:34:30 +0800 Barry Song <21cnbao@gmail.com> wrote:
>
> > Get square root of a 64-bit digit on 32bit platforms. CSR SiRFSoC
> > touchscreen driver needs it.
>
> Looks OK to me. Can this be carried in whatever tree holds the "CSR
> SiRFSoC touchscreen driver"?
Yeah, I'll pick it up.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Rafael J. Wysocki @ 2014-07-08 23:06 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Li, Aubrey, linux-input@vger.kernel.org, LKML,
One Thousand Gnomes, Linux PM list
In-Reply-To: <20140708221114.GA29203@core.coreip.homeip.net>
On Tuesday, July 08, 2014 03:11:14 PM Dmitry Torokhov wrote:
> On Tue, Jul 08, 2014 at 11:47:01PM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, July 08, 2014 02:12:59 PM Dmitry Torokhov wrote:
> > > On Tue, Jul 08, 2014 at 11:06:17PM +0200, Rafael J. Wysocki wrote:
> > > > On Tuesday, July 08, 2014 01:45:30 PM Dmitry Torokhov wrote:
> > > > > On Tue, Jul 08, 2014 at 10:52:52PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> > > > > > > When the wakeup attribute is set, the GPIO button is capable of
> > > > > > > waking up the system from sleep states, including the "freeze"
> > > > > > > sleep state. For that to work, its driver needs to pass the
> > > > > > > IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> > > > > > > interrupt will be disabled by suspend_device_irqs() and the
> > > > > > > system won't be woken up by it from the "freeze" sleep state.
> > > > > > >
> > > > > > > The suspend_device_irqs() routine is a workaround for drivers
> > > > > > > that mishandle interrupts triggered when the devices handled
> > > > > > > by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> > > > > > > all drivers that don't have that problem.
> > > > > > >
> > > > > > > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> > > > > > >
> > > > > > > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> > > > > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > >
> > > > > > OK
> > > > > >
> > > > > > Due to the lack of response (ie. no objections) and because the issue
> > > > > > addressed by this patch is real, I'm queuing it up as a PM-related fix
> > > > > > for 3.17.
> > > > >
> > > > > Please do not. The response is till the same: board code should make sure
> > > > > that enable_irq_wake() does the right thing and keeps interrupts enabled.
> > > >
> > > > Which board code? That's nothing like that for the platforms in question.
> > >
> > > Then it needs to be written.
> >
> > Well, excuse me, but I don't get it. Why would I need to write any board code
> > for an ACPI-based system?
>
> Why would not you? What is the difference between ACPI systems and all
> other systems? If ACPI-based systems need certain behavior they need to
> implement it, the same as DT-based systems or custom boards.
I'm not sure what you're talking about.
This isn't an ACPI-based system that needs certain behavior. That's certain
behavior we want from GPIO buttons that can wake up *in* *general*.
Regardless of the platform, we want interrupts from those buttons to happen
after calling suspend_device_irqs(). Why? Because we want them to be able
to happen while freeze_enter() is being executed and *that* is platform
independent.
> > > > > It is wrong to patch drivers for this.
> > > >
> > > > Why is it? Only drivers know if they can handle incoming interrupts after
> > > > having suspended their devices.
> > >
> > > The driver correctly used enable_irq_wake() to indicate that interrupt should
> > > be a wakeup source, the now the core/board needs to make sure the interrupt
> > > gets delivered to the driver properly. We should not be patching every driver
> > > that uses enable_irq_wake() with IRQF_NO_SUSPEND.
> >
> > Interrupts that can wake up from the "freeze" sleep state need not be set up
> > with enable_irq_wake() and the flag doesn't say "this is a wakeup interrupt".
> > It says "do not suspend this interrupt, I can handle it after the device has
> > been suspended" (as I said).
>
> And the driver does not really care about this and whether the sleep
> state is suspend or freeze or something else, it is your platform that
> cares and has certain limitations that require interrupts to be not
> suspended in certain cases.
Not in "certain cases", but actually never and it is not about any platform
limitations. I'm not sure what other words I need to use to make it more
clear.
> From the driver POV it says that the device can be a waekup source
> (again it does not care about details as to which sleep state we woudl
> be waking from) and it expects the PM core to handle the things
> properly. If certain sleep state requires interrupts to be kept on then
> PM core should make them as such, not driver.
That would be the case in an ideal world, but the real one is not ideal,
unfortunately. The problem is that the PM core actually cannot decide
which interrupts to keep on, because it doesn't know which drivers can
cope with interrupts that happen after their devices have been suspended.
Using IRQF_NO_SUSPEND is the way to experess that by a driver.
And again, that has nothing to do with platform limitations or requirements.
All this is about is whether or not to allow interrupts to be *handled* by
drivers after certain point in the suspend sequence, which is
suspend_device_irqs(). By using IRQF_NO_SUSPEND drivers say, basically "it is
OK to leave this IRQ as is, I promise to take care of it going forward" and
that covers *all* suspend/hibernate transitions.
The reason why the "freeze" sleep state is somewhat special is that it doesn't
do any platform-specific magic and needs *normal* device interrupts to work
after suspend_device_irqs(). And I would *love* *to* drop suspend_device_irqs()
at this point, but I can't, because there still are drivers that would have
broken had I done that.
And I'm saying "somewhat" above, because that behavior is actually needed to
prevent wakeup events occuring *during* suspend transitions from being lost
for all kinds of those transitions (if someone actually cares).
> >
> > And if it is OK for a driver to set IRQF_SHARED, it is equally OK for it to
> > set IRQF_NO_SUSPEND, because, in fact, those two flags are related.
>
> Are you proposing for IRQ core to automatically set IRQF_NO_SUSPEND for
> IRQF_SHARED interrupts? That wold be fine with me.
No, I'm not.
The first choice basically is to go through all drivers that don't use IRQF_NO_SUSPEND
and which have _noirq suspend callbacks (that covers all PCI drivers and some
non-PCI ones too IIRC) and audit their interrupt handlers to check whether or not
they can cope with interrupts coming after their devices have been suspended. Fix
the ones that can't and we can drop suspend_device_irqs().
But I guess I may be forgiven for regarding that as rather unattractive. And we
actually could have done that to start with, guess why we didn't?
The second choice is to use IRQF_NO_SUSPEND in the drivers that are OK. That
isn't too attractive either but has been practice for quite a while.
> > > If you look at the earlier patch discussion Tegra folks managed to implement
> > > this behavior just fine.
> >
> > I'm not sure whose idea it was that IRQF_NO_SUSPEND was not to be set by drivers,
> > but it is not a correct one. I know why suspend_device_irqs() was introduced
> > and I'm telling you this has nothing to do with setting up the IRQ chip to do
> > system wakeup.
>
> I do not believe I asked why suspend_device_irqs() was introduced.
But you should, because suspend_device_irqs() is the very reason why
IRQF_NO_SUSPEND exists. :-)
> >
> > And please grep for IRQF_NO_SUSPEND to see how drivers generally use it.
>
> I see that just handful of them use IRQF_NO_SUSPEND (not sure how many
> are actully required), I see that a lot more drivers use
> enable_irq_wake() and do not bother setting IRQF_NO_SUSPEND.
And they will have problems with the "freeze" sleep state.
enable_irq_wake() is *not* a replacement for IRQF_NO_SUSPEND, nor the other
way around. They are different things.
Rafael
^ permalink raw reply
* Re: [PATCH] GPIO button wth wakeup attribute is supposed to wake the system up
From: Dmitry Torokhov @ 2014-07-09 0:15 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Li, Aubrey, linux-input@vger.kernel.org, LKML,
One Thousand Gnomes, Linux PM list
In-Reply-To: <2213522.hTnsy9lMxO@vostro.rjw.lan>
On Wed, Jul 09, 2014 at 01:06:07AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 08, 2014 03:11:14 PM Dmitry Torokhov wrote:
> > On Tue, Jul 08, 2014 at 11:47:01PM +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, July 08, 2014 02:12:59 PM Dmitry Torokhov wrote:
> > > > On Tue, Jul 08, 2014 at 11:06:17PM +0200, Rafael J. Wysocki wrote:
> > > > > On Tuesday, July 08, 2014 01:45:30 PM Dmitry Torokhov wrote:
> > > > > > On Tue, Jul 08, 2014 at 10:52:52PM +0200, Rafael J. Wysocki wrote:
> > > > > > > On Thursday, June 19, 2014 08:51:25 AM Li, Aubrey wrote:
> > > > > > > > When the wakeup attribute is set, the GPIO button is capable of
> > > > > > > > waking up the system from sleep states, including the "freeze"
> > > > > > > > sleep state. For that to work, its driver needs to pass the
> > > > > > > > IRQF_NO_SUSPEND flag to devm_request_any_context_irq(), or the
> > > > > > > > interrupt will be disabled by suspend_device_irqs() and the
> > > > > > > > system won't be woken up by it from the "freeze" sleep state.
> > > > > > > >
> > > > > > > > The suspend_device_irqs() routine is a workaround for drivers
> > > > > > > > that mishandle interrupts triggered when the devices handled
> > > > > > > > by them are suspended, so it is safe to use IRQF_NO_SUSPEND in
> > > > > > > > all drivers that don't have that problem.
> > > > > > > >
> > > > > > > > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
> > > > > > > >
> > > > > > > > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> > > > > > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > >
> > > > > > > OK
> > > > > > >
> > > > > > > Due to the lack of response (ie. no objections) and because the issue
> > > > > > > addressed by this patch is real, I'm queuing it up as a PM-related fix
> > > > > > > for 3.17.
> > > > > >
> > > > > > Please do not. The response is till the same: board code should make sure
> > > > > > that enable_irq_wake() does the right thing and keeps interrupts enabled.
> > > > >
> > > > > Which board code? That's nothing like that for the platforms in question.
> > > >
> > > > Then it needs to be written.
> > >
> > > Well, excuse me, but I don't get it. Why would I need to write any board code
> > > for an ACPI-based system?
> >
> > Why would not you? What is the difference between ACPI systems and all
> > other systems? If ACPI-based systems need certain behavior they need to
> > implement it, the same as DT-based systems or custom boards.
>
> I'm not sure what you're talking about.
>
> This isn't an ACPI-based system that needs certain behavior. That's certain
> behavior we want from GPIO buttons that can wake up *in* *general*.
>
> Regardless of the platform, we want interrupts from those buttons to happen
> after calling suspend_device_irqs(). Why? Because we want them to be able
> to happen while freeze_enter() is being executed and *that* is platform
> independent.
Tell me this: do we always call suspend_device_irqs()?
>
> > > > > > It is wrong to patch drivers for this.
> > > > >
> > > > > Why is it? Only drivers know if they can handle incoming interrupts after
> > > > > having suspended their devices.
> > > >
> > > > The driver correctly used enable_irq_wake() to indicate that interrupt should
> > > > be a wakeup source, the now the core/board needs to make sure the interrupt
> > > > gets delivered to the driver properly. We should not be patching every driver
> > > > that uses enable_irq_wake() with IRQF_NO_SUSPEND.
> > >
> > > Interrupts that can wake up from the "freeze" sleep state need not be set up
> > > with enable_irq_wake() and the flag doesn't say "this is a wakeup interrupt".
> > > It says "do not suspend this interrupt, I can handle it after the device has
> > > been suspended" (as I said).
> >
> > And the driver does not really care about this and whether the sleep
> > state is suspend or freeze or something else, it is your platform that
> > cares and has certain limitations that require interrupts to be not
> > suspended in certain cases.
>
> Not in "certain cases", but actually never and it is not about any platform
> limitations. I'm not sure what other words I need to use to make it more
> clear.
>
> > From the driver POV it says that the device can be a waekup source
> > (again it does not care about details as to which sleep state we woudl
> > be waking from) and it expects the PM core to handle the things
> > properly. If certain sleep state requires interrupts to be kept on then
> > PM core should make them as such, not driver.
>
> That would be the case in an ideal world, but the real one is not ideal,
> unfortunately. The problem is that the PM core actually cannot decide
> which interrupts to keep on, because it doesn't know which drivers can
> cope with interrupts that happen after their devices have been suspended.
> Using IRQF_NO_SUSPEND is the way to experess that by a driver.
When device driver marks IRQ as a wakeup source I believe it is prepared
to handle it (or it would shut it off explicitly).
>
> And again, that has nothing to do with platform limitations or requirements.
> All this is about is whether or not to allow interrupts to be *handled* by
> drivers after certain point in the suspend sequence, which is
> suspend_device_irqs(). By using IRQF_NO_SUSPEND drivers say, basically "it is
> OK to leave this IRQ as is, I promise to take care of it going forward" and
> that covers *all* suspend/hibernate transitions.
>
> The reason why the "freeze" sleep state is somewhat special is that it doesn't
> do any platform-specific magic and needs *normal* device interrupts to work
> after suspend_device_irqs(). And I would *love* *to* drop suspend_device_irqs()
> at this point, but I can't, because there still are drivers that would have
> broken had I done that.
>
> And I'm saying "somewhat" above, because that behavior is actually needed to
> prevent wakeup events occuring *during* suspend transitions from being lost
> for all kinds of those transitions (if someone actually cares).
>
> > >
> > > And if it is OK for a driver to set IRQF_SHARED, it is equally OK for it to
> > > set IRQF_NO_SUSPEND, because, in fact, those two flags are related.
> >
> > Are you proposing for IRQ core to automatically set IRQF_NO_SUSPEND for
> > IRQF_SHARED interrupts? That wold be fine with me.
>
> No, I'm not.
Then they are not really related that closely, are they?
>
> The first choice basically is to go through all drivers that don't use IRQF_NO_SUSPEND
> and which have _noirq suspend callbacks (that covers all PCI drivers and some
> non-PCI ones too IIRC) and audit their interrupt handlers to check whether or not
> they can cope with interrupts coming after their devices have been suspended. Fix
> the ones that can't and we can drop suspend_device_irqs().
>
> But I guess I may be forgiven for regarding that as rather unattractive. And we
> actually could have done that to start with, guess why we didn't?
>
> The second choice is to use IRQF_NO_SUSPEND in the drivers that are OK. That
> isn't too attractive either but has been practice for quite a while.
>
> > > > If you look at the earlier patch discussion Tegra folks managed to implement
> > > > this behavior just fine.
> > >
> > > I'm not sure whose idea it was that IRQF_NO_SUSPEND was not to be set by drivers,
> > > but it is not a correct one. I know why suspend_device_irqs() was introduced
> > > and I'm telling you this has nothing to do with setting up the IRQ chip to do
> > > system wakeup.
> >
> > I do not believe I asked why suspend_device_irqs() was introduced.
>
> But you should, because suspend_device_irqs() is the very reason why
> IRQF_NO_SUSPEND exists. :-)
Then you should have shared this knowledge instead of asserting that you
possess it.
>
> > >
> > > And please grep for IRQF_NO_SUSPEND to see how drivers generally use it.
> >
> > I see that just handful of them use IRQF_NO_SUSPEND (not sure how many
> > are actully required), I see that a lot more drivers use
> > enable_irq_wake() and do not bother setting IRQF_NO_SUSPEND.
>
> And they will have problems with the "freeze" sleep state.
>
> enable_irq_wake() is *not* a replacement for IRQF_NO_SUSPEND, nor the other
> way around. They are different things.
However what I hear is that one has to use one when using another, and
form this fact comes my request: when entering freeze sleep state
interrupts that are marked as wakeup sources should be automatically
excluded form the list of IRQs that need to be suspended.
Thanks.
--
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox