Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9338a2c2-fda5-d1d6-e4ad-c37de6bb5636@microchip.com>
Date:   Thu, 13 Apr 2023 18:24:38 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [linux][PATCH] pinctrl: at91-pio4: Add push-pull drive
 configuration
Content-Language: en-US
To:     <Ryan.Wanner@microchip.com>, <ludovic.desroches@microchip.com>,
        <linus.walleij@linaro.org>, <alexandre.belloni@bootlin.com>,
        <claudiu.beznea@microchip.com>
CC:     <linux-arm-kernel@lists.infradead.org>,
        <linux-gpio@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230412175146.886684-1-Ryan.Wanner@microchip.com>
From:   Nicolas Ferre <nicolas.ferre@microchip.com>
Organization: microchip
In-Reply-To: <20230412175146.886684-1-Ryan.Wanner@microchip.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/04/2023 at 19:51, Ryan.Wanner@microchip.com wrote:
> From: Ryan Wanner <Ryan.Wanner@microchip.com>
> 
> The PIO4 does support push-pull configuration as this is the default
> state of the open-drain register. Adapt the driver for this.
> 
> Signed-off-by: Ryan Wanner <Ryan.Wanner@microchip.com>
> ---
> This patch also fixes the warning of unsupported
> configuration param 8.
> 
> This was tested on both sama5d2-som1-ek and sama7g5ek. I used dbg_show
> fucntion to test if the configuration was correct when adding
> drive-open-drain or drive-push-pull to the dts file.
> 
>   drivers/pinctrl/pinctrl-at91-pio4.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
> index a30c6f7c9016..9a0cddfeaf92 100644
> --- a/drivers/pinctrl/pinctrl-at91-pio4.c
> +++ b/drivers/pinctrl/pinctrl-at91-pio4.c
> @@ -776,6 +776,11 @@ static int atmel_conf_pin_config_group_get(struct pinctrl_dev *pctldev,
>   			return -EINVAL;
>   		arg = 1;
>   		break;
> +	case PIN_CONFIG_DRIVE_PUSH_PULL:
> +		if ((res & ATMEL_PIO_OPD_MASK))
> +			return -EINVAL;
> +		arg = 1;
> +		break;
>   	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
>   		if (!(res & ATMEL_PIO_SCHMITT_MASK))
>   			return -EINVAL;
> @@ -839,10 +844,10 @@ static int atmel_conf_pin_config_group_set(struct pinctrl_dev *pctldev,
>   			conf &= (~ATMEL_PIO_PUEN_MASK);
>   			break;
>   		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> -			if (arg == 0)
> -				conf &= (~ATMEL_PIO_OPD_MASK);
> -			else
> -				conf |= ATMEL_PIO_OPD_MASK;
> +			conf |= ATMEL_PIO_OPD_MASK;
> +			break;

This chunk seems to change the behavior of the driver in case of 
OPEN_DRAIN by ignoring the "arg" encoded parameter.

It seems that arg was used equal to 0 at least here:
https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2504

(we talked about this internally, but I didn't see it this way: sorry Ryan)

Now we set the OPD bit unconditionally, which seems the proper way, but 
it changes things: I would advice to separate this change into another 
patch.

I'm very surprised we didn't see this "bug" earlier because it seems to 
be present since the creation of the file.

The rest of the addition looks good to me.

> +		case PIN_CONFIG_DRIVE_PUSH_PULL:
> +			conf &= (~ATMEL_PIO_OPD_MASK);
>   			break;
>   		case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
>   			if (arg == 0)
> @@ -937,8 +942,10 @@ static void atmel_conf_pin_config_dbg_show(struct pinctrl_dev *pctldev,
>   		seq_printf(s, "%s ", "pull-down");
>   	if (conf & ATMEL_PIO_IFEN_MASK)
>   		seq_printf(s, "%s ", "debounce");
> -	if (conf & ATMEL_PIO_OPD_MASK)
> +	if ((conf & ATMEL_PIO_OPD_MASK) > 0)

The 2 lines are equivalent, keep the former one.

>   		seq_printf(s, "%s ", "open-drain");
> +	if ((conf & ATMEL_PIO_OPD_MASK) == 0)

here, simply a "else" does the trick

> +		seq_printf(s, "%s ", "push-pull");
>   	if (conf & ATMEL_PIO_SCHMITT_MASK)
>   		seq_printf(s, "%s ", "schmitt");
>   	if (atmel_pioctrl->slew_rate_support && (conf & ATMEL_PIO_SR_MASK))

Thanks, best regards,
   Nicolas

-- 
Nicolas Ferre