From: ChiaEn Wu <chiaen_wu@richtek.com>
Add Mediatek MT6370 Backlight support.
Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.com>
---
v4:
- Replace text "Mediatek" with "MediaTek" in Kconfig
- Add Copyright in the source code
- Revise the comment of "PWM HYS STEPS"
---
drivers/video/backlight/Kconfig | 12 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/mt6370-backlight.c | 352 +++++++++++++++++++++++++++++
3 files changed, 365 insertions(+)
create mode 100644 drivers/video/backlight/mt6370-backlight.c
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index a003e02..099b0d0 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -268,6 +268,18 @@ config BACKLIGHT_MAX8925
If you have a LCD backlight connected to the WLED output of MAX8925
WLED output, say Y here to enable this driver.
+config BACKLIGHT_MT6370
+ tristate "MediaTek MT6370 Backlight Driver"
+ depends on MFD_MT6370
+ help
+ This enables support for Mediatek MT6370 Backlight driver.
+ It's commonly used 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.
+
+ This driver can also be built as a module. If so the module
+ will be called "mt6370-backlight.ko".
+
config BACKLIGHT_APPLE
tristate "Apple Backlight Driver"
depends on X86 && ACPI
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index cae2c83..e815f3f 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_BACKLIGHT_LP855X) += lp855x_bl.o
obj-$(CONFIG_BACKLIGHT_LP8788) += lp8788_bl.o
obj-$(CONFIG_BACKLIGHT_LV5207LP) += lv5207lp.o
obj-$(CONFIG_BACKLIGHT_MAX8925) += max8925_bl.o
+obj-$(CONFIG_BACKLIGHT_MT6370) += mt6370-backlight.o
obj-$(CONFIG_BACKLIGHT_OMAP1) += omap1_bl.o
obj-$(CONFIG_BACKLIGHT_PANDORA) += pandora_bl.o
obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c
new file mode 100644
index 0000000..655f585
--- /dev/null
+++ b/drivers/video/backlight/mt6370-backlight.c
@@ -0,0 +1,352 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Richtek Technology Corp.
+ *
+ * Author: ChiaEn Wu <chiaen_wu@richtek.com>
+ */
+
+#include <linux/backlight.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/kernel.h>
+#include <linux/log2.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define MT6370_REG_DEV_INFO 0x100
+#define MT6370_REG_BL_EN 0x1A0
+#define MT6370_REG_BL_BSTCTRL 0x1A1
+#define MT6370_REG_BL_PWM 0x1A2
+#define MT6370_REG_BL_DIM2 0x1A4
+
+#define MT6370_VENID_MASK GENMASK(7, 4)
+#define MT6370_BL_EXT_EN_MASK BIT(7)
+#define MT6370_BL_EN_MASK BIT(6)
+#define MT6370_BL_CONFIG_MASK BIT(0)
+#define MT6370_BL_CH_MASK GENMASK(5, 2)
+#define MT6370_BL_DIM2_MASK GENMASK(2, 0)
+#define MT6370_BL_DUMMY_6372_MASK GENMASK(2, 0)
+#define MT6370_BL_DIM2_6372_SHIFT 3
+#define MT6370_BL_PWM_EN_MASK BIT(7)
+#define MT6370_BL_PWM_HYS_EN_MASK BIT(2)
+#define MT6370_BL_PWM_HYS_SEL_MASK GENMASK(1, 0)
+#define MT6370_BL_OVP_EN_MASK BIT(7)
+#define MT6370_BL_OVP_SEL_MASK GENMASK(6, 5)
+#define MT6370_BL_OC_EN_MASK BIT(3)
+#define MT6370_BL_OC_SEL_MASK GENMASK(2, 1)
+
+#define MT6370_BL_PWM_HYS_TH_MIN_STEP 1
+#define MT6370_BL_PWM_HYS_TH_MAX_STEP 64
+#define MT6370_BL_OVP_MIN_UV 17000000
+#define MT6370_BL_OVP_MAX_UV 29000000
+#define MT6370_BL_OVP_STEP_UV 4000000
+#define MT6370_BL_OCP_MIN_UA 900000
+#define MT6370_BL_OCP_MAX_UA 1800000
+#define MT6370_BL_OCP_STEP_UA 300000
+#define MT6370_BL_MAX_BRIGHTNESS 2048
+#define MT6370_BL_MAX_CH 15
+
+enum {
+ MT6370_VID_COMMON = 0,
+ MT6370_VID_6372,
+};
+
+struct mt6370_priv {
+ int vid_type;
+ struct backlight_device *bl;
+ struct device *dev;
+ struct gpio_desc *enable_gpio;
+ struct regmap *regmap;
+};
+
+static int mt6370_bl_update_status(struct backlight_device *bl_dev)
+{
+ struct mt6370_priv *priv = bl_get_data(bl_dev);
+ int brightness = backlight_get_brightness(bl_dev);
+ unsigned int enable_val;
+ u8 brightness_val[2];
+ int ret;
+
+ if (brightness) {
+ brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK;
+ brightness_val[1] = (brightness - 1)
+ >> fls(MT6370_BL_DIM2_MASK);
+
+ /*
+ * 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
+ * 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;
+ }
+
+ ret = regmap_raw_write(priv->regmap, MT6370_REG_BL_DIM2,
+ brightness_val, sizeof(brightness_val));
+ if (ret)
+ return ret;
+ }
+
+ if (priv->enable_gpio)
+ gpiod_set_value(priv->enable_gpio, brightness ? 1 : 0);
+
+ enable_val = brightness ? MT6370_BL_EN_MASK : 0;
+ return regmap_update_bits(priv->regmap, MT6370_REG_BL_EN,
+ MT6370_BL_EN_MASK, enable_val);
+}
+
+static int mt6370_bl_get_brightness(struct backlight_device *bl_dev)
+{
+ struct mt6370_priv *priv = bl_get_data(bl_dev);
+ unsigned int enable;
+ u8 brightness_val[2];
+ int brightness, ret;
+
+ ret = regmap_read(priv->regmap, MT6370_REG_BL_EN, &enable);
+ if (ret)
+ return ret;
+
+ if (!(enable & MT6370_BL_EN_MASK))
+ return 0;
+
+ ret = regmap_raw_read(priv->regmap, MT6370_REG_BL_DIM2,
+ brightness_val, sizeof(brightness_val));
+ if (ret)
+ return ret;
+
+ if (priv->vid_type == MT6370_VID_6372)
+ brightness_val[0] >>= MT6370_BL_DIM2_6372_SHIFT;
+
+ brightness = brightness_val[1] << fls(MT6370_BL_DIM2_MASK);
+ brightness += (brightness_val[0] & MT6370_BL_DIM2_MASK);
+
+ return brightness + 1;
+}
+
+static const struct backlight_ops mt6370_bl_ops = {
+ .options = BL_CORE_SUSPENDRESUME,
+ .update_status = mt6370_bl_update_status,
+ .get_brightness = mt6370_bl_get_brightness,
+};
+
+static int mt6370_init_backlight_properties(struct mt6370_priv *priv,
+ struct backlight_properties *props)
+{
+ struct device *dev = priv->dev;
+ u8 prop_val;
+ u32 brightness, ovp_uV, ocp_uA;
+ unsigned int mask, val;
+ int ret;
+
+ /* Vendor optional properties */
+ val = 0;
+ if (device_property_read_bool(dev, "mediatek,bled-pwm-enable"))
+ val |= MT6370_BL_PWM_EN_MASK;
+
+ if (device_property_read_bool(dev, "mediatek,bled-pwm-hys-enable"))
+ val |= MT6370_BL_PWM_HYS_EN_MASK;
+
+ ret = device_property_read_u8(dev,
+ "mediatek,bled-pwm-hys-input-th-steps",
+ &prop_val);
+ if (!ret) {
+ prop_val = clamp_val(prop_val,
+ MT6370_BL_PWM_HYS_TH_MIN_STEP,
+ MT6370_BL_PWM_HYS_TH_MAX_STEP);
+ /*
+ * 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;
+ val |= prop_val << (ffs(MT6370_BL_PWM_HYS_SEL_MASK) - 1);
+ }
+
+ ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_PWM,
+ val, val);
+ if (ret)
+ return ret;
+
+ val = 0;
+ if (device_property_read_bool(dev, "mediatek,bled-ovp-shutdown"))
+ val |= MT6370_BL_OVP_EN_MASK;
+
+ ret = device_property_read_u32(dev, "mediatek,bled-ovp-microvolt",
+ &ovp_uV);
+ if (!ret) {
+ ovp_uV = clamp_val(ovp_uV, MT6370_BL_OVP_MIN_UV,
+ MT6370_BL_OVP_MAX_UV);
+ ovp_uV = DIV_ROUND_UP(ovp_uV - MT6370_BL_OVP_MIN_UV,
+ MT6370_BL_OVP_STEP_UV);
+ val |= ovp_uV << (ffs(MT6370_BL_OVP_SEL_MASK) - 1);
+ }
+
+ if (device_property_read_bool(dev, "mediatek,bled-ocp-shutdown"))
+ val |= MT6370_BL_OC_EN_MASK;
+
+ ret = device_property_read_u32(dev, "mediatek,bled-ocp-microamp",
+ &ocp_uA);
+ if (!ret) {
+ ocp_uA = clamp_val(ocp_uA, MT6370_BL_OCP_MIN_UA,
+ MT6370_BL_OCP_MAX_UA);
+ ocp_uA = DIV_ROUND_UP(ocp_uA - MT6370_BL_OCP_MIN_UA,
+ MT6370_BL_OCP_STEP_UA);
+ val |= ocp_uA << (ffs(MT6370_BL_OC_SEL_MASK) - 1);
+ }
+
+ ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BSTCTRL,
+ val, val);
+ if (ret)
+ return ret;
+
+ /* Common properties */
+ ret = device_property_read_u32(dev, "max-brightness", &brightness);
+ if (ret)
+ brightness = MT6370_BL_MAX_BRIGHTNESS;
+
+ props->max_brightness = min_t(u32, brightness,
+ MT6370_BL_MAX_BRIGHTNESS);
+
+ ret = device_property_read_u32(dev, "default-brightness", &brightness);
+ if (ret)
+ brightness = props->max_brightness;
+
+ props->brightness = min_t(u32, brightness, props->max_brightness);
+
+ val = 0;
+ prop_val = 0;
+ 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;
+ }
+
+ mask = MT6370_BL_EXT_EN_MASK | MT6370_BL_CH_MASK;
+ val = prop_val << (ffs(MT6370_BL_CH_MASK) - 1);
+
+ if (priv->enable_gpio)
+ val |= MT6370_BL_EXT_EN_MASK;
+
+ return regmap_update_bits(priv->regmap, MT6370_REG_BL_EN, mask, val);
+}
+
+static int mt6370_check_vendor_info(struct mt6370_priv *priv)
+{
+ /*
+ * MT6372 uses 14 bits to control the brightness but MT6370 and MT6371
+ * use 11 bits. They are different so we have to use this function to
+ * check the vendor ID and use different methods to calculate the
+ * brightness.
+ */
+ unsigned int dev_info, vid;
+ int ret;
+
+ ret = regmap_read(priv->regmap, MT6370_REG_DEV_INFO, &dev_info);
+ if (ret)
+ return ret;
+
+ vid = FIELD_GET(MT6370_VENID_MASK, dev_info);
+ if (vid == 0x9 || vid == 0xb)
+ priv->vid_type = MT6370_VID_6372;
+ else
+ priv->vid_type = MT6370_VID_COMMON;
+
+ return 0;
+}
+
+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;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = &pdev->dev;
+
+ priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!priv->regmap)
+ return dev_err_probe(&pdev->dev, -ENODEV,
+ "Failed to get regmap\n");
+
+ ret = mt6370_check_vendor_info(priv);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "Failed to check vendor info\n");
+
+ priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(priv->enable_gpio))
+ dev_err(&pdev->dev, "Failed to get 'enable' gpio\n");
+
+ ret = mt6370_init_backlight_properties(priv, &props);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "Failed to init backlight properties\n");
+
+ priv->bl = devm_backlight_device_register(&pdev->dev, pdev->name,
+ &pdev->dev, priv,
+ &mt6370_bl_ops, &props);
+ if (IS_ERR(priv->bl))
+ return dev_err_probe(&pdev->dev, PTR_ERR(priv->bl),
+ "Failed to register backlight\n");
+
+ backlight_update_status(priv->bl);
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static int mt6370_bl_remove(struct platform_device *pdev)
+{
+ struct mt6370_priv *priv = platform_get_drvdata(pdev);
+ struct backlight_device *bl_dev = priv->bl;
+
+ bl_dev->props.brightness = 0;
+ backlight_update_status(priv->bl);
+
+ return 0;
+}
+
+static const struct of_device_id __maybe_unused mt6370_bl_of_match[] = {
+ { .compatible = "mediatek,mt6370-backlight", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mt6370_bl_of_match);
+
+static struct platform_driver mt6370_bl_driver = {
+ .driver = {
+ .name = "mt6370-backlight",
+ .of_match_table = mt6370_bl_of_match,
+ },
+ .probe = mt6370_bl_probe,
+ .remove = mt6370_bl_remove,
+};
+module_platform_driver(mt6370_bl_driver);
+
+MODULE_AUTHOR("ChiaEn Wu <chiaen_wu@richtek.com>");
+MODULE_DESCRIPTION("MediaTek MT6370 Backlight Driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
On Mon, Jul 4, 2022 at 7:43 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > From: ChiaEn Wu <chiaen_wu@richtek.com> > > 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
Hi Andy, Thanks for your reply! I have some questions want to ask you below. Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月5日 週二 清晨5:14寫道: > > On Mon, Jul 4, 2022 at 7:43 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > > > From: ChiaEn Wu <chiaen_wu@richtek.com> > > > > 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? Well... if indent to one line, it will be over 80 characters(or called columns?) From my understanding, it is not allowed, right?? > > ... > > > + 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()? I will revise it to "(get_order(prop_va * PAGE_SIZE) + 1) / 2" and this change is meet your expectations?? > > ... > > > + props->max_brightness = min_t(u32, brightness, > > + MT6370_BL_MAX_BRIGHTNESS); > > One line? Ditto, it will be over 80 characters... > > ... > > > + 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 Best regards, ChiaEn Wu
On Wed, Jul 13, 2022 at 12:53 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月5日 週二 清晨5:14寫道: > > On Mon, Jul 4, 2022 at 7:43 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: Please, remove unneeded context when replying! ... > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > + brightness_val[1] = (brightness - 1) > > > + >> fls(MT6370_BL_DIM2_MASK); > > > > Bad indentation. One line? > > Well... if indent to one line, it will be over 80 characters(or called columns?) > From my understanding, it is not allowed, right?? It's allowed to some extent.Use your common sense. Here it's obviously broken indentation. ... > > > + prop_val = (ilog2(roundup_pow_of_two(prop_val)) + 1) >> 1; > > > > Isn't something closer to get_order() or fls()? > > I will revise it to "(get_order(prop_va * PAGE_SIZE) + 1) / 2" and > this change is meet your expectations?? Nope. Try again. What about fls()? ... > > > + props->max_brightness = min_t(u32, brightness, > > > + MT6370_BL_MAX_BRIGHTNESS); > > > > One line? > > Ditto, it will be over 80 characters... As per above. -- With Best Regards, Andy Shevchenko
Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月13日 週三 晚上8:07寫道: > > On Wed, Jul 13, 2022 at 12:53 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月5日 週二 清晨5:14寫道: > > > On Mon, Jul 4, 2022 at 7:43 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > Please, remove unneeded context when replying! > > ... > > > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > > + brightness_val[1] = (brightness - 1) > > > > + >> fls(MT6370_BL_DIM2_MASK); > > > > > > Bad indentation. One line? > > > > Well... if indent to one line, it will be over 80 characters(or called columns?) > > From my understanding, it is not allowed, right?? > > It's allowed to some extent.Use your common sense. > Here it's obviously broken indentation. > > ... > > > > > + prop_val = (ilog2(roundup_pow_of_two(prop_val)) + 1) >> 1; > > > > > > Isn't something closer to get_order() or fls()? > > > > I will revise it to "(get_order(prop_va * PAGE_SIZE) + 1) / 2" and > > this change is meet your expectations?? > > Nope. Try again. What about fls()? I have tried two methods so far, as follows ------------------------------------------------------------- /* * prop_val = 1 --> 1 steps --> b'00 * prop_val = 2 ~ 4 --> 4 steps --> b'01 * prop_val = 5 ~ 16 --> 16 steps --> b'10 * prop_val = 17 ~ 64 --> 64 steps --> b'11 */ // 1. use fls() and ffs() combination prop_val = ffs(prop_val) == fls(prop_val) ? fls(prop_val) >> 1 : (fls(prop_val) + 1) >> 1; // 2. use one line for-loop, but without fls() for (i = --prop_val, prop_val = 0; i >> 2 * prop_val != 0; prop_val++); ------------------------------------------------------------- Do these changes meet your expectations?? > > ... > > > > > + props->max_brightness = min_t(u32, brightness, > > > > + MT6370_BL_MAX_BRIGHTNESS); > > > > > > One line? > > > > Ditto, it will be over 80 characters... > > As per above. > > -- > With Best Regards, > Andy Shevchenko
On Thu, Jul 14, 2022 at 9:13 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月13日 週三 晚上8:07寫道: > > On Wed, Jul 13, 2022 at 12:53 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月5日 週二 清晨5:14寫道: > > > > On Mon, Jul 4, 2022 at 7:43 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: Please, once again, remove unneeded context when replying! ^^^^^^^ ... > > > > > + prop_val = (ilog2(roundup_pow_of_two(prop_val)) + 1) >> 1; > > > > > > > > Isn't something closer to get_order() or fls()? > > > > > > I will revise it to "(get_order(prop_va * PAGE_SIZE) + 1) / 2" and > > > this change is meet your expectations?? > > > > Nope. Try again. What about fls()? > > I have tried two methods so far, as follows > ------------------------------------------------------------- > /* > * prop_val = 1 --> 1 steps --> b'00 > * prop_val = 2 ~ 4 --> 4 steps --> b'01 > * prop_val = 5 ~ 16 --> 16 steps --> b'10 > * prop_val = 17 ~ 64 --> 64 steps --> b'11 > */ So, for 1 --> 0, for 2 --> 1, for 5 --> 2, and for 17 --> 3. Now, consider x - 1: 0 ( 0 ) --> 0 1 (2^0) --> 1 4 (2^2) --> 2 16 (2^4) --> 3 64 (2^6) --> ? (but let's consider that the range has been checked already) Since we take the lower limit, it means ffs(): y = (ffs(x - 1) + 1) / 2; Does it work for you? > // 1. use fls() and ffs() combination > prop_val = ffs(prop_val) == fls(prop_val) ? fls(prop_val) >> 1 : > (fls(prop_val) + 1) >> 1; > > // 2. use one line for-loop, but without fls() > for (i = --prop_val, prop_val = 0; i >> 2 * prop_val != 0; prop_val++); > ------------------------------------------------------------- > Do these changes meet your expectations?? No, this is ugly. Yes, I understand that a bit arithmetics is hard... -- With Best Regards, Andy Shevchenko
On Thu, Jul 14, 2022 at 11:27:07AM +0200, Andy Shevchenko wrote: > On Thu, Jul 14, 2022 at 9:13 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > I have tried two methods so far, as follows > > ------------------------------------------------------------- > > /* > > * prop_val = 1 --> 1 steps --> b'00 > > * prop_val = 2 ~ 4 --> 4 steps --> b'01 > > * prop_val = 5 ~ 16 --> 16 steps --> b'10 > > * prop_val = 17 ~ 64 --> 64 steps --> b'11 > > */ > > So, for 1 --> 0, for 2 --> 1, for 5 --> 2, and for 17 --> 3. > Now, consider x - 1: > 0 ( 0 ) --> 0 > 1 (2^0) --> 1 > 4 (2^2) --> 2 > 16 (2^4) --> 3 > 64 (2^6) --> ? (but let's consider that the range has been checked already) > > Since we take the lower limit, it means ffs(): > > y = (ffs(x - 1) + 1) / 2; > > Does it work for you? To be honest, for this tiny table, writing code that *doesn't* require intricate deciphering together with a huge comment saying what is does would probably be better: prop_val = (prop_val <= 1 ? 0 : prop_val <= 4 ? 1 : prop_val <= 16 ? 2 : 3); This would be "obviously correct" and require no comment. Daniel.
On Thu, Jul 14, 2022 at 11:47 AM Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Thu, Jul 14, 2022 at 11:27:07AM +0200, Andy Shevchenko wrote: > > On Thu, Jul 14, 2022 at 9:13 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > > I have tried two methods so far, as follows > > > ------------------------------------------------------------- > > > /* > > > * prop_val = 1 --> 1 steps --> b'00 > > > * prop_val = 2 ~ 4 --> 4 steps --> b'01 > > > * prop_val = 5 ~ 16 --> 16 steps --> b'10 > > > * prop_val = 17 ~ 64 --> 64 steps --> b'11 > > > */ > > > > So, for 1 --> 0, for 2 --> 1, for 5 --> 2, and for 17 --> 3. > > Now, consider x - 1: > > 0 ( 0 ) --> 0 > > 1 (2^0) --> 1 > > 4 (2^2) --> 2 > > 16 (2^4) --> 3 > > 64 (2^6) --> ? (but let's consider that the range has been checked already) > > > > Since we take the lower limit, it means ffs(): > > > > y = (ffs(x - 1) + 1) / 2; > > > > Does it work for you? > > To be honest, for this tiny table, writing code that *doesn't* require intricate > deciphering together with a huge comment saying what is does would probably be > better: > > prop_val = (prop_val <= 1 ? 0 : > prop_val <= 4 ? 1 : > prop_val <= 16 ? 2 : > 3); > > This would be "obviously correct" and require no comment. Agree. It will also limit checking (and whatever needed for that). -- With Best Regards, Andy Shevchenko
On Thu, Jul 14, 2022 at 11:27 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Jul 14, 2022 at 9:13 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月13日 週三 晚上8:07寫道: ... > > I have tried two methods so far, as follows > > ------------------------------------------------------------- > > /* > > * prop_val = 1 --> 1 steps --> b'00 > > * prop_val = 2 ~ 4 --> 4 steps --> b'01 > > * prop_val = 5 ~ 16 --> 16 steps --> b'10 > > * prop_val = 17 ~ 64 --> 64 steps --> b'11 > > */ > > So, for 1 --> 0, for 2 --> 1, for 5 --> 2, and for 17 --> 3. > Now, consider x - 1: > 0 ( 0 ) --> 0 > 1 (2^0) --> 1 > 4 (2^2) --> 2 > 16 (2^4) --> 3 > 64 (2^6) --> ? (but let's consider that the range has been checked already) > > Since we take the lower limit, it means ffs(): > > y = (ffs(x - 1) + 1) / 2; > > Does it work for you? It wouldn't, because we need to use fls() against it actually. So, 0..1 (-1..0) --> 0 2..4 (1..3) --> 1 5..16 (4..15) --> 2 17..64 (16..63) --> 3 y = x ? ((fls(x - 1) + 1) / 2 : 0; -- With Best Regards, Andy Shevchenko
On Thu, Jul 14, 2022 at 11:43 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Jul 14, 2022 at 11:27 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Thu, Jul 14, 2022 at 9:13 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月13日 週三 晚上8:07寫道: ... > > > * prop_val = 1 --> 1 steps --> b'00 > > > * prop_val = 2 ~ 4 --> 4 steps --> b'01 > > > * prop_val = 5 ~ 16 --> 16 steps --> b'10 > > > * prop_val = 17 ~ 64 --> 64 steps --> b'11 > > > > So, for 1 --> 0, for 2 --> 1, for 5 --> 2, and for 17 --> 3. > > Now, consider x - 1: > > 0 ( 0 ) --> 0 > > 1 (2^0) --> 1 > > 4 (2^2) --> 2 > > 16 (2^4) --> 3 > > 64 (2^6) --> ? (but let's consider that the range has been checked already) > > > > Since we take the lower limit, it means ffs(): > > > > y = (ffs(x - 1) + 1) / 2; > > > > Does it work for you? > > It wouldn't, because we need to use fls() against it actually. > > So, > 0..1 (-1..0) --> 0 > 2..4 (1..3) --> 1 > 5..16 (4..15) --> 2 > 17..64 (16..63) --> 3 > > y = x ? ((fls(x - 1) + 1) / 2 : 0; Okay, I nailed it down, but Daniel is right, it's simpler to have just conditionals. y = x >=2 ? __fls(x - 1) / 2 + 1 : 0; -- With Best Regards, Andy Shevchenko
On Wed, 13 Jul 2022, Andy Shevchenko wrote: > On Wed, Jul 13, 2022 at 12:53 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年7月5日 週二 清晨5:14寫道: > > > On Mon, Jul 4, 2022 at 7:43 AM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > Please, remove unneeded context when replying! > > ... > > > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > > + brightness_val[1] = (brightness - 1) > > > > + >> fls(MT6370_BL_DIM2_MASK); > > > > > > Bad indentation. One line? > > > > Well... if indent to one line, it will be over 80 characters(or called columns?) > > From my understanding, it is not allowed, right?? > > It's allowed to some extent.Use your common sense. > Here it's obviously broken indentation. Refrain from going crazy though - hard limit is 100 chars. -- Lee Jones [李琼斯] Principal Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
© 2016 - 2024 Red Hat, Inc.