Re: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller support

[Marc Zyngier]

On 25/02/16 03:21, Minghuan Lian wrote:
> Hi Marc,
> 
> I am sorry for the delayed response due to the Chinese Spring Festival holiday.
> Thank you very much for the review.
> Please see my comments inline.
> 
> Thanks,
> Minghuan
> 

[...]

>>> +static int ls_scfg_msi_probe(struct platform_device *pdev) {
>>> +	struct ls_scfg_msi *msi_data;
>>> +	struct resource *res;
>>> +	int ret;
>>> +
>>> +	msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
>>> +	if (!msi_data)
>>> +		return -ENOMEM;
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	msi_data->regs = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(msi_data->regs)) {
>>> +		dev_err(&pdev->dev, "failed to initialize 'regs'\n");
>>> +		return PTR_ERR(msi_data->regs);
>>> +	}
>>> +	msi_data->msiir_addr = res->start;
>>> +
>>> +	msi_data->irq = platform_get_irq(pdev, 0);
>>> +	if (msi_data->irq <= 0) {
>>> +		dev_err(&pdev->dev, "failed to get MSI irq\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	msi_data->pdev = pdev;
>>> +	msi_data->nr_irqs = MSI_MAX_IRQS;
>>
>> So this is hardcoded, always. Why do you need a nr_irqs variable at all?
> [Lian Minghuan-B31939] Currently, nr_irqs is always 32, but in the
> future, the MSI controller may be extended to support more IRQs. And,
> we may set nr_irqs the value of less than 32 to reserve some IRQs for
> special usage. So nr_irqs can bring flexibility

You have to choose: either this is configurable and you describe it in
DT, or this is not and you drop this field from the structure.

As for the "reserved interrupts", that would need to be described too.

Thanks,

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