Hi Pavel, Sorry for resending the mail, I add all reviewers this time. Pavel Machek <pavel@xxxxxx> 於 2022年7月31日 週日 清晨5:39寫道: > > Hi! > ... > > > +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. > > > > + 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? > > +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. > > +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 <pavel@xxxxxx> 於 2022年7月31日 週日 清晨5:39寫道: > > Hi! > > > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx> > > > > 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. > > > > > +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. > > > > + Isink4 channel can also be used as a CHG_VIN power good indicator. > > That does not really belong here. > > > +struct mt6370_priv { > > + /* Per LED access lock */ > > + struct mutex lock; > > Do we really need per-led locking? > > > +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? > > > +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. > > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "led %d, no color specified\n", > > + led->index); > > led->LED. > > > + if (num_color < 2) > > + return dev_err_probe(dev, -EINVAL, > > + "Multicolor must include > > 2 or more led channel\n"); > > "LED channels". > > > +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.