| Message ID | X9FZFs3NZADoIhhH@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 03161a952c7c564aa186f94cf2cdbf834c8e624c |
| Headers | show |
| Series | Input: edt-ft5x06 - consolidate handling of number of electrodes | expand |
On Wed, Dec 09, 2020 at 03:09:10PM -0800, Dmitry Torokhov wrote: > Instead of using special-casing retrieval of number of X/Y electrodes > based on the firmware, let's select default values and mark registers as > non-existent on firmwares that do not support this operation. > > Also mark "report rate" register as non-existent for generic firmwares as > having it set to 0 does not make sense. Makes sense, FWIW, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/touchscreen/edt-ft5x06.c | 43 ++++++++++---------------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c > index 6ff81d48da86..2eefbc2485bc 100644 > --- a/drivers/input/touchscreen/edt-ft5x06.c > +++ b/drivers/input/touchscreen/edt-ft5x06.c > @@ -69,6 +69,9 @@ > #define EDT_RAW_DATA_RETRIES 100 > #define EDT_RAW_DATA_DELAY 1000 /* usec */ > > +#define EDT_DEFAULT_NUM_X 1024 > +#define EDT_DEFAULT_NUM_Y 1024 > + > enum edt_pmode { > EDT_PMODE_NOT_SUPPORTED, > EDT_PMODE_HIBERNATE, > @@ -977,8 +980,7 @@ static void edt_ft5x06_ts_get_defaults(struct device *dev, > } > } > > -static void > -edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata) > +static void edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata) > { > struct edt_reg_addr *reg_addr = &tsdata->reg_addr; > > @@ -997,21 +999,17 @@ edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata) > if (reg_addr->reg_report_rate != NO_REGISTER) > tsdata->report_rate = edt_ft5x06_register_read(tsdata, > reg_addr->reg_report_rate); > - if (tsdata->version == EDT_M06 || > - tsdata->version == EDT_M09 || > - tsdata->version == EDT_M12) { > + tsdata->num_x = EDT_DEFAULT_NUM_X; > + if (reg_addr->reg_num_x != NO_REGISTER) > tsdata->num_x = edt_ft5x06_register_read(tsdata, > reg_addr->reg_num_x); > + tsdata->num_y = EDT_DEFAULT_NUM_Y; > + if (reg_addr->reg_num_y != NO_REGISTER) > tsdata->num_y = edt_ft5x06_register_read(tsdata, > reg_addr->reg_num_y); > - } else { > - tsdata->num_x = -1; > - tsdata->num_y = -1; > - } > } > > -static void > -edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata) > +static void edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata) > { > struct edt_reg_addr *reg_addr = &tsdata->reg_addr; > > @@ -1041,22 +1039,25 @@ edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata) > > case EV_FT: > reg_addr->reg_threshold = EV_REGISTER_THRESHOLD; > + reg_addr->reg_report_rate = NO_REGISTER; > reg_addr->reg_gain = EV_REGISTER_GAIN; > reg_addr->reg_offset = NO_REGISTER; > reg_addr->reg_offset_x = EV_REGISTER_OFFSET_X; > reg_addr->reg_offset_y = EV_REGISTER_OFFSET_Y; > reg_addr->reg_num_x = NO_REGISTER; > reg_addr->reg_num_y = NO_REGISTER; > - reg_addr->reg_report_rate = NO_REGISTER; > break; > > case GENERIC_FT: > /* this is a guesswork */ > reg_addr->reg_threshold = M09_REGISTER_THRESHOLD; > + reg_addr->reg_report_rate = NO_REGISTER; > reg_addr->reg_gain = M09_REGISTER_GAIN; > reg_addr->reg_offset = M09_REGISTER_OFFSET; > reg_addr->reg_offset_x = NO_REGISTER; > reg_addr->reg_offset_y = NO_REGISTER; > + reg_addr->reg_num_x = NO_REGISTER; > + reg_addr->reg_num_y = NO_REGISTER; > break; > } > } > @@ -1195,20 +1196,10 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, > input->id.bustype = BUS_I2C; > input->dev.parent = &client->dev; > > - if (tsdata->version == EDT_M06 || > - tsdata->version == EDT_M09 || > - tsdata->version == EDT_M12) { > - input_set_abs_params(input, ABS_MT_POSITION_X, > - 0, tsdata->num_x * 64 - 1, 0, 0); > - input_set_abs_params(input, ABS_MT_POSITION_Y, > - 0, tsdata->num_y * 64 - 1, 0, 0); > - } else { > - /* Unknown maximum values. Specify via devicetree */ > - input_set_abs_params(input, ABS_MT_POSITION_X, > - 0, 65535, 0, 0); > - input_set_abs_params(input, ABS_MT_POSITION_Y, > - 0, 65535, 0, 0); > - } > + input_set_abs_params(input, ABS_MT_POSITION_X, > + 0, tsdata->num_x * 64 - 1, 0, 0); > + input_set_abs_params(input, ABS_MT_POSITION_Y, > + 0, tsdata->num_y * 64 - 1, 0, 0); > > touchscreen_parse_properties(input, true, &tsdata->prop); > > -- > 2.29.2.576.ga3fc446d84-goog > > > -- > Dmitry
Hi Dmitry, On 20-12-09 15:09, Dmitry Torokhov wrote: > Instead of using special-casing retrieval of number of X/Y electrodes > based on the firmware, let's select default values and mark registers as > non-existent on firmwares that do not support this operation. > > Also mark "report rate" register as non-existent for generic firmwares as > having it set to 0 does not make sense. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/touchscreen/edt-ft5x06.c | 43 ++++++++++---------------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c > index 6ff81d48da86..2eefbc2485bc 100644 > --- a/drivers/input/touchscreen/edt-ft5x06.c > +++ b/drivers/input/touchscreen/edt-ft5x06.c > @@ -69,6 +69,9 @@ ... > case EV_FT: > reg_addr->reg_threshold = EV_REGISTER_THRESHOLD; > + reg_addr->reg_report_rate = NO_REGISTER; > reg_addr->reg_gain = EV_REGISTER_GAIN; > reg_addr->reg_offset = NO_REGISTER; > reg_addr->reg_offset_x = EV_REGISTER_OFFSET_X; > reg_addr->reg_offset_y = EV_REGISTER_OFFSET_Y; > reg_addr->reg_num_x = NO_REGISTER; > reg_addr->reg_num_y = NO_REGISTER; > - reg_addr->reg_report_rate = NO_REGISTER; > break; Nit: Unrelated change. However the patch looks good, thanks. Acked-by: Marco Felsch <m.felsch@pengutronix.de> Regards, Marco
On Thu, Dec 10, 2020 at 11:08:03AM +0100, Marco Felsch wrote: > On 20-12-09 15:09, Dmitry Torokhov wrote: ... > > case EV_FT: > > reg_addr->reg_threshold = EV_REGISTER_THRESHOLD; > > + reg_addr->reg_report_rate = NO_REGISTER; > > reg_addr->reg_gain = EV_REGISTER_GAIN; > > reg_addr->reg_offset = NO_REGISTER; > > reg_addr->reg_offset_x = EV_REGISTER_OFFSET_X; > > reg_addr->reg_offset_y = EV_REGISTER_OFFSET_Y; > > reg_addr->reg_num_x = NO_REGISTER; > > reg_addr->reg_num_y = NO_REGISTER; > > - reg_addr->reg_report_rate = NO_REGISTER; > > break; > > Nit: > Unrelated change. I guess the motive is to get these assignments consistent between the cases. Documentation actually allows this kind of modifications of code in one change when them are toughly related.
On Wed, 2020-12-09 at 15:09 -0800, Dmitry Torokhov wrote: > Instead of using special-casing retrieval of number of X/Y electrodes > based on the firmware, let's select default values and mark registers > as > non-existent on firmwares that do not support this operation. > > Also mark "report rate" register as non-existent for generic > firmwares as > having it set to 0 does not make sense. Looks good. Reviewed-by: Simon Budig <simon.budig@kernelconcepts.de> Thanks, Simon
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c index 6ff81d48da86..2eefbc2485bc 100644 --- a/drivers/input/touchscreen/edt-ft5x06.c +++ b/drivers/input/touchscreen/edt-ft5x06.c @@ -69,6 +69,9 @@ #define EDT_RAW_DATA_RETRIES 100 #define EDT_RAW_DATA_DELAY 1000 /* usec */ +#define EDT_DEFAULT_NUM_X 1024 +#define EDT_DEFAULT_NUM_Y 1024 + enum edt_pmode { EDT_PMODE_NOT_SUPPORTED, EDT_PMODE_HIBERNATE, @@ -977,8 +980,7 @@ static void edt_ft5x06_ts_get_defaults(struct device *dev, } } -static void -edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata) +static void edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata) { struct edt_reg_addr *reg_addr = &tsdata->reg_addr; @@ -997,21 +999,17 @@ edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata) if (reg_addr->reg_report_rate != NO_REGISTER) tsdata->report_rate = edt_ft5x06_register_read(tsdata, reg_addr->reg_report_rate); - if (tsdata->version == EDT_M06 || - tsdata->version == EDT_M09 || - tsdata->version == EDT_M12) { + tsdata->num_x = EDT_DEFAULT_NUM_X; + if (reg_addr->reg_num_x != NO_REGISTER) tsdata->num_x = edt_ft5x06_register_read(tsdata, reg_addr->reg_num_x); + tsdata->num_y = EDT_DEFAULT_NUM_Y; + if (reg_addr->reg_num_y != NO_REGISTER) tsdata->num_y = edt_ft5x06_register_read(tsdata, reg_addr->reg_num_y); - } else { - tsdata->num_x = -1; - tsdata->num_y = -1; - } } -static void -edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata) +static void edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata) { struct edt_reg_addr *reg_addr = &tsdata->reg_addr; @@ -1041,22 +1039,25 @@ edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata) case EV_FT: reg_addr->reg_threshold = EV_REGISTER_THRESHOLD; + reg_addr->reg_report_rate = NO_REGISTER; reg_addr->reg_gain = EV_REGISTER_GAIN; reg_addr->reg_offset = NO_REGISTER; reg_addr->reg_offset_x = EV_REGISTER_OFFSET_X; reg_addr->reg_offset_y = EV_REGISTER_OFFSET_Y; reg_addr->reg_num_x = NO_REGISTER; reg_addr->reg_num_y = NO_REGISTER; - reg_addr->reg_report_rate = NO_REGISTER; break; case GENERIC_FT: /* this is a guesswork */ reg_addr->reg_threshold = M09_REGISTER_THRESHOLD; + reg_addr->reg_report_rate = NO_REGISTER; reg_addr->reg_gain = M09_REGISTER_GAIN; reg_addr->reg_offset = M09_REGISTER_OFFSET; reg_addr->reg_offset_x = NO_REGISTER; reg_addr->reg_offset_y = NO_REGISTER; + reg_addr->reg_num_x = NO_REGISTER; + reg_addr->reg_num_y = NO_REGISTER; break; } } @@ -1195,20 +1196,10 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, input->id.bustype = BUS_I2C; input->dev.parent = &client->dev; - if (tsdata->version == EDT_M06 || - tsdata->version == EDT_M09 || - tsdata->version == EDT_M12) { - input_set_abs_params(input, ABS_MT_POSITION_X, - 0, tsdata->num_x * 64 - 1, 0, 0); - input_set_abs_params(input, ABS_MT_POSITION_Y, - 0, tsdata->num_y * 64 - 1, 0, 0); - } else { - /* Unknown maximum values. Specify via devicetree */ - input_set_abs_params(input, ABS_MT_POSITION_X, - 0, 65535, 0, 0); - input_set_abs_params(input, ABS_MT_POSITION_Y, - 0, 65535, 0, 0); - } + input_set_abs_params(input, ABS_MT_POSITION_X, + 0, tsdata->num_x * 64 - 1, 0, 0); + input_set_abs_params(input, ABS_MT_POSITION_Y, + 0, tsdata->num_y * 64 - 1, 0, 0); touchscreen_parse_properties(input, true, &tsdata->prop);
Instead of using special-casing retrieval of number of X/Y electrodes based on the firmware, let's select default values and mark registers as non-existent on firmwares that do not support this operation. Also mark "report rate" register as non-existent for generic firmwares as having it set to 0 does not make sense. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/touchscreen/edt-ft5x06.c | 43 ++++++++++---------------- 1 file changed, 17 insertions(+), 26 deletions(-)