Re: [PATCH v3 3/3] leds: tps68470: Add LED control for tps68470

[Hans de Goede <hdegoede@redhat.com>]

Hi Pavel,

On 3/23/23 12:15, Pavel Machek wrote:
> Hi!
> 
>> There are two LED controllers, LEDA indicator LED and LEDB flash LED for
>> tps68470. LEDA can be enabled by setting TPS68470_ILEDCTL_ENA. Moreover,
>> tps68470 provides four levels of power status for LEDB. If the
>> properties called "ti,ledb-current" can be found, the current will be
>> set according to the property values. These two LEDs can be controlled
>> through the LED class of sysfs (tps68470-leda and tps68470-ledb).
> 
> If the LED can have four different currents, should it have 4
> brightness levels?

No this was already discussed with an earlier version. This is in
indicator LED output. The current setting is a one time boot configure
thing after which the indicator LED is either on or off.

> 
>> +++ b/drivers/leds/Kconfig
>> @@ -827,6 +827,18 @@ config LEDS_TPS6105X
>>  	  It is a single boost converter primarily for white LEDs and
>>  	  audio amplifiers.
>>  
>> +config LEDS_TPS68470
>> +	tristate "LED support for TI TPS68470"
>> +	depends on LEDS_CLASS
>> +	depends on INTEL_SKL_INT3472
>> +	help
>> +	  This driver supports TPS68470 PMIC with LED chip.
>> +	  It provides two LED controllers, with the ability to drive 2
>> +	  indicator LEDs and 2 flash LEDs.
>> +
>> +	  To compile this driver as a module, choose M and it will be
>> +	  called leds-tps68470
> 
> . at end of line.
> 
>> +static const char *tps68470_led_names[] = {
>> +	[TPS68470_ILED_A] = "tps68470-iled_a",
>> +	[TPS68470_ILED_B] = "tps68470-iled_b",
> 
> No. See Documentation/leds/well-known-leds.txt . We want the LEDs to
> be named after their function.
> 
>> +static int tps68470_ledb_current_init(struct platform_device *pdev,
>> +				      struct tps68470_device *tps68470)
>> +{
>> +	int ret = 0;
>> +	unsigned int curr;
>> +
>> +	/* configure LEDB current if the properties can be got */
> 
> english?
> 
>> +	if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) {
>> +		if (curr > CTRLB_16MA) {
> 
> We'll need device tree binding documentation, right?

For now this PMIC is only used for the camera in some x86/acpi designs,
so no we don't need dt binding documentation (the dt binding maintainers
have explicitly asked to not add dt binding documentation for things
not actually used with dt).

Or I guess we should simply add this to the platform_data which
one of Daniel's later patches introduces and drop the initializing
of the LEDB current setting from the initial driver addition.

I think that that (moving this to the later added platform_data)
would be best. Since now after Daniel's patches we have a mix
of platform_data + the 1 device-property for this.

Daniel, what do you think about moving the LEDB current setting to your
"[PATCH 2/8] leds: tps68470: Init LED registers using platform data"
patch ?

Regards,

Hans