Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Joe Perches <joe@perches.com>]

On Wed, 2015-12-09 at 22:02 +0200, Andy Shevchenko wrote:
> On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe@perches.com> wrote:
[]
> > > > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > > > index 992457b..49113aa 100644
> > > > --- a/lib/hexdump.c
> > > > +++ b/lib/hexdump.c
> > > > @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
> > > >   * @len: number of bytes in the @buf
> > > >   * @rowsize: number of bytes to print per line; must be 16 or 32
> > > >   * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > > > + *             OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
> > > >   * @linebuf: where to put the converted data
> > > >   * @linebuflen: total size of @linebuf, including space for terminating NUL
> > > >   * @ascii: include ASCII after the hex output
> > > > @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> > > >         int j, lx = 0;
> > > >         int ascii_column;
> > > >         int ret;
> > > > +       int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
> > > 
> > > I would rather prefer to have function parameter to be renamed.
> > > 
> > > E.g. gsflags ?
> > > 
> > 
> > Well, it's a bit simpler changelog.
> 
> Generally I'm fine with this version, though take into account above and below.
> 
> > ---
> >  include/linux/printk.h |  7 +++++++
> >  lib/hexdump.c          | 39 +++++++++++++++++++++++++++++++--------
> >  2 files changed, 38 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > index 9729565..4be190c 100644
> > --- a/include/linux/printk.h
> > +++ b/include/linux/printk.h
> > @@ -424,6 +424,13 @@ enum {
> >         DUMP_PREFIX_ADDRESS,
> >         DUMP_PREFIX_OFFSET
> >  };
> > +
> > +enum {
> > +       DUMP_TYPE_CPU = 0,
> 
> And still open this, do we need it? I think you may just mention in
> the documentation that default behaviour is CPU like.

The only documentation I'm aware of is the kernel-doc

> > +       DUMP_TYPE_LE = BIT(30),
> > +       DUMP_TYPE_BE = BIT(31)
> > +};
> > +
> >  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> >                               int groupsize, char *linebuf, size_t linebuflen,
> 
> Here as well to change.

Right, thanks.

> >                               bool ascii);
> > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > index 992457b..5b1eda70 100644
> > --- a/lib/hexdump.c
> > +++ b/lib/hexdump.c
> > @@ -80,7 +80,8 @@ EXPORT_SYMBOL(bin2hex);
> >   * @buf: data blob to dump
> >   * @len: number of bytes in the @buf
> >   * @rowsize: number of bytes to print per line; must be 16 or 32
> > - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + *              OR with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
> 
> Maybe specify "bitwise OR with.." ?