Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751857Ab1BDLT3 (ORCPT ); Fri, 4 Feb 2011 06:19:29 -0500 Received: from caramon.arm.linux.org.uk ([78.32.30.218]:45559 "EHLO caramon.arm.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751559Ab1BDLT2 (ORCPT ); Fri, 4 Feb 2011 06:19:28 -0500 Date: Fri, 4 Feb 2011 11:18:41 +0000 From: Russell King - ARM Linux To: Jassi Brar Cc: Richard Zhao , Nicolas Pitre , Lorenzo Pieralisi , Vincent Guittot , linux-sh@vger.kernel.org, Ben Herrenschmidt , Sascha Hauer , Paul Mundt , Stephen Boyd , linux-kernel@vger.kernel.org, Dima Zavin , Saravana Kannan , Ben Dooks , Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= , Jeremy Kerr , linux-arm-kernel@lists.infradead.org Subject: Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare Message-ID: <20110204111841.GG14627@n2100.arm.linux.org.uk> References: <20110201131512.GH31216@n2100.arm.linux.org.uk> <20110201141837.GA1147@pengutronix.de> <20110201143932.GK31216@n2100.arm.linux.org.uk> <20110201151846.GD1147@pengutronix.de> <20110201152458.GP31216@n2100.arm.linux.org.uk> <4D48741F.8060006@codeaurora.org> <20110201212409.GU31216@n2100.arm.linux.org.uk> <20110204095424.GB2347@richard-laptop> <20110204104832.GE14627@n2100.arm.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.19 (2009-01-05) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3009 Lines: 107 On Fri, Feb 04, 2011 at 08:04:03PM +0900, Jassi Brar wrote: > On Fri, Feb 4, 2011 at 7:48 PM, Russell King - ARM Linux > wrote: > > > int clk_enable(struct clk *clk) > > { > > ? ? ? ?unsigned long flags; > > ? ? ? ?int ret = 0; > > > > ? ? ? ?if (clk) { > > ? ? ? ? ? ? ? ?if (WARN_ON(!clk->prepare_count)) > > ? ? ? ? ? ? ? ? ? ? ? ?return -EINVAL; > > > > ? ? ? ? ? ? ? ?spin_lock_irqsave(&clk->lock, flags); > > ? ? ? ? ? ? ? ?if (clk->enable_count++ == 0) > > ? ? ? ? ? ? ? ? ? ? ? ?ret = clk->ops->enable(clk); > > ? ? ? ? ? ? ? ?spin_unlock_irqrestore(&clk->lock, flags); > > ? ? ? ?} > > ? ? ? ?return ret; > > } > > > > is entirely sufficient to catch the case of a single-use clock not being > > prepared before clk_enable() is called. > > > > We're after detecting drivers missing calls to clk_prepare(), we're not > > after detecting concurrent calls to clk_prepare()/clk_unprepare(). > > I hope you mean 'making sure the clock is prepared before it's enabled > ' rather than > 'catching a driver that doesn't do clk_prepare before clk_enable'. > Because, the above implementation still doesn't catch a driver that > doesn't call clk_prepare > but simply uses a clock that happens to have been already prepare'd by > some other > driver or the platform. No, I mean what I said. The only way to do what you're asking is to attach a list of identifiers which have prepared a clock to the struct clk, where each identifier is unique to each driver instance. So what that becomes is: struct prepared_instance { struct list_head node; void *driver_id; }; int clk_prepare(struct clk *clk, void *driver_id) { struct prepared_instance *inst; int ret = 0; if (clk) { inst = kmalloc(sizeof(*inst), GFP_KERNEL); if (!inst) return -ENOMEM; inst->driver_id = driver_id; mutex_lock(&clk->mutex); if (clk->prepare_count++ == 0) ret = clk->ops->prepare(clk); if (ret == 0) { spin_lock_irqsave(&clk->lock, flags); list_add(&inst->node, &clk->prepare_list); spin_unlock_irqrestore(&clk->lock, flags); } else clk->prepare_count--; mutex_unlock(&clk->mutex); } return ret; } int clk_enable(struct clk *clk, void *driver_id) { unsigned long flags; int ret = 0; if (clk) { struct prepare_instance *inst; spin_lock_irqsave(&clk->lock, flags); list_for_each_entry(inst, &clk->prepare_list, node) if (inst == driver_id) ret = -EINVAL; if (ret == 0 && clk->enable_count++ == 0) { ret = clk->ops->enable(clk); if (ret) clk->enable_count--; } spin_unlock_irqrestore(&clk->lock, flags); } return ret; } I think that's going completely over the top, and adds needless complexity to drivers, which now have to pass an instance specific cookie into every clk API call. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/