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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D13C0C433EF for ; Tue, 26 Jul 2022 09:31:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238200AbiGZJbJ (ORCPT ); Tue, 26 Jul 2022 05:31:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36290 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238122AbiGZJbF (ORCPT ); Tue, 26 Jul 2022 05:31:05 -0400 Received: from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com [IPv6:2a00:1450:4864:20::42a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C8AE1275E5 for ; Tue, 26 Jul 2022 02:31:02 -0700 (PDT) Received: by mail-wr1-x42a.google.com with SMTP id m17so19032421wrw.7 for ; Tue, 26 Jul 2022 02:31:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=6DKLKhCuv0oTIK2mIKjow1hORCEme2MquQ5buN/wlGc=; b=vwxd0FJzG20ErAwFpJwytDA+XPSph7dO2hF3++04iGbvWolycV3nT1x50PhyF4yHVv t1GLnxbnwGZ+FzFIlfK797RF6mQyiWslVMWv1aGlCeqirOa69WQl16hKksjuQVfFBH02 f5piNzeRWcNmB1Tskni+qHzA/ciJm1Nhz77bR7dqguiDaWeWbf4VGMiBpKBfhuOnl6Nu VHMCRnErCFSCik4l0zZGsC6NRnN7/mU95iBQp/fRhvvTk0HgHXm+PjCUenlLDLOJL3dZ iGRwb4BOjNyYMLApd/GQSMApO5uqSogxp5chUadtE3p2WwR/0f3m4A0tA6FeBBrLQJJw PyxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=6DKLKhCuv0oTIK2mIKjow1hORCEme2MquQ5buN/wlGc=; b=dBR5hgKMUV85jU5pzqYzQGMLcZ/s7uN6TZdk52RyxhErjzaGjsmpytTMiVfND0pgmC QVH1IcNX/1UJsOgKSEUNJGy38v4N8d7nw1ZNtGiVk/Q36ZUvqogHvWIgPkEXRkk90O7U GwF8Uiqp72M8tZvHKEQ0TXgcwJvAWJ5hUVVeL/HPT1bvpGo5byHWdvQP6FQgqZOH7fah LCigESxqB++lKpoXQaZ3/KXS01K/d4EGmoCzOilCnqj/KGNishgtEbykqQQr1GBBUGRI xoF05PhaxbK1UBUcxFIVcF9t/iNMgEsFVeWlp5JGYTsWP5d9kAtjx8Fri5nQQy+T9m7j 7LMg== X-Gm-Message-State: AJIora/o5KV0PKAaQB461L5qk3QjIXt+Bj78F5uPEDA2DZGNwAfe/RzB 3Dlwh0dS3eYcZz5WOsQlmHErNg== X-Google-Smtp-Source: AGRyM1upJ6Koxcaxp7uEGgh1VKOfPB35WonHb+9rPNcP2pyI1+sRFKmJXTmiMFmK1V6L3XLfn6REKg== X-Received: by 2002:adf:db8e:0:b0:21e:3fff:6bae with SMTP id u14-20020adfdb8e000000b0021e3fff6baemr9890443wri.184.1658827861173; Tue, 26 Jul 2022 02:31:01 -0700 (PDT) Received: from maple.lan (cpc141216-aztw34-2-0-cust174.18-1.cable.virginm.net. [80.7.220.175]) by smtp.gmail.com with ESMTPSA id o15-20020a05600c510f00b003a30fbde91dsm23407540wms.20.2022.07.26.02.30.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Jul 2022 02:31:00 -0700 (PDT) Date: Tue, 26 Jul 2022 10:30:58 +0100 From: Daniel Thompson To: ChiaEn Wu Cc: Lee Jones , Jingoo Han , Pavel Machek , Rob Herring , Krzysztof Kozlowski , Matthias Brugger , Sebastian Reichel , Chunfeng Yun , Greg Kroah-Hartman , Jonathan Cameron , Lars-Peter Clausen , Liam Girdwood , Mark Brown , Guenter Roeck , "Krogerus, Heikki" , Helge Deller , Andy Shevchenko , ChiaEn Wu , Alice Chen , ChiYuan Huang , dri-devel , Linux LED Subsystem , devicetree , linux-arm Mailing List , "moderated list:ARM/Mediatek SoC support" , Linux Kernel Mailing List , Linux PM , USB , linux-iio , "open list:FRAMEBUFFER LAYER" , szuni chen Subject: Re: [PATCH v6 13/13] video: backlight: mt6370: Add MediaTek MT6370 support Message-ID: <20220726093058.2fz2p2vg7xpfsnfe@maple.lan> References: <20220722102407.2205-1-peterwu.pub@gmail.com> <20220722102407.2205-14-peterwu.pub@gmail.com> <20220725103128.xtaw2c4y5fobowg7@maple.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-leds@vger.kernel.org On Tue, Jul 26, 2022 at 10:20:02AM +0800, ChiaEn Wu wrote: > On Mon, Jul 25, 2022 at 6:31 PM Daniel Thompson > wrote: > > > > On Fri, Jul 22, 2022 at 06:24:07PM +0800, ChiaEn Wu wrote: > > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > > > index a003e02..846dbe7 100644 > > > --- a/drivers/video/backlight/Kconfig > > > +++ b/drivers/video/backlight/Kconfig > > > @@ -268,6 +268,18 @@ config BACKLIGHT_MAX8925 > > > If you have a LCD backlight connected to the WLED output of MAX8925 > > > WLED output, say Y here to enable this driver. > > > > > > +config BACKLIGHT_MT6370 > > > + tristate "MediaTek MT6370 Backlight Driver" > > > + depends on MFD_MT6370 > > > + help > > > + This enables support for Mediatek MT6370 Backlight driver. > > > + It's commonly used to drive the display WLED. There are 4 channels > > > + inside, and each channel supports up to 30mA of current capability > > > + with 2048 current steps in exponential or linear mapping curves. > > > > Does the MT6372 support more steps than this? In other words does it use > > a fourteen bit scale or does it use an 11-bit scale at a different > > register location? > > Hi Daniel, > > Thanks for your reply. > Yes, MT6372 can support 16384 steps and uses a 14-bit scale register > location. But the maximum current of each > channel of MT6372 is the same as MT6370 and MT6371, both 30mA. > The main reason why MT6372 is designed this way is that one of the > customers asked for a more delicate > adjustment of the backlight brightness. But other customers actually > do not have such requirements. > Therefore, we designed it this way for maximum compatibility in software. I don't think that is an acceptable approach for the upstream kernel. To be "compatible" with (broken) software this driver ends up reducing the capability of the upstream kernel to the point it becomes unable to meet requirements for delicate adjustment (requirements that were sufficiently important to change the hardware design so you could meet them). > > > + > > > + This driver can also be built as a module. If so, the module > > > + will be called "mt6370-backlight". > > > + > > > [...] > > > diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c > > > new file mode 100644 > > > index 0000000..ba00a8f > > > --- /dev/null > > > +++ b/drivers/video/backlight/mt6370-backlight.c > > > [...] > > > +static int mt6370_bl_update_status(struct backlight_device *bl_dev) > > > +{ > > > + struct mt6370_priv *priv = bl_get_data(bl_dev); > > > + int brightness = backlight_get_brightness(bl_dev); > > > + unsigned int enable_val; > > > + u8 brightness_val[2]; > > > + int ret; > > > + > > > + if (brightness) { > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > > > + > > > + /* > > > + * To make MT6372 using 14 bits to control the brightness > > > + * backward compatible with 11 bits brightness control > > > + * (like MT6370 and MT6371 do), we left shift the value > > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > > + * backlight brightness will be almost the same as MT6370's > > > + * and MT6371's. > > > + */ > > > + if (priv->vid_type == MT6370_VID_6372) { > > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > > + } > > > > This somewhat depends on the answer to the first question above, but > > what is the point of this shifting? If the range is 14-bit then the > > driver should set max_brightness to 16384 and present the full range of > > the MT6372 to the user. > > So should we make all 16384 steps of MT6372 available to users? Yes. > Does that mean the DTS needs to be modified as well? Yes... the property to set initial brightness needs a 14-bit range. It would also be a good idea to discuss with the DT maintainers whether you should introduce a second compatible string (ending 6372) in order to allow the DT validation checks to detect accidental use of MT6372 ranges on MT6370 hardware. > Or, for the reasons, I have just explained (just one customer has this > requirement), then we do not make any changes for compatibility > reasons? I'd be curious what the compatiblity reasons are. In other words what software breaks? Normally the userspace backlight code reads the max_brightness property and configures things accordingly (and therefore if you the component that breaks is something like an Android HAL then fix the HAL instead). 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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 061F2C43334 for ; Tue, 26 Jul 2022 09:31:06 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 642A6113DDE; Tue, 26 Jul 2022 09:31:05 +0000 (UTC) Received: from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com [IPv6:2a00:1450:4864:20::42a]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2A248112DA7 for ; Tue, 26 Jul 2022 09:31:02 +0000 (UTC) Received: by mail-wr1-x42a.google.com with SMTP id k12so1592854wrm.13 for ; Tue, 26 Jul 2022 02:31:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=6DKLKhCuv0oTIK2mIKjow1hORCEme2MquQ5buN/wlGc=; b=vwxd0FJzG20ErAwFpJwytDA+XPSph7dO2hF3++04iGbvWolycV3nT1x50PhyF4yHVv t1GLnxbnwGZ+FzFIlfK797RF6mQyiWslVMWv1aGlCeqirOa69WQl16hKksjuQVfFBH02 f5piNzeRWcNmB1Tskni+qHzA/ciJm1Nhz77bR7dqguiDaWeWbf4VGMiBpKBfhuOnl6Nu VHMCRnErCFSCik4l0zZGsC6NRnN7/mU95iBQp/fRhvvTk0HgHXm+PjCUenlLDLOJL3dZ iGRwb4BOjNyYMLApd/GQSMApO5uqSogxp5chUadtE3p2WwR/0f3m4A0tA6FeBBrLQJJw PyxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=6DKLKhCuv0oTIK2mIKjow1hORCEme2MquQ5buN/wlGc=; b=J3UCAS+GW3xNO43J0kQm+PD+jR8Gsm/oDAYIK3lTxx3FUeXv3GRp7Jw6fK87JUyLJa 8PSbAhoGG6qPNJpPdhzTZ+Wby33DcSswziK+myjqNmvXcv/RXt7t/9IGDb+eQomPetEG qBqV5X4N5gQuQX8lfyjVHV1LMQBjxe1nBDEPxxJmhvP4+CujwWglrKac1tG0dnpXachE sZRntm3Kvr2e3KJWRqMcXNPYwobhhP/DepQQE0WfJY5ZElUOKKmTy66Br3xUO/jvJQZS yGuHRCjX14xVAmHXLcBy9QrsxUGuqPSYg5t1u7BAbTdSmVjY2RRqMnzJ98dvWSqWbJZP NTFQ== X-Gm-Message-State: AJIora8l3tzFuEJR0S74akBQJS9GEIEtcSKuYkBEocUI1xfEFhQL/POs Vi0MJQXujlCLkERHK+Vna0sRYA== X-Google-Smtp-Source: AGRyM1upJ6Koxcaxp7uEGgh1VKOfPB35WonHb+9rPNcP2pyI1+sRFKmJXTmiMFmK1V6L3XLfn6REKg== X-Received: by 2002:adf:db8e:0:b0:21e:3fff:6bae with SMTP id u14-20020adfdb8e000000b0021e3fff6baemr9890443wri.184.1658827861173; Tue, 26 Jul 2022 02:31:01 -0700 (PDT) Received: from maple.lan (cpc141216-aztw34-2-0-cust174.18-1.cable.virginm.net. [80.7.220.175]) by smtp.gmail.com with ESMTPSA id o15-20020a05600c510f00b003a30fbde91dsm23407540wms.20.2022.07.26.02.30.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Jul 2022 02:31:00 -0700 (PDT) Date: Tue, 26 Jul 2022 10:30:58 +0100 From: Daniel Thompson To: ChiaEn Wu Subject: Re: [PATCH v6 13/13] video: backlight: mt6370: Add MediaTek MT6370 support Message-ID: <20220726093058.2fz2p2vg7xpfsnfe@maple.lan> References: <20220722102407.2205-1-peterwu.pub@gmail.com> <20220722102407.2205-14-peterwu.pub@gmail.com> <20220725103128.xtaw2c4y5fobowg7@maple.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: "open list:FRAMEBUFFER LAYER" , "Krogerus, Heikki" , Krzysztof Kozlowski , Alice Chen , linux-iio , dri-devel , Liam Girdwood , ChiYuan Huang , Pavel Machek , Lee Jones , Linux LED Subsystem , Helge Deller , Rob Herring , Andy Shevchenko , Chunfeng Yun , Guenter Roeck , devicetree , Linux PM , szuni chen , Mark Brown , "moderated list:ARM/Mediatek SoC support" , Matthias Brugger , linux-arm Mailing List , Jingoo Han , USB , Sebastian Reichel , Linux Kernel Mailing List , ChiaEn Wu , Greg Kroah-Hartman , Jonathan Cameron Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Tue, Jul 26, 2022 at 10:20:02AM +0800, ChiaEn Wu wrote: > On Mon, Jul 25, 2022 at 6:31 PM Daniel Thompson > wrote: > > > > On Fri, Jul 22, 2022 at 06:24:07PM +0800, ChiaEn Wu wrote: > > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > > > index a003e02..846dbe7 100644 > > > --- a/drivers/video/backlight/Kconfig > > > +++ b/drivers/video/backlight/Kconfig > > > @@ -268,6 +268,18 @@ config BACKLIGHT_MAX8925 > > > If you have a LCD backlight connected to the WLED output of MAX8925 > > > WLED output, say Y here to enable this driver. > > > > > > +config BACKLIGHT_MT6370 > > > + tristate "MediaTek MT6370 Backlight Driver" > > > + depends on MFD_MT6370 > > > + help > > > + This enables support for Mediatek MT6370 Backlight driver. > > > + It's commonly used to drive the display WLED. There are 4 channels > > > + inside, and each channel supports up to 30mA of current capability > > > + with 2048 current steps in exponential or linear mapping curves. > > > > Does the MT6372 support more steps than this? In other words does it use > > a fourteen bit scale or does it use an 11-bit scale at a different > > register location? > > Hi Daniel, > > Thanks for your reply. > Yes, MT6372 can support 16384 steps and uses a 14-bit scale register > location. But the maximum current of each > channel of MT6372 is the same as MT6370 and MT6371, both 30mA. > The main reason why MT6372 is designed this way is that one of the > customers asked for a more delicate > adjustment of the backlight brightness. But other customers actually > do not have such requirements. > Therefore, we designed it this way for maximum compatibility in software. I don't think that is an acceptable approach for the upstream kernel. To be "compatible" with (broken) software this driver ends up reducing the capability of the upstream kernel to the point it becomes unable to meet requirements for delicate adjustment (requirements that were sufficiently important to change the hardware design so you could meet them). > > > + > > > + This driver can also be built as a module. If so, the module > > > + will be called "mt6370-backlight". > > > + > > > [...] > > > diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c > > > new file mode 100644 > > > index 0000000..ba00a8f > > > --- /dev/null > > > +++ b/drivers/video/backlight/mt6370-backlight.c > > > [...] > > > +static int mt6370_bl_update_status(struct backlight_device *bl_dev) > > > +{ > > > + struct mt6370_priv *priv = bl_get_data(bl_dev); > > > + int brightness = backlight_get_brightness(bl_dev); > > > + unsigned int enable_val; > > > + u8 brightness_val[2]; > > > + int ret; > > > + > > > + if (brightness) { > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > > > + > > > + /* > > > + * To make MT6372 using 14 bits to control the brightness > > > + * backward compatible with 11 bits brightness control > > > + * (like MT6370 and MT6371 do), we left shift the value > > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > > + * backlight brightness will be almost the same as MT6370's > > > + * and MT6371's. > > > + */ > > > + if (priv->vid_type == MT6370_VID_6372) { > > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > > + } > > > > This somewhat depends on the answer to the first question above, but > > what is the point of this shifting? If the range is 14-bit then the > > driver should set max_brightness to 16384 and present the full range of > > the MT6372 to the user. > > So should we make all 16384 steps of MT6372 available to users? Yes. > Does that mean the DTS needs to be modified as well? Yes... the property to set initial brightness needs a 14-bit range. It would also be a good idea to discuss with the DT maintainers whether you should introduce a second compatible string (ending 6372) in order to allow the DT validation checks to detect accidental use of MT6372 ranges on MT6370 hardware. > Or, for the reasons, I have just explained (just one customer has this > requirement), then we do not make any changes for compatibility > reasons? I'd be curious what the compatiblity reasons are. In other words what software breaks? Normally the userspace backlight code reads the max_brightness property and configures things accordingly (and therefore if you the component that breaks is something like an Android HAL then fix the HAL instead). 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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B6A7FC43334 for ; Tue, 26 Jul 2022 09:32:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=YCUy6Tqbuz5TGk+Z8k+3TZ84BdOKVSysgrHuJYazfb4=; b=njG66JvOq9cQIg 9+Au8Fu2rm9gwywJKAfjjTo1bk2QyPF9HN5X7bR8qiNOc/Ak61G5tDKVnhS6gVm8wP2bI5ZKdCSG5 GJ7kCeAHdWjDxsThcwFzjMoH6pYieFhuE67bbX0YPBoE/VtnOLnTLkgqIUNMPVFAfJ7E2hI5UVyy3 7Ecp0W/6aHQaoJ4DID4pphxyxa8p6NCjUxS1sOFETZLAGEe5nre++hGMvbQs56qISFDq/kk06ZWss +3wbo3I8T3JnNliglSIBPzTcgvoQ77XNueXsnJ5jkFqMXLJf7hWZMQSjiE3u9c6IdKh8WuI16ZVpp Dx5ANdyDA+d0WFGQCqSw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oGGuD-00BSWT-P1; Tue, 26 Jul 2022 09:31:09 +0000 Received: from mail-wr1-x42c.google.com ([2a00:1450:4864:20::42c]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oGGu8-00BSK4-Qu for linux-arm-kernel@lists.infradead.org; Tue, 26 Jul 2022 09:31:06 +0000 Received: by mail-wr1-x42c.google.com with SMTP id v13so11539923wru.12 for ; Tue, 26 Jul 2022 02:31:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=6DKLKhCuv0oTIK2mIKjow1hORCEme2MquQ5buN/wlGc=; b=vwxd0FJzG20ErAwFpJwytDA+XPSph7dO2hF3++04iGbvWolycV3nT1x50PhyF4yHVv t1GLnxbnwGZ+FzFIlfK797RF6mQyiWslVMWv1aGlCeqirOa69WQl16hKksjuQVfFBH02 f5piNzeRWcNmB1Tskni+qHzA/ciJm1Nhz77bR7dqguiDaWeWbf4VGMiBpKBfhuOnl6Nu VHMCRnErCFSCik4l0zZGsC6NRnN7/mU95iBQp/fRhvvTk0HgHXm+PjCUenlLDLOJL3dZ iGRwb4BOjNyYMLApd/GQSMApO5uqSogxp5chUadtE3p2WwR/0f3m4A0tA6FeBBrLQJJw PyxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=6DKLKhCuv0oTIK2mIKjow1hORCEme2MquQ5buN/wlGc=; b=g04WTKOcInbkefS2OQAVqI3VxuFExrOQanMeh1AFS0gs83MhhukopHA0fDdx8Sty3r hWTwm2A5BbWyOYb5NFqfoZipqNxguSuBHa2bqtCckm43GS2/OaJ3D5zY1Rs+iXoYoxfL St63mrxSPapS5QblYY9Al8wOu/eg7A02XEu86uLTOD+zC3TBky2UpXrLF96b4pEAVC6Y +M/1H+NBmuY0CeILBo0z85kDxypX04sLD0+f6IClIp0qp3UU/17fOVrMWBHKcr89FL5j AjDwv3T848PPkD/GXtBAIjBlrl6lSfPUmP6r9Zn59H9FmUMrTIQW2YELFQLdW0qYauU/ 0wnQ== X-Gm-Message-State: AJIora8lIBjhmnB003QkgKDmw9bs6vxyfVo7Zh0dqO2cgDvRXLPwbpPf lcx4P6sciA8vhoGp+A7h4ls+rA== X-Google-Smtp-Source: AGRyM1upJ6Koxcaxp7uEGgh1VKOfPB35WonHb+9rPNcP2pyI1+sRFKmJXTmiMFmK1V6L3XLfn6REKg== X-Received: by 2002:adf:db8e:0:b0:21e:3fff:6bae with SMTP id u14-20020adfdb8e000000b0021e3fff6baemr9890443wri.184.1658827861173; Tue, 26 Jul 2022 02:31:01 -0700 (PDT) Received: from maple.lan (cpc141216-aztw34-2-0-cust174.18-1.cable.virginm.net. [80.7.220.175]) by smtp.gmail.com with ESMTPSA id o15-20020a05600c510f00b003a30fbde91dsm23407540wms.20.2022.07.26.02.30.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Jul 2022 02:31:00 -0700 (PDT) Date: Tue, 26 Jul 2022 10:30:58 +0100 From: Daniel Thompson To: ChiaEn Wu Cc: Lee Jones , Jingoo Han , Pavel Machek , Rob Herring , Krzysztof Kozlowski , Matthias Brugger , Sebastian Reichel , Chunfeng Yun , Greg Kroah-Hartman , Jonathan Cameron , Lars-Peter Clausen , Liam Girdwood , Mark Brown , Guenter Roeck , "Krogerus, Heikki" , Helge Deller , Andy Shevchenko , ChiaEn Wu , Alice Chen , ChiYuan Huang , dri-devel , Linux LED Subsystem , devicetree , linux-arm Mailing List , "moderated list:ARM/Mediatek SoC support" , Linux Kernel Mailing List , Linux PM , USB , linux-iio , "open list:FRAMEBUFFER LAYER" , szuni chen Subject: Re: [PATCH v6 13/13] video: backlight: mt6370: Add MediaTek MT6370 support Message-ID: <20220726093058.2fz2p2vg7xpfsnfe@maple.lan> References: <20220722102407.2205-1-peterwu.pub@gmail.com> <20220722102407.2205-14-peterwu.pub@gmail.com> <20220725103128.xtaw2c4y5fobowg7@maple.lan> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220726_023105_005730_91E8063B X-CRM114-Status: GOOD ( 51.33 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jul 26, 2022 at 10:20:02AM +0800, ChiaEn Wu wrote: > On Mon, Jul 25, 2022 at 6:31 PM Daniel Thompson > wrote: > > > > On Fri, Jul 22, 2022 at 06:24:07PM +0800, ChiaEn Wu wrote: > > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > > > index a003e02..846dbe7 100644 > > > --- a/drivers/video/backlight/Kconfig > > > +++ b/drivers/video/backlight/Kconfig > > > @@ -268,6 +268,18 @@ config BACKLIGHT_MAX8925 > > > If you have a LCD backlight connected to the WLED output of MAX8925 > > > WLED output, say Y here to enable this driver. > > > > > > +config BACKLIGHT_MT6370 > > > + tristate "MediaTek MT6370 Backlight Driver" > > > + depends on MFD_MT6370 > > > + help > > > + This enables support for Mediatek MT6370 Backlight driver. > > > + It's commonly used to drive the display WLED. There are 4 channels > > > + inside, and each channel supports up to 30mA of current capability > > > + with 2048 current steps in exponential or linear mapping curves. > > > > Does the MT6372 support more steps than this? In other words does it use > > a fourteen bit scale or does it use an 11-bit scale at a different > > register location? > > Hi Daniel, > > Thanks for your reply. > Yes, MT6372 can support 16384 steps and uses a 14-bit scale register > location. But the maximum current of each > channel of MT6372 is the same as MT6370 and MT6371, both 30mA. > The main reason why MT6372 is designed this way is that one of the > customers asked for a more delicate > adjustment of the backlight brightness. But other customers actually > do not have such requirements. > Therefore, we designed it this way for maximum compatibility in software. I don't think that is an acceptable approach for the upstream kernel. To be "compatible" with (broken) software this driver ends up reducing the capability of the upstream kernel to the point it becomes unable to meet requirements for delicate adjustment (requirements that were sufficiently important to change the hardware design so you could meet them). > > > + > > > + This driver can also be built as a module. If so, the module > > > + will be called "mt6370-backlight". > > > + > > > [...] > > > diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c > > > new file mode 100644 > > > index 0000000..ba00a8f > > > --- /dev/null > > > +++ b/drivers/video/backlight/mt6370-backlight.c > > > [...] > > > +static int mt6370_bl_update_status(struct backlight_device *bl_dev) > > > +{ > > > + struct mt6370_priv *priv = bl_get_data(bl_dev); > > > + int brightness = backlight_get_brightness(bl_dev); > > > + unsigned int enable_val; > > > + u8 brightness_val[2]; > > > + int ret; > > > + > > > + if (brightness) { > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > > > + > > > + /* > > > + * To make MT6372 using 14 bits to control the brightness > > > + * backward compatible with 11 bits brightness control > > > + * (like MT6370 and MT6371 do), we left shift the value > > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > > + * backlight brightness will be almost the same as MT6370's > > > + * and MT6371's. > > > + */ > > > + if (priv->vid_type == MT6370_VID_6372) { > > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > > + } > > > > This somewhat depends on the answer to the first question above, but > > what is the point of this shifting? If the range is 14-bit then the > > driver should set max_brightness to 16384 and present the full range of > > the MT6372 to the user. > > So should we make all 16384 steps of MT6372 available to users? Yes. > Does that mean the DTS needs to be modified as well? Yes... the property to set initial brightness needs a 14-bit range. It would also be a good idea to discuss with the DT maintainers whether you should introduce a second compatible string (ending 6372) in order to allow the DT validation checks to detect accidental use of MT6372 ranges on MT6370 hardware. > Or, for the reasons, I have just explained (just one customer has this > requirement), then we do not make any changes for compatibility > reasons? I'd be curious what the compatiblity reasons are. In other words what software breaks? Normally the userspace backlight code reads the max_brightness property and configures things accordingly (and therefore if you the component that breaks is something like an Android HAL then fix the HAL instead). Daniel. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel