Re: [PATCH v2 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

["Dan O'Donovan" <dan.odonovan@xxxxxxxxxx>]

Hi Linus

Many thanks for your feedback below, and sorry that I'm only getting to follow up on this now.  I've added some initial responses inline below.

On 10/22/18 10:07 AM, Linus Walleij wrote:
> On Fri, Oct 19, 2018 at 7:16 PM Dan O'Donovan <dan@xxxxxxxxxx> wrote:
>
>> From: Javier Arteaga <javier@xxxxxxxxxx>
>>
>> The UP2 board features a Raspberry Pi compatible pin header (HAT) and a
>> board-specific expansion connector (EXHAT).
> Which makes me want to have Eric Anholt's review on this patch
> so as to secure basic interoperability with that header.
Sure.  I've added Eric to the CC list now.
>
>> Both expose assorted
>> functions from either the SoC (such as GPIO, I2C, SPI, UART...) or other
>> on-board devices (ADC, FPGA IP blocks...).
> OK
> Look at how RPi define names for their GPIO lines in the
> DTS file:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/bcm2835-rpi-b.dts
>
> Please follow this pattern with your patch.
>
> As you probably do not have device tree or anything similar
> for ACPI to name the lines (correct me if I'm wrong)
> you can use the .names array in struct gpio_chip for
> hardcoding the proper line names.
>
> lsgpio should give the same line names as it does on
> the corresponding RPi header IMO.
The UP board emulates the same basic pin layout as the RPi and, where possible, provides similar functions (e.g. I2C, SPI) in addition to GPIO, but it is not 100% compatible.  As such, some of the RPi pin names (e.g. SD_DA*) would not normally make sense for the UP board and don't match the names used on the UP board datasheet.  However, if its recommended to adhere to the RPi names anyway to aid application portability, I can certainly look at adding that.
>
>> +config PINCTRL_UPBOARD
>> +       tristate "UP Squared pinctrl and GPIO driver"
>> +       depends on ACPI
>> +       depends on MFD_UPBOARD
>> +       select PINMUX
> select GPIOLIB
>
> as you're using it.
Will do
>
>> +static int upboard_get_functions_count(struct pinctrl_dev *pctldev)
>> +{
>> +       return 0;
>> +}
>> +
>> +static int upboard_get_function_groups(struct pinctrl_dev *pctldev,
>> +                                      unsigned int selector,
>> +                                      const char * const **groups,
>> +                                      unsigned int *num_groups)
>> +{
>> +       *groups = NULL;
>> +       *num_groups = 0;
>> +       return 0;
>> +}
>> +
>> +static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
>> +                                            unsigned int selector)
>> +{
>> +       return NULL;
>> +}
>> +
>> +static int upboard_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
>> +                          unsigned int group)
>> +{
>> +       return 0;
>> +}
> This just looks weird.
>
> There seems to be code to disable pins and turn them into
> GPIOs in upboard_gpio_request_enable() but no way to
> switch them back to the original function, is that how it works?
>
> I guess it is fine if that is how it's supposed to be used. But
> won't some grumpy users come around and complain about
> this one day?
>
> We can fix it when it happens though.
That is correct.  The BIOS may configure a default pin function and direction (e.g. SPI MOSI) at boot, and this driver aims to allow the user to subsequently switch the pin to GPIO mode and change its direction/state at run-time.  I think I need to explain more about the hardware architecture, so its best to skip down to see my response to your comment at the end below.
>
>> +static int upboard_gpio_request(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +       struct upboard_pinctrl *pctrl =
>> +               container_of(gc, struct upboard_pinctrl, chip);
>> +       struct gpio_desc *desc;
>> +       int ret;
>> +
>> +       ret = pinctrl_gpio_request(gc->base + offset);
>> +       if (ret)
>> +               return ret;
>> +
>> +       desc = devm_gpiod_get_index(gc->parent, "external", offset, GPIOD_ASIS);
>> +       if (IS_ERR(desc))
>> +               return PTR_ERR(desc);
> No please don't do this. The consumers should request
> the gpio, not the driver.
>
>> +static void upboard_gpio_free(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +       struct upboard_pinctrl *pctrl =
>> +               container_of(gc, struct upboard_pinctrl, chip);
>> +
>> +       if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset])
>> +               return;
>> +
>> +       devm_gpiod_put(gc->parent, pctrl->soc_gpios[offset]);
> Dito.
>
>> +static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +       struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
>> +
>> +       if (IS_ERR(desc))
>> +               return PTR_ERR(desc);
>> +
>> +       return gpiod_get_direction(desc);
>> +}
> This is just confusing me even more...
>
> If you need pinctrl_gpio_get_direction() then it should be
> added to the API in <linux/pinctrl/consumer.h>.
>
>> +static int upboard_gpio_direction_output(struct gpio_chip *gc,
>> +                                        unsigned int offset, int value)
>> +{
>> +       struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
>> +       int ret;
>> +
>> +       if (IS_ERR(desc))
>> +               return PTR_ERR(desc);
>> +
>> +       ret = pinctrl_gpio_direction_output(gc->base + offset);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return gpiod_direction_output(desc, value);
> No this looks confusing too.
>
>> +static int upboard_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +       struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc, offset);
>> +
>> +       if (IS_ERR(desc))
>> +               return PTR_ERR(desc);
>> +
>> +       return gpiod_get_value(desc);
> I don't get this masking one GPIO chip behind another GPIO chip.
> It looks really weird.
>
> What we usually have is a GPIO chip in front of a pin controller
> utilizing
> extern int pinctrl_gpio_request(unsigned gpio);
> extern void pinctrl_gpio_free(unsigned gpio);
> extern int pinctrl_gpio_direction_input(unsigned gpio);
> extern int pinctrl_gpio_direction_output(unsigned gpio);
> extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long config);
>
> these things for the GPIO chip to talk to the pin control
> back-end.
>
> This driver seems to use a GPIO chip in front of a
> GPIO chip and a pin controller too (or something like
> that) and that makes me very uneasy.
>
> I need a clear picture of the internal architectur of
> the GPIO parts of this driver, why the GPIO accessors
> are calling back into the GPIO layer etc. It looks very
> unorthodox to me, and I get very confused.
>
> Yours.
> Linus Walleij

You're right, the architecture here is certainly unconventional.  I'll start by clarifying some hardware design details.

If the GPIO signals from the Apollo Lake SoC on this UP Squared board were connected directly to the external RPi-like pin header, this driver wouldn't exist.  But the SoC signals are 1.8V and the pin header needs to support 3.3V, so an FPGA has been used to provide voltage level shifting and buffering between the SoC and the pin header.  The correct input/output direction for each I/O signal needs to be configured on the FPGA, and some of the I/O signals need to be switched depending on the function required.  So it can be said that the FPGA is providing some pin control functionality outside of the SoC, and needs to be configured in tandem with the pins on the SoC itself.  E.g. if a GPIO pin on the SoC is switched from input to output mode, this configuration needs to be echoed on the FPGA as well.

        +--------------+    +--------+    +-------------------------------+
        |  SoC (1.8V)  |----|  FPGA  |----| 40-pin RPi-like header (3.3V) |
        +--------------+    +--------+    +-------------------------------+

(The FPGA provides some other functions as well (e.g. LED control) but this particular driver is focused only on the GPIO pin control aspect)

On one hand, it could be viewed as a second-level pin controller to be synchronised with the first-level GPIO pin controller on the SoC.

On the other hand, it could be viewed as a separate external GPIO controller.  But unlike, say a typical GPIO controller connected via I2C or SPI where the state of each of its external I/O signals is controlled by a bit in a register, the external I/O signal states here are each controlled using dedicated GPIO signals on the SoC.

After some internal debate, the 2nd approach above was chosen as it seemed to fit more easily within the existing gpio/pinctrl driver framework in Linux.  So this is why this is presented as a GPIO chip driver that is providing 28 "external" GPIO signals and, in turn, is consuming a set of 28 "internal" GPIO resources on the SoC.  Each GPIO on this external GPIO controller is connected to a dedicated SoC GPIO.  So, when a user the mode/direction of a pin on the external header, this driver will ensure that the FPGA and the internal SoC gpio and pinctrl elements are all reconfigured together.

I'd be delighted to get your thoughts on how you would prefer to see this implemented.