Re: [PATCH v4 1/3] gpio: xgene: Enable X-Gene standby GPIO as interrupt controller

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

Quan,

On 27/01/16 12:48, Quan Nguyen wrote:
> On Wed, Jan 27, 2016 at 12:39 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 26/01/16 16:27, Quan Nguyen wrote:
>>> On Tue, Jan 26, 2016 at 5:34 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>
>>>> On 26/01/16 07:22, Quan Nguyen wrote:
>>>>> Enable X-Gene standby GPIO controller as interrupt controller to provide
>>>>> its own resources. This avoids ambiguity where GIC interrupt resource is
>>>>> use as X-Gene standby GPIO interrupt resource in user driver.
>>>>>
>>>>> Signed-off-by: Y Vo <yvo@apm.com>
>>>>> Signed-off-by: Quan Nguyen <qnguyen@apm.com>
>>>>> ---
>>>>>  drivers/gpio/gpio-xgene-sb.c | 331 ++++++++++++++++++++++++++++++++++++-------
>>>>>  1 file changed, 276 insertions(+), 55 deletions(-)
>>
>> [...]
>>>>> @@ -90,6 +301,32 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
>>>>>       if (IS_ERR(regs))
>>>>>               return PTR_ERR(regs);
>>>>>
>>>>> +     priv->regs = regs;
>>>>> +
>>>>> +     of_id = of_match_device(xgene_gpio_sb_of_match, &pdev->dev);
>>>>> +     if (of_id)
>>>>> +             priv->flags = (uintptr_t)of_id->data;
>>>>
>>>> Wait. Everything is hardcoded? So why do we have to deal with looking
>>>> into that structure if nothing is actually parametrized?
>>>
>>> There will be other instances with difference number of irq pins /gpio
>>> /start_irq_base etc.
>>
>> Then it has to be described in DT right now.
>>
> 
> What I was thinking is to have other id to match with difference
> instances and these code can be use for ACPI also. Let say
> "apm,xgene2-gpio-sb"
> Please help correcting me if it is not right.

I still think this is the wrong thing to do. You are hiding magic values
in the driver, for no good reason. If ACPI has such a broken model that
it cannot give you the various parameters you need, then this is an ACPI
problem you can solve in the ACPI-specific code. Alternatively, you
could also fix your ACPI tables to be less braindead.

But please do not turn the DT code into the same mess for the sake of
using the lowest common denominator. Have a set of properties describing
the HW, a compatibility string to nicely identify the revision, and use
that. You can still hardcode things for ACPI if you desperately need it.

Thanks,

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