diff mbox

[v2,4/9] media: hantro: make irq names configurable

Message ID 20190529095424.23614-5-p.zabel@pengutronix.de (mailing list archive)
State Superseded, archived
Delegated to: Hans Verkuil
Headers

Commit Message

Philipp Zabel May 29, 2019, 9:54 a.m. UTC 
The i.MX8MQ bindings will use different IRQ names ("g1" instead of
"vdpu", and "g2"), so make them configurable. This also allows to
register more than two IRQs, which will be required for i.MX8MM support
later (it will add "h1" instead of "vepu").

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1 [1]:
 - Rebased onto "[PATCH v6] Add MPEG-2 decoding to Rockchip VPU" series.

[1] https://patchwork.linuxtv.org/patch/56285/
---
 drivers/staging/media/hantro/hantro.h        | 11 ++++---
 drivers/staging/media/hantro/hantro_drv.c    | 31 +++++++-------------
 drivers/staging/media/hantro/rk3288_vpu_hw.c |  5 ++--
 drivers/staging/media/hantro/rk3399_vpu_hw.c |  9 ++++--
 4 files changed, 26 insertions(+), 30 deletions(-)

Comments

Boris Brezillon May 29, 2019, 11:34 a.m. UTC | #1 
On Wed, 29 May 2019 11:54:19 +0200
Philipp Zabel <p.zabel@pengutronix.de> wrote:

> The i.MX8MQ bindings will use different IRQ names ("g1" instead of
> "vdpu", and "g2"), so make them configurable. This also allows to
> register more than two IRQs, which will be required for i.MX8MM support
> later (it will add "h1" instead of "vepu").
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v1 [1]:
>  - Rebased onto "[PATCH v6] Add MPEG-2 decoding to Rockchip VPU" series.
> 
> [1] https://patchwork.linuxtv.org/patch/56285/
> ---
>  drivers/staging/media/hantro/hantro.h        | 11 ++++---
>  drivers/staging/media/hantro/hantro_drv.c    | 31 +++++++-------------
>  drivers/staging/media/hantro/rk3288_vpu_hw.c |  5 ++--
>  drivers/staging/media/hantro/rk3399_vpu_hw.c |  9 ++++--
>  4 files changed, 26 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> index 296b9ffad547..6b90fe48bcdf 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -26,6 +26,7 @@
>  #include "hantro_hw.h"
>  
>  #define HANTRO_MAX_CLOCKS		4
> +#define HANTRO_MAX_IRQS			3
>  
>  #define MPEG2_MB_DIM			16
>  #define MPEG2_MB_WIDTH(w)		DIV_ROUND_UP(w, MPEG2_MB_DIM)
> @@ -57,8 +58,9 @@ struct hantro_codec_ops;
>   * @codec_ops:			Codec ops.
>   * @init:			Initialize hardware.
>   * @runtime_resume:		reenable hardware after power gating
> - * @vepu_irq:			encoder interrupt handler
> - * @vdpu_irq:			decoder interrupt handler
> + * @irq_handlers:		interrupt handlers, same order as irq names
> + * @irq_names:			array of irq names
> + * @num_irqs:			number of irqs in the arrays
>   * @clk_names:			array of clock names
>   * @num_clocks:			number of clocks in the array
>   */
> @@ -73,8 +75,9 @@ struct hantro_variant {
>  	const struct hantro_codec_ops *codec_ops;
>  	int (*init)(struct hantro_dev *vpu);
>  	int (*runtime_resume)(struct hantro_dev *vpu);
> -	irqreturn_t (*vepu_irq)(int irq, void *priv);
> -	irqreturn_t (*vdpu_irq)(int irq, void *priv);
> +	irqreturn_t (*irq_handlers[HANTRO_MAX_IRQS])(int irq, void *priv);
> +	const char *irq_names[HANTRO_MAX_IRQS];

Can we have a struct instead of an array for all handlers and another
array for irq names:

	struct {
		const char *name;
		irqreturn_t (*handler)(int irq, void *priv);
	} irqs[HANTRO_MAX_IRQS];

> +	int num_irqs;

Or we could have the struct defined outside of hantro_variant and get
rid of HANTRO_MAX_IRQS (I find it annoying to have to update the MAX
value every time a new variant needs more than what was previously
defined as MAX):

struct hantro_irq {
	const char *name;
	irqreturn_t (*handler)(int irq, void *priv);
};

struct hantro_variant {
	...
	unsigned int num_irqs;
	const struct hantro_irq *irqs;
};

static const struct hantro_irq xxxx_irqs[] = {
	{ ... },
	{ ... },

};

static const struct hantro_variant xxxx_variant = {
	.num_irqs = ARRAY_SIZE(xxxx_irqs),
	.irqs = xxxx_irqs,
};
Philipp Zabel May 29, 2019, 12:24 p.m. UTC | #2 
Hi Boris,

On Wed, 2019-05-29 at 13:34 +0200, Boris Brezillon wrote:
> On Wed, 29 May 2019 11:54:19 +0200
> Philipp Zabel <p.zabel@pengutronix.de> wrote:
> 
> > The i.MX8MQ bindings will use different IRQ names ("g1" instead of
> > "vdpu", and "g2"), so make them configurable. This also allows to
> > register more than two IRQs, which will be required for i.MX8MM support
> > later (it will add "h1" instead of "vepu").
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes since v1 [1]:
> >  - Rebased onto "[PATCH v6] Add MPEG-2 decoding to Rockchip VPU" series.
> > 
> > [1] https://patchwork.linuxtv.org/patch/56285/
> > ---
> >  drivers/staging/media/hantro/hantro.h        | 11 ++++---
> >  drivers/staging/media/hantro/hantro_drv.c    | 31 +++++++-------------
> >  drivers/staging/media/hantro/rk3288_vpu_hw.c |  5 ++--
> >  drivers/staging/media/hantro/rk3399_vpu_hw.c |  9 ++++--
> >  4 files changed, 26 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> > index 296b9ffad547..6b90fe48bcdf 100644
> > --- a/drivers/staging/media/hantro/hantro.h
> > +++ b/drivers/staging/media/hantro/hantro.h
> > @@ -26,6 +26,7 @@
> >  #include "hantro_hw.h"
> >  
> >  #define HANTRO_MAX_CLOCKS		4
> > +#define HANTRO_MAX_IRQS			3
> >  
> >  #define MPEG2_MB_DIM			16
> >  #define MPEG2_MB_WIDTH(w)		DIV_ROUND_UP(w, MPEG2_MB_DIM)
> > @@ -57,8 +58,9 @@ struct hantro_codec_ops;
> >   * @codec_ops:			Codec ops.
> >   * @init:			Initialize hardware.
> >   * @runtime_resume:		reenable hardware after power gating
> > - * @vepu_irq:			encoder interrupt handler
> > - * @vdpu_irq:			decoder interrupt handler
> > + * @irq_handlers:		interrupt handlers, same order as irq names
> > + * @irq_names:			array of irq names
> > + * @num_irqs:			number of irqs in the arrays
> >   * @clk_names:			array of clock names
> >   * @num_clocks:			number of clocks in the array
> >   */
> > @@ -73,8 +75,9 @@ struct hantro_variant {
> >  	const struct hantro_codec_ops *codec_ops;
> >  	int (*init)(struct hantro_dev *vpu);
> >  	int (*runtime_resume)(struct hantro_dev *vpu);
> > -	irqreturn_t (*vepu_irq)(int irq, void *priv);
> > -	irqreturn_t (*vdpu_irq)(int irq, void *priv);
> > +	irqreturn_t (*irq_handlers[HANTRO_MAX_IRQS])(int irq, void *priv);
> > +	const char *irq_names[HANTRO_MAX_IRQS];
> 
> Can we have a struct instead of an array for all handlers and another
> array for irq names:
> 
> 	struct {
> 		const char *name;
> 		irqreturn_t (*handler)(int irq, void *priv);
> 	} irqs[HANTRO_MAX_IRQS];
> 
> > +	int num_irqs;
> 
> Or we could have the struct defined outside of hantro_variant and get
> rid of HANTRO_MAX_IRQS (I find it annoying to have to update the MAX
> value every time a new variant needs more than what was previously
> defined as MAX):
> 
> struct hantro_irq {
> 	const char *name;
> 	irqreturn_t (*handler)(int irq, void *priv);
> };
> 
> struct hantro_variant {
> 	...
> 	unsigned int num_irqs;
> 	const struct hantro_irq *irqs;
> };
> 
> static const struct hantro_irq xxxx_irqs[] = {
> 	{ ... },
> 	{ ... },
> 
> };
> 
> static const struct hantro_variant xxxx_variant = {
> 	.num_irqs = ARRAY_SIZE(xxxx_irqs),
> 	.irqs = xxxx_irqs,
> };

Thank you, that looks better. I'll change this for v3.

regards
Philipp
diff mbox

Patch

diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
index 296b9ffad547..6b90fe48bcdf 100644
--- a/drivers/staging/media/hantro/hantro.h
+++ b/drivers/staging/media/hantro/hantro.h
@@ -26,6 +26,7 @@ 
 #include "hantro_hw.h"
 
 #define HANTRO_MAX_CLOCKS		4
+#define HANTRO_MAX_IRQS			3
 
 #define MPEG2_MB_DIM			16
 #define MPEG2_MB_WIDTH(w)		DIV_ROUND_UP(w, MPEG2_MB_DIM)
@@ -57,8 +58,9 @@  struct hantro_codec_ops;
  * @codec_ops:			Codec ops.
  * @init:			Initialize hardware.
  * @runtime_resume:		reenable hardware after power gating
- * @vepu_irq:			encoder interrupt handler
- * @vdpu_irq:			decoder interrupt handler
+ * @irq_handlers:		interrupt handlers, same order as irq names
+ * @irq_names:			array of irq names
+ * @num_irqs:			number of irqs in the arrays
  * @clk_names:			array of clock names
  * @num_clocks:			number of clocks in the array
  */
@@ -73,8 +75,9 @@  struct hantro_variant {
 	const struct hantro_codec_ops *codec_ops;
 	int (*init)(struct hantro_dev *vpu);
 	int (*runtime_resume)(struct hantro_dev *vpu);
-	irqreturn_t (*vepu_irq)(int irq, void *priv);
-	irqreturn_t (*vdpu_irq)(int irq, void *priv);
+	irqreturn_t (*irq_handlers[HANTRO_MAX_IRQS])(int irq, void *priv);
+	const char *irq_names[HANTRO_MAX_IRQS];
+	int num_irqs;
 	const char *clk_names[HANTRO_MAX_CLOCKS];
 	int num_clocks;
 };
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index fb5f140dbaae..f677b40bcd2d 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -705,36 +705,25 @@  static int hantro_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	if (vpu->variant->vdpu_irq) {
+	for (i = 0; i < vpu->variant->num_irqs; i++) {
+		const char *irq_name = vpu->variant->irq_names[i];
 		int irq;
 
-		irq = platform_get_irq_byname(vpu->pdev, "vdpu");
-		if (irq <= 0) {
-			dev_err(vpu->dev, "Could not get vdpu IRQ.\n");
-			return -ENXIO;
-		}
-
-		ret = devm_request_irq(vpu->dev, irq, vpu->variant->vdpu_irq,
-				       0, dev_name(vpu->dev), vpu);
-		if (ret) {
-			dev_err(vpu->dev, "Could not request vdpu IRQ.\n");
-			return ret;
-		}
-	}
-
-	if (vpu->variant->vepu_irq) {
-		int irq;
+		if (!vpu->variant->irq_handlers[i])
+			continue;
 
-		irq = platform_get_irq_byname(vpu->pdev, "vepu");
+		irq = platform_get_irq_byname(vpu->pdev, irq_name);
 		if (irq <= 0) {
-			dev_err(vpu->dev, "Could not get vepu IRQ.\n");
+			dev_err(vpu->dev, "Could not get %s IRQ.\n", irq_name);
 			return -ENXIO;
 		}
 
-		ret = devm_request_irq(vpu->dev, irq, vpu->variant->vepu_irq,
+		ret = devm_request_irq(vpu->dev, irq,
+				       vpu->variant->irq_handlers[i],
 				       0, dev_name(vpu->dev), vpu);
 		if (ret) {
-			dev_err(vpu->dev, "Could not request vepu IRQ.\n");
+			dev_err(vpu->dev, "Could not request %s IRQ.\n",
+				irq_name);
 			return ret;
 		}
 	}
diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
index 38adf74037fc..6263037a9d0c 100644
--- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
+++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
@@ -169,8 +169,9 @@  const struct hantro_variant rk3288_vpu_variant = {
 	.num_dec_fmts = ARRAY_SIZE(rk3288_vpu_dec_fmts),
 	.codec = HANTRO_JPEG_ENCODER | HANTRO_MPEG2_DECODER,
 	.codec_ops = rk3288_vpu_codec_ops,
-	.vepu_irq = rk3288_vepu_irq,
-	.vdpu_irq = rk3288_vdpu_irq,
+	.irq_handlers = { rk3288_vepu_irq, rk3288_vdpu_irq },
+	.irq_names = {"vepu", "vdpu"},
+	.num_irqs = 2,
 	.init = rk3288_vpu_hw_init,
 	.clk_names = {"aclk", "hclk"},
 	.num_clocks = 2
diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw.c b/drivers/staging/media/hantro/rk3399_vpu_hw.c
index 5e51def41f57..ce5ace895393 100644
--- a/drivers/staging/media/hantro/rk3399_vpu_hw.c
+++ b/drivers/staging/media/hantro/rk3399_vpu_hw.c
@@ -169,8 +169,9 @@  const struct hantro_variant rk3399_vpu_variant = {
 	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
 	.codec = HANTRO_JPEG_ENCODER | HANTRO_MPEG2_DECODER,
 	.codec_ops = rk3399_vpu_codec_ops,
-	.vepu_irq = rk3399_vepu_irq,
-	.vdpu_irq = rk3399_vdpu_irq,
+	.irq_handlers = { rk3399_vepu_irq, rk3399_vdpu_irq },
+	.irq_names = {"vepu", "vdpu"},
+	.num_irqs = 2,
 	.init = rk3399_vpu_hw_init,
 	.clk_names = {"aclk", "hclk"},
 	.num_clocks = 2
@@ -182,7 +183,9 @@  const struct hantro_variant rk3328_vpu_variant = {
 	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
 	.codec = HANTRO_MPEG2_DECODER,
 	.codec_ops = rk3399_vpu_codec_ops,
-	.vdpu_irq = rk3399_vdpu_irq,
+	.irq_handlers = { rk3399_vdpu_irq },
+	.irq_names = {"vdpu"},
+	.num_irqs = 1,
 	.init = rk3399_vpu_hw_init,
 	.clk_names = {"aclk", "hclk"},
 	.num_clocks = 2