[PATCH 0/4] media: sunxi-cir: Cleanup and power management

[PATCH 0/4] media: sunxi-cir: Cleanup and power management

[Samuel Holland]

This series cleans up some dead code in the sunxi-cir driver and adds
system power management hooks.

Samuel Holland (4):
  media: sunxi-cir: Clean up dead register writes
  media: sunxi-cir: Remove unnecessary spinlock
  media: sunxi-cir: Factor out hardware initialization
  media: sunxi-cir: Implement suspend/resume/shutdown callbacks

 drivers/media/rc/sunxi-cir.c | 167 ++++++++++++++++++++---------------
 1 file changed, 94 insertions(+), 73 deletions(-)

-- 
2.26.2

[PATCH 1/4] media: sunxi-cir: Clean up dead register writes

[Samuel Holland]

The register writes during driver removal occur after the device is
already put back in reset, so they never had any effect.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/media/rc/sunxi-cir.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
index 8555c7798706..0a7f7eab3cc3 100644
--- a/drivers/media/rc/sunxi-cir.c
+++ b/drivers/media/rc/sunxi-cir.c
@@ -342,22 +342,12 @@ static int sunxi_ir_probe(struct platform_device *pdev)
 
 static int sunxi_ir_remove(struct platform_device *pdev)
 {
-	unsigned long flags;
 	struct sunxi_ir *ir = platform_get_drvdata(pdev);
 
 	clk_disable_unprepare(ir->clk);
 	clk_disable_unprepare(ir->apb_clk);
 	reset_control_assert(ir->rst);
 
-	spin_lock_irqsave(&ir->ir_lock, flags);
-	/* disable IR IRQ */
-	writel(0, ir->base + SUNXI_IR_RXINT_REG);
-	/* clear All Rx Interrupt Status */
-	writel(REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
-	/* disable IR */
-	writel(0, ir->base + SUNXI_IR_CTL_REG);
-	spin_unlock_irqrestore(&ir->ir_lock, flags);
-
 	rc_unregister_device(ir->rc);
 	return 0;
 }
-- 
2.26.2

[PATCH 2/4] media: sunxi-cir: Remove unnecessary spinlock

[Samuel Holland]

Only one register, SUNXI_IR_CIR_REG, is accessed from outside the
interrupt handler, and that register is not accessed from inside it.
As there is no overlap between different contexts, no lock is needed.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/media/rc/sunxi-cir.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
index 0a7f7eab3cc3..48be400421cd 100644
--- a/drivers/media/rc/sunxi-cir.c
+++ b/drivers/media/rc/sunxi-cir.c
@@ -86,7 +86,6 @@ struct sunxi_ir_quirks {
 };
 
 struct sunxi_ir {
-	spinlock_t      ir_lock;
 	struct rc_dev   *rc;
 	void __iomem    *base;
 	int             irq;
@@ -105,8 +104,6 @@ static irqreturn_t sunxi_ir_irq(int irqno, void *dev_id)
 	struct sunxi_ir *ir = dev_id;
 	struct ir_raw_event rawir = {};
 
-	spin_lock(&ir->ir_lock);
-
 	status = readl(ir->base + SUNXI_IR_RXSTA_REG);
 
 	/* clean all pending statuses */
@@ -137,8 +134,6 @@ static irqreturn_t sunxi_ir_irq(int irqno, void *dev_id)
 		ir_raw_event_handle(ir->rc);
 	}
 
-	spin_unlock(&ir->ir_lock);
-
 	return IRQ_HANDLED;
 }
 
@@ -160,17 +155,14 @@ static int sunxi_ir_set_timeout(struct rc_dev *rc_dev, unsigned int timeout)
 {
 	struct sunxi_ir *ir = rc_dev->priv;
 	unsigned int base_clk = clk_get_rate(ir->clk);
-	unsigned long flags;
 
 	unsigned int ithr = sunxi_usec_to_ithr(base_clk, timeout);
 
 	dev_dbg(rc_dev->dev.parent, "setting idle threshold to %u\n", ithr);
 
-	spin_lock_irqsave(&ir->ir_lock, flags);
 	/* Set noise threshold and idle threshold */
 	writel(REG_CIR_NTHR(SUNXI_IR_RXNOISE) | REG_CIR_ITHR(ithr),
 	       ir->base + SUNXI_IR_CIR_REG);
-	spin_unlock_irqrestore(&ir->ir_lock, flags);
 
 	rc_dev->timeout = sunxi_ithr_to_usec(base_clk, ithr);
 
@@ -199,8 +191,6 @@ static int sunxi_ir_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	spin_lock_init(&ir->ir_lock);
-
 	ir->fifo_size = quirks->fifo_size;
 
 	/* Clock */
-- 
2.26.2

[PATCH 3/4] media: sunxi-cir: Factor out hardware initialization

[Samuel Holland]

In preparation for adding suspend/resume hooks, factor out the hardware
initialization from the driver probe/remove functions.

The timeout programmed during init is taken from the `struct rc_dev` so
it is maintained across an exit/init cycle.

This resolves some trivial issues with the probe function: throwing away
the error from clk_prepare_enable and using the wrong type for the
temporary register value.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/media/rc/sunxi-cir.c | 128 ++++++++++++++++++++---------------
 1 file changed, 74 insertions(+), 54 deletions(-)

diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
index 48be400421cd..ccb9d6b4225d 100644
--- a/drivers/media/rc/sunxi-cir.c
+++ b/drivers/media/rc/sunxi-cir.c
@@ -169,10 +169,74 @@ static int sunxi_ir_set_timeout(struct rc_dev *rc_dev, unsigned int timeout)
 	return 0;
 }
 
+static int sunxi_ir_hw_init(struct device *dev)
+{
+	struct sunxi_ir *ir = dev_get_drvdata(dev);
+	u32 tmp;
+	int ret;
+
+	ret = reset_control_deassert(ir->rst);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(ir->apb_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable apb clk\n");
+		goto exit_assert_reset;
+	}
+
+	ret = clk_prepare_enable(ir->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable ir clk\n");
+		goto exit_disable_apb_clk;
+	}
+
+	/* Enable CIR Mode */
+	writel(REG_CTL_MD, ir->base + SUNXI_IR_CTL_REG);
+
+	/* Set noise threshold and idle threshold */
+	sunxi_ir_set_timeout(ir->rc, ir->rc->timeout);
+
+	/* Invert Input Signal */
+	writel(REG_RXCTL_RPPI, ir->base + SUNXI_IR_RXCTL_REG);
+
+	/* Clear All Rx Interrupt Status */
+	writel(REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
+
+	/*
+	 * Enable IRQ on overflow, packet end, FIFO available with trigger
+	 * level
+	 */
+	writel(REG_RXINT_ROI_EN | REG_RXINT_RPEI_EN |
+	       REG_RXINT_RAI_EN | REG_RXINT_RAL(ir->fifo_size / 2 - 1),
+	       ir->base + SUNXI_IR_RXINT_REG);
+
+	/* Enable IR Module */
+	tmp = readl(ir->base + SUNXI_IR_CTL_REG);
+	writel(tmp | REG_CTL_GEN | REG_CTL_RXEN, ir->base + SUNXI_IR_CTL_REG);
+
+	return 0;
+
+exit_disable_apb_clk:
+	clk_disable_unprepare(ir->apb_clk);
+exit_assert_reset:
+	reset_control_assert(ir->rst);
+
+	return ret;
+}
+
+static void sunxi_ir_hw_exit(struct device *dev)
+{
+	struct sunxi_ir *ir = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(ir->clk);
+	clk_disable_unprepare(ir->apb_clk);
+	reset_control_assert(ir->rst);
+}
+
 static int sunxi_ir_probe(struct platform_device *pdev)
 {
 	int ret = 0;
-	unsigned long tmp = 0;
 
 	struct device *dev = &pdev->dev;
 	struct device_node *dn = dev->of_node;
@@ -213,43 +277,26 @@ static int sunxi_ir_probe(struct platform_device *pdev)
 		ir->rst = devm_reset_control_get_exclusive(dev, NULL);
 		if (IS_ERR(ir->rst))
 			return PTR_ERR(ir->rst);
-		ret = reset_control_deassert(ir->rst);
-		if (ret)
-			return ret;
 	}
 
 	ret = clk_set_rate(ir->clk, b_clk_freq);
 	if (ret) {
 		dev_err(dev, "set ir base clock failed!\n");
-		goto exit_reset_assert;
+		return ret;
 	}
 	dev_dbg(dev, "set base clock frequency to %d Hz.\n", b_clk_freq);
 
-	if (clk_prepare_enable(ir->apb_clk)) {
-		dev_err(dev, "try to enable apb_ir_clk failed\n");
-		ret = -EINVAL;
-		goto exit_reset_assert;
-	}
-
-	if (clk_prepare_enable(ir->clk)) {
-		dev_err(dev, "try to enable ir_clk failed\n");
-		ret = -EINVAL;
-		goto exit_clkdisable_apb_clk;
-	}
-
 	/* IO */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ir->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(ir->base)) {
-		ret = PTR_ERR(ir->base);
-		goto exit_clkdisable_clk;
+		return PTR_ERR(ir->base);
 	}
 
 	ir->rc = rc_allocate_device(RC_DRIVER_IR_RAW);
 	if (!ir->rc) {
 		dev_err(dev, "failed to allocate device\n");
-		ret = -ENOMEM;
-		goto exit_clkdisable_clk;
+		return -ENOMEM;
 	}
 
 	ir->rc->priv = ir;
@@ -265,6 +312,7 @@ static int sunxi_ir_probe(struct platform_device *pdev)
 	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
 	/* Frequency after IR internal divider with sample period in us */
 	ir->rc->rx_resolution = (USEC_PER_SEC / (b_clk_freq / 64));
+	ir->rc->timeout = IR_DEFAULT_TIMEOUT;
 	ir->rc->min_timeout = sunxi_ithr_to_usec(b_clk_freq, 0);
 	ir->rc->max_timeout = sunxi_ithr_to_usec(b_clk_freq, 255);
 	ir->rc->s_timeout = sunxi_ir_set_timeout;
@@ -291,41 +339,15 @@ static int sunxi_ir_probe(struct platform_device *pdev)
 		goto exit_free_dev;
 	}
 
-	/* Enable CIR Mode */
-	writel(REG_CTL_MD, ir->base+SUNXI_IR_CTL_REG);
-
-	/* Set noise threshold and idle threshold */
-	sunxi_ir_set_timeout(ir->rc, IR_DEFAULT_TIMEOUT);
-
-	/* Invert Input Signal */
-	writel(REG_RXCTL_RPPI, ir->base + SUNXI_IR_RXCTL_REG);
-
-	/* Clear All Rx Interrupt Status */
-	writel(REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
-
-	/*
-	 * Enable IRQ on overflow, packet end, FIFO available with trigger
-	 * level
-	 */
-	writel(REG_RXINT_ROI_EN | REG_RXINT_RPEI_EN |
-	       REG_RXINT_RAI_EN | REG_RXINT_RAL(ir->fifo_size / 2 - 1),
-	       ir->base + SUNXI_IR_RXINT_REG);
-
-	/* Enable IR Module */
-	tmp = readl(ir->base + SUNXI_IR_CTL_REG);
-	writel(tmp | REG_CTL_GEN | REG_CTL_RXEN, ir->base + SUNXI_IR_CTL_REG);
+	ret = sunxi_ir_hw_init(dev);
+	if (ret)
+		goto exit_free_dev;
 
 	dev_info(dev, "initialized sunXi IR driver\n");
 	return 0;
 
 exit_free_dev:
 	rc_free_device(ir->rc);
-exit_clkdisable_clk:
-	clk_disable_unprepare(ir->clk);
-exit_clkdisable_apb_clk:
-	clk_disable_unprepare(ir->apb_clk);
-exit_reset_assert:
-	reset_control_assert(ir->rst);
 
 	return ret;
 }
@@ -334,11 +356,9 @@ static int sunxi_ir_remove(struct platform_device *pdev)
 {
 	struct sunxi_ir *ir = platform_get_drvdata(pdev);
 
-	clk_disable_unprepare(ir->clk);
-	clk_disable_unprepare(ir->apb_clk);
-	reset_control_assert(ir->rst);
-
+	sunxi_ir_hw_exit(&pdev->dev);
 	rc_unregister_device(ir->rc);
+
 	return 0;
 }
 
-- 
2.26.2

[PATCH 4/4] media: sunxi-cir: Implement suspend/resume/shutdown callbacks

[Samuel Holland]

To save power, gate/reset the hardware block while the system is
asleep or powered off.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/media/rc/sunxi-cir.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
index ccb9d6b4225d..a0bdbf6f66c9 100644
--- a/drivers/media/rc/sunxi-cir.c
+++ b/drivers/media/rc/sunxi-cir.c
@@ -234,6 +234,20 @@ static void sunxi_ir_hw_exit(struct device *dev)
 	reset_control_assert(ir->rst);
 }
 
+static int __maybe_unused sunxi_ir_suspend(struct device *dev)
+{
+	sunxi_ir_hw_exit(dev);
+
+	return 0;
+}
+
+static int __maybe_unused sunxi_ir_resume(struct device *dev)
+{
+	return sunxi_ir_hw_init(dev);
+}
+
+static SIMPLE_DEV_PM_OPS(sunxi_ir_pm_ops, sunxi_ir_suspend, sunxi_ir_resume);
+
 static int sunxi_ir_probe(struct platform_device *pdev)
 {
 	int ret = 0;
@@ -362,6 +376,11 @@ static int sunxi_ir_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void sunxi_ir_shutdown(struct platform_device *pdev)
+{
+	sunxi_ir_hw_exit(&pdev->dev);
+}
+
 static const struct sunxi_ir_quirks sun4i_a10_ir_quirks = {
 	.has_reset = false,
 	.fifo_size = 16,
@@ -397,9 +416,11 @@ MODULE_DEVICE_TABLE(of, sunxi_ir_match);
 static struct platform_driver sunxi_ir_driver = {
 	.probe          = sunxi_ir_probe,
 	.remove         = sunxi_ir_remove,
+	.shutdown       = sunxi_ir_shutdown,
 	.driver = {
 		.name = SUNXI_IR_DEV,
 		.of_match_table = sunxi_ir_match,
+		.pm = &sunxi_ir_pm_ops,
 	},
 };
 
-- 
2.26.2

Re: [PATCH 0/4] media: sunxi-cir: Cleanup and power management

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 231 bytes --]

On Tue, Jan 12, 2021 at 10:51:28PM -0600, Samuel Holland wrote:
> This series cleans up some dead code in the sunxi-cir driver and adds
> system power management hooks.

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] media: sunxi-cir: Clean up dead register writes

[Sean Young]

On Tue, Jan 12, 2021 at 10:51:29PM -0600, Samuel Holland wrote:
> The register writes during driver removal occur after the device is
> already put back in reset, so they never had any effect.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/media/rc/sunxi-cir.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> index 8555c7798706..0a7f7eab3cc3 100644
> --- a/drivers/media/rc/sunxi-cir.c
> +++ b/drivers/media/rc/sunxi-cir.c
> @@ -342,22 +342,12 @@ static int sunxi_ir_probe(struct platform_device *pdev)
>  
>  static int sunxi_ir_remove(struct platform_device *pdev)
>  {
> -	unsigned long flags;
>  	struct sunxi_ir *ir = platform_get_drvdata(pdev);
>  
>  	clk_disable_unprepare(ir->clk);
>  	clk_disable_unprepare(ir->apb_clk);
>  	reset_control_assert(ir->rst);
>  
> -	spin_lock_irqsave(&ir->ir_lock, flags);
> -	/* disable IR IRQ */
> -	writel(0, ir->base + SUNXI_IR_RXINT_REG);
> -	/* clear All Rx Interrupt Status */
> -	writel(REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
> -	/* disable IR */
> -	writel(0, ir->base + SUNXI_IR_CTL_REG);
> -	spin_unlock_irqrestore(&ir->ir_lock, flags);
> -
>  	rc_unregister_device(ir->rc);
>  	return 0;
>  }

I don't think there is anything wrong with the patch, however here the
driver does rc_unregister_device() *after* disabling it. Userspace can
still hold a file descriptor open, and call e.g. LIRC_SET_REC_TIMEOUT
ioctl, which causes various writes the sunxi-cir registers.

The order should be reversed.


Sean

Re: [PATCH 3/4] media: sunxi-cir: Factor out hardware initialization

[Sean Young]

On Tue, Jan 12, 2021 at 10:51:31PM -0600, Samuel Holland wrote:
> In preparation for adding suspend/resume hooks, factor out the hardware
> initialization from the driver probe/remove functions.
> 
> The timeout programmed during init is taken from the `struct rc_dev` so
> it is maintained across an exit/init cycle.
> 
> This resolves some trivial issues with the probe function: throwing away
> the error from clk_prepare_enable and using the wrong type for the
> temporary register value.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/media/rc/sunxi-cir.c | 128 ++++++++++++++++++++---------------
>  1 file changed, 74 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> index 48be400421cd..ccb9d6b4225d 100644
> --- a/drivers/media/rc/sunxi-cir.c
> +++ b/drivers/media/rc/sunxi-cir.c
> @@ -169,10 +169,74 @@ static int sunxi_ir_set_timeout(struct rc_dev *rc_dev, unsigned int timeout)
>  	return 0;
>  }
>  
> +static int sunxi_ir_hw_init(struct device *dev)
> +{
> +	struct sunxi_ir *ir = dev_get_drvdata(dev);
> +	u32 tmp;
> +	int ret;
> +
> +	ret = reset_control_deassert(ir->rst);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(ir->apb_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable apb clk\n");
> +		goto exit_assert_reset;
> +	}
> +
> +	ret = clk_prepare_enable(ir->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable ir clk\n");
> +		goto exit_disable_apb_clk;
> +	}
> +
> +	/* Enable CIR Mode */
> +	writel(REG_CTL_MD, ir->base + SUNXI_IR_CTL_REG);
> +
> +	/* Set noise threshold and idle threshold */
> +	sunxi_ir_set_timeout(ir->rc, ir->rc->timeout);
> +
> +	/* Invert Input Signal */
> +	writel(REG_RXCTL_RPPI, ir->base + SUNXI_IR_RXCTL_REG);
> +
> +	/* Clear All Rx Interrupt Status */
> +	writel(REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
> +
> +	/*
> +	 * Enable IRQ on overflow, packet end, FIFO available with trigger
> +	 * level
> +	 */
> +	writel(REG_RXINT_ROI_EN | REG_RXINT_RPEI_EN |
> +	       REG_RXINT_RAI_EN | REG_RXINT_RAL(ir->fifo_size / 2 - 1),
> +	       ir->base + SUNXI_IR_RXINT_REG);
> +
> +	/* Enable IR Module */
> +	tmp = readl(ir->base + SUNXI_IR_CTL_REG);
> +	writel(tmp | REG_CTL_GEN | REG_CTL_RXEN, ir->base + SUNXI_IR_CTL_REG);
> +
> +	return 0;
> +
> +exit_disable_apb_clk:
> +	clk_disable_unprepare(ir->apb_clk);
> +exit_assert_reset:
> +	reset_control_assert(ir->rst);
> +
> +	return ret;
> +}
> +
> +static void sunxi_ir_hw_exit(struct device *dev)
> +{
> +	struct sunxi_ir *ir = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(ir->clk);
> +	clk_disable_unprepare(ir->apb_clk);
> +	reset_control_assert(ir->rst);
> +}
> +
>  static int sunxi_ir_probe(struct platform_device *pdev)
>  {
>  	int ret = 0;
> -	unsigned long tmp = 0;
>  
>  	struct device *dev = &pdev->dev;
>  	struct device_node *dn = dev->of_node;
> @@ -213,43 +277,26 @@ static int sunxi_ir_probe(struct platform_device *pdev)
>  		ir->rst = devm_reset_control_get_exclusive(dev, NULL);
>  		if (IS_ERR(ir->rst))
>  			return PTR_ERR(ir->rst);
> -		ret = reset_control_deassert(ir->rst);
> -		if (ret)
> -			return ret;
>  	}
>  
>  	ret = clk_set_rate(ir->clk, b_clk_freq);
>  	if (ret) {
>  		dev_err(dev, "set ir base clock failed!\n");
> -		goto exit_reset_assert;
> +		return ret;
>  	}
>  	dev_dbg(dev, "set base clock frequency to %d Hz.\n", b_clk_freq);
>  
> -	if (clk_prepare_enable(ir->apb_clk)) {
> -		dev_err(dev, "try to enable apb_ir_clk failed\n");
> -		ret = -EINVAL;
> -		goto exit_reset_assert;
> -	}
> -
> -	if (clk_prepare_enable(ir->clk)) {
> -		dev_err(dev, "try to enable ir_clk failed\n");
> -		ret = -EINVAL;
> -		goto exit_clkdisable_apb_clk;
> -	}
> -
>  	/* IO */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	ir->base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(ir->base)) {
> -		ret = PTR_ERR(ir->base);
> -		goto exit_clkdisable_clk;
> +		return PTR_ERR(ir->base);
>  	}
>  
>  	ir->rc = rc_allocate_device(RC_DRIVER_IR_RAW);
>  	if (!ir->rc) {
>  		dev_err(dev, "failed to allocate device\n");
> -		ret = -ENOMEM;
> -		goto exit_clkdisable_clk;
> +		return -ENOMEM;
>  	}
>  
>  	ir->rc->priv = ir;
> @@ -265,6 +312,7 @@ static int sunxi_ir_probe(struct platform_device *pdev)
>  	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
>  	/* Frequency after IR internal divider with sample period in us */
>  	ir->rc->rx_resolution = (USEC_PER_SEC / (b_clk_freq / 64));
> +	ir->rc->timeout = IR_DEFAULT_TIMEOUT;

Why? This is set from sunxi_ir_set_timeout().

>  	ir->rc->min_timeout = sunxi_ithr_to_usec(b_clk_freq, 0);
>  	ir->rc->max_timeout = sunxi_ithr_to_usec(b_clk_freq, 255);
>  	ir->rc->s_timeout = sunxi_ir_set_timeout;
> @@ -291,41 +339,15 @@ static int sunxi_ir_probe(struct platform_device *pdev)
>  		goto exit_free_dev;
>  	}
>  
> -	/* Enable CIR Mode */
> -	writel(REG_CTL_MD, ir->base+SUNXI_IR_CTL_REG);
> -
> -	/* Set noise threshold and idle threshold */
> -	sunxi_ir_set_timeout(ir->rc, IR_DEFAULT_TIMEOUT);
> -
> -	/* Invert Input Signal */
> -	writel(REG_RXCTL_RPPI, ir->base + SUNXI_IR_RXCTL_REG);
> -
> -	/* Clear All Rx Interrupt Status */
> -	writel(REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
> -
> -	/*
> -	 * Enable IRQ on overflow, packet end, FIFO available with trigger
> -	 * level
> -	 */
> -	writel(REG_RXINT_ROI_EN | REG_RXINT_RPEI_EN |
> -	       REG_RXINT_RAI_EN | REG_RXINT_RAL(ir->fifo_size / 2 - 1),
> -	       ir->base + SUNXI_IR_RXINT_REG);
> -
> -	/* Enable IR Module */
> -	tmp = readl(ir->base + SUNXI_IR_CTL_REG);
> -	writel(tmp | REG_CTL_GEN | REG_CTL_RXEN, ir->base + SUNXI_IR_CTL_REG);
> +	ret = sunxi_ir_hw_init(dev);
> +	if (ret)
> +		goto exit_free_dev;
>  
>  	dev_info(dev, "initialized sunXi IR driver\n");
>  	return 0;
>  
>  exit_free_dev:
>  	rc_free_device(ir->rc);
> -exit_clkdisable_clk:
> -	clk_disable_unprepare(ir->clk);
> -exit_clkdisable_apb_clk:
> -	clk_disable_unprepare(ir->apb_clk);
> -exit_reset_assert:
> -	reset_control_assert(ir->rst);
>  
>  	return ret;
>  }
> @@ -334,11 +356,9 @@ static int sunxi_ir_remove(struct platform_device *pdev)
>  {
>  	struct sunxi_ir *ir = platform_get_drvdata(pdev);
>  
> -	clk_disable_unprepare(ir->clk);
> -	clk_disable_unprepare(ir->apb_clk);
> -	reset_control_assert(ir->rst);
> -
> +	sunxi_ir_hw_exit(&pdev->dev);
>  	rc_unregister_device(ir->rc);
> +
>  	return 0;
>  }
>  
> -- 
> 2.26.2

Re: [PATCH 3/4] media: sunxi-cir: Factor out hardware initialization

[Samuel Holland]

On 1/13/21 8:36 AM, Sean Young wrote:
> On Tue, Jan 12, 2021 at 10:51:31PM -0600, Samuel Holland wrote:
>> In preparation for adding suspend/resume hooks, factor out the hardware
>> initialization from the driver probe/remove functions.
>>
>> The timeout programmed during init is taken from the `struct rc_dev` so
>> it is maintained across an exit/init cycle.
>>
>> This resolves some trivial issues with the probe function: throwing away
>> the error from clk_prepare_enable and using the wrong type for the
>> temporary register value.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  drivers/media/rc/sunxi-cir.c | 128 ++++++++++++++++++++---------------
>>  1 file changed, 74 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
>> index 48be400421cd..ccb9d6b4225d 100644
>> --- a/drivers/media/rc/sunxi-cir.c
>> +++ b/drivers/media/rc/sunxi-cir.c
>> @@ -169,10 +169,74 @@ static int sunxi_ir_set_timeout(struct rc_dev *rc_dev, unsigned int timeout)
>>  	return 0;
>>  }
>>  
>> +static int sunxi_ir_hw_init(struct device *dev)
>> +{
>> +	struct sunxi_ir *ir = dev_get_drvdata(dev);
>> +	u32 tmp;
>> +	int ret;
>> +
>> +	ret = reset_control_deassert(ir->rst);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = clk_prepare_enable(ir->apb_clk);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable apb clk\n");
>> +		goto exit_assert_reset;
>> +	}
>> +
>> +	ret = clk_prepare_enable(ir->clk);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable ir clk\n");
>> +		goto exit_disable_apb_clk;
>> +	}
>> +
>> +	/* Enable CIR Mode */
>> +	writel(REG_CTL_MD, ir->base + SUNXI_IR_CTL_REG);
>> +
>> +	/* Set noise threshold and idle threshold */
>> +	sunxi_ir_set_timeout(ir->rc, ir->rc->timeout);

Initializing ir->rc->timeout in .probe is needed because of this line.
As the changelog mentions, this reprograms the user-configured timeout
after an exit/init (suspend/resume) cycle. It needs some default value
the first time, when called from .probe.

>> +
>> +	/* Invert Input Signal */
>> +	writel(REG_RXCTL_RPPI, ir->base + SUNXI_IR_RXCTL_REG);
>> +
>> +	/* Clear All Rx Interrupt Status */
>> +	writel(REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
>> +
>> +	/*
>> +	 * Enable IRQ on overflow, packet end, FIFO available with trigger
>> +	 * level
>> +	 */
>> +	writel(REG_RXINT_ROI_EN | REG_RXINT_RPEI_EN |
>> +	       REG_RXINT_RAI_EN | REG_RXINT_RAL(ir->fifo_size / 2 - 1),
>> +	       ir->base + SUNXI_IR_RXINT_REG);
>> +
>> +	/* Enable IR Module */
>> +	tmp = readl(ir->base + SUNXI_IR_CTL_REG);
>> +	writel(tmp | REG_CTL_GEN | REG_CTL_RXEN, ir->base + SUNXI_IR_CTL_REG);
>> +
>> +	return 0;
>> +
>> +exit_disable_apb_clk:
>> +	clk_disable_unprepare(ir->apb_clk);
>> +exit_assert_reset:
>> +	reset_control_assert(ir->rst);
>> +
>> +	return ret;
>> +}
>> +
>> +static void sunxi_ir_hw_exit(struct device *dev)
>> +{
>> +	struct sunxi_ir *ir = dev_get_drvdata(dev);
>> +
>> +	clk_disable_unprepare(ir->clk);
>> +	clk_disable_unprepare(ir->apb_clk);
>> +	reset_control_assert(ir->rst);
>> +}
>> +
>>  static int sunxi_ir_probe(struct platform_device *pdev)
>>  {
>>  	int ret = 0;
>> -	unsigned long tmp = 0;
>>  
>>  	struct device *dev = &pdev->dev;
>>  	struct device_node *dn = dev->of_node;
>> @@ -213,43 +277,26 @@ static int sunxi_ir_probe(struct platform_device *pdev)
>>  		ir->rst = devm_reset_control_get_exclusive(dev, NULL);
>>  		if (IS_ERR(ir->rst))
>>  			return PTR_ERR(ir->rst);
>> -		ret = reset_control_deassert(ir->rst);
>> -		if (ret)
>> -			return ret;
>>  	}
>>  
>>  	ret = clk_set_rate(ir->clk, b_clk_freq);
>>  	if (ret) {
>>  		dev_err(dev, "set ir base clock failed!\n");
>> -		goto exit_reset_assert;
>> +		return ret;
>>  	}
>>  	dev_dbg(dev, "set base clock frequency to %d Hz.\n", b_clk_freq);
>>  
>> -	if (clk_prepare_enable(ir->apb_clk)) {
>> -		dev_err(dev, "try to enable apb_ir_clk failed\n");
>> -		ret = -EINVAL;
>> -		goto exit_reset_assert;
>> -	}
>> -
>> -	if (clk_prepare_enable(ir->clk)) {
>> -		dev_err(dev, "try to enable ir_clk failed\n");
>> -		ret = -EINVAL;
>> -		goto exit_clkdisable_apb_clk;
>> -	}
>> -
>>  	/* IO */
>>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  	ir->base = devm_ioremap_resource(dev, res);
>>  	if (IS_ERR(ir->base)) {
>> -		ret = PTR_ERR(ir->base);
>> -		goto exit_clkdisable_clk;
>> +		return PTR_ERR(ir->base);
>>  	}
>>  
>>  	ir->rc = rc_allocate_device(RC_DRIVER_IR_RAW);
>>  	if (!ir->rc) {
>>  		dev_err(dev, "failed to allocate device\n");
>> -		ret = -ENOMEM;
>> -		goto exit_clkdisable_clk;
>> +		return -ENOMEM;
>>  	}
>>  
>>  	ir->rc->priv = ir;
>> @@ -265,6 +312,7 @@ static int sunxi_ir_probe(struct platform_device *pdev)
>>  	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
>>  	/* Frequency after IR internal divider with sample period in us */
>>  	ir->rc->rx_resolution = (USEC_PER_SEC / (b_clk_freq / 64));
>> +	ir->rc->timeout = IR_DEFAULT_TIMEOUT;
> 
> Why? This is set from sunxi_ir_set_timeout().

Because it is also sent as an argument to sunxi_ir_set_timeout().

>>  	ir->rc->min_timeout = sunxi_ithr_to_usec(b_clk_freq, 0);
>>  	ir->rc->max_timeout = sunxi_ithr_to_usec(b_clk_freq, 255);
>>  	ir->rc->s_timeout = sunxi_ir_set_timeout;
>> @@ -291,41 +339,15 @@ static int sunxi_ir_probe(struct platform_device *pdev)
>>  		goto exit_free_dev;
>>  	}
>>  
>> -	/* Enable CIR Mode */
>> -	writel(REG_CTL_MD, ir->base+SUNXI_IR_CTL_REG);
>> -
>> -	/* Set noise threshold and idle threshold */
>> -	sunxi_ir_set_timeout(ir->rc, IR_DEFAULT_TIMEOUT);

This is where the default timeout was originally programmed.

>> -
>> -	/* Invert Input Signal */
>> -	writel(REG_RXCTL_RPPI, ir->base + SUNXI_IR_RXCTL_REG);
>> -
>> -	/* Clear All Rx Interrupt Status */
>> -	writel(REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
>> -
>> -	/*
>> -	 * Enable IRQ on overflow, packet end, FIFO available with trigger
>> -	 * level
>> -	 */
>> -	writel(REG_RXINT_ROI_EN | REG_RXINT_RPEI_EN |
>> -	       REG_RXINT_RAI_EN | REG_RXINT_RAL(ir->fifo_size / 2 - 1),
>> -	       ir->base + SUNXI_IR_RXINT_REG);
>> -
>> -	/* Enable IR Module */
>> -	tmp = readl(ir->base + SUNXI_IR_CTL_REG);
>> -	writel(tmp | REG_CTL_GEN | REG_CTL_RXEN, ir->base + SUNXI_IR_CTL_REG);
>> +	ret = sunxi_ir_hw_init(dev);
>> +	if (ret)
>> +		goto exit_free_dev;
>>  
>>  	dev_info(dev, "initialized sunXi IR driver\n");
>>  	return 0;
>>  
>>  exit_free_dev:
>>  	rc_free_device(ir->rc);
>> -exit_clkdisable_clk:
>> -	clk_disable_unprepare(ir->clk);
>> -exit_clkdisable_apb_clk:
>> -	clk_disable_unprepare(ir->apb_clk);
>> -exit_reset_assert:
>> -	reset_control_assert(ir->rst);
>>  
>>  	return ret;
>>  }
>> @@ -334,11 +356,9 @@ static int sunxi_ir_remove(struct platform_device *pdev)
>>  {
>>  	struct sunxi_ir *ir = platform_get_drvdata(pdev);
>>  
>> -	clk_disable_unprepare(ir->clk);
>> -	clk_disable_unprepare(ir->apb_clk);
>> -	reset_control_assert(ir->rst);
>> -
>> +	sunxi_ir_hw_exit(&pdev->dev);
>>  	rc_unregister_device(ir->rc);

I can swap these lines to fix your comment on patch 1.

>> +
>>  	return 0;
>>  }
>>  
>> -- 
>> 2.26.2

Re: [PATCH 3/4] media: sunxi-cir: Factor out hardware initialization

[Sean Young]

Hi Samuel,

On Wed, Jan 13, 2021 at 09:00:20AM -0600, Samuel Holland wrote:
> On 1/13/21 8:36 AM, Sean Young wrote:
> > On Tue, Jan 12, 2021 at 10:51:31PM -0600, Samuel Holland wrote:
> >> In preparation for adding suspend/resume hooks, factor out the hardware
> >> initialization from the driver probe/remove functions.
> >>
> >> The timeout programmed during init is taken from the `struct rc_dev` so
> >> it is maintained across an exit/init cycle.
> >>
> >> This resolves some trivial issues with the probe function: throwing away
> >> the error from clk_prepare_enable and using the wrong type for the
> >> temporary register value.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>  drivers/media/rc/sunxi-cir.c | 128 ++++++++++++++++++++---------------
> >>  1 file changed, 74 insertions(+), 54 deletions(-)
> >>
> >> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> >> index 48be400421cd..ccb9d6b4225d 100644
> >> --- a/drivers/media/rc/sunxi-cir.c
> >> +++ b/drivers/media/rc/sunxi-cir.c
> >> @@ -169,10 +169,74 @@ static int sunxi_ir_set_timeout(struct rc_dev *rc_dev, unsigned int timeout)
> >>  	return 0;
> >>  }
> >>  
> >> +static int sunxi_ir_hw_init(struct device *dev)
> >> +{
> >> +	struct sunxi_ir *ir = dev_get_drvdata(dev);
> >> +	u32 tmp;
> >> +	int ret;
> >> +
> >> +	ret = reset_control_deassert(ir->rst);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = clk_prepare_enable(ir->apb_clk);
> >> +	if (ret) {
> >> +		dev_err(dev, "failed to enable apb clk\n");
> >> +		goto exit_assert_reset;
> >> +	}
> >> +
> >> +	ret = clk_prepare_enable(ir->clk);
> >> +	if (ret) {
> >> +		dev_err(dev, "failed to enable ir clk\n");
> >> +		goto exit_disable_apb_clk;
> >> +	}
> >> +
> >> +	/* Enable CIR Mode */
> >> +	writel(REG_CTL_MD, ir->base + SUNXI_IR_CTL_REG);
> >> +
> >> +	/* Set noise threshold and idle threshold */
> >> +	sunxi_ir_set_timeout(ir->rc, ir->rc->timeout);
> 
> Initializing ir->rc->timeout in .probe is needed because of this line.
> As the changelog mentions, this reprograms the user-configured timeout
> after an exit/init (suspend/resume) cycle. It needs some default value
> the first time, when called from .probe.

Yes, you're completely right. Sorry about that.

> >> +
> >> +	/* Invert Input Signal */
> >> +	writel(REG_RXCTL_RPPI, ir->base + SUNXI_IR_RXCTL_REG);
> >> +
> >> +	/* Clear All Rx Interrupt Status */
> >> +	writel(REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
> >> +
> >> +	/*
> >> +	 * Enable IRQ on overflow, packet end, FIFO available with trigger
> >> +	 * level
> >> +	 */
> >> +	writel(REG_RXINT_ROI_EN | REG_RXINT_RPEI_EN |
> >> +	       REG_RXINT_RAI_EN | REG_RXINT_RAL(ir->fifo_size / 2 - 1),
> >> +	       ir->base + SUNXI_IR_RXINT_REG);
> >> +
> >> +	/* Enable IR Module */
> >> +	tmp = readl(ir->base + SUNXI_IR_CTL_REG);
> >> +	writel(tmp | REG_CTL_GEN | REG_CTL_RXEN, ir->base + SUNXI_IR_CTL_REG);
> >> +
> >> +	return 0;
> >> +
> >> +exit_disable_apb_clk:
> >> +	clk_disable_unprepare(ir->apb_clk);
> >> +exit_assert_reset:
> >> +	reset_control_assert(ir->rst);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static void sunxi_ir_hw_exit(struct device *dev)
> >> +{
> >> +	struct sunxi_ir *ir = dev_get_drvdata(dev);
> >> +
> >> +	clk_disable_unprepare(ir->clk);
> >> +	clk_disable_unprepare(ir->apb_clk);
> >> +	reset_control_assert(ir->rst);
> >> +}
> >> +
> >>  static int sunxi_ir_probe(struct platform_device *pdev)
> >>  {
> >>  	int ret = 0;
> >> -	unsigned long tmp = 0;
> >>  
> >>  	struct device *dev = &pdev->dev;
> >>  	struct device_node *dn = dev->of_node;
> >> @@ -213,43 +277,26 @@ static int sunxi_ir_probe(struct platform_device *pdev)
> >>  		ir->rst = devm_reset_control_get_exclusive(dev, NULL);
> >>  		if (IS_ERR(ir->rst))
> >>  			return PTR_ERR(ir->rst);
> >> -		ret = reset_control_deassert(ir->rst);
> >> -		if (ret)
> >> -			return ret;
> >>  	}
> >>  
> >>  	ret = clk_set_rate(ir->clk, b_clk_freq);
> >>  	if (ret) {
> >>  		dev_err(dev, "set ir base clock failed!\n");
> >> -		goto exit_reset_assert;
> >> +		return ret;
> >>  	}
> >>  	dev_dbg(dev, "set base clock frequency to %d Hz.\n", b_clk_freq);
> >>  
> >> -	if (clk_prepare_enable(ir->apb_clk)) {
> >> -		dev_err(dev, "try to enable apb_ir_clk failed\n");
> >> -		ret = -EINVAL;
> >> -		goto exit_reset_assert;
> >> -	}
> >> -
> >> -	if (clk_prepare_enable(ir->clk)) {
> >> -		dev_err(dev, "try to enable ir_clk failed\n");
> >> -		ret = -EINVAL;
> >> -		goto exit_clkdisable_apb_clk;
> >> -	}
> >> -
> >>  	/* IO */
> >>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>  	ir->base = devm_ioremap_resource(dev, res);
> >>  	if (IS_ERR(ir->base)) {
> >> -		ret = PTR_ERR(ir->base);
> >> -		goto exit_clkdisable_clk;
> >> +		return PTR_ERR(ir->base);
> >>  	}
> >>  
> >>  	ir->rc = rc_allocate_device(RC_DRIVER_IR_RAW);
> >>  	if (!ir->rc) {
> >>  		dev_err(dev, "failed to allocate device\n");
> >> -		ret = -ENOMEM;
> >> -		goto exit_clkdisable_clk;
> >> +		return -ENOMEM;
> >>  	}
> >>  
> >>  	ir->rc->priv = ir;
> >> @@ -265,6 +312,7 @@ static int sunxi_ir_probe(struct platform_device *pdev)
> >>  	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
> >>  	/* Frequency after IR internal divider with sample period in us */
> >>  	ir->rc->rx_resolution = (USEC_PER_SEC / (b_clk_freq / 64));
> >> +	ir->rc->timeout = IR_DEFAULT_TIMEOUT;
> > 
> > Why? This is set from sunxi_ir_set_timeout().
> 
> Because it is also sent as an argument to sunxi_ir_set_timeout().

Indeed it is.

> >>  	ir->rc->min_timeout = sunxi_ithr_to_usec(b_clk_freq, 0);
> >>  	ir->rc->max_timeout = sunxi_ithr_to_usec(b_clk_freq, 255);
> >>  	ir->rc->s_timeout = sunxi_ir_set_timeout;
> >> @@ -291,41 +339,15 @@ static int sunxi_ir_probe(struct platform_device *pdev)
> >>  		goto exit_free_dev;
> >>  	}
> >>  
> >> -	/* Enable CIR Mode */
> >> -	writel(REG_CTL_MD, ir->base+SUNXI_IR_CTL_REG);
> >> -
> >> -	/* Set noise threshold and idle threshold */
> >> -	sunxi_ir_set_timeout(ir->rc, IR_DEFAULT_TIMEOUT);
> 
> This is where the default timeout was originally programmed.
> 
> >> -
> >> -	/* Invert Input Signal */
> >> -	writel(REG_RXCTL_RPPI, ir->base + SUNXI_IR_RXCTL_REG);
> >> -
> >> -	/* Clear All Rx Interrupt Status */
> >> -	writel(REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
> >> -
> >> -	/*
> >> -	 * Enable IRQ on overflow, packet end, FIFO available with trigger
> >> -	 * level
> >> -	 */
> >> -	writel(REG_RXINT_ROI_EN | REG_RXINT_RPEI_EN |
> >> -	       REG_RXINT_RAI_EN | REG_RXINT_RAL(ir->fifo_size / 2 - 1),
> >> -	       ir->base + SUNXI_IR_RXINT_REG);
> >> -
> >> -	/* Enable IR Module */
> >> -	tmp = readl(ir->base + SUNXI_IR_CTL_REG);
> >> -	writel(tmp | REG_CTL_GEN | REG_CTL_RXEN, ir->base + SUNXI_IR_CTL_REG);
> >> +	ret = sunxi_ir_hw_init(dev);
> >> +	if (ret)
> >> +		goto exit_free_dev;
> >>  
> >>  	dev_info(dev, "initialized sunXi IR driver\n");
> >>  	return 0;
> >>  
> >>  exit_free_dev:
> >>  	rc_free_device(ir->rc);
> >> -exit_clkdisable_clk:
> >> -	clk_disable_unprepare(ir->clk);
> >> -exit_clkdisable_apb_clk:
> >> -	clk_disable_unprepare(ir->apb_clk);
> >> -exit_reset_assert:
> >> -	reset_control_assert(ir->rst);
> >>  
> >>  	return ret;
> >>  }
> >> @@ -334,11 +356,9 @@ static int sunxi_ir_remove(struct platform_device *pdev)
> >>  {
> >>  	struct sunxi_ir *ir = platform_get_drvdata(pdev);
> >>  
> >> -	clk_disable_unprepare(ir->clk);
> >> -	clk_disable_unprepare(ir->apb_clk);
> >> -	reset_control_assert(ir->rst);
> >> -
> >> +	sunxi_ir_hw_exit(&pdev->dev);
> >>  	rc_unregister_device(ir->rc);
> 
> I can swap these lines to fix your comment on patch 1.

Please do, and mention it in the commit message.

It might be harmless to write to SUNXI_IR_CIR_REG but it's not really the
right thing to do. It should be idiomatic to call rc_unregister_device()
first in the remove() function.

Thank you for that.

Sean

> 
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> -- 
> >> 2.26.2