Re: [PATCH] staging: ks7010: replace DPRINTK traces in favour of preferred netdev_*

[Dan Carpenter]

On Tue, Mar 13, 2018 at 11:57:35AM +0100, Sergio Paracuellos wrote:
> On Tue, Mar 13, 2018 at 11:04 AM, Dan Carpenter
>  wrote:
> > It takes a long time to review this, not because it's hard but because
> > I have to look at each line and think "Is this really a worthwhile line
> > to keep?" and a lot of them are marginal but perhaps not necessarily
> > bad?
> 
> I see. I though if they were there, it might be useful traces for
> somebody in any kind of way :-).
> 

Yeah.  I know, right?  It feels like the original author must have
thought they were super useful and you and I probably don't even have
this hardware (I don't) so it feels wrong to delete too much stuff...

But it's probably the right thing.

I can bet that I will automatically approve of almost anything error
message deletion but it's hard for me to delete the stuff myself because
I am a hoarder by nature.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ks7010: replace DPRINTK traces in favour of preferred netdev_*

[Sergio Paracuellos]

On Tue, Mar 13, 2018 at 11:04 AM, Dan Carpenter
 wrote:
> It takes a long time to review this, not because it's hard but because
> I have to look at each line and think "Is this really a worthwhile line
> to keep?" and a lot of them are marginal but perhaps not necessarily
> bad?

I see. I though if they were there, it might be useful traces for
somebody in any kind of way :-).

>
> You've deleted some obviously rubbish printks but I feel you could have
> gone much further.  That would be an easy patch to review if it just
> deleted printks.  Maybe say [PATCH 1/2] delete stuff.  [PATCH 2/2] Use
> netdev_*.
>

I'll review these changes in the way you are saying and resend a new
patchset with this fixed.


> On Tue, Mar 13, 2018 at 07:42:22AM +0100, Sergio Paracuellos wrote:
>> @@ -767,20 +753,20 @@ static int ks7010_upload_firmware(struct ks_sdio_card 
>> *card)
>>
>>  static void ks7010_card_init(struct ks_wlan_private *priv)
>>  {
>> - DPRINTK(5, "\ncard_init_task()\n");
>> + netdev_dbg(priv->net_dev, "\ncard_init_task()\n");
>>
>
> This one is obviously useless.  We already have ftrace to tell use when
> functions are called.  And also the \n at the start is wrong because
> it messes up the dmesg log levels.

Agreed. Thanks for pointing this out.

>
> regards,
> dan carpenter

Best regards,
Sergio Paracuellos
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ks7010: replace DPRINTK traces in favour of preferred netdev_*

[Dan Carpenter]

It takes a long time to review this, not because it's hard but because
I have to look at each line and think "Is this really a worthwhile line
to keep?" and a lot of them are marginal but perhaps not necessarily
bad?

You've deleted some obviously rubbish printks but I feel you could have
gone much further.  That would be an easy patch to review if it just
deleted printks.  Maybe say [PATCH 1/2] delete stuff.  [PATCH 2/2] Use
netdev_*.

On Tue, Mar 13, 2018 at 07:42:22AM +0100, Sergio Paracuellos wrote:
> @@ -767,20 +753,20 @@ static int ks7010_upload_firmware(struct ks_sdio_card 
> *card)
>  
>  static void ks7010_card_init(struct ks_wlan_private *priv)
>  {
> - DPRINTK(5, "\ncard_init_task()\n");
> + netdev_dbg(priv->net_dev, "\ncard_init_task()\n");
>  

This one is obviously useless.  We already have ftrace to tell use when
functions are called.  And also the \n at the start is wrong because
it messes up the dmesg log levels.

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel