Linus Walleij wrote at Monday, June 13, 2011 10:58 AM:
This creates a subsystem for handling of pinmux devices. These are devices that enable and disable groups of pins on primarily PGA and BGA type of chip packages and common in embedded systems.
I'm a little confused by this version. In particular:
* I don't see some changes that I thought we'd agreed upon during earlier review rounds, such as:
** Removing pinmux_ops members list_functions, get_function_name, get_function_pins; it seems odd not to push this into the pinmux core as data, but instead have the core pull it out of the driver in a "polling" function.
** Similarly, I'd asked for at least some documentation about how to handle the "multi-width bus" problem, but I didn't see that.
* I'm confused by the new data model even more than before:
** How can the board/machine name pins? That should be a function of the chip (i.e. pinmux driver), since that's where the pins are located. A chip's pins don't have different names on different boards; the names of the signals on the PCB connected to those pins are defined by the board, but not the names of the actual pins themselves.
** struct pinmux_map requires that each function name be globally unique, since one can only specify "function" not "function on a specific chip". This can't be a requirement; what if there are two instances of the same chip on the board (think some expansion chip attached to a memory-mapped bus rather than the primary SoC itself).
** Perhaps primarily due to the lack of consideration in the documentation, I'm not convinced we have a clearly defined path to solve the "multi-width bus" issue. This needs to be a feature of the core pinmux API, rather than something that's deferred to the board/machine files setting up different function mappings, since it's not possible to solve purely in function mappins as they're defined today.
Some minor mainly typo, patch-separation, etc. feedback below.
...
+Pinmux, also known as padmux, ballmux, alternate functions or mission modes +is a way for chip vendors producing some kind of electrical packages to use +a certain physical bin (ball, pad, finger, etc) for multiple mutually exclusive
s/bin/pin/
...
+A simple driver for the above example will work by setting bits 0, 1 or 2 +into some register mamed MUX, so it enumerates its available settings and
s/mamed/named
+++ b/drivers/pinctrl/Kconfig
...
+config PINMUX_U300
- bool "U300 pinmux driver"
- depends on ARCH_U300
- help
Say Y here to enable the U300 pinmux driver+endif
Shouldn't that portion be part of the second patch?
+++ b/drivers/pinctrl/Makefile
...
+obj-$(CONFIG_PINMUX_U300) += pinmux-u300.o
Same here.
+++ b/include/linux/pinctrl/machine.h +/**
- struct pinmux_map - boards/machines shall provide this map for devices
- @function: a functional name for this mapping so it can be passed down
- to the driver to invoke that function and be referenced by this ID
- in e.g. pinmux_get()
- @dev: the device using this specific mapping, may be NULL if you provide
- .dev_name instead (this is more common)
- @dev_name: the name of the device using this specific mapping, the name
- must be the same that will return your struct device*
s/that will return/as in/ ?
+++ b/include/linux/pinctrl/pinmux.h +struct pinmux;
+#ifdef CONFIG_PINCTRL
+struct pinmux_dev;
I think "struct pinmux_map" is needed outside that ifdef, or an include of <pinctrl/machine.h>.
+/**
- struct pinmux_ops - pinmux operations, to be implemented by drivers
- @request: called by the core to see if a certain pin can be muxed in
- and made available in a certain mux setting The driver is allowed
- to answer "no" by returning a negative error code
That says "and made available in a certain mux setting", but no mux setting is passed in. s/a certain/the current/?
Documentation for @free is missing here
+/**
- struct pinmux_desc - pinmux descriptor, register this to pinmux subsystem
- @name: name for the pinmux
- @ops: pinmux operation table
- @owner: module providing the pinmux, used for refcounting
- @base: the number of the first pin handled by this pinmux, in the global
- pin space, subtracted from a given pin to get the offset into the range
- of a certain pinmux
- @npins: the number of pins handled by this pinmux - note that
- this is the number of possible pin settings, if your driver handles
- 8 pins that each can be muxed in 3 different ways, you reserve 24
- pins in the global pin space and set this to 24
That's not correct, right; if you have 8 pins, you just say 8 here?
The multiplier for N functions per pin is through list_functions, get_function_name, get_function_pins as I understand it.
+static inline int pinmux_register_mappings(struct pinmux_map const *map,
unsigned num_maps)+{
- return 0;
+}
I think you moved that to <pinmux/machine.h>?
-- nvpublic