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=-1.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS autolearn=ham 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 9099FC636A2 for ; Sun, 20 Jan 2019 15:32:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 42EC92087B for ; Sun, 20 Jan 2019 15:32:37 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="QOuBN/9N" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726678AbfATPcc (ORCPT ); Sun, 20 Jan 2019 10:32:32 -0500 Received: from mail-lf1-f67.google.com ([209.85.167.67]:35597 "EHLO mail-lf1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725977AbfATPcb (ORCPT ); Sun, 20 Jan 2019 10:32:31 -0500 Received: by mail-lf1-f67.google.com with SMTP id e26so13745233lfc.2; Sun, 20 Jan 2019 07:32:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=jxeAlVijiaP+HRVE3CNUhzoNrwHZKtvDOFAIaMDxWe0=; b=QOuBN/9NlPoVLE2Q92qgAuF0ctdlpdCUvMnamHmCkbE9RakIcBnB2tozPT2riIeumY a3JDMNNZGi1Wi3VFizjfXLqoNeG1CqUsnN/ZxglR7bRJFXwyahAtoBDoeiY1AJWetDBB D2LHuK4zPm7EQRYbNpThap6D47/MVvGDoERyKofzfyYSysLWyVj+PZEbhycDYOuanpy6 xyDv9DtA4tIjiQ21eTeuzhgD4YhrWU+qpz5M6DqtYrQJEOquC/2DWGMSfXXacA4K16IU KNIGwrC4hDkhv+on2Pmvds6eM2cV7bFGor4fgx2Tm0I6JjtDrnW7EJ92R3j62lopMxGg FRLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=jxeAlVijiaP+HRVE3CNUhzoNrwHZKtvDOFAIaMDxWe0=; b=Sl2WC7vgqfUoVAISI2nwlnRE4VO8jwATOOvSqrdAtoCpLD+hjRaxVw146X2yQwSMCP 3AWEE4qBWnBKAdDAUR5LtpN/U7GS4SPRPF6DhU65G2yQxP2TtcMpdVMzsFqfjxS24QXQ 8Zl2hDqgmCOnt4UXKL0NuMXz5j++BRB3O0ZzJNkImRCFhWw39UWqdf1wb+nZ528xpZG9 pAvC3xyZbjqhPmk7OXwRxNw3RHJuLY6siefhX5GwdavAqW0YJ/YNE/r/r9GGQLBT3NCH W0eUzXXJ+BpHkNNO4JT7J4BB6O1LdChfSBK+ZbYuSE8NXyT3UVwFd1eG72Lsv3RIvUmi eeHQ== X-Gm-Message-State: AJcUukc46Yfx5jTdjTbq45qBqKOXfVlhwFNB6fBf0merCIbvhJ1AqV7+ GoMKmU9J16CubzsBmae2YhE= X-Google-Smtp-Source: ALg8bN4KZQtygwxFSFatLuWBMPJAEVDZsguK3Jhtd9hDWMRngpVVDqW0d085KOpml5Eg0HXUm7kiwA== X-Received: by 2002:a19:9a8c:: with SMTP id c134mr15776319lfe.152.1547998348290; Sun, 20 Jan 2019 07:32:28 -0800 (PST) Received: from [192.168.1.18] (bdu110.neoplus.adsl.tpnet.pl. [83.28.6.110]) by smtp.gmail.com with ESMTPSA id c22sm1783997lfd.88.2019.01.20.07.32.26 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 20 Jan 2019 07:32:27 -0800 (PST) Subject: Re: [PATCH v2 2/2] leds: lp50xx: Add the LP50XX family of the RGB LED driver To: Dan Murphy , Pavel Machek Cc: linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, dachaac@gmail.com, robh+dt@kernel.org References: <20190114211723.11186-1-dmurphy@ti.com> <20190114211723.11186-2-dmurphy@ti.com> <20190115222223.GA17363@amd> <79394d17-3124-75b2-ccac-dc1046499d14@ti.com> <20190116105537.GA1803@amd> <86299268-3202-814a-134b-04bd2170faab@ti.com> <8dfa2854-2814-6874-ab8e-1858e9a18acf@gmail.com> <9f87e1ac-be66-2998-578c-de2c1794a00a@ti.com> From: Jacek Anaszewski Message-ID: <8bd788f2-7e9d-0673-91cc-1d6565dc9446@gmail.com> Date: Sun, 20 Jan 2019 16:32:25 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <9f87e1ac-be66-2998-578c-de2c1794a00a@ti.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Dan, On 1/18/19 2:45 PM, Dan Murphy wrote: > Jacek > > On 1/17/19 3:10 PM, Jacek Anaszewski wrote: >> Hi Dan, >> >> On 1/16/19 7:41 PM, Dan Murphy wrote: >>> Hello >>> >>> On 1/16/19 4:55 AM, Pavel Machek wrote: >>>> Hi! >>>> >>>>> On 1/15/19 4:22 PM, Pavel Machek wrote: >>>>>> Hi! >>>>>> >>>>>>>> +The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB >>>>>>>> +XX - Do not care ignored by the driver >>>>>>>> +RR - is the 8 bit Red LED value >>>>>>>> +GG - is the 8 bit Green LED value >>>>>>>> +BB - is the 8 bit Blue LED value >>>>>>>> + >>>>>>>> +Example: >>>>>>>> +LED module output 4 of the LP5024 will be a yellow color: >>>>>>>> +echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color >>>>>>>> + >>>>>>>> +LED module output 4 of the LP5024 will be dimmed 50%: >>>>>>>> +echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness >>>>>>>> + >>>>>>>> +LED banked RGBs of the LP5036 will be a white color: >>>>>>>> +echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color >>>>>>> >>>>>>> This part with example cans remain in Documentation/leds if you >>>>>>>> like. >>>>>> >>>>>> Does it actually work like that on hardware? >>>>> >>>>> What? >>>> >>>> If you do echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color, >>>> does it actually produce white? With all the different RGB modules >>>> manufacturers can use with lp5024P? >>>> >>>> If you do echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color, does >>>> it actually produce yellow, with all the different RGB modules >>>> manufacturers can use with lp5024P? >>>> >>> >>> I believe the answer to the general questions is no for any RGB cluster and driver out there. >>> Because if you set the same values on each and every RGB device out there you will get varying shades of the color. >>> But for this device yes the color does appear to be yellow to me versus what was displayed on my monitor by the HSL picker. >>> But everyone interprets colors differently. >>> >>> If you write the same value for yellow or white on a droid 4 and the N900 do they produce the same color side by side? >>> Most probably not. >>> >>> As you pointed out the PWM needs to be modified to obtain the correct white color to account for LED and other device constraints. >>> >>> But we need to take into account the light pipe.  Pools nowadays have RGB LED spot lights in them.  It can >>> be set to white.  On my pool right off the lens the color has a purplish hue to it.  As the light is diffracted into >>> the pool the color becomes white.  The pool is clear.  When I add chemicals to the pool and make it cloudy >>> and turn on the lights the color off the lens is now white.  This is an example on a large scale but the issue >>> scales down to the hand helds and smart home applications. >>> >>> If the cluster is piped through a flexible optic 0xffffff may produce the "white" you want on its output. >>> >>> So an expectation of certain color without proper piping based on a single RGB value may be a little unreasonable. >>> There may need to be a way to attenuate the values based on the hardware aspect of the equation ie light pipe (or lack thereof) and LED vendor. >>> So if we write 0xffffff to the RGB driver the driver could adjust the intensity of the individual LEDs based on the diffraction >>> coefficients. >>> >>> I also think that is an unreasonable expectation here that writing a single value to any LED RGB driver would produce >>> a "rest of the world" absolute color.  Maybe it can produce something similar but not identical. >>> As you indicated in the requirements there is more involved here then just the LED and the values written. >>> The colors should be close but may not be identical. >>> >>> A 10 year old N900 should not be considered the gold standard for color production due to advancements in LED, >>> light pipe and LED driver technology. >>> The single package RGB clusters on the board I am testing is about the size of a single RGB LED from 10 years ago. >>> >>> I agree that the interface developed should work on the device but the algorithm derived to obtain the color needs to have >>> a hardware aspect to the calculation. >>> >>>>>> Is it supposed to support "normal" RGB colors as seen on monitors? >>>>> >>>>> Monitors are not an application for this part. >>>> >>>> You did not answer the question. When you talk about yellow, is it >>>> same yellow the rest of world talks about? >>>> >>> >>> See above.  It is close to what was on my monitor displayed. >>> >>>>>> Because 100% PWM on all channels does not result in white on hardware >>>>>> I have. >>>>> >>>>> I don't know I am usually blinded by the light and have no diffuser over >>>>> the LEDs to disperse the light so when I look I see all 3 colors. >>>> >>>> How can we have useful discussion about colors when you don't see the >>>> colors? >>>> >>>> Place a piece of paper over the LEDs.... >>>> >>> >>> Good suggestion for a rough test. >>> >>>>>> But... >>>>>> >>>>>> I believe we should have a reasonable design before we do something >>>>>> like this. There's no guarantee someone will not use lp50xx with just >>>>>> the white LEDs for example. How will this work? Plus existing hardware >>>>>> already uses three separate LEDs for RGB LED. Why not provide same >>>>>> interface? >>>>> >>>>> Which existing hardware?  Are they using this part? >>>> >>>> Nokia N900. They are not using this part, but any interface we invent >>>> should work there, too. >>>> >>> >>> Yes a common interface would be nice with some sort of hardware tuning coefficient. >>> >>>>> >>>>> Why are we delaying getting the RGB framework or HSV in? >>>>> I would rather design against something you want instead of having >>>>> everyone complain about every implementation I post. >>>>> >>>> >>>> Because you insist on creating new kernel interfaces, when existing >>>> interfaces work, and are doing that badly. >>>> >>>> Because your patches are of lower quality than is acceptable for linux >>>> kernel. >>>> >>>> Because you don't seem to be reading the emails. >>>> >>>> I sent list of requirements for RGB led support. This does not meet >>>> them. >>>> >>> >>> Sigh.  You did not answer my question. >>> >>> Your requirements seem to be centered around monitors but that is only one application of the current >>> RGB LED landscape. >>> >>> I am willing to work with you on the HSV and adapting the LP50xx part to this framework. >>> Or any RGB framework for that matter.  I still don't agree with the kernel needing to declare colors >>>   maybe color capabilities but not specific colors. >> >> Dan, if you have a bandwidth for LED RGB class implementation >> then please go ahead. It would be good to compare colors produced >> by software HSV->RGB algorithm to what can be achieved with >> LEDn_BRIGHTNESS feature. >> >> The requirements for LED RGB class as I would see it: >> >> sysfs interface: >> >> brightness-model: space separated list of available options: >> - rgb (default): >>   - creates color file with "red green blue" decimal values > > What about other colored LEDs? Presenting RGB for an Amber LED does not seem right. > Should the LED color come from the DT? This is out of this discussion scope. Here we're discussing RGB LEDs. I believe LEDn_BRIGHTNESS of lp50xx would also not work as intended with colors other than RGB. Best regards, Jacek Anaszewski > How to validate that the color is real? > Or do we present a list of possible colors and validate that the color is appropriate? > > I believe this could leverage the work you are doing on the LED label for color. > >>   - creates brightness file >>     a) for devices with hardware support for adjusting color >>            intensity it maps to corresponding register > > If we group LEDs as proposed we will have independent devices that give each LED a separate brightness control register. > We would need to write to each brightness register here. > >>         b) for the rest writing any value greater than 0 will result >>            in setting all color registers to max >> - hsv: >>   - creates color file with "h s v" values - it shall >>     use software HSV->RGB algorithm for setting color registers >> >> - any other custom color ranges defined in DT, but it can be covered >>   later >> - other options? >> >> Best regards, >> Jacek Anaszewski >> >> >>> It was agreed to continue forward with this particular implementation. >>> At least thats what the email (I apparently did not read) stated. >>> >>> I need to fix the code to use the space separated value as pointed out and shown by Vesa. >>> This will map nicely into this device with the color file as what I implemented is in theory >>> they same code except for the space separated values. >>> >>>>> It is not a normal RGB driver.  The device collates the individual RGB >>>>> clusters into a single brightness register and you can modify the intensity of the individual >>>>> LEDs via other registers.  If brightness is 0 then the cluster is OFF regardless of the color >>>>> set in the individual registers. >>>> >>>> I understand that. So just set cluster brightness to 255 and you have >>>> normal RGB driver you can control with existing interfaces. You don't >>>> have to use every feature your hardware has. >>>> >>> >>> The brightness file is available and adjusts the brightness of the RGB cluster. >>> I am not attempting to implement every feature the device has.  But I am attempting to use >>> the basic features that are available and useful. >>> >>>> You did not answer the "what if someone uses this with all white LEDs" >>>> question. >>>> >>> >>> Are you asking what if someone places a white LED instead of a RGB on the hardware? >>> Well then they need to go back and have a review of the data sheet and what they are trying to >>> achieve.  That would be a misapplication of the LED driver itself and something software cannot fix. >>> >>> But if they do determine they want to control these white LEDs with this device >>> then they can ignore the "color" file and control the cluster via the brightness file like we >>> do today.  The color file will only change the intensity of the single output (assuming LED module mode) >>> or the banked output. >>> >>> If a user wants to place a RGB cluster down on the hardware and have white as the consistent color >>> well then that is fine as the RGB outputs are all set to 0xff and the intensity of the cluster is >>> controlled by the brightness file.  If they cannot achieve the "white" with the default settings then >>> on init they can set the color file once to obtain the "white" color and continue to use the brightness >>> file to control the overall brightness of the cluster. >>> >>> It was determined in the email chain not to expose a brightness file per output as this device >>> does not lend itself to that convention. >>> >>>> You know what? First, submit driver with similar functionality to >>>> existing RGB drivers, using same interface existing drivers are >>>> using. When that is accepted, we can talk about extending >>>> kernel<->user interfaces. >>>> >>> >>> I could do that but then there is no way for users to have any other color but "white" with this driver. >>> That defeats the purpose of the device itself. >>> >>> This is why I would rather align the interfaces with what is being proposed so the interfaces won't change only >>> the engine underneath will. >>> >>> I am not sure if you are aware of this or care but I found this recent blog on this effort: >>> https://www.phoronix.com/scan.php?page=news_item&px=Linux-RGB-LED-Interface >>> See some of the comments. >>> >>> Dan >>> >>>> Thanks, >>>>                                     Pavel >>>> >>> >>> >> >> > >