| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200319161149.GB5477@dell>
Date: Thu, 19 Mar 2020 16:11:49 +0000
From: Lee Jones <lee.jones@...aro.org>
To: Andreas Kemnade <andreas@...nade.info>
Cc: robh+dt@...nel.org, mark.rutland@....com, a.zummo@...ertech.it,
alexandre.belloni@...tlin.com, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-rtc@...r.kernel.org,
stefan@...er.ch, b.galvani@...il.com, phh@....me,
letux-kernel@...nphoenux.org, knaack.h@....de, lars@...afoo.de,
pmeerw@...erw.net, linux-iio@...r.kernel.org, jic23@...nel.org
Subject: Re: [PATCH RESEND v6 2/7] mfd: rn5t618: add IRQ support
On Fri, 13 Mar 2020, Andreas Kemnade wrote:
> This adds support for IRQ handling in the RC5T619 which is required
> for properly implementing subdevices like RTC.
> For now only definitions for the variant RC5T619 are included.
>
> Signed-off-by: Andreas Kemnade <andreas@...nade.info>
> ---
> Changes in v5:
> rn5t618_irq_init static
>
> Changes in v4:
> merge rn5t618-irq.c into rn5t618.c
> use macros for IRQ table
>
> Changes in v3:
> alignment cleanup
>
> Changes in v2:
> - no dead code, did some more testing and thinking for that
> - remove extra empty lines
> drivers/mfd/Kconfig | 1 +
> drivers/mfd/rn5t618.c | 88 +++++++++++++++++++++++++++++++++++++
> include/linux/mfd/rn5t618.h | 15 +++++++
> 3 files changed, 104 insertions(+)
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 2b203290e7b9..a7067888a41e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1058,6 +1058,7 @@ config MFD_RN5T618
> depends on OF
> select MFD_CORE
> select REGMAP_I2C
> + select REGMAP_IRQ
> help
> Say yes here to add support for the Ricoh RN5T567,
> RN5T618, RC5T619 PMIC.
> diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
> index ead2e79036a9..849a33d64860 100644
> --- a/drivers/mfd/rn5t618.c
> +++ b/drivers/mfd/rn5t618.c
> @@ -8,6 +8,8 @@
>
> #include <linux/delay.h>
> #include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> #include <linux/mfd/core.h>
> #include <linux/mfd/rn5t618.h>
> #include <linux/module.h>
> @@ -46,9 +48,63 @@ static const struct regmap_config rn5t618_regmap_config = {
> .cache_type = REGCACHE_RBTREE,
> };
>
> +static const struct regmap_irq rc5t619_irqs[] = {
> + REGMAP_IRQ_REG(RN5T618_IRQ_SYS, 0, BIT(0)),
> + REGMAP_IRQ_REG(RN5T618_IRQ_DCDC, 0, BIT(1)),
> + REGMAP_IRQ_REG(RN5T618_IRQ_RTC, 0, BIT(2)),
> + REGMAP_IRQ_REG(RN5T618_IRQ_ADC, 0, BIT(3)),
> + REGMAP_IRQ_REG(RN5T618_IRQ_GPIO, 0, BIT(4)),
> + REGMAP_IRQ_REG(RN5T618_IRQ_CHG, 0, BIT(6)),
> +};
> +
> +static const struct regmap_irq_chip rc5t619_irq_chip = {
> + .name = "rc5t619",
> + .irqs = rc5t619_irqs,
> + .num_irqs = ARRAY_SIZE(rc5t619_irqs),
> + .num_regs = 1,
> + .status_base = RN5T618_INTMON,
> + .mask_base = RN5T618_INTEN,
> + .mask_invert = true,
> +};
> +
> static struct rn5t618 *rn5t618_pm_power_off;
> static struct notifier_block rn5t618_restart_handler;
>
> +static int rn5t618_irq_init(struct rn5t618 *rn5t618)
> +{
> + const struct regmap_irq_chip *irq_chip;
> + int ret;
> +
> + if (!rn5t618->irq)
> + return 0;
> +
> + switch (rn5t618->variant) {
> + case RC5T619:
> + irq_chip = &rc5t619_irq_chip;
> + break;
> + default:
> + irq_chip = NULL;
If you initialised this at the top, you can rid this line.
Don't you want to put a message in here about an unsupported device?
> + break;
> + }
> +
> + if (!irq_chip) {
> + dev_err(rn5t618->dev, "Currently no IRQ support for variant %d\n",
> + (int)rn5t618->variant);
> + return -ENOENT;
> + }
Ah yes. Put this in 'default' instead.
> + ret = devm_regmap_add_irq_chip(rn5t618->dev, rn5t618->regmap,
> + rn5t618->irq,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + 0, irq_chip, &rn5t618->irq_data);
> + if (ret) {
> + dev_err(rn5t618->dev, "Failed to register IRQ chip\n");
> + return ret;
Make this a one line if.
> + }
> +
> + return 0;
... and return ret.
> +}
> +
> static void rn5t618_trigger_poweroff_sequence(bool repower)
> {
> /* disable automatic repower-on */
> @@ -106,6 +162,8 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
>
> i2c_set_clientdata(i2c, priv);
> priv->variant = (long)of_id->data;
> + priv->irq = i2c->irq;
> + priv->dev = &i2c->dev;
>
> priv->regmap = devm_regmap_init_i2c(i2c, &rn5t618_regmap_config);
> if (IS_ERR(priv->regmap)) {
> @@ -138,6 +196,11 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
> return ret;
> }
>
> + if (priv->irq > 0) {
There are already have checks in rn5t618_irq_init().
Just call it regardless.
> + if (rn5t618_irq_init(priv))
If this returns an error, you should return that error from .probe().
> + priv->irq = 0;
> + }
> +
> return 0;
> }
>
> @@ -155,15 +218,40 @@ static int rn5t618_i2c_remove(struct i2c_client *i2c)
> return 0;
> }
>
> +static int __maybe_unused rn5t618_i2c_suspend(struct device *dev)
> +{
> + struct rn5t618 *priv = dev_get_drvdata(dev);
> +
> + if (priv->irq)
> + disable_irq(priv->irq);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused rn5t618_i2c_resume(struct device *dev)
> +{
> + struct rn5t618 *priv = dev_get_drvdata(dev);
> +
> + if (priv->irq)
> + enable_irq(priv->irq);
> +
> + return 0;
> +}
> +
> static const struct i2c_device_id rn5t618_i2c_id[] = {
> { }
> };
> MODULE_DEVICE_TABLE(i2c, rn5t618_i2c_id);
>
> +static SIMPLE_DEV_PM_OPS(rn5t618_i2c_dev_pm_ops,
> + rn5t618_i2c_suspend,
> + rn5t618_i2c_resume);
> +
> static struct i2c_driver rn5t618_i2c_driver = {
> .driver = {
> .name = "rn5t618",
> .of_match_table = of_match_ptr(rn5t618_of_match),
> + .pm = &rn5t618_i2c_dev_pm_ops,
> },
> .probe = rn5t618_i2c_probe,
> .remove = rn5t618_i2c_remove,
> diff --git a/include/linux/mfd/rn5t618.h b/include/linux/mfd/rn5t618.h
> index d62ef48060b5..739571656f2b 100644
> --- a/include/linux/mfd/rn5t618.h
> +++ b/include/linux/mfd/rn5t618.h
> @@ -242,9 +242,24 @@ enum {
> RC5T619,
> };
>
> +/* RN5T618 IRQ definitions */
> +enum {
> + RN5T618_IRQ_SYS = 0,
> + RN5T618_IRQ_DCDC,
> + RN5T618_IRQ_RTC,
> + RN5T618_IRQ_ADC,
> + RN5T618_IRQ_GPIO,
> + RN5T618_IRQ_CHG,
> + RN5T618_NR_IRQS,
> +};
> +
> struct rn5t618 {
> struct regmap *regmap;
> + struct device *dev;
> long variant;
> +
> + int irq;
> + struct regmap_irq_chip_data *irq_data;
> };
>
> #endif /* __LINUX_MFD_RN5T618_H */
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists