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
--
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
