Re: [PATCH V3 3/3] gpio: modepin: Add driver support for modepin GPIO controller

[Ahmad Fatoum]

Hello Piyush,

On 18.08.21 12:09, Piyush Mehta wrote:
> Hi Ahmad,
> 
> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx> 
> Sent: Wednesday, August 18, 2021 2:22 PM
> To: Piyush Mehta <piyushm@xxxxxxxxxx>; arnd@xxxxxxxx; zou_wei@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linus.walleij@xxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>; Jiaying Liang <jliang@xxxxxxxxxx>; iwamatsu@xxxxxxxxxxx; bgolaszewski@xxxxxxxxxxxx; robh+dt@xxxxxxxxxx; Rajan Vaja <RAJANV@xxxxxxxxxx>
> Cc: linux-gpio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; git <git@xxxxxxxxxx>; Srinivas Goud <sgoud@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH V3 3/3] gpio: modepin: Add driver support for modepin GPIO controller
> 
> On 18.08.21 10:10, Piyush Mehta wrote:
>> This patch adds driver support for the zynqmp modepin GPIO controller.
>> GPIO modepin driver set and get the value and status of the PS_MODE 
>> pin, based on device-tree pin configuration. These four mode pins are 
>> configurable as input/output. The mode pin has a control register, 
>> which have lower four-bits [0:3] are configurable as input/output, 
>> next four-bits can be used for reading the data  as input[4:7], and 
>> next setting the output pin state output[8:11].
>>
>> Signed-off-by: Piyush Mehta <piyush.mehta@xxxxxxxxxx>
>> Acked-by: Michal Simek <michal.simek@xxxxxxxxxx>
>> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>> ---
> 
>> +/**
>> + * modepin_gpio_dir_in - Set the direction of the specified GPIO pin as input
>> + * @chip:	gpio_chip instance to be worked on
>> + * @pin:	gpio pin number within the device
>> + *
>> + * Return: 0 always
>> + */
>> +static int modepin_gpio_dir_in(struct gpio_chip *chip, unsigned int 
>> +pin) {
>> +	return 0;
>> +}
> 
> You say the gpio controller can configure pins as inputs or outputs.
> These pins are controller via firmware driver. We are updating BOOT_PIN_CTRL 	0xFF5E0250 register.
> [0:3]  = When 0, the pins will be inputs from the board to the PS. When 1, the PS will drive these pins

Ok. So if you want to configure the pin as input, you should call zynqmp_pm_bootmode_write
to write a zero into that bit.

But there's only one zynqmp_pm_bootmode_write in the GPIO driver and it's in modepin_gpio_set_value,
which does output, not input. If I understand you right, there should be a modepin_gpio_set_value in
modepin_gpio_dir_in as well?
 
> Yet, .direction_input is doing nothing. So, it's not clear to me, how this sequence could work:
> 
>  - set gpio output high (writes bootmode)
>  - set gpio to input (no-op, pin will remain high, not high impedance)

This is a valid sequence for a GPIO consumer and I don't see how this GPIO driver could
honor it. Could you clarify?

Cheers,
Ahmad

> 
> 
> 
> 
> 
> 
> I didn't check the previous discussions, but if this indeed works as intended, the how should be written here into the driver. That is a more useful comment than kernel doc for a stub function.
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |