| Message ID | 20210203111239.18313-1-colin.king@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [next] usb: cdnsp: Fix spelling mistake "delagete" -> "delegate" | expand |
Hi Dan, >> From: Colin Ian King <colin.king@canonical.com> >> >> There is a spelling mistake in a literal string. Fix it. >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> drivers/usb/cdns3/cdnsp-ep0.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c >> index e2b1bcb3f80e..e30931ebc870 100644 >> --- a/drivers/usb/cdns3/cdnsp-ep0.c >> +++ b/drivers/usb/cdns3/cdnsp-ep0.c >> @@ -45,7 +45,7 @@ static int cdnsp_ep0_delegate_req(struct cdnsp_device *pdev, >> { >> int ret; >> >> - trace_cdnsp_ep0_request("delagete"); >> + trace_cdnsp_ep0_request("delegate"); >> > >This printk is useless and should just be deleted. Use ftrace instead. Maybe this printk is redundant but it's more comfortable in use. To debug I can simply enable cdns-dev events (echo cdnsp-dev:* > set_event) and I will get the full picture of what the driver is doing. Otherwise, I must remember which function I need to add to set_ftrace_filter. Of course, by default I can simply add all cdnsp* functions (echo cdnsp* > set_ftrace_filter) but it increases the trace log and makes it a little more difficult to analyze. So maybe in some cases we shouldn't complain for such printk ? It's my private opinion and not necessarily correct :) Thanks, Pawel Laszczak > >regards, >dan carpenter
On Thu, Feb 04, 2021 at 05:07:16AM +0000, Pawel Laszczak wrote: > Hi Dan, > > >> From: Colin Ian King <colin.king@canonical.com> > >> > >> There is a spelling mistake in a literal string. Fix it. > >> > >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > >> --- > >> drivers/usb/cdns3/cdnsp-ep0.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c > >> index e2b1bcb3f80e..e30931ebc870 100644 > >> --- a/drivers/usb/cdns3/cdnsp-ep0.c > >> +++ b/drivers/usb/cdns3/cdnsp-ep0.c > >> @@ -45,7 +45,7 @@ static int cdnsp_ep0_delegate_req(struct cdnsp_device *pdev, > >> { > >> int ret; > >> > >> - trace_cdnsp_ep0_request("delagete"); > >> + trace_cdnsp_ep0_request("delegate"); > >> > > > >This printk is useless and should just be deleted. Use ftrace instead. > > Maybe this printk is redundant but it's more comfortable in use. > To debug I can simply enable cdns-dev events (echo cdnsp-dev:* > set_event) > and I will get the full picture of what the driver is doing. > > Otherwise, I must remember which function I need to add to set_ftrace_filter. > Of course, by default I can simply add all cdnsp* functions (echo cdnsp* > set_ftrace_filter) but it > increases the trace log and makes it a little more difficult to analyze. > > So maybe in some cases we shouldn't complain for such printk ? > > It's my private opinion and not necessarily correct :) Please don't have duplicate tracepoints for something like "this function is now called", it's redundant. thanks, greg k-h
I've sent the patch that remove this one and others similar printk from driver. > > >On Thu, Feb 04, 2021 at 05:07:16AM +0000, Pawel Laszczak wrote: >> Hi Dan, >> >> >> From: Colin Ian King <colin.king@canonical.com> >> >> >> >> There is a spelling mistake in a literal string. Fix it. >> >> >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> >> --- >> >> drivers/usb/cdns3/cdnsp-ep0.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c >> >> index e2b1bcb3f80e..e30931ebc870 100644 >> >> --- a/drivers/usb/cdns3/cdnsp-ep0.c >> >> +++ b/drivers/usb/cdns3/cdnsp-ep0.c >> >> @@ -45,7 +45,7 @@ static int cdnsp_ep0_delegate_req(struct cdnsp_device *pdev, >> >> { >> >> int ret; >> >> >> >> - trace_cdnsp_ep0_request("delagete"); >> >> + trace_cdnsp_ep0_request("delegate"); >> >> >> > >> >This printk is useless and should just be deleted. Use ftrace instead. >> >> Maybe this printk is redundant but it's more comfortable in use. >> To debug I can simply enable cdns-dev events (echo cdnsp-dev:* > set_event) >> and I will get the full picture of what the driver is doing. >> >> Otherwise, I must remember which function I need to add to set_ftrace_filter. >> Of course, by default I can simply add all cdnsp* functions (echo cdnsp* > set_ftrace_filter) but it >> increases the trace log and makes it a little more difficult to analyze. >> >> So maybe in some cases we shouldn't complain for such printk ? >> >> It's my private opinion and not necessarily correct :) > >Please don't have duplicate tracepoints for something like "this >function is now called", it's redundant. > Thanks, Pawel Laszczak
On 04/02/2021 09:25, Pawel Laszczak wrote: > > I've sent the patch that remove this one and others similar printk from driver. Thanks Pawel. Colin > >> >> >> On Thu, Feb 04, 2021 at 05:07:16AM +0000, Pawel Laszczak wrote: >>> Hi Dan, >>> >>>>> From: Colin Ian King <colin.king@canonical.com> >>>>> >>>>> There is a spelling mistake in a literal string. Fix it. >>>>> >>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>>>> --- >>>>> drivers/usb/cdns3/cdnsp-ep0.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c >>>>> index e2b1bcb3f80e..e30931ebc870 100644 >>>>> --- a/drivers/usb/cdns3/cdnsp-ep0.c >>>>> +++ b/drivers/usb/cdns3/cdnsp-ep0.c >>>>> @@ -45,7 +45,7 @@ static int cdnsp_ep0_delegate_req(struct cdnsp_device *pdev, >>>>> { >>>>> int ret; >>>>> >>>>> - trace_cdnsp_ep0_request("delagete"); >>>>> + trace_cdnsp_ep0_request("delegate"); >>>>> >>>> >>>> This printk is useless and should just be deleted. Use ftrace instead. >>> >>> Maybe this printk is redundant but it's more comfortable in use. >>> To debug I can simply enable cdns-dev events (echo cdnsp-dev:* > set_event) >>> and I will get the full picture of what the driver is doing. >>> >>> Otherwise, I must remember which function I need to add to set_ftrace_filter. >>> Of course, by default I can simply add all cdnsp* functions (echo cdnsp* > set_ftrace_filter) but it >>> increases the trace log and makes it a little more difficult to analyze. >>> >>> So maybe in some cases we shouldn't complain for such printk ? >>> >>> It's my private opinion and not necessarily correct :) >> >> Please don't have duplicate tracepoints for something like "this >> function is now called", it's redundant. >> > > Thanks, > Pawel Laszczak >
diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c index e2b1bcb3f80e..e30931ebc870 100644 --- a/drivers/usb/cdns3/cdnsp-ep0.c +++ b/drivers/usb/cdns3/cdnsp-ep0.c @@ -45,7 +45,7 @@ static int cdnsp_ep0_delegate_req(struct cdnsp_device *pdev, { int ret; - trace_cdnsp_ep0_request("delagete"); + trace_cdnsp_ep0_request("delegate"); spin_unlock(&pdev->lock); ret = pdev->gadget_driver->setup(&pdev->gadget, ctrl);