On Fri, Jul 22, 2022 at 12:25 PM ChiaEn Wu [off-list ref] wrote:

From: ChiYuan Huang <redacted>

^^^^ (Note this and read below)

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.

Signed-off-by: Alice Chen <redacted>
Signed-off-by: ChiYuan Huang <redacted>

In conjunction with above what SoB of Alice means?

You really need to take your time and (re-)read
https://www.kernel.org/doc/html/latest/process/submitting-patches.html.

...

+ * Author: Alice Chen [off-list ref]
+ * Author: ChiYuan Huang [off-list ref]

Would
 * Authors:
 *    Name_of_Author 1
 *    Name_of_Author 2

work for you?

...

+struct mt6370_led {
+       union {
+               struct led_classdev isink;
+               struct led_classdev_mc mc;
+       };

Where is the field that makes union work?

+       struct mt6370_priv *priv;
+       u32 default_state;
+       u32 index;
+};

...

+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);

Isn't it something like put_unaligned_be24()/put_unaligned_le24()?

+       return 0;
+}

...

+static inline int mt6370_mc_pattern_clear(struct led_classdev *lcdev)
+{
+       struct led_classdev_mc *mccdev = lcdev_to_mccdev(lcdev);
+       struct mt6370_led *led = container_of(mccdev, struct mt6370_led, mc);
+       struct mt6370_priv *priv = led->priv;
+       struct mc_subled *subled;

+       int i, ret = 0;

Redundant assignment.

+       mutex_lock(&led->priv->lock);
+
+       for (i = 0; i < mccdev->num_colors; i++) {
+               subled = mccdev->subled_info + i;
+
+               ret = mt6370_set_led_mode(priv, subled->channel,
+                                         MT6370_LED_REG_MODE);
+               if (ret)
+                       break;
+       }
+
+       mutex_unlock(&led->priv->lock);
+
+       return ret;
+}

...

+       if (!fwnode_property_read_string(init_data->fwnode, "default-state",
+                                        &stat_str)) {

ret = fwnode_...(...);
if (!ret)

+               ret = match_string(states, ARRAY_SIZE(states), stat_str);
+               if (ret < 0)
+                       ret = STATE_OFF;
+
+               led->default_state = ret;
+       }

...

+       int i = 0, ret;

unsigned int i = 0;
int ret;

-- 
With Best Regards,
Andy Shevchenko