| Message ID | 20250512-dev-adp5589-fw-v3-13-092b14b79a88@analog.com |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: adp5585: support keymap events and drop legacy Input driver | expand |
On Tue, 2025-05-13 at 16:59 +0100, Lee Jones wrote: > On Mon, 12 May 2025, Nuno Sá via B4 Relay wrote: > > > From: Nuno Sá <nuno.sa@analog.com> > > > > These devices are capable of generate FIFO based events based on KEY or > > GPI presses. Add support for handling these events. This is in > > preparation of adding full support for keymap and gpis based events. > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/mfd/adp5585.c | 180 ++++++++++++++++++++++++++++++++++++++++++-- > > include/linux/mfd/adp5585.h | 48 ++++++++++++ > > 2 files changed, 223 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > index > > 8be7a76842f639cbe90ad0eb956a7a3eef43fa3d..5851ad30e7323bbb891878167d0786bc60ef5d9 > > 0 100644 > > --- a/drivers/mfd/adp5585.c > > +++ b/drivers/mfd/adp5585.c > > @@ -154,10 +154,16 @@ static const struct regmap_config > > adp5585_regmap_config_template = { > > > > static const struct adp5585_regs adp5585_regs = { > > .ext_cfg = ADP5585_PIN_CONFIG_C, > > + .int_en = ADP5585_INT_EN, > > + .gen_cfg = ADP5585_GENERAL_CFG, > > + .poll_ptime_cfg = ADP5585_POLL_PTIME_CFG, > > }; > > > > static const struct adp5585_regs adp5589_regs = { > > .ext_cfg = ADP5589_PIN_CONFIG_D, > > + .int_en = ADP5589_INT_EN, > > + .gen_cfg = ADP5589_GENERAL_CFG, > > + .poll_ptime_cfg = ADP5589_POLL_PTIME_CFG, > > }; > > > > static int adp5585_fill_chip_configs(struct adp5585_dev *adp5585, > > @@ -214,6 +220,8 @@ static int adp5585_parse_fw(struct device *dev, struct > > adp5585_dev *adp5585, > > { > > unsigned int has_pwm = 0, has_gpio = 0, rc = 0; > > const struct mfd_cell *cells; > > + unsigned int prop_val; > > + int ret; > > > > if (device_property_present(dev, "#pwm-cells")) > > has_pwm = 1; > > @@ -224,6 +232,25 @@ static int adp5585_parse_fw(struct device *dev, struct > > adp5585_dev *adp5585, > > if (!has_pwm && !has_gpio) > > return -ENODEV; > > > > + ret = device_property_read_u32(dev, "poll-interval", &prop_val); > > + if (!ret) { > > + switch (prop_val) { > > + case 10: > > + fallthrough; > > + case 20: > > + fallthrough; > > + case 30: > > + fallthrough; > > + case 40: > > + adp5585->ev_poll_time = prop_val / 10 - 1; > > + break; > > + default: > > + return dev_err_probe(dev, -EINVAL, > > + "Invalid value(%u) for poll- > > interval\n", > > + prop_val); > > + } > > + } > > This all seems like a lot of code for: > > ev_poll_time = prop_val / 10 - 1; > if (ev_poll_time > 3 || ev_poll_time < 0) > return dev_err_probe(); Yes. In this enum FW parsing stuff I tend to like this switch() so it's easier to reason about the allowed values. No strong feelings though... > > > + > > *devs = devm_kcalloc(dev, has_pwm + has_gpio, sizeof(struct mfd_cell), > > GFP_KERNEL); > > if (!*devs) > > @@ -249,6 +276,135 @@ static void adp5585_osc_disable(void *data) > > regmap_write(adp5585->regmap, ADP5585_GENERAL_CFG, 0); > > } > > > > +static void adp5585_report_events(struct adp5585_dev *adp5585, int ev_cnt) > > +{ > > + struct adp5585_ev_handler *h; > > + unsigned int i; > > + > > + guard(mutex)(&adp5585->ev_lock); > > + > > + if (list_empty(&adp5585->ev_handlers)) { > > + dev_warn_ratelimited(adp5585->dev, "No event handlers > > registered\n"); > > + return; > > + } > > + > > + for (i = 0; i < ev_cnt; i++) { > > + unsigned int key, key_val, key_press; > > + int ret; > > + > > + ret = regmap_read(adp5585->regmap, ADP5585_FIFO_1 + i, &key); > > + if (ret) > > + return; > > + > > + key_val = FIELD_GET(ADP5585_KEY_EVENT_MASK, key); > > + key_press = FIELD_GET(ADP5585_KEV_EV_PRESS_MASK, key); > > + > > + list_for_each_entry(h, &adp5585->ev_handlers, entry) { > > + ret = h->handler(h->dev, key_val, key_press); > > Rather than rolling your own call-back handler mechanism. Are you sure > the kernel doesn't provide a generic solution for this? For instance, > would a shared workqueue be better? This way you could also exit IRQ > context sooner as well. > I'll have to check it but not sure there's anything simple enough. I'm not too convinced with the workqueue and deferring the handling to a worker. > > + if (!ret) > > + /* handled! */ > > All comments should start with an upper case char. > > > + break; > > + } > > + } > > +} > > + > > +static irqreturn_t adp5585_irq(int irq, void *data) > > +{ > > + struct adp5585_dev *adp5585 = data; > > + unsigned int status, ev_cnt; > > + int ret; > > + > > + ret = regmap_read(adp5585->regmap, ADP5585_INT_STATUS, &status); > > + if (ret) > > + return IRQ_HANDLED; > > + > > + if (status & ADP5585_OVRFLOW_INT) > > + dev_err_ratelimited(adp5585->dev, "Event Overflow Error\n"); > > Strange capitalisation. > > > + > > + if (!(status & ADP5585_EVENT_INT)) > > + goto out_irq; > > + > > + ret = regmap_read(adp5585->regmap, ADP5585_STATUS, &ev_cnt); > > + if (ret) > > + goto out_irq; > > + > > + ev_cnt = FIELD_GET(ADP5585_EC_MASK, ev_cnt); > > + if (!ev_cnt) > > + goto out_irq; > > + > > + adp5585_report_events(adp5585, ev_cnt); > > You don't want to propagate any errors? We should return irqreturn_t anyways... That's why I ignore it. > > > +out_irq: > > + regmap_write(adp5585->regmap, ADP5585_INT_STATUS, status); > > + return IRQ_HANDLED; > > +} > > + > > +static int adp5585_setup(struct adp5585_dev *adp5585) > > +{ > > + const struct adp5585_regs *regs = adp5585->info->regs; > > + unsigned int reg_val, i; > > + int ret; > > The final version of this function needs some nice commentary to explain > what each step is doing. May as well start now. > ack > > + for (i = 0; i < ADP5585_EV_MAX; i++) { > > + ret = regmap_read(adp5585->regmap, ADP5585_FIFO_1 + i, > > ®_val); > > + if (ret) > > + return ret; > > + } > > + > > + ret = regmap_write(adp5585->regmap, regs->poll_ptime_cfg, > > + adp5585->ev_poll_time); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(adp5585->regmap, regs->gen_cfg, > > + ADP5585_OSC_FREQ_500KHZ | ADP5585_INT_CFG | > > + ADP5585_OSC_EN); > > + if (ret) > > + return ret; > > + > > + return devm_add_action_or_reset(adp5585->dev, adp5585_osc_disable, > > + adp5585); > > +} > > + > > +static void adp5585_irq_disable(void *data) > > +{ > > + struct adp5585_dev *adp5585 = data; > > + > > + regmap_write(adp5585->regmap, adp5585->info->regs->int_en, 0); > > +} > > + > > +static int adp5585_irq_enable(struct i2c_client *i2c, > > + struct adp5585_dev *adp5585) > > +{ > > + const struct adp5585_regs *regs = adp5585->info->regs; > > + unsigned int stat; > > + int ret; > > + > > + if (i2c->irq <= 0) > > + return 0; > > + > > + ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL, adp5585_irq, > > + IRQF_ONESHOT, i2c->name, adp5585); > > + if (ret) > > + return ret; > > + > > + /* clear any possible outstanding interrupt before enabling them... */ > > Uppercase char (I won't report on this again) and now silly punctuation > please... > > > + ret = regmap_read(adp5585->regmap, ADP5585_INT_STATUS, &stat); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(adp5585->regmap, ADP5585_INT_STATUS, stat); > > + if (ret) > > + return ret; > > What does reading status values then writing them right back do? > That's how we clear outstanding interrupts. I do have the comment but I can be more explicit :) > Commentary throughout please. > > > + ret = regmap_write(adp5585->regmap, regs->int_en, > > + ADP5585_OVRFLOW_IEN | ADP5585_EVENT_IEN); > > + if (ret) > > + return ret; > > + > > + return devm_add_action_or_reset(&i2c->dev, adp5585_irq_disable, > > + adp5585); > > Feel free to use 100-chars to help with these early line breaks. > > > +} > > + > > static int adp5585_i2c_probe(struct i2c_client *i2c) > > { > > struct regmap_config regmap_config; > > @@ -282,16 +438,19 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > return dev_err_probe(&i2c->dev, -ENODEV, > > "Invalid device ID 0x%02x\n", id); > > > > + adp5585->dev = &i2c->dev; > > + adp5585->irq = i2c->irq; > > + INIT_LIST_HEAD(&adp5585->ev_handlers); > > + > > n_devs = adp5585_parse_fw(&i2c->dev, adp5585, &devs); > > if (n_devs < 0) > > return n_devs; > > > > - ret = regmap_set_bits(adp5585->regmap, ADP5585_GENERAL_CFG, > > - ADP5585_OSC_EN); > > + ret = adp5585_setup(adp5585); > > if (ret) > > return ret; > > > > - ret = devm_add_action_or_reset(&i2c->dev, adp5585_osc_disable, adp5585); > > + ret = devm_mutex_init(&i2c->dev, &adp5585->ev_lock); > > if (ret) > > return ret; > > > > @@ -301,13 +460,16 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > return dev_err_probe(&i2c->dev, ret, > > "Failed to add child devices\n"); > > > > - return 0; > > + return adp5585_irq_enable(i2c, adp5585); > > } > > > > static int adp5585_suspend(struct device *dev) > > { > > struct adp5585_dev *adp5585 = dev_get_drvdata(dev); > > > > + if (adp5585->irq) > > + disable_irq(adp5585->irq); > > + > > regcache_cache_only(adp5585->regmap, true); > > > > return 0; > > @@ -316,11 +478,19 @@ static int adp5585_suspend(struct device *dev) > > static int adp5585_resume(struct device *dev) > > { > > struct adp5585_dev *adp5585 = dev_get_drvdata(dev); > > + int ret; > > > > regcache_cache_only(adp5585->regmap, false); > > regcache_mark_dirty(adp5585->regmap); > > > > - return regcache_sync(adp5585->regmap); > > + ret = regcache_sync(adp5585->regmap); > > + if (ret) > > + return ret; > > + > > + if (adp5585->irq) > > + enable_irq(adp5585->irq); > > + > > + return 0; > > } > > > > static DEFINE_SIMPLE_DEV_PM_OPS(adp5585_pm, adp5585_suspend, adp5585_resume); > > diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h > > index > > 9a925a91c772722db559c9ec8ae334b2159ede79..218c56bed2e0304de8b81c7090386fb4e1b6c28 > > 1 100644 > > --- a/include/linux/mfd/adp5585.h > > +++ b/include/linux/mfd/adp5585.h > > @@ -10,13 +10,23 @@ > > #define __MFD_ADP5585_H_ > > > > #include <linux/bits.h> > > +#include <linux/cleanup.h> > > +#include <linux/device.h> > > +#include <linux/list.h> > > +#include <linux/mutex.h> > > > > #define ADP5585_ID 0x00 > > #define ADP5585_MAN_ID_VALUE 0x20 > > #define ADP5585_MAN_ID_MASK GENMASK(7, 4) > > +#define ADP5585_REV_ID_MASK GENMASK(3, 0) > > #define ADP5585_INT_STATUS 0x01 > > +#define ADP5585_OVRFLOW_INT BIT(2) > > +#define ADP5585_EVENT_INT BIT(0) > > #define ADP5585_STATUS 0x02 > > +#define ADP5585_EC_MASK GENMASK(4, 0) > > #define ADP5585_FIFO_1 0x03 > > +#define ADP5585_KEV_EV_PRESS_MASK BIT(7) > > +#define ADP5585_KEY_EVENT_MASK GENMASK(6, 0) > > #define ADP5585_FIFO_2 0x04 > > #define ADP5585_FIFO_3 0x05 > > #define ADP5585_FIFO_4 0x06 > > @@ -32,6 +42,7 @@ > > #define ADP5585_FIFO_14 0x10 > > #define ADP5585_FIFO_15 0x11 > > #define ADP5585_FIFO_16 0x12 > > +#define ADP5585_EV_MAX (ADP5585_FIFO_16 - > > ADP5585_FIFO_1 + 1) > > #define ADP5585_GPI_INT_STAT_A 0x13 > > #define ADP5585_GPI_INT_STAT_B 0x14 > > #define ADP5585_GPI_STATUS_A 0x15 > > @@ -104,6 +115,8 @@ > > #define ADP5585_INT_CFG BIT(1) > > #define ADP5585_RST_CFG BIT(0) > > #define ADP5585_INT_EN 0x3c > > +#define ADP5585_OVRFLOW_IEN BIT(2) > > +#define ADP5585_EVENT_IEN BIT(0) > > > > #define ADP5585_MAX_REG ADP5585_INT_EN > > > > @@ -121,7 +134,9 @@ > > #define ADP5589_PWM_OFFT_LOW 0x3e > > #define ADP5589_PWM_ONT_LOW 0x40 > > #define ADP5589_PWM_CFG 0x42 > > +#define ADP5589_POLL_PTIME_CFG 0x48 > > #define ADP5589_PIN_CONFIG_D 0x4C > > +#define ADP5589_GENERAL_CFG 0x4d > > #define ADP5589_INT_EN 0x4e > > #define ADP5589_MAX_REG ADP5589_INT_EN > > > > @@ -138,8 +153,18 @@ enum adp5585_regmap_type { > > ADP5589_REGMAP_02, > > }; > > > > +struct adp5585_ev_handler { > > + struct list_head entry; > > + struct device *dev; > > + int (*handler)(struct device *dev, unsigned int key_val, > > + unsigned int key_press); > > Pointer to functions outside of subsystem-level ops are generally > frowned upon. Are you sure there isn't a standard way to achieve your > goal without them? I'll see but nothing that comes from the top of my head. You really have an aversion to function pointers =) - Nuno Sá > > > +}; > > + > > struct adp5585_regs { > > + unsigned int gen_cfg; > > unsigned int ext_cfg; > > + unsigned int int_en; > > + unsigned int poll_ptime_cfg; > > }; > > > > struct adp5585_info { > > @@ -150,7 +175,30 @@ struct adp5585_info { > > > > struct adp5585_dev { > > struct regmap *regmap; > > + struct device *dev; > > const struct adp5585_info *info; > > + /* Used to synchronize the availability of the event handlers */ > > + struct mutex ev_lock; > > + struct list_head ev_handlers; > > + int irq; > > + unsigned int ev_poll_time; > > }; > > > > +static inline void adp5585_ev_handler_remove(void *data) > > +{ > > + struct adp5585_ev_handler *handler = data; > > + struct adp5585_dev *adp5585 = dev_get_drvdata(handler->dev->parent); > > + > > + guard(mutex)(&adp5585->ev_lock); > > + list_del(&handler->entry); > > +} > > + > > +static inline int devm_adp5585_ev_handler_add(struct adp5585_dev *adp5585, > > + struct adp5585_ev_handler > > *handler) > > +{ > > + guard(mutex)(&adp5585->ev_lock); > > + list_add_tail(&handler->entry, &adp5585->ev_handlers); > > + return devm_add_action_or_reset(handler->dev, adp5585_ev_handler_remove, > > + handler); > > +} > > #endif > > > > -- > > 2.49.0 > > > > >
On Tue, 2025-05-13 at 16:59 +0100, Lee Jones wrote: > On Mon, 12 May 2025, Nuno Sá via B4 Relay wrote: > > > From: Nuno Sá <nuno.sa@analog.com> > > > > These devices are capable of generate FIFO based events based on KEY or > > GPI presses. Add support for handling these events. This is in > > preparation of adding full support for keymap and gpis based events. > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/mfd/adp5585.c | 180 ++++++++++++++++++++++++++++++++++++++++++-- > > include/linux/mfd/adp5585.h | 48 ++++++++++++ > > 2 files changed, 223 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > index > > 8be7a76842f639cbe90ad0eb956a7a3eef43fa3d..5851ad30e7323bbb891878167d0786bc60ef5d9 > > 0 100644 > > --- a/drivers/mfd/adp5585.c > > +++ b/drivers/mfd/adp5585.c > > @@ -154,10 +154,16 @@ static const struct regmap_config > > adp5585_regmap_config_template = { > > > > static const struct adp5585_regs adp5585_regs = { > > .ext_cfg = ADP5585_PIN_CONFIG_C, > > + .int_en = ADP5585_INT_EN, > > + .gen_cfg = ADP5585_GENERAL_CFG, > > + .poll_ptime_cfg = ADP5585_POLL_PTIME_CFG, > > }; > > > > static const struct adp5585_regs adp5589_regs = { > > .ext_cfg = ADP5589_PIN_CONFIG_D, > > + .int_en = ADP5589_INT_EN, > > + .gen_cfg = ADP5589_GENERAL_CFG, > > + .poll_ptime_cfg = ADP5589_POLL_PTIME_CFG, > > }; > > > > static int adp5585_fill_chip_configs(struct adp5585_dev *adp5585, > > @@ -214,6 +220,8 @@ static int adp5585_parse_fw(struct device *dev, struct > > adp5585_dev *adp5585, > > { > > unsigned int has_pwm = 0, has_gpio = 0, rc = 0; > > const struct mfd_cell *cells; > > + unsigned int prop_val; > > + int ret; > > > > if (device_property_present(dev, "#pwm-cells")) > > has_pwm = 1; > > @@ -224,6 +232,25 @@ static int adp5585_parse_fw(struct device *dev, struct > > adp5585_dev *adp5585, > > if (!has_pwm && !has_gpio) > > return -ENODEV; > > > > + ret = device_property_read_u32(dev, "poll-interval", &prop_val); > > + if (!ret) { > > + switch (prop_val) { > > + case 10: > > + fallthrough; > > + case 20: > > + fallthrough; > > + case 30: > > + fallthrough; > > + case 40: > > + adp5585->ev_poll_time = prop_val / 10 - 1; > > + break; > > + default: > > + return dev_err_probe(dev, -EINVAL, > > + "Invalid value(%u) for poll- > > interval\n", > > + prop_val); > > + } > > + } > > This all seems like a lot of code for: > > ev_poll_time = prop_val / 10 - 1; > if (ev_poll_time > 3 || ev_poll_time < 0) > return dev_err_probe(); > > > + > > *devs = devm_kcalloc(dev, has_pwm + has_gpio, sizeof(struct mfd_cell), > > GFP_KERNEL); > > if (!*devs) > > @@ -249,6 +276,135 @@ static void adp5585_osc_disable(void *data) > > regmap_write(adp5585->regmap, ADP5585_GENERAL_CFG, 0); > > } > > > > +static void adp5585_report_events(struct adp5585_dev *adp5585, int ev_cnt) > > +{ > > + struct adp5585_ev_handler *h; > > + unsigned int i; > > + > > + guard(mutex)(&adp5585->ev_lock); > > + > > + if (list_empty(&adp5585->ev_handlers)) { > > + dev_warn_ratelimited(adp5585->dev, "No event handlers > > registered\n"); > > + return; > > + } > > + > > + for (i = 0; i < ev_cnt; i++) { > > + unsigned int key, key_val, key_press; > > + int ret; > > + > > + ret = regmap_read(adp5585->regmap, ADP5585_FIFO_1 + i, &key); > > + if (ret) > > + return; > > + > > + key_val = FIELD_GET(ADP5585_KEY_EVENT_MASK, key); > > + key_press = FIELD_GET(ADP5585_KEV_EV_PRESS_MASK, key); > > + > > + list_for_each_entry(h, &adp5585->ev_handlers, entry) { > > + ret = h->handler(h->dev, key_val, key_press); > > Rather than rolling your own call-back handler mechanism. Are you sure > the kernel doesn't provide a generic solution for this? For instance, > would a shared workqueue be better? This way you could also exit IRQ > context sooner as well. > > > + if (!ret) > > + /* handled! */ > > All comments should start with an upper case char. > > > + break; > > + } > > + } > > +} > > + > > +static irqreturn_t adp5585_irq(int irq, void *data) > > +{ > > + struct adp5585_dev *adp5585 = data; > > + unsigned int status, ev_cnt; > > + int ret; > > + > > + ret = regmap_read(adp5585->regmap, ADP5585_INT_STATUS, &status); > > + if (ret) > > + return IRQ_HANDLED; > > + > > + if (status & ADP5585_OVRFLOW_INT) > > + dev_err_ratelimited(adp5585->dev, "Event Overflow Error\n"); > > Strange capitalisation. > > > + > > + if (!(status & ADP5585_EVENT_INT)) > > + goto out_irq; > > + > > + ret = regmap_read(adp5585->regmap, ADP5585_STATUS, &ev_cnt); > > + if (ret) > > + goto out_irq; > > + > > + ev_cnt = FIELD_GET(ADP5585_EC_MASK, ev_cnt); > > + if (!ev_cnt) > > + goto out_irq; > > + > > + adp5585_report_events(adp5585, ev_cnt); > > You don't want to propagate any errors? > > > +out_irq: > > + regmap_write(adp5585->regmap, ADP5585_INT_STATUS, status); > > + return IRQ_HANDLED; > > +} > > + > > +static int adp5585_setup(struct adp5585_dev *adp5585) > > +{ > > + const struct adp5585_regs *regs = adp5585->info->regs; > > + unsigned int reg_val, i; > > + int ret; > > The final version of this function needs some nice commentary to explain > what each step is doing. May as well start now. > > > + for (i = 0; i < ADP5585_EV_MAX; i++) { > > + ret = regmap_read(adp5585->regmap, ADP5585_FIFO_1 + i, > > ®_val); > > + if (ret) > > + return ret; > > + } > > + > > + ret = regmap_write(adp5585->regmap, regs->poll_ptime_cfg, > > + adp5585->ev_poll_time); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(adp5585->regmap, regs->gen_cfg, > > + ADP5585_OSC_FREQ_500KHZ | ADP5585_INT_CFG | > > + ADP5585_OSC_EN); > > + if (ret) > > + return ret; > > + > > + return devm_add_action_or_reset(adp5585->dev, adp5585_osc_disable, > > + adp5585); > > +} > > + > > +static void adp5585_irq_disable(void *data) > > +{ > > + struct adp5585_dev *adp5585 = data; > > + > > + regmap_write(adp5585->regmap, adp5585->info->regs->int_en, 0); > > +} > > + > > +static int adp5585_irq_enable(struct i2c_client *i2c, > > + struct adp5585_dev *adp5585) > > +{ > > + const struct adp5585_regs *regs = adp5585->info->regs; > > + unsigned int stat; > > + int ret; > > + > > + if (i2c->irq <= 0) > > + return 0; > > + > > + ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL, adp5585_irq, > > + IRQF_ONESHOT, i2c->name, adp5585); > > + if (ret) > > + return ret; > > + > > + /* clear any possible outstanding interrupt before enabling them... */ > > Uppercase char (I won't report on this again) and now silly punctuation > please... > > > + ret = regmap_read(adp5585->regmap, ADP5585_INT_STATUS, &stat); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(adp5585->regmap, ADP5585_INT_STATUS, stat); > > + if (ret) > > + return ret; > > What does reading status values then writing them right back do? > > Commentary throughout please. > > > + ret = regmap_write(adp5585->regmap, regs->int_en, > > + ADP5585_OVRFLOW_IEN | ADP5585_EVENT_IEN); > > + if (ret) > > + return ret; > > + > > + return devm_add_action_or_reset(&i2c->dev, adp5585_irq_disable, > > + adp5585); > > Feel free to use 100-chars to help with these early line breaks. > > > +} > > + > > static int adp5585_i2c_probe(struct i2c_client *i2c) > > { > > struct regmap_config regmap_config; > > @@ -282,16 +438,19 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > return dev_err_probe(&i2c->dev, -ENODEV, > > "Invalid device ID 0x%02x\n", id); > > > > + adp5585->dev = &i2c->dev; > > + adp5585->irq = i2c->irq; > > + INIT_LIST_HEAD(&adp5585->ev_handlers); > > + > > n_devs = adp5585_parse_fw(&i2c->dev, adp5585, &devs); > > if (n_devs < 0) > > return n_devs; > > > > - ret = regmap_set_bits(adp5585->regmap, ADP5585_GENERAL_CFG, > > - ADP5585_OSC_EN); > > + ret = adp5585_setup(adp5585); > > if (ret) > > return ret; > > > > - ret = devm_add_action_or_reset(&i2c->dev, adp5585_osc_disable, adp5585); > > + ret = devm_mutex_init(&i2c->dev, &adp5585->ev_lock); > > if (ret) > > return ret; > > > > @@ -301,13 +460,16 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > return dev_err_probe(&i2c->dev, ret, > > "Failed to add child devices\n"); > > > > - return 0; > > + return adp5585_irq_enable(i2c, adp5585); > > } > > > > static int adp5585_suspend(struct device *dev) > > { > > struct adp5585_dev *adp5585 = dev_get_drvdata(dev); > > > > + if (adp5585->irq) > > + disable_irq(adp5585->irq); > > + > > regcache_cache_only(adp5585->regmap, true); > > > > return 0; > > @@ -316,11 +478,19 @@ static int adp5585_suspend(struct device *dev) > > static int adp5585_resume(struct device *dev) > > { > > struct adp5585_dev *adp5585 = dev_get_drvdata(dev); > > + int ret; > > > > regcache_cache_only(adp5585->regmap, false); > > regcache_mark_dirty(adp5585->regmap); > > > > - return regcache_sync(adp5585->regmap); > > + ret = regcache_sync(adp5585->regmap); > > + if (ret) > > + return ret; > > + > > + if (adp5585->irq) > > + enable_irq(adp5585->irq); > > + > > + return 0; > > } > > > > static DEFINE_SIMPLE_DEV_PM_OPS(adp5585_pm, adp5585_suspend, adp5585_resume); > > diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h > > index > > 9a925a91c772722db559c9ec8ae334b2159ede79..218c56bed2e0304de8b81c7090386fb4e1b6c28 > > 1 100644 > > --- a/include/linux/mfd/adp5585.h > > +++ b/include/linux/mfd/adp5585.h > > @@ -10,13 +10,23 @@ > > #define __MFD_ADP5585_H_ > > > > #include <linux/bits.h> > > +#include <linux/cleanup.h> > > +#include <linux/device.h> > > +#include <linux/list.h> > > +#include <linux/mutex.h> > > > > #define ADP5585_ID 0x00 > > #define ADP5585_MAN_ID_VALUE 0x20 > > #define ADP5585_MAN_ID_MASK GENMASK(7, 4) > > +#define ADP5585_REV_ID_MASK GENMASK(3, 0) > > #define ADP5585_INT_STATUS 0x01 > > +#define ADP5585_OVRFLOW_INT BIT(2) > > +#define ADP5585_EVENT_INT BIT(0) > > #define ADP5585_STATUS 0x02 > > +#define ADP5585_EC_MASK GENMASK(4, 0) > > #define ADP5585_FIFO_1 0x03 > > +#define ADP5585_KEV_EV_PRESS_MASK BIT(7) > > +#define ADP5585_KEY_EVENT_MASK GENMASK(6, 0) > > #define ADP5585_FIFO_2 0x04 > > #define ADP5585_FIFO_3 0x05 > > #define ADP5585_FIFO_4 0x06 > > @@ -32,6 +42,7 @@ > > #define ADP5585_FIFO_14 0x10 > > #define ADP5585_FIFO_15 0x11 > > #define ADP5585_FIFO_16 0x12 > > +#define ADP5585_EV_MAX (ADP5585_FIFO_16 - > > ADP5585_FIFO_1 + 1) > > #define ADP5585_GPI_INT_STAT_A 0x13 > > #define ADP5585_GPI_INT_STAT_B 0x14 > > #define ADP5585_GPI_STATUS_A 0x15 > > @@ -104,6 +115,8 @@ > > #define ADP5585_INT_CFG BIT(1) > > #define ADP5585_RST_CFG BIT(0) > > #define ADP5585_INT_EN 0x3c > > +#define ADP5585_OVRFLOW_IEN BIT(2) > > +#define ADP5585_EVENT_IEN BIT(0) > > > > #define ADP5585_MAX_REG ADP5585_INT_EN > > > > @@ -121,7 +134,9 @@ > > #define ADP5589_PWM_OFFT_LOW 0x3e > > #define ADP5589_PWM_ONT_LOW 0x40 > > #define ADP5589_PWM_CFG 0x42 > > +#define ADP5589_POLL_PTIME_CFG 0x48 > > #define ADP5589_PIN_CONFIG_D 0x4C > > +#define ADP5589_GENERAL_CFG 0x4d > > #define ADP5589_INT_EN 0x4e > > #define ADP5589_MAX_REG ADP5589_INT_EN > > > > @@ -138,8 +153,18 @@ enum adp5585_regmap_type { > > ADP5589_REGMAP_02, > > }; > > > > +struct adp5585_ev_handler { > > + struct list_head entry; > > + struct device *dev; > > + int (*handler)(struct device *dev, unsigned int key_val, > > + unsigned int key_press); > > Pointer to functions outside of subsystem-level ops are generally > frowned upon. Are you sure there isn't a standard way to achieve your > goal without them? Actually, it was fast. I think the notifier API will do fine here. I guess that would make you happier? - Nuno Sá > > > +}; > > + > > struct adp5585_regs { > > + unsigned int gen_cfg; > > unsigned int ext_cfg; > > + unsigned int int_en; > > + unsigned int poll_ptime_cfg; > > }; > > > > struct adp5585_info { > > @@ -150,7 +175,30 @@ struct adp5585_info { > > > > struct adp5585_dev { > > struct regmap *regmap; > > + struct device *dev; > > const struct adp5585_info *info; > > + /* Used to synchronize the availability of the event handlers */ > > + struct mutex ev_lock; > > + struct list_head ev_handlers; > > + int irq; > > + unsigned int ev_poll_time; > > }; > > > > +static inline void adp5585_ev_handler_remove(void *data) > > +{ > > + struct adp5585_ev_handler *handler = data; > > + struct adp5585_dev *adp5585 = dev_get_drvdata(handler->dev->parent); > > + > > + guard(mutex)(&adp5585->ev_lock); > > + list_del(&handler->entry); > > +} > > + > > +static inline int devm_adp5585_ev_handler_add(struct adp5585_dev *adp5585, > > + struct adp5585_ev_handler > > *handler) > > +{ > > + guard(mutex)(&adp5585->ev_lock); > > + list_add_tail(&handler->entry, &adp5585->ev_handlers); > > + return devm_add_action_or_reset(handler->dev, adp5585_ev_handler_remove, > > + handler); > > +} > > #endif > > > > -- > > 2.49.0 > > > > >
diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c index 8be7a76842f639cbe90ad0eb956a7a3eef43fa3d..5851ad30e7323bbb891878167d0786bc60ef5d90 100644 --- a/drivers/mfd/adp5585.c +++ b/drivers/mfd/adp5585.c @@ -154,10 +154,16 @@ static const struct regmap_config adp5585_regmap_config_template = { static const struct adp5585_regs adp5585_regs = { .ext_cfg = ADP5585_PIN_CONFIG_C, + .int_en = ADP5585_INT_EN, + .gen_cfg = ADP5585_GENERAL_CFG, + .poll_ptime_cfg = ADP5585_POLL_PTIME_CFG, }; static const struct adp5585_regs adp5589_regs = { .ext_cfg = ADP5589_PIN_CONFIG_D, + .int_en = ADP5589_INT_EN, + .gen_cfg = ADP5589_GENERAL_CFG, + .poll_ptime_cfg = ADP5589_POLL_PTIME_CFG, }; static int adp5585_fill_chip_configs(struct adp5585_dev *adp5585, @@ -214,6 +220,8 @@ static int adp5585_parse_fw(struct device *dev, struct adp5585_dev *adp5585, { unsigned int has_pwm = 0, has_gpio = 0, rc = 0; const struct mfd_cell *cells; + unsigned int prop_val; + int ret; if (device_property_present(dev, "#pwm-cells")) has_pwm = 1; @@ -224,6 +232,25 @@ static int adp5585_parse_fw(struct device *dev, struct adp5585_dev *adp5585, if (!has_pwm && !has_gpio) return -ENODEV; + ret = device_property_read_u32(dev, "poll-interval", &prop_val); + if (!ret) { + switch (prop_val) { + case 10: + fallthrough; + case 20: + fallthrough; + case 30: + fallthrough; + case 40: + adp5585->ev_poll_time = prop_val / 10 - 1; + break; + default: + return dev_err_probe(dev, -EINVAL, + "Invalid value(%u) for poll-interval\n", + prop_val); + } + } + *devs = devm_kcalloc(dev, has_pwm + has_gpio, sizeof(struct mfd_cell), GFP_KERNEL); if (!*devs) @@ -249,6 +276,135 @@ static void adp5585_osc_disable(void *data) regmap_write(adp5585->regmap, ADP5585_GENERAL_CFG, 0); } +static void adp5585_report_events(struct adp5585_dev *adp5585, int ev_cnt) +{ + struct adp5585_ev_handler *h; + unsigned int i; + + guard(mutex)(&adp5585->ev_lock); + + if (list_empty(&adp5585->ev_handlers)) { + dev_warn_ratelimited(adp5585->dev, "No event handlers registered\n"); + return; + } + + for (i = 0; i < ev_cnt; i++) { + unsigned int key, key_val, key_press; + int ret; + + ret = regmap_read(adp5585->regmap, ADP5585_FIFO_1 + i, &key); + if (ret) + return; + + key_val = FIELD_GET(ADP5585_KEY_EVENT_MASK, key); + key_press = FIELD_GET(ADP5585_KEV_EV_PRESS_MASK, key); + + list_for_each_entry(h, &adp5585->ev_handlers, entry) { + ret = h->handler(h->dev, key_val, key_press); + if (!ret) + /* handled! */ + break; + } + } +} + +static irqreturn_t adp5585_irq(int irq, void *data) +{ + struct adp5585_dev *adp5585 = data; + unsigned int status, ev_cnt; + int ret; + + ret = regmap_read(adp5585->regmap, ADP5585_INT_STATUS, &status); + if (ret) + return IRQ_HANDLED; + + if (status & ADP5585_OVRFLOW_INT) + dev_err_ratelimited(adp5585->dev, "Event Overflow Error\n"); + + if (!(status & ADP5585_EVENT_INT)) + goto out_irq; + + ret = regmap_read(adp5585->regmap, ADP5585_STATUS, &ev_cnt); + if (ret) + goto out_irq; + + ev_cnt = FIELD_GET(ADP5585_EC_MASK, ev_cnt); + if (!ev_cnt) + goto out_irq; + + adp5585_report_events(adp5585, ev_cnt); +out_irq: + regmap_write(adp5585->regmap, ADP5585_INT_STATUS, status); + return IRQ_HANDLED; +} + +static int adp5585_setup(struct adp5585_dev *adp5585) +{ + const struct adp5585_regs *regs = adp5585->info->regs; + unsigned int reg_val, i; + int ret; + + for (i = 0; i < ADP5585_EV_MAX; i++) { + ret = regmap_read(adp5585->regmap, ADP5585_FIFO_1 + i, ®_val); + if (ret) + return ret; + } + + ret = regmap_write(adp5585->regmap, regs->poll_ptime_cfg, + adp5585->ev_poll_time); + if (ret) + return ret; + + ret = regmap_write(adp5585->regmap, regs->gen_cfg, + ADP5585_OSC_FREQ_500KHZ | ADP5585_INT_CFG | + ADP5585_OSC_EN); + if (ret) + return ret; + + return devm_add_action_or_reset(adp5585->dev, adp5585_osc_disable, + adp5585); +} + +static void adp5585_irq_disable(void *data) +{ + struct adp5585_dev *adp5585 = data; + + regmap_write(adp5585->regmap, adp5585->info->regs->int_en, 0); +} + +static int adp5585_irq_enable(struct i2c_client *i2c, + struct adp5585_dev *adp5585) +{ + const struct adp5585_regs *regs = adp5585->info->regs; + unsigned int stat; + int ret; + + if (i2c->irq <= 0) + return 0; + + ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL, adp5585_irq, + IRQF_ONESHOT, i2c->name, adp5585); + if (ret) + return ret; + + /* clear any possible outstanding interrupt before enabling them... */ + ret = regmap_read(adp5585->regmap, ADP5585_INT_STATUS, &stat); + if (ret) + return ret; + + ret = regmap_write(adp5585->regmap, ADP5585_INT_STATUS, stat); + if (ret) + return ret; + + ret = regmap_write(adp5585->regmap, regs->int_en, + ADP5585_OVRFLOW_IEN | ADP5585_EVENT_IEN); + if (ret) + return ret; + + return devm_add_action_or_reset(&i2c->dev, adp5585_irq_disable, + adp5585); +} + static int adp5585_i2c_probe(struct i2c_client *i2c) { struct regmap_config regmap_config; @@ -282,16 +438,19 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) return dev_err_probe(&i2c->dev, -ENODEV, "Invalid device ID 0x%02x\n", id); + adp5585->dev = &i2c->dev; + adp5585->irq = i2c->irq; + INIT_LIST_HEAD(&adp5585->ev_handlers); + n_devs = adp5585_parse_fw(&i2c->dev, adp5585, &devs); if (n_devs < 0) return n_devs; - ret = regmap_set_bits(adp5585->regmap, ADP5585_GENERAL_CFG, - ADP5585_OSC_EN); + ret = adp5585_setup(adp5585); if (ret) return ret; - ret = devm_add_action_or_reset(&i2c->dev, adp5585_osc_disable, adp5585); + ret = devm_mutex_init(&i2c->dev, &adp5585->ev_lock); if (ret) return ret; @@ -301,13 +460,16 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) return dev_err_probe(&i2c->dev, ret, "Failed to add child devices\n"); - return 0; + return adp5585_irq_enable(i2c, adp5585); } static int adp5585_suspend(struct device *dev) { struct adp5585_dev *adp5585 = dev_get_drvdata(dev); + if (adp5585->irq) + disable_irq(adp5585->irq); + regcache_cache_only(adp5585->regmap, true); return 0; @@ -316,11 +478,19 @@ static int adp5585_suspend(struct device *dev) static int adp5585_resume(struct device *dev) { struct adp5585_dev *adp5585 = dev_get_drvdata(dev); + int ret; regcache_cache_only(adp5585->regmap, false); regcache_mark_dirty(adp5585->regmap); - return regcache_sync(adp5585->regmap); + ret = regcache_sync(adp5585->regmap); + if (ret) + return ret; + + if (adp5585->irq) + enable_irq(adp5585->irq); + + return 0; } static DEFINE_SIMPLE_DEV_PM_OPS(adp5585_pm, adp5585_suspend, adp5585_resume); diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h index 9a925a91c772722db559c9ec8ae334b2159ede79..218c56bed2e0304de8b81c7090386fb4e1b6c281 100644 --- a/include/linux/mfd/adp5585.h +++ b/include/linux/mfd/adp5585.h @@ -10,13 +10,23 @@ #define __MFD_ADP5585_H_ #include <linux/bits.h> +#include <linux/cleanup.h> +#include <linux/device.h> +#include <linux/list.h> +#include <linux/mutex.h> #define ADP5585_ID 0x00 #define ADP5585_MAN_ID_VALUE 0x20 #define ADP5585_MAN_ID_MASK GENMASK(7, 4) +#define ADP5585_REV_ID_MASK GENMASK(3, 0) #define ADP5585_INT_STATUS 0x01 +#define ADP5585_OVRFLOW_INT BIT(2) +#define ADP5585_EVENT_INT BIT(0) #define ADP5585_STATUS 0x02 +#define ADP5585_EC_MASK GENMASK(4, 0) #define ADP5585_FIFO_1 0x03 +#define ADP5585_KEV_EV_PRESS_MASK BIT(7) +#define ADP5585_KEY_EVENT_MASK GENMASK(6, 0) #define ADP5585_FIFO_2 0x04 #define ADP5585_FIFO_3 0x05 #define ADP5585_FIFO_4 0x06 @@ -32,6 +42,7 @@ #define ADP5585_FIFO_14 0x10 #define ADP5585_FIFO_15 0x11 #define ADP5585_FIFO_16 0x12 +#define ADP5585_EV_MAX (ADP5585_FIFO_16 - ADP5585_FIFO_1 + 1) #define ADP5585_GPI_INT_STAT_A 0x13 #define ADP5585_GPI_INT_STAT_B 0x14 #define ADP5585_GPI_STATUS_A 0x15 @@ -104,6 +115,8 @@ #define ADP5585_INT_CFG BIT(1) #define ADP5585_RST_CFG BIT(0) #define ADP5585_INT_EN 0x3c +#define ADP5585_OVRFLOW_IEN BIT(2) +#define ADP5585_EVENT_IEN BIT(0) #define ADP5585_MAX_REG ADP5585_INT_EN @@ -121,7 +134,9 @@ #define ADP5589_PWM_OFFT_LOW 0x3e #define ADP5589_PWM_ONT_LOW 0x40 #define ADP5589_PWM_CFG 0x42 +#define ADP5589_POLL_PTIME_CFG 0x48 #define ADP5589_PIN_CONFIG_D 0x4C +#define ADP5589_GENERAL_CFG 0x4d #define ADP5589_INT_EN 0x4e #define ADP5589_MAX_REG ADP5589_INT_EN @@ -138,8 +153,18 @@ enum adp5585_regmap_type { ADP5589_REGMAP_02, }; +struct adp5585_ev_handler { + struct list_head entry; + struct device *dev; + int (*handler)(struct device *dev, unsigned int key_val, + unsigned int key_press); +}; + struct adp5585_regs { + unsigned int gen_cfg; unsigned int ext_cfg; + unsigned int int_en; + unsigned int poll_ptime_cfg; }; struct adp5585_info { @@ -150,7 +175,30 @@ struct adp5585_info { struct adp5585_dev { struct regmap *regmap; + struct device *dev; const struct adp5585_info *info; + /* Used to synchronize the availability of the event handlers */ + struct mutex ev_lock; + struct list_head ev_handlers; + int irq; + unsigned int ev_poll_time; }; +static inline void adp5585_ev_handler_remove(void *data) +{ + struct adp5585_ev_handler *handler = data; + struct adp5585_dev *adp5585 = dev_get_drvdata(handler->dev->parent); + + guard(mutex)(&adp5585->ev_lock); + list_del(&handler->entry); +} + +static inline int devm_adp5585_ev_handler_add(struct adp5585_dev *adp5585, + struct adp5585_ev_handler *handler) +{ + guard(mutex)(&adp5585->ev_lock); + list_add_tail(&handler->entry, &adp5585->ev_handlers); + return devm_add_action_or_reset(handler->dev, adp5585_ev_handler_remove, + handler); +} #endif