Re: [PATCH i-g-t 09/20] tools/intel_vbt_decode: Extract dump_pnp_id()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, May 31, 2024 at 05:48:21PM +0300, Jani Nikula wrote:
> On Fri, 31 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Several different VBT blocks contain PnP IDs. Extract the
> > helper to dump them out.
> 
> Just nitpicking inline, can be fixed in follow-up. The patch is an
> improvement as-is.
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  tools/intel_vbt_decode.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/intel_vbt_decode.c b/tools/intel_vbt_decode.c
> > index 6cf6d471b5e2..2a11490eed22 100644
> > --- a/tools/intel_vbt_decode.c
> > +++ b/tools/intel_vbt_decode.c
> > @@ -1394,6 +1394,18 @@ static char *decode_pnp_id(u16 mfg_name, char str[4])
> >  	return str;
> >  }
> >  
> > +static void dump_pnp_id(const struct bdb_edid_pnp_id *pnp_id)
> 
> Maybe pass prefix or indent level?

Yeah, thought about doing something like that, but decided
to leave it out for now as the series was ballooning out
of control already.

> 
> > +{
> > +	char mfg[4];
> > +
> > +	printf("\t\t  Mfg name: %s (0x%x)\n",
> > +	       decode_pnp_id(pnp_id->mfg_name, mfg), pnp_id->mfg_name);
> > +	printf("\t\t  Product code: %u\n", pnp_id->product_code);
> > +	printf("\t\t  Serial: %u\n", pnp_id->serial);
> > +	printf("\t\t  Mfg week: %d\n", pnp_id->mfg_week);
> > +	printf("\t\t  Mfg year: %d\n", 1990 + pnp_id->mfg_year);
> 
> These could be improved to match spec (see decode_date() in kernel
> drm_edid.c).
> 
> > +}
> > +
> >  static void dump_lfp_data(struct context *context,
> >  			  const struct bdb_block *block)
> >  {
> > @@ -1419,7 +1431,6 @@ static void dump_lfp_data(struct context *context,
> >  			block_data(block) + ptrs->ptr[i].panel_pnp_id.offset;
> >  		const struct bdb_lfp_data_tail *tail =
> >  			block_data(block) + ptrs->panel_name.offset;
> > -		char mfg[4];
> >  
> >  		if (!dump_panel(context, i))
> >  			continue;
> > @@ -1457,12 +1468,7 @@ static void dump_lfp_data(struct context *context,
> >  		       "BAD!" : "good");
> >  
> >  		printf("\t\tPnP ID:\n");
> 
> I'd do this in dump_pnp_id() too.

That's more of a sub-heading for this block type. For other
block types it would probably be a bit redundant.

> 
> > -		printf("\t\t  Mfg name: %s (0x%x)\n",
> > -		       decode_pnp_id(pnp_id->mfg_name, mfg), pnp_id->mfg_name);
> > -		printf("\t\t  Product code: %u\n", pnp_id->product_code);
> > -		printf("\t\t  Serial: %u\n", pnp_id->serial);
> > -		printf("\t\t  Mfg week: %d\n", pnp_id->mfg_week);
> > -		printf("\t\t  Mfg year: %d\n", 1990 + pnp_id->mfg_year);
> > +		dump_pnp_id(pnp_id);
> >  
> >  		if (!ptrs->panel_name.table_size)
> >  			continue;
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel