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

2014-10-27 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-10-13 Thread Mark Brown 
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.


signature.asc
Description: Digital signature

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

2014-10-08 Thread 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

--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-10-08 Thread Mark Brown 
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.


signature.asc
Description: Digital signature

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

2014-10-08 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-10-07 Thread 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 iiva...@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_ROWS18
@@ -28,9 +29,6 @@
 #define PM8XXX_ROW_SHIFT   3
 #define PM8XXX_MATRIX_MAX_SIZE (PM8XXX_MAX_ROWS * PM8XXX_MAX_COLS)
 
-#define PM8XXX_MIN_ROWS5
-#define PM8XXX_MIN_COLS5
-
 #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_MIN5
-#define KEYP_CTRL_SCAN_COLS_BITS   0x3
-
-#define KEYP_CTRL_SCAN_ROWS_SHIFT  2
-#define KEYP_CTRL_SCAN_ROWS_MIN5
-#define KEYP_CTRL_SCAN_ROWS_BITS   0x7
-
-#define KEYP_CTRL_KEYP_EN  BIT(7)
-
-#define KEYP_SCAN  0x149
-
-#define KEYP_SCAN_DBOUNCE_SHIFT1
-#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_FREQ32768
+#define KEYP_CLOCK_FREQ32768
+
+/* 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) {

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

2014-10-07 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html