Message ID | 20200529001203.235304-1-kevin+linux@km6g.us |
---|---|
State | Superseded |
Headers | show |
Series | rtc: abx80x: Provide feedback for invalid dt properties | expand |
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.
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",
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(-)