Hi Dmitry,
> Hi Lukasz,
>
> On Fri, Jul 12, 2019 at 12:23:46AM +0200, Lukasz Majewski wrote:
> > From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> >
> > 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 <s.hauer@xxxxxxxxxxxxxx>
> > Signed-off-by: Lukasz Majewski <lukma@xxxxxxx>
> >
> > ---
> > 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: lukma@xxxxxxx
Attachment:
pgpvGbWIdb0V9.pgp
Description: OpenPGP digital signature
