Re: [RFC v1 PATCH 2/6] input: pm8058_keypad: Qualcomm PMIC8058 keypad controller driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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