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]
pgpgCP2TwWVzy.pgp
Description: OpenPGP digital signature
