Message-ID: <b9c6c74b-65d2-46bf-bd7c-e031d420f31c@mind.be>
Date:   Thu, 2 Feb 2023 14:35:14 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: [PATCH v4 1/2] dt-bindings: leds-lp55xx: add ti,charge-pump-mode
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Pavel Machek <pavel@ucw.cz>, Lee Jones <lee@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc:     linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230202101032.26737-1-maarten.zanders@mind.be>
 <20230202101032.26737-2-maarten.zanders@mind.be>
 <20eb5589-8287-90bd-3703-2818b61c6ba3@linaro.org>
From:   Maarten Zanders <maarten.zanders@mind.be>
In-Reply-To: <20eb5589-8287-90bd-3703-2818b61c6ba3@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

First off, bear with me here, I only recently started upstreaming 
patches to kernel. It still feels like navigating an extremely busy 
shipping lane... Either way, your feedback is highly valued.

On 2/2/23 14:15, Krzysztof Kozlowski wrote:
>
>> diff --git a/include/dt-bindings/leds/leds-lp55xx.h b/include/dt-bindings/leds/leds-lp55xx.h
>> new file mode 100644
>> index 000000000000..8f59c1c12dee
>> --- /dev/null
>> +++ b/include/dt-bindings/leds/leds-lp55xx.h
>> @@ -0,0 +1,10 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _DT_BINDINGS_LEDS_LP55XX_H
>> +#define _DT_BINDINGS_LEDS_LP55XX_H
>> +
>> +#define LP55XX_CP_OFF		0
>> +#define LP55XX_CP_BYPASS	1
>> +#define LP55XX_CP_BOOST		2
>> +#define LP55XX_CP_AUTO		3
> Additionally, these are not used, so it's a dead binding. Drop. Sorry,
> this is not the approach you should take.
>
> Best regards,
> Krzysztof
>
These definitions are intended to be used in the DTS's, so it seems 
normal to me that most of them go unused in the code? What am I missing?

As for the changes v2 > v3, this was based on input I got on v2 from Lee 
Jones, maintainer for leds, on the implementation of the parsing of this 
option:

>> +	pdata->charge_pump_mode = LP55XX_CP_AUTO;
>> +	ret = of_property_read_string(np, "ti,charge-pump-mode", &pm);
>> +	if (!ret) {
>> +		for (cp_mode = LP55XX_CP_OFF;
>> +		     cp_mode < ARRAY_SIZE(charge_pump_modes);
>> +		     cp_mode++) {
>> +			if (!strcasecmp(pm, charge_pump_modes[cp_mode])) {
>> +				pdata->charge_pump_mode = cp_mode;
>> +				break;
>> +			}
>> +		}
>> +	}
> A little over-engineered, no?
>
> Why not make the property a numerical value, then simply:
>
>    ret = of_property_read_u32(np, "ti,charge-pump-mode", &pdata->charge_pump_mode);
>    if (ret)
>            data->charge_pump_mode = LP55XX_CP_AUTO;

I found examples of similar configuration options of both types with 
other drivers in the kernel tree (ie string & uint8). I can appreciate 
both viewpoints but unfortunately cannot comply with both.


Best regards,
Maarten