linux-kernel - Re: [PATCH] drivers: create a pin control subsystem v8

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 30 Sep 2011 11:07:56 -0600
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Linus Walleij <linus.walleij@...ricsson.com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Stephen Warren <swarren@...dia.com>,
	Barry Song <21cnbao@...il.com>,
	Lee Jones <lee.jones@...aro.org>,
	Joe Perches <joe@...ches.com>,
	Russell King <linux@....linux.org.uk>,
	Linaro Dev <linaro-dev@...ts.linaro.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	David Brown <davidb@...eaurora.org>,
	Stijn Devriendt <highguy@...il.com>
Subject: Re: [PATCH] drivers: create a pin control subsystem v8

On Fri, Sep 30, 2011 at 9:05 AM, Linus Walleij <linus.walleij@...aro.org> wrote:
> On Fri, Sep 30, 2011 at 4:07 AM, Grant Likely <grant.likely@...retlab.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/