Re: [PATCH 4/6] mfd: max77759: modify irq configs

["André Draszik" <andre.draszik@linaro.org>]

Hi Amit,

Thanks for your patches to enable the charger!

> From: Amit Sunil Dhamne <amitsd@google.com>
> 
> Define specific bit-level masks for charger's registers and modify the
> irq mask for charger irq_chip. Also, configure the max77759 interrupt
> lines as active low to all interrupt registrations to ensure the
> interrupt controllers are configured with the correct trigger type.
> 
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> ---
>  drivers/mfd/max77759.c       | 24 +++++++++++++++++-------
>  include/linux/mfd/max77759.h |  9 +++++++++
>  2 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mfd/max77759.c b/drivers/mfd/max77759.c
> index 6cf6306c4a3b..5fe22884f362 100644
> --- a/drivers/mfd/max77759.c
> +++ b/drivers/mfd/max77759.c
> @@ -256,8 +256,17 @@ static const struct regmap_irq max77759_topsys_irqs[] = {
>  };
>  
>  static const struct regmap_irq max77759_chgr_irqs[] = {
> -	REGMAP_IRQ_REG(MAX77759_CHARGER_INT_1, 0, GENMASK(7, 0)),
> -	REGMAP_IRQ_REG(MAX77759_CHARGER_INT_2, 1, GENMASK(7, 0)),
> +	REGMAP_IRQ_REG(MAX77759_CHARGER_INT_1, 0,
> +		       MAX77759_CHGR_REG_CHG_INT_AICL |
> +		       MAX77759_CHGR_REG_CHG_INT_CHGIN |
> +		       MAX77759_CHGR_REG_CHG_INT_CHG |
> +		       MAX77759_CHGR_REG_CHG_INT_INLIM),
> +	REGMAP_IRQ_REG(MAX77759_CHARGER_INT_2, 1,
> +		       MAX77759_CHGR_REG_CHG_INT2_BAT_OILO |
> +		       MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CC |
> +		       MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CV |
> +		       MAX77759_CHGR_REG_CHG_INT2_CHG_STA_TO |
> +		       MAX77759_CHGR_REG_CHG_INT2_CHG_STA_DONE),
>  };
>  
>  static const struct regmap_irq_chip max77759_pmic_irq_chip = {
> @@ -486,8 +495,8 @@ static int max77759_add_chained_irq_chip(struct device *dev,
>  				     "failed to get parent vIRQ(%d) for chip %s\n",
>  				     pirq, chip->name);
>  
> -	ret = devm_regmap_add_irq_chip(dev, regmap, irq,
> -				       IRQF_ONESHOT | IRQF_SHARED, 0, chip,
> +	ret = devm_regmap_add_irq_chip(dev, regmap, irq, IRQF_ONESHOT |
> +				       IRQF_SHARED | IRQF_TRIGGER_LOW, 0, chip,
>  				       data);

Please correct me if I'm wrong, but I don't think this makes sense for a
chained IRQ in this case. What problem does this change fix?

>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to add %s IRQ chip\n",
> @@ -519,8 +528,9 @@ static int max77759_add_chained_maxq(struct i2c_client *client,
>  
>  	ret = devm_request_threaded_irq(&client->dev, apcmdres_irq,
>  					NULL, apcmdres_irq_handler,
> -					IRQF_ONESHOT | IRQF_SHARED,
> -					dev_name(&client->dev), max77759);
> +					IRQF_ONESHOT | IRQF_SHARED |
> +					IRQF_TRIGGER_LOW, dev_name(&client->dev),
> +					max77759);

dito.

>  	if (ret)
>  		return dev_err_probe(&client->dev, ret,
>  				     "MAX77759_MAXQ_INT_APCMDRESI failed\n");
> @@ -633,7 +643,7 @@ static int max77759_probe(struct i2c_client *client)
>  		return dev_err_probe(&client->dev, -EINVAL,
>  				     "invalid IRQ: %d\n", client->irq);
>  
> -	irq_flags = IRQF_ONESHOT | IRQF_SHARED;
> +	irq_flags = IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW;

I don't believe IRQF_TRIGGER_LOW should be added here, as this is board-specific.
The polarity is meant to be set via DT (and the only current user of this driver
does so).

>  	irq_flags |= irqd_get_trigger_type(irq_data);

That's what gets us the config from DT.

>  
>  	ret = devm_regmap_add_irq_chip(&client->dev, max77759->regmap_top,
> diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h
> index c6face34e385..0ef29a48deec 100644
> --- a/include/linux/mfd/max77759.h
> +++ b/include/linux/mfd/max77759.h
> @@ -62,7 +62,16 @@
>  #define MAX77759_CHGR_REG_CHG_INT               0xb0
>  #define MAX77759_CHGR_REG_CHG_INT2              0xb1
>  #define MAX77759_CHGR_REG_CHG_INT_MASK          0xb2
> +#define MAX77759_CHGR_REG_CHG_INT_AICL          BIT(7)
> +#define MAX77759_CHGR_REG_CHG_INT_CHGIN         BIT(6)
> +#define MAX77759_CHGR_REG_CHG_INT_CHG           BIT(4)
> +#define MAX77759_CHGR_REG_CHG_INT_INLIM         BIT(2)
>  #define MAX77759_CHGR_REG_CHG_INT2_MASK         0xb3
> +#define MAX77759_CHGR_REG_CHG_INT2_BAT_OILO     BIT(4)
> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CC   BIT(3)
> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CV   BIT(2)
> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_TO   BIT(1)
> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_DONE BIT(0)

Even if wireless out of scope, it'd still be nice to add macros for
the remaining bits to make this complete and avoid having to update
these again in case wireless support gets added in the future.

Also, would be nice to keep existing style and indent the bits from
the registers (see existing bit definitions in this file a few lines
further up).

Finally, can you add the bits below the respective register (0xb0 / 0xb1)
please, to keep suffix meaningful, and to follow existing style for cases
like this (see MAX77759_MAXQ_REG_UIC_INT1).


Cheers,
Andre'