* [PATCH 0/3] cap1106: add support for cap11xx variants
@ 2014-09-21 3:01 Matt Ranostay
2014-09-21 3:01 ` [PATCH 1/3] cap1106: Add support for various cap11xx devices Matt Ranostay
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Matt Ranostay @ 2014-09-21 3:01 UTC (permalink / raw)
To: galak, linux-input, linux-kernel, dmitry.torokhov, zonque, robh+dt
Cc: devicetree, Matt Ranostay
These patches enable using cap11xx devices that have different
number of capacitance channels, and using active-high on the
interrupt output.
Matt Ranostay (3):
cap1106: Add support for various cap11xx devices
cap1106: support for active-high interrupt option
dt: cap1106 active-high property addition
.../devicetree/bindings/input/cap1106.txt | 4 ++
drivers/input/keyboard/cap1106.c | 58 ++++++++++++----------
2 files changed, 35 insertions(+), 27 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] cap1106: Add support for various cap11xx devices
2014-09-21 3:01 [PATCH 0/3] cap1106: add support for cap11xx variants Matt Ranostay
@ 2014-09-21 3:01 ` Matt Ranostay
2014-09-21 9:58 ` Daniel Mack
2014-09-21 10:53 ` Daniel Mack
2014-09-21 3:01 ` [PATCH 2/3] cap1106: support for active-high interrupt option Matt Ranostay
2014-09-21 3:01 ` [PATCH 3/3] dt: cap1106 active-high property addition Matt Ranostay
2 siblings, 2 replies; 13+ messages in thread
From: Matt Ranostay @ 2014-09-21 3:01 UTC (permalink / raw)
To: galak, linux-input, linux-kernel, dmitry.torokhov, zonque, robh+dt
Cc: devicetree, Matt Ranostay
Several other variants of the cap11xx device exists with a varying
number of capacitance detection channels. Add support for creating
the channels dynamically.
Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
drivers/input/keyboard/cap1106.c | 54 +++++++++++++++++++---------------------
1 file changed, 26 insertions(+), 28 deletions(-)
diff --git a/drivers/input/keyboard/cap1106.c b/drivers/input/keyboard/cap1106.c
index d70b65a..b9c43b5 100644
--- a/drivers/input/keyboard/cap1106.c
+++ b/drivers/input/keyboard/cap1106.c
@@ -55,8 +55,6 @@
#define CAP1106_REG_MANUFACTURER_ID 0xfe
#define CAP1106_REG_REVISION 0xff
-#define CAP1106_NUM_CHN 6
-#define CAP1106_PRODUCT_ID 0x55
#define CAP1106_MANUFACTURER_ID 0x5d
struct cap1106_priv {
@@ -64,7 +62,8 @@ struct cap1106_priv {
struct input_dev *idev;
/* config */
- unsigned short keycodes[CAP1106_NUM_CHN];
+ u32 *keycodes;
+ unsigned int num_channels;
};
static const struct reg_default cap1106_reg_defaults[] = {
@@ -151,7 +150,7 @@ static irqreturn_t cap1106_thread_func(int irq_num, void *data)
if (ret < 0)
goto out;
- for (i = 0; i < CAP1106_NUM_CHN; i++)
+ for (i = 0; i < priv->num_channels; i++)
input_report_key(priv->idev, priv->keycodes[i],
status & (1 << i));
@@ -189,27 +188,23 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
struct cap1106_priv *priv;
struct device_node *node;
int i, error, irq, gain = 0;
- unsigned int val, rev;
- u32 gain32, keycodes[CAP1106_NUM_CHN];
+ unsigned int val, prod, rev;
+ u32 gain32;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
+ priv->num_channels = (unsigned long) id->driver_data;
+ priv->keycodes = devm_kzalloc(dev,
+ sizeof(u32) * priv->num_channels, GFP_KERNEL);
+ if (!priv->keycodes)
+ return -ENOMEM;
+
priv->regmap = devm_regmap_init_i2c(i2c_client, &cap1106_regmap_config);
if (IS_ERR(priv->regmap))
return PTR_ERR(priv->regmap);
- error = regmap_read(priv->regmap, CAP1106_REG_PRODUCT_ID, &val);
- if (error)
- return error;
-
- if (val != CAP1106_PRODUCT_ID) {
- dev_err(dev, "Product ID: Got 0x%02x, expected 0x%02x\n",
- val, CAP1106_PRODUCT_ID);
- return -ENODEV;
- }
-
error = regmap_read(priv->regmap, CAP1106_REG_MANUFACTURER_ID, &val);
if (error)
return error;
@@ -220,6 +215,10 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
return -ENODEV;
}
+ error = regmap_read(priv->regmap, CAP1106_REG_PRODUCT_ID, &prod);
+ if (error < 0)
+ return error;
+
error = regmap_read(priv->regmap, CAP1106_REG_REVISION, &rev);
if (error < 0)
return error;
@@ -235,17 +234,12 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
}
- BUILD_BUG_ON(ARRAY_SIZE(keycodes) != ARRAY_SIZE(priv->keycodes));
-
/* Provide some useful defaults */
- for (i = 0; i < ARRAY_SIZE(keycodes); i++)
- keycodes[i] = KEY_A + i;
+ for (i = 0; i < priv->num_channels; i++)
+ priv->keycodes[i] = KEY_A + i;
of_property_read_u32_array(node, "linux,keycodes",
- keycodes, ARRAY_SIZE(keycodes));
-
- for (i = 0; i < ARRAY_SIZE(keycodes); i++)
- priv->keycodes[i] = keycodes[i];
+ priv->keycodes, priv->num_channels);
error = regmap_update_bits(priv->regmap, CAP1106_REG_MAIN_CONTROL,
CAP1106_REG_MAIN_CONTROL_GAIN_MASK,
@@ -269,17 +263,17 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
if (of_property_read_bool(node, "autorepeat"))
__set_bit(EV_REP, priv->idev->evbit);
- for (i = 0; i < CAP1106_NUM_CHN; i++)
+ for (i = 0; i < priv->num_channels; i++)
__set_bit(priv->keycodes[i], priv->idev->keybit);
__clear_bit(KEY_RESERVED, priv->idev->keybit);
priv->idev->keycode = priv->keycodes;
priv->idev->keycodesize = sizeof(priv->keycodes[0]);
- priv->idev->keycodemax = ARRAY_SIZE(priv->keycodes);
+ priv->idev->keycodemax = priv->num_channels;
priv->idev->id.vendor = CAP1106_MANUFACTURER_ID;
- priv->idev->id.product = CAP1106_PRODUCT_ID;
+ priv->idev->id.product = prod;
priv->idev->id.version = rev;
priv->idev->open = cap1106_input_open;
@@ -313,12 +307,16 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
static const struct of_device_id cap1106_dt_ids[] = {
{ .compatible = "microchip,cap1106", },
+ { .compatible = "microchip,cap1126", },
+ { .compatible = "microchip,cap1188", },
{}
};
MODULE_DEVICE_TABLE(of, cap1106_dt_ids);
static const struct i2c_device_id cap1106_i2c_ids[] = {
- { "cap1106", 0 },
+ { "cap1106", 6 },
+ { "cap1126", 6 },
+ { "cap1188", 8 },
{}
};
MODULE_DEVICE_TABLE(i2c, cap1106_i2c_ids);
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] cap1106: support for active-high interrupt option
2014-09-21 3:01 [PATCH 0/3] cap1106: add support for cap11xx variants Matt Ranostay
2014-09-21 3:01 ` [PATCH 1/3] cap1106: Add support for various cap11xx devices Matt Ranostay
@ 2014-09-21 3:01 ` Matt Ranostay
2014-09-21 10:06 ` Daniel Mack
2014-09-21 3:01 ` [PATCH 3/3] dt: cap1106 active-high property addition Matt Ranostay
2 siblings, 1 reply; 13+ messages in thread
From: Matt Ranostay @ 2014-09-21 3:01 UTC (permalink / raw)
To: galak, linux-input, linux-kernel, dmitry.torokhov, zonque, robh+dt
Cc: devicetree, Matt Ranostay
Some applications need to use the active-high push-pull interrupt
option. This allows it be enabled in the device tree child node.
Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
drivers/input/keyboard/cap1106.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/input/keyboard/cap1106.c b/drivers/input/keyboard/cap1106.c
index b9c43b5..33e2590 100644
--- a/drivers/input/keyboard/cap1106.c
+++ b/drivers/input/keyboard/cap1106.c
@@ -234,6 +234,12 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
}
+ if (of_property_read_bool(node, "microchip,active-high")) {
+ error = regmap_write(priv->regmap, CAP1106_REG_CONFIG2, 0);
+ if (error)
+ return error;
+ }
+
/* Provide some useful defaults */
for (i = 0; i < priv->num_channels; i++)
priv->keycodes[i] = KEY_A + i;
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] dt: cap1106 active-high property addition
2014-09-21 3:01 [PATCH 0/3] cap1106: add support for cap11xx variants Matt Ranostay
2014-09-21 3:01 ` [PATCH 1/3] cap1106: Add support for various cap11xx devices Matt Ranostay
2014-09-21 3:01 ` [PATCH 2/3] cap1106: support for active-high interrupt option Matt Ranostay
@ 2014-09-21 3:01 ` Matt Ranostay
2 siblings, 0 replies; 13+ messages in thread
From: Matt Ranostay @ 2014-09-21 3:01 UTC (permalink / raw)
To: galak, linux-input, linux-kernel, dmitry.torokhov, zonque, robh+dt
Cc: devicetree, Matt Ranostay
Documenting the microchip,active-high property for interrupt pin
behavior.
Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
Documentation/devicetree/bindings/input/cap1106.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/cap1106.txt b/Documentation/devicetree/bindings/input/cap1106.txt
index 4b46390..7e64fd6 100644
--- a/Documentation/devicetree/bindings/input/cap1106.txt
+++ b/Documentation/devicetree/bindings/input/cap1106.txt
@@ -26,6 +26,10 @@ Optional properties:
Valid values are 1, 2, 4, and 8.
By default, a gain of 1 is set.
+ microchip,active-high: By default the interrupt pin is active low open
+ drain. This property allows using the active high
+ push-pull output.
+
linux,keycodes: Specifies an array of numeric keycode values to
be used for the channels. If this property is
omitted, KEY_A, KEY_B, etc are used as
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] cap1106: Add support for various cap11xx devices
2014-09-21 3:01 ` [PATCH 1/3] cap1106: Add support for various cap11xx devices Matt Ranostay
@ 2014-09-21 9:58 ` Daniel Mack
2014-09-21 22:46 ` Matt Ranostay
2014-09-22 0:28 ` Matt Ranostay
2014-09-21 10:53 ` Daniel Mack
1 sibling, 2 replies; 13+ messages in thread
From: Daniel Mack @ 2014-09-21 9:58 UTC (permalink / raw)
To: Matt Ranostay, galak, linux-input, linux-kernel, dmitry.torokhov,
robh+dt
Cc: devicetree
Hi,
On 09/21/2014 05:01 AM, Matt Ranostay wrote:
> Several other variants of the cap11xx device exists with a varying
> number of capacitance detection channels. Add support for creating
> the channels dynamically.
Thanks for the patches!
>
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
> drivers/input/keyboard/cap1106.c | 54 +++++++++++++++++++---------------------
Please also add a patch to rename the file to cap11xx.c, and make sure
to export the patch with 'git format-patch -M' to detect the rename.
> diff --git a/drivers/input/keyboard/cap1106.c b/drivers/input/keyboard/cap1106.c
> index d70b65a..b9c43b5 100644
> --- a/drivers/input/keyboard/cap1106.c
> +++ b/drivers/input/keyboard/cap1106.c
> @@ -55,8 +55,6 @@
> #define CAP1106_REG_MANUFACTURER_ID 0xfe
> #define CAP1106_REG_REVISION 0xff
>
> -#define CAP1106_NUM_CHN 6
> -#define CAP1106_PRODUCT_ID 0x55
> #define CAP1106_MANUFACTURER_ID 0x5d
>
> struct cap1106_priv {
> @@ -64,7 +62,8 @@ struct cap1106_priv {
> struct input_dev *idev;
>
> /* config */
> - unsigned short keycodes[CAP1106_NUM_CHN];
> + u32 *keycodes;
unsigned short *, please. See below.
> @@ -189,27 +188,23 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
> struct cap1106_priv *priv;
> struct device_node *node;
> int i, error, irq, gain = 0;
> - unsigned int val, rev;
> - u32 gain32, keycodes[CAP1106_NUM_CHN];
> + unsigned int val, prod, rev;
> + u32 gain32;
>
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
>
> + priv->num_channels = (unsigned long) id->driver_data;
priv->num_channels is unsigned int.
Also, a BUG_ON(!priv->num_channels) wouldn't harm.
> + priv->keycodes = devm_kzalloc(dev,
> + sizeof(u32) * priv->num_channels, GFP_KERNEL);
Use devm_kcalloc() to allocate an array.
> @@ -235,17 +234,12 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
> dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
> }
>
> - BUILD_BUG_ON(ARRAY_SIZE(keycodes) != ARRAY_SIZE(priv->keycodes));
> -
> /* Provide some useful defaults */
> - for (i = 0; i < ARRAY_SIZE(keycodes); i++)
> - keycodes[i] = KEY_A + i;
> + for (i = 0; i < priv->num_channels; i++)
> + priv->keycodes[i] = KEY_A + i;
>
> of_property_read_u32_array(node, "linux,keycodes",
> - keycodes, ARRAY_SIZE(keycodes));
> -
> - for (i = 0; i < ARRAY_SIZE(keycodes); i++)
> - priv->keycodes[i] = keycodes[i];
> + priv->keycodes, priv->num_channels);
Hmm, no. Internally, you have to store the keycodes as unsigned short,
otherwise EVIOC{G|S}KEYCODE from usespace doesn't work.
of_property_read_u16_array() should work here for unsigned short, I
guess. Otherwise, you'd have to open-code the routine.
> @@ -313,12 +307,16 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
>
> static const struct of_device_id cap1106_dt_ids[] = {
> { .compatible = "microchip,cap1106", },
> + { .compatible = "microchip,cap1126", },
> + { .compatible = "microchip,cap1188", },
Hmm, how can that work unless you set .data to the number of channels
here? Did you test that with a DT-enabled board?
Thanks,
Daniel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] cap1106: support for active-high interrupt option
2014-09-21 3:01 ` [PATCH 2/3] cap1106: support for active-high interrupt option Matt Ranostay
@ 2014-09-21 10:06 ` Daniel Mack
2014-09-22 5:56 ` Dmitry Torokhov
0 siblings, 1 reply; 13+ messages in thread
From: Daniel Mack @ 2014-09-21 10:06 UTC (permalink / raw)
To: Matt Ranostay, galak, linux-input, linux-kernel, dmitry.torokhov,
robh+dt
Cc: devicetree
On 09/21/2014 05:01 AM, Matt Ranostay wrote:
> Some applications need to use the active-high push-pull interrupt
> option. This allows it be enabled in the device tree child node.
>
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
> drivers/input/keyboard/cap1106.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/input/keyboard/cap1106.c b/drivers/input/keyboard/cap1106.c
> index b9c43b5..33e2590 100644
> --- a/drivers/input/keyboard/cap1106.c
> +++ b/drivers/input/keyboard/cap1106.c
> @@ -234,6 +234,12 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
> dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
> }
>
> + if (of_property_read_bool(node, "microchip,active-high")) {
I think the name of that property should make clear it's only changing
the interrupt output driver configuration. What about
"microchip,irq-active-high"?
Also, patch 3/3, which documents this new property, can be squashed into
this one.
> + error = regmap_write(priv->regmap, CAP1106_REG_CONFIG2, 0);
This register controls a lot more details than that. Overriding it with
0 doesn't seem right. Please use regmap_update_bits() to just update
ALT_POL, and also add a #define for it, so the next reader knows what
the code is doing :)
Thanks,
Daniel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] cap1106: Add support for various cap11xx devices
2014-09-21 3:01 ` [PATCH 1/3] cap1106: Add support for various cap11xx devices Matt Ranostay
2014-09-21 9:58 ` Daniel Mack
@ 2014-09-21 10:53 ` Daniel Mack
1 sibling, 0 replies; 13+ messages in thread
From: Daniel Mack @ 2014-09-21 10:53 UTC (permalink / raw)
To: Matt Ranostay, galak, linux-input, linux-kernel, dmitry.torokhov,
robh+dt
Cc: devicetree
Hi,
On 09/21/2014 05:01 AM, Matt Ranostay wrote:
> priv->regmap = devm_regmap_init_i2c(i2c_client, &cap1106_regmap_config);
> if (IS_ERR(priv->regmap))
> return PTR_ERR(priv->regmap);
>
> - error = regmap_read(priv->regmap, CAP1106_REG_PRODUCT_ID, &val);
> - if (error)
> - return error;
> -
> - if (val != CAP1106_PRODUCT_ID) {
> - dev_err(dev, "Product ID: Got 0x%02x, expected 0x%02x\n",
> - val, CAP1106_PRODUCT_ID);
> - return -ENODEV;
> - }
> -
Btw - the purpose of this code was to detect board configuration
mismatch. After all, I2C lacks a way to properly identify peripherals,
so the more runtime checks we do at probe time, the more of a chance we
have to detect wrong setups. This device is actually well implemented
and tells us something about itself.
Hence, I'd propose to define a structure like this:
struct cap11xx_hw_model {
uint8_t product_id;
unsigned int num_channels;
};
... and attach instances of that to the members of cap1106_dt_ids[] and
cap1106_i2c_ids[]. In the probe function, check that the contents of
CAP1106_PRODUCT_ID match what is expected by the configured model.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] cap1106: Add support for various cap11xx devices
2014-09-21 9:58 ` Daniel Mack
@ 2014-09-21 22:46 ` Matt Ranostay
2014-09-22 7:36 ` Daniel Mack
2014-09-22 0:28 ` Matt Ranostay
1 sibling, 1 reply; 13+ messages in thread
From: Matt Ranostay @ 2014-09-21 22:46 UTC (permalink / raw)
To: Daniel Mack
Cc: Kumar Gala, linux-input, linux-kernel, Dmitry Torokhov,
Rob Herring, devicetree
On Sun, Sep 21, 2014 at 2:58 AM, Daniel Mack <daniel@zonque.org> wrote:
>
> Hi,
>
> On 09/21/2014 05:01 AM, Matt Ranostay wrote:
> > Several other variants of the cap11xx device exists with a varying
> > number of capacitance detection channels. Add support for creating
> > the channels dynamically.
>
> Thanks for the patches!
>
> >
> > Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> > ---
> > drivers/input/keyboard/cap1106.c | 54 +++++++++++++++++++---------------------
>
> Please also add a patch to rename the file to cap11xx.c, and make sure
> to export the patch with 'git format-patch -M' to detect the rename.
>
> > diff --git a/drivers/input/keyboard/cap1106.c b/drivers/input/keyboard/cap1106.c
> > index d70b65a..b9c43b5 100644
> > --- a/drivers/input/keyboard/cap1106.c
> > +++ b/drivers/input/keyboard/cap1106.c
> > @@ -55,8 +55,6 @@
> > #define CAP1106_REG_MANUFACTURER_ID 0xfe
> > #define CAP1106_REG_REVISION 0xff
> >
> > -#define CAP1106_NUM_CHN 6
> > -#define CAP1106_PRODUCT_ID 0x55
> > #define CAP1106_MANUFACTURER_ID 0x5d
> >
> > struct cap1106_priv {
> > @@ -64,7 +62,8 @@ struct cap1106_priv {
> > struct input_dev *idev;
> >
> > /* config */
> > - unsigned short keycodes[CAP1106_NUM_CHN];
> > + u32 *keycodes;
>
> unsigned short *, please. See below.
>
> > @@ -189,27 +188,23 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
> > struct cap1106_priv *priv;
> > struct device_node *node;
> > int i, error, irq, gain = 0;
> > - unsigned int val, rev;
> > - u32 gain32, keycodes[CAP1106_NUM_CHN];
> > + unsigned int val, prod, rev;
> > + u32 gain32;
> >
> > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > if (!priv)
> > return -ENOMEM;
> >
> > + priv->num_channels = (unsigned long) id->driver_data;
>
> priv->num_channels is unsigned int.
>
> Also, a BUG_ON(!priv->num_channels) wouldn't harm.
Oops. Will fix..
>
> > + priv->keycodes = devm_kzalloc(dev,
> > + sizeof(u32) * priv->num_channels, GFP_KERNEL);
>
> Use devm_kcalloc() to allocate an array.
>
> > @@ -235,17 +234,12 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
> > dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
> > }
> >
> > - BUILD_BUG_ON(ARRAY_SIZE(keycodes) != ARRAY_SIZE(priv->keycodes));
> > -
> > /* Provide some useful defaults */
> > - for (i = 0; i < ARRAY_SIZE(keycodes); i++)
> > - keycodes[i] = KEY_A + i;
> > + for (i = 0; i < priv->num_channels; i++)
> > + priv->keycodes[i] = KEY_A + i;
> >
> > of_property_read_u32_array(node, "linux,keycodes",
> > - keycodes, ARRAY_SIZE(keycodes));
> > -
> > - for (i = 0; i < ARRAY_SIZE(keycodes); i++)
> > - priv->keycodes[i] = keycodes[i];
> > + priv->keycodes, priv->num_channels);
>
> Hmm, no. Internally, you have to store the keycodes as unsigned short,
> otherwise EVIOC{G|S}KEYCODE from usespace doesn't work.
>
> of_property_read_u16_array() should work here for unsigned short, I
> guess. Otherwise, you'd have to open-code the routine.
>
Ok I'l test that.
> > @@ -313,12 +307,16 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
> >
> > static const struct of_device_id cap1106_dt_ids[] = {
> > { .compatible = "microchip,cap1106", },
> > + { .compatible = "microchip,cap1126", },
> > + { .compatible = "microchip,cap1188", },
>
> Hmm, how can that work unless you set .data to the number of channels
> here? Did you test that with a DT-enabled board?
>
Yes it was tested on a BBB. The num_channels is set from cap1106_i2c_ids
>
> Thanks,
> Daniel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] cap1106: Add support for various cap11xx devices
2014-09-21 9:58 ` Daniel Mack
2014-09-21 22:46 ` Matt Ranostay
@ 2014-09-22 0:28 ` Matt Ranostay
2014-09-22 5:59 ` Dmitry Torokhov
1 sibling, 1 reply; 13+ messages in thread
From: Matt Ranostay @ 2014-09-22 0:28 UTC (permalink / raw)
To: Daniel Mack
Cc: Kumar Gala, linux-input, linux-kernel, Dmitry Torokhov,
Rob Herring, devicetree
On Sun, Sep 21, 2014 at 2:58 AM, Daniel Mack <daniel@zonque.org> wrote:
> Hi,
>
> On 09/21/2014 05:01 AM, Matt Ranostay wrote:
>> Several other variants of the cap11xx device exists with a varying
>> number of capacitance detection channels. Add support for creating
>> the channels dynamically.
>
> Thanks for the patches!
>
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> ---
>> drivers/input/keyboard/cap1106.c | 54 +++++++++++++++++++---------------------
>
> Please also add a patch to rename the file to cap11xx.c, and make sure
> to export the patch with 'git format-patch -M' to detect the rename.
>
>> diff --git a/drivers/input/keyboard/cap1106.c b/drivers/input/keyboard/cap1106.c
>> index d70b65a..b9c43b5 100644
>> --- a/drivers/input/keyboard/cap1106.c
>> +++ b/drivers/input/keyboard/cap1106.c
>> @@ -55,8 +55,6 @@
>> #define CAP1106_REG_MANUFACTURER_ID 0xfe
>> #define CAP1106_REG_REVISION 0xff
>>
>> -#define CAP1106_NUM_CHN 6
>> -#define CAP1106_PRODUCT_ID 0x55
>> #define CAP1106_MANUFACTURER_ID 0x5d
>>
>> struct cap1106_priv {
>> @@ -64,7 +62,8 @@ struct cap1106_priv {
>> struct input_dev *idev;
>>
>> /* config */
>> - unsigned short keycodes[CAP1106_NUM_CHN];
>> + u32 *keycodes;
>
> unsigned short *, please. See below.
>
>> @@ -189,27 +188,23 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
>> struct cap1106_priv *priv;
>> struct device_node *node;
>> int i, error, irq, gain = 0;
>> - unsigned int val, rev;
>> - u32 gain32, keycodes[CAP1106_NUM_CHN];
>> + unsigned int val, prod, rev;
>> + u32 gain32;
>>
>> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> if (!priv)
>> return -ENOMEM;
>>
>> + priv->num_channels = (unsigned long) id->driver_data;
>
> priv->num_channels is unsigned int.
>
> Also, a BUG_ON(!priv->num_channels) wouldn't harm.
>
>> + priv->keycodes = devm_kzalloc(dev,
>> + sizeof(u32) * priv->num_channels, GFP_KERNEL);
>
> Use devm_kcalloc() to allocate an array.
>
>> @@ -235,17 +234,12 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
>> dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
>> }
>>
>> - BUILD_BUG_ON(ARRAY_SIZE(keycodes) != ARRAY_SIZE(priv->keycodes));
>> -
>> /* Provide some useful defaults */
>> - for (i = 0; i < ARRAY_SIZE(keycodes); i++)
>> - keycodes[i] = KEY_A + i;
>> + for (i = 0; i < priv->num_channels; i++)
>> + priv->keycodes[i] = KEY_A + i;
>>
>> of_property_read_u32_array(node, "linux,keycodes",
>> - keycodes, ARRAY_SIZE(keycodes));
>> -
>> - for (i = 0; i < ARRAY_SIZE(keycodes); i++)
>> - priv->keycodes[i] = keycodes[i];
>> + priv->keycodes, priv->num_channels);
>
> Hmm, no. Internally, you have to store the keycodes as unsigned short,
> otherwise EVIOC{G|S}KEYCODE from usespace doesn't work.
>
> of_property_read_u16_array() should work here for unsigned short, I
> guess. Otherwise, you'd have to open-code the routine.
>
Problem with u16 is you'll to mark it */bits/ 16* in the device tree
entry.. I doubt that is acceptable
>> @@ -313,12 +307,16 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
>>
>> static const struct of_device_id cap1106_dt_ids[] = {
>> { .compatible = "microchip,cap1106", },
>> + { .compatible = "microchip,cap1126", },
>> + { .compatible = "microchip,cap1188", },
>
> Hmm, how can that work unless you set .data to the number of channels
> here? Did you test that with a DT-enabled board?
>
>
> Thanks,
> Daniel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] cap1106: support for active-high interrupt option
2014-09-21 10:06 ` Daniel Mack
@ 2014-09-22 5:56 ` Dmitry Torokhov
2014-09-22 7:44 ` Daniel Mack
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2014-09-22 5:56 UTC (permalink / raw)
To: Daniel Mack
Cc: Matt Ranostay, galak, linux-input, linux-kernel, robh+dt, devicetree
On Sun, Sep 21, 2014 at 12:06:30PM +0200, Daniel Mack wrote:
> On 09/21/2014 05:01 AM, Matt Ranostay wrote:
> > Some applications need to use the active-high push-pull interrupt
> > option. This allows it be enabled in the device tree child node.
> >
> > Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> > ---
> > drivers/input/keyboard/cap1106.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/input/keyboard/cap1106.c b/drivers/input/keyboard/cap1106.c
> > index b9c43b5..33e2590 100644
> > --- a/drivers/input/keyboard/cap1106.c
> > +++ b/drivers/input/keyboard/cap1106.c
> > @@ -234,6 +234,12 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
> > dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
> > }
> >
> > + if (of_property_read_bool(node, "microchip,active-high")) {
>
> I think the name of that property should make clear it's only changing
> the interrupt output driver configuration. What about
> "microchip,irq-active-high"?
Can we infer the setting from IRQ flags by chance?
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] cap1106: Add support for various cap11xx devices
2014-09-22 0:28 ` Matt Ranostay
@ 2014-09-22 5:59 ` Dmitry Torokhov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Torokhov @ 2014-09-22 5:59 UTC (permalink / raw)
To: Matt Ranostay
Cc: Daniel Mack, Kumar Gala, linux-input, linux-kernel, Rob Herring,
devicetree
On Sun, Sep 21, 2014 at 05:28:05PM -0700, Matt Ranostay wrote:
> On Sun, Sep 21, 2014 at 2:58 AM, Daniel Mack <daniel@zonque.org> wrote:
> > Hi,
> >
> > On 09/21/2014 05:01 AM, Matt Ranostay wrote:
> >> Several other variants of the cap11xx device exists with a varying
> >> number of capacitance detection channels. Add support for creating
> >> the channels dynamically.
> >
> > Thanks for the patches!
> >
> >>
> >> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> >> ---
> >> drivers/input/keyboard/cap1106.c | 54 +++++++++++++++++++---------------------
> >
> > Please also add a patch to rename the file to cap11xx.c, and make sure
> > to export the patch with 'git format-patch -M' to detect the rename.
> >
> >> diff --git a/drivers/input/keyboard/cap1106.c b/drivers/input/keyboard/cap1106.c
> >> index d70b65a..b9c43b5 100644
> >> --- a/drivers/input/keyboard/cap1106.c
> >> +++ b/drivers/input/keyboard/cap1106.c
> >> @@ -55,8 +55,6 @@
> >> #define CAP1106_REG_MANUFACTURER_ID 0xfe
> >> #define CAP1106_REG_REVISION 0xff
> >>
> >> -#define CAP1106_NUM_CHN 6
> >> -#define CAP1106_PRODUCT_ID 0x55
> >> #define CAP1106_MANUFACTURER_ID 0x5d
> >>
> >> struct cap1106_priv {
> >> @@ -64,7 +62,8 @@ struct cap1106_priv {
> >> struct input_dev *idev;
> >>
> >> /* config */
> >> - unsigned short keycodes[CAP1106_NUM_CHN];
> >> + u32 *keycodes;
> >
> > unsigned short *, please. See below.
> >
> >> @@ -189,27 +188,23 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
> >> struct cap1106_priv *priv;
> >> struct device_node *node;
> >> int i, error, irq, gain = 0;
> >> - unsigned int val, rev;
> >> - u32 gain32, keycodes[CAP1106_NUM_CHN];
> >> + unsigned int val, prod, rev;
> >> + u32 gain32;
> >>
> >> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >> if (!priv)
> >> return -ENOMEM;
> >>
> >> + priv->num_channels = (unsigned long) id->driver_data;
> >
> > priv->num_channels is unsigned int.
> >
> > Also, a BUG_ON(!priv->num_channels) wouldn't harm.
> >
> >> + priv->keycodes = devm_kzalloc(dev,
> >> + sizeof(u32) * priv->num_channels, GFP_KERNEL);
You do not need to allocate keycodes separately if you make a flexible length
array at the end of the main structure.
> >
> > Use devm_kcalloc() to allocate an array.
> >
> >> @@ -235,17 +234,12 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
> >> dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
> >> }
> >>
> >> - BUILD_BUG_ON(ARRAY_SIZE(keycodes) != ARRAY_SIZE(priv->keycodes));
> >> -
> >> /* Provide some useful defaults */
> >> - for (i = 0; i < ARRAY_SIZE(keycodes); i++)
> >> - keycodes[i] = KEY_A + i;
> >> + for (i = 0; i < priv->num_channels; i++)
> >> + priv->keycodes[i] = KEY_A + i;
> >>
> >> of_property_read_u32_array(node, "linux,keycodes",
> >> - keycodes, ARRAY_SIZE(keycodes));
> >> -
> >> - for (i = 0; i < ARRAY_SIZE(keycodes); i++)
> >> - priv->keycodes[i] = keycodes[i];
> >> + priv->keycodes, priv->num_channels);
> >
> > Hmm, no. Internally, you have to store the keycodes as unsigned short,
> > otherwise EVIOC{G|S}KEYCODE from usespace doesn't work.
> >
> > of_property_read_u16_array() should work here for unsigned short, I
> > guess. Otherwise, you'd have to open-code the routine.
> >
> Problem with u16 is you'll to mark it */bits/ 16* in the device tree
> entry.. I doubt that is acceptable
You can make keymap u32. As long as you set input->keycodesize appropriately
(and we do) everything should work just fine.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] cap1106: Add support for various cap11xx devices
2014-09-21 22:46 ` Matt Ranostay
@ 2014-09-22 7:36 ` Daniel Mack
0 siblings, 0 replies; 13+ messages in thread
From: Daniel Mack @ 2014-09-22 7:36 UTC (permalink / raw)
To: Matt Ranostay
Cc: Kumar Gala, linux-input, linux-kernel, Dmitry Torokhov,
Rob Herring, devicetree
On 09/22/2014 12:46 AM, Matt Ranostay wrote:
> On Sun, Sep 21, 2014 at 2:58 AM, Daniel Mack <daniel@zonque.org> wrote:
>> On 09/21/2014 05:01 AM, Matt Ranostay wrote:
>>> @@ -313,12 +307,16 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
>>>
>>> static const struct of_device_id cap1106_dt_ids[] = {
>>> { .compatible = "microchip,cap1106", },
>>> + { .compatible = "microchip,cap1126", },
>>> + { .compatible = "microchip,cap1188", },
>>
>> Hmm, how can that work unless you set .data to the number of channels
>> here? Did you test that with a DT-enabled board?
>>
> Yes it was tested on a BBB. The num_channels is set from cap1106_i2c_ids
Ah ok. I forgot there's this fallback to the i2c ids. What others driver
do is to use of_match_device() in the probe function, and then access
->data of the returned match.
But I'm fine with falling back to cap1106_i2c_ids unless anyone else has
objections.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] cap1106: support for active-high interrupt option
2014-09-22 5:56 ` Dmitry Torokhov
@ 2014-09-22 7:44 ` Daniel Mack
0 siblings, 0 replies; 13+ messages in thread
From: Daniel Mack @ 2014-09-22 7:44 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Matt Ranostay, galak, linux-input, linux-kernel, robh+dt, devicetree
On 09/22/2014 07:56 AM, Dmitry Torokhov wrote:
> On Sun, Sep 21, 2014 at 12:06:30PM +0200, Daniel Mack wrote:
>> On 09/21/2014 05:01 AM, Matt Ranostay wrote:
>>> Some applications need to use the active-high push-pull interrupt
>>> option. This allows it be enabled in the device tree child node.
>>>
>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>> ---
>>> drivers/input/keyboard/cap1106.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/input/keyboard/cap1106.c b/drivers/input/keyboard/cap1106.c
>>> index b9c43b5..33e2590 100644
>>> --- a/drivers/input/keyboard/cap1106.c
>>> +++ b/drivers/input/keyboard/cap1106.c
>>> @@ -234,6 +234,12 @@ static int cap1106_i2c_probe(struct i2c_client *i2c_client,
>>> dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
>>> }
>>>
>>> + if (of_property_read_bool(node, "microchip,active-high")) {
>>
>> I think the name of that property should make clear it's only changing
>> the interrupt output driver configuration. What about
>> "microchip,irq-active-high"?
>
> Can we infer the setting from IRQ flags by chance?
Hmm, I thought of that as well, but there could be electrical wiring
setups that want the CPU's hardware pin in push/pull mode but the one on
the sensor chip in open-drain. I'd rather not make the assuption the
pins are directly connected and have both sides individually configurable.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-09-22 7:44 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-21 3:01 [PATCH 0/3] cap1106: add support for cap11xx variants Matt Ranostay
2014-09-21 3:01 ` [PATCH 1/3] cap1106: Add support for various cap11xx devices Matt Ranostay
2014-09-21 9:58 ` Daniel Mack
2014-09-21 22:46 ` Matt Ranostay
2014-09-22 7:36 ` Daniel Mack
2014-09-22 0:28 ` Matt Ranostay
2014-09-22 5:59 ` Dmitry Torokhov
2014-09-21 10:53 ` Daniel Mack
2014-09-21 3:01 ` [PATCH 2/3] cap1106: support for active-high interrupt option Matt Ranostay
2014-09-21 10:06 ` Daniel Mack
2014-09-22 5:56 ` Dmitry Torokhov
2014-09-22 7:44 ` Daniel Mack
2014-09-21 3:01 ` [PATCH 3/3] dt: cap1106 active-high property addition Matt Ranostay
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).