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

From: ChiaEn Wu <redacted>

MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger
with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight
driver, display bias voltage supply, one general purpose LDO, and the
USB Type-C & PD controller complies with the latest USB Type-C and PD
standards.

Add a support the MT6370 ADC driver for system monitoring, including

support for the

charger current, voltage, and temperature.

...

+#define MT6370_AICR_400_mA             0x6
+#define MT6370_ICHG_500_mA             0x4
+#define MT6370_ICHG_900_mA             0x8

^^^^ (Note this and read below)

...

+               reg_val = FIELD_GET(MT6370_AICR_ICHG_MASK, reg_val);
+               if (reg_val < MT6370_AICR_400_mA)
+                       *val1 = 3350;
+               else
+                       *val1 = 5000;

Here...

...

+               if (reg_val < MT6370_ICHG_500_mA)
+                       *val1 = 2375;
+               else if (reg_val >= MT6370_ICHG_500_mA &&
+                        reg_val < MT6370_ICHG_900_mA)
+                       *val1 = 2680;
+               else
+                       *val1 = 5000;

...and especially here it is so counterintuitive to have an if-else
chain because the values are not ordered by semantic meaning.

What if the new standard/hardware decides to use 0x7 for 100mA (hypothetically)?

So, please use switch cases or other robust methods.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel