[PATCH 1/4] Input: pmic8xxx-keypad - remove unused register and bit definitions

[PATCH 1/4] Input: pmic8xxx-keypad - remove unused register and bit definitions

[Ivan T. Ivanov]

These defines are not used by driver. Remove them.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 drivers/input/keyboard/pmic8xxx-keypad.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/input/keyboard/pmic8xxx-keypad.c b/drivers/input/keyboard/pmic8xxx-keypad.c
index 80c6b0e..dd1fc95 100644
--- a/drivers/input/keyboard/pmic8xxx-keypad.c
+++ b/drivers/input/keyboard/pmic8xxx-keypad.c
@@ -43,7 +43,6 @@
 
 #define KEYP_CTRL			0x148
 
-#define KEYP_CTRL_EVNTS			BIT(0)
 #define KEYP_CTRL_EVNTS_MASK		0x3
 
 #define KEYP_CTRL_SCAN_COLS_SHIFT	5
@@ -58,21 +57,10 @@
 
 #define KEYP_SCAN			0x149
 
-#define KEYP_SCAN_READ_STATE		BIT(0)
 #define KEYP_SCAN_DBOUNCE_SHIFT		1
 #define KEYP_SCAN_PAUSE_SHIFT		3
 #define KEYP_SCAN_ROW_HOLD_SHIFT	6
 
-#define KEYP_TEST			0x14A
-
-#define KEYP_TEST_CLEAR_RECENT_SCAN	BIT(6)
-#define KEYP_TEST_CLEAR_OLD_SCAN	BIT(5)
-#define KEYP_TEST_READ_RESET		BIT(4)
-#define KEYP_TEST_DTEST_EN		BIT(3)
-#define KEYP_TEST_ABORT_READ		BIT(0)
-
-#define KEYP_TEST_DBG_SELECT_SHIFT	1
-
 /* bits of these registers represent
  * '0' for key press
  * '1' for key release
-- 
1.9.1

[PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Ivan T. Ivanov]

Abstract access to bit and register definitions to simplify adding
support for similar controller, which be found on SPMI based pm8941.

Group hardware capabilities to controller specific structure, and
pass it to driver, based on compatible string.

Pre-compute minimum number of rows and columns to avoid runtime checks.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 drivers/input/keyboard/pmic8xxx-keypad.c | 260 ++++++++++++++++++-------------
 1 file changed, 154 insertions(+), 106 deletions(-)

diff --git a/drivers/input/keyboard/pmic8xxx-keypad.c b/drivers/input/keyboard/pmic8xxx-keypad.c
index dd1fc95..b82d161 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/regmap.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/input/matrix_keypad.h>
 
 #define PM8XXX_MAX_ROWS		18
@@ -28,9 +29,6 @@
 #define PM8XXX_ROW_SHIFT	3
 #define PM8XXX_MATRIX_MAX_SIZE	(PM8XXX_MAX_ROWS * PM8XXX_MAX_COLS)
 
-#define PM8XXX_MIN_ROWS		5
-#define PM8XXX_MIN_COLS		5
-
 #define MAX_SCAN_DELAY		128
 #define MIN_SCAN_DELAY		1
 
@@ -41,34 +39,34 @@
 #define MAX_DEBOUNCE_TIME	20
 #define MIN_DEBOUNCE_TIME	5
 
-#define KEYP_CTRL			0x148
-
-#define KEYP_CTRL_EVNTS_MASK		0x3
-
-#define KEYP_CTRL_SCAN_COLS_SHIFT	5
-#define KEYP_CTRL_SCAN_COLS_MIN		5
-#define KEYP_CTRL_SCAN_COLS_BITS	0x3
-
-#define KEYP_CTRL_SCAN_ROWS_SHIFT	2
-#define KEYP_CTRL_SCAN_ROWS_MIN		5
-#define KEYP_CTRL_SCAN_ROWS_BITS	0x7
-
-#define KEYP_CTRL_KEYP_EN		BIT(7)
-
-#define KEYP_SCAN			0x149
-
-#define KEYP_SCAN_DBOUNCE_SHIFT		1
-#define KEYP_SCAN_PAUSE_SHIFT		3
-#define KEYP_SCAN_ROW_HOLD_SHIFT	6
-
-/* bits of these registers represent
- * '0' for key press
- * '1' for key release
- */
-#define KEYP_RECENT_DATA		0x14B
-#define KEYP_OLD_DATA			0x14C
-
-#define KEYP_CLOCK_FREQ			32768
+#define KEYP_CLOCK_FREQ		32768
+
+/* Keypad capabilities and registers description */
+struct pmic8xxx_kp_info {
+	unsigned int max_rows;
+	unsigned int min_rows;
+	unsigned int max_cols;
+	unsigned int min_cols;
+
+	unsigned int rows_select[PM8XXX_MAX_ROWS];
+	/*
+	 * bits of these registers represent
+	 * '0' for key press
+	 * '1' for key release
+	 */
+	unsigned int recent_data;
+	unsigned int old_data;
+	unsigned int read_stride;
+
+	struct reg_field events;
+	struct reg_field scan_rows;
+	struct reg_field scan_cols;
+	struct reg_field enable;
+	struct reg_field read_state;
+	struct reg_field dbonce;
+	struct reg_field pause;
+	struct reg_field row_hold;
+};
 
 /**
  * struct pmic8xxx_kp - internal keypad data structure
@@ -98,7 +96,44 @@ struct pmic8xxx_kp {
 	u16 keystate[PM8XXX_MAX_ROWS];
 	u16 stuckstate[PM8XXX_MAX_ROWS];
 
-	u8 ctrl_reg;
+	struct regmap_field *events;
+	struct regmap_field *scan_rows;
+	struct regmap_field *scan_cols;
+	struct regmap_field *enable;
+	struct regmap_field *read_state;
+	struct regmap_field *dbonce;
+	struct regmap_field *pause;
+	struct regmap_field *row_hold;
+
+	unsigned int recent_data;
+	unsigned int old_data;
+	unsigned int read_stride;
+};
+
+static const struct pmic8xxx_kp_info ssbi_kp = {
+	.max_rows	= 18,
+	.min_rows	= 5,
+	.max_cols	= 8,
+	.min_cols	= 5,
+
+	.rows_select = {
+		0, 1, 2, 3, 4, 4, 5,
+		5, 6, 6, 6, 7, 7, 7,
+	},
+
+	.recent_data	= 0x14b,
+	.old_data	= 0x14c,
+	.read_stride	= 0,
+
+	.events		= REG_FIELD(0x148, 0, 1),
+	.scan_rows	= REG_FIELD(0x148, 2, 4),
+	.scan_cols	= REG_FIELD(0x148, 5, 6),
+	.enable		= REG_FIELD(0x148, 7, 7),
+
+	.read_state	= REG_FIELD(0x149, 0, 0),
+	.dbonce		= REG_FIELD(0x149, 1, 2),
+	.pause		= REG_FIELD(0x149, 3, 5),
+	.row_hold	= REG_FIELD(0x149, 6, 7),
 };
 
 static u8 pmic8xxx_col_state(struct pmic8xxx_kp *kp, u8 col)
@@ -125,17 +160,8 @@ static u8 pmic8xxx_col_state(struct pmic8xxx_kp *kp, u8 col)
 static int pmic8xxx_chk_sync_read(struct pmic8xxx_kp *kp)
 {
 	int rc;
-	unsigned int scan_val;
-
-	rc = regmap_read(kp->regmap, KEYP_SCAN, &scan_val);
-	if (rc < 0) {
-		dev_err(kp->dev, "Error reading KEYP_SCAN reg, rc=%d\n", rc);
-		return rc;
-	}
-
-	scan_val |= 0x1;
 
-	rc = regmap_write(kp->regmap, KEYP_SCAN, scan_val);
+	rc = regmap_field_write(kp->read_state, 1);
 	if (rc < 0) {
 		dev_err(kp->dev, "Error writing KEYP_SCAN reg, rc=%d\n", rc);
 		return rc;
@@ -151,10 +177,11 @@ static int pmic8xxx_kp_read_data(struct pmic8xxx_kp *kp, u16 *state,
 					u16 data_reg, int read_rows)
 {
 	int rc, row;
-	unsigned int val;
+	unsigned int val, offset;
 
 	for (row = 0; row < read_rows; row++) {
-		rc = regmap_read(kp->regmap, data_reg, &val);
+		offset = data_reg + row * kp->read_stride;
+		rc = regmap_read(kp->regmap, offset, &val);
 		if (rc)
 			return rc;
 		dev_dbg(kp->dev, "%d = %d\n", row, val);
@@ -168,17 +195,13 @@ static int pmic8xxx_kp_read_matrix(struct pmic8xxx_kp *kp, u16 *new_state,
 					 u16 *old_state)
 {
 	int rc, read_rows;
-	unsigned int scan_val;
 
-	if (kp->num_rows < PM8XXX_MIN_ROWS)
-		read_rows = PM8XXX_MIN_ROWS;
-	else
-		read_rows = kp->num_rows;
+	read_rows = kp->num_rows;
 
 	pmic8xxx_chk_sync_read(kp);
 
 	if (old_state) {
-		rc = pmic8xxx_kp_read_data(kp, old_state, KEYP_OLD_DATA,
+		rc = pmic8xxx_kp_read_data(kp, old_state, kp->old_data,
 						read_rows);
 		if (rc < 0) {
 			dev_err(kp->dev,
@@ -187,7 +210,7 @@ static int pmic8xxx_kp_read_matrix(struct pmic8xxx_kp *kp, u16 *new_state,
 		}
 	}
 
-	rc = pmic8xxx_kp_read_data(kp, new_state, KEYP_RECENT_DATA,
+	rc = pmic8xxx_kp_read_data(kp, new_state, kp->recent_data,
 					 read_rows);
 	if (rc < 0) {
 		dev_err(kp->dev,
@@ -198,14 +221,7 @@ static int pmic8xxx_kp_read_matrix(struct pmic8xxx_kp *kp, u16 *new_state,
 	/* 4 * 32KHz clocks */
 	udelay((4 * DIV_ROUND_UP(USEC_PER_SEC, KEYP_CLOCK_FREQ)) + 1);
 
-	rc = regmap_read(kp->regmap, KEYP_SCAN, &scan_val);
-	if (rc < 0) {
-		dev_err(kp->dev, "Error reading KEYP_SCAN reg, rc=%d\n", rc);
-		return rc;
-	}
-
-	scan_val &= 0xFE;
-	rc = regmap_write(kp->regmap, KEYP_SCAN, scan_val);
+	rc = regmap_field_write(kp->read_state, 0);
 	if (rc < 0)
 		dev_err(kp->dev, "Error writing KEYP_SCAN reg, rc=%d\n", rc);
 
@@ -341,17 +357,15 @@ static irqreturn_t pmic8xxx_kp_stuck_irq(int irq, void *data)
 static irqreturn_t pmic8xxx_kp_irq(int irq, void *data)
 {
 	struct pmic8xxx_kp *kp = data;
-	unsigned int ctrl_val, events;
+	unsigned int events;
 	int rc;
 
-	rc = regmap_read(kp->regmap, KEYP_CTRL, &ctrl_val);
+	rc = regmap_field_read(kp->events, &events);
 	if (rc < 0) {
 		dev_err(kp->dev, "failed to read keyp_ctrl register\n");
 		return IRQ_HANDLED;
 	}
 
-	events = ctrl_val & KEYP_CTRL_EVNTS_MASK;
-
 	rc = pmic8xxx_kp_scan_matrix(kp, events);
 	if (rc < 0)
 		dev_err(kp->dev, "failed to scan matrix\n");
@@ -360,35 +374,28 @@ static irqreturn_t pmic8xxx_kp_irq(int irq, void *data)
 }
 
 static int pmic8xxx_kpd_init(struct pmic8xxx_kp *kp,
-			     struct platform_device *pdev)
+			     struct platform_device *pdev,
+			     const struct pmic8xxx_kp_info *info)
 {
 	const struct device_node *of_node = pdev->dev.of_node;
 	unsigned int scan_delay_ms;
 	unsigned int row_hold_ns;
 	unsigned int debounce_ms;
 	int bits, rc, cycles;
-	u8 scan_val = 0, ctrl_val = 0;
-	static const u8 row_bits[] = {
-		0, 1, 2, 3, 4, 4, 5, 5, 6, 6, 6, 7, 7, 7,
-	};
 
 	/* Find column bits */
-	if (kp->num_cols < KEYP_CTRL_SCAN_COLS_MIN)
-		bits = 0;
-	else
-		bits = kp->num_cols - KEYP_CTRL_SCAN_COLS_MIN;
-	ctrl_val = (bits & KEYP_CTRL_SCAN_COLS_BITS) <<
-		KEYP_CTRL_SCAN_COLS_SHIFT;
+	bits = kp->num_cols - info->min_cols;
 
-	/* Find row bits */
-	if (kp->num_rows < KEYP_CTRL_SCAN_ROWS_MIN)
-		bits = 0;
-	else
-		bits = row_bits[kp->num_rows - KEYP_CTRL_SCAN_ROWS_MIN];
+	rc = regmap_field_write(kp->scan_cols, bits);
+	if (rc < 0) {
+		dev_err(kp->dev, "Error writing KEYP_CTRL reg, rc=%d\n", rc);
+		return rc;
+	}
 
-	ctrl_val |= (bits << KEYP_CTRL_SCAN_ROWS_SHIFT);
+	/* Find row bits */
+	bits = info->rows_select[kp->num_rows - info->min_rows];
 
-	rc = regmap_write(kp->regmap, KEYP_CTRL, ctrl_val);
+	rc = regmap_field_write(kp->scan_rows, bits);
 	if (rc < 0) {
 		dev_err(kp->dev, "Error writing KEYP_CTRL reg, rc=%d\n", rc);
 		return rc;
@@ -425,17 +432,20 @@ static int pmic8xxx_kpd_init(struct pmic8xxx_kp *kp,
 
 	bits = (debounce_ms / 5) - 1;
 
-	scan_val |= (bits << KEYP_SCAN_DBOUNCE_SHIFT);
+	rc = regmap_field_write(kp->dbonce, bits);
+	if (rc)
+		dev_err(kp->dev, "Error writing KEYP_SCAN reg, rc=%d\n", rc);
 
 	bits = fls(scan_delay_ms) - 1;
-	scan_val |= (bits << KEYP_SCAN_PAUSE_SHIFT);
+
+	rc = regmap_field_write(kp->pause, bits);
+	if (rc)
+		dev_err(kp->dev, "Error writing KEYP_SCAN reg, rc=%d\n", rc);
 
 	/* Row hold time is a multiple of 32KHz cycles. */
 	cycles = (row_hold_ns * KEYP_CLOCK_FREQ) / NSEC_PER_SEC;
 
-	scan_val |= (cycles << KEYP_SCAN_ROW_HOLD_SHIFT);
-
-	rc = regmap_write(kp->regmap, KEYP_SCAN, scan_val);
+	rc = regmap_field_write(kp->row_hold, cycles);
 	if (rc)
 		dev_err(kp->dev, "Error writing KEYP_SCAN reg, rc=%d\n", rc);
 
@@ -447,9 +457,7 @@ static int pmic8xxx_kp_enable(struct pmic8xxx_kp *kp)
 {
 	int rc;
 
-	kp->ctrl_reg |= KEYP_CTRL_KEYP_EN;
-
-	rc = regmap_write(kp->regmap, KEYP_CTRL, kp->ctrl_reg);
+	rc = regmap_field_write(kp->enable, 1);
 	if (rc < 0)
 		dev_err(kp->dev, "Error writing KEYP_CTRL reg, rc=%d\n", rc);
 
@@ -460,9 +468,7 @@ static int pmic8xxx_kp_disable(struct pmic8xxx_kp *kp)
 {
 	int rc;
 
-	kp->ctrl_reg &= ~KEYP_CTRL_KEYP_EN;
-
-	rc = regmap_write(kp->regmap, KEYP_CTRL, kp->ctrl_reg);
+	rc = regmap_field_write(kp->enable, 0);
 	if (rc < 0)
 		return rc;
 
@@ -483,6 +489,8 @@ static void pmic8xxx_kp_close(struct input_dev *dev)
 	pmic8xxx_kp_disable(kp);
 }
 
+static const struct of_device_id pm8xxx_match_table[];
+
 /*
  * keypad controller should be initialized in the following sequence
  * only, otherwise it might get into FSM stuck state.
@@ -495,19 +503,22 @@ static void pmic8xxx_kp_close(struct input_dev *dev)
  */
 static int pmic8xxx_kp_probe(struct platform_device *pdev)
 {
+	const struct pmic8xxx_kp_info *info;
+	const struct of_device_id *id;
 	unsigned int rows, cols;
 	bool repeat;
 	bool wakeup;
 	struct pmic8xxx_kp *kp;
 	int rc;
-	unsigned int ctrl_val;
+
+	id = of_match_device(pm8xxx_match_table, &pdev->dev);
+	info = (const struct pmic8xxx_kp_info *)id->data;
 
 	rc = matrix_keypad_parse_of_params(&pdev->dev, &rows, &cols);
 	if (rc)
 		return rc;
 
-	if (cols > PM8XXX_MAX_COLS || rows > PM8XXX_MAX_ROWS ||
-	    cols < PM8XXX_MIN_COLS) {
+	if (cols > info->max_cols || rows > info->max_rows) {
 		dev_err(&pdev->dev, "invalid platform data\n");
 		return -EINVAL;
 	}
@@ -527,10 +538,55 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, kp);
 
+	if (rows < info->min_rows)
+		rows = info->min_rows;
+
+	if (cols < info->min_cols)
+		cols = info->min_cols;
+
 	kp->num_rows	= rows;
 	kp->num_cols	= cols;
 	kp->dev		= &pdev->dev;
 
+	kp->events = devm_regmap_field_alloc(kp->dev, kp->regmap,
+					     info->events);
+	if (IS_ERR(kp->events))
+		return PTR_ERR(kp->events);
+
+	kp->scan_rows = devm_regmap_field_alloc(kp->dev, kp->regmap,
+						info->scan_rows);
+	if (IS_ERR(kp->scan_rows))
+		return PTR_ERR(kp->scan_rows);
+
+	kp->scan_cols = devm_regmap_field_alloc(kp->dev, kp->regmap,
+						info->scan_cols);
+	if (IS_ERR(kp->scan_cols))
+		return PTR_ERR(kp->scan_cols);
+
+	kp->enable = devm_regmap_field_alloc(kp->dev, kp->regmap,
+					     info->enable);
+	if (IS_ERR(kp->enable))
+		return PTR_ERR(kp->enable);
+
+	kp->read_state = devm_regmap_field_alloc(kp->dev, kp->regmap,
+						 info->read_state);
+	if (IS_ERR(kp->read_state))
+		return PTR_ERR(kp->read_state);
+
+	kp->dbonce = devm_regmap_field_alloc(kp->dev, kp->regmap,
+					     info->dbonce);
+	if (IS_ERR(kp->dbonce))
+		return PTR_ERR(kp->dbonce);
+
+	kp->pause = devm_regmap_field_alloc(kp->dev, kp->regmap, info->pause);
+	if (IS_ERR(kp->pause))
+		return PTR_ERR(kp->pause);
+
+	kp->row_hold = devm_regmap_field_alloc(kp->dev, kp->regmap,
+					       info->row_hold);
+	if (IS_ERR(kp->row_hold))
+		return PTR_ERR(kp->row_hold);
+
 	kp->input = devm_input_allocate_device(&pdev->dev);
 	if (!kp->input) {
 		dev_err(&pdev->dev, "unable to allocate input device\n");
@@ -578,7 +634,7 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
 	memset(kp->keystate, 0xff, sizeof(kp->keystate));
 	memset(kp->stuckstate, 0xff, sizeof(kp->stuckstate));
 
-	rc = pmic8xxx_kpd_init(kp, pdev);
+	rc = pmic8xxx_kpd_init(kp, pdev, info);
 	if (rc < 0) {
 		dev_err(&pdev->dev, "unable to initialize keypad controller\n");
 		return rc;
@@ -600,14 +656,6 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
 		return rc;
 	}
 
-	rc = regmap_read(kp->regmap, KEYP_CTRL, &ctrl_val);
-	if (rc < 0) {
-		dev_err(&pdev->dev, "failed to read KEYP_CTRL register\n");
-		return rc;
-	}
-
-	kp->ctrl_reg = ctrl_val;
-
 	rc = input_register_device(kp->input);
 	if (rc < 0) {
 		dev_err(&pdev->dev, "unable to register keypad input device\n");
@@ -665,8 +713,8 @@ static SIMPLE_DEV_PM_OPS(pm8xxx_kp_pm_ops,
 			 pmic8xxx_kp_suspend, pmic8xxx_kp_resume);
 
 static const struct of_device_id pm8xxx_match_table[] = {
-	{ .compatible = "qcom,pm8058-keypad" },
-	{ .compatible = "qcom,pm8921-keypad" },
+	{ .compatible = "qcom,pm8058-keypad", .data = (void *)&ssbi_kp },
+	{ .compatible = "qcom,pm8921-keypad", .data = (void *)&ssbi_kp },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pm8xxx_match_table);
-- 
1.9.1

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Dmitry Torokhov]

Hi Ivan,

On Tue, Oct 07, 2014 at 12:50:46PM +0300, Ivan T. Ivanov wrote:
> @@ -527,10 +538,55 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, kp);
>  
> +	if (rows < info->min_rows)
> +		rows = info->min_rows;
> +
> +	if (cols < info->min_cols)
> +		cols = info->min_cols;
> +
>  	kp->num_rows	= rows;
>  	kp->num_cols	= cols;
>  	kp->dev		= &pdev->dev;
>  
> +	kp->events = devm_regmap_field_alloc(kp->dev, kp->regmap,
> +					     info->events);
> +	if (IS_ERR(kp->events))
> +		return PTR_ERR(kp->events);
> +
> +	kp->scan_rows = devm_regmap_field_alloc(kp->dev, kp->regmap,
> +						info->scan_rows);
> +	if (IS_ERR(kp->scan_rows))
> +		return PTR_ERR(kp->scan_rows);
> +
> +	kp->scan_cols = devm_regmap_field_alloc(kp->dev, kp->regmap,
> +						info->scan_cols);
> +	if (IS_ERR(kp->scan_cols))
> +		return PTR_ERR(kp->scan_cols);
> +
> +	kp->enable = devm_regmap_field_alloc(kp->dev, kp->regmap,
> +					     info->enable);
> +	if (IS_ERR(kp->enable))
> +		return PTR_ERR(kp->enable);
> +
> +	kp->read_state = devm_regmap_field_alloc(kp->dev, kp->regmap,
> +						 info->read_state);
> +	if (IS_ERR(kp->read_state))
> +		return PTR_ERR(kp->read_state);
> +
> +	kp->dbonce = devm_regmap_field_alloc(kp->dev, kp->regmap,
> +					     info->dbonce);
> +	if (IS_ERR(kp->dbonce))
> +		return PTR_ERR(kp->dbonce);
> +
> +	kp->pause = devm_regmap_field_alloc(kp->dev, kp->regmap, info->pause);
> +	if (IS_ERR(kp->pause))
> +		return PTR_ERR(kp->pause);
> +
> +	kp->row_hold = devm_regmap_field_alloc(kp->dev, kp->regmap,
> +					       info->row_hold);
> +	if (IS_ERR(kp->row_hold))
> +		return PTR_ERR(kp->row_hold);

Why do we have to allocate all regmap fields separately instead of
embedding them into keypad structure?

Thanks.

-- 
Dmitry

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Ivan T. Ivanov]

On Tue, 2014-10-07 at 10:26 -0700, Dmitry Torokhov wrote:
> Hi Ivan,
> 
> On Tue, Oct 07, 2014 at 12:50:46PM +0300, Ivan T. Ivanov wrote:
> > @@ -527,10 +538,55 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
> >  

> > +
> > +	kp->row_hold = devm_regmap_field_alloc(kp->dev, kp->regmap,
> > +					       info->row_hold);
> > +	if (IS_ERR(kp->row_hold))
> > +		return PTR_ERR(kp->row_hold);
> 
> Why do we have to allocate all regmap fields separately instead of
> embedding them into keypad structure?
> 

No particular reason. Will rework it.

Thank you,
Ivan

> Thanks.
> 

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Ivan T. Ivanov]

On Wed, 2014-10-08 at 12:13 +0300, Ivan T. Ivanov wrote:
> On Tue, 2014-10-07 at 10:26 -0700, Dmitry Torokhov wrote:
> > Hi Ivan,
> > 
> > On Tue, Oct 07, 2014 at 12:50:46PM +0300, Ivan T. Ivanov wrote:
> > > @@ -527,10 +538,55 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
> > >  
> 
> > > +
> > > +	kp->row_hold = devm_regmap_field_alloc(kp->dev, kp->regmap,
> > > +					       info->row_hold);
> > > +	if (IS_ERR(kp->row_hold))
> > > +		return PTR_ERR(kp->row_hold);
> > 
> > Why do we have to allocate all regmap fields separately instead of
> > embedding them into keypad structure?
> > 
> 
> No particular reason. Will rework it.
> 

Oops. struct regmap_field is opaque. It seems that the allocation
is the only way that I could have instance of it.

Regards,
Ivan

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Stephen Boyd]

On 10/08/2014 02:30 AM, Ivan T. Ivanov wrote:
> On Wed, 2014-10-08 at 12:13 +0300, Ivan T. Ivanov wrote:
>> On Tue, 2014-10-07 at 10:26 -0700, Dmitry Torokhov wrote:
>>> Hi Ivan,
>>>
>>> On Tue, Oct 07, 2014 at 12:50:46PM +0300, Ivan T. Ivanov wrote:
>>>> @@ -527,10 +538,55 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
>>>>   
>>>> +
>>>> +	kp->row_hold = devm_regmap_field_alloc(kp->dev, kp->regmap,
>>>> +					       info->row_hold);
>>>> +	if (IS_ERR(kp->row_hold))
>>>> +		return PTR_ERR(kp->row_hold);
>>> Why do we have to allocate all regmap fields separately instead of
>>> embedding them into keypad structure?
>>>
>> No particular reason. Will rework it.
>>
> Oops. struct regmap_field is opaque. It seems that the allocation
> is the only way that I could have instance of it.
>

Maybe we can add an API to allocate an array of fields?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Dmitry Torokhov]

On Wed, Oct 08, 2014 at 11:00:07AM -0700, Stephen Boyd wrote:
> On 10/08/2014 02:30 AM, Ivan T. Ivanov wrote:
> >On Wed, 2014-10-08 at 12:13 +0300, Ivan T. Ivanov wrote:
> >>On Tue, 2014-10-07 at 10:26 -0700, Dmitry Torokhov wrote:
> >>>Hi Ivan,
> >>>
> >>>On Tue, Oct 07, 2014 at 12:50:46PM +0300, Ivan T. Ivanov wrote:
> >>>>@@ -527,10 +538,55 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
> >>>>+
> >>>>+	kp->row_hold = devm_regmap_field_alloc(kp->dev, kp->regmap,
> >>>>+					       info->row_hold);
> >>>>+	if (IS_ERR(kp->row_hold))
> >>>>+		return PTR_ERR(kp->row_hold);
> >>>Why do we have to allocate all regmap fields separately instead of
> >>>embedding them into keypad structure?
> >>>
> >>No particular reason. Will rework it.
> >>
> >Oops. struct regmap_field is opaque. It seems that the allocation
> >is the only way that I could have instance of it.
> >
> 
> Maybe we can add an API to allocate an array of fields?

Maybe we could make the structure public instead? I do not see any
reason for allocating something separately that has exactly the same
lifetime as owning structure.

Thanks.

-- 
Dmitry

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Stephen Boyd]

Adding Mark and Srini

On 10/08/2014 11:13 AM, Dmitry Torokhov wrote:
> On Wed, Oct 08, 2014 at 11:00:07AM -0700, Stephen Boyd wrote:
>> On 10/08/2014 02:30 AM, Ivan T. Ivanov wrote:
>>> On Wed, 2014-10-08 at 12:13 +0300, Ivan T. Ivanov wrote:
>>>> On Tue, 2014-10-07 at 10:26 -0700, Dmitry Torokhov wrote:
>>>>> Hi Ivan,
>>>>>
>>>>> On Tue, Oct 07, 2014 at 12:50:46PM +0300, Ivan T. Ivanov wrote:
>>>>>> @@ -527,10 +538,55 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
>>>>>> +
>>>>>> +	kp->row_hold = devm_regmap_field_alloc(kp->dev, kp->regmap,
>>>>>> +					       info->row_hold);
>>>>>> +	if (IS_ERR(kp->row_hold))
>>>>>> +		return PTR_ERR(kp->row_hold);
>>>>> Why do we have to allocate all regmap fields separately instead of
>>>>> embedding them into keypad structure?
>>>>>
>>>> No particular reason. Will rework it.
>>>>
>>> Oops. struct regmap_field is opaque. It seems that the allocation
>>> is the only way that I could have instance of it.
>>>
>> Maybe we can add an API to allocate an array of fields?
> Maybe we could make the structure public instead? I do not see any
> reason for allocating something separately that has exactly the same
> lifetime as owning structure.
>

Srini/Mark, any reason why the regmap_field structure is opaque?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 786 bytes --]

On Wed, Oct 08, 2014 at 11:20:58AM -0700, Stephen Boyd wrote:
> On 10/08/2014 11:13 AM, Dmitry Torokhov wrote:

> >>>Oops. struct regmap_field is opaque. It seems that the allocation
> >>>is the only way that I could have instance of it.

> >>Maybe we can add an API to allocate an array of fields?

> >Maybe we could make the structure public instead? I do not see any
> >reason for allocating something separately that has exactly the same
> >lifetime as owning structure.

The lifetime may be different to that of the register map it references,
consider MFD function devices for example.  

> Srini/Mark, any reason why the regmap_field structure is opaque?

So you can't peer into it and rely on the contents.  I can see it being
useful to add a bulk allocator.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Dmitry Torokhov]

On Wed, Oct 08, 2014 at 09:04:26PM +0100, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 11:20:58AM -0700, Stephen Boyd wrote:
> > On 10/08/2014 11:13 AM, Dmitry Torokhov wrote:
> 
> > >>>Oops. struct regmap_field is opaque. It seems that the allocation
> > >>>is the only way that I could have instance of it.
> 
> > >>Maybe we can add an API to allocate an array of fields?
> 
> > >Maybe we could make the structure public instead? I do not see any
> > >reason for allocating something separately that has exactly the same
> > >lifetime as owning structure.
> 
> The lifetime may be different to that of the register map it references,
> consider MFD function devices for example.  

Exactly, it is different than lifetime of the register map, but the same
as device structure of the driver instantiating it. And so instead of
doing 10+ separate allocations one could do just one.

> 
> > Srini/Mark, any reason why the regmap_field structure is opaque?
> 
> So you can't peer into it and rely on the contents.  I can see it being
> useful to add a bulk allocator.

And then one have to define offsets in an array and use awkward syntax
to access individual fields. Can we just reply on reviews/documentation
for users to not do wrong thing?

Thanks.

-- 
Dmitry

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

On Wed, Oct 08, 2014 at 01:32:33PM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 08, 2014 at 09:04:26PM +0100, Mark Brown wrote:
> > On Wed, Oct 08, 2014 at 11:20:58AM -0700, Stephen Boyd wrote:

> > > Srini/Mark, any reason why the regmap_field structure is opaque?

> > So you can't peer into it and rely on the contents.  I can see it being
> > useful to add a bulk allocator.

> And then one have to define offsets in an array and use awkward syntax
> to access individual fields. Can we just reply on reviews/documentation
> for users to not do wrong thing?

I have very little confidence in users not doing awful things to be
honest, this is the sort of API where the users are just random things
all over the kernel so this sort of thing tends to be found after the
fact.  I get a lot of these in drivers that just got thrown over the
wall so nobody really knows what things are doing when you do find them.

If the standard allocators aren't doing a good job (I've not checked)
I'd much rather handle this inside the API if we can.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Ivan T. Ivanov]

Hi Dmitry, 

On Mon, 2014-10-13 at 16:02 +0200, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 01:32:33PM -0700, Dmitry Torokhov wrote:
> > On Wed, Oct 08, 2014 at 09:04:26PM +0100, Mark Brown wrote:
> > > On Wed, Oct 08, 2014 at 11:20:58AM -0700, Stephen Boyd wrote:
> 
> > > > Srini/Mark, any reason why the regmap_field structure is opaque?
> 
> > > So you can't peer into it and rely on the contents.  I can see it being
> > > useful to add a bulk allocator.
> 
> > And then one have to define offsets in an array and use awkward syntax
> > to access individual fields. Can we just reply on reviews/documentation
> > for users to not do wrong thing?
> 
> I have very little confidence in users not doing awful things to be
> honest, this is the sort of API where the users are just random things
> all over the kernel so this sort of thing tends to be found after the
> fact.  I get a lot of these in drivers that just got thrown over the
> wall so nobody really knows what things are doing when you do find them.
> 
> If the standard allocators aren't doing a good job (I've not checked)
> I'd much rather handle this inside the API if we can.
> 

Is there something that I can help here or patches are good as they are? :-)

Regards,
Ivan

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Dmitry Torokhov]

On Mon, Oct 27, 2014 at 05:30:41PM +0200, Ivan T. Ivanov wrote:
> 
> Hi Dmitry, 
> 
> On Mon, 2014-10-13 at 16:02 +0200, Mark Brown wrote:
> > On Wed, Oct 08, 2014 at 01:32:33PM -0700, Dmitry Torokhov wrote:
> > > On Wed, Oct 08, 2014 at 09:04:26PM +0100, Mark Brown wrote:
> > > > On Wed, Oct 08, 2014 at 11:20:58AM -0700, Stephen Boyd wrote:
> > 
> > > > > Srini/Mark, any reason why the regmap_field structure is opaque?
> > 
> > > > So you can't peer into it and rely on the contents.  I can see it being
> > > > useful to add a bulk allocator.
> > 
> > > And then one have to define offsets in an array and use awkward syntax
> > > to access individual fields. Can we just reply on reviews/documentation
> > > for users to not do wrong thing?
> > 
> > I have very little confidence in users not doing awful things to be
> > honest, this is the sort of API where the users are just random things
> > all over the kernel so this sort of thing tends to be found after the
> > fact.  I get a lot of these in drivers that just got thrown over the
> > wall so nobody really knows what things are doing when you do find them.
> > 
> > If the standard allocators aren't doing a good job (I've not checked)
> > I'd much rather handle this inside the API if we can.
> > 
> 
> Is there something that I can help here or patches are good as they are? :-)

Well, as far as I can see Bjorn (in the other thread) has some
objections on merging these devices together, so I ma just waiting for
you settle this issue.

I see he's not on this therad so let's CC him.

For the record I ma still not happy that we need to dynamically allocate
all these regmap fields.

Thanks.

-- 
Dmitry

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Ivan T. Ivanov]

On Mon, 2014-10-27 at 09:06 -0700, Dmitry Torokhov wrote:
> On Mon, Oct 27, 2014 at 05:30:41PM +0200, Ivan T. Ivanov wrote:
> > Hi Dmitry,
> > 
> > On Mon, 2014-10-13 at 16:02 +0200, Mark Brown wrote:
> > > On Wed, Oct 08, 2014 at 01:32:33PM -0700, Dmitry Torokhov wrote:
> > > > On Wed, Oct 08, 2014 at 09:04:26PM +0100, Mark Brown wrote:
> > > > > On Wed, Oct 08, 2014 at 11:20:58AM -0700, Stephen Boyd wrote:
> > > 
> > > > > > Srini/Mark, any reason why the regmap_field structure is opaque?
> > > 
> > > > > So you can't peer into it and rely on the contents.  I can see it being
> > > > > useful to add a bulk allocator.
> > > 
> > > > And then one have to define offsets in an array and use awkward syntax
> > > > to access individual fields. Can we just reply on reviews/documentation
> > > > for users to not do wrong thing?
> > > 
> > > I have very little confidence in users not doing awful things to be
> > > honest, this is the sort of API where the users are just random things
> > > all over the kernel so this sort of thing tends to be found after the
> > > fact.  I get a lot of these in drivers that just got thrown over the
> > > wall so nobody really knows what things are doing when you do find them.
> > > 
> > > If the standard allocators aren't doing a good job (I've not checked)
> > > I'd much rather handle this inside the API if we can.
> > > 
> > 
> > Is there something that I can help here or patches are good as they are? :-)
> 
> Well, as far as I can see Bjorn (in the other thread) has some
> objections on merging these devices together, so I ma just waiting for
> you settle this issue.

In the other thread we are discussing "pm8xxx-pwrkey" driver not this 
"pm8xxx-keypad" driver. Unless I not missing something? 


Regards,
Ivan

> 
> I see he's not on this therad so let's CC him.
> 
> For the record I ma still not happy that we need to dynamically allocate
> all these regmap fields.
> 
> Thanks.
> 
> 

Re: [PATCH 2/4] Input: pmic8xxx-keypad - use regmap_field for register access

[Dmitry Torokhov]

On Monday, October 27, 2014 06:20:31 PM Ivan T. Ivanov wrote:
> On Mon, 2014-10-27 at 09:06 -0700, Dmitry Torokhov wrote:
> > On Mon, Oct 27, 2014 at 05:30:41PM +0200, Ivan T. Ivanov wrote:
> > > Hi Dmitry,
> > > 
> > > On Mon, 2014-10-13 at 16:02 +0200, Mark Brown wrote:
> > > > On Wed, Oct 08, 2014 at 01:32:33PM -0700, Dmitry Torokhov wrote:
> > > > > On Wed, Oct 08, 2014 at 09:04:26PM +0100, Mark Brown wrote:
> > > > > > On Wed, Oct 08, 2014 at 11:20:58AM -0700, Stephen Boyd wrote:
> > > > > > > Srini/Mark, any reason why the regmap_field structure is opaque?
> > > > > > 
> > > > > > So you can't peer into it and rely on the contents.  I can see it
> > > > > > being
> > > > > > useful to add a bulk allocator.
> > > > > 
> > > > > And then one have to define offsets in an array and use awkward
> > > > > syntax
> > > > > to access individual fields. Can we just reply on
> > > > > reviews/documentation
> > > > > for users to not do wrong thing?
> > > > 
> > > > I have very little confidence in users not doing awful things to be
> > > > honest, this is the sort of API where the users are just random things
> > > > all over the kernel so this sort of thing tends to be found after the
> > > > fact.  I get a lot of these in drivers that just got thrown over the
> > > > wall so nobody really knows what things are doing when you do find
> > > > them.
> > > > 
> > > > If the standard allocators aren't doing a good job (I've not checked)
> > > > I'd much rather handle this inside the API if we can.
> > > 
> > > Is there something that I can help here or patches are good as they are?
> > > :-)> 
> > Well, as far as I can see Bjorn (in the other thread) has some
> > objections on merging these devices together, so I ma just waiting for
> > you settle this issue.
> 
> In the other thread we are discussing "pm8xxx-pwrkey" driver not this
> "pm8xxx-keypad" driver. Unless I not missing something?

Ah, sorry, I guess my parsing stops right after pm8xxx... Let me take another
look.

Thanks.

-- 
Dmitry

[PATCH 3/4] Input: pmic8xxx-keypad - add support keypad found in pm8941

[Ivan T. Ivanov]

Controller seems to be the same. Just access to it is over SPMI bus
and registers and bits are reshuffled. Hopefully this is nicely
abstracted by regmap helpers.

Signed-off-by: Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
---
 .../bindings/input/qcom,pm8xxx-keypad.txt          |  1 +
 drivers/input/keyboard/pmic8xxx-keypad.c           | 27 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
index 7d8cb92..2b60f8a 100644
--- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
+++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
@@ -8,6 +8,7 @@ PROPERTIES
 	Definition: must be one of:
 		    "qcom,pm8058-keypad"
 		    "qcom,pm8921-keypad"
+		    "qcom,pm8941-keypad"
 
 - reg:
 	Usage: required
diff --git a/drivers/input/keyboard/pmic8xxx-keypad.c b/drivers/input/keyboard/pmic8xxx-keypad.c
index b82d161..2d4fa07 100644
--- a/drivers/input/keyboard/pmic8xxx-keypad.c
+++ b/drivers/input/keyboard/pmic8xxx-keypad.c
@@ -136,6 +136,32 @@ static const struct pmic8xxx_kp_info ssbi_kp = {
 	.row_hold	= REG_FIELD(0x149, 6, 7),
 };
 
+static const struct pmic8xxx_kp_info spmi_kp = {
+	.max_rows	= 10,
+	.min_rows	= 2,
+	.max_cols	= 8,
+	.min_cols	= 1,
+
+	.rows_select = {
+		1, 2, 3, 4, 5, 6, 7,
+		8, 9, 10
+	},
+
+	.recent_data	= 0xa87c,
+	.old_data	= 0xa85c,
+	.read_stride	= 2,
+
+	.events		= REG_FIELD(0xa808, 0, 1),
+	.scan_rows	= REG_FIELD(0xa840, 0, 3),
+	.scan_cols	= REG_FIELD(0xa840, 4, 6),
+	.enable		= REG_FIELD(0xa846, 7, 7),
+
+	.read_state	= REG_FIELD(0xa844, 0, 0),
+	.dbonce		= REG_FIELD(0xa842, 0, 1),
+	.pause		= REG_FIELD(0xa842, 3, 5),
+	.row_hold	= REG_FIELD(0xa842, 6, 7),
+};
+
 static u8 pmic8xxx_col_state(struct pmic8xxx_kp *kp, u8 col)
 {
 	/* all keys pressed on that particular row? */
@@ -715,6 +741,7 @@ static SIMPLE_DEV_PM_OPS(pm8xxx_kp_pm_ops,
 static const struct of_device_id pm8xxx_match_table[] = {
 	{ .compatible = "qcom,pm8058-keypad", .data = (void *)&ssbi_kp },
 	{ .compatible = "qcom,pm8921-keypad", .data = (void *)&ssbi_kp },
+	{ .compatible = "qcom,pm8941-keypad", .data = (void *)&spmi_kp },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pm8xxx_match_table);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] Input: pmic8xxx-keypad - update DT bindings documentation

[Ivan T. Ivanov]

Fix incorrect dimensions for 'debonce' and 'scan-delay' times.
Now they represent what driver really expect. Add possible
time quants for 'debonce', 'scan-delay' and 'row-hold' times.
Update bindings example.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
index 2b60f8a..df5e2a1 100644
--- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
+++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
@@ -54,20 +54,23 @@ PROPERTIES
 - debounce:
 	Usage: optional
 	Value type: <u32>
-	Definition: time in microseconds that key must be pressed or release
+	Definition: time in miliseconds that key must be pressed or release
 		    for key sense interrupt to trigger.
+		    Possible values: 5, 10, 15, 20.
 
 - scan-delay:
 	Usage: optional
 	Value type: <u32>
-	Definition: time in microseconds to pause between successive scans
+	Definition: time in miliseconds to pause between successive scans
 		    of the matrix array.
+		    Possible values: 1, 2, 4, 8, 16, 32, 64, 128.
 
 - row-hold:
 	Usage: optional
 	Value type: <u32>
 	Definition: time in nanoseconds to pause between scans of each row in
 		    the matrix array.
+		    Possible values: 30500, 61000, 122000.
 
 EXAMPLE
 
@@ -86,5 +89,5 @@ EXAMPLE
 		keypad,num-columns = <5>;
 		debounce = <15>;
 		scan-delay = <32>;
-		row-hold = <91500>;
+		row-hold = <61000>;
 	};
-- 
1.9.1