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

[Lorenzo Bianconi]

On Jul 27, Mikhail Kshevetskiy wrote:
> 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 <mikhail.kshevetskiy@xxxxxxxxx>
> >> ---
> >>  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?

I guess it is better to use a mask of just one bit here.

Regards,
Lorenzo

> >  
> >
> >> +	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
> >>

Attachment: signature.asc
Description: PGP signature