Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3 1/2] leds: pwm: Allow automatic labels for DT based
 devices
To:     Pavel Machek <pavel@ucw.cz>, linux-leds@vger.kernel.org,
        devicetree@vger.kernel.org, Dan Murphy <dmurphy@ti.com>,
        Rob Herring <robh+dt@kernel.org>, linux-kernel@vger.kernel.org,
        Alexander Dahl <ada@thorsis.com>
References: <20200907043459.2961-1-post@lespocky.de>
 <20200907043459.2961-2-post@lespocky.de> <20200909090736.GE10891@amd>
 <20200909202907.3z425uujvu532qxs@falbala.internal.home.lespocky.de>
From:   Jacek Anaszewski <jacek.anaszewski@gmail.com>
Message-ID: <9e78d793-d0d5-737c-163e-30736c1c7bdf@gmail.com>
Date:   Wed, 9 Sep 2020 22:47:46 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <20200909202907.3z425uujvu532qxs@falbala.internal.home.lespocky.de>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Alexander,

On 9/9/20 10:29 PM, Alexander Dahl wrote:
> Hei hei,
> 
> On Wed, Sep 09, 2020 at 11:07:36AM +0200, Pavel Machek wrote:
>> Hi!
>>
>>>   	pwm_init_state(led_data->pwm, &led_data->pwmstate);
>>>   
>>> -	ret = devm_led_classdev_register(dev, &led_data->cdev);
>>> +	if (fwnode) {
>>> +		init_data.fwnode = fwnode;
>>> +		ret = devm_led_classdev_register_ext(dev, &led_data->cdev,
>>> +						     &init_data);
>>> +	} else {
>>> +		ret = devm_led_classdev_register(dev, &led_data->cdev);
>>> +	}
>>
>> Can you always use _ext version, even with null fwnode?
> 
> I did not try on real hardware, but from reading the code I would say
> the following would happen: led_classdev_register_ext() calls
> led_compose_name(parent, init_data, composed_name) which itself calls
> led_parse_fwnode_props(dev, fwnode, &props); that returns early due to
> fwnode==NULL without changing props, thus this stays as initialized
> with {}, so led_compose_name() would return -EINVAL which would let
> led_classdev_register_ext() fail, too.
> 
>> If not, can you fix the core to accept that? Having that conditional
>> in driver is ugly.
> 
> It is ugly, although the approach is inspired by the leds-gpio driver.
> I'll see if I can come up with a change to led-core, but I'm also open
> for suggestions. ;-)

devm_led_classdev_register() calls devm_led_classdev_register_ext()
with NULL passed in place of init_data, so you could do something like
below to achieve the same without touching LED core:

struct led_init_data init_data_impl = { .fwnode = fwnode };
struct led_init_data *init_data = NULL;

if (fwnode)
	init_data = &init_data_impl;

devm_led_classdev_register_ext(dev, &led_data->cdev, init_data);

> fyi: Peter Ujfalusi answered and would give his Ack to the changed
> dual license for the yaml file.  You can expect that for v4.
> 
> Stay tuned
> Alex
> 

-- 
Best regards,
Jacek Anaszewski