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