Hi Linus,
On Fri, Aug 19, 2011 at 04:04:54PM +0200, Linus Walleij wrote:
On Fri, Aug 19, 2011 at 12:48 PM, Jamie Iles jamie@jamieiles.com wrote:
On Fri, Aug 19, 2011 at 11:53:50AM +0200, Linus Walleij wrote:
+Interaction with the GPIO subsystem +===================================
+The GPIO drivers may want to perform operations of various types on the same +physical pins that are also registered as GPIO pins.
+Since the pin controller subsystem have its pinspace local to the pin +controller we need a mapping so that the pin control subsystem can figure out +which pin controller handles control of a certain GPIO pin. This member +in the pin controller descriptor handles this mapping:
+static struct pinctrl_desc foo_desc = {
- ...
- .gpio_base = FIRST_PIN,
+};
+When GPIO-specific functions in the pin control subsystem are called, these +mappings will be used to look up the apropriate pin controller by inspecting +and matching the pin to this pin range.
On our (difficultly muxed!) platform we have two types of GPIO - a Synopsys controller which is a fairly conventional GPIO controller, then a sigma-delta GPIO controller which can also do a an analogue type output (as well as digital).
Does that mean it is really not a GPIO controller but a kind of D/A converter?
Kind of. In the basic mode it's just a GPIO controller that does digital I/O. In the SD mode it all really depends on what the external filter looks like. As gpio_set_value() takes an int as the value, then the gpio controller theoretically _could_ treat that as an analogue output value and use the pinctrl api to set the converter and rate sizes but I don't really want to go there yet as it's a bit of an abuse of the gpio API!
For lots of our pads they can either be ARM GPIO, SD GPIO or some other function, so I don't see how this fits in with a single GPIO base.
And each of them are modeled as a separate gpio_chip I guess?
Otherwise it's a bad match with reality. We had this discussion with GRant where two gpio_chips would use the same number range in the GPIO global pinspace, and it's basically not allowed IIRC.
Yes, the SD-GPIO isn't memory mapped so has a completely separte gpio_chip.
But yes, there is an assumption that each pin controller will only deal with one block of GPIO pins. So if I make it possible to support several GPIO ranges for one pin controller, does that solve your problem?
Like this:
struct pinctrl_gpio_range { char *name; unsigned int base; unsigned int npins; }
static unsigned int gpio_ranges[] = { { .name="chip1", .base = 0, .npins = 16, }, { .name =" chip2", .base = 32, .npins = 16, }, { .name = "chip3", .base = 64, .npins = 16, }, };
static struct pinctrl_desc foo_desc = { ... .gpio_ranges = gpio_ranges, .num_gpio_ranges = ARRAY_SIZE(gpio_ranges), };
For three different 32-bit GPIO controllers muxed on pins 0..31 using GPIO space pins from 0..95.
Then I pass the number of the instance down to the driver in the gpio_request_enable() callback like this:
int (*gpio_request_enable) (struct pinctrl_dev *pctldev, unsigned instance, unsigned offset);
Would this work?
This has a restriction: the GPIO space must be mapped in continous ranges, as must the pin controller. Else we need one entry per pin in the list above...
OK, that looks perfect!
+The correspondence for the range from the GPIO subsystem to the pin controller +subsystem must be one-to-one. Thus the GPIO pins are in the pin controller +range [0 .. maxpin] where maxpin is the specified end of the pin range.
So doesn't this mean that the enumeration that was initially described as arbitrary actually has to enumerate the GPIO pins first?
If you use GPIO accessors, the enumeration has to match. So I rewrite it like this:
"this enumeration was arbitrarily chosen, in practice you need to think through your numbering system so that it matches the layout of registers and such things in your driver, or the code may become complicated. You must also consider matching of offsets to the GPIO ranges that may be handled by the pin controller."
OK?
Sounds good.
+static struct class pinctrl_class = {
- .name = "pinctrl",
- .dev_release = pinctrl_dev_release,
- .dev_attrs = pinctrl_dev_attrs,
+};
Greg K-H has mentioned in the past that class is now deprecated for new use and that a bus_type should be used instead.
Can you provide a reference with some detail? The pin control devices are usually aleady on a bus like the platform_bus or amba_bus or i2c_bus, then they register a class device in this case.
The kerneldoc documentation says "A bus is a channel between the processor and one or more devices."
This isn't the case here.
Anyhthing that help me understand this is appreciated, Arnd?
There's a thread here https://lkml.org/lkml/2011/3/25/443 where this has been discussed (I originally had a class too).
+/**
- struct pinctrl_desc - pin controller descriptor, register this to pin
- control subsystem
- @name: name for the pin controller
- @pins: an array of pin descriptors describing all the pins handled by
- this pin controller
- @npins: number of descriptors in the array, usually just ARRAY_SIZE()
- of the pins field above
- @maxpin: since pin spaces may be sparse, there can he "holes" in the
- pin range, this attribute gives the maximum pin number in the
- total range. This should not be lower than npins for example,
- but may be equal to npins if you have no holes in the pin range.
- @pmxops: pinmux operation vtable, if you support pinmuxing in your driver
- @gpio_base: the base offset of the pin range in the GPIO subsystem that
- is handled by this controller, if applicable. This member is only
- relevant if you want to e.g. control pins from the GPIO subsystem.
- @gpio_pins: the number of pins from (and including) the gpio_base offset
- handled by this pin controller.
- @owner: module providing the pin controller, used for refcounting
- */
+struct pinctrl_desc {
- const char *name;
- struct pinctrl_pin_desc const *pins;
- unsigned int npins;
- unsigned int maxpin;
- struct pinmux_ops *pmxops;
- unsigned int gpio_base;
- unsigned int gpio_pins;
- struct module *owner;
Would it be better to put the owner in the ops structure like file_ops? I'm sure I remember someone saying that it's better to do that so that it's in the modules .data/.rodata section but I can't find that reference.
I can't do that because the pinmux_ops vtable is not mandatory.
The plan is to add more ops for other things like pin bias etc.
OK, I'd missed that.
+/**
- struct pinctrl_dev - pin control class device
- @desc: the pin controller descriptor supplied when initializing this pin
- controller
- @node: node to include this pin controller in the global pin controller list
- @dev: the device entry for this pin controller
- @owner: module providing the pin controller, used for refcounting
- @driver_data: driver data for drivers registering to the pin controller
- subsystem
- This should be dereferenced and used by the pin controller core ONLY
- */
+struct pinctrl_dev {
- struct pinctrl_desc *desc;
- struct radix_tree_root pin_desc_tree;
- spinlock_t pin_desc_tree_lock;
- struct list_head node;
- struct device dev;
- struct module *owner;
- void *driver_data;
Couldn't the struct device driver_data be used here?
I'm already using dev_set_drvdata(&pctldev->dev, pctldev); So I can retrieve the pctldev withdev_get_drvdata(dev); in sysfs...
If I wasn't registering sysfs nodes I couldv'e used it.
OK, fair enough.
+/* These should only be used from drives */
s/drives/drivers?
OK.
+/**
- struct pinmux_ops - pinmux operations, to be implemented by pin controller
- drivers that support pinmuxing
- @request: called by the core to see if a certain pin can be made available
- available for muxing. This is called by the core to acquire the pins
- before selecting any actual mux setting across a function. The driver
- is allowed to answer "no" by returning a negative error code
- @free: the reverse function of the request() callback, frees a pin after
- being requested
So is the request like gpio_request() or just test if it the pin is available? If its the latter then perhaps pin_is_available() might be a better name?
It's the former. The pin controller may have some electrical properties that makes it impossible to request a certain pin for muxing at a certain time. (Found out on earlier list review.)
OK, it's a bit of a language nitpick, but I interpreted that as meaning it was purely a test.
@request: called by the core to reserve a pin for muxing. This is called by the core to acquire the pins before selecting any actual mux setting across a function. The driver is allowed to answer "no" by returning a negative error code.
maybe makes things a little clearer (to me anyway!)?
+#else /* !CONFIG_PINMUX */
+static inline int pinmux_request_gpio(unsigned gpio) +{
- return 0;
+}
+static inline void pinmux_free_gpio(unsigned gpio) +{ +}
+static inline struct pinmux *pinmux_get(struct device *dev, const char *func) +{
- return NULL;
+}
The CONFIG_PINMUX=y version of pinmux_get returns an ERR_PTR() encoded error so perhaps this should return something like ERR_PTR(-ENXIO)?
No this is modeled more like the regulator stubs in <linux/regulator.h>. For example, if you're compiling for a platform that does not support regulators they are assumed to be always on.
In this case, if compiled for a platform without pinmux, we assume we got the pins we need already so it just works, not fail.
That makes perfect sense! Doh!
Thanks a lot for doing this work Linus, it's really appreciated and looking great! It's a bit difficult for me to test this on my platform at the moment as I'm still trying to handle all of the device tree stuff but I'll give this a good test as soon as I can.
Jamie