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

[Andrew Davis]

On 12/18/24 8:45 AM, Sverdlin, Alexander wrote:
> 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.
> 

If giving specific error messages is not important, then this is
fine how it is.

Andrew

>>> +
>>> +	/* 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;
>>> +}
>