Sanjeev,
Please see my comments below:
> +EXPORT_SYMBOL(hwspinlock_trylock);
[sp] The code is almost duplicated in the functions hwspinlock_trylock()
and hwspinlock_lock(). The difference being try-once/ try-multiple.
I believe one function with additional arg - say limit - should be
sufficient.
If there is need to abstract additional arg, you can define macros
for the same.
[SQ] Good point, done.
> + for (i = 0; i < hwspinlock_state.num_locks && !found; i++) {
> + if (!hwspinlock_array[i].is_allocated) {
> + found = true;
> + handle = &hwspinlock_array[i];
> + }
[sp] Starting from index 0 every time can be quite time consuming
esp. in the situations where we need spinlocks. IRQs being
disabled for long can impact the performance.
Wouldn't a used and free list be better alternative?
[SQ] Done.
> + if (WARN_ON(hwspinlock_array[id].is_allocated))
> + goto exit;
> +
[sp] if (id > hwspinlock_state.num_locks) then ??
[SQ] Good catch. Fixed.
> +EXPORT_SYMBOL(hwspinlock_request_specific);
[sp] What if either of the "request" funcs are called before
hwspinlock_probe() gets executed?
[SQ] Added a check for that.
> +/* Probe function */
> +static int __devinit hwspinlock_probe(struct platform_device *pdev) {
> + struct resource *res;
> + void __iomem *io_base;
> + int id;
> +
[sp] Extra line?
[SQ] Yeah, got rid of it.
> + void __iomem *sysstatus_reg;
[sp] can be combined with op_base declaration.
[SQ] Okay done.
> +static struct platform_driver hwspinlock_driver = {
> + .probe = hwspinlock_probe,
> + .driver = {
> + .name = "hwspinlock",
[sp] Extra TAB?
[SQ] No, it's one level down. Notice the extra { }'s.
Thanks,
Simon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
