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:

quoted

On Thu, Oct 11, 2012 at 04:15:56AM +0000, Christopher Heiny wrote:

quoted

On Thursday, October 11, 2012 02:21:53 AM you wrote:

quoted

On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny [off-list ref] 

wrote:

quoted

quoted

quoted

quoted

+
+/** 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.

quoted

quoted

quoted

(...)

quoted

+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