Re: [PATCH v2 5/5] platform/x86: int3472: Clean up GPIO parsing

Moi,

On Tue, Aug 25, 2026 at 01:40:25PM +0300, Ilpo Järvinen wrote:
> On Tue, 25 Aug 2026, Sakari Ailus wrote:
> 
> > In skl_int3472_handle_gpio_resources(), return an error where it happens,
> > except when we're holding a reference to a GPIO. This involves
> > incrementing ngpios earlier on but that does not introduce a functional
> > change.
> > 
> > Signed-off-by: Sakari Ailus <[email protected]>
> > ---
> >  drivers/platform/x86/intel/int3472/discrete.c | 42 +++++++++----------
> >  1 file changed, 21 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> > index 2024eaa09033..400bee9e4cb8 100644
> > --- a/drivers/platform/x86/intel/int3472/discrete.c
> > +++ b/drivers/platform/x86/intel/int3472/discrete.c
> > @@ -369,15 +369,23 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
> >  		agpio->resource_source.string_ptr, agpio->pin_table[0],
> >  		str_high_low(gpio_flags == GPIO_ACTIVE_HIGH));
> >  
> > +	/*
> > +	 * int3472->ngpios can be incremented here as it is an argument to the
> > +	 * _DSM, not e.g. an index to an array in C. Additionally, in case of an
> > +	 * error the value won't be used.
> > +	 */
> 
> I wasn't expecting comment for this. You might have added it because of 
> my questions but IMO it's not needed.
> 
> I think just the old way/place was confusing, especially given how 
> some/part of the error cases did increment it. But after moving it here, 
> it makes much more sense even without that comment.
> 
> And one can determine it's not being used for C arrays by using grep so 
> that too seems just unnecessary detail.

Ok, I'll drop it.

> 
> > +	int3472->ngpios++;
> > +
> >  	switch (type) {
> >  	case INT3472_GPIO_TYPE_RESET:
> >  	case INT3472_GPIO_TYPE_POWERDOWN:
> >  	case INT3472_GPIO_TYPE_HOTPLUG_DETECT:
> >  		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, con_id, gpio_flags);
> >  		if (ret)
> > -			dev_err_probe(int3472->dev, ret, "Failed to map GPIO pin to sensor\n");
> > +			return dev_err_probe(int3472->dev, ret,
> > +					     "Failed to map GPIO pin to sensor\n");
> >  
> > -		break;
> > +		return 0;
> >  	case INT3472_GPIO_TYPE_CLK_ENABLE:
> >  	case INT3472_GPIO_TYPE_PRIVACY_LED:
> >  	case INT3472_GPIO_TYPE_STROBE:
> > @@ -385,24 +393,24 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
> >  	case INT3472_GPIO_TYPE_DOVDD:
> >  	case INT3472_GPIO_TYPE_HANDSHAKE:
> >  		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, con_id, gpio_flags);
> > -		if (IS_ERR(gpio)) {
> > -			ret = PTR_ERR(gpio);
> > -			dev_err_probe(int3472->dev, ret, "Failed to get GPIO\n");
> > -			break;
> > -		}
> > +		if (IS_ERR(gpio))
> > +			return dev_err_probe(int3472->dev, PTR_ERR(gpio),
> > +					     "Failed to get GPIO\n");
> >  
> >  		switch (type) {
> >  		case INT3472_GPIO_TYPE_CLK_ENABLE:
> >  			ret = skl_int3472_register_gpio_clock(int3472, gpio);
> >  			if (ret)
> > -				dev_err_probe(int3472->dev, ret, "Failed to register clock\n");
> > +				dev_err_probe(int3472->dev, ret,
> > +					      "Failed to register clock\n");
> 
> Don't know why changed linesplits in this instead doing them directly in 
> the 2nd patch.

Oops, that's where I intended to make them. I'll fix that for v3.

-- 
Sakari Ailus