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.

This adds support for MediaTek MT6370 Backlight driver. It's commonly used

Read Submitting Patches, please!

(In this case, find "This patch" in the above mentioned document, read
and act accordingly)

to drive the display WLED. There are 4 channels inside, and each channel
supports up to 30mA of current capability with 2048 current steps in
exponential or linear mapping curves.

...

+               brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK);

(see below)

...

+               /*
+                * To make MT6372 using 14 bits to control the brightness
+                * backward compatible with 11 bits brightness control
+                * (like MT6370 and MT6371 do), we left shift the value
+                * and pad with 1 to remaining bits. Hence, the MT6372's

to the remaining

+                * backlight brightness will be almost the same as MT6370's
+                * and MT6371's.
+                */
+               if (priv->vid_type == MT6370_VID_6372) {
+                       brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT;
+                       brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK;
+               }

Nice! Why not...

...

+       gpiod_set_value(priv->enable_gpio, brightness ? 1 : 0);

!!brightness will do as well.

...

+       brightness = brightness_val[1] << fls(MT6370_BL_DIM2_MASK);

+               val |= prop_val << (ffs(MT6370_BL_PWM_HYS_SEL_MASK) - 1);

+               val |= ovp_uV << (ffs(MT6370_BL_OVP_SEL_MASK) - 1);

+               val |= ocp_uA << (ffs(MT6370_BL_OC_SEL_MASK) - 1);

+       val = prop_val << (ffs(MT6370_BL_CH_MASK) - 1);

...to use respective _SHIFTs in all these?

...

+       priv->enable_gpio = devm_gpiod_get_optional(dev, "enable",
+                                                   GPIOD_OUT_HIGH);
+       if (IS_ERR(priv->enable_gpio))
+               dev_err(dev, "Failed to get 'enable' gpio\n");

What does this mean? Shouldn't be

  return dev_err_probe()?

-- 
With Best Regards,
Andy Shevchenko