[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 at pengutronix.de> 
> Sent: Wednesday, August 18, 2021 2:22 PM
> To: Piyush Mehta <piyushm at xilinx.com>; arnd at arndb.de; zou_wei at huawei.com; gregkh at linuxfoundation.org; linus.walleij at linaro.org; Michal Simek <michals at xilinx.com>; Jiaying Liang <jliang at xilinx.com>; iwamatsu at nigauri.org; bgolaszewski at baylibre.com; robh+dt at kernel.org; Rajan Vaja <RAJANV at xilinx.com>
> Cc: linux-gpio at vger.kernel.org; devicetree at vger.kernel.org; git <git at xilinx.com>; Srinivas Goud <sgoud at xilinx.com>; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; Pengutronix Kernel Team <kernel at pengutronix.de>
> 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 at xilinx.com>
>> Acked-by: Michal Simek <michal.simek at xilinx.com>
>> Reviewed-by: Linus Walleij <linus.walleij at linaro.org>
>> ---
> 
>> +/**
>> + * 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 |