[PATCH 2/2 v7] pinmux: add a driver for the U300 pinmux

[Linus Walleij]

On Fri, Sep 30, 2011 at 4:12 AM, Grant Likely <grant.likely at secretlab.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