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

[Thomas Gleixner <tglx@linutronix.de>]

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. 

> +#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