On 03/02/2017 11:45 PM, Rafał Miłecki wrote:
> On 2 March 2017 at 21:29, Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> wrote:
>> On 03/02/2017 06:42 PM, Rafał Miłecki wrote:
>>> On 28 February 2017 at 22:20, Jacek Anaszewski
>>> <jacek.anaszewski@xxxxxxxxx> wrote:
>>>> Thanks for the patch. How about following what has been already
>>>> done for spi, i.e. adding a wrapper of_led_classdev_register()
>>>> (please refer to of_register_spi_device() in drivers/spi/spi.c)?
>>>
>>> I like this idea, but I'll need to split led_classdev_register into
>>> two functions: allocation & actual registering function. That way I
>>> can alloc, then read DT proprties and finally register LED (just like
>>> it's done in of_register_spi_device).
>>>
>>> Does it sound OK?
>>
>> Yes, after thinking a bit more about that I came to the same
>> conclusion.
>
> OK, I spent some extra time on this today, this seem a bit more complex.
>
> I was starring at of_register_spi_device for quite some time and it
> doesn't seem really related to this situation. In case of
> of_register_spi_device:
> 1) It's used internally in spi core code only
> 2) It handles DT controller child node totally on its own
>
> This patch I sent is about interaction between drivers registering
> LEDs and LED core code. There are few drivers (that register LEDs)
> parsing DT on their own. I'm afraid comparing this situation to
> of_register_spi_device is a bit confusing for me. I really don't see a
> place for of_led_classdev_register that would be called for every
> child of LED controller (?!).
>
> So I'd like to suggest not comparing this situation to
> of_register_spi_device and discussing this problem on our own.
>
> My current problem is that led_classdev_register:
> 1) Allocates struct device & registers LED
> 2) It means drivers can't touch struct device before having LED registered
> 3) It doesn't allow passing of_node
> 4) It is used in so many places it's almost impossible to modify all
> callers at once
>
> So maybe I could work on of_led_classdev_register that:
> 1) Would be almost identical to led_classdev_register
> 2) It would accept extra struct device_node argument
>
> Does it make sense? Or maybe it's even what you meant from the beginning?
I intended that of_led_classdev_register() was almost identical to
led_classdev_register() but additionally had device_node parameter.
When agreeing with you on splitting led_classdev_register() to
allocation and registering function I meant creating two helper
functions that could be used as follows:
[pseudocode]
led_classdev_register(...):
led_cdev->dev = alloc_led_classdev
register_led_classdev(led_cdev)
of_led_classdev_register(..., of_node):
led_cdev->dev = alloc_led_classdev
led_cdev->of_node = of_node
register_led_classdev(led_cdev)
I didn't mean updating all existing drivers to use new of_
prefixed API.
> Two things I'll need to take in mind while working on this:
> 1) I'll need devm variant
Right.
> 2) I should share code between (of_)led_classdev_register functions
Exactly.
--
Best regards,
Jacek Anaszewski
