Hi Dmitry,
On 5/5/2011 9:35 PM, Dmitry Torokhov wrote:
On Thu, May 05, 2011 at 11:03:23AM +0530, Anirudh Ghayal wrote:
Cc: Dmitry Torokhov<[email protected]>
Signed-off-by: Anirudh Ghayal<[email protected]>
Looks reasonable with the following nitpicks:
---
drivers/input/keyboard/pmic8xxx-keypad.c | 74 ++++++++++++++++++++++++++++++
include/linux/input/pmic8xxx-keypad.h | 2 +
2 files changed, 76 insertions(+), 0 deletions(-)
diff --git a/drivers/input/keyboard/pmic8xxx-keypad.c
b/drivers/input/keyboard/pmic8xxx-keypad.c
index 0229325..cad09d7 100644
--- a/drivers/input/keyboard/pmic8xxx-keypad.c
+++ b/drivers/input/keyboard/pmic8xxx-keypad.c
@@ -21,6 +21,7 @@
#include<linux/mutex.h>
#include<linux/mfd/pm8xxx/core.h>
+#include<linux/mfd/pm8xxx/gpio.h>
#include<linux/input/pmic8xxx-keypad.h>
#define PM8XXX_MAX_ROWS 18
@@ -446,6 +447,64 @@ static int __devinit pmic8xxx_kpd_init(struct pmic8xxx_kp
*kp)
}
+static int __devinit pmic8xxx_kp_config_drv(int gpio_start, int num_gpios)
+{
+ int rc;
+ struct pm_gpio kypd_drv = {
+ .direction = PM_GPIO_DIR_OUT,
+ .output_buffer = PM_GPIO_OUT_BUF_OPEN_DRAIN,
+ .output_value = 0,
+ .pull = PM_GPIO_PULL_NO,
+ .vin_sel = PM_GPIO_VIN_S3,
+ .out_strength = PM_GPIO_STRENGTH_LOW,
+ .function = PM_GPIO_FUNC_1,
+ .inv_int_pol = 1,
+ };
+
+ if (gpio_start< 0 || num_gpios< 0 ||
+ num_gpios> (PM8XXX_MAX_ROWS + gpio_start))
+ return -EINVAL;
+
+ while (num_gpios--) {
Personally I prefer "while (x--)" constructs when you really need to
count backwards, for example when unwinding in error handling path.
I'd prefer more straightforward
Okay.
for (i = 0; i< num_gpios; i++) {
rc = pm8xxx_gpio_config(gpio_start + i,&kypd_drv);
+ rc = pm8xxx_gpio_config(gpio_start++,&kypd_drv);
+ if (rc) {
+ pr_err("%s: FAIL pm8xxx_gpio_config(): rc=%d.\n",
+ __func__, rc);
Use dev_err() here as well, and probably print the pin number that failed?
Okay.
+ return rc;
+ }
+ }
+
+ return 0;
+}
+
+static int __devinit pmic8xxx_kp_config_sns(int gpio_start, int num_gpios)
+{
+ int rc;
+ struct pm_gpio kypd_sns = {
+ .direction = PM_GPIO_DIR_IN,
+ .pull = PM_GPIO_PULL_UP_31P5,
+ .vin_sel = PM_GPIO_VIN_S3,
+ .out_strength = PM_GPIO_STRENGTH_NO,
+ .function = PM_GPIO_FUNC_NORMAL,
+ .inv_int_pol = 1,
+ };
+
+ if (gpio_start< 0 || num_gpios< 0 ||
+ num_gpios> (PM8XXX_MAX_COLS + gpio_start))
+ return -EINVAL;
+
+ while (num_gpios--) {
+ rc = pm8xxx_gpio_config(gpio_start++,&kypd_sns);
+ if (rc) {
+ pr_err("%s: FAIL pm8xxx_gpio_config(): rc=%d.\n",
+ __func__, rc);
+ return rc;
+ }
+ }
+
This function is exactly as the one before. You can fold them together
if you pass "struct pm_gpio" as a 3rd argument.
Agreed, I will do that.
+ return 0;
+}
+
static int pmic8xxx_kp_enable(struct pmic8xxx_kp *kp)
{
int rc;
@@ -609,6 +668,20 @@ static int __devinit pmic8xxx_kp_probe(struct
platform_device *pdev)
goto err_get_irq;
}
+ rc = pmic8xxx_kp_config_sns(pdata->cols_gpio_start,
+ pdata->num_cols);
+ if (rc< 0) {
+ dev_err(&pdev->dev, "unable to configure keypad sense lines\n");
+ goto err_gpio_config;
+ }
+
+ rc = pmic8xxx_kp_config_drv(pdata->rows_gpio_start,
+ pdata->num_rows);
+ if (rc< 0) {
+ dev_err(&pdev->dev, "unable to configure keypad drive lines\n");
+ goto err_gpio_config;
+ }
+
rc = request_any_context_irq(kp->key_sense_irq, pmic8xxx_kp_irq,
IRQF_TRIGGER_RISING, "pmic-keypad", kp);
if (rc< 0) {
@@ -645,6 +718,7 @@ err_pmic_reg_read:
free_irq(kp->key_stuck_irq, NULL);
err_req_stuck_irq:
free_irq(kp->key_sense_irq, NULL);
+err_gpio_config:
err_get_irq:
input_free_device(kp->input);
err_alloc_device:
diff --git a/include/linux/input/pmic8xxx-keypad.h
b/include/linux/input/pmic8xxx-keypad.h
index fc2ac4c..5f1e2f9 100644
--- a/include/linux/input/pmic8xxx-keypad.h
+++ b/include/linux/input/pmic8xxx-keypad.h
@@ -38,6 +38,8 @@ struct pm8xxx_keypad_platform_data {
unsigned int num_cols;
unsigned int num_rows;
+ unsigned int rows_gpio_start;
+ unsigned int cols_gpio_start;
unsigned int debounce_ms;
unsigned int scan_delay_ms;
Thanks.
Thank you for the comments. I will submit a patch with these changes.
~Anirudh
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
