| Message ID | 20191129212045.18325-4-andreas@kemnade.info |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add rtc support for rn5t618 mfd | expand |
On Fri, 29 Nov 2019, Andreas Kemnade wrote: > This adds support for irq handling in the rc5t619 which is required Please capitalise abbreviations and device names (as they do in the datasheet). > for properly implementing subdevices like rtc. "RTC" > For now only definitions for the variant rc5t619 are included. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > 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/Makefile | 2 +- > drivers/mfd/rn5t618-core.c | 34 ++++++++++++++- > drivers/mfd/rn5t618-irq.c | 85 +++++++++++++++++++++++++++++++++++++ > include/linux/mfd/rn5t618.h | 16 +++++++ > 5 files changed, 136 insertions(+), 2 deletions(-) > create mode 100644 drivers/mfd/rn5t618-irq.c > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index ae24d3ea68ea..522e068d0082 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1057,6 +1057,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/Makefile b/drivers/mfd/Makefile > index 110ea700231b..2906d5db67d0 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -217,7 +217,7 @@ obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o > obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o > obj-$(CONFIG_MFD_RK808) += rk808.o > > -rn5t618-objs := rn5t618-core.o > +rn5t618-objs := rn5t618-core.o rn5t618-irq.o > obj-$(CONFIG_MFD_RN5T618) += rn5t618.o > obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o > obj-$(CONFIG_MFD_SYSCON) += syscon.o > diff --git a/drivers/mfd/rn5t618-core.c b/drivers/mfd/rn5t618-core.c > index da5cd9c92a59..1e2326217681 100644 > --- a/drivers/mfd/rn5t618-core.c > +++ b/drivers/mfd/rn5t618-core.c > @@ -8,6 +8,7 @@ > > #include <linux/delay.h> > #include <linux/i2c.h> > +#include <linux/interrupt.h> > #include <linux/mfd/core.h> > #include <linux/mfd/rn5t618.h> > #include <linux/module.h> > @@ -105,7 +106,8 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, > > i2c_set_clientdata(i2c, priv); > priv->variant = (long)of_id->data; > - > + priv->chip_irq = i2c->irq; > + priv->dev = &i2c->dev; '\n' > priv->regmap = devm_regmap_init_i2c(i2c, &rn5t618_regmap_config); > if (IS_ERR(priv->regmap)) { > ret = PTR_ERR(priv->regmap); > @@ -137,6 +139,11 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, > return ret; > } > > + if (priv->chip_irq > 0) { > + if (rn5t618_irq_init(priv)) > + priv->chip_irq = 0; > + } > + > return 0; > } > > @@ -154,15 +161,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->chip_irq) > + disable_irq(priv->chip_irq); > + > + return 0; > +} > + > +static int __maybe_unused rn5t618_i2c_resume(struct device *dev) > +{ > + struct rn5t618 *priv = dev_get_drvdata(dev); > + > + if (priv->chip_irq) > + enable_irq(priv->chip_irq); > + > + return 0; > +} > + > static const struct i2c_device_id rn5t618_i2c_id[] = { > { } > }; > MODULE_DEVICE_TABLE(i2c, rn5t618_i2c_id); Not this patch I know, but it's strange to see this empty. > +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/drivers/mfd/rn5t618-irq.c b/drivers/mfd/rn5t618-irq.c Why does this need to be separate from the core file? > new file mode 100644 > index 000000000000..8a4c56429768 > --- /dev/null > +++ b/drivers/mfd/rn5t618-irq.c > @@ -0,0 +1,85 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2019 Andreas Kemnade > + */ > +#include <linux/device.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > + > +#include <linux/mfd/rn5t618.h> > + > +static const struct regmap_irq rc5t619_irqs[] = { > + [RN5T618_IRQ_SYS] = { > + .reg_offset = 0, > + .mask = (0 << 1) > + }, > + [RN5T618_IRQ_DCDC] = { > + .reg_offset = 0, > + .mask = (1 << 1) BIT() > + }, > + [RN5T618_IRQ_RTC] = { > + .reg_offset = 0, > + .mask = (1 << 2) > + }, > + [RN5T618_IRQ_ADC] = { > + .reg_offset = 0, > + .mask = (1 << 3) > + }, > + [RN5T618_IRQ_GPIO] = { > + .reg_offset = 0, > + .mask = (1 << 4) > + }, > + [RN5T618_IRQ_CHG] = { > + .reg_offset = 0, > + .mask = (1 << 6), > + } > +}; There are probably macros available to tidy this up. Take a look in include/linux/regmap.h > +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, > +}; > + > +int rn5t618_irq_init(struct rn5t618 *rn5t618) > +{ > + const struct regmap_irq_chip *irq_chip; > + int ret; > + > + if (!rn5t618->chip_irq) > + return 0; > + > + switch (rn5t618->variant) { > + case RC5T619: > + irq_chip = &rc5t619_irq_chip; > + break; > + > + /* TODO: check irq definitions for other variants */ No need for this. It's implied. OOI, when support for more variants be added? > + default: > + irq_chip = NULL; > + break; > + } > + > + if (!irq_chip) { > + dev_err(rn5t618->dev, "no IRQ definition known for variant\n"); How about '"Variant %d not currently supported", rn5t618->variant' > + return -ENOENT; > + } > + > + ret = devm_regmap_add_irq_chip(rn5t618->dev, rn5t618->regmap, > + rn5t618->chip_irq, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + 0, irq_chip, &rn5t618->irq_data); > + if (ret != 0) { if (ret) > + dev_err(rn5t618->dev, "Failed to register IRQ chip\n"); > + return ret; > + } > + > + return 0; > +} > diff --git a/include/linux/mfd/rn5t618.h b/include/linux/mfd/rn5t618.h > index d62ef48060b5..edd2b6485e3b 100644 > --- a/include/linux/mfd/rn5t618.h > +++ b/include/linux/mfd/rn5t618.h > @@ -242,9 +242,25 @@ 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 chip_irq; Are there any other kinds of IRQ? If you don't have to differentiate between multiple, just 'irq' will do. This could also get confused with 'irq_chip'. > + struct regmap_irq_chip_data *irq_data; > }; > > +extern int rn5t618_irq_init(struct rn5t618 *rn5t618); > #endif /* __LINUX_MFD_RN5T618_H */
On Tue, 10 Dec 2019 09:32:25 +0000 Lee Jones <lee.jones@linaro.org> wrote: > On Fri, 29 Nov 2019, Andreas Kemnade wrote: > > > This adds support for irq handling in the rc5t619 which is required > > Please capitalise abbreviations and device names (as they do in the > datasheet). > for IRQ vs. irq: I see both things in commit messages. Is there any rule about that? > > for properly implementing subdevices like rtc. > > "RTC" > > > For now only definitions for the variant rc5t619 are included. > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > --- > > 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/Makefile | 2 +- > > drivers/mfd/rn5t618-core.c | 34 ++++++++++++++- > > drivers/mfd/rn5t618-irq.c | 85 +++++++++++++++++++++++++++++++++++++ > > include/linux/mfd/rn5t618.h | 16 +++++++ > > 5 files changed, 136 insertions(+), 2 deletions(-) > > create mode 100644 drivers/mfd/rn5t618-irq.c > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index ae24d3ea68ea..522e068d0082 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -1057,6 +1057,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/Makefile b/drivers/mfd/Makefile > > index 110ea700231b..2906d5db67d0 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -217,7 +217,7 @@ obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o > > obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o > > obj-$(CONFIG_MFD_RK808) += rk808.o > > > > -rn5t618-objs := rn5t618-core.o > > +rn5t618-objs := rn5t618-core.o rn5t618-irq.o > > obj-$(CONFIG_MFD_RN5T618) += rn5t618.o > > obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o > > obj-$(CONFIG_MFD_SYSCON) += syscon.o > > diff --git a/drivers/mfd/rn5t618-core.c b/drivers/mfd/rn5t618-core.c > > index da5cd9c92a59..1e2326217681 100644 > > --- a/drivers/mfd/rn5t618-core.c > > +++ b/drivers/mfd/rn5t618-core.c > > @@ -8,6 +8,7 @@ > > > > #include <linux/delay.h> > > #include <linux/i2c.h> > > +#include <linux/interrupt.h> > > #include <linux/mfd/core.h> > > #include <linux/mfd/rn5t618.h> > > #include <linux/module.h> > > @@ -105,7 +106,8 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, > > > > i2c_set_clientdata(i2c, priv); > > priv->variant = (long)of_id->data; > > - > > + priv->chip_irq = i2c->irq; > > + priv->dev = &i2c->dev; > > '\n' > > > priv->regmap = devm_regmap_init_i2c(i2c, &rn5t618_regmap_config); > > if (IS_ERR(priv->regmap)) { > > ret = PTR_ERR(priv->regmap); > > @@ -137,6 +139,11 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, > > return ret; > > } > > > > + if (priv->chip_irq > 0) { > > + if (rn5t618_irq_init(priv)) > > + priv->chip_irq = 0; > > + } > > + > > return 0; > > } > > > > @@ -154,15 +161,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->chip_irq) > > + disable_irq(priv->chip_irq); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused rn5t618_i2c_resume(struct device *dev) > > +{ > > + struct rn5t618 *priv = dev_get_drvdata(dev); > > + > > + if (priv->chip_irq) > > + enable_irq(priv->chip_irq); > > + > > + return 0; > > +} > > + > > static const struct i2c_device_id rn5t618_i2c_id[] = { > > { } > > }; > > MODULE_DEVICE_TABLE(i2c, rn5t618_i2c_id); > > Not this patch I know, but it's strange to see this empty. > Yes, should be cleaned up. For now the device tree stuff seems to kick in. > > +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/drivers/mfd/rn5t618-irq.c b/drivers/mfd/rn5t618-irq.c > > Why does this need to be separate from the core file? > It does not need. It is not that complex. There will just be another set of tables for the rn5t618 > > new file mode 100644 > > index 000000000000..8a4c56429768 > > --- /dev/null > > +++ b/drivers/mfd/rn5t618-irq.c > > @@ -0,0 +1,85 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright 2019 Andreas Kemnade > > + */ > > +#include <linux/device.h> > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > +#include <linux/module.h> > > +#include <linux/regmap.h> > > + > > +#include <linux/mfd/rn5t618.h> > > + > > +static const struct regmap_irq rc5t619_irqs[] = { > > + [RN5T618_IRQ_SYS] = { > > + .reg_offset = 0, > > + .mask = (0 << 1) > > + }, > > + [RN5T618_IRQ_DCDC] = { > > + .reg_offset = 0, > > + .mask = (1 << 1) > > BIT() > yes, makes things more readable. > > + }, > > + [RN5T618_IRQ_RTC] = { > > + .reg_offset = 0, > > + .mask = (1 << 2) > > + }, > > + [RN5T618_IRQ_ADC] = { > > + .reg_offset = 0, > > + .mask = (1 << 3) > > + }, > > + [RN5T618_IRQ_GPIO] = { > > + .reg_offset = 0, > > + .mask = (1 << 4) > > + }, > > + [RN5T618_IRQ_CHG] = { > > + .reg_offset = 0, > > + .mask = (1 << 6), > > + } > > +}; > > There are probably macros available to tidy this up. > > Take a look in include/linux/regmap.h > I will have a look. > > +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, > > +}; > > + > > +int rn5t618_irq_init(struct rn5t618 *rn5t618) > > +{ > > + const struct regmap_irq_chip *irq_chip; > > + int ret; > > + > > + if (!rn5t618->chip_irq) > > + return 0; > > + > > + switch (rn5t618->variant) { > > + case RC5T619: > > + irq_chip = &rc5t619_irq_chip; > > + break; > > + > > + /* TODO: check irq definitions for other variants */ > > No need for this. It's implied. > > OOI, when support for more variants be added? > I have done research about the RN5T618. It has just the RTC IRQ missing, I could just add the table for it to prepare the path for others. I cannot test it but since there are no users yet, it does not harm that it is not well-tested. No idea about the RN5T567. > > + default: > > + irq_chip = NULL; > > + break; > > + } > > + > > + if (!irq_chip) { > > + dev_err(rn5t618->dev, "no IRQ definition known for variant\n"); > > How about '"Variant %d not currently supported", rn5t618->variant' > makes sense. > > + return -ENOENT; > > + } > > + > > + ret = devm_regmap_add_irq_chip(rn5t618->dev, rn5t618->regmap, > > + rn5t618->chip_irq, > > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > + 0, irq_chip, &rn5t618->irq_data); > > + if (ret != 0) { > > if (ret) > > > + dev_err(rn5t618->dev, "Failed to register IRQ chip\n"); > > + return ret; > > + } > > + > > + return 0; > > +} > > diff --git a/include/linux/mfd/rn5t618.h b/include/linux/mfd/rn5t618.h > > index d62ef48060b5..edd2b6485e3b 100644 > > --- a/include/linux/mfd/rn5t618.h > > +++ b/include/linux/mfd/rn5t618.h > > @@ -242,9 +242,25 @@ 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 chip_irq; > > Are there any other kinds of IRQ? > there is some separate battery low input for the charger which could be modeled as an IRQ. But that could be handled entirely there when I am at it and in the corresponding subdevice. Regards, Andreas
On Tue, 10 Dec 2019, Andreas Kemnade wrote: > On Tue, 10 Dec 2019 09:32:25 +0000 > Lee Jones <lee.jones@linaro.org> wrote: > > > On Fri, 29 Nov 2019, Andreas Kemnade wrote: > > > > > This adds support for irq handling in the rc5t619 which is required > > > > Please capitalise abbreviations and device names (as they do in the > > datasheet). > > > for IRQ vs. irq: I see both things in commit messages. Is there any rule about > that? No, it's preference. Mine is to follow the conventions set out by standard English grammar. Capital letters to start sentences, proper nouns and abbreviations, etc. > > > for properly implementing subdevices like rtc. > > > > "RTC" > > > > > For now only definitions for the variant rc5t619 are included. > > > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > > --- > > > 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/Makefile | 2 +- > > > drivers/mfd/rn5t618-core.c | 34 ++++++++++++++- > > > drivers/mfd/rn5t618-irq.c | 85 +++++++++++++++++++++++++++++++++++++ > > > include/linux/mfd/rn5t618.h | 16 +++++++ > > > 5 files changed, 136 insertions(+), 2 deletions(-) > > > create mode 100644 drivers/mfd/rn5t618-irq.c > > > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > > index ae24d3ea68ea..522e068d0082 100644 > > > --- a/drivers/mfd/Kconfig > > > +++ b/drivers/mfd/Kconfig > > > @@ -1057,6 +1057,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/Makefile b/drivers/mfd/Makefile > > > index 110ea700231b..2906d5db67d0 100644 > > > --- a/drivers/mfd/Makefile > > > +++ b/drivers/mfd/Makefile > > > @@ -217,7 +217,7 @@ obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o > > > obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o > > > obj-$(CONFIG_MFD_RK808) += rk808.o > > > > > > -rn5t618-objs := rn5t618-core.o > > > +rn5t618-objs := rn5t618-core.o rn5t618-irq.o > > > obj-$(CONFIG_MFD_RN5T618) += rn5t618.o > > > obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o > > > obj-$(CONFIG_MFD_SYSCON) += syscon.o > > > diff --git a/drivers/mfd/rn5t618-core.c b/drivers/mfd/rn5t618-core.c > > > index da5cd9c92a59..1e2326217681 100644 > > > --- a/drivers/mfd/rn5t618-core.c > > > +++ b/drivers/mfd/rn5t618-core.c > > > @@ -8,6 +8,7 @@ > > > > > > #include <linux/delay.h> > > > #include <linux/i2c.h> > > > +#include <linux/interrupt.h> > > > #include <linux/mfd/core.h> > > > #include <linux/mfd/rn5t618.h> > > > #include <linux/module.h> > > > @@ -105,7 +106,8 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, > > > > > > i2c_set_clientdata(i2c, priv); > > > priv->variant = (long)of_id->data; > > > - > > > + priv->chip_irq = i2c->irq; > > > + priv->dev = &i2c->dev; > > > > '\n' > > > > > priv->regmap = devm_regmap_init_i2c(i2c, &rn5t618_regmap_config); > > > if (IS_ERR(priv->regmap)) { > > > ret = PTR_ERR(priv->regmap); > > > @@ -137,6 +139,11 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, > > > return ret; > > > } > > > > > > + if (priv->chip_irq > 0) { > > > + if (rn5t618_irq_init(priv)) > > > + priv->chip_irq = 0; > > > + } > > > + > > > return 0; > > > } > > > > > > @@ -154,15 +161,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->chip_irq) > > > + disable_irq(priv->chip_irq); > > > + > > > + return 0; > > > +} > > > + > > > +static int __maybe_unused rn5t618_i2c_resume(struct device *dev) > > > +{ > > > + struct rn5t618 *priv = dev_get_drvdata(dev); > > > + > > > + if (priv->chip_irq) > > > + enable_irq(priv->chip_irq); > > > + > > > + return 0; > > > +} > > > + > > > static const struct i2c_device_id rn5t618_i2c_id[] = { > > > { } > > > }; > > > MODULE_DEVICE_TABLE(i2c, rn5t618_i2c_id); > > > > Not this patch I know, but it's strange to see this empty. > > Yes, should be cleaned up. For now the device tree stuff seems to kick in. I think this can be removed completely. Just make sure you use .probe2 and it should be automatic. [...] > > > + switch (rn5t618->variant) { > > > + case RC5T619: > > > + irq_chip = &rc5t619_irq_chip; > > > + break; > > > + > > > + /* TODO: check irq definitions for other variants */ > > > > No need for this. It's implied. > > > > OOI, when support for more variants be added? > > > I have done research about the RN5T618. It has just the RTC IRQ missing, I could just > add the table for it to prepare the path for others. I cannot test it but > since there are no users yet, it does not harm that it is not well-tested. If there are no users and it can't be tested, it should be left out. We don't want potentially broken, untested code with no users in the kernel.
On Wed, 11 Dec 2019 07:50:21 +0000 Lee Jones <lee.jones@linaro.org> wrote: [...] > > > > + > > > > static const struct i2c_device_id rn5t618_i2c_id[] = { > > > > { } > > > > }; > > > > MODULE_DEVICE_TABLE(i2c, rn5t618_i2c_id); > > > > > > Not this patch I know, but it's strange to see this empty. > > > > Yes, should be cleaned up. For now the device tree stuff seems to kick in. > > I think this can be removed completely. > > Just make sure you use .probe2 and it should be automatic. > Hmm, I cannot find probe2 but probe_new. So you mean probe_new? I will send a separate cleanup patch Regards, Andreas
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index ae24d3ea68ea..522e068d0082 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -1057,6 +1057,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/Makefile b/drivers/mfd/Makefile index 110ea700231b..2906d5db67d0 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -217,7 +217,7 @@ obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o obj-$(CONFIG_MFD_RK808) += rk808.o -rn5t618-objs := rn5t618-core.o +rn5t618-objs := rn5t618-core.o rn5t618-irq.o obj-$(CONFIG_MFD_RN5T618) += rn5t618.o obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o obj-$(CONFIG_MFD_SYSCON) += syscon.o diff --git a/drivers/mfd/rn5t618-core.c b/drivers/mfd/rn5t618-core.c index da5cd9c92a59..1e2326217681 100644 --- a/drivers/mfd/rn5t618-core.c +++ b/drivers/mfd/rn5t618-core.c @@ -8,6 +8,7 @@ #include <linux/delay.h> #include <linux/i2c.h> +#include <linux/interrupt.h> #include <linux/mfd/core.h> #include <linux/mfd/rn5t618.h> #include <linux/module.h> @@ -105,7 +106,8 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, i2c_set_clientdata(i2c, priv); priv->variant = (long)of_id->data; - + priv->chip_irq = i2c->irq; + priv->dev = &i2c->dev; priv->regmap = devm_regmap_init_i2c(i2c, &rn5t618_regmap_config); if (IS_ERR(priv->regmap)) { ret = PTR_ERR(priv->regmap); @@ -137,6 +139,11 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, return ret; } + if (priv->chip_irq > 0) { + if (rn5t618_irq_init(priv)) + priv->chip_irq = 0; + } + return 0; } @@ -154,15 +161,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->chip_irq) + disable_irq(priv->chip_irq); + + return 0; +} + +static int __maybe_unused rn5t618_i2c_resume(struct device *dev) +{ + struct rn5t618 *priv = dev_get_drvdata(dev); + + if (priv->chip_irq) + enable_irq(priv->chip_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/drivers/mfd/rn5t618-irq.c b/drivers/mfd/rn5t618-irq.c new file mode 100644 index 000000000000..8a4c56429768 --- /dev/null +++ b/drivers/mfd/rn5t618-irq.c @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2019 Andreas Kemnade + */ +#include <linux/device.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/module.h> +#include <linux/regmap.h> + +#include <linux/mfd/rn5t618.h> + +static const struct regmap_irq rc5t619_irqs[] = { + [RN5T618_IRQ_SYS] = { + .reg_offset = 0, + .mask = (0 << 1) + }, + [RN5T618_IRQ_DCDC] = { + .reg_offset = 0, + .mask = (1 << 1) + }, + [RN5T618_IRQ_RTC] = { + .reg_offset = 0, + .mask = (1 << 2) + }, + [RN5T618_IRQ_ADC] = { + .reg_offset = 0, + .mask = (1 << 3) + }, + [RN5T618_IRQ_GPIO] = { + .reg_offset = 0, + .mask = (1 << 4) + }, + [RN5T618_IRQ_CHG] = { + .reg_offset = 0, + .mask = (1 << 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, +}; + +int rn5t618_irq_init(struct rn5t618 *rn5t618) +{ + const struct regmap_irq_chip *irq_chip; + int ret; + + if (!rn5t618->chip_irq) + return 0; + + switch (rn5t618->variant) { + case RC5T619: + irq_chip = &rc5t619_irq_chip; + break; + + /* TODO: check irq definitions for other variants */ + + default: + irq_chip = NULL; + break; + } + + if (!irq_chip) { + dev_err(rn5t618->dev, "no IRQ definition known for variant\n"); + return -ENOENT; + } + + ret = devm_regmap_add_irq_chip(rn5t618->dev, rn5t618->regmap, + rn5t618->chip_irq, + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + 0, irq_chip, &rn5t618->irq_data); + if (ret != 0) { + dev_err(rn5t618->dev, "Failed to register IRQ chip\n"); + return ret; + } + + return 0; +} diff --git a/include/linux/mfd/rn5t618.h b/include/linux/mfd/rn5t618.h index d62ef48060b5..edd2b6485e3b 100644 --- a/include/linux/mfd/rn5t618.h +++ b/include/linux/mfd/rn5t618.h @@ -242,9 +242,25 @@ enum { RC5T619, }; +/* RN5T618 IRQ definitions */ +enum { + RN5T618_IRQ_SYS, + 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 chip_irq; + struct regmap_irq_chip_data *irq_data; }; +extern int rn5t618_irq_init(struct rn5t618 *rn5t618); #endif /* __LINUX_MFD_RN5T618_H */
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@kemnade.info> --- 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/Makefile | 2 +- drivers/mfd/rn5t618-core.c | 34 ++++++++++++++- drivers/mfd/rn5t618-irq.c | 85 +++++++++++++++++++++++++++++++++++++ include/linux/mfd/rn5t618.h | 16 +++++++ 5 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 drivers/mfd/rn5t618-irq.c