Re: [RFC PATCH 3/5] gpio-dmec: gpio support for dmec

[Linus Walleij]

On Mon, Oct 31, 2016 at 9:50 AM, Zahari Doychev
<zahari.doychev@xxxxxxxxx> wrote:
> On Sat, Oct 29, 2016 at 11:05:29AM +0200, Linus Walleij wrote:

>> > +static int dmec_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>> > +{
>> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> > +       struct dmec_gpio_priv *priv = container_of(gc, struct dmec_gpio_priv,
>> > +                                                  gpio_chip);
>> > +       struct regmap *regmap = priv->regmap;
>> > +       unsigned int offset, mask, val;
>> > +
>> > +       offset = DMEC_GPIO_IRQTYPE_OFFSET(priv) + (d->hwirq >> 2);
>> > +       mask = ((d->hwirq & 3) << 1);
>> > +
>> > +       regmap_read(regmap, offset, &val);
>> > +
>> > +       val &= ~(3 << mask);
>> > +       switch (type & IRQ_TYPE_SENSE_MASK) {
>> > +       case IRQ_TYPE_LEVEL_LOW:
>> > +               break;
>> > +       case IRQ_TYPE_EDGE_RISING:
>> > +               val |= (1 << mask);
>> > +               break;
>> > +       case IRQ_TYPE_EDGE_FALLING:
>> > +               val |= (2 << mask);
>> > +               break;
>> > +       case IRQ_TYPE_EDGE_BOTH:
>> > +               val |= (3 << mask);
>> > +               break;
>> > +       default:
>> > +               return -EINVAL;
>> > +       }
>> > +
>> > +       regmap_write(regmap, offset, val);
>> > +
>> > +       return 0;
>> > +}
>>
>> This chip uses handle_simple_irq() which is fine if the chip really has no
>> edge detector ACK register.
>>
>> What some chips have is a special register to clearing (ACK:ing) the edge
>> IRQ which makes it possible for a new IRQ to be handled as soon as
>> that has happened, and those need to use handle_edge_irq() for edge IRQs
>> and handle_level_irq() for level IRQs, with the side effect that the edge
>> IRQ path will additionally call the .irq_ack() callback on the irqchip
>> when handle_edge_irq() is used. In this case we set handle_bad_irq()
>> as default handler and set up the right handler i .set_type().
>>
>> Look at drivers/gpio/gpio-pl061.c for an example.
>>
>> If you DON'T have a special edge ACK register, keep it like this.
>
> What is the difference between this special edge ACK register and the normal
> interrupt ACK register?

With level interrupts there is seldom use of any ACK register.
They will by definition just hold the line low until the clients stop
asserting their IRQs.

With edge triggered interrupts, you can have a transient event so
that you need an ACK register to tell the hardware that you have
seen and acknowledged this IRQ, so it can go ahead and fire a
second IRQ on the same line while you are still processing the
first one.

> I think I do not have such dedicated register
> but I will have to check this again.

Read the documentation for the register(s) and see what the
use case is.

>> > +static int dmec_gpio_probe(struct platform_device *pdev)
>> > +{
>> > +       struct device *dev = &pdev->dev;
>> > +       struct dmec_gpio_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> > +       struct dmec_gpio_priv *priv;
>> > +       struct gpio_chip *gpio_chip;
>> > +       struct irq_chip *irq_chip;
>> > +       int ret = 0;
>> > +
>> > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> > +       if (!priv)
>> > +               return -ENOMEM;
>> > +
>> > +       priv->regmap = dmec_get_regmap(pdev->dev.parent);
>>
>> Do you really need a special accessor function to get the regmap like this?
>> If you just use syscon you get these kind of helpers for free. I don't know
>> if you can use syscon though, just a suggestion.
>
> The I/O memory is mapped in the mfd driver. The addresing mode is also set
> there which should be the same for all child devices. So in this way I have
> dependcy between the mfd and the rest of the drivers. I am not sure that I
> can use syscon as the driver is getting its resources from acpi.

OK.

Yours,
Linus Walleij