ChiYuan Huang [off-list ref] 於 2022年7月20日 週三 下午5:48寫道：

ChiYuan Huang [off-list ref] 於 2022年7月20日 週三 下午5:45寫道：

quoted

On Fri, Jul 15, 2022 at 08:29:42PM +0200, Andy Shevchenko wrote:

quoted

On Fri, Jul 15, 2022 at 1:29 PM ChiaEn Wu [off-list ref] wrote:

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

+         This driver can also be built as a module. If so the module

so, the

quoted

+         will be called "leds-mt6370.ko".

No ".ko".

Why did you ignore these comments? Please go and fix _everywhere_ in
your series.
It's basically the rule of thumb, if the reviewer gives a comment
against an occurrence of something, go through entire series and check
if there are other places like commented one and address them all.

...

quoted

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

Strange, the commit message doesn't have a corresponding SoB, why?

Yes, there're two authors Alice and me.
I'll correct it in next.

quoted

...

quoted

+#define MT6370_PWM_DUTY                                31
+#define MT6372_PMW_DUTY                                255

Looks like these are limits by hardware?
Check with the datasheet if (BIT(x) - 1) makes more sense here.

...

quoted

+       switch (led_no) {
+       case MT6370_LED_ISNK1:
+               sel_field = F_LED1_DUTY;
+               break;
+       case MT6370_LED_ISNK2:
+               sel_field = F_LED2_DUTY;
+               break;
+       case MT6370_LED_ISNK3:
+               sel_field = F_LED3_DUTY;
+               break;
+       default:
+               sel_field = F_LED4_DUTY;

Missed break;

quoted

+       }

...

quoted

+       switch (led_no) {
+       case MT6370_LED_ISNK1:
+               sel_field = F_LED1_FREQ;
+               break;
+       case MT6370_LED_ISNK2:
+               sel_field = F_LED2_FREQ;
+               break;
+       case MT6370_LED_ISNK3:
+               sel_field = F_LED3_FREQ;
+               break;
+       default:
+               sel_field = F_LED4_FREQ;

Ditto.

quoted

+       }

...

quoted

+       switch (led_no) {
+       case MT6370_LED_ISNK1:
+       case MT6370_LED_ISNK2:
+       case MT6370_LED_ISNK3:
+               *base = MT6370_REG_RGB1_TR + led_no * 3;
+               break;
+       default:
+               *base = MT6370_REG_RGB_CHRIND_TR;

Ditto.
It seems you dropped them for all switch-cases. It's not goot, please
restore them back.

quoted

+       }

...

quoted

+       u8 val[P_MAX_PATTERNS / 2] = {0};

{ } should suffice

In the above range selector, we use the 'logic or' to generate the

typo, it's 'below'.

quoted

pattern values.

Ah, found in c11 standard 6.7.9 item 21
It is the same as 'static storage duration'.
I will follow your comment to revise it.
Thanks.

quoted

If to change it from '{0} to '{ }', is it correct?

quoted

quoted

+       /*
+        * 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;
+}

...

quoted

+out:

out_unlock:

quoted

+       mutex_unlock(&priv->lock);
+
+       return ret;

...

quoted

+out:

Ditto. And so on.

quoted

+       mutex_unlock(&priv->lock);
+
+       return ret;

...

quoted

+               sub_led = devm_kzalloc(priv->dev,
+                                      sizeof(*sub_led) * MC_CHANNEL_NUM,
+                                      GFP_KERNEL);

NIH devm_kcalloc(). Also check if you really need zeroed data.

Ok, and after the check, I also need to add one line to set the intensity to 0.

quoted

quoted

+               if (!sub_led)
+                       return -ENOMEM;

...

quoted

+                       ret = fwnode_property_read_u32(child, "color", &color);
+                       if (ret) {
+                               dev_err(priv->dev,
+                                       "led %d, no color specified\n",
+                                       led->index);
+                               return ret;

return dev_err_probe(...) ; ?

Ditto for many places in your entire series.

quoted

+                       }

...

quoted

+       priv = devm_kzalloc(&pdev->dev,
+                           struct_size(priv, leds, count), GFP_KERNEL);

At least one parameter can be placed on the previous line.

quoted

+       if (!priv)
+               return -ENOMEM;

--
With Best Regards,
Andy Shevchenko