Re: [PATCH V2 2/2] leds: add mp3326 driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 12/06/2024 04:23, Yuxi (Yuxi) Wang wrote:
>>
>>> +/*
>>> + * PWM in the range of [0 255]
>>> + */
>>> +static int led_pwm_store(struct device *dev, struct device_attribute *attr,
>>> +			     const char *buf, size_t count)
>>
>> Nope.
> Hi Krzysztof,
> 
> What do you mean this Nope?
> Is it  format or function?

As sorry, you are reimplementing kernel interfaces, so that's a no.
> 
>>
>> ...
>>
>>> +	}
>>> +	r_val = r_val * 255 / 4095 + (r_val * 255 % 4095) / (4095 / 2);
>>> +	g_val = g_val * 255 / 4095 + (g_val * 255 % 4095) / (4095 / 2);
>>> +	b_val = b_val * 255 / 4095 + (b_val * 255 % 4095) / (4095 / 2);
>>> +	if (led->num_colors == 1)
>>> +		return sysfs_emit(buf, "0x%x\n", r_val);
>>> +	else
>>> +		return sysfs_emit(buf, "0x%x 0x%x 0x%x\n", r_val, g_val, b_val);
>>> +}
>>> +
>>> +static int led_enable_store(struct device *dev,
>>> +				struct device_attribute *attr, const char *buf,
>>> +				size_t count)
>>
>> Eeeee? store to enable LED? Really?
> Yes. The users need this function and we provide it.

NAK, I don't care about your users. You re-implemented existing ABI.
Without any ABI docs that's just pure duplication.

> 
> 
>>
>> ...
>>
>>> +{
>>> +	struct led_classdev *lcdev = dev_get_drvdata(dev);
>>> +	struct mp3326_led *led = container_of(lcdev, struct mp3326_led, cdev);
>>> +	struct mp3326 *chip = led->private_data;
>>> +	int ret;
>>> +	uint val, i;
>>
>>
>>> +
>>> +static DEVICE_ATTR_RW(led_pwm);
>>> +static DEVICE_ATTR_RW(led_enable);
>>> +static DEVICE_ATTR_RO(led_short_fault);
>>> +static DEVICE_ATTR_RO(led_open_fault);
>>
>> No, for multiple reasons:
>> 1. Where ABI documentation?
>> 2. There is a standard sysfs interface. No need for most of that. Please
>> explain why standard interface does not fit your needs - for each new
>> interface.
> Hi krzysztof,
> 
> 1. Where ABI documentation?
> A: 
> Sorry, the abi is insufficient.

Which one is insufficient?

> 
> Can I add it as comment above the function?
> 
> 2. There is a standard sysfs interface. No need for most of that. Please
> explain why standard interface does not fit your needs - for each new
>  interface.
> A:
> Leds has two ways to light dim. One is analog dimming, another pwm dimming.
> They are different in practice. 
> 
> In RGB module, pwm dimming can control color and analog dimming can control intensity.
> 
> Mp3326 supports the two ways which can operate contemporary.
> 
> In practice, I have needs below.
> 1. Operate rgb color and intensity.
> 2. enable/disable some channel
> 3. short/open fault notice.
> 
> However, The standard interface only has three functions below, they are not fit my needs.
> 1. multi_index
> 2. multi_intensity(only can dim using one way, so I use it as analog dimming)
> 3. led_mc_calc_color
> 

Please point to which ABI is not sufficient.

> 
> In order to fit my needs, I add the interface below.
> led_pwm       
> led_enable

Because especially this one sounds like you are mocking us.

> led_short_fault
> led_open_fault
> 


Best regards,
Krzysztof