Re: [PATCH v2 29/49] Input: atmel_mxt_ts - implement debug output for messages

[Jiada Wang]

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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > (cherry picked from ndyer/linux/for-upstream commit 3c3fcfdd4889dfeb1c80ae8cd94a622c6342b06a)
> > [gdavis: Forward port and fix conflicts.]
> > Signed-off-by: George G. Davis <george_davis@xxxxxxxxxx>
> > Signed-off-by: Jiada Wang <jiada_wang@xxxxxxxxxx>
> > ---
> >   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.