Hi Dmitry
On 12/6/2010 11:44 PM, Dmitry Torokhov wrote:
> Hi Trilok,
>
> On Wed, Nov 10, 2010 at 06:17:57PM +0530, Trilok Soni wrote:
>> Add Qualcomm PMIC8058 based keypad controller driver
>> supporting upto 18x8 matrix configuration.
>>
>
> Looks very good, just a couple of small things:
Thanks for reviewing same revision of the patch again :)
>
>> +
>> +#include <linux/input/pmic8058-keypad.h>
>
> Move to MFD directory with the rest of pmic8058 definitions?
As discussed earlier we can either have one big header file having all of the
sub-device(s) or have separate file for each sub-device driver, I need to check
which one would be better but I will atleast move pmic8058-keypad.h header
to "mfd" directory, as these patch series is in RFC state, we can have final
changes like along with when core-driver gets submitted for review.
>
>> + */
>> +struct pmic8058_kp {
>> + const struct pmic8058_keypad_data *pdata;
>> + struct input_dev *input;
>> + int key_sense_irq;
>> + int key_stuck_irq;
>> +
>> + unsigned short *keycodes;
>
> I'd pull the keycodes into this structure (at the end) so it can be
> allocated in one shot. Hmm it even appears to be constant-sized. So just
> declare it right here and be done with it.
Done. You had already commented on it during the 1st review :)
>
>> +
>> +static int pmic8058_detect_ghost_keys(struct pmic8058_kp *kp, u16 *new_state)
>
> bool
Done.
>
>> +{
>> + int row, found_first = -1;
>> + u16 check, row_state;
>> +
>> + check = 0;
>> + for (row = 0; row < kp->pdata->num_rows; row++) {
>> + row_state = (~new_state[row]) &
>> + ((1 << kp->pdata->num_cols) - 1);
>> +
>> + if (hweight16(row_state) > 1) {
>> + if (found_first == -1)
>> + found_first = row;
>> + if (check & row_state) {
>> + dev_dbg(kp->dev, "detected ghost key on row[%d]"
>> + " and row[%d]\n", found_first, row);
>> + return 1;
>
> true
>
>> + }
>> + }
>> + check |= row_state;
>> + }
>> + return 0;
>
> false
Done.
>
>> +
>> +static int pmic8058_kpd_init(struct pmic8058_kp *kp)
>> +{
>> + int bits, rc, cycles;
>> + u8 scan_val = 0, ctrl_val = 0;
>> + static u8 row_bits[] = {
>
> const?
>
>> + 0, 1, 2, 3, 4, 4, 5, 5, 6, 6, 6, 7, 7, 7,
>> + };
Sure.
>> +
>> +static int pmic8058_kp_open(struct input_dev *dev)
>> +{
>> + struct pmic8058_kp *kp = input_get_drvdata(dev);
>> +
>> + return pmic8058_kp_enable(kp);
>> +}
>> +
>> +static void pmic8058_kp_close(struct input_dev *dev)
>> +{
>> + struct pmic8058_kp *kp = input_get_drvdata(dev);
>> +
>> + pmic8058_kp_disable(kp);
>> +}
>> +
>
> You need to protect suspend/resume from racing with open_close. Take
> dev->mutex and act depending on whether there are users of the device.
Sure.
>
>> + if (pdata->rows_gpio_start < 0 || pdata->cols_gpio_start < 0) {
>> + dev_err(&pdev->dev, "invalid gpio_start platform data\n");
>> + return -EINVAL;
>
> These are declared as unsigned. Hmm, doesn't sparse catch it?
I will fix it.
---Trilok Soni
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
