This patch set provides several enhancements to mc13xxx MFD family
of devices by introducing mc34708 as a separate device.
This IC has dedicated pen detection feature, which allows better
touchscreen experience.
This is the seconf version of this code (v2).
Discussion regarding v1 can be found here:
https://lkml.org/lkml/2018/4/12/351
https://www.mail-archive.com/[email protected]/msg1661934.html
https://www.mail-archive.com/[email protected]/msg1664296.html
Sascha Hauer (3):
mfd: mc13xxx: Add mc34708 adc support
input: touchscreen mc13xxx: Make platform data optional
input: touchscreen mc13xxx: Add mc34708 support
drivers/input/touchscreen/mc13783_ts.c | 74 ++++++++++++++++++++----
drivers/mfd/mc13xxx-core.c | 102 ++++++++++++++++++++++++++++++++-
drivers/mfd/mc13xxx.h | 3 +
include/linux/mfd/mc34708.h | 37 ++++++++++++
4 files changed, 204 insertions(+), 12 deletions(-)
create mode 100644 include/linux/mfd/mc34708.h
--
2.11.0
From: Sascha Hauer <[email protected]>
The mc34708 has a different bit to enable pen detection. This
adds the driver data and devtype necessary to probe the device
and to distinguish between the mc13783 and the mc34708.
Signed-off-by: Sascha Hauer <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
---
Changes for v2:
- Change nested if statements to a single one (with cr0 > ...)
- Replace hardcoded max resistance value (4080) with a generic driver data
value.
- Introduce new include/linux/mfd/mc34708.h header file for mc34708 specific
defines
- Define as driver data mask and value for accessing mc13xxx registers
Changes from the original patch:
- Simplify the mcXXXXX_set_pen_detection functions
- Fix checkpatch warnings
---
drivers/input/touchscreen/mc13783_ts.c | 59 +++++++++++++++++++++++++++++++---
1 file changed, 55 insertions(+), 4 deletions(-)
diff --git a/drivers/input/touchscreen/mc13783_ts.c b/drivers/input/touchscreen/mc13783_ts.c
index edd49e44e0c9..8fd3d0e47f57 100644
--- a/drivers/input/touchscreen/mc13783_ts.c
+++ b/drivers/input/touchscreen/mc13783_ts.c
@@ -10,6 +10,7 @@
*/
#include <linux/platform_device.h>
#include <linux/mfd/mc13783.h>
+#include <linux/mfd/mc34708.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/input.h>
@@ -30,6 +31,8 @@ MODULE_PARM_DESC(sample_tolerance,
"is supposed to be wrong and is discarded. Set to 0 to "
"disable this check.");
+struct mc13xxx_driver_data;
+
struct mc13783_ts_priv {
struct input_dev *idev;
struct mc13xxx *mc13xxx;
@@ -37,6 +40,33 @@ struct mc13783_ts_priv {
unsigned int sample[4];
u8 ato;
bool atox;
+ struct mc13xxx_driver_data *drvdata;
+};
+
+enum mc13xxx_type {
+ MC13XXX_TYPE_MC13783,
+ MC13XXX_TYPE_MC34708,
+};
+
+struct mc13xxx_driver_data {
+ enum mc13xxx_type type;
+ int max_resistance;
+ u32 reg_mask;
+ u32 reg_value;
+};
+
+static struct mc13xxx_driver_data mc13783_driver_data = {
+ .type = MC13XXX_TYPE_MC13783,
+ .max_resistance = 4096,
+ .reg_mask = MC13XXX_ADC0_TSMOD_MASK,
+ .reg_value = MC13XXX_ADC0_TSMOD0,
+};
+
+static struct mc13xxx_driver_data mc34708_driver_data = {
+ .type = MC13XXX_TYPE_MC34708,
+ .max_resistance = 4080,
+ .reg_mask = MC34708_ADC0_TSMASK,
+ .reg_value = MC34708_ADC0_TSPENDETEN,
};
static irqreturn_t mc13783_ts_handler(int irq, void *data)
@@ -93,6 +123,10 @@ static void mc13783_ts_report_sample(struct mc13783_ts_priv *priv)
cr0 = (cr0 + cr1) / 2;
+ if (priv->drvdata->type == MC13XXX_TYPE_MC34708 &&
+ cr0 > priv->drvdata->max_resistance)
+ cr0 = 0;
+
if (!cr0 || !sample_tolerance ||
(x2 - x0 < sample_tolerance &&
y2 - y0 < sample_tolerance)) {
@@ -102,14 +136,14 @@ static void mc13783_ts_report_sample(struct mc13783_ts_priv *priv)
input_report_abs(idev, ABS_Y, y1);
dev_dbg(&idev->dev, "report (%d, %d, %d)\n",
- x1, y1, 0x1000 - cr0);
+ x1, y1, priv->drvdata->max_resistance - cr0);
schedule_delayed_work(&priv->work, HZ / 50);
} else {
dev_dbg(&idev->dev, "report release\n");
}
input_report_abs(idev, ABS_PRESSURE,
- cr0 ? 0x1000 - cr0 : cr0);
+ cr0 ? priv->drvdata->max_resistance - cr0 : 0);
input_report_key(idev, BTN_TOUCH, cr0);
input_sync(idev);
} else {
@@ -146,7 +180,8 @@ static int mc13783_ts_open(struct input_dev *dev)
goto out;
ret = mc13xxx_reg_rmw(priv->mc13xxx, MC13XXX_ADC0,
- MC13XXX_ADC0_TSMOD_MASK, MC13XXX_ADC0_TSMOD0);
+ priv->drvdata->reg_mask,
+ priv->drvdata->reg_value);
if (ret)
mc13xxx_irq_free(priv->mc13xxx, MC13XXX_IRQ_TS, priv);
out:
@@ -160,7 +195,7 @@ static void mc13783_ts_close(struct input_dev *dev)
mc13xxx_lock(priv->mc13xxx);
mc13xxx_reg_rmw(priv->mc13xxx, MC13XXX_ADC0,
- MC13XXX_ADC0_TSMOD_MASK, 0);
+ priv->drvdata->reg_mask, 0);
mc13xxx_irq_free(priv->mc13xxx, MC13XXX_IRQ_TS, priv);
mc13xxx_unlock(priv->mc13xxx);
@@ -172,6 +207,7 @@ static int __init mc13783_ts_probe(struct platform_device *pdev)
struct mc13783_ts_priv *priv;
struct mc13xxx_ts_platform_data *pdata = dev_get_platdata(&pdev->dev);
struct input_dev *idev;
+ const struct platform_device_id *id = platform_get_device_id(pdev);
int ret = -ENOMEM;
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -182,6 +218,7 @@ static int __init mc13783_ts_probe(struct platform_device *pdev)
INIT_DELAYED_WORK(&priv->work, mc13783_ts_work);
priv->mc13xxx = dev_get_drvdata(pdev->dev.parent);
priv->idev = idev;
+ priv->drvdata = (void *)id->driver_data;
if (pdata) {
priv->atox = pdata->atox;
@@ -228,7 +265,21 @@ static int mc13783_ts_remove(struct platform_device *pdev)
return 0;
}
+static const struct platform_device_id mc13xxx_ts_idtable[] = {
+ {
+ .name = "mc13783-ts",
+ .driver_data = (kernel_ulong_t)&mc13783_driver_data,
+ }, {
+ .name = "mc34708-ts",
+ .driver_data = (kernel_ulong_t)&mc34708_driver_data,
+ }, {
+ /* sentinel */
+ }
+};
+MODULE_DEVICE_TABLE(platform, mc13xxx_ts_idtable);
+
static struct platform_driver mc13783_ts_driver = {
+ .id_table = mc13xxx_ts_idtable,
.remove = mc13783_ts_remove,
.driver = {
.name = MC13783_TS_NAME,
--
2.11.0
Hi Lukasz,
On Fri, Jul 12, 2019 at 12:23:46AM +0200, Lukasz Majewski wrote:
> From: Sascha Hauer <[email protected]>
>
> The mc34708 has a different bit to enable pen detection. This
> adds the driver data and devtype necessary to probe the device
> and to distinguish between the mc13783 and the mc34708.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Signed-off-by: Lukasz Majewski <[email protected]>
>
> ---
> Changes for v2:
> - Change nested if statements to a single one (with cr0 > ...)
> - Replace hardcoded max resistance value (4080) with a generic driver data
> value.
> - Introduce new include/linux/mfd/mc34708.h header file for mc34708 specific
> defines
> - Define as driver data mask and value for accessing mc13xxx registers
>
> Changes from the original patch:
> - Simplify the mcXXXXX_set_pen_detection functions
> - Fix checkpatch warnings
> ---
> drivers/input/touchscreen/mc13783_ts.c | 59 +++++++++++++++++++++++++++++++---
> 1 file changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/touchscreen/mc13783_ts.c b/drivers/input/touchscreen/mc13783_ts.c
> index edd49e44e0c9..8fd3d0e47f57 100644
> --- a/drivers/input/touchscreen/mc13783_ts.c
> +++ b/drivers/input/touchscreen/mc13783_ts.c
> @@ -10,6 +10,7 @@
> */
> #include <linux/platform_device.h>
> #include <linux/mfd/mc13783.h>
> +#include <linux/mfd/mc34708.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/input.h>
> @@ -30,6 +31,8 @@ MODULE_PARM_DESC(sample_tolerance,
> "is supposed to be wrong and is discarded. Set to 0 to "
> "disable this check.");
>
> +struct mc13xxx_driver_data;
Why don't you define the structure here instead of sing forward
declaration? The structure is also commonly called as xxx_chip, so
struct mc13xxx_chip {
...
};
> +
> struct mc13783_ts_priv {
> struct input_dev *idev;
> struct mc13xxx *mc13xxx;
> @@ -37,6 +40,33 @@ struct mc13783_ts_priv {
> unsigned int sample[4];
> u8 ato;
> bool atox;
> + struct mc13xxx_driver_data *drvdata;
const struct mc13xxx_chip *chip;
> +};
> +
> +enum mc13xxx_type {
> + MC13XXX_TYPE_MC13783,
> + MC13XXX_TYPE_MC34708,
> +};
> +
> +struct mc13xxx_driver_data {
> + enum mc13xxx_type type;
> + int max_resistance;
> + u32 reg_mask;
> + u32 reg_value;
> +};
> +
> +static struct mc13xxx_driver_data mc13783_driver_data = {
> + .type = MC13XXX_TYPE_MC13783,
> .max_resistance = 4096,
> + .reg_mask = MC13XXX_ADC0_TSMOD_MASK,
> + .reg_value = MC13XXX_ADC0_TSMOD0,
> +};
> +
> +static struct mc13xxx_driver_data mc34708_driver_data = {
> + .type = MC13XXX_TYPE_MC34708,
> + .max_resistance = 4080,
> + .reg_mask = MC34708_ADC0_TSMASK,
> + .reg_value = MC34708_ADC0_TSPENDETEN,
> };
Have these 2 closer to the ID table.
>
> static irqreturn_t mc13783_ts_handler(int irq, void *data)
> @@ -93,6 +123,10 @@ static void mc13783_ts_report_sample(struct mc13783_ts_priv *priv)
>
> cr0 = (cr0 + cr1) / 2;
>
> + if (priv->drvdata->type == MC13XXX_TYPE_MC34708 &&
> + cr0 > priv->drvdata->max_resistance)
> + cr0 = 0;
I would like to avoid the type comparisons. Given that both cr0 and cr1
can't be more than 4095 (because we limit them when parsing sampling
data) I think we can simply say
if (cr0 > priv->chip->max_resistance)
cr0 = 0;
Thanks.
--
Dmitry
Hi Dmitry,
> Hi Lukasz,
>
> On Fri, Jul 12, 2019 at 12:23:46AM +0200, Lukasz Majewski wrote:
> > From: Sascha Hauer <[email protected]>
> >
> > The mc34708 has a different bit to enable pen detection. This
> > adds the driver data and devtype necessary to probe the device
> > and to distinguish between the mc13783 and the mc34708.
> >
> > Signed-off-by: Sascha Hauer <[email protected]>
> > Signed-off-by: Lukasz Majewski <[email protected]>
> >
> > ---
> > Changes for v2:
> > - Change nested if statements to a single one (with cr0 > ...)
> > - Replace hardcoded max resistance value (4080) with a generic
> > driver data value.
> > - Introduce new include/linux/mfd/mc34708.h header file for mc34708
> > specific defines
> > - Define as driver data mask and value for accessing mc13xxx
> > registers
> >
> > Changes from the original patch:
> > - Simplify the mcXXXXX_set_pen_detection functions
> > - Fix checkpatch warnings
> > ---
> > drivers/input/touchscreen/mc13783_ts.c | 59
> > +++++++++++++++++++++++++++++++--- 1 file changed, 55
> > insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/mc13783_ts.c
> > b/drivers/input/touchscreen/mc13783_ts.c index
> > edd49e44e0c9..8fd3d0e47f57 100644 ---
> > a/drivers/input/touchscreen/mc13783_ts.c +++
> > b/drivers/input/touchscreen/mc13783_ts.c @@ -10,6 +10,7 @@
> > */
> > #include <linux/platform_device.h>
> > #include <linux/mfd/mc13783.h>
> > +#include <linux/mfd/mc34708.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/input.h>
> > @@ -30,6 +31,8 @@ MODULE_PARM_DESC(sample_tolerance,
> > "is supposed to be wrong and is discarded. Set to
> > 0 to " "disable this check.");
> >
> > +struct mc13xxx_driver_data;
>
> Why don't you define the structure here instead of sing forward
> declaration?
I will define the structure here.
> The structure is also commonly called as xxx_chip, so
> struct mc13xxx_chip {
> ...
> };
Ok.
>
> > +
> > struct mc13783_ts_priv {
> > struct input_dev *idev;
> > struct mc13xxx *mc13xxx;
> > @@ -37,6 +40,33 @@ struct mc13783_ts_priv {
> > unsigned int sample[4];
> > u8 ato;
> > bool atox;
> > + struct mc13xxx_driver_data *drvdata;
>
> const struct mc13xxx_chip *chip;
Ok. I will adjust the name.
>
> > +};
> > +
> > +enum mc13xxx_type {
> > + MC13XXX_TYPE_MC13783,
> > + MC13XXX_TYPE_MC34708,
> > +};
> > +
> > +struct mc13xxx_driver_data {
> > + enum mc13xxx_type type;
> > + int max_resistance;
> > + u32 reg_mask;
> > + u32 reg_value;
> > +};
> > +
> > +static struct mc13xxx_driver_data mc13783_driver_data = {
> > + .type = MC13XXX_TYPE_MC13783,
> > .max_resistance = 4096,
> > + .reg_mask = MC13XXX_ADC0_TSMOD_MASK,
> > + .reg_value = MC13XXX_ADC0_TSMOD0,
> > +};
> > +
> > +static struct mc13xxx_driver_data mc34708_driver_data = {
> > + .type = MC13XXX_TYPE_MC34708,
> > + .max_resistance = 4080,
> > + .reg_mask = MC34708_ADC0_TSMASK,
> > + .reg_value = MC34708_ADC0_TSPENDETEN,
> > };
>
> Have these 2 closer to the ID table.
I will move those two instances of struct mc13xxx_chip closer to the ID
table.
>
> >
> > static irqreturn_t mc13783_ts_handler(int irq, void *data)
> > @@ -93,6 +123,10 @@ static void mc13783_ts_report_sample(struct
> > mc13783_ts_priv *priv)
> > cr0 = (cr0 + cr1) / 2;
> >
> > + if (priv->drvdata->type == MC13XXX_TYPE_MC34708 &&
> > + cr0 > priv->drvdata->max_resistance)
> > + cr0 = 0;
>
> I would like to avoid the type comparisons. Given that both cr0 and
> cr1 can't be more than 4095 (because we limit them when parsing
> sampling data) I think we can simply say
>
> if (cr0 > priv->chip->max_resistance)
> cr0 = 0;
Ok.
>
> Thanks.
>
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: [email protected]