[PATCH v4 2/2] leds: lp8864: New driver

[Sverdlin, Alexander]

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