| Message ID | 20200927090820.61859-1-f4bug@amsat.org |
|---|---|
| Headers | show |
| Series | qdev-clock: Minor improvements to the Clock API | expand |
Hi Philippe, On 11:08 Sun 27 Sep , Philippe Mathieu-Daudé wrote: > Introduce freq_to_str() to convert frequency values in human > friendly units using the SI units for Hertz. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/qemu/cutils.h | 12 ++++++++++++ > util/cutils.c | 10 ++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h > index eb59852dfdf..0186c846e9c 100644 > --- a/include/qemu/cutils.h > +++ b/include/qemu/cutils.h > @@ -158,6 +158,18 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result); > > char *size_to_str(uint64_t val); > > +/** > + * freq_to_str: > + * @freq_hz: frequency to stringify > + * > + * Return human readable string for frequency @freq_hz. > + * Use SI units like KHz, MHz, and so forth. > + * > + * The caller is responsible for releasing the value returned with g_free() > + * after use. > + */ > +char *freq_to_str(uint64_t freq_hz); > + > /* used to print char* safely */ > #define STR_OR_NULL(str) ((str) ? (str) : "null") > > diff --git a/util/cutils.c b/util/cutils.c > index 36ce712271f..dab837fd8b8 100644 > --- a/util/cutils.c > +++ b/util/cutils.c > @@ -885,6 +885,16 @@ char *size_to_str(uint64_t val) > return g_strdup_printf("%0.3g %sB", (double)val / div, suffixes[i]); > } > > +char *freq_to_str(uint64_t freq_hz) > +{ > + static const char *suffixes[] = { "", "K", "M", "G", "T", "P", "E" }; > + unsigned unit_index = log10(freq_hz) / 3; > + > + return g_strdup_printf("%0.3g %sHz", > + freq_hz / pow(10.0, unit_index * 3.0), > + suffixes[unit_index]); You could end up going out of your 'suffixes' array if freq_hz is very high. Also, to avoid the complexity of log10/pow, maybe something like: double freq = freq_hz; size_t idx = 0; while (freq >= 1000.0 && idx < ARRAY_LENGTH(suffixes)) { freq /= 1000.0; idx++; } return g_strdup_printf("%0.3g %sHz", freq, suffixes[idx]); is enough?
On 11:08 Sun 27 Sep , Philippe Mathieu-Daudé wrote: > Instead of directly aborting, display a hint to help the developer > figure out the problem (likely trying to connect a clock to a device > pre-dating the Clock API, thus not expecting clocks). > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Luc Michel <luc@lmichel.fr> > --- > hw/core/qdev-clock.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c > index 47ecb5b4fae..33bd4a9d520 100644 > --- a/hw/core/qdev-clock.c > +++ b/hw/core/qdev-clock.c > @@ -12,6 +12,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/error-report.h" > #include "hw/qdev-clock.h" > #include "hw/qdev-core.h" > #include "qapi/error.h" > @@ -153,6 +154,11 @@ Clock *qdev_get_clock_in(DeviceState *dev, const char *name) > assert(name); > > ncl = qdev_get_clocklist(dev, name); > + if (!ncl) { > + error_report("can not find clock-in '%s' for device type '%s'", > + name, object_get_typename(OBJECT(dev))); > + abort(); > + } > assert(!ncl->output); > > return ncl->clock; > @@ -165,6 +171,11 @@ Clock *qdev_get_clock_out(DeviceState *dev, const char *name) > assert(name); > > ncl = qdev_get_clocklist(dev, name); > + if (!ncl) { > + error_report("can not find clock-out '%s' for device type '%s'", > + name, object_get_typename(OBJECT(dev))); > + abort(); > + } > assert(ncl->output); > > return ncl->clock; > -- > 2.26.2 >
On 9/28/20 9:53 AM, Luc Michel wrote: > On 11:08 Sun 27 Sep , Philippe Mathieu-Daudé wrote: >> Instead of directly aborting, display a hint to help the developer >> figure out the problem (likely trying to connect a clock to a device >> pre-dating the Clock API, thus not expecting clocks). >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Reviewed-by: Luc Michel <luc@lmichel.fr> Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> > >> --- >> hw/core/qdev-clock.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c >> index 47ecb5b4fae..33bd4a9d520 100644 >> --- a/hw/core/qdev-clock.c >> +++ b/hw/core/qdev-clock.c >> @@ -12,6 +12,7 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/error-report.h" >> #include "hw/qdev-clock.h" >> #include "hw/qdev-core.h" >> #include "qapi/error.h" >> @@ -153,6 +154,11 @@ Clock *qdev_get_clock_in(DeviceState *dev, const char *name) >> assert(name); >> >> ncl = qdev_get_clocklist(dev, name); >> + if (!ncl) { >> + error_report("can not find clock-in '%s' for device type '%s'", >> + name, object_get_typename(OBJECT(dev))); >> + abort(); >> + } >> assert(!ncl->output); >> >> return ncl->clock; >> @@ -165,6 +171,11 @@ Clock *qdev_get_clock_out(DeviceState *dev, const char *name) >> assert(name); >> >> ncl = qdev_get_clocklist(dev, name); >> + if (!ncl) { >> + error_report("can not find clock-out '%s' for device type '%s'", >> + name, object_get_typename(OBJECT(dev))); >> + abort(); >> + } >> assert(ncl->output); >> >> return ncl->clock; >> -- >> 2.26.2 >>
On Mon, Sep 28, 2020 at 12:45:15PM +0200, Damien Hedde wrote: > > > On 9/28/20 9:53 AM, Luc Michel wrote: > > On 11:08 Sun 27 Sep , Philippe Mathieu-Daudé wrote: > >> Instead of directly aborting, display a hint to help the developer > >> figure out the problem (likely trying to connect a clock to a device > >> pre-dating the Clock API, thus not expecting clocks). > >> > >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > > Reviewed-by: Luc Michel <luc@lmichel.fr> > > Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > > > >> --- > >> hw/core/qdev-clock.c | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c > >> index 47ecb5b4fae..33bd4a9d520 100644 > >> --- a/hw/core/qdev-clock.c > >> +++ b/hw/core/qdev-clock.c > >> @@ -12,6 +12,7 @@ > >> */ > >> > >> #include "qemu/osdep.h" > >> +#include "qemu/error-report.h" > >> #include "hw/qdev-clock.h" > >> #include "hw/qdev-core.h" > >> #include "qapi/error.h" > >> @@ -153,6 +154,11 @@ Clock *qdev_get_clock_in(DeviceState *dev, const char *name) > >> assert(name); > >> > >> ncl = qdev_get_clocklist(dev, name); > >> + if (!ncl) { > >> + error_report("can not find clock-in '%s' for device type '%s'", > >> + name, object_get_typename(OBJECT(dev))); > >> + abort(); > >> + } > >> assert(!ncl->output); > >> > >> return ncl->clock; > >> @@ -165,6 +171,11 @@ Clock *qdev_get_clock_out(DeviceState *dev, const char *name) > >> assert(name); > >> > >> ncl = qdev_get_clocklist(dev, name); > >> + if (!ncl) { > >> + error_report("can not find clock-out '%s' for device type '%s'", > >> + name, object_get_typename(OBJECT(dev))); > >> + abort(); > >> + } > >> assert(ncl->output); > >> > >> return ncl->clock; > >> -- > >> 2.26.2 > >>
On 9/28/20 9:50 AM, Luc Michel wrote: > Hi Philippe, > > On 11:08 Sun 27 Sep , Philippe Mathieu-Daudé wrote: >> Introduce freq_to_str() to convert frequency values in human >> friendly units using the SI units for Hertz. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> include/qemu/cutils.h | 12 ++++++++++++ >> util/cutils.c | 10 ++++++++++ >> 2 files changed, 22 insertions(+) >> >> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h >> index eb59852dfdf..0186c846e9c 100644 >> --- a/include/qemu/cutils.h >> +++ b/include/qemu/cutils.h >> @@ -158,6 +158,18 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result); >> >> char *size_to_str(uint64_t val); >> >> +/** >> + * freq_to_str: >> + * @freq_hz: frequency to stringify >> + * >> + * Return human readable string for frequency @freq_hz. >> + * Use SI units like KHz, MHz, and so forth. >> + * >> + * The caller is responsible for releasing the value returned with g_free() >> + * after use. >> + */ >> +char *freq_to_str(uint64_t freq_hz); >> + >> /* used to print char* safely */ >> #define STR_OR_NULL(str) ((str) ? (str) : "null") >> >> diff --git a/util/cutils.c b/util/cutils.c >> index 36ce712271f..dab837fd8b8 100644 >> --- a/util/cutils.c >> +++ b/util/cutils.c >> @@ -885,6 +885,16 @@ char *size_to_str(uint64_t val) >> return g_strdup_printf("%0.3g %sB", (double)val / div, suffixes[i]); >> } >> >> +char *freq_to_str(uint64_t freq_hz) >> +{ >> + static const char *suffixes[] = { "", "K", "M", "G", "T", "P", "E" }; >> + unsigned unit_index = log10(freq_hz) / 3; >> + >> + return g_strdup_printf("%0.3g %sHz", >> + freq_hz / pow(10.0, unit_index * 3.0), >> + suffixes[unit_index]); > > You could end up going out of your 'suffixes' array if freq_hz is very > high. Oh, good point. > Also, to avoid the complexity of log10/pow, maybe something like: > > double freq = freq_hz; > size_t idx = 0; > > while (freq >= 1000.0 && idx < ARRAY_LENGTH(suffixes)) { > freq /= 1000.0; > idx++; > } KISS, I like it :) > > return g_strdup_printf("%0.3g %sHz", freq, suffixes[idx]); > > is enough? > I'll respin, thanks! Phil.