From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755181AbbCBV7y (ORCPT ); Mon, 2 Mar 2015 16:59:54 -0500 Received: from mail-gw3-out.broadcom.com ([216.31.210.64]:10865 "EHLO mail-gw3-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755038AbbCBV7w (ORCPT ); Mon, 2 Mar 2015 16:59:52 -0500 X-IronPort-AV: E=Sophos;i="5.09,677,1418112000"; d="scan'208";a="58233360" Message-ID: <54F4DD33.2050803@broadcom.com> Date: Mon, 2 Mar 2015 13:59:15 -0800 From: Scott Branden User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Dmitry Torokhov CC: , Rob Herring , Pawel Moll , Mark Rutland , "Ian Campbell" , Kumar Gala , Grant Likely , Ray Jui , Jonathan Richardson , Dmitry Torokhov , Anatol Pomazao , , , Subject: Re: [PATCH v4 2/2] Input: bcm-keypad: Add Broadcom keypad controller References: <1425141357-7807-1-git-send-email-sbranden@broadcom.com> <1425141357-7807-3-git-send-email-sbranden@broadcom.com> <20150228221022.GA36282@dtor-ws> <20150228221518.GB36282@dtor-ws> <54F4BC15.4020205@broadcom.com> <20150302202342.GE17094@dtor-ws> In-Reply-To: <20150302202342.GE17094@dtor-ws> 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 Hi Dmitry, I understand what's missing now. Thanks, Scott On 15-03-02 12:23 PM, Dmitry Torokhov wrote: > Hi Scott, > >>> Hmm, also bcm_kp_start() and bcm_kp_stop() should check if kp->clk is >>> valid before trying to enable/disable it. >>> >> I checked and other keyboard drivers do not check this. I return an >> error in bcm_kp_start if the clk enable fails. On stop, if the clk >> is not valid something is really, really wrong as well. > > The other drivers simply abort probe() if they can't get clock, you > decided to allow probe() to finish and assume that clock is already > enabled, leaving kp->clk == ERR_PTR(-ENOENT) in your version. If you try > then calling clk_prepare_enable() with that pointer it is going to bomb, > that is why I said you need to check pointer validity in bcm_kp_start() > and bcm_kp_stop(). > > Thanks. > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Branden Subject: Re: [PATCH v4 2/2] Input: bcm-keypad: Add Broadcom keypad controller Date: Mon, 2 Mar 2015 13:59:15 -0800 Message-ID: <54F4DD33.2050803@broadcom.com> References: <1425141357-7807-1-git-send-email-sbranden@broadcom.com> <1425141357-7807-3-git-send-email-sbranden@broadcom.com> <20150228221022.GA36282@dtor-ws> <20150228221518.GB36282@dtor-ws> <54F4BC15.4020205@broadcom.com> <20150302202342.GE17094@dtor-ws> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150302202342.GE17094@dtor-ws> Sender: linux-kernel-owner@vger.kernel.org To: Dmitry Torokhov Cc: bcm-kernel-feedback-list@broadcom.com, Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Grant Likely , Ray Jui , Jonathan Richardson , Dmitry Torokhov , Anatol Pomazao , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org List-Id: devicetree@vger.kernel.org Hi Dmitry, I understand what's missing now. Thanks, Scott On 15-03-02 12:23 PM, Dmitry Torokhov wrote: > Hi Scott, > >>> Hmm, also bcm_kp_start() and bcm_kp_stop() should check if kp->clk is >>> valid before trying to enable/disable it. >>> >> I checked and other keyboard drivers do not check this. I return an >> error in bcm_kp_start if the clk enable fails. On stop, if the clk >> is not valid something is really, really wrong as well. > > The other drivers simply abort probe() if they can't get clock, you > decided to allow probe() to finish and assume that clock is already > enabled, leaving kp->clk == ERR_PTR(-ENOENT) in your version. If you try > then calling clk_prepare_enable() with that pointer it is going to bomb, > that is why I said you need to check pointer validity in bcm_kp_start() > and bcm_kp_stop(). > > Thanks. > From mboxrd@z Thu Jan 1 00:00:00 1970 From: sbranden@broadcom.com (Scott Branden) Date: Mon, 2 Mar 2015 13:59:15 -0800 Subject: [PATCH v4 2/2] Input: bcm-keypad: Add Broadcom keypad controller In-Reply-To: <20150302202342.GE17094@dtor-ws> References: <1425141357-7807-1-git-send-email-sbranden@broadcom.com> <1425141357-7807-3-git-send-email-sbranden@broadcom.com> <20150228221022.GA36282@dtor-ws> <20150228221518.GB36282@dtor-ws> <54F4BC15.4020205@broadcom.com> <20150302202342.GE17094@dtor-ws> Message-ID: <54F4DD33.2050803@broadcom.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Dmitry, I understand what's missing now. Thanks, Scott On 15-03-02 12:23 PM, Dmitry Torokhov wrote: > Hi Scott, > >>> Hmm, also bcm_kp_start() and bcm_kp_stop() should check if kp->clk is >>> valid before trying to enable/disable it. >>> >> I checked and other keyboard drivers do not check this. I return an >> error in bcm_kp_start if the clk enable fails. On stop, if the clk >> is not valid something is really, really wrong as well. > > The other drivers simply abort probe() if they can't get clock, you > decided to allow probe() to finish and assume that clock is already > enabled, leaving kp->clk == ERR_PTR(-ENOENT) in your version. If you try > then calling clk_prepare_enable() with that pointer it is going to bomb, > that is why I said you need to check pointer validity in bcm_kp_start() > and bcm_kp_stop(). > > Thanks. >