Re: [PATCH 3/3] omap: add hwspinlock device

[Grant Likely]

On Tue, Oct 19, 2010 at 11:03 AM, Kevin Hilman
<khilman@xxxxxxxxxxxxxxxxxxx> wrote:
> Ohad Ben-Cohen <ohad@xxxxxxxxxx> writes:
>
>> From: Simon Que <sque@xxxxxx>
>>
>> Build and register an hwspinlock platform device.
>>
>> Although only OMAP4 supports the hardware spinlock module (for now),
>> it is still safe to run this initcall on all omaps, because hwmod lookup
>> will simply fail on hwspinlock-less platforms.
>>
>> Signed-off-by: Simon Que <sque@xxxxxx>
>> Signed-off-by: Hari Kanigeri <h-kanigeri2@xxxxxx>
>> Signed-off-by: Ohad Ben-Cohen <ohad@xxxxxxxxxx>
>> Cc: Benoit Cousson <b-cousson@xxxxxx>
>> ---
>>  arch/arm/mach-omap2/Makefile     |    1 +
>>  arch/arm/mach-omap2/hwspinlock.c |   67 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 68 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/mach-omap2/hwspinlock.c
>>
>> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
>> index 7352412..e55d1c5 100644
>> --- a/arch/arm/mach-omap2/Makefile
>> +++ b/arch/arm/mach-omap2/Makefile
>> @@ -190,3 +190,4 @@ obj-y                                     += $(smc91x-m) $(smc91x-y)
>>
>>  smsc911x-$(CONFIG_SMSC911X)          := gpmc-smsc911x.o
>>  obj-y                                        += $(smsc911x-m) $(smsc911x-y)
>> +obj-$(CONFIG_ARCH_OMAP4)             += hwspinlock.o
>> diff --git a/arch/arm/mach-omap2/hwspinlock.c b/arch/arm/mach-omap2/hwspinlock.c
>> new file mode 100644
>> index 0000000..641a6d4
>> --- /dev/null
>> +++ b/arch/arm/mach-omap2/hwspinlock.c
>> @@ -0,0 +1,67 @@
>> +/*
>> + * OMAP hardware spinlock device initialization
>> + *
>> + * Copyright (C) 2010 Texas Instruments. All rights reserved.
>> + *
>> + * Contact: Simon Que <sque@xxxxxx>
>> + *          Hari Kanigeri <h-kanigeri2@xxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
>> + * 02110-1301 USA
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/err.h>
>> +
>> +#include <plat/omap_hwmod.h>
>> +#include <plat/omap_device.h>
>> +
>> +struct omap_device_pm_latency omap_spinlock_latency[] = {
>> +     {
>> +             .deactivate_func = omap_device_idle_hwmods,
>> +             .activate_func   = omap_device_enable_hwmods,
>> +             .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
>> +     }
>> +};
>> +
>> +int __init hwspinlocks_init(void)
>> +{
>> +     int retval = 0;
>> +     struct omap_hwmod *oh;
>> +     struct omap_device *od;
>> +     const char *oh_name = "spinlock";
>> +     const char *dev_name = "omap_hwspinlock";
>> +
>> +     /*
>> +      * Hwmod lookup will fail in case our platform doesn't support the
>> +      * hardware spinlock module, so it is safe to run this initcall
>> +      * on all omaps
>> +      */
>> +     oh = omap_hwmod_lookup(oh_name);
>> +     if (oh == NULL)
>> +             return -EINVAL;
>> +
>> +     od = omap_device_build(dev_name, 0, oh, NULL, 0,
>> +                             omap_spinlock_latency,
>> +                             ARRAY_SIZE(omap_spinlock_latency), false);
>> +     if (IS_ERR(od)) {
>> +             pr_err("Can't build omap_device for %s:%s\n", dev_name,
>> +                                                             oh_name);
>> +             retval = PTR_ERR(od);
>> +     }
>> +
>> +     return retval;
>> +}
>> +postcore_initcall(hwspinlocks_init);
>
> Any reason this needs to be a postcore_initcall?  Are there users of
> hwspinlocks this early in boot?  Probaly subsys or even device_initcall
> is more appropriate here.
>
> I would've suspected that any users of hwspinlocks will be dependent on
> drivers for the other cores (e.g. syslink) which would likely be
> initialized much later.

On that note, is there any reason why this file cannot be selected as a module?

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/