Message ID | 20220421234240.1694-19-laurent.pinchart@ideasonboard.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | media: rkisp1: Misc bug fixes and cleanups | expand |
Laurent Pinchart wrote: > Don't hardcode the register name maximum length (used for alignment > purposes) to 14, but compute it dynamically. The small performance > impact isn't an issue as this is debugging code. Not sure if we want this. Different files will have different alignment, which will look ugly if we cat them. Besides that, the code looks correct. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > --- > .../platform/rockchip/rkisp1/rkisp1-debug.c | 21 +++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c > index 2c226f20f525..28a69323cb38 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c > @@ -11,8 +11,10 @@ > #include <linux/debugfs.h> > #include <linux/delay.h> > #include <linux/device.h> > +#include <linux/minmax.h> > #include <linux/pm_runtime.h> > #include <linux/seq_file.h> > +#include <linux/string.h> > > #include "rkisp1-common.h" > #include "rkisp1-regs.h" > @@ -32,22 +34,29 @@ static int rkisp1_debug_dump_regs(struct rkisp1_device *rkisp1, > struct seq_file *m, unsigned int offset, > const struct rkisp1_debug_register *regs) > { > + const struct rkisp1_debug_register *reg; > + int width = 0; > u32 val, shd; > int ret; > > + for (reg = regs; reg->name; ++reg) > + width = max_t(int, width, strlen(reg->name)); > + > + width += 1; > + > ret = pm_runtime_get_if_in_use(rkisp1->dev); > if (ret <= 0) > return ret ? : -ENODATA; > > - for ( ; regs->name; ++regs) { > - val = rkisp1_read(rkisp1, offset + regs->reg); > + for (reg = regs; reg->name; ++reg) { > + val = rkisp1_read(rkisp1, offset + reg->reg); > > - if (regs->shd) { > - shd = rkisp1_read(rkisp1, offset + regs->shd); > - seq_printf(m, "%14s: 0x%08x/0x%08x\n", regs->name, > + if (reg->shd) { > + shd = rkisp1_read(rkisp1, offset + reg->shd); > + seq_printf(m, "%*s: 0x%08x/0x%08x\n", width, reg->name, > val, shd); > } else { > - seq_printf(m, "%14s: 0x%08x\n", regs->name, val); > + seq_printf(m, "%*s: 0x%08x\n", width, reg->name, val); > } > } > > -- > Regards, > > Laurent Pinchart > >
Hi Ricardo, On Mon, Apr 25, 2022 at 01:49:57PM +0200, Ricardo Ribalda wrote: > Laurent Pinchart wrote: > > > Don't hardcode the register name maximum length (used for alignment > > purposes) to 14, but compute it dynamically. The small performance > > impact isn't an issue as this is debugging code. > > Not sure if we want this. Different files will have different alignment, > which will look ugly if we cat them. That's a good point. I wrote this patch to avoid maintaining the maximum register name length manually, but I'm fine moving it to a macro instead if that's preferred. It's debug code, so I don't mind much. > Besides that, the code looks correct. > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > .../platform/rockchip/rkisp1/rkisp1-debug.c | 21 +++++++++++++------ > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c > > index 2c226f20f525..28a69323cb38 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c > > @@ -11,8 +11,10 @@ > > #include <linux/debugfs.h> > > #include <linux/delay.h> > > #include <linux/device.h> > > +#include <linux/minmax.h> > > #include <linux/pm_runtime.h> > > #include <linux/seq_file.h> > > +#include <linux/string.h> > > > > #include "rkisp1-common.h" > > #include "rkisp1-regs.h" > > @@ -32,22 +34,29 @@ static int rkisp1_debug_dump_regs(struct rkisp1_device *rkisp1, > > struct seq_file *m, unsigned int offset, > > const struct rkisp1_debug_register *regs) > > { > > + const struct rkisp1_debug_register *reg; > > + int width = 0; > > u32 val, shd; > > int ret; > > > > + for (reg = regs; reg->name; ++reg) > > + width = max_t(int, width, strlen(reg->name)); > > + > > + width += 1; > > + > > ret = pm_runtime_get_if_in_use(rkisp1->dev); > > if (ret <= 0) > > return ret ? : -ENODATA; > > > > - for ( ; regs->name; ++regs) { > > - val = rkisp1_read(rkisp1, offset + regs->reg); > > + for (reg = regs; reg->name; ++reg) { > > + val = rkisp1_read(rkisp1, offset + reg->reg); > > > > - if (regs->shd) { > > - shd = rkisp1_read(rkisp1, offset + regs->shd); > > - seq_printf(m, "%14s: 0x%08x/0x%08x\n", regs->name, > > + if (reg->shd) { > > + shd = rkisp1_read(rkisp1, offset + reg->shd); > > + seq_printf(m, "%*s: 0x%08x/0x%08x\n", width, reg->name, > > val, shd); > > } else { > > - seq_printf(m, "%14s: 0x%08x\n", regs->name, val); > > + seq_printf(m, "%*s: 0x%08x\n", width, reg->name, val); > > } > > } > >
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c index 2c226f20f525..28a69323cb38 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c @@ -11,8 +11,10 @@ #include <linux/debugfs.h> #include <linux/delay.h> #include <linux/device.h> +#include <linux/minmax.h> #include <linux/pm_runtime.h> #include <linux/seq_file.h> +#include <linux/string.h> #include "rkisp1-common.h" #include "rkisp1-regs.h" @@ -32,22 +34,29 @@ static int rkisp1_debug_dump_regs(struct rkisp1_device *rkisp1, struct seq_file *m, unsigned int offset, const struct rkisp1_debug_register *regs) { + const struct rkisp1_debug_register *reg; + int width = 0; u32 val, shd; int ret; + for (reg = regs; reg->name; ++reg) + width = max_t(int, width, strlen(reg->name)); + + width += 1; + ret = pm_runtime_get_if_in_use(rkisp1->dev); if (ret <= 0) return ret ? : -ENODATA; - for ( ; regs->name; ++regs) { - val = rkisp1_read(rkisp1, offset + regs->reg); + for (reg = regs; reg->name; ++reg) { + val = rkisp1_read(rkisp1, offset + reg->reg); - if (regs->shd) { - shd = rkisp1_read(rkisp1, offset + regs->shd); - seq_printf(m, "%14s: 0x%08x/0x%08x\n", regs->name, + if (reg->shd) { + shd = rkisp1_read(rkisp1, offset + reg->shd); + seq_printf(m, "%*s: 0x%08x/0x%08x\n", width, reg->name, val, shd); } else { - seq_printf(m, "%14s: 0x%08x\n", regs->name, val); + seq_printf(m, "%*s: 0x%08x\n", width, reg->name, val); } }
Don't hardcode the register name maximum length (used for alignment purposes) to 14, but compute it dynamically. The small performance impact isn't an issue as this is debugging code. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- .../platform/rockchip/rkisp1/rkisp1-debug.c | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)