| Message ID | 1480331524-18741-2-git-send-email-wxt@rock-chips.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
Hello, On Mon, Nov 28, 2016 at 07:12:00PM +0800, Caesar Wang wrote: > From: Brian Norris <briannorris@chromium.org> > > These error messages don't give much information about what went wrong. > It would be nice, for one, to see what invalid temperature was being > requested when conversion fails. It's also good to return an error when > we can't handle a conversion properly. > > While we're at it, fix the grammar too. > > Signed-off-by: Brian Norris <briannorris@chromium.org> > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > > --- > > Changes in v3: None > Changes in v2: None > Changes in v1: > - The original Brian posted on https://patchwork.kernel.org/patch/9437686 > Note: it'd probably be even nicer to know which sensor this was, but we've > kinda abstracted that one away by this point... > > drivers/thermal/rockchip_thermal.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c > index b811b0f..26c247c 100644 > --- a/drivers/thermal/rockchip_thermal.c > +++ b/drivers/thermal/rockchip_thermal.c > @@ -424,7 +424,8 @@ static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table, > } > > exit: > - pr_err("Invalid the conversion, error=%d\n", error); > + pr_err("%s: invalid temperature, temp=%d error=%d\n", > + __func__, temp, error); > return error; > } > > @@ -475,7 +476,9 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code, > } > break; > default: > - pr_err("Invalid the conversion table\n"); > + pr_err("%s: invalid conversion table, mode=%d\n", > + __func__, table.mode); Given that we are improving messages, would it be more informative to say that you have an invalid table mode? > + return -EINVAL; > } > > /* > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Nov 28, 2016 at 05:51:55PM -0800, Eduardo Valentin wrote: > On Mon, Nov 28, 2016 at 07:12:00PM +0800, Caesar Wang wrote: > > From: Brian Norris <briannorris@chromium.org> > > > > These error messages don't give much information about what went wrong. > > It would be nice, for one, to see what invalid temperature was being > > requested when conversion fails. It's also good to return an error when > > we can't handle a conversion properly. > > > > While we're at it, fix the grammar too. > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > > > > --- > > > > Changes in v3: None > > Changes in v2: None > > Changes in v1: > > - The original Brian posted on https://patchwork.kernel.org/patch/9437686 > > Note: it'd probably be even nicer to know which sensor this was, but we've > > kinda abstracted that one away by this point... > > > > drivers/thermal/rockchip_thermal.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c > > index b811b0f..26c247c 100644 > > --- a/drivers/thermal/rockchip_thermal.c > > +++ b/drivers/thermal/rockchip_thermal.c > > @@ -424,7 +424,8 @@ static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table, > > } > > > > exit: > > - pr_err("Invalid the conversion, error=%d\n", error); > > + pr_err("%s: invalid temperature, temp=%d error=%d\n", > > + __func__, temp, error); > > return error; > > } > > > > @@ -475,7 +476,9 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code, > > } > > break; > > default: > > - pr_err("Invalid the conversion table\n"); > > + pr_err("%s: invalid conversion table, mode=%d\n", > > + __func__, table.mode); > > Given that we are improving messages, would it be more informative to > say that you have an invalid table mode? I considered the mode and ID listing to go hand in hand, so it was the whole table that is wrong. But it is just as well to say the "table mode" is wrong. Maybe even better: "%s: unknown table mode: %d\n". And I guess same answer for patch 4, where you had the same question. Brian > > + return -EINVAL; > > } > > > > /* > > -- > > 2.7.4 > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey, On Mon, Nov 28, 2016 at 09:47:29PM -0800, Brian Norris wrote: > Hi, > <cut> > > > + __func__, table.mode); > > > > Given that we are improving messages, would it be more informative to > > say that you have an invalid table mode? > > I considered the mode and ID listing to go hand in hand, so it was the > whole table that is wrong. But it is just as well to say the "table > mode" is wrong. > > Maybe even better: "%s: unknown table mode: %d\n". > Yup, that works for me, even better. > And I guess same answer for patch 4, where you had the same question. yes > > Brian > > > > + return -EINVAL; -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
在 2016年11月30日 13:04, Eduardo Valentin 写道: > Hey, > > On Mon, Nov 28, 2016 at 09:47:29PM -0800, Brian Norris wrote: >> Hi, >> > <cut> > >>>> + __func__, table.mode); >>> Given that we are improving messages, would it be more informative to >>> say that you have an invalid table mode? >> I considered the mode and ID listing to go hand in hand, so it was the >> whole table that is wrong. But it is just as well to say the "table >> mode" is wrong. >> >> Maybe even better: "%s: unknown table mode: %d\n". >> > Yup, that works for me, even better. Done. Sorry for delay. >> And I guess same answer for patch 4, where you had the same question. > yes > >> Brian >> >>>> + return -EINVAL; > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c index b811b0f..26c247c 100644 --- a/drivers/thermal/rockchip_thermal.c +++ b/drivers/thermal/rockchip_thermal.c @@ -424,7 +424,8 @@ static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table, } exit: - pr_err("Invalid the conversion, error=%d\n", error); + pr_err("%s: invalid temperature, temp=%d error=%d\n", + __func__, temp, error); return error; } @@ -475,7 +476,9 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code, } break; default: - pr_err("Invalid the conversion table\n"); + pr_err("%s: invalid conversion table, mode=%d\n", + __func__, table.mode); + return -EINVAL; } /*