Re: [RFC PATCH 02/06] input/rmi4: Core files
On Mon, Nov 26, 2012 at 11:54 PM, Christopher Heiny
wrote:
> On 11/26/2012 10:41 AM, Benjamin Tissoires wrote:
>>
>> Hi Christopher,
>>
>> On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny
>> wrote:
>>>
>>> rmi_bus.c implements the basic functionality of the RMI bus. This file
>>> is
>>> greatly simplified compared to the previous patch - we've switched from
>>> "do it yourself" device/driver binding to using device_type to
>>> distinguish
>>> between the two kinds of devices on the bus (sensor devices and function
>>> specific devices) and using the standard bus implementation to manage
>>> devices
>>> and drivers.
>>>
>>>
>>> rmi_driver.c is a driver for the general functionality of the RMI sensor
>>> as a
>>> whole, managing those behaviors (including IRQ handling) that are not
>>> specific
>>> to any RMI4 function. It has some unavoidable dependencies on F01
>>> behavior,
>>> though we have worked to minimize those as far as possible.
>>>
>>>
>>> The header file rmi_driver.h provides definitions that are shared among
>>> the modules of the RMI implementation, but not thought to be necessary
>>> outside it.
>>>
>>>
>>> Greg KH - Linus Walleij recommended that we seek your input on these core
>>> files, particularly the bus implementation.
>>>
>>>
>>> Signed-off-by: Christopher Heiny
>>>
>>> Cc: Greg Kroah-Hartman
>>> Cc: Dmitry Torokhov
>>> Cc: Linus Walleij
>>> Cc: Naveen Kumar Gaddipati
>>> Cc: Joeri de Gram
>>>
>>> ---
>>>
>>> drivers/input/rmi4/rmi_bus.c| 248 ++
>>> drivers/input/rmi4/rmi_driver.c | 1663
>>> +++
>>> drivers/input/rmi4/rmi_driver.h | 139
>>> include/uapi/linux/input.h |1 +
>>> 4 files changed, 2051 insertions(+), 0 deletions(-)
>>>
>> [snipped]
>>>
>>> diff --git a/drivers/input/rmi4/rmi_driver.c
>>> b/drivers/input/rmi4/rmi_driver.c
>>> new file mode 100644
>>> index 000..05a73ae
>>> --- /dev/null
>>> +++ b/drivers/input/rmi4/rmi_driver.c
>>
>> [snipped]
>>>
>>> +/* extract product ID */
>>> +void get_prod_id(struct rmi_device *rmi_dev, struct rmi_driver_data
>>> *drvdata)
>>> +{
>>> + struct device *dev = _dev->dev;
>>> + int retval;
>>> + int board = 0, rev = 0;
>>> + int i;
>>> + static const char * const pattern[] = {
>>> + "tm%4d-%d", "s%4d-%d", "s%4d-ver%1d"};
>>> + u8 product_id[RMI_PRODUCT_ID_LENGTH+1];
>>> +
>>> + retval = rmi_read_block(rmi_dev,
>>> + drvdata->f01_container->fd.query_base_addr+
>>> + sizeof(struct f01_basic_queries),
>>> + product_id, RMI_PRODUCT_ID_LENGTH);
>>> + if (retval < 0) {
>>> + dev_err(dev, "Failed to read product id, code=%d!",
>>> retval);
>>> + return;
>>> + }
>>> + product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
>>> +
>>> + for (i = 0; i < sizeof(product_id); i++)
>>> + product_id[i] = tolower(product_id[i]);
>>> +
>>> + for (i = 0; i < sizeof(pattern); i++) {
>>
>>
>> This should be ARRAY_SIZE(pattern).
>> It gave me a wonderful kernel oops :)
>
>
> Yep, that's a bug! Oddly enough, it runs without barfing on my systems
> (though who knows what horrible things are happening under the hood). Thanks
> for letting us know.
That's because the product id advertised by the touchpad you sent to
me is 'DS4 R3.0'. So it's not compliant with the patterns that are
advertised: "tm%4d-%d", "s%4d-%d", "s%4d-ver%1d".
If you present one of these patterns, thanks to the break, you leave
the for loop. But as I'm not in this optimal case, I'm having i > 2,
which introduced segfault.
Cheers,
Benjamin
>
> Chris
>
>>
>> Cheers,
>> Benjamin
>
>
> [snip]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Mon, Nov 26, 2012 at 11:54 PM, Christopher Heiny
che...@synaptics.com wrote:
On 11/26/2012 10:41 AM, Benjamin Tissoires wrote:
Hi Christopher,
On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny che...@synaptics.com
wrote:
rmi_bus.c implements the basic functionality of the RMI bus. This file
is
greatly simplified compared to the previous patch - we've switched from
do it yourself device/driver binding to using device_type to
distinguish
between the two kinds of devices on the bus (sensor devices and function
specific devices) and using the standard bus implementation to manage
devices
and drivers.
rmi_driver.c is a driver for the general functionality of the RMI sensor
as a
whole, managing those behaviors (including IRQ handling) that are not
specific
to any RMI4 function. It has some unavoidable dependencies on F01
behavior,
though we have worked to minimize those as far as possible.
The header file rmi_driver.h provides definitions that are shared among
the modules of the RMI implementation, but not thought to be necessary
outside it.
Greg KH - Linus Walleij recommended that we seek your input on these core
files, particularly the bus implementation.
Signed-off-by: Christopher Heiny che...@synaptics.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Dmitry Torokhov dmitry.torok...@gmail.com
Cc: Linus Walleij linus.wall...@stericsson.com
Cc: Naveen Kumar Gaddipati naveen.gaddip...@stericsson.com
Cc: Joeri de Gram j.de.g...@gmail.com
---
drivers/input/rmi4/rmi_bus.c| 248 ++
drivers/input/rmi4/rmi_driver.c | 1663
+++
drivers/input/rmi4/rmi_driver.h | 139
include/uapi/linux/input.h |1 +
4 files changed, 2051 insertions(+), 0 deletions(-)
[snipped]
diff --git a/drivers/input/rmi4/rmi_driver.c
b/drivers/input/rmi4/rmi_driver.c
new file mode 100644
index 000..05a73ae
--- /dev/null
+++ b/drivers/input/rmi4/rmi_driver.c
[snipped]
+/* extract product ID */
+void get_prod_id(struct rmi_device *rmi_dev, struct rmi_driver_data
*drvdata)
+{
+ struct device *dev = rmi_dev-dev;
+ int retval;
+ int board = 0, rev = 0;
+ int i;
+ static const char * const pattern[] = {
+ tm%4d-%d, s%4d-%d, s%4d-ver%1d};
+ u8 product_id[RMI_PRODUCT_ID_LENGTH+1];
+
+ retval = rmi_read_block(rmi_dev,
+ drvdata-f01_container-fd.query_base_addr+
+ sizeof(struct f01_basic_queries),
+ product_id, RMI_PRODUCT_ID_LENGTH);
+ if (retval 0) {
+ dev_err(dev, Failed to read product id, code=%d!,
retval);
+ return;
+ }
+ product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
+
+ for (i = 0; i sizeof(product_id); i++)
+ product_id[i] = tolower(product_id[i]);
+
+ for (i = 0; i sizeof(pattern); i++) {
This should be ARRAY_SIZE(pattern).
It gave me a wonderful kernel oops :)
Yep, that's a bug! Oddly enough, it runs without barfing on my systems
(though who knows what horrible things are happening under the hood). Thanks
for letting us know.
That's because the product id advertised by the touchpad you sent to
me is 'DS4 R3.0'. So it's not compliant with the patterns that are
advertised: tm%4d-%d, s%4d-%d, s%4d-ver%1d.
If you present one of these patterns, thanks to the break, you leave
the for loop. But as I'm not in this optimal case, I'm having i 2,
which introduced segfault.
Cheers,
Benjamin
Chris
Cheers,
Benjamin
[snip]
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On 11/26/2012 10:41 AM, Benjamin Tissoires wrote:
Hi Christopher,
On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny wrote:
rmi_bus.c implements the basic functionality of the RMI bus. This file is
greatly simplified compared to the previous patch - we've switched from
"do it yourself" device/driver binding to using device_type to distinguish
between the two kinds of devices on the bus (sensor devices and function
specific devices) and using the standard bus implementation to manage devices
and drivers.
rmi_driver.c is a driver for the general functionality of the RMI sensor as a
whole, managing those behaviors (including IRQ handling) that are not specific
to any RMI4 function. It has some unavoidable dependencies on F01 behavior,
though we have worked to minimize those as far as possible.
The header file rmi_driver.h provides definitions that are shared among
the modules of the RMI implementation, but not thought to be necessary
outside it.
Greg KH - Linus Walleij recommended that we seek your input on these core
files, particularly the bus implementation.
Signed-off-by: Christopher Heiny
Cc: Greg Kroah-Hartman
Cc: Dmitry Torokhov
Cc: Linus Walleij
Cc: Naveen Kumar Gaddipati
Cc: Joeri de Gram
---
drivers/input/rmi4/rmi_bus.c| 248 ++
drivers/input/rmi4/rmi_driver.c | 1663 +++
drivers/input/rmi4/rmi_driver.h | 139
include/uapi/linux/input.h |1 +
4 files changed, 2051 insertions(+), 0 deletions(-)
[snipped]
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
new file mode 100644
index 000..05a73ae
--- /dev/null
+++ b/drivers/input/rmi4/rmi_driver.c
[snipped]
+/* extract product ID */
+void get_prod_id(struct rmi_device *rmi_dev, struct rmi_driver_data *drvdata)
+{
+ struct device *dev = _dev->dev;
+ int retval;
+ int board = 0, rev = 0;
+ int i;
+ static const char * const pattern[] = {
+ "tm%4d-%d", "s%4d-%d", "s%4d-ver%1d"};
+ u8 product_id[RMI_PRODUCT_ID_LENGTH+1];
+
+ retval = rmi_read_block(rmi_dev,
+ drvdata->f01_container->fd.query_base_addr+
+ sizeof(struct f01_basic_queries),
+ product_id, RMI_PRODUCT_ID_LENGTH);
+ if (retval < 0) {
+ dev_err(dev, "Failed to read product id, code=%d!", retval);
+ return;
+ }
+ product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
+
+ for (i = 0; i < sizeof(product_id); i++)
+ product_id[i] = tolower(product_id[i]);
+
+ for (i = 0; i < sizeof(pattern); i++) {
This should be ARRAY_SIZE(pattern).
It gave me a wonderful kernel oops :)
Yep, that's a bug! Oddly enough, it runs without barfing on my systems
(though who knows what horrible things are happening under the hood).
Thanks for letting us know.
Chris
Cheers,
Benjamin
[snip]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
Hi Christopher,
On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny wrote:
> rmi_bus.c implements the basic functionality of the RMI bus. This file is
> greatly simplified compared to the previous patch - we've switched from
> "do it yourself" device/driver binding to using device_type to distinguish
> between the two kinds of devices on the bus (sensor devices and function
> specific devices) and using the standard bus implementation to manage devices
> and drivers.
>
>
> rmi_driver.c is a driver for the general functionality of the RMI sensor as a
> whole, managing those behaviors (including IRQ handling) that are not specific
> to any RMI4 function. It has some unavoidable dependencies on F01 behavior,
> though we have worked to minimize those as far as possible.
>
>
> The header file rmi_driver.h provides definitions that are shared among
> the modules of the RMI implementation, but not thought to be necessary
> outside it.
>
>
> Greg KH - Linus Walleij recommended that we seek your input on these core
> files, particularly the bus implementation.
>
>
> Signed-off-by: Christopher Heiny
>
> Cc: Greg Kroah-Hartman
> Cc: Dmitry Torokhov
> Cc: Linus Walleij
> Cc: Naveen Kumar Gaddipati
> Cc: Joeri de Gram
>
> ---
>
> drivers/input/rmi4/rmi_bus.c| 248 ++
> drivers/input/rmi4/rmi_driver.c | 1663
> +++
> drivers/input/rmi4/rmi_driver.h | 139
> include/uapi/linux/input.h |1 +
> 4 files changed, 2051 insertions(+), 0 deletions(-)
>
[snipped]
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> new file mode 100644
> index 000..05a73ae
> --- /dev/null
> +++ b/drivers/input/rmi4/rmi_driver.c
[snipped]
> +/* extract product ID */
> +void get_prod_id(struct rmi_device *rmi_dev, struct rmi_driver_data *drvdata)
> +{
> + struct device *dev = _dev->dev;
> + int retval;
> + int board = 0, rev = 0;
> + int i;
> + static const char * const pattern[] = {
> + "tm%4d-%d", "s%4d-%d", "s%4d-ver%1d"};
> + u8 product_id[RMI_PRODUCT_ID_LENGTH+1];
> +
> + retval = rmi_read_block(rmi_dev,
> + drvdata->f01_container->fd.query_base_addr+
> + sizeof(struct f01_basic_queries),
> + product_id, RMI_PRODUCT_ID_LENGTH);
> + if (retval < 0) {
> + dev_err(dev, "Failed to read product id, code=%d!", retval);
> + return;
> + }
> + product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
> +
> + for (i = 0; i < sizeof(product_id); i++)
> + product_id[i] = tolower(product_id[i]);
> +
> + for (i = 0; i < sizeof(pattern); i++) {
This should be ARRAY_SIZE(pattern).
It gave me a wonderful kernel oops :)
Cheers,
Benjamin
> + retval = sscanf(product_id, pattern[i], , );
> + if (retval)
> + break;
> + }
> + /* save board and rev data in the rmi_driver_data */
> + drvdata->board = board;
> + drvdata->rev = rev;
> + dev_dbg(dev, "Rmi_driver getProdID, set board: %d rev: %d\n",
> + drvdata->board, drvdata->rev);
> +}
> +
[snipped]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
Hi Christopher,
On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny che...@synaptics.com wrote:
rmi_bus.c implements the basic functionality of the RMI bus. This file is
greatly simplified compared to the previous patch - we've switched from
do it yourself device/driver binding to using device_type to distinguish
between the two kinds of devices on the bus (sensor devices and function
specific devices) and using the standard bus implementation to manage devices
and drivers.
rmi_driver.c is a driver for the general functionality of the RMI sensor as a
whole, managing those behaviors (including IRQ handling) that are not specific
to any RMI4 function. It has some unavoidable dependencies on F01 behavior,
though we have worked to minimize those as far as possible.
The header file rmi_driver.h provides definitions that are shared among
the modules of the RMI implementation, but not thought to be necessary
outside it.
Greg KH - Linus Walleij recommended that we seek your input on these core
files, particularly the bus implementation.
Signed-off-by: Christopher Heiny che...@synaptics.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Dmitry Torokhov dmitry.torok...@gmail.com
Cc: Linus Walleij linus.wall...@stericsson.com
Cc: Naveen Kumar Gaddipati naveen.gaddip...@stericsson.com
Cc: Joeri de Gram j.de.g...@gmail.com
---
drivers/input/rmi4/rmi_bus.c| 248 ++
drivers/input/rmi4/rmi_driver.c | 1663
+++
drivers/input/rmi4/rmi_driver.h | 139
include/uapi/linux/input.h |1 +
4 files changed, 2051 insertions(+), 0 deletions(-)
[snipped]
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
new file mode 100644
index 000..05a73ae
--- /dev/null
+++ b/drivers/input/rmi4/rmi_driver.c
[snipped]
+/* extract product ID */
+void get_prod_id(struct rmi_device *rmi_dev, struct rmi_driver_data *drvdata)
+{
+ struct device *dev = rmi_dev-dev;
+ int retval;
+ int board = 0, rev = 0;
+ int i;
+ static const char * const pattern[] = {
+ tm%4d-%d, s%4d-%d, s%4d-ver%1d};
+ u8 product_id[RMI_PRODUCT_ID_LENGTH+1];
+
+ retval = rmi_read_block(rmi_dev,
+ drvdata-f01_container-fd.query_base_addr+
+ sizeof(struct f01_basic_queries),
+ product_id, RMI_PRODUCT_ID_LENGTH);
+ if (retval 0) {
+ dev_err(dev, Failed to read product id, code=%d!, retval);
+ return;
+ }
+ product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
+
+ for (i = 0; i sizeof(product_id); i++)
+ product_id[i] = tolower(product_id[i]);
+
+ for (i = 0; i sizeof(pattern); i++) {
This should be ARRAY_SIZE(pattern).
It gave me a wonderful kernel oops :)
Cheers,
Benjamin
+ retval = sscanf(product_id, pattern[i], board, rev);
+ if (retval)
+ break;
+ }
+ /* save board and rev data in the rmi_driver_data */
+ drvdata-board = board;
+ drvdata-rev = rev;
+ dev_dbg(dev, Rmi_driver getProdID, set board: %d rev: %d\n,
+ drvdata-board, drvdata-rev);
+}
+
[snipped]
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On 11/26/2012 10:41 AM, Benjamin Tissoires wrote:
Hi Christopher,
On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny che...@synaptics.com wrote:
rmi_bus.c implements the basic functionality of the RMI bus. This file is
greatly simplified compared to the previous patch - we've switched from
do it yourself device/driver binding to using device_type to distinguish
between the two kinds of devices on the bus (sensor devices and function
specific devices) and using the standard bus implementation to manage devices
and drivers.
rmi_driver.c is a driver for the general functionality of the RMI sensor as a
whole, managing those behaviors (including IRQ handling) that are not specific
to any RMI4 function. It has some unavoidable dependencies on F01 behavior,
though we have worked to minimize those as far as possible.
The header file rmi_driver.h provides definitions that are shared among
the modules of the RMI implementation, but not thought to be necessary
outside it.
Greg KH - Linus Walleij recommended that we seek your input on these core
files, particularly the bus implementation.
Signed-off-by: Christopher Heiny che...@synaptics.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Dmitry Torokhov dmitry.torok...@gmail.com
Cc: Linus Walleij linus.wall...@stericsson.com
Cc: Naveen Kumar Gaddipati naveen.gaddip...@stericsson.com
Cc: Joeri de Gram j.de.g...@gmail.com
---
drivers/input/rmi4/rmi_bus.c| 248 ++
drivers/input/rmi4/rmi_driver.c | 1663 +++
drivers/input/rmi4/rmi_driver.h | 139
include/uapi/linux/input.h |1 +
4 files changed, 2051 insertions(+), 0 deletions(-)
[snipped]
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
new file mode 100644
index 000..05a73ae
--- /dev/null
+++ b/drivers/input/rmi4/rmi_driver.c
[snipped]
+/* extract product ID */
+void get_prod_id(struct rmi_device *rmi_dev, struct rmi_driver_data *drvdata)
+{
+ struct device *dev = rmi_dev-dev;
+ int retval;
+ int board = 0, rev = 0;
+ int i;
+ static const char * const pattern[] = {
+ tm%4d-%d, s%4d-%d, s%4d-ver%1d};
+ u8 product_id[RMI_PRODUCT_ID_LENGTH+1];
+
+ retval = rmi_read_block(rmi_dev,
+ drvdata-f01_container-fd.query_base_addr+
+ sizeof(struct f01_basic_queries),
+ product_id, RMI_PRODUCT_ID_LENGTH);
+ if (retval 0) {
+ dev_err(dev, Failed to read product id, code=%d!, retval);
+ return;
+ }
+ product_id[RMI_PRODUCT_ID_LENGTH] = '\0';
+
+ for (i = 0; i sizeof(product_id); i++)
+ product_id[i] = tolower(product_id[i]);
+
+ for (i = 0; i sizeof(pattern); i++) {
This should be ARRAY_SIZE(pattern).
It gave me a wonderful kernel oops :)
Yep, that's a bug! Oddly enough, it runs without barfing on my systems
(though who knows what horrible things are happening under the hood).
Thanks for letting us know.
Chris
Cheers,
Benjamin
[snip]
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On 11/17/2012 01:54 PM, Greg Kroah-Hartman wrote:
On Fri, Nov 16, 2012 at 07:58:50PM -0800, Christopher Heiny wrote:
+static void release_rmidev_device(struct device *dev)
+{
+ device_unregister(dev);
+}
You just leaked memory here, right?
Also, you already unregistered the device, otherwise this function would
have never been called, so you just ended up in a loop?
Roger. We'll fix that.
Have you ever tried removing a device? Are you sure it's working
properly?
H. If it leads to the loop you mention above, then the test I'm
using must not be doing what I thought it was. I'll fix that, too.
+EXPORT_SYMBOL(rmi_register_phys_device);
Just curious, but why not EXPORT_SYMBOL_GPL() on all of these new
symbols you are creating?
We'll change that.
Thanks very much!
Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On 11/17/2012 01:54 PM, Greg Kroah-Hartman wrote:
On Fri, Nov 16, 2012 at 07:58:50PM -0800, Christopher Heiny wrote:
+static void release_rmidev_device(struct device *dev)
+{
+ device_unregister(dev);
+}
You just leaked memory here, right?
Also, you already unregistered the device, otherwise this function would
have never been called, so you just ended up in a loop?
Roger. We'll fix that.
Have you ever tried removing a device? Are you sure it's working
properly?
H. If it leads to the loop you mention above, then the test I'm
using must not be doing what I thought it was. I'll fix that, too.
+EXPORT_SYMBOL(rmi_register_phys_device);
Just curious, but why not EXPORT_SYMBOL_GPL() on all of these new
symbols you are creating?
We'll change that.
Thanks very much!
Chris
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny wrote: > rmi_bus.c implements the basic functionality of the RMI bus. This file is > greatly simplified compared to the previous patch - we've switched from > "do it yourself" device/driver binding to using device_type to distinguish > between the two kinds of devices on the bus (sensor devices and function > specific devices) and using the standard bus implementation to manage devices > and drivers. I have no further comments, I have reviewed up to my level of competence. Next you need to fix comments from Greg & Dmitry, which will probably be just as demanding :-) But FWIW: Reviewed-by: Linus Walleij Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Fri, Nov 16, 2012 at 07:58:50PM -0800, Christopher Heiny wrote:
> +static void release_rmidev_device(struct device *dev)
> +{
> + device_unregister(dev);
> +}
You just leaked memory here, right?
Also, you already unregistered the device, otherwise this function would
have never been called, so you just ended up in a loop?
Have you ever tried removing a device? Are you sure it's working
properly?
> +EXPORT_SYMBOL(rmi_register_phys_device);
Just curious, but why not EXPORT_SYMBOL_GPL() on all of these new
symbols you are creating?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Fri, Nov 16, 2012 at 07:58:50PM -0800, Christopher Heiny wrote:
+static void release_rmidev_device(struct device *dev)
+{
+ device_unregister(dev);
+}
You just leaked memory here, right?
Also, you already unregistered the device, otherwise this function would
have never been called, so you just ended up in a loop?
Have you ever tried removing a device? Are you sure it's working
properly?
+EXPORT_SYMBOL(rmi_register_phys_device);
Just curious, but why not EXPORT_SYMBOL_GPL() on all of these new
symbols you are creating?
thanks,
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny che...@synaptics.com wrote: rmi_bus.c implements the basic functionality of the RMI bus. This file is greatly simplified compared to the previous patch - we've switched from do it yourself device/driver binding to using device_type to distinguish between the two kinds of devices on the bus (sensor devices and function specific devices) and using the standard bus implementation to manage devices and drivers. I have no further comments, I have reviewed up to my level of competence. Next you need to fix comments from Greg Dmitry, which will probably be just as demanding :-) But FWIW: Reviewed-by: Linus Walleij linus.wall...@linaro.org Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 02/06] input/rmi4: Core files
rmi_bus.c implements the basic functionality of the RMI bus. This file is
greatly simplified compared to the previous patch - we've switched from
"do it yourself" device/driver binding to using device_type to distinguish
between the two kinds of devices on the bus (sensor devices and function
specific devices) and using the standard bus implementation to manage devices
and drivers.
rmi_driver.c is a driver for the general functionality of the RMI sensor as a
whole, managing those behaviors (including IRQ handling) that are not specific
to any RMI4 function. It has some unavoidable dependencies on F01 behavior,
though we have worked to minimize those as far as possible.
The header file rmi_driver.h provides definitions that are shared among
the modules of the RMI implementation, but not thought to be necessary
outside it.
Greg KH - Linus Walleij recommended that we seek your input on these core
files, particularly the bus implementation.
Signed-off-by: Christopher Heiny
Cc: Greg Kroah-Hartman
Cc: Dmitry Torokhov
Cc: Linus Walleij
Cc: Naveen Kumar Gaddipati
Cc: Joeri de Gram
---
drivers/input/rmi4/rmi_bus.c| 248 ++
drivers/input/rmi4/rmi_driver.c | 1663 +++
drivers/input/rmi4/rmi_driver.h | 139
include/uapi/linux/input.h |1 +
4 files changed, 2051 insertions(+), 0 deletions(-)
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
new file mode 100644
index 000..a912349
--- /dev/null
+++ b/drivers/input/rmi4/rmi_bus.c
@@ -0,0 +1,248 @@
+/*
+ * Copyright (c) 2011, 2012 Synaptics Incorporated
+ * Copyright (c) 2011 Unixphere
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "rmi_driver.h"
+
+DEFINE_MUTEX(rmi_bus_mutex);
+
+static struct attribute *function_dev_attrs[] = {
+ NULL,
+};
+
+static struct attribute_group function_dev_attr_group = {
+ .attrs = function_dev_attrs,
+};
+
+static const struct attribute_group *function_dev_attr_groups[] = {
+ _dev_attr_group,
+ NULL,
+};
+
+struct device_type rmi_function_type = {
+ .name = "rmi_function",
+ .groups = function_dev_attr_groups,
+};
+EXPORT_SYMBOL_GPL(rmi_function_type);
+
+static struct attribute *sensor_dev_attrs[] = {
+ NULL,
+};
+static struct attribute_group sensor_dev_attr_group = {
+ .attrs = sensor_dev_attrs,
+};
+
+static const struct attribute_group *sensor_dev_attr_groups[] = {
+ _dev_attr_group,
+ NULL,
+};
+
+struct device_type rmi_sensor_type = {
+ .name = "rmi_sensor",
+ .groups = sensor_dev_attr_groups,
+};
+EXPORT_SYMBOL_GPL(rmi_sensor_type);
+
+static atomic_t physical_device_count = ATOMIC_INIT(0);
+
+#ifdef CONFIG_RMI4_DEBUG
+static struct dentry *rmi_debugfs_root;
+#endif
+
+#ifdef CONFIG_PM
+static int rmi_bus_suspend(struct device *dev)
+{
+ struct device_driver *driver = dev->driver;
+ const struct dev_pm_ops *pm;
+
+ if (!driver)
+ return 0;
+
+ pm = driver->pm;
+ if (pm && pm->suspend)
+ return pm->suspend(dev);
+ if (driver->suspend)
+ return driver->suspend(dev, PMSG_SUSPEND);
+
+ return 0;
+}
+
+static int rmi_bus_resume(struct device *dev)
+{
+ struct device_driver *driver = dev->driver;
+ const struct dev_pm_ops *pm;
+
+ if (!driver)
+ return 0;
+
+ pm = driver->pm;
+ if (pm && pm->resume)
+ return pm->resume(dev);
+ if (driver->resume)
+ return driver->resume(dev);
+
+ return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(rmi_bus_pm_ops,
+rmi_bus_suspend, rmi_bus_resume);
+
+struct bus_type rmi_bus_type = {
+ .name = "rmi",
+ .pm = _bus_pm_ops
+};
+EXPORT_SYMBOL_GPL(rmi_bus_type);
+
+static void release_rmidev_device(struct device *dev)
+{
+ device_unregister(dev);
+}
+
+/**
+ * rmi_register_phys_device - register a physical device connection on the RMI
+ * bus. Physical drivers provide communication from the devices on the bus to
+ * the RMI4 sensor on a bus such as SPI, I2C, and so on.
+ *
+ * @phys: the physical device to register
+ */
+int
[RFC PATCH 02/06] input/rmi4: Core files
rmi_bus.c implements the basic functionality of the RMI bus. This file is
greatly simplified compared to the previous patch - we've switched from
do it yourself device/driver binding to using device_type to distinguish
between the two kinds of devices on the bus (sensor devices and function
specific devices) and using the standard bus implementation to manage devices
and drivers.
rmi_driver.c is a driver for the general functionality of the RMI sensor as a
whole, managing those behaviors (including IRQ handling) that are not specific
to any RMI4 function. It has some unavoidable dependencies on F01 behavior,
though we have worked to minimize those as far as possible.
The header file rmi_driver.h provides definitions that are shared among
the modules of the RMI implementation, but not thought to be necessary
outside it.
Greg KH - Linus Walleij recommended that we seek your input on these core
files, particularly the bus implementation.
Signed-off-by: Christopher Heiny che...@synaptics.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Dmitry Torokhov dmitry.torok...@gmail.com
Cc: Linus Walleij linus.wall...@stericsson.com
Cc: Naveen Kumar Gaddipati naveen.gaddip...@stericsson.com
Cc: Joeri de Gram j.de.g...@gmail.com
---
drivers/input/rmi4/rmi_bus.c| 248 ++
drivers/input/rmi4/rmi_driver.c | 1663 +++
drivers/input/rmi4/rmi_driver.h | 139
include/uapi/linux/input.h |1 +
4 files changed, 2051 insertions(+), 0 deletions(-)
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
new file mode 100644
index 000..a912349
--- /dev/null
+++ b/drivers/input/rmi4/rmi_bus.c
@@ -0,0 +1,248 @@
+/*
+ * Copyright (c) 2011, 2012 Synaptics Incorporated
+ * Copyright (c) 2011 Unixphere
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include linux/kernel.h
+#include linux/device.h
+#include linux/kconfig.h
+#include linux/list.h
+#include linux/pm.h
+#include linux/rmi.h
+#include linux/slab.h
+#include linux/types.h
+#include linux/debugfs.h
+#include rmi_driver.h
+
+DEFINE_MUTEX(rmi_bus_mutex);
+
+static struct attribute *function_dev_attrs[] = {
+ NULL,
+};
+
+static struct attribute_group function_dev_attr_group = {
+ .attrs = function_dev_attrs,
+};
+
+static const struct attribute_group *function_dev_attr_groups[] = {
+ function_dev_attr_group,
+ NULL,
+};
+
+struct device_type rmi_function_type = {
+ .name = rmi_function,
+ .groups = function_dev_attr_groups,
+};
+EXPORT_SYMBOL_GPL(rmi_function_type);
+
+static struct attribute *sensor_dev_attrs[] = {
+ NULL,
+};
+static struct attribute_group sensor_dev_attr_group = {
+ .attrs = sensor_dev_attrs,
+};
+
+static const struct attribute_group *sensor_dev_attr_groups[] = {
+ sensor_dev_attr_group,
+ NULL,
+};
+
+struct device_type rmi_sensor_type = {
+ .name = rmi_sensor,
+ .groups = sensor_dev_attr_groups,
+};
+EXPORT_SYMBOL_GPL(rmi_sensor_type);
+
+static atomic_t physical_device_count = ATOMIC_INIT(0);
+
+#ifdef CONFIG_RMI4_DEBUG
+static struct dentry *rmi_debugfs_root;
+#endif
+
+#ifdef CONFIG_PM
+static int rmi_bus_suspend(struct device *dev)
+{
+ struct device_driver *driver = dev-driver;
+ const struct dev_pm_ops *pm;
+
+ if (!driver)
+ return 0;
+
+ pm = driver-pm;
+ if (pm pm-suspend)
+ return pm-suspend(dev);
+ if (driver-suspend)
+ return driver-suspend(dev, PMSG_SUSPEND);
+
+ return 0;
+}
+
+static int rmi_bus_resume(struct device *dev)
+{
+ struct device_driver *driver = dev-driver;
+ const struct dev_pm_ops *pm;
+
+ if (!driver)
+ return 0;
+
+ pm = driver-pm;
+ if (pm pm-resume)
+ return pm-resume(dev);
+ if (driver-resume)
+ return driver-resume(dev);
+
+ return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(rmi_bus_pm_ops,
+rmi_bus_suspend, rmi_bus_resume);
+
+struct bus_type rmi_bus_type = {
+ .name = rmi,
+ .pm = rmi_bus_pm_ops
+};
+EXPORT_SYMBOL_GPL(rmi_bus_type);
+
+static void release_rmidev_device(struct device *dev)
+{
+ device_unregister(dev);
+}
+
+/**
+ * rmi_register_phys_device -
Re: [RFC PATCH 02/06] input/rmi4: Core files
On 10/23/2012 05:11 PM, Dmitry Torokhov wrote:
On Tuesday, October 23, 2012 04:46:28 PM Christopher Heiny wrote:
On 10/11/2012 01:13 AM, Dmitry Torokhov wrote:
On Thu, Oct 11, 2012 at 04:15:56AM +, Christopher Heiny wrote:
On Thursday, October 11, 2012 02:21:53 AM you wrote:
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny
wrote:
[snip]
+static int process_interrupt_requests(struct rmi_device *rmi_dev)
+{
+ struct rmi_driver_data *data = dev_get_drvdata(_dev->dev);
+ struct device *dev = _dev->dev;
+ struct rmi_function_container *entry;
+ u8 irq_status[data->num_of_irq_regs];
Looking at this...
What does the data->num_of_irq_regs actually contain?
I just fear that it is something constant like always 2 or always 4,
so there is actually, in reality, a 16 or 32 bit register hiding in
there.
In that case what you should do is to represent it as a u16 or u32 here,
just or the bits into a status word, and then walk over that status
word with something like ffs(bitword); ...
Nope, it's not constant. In theory, and RMI4 based sensor can have up
to 128 functions (in practice, it's far fewer), and each function can
have as many as 7 interrupts. So the number of IRQ registers can vary
from RMI4 sensor to RMI4 sensor, and needs to be computed during the
scan of the product descriptor table.
Is it a good idea to have it on stack then? Should it be part of
rmi_device instead?
It's not coming off the stack. We're allocating it via devm_kzalloc()
in rmi_driver_probe().
No, look at the part of the code that was quoted. "u8 irq_status[data-
num_of_irq_regs];" is on stack.
Sorry - I thought you were referring to data->num_of_irq_regs rather
than irq_status. We'll move that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Tuesday, October 23, 2012 04:46:28 PM Christopher Heiny wrote:
> On 10/11/2012 01:13 AM, Dmitry Torokhov wrote:
> > On Thu, Oct 11, 2012 at 04:15:56AM +, Christopher Heiny wrote:
> >> On Thursday, October 11, 2012 02:21:53 AM you wrote:
> >>> On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny
wrote:
> +
> +/** This is here because all those casts made for some ugly code.
> + */
> +static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
> +{
> + bitmap_and((long unsigned int *) dest,
> + (long unsigned int *) target1,
> + (long unsigned int *) target2,
> + nbits);
> +}
> >>>
> >>> Hm, getting rid of unreadable casts is a valid case.
> >>>
> >>> I'll be OK with this but maybe the real solution is to introduce such
> >>> helpers into ?
> >>
> >> Hmmm. We'll give that some thought. Thought I'd like to get the RMI4
> >> driver nailed down, just to keep the area of change small. Once we've
> >> got all the kinks worked out here, we'll look at bitmap.h helpers.
> >
> > The question is why you are using u8 for bitmaps instead of doing
> > DECALRE_BITMAP() and using it instead? Then you would not need silly
> > wrappers around existing APIs.
>
> OK, we'll look into that. My big concern is whether the bit-order in
> bitmask.h will be the same as the bit order in the RMI4 sensor
> registers. If that works out OK, we'll switch.
I think if you properly convert data to/from cpu-endianness it will clear
matters a lot.
>
> >>> (...)
> >>>
> +static int process_interrupt_requests(struct rmi_device *rmi_dev)
> +{
> + struct rmi_driver_data *data = dev_get_drvdata(_dev->dev);
> + struct device *dev = _dev->dev;
> + struct rmi_function_container *entry;
> + u8 irq_status[data->num_of_irq_regs];
> >>>
> >>> Looking at this...
> >>>
> >>> What does the data->num_of_irq_regs actually contain?
> >>>
> >>> I just fear that it is something constant like always 2 or always 4,
> >>> so there is actually, in reality, a 16 or 32 bit register hiding in
> >>> there.
> >>>
> >>> In that case what you should do is to represent it as a u16 or u32 here,
> >>> just or the bits into a status word, and then walk over that status
> >>> word with something like ffs(bitword); ...
> >>
> >> Nope, it's not constant. In theory, and RMI4 based sensor can have up
> >> to 128 functions (in practice, it's far fewer), and each function can
> >> have as many as 7 interrupts. So the number of IRQ registers can vary
> >> from RMI4 sensor to RMI4 sensor, and needs to be computed during the
> >> scan of the product descriptor table.
> >
> > Is it a good idea to have it on stack then? Should it be part of
> > rmi_device instead?
>
> It's not coming off the stack. We're allocating it via devm_kzalloc()
> in rmi_driver_probe().
No, look at the part of the code that was quoted. "u8 irq_status[data-
>num_of_irq_regs];" is on stack.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On 10/11/2012 01:13 AM, Dmitry Torokhov wrote:
On Thu, Oct 11, 2012 at 04:15:56AM +, Christopher Heiny wrote:
On Thursday, October 11, 2012 02:21:53 AM you wrote:
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny wrote:
+
+/** This is here because all those casts made for some ugly code.
+ */
+static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
+{
+ bitmap_and((long unsigned int *) dest,
+ (long unsigned int *) target1,
+ (long unsigned int *) target2,
+ nbits);
+}
Hm, getting rid of unreadable casts is a valid case.
I'll be OK with this but maybe the real solution is to introduce such
helpers into ?
Hmmm. We'll give that some thought. Thought I'd like to get the RMI4
driver nailed down, just to keep the area of change small. Once we've
got all the kinks worked out here, we'll look at bitmap.h helpers.
The question is why you are using u8 for bitmaps instead of doing
DECALRE_BITMAP() and using it instead? Then you would not need silly
wrappers around existing APIs.
OK, we'll look into that. My big concern is whether the bit-order in
bitmask.h will be the same as the bit order in the RMI4 sensor
registers. If that works out OK, we'll switch.
(...)
+static int process_interrupt_requests(struct rmi_device *rmi_dev)
+{
+ struct rmi_driver_data *data = dev_get_drvdata(_dev->dev);
+ struct device *dev = _dev->dev;
+ struct rmi_function_container *entry;
+ u8 irq_status[data->num_of_irq_regs];
Looking at this...
What does the data->num_of_irq_regs actually contain?
I just fear that it is something constant like always 2 or always 4,
so there is actually, in reality, a 16 or 32 bit register hiding in there.
In that case what you should do is to represent it as a u16 or u32 here,
just or the bits into a status word, and then walk over that status
word with something like ffs(bitword); ...
Nope, it's not constant. In theory, and RMI4 based sensor can have up
to 128 functions (in practice, it's far fewer), and each function can
have as many as 7 interrupts. So the number of IRQ registers can vary
from RMI4 sensor to RMI4 sensor, and needs to be computed during the
scan of the product descriptor table.
Is it a good idea to have it on stack then? Should it be part of
rmi_device instead?
It's not coming off the stack. We're allocating it via devm_kzalloc()
in rmi_driver_probe().
+#define simple_show_union_struct(regtype, propname, fmt)\
+static ssize_t tricat(rmi_fn_, FNUM, _##propname##_show)(struct device
*dev,\ + struct device_attribute *attr,
char *buf) {\ + struct rmi_function_container *fc;\
+ struct FUNCTION_DATA *data;\
+\
+ fc = to_rmi_function_container(dev);\
+ data = fc->data;\
+\
+ return snprintf(buf, PAGE_SIZE, fmt,\
+ data->regtype.propname);\
+}
OK I see the point, but is there really no other way to do this than
to #define huge static inlines like these? Is it really not possible to
create just generic functions instead of going this far?
(same comment for all)
We tried generic functions previously, and it wound up really really ugly.
We'd be willing to look at it again, though, since this isn't real beautiful
either. If you've got an example implementation in mind. a pointer would help
a great deal.
You just need to wrap around a custome structure around struct
device_attribute and then you shoudl be able to use generics. If you
look into trackpoint.c you should gett the idea.
That looks a lot tidier. We'll work on it for sysfs (and maybe for
debugfs, too). It might not be applicable in all cases, but it promises
to make a lot of things tidier.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On 10/11/2012 01:13 AM, Dmitry Torokhov wrote:
On Thu, Oct 11, 2012 at 04:15:56AM +, Christopher Heiny wrote:
On Thursday, October 11, 2012 02:21:53 AM you wrote:
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com wrote:
+
+/** This is here because all those casts made for some ugly code.
+ */
+static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
+{
+ bitmap_and((long unsigned int *) dest,
+ (long unsigned int *) target1,
+ (long unsigned int *) target2,
+ nbits);
+}
Hm, getting rid of unreadable casts is a valid case.
I'll be OK with this but maybe the real solution is to introduce such
helpers into linux/bitmap.h?
Hmmm. We'll give that some thought. Thought I'd like to get the RMI4
driver nailed down, just to keep the area of change small. Once we've
got all the kinks worked out here, we'll look at bitmap.h helpers.
The question is why you are using u8 for bitmaps instead of doing
DECALRE_BITMAP() and using it instead? Then you would not need silly
wrappers around existing APIs.
OK, we'll look into that. My big concern is whether the bit-order in
bitmask.h will be the same as the bit order in the RMI4 sensor
registers. If that works out OK, we'll switch.
(...)
+static int process_interrupt_requests(struct rmi_device *rmi_dev)
+{
+ struct rmi_driver_data *data = dev_get_drvdata(rmi_dev-dev);
+ struct device *dev = rmi_dev-dev;
+ struct rmi_function_container *entry;
+ u8 irq_status[data-num_of_irq_regs];
Looking at this...
What does the data-num_of_irq_regs actually contain?
I just fear that it is something constant like always 2 or always 4,
so there is actually, in reality, a 16 or 32 bit register hiding in there.
In that case what you should do is to represent it as a u16 or u32 here,
just or the bits into a status word, and then walk over that status
word with something like ffs(bitword); ...
Nope, it's not constant. In theory, and RMI4 based sensor can have up
to 128 functions (in practice, it's far fewer), and each function can
have as many as 7 interrupts. So the number of IRQ registers can vary
from RMI4 sensor to RMI4 sensor, and needs to be computed during the
scan of the product descriptor table.
Is it a good idea to have it on stack then? Should it be part of
rmi_device instead?
It's not coming off the stack. We're allocating it via devm_kzalloc()
in rmi_driver_probe().
+#define simple_show_union_struct(regtype, propname, fmt)\
+static ssize_t tricat(rmi_fn_, FNUM, _##propname##_show)(struct device
*dev,\ + struct device_attribute *attr,
char *buf) {\ + struct rmi_function_container *fc;\
+ struct FUNCTION_DATA *data;\
+\
+ fc = to_rmi_function_container(dev);\
+ data = fc-data;\
+\
+ return snprintf(buf, PAGE_SIZE, fmt,\
+ data-regtype.propname);\
+}
OK I see the point, but is there really no other way to do this than
to #define huge static inlines like these? Is it really not possible to
create just generic functions instead of going this far?
(same comment for all)
We tried generic functions previously, and it wound up really really ugly.
We'd be willing to look at it again, though, since this isn't real beautiful
either. If you've got an example implementation in mind. a pointer would help
a great deal.
You just need to wrap around a custome structure around struct
device_attribute and then you shoudl be able to use generics. If you
look into trackpoint.c you should gett the idea.
That looks a lot tidier. We'll work on it for sysfs (and maybe for
debugfs, too). It might not be applicable in all cases, but it promises
to make a lot of things tidier.
Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Tuesday, October 23, 2012 04:46:28 PM Christopher Heiny wrote:
On 10/11/2012 01:13 AM, Dmitry Torokhov wrote:
On Thu, Oct 11, 2012 at 04:15:56AM +, Christopher Heiny wrote:
On Thursday, October 11, 2012 02:21:53 AM you wrote:
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com
wrote:
+
+/** This is here because all those casts made for some ugly code.
+ */
+static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
+{
+ bitmap_and((long unsigned int *) dest,
+ (long unsigned int *) target1,
+ (long unsigned int *) target2,
+ nbits);
+}
Hm, getting rid of unreadable casts is a valid case.
I'll be OK with this but maybe the real solution is to introduce such
helpers into linux/bitmap.h?
Hmmm. We'll give that some thought. Thought I'd like to get the RMI4
driver nailed down, just to keep the area of change small. Once we've
got all the kinks worked out here, we'll look at bitmap.h helpers.
The question is why you are using u8 for bitmaps instead of doing
DECALRE_BITMAP() and using it instead? Then you would not need silly
wrappers around existing APIs.
OK, we'll look into that. My big concern is whether the bit-order in
bitmask.h will be the same as the bit order in the RMI4 sensor
registers. If that works out OK, we'll switch.
I think if you properly convert data to/from cpu-endianness it will clear
matters a lot.
(...)
+static int process_interrupt_requests(struct rmi_device *rmi_dev)
+{
+ struct rmi_driver_data *data = dev_get_drvdata(rmi_dev-dev);
+ struct device *dev = rmi_dev-dev;
+ struct rmi_function_container *entry;
+ u8 irq_status[data-num_of_irq_regs];
Looking at this...
What does the data-num_of_irq_regs actually contain?
I just fear that it is something constant like always 2 or always 4,
so there is actually, in reality, a 16 or 32 bit register hiding in
there.
In that case what you should do is to represent it as a u16 or u32 here,
just or the bits into a status word, and then walk over that status
word with something like ffs(bitword); ...
Nope, it's not constant. In theory, and RMI4 based sensor can have up
to 128 functions (in practice, it's far fewer), and each function can
have as many as 7 interrupts. So the number of IRQ registers can vary
from RMI4 sensor to RMI4 sensor, and needs to be computed during the
scan of the product descriptor table.
Is it a good idea to have it on stack then? Should it be part of
rmi_device instead?
It's not coming off the stack. We're allocating it via devm_kzalloc()
in rmi_driver_probe().
No, look at the part of the code that was quoted. u8 irq_status[data-
num_of_irq_regs]; is on stack.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On 10/23/2012 05:11 PM, Dmitry Torokhov wrote:
On Tuesday, October 23, 2012 04:46:28 PM Christopher Heiny wrote:
On 10/11/2012 01:13 AM, Dmitry Torokhov wrote:
On Thu, Oct 11, 2012 at 04:15:56AM +, Christopher Heiny wrote:
On Thursday, October 11, 2012 02:21:53 AM you wrote:
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com
wrote:
[snip]
+static int process_interrupt_requests(struct rmi_device *rmi_dev)
+{
+ struct rmi_driver_data *data = dev_get_drvdata(rmi_dev-dev);
+ struct device *dev = rmi_dev-dev;
+ struct rmi_function_container *entry;
+ u8 irq_status[data-num_of_irq_regs];
Looking at this...
What does the data-num_of_irq_regs actually contain?
I just fear that it is something constant like always 2 or always 4,
so there is actually, in reality, a 16 or 32 bit register hiding in
there.
In that case what you should do is to represent it as a u16 or u32 here,
just or the bits into a status word, and then walk over that status
word with something like ffs(bitword); ...
Nope, it's not constant. In theory, and RMI4 based sensor can have up
to 128 functions (in practice, it's far fewer), and each function can
have as many as 7 interrupts. So the number of IRQ registers can vary
from RMI4 sensor to RMI4 sensor, and needs to be computed during the
scan of the product descriptor table.
Is it a good idea to have it on stack then? Should it be part of
rmi_device instead?
It's not coming off the stack. We're allocating it via devm_kzalloc()
in rmi_driver_probe().
No, look at the part of the code that was quoted. u8 irq_status[data-
num_of_irq_regs]; is on stack.
Sorry - I thought you were referring to data-num_of_irq_regs rather
than irq_status. We'll move that.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On 10/10/2012 08:06 PM, Joe Perches wrote:
On Thu, 2012-10-11 at 02:49 +, Christopher Heiny wrote:
Joe Perches wrote:
[]
+ list_for_each_entry(entry, >rmi_functions.list, list)
+ if (entry->irq_mask)
+ process_one_interrupt(entry, irq_status,
+ data);
style nit, it'd be nicer with braces.
I agree with you, but checkpatch.pl doesn't. :-(
Sure it does.
$ cat t.c
{
list_for_each_entry(entry, >rmi_functions.list, list) {
if (entry->irq_mask)
process_one_interrupt(entry, irq_status, data);
}
}
$ ./scripts/checkpatch.pl --strict -f t.c
total: 0 errors, 0 warnings, 0 checks, 7 lines checked
t.c has no obvious style problems and is ready for submission.
I stand corrected. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On 10/10/2012 08:06 PM, Joe Perches wrote:
On Thu, 2012-10-11 at 02:49 +, Christopher Heiny wrote:
Joe Perches wrote:
[]
+ list_for_each_entry(entry, data-rmi_functions.list, list)
+ if (entry-irq_mask)
+ process_one_interrupt(entry, irq_status,
+ data);
style nit, it'd be nicer with braces.
I agree with you, but checkpatch.pl doesn't. :-(
Sure it does.
$ cat t.c
{
list_for_each_entry(entry, data-rmi_functions.list, list) {
if (entry-irq_mask)
process_one_interrupt(entry, irq_status, data);
}
}
$ ./scripts/checkpatch.pl --strict -f t.c
total: 0 errors, 0 warnings, 0 checks, 7 lines checked
t.c has no obvious style problems and is ready for submission.
I stand corrected. Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Thu, Oct 11, 2012 at 6:15 AM, Christopher Heiny wrote:
> On Thursday, October 11, 2012 02:21:53 AM I wrote:
>> > +union pdt_properties {
>> > + struct {
>> > + u8 reserved_1:6;
>> > + u8 has_bsr:1;
>> > + u8 reserved_2:1;
>> > + } __attribute__((__packed__));
>> > + u8 regs[1];
>>
>> I don't understand what this union is trying to achieve.
>>
>> regs[1] does not look right considering what you're trying to
>> achieve. Since the above fields require a regs[2] (9 bits!)
>> to be stored. Maybe write out what you're trying to do here
>> so I can understand it? (If everyone else in the world gets
>> it immediately, it's maybe me that need fixing instead...)
>>
>> Apart from these remarks it's looking real nice now!
>
> I only count 8 bits there, unless there's something about
> packing I'm not aware of. Is there something else you
> found confusing about the union?
I just did bad maths, too many figured in the struct...
But consider Dmitry's suggestion that you might get rid
of this unionizing.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Thu, Oct 11, 2012 at 04:15:56AM +, Christopher Heiny wrote:
> On Thursday, October 11, 2012 02:21:53 AM you wrote:
> > On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny
> > wrote:
>
> >
> > > +
> > > +/** This is here because all those casts made for some ugly code.
> > > + */
> > > +static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
> > > +{
> > > + bitmap_and((long unsigned int *) dest,
> > > + (long unsigned int *) target1,
> > > + (long unsigned int *) target2,
> > > + nbits);
> > > +}
> >
> > Hm, getting rid of unreadable casts is a valid case.
> >
> > I'll be OK with this but maybe the real solution is to introduce such
> > helpers into ?
>
> Hmmm. We'll give that some thought. Thought I'd like to get the RMI4
> driver nailed down, just to keep the area of change small. Once we've
> got all the kinks worked out here, we'll look at bitmap.h helpers.
The question is why you are using u8 for bitmaps instead of doing
DECALRE_BITMAP() and using it instead? Then you would not need silly
wrappers around existing APIs.
>
> >
> > (...)
> >
> > > +static int process_interrupt_requests(struct rmi_device *rmi_dev)
> > > +{
> > > + struct rmi_driver_data *data = dev_get_drvdata(_dev->dev);
> > > + struct device *dev = _dev->dev;
> > > + struct rmi_function_container *entry;
> > > + u8 irq_status[data->num_of_irq_regs];
> >
> > Looking at this...
> >
> > What does the data->num_of_irq_regs actually contain?
> >
> > I just fear that it is something constant like always 2 or always 4,
> > so there is actually, in reality, a 16 or 32 bit register hiding in there.
> >
> > In that case what you should do is to represent it as a u16 or u32 here,
> > just or the bits into a status word, and then walk over that status
> > word with something like ffs(bitword); ...
>
> Nope, it's not constant. In theory, and RMI4 based sensor can have up
> to 128 functions (in practice, it's far fewer), and each function can
> have as many as 7 interrupts. So the number of IRQ registers can vary
> from RMI4 sensor to RMI4 sensor, and needs to be computed during the
> scan of the product descriptor table.
Is it a good idea to have it on stack then? Should it be part of
rmi_device instead?
> >
> > > +#define simple_show_union_struct(regtype, propname, fmt)\
> > > +static ssize_t tricat(rmi_fn_, FNUM, _##propname##_show)(struct device
> > > *dev,\ + struct device_attribute *attr,
> > > char *buf) {\ + struct rmi_function_container *fc;\
> > > + struct FUNCTION_DATA *data;\
> > > +\
> > > + fc = to_rmi_function_container(dev);\
> > > + data = fc->data;\
> > > +\
> > > + return snprintf(buf, PAGE_SIZE, fmt,\
> > > + data->regtype.propname);\
> > > +}
> >
> > OK I see the point, but is there really no other way to do this than
> > to #define huge static inlines like these? Is it really not possible to
> > create just generic functions instead of going this far?
> >
> > (same comment for all)
>
> We tried generic functions previously, and it wound up really really ugly.
> We'd be willing to look at it again, though, since this isn't real beautiful
> either. If you've got an example implementation in mind. a pointer would
> help a great deal.
You just need to wrap around a custome structure around struct
device_attribute and then you shoudl be able to use generics. If you
look into trackpoint.c you should gett the idea.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Thu, Oct 11, 2012 at 04:15:56AM +, Christopher Heiny wrote:
On Thursday, October 11, 2012 02:21:53 AM you wrote:
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com
wrote:
+
+/** This is here because all those casts made for some ugly code.
+ */
+static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
+{
+ bitmap_and((long unsigned int *) dest,
+ (long unsigned int *) target1,
+ (long unsigned int *) target2,
+ nbits);
+}
Hm, getting rid of unreadable casts is a valid case.
I'll be OK with this but maybe the real solution is to introduce such
helpers into linux/bitmap.h?
Hmmm. We'll give that some thought. Thought I'd like to get the RMI4
driver nailed down, just to keep the area of change small. Once we've
got all the kinks worked out here, we'll look at bitmap.h helpers.
The question is why you are using u8 for bitmaps instead of doing
DECALRE_BITMAP() and using it instead? Then you would not need silly
wrappers around existing APIs.
(...)
+static int process_interrupt_requests(struct rmi_device *rmi_dev)
+{
+ struct rmi_driver_data *data = dev_get_drvdata(rmi_dev-dev);
+ struct device *dev = rmi_dev-dev;
+ struct rmi_function_container *entry;
+ u8 irq_status[data-num_of_irq_regs];
Looking at this...
What does the data-num_of_irq_regs actually contain?
I just fear that it is something constant like always 2 or always 4,
so there is actually, in reality, a 16 or 32 bit register hiding in there.
In that case what you should do is to represent it as a u16 or u32 here,
just or the bits into a status word, and then walk over that status
word with something like ffs(bitword); ...
Nope, it's not constant. In theory, and RMI4 based sensor can have up
to 128 functions (in practice, it's far fewer), and each function can
have as many as 7 interrupts. So the number of IRQ registers can vary
from RMI4 sensor to RMI4 sensor, and needs to be computed during the
scan of the product descriptor table.
Is it a good idea to have it on stack then? Should it be part of
rmi_device instead?
+#define simple_show_union_struct(regtype, propname, fmt)\
+static ssize_t tricat(rmi_fn_, FNUM, _##propname##_show)(struct device
*dev,\ + struct device_attribute *attr,
char *buf) {\ + struct rmi_function_container *fc;\
+ struct FUNCTION_DATA *data;\
+\
+ fc = to_rmi_function_container(dev);\
+ data = fc-data;\
+\
+ return snprintf(buf, PAGE_SIZE, fmt,\
+ data-regtype.propname);\
+}
OK I see the point, but is there really no other way to do this than
to #define huge static inlines like these? Is it really not possible to
create just generic functions instead of going this far?
(same comment for all)
We tried generic functions previously, and it wound up really really ugly.
We'd be willing to look at it again, though, since this isn't real beautiful
either. If you've got an example implementation in mind. a pointer would
help a great deal.
You just need to wrap around a custome structure around struct
device_attribute and then you shoudl be able to use generics. If you
look into trackpoint.c you should gett the idea.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Thu, Oct 11, 2012 at 6:15 AM, Christopher Heiny che...@synaptics.com wrote:
On Thursday, October 11, 2012 02:21:53 AM I wrote:
+union pdt_properties {
+ struct {
+ u8 reserved_1:6;
+ u8 has_bsr:1;
+ u8 reserved_2:1;
+ } __attribute__((__packed__));
+ u8 regs[1];
I don't understand what this union is trying to achieve.
regs[1] does not look right considering what you're trying to
achieve. Since the above fields require a regs[2] (9 bits!)
to be stored. Maybe write out what you're trying to do here
so I can understand it? (If everyone else in the world gets
it immediately, it's maybe me that need fixing instead...)
Apart from these remarks it's looking real nice now!
I only count 8 bits there, unless there's something about
packing I'm not aware of. Is there something else you
found confusing about the union?
I just did bad maths, too many figured in the struct...
But consider Dmitry's suggestion that you might get rid
of this unionizing.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC PATCH 02/06] input/rmi4: Core files
On Thursday, October 11, 2012 02:21:53 AM you wrote:
> On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny
> wrote:
> > rmi_bus.c implements the basic functionality of the RMI bus. This file is
> > greatly simplified compared to the previous patch - we've switched from
> > "do it yourself" device/driver binding to using device_type to distinguish
> > between the two kinds of devices on the bus (sensor devices and function
> > specific devices) and using the standard bus implementation to manage
> > devices and drivers.
>
> So I think you really want Greg KH to look at this bus implementation
> now. Please include Greg on future mailings...
>
> It looks much improved from previous versions, and sorry if I am now
> adding even more comments, but it's because you cleared out some
> noise that was disturbing my perception so I can cleanly review
> the architecture of this thing now. (I'm impressed by your work and
> new high-speed turnaround time!)
Thanks for the praise - it means a lot to me and my team.
We'll cc Greg on the next patch drop.
[snip some items covered in a previous email]
>
> (...)
>
> > diff --git a/drivers/input/rmi4/rmi_driver.c
> > b/drivers/input/rmi4/rmi_driver.c
[snip some items covered in a previous email]
>
> > +#define DELAY_NAME "delay"
>
> This is only used in one place, why not just use the string
> "delay" there?
>
> (...)
>
> > + if (IS_ENABLED(CONFIG_RMI4_SPI) && !strncmp("spi", info->proto,
> > 3)) { + data->debugfs_delay =
> > debugfs_create_file(DELAY_NAME,
> > + RMI_RW_ATTR, rmi_dev->debugfs_root,
> > rmi_dev, + _fops);
>
> i.e. there.
That's a left-over. We'll consolidate it.
>
> (...)
>
> > +/* Useful helper functions for u8* */
> > +
> > +static bool u8_is_any_set(u8 *target, int size)
> > +{
> > + int i;
> > + /* We'd like to use find_first_bit, but it ALWAYS returns 1,
> > + * no matter what we pass it. So we have to do this the hard way.
> > + * return find_first_bit((long unsigned int *) target, size) !=
> > 0;
> > + */
> > + for (i = 0; i < size; i++) {
> > + if (target[i])
> > + return true;
> > + }
> > + return false;
> > +}
>
> Instead of:
>
> if (u8_is_any_set(foo, 128) {}
>
> Why can't you use:
>
> if (!bitmap_empty(foo, 128*8) {}
>
> ?
>
> If you look at the implementation in the header
> and __bitmap_empty() in lib/bitmap.c you will realize that this
> function is already optimized like this (and I actually don't think
> the RMI4 code is performance-critical for these functions anyway,
> but prove me wrong!)
We'll give !bitmap_empty a try.
>
> > +
> > +/** This is here because all those casts made for some ugly code.
> > + */
> > +static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
> > +{
> > + bitmap_and((long unsigned int *) dest,
> > + (long unsigned int *) target1,
> > + (long unsigned int *) target2,
> > + nbits);
> > +}
>
> Hm, getting rid of unreadable casts is a valid case.
>
> I'll be OK with this but maybe the real solution is to introduce such
> helpers into ?
Hmmm. We'll give that some thought. Thought I'd like to get the RMI4 driver
nailed down, just to keep the area of change small. Once we've got all the
kinks worked out here, we'll look at bitmap.h helpers.
>
> (...)
>
> > +static int process_interrupt_requests(struct rmi_device *rmi_dev)
> > +{
> > + struct rmi_driver_data *data = dev_get_drvdata(_dev->dev);
> > + struct device *dev = _dev->dev;
> > + struct rmi_function_container *entry;
> > + u8 irq_status[data->num_of_irq_regs];
>
> Looking at this...
>
> What does the data->num_of_irq_regs actually contain?
>
> I just fear that it is something constant like always 2 or always 4,
> so there is actually, in reality, a 16 or 32 bit register hiding in there.
>
> In that case what you should do is to represent it as a u16 or u32 here,
> just or the bits into a status word, and then walk over that status
> word with something like ffs(bitword); ...
Nope, it's not constant. In theory, and RMI4 based sensor can have up to 128
functions (in practice, it's far fewer), and each function can have as many as
7 interrupts. So the number of IRQ registers can vary from RMI4 sensor to RMI4
sensor, and needs to be computed during the scan of the product descriptor
table.
>
> (...)
>
> > +static int standard_resume(struct rmi_device *rmi_dev)
>
> Standard eh?
>
> Atleast prefix with rmi4_*...
Ooops - we excised the Android based stuff, but forgot to change that function
name.
>
> > +static int rmi_driver_suspend(struct device *dev)
> > +{
> > + struct rmi_device *rmi_dev = to_rmi_device(dev);
> > + return standard_suspend(rmi_dev);
> > +}
> > +
> > +static int rmi_driver_resume(struct device *dev)
> > +{
> > + struct
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Thu, 2012-10-11 at 02:49 +, Christopher Heiny wrote:
> Joe Perches wrote:
[]
> > > + list_for_each_entry(entry, >rmi_functions.list, list)
> > > + if (entry->irq_mask)
> > > + process_one_interrupt(entry, irq_status,
> > > + data);
> >
> > style nit, it'd be nicer with braces.
>
> I agree with you, but checkpatch.pl doesn't. :-(
Sure it does.
$ cat t.c
{
list_for_each_entry(entry, >rmi_functions.list, list) {
if (entry->irq_mask)
process_one_interrupt(entry, irq_status, data);
}
}
$ ./scripts/checkpatch.pl --strict -f t.c
total: 0 errors, 0 warnings, 0 checks, 7 lines checked
t.c has no obvious style problems and is ready for submission.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC PATCH 02/06] input/rmi4: Core files
Joe Perches wrote:
> On Fri, 2012-10-05 at 21:09 -0700, Christopher Heiny wrote:
> []
>
> Just some trivial comments:
Thanks - see below for responses.
> > diff --git a/drivers/input/rmi4/rmi_driver.c
> > b/drivers/input/rmi4/rmi_driver.c
> []
>
> > @@ -0,0 +1,1529 @@
>
> []
>
> > +static ssize_t delay_write(struct file *filp, const char __user *buffer,
> > +size_t size, loff_t *offset) {
> > + struct driver_debugfs_data *data = filp->private_data;
> > + struct rmi_device_platform_data *pdata =
> > + data->rmi_dev->phys->dev->platform_data;
> > + int retval;
> > + char local_buf[size];
> > + unsigned int new_read_delay;
> > + unsigned int new_write_delay;
> > + unsigned int new_block_delay;
> > + unsigned int new_pre_delay;
> > + unsigned int new_post_delay;
> > +
> > + retval = copy_from_user(local_buf, buffer, size);
> > + if (retval)
> > + return -EFAULT;
> > +
> > + retval = sscanf(local_buf, "%u %u %u %u %u", _read_delay,
> > + _write_delay, _block_delay,
> > + _pre_delay, _post_delay);
> > + if (retval != 5) {
> > + dev_err(>rmi_dev->dev,
> > + "Incorrect number of values provided for delay.");
> > + return -EINVAL;
> > + }
> > + if (new_read_delay < 0) {
>
> These are unnecessary tests as unsigned values are never < 0.
Right. Thought we'd taken care of most of the silliness like that, but
obviously missed some. We'll recheck the codebase.
[snip]
> > +static ssize_t phys_read(struct file *filp, char __user *buffer, size_t
> > size, + loff_t *offset) {
> > + struct driver_debugfs_data *data = filp->private_data;
> > + struct rmi_phys_info *info = >rmi_dev->phys->info;
> > + int retval;
> > + char local_buf[size];
>
> size comes from where? possible stack overflow?
H. Good point. We'll look at this.
[snip]
> []
>
> > + list_for_each_entry(entry, >rmi_functions.list, list)
> > + if (entry->irq_mask)
> > + process_one_interrupt(entry, irq_status,
> > + data);
>
> style nit, it'd be nicer with braces.
I agree with you, but checkpatch.pl doesn't. :-(
>
> > diff --git a/drivers/input/rmi4/rmi_driver.h
> > b/drivers/input/rmi4/rmi_driver.h
> []
>
> > @@ -0,0 +1,438 @@
> >
> > +
> > +#define tricat(x, y, z) tricat_(x, y, z)
> > +
> > +#define tricat_(x, y, z) x##y##z
>
> I think these tricat macros are merely obfuscating
> and don't need to be used.
tricat is used internally by another collection of macros that helps generate
sysfs files. In particular, it's used to generate the RMI4 register name
symbols.--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC PATCH 02/06] input/rmi4: Core files
Joe Perches wrote:
On Fri, 2012-10-05 at 21:09 -0700, Christopher Heiny wrote:
[]
Just some trivial comments:
Thanks - see below for responses.
diff --git a/drivers/input/rmi4/rmi_driver.c
b/drivers/input/rmi4/rmi_driver.c
[]
@@ -0,0 +1,1529 @@
[]
+static ssize_t delay_write(struct file *filp, const char __user *buffer,
+size_t size, loff_t *offset) {
+ struct driver_debugfs_data *data = filp-private_data;
+ struct rmi_device_platform_data *pdata =
+ data-rmi_dev-phys-dev-platform_data;
+ int retval;
+ char local_buf[size];
+ unsigned int new_read_delay;
+ unsigned int new_write_delay;
+ unsigned int new_block_delay;
+ unsigned int new_pre_delay;
+ unsigned int new_post_delay;
+
+ retval = copy_from_user(local_buf, buffer, size);
+ if (retval)
+ return -EFAULT;
+
+ retval = sscanf(local_buf, %u %u %u %u %u, new_read_delay,
+ new_write_delay, new_block_delay,
+ new_pre_delay, new_post_delay);
+ if (retval != 5) {
+ dev_err(data-rmi_dev-dev,
+ Incorrect number of values provided for delay.);
+ return -EINVAL;
+ }
+ if (new_read_delay 0) {
These are unnecessary tests as unsigned values are never 0.
Right. Thought we'd taken care of most of the silliness like that, but
obviously missed some. We'll recheck the codebase.
[snip]
+static ssize_t phys_read(struct file *filp, char __user *buffer, size_t
size, + loff_t *offset) {
+ struct driver_debugfs_data *data = filp-private_data;
+ struct rmi_phys_info *info = data-rmi_dev-phys-info;
+ int retval;
+ char local_buf[size];
size comes from where? possible stack overflow?
H. Good point. We'll look at this.
[snip]
[]
+ list_for_each_entry(entry, data-rmi_functions.list, list)
+ if (entry-irq_mask)
+ process_one_interrupt(entry, irq_status,
+ data);
style nit, it'd be nicer with braces.
I agree with you, but checkpatch.pl doesn't. :-(
diff --git a/drivers/input/rmi4/rmi_driver.h
b/drivers/input/rmi4/rmi_driver.h
[]
@@ -0,0 +1,438 @@
+
+#define tricat(x, y, z) tricat_(x, y, z)
+
+#define tricat_(x, y, z) x##y##z
I think these tricat macros are merely obfuscating
and don't need to be used.
tricat is used internally by another collection of macros that helps generate
sysfs files. In particular, it's used to generate the RMI4 register name
symbols.--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Thu, 2012-10-11 at 02:49 +, Christopher Heiny wrote:
Joe Perches wrote:
[]
+ list_for_each_entry(entry, data-rmi_functions.list, list)
+ if (entry-irq_mask)
+ process_one_interrupt(entry, irq_status,
+ data);
style nit, it'd be nicer with braces.
I agree with you, but checkpatch.pl doesn't. :-(
Sure it does.
$ cat t.c
{
list_for_each_entry(entry, data-rmi_functions.list, list) {
if (entry-irq_mask)
process_one_interrupt(entry, irq_status, data);
}
}
$ ./scripts/checkpatch.pl --strict -f t.c
total: 0 errors, 0 warnings, 0 checks, 7 lines checked
t.c has no obvious style problems and is ready for submission.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC PATCH 02/06] input/rmi4: Core files
On Thursday, October 11, 2012 02:21:53 AM you wrote:
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com
wrote:
rmi_bus.c implements the basic functionality of the RMI bus. This file is
greatly simplified compared to the previous patch - we've switched from
do it yourself device/driver binding to using device_type to distinguish
between the two kinds of devices on the bus (sensor devices and function
specific devices) and using the standard bus implementation to manage
devices and drivers.
So I think you really want Greg KH to look at this bus implementation
now. Please include Greg on future mailings...
It looks much improved from previous versions, and sorry if I am now
adding even more comments, but it's because you cleared out some
noise that was disturbing my perception so I can cleanly review
the architecture of this thing now. (I'm impressed by your work and
new high-speed turnaround time!)
Thanks for the praise - it means a lot to me and my team.
We'll cc Greg on the next patch drop.
[snip some items covered in a previous email]
(...)
diff --git a/drivers/input/rmi4/rmi_driver.c
b/drivers/input/rmi4/rmi_driver.c
[snip some items covered in a previous email]
+#define DELAY_NAME delay
This is only used in one place, why not just use the string
delay there?
(...)
+ if (IS_ENABLED(CONFIG_RMI4_SPI) !strncmp(spi, info-proto,
3)) { + data-debugfs_delay =
debugfs_create_file(DELAY_NAME,
+ RMI_RW_ATTR, rmi_dev-debugfs_root,
rmi_dev, + delay_fops);
i.e. there.
That's a left-over. We'll consolidate it.
(...)
+/* Useful helper functions for u8* */
+
+static bool u8_is_any_set(u8 *target, int size)
+{
+ int i;
+ /* We'd like to use find_first_bit, but it ALWAYS returns 1,
+ * no matter what we pass it. So we have to do this the hard way.
+ * return find_first_bit((long unsigned int *) target, size) !=
0;
+ */
+ for (i = 0; i size; i++) {
+ if (target[i])
+ return true;
+ }
+ return false;
+}
Instead of:
if (u8_is_any_set(foo, 128) {}
Why can't you use:
if (!bitmap_empty(foo, 128*8) {}
?
If you look at the implementation in the linux/bitmap.h header
and __bitmap_empty() in lib/bitmap.c you will realize that this
function is already optimized like this (and I actually don't think
the RMI4 code is performance-critical for these functions anyway,
but prove me wrong!)
We'll give !bitmap_empty a try.
+
+/** This is here because all those casts made for some ugly code.
+ */
+static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
+{
+ bitmap_and((long unsigned int *) dest,
+ (long unsigned int *) target1,
+ (long unsigned int *) target2,
+ nbits);
+}
Hm, getting rid of unreadable casts is a valid case.
I'll be OK with this but maybe the real solution is to introduce such
helpers into linux/bitmap.h?
Hmmm. We'll give that some thought. Thought I'd like to get the RMI4 driver
nailed down, just to keep the area of change small. Once we've got all the
kinks worked out here, we'll look at bitmap.h helpers.
(...)
+static int process_interrupt_requests(struct rmi_device *rmi_dev)
+{
+ struct rmi_driver_data *data = dev_get_drvdata(rmi_dev-dev);
+ struct device *dev = rmi_dev-dev;
+ struct rmi_function_container *entry;
+ u8 irq_status[data-num_of_irq_regs];
Looking at this...
What does the data-num_of_irq_regs actually contain?
I just fear that it is something constant like always 2 or always 4,
so there is actually, in reality, a 16 or 32 bit register hiding in there.
In that case what you should do is to represent it as a u16 or u32 here,
just or the bits into a status word, and then walk over that status
word with something like ffs(bitword); ...
Nope, it's not constant. In theory, and RMI4 based sensor can have up to 128
functions (in practice, it's far fewer), and each function can have as many as
7 interrupts. So the number of IRQ registers can vary from RMI4 sensor to RMI4
sensor, and needs to be computed during the scan of the product descriptor
table.
(...)
+static int standard_resume(struct rmi_device *rmi_dev)
Standard eh?
Atleast prefix with rmi4_*...
Ooops - we excised the Android based stuff, but forgot to change that function
name.
+static int rmi_driver_suspend(struct device *dev)
+{
+ struct rmi_device *rmi_dev = to_rmi_device(dev);
+ return standard_suspend(rmi_dev);
+}
+
+static int rmi_driver_resume(struct device *dev)
+{
+ struct rmi_device *rmi_dev = to_rmi_device(dev);
+ return standard_resume(rmi_dev);
+}
If all these two are doing are to call another
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny wrote:
> rmi_bus.c implements the basic functionality of the RMI bus. This file is
> greatly simplified compared to the previous patch - we've switched from
> "do it yourself" device/driver binding to using device_type to distinguish
> between the two kinds of devices on the bus (sensor devices and function
> specific devices) and using the standard bus implementation to manage devices
> and drivers.
So I think you really want Greg KH to look at this bus implementation
now. Please include Greg on future mailings...
It looks much improved from previous versions, and sorry if I am now
adding even more comments, but it's because you cleared out some
noise that was disturbing my perception so I can cleanly review
the architecture of this thing now. (I'm impressed by your work and
new high-speed turnaround time!)
> +#ifdef CONFIG_RMI4_DEBUG
> +#include
> +#endif
As noted previously, drop the #ifdef:s
> +#ifdef CONFIG_RMI4_DEBUG
> +static struct dentry *rmi_debugfs_root;
> +#endif
I'd use #ifdef CONFIG_DEBUG_FS and try to
move this declaration closer to the actual debugfs
code block.
Apart from this the core bus looks good to me, but it's not my
area of expertise...
(...)
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> +#ifdef CONFIG_RMI4_DEBUG
I'd just use CONFIG_DEBUG_FS
> +struct driver_debugfs_data {
> + bool done;
> + struct rmi_device *rmi_dev;
> +};
(...)
> +#define DELAY_NAME "delay"
This is only used in one place, why not just use the string
"delay" there?
(...)
> + if (IS_ENABLED(CONFIG_RMI4_SPI) && !strncmp("spi", info->proto, 3)) {
> + data->debugfs_delay = debugfs_create_file(DELAY_NAME,
> + RMI_RW_ATTR, rmi_dev->debugfs_root, rmi_dev,
> + _fops);
i.e. there.
(...)
> +/* Useful helper functions for u8* */
> +
> +static bool u8_is_any_set(u8 *target, int size)
> +{
> + int i;
> + /* We'd like to use find_first_bit, but it ALWAYS returns 1,
> + * no matter what we pass it. So we have to do this the hard way.
> + * return find_first_bit((long unsigned int *) target, size) != 0;
> + */
> + for (i = 0; i < size; i++) {
> + if (target[i])
> + return true;
> + }
> + return false;
> +}
Instead of:
if (u8_is_any_set(foo, 128) {}
Why can't you use:
if (!bitmap_empty(foo, 128*8) {}
?
If you look at the implementation in the header
and __bitmap_empty() in lib/bitmap.c you will realize that this
function is already optimized like this (and I actually don't think
the RMI4 code is performance-critical for these functions anyway,
but prove me wrong!)
> +
> +/** This is here because all those casts made for some ugly code.
> + */
> +static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
> +{
> + bitmap_and((long unsigned int *) dest,
> + (long unsigned int *) target1,
> + (long unsigned int *) target2,
> + nbits);
> +}
Hm, getting rid of unreadable casts is a valid case.
I'll be OK with this but maybe the real solution is to introduce such
helpers into ?
(...)
> +static int process_interrupt_requests(struct rmi_device *rmi_dev)
> +{
> + struct rmi_driver_data *data = dev_get_drvdata(_dev->dev);
> + struct device *dev = _dev->dev;
> + struct rmi_function_container *entry;
> + u8 irq_status[data->num_of_irq_regs];
Looking at this...
What does the data->num_of_irq_regs actually contain?
I just fear that it is something constant like always 2 or always 4,
so there is actually, in reality, a 16 or 32 bit register hiding in there.
In that case what you should do is to represent it as a u16 or u32 here,
just or the bits into a status word, and then walk over that status
word with something like ffs(bitword); ...
(...)
> +static int standard_resume(struct rmi_device *rmi_dev)
Standard eh? :-)
Atleast prefix with rmi4_*...
> +static int rmi_driver_suspend(struct device *dev)
> +{
> + struct rmi_device *rmi_dev = to_rmi_device(dev);
> + return standard_suspend(rmi_dev);
> +}
> +
> +static int rmi_driver_resume(struct device *dev)
> +{
> + struct rmi_device *rmi_dev = to_rmi_device(dev);
> + return standard_resume(rmi_dev);
> +}
If all these two are doing are to call another function with almost
the same name, what is the point? Just sink the code from
"standard_suspend()" and "standard_resume()" into these
functions and remove a pointless layer of abstraction.
Apart from this the core driver looks good to me.
(...)
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
(...)
> +#define simple_show_union_struct(regtype, propname, fmt)\
> +static ssize_t tricat(rmi_fn_, FNUM, _##propname##_show)(struct device *dev,\
> + struct device_attribute *attr, char
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com wrote:
rmi_bus.c implements the basic functionality of the RMI bus. This file is
greatly simplified compared to the previous patch - we've switched from
do it yourself device/driver binding to using device_type to distinguish
between the two kinds of devices on the bus (sensor devices and function
specific devices) and using the standard bus implementation to manage devices
and drivers.
So I think you really want Greg KH to look at this bus implementation
now. Please include Greg on future mailings...
It looks much improved from previous versions, and sorry if I am now
adding even more comments, but it's because you cleared out some
noise that was disturbing my perception so I can cleanly review
the architecture of this thing now. (I'm impressed by your work and
new high-speed turnaround time!)
+#ifdef CONFIG_RMI4_DEBUG
+#include linux/debugfs.h
+#endif
As noted previously, drop the #ifdef:s
+#ifdef CONFIG_RMI4_DEBUG
+static struct dentry *rmi_debugfs_root;
+#endif
I'd use #ifdef CONFIG_DEBUG_FS and try to
move this declaration closer to the actual debugfs
code block.
Apart from this the core bus looks good to me, but it's not my
area of expertise...
(...)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
+#ifdef CONFIG_RMI4_DEBUG
I'd just use CONFIG_DEBUG_FS
+struct driver_debugfs_data {
+ bool done;
+ struct rmi_device *rmi_dev;
+};
(...)
+#define DELAY_NAME delay
This is only used in one place, why not just use the string
delay there?
(...)
+ if (IS_ENABLED(CONFIG_RMI4_SPI) !strncmp(spi, info-proto, 3)) {
+ data-debugfs_delay = debugfs_create_file(DELAY_NAME,
+ RMI_RW_ATTR, rmi_dev-debugfs_root, rmi_dev,
+ delay_fops);
i.e. there.
(...)
+/* Useful helper functions for u8* */
+
+static bool u8_is_any_set(u8 *target, int size)
+{
+ int i;
+ /* We'd like to use find_first_bit, but it ALWAYS returns 1,
+ * no matter what we pass it. So we have to do this the hard way.
+ * return find_first_bit((long unsigned int *) target, size) != 0;
+ */
+ for (i = 0; i size; i++) {
+ if (target[i])
+ return true;
+ }
+ return false;
+}
Instead of:
if (u8_is_any_set(foo, 128) {}
Why can't you use:
if (!bitmap_empty(foo, 128*8) {}
?
If you look at the implementation in the linux/bitmap.h header
and __bitmap_empty() in lib/bitmap.c you will realize that this
function is already optimized like this (and I actually don't think
the RMI4 code is performance-critical for these functions anyway,
but prove me wrong!)
+
+/** This is here because all those casts made for some ugly code.
+ */
+static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
+{
+ bitmap_and((long unsigned int *) dest,
+ (long unsigned int *) target1,
+ (long unsigned int *) target2,
+ nbits);
+}
Hm, getting rid of unreadable casts is a valid case.
I'll be OK with this but maybe the real solution is to introduce such
helpers into linux/bitmap.h?
(...)
+static int process_interrupt_requests(struct rmi_device *rmi_dev)
+{
+ struct rmi_driver_data *data = dev_get_drvdata(rmi_dev-dev);
+ struct device *dev = rmi_dev-dev;
+ struct rmi_function_container *entry;
+ u8 irq_status[data-num_of_irq_regs];
Looking at this...
What does the data-num_of_irq_regs actually contain?
I just fear that it is something constant like always 2 or always 4,
so there is actually, in reality, a 16 or 32 bit register hiding in there.
In that case what you should do is to represent it as a u16 or u32 here,
just or the bits into a status word, and then walk over that status
word with something like ffs(bitword); ...
(...)
+static int standard_resume(struct rmi_device *rmi_dev)
Standard eh? :-)
Atleast prefix with rmi4_*...
+static int rmi_driver_suspend(struct device *dev)
+{
+ struct rmi_device *rmi_dev = to_rmi_device(dev);
+ return standard_suspend(rmi_dev);
+}
+
+static int rmi_driver_resume(struct device *dev)
+{
+ struct rmi_device *rmi_dev = to_rmi_device(dev);
+ return standard_resume(rmi_dev);
+}
If all these two are doing are to call another function with almost
the same name, what is the point? Just sink the code from
standard_suspend() and standard_resume() into these
functions and remove a pointless layer of abstraction.
Apart from this the core driver looks good to me.
(...)
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
(...)
+#define simple_show_union_struct(regtype, propname, fmt)\
+static ssize_t tricat(rmi_fn_, FNUM, _##propname##_show)(struct device *dev,\
+ struct device_attribute *attr, char *buf) {\
+
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Sat, Oct 6, 2012 at 9:06 AM, devendra.aaru wrote:
> On Sat, Oct 6, 2012 at 8:19 AM, Joe Perches wrote:
>> On Fri, 2012-10-05 at 21:09 -0700, Christopher Heiny wrote:
>> []
>>
>> Just some trivial comments:
>>
>>> diff --git a/drivers/input/rmi4/rmi_driver.c
>>> b/drivers/input/rmi4/rmi_driver.c
>> []
>>> @@ -0,0 +1,1529 @@
>> []
>>> +static ssize_t delay_write(struct file *filp, const char __user *buffer,
>>> +size_t size, loff_t *offset) {
>>> + struct driver_debugfs_data *data = filp->private_data;
>>> + struct rmi_device_platform_data *pdata =
>>> + data->rmi_dev->phys->dev->platform_data;
>>> + int retval;
>>> + char local_buf[size];
>>> + unsigned int new_read_delay;
>>> + unsigned int new_write_delay;
>>> + unsigned int new_block_delay;
>>> + unsigned int new_pre_delay;
>>> + unsigned int new_post_delay;
>>> +
>>> + retval = copy_from_user(local_buf, buffer, size);
>>> + if (retval)
>>> + return -EFAULT;
>>> +
>>> + retval = sscanf(local_buf, "%u %u %u %u %u", _read_delay,
>>> + _write_delay, _block_delay,
>>> + _pre_delay, _post_delay);
>>> + if (retval != 5) {
>>> + dev_err(>rmi_dev->dev,
>>> + "Incorrect number of values provided for delay.");
>>> + return -EINVAL;
>>> + }
>>> + if (new_read_delay < 0) {
>>
>> These are unnecessary tests as unsigned values are never < 0.
>>
>
Oops, i m sorry, i mistakenly took the variable as int, it should be
unsinged int.
sorry joe, you are right the are never < 0.
Thanks,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Sat, Oct 6, 2012 at 8:19 AM, Joe Perches wrote:
> On Fri, 2012-10-05 at 21:09 -0700, Christopher Heiny wrote:
> []
>
> Just some trivial comments:
>
>> diff --git a/drivers/input/rmi4/rmi_driver.c
>> b/drivers/input/rmi4/rmi_driver.c
> []
>> @@ -0,0 +1,1529 @@
> []
>> +static ssize_t delay_write(struct file *filp, const char __user *buffer,
>> +size_t size, loff_t *offset) {
>> + struct driver_debugfs_data *data = filp->private_data;
>> + struct rmi_device_platform_data *pdata =
>> + data->rmi_dev->phys->dev->platform_data;
>> + int retval;
>> + char local_buf[size];
>> + unsigned int new_read_delay;
>> + unsigned int new_write_delay;
>> + unsigned int new_block_delay;
>> + unsigned int new_pre_delay;
>> + unsigned int new_post_delay;
>> +
>> + retval = copy_from_user(local_buf, buffer, size);
>> + if (retval)
>> + return -EFAULT;
>> +
>> + retval = sscanf(local_buf, "%u %u %u %u %u", _read_delay,
>> + _write_delay, _block_delay,
>> + _pre_delay, _post_delay);
>> + if (retval != 5) {
>> + dev_err(>rmi_dev->dev,
>> + "Incorrect number of values provided for delay.");
>> + return -EINVAL;
>> + }
>> + if (new_read_delay < 0) {
>
> These are unnecessary tests as unsigned values are never < 0.
>
Nope.
1 main()
2 {
3 char buf[100] = "1 -2";
4 int t, t2;
5
6 sscanf(buf, "%u %u", , );
7
8 if (t > 0) {
9 printf("greater\n");
10 }
11
12 if (t2 > 0) {
13 printf("greater\n");
14 } else {
15 printf("lesser\n");
16 }
17 }
Thanks,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Fri, 2012-10-05 at 21:09 -0700, Christopher Heiny wrote:
[]
Just some trivial comments:
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
[]
> @@ -0,0 +1,1529 @@
[]
> +static ssize_t delay_write(struct file *filp, const char __user *buffer,
> +size_t size, loff_t *offset) {
> + struct driver_debugfs_data *data = filp->private_data;
> + struct rmi_device_platform_data *pdata =
> + data->rmi_dev->phys->dev->platform_data;
> + int retval;
> + char local_buf[size];
> + unsigned int new_read_delay;
> + unsigned int new_write_delay;
> + unsigned int new_block_delay;
> + unsigned int new_pre_delay;
> + unsigned int new_post_delay;
> +
> + retval = copy_from_user(local_buf, buffer, size);
> + if (retval)
> + return -EFAULT;
> +
> + retval = sscanf(local_buf, "%u %u %u %u %u", _read_delay,
> + _write_delay, _block_delay,
> + _pre_delay, _post_delay);
> + if (retval != 5) {
> + dev_err(>rmi_dev->dev,
> + "Incorrect number of values provided for delay.");
> + return -EINVAL;
> + }
> + if (new_read_delay < 0) {
These are unnecessary tests as unsigned values are never < 0.
> + dev_err(>rmi_dev->dev,
> + "Byte delay must be positive microseconds.\n");
> + return -EINVAL;
> + }
> + if (new_write_delay < 0) {
etc.
> +static ssize_t phys_read(struct file *filp, char __user *buffer, size_t size,
> + loff_t *offset) {
> + struct driver_debugfs_data *data = filp->private_data;
> + struct rmi_phys_info *info = >rmi_dev->phys->info;
> + int retval;
> + char local_buf[size];
size comes from where? possible stack overflow?
> +static ssize_t irq_debug_write(struct file *filp, const char __user *buffer,
> +size_t size, loff_t *offset) {
> + int retval;
> + char local_buf[size];
here too
[]
> +static int process_interrupt_requests(struct rmi_device *rmi_dev)
> +{
[]
> + list_for_each_entry(entry, >rmi_functions.list, list)
> + if (entry->irq_mask)
> + process_one_interrupt(entry, irq_status,
> + data);
style nit, it'd be nicer with braces.
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
[]
> @@ -0,0 +1,438 @@
> +
> +#define tricat(x, y, z) tricat_(x, y, z)
> +
> +#define tricat_(x, y, z) x##y##z
I think these tricat macros are merely obfuscating
and don't need to be used.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Fri, 2012-10-05 at 21:09 -0700, Christopher Heiny wrote:
[]
Just some trivial comments:
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
[]
@@ -0,0 +1,1529 @@
[]
+static ssize_t delay_write(struct file *filp, const char __user *buffer,
+size_t size, loff_t *offset) {
+ struct driver_debugfs_data *data = filp-private_data;
+ struct rmi_device_platform_data *pdata =
+ data-rmi_dev-phys-dev-platform_data;
+ int retval;
+ char local_buf[size];
+ unsigned int new_read_delay;
+ unsigned int new_write_delay;
+ unsigned int new_block_delay;
+ unsigned int new_pre_delay;
+ unsigned int new_post_delay;
+
+ retval = copy_from_user(local_buf, buffer, size);
+ if (retval)
+ return -EFAULT;
+
+ retval = sscanf(local_buf, %u %u %u %u %u, new_read_delay,
+ new_write_delay, new_block_delay,
+ new_pre_delay, new_post_delay);
+ if (retval != 5) {
+ dev_err(data-rmi_dev-dev,
+ Incorrect number of values provided for delay.);
+ return -EINVAL;
+ }
+ if (new_read_delay 0) {
These are unnecessary tests as unsigned values are never 0.
+ dev_err(data-rmi_dev-dev,
+ Byte delay must be positive microseconds.\n);
+ return -EINVAL;
+ }
+ if (new_write_delay 0) {
etc.
+static ssize_t phys_read(struct file *filp, char __user *buffer, size_t size,
+ loff_t *offset) {
+ struct driver_debugfs_data *data = filp-private_data;
+ struct rmi_phys_info *info = data-rmi_dev-phys-info;
+ int retval;
+ char local_buf[size];
size comes from where? possible stack overflow?
+static ssize_t irq_debug_write(struct file *filp, const char __user *buffer,
+size_t size, loff_t *offset) {
+ int retval;
+ char local_buf[size];
here too
[]
+static int process_interrupt_requests(struct rmi_device *rmi_dev)
+{
[]
+ list_for_each_entry(entry, data-rmi_functions.list, list)
+ if (entry-irq_mask)
+ process_one_interrupt(entry, irq_status,
+ data);
style nit, it'd be nicer with braces.
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
[]
@@ -0,0 +1,438 @@
+
+#define tricat(x, y, z) tricat_(x, y, z)
+
+#define tricat_(x, y, z) x##y##z
I think these tricat macros are merely obfuscating
and don't need to be used.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Sat, Oct 6, 2012 at 8:19 AM, Joe Perches j...@perches.com wrote:
On Fri, 2012-10-05 at 21:09 -0700, Christopher Heiny wrote:
[]
Just some trivial comments:
diff --git a/drivers/input/rmi4/rmi_driver.c
b/drivers/input/rmi4/rmi_driver.c
[]
@@ -0,0 +1,1529 @@
[]
+static ssize_t delay_write(struct file *filp, const char __user *buffer,
+size_t size, loff_t *offset) {
+ struct driver_debugfs_data *data = filp-private_data;
+ struct rmi_device_platform_data *pdata =
+ data-rmi_dev-phys-dev-platform_data;
+ int retval;
+ char local_buf[size];
+ unsigned int new_read_delay;
+ unsigned int new_write_delay;
+ unsigned int new_block_delay;
+ unsigned int new_pre_delay;
+ unsigned int new_post_delay;
+
+ retval = copy_from_user(local_buf, buffer, size);
+ if (retval)
+ return -EFAULT;
+
+ retval = sscanf(local_buf, %u %u %u %u %u, new_read_delay,
+ new_write_delay, new_block_delay,
+ new_pre_delay, new_post_delay);
+ if (retval != 5) {
+ dev_err(data-rmi_dev-dev,
+ Incorrect number of values provided for delay.);
+ return -EINVAL;
+ }
+ if (new_read_delay 0) {
These are unnecessary tests as unsigned values are never 0.
Nope.
1 main()
2 {
3 char buf[100] = 1 -2;
4 int t, t2;
5
6 sscanf(buf, %u %u, t, t2);
7
8 if (t 0) {
9 printf(greater\n);
10 }
11
12 if (t2 0) {
13 printf(greater\n);
14 } else {
15 printf(lesser\n);
16 }
17 }
Thanks,
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Sat, Oct 6, 2012 at 9:06 AM, devendra.aaru devendra.a...@gmail.com wrote:
On Sat, Oct 6, 2012 at 8:19 AM, Joe Perches j...@perches.com wrote:
On Fri, 2012-10-05 at 21:09 -0700, Christopher Heiny wrote:
[]
Just some trivial comments:
diff --git a/drivers/input/rmi4/rmi_driver.c
b/drivers/input/rmi4/rmi_driver.c
[]
@@ -0,0 +1,1529 @@
[]
+static ssize_t delay_write(struct file *filp, const char __user *buffer,
+size_t size, loff_t *offset) {
+ struct driver_debugfs_data *data = filp-private_data;
+ struct rmi_device_platform_data *pdata =
+ data-rmi_dev-phys-dev-platform_data;
+ int retval;
+ char local_buf[size];
+ unsigned int new_read_delay;
+ unsigned int new_write_delay;
+ unsigned int new_block_delay;
+ unsigned int new_pre_delay;
+ unsigned int new_post_delay;
+
+ retval = copy_from_user(local_buf, buffer, size);
+ if (retval)
+ return -EFAULT;
+
+ retval = sscanf(local_buf, %u %u %u %u %u, new_read_delay,
+ new_write_delay, new_block_delay,
+ new_pre_delay, new_post_delay);
+ if (retval != 5) {
+ dev_err(data-rmi_dev-dev,
+ Incorrect number of values provided for delay.);
+ return -EINVAL;
+ }
+ if (new_read_delay 0) {
These are unnecessary tests as unsigned values are never 0.
Oops, i m sorry, i mistakenly took the variable as int, it should be
unsinged int.
sorry joe, you are right the are never 0.
Thanks,
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
