RE: [PATCH V5] mfd: da9062: add support for interrupt polarity defined in device tree

[Adam Thomson]

On 17 February 2020 00:44, Shreyas Joshi wrote:

> The da9062 interrupt handler cannot necessarily be low active.
> Add a function to configure the interrupt type based on what is defined in the
> device tree.
> The allowable interrupt type is either low or high level trigger.
> 
> Signed-off-by: Shreyas Joshi <shreyas.joshi@xxxxxxxxx>
> ---

This is V5 of the patch and there's still no revision history information here.
Please add this in the future when submitting patches as it helps reviewers
understand (or reminds them) what has come before.

>  drivers/mfd/da9062-core.c | 43 ++++++++++++++++++++++++++++++++++++--
> -
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
> index 419c73533401..cd3c4c80699e 100644
> --- a/drivers/mfd/da9062-core.c
> +++ b/drivers/mfd/da9062-core.c
> @@ -21,6 +21,9 @@
>  #define	DA9062_REG_EVENT_B_OFFSET	1
>  #define	DA9062_REG_EVENT_C_OFFSET	2
> 
> +#define	DA9062_IRQ_LOW	0
> +#define	DA9062_IRQ_HIGH	1
> +
>  static struct regmap_irq da9061_irqs[] = {
>  	/* EVENT A */
>  	[DA9061_IRQ_ONKEY] = {
> @@ -369,6 +372,33 @@ static int da9062_get_device_type(struct da9062 *chip)
>  	return ret;
>  }
> 
> +static u32 da9062_configure_irq_type(struct da9062 *chip, int irq, u32 *trigger)
> +{
> +	u32 irq_type = 0;
> +	struct irq_data *irq_data = irq_get_irq_data(irq);
> +
> +	if (!irq_data) {
> +		dev_err(chip->dev, "Invalid IRQ: %d\n", irq);
> +		return -EINVAL;
> +	}
> +	*trigger = irqd_get_trigger_type(irq_data);
> +
> +	switch (*trigger) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		irq_type = DA9062_IRQ_HIGH;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		irq_type = DA9062_IRQ_LOW;
> +		break;
> +	default:
> +		dev_warn(chip->dev, "Unsupported IRQ type: %d\n", *trigger);
> +		return -EINVAL;
> +	}
> +	return regmap_update_bits(chip->regmap, DA9062AA_CONFIG_A,
> +			DA9062AA_IRQ_TYPE_MASK,
> +			irq_type << DA9062AA_IRQ_TYPE_SHIFT);
> +}
> +
>  static const struct regmap_range da9061_aa_readable_ranges[] = {
>  	regmap_reg_range(DA9062AA_PAGE_CON, DA9062AA_STATUS_B),
>  	regmap_reg_range(DA9062AA_STATUS_D, DA9062AA_EVENT_C),
> @@ -417,6 +447,7 @@ static const struct regmap_range
> da9061_aa_writeable_ranges[] = {
>  	regmap_reg_range(DA9062AA_VBUCK1_A, DA9062AA_VBUCK4_A),
>  	regmap_reg_range(DA9062AA_VBUCK3_A, DA9062AA_VBUCK3_A),
>  	regmap_reg_range(DA9062AA_VLDO1_A, DA9062AA_VLDO4_A),
> +	regmap_reg_range(DA9062AA_CONFIG_A, DA9062AA_CONFIG_B),

Hmm. I don't believe we need access to CONFIG_B here do we? Just CONFIG_A to
configure polarity. Also we will need this register to be part of the
aa_readable_ranges table for regmap_update_bits() to work I believe.

Otherwise I think this looks ok so will give the nod once these changes have
been made.

>  	regmap_reg_range(DA9062AA_VBUCK1_B, DA9062AA_VBUCK4_B),
>  	regmap_reg_range(DA9062AA_VBUCK3_B, DA9062AA_VBUCK3_B),
>  	regmap_reg_range(DA9062AA_VLDO1_B, DA9062AA_VLDO4_B),
> @@ -596,6 +627,7 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
>  	const struct regmap_irq_chip *irq_chip;
>  	const struct regmap_config *config;
>  	int cell_num;
> +	u32 trigger_type = 0;
>  	int ret;
> 
>  	chip = devm_kzalloc(&i2c->dev, sizeof(*chip), GFP_KERNEL);
> @@ -654,10 +686,15 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
>  	if (ret)
>  		return ret;
> 
> +	ret = da9062_configure_irq_type(chip, i2c->irq, &trigger_type);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "Failed to configure IRQ type\n");
> +		return ret;
> +	}
> +
>  	ret = regmap_add_irq_chip(chip->regmap, i2c->irq,
> -			IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED,
> -			-1, irq_chip,
> -			&chip->regmap_irq);
> +			trigger_type | IRQF_SHARED | IRQF_ONESHOT,
> +			-1, irq_chip, &chip->regmap_irq);
>  	if (ret) {
>  		dev_err(chip->dev, "Failed to request IRQ %d: %d\n",
>  			i2c->irq, ret);
> --
> 2.20.1