No subject

+	/*
+	 * If there is no kind of request function for the pin we just assume
+	 * we got it by default and proceed.
+	 */
+	if (gpio && ops->gpio_request_enable)
+		/* This requests and enables a single GPIO pin */
+		status =3D ops->gpio_request_enable(pmxdev,
+						  pin - pmxdev->desc->base);
+	else if (ops->request)
+		status =3D ops->request(pmxdev,
+				      pin - pmxdev->desc->base);
+	else
+		status =3D 0;

a) I feel the default should be to error out, not succeed; the whole
point of the API is for HW where something must be programmed to enable
each function. As such, if you don't provide that ops->request* function,
that seems like an error.

I think the logic above should be to always call ops->gpio_request_enable
if (gpio):

	if (gpio)
		if (ops->gpio_request_enable)
			/* This requests and enables a single GPIO pin */
			status =3D ops->gpio_request_enable(pmxdev,
						  pin - pmxdev->desc->base);
	else
		status =3D ops->request(pmxdev,
				      pin - pmxdev->desc->base);

(ops->gpio_request_enable could be optional if GPIOs aren't supported on
any pinmux pins, whereas ops->request isn't optional, and should be checked
during pinmux_register)

Also, ops->gpio_request_enable should also be passed the GPIO number,
so the driver can validate that it's the correct one. Often, only
one value would be legal, but perhaps some pinmuxs allow 1 of N different
GPIOs to be mapped to some pin, so selection of which GPIO is on par with
selection of which special function to mux out, yet the driver still
doesn't want to expose every GPIO possibility as a pinmux "function" due
to explosion of the number of functions if it did that.

b) When ops->gpio_request_enable is called to request a pin, it'd be nice
if the core could track this, and specifically call a new ops->gpio_disable
to free it, rather than the generic ops->free. There are two cases in HW:

b1) "GPIO" is just another special function in a list, e.g. SPI, I2C, GPIO.
In this case, free for a GPIO needs to either do nothing (the next call to
request will simply set the desired function), or possibly do something to
disable the pin in the same way as any other function. This works fine with
a single free function.

b2) GPIO enable/disable is a separate concept from the pinmuxing; on Tegra
it'll be implemented by calling tegra_gpio_enable/disable. As such, a
single free function would require the pinmux driver to store state
indicating whether ops->free should call tegra_gpio_disable, or whether it
should do cleanup to the pinmux registers. Since the core is managing GPIO
vs. function allocation, I think the storing that state in the core makes
sense.

Thanks for reading. Hopefully this email didn't jump about too much.

--=20
nvpublic