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
