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

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

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 good, just a couple of nitpicks:

> +struct pmic8058_kp {
> +	const struct pmic8058_keypad_data *pdata;
> +	struct input_dev *input;
> +	int key_sense_irq;
> +	int key_stuck_irq;
> +
> +	unsigned short *keycodes;

Since the size of keycode table is constant there is no need to allocate
it separately (and even if it was variable you still could move it to
the end of the structure and allocate together).

> +
> +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 __devinit pmic8058_kp_probe(struct platform_device *pdev)
> +{
> +	struct pmic8058_keypad_data *pdata = pdev->dev.platform_data;
> +	const struct matrix_keymap_data *keymap_data;
> +	struct pmic8058_kp *kp;
> +	int rc;
> +	unsigned short *keycodes;
> +	u8 ctrl_val;
> +	struct pm8058_chip	*pm_chip;
> +
> +	pm_chip = platform_get_drvdata(pdev);
> +	if (pm_chip == NULL) {
> +		dev_err(&pdev->dev, "no parent data passed in\n");
> +		return -EFAULT;

EFAULT is really odd, maybe -EINVAL?

> +	}
> +
> +	/* Check PMIC8058 version. A0 version is not supported */
> +	if (pm8058_rev(pm_chip) == PM_8058_REV_1p0) {
> +		dev_err(&pdev->dev, "PMIC8058 1.0 version is not supported\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!pdata || !pdata->num_cols || !pdata->num_rows ||
> +		pdata->num_cols > PM8058_MAX_COLS ||
> +		pdata->num_rows > PM8058_MAX_ROWS ||
> +		pdata->num_cols < PM8058_MIN_COLS ||
> +		pdata->num_rows < PM8058_MIN_ROWS) {
> +		dev_err(&pdev->dev, "invalid platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	if (pdata->rows_gpio_start < 0 || pdata->cols_gpio_start < 0) {
> +		dev_err(&pdev->dev, "invalid gpio_start platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!pdata->scan_delay_ms || pdata->scan_delay_ms > MAX_SCAN_DELAY
> +		|| pdata->scan_delay_ms < MIN_SCAN_DELAY ||
> +		!is_power_of_2(pdata->scan_delay_ms)) {
> +		dev_err(&pdev->dev, "invalid keypad scan time supplied\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!pdata->row_hold_ns || pdata->row_hold_ns > MAX_ROW_HOLD_DELAY
> +		|| pdata->row_hold_ns < MIN_ROW_HOLD_DELAY ||
> +		((pdata->row_hold_ns % MIN_ROW_HOLD_DELAY) != 0)) {
> +		dev_err(&pdev->dev, "invalid keypad row hold time supplied\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!pdata->debounce_ms
> +		|| ((pdata->debounce_ms % 5) != 0)
> +		|| pdata->debounce_ms > MAX_DEBOUNCE_TIME
> +		|| pdata->debounce_ms < MIN_DEBOUNCE_TIME) {
> +		dev_err(&pdev->dev, "invalid debounce time supplied\n");

Mixing style (logical || either in front of expression or after), please use
same style (and I prefer after).


> +		return -EINVAL;
> +	}
> +
> +	keymap_data = pdata->keymap_data;
> +	if (!keymap_data) {
> +		dev_err(&pdev->dev, "no keymap data supplied\n");
> +		return -EINVAL;
> +	}
> +
> +	kp = kzalloc(sizeof(*kp), GFP_KERNEL);
> +	if (!kp)
> +		return -ENOMEM;
> +
> +	keycodes = kzalloc(PM8058_MATRIX_MAX_SIZE * sizeof(*keycodes),
> +				 GFP_KERNEL);
> +	if (!keycodes) {
> +		rc = -ENOMEM;
> +		goto err_alloc_mem;
> +	}
> +
> +	platform_set_drvdata(pdev, kp);
> +
> +	kp->pdata	= pdata;
> +	kp->dev		= &pdev->dev;
> +	kp->keycodes	= keycodes;
> +	kp->pm_chip	= pm_chip;
> +
> +	kp->input = input_allocate_device();
> +	if (!kp->input) {
> +		dev_err(&pdev->dev, "unable to allocate input device\n");
> +		rc = -ENOMEM;
> +		goto err_alloc_device;
> +	}
> +
> +	kp->key_sense_irq = platform_get_irq(pdev, 0);
> +	if (kp->key_sense_irq < 0) {
> +		dev_err(&pdev->dev, "unable to get keypad sense irq\n");
> +		rc = -ENXIO;
> +		goto err_get_irq;
> +	}
> +
> +	kp->key_stuck_irq = platform_get_irq(pdev, 1);
> +	if (kp->key_stuck_irq < 0) {
> +		dev_err(&pdev->dev, "unable to get keypad stuck irq\n");
> +		rc = -ENXIO;
> +		goto err_get_irq;
> +	}
> +
> +	if (pdata->input_name)
> +		kp->input->name = pdata->input_name;
> +	else
> +		kp->input->name = "PMIC8058 keypad";
> +
> +	if (pdata->input_phys_device)
> +		kp->input->phys = pdata->input_phys_device;
> +	else
> +		kp->input->phys = "pmic8058_keypad/input0";

	kp->input->phys = pdata->input_phys_device ?: "pmic8058_keypad/input0";

Is shorter.

Thanks.

-- 
Dmitry