Re: [PATCH v3 04/13] clk: at91: make IRQ optional and register them later

[Boris Brezillon]

Hi Stephen,

On Thu, 14 Jan 2016 18:02:37 -0800
Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:

> On 12/04, Alexandre Belloni wrote:
> > @@ -686,3 +652,45 @@ static void __init of_at91sam9x5_clk_main_setup(struct device_node *np)
> >  }
> >  CLK_OF_DECLARE(at91sam9x5_clk_main, "atmel,at91sam9x5-clk-main",
> >  	       of_at91sam9x5_clk_main_setup);
> > +
> > +static const struct of_device_id atmel_clk_main_dt_ids[] = {
> > +	{ .compatible = "atmel,at91rm9200-clk-main-osc" },
> > +	{ .compatible = "atmel,at91sam9x5-clk-main-rc-osc" },
> > +	{ .compatible = "atmel,at91sam9x5-clk-main" },
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static int __init atmel_clk_main_probe(struct platform_device *pdev)
> > +{
> > +	struct of_phandle_args clkspec = { .np = pdev->dev.of_node};
> > +	struct clk_main *clkmain;
> > +	struct clk_hw *hw;
> > +	int ret;
> > +
> > +	hw = __clk_get_hw(of_clk_get_from_provider(&clkspec));
> > +	if (!hw)
> > +		return -ENODEV;
> > +
> > +	clkmain = to_clk_main(hw);
> > +	clkmain->irq = platform_get_irq(pdev, 0);
> > +	if (!clkmain->irq)
> > +		return 0;
> 
> Is there any way to get the irq into this probe function without
> getting a clk pointer and then unwrapping it to get an irq
> value out of the clk_hw wrapper structure? That's a pretty
> convoluted design.

Not sure I get what you're suggesting, but the only solution I see to
avoid this "get main clk pointer dance" would be to have a global
variable storing the clk_main instance (or a list of clk_main
instances).
The thing is, I'd like to avoid adding new global variables in this
clk drivers as much as possible. Moreover, the core is already taking
care of keeping the list of all registered clks, with their associated
of_node, so, is there a real point in creating a duplicated association
list in this driver too?

Maybe we could create an of_clk_hw_get_from_provider(), which would
prevent the creation of this useless per-user clk instance (see the patch
below).

> 
> > +
> > +	init_waitqueue_head(&clkmain->wait);
> > +	irq_set_status_flags(clkmain->irq, IRQ_NOAUTOEN);
> > +	ret = devm_request_irq(&pdev->dev, clkmain->irq, clk_main_irq_handler,
> > +			       IRQF_TRIGGER_HIGH, __clk_get_name(hw->clk),
> > +			       clkmain);
> > +	if (ret)
> > +		clkmain->irq = 0;
> > +

You should probably call clk_put() on the pointer returned by
of_clk_get_from_provider(&clkspec).

> > +	return ret;
> > +}
> > +
> 

Best Regards,

Boris

--- >8 ---