lkml.org 
[lkml]   [2023]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4] leds: max597x: Add support for max597x
From
Hi Pavel,

On 13-04-2023 04:43 pm, Pavel Machek wrote:
> Hi!
>
>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>
>> max597x is hot swap controller with indicator LED support.
>> This driver uses DT property to configure led during boot time &
>> also provide the LED control in sysfs.
>>
>> DTS example:
>> i2c {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> regulator@3a {
>> compatible = "maxim,max5978";
>> reg = <0x3a>;
>> vss1-supply = <&p3v3>;
>>
>> regulators {
>> sw0_ref_0: sw0 {
>> shunt-resistor-micro-ohms = <12000>;
>> };
>> };
>>
>> leds {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> led@0 {
>> reg = <0>;
>> label = "led0";
>> default-state = "on";
>> };
>> led@1 {
>> reg = <1>;
>> label = "led1";
>> default-state = "on";
>> };
>> };
>> };
>> };
>>
>
> Yeah, well, dts bindings need to be properly documented, and example
> goes to the binding, right?
Yes.
>
> Plus, we should have better names than led0/led1. Something like
> hdd0:green:something, based on what the LED does and what are you hot
> swapping. See/modify Documentation/leds/well-known-leds.txt .
Yeah sure.
>
>
>> +static int max597x_led_set_brightness(struct led_classdev *cdev,
>> + enum led_brightness brightness)
>> +{
>> + struct max597x_led *ddata = ldev_to_maxled(cdev);
>> + int ret, val;
>> +
>> + if (!ddata->regmap)
>> + return -ENODEV;
>> +
>> + /* Set/Clear corresponding bit for given led index */
>
> "/clear".
Ack.
>
>> + val = !brightness ? BIT(ddata->index) : 0;
>> + ret = regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLASH, BIT(ddata->index), val);
>> + if (ret < 0)
>> + dev_err(cdev->dev, "failed to set brightness %d", ret);
>
> '\n"'.
Ack
>
>> + ddata->cdev.brightness_set_blocking = max597x_led_set_brightness;
>> + ddata->cdev.default_trigger = "none";
>
> So what do these leds normally do? Should they be registered with
> common function to handle default triggers etc?
Not sure if I understand your question right. In my use case in server
motherboards, they are use to indicate presence of power for individual
PCIe NVME connectors. Turning the LED on/off is handled by another
application based on some conditions.
>
>> + ddata->index = reg;
>> + ddata->regmap = regmap;
>> + ret = devm_led_classdev_register(dev, &ddata->cdev);
>> + if (ret)
>> + dev_err(dev, "Error initializing LED %s", ddata->cdev.name);
>
> '\n"'.
Ack
>
>> +module_platform_driver(max597x_led_driver);
>> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
>> +MODULE_DESCRIPTION("MAX5970_hot-swap controller LED driver");
>
> "MAX5970 hot-swap"?
max5970 is primarily a hotswap controller & it also had indication led
support.
>
>> +MODULE_LICENSE("GPL");
>>
>> base-commit: 11e572d06c23d61683e20a98bd16f550ef17ac65
>> prerequisite-patch-id: 456044abe991b2ff3b521d337825432789d71b29
>
> ?? Send prerequisites in the series?
Will fix in v5.
>
> BR,
> Pavel
Regards,
Naresh

\
 
 \ /
  Last update: 2023-04-17 11:44    [W:0.043 / U:0.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site