Hi Andrew!
On Tue, 2024-12-17 at 09:56 -0600, Andrew Davis wrote:
> > +static int lp8864_fault_check(struct lp8864_led *led)
> > +{
> > + int ret, i;
> > + unsigned int val;
> > +
> > + ret = regmap_read(led->regmap, LP8864_SUPPLY_STATUS, &val);
> > + if (ret)
> > + goto err;
>
> You could probably keep this simple and print the exact error here
> and return, vs the common error message at the end
This would mean 6x dev_err() instead of one? While I have no idea
what we could do with individual error messages here.
> > +
> > + /* Odd bits are status bits, even bits are clear bits */
> > + for (i = 0; i < ARRAY_SIZE(lp8864_supply_status_msg); i++)
> > + if (val & BIT(i * 2 + 1))
> > + dev_warn(&led->client->dev, "%s\n",
> > lp8864_supply_status_msg[i]);
> > +
> > + /*
> > + * Clear bits have an index preceding the corresponding Status bits;
> > + * both have to be written "1" simultaneously to clear the corresponding
> > + * Status bit.
> > + */
> > + if (val)
> > + ret = regmap_write(led->regmap, LP8864_SUPPLY_STATUS, val >> 1
> > | val);
> > + if (ret)
> > + goto err;
> > +
> > + ret = regmap_read(led->regmap, LP8864_BOOST_STATUS, &val);
> > + if (ret)
> > + goto err;
> > +
> > + /* Odd bits are status bits, even bits are clear bits */
> > + for (i = 0; i < ARRAY_SIZE(lp8864_boost_status_msg); i++)
> > + if (val & BIT(i * 2 + 1))
> > + dev_warn(&led->client->dev, "%s\n",
> > lp8864_boost_status_msg[i]);
> > +
> > + if (val)
> > + ret = regmap_write(led->regmap, LP8864_BOOST_STATUS, val >> 1 |
> > val);
> > + if (ret)
> > + goto err;
> > +
> > + ret = regmap_read(led->regmap, LP8864_LED_STATUS, &val);
> > + if (ret)
> > + goto err;
> > +
> > + /*
> > + * Clear already reported faults that maintain their value until device
> > + * power-down
> > + */
> > + val &= ~led->led_status_mask;
> > +
> > + for (i = 0; i < ARRAY_SIZE(lp8864_led_status_msg); i++)
> > + if (lp8864_led_status_msg[i] && val & BIT(i))
> > + dev_warn(&led->client->dev, "%s\n",
> > lp8864_led_status_msg[i]);
> > +
> > + /*
> > + * Mark those which maintain their value until device power-down as
> > + * "already reported"
> > + */
> > + led->led_status_mask |= val & ~LP8864_LED_STATUS_WR_MASK;
> > +
> > + /*
> > + * Only bits 14, 12, 10 have to be cleared here, but others are RO,
> > + * we don't care what we write to them.
> > + */
> > + if (val & LP8864_LED_STATUS_WR_MASK)
> > + ret = regmap_write(led->regmap, LP8864_LED_STATUS, val >> 1 |
> > val);
> > + if (ret)
> > + goto err;
> > +
> > + return 0;
> > +
> > +err:
> > + dev_err(&led->client->dev, "Failed to read/clear faults (%pe)\n",
> > ERR_PTR(ret));
> > +
> > + return ret;
> > +}
--
Alexander Sverdlin
Siemens AG
www.siemens.com
