| Message ID | 20191211215409.32764-1-andreas@kemnade.info |
|---|---|
| Headers | show |
| Series | Add rtc support for rn5t618 mfd | expand |
On Wed, 11 Dec 2019, 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@kemnade.info> > --- > 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 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/rn5t618.c b/drivers/mfd/rn5t618.c > index da5cd9c92a59..76d997c0cfe4 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> > @@ -45,9 +47,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; > > +int rn5t618_irq_init(struct rn5t618 *rn5t618) Static?
On Wed, 11 Dec 2019, Andreas Kemnade wrote: > Defines for some RTC related registers were missing, also > they were not included in the volatile register list > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > > drivers/mfd/rn5t618.c | 2 ++ > include/linux/mfd/rn5t618.h | 11 +++++++++++ > 2 files changed, 13 insertions(+) For my own reference: Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
On Wed, 11 Dec 2019, Andreas Kemnade wrote: > The RC5T619 has a RTC which is missing in the > RN5T618. Add it as subdevice to prepare for their implementation > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > Changes in v3: > - alignment cleanup > drivers/mfd/rn5t618.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > index d78eb29b94a4..18d56a732b20 100644 > --- a/drivers/mfd/rn5t618.c > +++ b/drivers/mfd/rn5t618.c > @@ -22,6 +22,12 @@ static const struct mfd_cell rn5t618_cells[] = { > { .name = "rn5t618-wdt" }, > }; > > +static const struct mfd_cell rc5t619_cells[] = { > + { .name = "rn5t618-regulator" }, > + { .name = "rc5t619-rtc" }, > + { .name = "rn5t618-wdt" }, > +}; > + > static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg) > { > switch (reg) { > @@ -173,8 +179,14 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, > return ret; > } > > - ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells, > - ARRAY_SIZE(rn5t618_cells), NULL, 0, NULL); > + if (priv->variant == RC5T619) > + ret = devm_mfd_add_devices(&i2c->dev, -1, rc5t619_cells, Ref: The "-1", please use this as an opportunity to use the defines. > + ARRAY_SIZE(rc5t619_cells), > + NULL, 0, NULL); > + else Are you sure it's not possible for 'variant' to be an unsupported device? > + ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells, > + ARRAY_SIZE(rn5t618_cells), > + NULL, 0, NULL); > if (ret) { > dev_err(&i2c->dev, "failed to add sub-devices: %d\n", ret); > return ret;
On Mon, 16 Dec 2019 15:31:06 +0000 Lee Jones <lee.jones@linaro.org> wrote: > On Wed, 11 Dec 2019, Andreas Kemnade wrote: > > > The RC5T619 has a RTC which is missing in the > > RN5T618. Add it as subdevice to prepare for their implementation > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > --- > > Changes in v3: > > - alignment cleanup > > drivers/mfd/rn5t618.c | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > index d78eb29b94a4..18d56a732b20 100644 > > --- a/drivers/mfd/rn5t618.c > > +++ b/drivers/mfd/rn5t618.c > > @@ -22,6 +22,12 @@ static const struct mfd_cell rn5t618_cells[] = { > > { .name = "rn5t618-wdt" }, > > }; > > > > +static const struct mfd_cell rc5t619_cells[] = { > > + { .name = "rn5t618-regulator" }, > > + { .name = "rc5t619-rtc" }, > > + { .name = "rn5t618-wdt" }, > > +}; > > + > > static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg) > > { > > switch (reg) { > > @@ -173,8 +179,14 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, > > return ret; > > } > > > > - ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells, > > - ARRAY_SIZE(rn5t618_cells), NULL, 0, NULL); > > + if (priv->variant == RC5T619) > > + ret = devm_mfd_add_devices(&i2c->dev, -1, rc5t619_cells, > > Ref: The "-1", please use this as an opportunity to use the defines. > > > + ARRAY_SIZE(rc5t619_cells), > > + NULL, 0, NULL); > > + else > > Are you sure it's not possible for 'variant' to be an unsupported > device? > Well, does it change the behavior for devices other than the rc5t619? I do not think so. If the mfd driver is bound to unsupported devices, rn5t618_of_match is wrong. Or do you want separate cell arrays for each of the three variant now to ease future extensions? Regards, Andreas
On Mon, 16 Dec 2019 15:27:15 +0000 Lee Jones <lee.jones@linaro.org> wrote: > On Wed, 11 Dec 2019, 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@kemnade.info> > > --- > > 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 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/rn5t618.c b/drivers/mfd/rn5t618.c > > index da5cd9c92a59..76d997c0cfe4 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> > > @@ -45,9 +47,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; > > > > +int rn5t618_irq_init(struct rn5t618 *rn5t618) > > Static? > yes, it should be static since IRQ and core do live in the same file now. Regards, Andreas
On Mon, 16 Dec 2019, Andreas Kemnade wrote: > On Mon, 16 Dec 2019 15:31:06 +0000 > Lee Jones <lee.jones@linaro.org> wrote: > > > On Wed, 11 Dec 2019, Andreas Kemnade wrote: > > > > > The RC5T619 has a RTC which is missing in the > > > RN5T618. Add it as subdevice to prepare for their implementation > > > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > > --- > > > Changes in v3: > > > - alignment cleanup > > > drivers/mfd/rn5t618.c | 16 ++++++++++++++-- > > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > > index d78eb29b94a4..18d56a732b20 100644 > > > --- a/drivers/mfd/rn5t618.c > > > +++ b/drivers/mfd/rn5t618.c > > > @@ -22,6 +22,12 @@ static const struct mfd_cell rn5t618_cells[] = { > > > { .name = "rn5t618-wdt" }, > > > }; > > > > > > +static const struct mfd_cell rc5t619_cells[] = { > > > + { .name = "rn5t618-regulator" }, > > > + { .name = "rc5t619-rtc" }, > > > + { .name = "rn5t618-wdt" }, > > > +}; > > > + > > > static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg) > > > { > > > switch (reg) { > > > @@ -173,8 +179,14 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, > > > return ret; > > > } > > > > > > - ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells, > > > - ARRAY_SIZE(rn5t618_cells), NULL, 0, NULL); > > > + if (priv->variant == RC5T619) > > > + ret = devm_mfd_add_devices(&i2c->dev, -1, rc5t619_cells, > > > > Ref: The "-1", please use this as an opportunity to use the defines. > > > > > + ARRAY_SIZE(rc5t619_cells), > > > + NULL, 0, NULL); > > > + else > > > > Are you sure it's not possible for 'variant' to be an unsupported > > device? > > > Well, does it change the behavior for devices other than the rc5t619? > I do not think so. If the mfd driver is bound to unsupported devices, > rn5t618_of_match is wrong. Right, and can you catch that? > Or do you want separate cell arrays for each of the three variant now to > ease future extensions? No need for that, thank you.
On Tue, 17 Dec 2019 08:01:46 +0000 Lee Jones <lee.jones@linaro.org> wrote: > On Mon, 16 Dec 2019, Andreas Kemnade wrote: > > > On Mon, 16 Dec 2019 15:31:06 +0000 > > Lee Jones <lee.jones@linaro.org> wrote: > > > > > On Wed, 11 Dec 2019, Andreas Kemnade wrote: > > > > > > > The RC5T619 has a RTC which is missing in the > > > > RN5T618. Add it as subdevice to prepare for their implementation > > > > > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > > > --- > > > > Changes in v3: > > > > - alignment cleanup > > > > drivers/mfd/rn5t618.c | 16 ++++++++++++++-- > > > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > > > index d78eb29b94a4..18d56a732b20 100644 > > > > --- a/drivers/mfd/rn5t618.c > > > > +++ b/drivers/mfd/rn5t618.c > > > > @@ -22,6 +22,12 @@ static const struct mfd_cell rn5t618_cells[] = { > > > > { .name = "rn5t618-wdt" }, > > > > }; > > > > > > > > +static const struct mfd_cell rc5t619_cells[] = { > > > > + { .name = "rn5t618-regulator" }, > > > > + { .name = "rc5t619-rtc" }, > > > > + { .name = "rn5t618-wdt" }, > > > > +}; > > > > + > > > > static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg) > > > > { > > > > switch (reg) { > > > > @@ -173,8 +179,14 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, > > > > return ret; > > > > } > > > > > > > > - ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells, > > > > - ARRAY_SIZE(rn5t618_cells), NULL, 0, NULL); > > > > + if (priv->variant == RC5T619) > > > > + ret = devm_mfd_add_devices(&i2c->dev, -1, rc5t619_cells, > > > > > > Ref: The "-1", please use this as an opportunity to use the defines. > > > BTW: of course I can clean that up. > > > > + ARRAY_SIZE(rc5t619_cells), > > > > + NULL, 0, NULL); > > > > + else > > > > > > Are you sure it's not possible for 'variant' to be an unsupported > > > device? > > > > > Well, does it change the behavior for devices other than the rc5t619? > > I do not think so. If the mfd driver is bound to unsupported devices, > > rn5t618_of_match is wrong. > > Right, and can you catch that? > Well, maybe we don't get each other. RC5T619 has an RTC. If I understand the code right, priv->variant is set to RC5T619. if there is compatible = "ricoh,rc5t619" in the device tree. So in that and only in that case I have an extended subdevice list, which includes the RTC subdevice instead of only regulator and wdt. For everything else I do not touch it. So now you have doubts wether the existing regulator and watchdog subdevice drivers (which I do not touch) works with the other two variants (RN5T618 and RN5T567) and want me to check that again as part of my work to get RTC support for RC5T619 in? Regards, Andreas
On Tue, 17 Dec 2019 12:25:26 +0100 Andreas Kemnade <andreas@kemnade.info> wrote: > On Tue, 17 Dec 2019 08:01:46 +0000 > Lee Jones <lee.jones@linaro.org> wrote: > > > On Mon, 16 Dec 2019, Andreas Kemnade wrote: > > > > > On Mon, 16 Dec 2019 15:31:06 +0000 > > > Lee Jones <lee.jones@linaro.org> wrote: > > > > > > > On Wed, 11 Dec 2019, Andreas Kemnade wrote: > > > > > > > > > The RC5T619 has a RTC which is missing in the > > > > > RN5T618. Add it as subdevice to prepare for their implementation > > > > > > > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > > > > --- > > > > > Changes in v3: > > > > > - alignment cleanup > > > > > drivers/mfd/rn5t618.c | 16 ++++++++++++++-- > > > > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > > > > index d78eb29b94a4..18d56a732b20 100644 > > > > > --- a/drivers/mfd/rn5t618.c > > > > > +++ b/drivers/mfd/rn5t618.c > > > > > @@ -22,6 +22,12 @@ static const struct mfd_cell rn5t618_cells[] = { > > > > > { .name = "rn5t618-wdt" }, > > > > > }; > > > > > > > > > > +static const struct mfd_cell rc5t619_cells[] = { > > > > > + { .name = "rn5t618-regulator" }, > > > > > + { .name = "rc5t619-rtc" }, > > > > > + { .name = "rn5t618-wdt" }, > > > > > +}; > > > > > + > > > > > static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg) > > > > > { > > > > > switch (reg) { > > > > > @@ -173,8 +179,14 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c, > > > > > return ret; > > > > > } > > > > > > > > > > - ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells, > > > > > - ARRAY_SIZE(rn5t618_cells), NULL, 0, NULL); > > > > > + if (priv->variant == RC5T619) > > > > > + ret = devm_mfd_add_devices(&i2c->dev, -1, rc5t619_cells, > > > > > > > > Ref: The "-1", please use this as an opportunity to use the defines. > > > > > BTW: of course I can clean that up. > > > > > > + ARRAY_SIZE(rc5t619_cells), > > > > > + NULL, 0, NULL); > > > > > + else > > > > > > > > Are you sure it's not possible for 'variant' to be an unsupported > > > > device? > > > > > > > Well, does it change the behavior for devices other than the rc5t619? > > > I do not think so. If the mfd driver is bound to unsupported devices, > > > rn5t618_of_match is wrong. > > > > Right, and can you catch that? > > > Well, maybe we don't get each other. RC5T619 has an RTC. > If I understand the code right, priv->variant is set to RC5T619. > if there is compatible = "ricoh,rc5t619" in the device tree. > So in that and only in that case I have an extended subdevice list, > which includes the RTC subdevice instead of only regulator and wdt. > For everything else I do not touch it. > Hmm, if the driver might be probed without device tree node, there is: of_id = of_match_device(rn5t618_of_match, &i2c->dev); if (!of_id) { dev_err(&i2c->dev, "Failed to find matching DT ID\n"); return -EINVAL; } so nothing will be registered at all. So the only reasons I see the driver might be bound to unsupported devices could be: - a bug in of_match_device() - nonsense in rn5t618_of_match (I think that are well-thought entries) Regards, Andreas