Re: [PATCH v6 11/13] leds: rgb: mt6370: Add MediaTek MT6370 current sink type LED Indicator support
From: szuni chen <hidden>
Date: 2022-08-01 07:55:23
Also in:
linux-arm-kernel, linux-fbdev, linux-iio, linux-leds, linux-mediatek, linux-pm, linux-usb, lkml
Hi Pavel, Sorry for resending the mail, I add all reviewers this time. Pavel Machek [off-list ref] 於 2022年7月31日 週日 清晨5:39寫道：
Hi!
...
quoted
+config LEDS_MT6370_RGB + tristate "LED Support for MediaTek MT6370 PMIC" + depends on MFD_MT6370 + select LINEAR_RANGE + help + Say Y here to enable support for MT6370_RGB LED device. + In MT6370, there are four channel current-sink LED drivers that + support hardware pattern for constant current, PWM, and breath mode.quoted
+ Isink4 channel can also be used as a CHG_VIN power good indicator.That does not really belong here.
Should we just remove it, or describe Isink4 in another position?
quoted
+struct mt6370_priv { + /* Per LED access lock */ + struct mutex lock;Do we really need per-led locking?
Sorry, maybe the comment is not precise. The lock is used to prevent LEDs from accessing the HW at the same time. If I use /* LED access lock, only one LED can access the HW at the same time */ will it look better? No, we aren't. There are six steps tr1, tr2, tf1, tf2, ton, and toff in MT6370 led breath mode. We parse duration settings from node "hw_pattern" and set them to the registers. This function is used to generate duration settings from hw_pattern. The brightness of the six steps mentioned above in breath mode is limited to the node "brightness". The target brightness of tr1 and tf1 is 25% of node "brightness", and they are automatically set by HW.
quoted
+static int mt6370_init_led_properties(struct mt6370_led *led, + struct led_init_data *init_data) +{ + struct mt6370_priv *priv = led->priv; + struct device *dev = priv->dev; + struct led_classdev *lcdev; + struct fwnode_handle *child; + enum mt6370_led_ranges sel_range; + u32 max_uA, max_level; + const char * const states[] = { "off", "keep", "on" };We'd really preffer not to add "keep" / "on" support unless you need it.
Forgive me, but I would like to know why "keep" / "on" is not preferred. We think the users might have some conditions that need them. Best Regards, Alice Pavel Machek [off-list ref] 於 2022年7月31日 週日 清晨5:39寫道：
Hi!quoted
From: ChiYuan Huang <redacted> The MediaTek MT6370 is a highly-integrated smart power management IC, which includes a single cell Li-Ion/Li-Polymer switching battery charger, a USB Type-C & Power Delivery (PD) controller, dual Flash LED current sources, a RGB LED driver, a backlight WLED driver, a display bias driver and a general LDO for portable devices. In MediaTek MT6370, there are four channel current-sink RGB LEDs that support hardware pattern for constant current, PWM, and breath mode. Isink4 channel can also be used as a CHG_VIN power good indicator.quoted
+config LEDS_MT6370_RGB + tristate "LED Support for MediaTek MT6370 PMIC" + depends on MFD_MT6370 + select LINEAR_RANGE + help + Say Y here to enable support for MT6370_RGB LED device. + In MT6370, there are four channel current-sink LED drivers that + support hardware pattern for constant current, PWM, and breath mode.quoted
+ Isink4 channel can also be used as a CHG_VIN power good indicator.That does not really belong here.quoted
+struct mt6370_priv { + /* Per LED access lock */ + struct mutex lock;Do we really need per-led locking?quoted
+static int mt6370_gen_breath_pattern(struct mt6370_priv *priv, + struct led_pattern *pattern, u32 len, + u8 *pattern_val, u32 val_len) +{ + enum mt6370_led_ranges sel_range; + struct led_pattern *curr; + unsigned int sel; + u8 val[P_MAX_PATTERNS / 2] = {}; + int i; + + if (len < P_MAX_PATTERNS && val_len < P_MAX_PATTERNS / 2) + return -EINVAL; + + /* + * Pattern list + * tr1: byte 0, b'[7: 4] + * tr2: byte 0, b'[3: 0] + * tf1: byte 1, b'[7: 4] + * tf2: byte 1, b'[3: 0] + * ton: byte 2, b'[7: 4] + * toff: byte 2, b'[3: 0] + */ + for (i = 0; i < P_MAX_PATTERNS; i++) { + curr = pattern + i; + + sel_range = i == P_LED_TOFF ? R_LED_TOFF : R_LED_TRFON; + + linear_range_get_selector_within(priv->ranges + sel_range, + curr->delta_t, &sel); + + val[i / 2] |= sel << (4 * ((i + 1) % 2)); + } + + memcpy(pattern_val, val, 3); + + return 0; +}I wonder how this works... you are not creating private sysfs interface, are you?quoted
+static int mt6370_init_led_properties(struct mt6370_led *led, + struct led_init_data *init_data) +{ + struct mt6370_priv *priv = led->priv; + struct device *dev = priv->dev; + struct led_classdev *lcdev; + struct fwnode_handle *child; + enum mt6370_led_ranges sel_range; + u32 max_uA, max_level; + const char * const states[] = { "off", "keep", "on" };We'd really preffer not to add "keep" / "on" support unless you need it.quoted
+ if (ret) + return dev_err_probe(dev, ret, + "led %d, no color specified\n", + led->index);led->LED.quoted
+ if (num_color < 2) + return dev_err_probe(dev, -EINVAL, + "Multicolor must include 2 or more led channel\n");"LED channels".quoted
+static int mt6370_isnk_init_default_state(struct mt6370_led *led) +{ + struct mt6370_priv *priv = led->priv; + unsigned int enable, level; + int ret; + + ret = mt6370_get_led_brightness(priv, led->index, &level); + if (ret) + return ret; + + ret = regmap_field_read(priv->fields[F_RGB_EN], &enable); + if (ret) + return ret; + + if (!(enable & MT6370_CHEN_BIT(led->index))) + level = LED_OFF;Just use 0 instead of LED_OFF. Best regards, Pavel -- People of Russia, stop Putin before his war on Ukraine escalates.