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 v7 1/5] leds: flash: Add flash registration with undefined
 CONFIG_LEDS_CLASS_FLASH
To:     Gene Chen <gene.chen.richtek@gmail.com>
Cc:     Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Dan Murphy <dmurphy@ti.com>,
        Linux LED Subsystem <linux-leds@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Gene Chen <gene_chen@richtek.com>, Wilma.Wu@mediatek.com,
        shufan_lee@richtek.com, cy_huang@richtek.com,
        benjamin.chao@mediatek.com
References: <1605696462-391-1-git-send-email-gene.chen.richtek@gmail.com>
 <1605696462-391-2-git-send-email-gene.chen.richtek@gmail.com>
 <3164b1ed-9e47-88cd-d492-ff5a9243e5ef@gmail.com>
 <CAE+NS350vuY1qNwn4_7ow8Z22_DfHrJAnKX1dsFM_WbaHziZiw@mail.gmail.com>
 <5c4a5780-afec-fa7f-307e-b969192ec677@gmail.com>
 <CAE+NS36yU_ho5eV=j2rd36XqGXBKj3d8KP-bsrCCnWvxzV3Afw@mail.gmail.com>
From:   Jacek Anaszewski <jacek.anaszewski@gmail.com>
Message-ID: <e792dbfc-4b98-058d-bbe5-fd556463a757@gmail.com>
Date:   Tue, 24 Nov 2020 22:15:20 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.5.0
MIME-Version: 1.0
In-Reply-To: <CAE+NS36yU_ho5eV=j2rd36XqGXBKj3d8KP-bsrCCnWvxzV3Afw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/24/20 7:08 AM, Gene Chen wrote:
[...]
>>>> This function should be placed after #ifdef block because its
>>>> shape is the same for both cases.
>>>>
>>>>> +static inline void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev) {};
>>>>> +static inline int devm_led_classdev_flash_register_ext(struct device *parent,
>>>>> +                                  struct led_classdev_flash *fled_cdev,
>>>>> +                                  struct led_init_data *init_data)
>>>>> +{
>>>>> +     return -EINVAL;
>>>>
>>>> /-EINVAL/0/
>>>>
>>>> Please do the same fix in all no-ops in the led-class-multicolor.h,
>>>> as we've discussed.
>>>>
>>>
>>> I think return -EINVAL is correct, because I should register flash
>>> light device if I define FLED in DTS node.

OK, I think I'm getting your concerns now. So you're only partially
correct - the driver should register flash LED device if there is
corresponding node in DT, but only if CONFIG_LEDS_CLASS_FLASH is 
enabled. In case it is disabled the no-op will come into play
and return 0, allowing the probe() to proceed as if the registration
succeeded.

 From the driver point of view nothing changes, except that flash LED
ops will not be called afterwards. This is common pattern. If in doubt
skim through the headers in include/linux.

>>
>> I don't quite follow your logic here.
>>
>> No-op function's purpose is to simplify the code on the caller's side.
>> Therefore it should report success.
>>
>> Please return 0 from it.
>>
> 
> Just like those functions in led-class-multicolor.h, caller may use
> return value to check whether FLED is registered successfully or not.
> For this case, is returning 0 a little bit misleading?

Please note that I've already admitted that led-class-multicolor.h
class is buggy and should also be fixed to return 0 from its no-ops.
Please apply the "s/-EINVAL/0/" fixes to it as well - your driver will
need that.

-- 
Best regards,
Jacek Anaszewski