From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.6 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4484CC10F29 for ; Wed, 11 Mar 2020 06:41:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1F40C2192A for ; Wed, 11 Mar 2020 06:41:03 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=mg.codeaurora.org header.i=@mg.codeaurora.org header.b="Z0epLqAD" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725976AbgCKGlC (ORCPT ); Wed, 11 Mar 2020 02:41:02 -0400 Received: from mail26.static.mailgun.info ([104.130.122.26]:20658 "EHLO mail26.static.mailgun.info" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728120AbgCKGlC (ORCPT ); Wed, 11 Mar 2020 02:41:02 -0400 DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=mg.codeaurora.org; q=dns/txt; s=smtp; t=1583908862; h=Message-ID: References: In-Reply-To: Subject: Cc: To: From: Date: Content-Transfer-Encoding: Content-Type: MIME-Version: Sender; bh=9Ma3v93cOhH+P2MvDLMRQOh7LMKfyPwnXpDIHPd4gMM=; b=Z0epLqADKSkqpSakQ8XPh1W5xHwhffnP1TX3XZ+zNkkMhvb6DBrMWmcNYvy6BBUy16fH4iJs mdkUBXGHgGlOCBnDxGtgC1EqAd5SkMJWBdOwzwjLbspOQgJpP4ydVu663BultMpAGA3gjdDB xskbCnNtUYqt/ZZUKOaw45Xo0z8= X-Mailgun-Sending-Ip: 104.130.122.26 X-Mailgun-Sid: WyJkODczOCIsICJsaW51eC1sZWRzQHZnZXIua2VybmVsLm9yZyIsICJiZTllNGEiXQ== Received: from smtp.codeaurora.org (ec2-35-166-182-171.us-west-2.compute.amazonaws.com [35.166.182.171]) by mxa.mailgun.org with ESMTP id 5e6887fd.7f3e91b80030-smtp-out-n02; Wed, 11 Mar 2020 06:41:01 -0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 1001) id 41753C433D2; Wed, 11 Mar 2020 06:41:01 +0000 (UTC) Received: from mail.codeaurora.org (localhost.localdomain [127.0.0.1]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: kgunda) by smtp.codeaurora.org (Postfix) with ESMTPSA id 5D1F7C433CB; Wed, 11 Mar 2020 06:41:00 +0000 (UTC) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Wed, 11 Mar 2020 12:11:00 +0530 From: kgunda@codeaurora.org To: Daniel Thompson Cc: bjorn.andersson@linaro.org, jingoohan1@gmail.com, lee.jones@linaro.org, b.zolnierkie@samsung.com, dri-devel@lists.freedesktop.org, jacek.anaszewski@gmail.com, pavel@ucw.cz, robh+dt@kernel.org, mark.rutland@arm.com, linux-leds@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Andy Gross , linux-arm-msm@vger.kernel.org, linux-fbdev@vger.kernel.org, linux-arm-msm-owner@vger.kernel.org Subject: Re: [PATCH V3 2/4] backlight: qcom-wled: Add callback functions In-Reply-To: <20200310152719.5hpzh6osq22y4qbn@holly.lan> References: <1583760362-26978-1-git-send-email-kgunda@codeaurora.org> <1583760362-26978-3-git-send-email-kgunda@codeaurora.org> <20200310152719.5hpzh6osq22y4qbn@holly.lan> Message-ID: <05ab744dfbd83b6704bd394ce3c3dfc9@codeaurora.org> X-Sender: kgunda@codeaurora.org User-Agent: Roundcube Webmail/1.3.9 Sender: linux-leds-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-leds@vger.kernel.org On 2020-03-10 20:57, Daniel Thompson wrote: > On Mon, Mar 09, 2020 at 06:56:00PM +0530, Kiran Gunda wrote: >> Add cabc_config, sync_toggle, wled_ovp_fault_status and wled_ovp_delay >> callback functions to prepare the driver for adding WLED5 support. >> >> Signed-off-by: Kiran Gunda > > Overall this code would a lot easier to review if >> --- >> drivers/video/backlight/qcom-wled.c | 196 >> +++++++++++++++++++++++------------- >> 1 file changed, 126 insertions(+), 70 deletions(-) >> >> diff --git a/drivers/video/backlight/qcom-wled.c >> b/drivers/video/backlight/qcom-wled.c >> index 3d276b3..b73f273 100644 >> --- a/drivers/video/backlight/qcom-wled.c >> +++ b/drivers/video/backlight/qcom-wled.c >> @@ -128,6 +128,7 @@ struct wled_config { >> bool cs_out_en; >> bool ext_gen; >> bool cabc; >> + bool en_cabc; > > Does this ever get set to true? > Yes. If user wants use the cabc pin to control the brightness and use the "qcom,cabc" DT property in the device tree. >> bool external_pfet; >> bool auto_detection_enabled; >> }; >> @@ -147,14 +148,20 @@ struct wled { >> u32 max_brightness; >> u32 short_count; >> u32 auto_detect_count; >> + u32 version; >> bool disabled_by_short; >> bool has_short_detect; >> + bool cabc_disabled; >> int short_irq; >> int ovp_irq; >> >> struct wled_config cfg; >> struct delayed_work ovp_work; >> int (*wled_set_brightness)(struct wled *wled, u16 brightness); >> + int (*cabc_config)(struct wled *wled, bool enable); >> + int (*wled_sync_toggle)(struct wled *wled); >> + int (*wled_ovp_fault_status)(struct wled *wled, bool *fault_set); >> + int (*wled_ovp_delay)(struct wled *wled); > > Let's get some doc comments explaining what these callbacks do (and > which versions they apply to). > Sure. I will update it in the commit text in next post. > cabc_config() in particular appears to have a very odd interface for > wled4. It looks like it relies on being initially called with enable > set a particular way and prevents itself from acting again. Therefore > if > the comment you end up writing doesn't sound "right" then please also > fix the API! > Actually this variable is useful for WLED5, where the default HW state is CABC1 enabled mode. So, if the user doesn't want to use the CABC we are configuring the HW state to "0" based on the DT property and then setting a flag to not enable it again. This is not needed for WLED4. I will remove it for WLED4 in next post. > Finally, why is everything except cabc_config() prefixed with wled? > It is typo. I will correct it in the next post. > > Daniel. From mboxrd@z Thu Jan 1 00:00:00 1970 From: kgunda@codeaurora.org Date: Wed, 11 Mar 2020 06:53:00 +0000 Subject: Re: [PATCH V3 2/4] backlight: qcom-wled: Add callback functions Message-Id: <05ab744dfbd83b6704bd394ce3c3dfc9@codeaurora.org> List-Id: References: <1583760362-26978-1-git-send-email-kgunda@codeaurora.org> <1583760362-26978-3-git-send-email-kgunda@codeaurora.org> <20200310152719.5hpzh6osq22y4qbn@holly.lan> In-Reply-To: <20200310152719.5hpzh6osq22y4qbn@holly.lan> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Daniel Thompson Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, linux-fbdev@vger.kernel.org, b.zolnierkie@samsung.com, jingoohan1@gmail.com, Andy Gross , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, bjorn.andersson@linaro.org, robh+dt@kernel.org, jacek.anaszewski@gmail.com, pavel@ucw.cz, linux-arm-msm@vger.kernel.org, lee.jones@linaro.org, linux-arm-msm-owner@vger.kernel.org, linux-leds@vger.kernel.org On 2020-03-10 20:57, Daniel Thompson wrote: > On Mon, Mar 09, 2020 at 06:56:00PM +0530, Kiran Gunda wrote: >> Add cabc_config, sync_toggle, wled_ovp_fault_status and wled_ovp_delay >> callback functions to prepare the driver for adding WLED5 support. >> >> Signed-off-by: Kiran Gunda > > Overall this code would a lot easier to review if >> --- >> drivers/video/backlight/qcom-wled.c | 196 >> +++++++++++++++++++++++------------- >> 1 file changed, 126 insertions(+), 70 deletions(-) >> >> diff --git a/drivers/video/backlight/qcom-wled.c >> b/drivers/video/backlight/qcom-wled.c >> index 3d276b3..b73f273 100644 >> --- a/drivers/video/backlight/qcom-wled.c >> +++ b/drivers/video/backlight/qcom-wled.c >> @@ -128,6 +128,7 @@ struct wled_config { >> bool cs_out_en; >> bool ext_gen; >> bool cabc; >> + bool en_cabc; > > Does this ever get set to true? > Yes. If user wants use the cabc pin to control the brightness and use the "qcom,cabc" DT property in the device tree. >> bool external_pfet; >> bool auto_detection_enabled; >> }; >> @@ -147,14 +148,20 @@ struct wled { >> u32 max_brightness; >> u32 short_count; >> u32 auto_detect_count; >> + u32 version; >> bool disabled_by_short; >> bool has_short_detect; >> + bool cabc_disabled; >> int short_irq; >> int ovp_irq; >> >> struct wled_config cfg; >> struct delayed_work ovp_work; >> int (*wled_set_brightness)(struct wled *wled, u16 brightness); >> + int (*cabc_config)(struct wled *wled, bool enable); >> + int (*wled_sync_toggle)(struct wled *wled); >> + int (*wled_ovp_fault_status)(struct wled *wled, bool *fault_set); >> + int (*wled_ovp_delay)(struct wled *wled); > > Let's get some doc comments explaining what these callbacks do (and > which versions they apply to). > Sure. I will update it in the commit text in next post. > cabc_config() in particular appears to have a very odd interface for > wled4. It looks like it relies on being initially called with enable > set a particular way and prevents itself from acting again. Therefore > if > the comment you end up writing doesn't sound "right" then please also > fix the API! > Actually this variable is useful for WLED5, where the default HW state is CABC1 enabled mode. So, if the user doesn't want to use the CABC we are configuring the HW state to "0" based on the DT property and then setting a flag to not enable it again. This is not needed for WLED4. I will remove it for WLED4 in next post. > Finally, why is everything except cabc_config() prefixed with wled? > It is typo. I will correct it in the next post. > > Daniel. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.6 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2153EC10F27 for ; Wed, 11 Mar 2020 08:33:05 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E9DCD20848 for ; Wed, 11 Mar 2020 08:33:04 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=mg.codeaurora.org header.i=@mg.codeaurora.org header.b="VBVahBc+" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E9DCD20848 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 87A436E932; Wed, 11 Mar 2020 08:32:43 +0000 (UTC) Received: from mail26.static.mailgun.info (mail26.static.mailgun.info [104.130.122.26]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3CBCB89BD2 for ; Wed, 11 Mar 2020 06:41:06 +0000 (UTC) DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=mg.codeaurora.org; q=dns/txt; s=smtp; t=1583908867; h=Message-ID: References: In-Reply-To: Subject: Cc: To: From: Date: Content-Transfer-Encoding: Content-Type: MIME-Version: Sender; bh=9Ma3v93cOhH+P2MvDLMRQOh7LMKfyPwnXpDIHPd4gMM=; b=VBVahBc+4gENeUkjXu4E2P3Q9LyzntJ5ZCP0qRoKCkHLkxQGRV0e6aQ+eCL1dpEHV0x/+EV5 tPhbNkTCY7vg3tjBa8YsSccctA+d39tPVK4BnZlS6ba21PPxZq5igoW5RTXq2A7rDxZxHlQh OcFM8g+dEzusTindMr+sH8fNXfs= X-Mailgun-Sending-Ip: 104.130.122.26 X-Mailgun-Sid: WyJkOTU5ZSIsICJkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnIiwgImJlOWU0YSJd Received: from smtp.codeaurora.org (ec2-35-166-182-171.us-west-2.compute.amazonaws.com [35.166.182.171]) by mxa.mailgun.org with ESMTP id 5e6887fd.7f0ea6117378-smtp-out-n01; Wed, 11 Mar 2020 06:41:01 -0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 1001) id 54B9CC43637; Wed, 11 Mar 2020 06:41:01 +0000 (UTC) Received: from mail.codeaurora.org (localhost.localdomain [127.0.0.1]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: kgunda) by smtp.codeaurora.org (Postfix) with ESMTPSA id 5D1F7C433CB; Wed, 11 Mar 2020 06:41:00 +0000 (UTC) MIME-Version: 1.0 Date: Wed, 11 Mar 2020 12:11:00 +0530 From: kgunda@codeaurora.org To: Daniel Thompson Subject: Re: [PATCH V3 2/4] backlight: qcom-wled: Add callback functions In-Reply-To: <20200310152719.5hpzh6osq22y4qbn@holly.lan> References: <1583760362-26978-1-git-send-email-kgunda@codeaurora.org> <1583760362-26978-3-git-send-email-kgunda@codeaurora.org> <20200310152719.5hpzh6osq22y4qbn@holly.lan> Message-ID: <05ab744dfbd83b6704bd394ce3c3dfc9@codeaurora.org> X-Sender: kgunda@codeaurora.org User-Agent: Roundcube Webmail/1.3.9 X-Mailman-Approved-At: Wed, 11 Mar 2020 08:32:41 +0000 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, linux-fbdev@vger.kernel.org, b.zolnierkie@samsung.com, jingoohan1@gmail.com, Andy Gross , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, bjorn.andersson@linaro.org, robh+dt@kernel.org, jacek.anaszewski@gmail.com, pavel@ucw.cz, linux-arm-msm@vger.kernel.org, lee.jones@linaro.org, linux-arm-msm-owner@vger.kernel.org, linux-leds@vger.kernel.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On 2020-03-10 20:57, Daniel Thompson wrote: > On Mon, Mar 09, 2020 at 06:56:00PM +0530, Kiran Gunda wrote: >> Add cabc_config, sync_toggle, wled_ovp_fault_status and wled_ovp_delay >> callback functions to prepare the driver for adding WLED5 support. >> >> Signed-off-by: Kiran Gunda > > Overall this code would a lot easier to review if >> --- >> drivers/video/backlight/qcom-wled.c | 196 >> +++++++++++++++++++++++------------- >> 1 file changed, 126 insertions(+), 70 deletions(-) >> >> diff --git a/drivers/video/backlight/qcom-wled.c >> b/drivers/video/backlight/qcom-wled.c >> index 3d276b3..b73f273 100644 >> --- a/drivers/video/backlight/qcom-wled.c >> +++ b/drivers/video/backlight/qcom-wled.c >> @@ -128,6 +128,7 @@ struct wled_config { >> bool cs_out_en; >> bool ext_gen; >> bool cabc; >> + bool en_cabc; > > Does this ever get set to true? > Yes. If user wants use the cabc pin to control the brightness and use the "qcom,cabc" DT property in the device tree. >> bool external_pfet; >> bool auto_detection_enabled; >> }; >> @@ -147,14 +148,20 @@ struct wled { >> u32 max_brightness; >> u32 short_count; >> u32 auto_detect_count; >> + u32 version; >> bool disabled_by_short; >> bool has_short_detect; >> + bool cabc_disabled; >> int short_irq; >> int ovp_irq; >> >> struct wled_config cfg; >> struct delayed_work ovp_work; >> int (*wled_set_brightness)(struct wled *wled, u16 brightness); >> + int (*cabc_config)(struct wled *wled, bool enable); >> + int (*wled_sync_toggle)(struct wled *wled); >> + int (*wled_ovp_fault_status)(struct wled *wled, bool *fault_set); >> + int (*wled_ovp_delay)(struct wled *wled); > > Let's get some doc comments explaining what these callbacks do (and > which versions they apply to). > Sure. I will update it in the commit text in next post. > cabc_config() in particular appears to have a very odd interface for > wled4. It looks like it relies on being initially called with enable > set a particular way and prevents itself from acting again. Therefore > if > the comment you end up writing doesn't sound "right" then please also > fix the API! > Actually this variable is useful for WLED5, where the default HW state is CABC1 enabled mode. So, if the user doesn't want to use the CABC we are configuring the HW state to "0" based on the DT property and then setting a flag to not enable it again. This is not needed for WLED4. I will remove it for WLED4 in next post. > Finally, why is everything except cabc_config() prefixed with wled? > It is typo. I will correct it in the next post. > > Daniel. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel