Re: [PATCH v7 12/34] pinctrl: airoha: add missed get_direction() function for gpio_chip

On 7/27/26 11:04, Lorenzo Bianconi wrote:
>> This patch adds missed get_direction() function for gpio_chip.
>> Also it reimplements pinconf's get_direction() function using
>> newly defined function.
>>
>> Fixes: 1c8ace2d0725 ("pinctrl: airoha: Add support for EN7581 SoC")
>> Signed-off-by: Mikhail Kshevetskiy <[email protected]>
>> ---
>>  drivers/pinctrl/airoha/pinctrl-airoha.c | 41 ++++++++++++++++++-------
>>  1 file changed, 30 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
>> index 820b7b0443851..b52eb39c55ff3 100644
>> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
>> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
>> @@ -2502,6 +2502,27 @@ static int airoha_gpio_get(struct gpio_chip *chip, unsigned int gpio)
>>  	return err ? err : !!(val & BIT(pin));
>>  }
>>  
>> +static int airoha_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)
>> +{
>> +	struct airoha_pinctrl *pinctrl = gpiochip_get_data(chip);
>> +	u32 mask, index, val;
>> +	int err, field_shift;
>> +
>> +	field_shift = 2 * (gpio % AIROHA_REG_GPIOCTRL_NUM_PIN);
>> +	mask = GENMASK(field_shift + 1, field_shift);
> before we where checking just BIT(field_shift) as mask, while now we are
> checking two bits. 
As I know upper bits treated as reserved. I think we should not use gpio
operations if upper bit is not zero.
What is your opinion?
>  
>
>> +	index = gpio / AIROHA_REG_GPIOCTRL_NUM_PIN;
>> +
>> +	err = regmap_read(pinctrl->regmap,
>> +			  pinctrl->gpiochip.dir[index], &val);
>> +	if (err)
>> +		return err;
>> +
>> +	if ((val & mask) > BIT(field_shift))
>> +		return -EINVAL;
>> +
>> +	return (val & mask) ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> nit: unnecessary brackets.
>
>> +}
>> +
>>  static int airoha_gpio_direction_output(struct gpio_chip *chip,
>>  					unsigned int gpio, int value)
>>  {
>> @@ -2648,6 +2669,7 @@ static int airoha_pinctrl_add_gpiochip(struct airoha_pinctrl *pinctrl,
>>  	gc->free = gpiochip_generic_free;
>>  	gc->direction_input = pinctrl_gpio_direction_input;
>>  	gc->direction_output = airoha_gpio_direction_output;
>> +	gc->get_direction = airoha_gpio_get_direction;
>>  	gc->set = airoha_gpio_set;
>>  	gc->get = airoha_gpio_get;
>>  	gc->base = -1;
>> @@ -2855,21 +2877,18 @@ static int airoha_pinctrl_set_conf(struct airoha_pinctrl *pinctrl,
>>  static int airoha_pinconf_get_direction(struct pinctrl_dev *pctrl_dev, u32 p)
>>  {
>>  	struct airoha_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev);
>> -	u32 val, mask;
>> -	int err, pin;
>> -	u8 index;
>> +	int err, gpio;
>>  
>> -	pin = airoha_convert_pin_to_reg_offset(pctrl_dev, NULL, p);
>> -	if (pin < 0)
>> -		return pin;
>> +	gpio = airoha_convert_pin_to_reg_offset(pctrl_dev, NULL, p);
> if you do not rename pin in gpio here the patch will be simpler.
>
> Regards,
> Lorenzo
>
>> +	if (gpio < 0)
>> +		return gpio;
>>  
>> -	index = pin / AIROHA_REG_GPIOCTRL_NUM_PIN;
>> -	err = regmap_read(pinctrl->regmap, pinctrl->gpiochip.dir[index], &val);
>> -	if (err)
>> +	err = airoha_gpio_get_direction(&pinctrl->gpiochip.chip, gpio);
>> +	if (err < 0)
>>  		return err;
>>  
>> -	mask = BIT(2 * (pin % AIROHA_REG_GPIOCTRL_NUM_PIN));
>> -	return val & mask ? PIN_CONFIG_OUTPUT_ENABLE : PIN_CONFIG_INPUT_ENABLE;
>> +	return err == GPIO_LINE_DIRECTION_OUT ?
>> +	       PIN_CONFIG_OUTPUT_ENABLE : PIN_CONFIG_INPUT_ENABLE;
>>  }
>>  
>>  static int airoha_pinconf_get(struct pinctrl_dev *pctrl_dev,
>> -- 
>> 2.53.0
>>