rtc: abx80x: Provide feedback for invalid dt properties

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(-)

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
>

On Fri, May 29, 2020 at 4:54 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> > -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.

Will do.

> > +             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.

I'm using this driver via the Raspberry Pi device tree 'overlay'
mechanism, so I'm setting these parameters in a configuration file and
they are applied by the board's firmware before the kernel is booted.
As a result this is essentially 'runtime' configuration, it's not a
static device tree for the board, so end users like me could run into
this problem.

I'd be fine with changing it to dev_info though, and indicating that
the trickle charger won't be enabled in addition.

On 29/05/2020 06:28:56-0400, Kevin P. Fleming wrote:
> On Fri, May 29, 2020 at 4:54 AM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > > -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.
> 
> Will do.
> 
> > > +             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.
> 
> I'm using this driver via the Raspberry Pi device tree 'overlay'
> mechanism, so I'm setting these parameters in a configuration file and
> they are applied by the board's firmware before the kernel is booted.
> As a result this is essentially 'runtime' configuration, it's not a
> static device tree for the board, so end users like me could run into
> this problem.
> 

This is still board bringup, once it is correct, you will never need the
message anymore. If the issue is device tree validation (i.e. typo in
the string), then maybe you should convert the doc to yaml so you device
tree and overlay could be checked.


> I'd be fine with changing it to dev_info though, and indicating that
> the trickle charger won't be enabled in addition.

dev_info is worse, it is still bloating the kernel and also always
printing in the kernel logs.