Messages in this thread | | | Date | Mon, 6 Dec 2010 10:14:27 -0800 | From | Dmitry Torokhov <> | Subject | Re: [RFC v1 PATCH 2/6] input: pm8058_keypad: Qualcomm PMIC8058 keypad controller driver |
| |
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:
> + > +#include <linux/input/pmic8058-keypad.h>
Move to MFD directory with the rest of pmic8058 definitions?
> + */ > +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.
> + > +static int pmic8058_detect_ghost_keys(struct pmic8058_kp *kp, u16 *new_state)
bool
> +{ > + 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
> + > +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, > + }; > + > +} > + > +static int pmic8058_kp_enable(struct pmic8058_kp *kp) > +{ > + int rc; > + > + kp->ctrl_reg |= KEYP_CTRL_KEYP_EN; > + > + rc = pmic8058_kp_write_u8(kp, kp->ctrl_reg, KEYP_CTRL); > + if (rc < 0) > + return rc; > + > + enable_irq(kp->key_sense_irq); > + enable_irq(kp->key_stuck_irq); > + > + return rc; > +} > + > +static int pmic8058_kp_disable(struct pmic8058_kp *kp) > +{ > + int rc; > + > + kp->ctrl_reg &= ~KEYP_CTRL_KEYP_EN; > + > + rc = pmic8058_kp_write_u8(kp, kp->ctrl_reg, KEYP_CTRL); > + if (rc < 0) > + return rc; > + > + disable_irq(kp->key_sense_irq); > + disable_irq(kp->key_stuck_irq); > + > + return rc; > +} > + > +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.
> + 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?
Thanks.
-- Dmitry
| |