openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [RESEND PATCH] media: aspeed: add debugfs
@ 2021-09-29  1:16 Jammy Huang
  2021-09-29  5:30 ` Zev Weiss
  0 siblings, 1 reply; 3+ messages in thread
From: Jammy Huang @ 2021-09-29  1:16 UTC (permalink / raw)
  To: eajames, mchehab, joel, andrew, linux-media, openbmc,
	linux-arm-kernel, linux-aspeed, linux-kernel
  Cc: BMC-SW

To show video real-time information as below:

    Signal|           Resolution|       FRC
          |     Width     Height|
      Lock|      1920       1080|         0

    Frame#|       Frame Duration|       FPS
          |    Now    Min    Max|
       496|     26     25     30|        40

Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
---
 drivers/media/platform/aspeed-video.c | 100 ++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 8b3939b8052d..5b98dc7b7b15 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -21,6 +21,8 @@
 #include <linux/videodev2.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
+#include <linux/debugfs.h>
+#include <linux/ktime.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-dev.h>
 #include <media/v4l2-device.h>
@@ -203,6 +205,14 @@ struct aspeed_video_buffer {
 	struct list_head link;
 };
 
+struct aspeed_video_perf {
+	ktime_t last_sample;
+	u32 totaltime;
+	u32 duration;
+	u32 duration_min;
+	u32 duration_max;
+};
+
 #define to_aspeed_video_buffer(x) \
 	container_of((x), struct aspeed_video_buffer, vb)
 
@@ -241,6 +251,8 @@ struct aspeed_video {
 	unsigned int frame_left;
 	unsigned int frame_right;
 	unsigned int frame_top;
+
+	struct aspeed_video_perf perf;
 };
 
 #define to_aspeed_video(x) container_of((x), struct aspeed_video, v4l2_dev)
@@ -444,6 +456,18 @@ static void aspeed_video_write(struct aspeed_video *video, u32 reg, u32 val)
 		readl(video->base + reg));
 }
 
+static void update_perf(struct aspeed_video *v)
+{
+	v->perf.duration =
+		ktime_to_ms(ktime_sub(ktime_get(),  v->perf.last_sample));
+	v->perf.totaltime += v->perf.duration;
+
+	if (!v->perf.duration_max || v->perf.duration > v->perf.duration_max)
+		v->perf.duration_max = v->perf.duration;
+	if (!v->perf.duration_min || v->perf.duration < v->perf.duration_min)
+		v->perf.duration_min = v->perf.duration;
+}
+
 static int aspeed_video_start_frame(struct aspeed_video *video)
 {
 	dma_addr_t addr;
@@ -482,6 +506,8 @@ static int aspeed_video_start_frame(struct aspeed_video *video)
 	aspeed_video_update(video, VE_INTERRUPT_CTRL, 0,
 			    VE_INTERRUPT_COMP_COMPLETE);
 
+	video->perf.last_sample = ktime_get();
+
 	aspeed_video_update(video, VE_SEQ_CTRL, 0,
 			    VE_SEQ_CTRL_TRIG_CAPTURE | VE_SEQ_CTRL_TRIG_COMP);
 
@@ -600,6 +626,8 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
 		u32 frame_size = aspeed_video_read(video,
 						   VE_JPEG_COMP_SIZE_READ_BACK);
 
+		update_perf(video);
+
 		spin_lock(&video->lock);
 		clear_bit(VIDEO_FRAME_INPRG, &video->flags);
 		buf = list_first_entry_or_null(&video->buffers,
@@ -760,6 +788,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 	det->width = MIN_WIDTH;
 	det->height = MIN_HEIGHT;
 	video->v4l2_input_status = V4L2_IN_ST_NO_SIGNAL;
+	memset(&video->perf, 0, sizeof(video->perf));
 
 	do {
 		if (tries) {
@@ -1517,6 +1546,71 @@ static const struct vb2_ops aspeed_video_vb2_ops = {
 	.buf_queue =  aspeed_video_buf_queue,
 };
 
+#ifdef CONFIG_DEBUG_FS
+static int aspeed_video_debugfs_show(struct seq_file *s, void *data)
+{
+	struct aspeed_video *v = s->private;
+
+	seq_printf(s, "%10s|%21s|%10s\n",
+		   "Signal", "Resolution", "FRC");
+	seq_printf(s, "%10s|%10s%11s|%10s\n",
+		   "", "Width", "Height", "");
+	seq_printf(s, "%10s|%10d%11d|%10d\n",
+		   v->v4l2_input_status ? "Unlock" : "Lock",
+		   v->pix_fmt.width, v->pix_fmt.height, v->frame_rate);
+
+	seq_puts(s, "\n");
+
+	seq_printf(s, "%10s|%21s|%10s\n",
+		   "Frame#", "Frame Duration", "FPS");
+	seq_printf(s, "%10s|%7s%7s%7s|%10s\n",
+		   "", "Now", "Min", "Max", "");
+	seq_printf(s, "%10d|%7d%7d%7d|%10d\n",
+		   v->sequence, v->perf.duration, v->perf.duration_min,
+		   v->perf.duration_max, 1000/(v->perf.totaltime/v->sequence));
+
+	return 0;
+}
+
+int aspeed_video_proc_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, aspeed_video_debugfs_show, inode->i_private);
+}
+
+static const struct file_operations aspeed_video_debugfs_ops = {
+	.owner   = THIS_MODULE,
+	.open    = aspeed_video_proc_open,
+	.read    = seq_read,
+	.llseek  = seq_lseek,
+	.release = single_release,
+};
+
+static struct dentry *debugfs_entry;
+
+static void aspeed_video_debugfs_remove(struct aspeed_video *video)
+{
+	debugfs_remove_recursive(debugfs_entry);
+	debugfs_entry = NULL;
+}
+
+static int aspeed_video_debugfs_create(struct aspeed_video *video)
+{
+	debugfs_entry = debugfs_create_file(DEVICE_NAME, 0444, NULL,
+						   video,
+						   &aspeed_video_debugfs_ops);
+	if (!debugfs_entry)
+		aspeed_video_debugfs_remove(video);
+
+	return debugfs_entry == NULL ? -EIO : 0;
+}
+#else
+static void aspeed_video_debugfs_remove(struct aspeed_video *video) { }
+static int aspeed_video_debugfs_create(struct aspeed_video *video)
+{
+	return 0;
+}
+#endif /* CONFIG_DEBUG_FS */
+
 static int aspeed_video_setup_video(struct aspeed_video *video)
 {
 	const u64 mask = ~(BIT(V4L2_JPEG_CHROMA_SUBSAMPLING_444) |
@@ -1708,6 +1802,10 @@ static int aspeed_video_probe(struct platform_device *pdev)
 		return rc;
 	}
 
+	rc = aspeed_video_debugfs_create(video);
+	if (rc)
+		dev_err(video->dev, "debugfs create failed\n");
+
 	return 0;
 }
 
@@ -1719,6 +1817,8 @@ static int aspeed_video_remove(struct platform_device *pdev)
 
 	aspeed_video_off(video);
 
+	aspeed_video_debugfs_remove(video);
+
 	clk_unprepare(video->vclk);
 	clk_unprepare(video->eclk);
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [RESEND PATCH] media: aspeed: add debugfs
  2021-09-29  1:16 [RESEND PATCH] media: aspeed: add debugfs Jammy Huang
@ 2021-09-29  5:30 ` Zev Weiss
  2021-09-29  6:35   ` Jammy Huang
  0 siblings, 1 reply; 3+ messages in thread
From: Zev Weiss @ 2021-09-29  5:30 UTC (permalink / raw)
  To: Jammy Huang
  Cc: BMC-SW, linux-aspeed, andrew, openbmc, eajames, linux-kernel,
	mchehab, linux-arm-kernel, linux-media

On Tue, Sep 28, 2021 at 06:16:53PM PDT, Jammy Huang wrote:
>To show video real-time information as below:
>
>    Signal|           Resolution|       FRC
>          |     Width     Height|
>      Lock|      1920       1080|         0
>
>    Frame#|       Frame Duration|       FPS
>          |    Now    Min    Max|
>       496|     26     25     30|        40
>
>Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
>---
> drivers/media/platform/aspeed-video.c | 100 ++++++++++++++++++++++++++
> 1 file changed, 100 insertions(+)
>
>diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
>index 8b3939b8052d..5b98dc7b7b15 100644
>--- a/drivers/media/platform/aspeed-video.c
>+++ b/drivers/media/platform/aspeed-video.c
>@@ -21,6 +21,8 @@
> #include <linux/videodev2.h>
> #include <linux/wait.h>
> #include <linux/workqueue.h>
>+#include <linux/debugfs.h>
>+#include <linux/ktime.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-dev.h>
> #include <media/v4l2-device.h>
>@@ -203,6 +205,14 @@ struct aspeed_video_buffer {
> 	struct list_head link;
> };
>
>+struct aspeed_video_perf {
>+	ktime_t last_sample;
>+	u32 totaltime;
>+	u32 duration;
>+	u32 duration_min;
>+	u32 duration_max;
>+};
>+
> #define to_aspeed_video_buffer(x) \
> 	container_of((x), struct aspeed_video_buffer, vb)
>
>@@ -241,6 +251,8 @@ struct aspeed_video {
> 	unsigned int frame_left;
> 	unsigned int frame_right;
> 	unsigned int frame_top;
>+
>+	struct aspeed_video_perf perf;
> };
>
> #define to_aspeed_video(x) container_of((x), struct aspeed_video, v4l2_dev)
>@@ -444,6 +456,18 @@ static void aspeed_video_write(struct aspeed_video *video, u32 reg, u32 val)
> 		readl(video->base + reg));
> }
>
>+static void update_perf(struct aspeed_video *v)
>+{
>+	v->perf.duration =
>+		ktime_to_ms(ktime_sub(ktime_get(),  v->perf.last_sample));
>+	v->perf.totaltime += v->perf.duration;
>+
>+	if (!v->perf.duration_max || v->perf.duration > v->perf.duration_max)
>+		v->perf.duration_max = v->perf.duration;

How about

  v->perf.duration_max = max(v->perf.duration, v->perf.duration_max);

instead of manually testing & branching?

>+	if (!v->perf.duration_min || v->perf.duration < v->perf.duration_min)
>+		v->perf.duration_min = v->perf.duration;

And likewise with min(...) here.

As a minor style thing, I might suggest adding a variable declaration
like

  struct aspeed_video_perf *p = &v->perf;

and using that in the rest of the function to cut down on the
verbosity/repetition a bit.  Or actually, since it looks like there
aren't any other members of struct aspeed_video accessed in this
function, maybe just make struct aspeed_video_perf be the parameter
instead?

>+}
>+
> static int aspeed_video_start_frame(struct aspeed_video *video)
> {
> 	dma_addr_t addr;
>@@ -482,6 +506,8 @@ static int aspeed_video_start_frame(struct aspeed_video *video)
> 	aspeed_video_update(video, VE_INTERRUPT_CTRL, 0,
> 			    VE_INTERRUPT_COMP_COMPLETE);
>
>+	video->perf.last_sample = ktime_get();
>+
> 	aspeed_video_update(video, VE_SEQ_CTRL, 0,
> 			    VE_SEQ_CTRL_TRIG_CAPTURE | VE_SEQ_CTRL_TRIG_COMP);
>
>@@ -600,6 +626,8 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
> 		u32 frame_size = aspeed_video_read(video,
> 						   VE_JPEG_COMP_SIZE_READ_BACK);
>
>+		update_perf(video);
>+
> 		spin_lock(&video->lock);
> 		clear_bit(VIDEO_FRAME_INPRG, &video->flags);
> 		buf = list_first_entry_or_null(&video->buffers,
>@@ -760,6 +788,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
> 	det->width = MIN_WIDTH;
> 	det->height = MIN_HEIGHT;
> 	video->v4l2_input_status = V4L2_IN_ST_NO_SIGNAL;
>+	memset(&video->perf, 0, sizeof(video->perf));
>
> 	do {
> 		if (tries) {
>@@ -1517,6 +1546,71 @@ static const struct vb2_ops aspeed_video_vb2_ops = {
> 	.buf_queue =  aspeed_video_buf_queue,
> };
>
>+#ifdef CONFIG_DEBUG_FS
>+static int aspeed_video_debugfs_show(struct seq_file *s, void *data)
>+{
>+	struct aspeed_video *v = s->private;
>+
>+	seq_printf(s, "%10s|%21s|%10s\n",
>+		   "Signal", "Resolution", "FRC");
>+	seq_printf(s, "%10s|%10s%11s|%10s\n",
>+		   "", "Width", "Height", "");
>+	seq_printf(s, "%10s|%10d%11d|%10d\n",
>+		   v->v4l2_input_status ? "Unlock" : "Lock",
>+		   v->pix_fmt.width, v->pix_fmt.height, v->frame_rate);
>+
>+	seq_puts(s, "\n");
>+
>+	seq_printf(s, "%10s|%21s|%10s\n",
>+		   "Frame#", "Frame Duration", "FPS");
>+	seq_printf(s, "%10s|%7s%7s%7s|%10s\n",
>+		   "", "Now", "Min", "Max", "");
>+	seq_printf(s, "%10d|%7d%7d%7d|%10d\n",
>+		   v->sequence, v->perf.duration, v->perf.duration_min,
>+		   v->perf.duration_max, 1000/(v->perf.totaltime/v->sequence));
>+

This looks like a convenient format for eyeballing with 'cat', but also
like it would be kind of awkward to parse if you wanted to do any sort
of automated analysis of the performance data it provides.  Would a
key:value type format like

  width: %d
  height: %d
  frame_rate: %d
  frame_number: %d
  # etc.

maybe provide a decent compromise?  (Easily parseable, almost as easily
readable.)

>+	return 0;
>+}
>+
>+int aspeed_video_proc_open(struct inode *inode, struct file *file)
>+{
>+	return single_open(file, aspeed_video_debugfs_show, inode->i_private);
>+}
>+
>+static const struct file_operations aspeed_video_debugfs_ops = {
>+	.owner   = THIS_MODULE,
>+	.open    = aspeed_video_proc_open,
>+	.read    = seq_read,
>+	.llseek  = seq_lseek,
>+	.release = single_release,
>+};
>+
>+static struct dentry *debugfs_entry;

I don't know how realistic the odds are of a system ever having multiple
aspeed-video devices, but structurally would this make more sense as
part of struct aspeed_video instead of being a single global?

>+
>+static void aspeed_video_debugfs_remove(struct aspeed_video *video)
>+{
>+	debugfs_remove_recursive(debugfs_entry);
>+	debugfs_entry = NULL;
>+}
>+
>+static int aspeed_video_debugfs_create(struct aspeed_video *video)
>+{
>+	debugfs_entry = debugfs_create_file(DEVICE_NAME, 0444, NULL,
>+						   video,
>+						   &aspeed_video_debugfs_ops);
>+	if (!debugfs_entry)
>+		aspeed_video_debugfs_remove(video);
>+
>+	return debugfs_entry == NULL ? -EIO : 0;
>+}
>+#else
>+static void aspeed_video_debugfs_remove(struct aspeed_video *video) { }
>+static int aspeed_video_debugfs_create(struct aspeed_video *video)
>+{
>+	return 0;
>+}
>+#endif /* CONFIG_DEBUG_FS */
>+
> static int aspeed_video_setup_video(struct aspeed_video *video)
> {
> 	const u64 mask = ~(BIT(V4L2_JPEG_CHROMA_SUBSAMPLING_444) |
>@@ -1708,6 +1802,10 @@ static int aspeed_video_probe(struct platform_device *pdev)
> 		return rc;
> 	}
>
>+	rc = aspeed_video_debugfs_create(video);
>+	if (rc)
>+		dev_err(video->dev, "debugfs create failed\n");
>+
> 	return 0;
> }
>
>@@ -1719,6 +1817,8 @@ static int aspeed_video_remove(struct platform_device *pdev)
>
> 	aspeed_video_off(video);
>
>+	aspeed_video_debugfs_remove(video);
>+
> 	clk_unprepare(video->vclk);
> 	clk_unprepare(video->eclk);
>
>-- 
>2.25.1
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [RESEND PATCH] media: aspeed: add debugfs
  2021-09-29  5:30 ` Zev Weiss
@ 2021-09-29  6:35   ` Jammy Huang
  0 siblings, 0 replies; 3+ messages in thread
From: Jammy Huang @ 2021-09-29  6:35 UTC (permalink / raw)
  To: Zev Weiss
  Cc: BMC-SW, linux-aspeed, andrew, openbmc, eajames, linux-kernel,
	mchehab, linux-arm-kernel, linux-media

Hi Zev,

On 2021/9/29 下午 01:30, Zev Weiss wrote:
> On Tue, Sep 28, 2021 at 06:16:53PM PDT, Jammy Huang wrote:
>> To show video real-time information as below:
>>
>>     Signal|           Resolution|       FRC
>>           |     Width     Height|
>>       Lock|      1920       1080|         0
>>
>>     Frame#|       Frame Duration|       FPS
>>           |    Now    Min    Max|
>>        496|     26     25     30|        40
>>
>> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
>> ---
>> drivers/media/platform/aspeed-video.c | 100 ++++++++++++++++++++++++++
>> 1 file changed, 100 insertions(+)
>>
>> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
>> index 8b3939b8052d..5b98dc7b7b15 100644
>> --- a/drivers/media/platform/aspeed-video.c
>> +++ b/drivers/media/platform/aspeed-video.c
>> @@ -21,6 +21,8 @@
>> #include <linux/videodev2.h>
>> #include <linux/wait.h>
>> #include <linux/workqueue.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/ktime.h>
>> #include <media/v4l2-ctrls.h>
>> #include <media/v4l2-dev.h>
>> #include <media/v4l2-device.h>
>> @@ -203,6 +205,14 @@ struct aspeed_video_buffer {
>> 	struct list_head link;
>> };
>>
>> +struct aspeed_video_perf {
>> +	ktime_t last_sample;
>> +	u32 totaltime;
>> +	u32 duration;
>> +	u32 duration_min;
>> +	u32 duration_max;
>> +};
>> +
>> #define to_aspeed_video_buffer(x) \
>> 	container_of((x), struct aspeed_video_buffer, vb)
>>
>> @@ -241,6 +251,8 @@ struct aspeed_video {
>> 	unsigned int frame_left;
>> 	unsigned int frame_right;
>> 	unsigned int frame_top;
>> +
>> +	struct aspeed_video_perf perf;
>> };
>>
>> #define to_aspeed_video(x) container_of((x), struct aspeed_video, v4l2_dev)
>> @@ -444,6 +456,18 @@ static void aspeed_video_write(struct aspeed_video *video, u32 reg, u32 val)
>> 		readl(video->base + reg));
>> }
>>
>> +static void update_perf(struct aspeed_video *v)
>> +{
>> +	v->perf.duration =
>> +		ktime_to_ms(ktime_sub(ktime_get(),  v->perf.last_sample));
>> +	v->perf.totaltime += v->perf.duration;
>> +
>> +	if (!v->perf.duration_max || v->perf.duration > v->perf.duration_max)
>> +		v->perf.duration_max = v->perf.duration;
> How about
>
>    v->perf.duration_max = max(v->perf.duration, v->perf.duration_max);
>
> instead of manually testing & branching?
OK, this will be included in the next patch.
>> +	if (!v->perf.duration_min || v->perf.duration < v->perf.duration_min)
>> +		v->perf.duration_min = v->perf.duration;
> And likewise with min(...) here.
>
> As a minor style thing, I might suggest adding a variable declaration
> like
>
>    struct aspeed_video_perf *p = &v->perf;
>
> and using that in the rest of the function to cut down on the
> verbosity/repetition a bit.  Or actually, since it looks like there
> aren't any other members of struct aspeed_video accessed in this
> function, maybe just make struct aspeed_video_perf be the parameter
> instead?
OK, this will be included in the next patch.
>
>> +}
>> +
>> static int aspeed_video_start_frame(struct aspeed_video *video)
>> {
>> 	dma_addr_t addr;
>> @@ -482,6 +506,8 @@ static int aspeed_video_start_frame(struct aspeed_video *video)
>> 	aspeed_video_update(video, VE_INTERRUPT_CTRL, 0,
>> 			    VE_INTERRUPT_COMP_COMPLETE);
>>
>> +	video->perf.last_sample = ktime_get();
>> +
>> 	aspeed_video_update(video, VE_SEQ_CTRL, 0,
>> 			    VE_SEQ_CTRL_TRIG_CAPTURE | VE_SEQ_CTRL_TRIG_COMP);
>>
>> @@ -600,6 +626,8 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
>> 		u32 frame_size = aspeed_video_read(video,
>> 						   VE_JPEG_COMP_SIZE_READ_BACK);
>>
>> +		update_perf(video);
>> +
>> 		spin_lock(&video->lock);
>> 		clear_bit(VIDEO_FRAME_INPRG, &video->flags);
>> 		buf = list_first_entry_or_null(&video->buffers,
>> @@ -760,6 +788,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>> 	det->width = MIN_WIDTH;
>> 	det->height = MIN_HEIGHT;
>> 	video->v4l2_input_status = V4L2_IN_ST_NO_SIGNAL;
>> +	memset(&video->perf, 0, sizeof(video->perf));
>>
>> 	do {
>> 		if (tries) {
>> @@ -1517,6 +1546,71 @@ static const struct vb2_ops aspeed_video_vb2_ops = {
>> 	.buf_queue =  aspeed_video_buf_queue,
>> };
>>
>> +#ifdef CONFIG_DEBUG_FS
>> +static int aspeed_video_debugfs_show(struct seq_file *s, void *data)
>> +{
>> +	struct aspeed_video *v = s->private;
>> +
>> +	seq_printf(s, "%10s|%21s|%10s\n",
>> +		   "Signal", "Resolution", "FRC");
>> +	seq_printf(s, "%10s|%10s%11s|%10s\n",
>> +		   "", "Width", "Height", "");
>> +	seq_printf(s, "%10s|%10d%11d|%10d\n",
>> +		   v->v4l2_input_status ? "Unlock" : "Lock",
>> +		   v->pix_fmt.width, v->pix_fmt.height, v->frame_rate);
>> +
>> +	seq_puts(s, "\n");
>> +
>> +	seq_printf(s, "%10s|%21s|%10s\n",
>> +		   "Frame#", "Frame Duration", "FPS");
>> +	seq_printf(s, "%10s|%7s%7s%7s|%10s\n",
>> +		   "", "Now", "Min", "Max", "");
>> +	seq_printf(s, "%10d|%7d%7d%7d|%10d\n",
>> +		   v->sequence, v->perf.duration, v->perf.duration_min,
>> +		   v->perf.duration_max, 1000/(v->perf.totaltime/v->sequence));
>> +
> This looks like a convenient format for eyeballing with 'cat', but also
> like it would be kind of awkward to parse if you wanted to do any sort
> of automated analysis of the performance data it provides.  Would a
> key:value type format like
>
>    width: %d
>    height: %d
>    frame_rate: %d
>    frame_number: %d
>    # etc.
>
> maybe provide a decent compromise?  (Easily parseable, almost as easily
> readable.)

Your concern is reasonable, I will change the style to show information.

This will be included in the next patch.

>
>> +	return 0;
>> +}
>> +
>> +int aspeed_video_proc_open(struct inode *inode, struct file *file)
>> +{
>> +	return single_open(file, aspeed_video_debugfs_show, inode->i_private);
>> +}
>> +
>> +static const struct file_operations aspeed_video_debugfs_ops = {
>> +	.owner   = THIS_MODULE,
>> +	.open    = aspeed_video_proc_open,
>> +	.read    = seq_read,
>> +	.llseek  = seq_lseek,
>> +	.release = single_release,
>> +};
>> +
>> +static struct dentry *debugfs_entry;
> I don't know how realistic the odds are of a system ever having multiple
> aspeed-video devices, but structurally would this make more sense as
> part of struct aspeed_video instead of being a single global?

Since this is the driver for aspeed-video ip of SoC, it couldn't have 
multiple devices.

>> +
>> +static void aspeed_video_debugfs_remove(struct aspeed_video *video)
>> +{
>> +	debugfs_remove_recursive(debugfs_entry);
>> +	debugfs_entry = NULL;
>> +}
>> +
>> +static int aspeed_video_debugfs_create(struct aspeed_video *video)
>> +{
>> +	debugfs_entry = debugfs_create_file(DEVICE_NAME, 0444, NULL,
>> +						   video,
>> +						   &aspeed_video_debugfs_ops);
>> +	if (!debugfs_entry)
>> +		aspeed_video_debugfs_remove(video);
>> +
>> +	return debugfs_entry == NULL ? -EIO : 0;
>> +}
>> +#else
>> +static void aspeed_video_debugfs_remove(struct aspeed_video *video) { }
>> +static int aspeed_video_debugfs_create(struct aspeed_video *video)
>> +{
>> +	return 0;
>> +}
>> +#endif /* CONFIG_DEBUG_FS */
>> +
>> static int aspeed_video_setup_video(struct aspeed_video *video)
>> {
>> 	const u64 mask = ~(BIT(V4L2_JPEG_CHROMA_SUBSAMPLING_444) |
>> @@ -1708,6 +1802,10 @@ static int aspeed_video_probe(struct platform_device *pdev)
>> 		return rc;
>> 	}
>>
>> +	rc = aspeed_video_debugfs_create(video);
>> +	if (rc)
>> +		dev_err(video->dev, "debugfs create failed\n");
>> +
>> 	return 0;
>> }
>>
>> @@ -1719,6 +1817,8 @@ static int aspeed_video_remove(struct platform_device *pdev)
>>
>> 	aspeed_video_off(video);
>>
>> +	aspeed_video_debugfs_remove(video);
>> +
>> 	clk_unprepare(video->vclk);
>> 	clk_unprepare(video->eclk);
>>
>> -- 
>> 2.25.1
>>

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-09-29  6:36 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-29  1:16 [RESEND PATCH] media: aspeed: add debugfs Jammy Huang
2021-09-29  5:30 ` Zev Weiss
2021-09-29  6:35   ` Jammy Huang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).