Hi Daniel,
Thank you very much for your review, for some reason it ended in my spam
box, so I only saw it just now.
On 8/11/25 16:15, Daniel Thompson wrote:
On Fri, Jul 25, 2025 at 01:09:24PM +0200, Maud Spierings via B4 Relay wrote:
From: Maud Spierings <maudspieri...@gocontroll.com>
The Maxim MAX25014 is a 4-channel automotive grade backlight driver IC
with intgrated boost controller.
Signed-off-by: Maud Spierings maudspieri...@gocontroll.com
---
MAINTAINERS | 2 +
drivers/video/backlight/Kconfig | 7 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/max25014.c | 449 +++++++++++++++++++++++++++++++++
include/linux/platform_data/max25014.h | 24 ++
Who else included this header file? Can the code here simply be included
in the C file?
That was my instinct too, I was following a clearly incorrect pattern
from another driver, merged the fields from that struct into the main
max25014 struct.
diff --git a/drivers/video/backlight/max25014.c
b/drivers/video/backlight/max25014.c
new file mode 100644
index
0000000000000000000000000000000000000000..371b6017953ae5955f4dfef921980dfdedd65d85
--- /dev/null
+++ b/drivers/video/backlight/max25014.c
@@ -0,0 +1,449 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Backlight driver for Maxim MAX25014
+ *
+ * Copyright (C) 2025 GOcontroll B.V.
+ * Author: Maud Spierings <maudspieri...@gocontroll.com>
+ */
+
+#include <linux/backlight.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/platform_data/max25014.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define MAX25014_ISET_DEFAULT_100 11U
+#define MAX_BRIGHTNESS (100U)
+#define MIN_BRIGHTNESS (0U)
+#define TON_MAX (130720U) /* @153Hz */
+#define TON_STEP (1307U) /* @153Hz */
+#define TON_MIN (0U)
+
+#define MAX25014_DEV_ID (0x00U)
+#define MAX25014_REV_ID (0x01U)
+#define MAX25014_ISET (0x02U)
+#define MAX25014_IMODE (0x03U)
+#define MAX25014_TON1H (0x04U)
+#define MAX25014_TON1L (0x05U)
+#define MAX25014_TON2H (0x06U)
+#define MAX25014_TON2L (0x07U)
+#define MAX25014_TON3H (0x08U)
+#define MAX25014_TON3L (0x09U)
+#define MAX25014_TON4H (0x0AU)
+#define MAX25014_TON4L (0x0BU)
+#define MAX25014_TON_1_4_LSB (0x0CU)
+#define MAX25014_SETTING (0x12U)
+#define MAX25014_DISABLE (0x13U)
+#define MAX25014_BSTMON (0x14U)
+#define MAX25014_IOUT1 (0x15U)
+#define MAX25014_IOUT2 (0x16U)
+#define MAX25014_IOUT3 (0x17U)
+#define MAX25014_IOUT4 (0x18U)
+#define MAX25014_OPEN (0x1BU)
+#define MAX25014_SHORT_GND (0x1CU)
+#define MAX25014_SHORT_LED (0x1DU)
+#define MAX25014_MASK (0x1EU)
+#define MAX25014_DIAG (0x1FU)
Forcing all these constants to be unsigned is unusual. Is it really
needed?
Removed all the U's
+#define MAX25014_IMODE_HDIM BIT(2)
+#define MAX25014_ISET_ENABLE BIT(5)
+#define MAX25014_ISET_PSEN BIT(4)
+#define MAX25014_DIAG_HW_RST BIT(2)
+#define MAX25014_SETTING_FPWM GENMASK(6, 4)
+
+struct max25014;
This is redundant. Remove.
Thats an interesting leftover, removed.
+
+struct max25014 {
+ const char *chipname;
Why keep this value around? It is only used during the probe.
+ struct i2c_client *client;
+ struct backlight_device *bl;
+ struct device *dev;
It is necessary to cache this, is it just a copy of client->dev)?
yep completely unnecessary, removed.
+ struct regmap *regmap;
+ struct max25014_platform_data *pdata;
+ struct gpio_desc *enable;
+ struct regulator *vin; /* regulator for boost converter Vin rail */
+};
+
+static const struct regmap_config max25014_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = MAX25014_DIAG,
+};
+
+/**
+ * @brief get the bit mask for the DISABLE register.
+ *
+ * @param strings the led string configuration array.
+ * @return uint8_t bits to set in the register.
+ */
+static uint8_t strings_mask(struct max25014 *maxim)
+{
+ uint8_t res, i;
+
+ for (i = 0; i < 4; i++) {
+ if (maxim->pdata->strings[i] == 0)
+ res |= 1 << i;
+ }
+ return res;
Could this converison have happened during DT parsing?
inlined it, changed the strings field in to strings_mask and only store
the mask it calculates.
+}
+
+/**
+ * @brief control the brightness with i2c registers
+ *
+ * @param regmap trivial
+ * @param brt brightness
+ * @return int
+ */
+static int max25014_register_control(struct regmap *regmap, uint32_t brt)
+{
+ uint32_t reg = TON_STEP * brt;
+ int ret;
+ /*
+ * 18 bit number lowest, 2 bits in first register,
+ * next lowest 8 in the L register, next 8 in the H register
+ * Seemingly setting the strength of only one string controls all of
+ * them, individual settings don't affect the outcome.
+ */
+
+ ret = regmap_write(regmap, MAX25014_TON_1_4_LSB, reg & 0b00000011);
+ if (ret != 0)
+ return ret;
+ ret = regmap_write(regmap, MAX25014_TON1L, (reg >> 2) & 0b11111111);
+ if (ret != 0)
+ return ret;
+ return regmap_write(regmap, MAX25014_TON1H, (reg >> 10) & 0b11111111);
+}
+
+static int max25014_check_errors(struct max25014 *maxim)
+{
+ uint8_t i;
+ int ret;
+ uint32_t val;
+
+ ret = regmap_read(maxim->regmap, MAX25014_OPEN, &val);
+ if (ret != 0)
+ return ret;
+ if (val > 0) {
+ dev_err(maxim->dev, "Open led strings detected on:\n");
+ for (i = 0; i < 4; i++) {
+ if (val & 1 << i)
+ dev_err(maxim->dev, "string %d\n", i + 1);
+ }
+ return -EIO;
+ }
+
+ ret = regmap_read(maxim->regmap, MAX25014_SHORT_GND, &val);
+ if (ret != 0)
+ return ret;
+ if (val > 0) {
+ dev_err(maxim->dev, "Short to ground detected on:\n");
+ for (i = 0; i < 4; i++) {
+ if (val & 1 << i)
+ dev_err(maxim->dev, "string %d\n", i + 1);
+ }
+ return -EIO;
+ }
+
+ ret = regmap_read(maxim->regmap, MAX25014_SHORT_GND, &val);
+ if (ret != 0)
+ return ret;
+ if (val > 0) {
+ dev_err(maxim->dev, "Shorted led detected on:\n");
+ for (i = 0; i < 4; i++) {
+ if (val & 1 << i)
+ dev_err(maxim->dev, "string %d\n", i + 1);
+ }
+ return -EIO;
+ }
+
+ ret = regmap_read(maxim->regmap, MAX25014_DIAG, &val);
+ if (ret != 0)
+ return ret;
+ /*
+ * The HW_RST bit always starts at 1 after power up.
+ * It is reset on first read, does not indicate an error.
+ */
+ if (val > 0 && val != MAX25014_DIAG_HW_RST) {
+ if (val & 0b1)
+ dev_err(maxim->dev, "Overtemperature shutdown\n");
+ if (val & 0b10)
+ dev_warn(maxim->dev,
+ "Chip is getting too hot (>125C)\n");
+ if (val & 0b1000)
+ dev_err(maxim->dev, "Boost converter overvoltage\n");
+ if (val & 0b10000)
+ dev_err(maxim->dev, "Boost converter undervoltage\n");
+ if (val & 0b100000)
+ dev_err(maxim->dev, "IREF out of range\n");
+ return -EIO;
+ }
+ return 0;
+}
+
+/*
+ * 1. disable unused strings
+ * 2. set dim mode
+ * 3. set initial brightness
+ * 4. set setting register
+ * 5. enable the backlight
+ */
+static int max25014_configure(struct max25014 *maxim)
+{
+ int ret;
+ uint32_t val;
+
+ ret = regmap_write(maxim->regmap, MAX25014_DISABLE,
+ strings_mask(maxim));
+ if (ret != 0)
+ return ret;
+
+ ret = regmap_write(maxim->regmap, MAX25014_IMODE, MAX25014_IMODE_HDIM);
+ if (ret != 0)
+ return ret;
+
+ max25014_register_control(maxim->regmap,
+ maxim->pdata->initial_brightness);
+
+ ret = regmap_read(maxim->regmap, MAX25014_SETTING, &val);
+ if (ret != 0)
+ return ret;
+
+ ret = regmap_write(
+ maxim->regmap, MAX25014_SETTING,
+ val & ~MAX25014_SETTING_FPWM);
+ if (ret != 0)
+ return ret;
+
+ ret = regmap_write(maxim->regmap, MAX25014_ISET,
+ maxim->pdata->iset | MAX25014_ISET_ENABLE |
MAX25014_ISET_PSEN);
+ return ret;
+}
+
+static int max25014_update_status(struct backlight_device *bl_dev)
+{
+ struct max25014 *maxim = bl_get_data(bl_dev);
+
+ if (bl_dev->props.state & BL_CORE_SUSPENDED)
+ bl_dev->props.brightness = 0;
+
+ return max25014_register_control(maxim->regmap,
bl_dev->props.brightness);
+}
+
+static const struct backlight_ops max25014_bl_ops = {
+ .options = BL_CORE_SUSPENDRESUME,
+ .update_status = max25014_update_status,
+};
+
+static int max25014_backlight_register(struct max25014 *maxim)
+{
+ struct backlight_device *bl;
+ struct backlight_properties props;
+ struct max25014_platform_data *pdata = maxim->pdata;
+
+ memset(&props, 0, sizeof(props));
+ props.type = BACKLIGHT_PLATFORM;
+ props.max_brightness = MAX_BRIGHTNESS;
+
+ if (pdata->initial_brightness > props.max_brightness)
+ pdata->initial_brightness = props.max_brightness;
Handle this during DT parsing.
It is already handled there, this is double, so dropped.
+
+ props.brightness = pdata->initial_brightness;
+
+ bl = devm_backlight_device_register(maxim->dev, maxim->chipname,
maxim->dev,
+ maxim, &max25014_bl_ops, &props);
+ if (IS_ERR(bl))
+ return PTR_ERR(bl);
+
+ maxim->bl = bl;
+
+ return 0;
+}
Can max25014_backlight_register() be moved into the probe function?
There is no special control flow here so this function doesn't make the
probe function any simpler.
Done.
+
+#ifdef CONFIG_OF
+static int max25014_parse_dt(struct max25014 *maxim)
+{
+ struct device *dev = maxim->dev;
+ struct device_node *node = dev->of_node;
+ struct max25014_platform_data *pdata;
+
+ int res;
+
+ if (!node) {
+ dev_err(dev, "no platform data\n");
+ return -EINVAL;
+ }
+
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ res = of_property_count_u32_elems(node, "maxim,strings");
+ if (res == 4) {
+ of_property_read_u32_array(node, "maxim,strings",
pdata->strings, 4);
+ } else {
+ dev_err(dev, "strings property not correctly defined\n");
+ return -EINVAL;
+ }
+
+ pdata->initial_brightness = 50U;
+ of_property_read_u32(node, "default-brightness",
&pdata->initial_brightness);
+ pdata->iset = MAX25014_ISET_DEFAULT_100;
+ of_property_read_u32(node, "maxim,iset", &pdata->iset);
+
+ if (pdata->iset < 0 || pdata->iset > 15) {
+ dev_err(dev,
+ "Invalid iset, should be a value from 0-15, entered was
%d\n",
+ pdata->iset);
+ return -EINVAL;
+ }
+
+ if (pdata->initial_brightness < 0 || pdata->initial_brightness > 100) {
+ dev_err(dev,
+ "Invalid initial brightness, should be a value from 0-100,
entered was %d\n",
+ pdata->initial_brightness);
+ return -EINVAL;
+ }
+
+ maxim->pdata = pdata;
+
+ return 0;
+}
+#else
+static int max25014_parse_dt(struct max25014 *maxim)
+{
+ dev_err(maxim->dev,
+ "CONFIG_OF not configured, unable to parse devicetree");
+ return -EINVAL;
+}
What is the point of this method? New drivers shouldn't support platform
data so CONFIG_OF is required for this driver to work at all.
I think it is me following a bad pattern again, dropped the ifdef.
+#endif
+
+static int max25014_probe(struct i2c_client *cl)
...
Kind regards,
Maud
