Re: [PATCH v10 2/2] irqchip: Add driver for Cirrus Logic Madera codecs

[Richard Fitzgerald <rf@opensource.cirrus.com>]

On 30/08/18 11:31, Thomas Gleixner wrote:
> On Tue, 28 Aug 2018, Richard Fitzgerald wrote:
>> @@ -0,0 +1,244 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Interrupt support for Cirrus Logic Madera codecs
>> + *
>> + * Copyright (C) 2015-2018 Cirrus Logic
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; version 2.
> 
> You have the SPDX identifier above, which makes this boilerplate
> superfluous.
> 

Our legal people want it left here.
They are ok with the SPDX reference, but they want to keep an
explicit statement of the license that was intended.

>> +#ifdef CONFIG_PM_SLEEP
>> +static int madera_suspend_noirq(struct device *dev)
>> +{
>> +	struct madera *madera = dev_get_drvdata(dev->parent);
>> +
>> +	dev_dbg(madera->irq_dev, "No IRQ suspend, reenabling IRQ\n");
>> +
>> +	enable_irq(madera->irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static int madera_suspend(struct device *dev)
>> +{
>> +	struct madera *madera = dev_get_drvdata(dev->parent);
>> +
>> +	dev_dbg(madera->irq_dev, "Suspend, disabling IRQ\n");
>> +
>> +	disable_irq(madera->irq);
>> +
>> +	return 0;
>> +}
> 
> This really wants a comment why you are disabling it first and reenabling
> it afterwards. I have no idea why you are doing this and it doesn't make
> much sense from the S/R perspective either.
> 
>> +	/*
>> +	 * Read the flags from the interrupt controller if not specified
>> +	 * by pdata
>> +	 */
>> +	irq_flags = madera->pdata.irq_flags;
>> +	if (!irq_flags) {
>> +		irq_data = irq_get_irq_data(madera->irq);
>> +		if (!irq_data) {
>> +			dev_err(&pdev->dev, "Invalid IRQ: %d\n", madera->irq);
>> +			return -EINVAL;
>> +		}
>> +
>> +		irq_flags = irqd_get_trigger_type(irq_data);
>> +
>> +		/* Codec defaults to trigger low, use this if no flags given */
>> +		if (irq_flags == IRQ_TYPE_NONE)
>> +			irq_flags = IRQF_TRIGGER_LOW;
>> +	}
>> +
>> +	if (irq_flags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
>> +		dev_err(&pdev->dev, "Host interrupt not level-triggered\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (irq_flags & IRQF_TRIGGER_HIGH) {
>> +		ret = regmap_update_bits(madera->regmap, MADERA_IRQ1_CTRL,
>> +					 MADERA_IRQ_POL_MASK, 0);
>> +		if (ret) {
>> +			dev_err(&pdev->dev,
>> +				"Failed to set IRQ polarity: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
> 
> This looks wrong. Why are you only updating the polarity for trigger HIGH?
> 
>> diff --git a/include/linux/irqchip/irq-madera.h b/include/linux/irqchip/irq-madera.h
>> new file mode 100644
>> index 000000000000..5aeb7e6adc82
>> --- /dev/null
>> +++ b/include/linux/irqchip/irq-madera.h
>> @@ -0,0 +1,135 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Interrupt support for Cirrus Logic Madera codecs
>> + *
>> + * Copyright 2016-2018 Cirrus Logic
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; version 2.
> 
> See above.
> 
> Thanks,
> 
> 	tglx
>