Re: [PATCH v3 2/2] platform/x86: int3472: Add support for GPIO type 0x02 (strobe LED)

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Mar 26, 2026 at 12:57:49PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 26, 2026 at 12:55:43PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 25, 2026 at 11:38:23PM +0100, Marco Nenciarini wrote:

...

> > >  		case INT3472_GPIO_TYPE_PRIVACY_LED:
> > > -			ret = skl_int3472_register_led(int3472, gpio);
> > > +			ret = skl_int3472_register_led(int3472, gpio,
> > > +						       INT3472_LED_TYPE_PRIVACY);
> > 
> > Split this conversion to yet another separate change. At the end of the day
> > you should have something like this:
> > 
> > Patch 1: rename pled --> led
> > Patch 2: Use local led variable instead of dereferencing (as Ilpo asked for)
> > Patch 3: introduce led type and update API to use it
> > Patch 4: add IR flood support
> > 
> > > +			if (ret)
> > > +				err_msg = "Failed to register LED\n";
> > 
> > Move this now to the callee which may add a name/con_id of the led to the error
> > message.
> 
> With the above proposed split to the patches, this comment should be addressed
> already in patch 3.

Hmm... Looking into the current implementation, it's harder achieve than say.
So, then just

				err_msg = "Failed to register privacy LED\n";

...

Also consider using a separate data structure to list the map between type and
name (con_id)

static const char * const led_con_ids[] = {
	[INT3472_LED_TYPE_PRIVACY] = "privacy",
};

it will allow to get rid of switch-case.

-- 
With Best Regards,
Andy Shevchenko