Re: [PATCH v2 02/11] media: Introduce helpers to fill pixel format structs

[Ezequiel Garcia <ezequiel@collabora.com>]

On Tue, 2019-03-12 at 09:29 +0100, Hans Verkuil wrote:
> On 3/4/19 8:25 PM, Ezequiel Garcia wrote:
> > Add two new API helpers, v4l2_fill_pixfmt and v4l2_fill_pixfmt_mp,
> > to be used by drivers to calculate plane sizes and bytes per lines.
> > 
> > Note that driver-specific padding and alignment are not
> > taken into account, and must be done by drivers using this API.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-common.c | 186 ++++++++++++++++++++++++++
> >  include/media/v4l2-common.h           |  32 +++++
> >  2 files changed, 218 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 663730f088cd..11a16bb3efda 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -44,6 +44,7 @@
> >   * Added Gerd Knorrs v4l1 enhancements (Justin Schoeman)
> >   */
> >  
> > +#include <linux/ctype.h>
> >  #include <linux/module.h>
> >  #include <linux/types.h>
> >  #include <linux/kernel.h>
> > @@ -445,3 +446,188 @@ int v4l2_s_parm_cap(struct video_device *vdev,
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_s_parm_cap);
> > +
> > +static char printable_char(int c)
> > +{
> > +	return isascii(c) && isprint(c) ? c : '?';
> > +}
> > +
> > +const char *v4l2_get_fourcc_name(uint32_t format)
> > +{
> > +	static char buf[8];
> > +
> > +	snprintf(buf, 8,
> > +		 "%c%c%c%c%s",
> > +		 printable_char(format & 0xff),
> > +		 printable_char((format >> 8) & 0xff),
> > +		 printable_char((format >> 16) & 0xff),
> > +		 printable_char((format >> 24) & 0x7f),
> > +		 (format & BIT(31)) ? "-BE" : "");
> > +
> > +	return buf;
> > +}
> > +EXPORT_SYMBOL(v4l2_get_fourcc_name);
> 
> This function isn't re-entrant, but it should be. Multiple threads may be
> calling it at the same time.
> 
> It is probably best to pass the buffer pointer as an argument.
> 

Boy, shame on me, really missed this!

> I would also prefer to split this patch into two: the first adding
> v4l2_format_info, the second adding v4l2_get_fourcc_name. This in case
> that the v4l2_get_fourcc_name() function needs more work.
> 

OK.

Thanks a lot for the feedback,
Eze