linux-kernel - Re: [PATCH 2/2 v7] pinmux: add a driver for the U300 pinmux

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CACRpkdameaz=Ys1A5XAxTN=+zdKQhpkJao57BbYc38uF5wWVHw@mail.gmail.com>
Date:	Fri, 30 Sep 2011 15:37:01 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Grant Likely <grant.likely@...retlab.ca>
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>
Subject: Re: [PATCH 2/2 v7] pinmux: add a driver for the U300 pinmux

On Fri, Sep 30, 2011 at 4:12 AM, Grant Likely <grant.likely@...retlab.ca> wrote:

>>  config MACH_U300
>>       bool "U300"
>> +     select PINCTRL
>> +     select PINMUX_U300
>
> Shouldn't PINMUX_U300 select PINCTRL?

It selects PINMUX which is inside the if-clause for the
subsystem:

if PINCTRL

config PINMUX
	bool "Support pinmux controllers"
(...)

config PINMUX_U300
	bool "U300 pinmux driver"
	depends on ARCH_U300
	select PINMUX
(...)

endif

Having PINMUX or PINMUX_U300 select PINCTRL doesn'
work because it is deemed a circular dependency (indeed).
And Kconfig apparently does not resolve dependencies like
this, so it says:

# CONFIG_PINCTRL is not set
CONFIG_PINMUX_U300=y

And:

warning: (MACH_U300) selects PINMUX_U300 which has unmet direct
dependencies (PINCTRL && ARCH_U300)
warning: (MACH_U300) selects PINMUX_U300 which has unmet direct
dependencies (PINCTRL && ARCH_U300)

And then it breaks in the compile.

Actually the same thing seems to go for say this:

select ARCH_REQUIRE_GPIOLIB
select GPIO_FOO

You have to select both from your machine to get that driver,
just selecting GPIO_FOO is unable to auto-select GPIOLIB.

If you like this design pattern I can introduce

ARCH_REQUIRE_PINCTRL

In the same style as GPIOLIB, but it looks a bit
superfluous to me, select PINCTRL should be
fine?

>> +#include "pinmux-u300.h"
>
> There is only one file that uses this header data.  Just put it into
> the .c file.

OK!

>> +static int __init u300_pmx_probe(struct platform_device *pdev)
>> +{
>> +     int ret;
>> +     struct u300_pmx *upmx;
>> +     struct resource *res;
>> +
>> +     /* Create state holders etc for this driver */
>> +     upmx = kzalloc(sizeof(struct u300_pmx), GFP_KERNEL);
>
> devm_kzalloc()

OK, replaced kfree() with devm_kfree() too for consistency.

>> +static int __exit u300_pmx_remove(struct platform_device *pdev)
> __devexit
(...)
>> +     .remove = __exit_p(u300_pmx_remove),
> __devexit_p

But notice, no .probe member and:

>> static int __init u300_pmx_init(void)
>> {
>>        return platform_driver_probe(&u300_pmx_driver, u300_pmx_probe);
>> }

See drivers/base/platform.c:platform_driver_probe():

/**
 * platform_driver_probe - register driver for non-hotpluggable device
 * @drv: platform driver structure
 * @probe: the driver probe routine, probably from an __init section
 *
 * Use this instead of platform_driver_register() when you know the device
 * is not hotpluggable and has already been registered, and you want to
 * remove its run-once probe() infrastructure from memory after the driver
 * has bound to the device.
 *
 * One typical use for this would be with drivers for controllers integrated
 * into system-on-chip processors, where the controller devices have been
 * configured as part of board setup.

This driver won't ever load as a module, and new ones will never be
discovered after boot.

So I kind of like it that way...

Yours,
Linus Walleij
--
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/