* [PATCH] rtc: abx80x: Provide feedback for invalid dt properties
@ 2020-05-29 0:12 Kevin P. Fleming
2020-05-29 8:54 ` Alexandre Belloni
0 siblings, 1 reply; 4+ messages in thread
From: Kevin P. Fleming @ 2020-05-29 0:12 UTC (permalink / raw)
To: Alessandro Zummo, Alexandre Belloni, linux-rtc; +Cc: Kevin P. Fleming
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)
{
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);
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] rtc: abx80x: Provide feedback for invalid dt properties
2020-05-29 0:12 [PATCH] rtc: abx80x: Provide feedback for invalid dt properties Kevin P. Fleming
@ 2020-05-29 8:54 ` Alexandre Belloni
2020-05-29 10:28 ` Kevin P. Fleming
0 siblings, 1 reply; 4+ messages in thread
From: Alexandre Belloni @ 2020-05-29 8:54 UTC (permalink / raw)
To: Kevin P. Fleming; +Cc: Alessandro Zummo, linux-rtc
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] rtc: abx80x: Provide feedback for invalid dt properties
2020-05-29 8:54 ` Alexandre Belloni
@ 2020-05-29 10:28 ` Kevin P. Fleming
2020-05-29 13:16 ` Alexandre Belloni
0 siblings, 1 reply; 4+ messages in thread
From: Kevin P. Fleming @ 2020-05-29 10:28 UTC (permalink / raw)
To: Alexandre Belloni; +Cc: Kevin P. Fleming, Alessandro Zummo, linux-rtc
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] rtc: abx80x: Provide feedback for invalid dt properties
2020-05-29 10:28 ` Kevin P. Fleming
@ 2020-05-29 13:16 ` Alexandre Belloni
0 siblings, 0 replies; 4+ messages in thread
From: Alexandre Belloni @ 2020-05-29 13:16 UTC (permalink / raw)
To: Kevin P. Fleming; +Cc: Alessandro Zummo, linux-rtc
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.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-29 13:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-29 0:12 [PATCH] rtc: abx80x: Provide feedback for invalid dt properties Kevin P. Fleming
2020-05-29 8:54 ` Alexandre Belloni
2020-05-29 10:28 ` Kevin P. Fleming
2020-05-29 13:16 ` Alexandre Belloni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).