On Mon, Jul 4, 2022 at 7:43 AM ChiaEn Wu <peterwu.pub@xxxxxxxxx> wrote: > > From: ChiaEn Wu <chiaen_wu@xxxxxxxxxxx> > > Add Mediatek MT6370 Backlight support. ... > + This driver can also be built as a module. If so the module If so, > + will be called "mt6370-backlight.ko". No ".ko" part. ... > +#include <linux/gpio/driver.h> Can you elaborate on this? > +#include <linux/kernel.h> > +#include <linux/log2.h> > +#include <linux/minmax.h> > +#include <linux/module.h> > +#include <linux/of.h> Can you elaborate on this? > +#include <linux/platform_device.h> > +#include <linux/regmap.h> Missed mod_devicetable.h. ... > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > + brightness_val[1] = (brightness - 1) > + >> fls(MT6370_BL_DIM2_MASK); Bad indentation. One line? ... > + if (priv->enable_gpio) Dup check. > + gpiod_set_value(priv->enable_gpio, brightness ? 1 : 0); ... > + brightness = brightness_val[1] << fls(MT6370_BL_DIM2_MASK); > + brightness += (brightness_val[0] & MT6370_BL_DIM2_MASK); Too many parentheses. ... > + /* > + * prop_val = 1 --> 1 steps --> 0x00 > + * prop_val = 2 ~ 4 --> 4 steps --> 0x01 > + * prop_val = 5 ~ 16 --> 16 steps --> 0x10 > + * prop_val = 17 ~ 64 --> 64 steps --> 0x11 > + */ > + prop_val = (ilog2(roundup_pow_of_two(prop_val)) + 1) >> 1; Isn't something closer to get_order() or fls()? ... > + props->max_brightness = min_t(u32, brightness, > + MT6370_BL_MAX_BRIGHTNESS); One line? ... > + val = 0; Do you need this here? > + prop_val = 0; Useless. > + ret = device_property_read_u8(dev, "mediatek,bled-channel-use", > + &prop_val); > + if (ret) { > + dev_err(dev, "mediatek,bled-channel-use DT property missing\n"); > + return ret; > + } > + > + if (!prop_val || prop_val > MT6370_BL_MAX_CH) { > + dev_err(dev, > + "No channel specified or over than upper bound (%d)\n", > + prop_val); > + return -EINVAL; > + } ... > +static int mt6370_bl_probe(struct platform_device *pdev) > +{ > + struct mt6370_priv *priv; > + struct backlight_properties props = { > + .type = BACKLIGHT_RAW, > + .scale = BACKLIGHT_SCALE_LINEAR, > + }; > + int ret; struct device *dev = &pdev->dev; will save you a few LoCs. -- With Best Regards, Andy Shevchenko