Re: [PATCH v3 07/11] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 12/16/22 14:57, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:09PM +0100, Hans de Goede wrote:
>> Add a helper function to map the type returned by the _DSM
>> method to a function name + the default polarity for that function.
>>
>> And fold the INT3472_GPIO_TYPE_RESET and INT3472_GPIO_TYPE_POWERDOWN
>> cases into a single generic case.
>>
>> This is a preparation patch for further GPIO mapping changes.
> 
> ...
> 
>> +static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
> 
> I find a bit strange not making this a function that returns func:
> 
> static const char *int3472_get_func_and_polarity(u8 type, u32 *polarity)

Why make it return func and not polarity ?

Since there are 2 values to return it makes sense to be
consistent and return both by reference.

Also this sort of comments really get into the territory
of bikeshedding which is not helpful IMHO.

Regards,

Hans




> 
>> +{
>> +	switch (type) {
>> +	case INT3472_GPIO_TYPE_RESET:
>> +		*func = "reset";
>> +		*polarity = GPIO_ACTIVE_LOW;
> 
> 		return "reset";
> 
> etc.
> 
>> +		break;
>> +	case INT3472_GPIO_TYPE_POWERDOWN:
>> +		*func = "powerdown";
>> +		*polarity = GPIO_ACTIVE_LOW;
>> +		break;
>> +	case INT3472_GPIO_TYPE_CLK_ENABLE:
>> +		*func = "clk-enable";
>> +		*polarity = GPIO_ACTIVE_HIGH;
>> +		break;
>> +	case INT3472_GPIO_TYPE_PRIVACY_LED:
>> +		*func = "privacy-led";
>> +		*polarity = GPIO_ACTIVE_HIGH;
>> +		break;
>> +	case INT3472_GPIO_TYPE_POWER_ENABLE:
>> +		*func = "power-enable";
>> +		*polarity = GPIO_ACTIVE_HIGH;
>> +		break;
>> +	default:
>> +		*func = "unknown";
>> +		*polarity = GPIO_ACTIVE_HIGH;
>> +		break;
>> +	}
>> +}
>