| Message ID | 1360246668-2291-4-git-send-email-pmeerw@pmeerw.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Peter, Is the problem that the 224e is missing T28 object? This change seems extremely 224/224e specific. This driver, however, supports Atmel touch devices that are neither 224 or 224e, many have the SPT_CTECONFIG_T28 object. The driver does read out a supported object table from the device during initialize before handle_pdata. What sort of problem is there in trying to write the voltage to a non existent T28 object? If the object is missing, then mxt_write_object for voltage should simply return -EINVAL, and no harm should be done. On Thu, Feb 7, 2013 at 6:17 AM, Peter Meerwald <pmeerw@pmeerw.net> wrote: > From: Peter Meerwald <p.meerwald@bct-electronic.com> > > only the mxt224 has a SPT_CTECONFIG_T28 object, the mxt224e doesn't > > Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com> > --- > drivers/input/touchscreen/atmel_mxt_ts.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > index 63a82bb..17cc15a 100644 > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > @@ -41,6 +41,10 @@ > #define MXT_OBJECT_NUM 0x06 > #define MXT_OBJECT_START 0x07 > > +/* Family IDs */ > +#define MXT224_ID 0x80 > +#define MXT224E_ID 0x81 > + > #define MXT_OBJECT_SIZE 6 > > /* Object types */ > @@ -687,7 +691,7 @@ static void mxt_handle_pdata(struct mxt_data *data) > MXT_TOUCH_YRANGE_MSB, (pdata->y_size - 1) >> 8); > > /* Set touchscreen voltage */ > - if (pdata->voltage) { > + if (pdata->voltage && data->info.family_id == MXT224_ID) { As an alternative, could you not check for the existence of the voltage object in the table here using mxt_get_object rather than specifically switching on family ID number? > if (pdata->voltage < MXT_VOLTAGE_DEFAULT) { > voltage = (MXT_VOLTAGE_DEFAULT - pdata->voltage) / > MXT_VOLTAGE_STEP; > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Thanks. -- Benson Leung Software Engineer, Chrom* OS bleung@chromium.org -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Benson, > Is the problem that the 224e is missing T28 object? > This change seems extremely 224/224e specific. This driver, however, > supports Atmel touch devices that are neither 224 or 224e, many have > the SPT_CTECONFIG_T28 object. yes, I only have access to 224 and 224e; it is not clear what devices are supposed to be supported > The driver does read out a supported object table from the device > during initialize before handle_pdata. What sort of problem is there > in trying to write the voltage to a non existent T28 object? If the > object is missing, then mxt_write_object for voltage should simply > return -EINVAL, and no harm should be done. a bogus error message is written, 'Invalid object type' in mxt_get_object() > On Thu, Feb 7, 2013 at 6:17 AM, Peter Meerwald <pmeerw@pmeerw.net> wrote: > > From: Peter Meerwald <p.meerwald@bct-electronic.com> > > > > only the mxt224 has a SPT_CTECONFIG_T28 object, the mxt224e doesn't > > > > Signed-off-by: Peter Meerwald <p.meerwald@bct-electronic.com> > > --- > > drivers/input/touchscreen/atmel_mxt_ts.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > > index 63a82bb..17cc15a 100644 > > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > > @@ -41,6 +41,10 @@ > > #define MXT_OBJECT_NUM 0x06 > > #define MXT_OBJECT_START 0x07 > > > > +/* Family IDs */ > > +#define MXT224_ID 0x80 > > +#define MXT224E_ID 0x81 > > + > > #define MXT_OBJECT_SIZE 6 > > > > /* Object types */ > > @@ -687,7 +691,7 @@ static void mxt_handle_pdata(struct mxt_data *data) > > MXT_TOUCH_YRANGE_MSB, (pdata->y_size - 1) >> 8); > > > > /* Set touchscreen voltage */ > > - if (pdata->voltage) { > > + if (pdata->voltage && data->info.family_id == MXT224_ID) { > As an alternative, could you not check for the existence of the > voltage object in the table here using mxt_get_object rather than > specifically switching on family ID number? yes, I think that is a good suggestion > > if (pdata->voltage < MXT_VOLTAGE_DEFAULT) { > > voltage = (MXT_VOLTAGE_DEFAULT - pdata->voltage) / > > MXT_VOLTAGE_STEP; regards, p.
On Wed, Feb 20, 2013 at 2:22 PM, Peter Meerwald <pmeerw@pmeerw.net> wrote: > Hello Benson, >> The driver does read out a supported object table from the device >> during initialize before handle_pdata. What sort of problem is there >> in trying to write the voltage to a non existent T28 object? If the >> object is missing, then mxt_write_object for voltage should simply >> return -EINVAL, and no harm should be done. > > a bogus error message is written, 'Invalid object type' in > mxt_get_object() > Ah, I see. There is a dev_err in mxt_get_object. In that case, my suggestion to check mxt_get_object will also throw the same bogus error message. Maybe that should be a dev_warn instead?
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 63a82bb..17cc15a 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -41,6 +41,10 @@ #define MXT_OBJECT_NUM 0x06 #define MXT_OBJECT_START 0x07 +/* Family IDs */ +#define MXT224_ID 0x80 +#define MXT224E_ID 0x81 + #define MXT_OBJECT_SIZE 6 /* Object types */ @@ -687,7 +691,7 @@ static void mxt_handle_pdata(struct mxt_data *data) MXT_TOUCH_YRANGE_MSB, (pdata->y_size - 1) >> 8); /* Set touchscreen voltage */ - if (pdata->voltage) { + if (pdata->voltage && data->info.family_id == MXT224_ID) { if (pdata->voltage < MXT_VOLTAGE_DEFAULT) { voltage = (MXT_VOLTAGE_DEFAULT - pdata->voltage) / MXT_VOLTAGE_STEP;