On Fri, Sep 30, 2011 at 9:05 AM, Linus Walleij linus.walleij@linaro.org wrote:
On Fri, Sep 30, 2011 at 4:07 AM, Grant Likely grant.likely@secretlab.ca wrote:
Comments below, some a bit nitpicky, but overall I think it looks good. I haven't dug into it nearly deeply enough though. :-(
Hopefully we can patch the remaining bugs as we go along :-)
+/**
- Looks up a pin control device matching a certain device name or
- pure device pointer.
May as well actually do kerneldoc formatting here on the comment blocks.
OK.
+struct pinctrl_dev *get_pctldev_from_dev(struct device *dev,
- const char *devname)
Nit: I'm not too fond of a single function doing both name and pointer lookup at the same time. Presumably the caller would have one or the other and know what it needs to do. I'd prefer to see one by-name function and one by-reference. I'm not going to make a big deal about it though.
The caller currently does not know what it has or what to do.
This is basically an interator function that is called on a member tuple of device and device name to check which one you have and return a matching controller device for the key you do have.
- /* Register device with sysfs */
- pctldev->dev.parent = dev;
- pctldev->dev.bus = &pinctrl_bus;
I don't think there is an actual need for a pinctrl bus type. There aren't any drivers that are going to be bound to these things which is the primary functionality that a bus type provides. Am I missing something?
That is not the reason it's there actually, what we have discussed on the mailing list is getting sysfs entries for the same reason gpiolib registers a class: handle pin control from userspace, we can already see that coming and I already have a use case for it. (Modem SIM connector control from userspace daemon.)
So first it was registering a class, then Greg said classes are deprecated and we should use a bus instead. So it is registering a bus to get sysfs so we can get userspace controls.
Sure, but you don't need the bus yet. It can be added when it is actually needed. I'm not convinced that the sysfs approach is actually the right interface here (I'm certainly not a fan of the gpio sysfs i/f), and I'd rather not be putting in unneeded stuff until the userspace i/f is hammered out.
g.