Re: [PATCH v9 0/3] Tango PCIe controller support

[Marc Zyngier <marc.zyngier@arm.com>]

On 06/07/17 13:26, Mason wrote:
> On 06/07/2017 05:39, Bjorn Helgaas wrote:
> 
>> On Wed, Jul 05, 2017 at 11:59:33PM +0200, Mason wrote:
>>
>>> There were a few nits I wanted to address:
>>>
>>> - Since we added suppress_bind_attrs = true, probe()
>>> can only be called at init, so I wanted to mark __init
>>> all the probe functions, to save space.
>>>
>>> - I left the definition of MSI_MAX in the wrong patch
>>>
>>> - You put a pointer to the pdev in the struct tango_pcie.
>>> I think this is redundant, since the pdev already has a
>>> pointer to the struct, as drvdata.
>>> So I wanted to change tango_msi_probe() to take a pdev
>>> as argument (to make it more like an actual probe function)
>>> and derive pcie from pdev, instead of the other way around.
>>
>> I don't think tango_msi_probe() is really a "probe" function.  It's
>> all part of the tango driver, and it's not claiming a separate piece
>> of hardware.
> 
> I agree that tango_msi_probe() is not a probe function;
> it is merely a piece of the actual probe function (static
> with single call site). I split the probe function in two,
> because it seemed to make sense at the time.
> 
> Perhaps it's better to inline tango_msi_probe? That would
> avoid the issues of that function's name and parameters.
> 
> If you think it's better to keep the two pieces separate,
> I can rename the MSI part to tango_msi_init() or some such.
> But I'd like to avoid adding unnecessary fields to the struct.
> 
>>  So I would keep the name and structure similar to these:
>>
>>   advk_pcie_init_msi_irq_domain()
>>   nwl_pcie_init_msi_irq_domain()
>>
>> BTW, those functions use irq_domain_add_linear(), while you are one of
>> the very few callers of irq_domain_create_linear().  Why the difference?
>> If your code does basically the same thing, it's very helpful to me if
>> it *looks* basically the same.

irq_domain_add_linear() can only take an of_node as the identifier for
the domain, while the _create_ variants use a fwnode. Given that an
of+node is also a fwnode, the former is now deprecated in favour of the
latter.

> 
> It was a suggestion from Marc Z on 2017-03-23.
> 
> <QUOTE>
> + irq_dom = irq_domain_add_linear(NULL, MSI_COUNT, &msi_domain_ops, pcie);
> 
> Use irq_domain_create_linear, pass the same fwnode.
> </QUOTE>
> 
> It seems odd to pass NULL as the first argument.
> (As I had first done, when copying the Altera driver.)

Indeed, as it creates a "default" domain, which is almost always wrong.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...