From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965720AbbBCRtH (ORCPT ); Tue, 3 Feb 2015 12:49:07 -0500 Received: from smtp-out-020.synserver.de ([212.40.185.20]:1041 "EHLO smtp-out-017.synserver.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S966738AbbBCRtE (ORCPT ); Tue, 3 Feb 2015 12:49:04 -0500 X-SynServer-TrustedSrc: 1 X-SynServer-AuthUser: lars@metafoo.de X-SynServer-PPID: 27279 Message-ID: <54D10A0E.5050907@metafoo.de> Date: Tue, 03 Feb 2015 18:49:02 +0100 From: Lars-Peter Clausen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.4.0 MIME-Version: 1.0 To: Russell King - ARM Linux CC: alsa-devel , Richard Purdie , patches@opensource.wolfsonmicro.com, LKML , linux-sound@vger.kernel.org, Manuel Lauss , Mark Brown , Bo Shen , Manuel Lauss , linux-arm-kernel@lists.infradead.org, Liam Girdwood Subject: Re: [alsa-devel] [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself References: <1422934415-24957-1-git-send-email-voice.shen@atmel.com> <20150203124441.GK21293@sirena.org.uk> <54D0FD1C.9020305@metafoo.de> <20150203171733.GY8656@n2100.arm.linux.org.uk> <54D104AC.5010603@metafoo.de> In-Reply-To: <54D104AC.5010603@metafoo.de> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/03/2015 06:26 PM, Lars-Peter Clausen wrote: > On 02/03/2015 06:17 PM, Russell King - ARM Linux wrote: >> On Tue, Feb 03, 2015 at 05:53:48PM +0100, Lars-Peter Clausen wrote: >>> On 02/03/2015 01:44 PM, Mark Brown wrote: >>>> On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote: >>>> >>>>> + wm8731->mclk = devm_clk_get(&spi->dev, "mclk"); >>>>> + if (IS_ERR(wm8731->mclk)) { >>>>> + wm8731->mclk = NULL; >>>>> + dev_warn(&spi->dev, "assuming static MCLK\n"); >>>>> + } >>>> >>>> This is broken for both deferred probe and in the case where the clock >>>> API genuinely returns a NULL clock. Other than that it's the kind of >>>> thing that we've done for some other drivers, though it's not good to >>>> have to do this. Check them for correct behaviour. >>> >>> Ideally we'd introduce a {devm_}clk_get_optional(), with the same semantics >>> as gpiod_get_optional(), which handles the finer details of differentiating >>> between clock specified, but not yet probed, clock specified, but >>> incorrectly and no clock specified, so this doesn't have to be done over and >>> over by each driver. >> >> No, we don't need to. It clk_get() already knows this distinction, and >> it appropriately returns -ENOENT vs -EPROBE_DEFER according to whether >> there's a clock specified in DT or not. > > I know, but it returns a error when no clock is specified (-ENOENT), whereas > gpiod_get_optional()-like semantics mean, it would return no error. What I wanted to say is that pretty much every user of clk_get() that wants a optional clock gets the handling wrong. E.g. they check for PTR_ERR(clk) == -EPROBE_DEFER rather than checking for PTR_ERR(clk) != -ENOENT. Which causes errors when the clock is specified, but incorrectly specified (e.g. invalid phandle or specifier) to be silently ignored. My hope is that having a explicit API for requesting a optional clock might make it easier for users to gets things right. If you have coccinelle you can use the following script to find good and bad users: @@ expression clk; @@ clk = ( devm_clk_get | clk_get ) (...); <+... ( *PTR_ERR(clk) == -EPROBE_DEFER | *PTR_ERR(clk) != -ENOENT ) ...+>