| Message ID | 1479513177-81504-2-git-send-email-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Brian, 在 2016年11月19日 07:52, Brian Norris 写道: > 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> > --- > 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 e227a9f0acf7..35554d146b9d 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); I have do some similar for rockchip inside thermal driver. Forget to send for upstream. :( exit: pr_err("%s: Invalid conversion table: code=%d, temperature=%d\n", __func__, error, temp); > 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; > } > > /*
在 2016年11月19日 11:31, Caesar Wang 写道: > Brian, > > 在 2016年11月19日 07:52, Brian Norris 写道: >> 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> Reviewed-by: Caesar Wang@rock-chips.com Thanks the fixes. -Caesar >> --- >> 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 e227a9f0acf7..35554d146b9d 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); > > I have do some similar for rockchip inside thermal driver. Forget to > send for upstream. :( > exit: > pr_err("%s: Invalid conversion table: code=%d, temperature=%d\n", > __func__, error, temp); > >> 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; >> } >> /* >
在 2016年11月19日 11:31, Caesar Wang 写道: > Brian, > > 在 2016年11月19日 07:52, Brian Norris 写道: >> 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> >> --- >> 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 e227a9f0acf7..35554d146b9d 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); > > I have do some similar for rockchip inside thermal driver. Forget to > send for upstream. :( > exit: > pr_err("%s: Invalid conversion table: code=%d, temperature=%d\n", > __func__, error, temp); > >> 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; CHECK: Alignment should match open parenthesis #428: FILE: drivers/thermal/rockchip_thermal.c:428: + pr_err("%s: invalid temperature, temp=%d error=%d\n", + __func__, temp, error); CHECK: Alignment should match open parenthesis #480: FILE: drivers/thermal/rockchip_thermal.c:480: + pr_err("%s: invalid conversion table, mode=%d\n", + __func__, table->mode); I'm ready to resend all rockchip thermal patches. (contain them) >> } >> /* > > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Tue, Nov 22, 2016 at 09:51:23AM +0800, Caesar Wang wrote: > CHECK: Alignment should match open parenthesis > #428: FILE: drivers/thermal/rockchip_thermal.c:428: > + pr_err("%s: invalid temperature, temp=%d error=%d\n", > + __func__, temp, error); > > CHECK: Alignment should match open parenthesis > #480: FILE: drivers/thermal/rockchip_thermal.c:480: > + pr_err("%s: invalid conversion table, mode=%d\n", > + __func__, table->mode); What patch are you checking? I ran mine through checkpatch, and there are no problems. Did you perhaps mangle the tabs into spaces when you saved the patch? > I'm ready to resend all rockchip thermal patches. (contain them) I see no reason to resend so far; the only criticism was on the 1st patch (a non-critical patch to the core thermal code; the others are relatively independent, as long as you don't care that I'm adding another error return without fixing up the broken CONFIG_THERMAL_EMULATION support). Brian
在 2016年11月22日 10:15, Brian Norris 写道: > On Tue, Nov 22, 2016 at 09:51:23AM +0800, Caesar Wang wrote: >> CHECK: Alignment should match open parenthesis >> #428: FILE: drivers/thermal/rockchip_thermal.c:428: >> + pr_err("%s: invalid temperature, temp=%d error=%d\n", >> + __func__, temp, error); >> >> CHECK: Alignment should match open parenthesis >> #480: FILE: drivers/thermal/rockchip_thermal.c:480: >> + pr_err("%s: invalid conversion table, mode=%d\n", >> + __func__, table->mode); > What patch are you checking? I ran mine through checkpatch, and there > are no problems. That just checkcode on Chromeos kernelv4.4, that trivial things :) $chromiumos/src/third_party/kernel/v4.4$ checkcode drivers/thermal/rockchip_thermal.c CHECK: Alignment should match open parenthesis #428: FILE: drivers/thermal/rockchip_thermal.c:428: + pr_err("%s: invalid temperature, temp=%d error=%d\n", + __func__, temp, error); ... vi drivers/thermal/rockchip_thermal.c +428 or vi drivers/thermal/rockchip_thermal.c +480, > Did you perhaps mangle the tabs into spaces when you > saved the patch? > >> I'm ready to resend all rockchip thermal patches. (contain them) > I see no reason to resend so far; the only criticism was on the 1st > patch (a non-critical patch to the core thermal code; the others are > relatively independent, as long as you don't care that I'm adding > another error return without fixing up the broken > CONFIG_THERMAL_EMULATION support). > > Brian >
On Tue, Nov 22, 2016 at 10:33:27AM +0800, Caesar Wang wrote: > 在 2016年11月22日 10:15, Brian Norris 写道: > >On Tue, Nov 22, 2016 at 09:51:23AM +0800, Caesar Wang wrote: > >>CHECK: Alignment should match open parenthesis > >>#428: FILE: drivers/thermal/rockchip_thermal.c:428: > >>+ pr_err("%s: invalid temperature, temp=%d error=%d\n", > >>+ __func__, temp, error); > >> > >>CHECK: Alignment should match open parenthesis > >>#480: FILE: drivers/thermal/rockchip_thermal.c:480: > >>+ pr_err("%s: invalid conversion table, mode=%d\n", > >>+ __func__, table->mode); > >What patch are you checking? I ran mine through checkpatch, and there > >are no problems. > > That just checkcode on Chromeos kernelv4.4, that trivial things :) > $chromiumos/src/third_party/kernel/v4.4$ checkcode I'm not familiar with that tool, and that repository isn't upstream but... > drivers/thermal/rockchip_thermal.c > CHECK: Alignment should match open parenthesis > #428: FILE: drivers/thermal/rockchip_thermal.c:428: > + pr_err("%s: invalid temperature, temp=%d error=%d\n", > + __func__, temp, error); ...on approximately my 10th read of this...I guess maybe this tool has determined that 2 tabs is 1 character too much, because the second line lines up with the '%' instead of the '"'. If this is so important to you, you can of course edit my patch and include it with yours. Regards, Brian
On Tue, 2016-11-22 at 09:51 +0800, Caesar Wang wrote: > 在 2016年11月19日 11:31, Caesar Wang 写道: > > > > Brian, > > > > 在 2016年11月19日 07:52, Brian Norris 写道: > > > > > > 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> > > > --- > > > 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 e227a9f0acf7..35554d146b9d 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); > > I have do some similar for rockchip inside thermal driver. Forget > > to > > send for upstream. :( > > exit: > > pr_err("%s: Invalid conversion table: code=%d, > > temperature=%d\n", > > __func__, error, temp); > > > > > > > > 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; > CHECK: Alignment should match open parenthesis > #428: FILE: drivers/thermal/rockchip_thermal.c:428: > + pr_err("%s: invalid temperature, temp=%d error=%d\n", > + __func__, temp, error); > > CHECK: Alignment should match open parenthesis > #480: FILE: drivers/thermal/rockchip_thermal.c:480: > + pr_err("%s: invalid conversion table, mode=%d\n", > + __func__, table->mode); > > > I'm ready to resend all rockchip thermal patches. (contain them) > so I will ignore patch 2/3 and 3/3 for now and wait for your new patch set. thanks, rui > > > > > > > > } > > > /* > > > > > > _______________________________________________ > > Linux-rockchip mailing list > > Linux-rockchip@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
在 2016年11月22日 15:57, Zhang Rui 写道: > On Tue, 2016-11-22 at 09:51 +0800, Caesar Wang wrote: >> 在 2016年11月19日 11:31, Caesar Wang 写道: >>> Brian, >>> >>> 在 2016年11月19日 07:52, Brian Norris 写道: >>>> 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> >>>> --- >>>> 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 e227a9f0acf7..35554d146b9d 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); >>> I have do some similar for rockchip inside thermal driver. Forget >>> to >>> send for upstream. :( >>> exit: >>> pr_err("%s: Invalid conversion table: code=%d, >>> temperature=%d\n", >>> __func__, error, temp); >>> >>>> 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; >> CHECK: Alignment should match open parenthesis >> #428: FILE: drivers/thermal/rockchip_thermal.c:428: >> + pr_err("%s: invalid temperature, temp=%d error=%d\n", >> + __func__, temp, error); >> >> CHECK: Alignment should match open parenthesis >> #480: FILE: drivers/thermal/rockchip_thermal.c:480: >> + pr_err("%s: invalid conversion table, mode=%d\n", >> + __func__, table->mode); >> >> >> I'm ready to resend all rockchip thermal patches. (contain them) >> > so I will ignore patch 2/3 and 3/3 for now and wait for your new patch > set. Posted the patch set on https://lkml.org/lkml/2016/11/22/250 > > thanks, > rui >>>> } >>>> /* >>> >>> _______________________________________________ >>> Linux-rockchip mailing list >>> Linux-rockchip@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c index e227a9f0acf7..35554d146b9d 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; } /*
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> --- 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(-)