On 04/07/18 10:09, Miquel Raynal wrote:
> Hi Marc,
>
> Marc Zyngier <marc.zyngier@xxxxxxx> wrote on Thu, 28 Jun 2018 13:45:09
> +0100:
>
>> On 22/06/18 16:14, Miquel Raynal wrote:
>>> The ICU can handle several type of interrupt, each of them being handled
>>> differently on AP side. On CP side, the ICU should be able to make the
>>> distinction between each interrupt group by pointing to the right parent.
>>>
>>> This is done through the introduction of new bindings, presenting the ICU
>>> node as the parent of multiple ICU sub-nodes, each of them being an
>>> interrupt type with a different interrupt parent. ICU interrupt 'clients'
>>> now directly point to the right sub-node, avoiding the need for the extra
>>> ICU_GRP_* parameter.
>>>
>>> ICU subnodes are probed automatically with devm_platform_populate(). If
>>> the node as no child, the probe function for NSRs will still be called
>>> 'manually' in order to preserve backward compatibility with DT using the
>>> old binding.
>>>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
>>> ---
>>> drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
>>> 1 file changed, 74 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
>>> index 24d45186eb6b..f7c2ede9c222 100644
>>> --- a/drivers/irqchip/irq-mvebu-icu.c
>>> +++ b/drivers/irqchip/irq-mvebu-icu.c
>>> @@ -43,6 +43,7 @@ struct mvebu_icu {
>>> struct regmap *regmap;
>>> struct device *dev;
>>> atomic_t initialized;
>>> + bool legacy_bindings;
>>> };
>>>
>>> struct mvebu_icu_irq_data {
>>> @@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
>>> unsigned int type;
>>> };
>>>
>>> +static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
>>> +{
>>> + struct mvebu_icu *icu;
>>> +
>>> + /*
>>> + * Device data being populated means we should be using legacy bindings.
>>> + * Using the _parent_ device data means we should be using new bindings.
>>> + */
>>> + icu = dev_get_drvdata(&pdev->dev);
>>> + if (icu) {
>>> + if (!icu->legacy_bindings)
>>> + return ERR_PTR(-EINVAL);
>>> + } else {
>>> + icu = dev_get_drvdata(pdev->dev.parent);
>>> + if (!icu)
>>> + return ERR_PTR(-ENODEV);
>>> +
>>> + if (icu->legacy_bindings)
>>> + return ERR_PTR(-EINVAL);
>>> + }
>>
>> Doesn't this make legacy_bindings completely redundant? Either the
>> pointer is !NULL in the device, and this is using a legacy binging, or
>> it is stored in the parent, and this is a new binding. You could even
>> have a helper for that:
>>
>> static bool is_legacy(struct platform_device *pdev)
>> {
>> return !dev_get_drvdata(&pdev->dev);
>> }
>>
>> The driver really doesn't need to be defending against itself, if
>> anything, and it would save you quite a bit of error handling in the
>> callers of this function.
>
> I simplified the sanity checks but I had to keep an icu->is_legacy
> boolean because the above function would not have worked, for instance,
> in the *_translate() hook. As this hook does not receive a struct
> device * (or platform_device) as parameter, I tried to use icu->dev
> instead. This cannot work as it always points to the device having the
> driver data attached.
You could still have the pdev as part of the domain host_data, right?
Isn't that just a matter of having a pointer to the pdev as part of the
icu data structure?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
