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

[Richard Fitzgerald]

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