| Message ID | 20190827062943.20698-5-jiada_wang@mentor.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | atmel_mxt_ts misc | expand |
On Tue, Aug 27, 2019 at 03:29:23PM +0900, Jiada Wang wrote: > From: Nick Dyer <nick.dyer@itdev.co.uk> > > Add a debug switch which causes all messages from the touch controller to > be dumped to the dmesg log with a set prefix "MXT MSG:". This is used by > Atmel user-space utilities to debug touch operation. Enabling this output > does impact touch performance. > > Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk> > (cherry picked from ndyer/linux/for-upstream commit 3c3fcfdd4889dfeb1c80ae8cd94a622c6342b06a) > [gdavis: Forward port and fix conflicts.] > Signed-off-by: George G. Davis <george_davis@mentor.com> > Signed-off-by: Jiada Wang <jiada_wang@mentor.com> > --- > drivers/input/touchscreen/atmel_mxt_ts.c | 44 ++++++++++++++++++++++-- > 1 file changed, 41 insertions(+), 3 deletions(-) > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > index 2d2e8ea30547..941c6970cb70 100644 > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > @@ -335,6 +335,7 @@ struct mxt_data { > u8 t100_aux_ampl; > u8 t100_aux_area; > u8 t100_aux_vect; > + bool debug_enabled; > u8 max_reportid; > u32 config_crc; > u32 info_crc; > @@ -460,8 +461,8 @@ static bool mxt_object_readable(unsigned int type) > > static void mxt_dump_message(struct mxt_data *data, u8 *message) > { > - dev_dbg(&data->client->dev, "message: %*ph\n", > - data->T5_msg_size, message); > + dev_dbg(&data->client->dev, "MXT MSG: %*ph\n", > + data->T5_msg_size, message); I'm not 100% convinced that the kernel should change here (arguably the userspace utility should be modified instead) but if the messages are conforming to some sort of vendor specific protocol then some commenting is needed. > @@ -3538,6 +3573,8 @@ static DEVICE_ATTR(hw_version, S_IRUGO, mxt_hw_version_show, NULL); > static DEVICE_ATTR(object, S_IRUGO, mxt_object_show, NULL); > static DEVICE_ATTR(update_cfg, S_IWUSR, NULL, mxt_update_cfg_store); > static DEVICE_ATTR(config_crc, S_IRUGO, mxt_config_crc_show, NULL); > +static DEVICE_ATTR(debug_enable, S_IWUSR | S_IRUSR, mxt_debug_enable_show, > + mxt_debug_enable_store); Why isn't CONFIG_DYNAMIC_DEBUG sufficient to enable/disable the messages? Daniel.
Hi Daniel On 2019/08/30 0:31, Daniel Thompson wrote: > On Tue, Aug 27, 2019 at 03:29:23PM +0900, Jiada Wang wrote: >> From: Nick Dyer <nick.dyer@itdev.co.uk> >> >> Add a debug switch which causes all messages from the touch controller to >> be dumped to the dmesg log with a set prefix "MXT MSG:". This is used by >> Atmel user-space utilities to debug touch operation. Enabling this output >> does impact touch performance. >> >> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk> >> (cherry picked from ndyer/linux/for-upstream commit 3c3fcfdd4889dfeb1c80ae8cd94a622c6342b06a) >> [gdavis: Forward port and fix conflicts.] >> Signed-off-by: George G. Davis <george_davis@mentor.com> >> Signed-off-by: Jiada Wang <jiada_wang@mentor.com> >> --- >> drivers/input/touchscreen/atmel_mxt_ts.c | 44 ++++++++++++++++++++++-- >> 1 file changed, 41 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c >> index 2d2e8ea30547..941c6970cb70 100644 >> --- a/drivers/input/touchscreen/atmel_mxt_ts.c >> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c >> @@ -335,6 +335,7 @@ struct mxt_data { >> u8 t100_aux_ampl; >> u8 t100_aux_area; >> u8 t100_aux_vect; >> + bool debug_enabled; >> u8 max_reportid; >> u32 config_crc; >> u32 info_crc; >> @@ -460,8 +461,8 @@ static bool mxt_object_readable(unsigned int type) >> >> static void mxt_dump_message(struct mxt_data *data, u8 *message) >> { >> - dev_dbg(&data->client->dev, "message: %*ph\n", >> - data->T5_msg_size, message); >> + dev_dbg(&data->client->dev, "MXT MSG: %*ph\n", >> + data->T5_msg_size, message); > > I'm not 100% convinced that the kernel should change here (arguably the > userspace utility should be modified instead) but if the messages are > conforming to some sort of vendor specific protocol then some commenting > is needed. I will update with inline comment > > >> @@ -3538,6 +3573,8 @@ static DEVICE_ATTR(hw_version, S_IRUGO, mxt_hw_version_show, NULL); >> static DEVICE_ATTR(object, S_IRUGO, mxt_object_show, NULL); >> static DEVICE_ATTR(update_cfg, S_IWUSR, NULL, mxt_update_cfg_store); >> static DEVICE_ATTR(config_crc, S_IRUGO, mxt_config_crc_show, NULL); >> +static DEVICE_ATTR(debug_enable, S_IWUSR | S_IRUSR, mxt_debug_enable_show, >> + mxt_debug_enable_store); > > Why isn't CONFIG_DYNAMIC_DEBUG sufficient to enable/disable the > messages? > thanks for the comment, I think the only difference is, by only using CONFIG_DYNAMC_DEBUG, it's hard to differentiate the messages between valid report_id and exceptional case (explicitly set of "dump = true") Thanks, Jiada > > Daniel. >
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 2d2e8ea30547..941c6970cb70 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -335,6 +335,7 @@ struct mxt_data { u8 t100_aux_ampl; u8 t100_aux_area; u8 t100_aux_vect; + bool debug_enabled; u8 max_reportid; u32 config_crc; u32 info_crc; @@ -460,8 +461,8 @@ static bool mxt_object_readable(unsigned int type) static void mxt_dump_message(struct mxt_data *data, u8 *message) { - dev_dbg(&data->client->dev, "message: %*ph\n", - data->T5_msg_size, message); + dev_dbg(&data->client->dev, "MXT MSG: %*ph\n", + data->T5_msg_size, message); } static int mxt_wait_for_completion(struct mxt_data *data, @@ -1214,6 +1215,7 @@ static void mxt_proc_t93_messages(struct mxt_data *data, u8 *msg) static int mxt_proc_message(struct mxt_data *data, u8 *message) { u8 report_id = message[0]; + bool dump = data->debug_enabled; if (report_id == MXT_RPTID_NOMSG) return 0; @@ -1248,9 +1250,12 @@ static int mxt_proc_message(struct mxt_data *data, u8 *message) } else if (report_id == data->T93_reportid) { mxt_proc_t93_messages(data, message); } else { - mxt_dump_message(data, message); + dump = true; } + if (dump) + mxt_dump_message(data, message); + return 1; } @@ -3522,6 +3527,36 @@ static ssize_t mxt_update_cfg_store(struct device *dev, return ret; } +static ssize_t mxt_debug_enable_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct mxt_data *data = dev_get_drvdata(dev); + char c; + + c = data->debug_enabled ? '1' : '0'; + return scnprintf(buf, PAGE_SIZE, "%c\n", c); +} + +static ssize_t mxt_debug_enable_store(struct device *dev, + struct device_attribute *attr, const char *buf, size_t count) +{ + struct mxt_data *data = dev_get_drvdata(dev); + u8 i; + ssize_t ret; + + if (kstrtou8(buf, 0, &i) == 0 && i < 2) { + data->debug_enabled = (i == 1); + + dev_dbg(dev, "%s\n", i ? "debug enabled" : "debug disabled"); + ret = count; + } else { + dev_dbg(dev, "debug_enabled write error\n"); + ret = -EINVAL; + } + + return ret; +} + static DEVICE_ATTR(update_fw, S_IWUSR, NULL, mxt_update_fw_store); static struct attribute *mxt_fw_attrs[] = { @@ -3538,6 +3573,8 @@ static DEVICE_ATTR(hw_version, S_IRUGO, mxt_hw_version_show, NULL); static DEVICE_ATTR(object, S_IRUGO, mxt_object_show, NULL); static DEVICE_ATTR(update_cfg, S_IWUSR, NULL, mxt_update_cfg_store); static DEVICE_ATTR(config_crc, S_IRUGO, mxt_config_crc_show, NULL); +static DEVICE_ATTR(debug_enable, S_IWUSR | S_IRUSR, mxt_debug_enable_show, + mxt_debug_enable_store); static struct attribute *mxt_attrs[] = { &dev_attr_fw_version.attr, @@ -3545,6 +3582,7 @@ static struct attribute *mxt_attrs[] = { &dev_attr_object.attr, &dev_attr_update_cfg.attr, &dev_attr_config_crc.attr, + &dev_attr_debug_enable.attr, NULL };