| Message ID | 4a8ac6700e1503a69146f3eefd7cb515d11bfc9f.1585837575.git.mirq-linux@rere.qmqm.pl (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | power: supply: core: extensions and fixes | expand |
On Thu, Apr 02, 2020 at 07:52:19AM -0700, Guenter Roeck wrote: > On 4/2/20 7:46 AM, Michał Mirosław wrote: > > tempX_label files are swapped compared to what > > power_supply_hwmon_temp_to_property() uses. Make them match. > > While at it, make room for labeling other channels. > > > > Fixes: e67d4dfc9ff1 ("power: supply: Add HWMON compatibility layer") > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > --- > > drivers/power/supply/power_supply_hwmon.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c > > index 75cf861ba492..83318a21fb52 100644 > > --- a/drivers/power/supply/power_supply_hwmon.c > > +++ b/drivers/power/supply/power_supply_hwmon.c > > @@ -43,6 +43,11 @@ static int power_supply_hwmon_curr_to_property(u32 attr) > > } > > } > > > > +static const char *const ps_temp_label[] = { > > + "temp", > > + "ambient temp", > > +}; > > + > > static int power_supply_hwmon_temp_to_property(u32 attr, int channel) > > { > > if (channel) { > > @@ -144,7 +149,14 @@ static int power_supply_hwmon_read_string(struct device *dev, > > u32 attr, int channel, > > const char **str) > > { > > - *str = channel ? "temp" : "temp ambient"; > > + switch (type) { > > + case hwmon_temp: > > + *str = ps_temp_label[channel]; > > + break; > > + default: > > + break; > > That returns "no error" without setting *str, which is really asking for trouble. This is carried over from earlier version. No bug though, but I'll add a patch while I'm at it. Best Regards Michał Mirosław
On 4/2/20 8:00 AM, Michał Mirosław wrote: > On Thu, Apr 02, 2020 at 07:52:19AM -0700, Guenter Roeck wrote: >> On 4/2/20 7:46 AM, Michał Mirosław wrote: >>> tempX_label files are swapped compared to what >>> power_supply_hwmon_temp_to_property() uses. Make them match. >>> While at it, make room for labeling other channels. >>> >>> Fixes: e67d4dfc9ff1 ("power: supply: Add HWMON compatibility layer") >>> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> >>> --- >>> drivers/power/supply/power_supply_hwmon.c | 14 +++++++++++++- >>> 1 file changed, 13 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c >>> index 75cf861ba492..83318a21fb52 100644 >>> --- a/drivers/power/supply/power_supply_hwmon.c >>> +++ b/drivers/power/supply/power_supply_hwmon.c >>> @@ -43,6 +43,11 @@ static int power_supply_hwmon_curr_to_property(u32 attr) >>> } >>> } >>> >>> +static const char *const ps_temp_label[] = { >>> + "temp", >>> + "ambient temp", >>> +}; >>> + >>> static int power_supply_hwmon_temp_to_property(u32 attr, int channel) >>> { >>> if (channel) { >>> @@ -144,7 +149,14 @@ static int power_supply_hwmon_read_string(struct device *dev, >>> u32 attr, int channel, >>> const char **str) >>> { >>> - *str = channel ? "temp" : "temp ambient"; >>> + switch (type) { >>> + case hwmon_temp: >>> + *str = ps_temp_label[channel]; >>> + break; >>> + default: >>> + break; >> >> That returns "no error" without setting *str, which is really asking for trouble. > > This is carried over from earlier version. No bug though, but I'll add a > patch while I'm at it. > This is incorrect. The previous version does not check the type and always sets *str. - *str = channel ? "temp" : "temp ambient"; Guenter
On Thu, Apr 02, 2020 at 11:29:32AM -0700, Guenter Roeck wrote: > On 4/2/20 8:00 AM, Michał Mirosław wrote: > > On Thu, Apr 02, 2020 at 07:52:19AM -0700, Guenter Roeck wrote: > >> On 4/2/20 7:46 AM, Michał Mirosław wrote: > >>> tempX_label files are swapped compared to what > >>> power_supply_hwmon_temp_to_property() uses. Make them match. > >>> While at it, make room for labeling other channels. > >>> > >>> Fixes: e67d4dfc9ff1 ("power: supply: Add HWMON compatibility layer") > >>> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > >>> --- > >>> drivers/power/supply/power_supply_hwmon.c | 14 +++++++++++++- > >>> 1 file changed, 13 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c > >>> index 75cf861ba492..83318a21fb52 100644 > >>> --- a/drivers/power/supply/power_supply_hwmon.c > >>> +++ b/drivers/power/supply/power_supply_hwmon.c > >>> @@ -43,6 +43,11 @@ static int power_supply_hwmon_curr_to_property(u32 attr) > >>> } > >>> } > >>> > >>> +static const char *const ps_temp_label[] = { > >>> + "temp", > >>> + "ambient temp", > >>> +}; > >>> + > >>> static int power_supply_hwmon_temp_to_property(u32 attr, int channel) > >>> { > >>> if (channel) { > >>> @@ -144,7 +149,14 @@ static int power_supply_hwmon_read_string(struct device *dev, > >>> u32 attr, int channel, > >>> const char **str) > >>> { > >>> - *str = channel ? "temp" : "temp ambient"; > >>> + switch (type) { > >>> + case hwmon_temp: > >>> + *str = ps_temp_label[channel]; > >>> + break; > >>> + default: > >>> + break; > >> > >> That returns "no error" without setting *str, which is really asking for trouble. > > > > This is carried over from earlier version. No bug though, but I'll add a > > patch while I'm at it. > > > > This is incorrect. The previous version does not check the type and always sets *str. > > - *str = channel ? "temp" : "temp ambient"; The function is called only for channels that have HWMON_T_LABEL set. This is extended in later patches, though, so the type is going to be required then. I'll split the addition so its more clear. Best Regards, Michał Mirosław
diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c index 75cf861ba492..83318a21fb52 100644 --- a/drivers/power/supply/power_supply_hwmon.c +++ b/drivers/power/supply/power_supply_hwmon.c @@ -43,6 +43,11 @@ static int power_supply_hwmon_curr_to_property(u32 attr) } } +static const char *const ps_temp_label[] = { + "temp", + "ambient temp", +}; + static int power_supply_hwmon_temp_to_property(u32 attr, int channel) { if (channel) { @@ -144,7 +149,14 @@ static int power_supply_hwmon_read_string(struct device *dev, u32 attr, int channel, const char **str) { - *str = channel ? "temp" : "temp ambient"; + switch (type) { + case hwmon_temp: + *str = ps_temp_label[channel]; + break; + default: + break; + } + return 0; }
tempX_label files are swapped compared to what power_supply_hwmon_temp_to_property() uses. Make them match. While at it, make room for labeling other channels. Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- drivers/power/supply/power_supply_hwmon.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)