Re: [PATCH] rtc: abx80x: Provide feedback for invalid dt properties

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

Hi,


On 28/05/2020 20:12:03-0400, Kevin P. Fleming wrote:
> When the user provides an invalid value for tc-diode or
> tc-resistor generate an error message instead of silently
> ignoring it.
> 
> Signed-off-by: Kevin P. Fleming <kevin+linux@km6g.us>
> ---
>  drivers/rtc/rtc-abx80x.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 3521d8e8dc38..dae046e3484a 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
> @@ -554,7 +554,8 @@ static const struct rtc_class_ops abx80x_rtc_ops = {
>  	.ioctl		= abx80x_ioctl,
>  };
>  
> -static int abx80x_dt_trickle_cfg(struct device_node *np)
> +static int abx80x_dt_trickle_cfg(struct i2c_client *client,
> +				 struct device_node *np)

I would remove np from the parameters and use
struct device_node *np = client->dev.of_node;
in the function.
>  {
>  	const char *diode;
>  	int trickle_cfg = 0;
> @@ -565,12 +566,14 @@ static int abx80x_dt_trickle_cfg(struct device_node *np)
>  	if (ret)
>  		return ret;
>  
> -	if (!strcmp(diode, "standard"))
> +	if (!strcmp(diode, "standard")) {
>  		trickle_cfg |= ABX8XX_TRICKLE_STANDARD_DIODE;
> -	else if (!strcmp(diode, "schottky"))
> +	} else if (!strcmp(diode, "schottky")) {
>  		trickle_cfg |= ABX8XX_TRICKLE_SCHOTTKY_DIODE;
> -	else
> +	} else {
> +		dev_err(&client->dev, "Invalid tc-diode value: %s\n", diode);

Can you make that dev_dbg? This is only ever needed at board bring up/
development time, so it is not necessary to bloat the kernel with more
strings.

>  		return -EINVAL;
> +	}
>  
>  	ret = of_property_read_u32(np, "abracon,tc-resistor", &tmp);
>  	if (ret)
> @@ -580,8 +583,10 @@ static int abx80x_dt_trickle_cfg(struct device_node *np)
>  		if (trickle_resistors[i] == tmp)
>  			break;
>  
> -	if (i == sizeof(trickle_resistors))
> +	if (i == sizeof(trickle_resistors)) {
> +		dev_err(&client->dev, "Invalid tc-resistor value: %u\n", tmp);
>  		return -EINVAL;
> +	}
>  
>  	return (trickle_cfg | i);
>  }
> @@ -793,7 +798,7 @@ static int abx80x_probe(struct i2c_client *client,
>  	}
>  
>  	if (np && abx80x_caps[part].has_tc)
> -		trickle_cfg = abx80x_dt_trickle_cfg(np);
> +		trickle_cfg = abx80x_dt_trickle_cfg(client, np);
>  
>  	if (trickle_cfg > 0) {
>  		dev_info(&client->dev, "Enabling trickle charger: %02x\n",
> -- 
> 2.26.2
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com