From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752013AbcGMHpd (ORCPT ); Wed, 13 Jul 2016 03:45:33 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:10377 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751117AbcGMHp0 (ORCPT ); Wed, 13 Jul 2016 03:45:26 -0400 X-AuditID: cbfec7f4-f796c6d000001486-67-5785f17fca43 Message-id: <5785F17D.3010108@samsung.com> Date: Wed, 13 Jul 2016 09:45:01 +0200 From: Jacek Anaszewski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 MIME-version: 1.0 To: "H. Nikolaus Schaller" Cc: Jacek Anaszewski , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Richard Purdie , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org, kernel@pyra-handheld.com, marek@goldelico.com, letux-kernel@openphoenux.org, Andrey Utkin Subject: Re: [PATCH v3 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver References: <524f16ecba240bacf57924f43ec38404cfdcde8f.1468007377.git.hns@goldelico.com> <57854FB0.1080605@gmail.com> <55F90A44-0B60-4342-8143-56F85EA24772@goldelico.com> In-reply-to: <55F90A44-0B60-4342-8143-56F85EA24772@goldelico.com> Content-type: text/plain; charset=windows-1252; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrEIsWRmVeSWpSXmKPExsVy+t/xq7r1H1vDDXZOZ7P4/+0ai8X8I+dY LfrfLGS1+LHtK5PFuVcrGS1ub93AYnHpa43F1j+X2Cwu75rDZrH1zTpGi39Lt7BZLL1+kcli wvS1LBate4+wW+ze9ZTVgd9jzbw1jB6X+3qZPBa9nM/usXPWXXaPNe9PMXusXP6FzWPTqk42 j5ZJu9g9vrQ0M3vsmf+D1ePzJrkA7igum5TUnMyy1CJ9uwSujFMnGhgLXkpW/Nz6k6mB8ZZI FyMnh4SAicS/Y++YIWwxiQv31rN1MXJxCAksZZSY/PgblPOMUaKtbxYbSBWvgJbEt/O/wWwW AVWJc7fngnWzCRhK/HzxmgnEFhWIkPhzeh8rRL2gxI/J91hAbBEBPYnO739YQIYyC7xkltjw dBqQw8EhLBAvceBYHcSyG4wS72acAmvgFHCU+PfuBtggZgFbiQXv17FA2PISm9e8ZZ7AKDAL yY5ZSMpmISlbwMi8ilE0tTS5oDgpPddQrzgxt7g0L10vOT93EyMk5r7sYFx8zOoQowAHoxIP L6NIa7gQa2JZcWXuIUYJDmYlEd5z74FCvCmJlVWpRfnxRaU5qcWHGKU5WJTEeefueh8iJJCe WJKanZpakFoEk2Xi4JRqYFzc0rudW+Cp0qtX2dd2RL25e3aeyDWLwwv5Tm2R2HjrneZjdc6p H+0j9BU7tq7b837xVpnqDWFc/O+nXpstwlCi/6f76+zT/HIxCV0TZsxa8p9XLfjA19/qjXde LH6wzrS9rrmyaOKmbYa3eqR4j0xY/u/nnT6R8t+3G4TvckQVv7z87r2XaXOTEktxRqKhFnNR cSIA/3pBfbUCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Nikolaus, On 07/13/2016 08:09 AM, H. Nikolaus Schaller wrote: [...] >>> + /* >>> + * Kernel conventions require per-LED led-max-microamp property. >>> + * But the chip does not allow to limit individual LEDs. >>> + * So we take minimum from all subnodes. >> >> Why minimum? Choose maximum and reduce max_brightness properties >> of the sub-LEDs with lesser led-max-microamp. > > Hm. Is this really the correct way to handle it? > > Assume you connect an LED which is specified with 10mA peak current. > And another with 20mA peak current. > > So you define led-max-microamp in the DT as 10 mA and 20 mA. > > Firstly a user can set the brightness only to 50% of LED_FULL since it is limited > by a reduced max_brightness. And heshe finds that not all LEDs have the same > max_brightness. The first LED will have 127 and the second one 255 for reasons > not directly understandable. You have to know that led-max-microamp property was introduced to standarize Device Tree definition of maximum brightness allowed for a sub-LED. Earlier people defined max-brightness property in DT, but at some point we realized that brightness level is not a proper unit for describing a hardware property. It was not global max-microamp setting that appears in recent LED controllers that drove the introduction of led-max-microamp property. If you question the idea of having different maximum brightness per sub-LEDs controlled by the same device, then it means that you have objections to the entire idea of LED subsystem max_brightness property, whereas it has been broadly accepted and successfully used for years. > This entangles "brightness" with "max-current" - which are IMHO two independent > things. The fact that recent LED controllers use the same notion for one of its settings shouldn't mislead us. > Next, this will set the hardware limit to 20 mA. So there will be current peaks > of 20 mA for an LED where the DT developer thinks to have specified a led-max-microamp > of 10 mA. So you run the LED outside of its specs although the DT seems to > tell that it is inside and user-space thinks it is ok. This will reduce lifetime of LEDs. In what circumstances would such current peaks occur? On reset? Shouldn't such a device be considered as broken by design? What if all LEDs would have low amperage? Would there be some way to protect them from being damaged by current peaks? > So either "led-max-microamp" is the wrong name for this property (could better > be "led-max-average-microamp") or the whole logic is broken. > > This is why we hesitate to hide (or even disable because you can't set the limit > to 10mA by DT) the current limiting chip feature by such a difficult to understand > automatic feature. > > Using the minimum of all led-max-microamp keeps everything on the safe > side, running some LEDs with less current than specified. But never outside > of the spec. And all LEDs have the same max_brightness which is IMHO > more intuitive for the user. > > Our original proposal was to define led-max-microamp for the whole chip > instead of individual LEDs, which is IMHO much easier to understand, > because it corresponds one-to-one with the data sheet. -- Best regards, Jacek Anaszewski