pinctrl: qcom: add get_direction function

The get_direction callback function allows gpiolib to know the current direction (input vs output) for a given GPIO.

This is particularly useful on ACPI systems, where the GPIOs are configured only by firmware (typically UEFI), so the only way to know the initial values to query the hardware directly. Without this function, gpiolib thinks that all GPIOs are configured for input.

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

index f8e9e1c..c978be5 100644
@@ -422,6 +422,20 @@ static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, in
 	return 0;
 }
 
+static int msm_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
+	const struct msm_pingroup *g;
+	u32 val;
+
+	g = &pctrl->soc->groups[offset];
+
+	val = readl(pctrl->regs + g->ctl_reg);
+
+	/* 0 = output, 1 = input */
+	return val & BIT(g->oe_bit) ? 0 : 1;
+}
+
 static int msm_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
 	const struct msm_pingroup *g;

@@ -510,6 +524,7 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 static struct gpio_chip msm_gpio_template = {
 	.direction_input  = msm_gpio_direction_input,
 	.direction_output = msm_gpio_direction_output,
+	.get_direction    = msm_gpio_get_direction,
 	.get              = msm_gpio_get,
 	.set              = msm_gpio_set,
 	.request          = gpiochip_generic_request,

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Ok, I posted a v2.

Is it too late for this patch to make the 4.11 merge window, or maybe 4.11-rc2? From the perspective of our server platform, it's a bug fix.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Not too late when you put it like that :D

I've merged it for fixes.

Yours,
Linus Walleij

Actually no. The GPIOF* #defines are for the consumer side of GPIO, not the driver side. We actually just use 0/1 in drivers.

So I merged this v1 version.

Yours,
Linus Walleij

Thanks.

So I have a follow-up question.

On ACPI platforms, the kernel has no control over the muxing (aka function) of the various pins. Firmware configures the TLMM controller for all pins, and configures them for whatever functions they're supposed to be.

You can see an example of this in pinctrl-qdf2xxx.c. As you can see, each group consists of 1 pin, and there are 0 functions defined.

Function 0 is plain gpio I/O. The other functions (1-7, typically) are muxes for various devices, like UART TX, etc.

Therefore, on ACPI, the driver should never change the function of any pin. If it's set to something other than 0, then it should never touch that pin. Don't write to it, don't change the direction, and definitely don't change the function.

So would it be acceptable, for example, to change msm_gpio_set() such that if the function of that pin is non-zero, just return an error?

After all, if pin #17 is set to UART and not GPIO, then there's no point in setting that value to 1 or 0, because it's not muxed for GPIO and therefore, that 1/0 is not actually going anywhere. It won't be written to the pin.

I hope I'm making sense.

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

I think it would be better if pin control and ACPI play along, and I bet that will happen in the future. This is I guess a question for ACPI standardization work.

Does that mean that pins with 0 are free to play around with?

I would ask the driver maintainer about his opinion, and also whoever is an authority on ACPI for the TLMM-chips, I am no expert in ACPI. Hell I'm not even good at device tree. Not to mention SFI. Oh well...

In a way I guess, I'm just ignorant about how current ACPI implementations work with hardware in this case so it is likely that you know this better than me.

Yours,
Linus Walleij

Maybe. During the development of the ACPI standard, everyone made a big stink about how muxing should be handled by the firmware and never touched by the OS. It would be a significant reversal if they decide to add mux configuration to ACPI.

Yes.

Well, I was hoping that Stephen would respond to this question. :-)

My point is, if the driver knows that the GPIO cannot be written to (because it's muxed to something else), shouldn't the driver return with an error if the caller attempts to write?

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

(I reply faster when my name is written!)

I don't see any problem with failing msm_gpio_set() when the function is "not gpio", but I also wonder why it matters. Drivers shouldn't be doing that, because if the gpio is muxed to some other functionality they shouldn't be treating it as a gpio in the first place.

Perhaps we can have some sort of gpio validation debug option that the check goes under. Then we could fail and print a big warning if this happens, but if we aren't debugging then we don't do any checking and rely on drivers to do the right thing.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

The idea is to notify drivers with an error code when they make a mistake. Perhaps the device tree or the ACPI table has an error?

I could add that, but I still think returning an error code is appropriate. On the TLMM, we know for sure that the pin must be set to function 0 in order for the read/write routines to operate correctly.

I guess I should propose a patch and we can vote on it.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

In general the kernel isn't a firmware validator. At least that's the way I view it. Others may have different opinions here.

On ACPI we could make the gpio_get() path fail if the pin isn't in GPIO mode? That would work assuming ACPI can't change the pin muxing at runtime. On DT we might have runtime muxing though so I don't see how it would work there.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

I would be okay with wrapping that check around #ifdef CONFIG_DEBUG_GPIO.

Did you mean the gpio_chip.request callback? Currently that points to gpiochip_generic_request in pinctrl-msm.c.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

This will, more or less, only be useful for system integrators - whom likely won't enable DEBUG_GPIO. But I'm generally fine with failing gpio_set if we're not in function 0.

My question is if we should wrap that check in a WARN(), just to make it easy for said system integrator to spot the issue - or if that will just leave another chunk of printouts that people will ignore in their products.

It might be useful to fail gpio_get, as that gives a much nicer error path in the client. But I'm slightly concerned about the few cases where one of the pinmux states is gpio and that this would force the gpio_get() only to be called after switching to that particular mode.

@Linus, have there been any discussion around this in other drivers?

Regards,
Bjorn

Not more than the obvious, this driver has:

.request = gpiochip_generic_request,
.free = gpiochip_generic_free,

Which will call:

pinctrl_request_gpio()
pinctrl_free_gpio()

(Note: we now also have gpiochip_generic_config() and pinctrl_gpio_set_config(), you should maybe want to look into this because it makes it possible for the pin control back-end to support debouncing and open drain/source from the gpiolib side.)

Anyways in some drivers (not pinctrl-msm.c) this ends up calling this helper callback in pinmux_ops:

* @gpio_request_enable: requests and enables GPIO on a certain pin.

 *      Implement this only if you can mux every pin individually as GPIO. The
 *      affected GPIO range is passed along with an offset(pin number) into that
 *      specific GPIO range - function selectors and pin groups are orthogonal
 *      to this, the core will however make sure the pins do not collide.

If you do not have this kind of BIOS/ACPI playing around with the pins behind your back, then this is really helpful to avoid having to (in the DTS) mux all these pins to the GPIO function explicitly like this for example:

/* Interrupt line for the KXSD9 accelerometer */

dragon_kxsd9_gpios: kxsd9 {
    irq {
        pins = "gpio57"; /* IRQ line */
        bias-pull-up;
    };
};

Well, in this case, since we also need to set up a pull-up the DT node is needed anyways, but you get the idea.

So we have a bunch of fit-all callbacks but the subsystem was constructed for a scenario where the kernel has full control over the pins.

In the beg?nning ACPI people were saying they simply should not devise any pin control driver at all, because ACPI would handle all muxing behind the scenes. It turns out they were mostly too ambitious about that, one usecase is power optimizations that are not readily understood when writing the BIOS, other reasons are when you are building embedded systems and randomly adding some extra peripherals, in theory every vendor should ideally write a new ACPI table and reflash the BIOS but it appears that in practice they do not, so I think most of that hardware has some hybrid model.

I would ask the Intel people about their position of ACPI and pin control interwork, they have most experience in this. If you git-blame drivers/gpio/gpiolib-acpi.c you can see that it is mostly written by Mika and Rafael.

If you look in intel_pinmux_set_mux() you find that they do avoid muxing some pins:

        /*
         * All pins in the groups needs to be accessible and writable
         * before we can enable the mux for this group.
         */
        for (i = 0; i < grp->npins; i++) {
                if (!intel_pad_usable(pctrl, grp->pins[i])) {
                        raw_spin_unlock_irqrestore(&pctrl->lock, flags);
                        return -EBUSY;
                }
        }

Yours,
Linus Walleij