From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacek Anaszewski Subject: Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Date: Thu, 07 Jul 2016 13:09:13 +0200 Message-ID: <577E3859.1010008@samsung.com> References: <1467129919-27641-1-git-send-email-tony.makkiel@daqri.com> <577BA931.5070403@samsung.com> <577D0869.7090806@samsung.com> <2752967.IJ5pkPUWjW@vostro.rjw.lan> <577E011D.4030503@samsung.com> <577E3062.7050101@daqri.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mailout3.w1.samsung.com ([210.118.77.13]:13305 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750817AbcGGLJS (ORCPT ); Thu, 7 Jul 2016 07:09:18 -0400 In-reply-to: <577E3062.7050101@daqri.com> Sender: linux-leds-owner@vger.kernel.org List-Id: linux-leds@vger.kernel.org To: Tony Cc: "Rafael J. Wysocki" , "Rafael J. Wysocki" , linux-leds@vger.kernel.org, rpurdie@rpsys.net, Len Brown , Mika Westerberg , ACPI Devel Maling List , Al Stone On 07/07/2016 12:35 PM, Tony wrote: > > > On 07/07/16 08:13, Jacek Anaszewski wrote: >> On 07/06/2016 11:15 PM, Rafael J. Wysocki wrote: >>> On Wednesday, July 06, 2016 03:32:25 PM Jacek Anaszewski wrote: >>>> On 07/05/2016 02:33 PM, Jacek Anaszewski wrote: >>>>> On 07/05/2016 02:12 PM, Rafael J. Wysocki wrote: >>>>>> On Tue, Jul 5, 2016 at 12:47 PM, Jacek Anaszewski >>>>>> wrote: >>>>>>> PING >>>>>>> >>>>>>> >>>>>>> On 06/29/2016 12:54 PM, Jacek Anaszewski wrote: >>>>>>>> >>>>>>>> On 06/29/2016 12:07 PM, Tony wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 29/06/16 07:52, Jacek Anaszewski wrote: >>>>>>>>>> >>>>>>>>>> Hi Rafael, >>>>>>>>> >>>>>>>>> >>>>>>>>>>>> +#ifdef CONFIG_ACPI >>>>>>>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = { >>>>>>>>>>>> + {LP3952_ACPI_NAME, 0}, >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> No, you can't use "PRP0001" in this list. >>>>>>>>>>> >>>>>>>>>>>> + {} >>>>>>>>>>>> +}; >>>>>>>>>>>> + >>>>>>>>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match); >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> And you don't need this for the "PRP0001" thing to work. The >>>>>>>>>>> core will >>>>>>>>>>> take care of it for you then. >>>>>>>>>>> >>>>>>>>>>>> +#endif >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> So the entire ACPI block can be dropped for now. >>>>>>>>>>> >>>>>>>>>>> And the driver doesn't have to depend on CONFIG_ACPI any more, >>>>>>>>>>> does it? >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> The driver currently supports probing only with ACPI. >>>>>>>>>> I have one question BTW: isn't there anything similar to the >>>>>>>>>> device tree >>>>>>>>>> bindings documentation required for ACPI overlays? >>>>>>>>>> Pointer to the discussion which led us to this solution: >>>>>>>>>> >>>>>>>>>> http://www.spinics.net/lists/linux-leds/msg06230.html >>>>>>>>>> >>>>>>>>> _DSD is working now. I managed to get "PRP0001" working as >>>>>>>>> suggested by >>>>>>>>> Rafael in >>>>>>>>> http://marc.info/?l=linux-acpi&m=146711623115228&w=2 >>>>>>>>> with _DSD >>>>>>>> >>>>>>>> >>>>>>>> Thanks for the link. >>>>>>>> >>>>>>>> Rafael, "Package" entries seem to mimic Device Tree properties >>>>>>>> defined >>>>>>>> in the common leds bindings. Would it be possible to make it even >>>>>>>> more compatible and define every LED connected to the LED >>>>>>>> controller >>>>>>>> in the form of a child node, similarly as in case of LED DT >>>>>>>> bindings? >>>>>>>> >>>>>>>> See Documentation/devicetree/bindings/leds/common.txt and other >>>>>>>> bindings in Documentation/devicetree/bindings/leds. >>>>>> >>>>>> I'm not sure what you mean. >>>>>> >>>>>> If somebody decides to arrange the data in their ACPI tables to >>>>>> follow >>>>>> that scheme, it will just work IMO. >>>>>> >>>>>> Is there anything more that needs to be done in the kernel here? >>>>> >>>>> In case of Device Tree based platforms it is customary to define each >>>>> LED as a child node of the LED controller node. LED names can then >>>>> be obtained from the 'label' property or DT node name. >>>>> Tony has encountered some issues [1] while trying to follow this >>>>> pattern. Tony, could you please double check that? >>>>> >>>>> [1] http://www.spinics.net/lists/linux-leds/msg06230.html >>>> >>>> One more question: now anyone wanting to use the driver would have >>>> to investigate the code to infer what DSD properties need to be >>>> provided for it. Isn't there anything similar to >>>> Documentation/devicetree/bindings/* documentation required in case of >>>> DSD? >>> >>> There is a plan to set up a database for these, but that's still >>> under discussion (Al Stone has posted some related documentation for >>> comments recently). >>> >>>> I've also come across your presentation [1], which describes the >>>> device_property* API for parsing unified DT/ACPI device properties. >>>> IMO it would be good solution for this driver as the device seems not >>>> to be tightly coupled with only ACPI platforms. >>>> >>>> >>>> [1] >>>> http://events.linuxfoundation.org/sites/events/files/slides/ACPI_vs_DT.pdf >>>> >>>> >>> >>> The device_property* API should be used everywhere where (a) >>> properties are >>> used and (b) the code has no specific need to use a particular platform >>> firmware interface (ACPI or DT), so I agree. >> >> Tony, could you try switching to device_property* API and make >> the DSD properties as much compatible with Device Tree ones as possible? >> > Sure will replace 'acpi_dev_get_property' with > 'device_property_read_string'. > > I am new to both ACPI and Device Tree. So not sure whether there is any > naming convention to follow for led name handles in device tree. You can follow existing bindings in Documentation/devicetree/bindings /leds. > review comments for v8 patch, it was suggested to use "reg" property. > We shall continue the discussion on led name handles, on that > thread to avoid duplication. -- Best regards, Jacek Anaszewski