Re: [PATCH 03/10] mfd / platform: cros_ec: Miscellaneous character device to talk with the EC

[Greg Kroah-Hartman]

On Thu, Jun 06, 2019 at 11:01:17AM -0300, Ezequiel Garcia wrote:
> On Tue, 2019-06-04 at 20:59 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 04, 2019 at 11:39:21AM -0700, Guenter Roeck wrote:
> > > On Tue, Jun 4, 2019 at 11:35 AM Greg Kroah-Hartman
> > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > On Tue, Jun 04, 2019 at 01:58:38PM -0300, Ezequiel Garcia wrote:
> > > > > Hey Greg,
> > > > > 
> > > > > > > + dev_info(&pdev->dev, "Created misc device /dev/%s\n",
> > > > > > > +          data->misc.name);
> > > > > > 
> > > > > > No need to be noisy, if all goes well, your code should be quiet.
> > > > > > 
> > > > > 
> > > > > I sometimes wonder about this being noise or not, so I will slightly
> > > > > hijack this thread for this discussion.
> > > > > 
> > > > > > From a kernel developer point-of-view, or even from a platform
> > > > > developer or user with a debugging hat point-of-view, having
> > > > > a "device created" or "device registered" message is often very useful.
> > > > 
> > > > For you, yes.  For someone with 30000 devices attached to their system,
> > > > it is not, and causes booting to take longer than it should be.
> > > > 
> > > > > In fact, I wish people would do this more often, so I don't have to
> > > > > deal with dynamic debug, or hack my way:
> > > > > 
> > > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > > index 4589631798c9..473549b26bb2 100644
> > > > > --- a/drivers/media/i2c/ov5647.c
> > > > > +++ b/drivers/media/i2c/ov5647.c
> > > > > @@ -603,7 +603,7 @@ static int ov5647_probe(struct i2c_client *client,
> > > > >         if (ret < 0)
> > > > >                 goto error;
> > > > > 
> > > > > -       dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
> > > > > +       dev_info(dev, "OmniVision OV5647 camera driver probed\n");
> > > > >         return 0;
> > > > >  error:
> > > > >         media_entity_cleanup(&sd->entity);
> > > > > 
> > > > > In some subsystems, it's even a behavior I'm more or less relying on:
> > > > > 
> > > > > $ git grep v4l2_info.*registered drivers/media/ | wc -l
> > > > > 26
> > > > > 
> > > > > And on the downsides, I can't find much. It's just one little line,
> > > > > that is not even noticed unless you have logging turned on.
> > > > 
> > > > Its better to be quiet, which is why the "default driver registration"
> > > > macros do not have any printk messages in them.  When converting drivers
> > > > over to it, we made the boot process much more sane, don't try to go and
> > > > add messages for no good reason back in please.
> > > > 
> > > > dynamic debugging can be enabled on a module and line-by-line basis,
> > > > even from the boot command line.  So if you need debugging, you can
> > > > always ask someone to just reboot or unload/load the module and get the
> > > > message that way.
> > > > 
> > > 
> > > Can we by any chance make this an official policy ? I am kind of tired
> > > having to argue about this over and over again.
> > 
> > Sure, but how does anyone make any "official policy" in the kernel?  :)
> > 
> > I could just go through and delete all "look ma, a new driver/device!"
> > messages, but that might be annoying...
> > 
> 
> Well, I really need to task.

???

> If it's not an official policy (and won't be anytime soon?),

The ":)" there was that we really have very few "official" policies,
only things that we all strongly encourage to happen.  And get grumpy if
we see them in code reviews.  Like I did here.

> then what's preventing Enric from pushing this print on this driver,
> given he is the one maintaining the code?

Given that he wants people to review his code, why would you tell him to
ignore what people are trying to tell him?

Again, don't be noisy, it's not hard, and is how things have been
trending for many years now.

thanks,

greg k-h