Thanks for the review.
On 12/4/25 17:17, Daniel Thompson wrote:
On Mon, Dec 01, 2025 at 12:53:21PM +0100, Maud Spierings via B4 Relay wrote:
From: Maud Spierings <[email protected]>
The Maxim MAX25014 is a 4-channel automotive grade backlight driver IC
with integrated boost controller.
Signed-off-by: Maud Spierings <[email protected]>
<snip>
+static int max25014_check_errors(struct max25014 *maxim)
+{
+ uint32_t val;
+ uint8_t i;
+ int ret;
+
+ ret = regmap_read(maxim->regmap, MAX25014_OPEN, &val);
+ if (ret)
+ return ret;
+ if (val) {
+ dev_err(&maxim->client->dev, "Open led strings detected on:\n");
+ for (i = 0; i < 4; i++) {
+ if (val & 1 << i)
+ dev_err(&maxim->client->dev, "string %d\n", i +
1);
+ }
+ return -EIO;
+ }
+
+ ret = regmap_read(maxim->regmap, MAX25014_SHORT_GND, &val);
+ if (ret)
+ return ret;
+ if (val) {
+ dev_err(&maxim->client->dev, "Short to ground detected on:\n");
+ for (i = 0; i < 4; i++) {
+ if (val & 1 << i)
+ dev_err(&maxim->client->dev, "string %d\n", i +
1);
+ }
+ return -EIO;
+ }
+
+ ret = regmap_read(maxim->regmap, MAX25014_SHORT_GND, &val);
Shouldn't this be MAX25014_SHORT_LED?
yep you are absolutely right
+ if (ret)
+ return ret;
+ if (val) {
+ dev_err(&maxim->client->dev, "Shorted led detected on:\n");
+ for (i = 0; i < 4; i++) {
+ if (val & 1 << i)
+ dev_err(&maxim->client->dev, "string %d\n", i +
1);
+ }
+ return -EIO;
+ }
+
+ ret = regmap_read(maxim->regmap, MAX25014_DIAG, &val);
+ if (ret)
+ 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 && val != MAX25014_DIAG_HW_RST) {
+ if (val & MAX25014_DIAG_OT)
+ dev_err(&maxim->client->dev,
+ "Overtemperature shutdown\n");
+ if (val & MAX25014_DIAG_OTW)
+ dev_err(&maxim->client->dev,
+ "Chip is getting too hot (>125C)\n");
+ if (val & MAX25014_DIAG_BSTOV)
+ dev_err(&maxim->client->dev,
+ "Boost converter overvoltage\n");
+ if (val & MAX25014_DIAG_BSTUV)
+ dev_err(&maxim->client->dev,
+ "Boost converter undervoltage\n");
+ if (val & MAX25014_DIAG_IREFOOR)
+ dev_err(&maxim->client->dev, "IREF out of range\n");
+ return -EIO;
+ }
+ return 0;
+}
+
+/*
+ * 1. disable unused strings
+ * 2. set dim mode
+ * 3. set setting register
+ * 4. enable the backlight
+ */
+static int max25014_configure(struct max25014 *maxim, int initial_state)
+{
+ uint32_t val;
+ int ret;
+
+ /*
+ * Strings can only be disabled when MAX25014_ISET_ENA == 0, check if
+ * it needs to be changed at all to prevent the backlight flashing when
+ * it is configured correctly by the bootloader
+ */
Attach the comment to the if statement rather than the read.
will do
+ ret = regmap_read(maxim->regmap, MAX25014_DISABLE, &val);
+ if (ret)
+ return ret;
+
+ if (!((val & MAX25014_DISABLE_DIS_MASK) == maxim->strings_mask)) {
+ if (initial_state == BACKLIGHT_POWER_ON) {
+ ret = regmap_write(maxim->regmap, MAX25014_ISET, 0);
+ if (ret)
+ return ret;
+ }
+ ret = regmap_write(maxim->regmap, MAX25014_DISABLE,
maxim->strings_mask);
+ if (ret)
+ return ret;
+ }
+
+ ret = regmap_write(maxim->regmap, MAX25014_IMODE, MAX25014_IMODE_HDIM);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(maxim->regmap, MAX25014_SETTING, &val);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(maxim->regmap, MAX25014_SETTING,
+ val & ~MAX25014_SETTING_FPWM);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(maxim->regmap, MAX25014_ISET,
+ maxim->iset | MAX25014_ISET_ENA |
+ MAX25014_ISET_PSEN);
+ return ret;
+}
+
+static int max25014_update_status(struct backlight_device *bl_dev)
+{
+ struct max25014 *maxim = bl_get_data(bl_dev);
+ uint32_t reg;
+ int ret;
+
+ if (backlight_is_blank(maxim->bl))
+ bl_dev->props.brightness = 0;
This isn't right. Why would you change the backlight level just because
it is currently blanked (and sorry I missed this one last time).
so just remove this bit then jeah?
+
+ reg = TON_STEP * bl_dev->props.brightness;
The correct way to honour blanking is just go call
backlight_get_brightness() instead of reading the property directly.
will do.
+
+ /*
+ * 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(maxim->regmap, MAX25014_TON_1_4_LSB, reg &
0b00000011);
+ if (ret != 0)
+ return ret;
+ ret = regmap_write(maxim->regmap, MAX25014_TON1L, (reg >> 2) &
0b11111111);
+ if (ret != 0)
+ return ret;
+ return regmap_write(maxim->regmap, MAX25014_TON1H, (reg >> 10) &
0b11111111);
+}
+
+static const struct backlight_ops max25014_bl_ops = {
+ .options = BL_CORE_SUSPENDRESUME,
+ .update_status = max25014_update_status,
+};
+
+static int max25014_parse_dt(struct max25014 *maxim,
+ uint32_t *initial_brightness)
+{
+ struct device *dev = &maxim->client->dev;
+ struct device_node *node = dev->of_node;
+ struct fwnode_handle *child;
+ uint32_t strings[4];
+ int res, i;
+
+ if (!node)
+ return dev_err_probe(dev, -EINVAL, "no platform data\n");
+
+ child = device_get_next_child_node(dev, NULL);
+ if (child) {
+ res = fwnode_property_count_u32(child, "led-sources");
+ if (res > 0) {
+ fwnode_property_read_u32_array(child, "led-sources",
+ strings, res);
+
+ /* set all strings as disabled, then enable those in
led-sources*/
+ maxim->strings_mask = 0xf;
+ for (i = 0; i < res; i++) {
+ if (strings[i] <= 4)
+ maxim->strings_mask &= ~BIT(strings[i]);
+ }
+ }
+
+ fwnode_property_read_u32(child, "default-brightness",
+ initial_brightness);
+
+ fwnode_handle_put(child);
+ }
+
+ maxim->iset = MAX25014_ISET_DEFAULT_100;
+ of_property_read_u32(node, "maxim,iset", &maxim->iset);
+
+ if (maxim->iset > 15)
+ return dev_err_probe(dev, -EINVAL,
+ "Invalid iset, should be a value from 0-15,
entered was %d\n",
+ maxim->iset);
+
+ if (*initial_brightness > 100)
+ return dev_err_probe(dev, -EINVAL,
+ "Invalid initial brightness, should be a value
from 0-100, entered was %d\n",
+ *initial_brightness);
+
+ return 0;
+}
+
+static int max25014_probe(struct i2c_client *cl)
+{
+ const struct i2c_device_id *id = i2c_client_get_device_id(cl);
+ struct backlight_properties props;
+ uint32_t initial_brightness = 50;
+ struct backlight_device *bl;
+ struct max25014 *maxim;
+ int ret;
+
+ maxim = devm_kzalloc(&cl->dev, sizeof(struct max25014), GFP_KERNEL);
+ if (!maxim)
+ return -ENOMEM;
+
+ maxim->client = cl;
+
+ ret = max25014_parse_dt(maxim, &initial_brightness);
+ if (ret)
+ return ret;
+
+ maxim->vin = devm_regulator_get(&maxim->client->dev, "power");
+ if (IS_ERR(maxim->vin)) {
+ return dev_err_probe(&maxim->client->dev, PTR_ERR(maxim->vin),
+ "failed to get power-supply");
+ }
+
+ ret = regulator_enable(maxim->vin);
+ if (ret)
+ return dev_err_probe(&maxim->client->dev, ret,
+ "failed to enable power-supply\n");
Can this use devm_regulator_get_enable()?
Yeah guess I'll just switch to that for now, if ever power management
gets implemented it can be figured out if regulator control is desired.
+
+ maxim->enable = devm_gpiod_get_optional(&maxim->client->dev, "enable",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(maxim->enable)) {
+ ret = dev_err_probe(&maxim->client->dev, PTR_ERR(maxim->enable),
+ "failed to get enable gpio\n");
+ goto disable_vin;
+ }
+
+ /* Datasheet Electrical Characteristics tSTARTUP 2ms */
+ fsleep(2000);
+
+ maxim->regmap = devm_regmap_init_i2c(cl, &max25014_regmap_config);
+ if (IS_ERR(maxim->regmap)) {
+ ret = dev_err_probe(&maxim->client->dev, PTR_ERR(maxim->regmap),
+ "failed to initialize the i2c regmap\n");
+ goto disable_full;
+ }
+
+ i2c_set_clientdata(cl, maxim);
+
+ ret = max25014_check_errors(maxim);
+ if (ret) { /* error is already reported in the above function */
+ goto disable_full;
+ }
+
+ ret = max25014_initial_power_state(maxim);
+ if (ret < 0) {
+ dev_err_probe(&maxim->client->dev, ret, "Could not get enabled
state\n");
+ goto disable_full;
+ }
+
+ memset(&props, 0, sizeof(struct backlight_properties));
+ props.type = BACKLIGHT_PLATFORM;
+ props.max_brightness = MAX_BRIGHTNESS;
+ props.brightness = initial_brightness;
+ props.scale = BACKLIGHT_SCALE_LINEAR;
+ props.power = ret;
+
+ ret = max25014_configure(maxim, ret);
+ if (ret) {
+ dev_err_probe(&maxim->client->dev, ret, "device config error");
+ goto disable_full;
+ }
+
+ bl = devm_backlight_device_register(&maxim->client->dev, id->name,
+ &maxim->client->dev, maxim,
+ &max25014_bl_ops, &props);
+ if (IS_ERR(bl)) {
+ ret = dev_err_probe(&maxim->client->dev, PTR_ERR(bl),
+ "failed to register backlight\n");
+ goto disable_full;
+ }
+
+ maxim->bl = bl;
+
+ backlight_update_status(maxim->bl);
+
+ return 0;
+
+disable_full:
+ gpiod_set_value_cansleep(maxim->enable, 0);
Why is this needed? It was only ever set by devm_gpiod_get_optional().
oops thats a leftover from before that change, good spot.
+disable_vin:
+ regulator_disable(maxim->vin);
This is also not needed if you use devm_regulator_get_enable().
jeah I'll drop this then too
kind regards,
Maud
