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@xxxxxxxxxxxx> >> --- >> 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