Messages in this thread | | | Subject | Re: [PATCH v7 3/4] ASoC: rt5677: clear interrupts by polarity flip | From | Cezary Rojewski <> | Date | Sun, 16 Jun 2019 18:56:52 +0200 |
| |
On 2019-06-14 21:48, Fletcher Woodruff wrote: > From: Ben Zhang <benzh@chromium.org> > > The rt5677 jack detection function has a requirement that the polarity > of an interrupt be flipped after it fires in order to clear the > interrupt. > > This patch implements an irq_chip with irq_domain directly instead of > using regmap-irq, so that interrupt source line polarities can be > flipped in the irq handler. > > The reason that this patch does not add this feature within regmap-irq > is that future patches will add hotword detection support to this irq > handler. Those patches will require adding additional logic that would > not make sense to have in regmap-irq. > > Signed-off-by: Ben Zhang <benzh@chromium.org> > Signed-off-by: Fletcher Woodruff <fletcherw@chromium.org> > --- > sound/soc/codecs/rt5677.c | 170 ++++++++++++++++++++++++++++++-------- > sound/soc/codecs/rt5677.h | 7 +- > 2 files changed, 143 insertions(+), 34 deletions(-) > > diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c > index 87a92ba0d040b7..87466ee222ee59 100644 > --- a/sound/soc/codecs/rt5677.c > +++ b/sound/soc/codecs/rt5677.c > @@ -23,6 +23,10 @@ > #include <linux/firmware.h> > #include <linux/of_device.h> > #include <linux/property.h> > +#include <linux/irq.h> > +#include <linux/interrupt.h> > +#include <linux/irqdomain.h> > +#include <linux/workqueue.h> > #include <sound/core.h> > #include <sound/pcm.h> > #include <sound/pcm_params.h> > @@ -4620,7 +4624,6 @@ static void rt5677_gpio_config(struct rt5677_priv *rt5677, unsigned offset, > static int rt5677_to_irq(struct gpio_chip *chip, unsigned offset) > { > struct rt5677_priv *rt5677 = gpiochip_get_data(chip); > - struct regmap_irq_chip_data *data = rt5677->irq_data; > int irq; > > if ((rt5677->pdata.jd1_gpio == 1 && offset == RT5677_GPIO1) || > @@ -4646,7 +4649,7 @@ static int rt5677_to_irq(struct gpio_chip *chip, unsigned offset) > return -ENXIO; > } > > - return regmap_irq_get_virq(data, irq); > + return irq_create_mapping(rt5677->domain, irq); > } > > static const struct gpio_chip rt5677_template_chip = { > @@ -5042,30 +5045,130 @@ static void rt5677_read_device_properties(struct rt5677_priv *rt5677, > rt5677->pdata.jd3_gpio = val; > } > > -static struct regmap_irq rt5677_irqs[] = { > +struct rt5677_irq_desc { > + unsigned int enable_mask; > + unsigned int status_mask; > + unsigned int polarity_mask; > +}; > + > +static const struct rt5677_irq_desc rt5677_irq_descs[] = { > [RT5677_IRQ_JD1] = { > - .reg_offset = 0, > - .mask = RT5677_EN_IRQ_GPIO_JD1, > + .enable_mask = RT5677_EN_IRQ_GPIO_JD1, > + .status_mask = RT5677_STA_GPIO_JD1, > + .polarity_mask = RT5677_INV_GPIO_JD1, > }, > [RT5677_IRQ_JD2] = { > - .reg_offset = 0, > - .mask = RT5677_EN_IRQ_GPIO_JD2, > + .enable_mask = RT5677_EN_IRQ_GPIO_JD2, > + .status_mask = RT5677_STA_GPIO_JD2, > + .polarity_mask = RT5677_INV_GPIO_JD2, > }, > [RT5677_IRQ_JD3] = { > - .reg_offset = 0, > - .mask = RT5677_EN_IRQ_GPIO_JD3, > + .enable_mask = RT5677_EN_IRQ_GPIO_JD3, > + .status_mask = RT5677_STA_GPIO_JD3, > + .polarity_mask = RT5677_INV_GPIO_JD3, > }, > }; > > -static struct regmap_irq_chip rt5677_irq_chip = { > - .name = RT5677_DRV_NAME, > - .irqs = rt5677_irqs, > - .num_irqs = ARRAY_SIZE(rt5677_irqs), > +static irqreturn_t rt5677_irq(int unused, void *data) > +{ > + struct rt5677_priv *rt5677 = data; > + int ret = 0, i, reg_irq, virq; > + bool irq_fired = false; > + > + mutex_lock(&rt5677->irq_lock); > + /* Read interrupt status */ > + ret = regmap_read(rt5677->regmap, RT5677_IRQ_CTRL1, ®_irq); > + if (ret) { > + pr_err("rt5677: failed reading IRQ status: %d\n", ret);
The entire rt5677 makes use of dev_XXX with the exception of.. this very function. Consider reusing "component" field which is already part of struct rt5677_priv and removing pr_XXX.
> + goto exit; > + } > + > + for (i = 0; i < RT5677_IRQ_NUM; i++) { > + if (reg_irq & rt5677_irq_descs[i].status_mask) { > + irq_fired = true; > + virq = irq_find_mapping(rt5677->domain, i); > + if (virq) > + handle_nested_irq(virq); > + > + /* Clear the interrupt by flipping the polarity of the > + * interrupt source line that fired > + */ > + reg_irq ^= rt5677_irq_descs[i].polarity_mask; > + } > + } > + > + if (!irq_fired) > + goto exit; > + > + ret = regmap_write(rt5677->regmap, RT5677_IRQ_CTRL1, reg_irq); > + if (ret) { > + pr_err("rt5677: failed updating IRQ status: %d\n", ret);
Same here.
> + goto exit; > + } > +exit: > + mutex_unlock(&rt5677->irq_lock); > + if (irq_fired) > + return IRQ_HANDLED; > + else > + return IRQ_NONE; > +} > > - .num_regs = 1, > - .status_base = RT5677_IRQ_CTRL1, > - .mask_base = RT5677_IRQ_CTRL1, > - .mask_invert = 1, > +static void rt5677_irq_bus_lock(struct irq_data *data) > +{ > + struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data); > + > + mutex_lock(&rt5677->irq_lock); > +} > + > +static void rt5677_irq_bus_sync_unlock(struct irq_data *data) > +{ > + struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data); > + > + // Set the enable/disable bits for the jack detect IRQs. > + regmap_update_bits(rt5677->regmap, RT5677_IRQ_CTRL1, > + RT5677_EN_IRQ_GPIO_JD1 | RT5677_EN_IRQ_GPIO_JD2 | > + RT5677_EN_IRQ_GPIO_JD3, rt5677->irq_en); > + mutex_unlock(&rt5677->irq_lock); > +} > + > +static void rt5677_irq_enable(struct irq_data *data) > +{ > + struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data); > + > + rt5677->irq_en |= rt5677_irq_descs[data->hwirq].enable_mask; > +} > + > +static void rt5677_irq_disable(struct irq_data *data) > +{ > + struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data); > + > + rt5677->irq_en &= ~rt5677_irq_descs[data->hwirq].enable_mask; > +} > + > +static struct irq_chip rt5677_irq_chip = { > + .name = "rt5677_irq_chip", > + .irq_bus_lock = rt5677_irq_bus_lock, > + .irq_bus_sync_unlock = rt5677_irq_bus_sync_unlock, > + .irq_disable = rt5677_irq_disable, > + .irq_enable = rt5677_irq_enable, > +}; > + > +static int rt5677_irq_map(struct irq_domain *h, unsigned int virq, > + irq_hw_number_t hw) > +{ > + struct rt5677_priv *rt5677 = h->host_data; > + > + irq_set_chip_data(virq, rt5677); > + irq_set_chip(virq, &rt5677_irq_chip); > + irq_set_nested_thread(virq, 1); > + irq_set_noprobe(virq); > + return 0; > +} > + > + > +static const struct irq_domain_ops rt5677_domain_ops = { > + .map = rt5677_irq_map, > + .xlate = irq_domain_xlate_twocell, > }; > > static int rt5677_init_irq(struct i2c_client *i2c) > @@ -5084,6 +5187,8 @@ static int rt5677_init_irq(struct i2c_client *i2c) > return -EINVAL; > } > > + mutex_init(&rt5677->irq_lock); > + > /* > * Select RC as the debounce clock so that GPIO works even when > * MCLK is gated which happens when there is no audio stream > @@ -5092,7 +5197,6 @@ static int rt5677_init_irq(struct i2c_client *i2c) > regmap_update_bits(rt5677->regmap, RT5677_DIG_MISC, > RT5677_IRQ_DEBOUNCE_SEL_MASK, > RT5677_IRQ_DEBOUNCE_SEL_RC); > - > /* Enable auto power on RC when GPIO states are changed */ > regmap_update_bits(rt5677->regmap, RT5677_GEN_CTRL1, 0xff, 0xff); > > @@ -5115,26 +5219,25 @@ static int rt5677_init_irq(struct i2c_client *i2c) > regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL1, > RT5677_GPIO1_PIN_MASK, RT5677_GPIO1_PIN_IRQ); > > - ret = regmap_add_irq_chip(rt5677->regmap, i2c->irq, > - IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, 0, > - &rt5677_irq_chip, &rt5677->irq_data); > + /* Ready to listen for interrupts */ > + rt5677->domain = irq_domain_add_linear(i2c->dev.of_node, > + RT5677_IRQ_NUM, &rt5677_domain_ops, rt5677); > + if (!rt5677->domain) { > + dev_err(&i2c->dev, "Failed to create IRQ domain\n"); > + return -ENOMEM; > + } > > - if (ret != 0) { > - dev_err(&i2c->dev, "Failed to register IRQ chip: %d\n", ret); > + ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL, rt5677_irq, > + IRQF_TRIGGER_RISING | IRQF_ONESHOT, > + "rt5677", rt5677); > + if (ret) { > + dev_err(&i2c->dev, "Failed to request IRQ: %d\n", ret); > return ret;
You may want to simplify this, similarly to how's it done in your rt5677_i2c_probe - leave message alone and return "ret" explicitly at the end.
> } > > return 0; > } > > -static void rt5677_free_irq(struct i2c_client *i2c) > -{ > - struct rt5677_priv *rt5677 = i2c_get_clientdata(i2c); > - > - if (rt5677->irq_data) > - regmap_del_irq_chip(i2c->irq, rt5677->irq_data); > -} > - > static int rt5677_i2c_probe(struct i2c_client *i2c) > { > struct rt5677_priv *rt5677; > @@ -5259,7 +5362,9 @@ static int rt5677_i2c_probe(struct i2c_client *i2c) > RT5677_MICBIAS1_CTRL_VDD_3_3V); > > rt5677_init_gpio(i2c); > - rt5677_init_irq(i2c); > + ret = rt5677_init_irq(i2c); > + if (ret) > + dev_err(&i2c->dev, "Failed to initialize irq: %d\n", ret); > > return devm_snd_soc_register_component(&i2c->dev, > &soc_component_dev_rt5677, > @@ -5268,7 +5373,6 @@ static int rt5677_i2c_probe(struct i2c_client *i2c) > > static int rt5677_i2c_remove(struct i2c_client *i2c) > { > - rt5677_free_irq(i2c); > rt5677_free_gpio(i2c); > > return 0; > diff --git a/sound/soc/codecs/rt5677.h b/sound/soc/codecs/rt5677.h > index c26edd387e340b..d0ac26e562eb5f 100644 > --- a/sound/soc/codecs/rt5677.h > +++ b/sound/soc/codecs/rt5677.h > @@ -1749,6 +1749,7 @@ enum { > RT5677_IRQ_JD1, > RT5677_IRQ_JD2, > RT5677_IRQ_JD3, > + RT5677_IRQ_NUM, > }; > > enum rt5677_type { > @@ -1847,9 +1848,13 @@ struct rt5677_priv { > struct gpio_chip gpio_chip; > #endif > bool dsp_vad_en; > - struct regmap_irq_chip_data *irq_data; > bool is_dsp_mode; > bool is_vref_slow; > + > + /* Interrupt handling */ > + struct irq_domain *domain; > + struct mutex irq_lock; > + unsigned int irq_en; > }; > > int rt5677_sel_asrc_clk_src(struct snd_soc_component *component, >
| |