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 AEF90CCA48F for ; Tue, 26 Jul 2022 11:29:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238782AbiGZL32 (ORCPT ); Tue, 26 Jul 2022 07:29:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41688 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238584AbiGZL30 (ORCPT ); Tue, 26 Jul 2022 07:29:26 -0400 Received: from mail-qk1-x734.google.com (mail-qk1-x734.google.com [IPv6:2607:f8b0:4864:20::734]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6E91E3246D; Tue, 26 Jul 2022 04:29:25 -0700 (PDT) Received: by mail-qk1-x734.google.com with SMTP id o1so10659943qkg.9; Tue, 26 Jul 2022 04:29:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XmalPe35Kd6mBY2zrJx1/gBiFU0BeHwwnpYA4t5eT1A=; b=A+GVbwXYOQP9Ox0/Kg5yJeoRW3oXpzMBaZl6QHpaFwLO6M1T19td44HKBJCw3Vx3Ve ktmd1UtTbJD+BAaE6zqsTfR3VrFzvGc1ycrJ+1lK2/hvc3kasb+7PiA3SElolBBnXFtK qNU1M+I1TW7DWOgloMTsxyAJYT/wMoGf+hgjJGl9GvTc59XZdmliUpeVii4f7rRJUKci 5+dm/XriHCRguOo9xXqJD+t0HnoByVTO++9VWPfq2ouA2m5kGH3x5qWmIItqJ8Gb6hmg QDG8W9CtjH9uOf16p6XpJYeR5nrHxBx+z15ijLhKIUSlUrs9jUJNTlakGqIxckxJcedo 9gMg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=XmalPe35Kd6mBY2zrJx1/gBiFU0BeHwwnpYA4t5eT1A=; b=oO2/ndam3ttxxsTEDGx9amCTaYgdsnJ98QE/d8sKqQBOt9OOXcxIG4AQVTjNLUmlVo NKwVb9/HpsPCcr4SnMABbXUA9b2TEtKRSH4273brRHuofIQOqFg2Yp1n/bAzBQXN8fJs tS3Un33ZyKk4YHEIPB5bqsrS+iII6nF3Pzke+LclgInSC08jfFn6bWJQITVgCaeNVdUQ 3MFmxN/2pWI2TV2pfhnZpe/CSnQjGDAmrogkGRcB6elQ0B9zhwEwx8+bDhj8qNsojY// EeDpNiUAuu7k7veLnFzaukZ8D7ZJEM4YfuOFJXeAVEnX91LXtZl2/VG1gaa0ZIDLA0bw 1uPw== X-Gm-Message-State: AJIora/ZA3h3o+2pGxSUAjywoMT4zQXEnAlXcn10V01TJ3i4qAbf4yq5 lUXhrUYT3sXqMJqK6hJkGdavtKUpYv/Y5qXYTUY= X-Google-Smtp-Source: AGRyM1si+IPV1ALNFzlmWfFOBqoGQxrsIfnktGjjWvpLW/pQqJdQWp+MkeEPkSRN7kGJUl8EvpjK1o1GylYFhHZ2kkc= X-Received: by 2002:a05:620a:2942:b0:6b5:e33a:1771 with SMTP id n2-20020a05620a294200b006b5e33a1771mr11971449qkp.665.1658834964512; Tue, 26 Jul 2022 04:29:24 -0700 (PDT) MIME-Version: 1.0 References: <20220722102407.2205-1-peterwu.pub@gmail.com> <20220722102407.2205-14-peterwu.pub@gmail.com> <20220725103128.xtaw2c4y5fobowg7@maple.lan> <20220726093058.2fz2p2vg7xpfsnfe@maple.lan> In-Reply-To: <20220726093058.2fz2p2vg7xpfsnfe@maple.lan> From: ChiaEn Wu Date: Tue, 26 Jul 2022 19:28:48 +0800 Message-ID: Subject: Re: [PATCH v6 13/13] video: backlight: mt6370: Add MediaTek MT6370 support To: Daniel Thompson 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 Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-leds@vger.kernel.org On Tue, Jul 26, 2022 at 5:31 PM Daniel Thompson wrote: ... > > > 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. Sorry for I used of the wrong word, I mean is 'driver', not higher-level 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). Originally, we just wanted to use one version of the driver to cover all the SubPMIC of the 6370 series(6370~6372). And, the users who use this series SubPMIC can directly apply this driver to drive the backlight device without knowing the underlying hardware. To achieve this goal, we have designed it to look like this. > > ... > > > > + > > > > + 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. hmmm... I have just thought about it, maybe I can just modify the maximum value of default-brightness and max-brightness in DT to 16384, modify the description and add some comments. And then on the driver side, we can use mt6370_check_vendor_info( ) to determine whether it is MT6372. If no, then in mt6370_bl_update_status(), first brightness_val / 8 and then set. In mt6370_bl_get_brightness(), first brightness_val * 8 and then return; If I do this change, does this meet your requirements? > > > > 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? The reason is as above. We just hope the users who use this series SubPMIC can directly apply this driver to drive the backlight device without knowing the underlying hardware. Not software breaks. Thanks! > > 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. -- Best Regards, ChiaEn Wu 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 64D4FC43334 for ; Tue, 26 Jul 2022 11:29:28 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 643328D212; Tue, 26 Jul 2022 11:29:27 +0000 (UTC) Received: from mail-qk1-x735.google.com (mail-qk1-x735.google.com [IPv6:2607:f8b0:4864:20::735]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7AE6E8D213 for ; Tue, 26 Jul 2022 11:29:25 +0000 (UTC) Received: by mail-qk1-x735.google.com with SMTP id e16so10674243qka.5 for ; Tue, 26 Jul 2022 04:29:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XmalPe35Kd6mBY2zrJx1/gBiFU0BeHwwnpYA4t5eT1A=; b=A+GVbwXYOQP9Ox0/Kg5yJeoRW3oXpzMBaZl6QHpaFwLO6M1T19td44HKBJCw3Vx3Ve ktmd1UtTbJD+BAaE6zqsTfR3VrFzvGc1ycrJ+1lK2/hvc3kasb+7PiA3SElolBBnXFtK qNU1M+I1TW7DWOgloMTsxyAJYT/wMoGf+hgjJGl9GvTc59XZdmliUpeVii4f7rRJUKci 5+dm/XriHCRguOo9xXqJD+t0HnoByVTO++9VWPfq2ouA2m5kGH3x5qWmIItqJ8Gb6hmg QDG8W9CtjH9uOf16p6XpJYeR5nrHxBx+z15ijLhKIUSlUrs9jUJNTlakGqIxckxJcedo 9gMg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=XmalPe35Kd6mBY2zrJx1/gBiFU0BeHwwnpYA4t5eT1A=; b=vVu+G5TJRwl4G4WA0Ce1q2kCKQnE65rMReZwjhgD8J6HbldlWLMXk3hu+ScMqdW1sC PDDBb/HnKbknqXR+XlLNPbtVdGYpAxOVYM4P7f/U9AXjedSUXiyoKMrUy2itvF4vVqrX +xpz7i9HIyAYWHaC8jli9SEr93P/cF2iA1mQFpZr+sw0HTGJND5ArWJA4dXyOulI51Tm eP1ujJkQX8oCsE2+1JjgCLGfUhpDtweo3yuP+R33cYwiPtbGJUoT9yEVlxmz/SwXJBNj 1SZ4FcCrtd7icdP0wFw3fzuHFNVo6DNnXNrgkVWOSzdhAIyk5vT11bQ6Zv+ulJ5ddlE5 vfOA== X-Gm-Message-State: AJIora8SxMY+ddkMGRXuPyR+jV+Ou8MQPUSPrkBQaAnFXPwymgORwIql HB0NwHacsPIxdsFzBw5t/nqY4GII97DzIPNRcLY= X-Google-Smtp-Source: AGRyM1si+IPV1ALNFzlmWfFOBqoGQxrsIfnktGjjWvpLW/pQqJdQWp+MkeEPkSRN7kGJUl8EvpjK1o1GylYFhHZ2kkc= X-Received: by 2002:a05:620a:2942:b0:6b5:e33a:1771 with SMTP id n2-20020a05620a294200b006b5e33a1771mr11971449qkp.665.1658834964512; Tue, 26 Jul 2022 04:29:24 -0700 (PDT) MIME-Version: 1.0 References: <20220722102407.2205-1-peterwu.pub@gmail.com> <20220722102407.2205-14-peterwu.pub@gmail.com> <20220725103128.xtaw2c4y5fobowg7@maple.lan> <20220726093058.2fz2p2vg7xpfsnfe@maple.lan> In-Reply-To: <20220726093058.2fz2p2vg7xpfsnfe@maple.lan> From: ChiaEn Wu Date: Tue, 26 Jul 2022 19:28:48 +0800 Message-ID: Subject: Re: [PATCH v6 13/13] video: backlight: mt6370: Add MediaTek MT6370 support To: Daniel Thompson Content-Type: text/plain; charset="UTF-8" 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 5:31 PM Daniel Thompson wrote: ... > > > 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. Sorry for I used of the wrong word, I mean is 'driver', not higher-level 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). Originally, we just wanted to use one version of the driver to cover all the SubPMIC of the 6370 series(6370~6372). And, the users who use this series SubPMIC can directly apply this driver to drive the backlight device without knowing the underlying hardware. To achieve this goal, we have designed it to look like this. > > ... > > > > + > > > > + 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. hmmm... I have just thought about it, maybe I can just modify the maximum value of default-brightness and max-brightness in DT to 16384, modify the description and add some comments. And then on the driver side, we can use mt6370_check_vendor_info( ) to determine whether it is MT6372. If no, then in mt6370_bl_update_status(), first brightness_val / 8 and then set. In mt6370_bl_get_brightness(), first brightness_val * 8 and then return; If I do this change, does this meet your requirements? > > > > 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? The reason is as above. We just hope the users who use this series SubPMIC can directly apply this driver to drive the backlight device without knowing the underlying hardware. Not software breaks. Thanks! > > 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. -- Best Regards, ChiaEn Wu 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 96751C433EF for ; Tue, 26 Jul 2022 11:30:38 +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:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=LAjIqOSHAKPVLo85tCnrciLua4KdR2kovy6dqaBrQg4=; b=cFJK0kNn7W2JzO 4hVnqhXDXnD3r9tLLTaivISFKSL/SZ+L0dm/bcod3hqubuyvmGIr/vx6/CjJv1mJsVgQ/WKLEqkVZ 8/+EKaRNzOrmbjFAexXE4v/fSEHC0cApZujt6EwKGttqEORG39lUOyO0S3BtZNb7dgGpDsP+v+n5Y l5Olv5TPOgomfVS/pTxVIweEGTUNOUkuSgKCULvJGP4ZGcI7jGhej5nGNMJ0odq0V8KX9GIjyHUGS Dw19rjFGQXMmRI/Qo2a5/1gNLz56fpvEiOJ0iixWRSdjkeyid3cVUHuoxj5ax6IOjcS57jq20RI/b 84kERUALuhT/0gXJ2jTA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oGIkk-00Fon2-Ik; Tue, 26 Jul 2022 11:29:30 +0000 Received: from mail-qk1-x72f.google.com ([2607:f8b0:4864:20::72f]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oGIkh-00Fodh-ER; Tue, 26 Jul 2022 11:29:28 +0000 Received: by mail-qk1-x72f.google.com with SMTP id m16so10668942qka.12; Tue, 26 Jul 2022 04:29:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XmalPe35Kd6mBY2zrJx1/gBiFU0BeHwwnpYA4t5eT1A=; b=A+GVbwXYOQP9Ox0/Kg5yJeoRW3oXpzMBaZl6QHpaFwLO6M1T19td44HKBJCw3Vx3Ve ktmd1UtTbJD+BAaE6zqsTfR3VrFzvGc1ycrJ+1lK2/hvc3kasb+7PiA3SElolBBnXFtK qNU1M+I1TW7DWOgloMTsxyAJYT/wMoGf+hgjJGl9GvTc59XZdmliUpeVii4f7rRJUKci 5+dm/XriHCRguOo9xXqJD+t0HnoByVTO++9VWPfq2ouA2m5kGH3x5qWmIItqJ8Gb6hmg QDG8W9CtjH9uOf16p6XpJYeR5nrHxBx+z15ijLhKIUSlUrs9jUJNTlakGqIxckxJcedo 9gMg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=XmalPe35Kd6mBY2zrJx1/gBiFU0BeHwwnpYA4t5eT1A=; b=FiJBGSUtw4hgutT6YEHBy1vxdZTIfDRfBpPaEl1I754csiZSTlR8tTIZ2VwKuGSVyl pAH9gQQfZWKEbjRCPXwmRR0C9Df9WvrvwfzXvEsvB3wYJkTRjJLOoBqzq4eC9JbSUy+D sobv/0I0PmknH2HyubLfqg29RGrl5AghHcqt6eVQCgIR7p3mlIcDasADv+rrjApmJFQb ipyEHksVA+Cv+++x/JfOdOEsnR7IrXgh6Sm2mcvfnD/gJoNJWW6VfSPoPW6HTuJI2zSz SAvFXcaQgtninGnRBcup+QYBGA48Eo8pg4pNoPqZPIoSOMj2Y0HajLB+gjCHBs5GPo1N YMAg== X-Gm-Message-State: AJIora8xjx71tt21nwu236BkOAnrSLQFflyO2gLEXOjO7FBL/+MnLfwE MR6KGrbKYEYiN7ZaDXrxf74878ZvmlRrqCYOqJc= X-Google-Smtp-Source: AGRyM1si+IPV1ALNFzlmWfFOBqoGQxrsIfnktGjjWvpLW/pQqJdQWp+MkeEPkSRN7kGJUl8EvpjK1o1GylYFhHZ2kkc= X-Received: by 2002:a05:620a:2942:b0:6b5:e33a:1771 with SMTP id n2-20020a05620a294200b006b5e33a1771mr11971449qkp.665.1658834964512; Tue, 26 Jul 2022 04:29:24 -0700 (PDT) MIME-Version: 1.0 References: <20220722102407.2205-1-peterwu.pub@gmail.com> <20220722102407.2205-14-peterwu.pub@gmail.com> <20220725103128.xtaw2c4y5fobowg7@maple.lan> <20220726093058.2fz2p2vg7xpfsnfe@maple.lan> In-Reply-To: <20220726093058.2fz2p2vg7xpfsnfe@maple.lan> From: ChiaEn Wu Date: Tue, 26 Jul 2022 19:28:48 +0800 Message-ID: Subject: Re: [PATCH v6 13/13] video: backlight: mt6370: Add MediaTek MT6370 support To: Daniel Thompson 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 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220726_042927_532216_88542DF0 X-CRM114-Status: GOOD ( 49.51 ) 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 5:31 PM Daniel Thompson wrote: ... > > > 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. Sorry for I used of the wrong word, I mean is 'driver', not higher-level 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). Originally, we just wanted to use one version of the driver to cover all the SubPMIC of the 6370 series(6370~6372). And, the users who use this series SubPMIC can directly apply this driver to drive the backlight device without knowing the underlying hardware. To achieve this goal, we have designed it to look like this. > > ... > > > > + > > > > + 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. hmmm... I have just thought about it, maybe I can just modify the maximum value of default-brightness and max-brightness in DT to 16384, modify the description and add some comments. And then on the driver side, we can use mt6370_check_vendor_info( ) to determine whether it is MT6372. If no, then in mt6370_bl_update_status(), first brightness_val / 8 and then set. In mt6370_bl_get_brightness(), first brightness_val * 8 and then return; If I do this change, does this meet your requirements? > > > > 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? The reason is as above. We just hope the users who use this series SubPMIC can directly apply this driver to drive the backlight device without knowing the underlying hardware. Not software breaks. Thanks! > > 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. -- Best Regards, ChiaEn Wu _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel