diff mbox series

[v5,02/10] mfd: mt6397: extract irq related code from core driver

Message ID 1566531931-9772-3-git-send-email-hsin-hsiung.wang@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Add Support for MediaTek PMIC MT6358 | expand

Commit Message

Hsin-Hsiung Wang Aug. 23, 2019, 3:45 a.m. UTC
In order to support different types of irq design, we decide to add
separate irq drivers for different design and keep mt6397 mfd core
simple and reusable to all generations of PMICs so far.

Acked-for-mfd-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
---
 drivers/mfd/Makefile            |   3 +-
 drivers/mfd/mt6397-core.c       | 146 --------------------------------
 drivers/mfd/mt6397-irq.c        | 181 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/mt6397/core.h |   9 ++
 4 files changed, 192 insertions(+), 147 deletions(-)
 create mode 100644 drivers/mfd/mt6397-irq.c

Comments

Frank Wunderlich Aug. 23, 2019, 12:13 p.m. UTC | #1
Hi,

this commit breaks mt6323 pmic on BananaPi-R2

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=a4872e80ce7d2a1844328176dbf279d0a2b89bdb

resulting in this message in dmesg:

mt6397 1000d000.pwrap:mt6323: unsupported chip: 0x0
and multiple
mtk-cpufreq mtk-cpufreq: failed to initialize dvfs info for cpu0

see discussion here:
http://lists.infradead.org/pipermail/linux-mediatek/2019-August/022505.html

after reverting this one the errors are gone, please provide a fix

regards Frank


> Gesendet: Freitag, 23. August 2019 um 05:45 Uhr
> Von: "Hsin-Hsiung Wang" <hsin-hsiung.wang@mediatek.com>
> Betreff: [PATCH v5 02/10] mfd: mt6397: extract irq related code from core driver
>
> In order to support different types of irq design, we decide to add
> separate irq drivers for different design and keep mt6397 mfd core
> simple and reusable to all generations of PMICs so far.
>
> Acked-for-mfd-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
> ---
>  drivers/mfd/Makefile            |   3 +-
>  drivers/mfd/mt6397-core.c       | 146 --------------------------------
>  drivers/mfd/mt6397-irq.c        | 181 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/mt6397/core.h |   9 ++
>  4 files changed, 192 insertions(+), 147 deletions(-)
>  create mode 100644 drivers/mfd/mt6397-irq.c
Matthias Brugger Aug. 23, 2019, 2:53 p.m. UTC | #2
On 23/08/2019 05:45, Hsin-Hsiung Wang wrote:
> In order to support different types of irq design, we decide to add
> separate irq drivers for different design and keep mt6397 mfd core
> simple and reusable to all generations of PMICs so far.
> 
> Acked-for-mfd-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
> ---
>  drivers/mfd/Makefile            |   3 +-
>  drivers/mfd/mt6397-core.c       | 146 --------------------------------
>  drivers/mfd/mt6397-irq.c        | 181 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/mt6397/core.h |   9 ++
>  4 files changed, 192 insertions(+), 147 deletions(-)
>  create mode 100644 drivers/mfd/mt6397-irq.c
> 
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f026ada..9a96325 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -241,7 +241,8 @@ obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC)	+= intel_soc_pmic_bxtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC)	+= intel_soc_pmic_chtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI)	+= intel_soc_pmic_chtdc_ti.o
> -obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
> +mt6397-objs	:= mt6397-core.o mt6397-irq.o
> +obj-$(CONFIG_MFD_MT6397)	+= mt6397.o
>  
>  obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
>  obj-$(CONFIG_MFD_ALTERA_SYSMGR) += altera-sysmgr.o
> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> index c070862..93c8881 100644
> --- a/drivers/mfd/mt6397-core.c
> +++ b/drivers/mfd/mt6397-core.c
> @@ -18,10 +18,6 @@
>  #define MT6397_RTC_BASE		0xe000
>  #define MT6397_RTC_SIZE		0x3e
>  
> -#define MT6323_CHIP_ID		0x23
> -#define MT6391_CHIP_ID		0x91
> -#define MT6397_CHIP_ID		0x97
> -
>  static const struct resource mt6397_rtc_resources[] = {
>  	{
>  		.start = MT6397_RTC_BASE,
> @@ -86,148 +82,6 @@
>  	}
>  };
>  
> -static void mt6397_irq_lock(struct irq_data *data)
> -{
> -	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
> -
> -	mutex_lock(&mt6397->irqlock);
> -}
> -
> -static void mt6397_irq_sync_unlock(struct irq_data *data)
> -{
> -	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
> -
> -	regmap_write(mt6397->regmap, mt6397->int_con[0],
> -		     mt6397->irq_masks_cur[0]);
> -	regmap_write(mt6397->regmap, mt6397->int_con[1],
> -		     mt6397->irq_masks_cur[1]);
> -
> -	mutex_unlock(&mt6397->irqlock);
> -}
> -
> -static void mt6397_irq_disable(struct irq_data *data)
> -{
> -	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
> -	int shift = data->hwirq & 0xf;
> -	int reg = data->hwirq >> 4;
> -
> -	mt6397->irq_masks_cur[reg] &= ~BIT(shift);
> -}
> -
> -static void mt6397_irq_enable(struct irq_data *data)
> -{
> -	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
> -	int shift = data->hwirq & 0xf;
> -	int reg = data->hwirq >> 4;
> -
> -	mt6397->irq_masks_cur[reg] |= BIT(shift);
> -}
> -
> -#ifdef CONFIG_PM_SLEEP
> -static int mt6397_irq_set_wake(struct irq_data *irq_data, unsigned int on)
> -{
> -	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(irq_data);
> -	int shift = irq_data->hwirq & 0xf;
> -	int reg = irq_data->hwirq >> 4;
> -
> -	if (on)
> -		mt6397->wake_mask[reg] |= BIT(shift);
> -	else
> -		mt6397->wake_mask[reg] &= ~BIT(shift);
> -
> -	return 0;
> -}
> -#else
> -#define mt6397_irq_set_wake NULL
> -#endif
> -
> -static struct irq_chip mt6397_irq_chip = {
> -	.name = "mt6397-irq",
> -	.irq_bus_lock = mt6397_irq_lock,
> -	.irq_bus_sync_unlock = mt6397_irq_sync_unlock,
> -	.irq_enable = mt6397_irq_enable,
> -	.irq_disable = mt6397_irq_disable,
> -	.irq_set_wake = mt6397_irq_set_wake,
> -};
> -
> -static void mt6397_irq_handle_reg(struct mt6397_chip *mt6397, int reg,
> -		int irqbase)
> -{
> -	unsigned int status;
> -	int i, irq, ret;
> -
> -	ret = regmap_read(mt6397->regmap, reg, &status);
> -	if (ret) {
> -		dev_err(mt6397->dev, "Failed to read irq status: %d\n", ret);
> -		return;
> -	}
> -
> -	for (i = 0; i < 16; i++) {
> -		if (status & BIT(i)) {
> -			irq = irq_find_mapping(mt6397->irq_domain, irqbase + i);
> -			if (irq)
> -				handle_nested_irq(irq);
> -		}
> -	}
> -
> -	regmap_write(mt6397->regmap, reg, status);
> -}
> -
> -static irqreturn_t mt6397_irq_thread(int irq, void *data)
> -{
> -	struct mt6397_chip *mt6397 = data;
> -
> -	mt6397_irq_handle_reg(mt6397, mt6397->int_status[0], 0);
> -	mt6397_irq_handle_reg(mt6397, mt6397->int_status[1], 16);
> -
> -	return IRQ_HANDLED;
> -}
> -
> -static int mt6397_irq_domain_map(struct irq_domain *d, unsigned int irq,
> -					irq_hw_number_t hw)
> -{
> -	struct mt6397_chip *mt6397 = d->host_data;
> -
> -	irq_set_chip_data(irq, mt6397);
> -	irq_set_chip_and_handler(irq, &mt6397_irq_chip, handle_level_irq);
> -	irq_set_nested_thread(irq, 1);
> -	irq_set_noprobe(irq);
> -
> -	return 0;
> -}
> -
> -static const struct irq_domain_ops mt6397_irq_domain_ops = {
> -	.map = mt6397_irq_domain_map,
> -};
> -
> -static int mt6397_irq_init(struct mt6397_chip *mt6397)
> -{
> -	int ret;
> -
> -	mutex_init(&mt6397->irqlock);
> -
> -	/* Mask all interrupt sources */
> -	regmap_write(mt6397->regmap, mt6397->int_con[0], 0x0);
> -	regmap_write(mt6397->regmap, mt6397->int_con[1], 0x0);
> -
> -	mt6397->irq_domain = irq_domain_add_linear(mt6397->dev->of_node,
> -		MT6397_IRQ_NR, &mt6397_irq_domain_ops, mt6397);
> -	if (!mt6397->irq_domain) {
> -		dev_err(mt6397->dev, "could not create irq domain\n");
> -		return -ENOMEM;
> -	}
> -
> -	ret = devm_request_threaded_irq(mt6397->dev, mt6397->irq, NULL,
> -		mt6397_irq_thread, IRQF_ONESHOT, "mt6397-pmic", mt6397);
> -	if (ret) {
> -		dev_err(mt6397->dev, "failed to register irq=%d; err: %d\n",
> -			mt6397->irq, ret);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  #ifdef CONFIG_PM_SLEEP
>  static int mt6397_irq_suspend(struct device *dev)
>  {
> diff --git a/drivers/mfd/mt6397-irq.c b/drivers/mfd/mt6397-irq.c
> new file mode 100644
> index 0000000..b2d3ce1
> --- /dev/null
> +++ b/drivers/mfd/mt6397-irq.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2019 MediaTek Inc.
> +
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/mt6323/core.h>
> +#include <linux/mfd/mt6323/registers.h>
> +#include <linux/mfd/mt6397/core.h>
> +#include <linux/mfd/mt6397/registers.h>
> +
> +static void mt6397_irq_lock(struct irq_data *data)
> +{
> +	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
> +
> +	mutex_lock(&mt6397->irqlock);
> +}
> +
> +static void mt6397_irq_sync_unlock(struct irq_data *data)
> +{
> +	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
> +
> +	regmap_write(mt6397->regmap, mt6397->int_con[0],
> +		     mt6397->irq_masks_cur[0]);
> +	regmap_write(mt6397->regmap, mt6397->int_con[1],
> +		     mt6397->irq_masks_cur[1]);
> +
> +	mutex_unlock(&mt6397->irqlock);
> +}
> +
> +static void mt6397_irq_disable(struct irq_data *data)
> +{
> +	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
> +	int shift = data->hwirq & 0xf;
> +	int reg = data->hwirq >> 4;
> +
> +	mt6397->irq_masks_cur[reg] &= ~BIT(shift);
> +}
> +
> +static void mt6397_irq_enable(struct irq_data *data)
> +{
> +	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
> +	int shift = data->hwirq & 0xf;
> +	int reg = data->hwirq >> 4;
> +
> +	mt6397->irq_masks_cur[reg] |= BIT(shift);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mt6397_irq_set_wake(struct irq_data *irq_data, unsigned int on)
> +{
> +	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(irq_data);
> +	int shift = irq_data->hwirq & 0xf;
> +	int reg = irq_data->hwirq >> 4;
> +
> +	if (on)
> +		mt6397->wake_mask[reg] |= BIT(shift);
> +	else
> +		mt6397->wake_mask[reg] &= ~BIT(shift);
> +
> +	return 0;
> +}
> +#else
> +#define mt6397_irq_set_wake NULL
> +#endif
> +
> +static struct irq_chip mt6397_irq_chip = {
> +	.name = "mt6397-irq",
> +	.irq_bus_lock = mt6397_irq_lock,
> +	.irq_bus_sync_unlock = mt6397_irq_sync_unlock,
> +	.irq_enable = mt6397_irq_enable,
> +	.irq_disable = mt6397_irq_disable,
> +	.irq_set_wake = mt6397_irq_set_wake,
> +};
> +
> +static void mt6397_irq_handle_reg(struct mt6397_chip *mt6397, int reg,
> +				  int irqbase)
> +{
> +	unsigned int status;
> +	int i, irq, ret;
> +
> +	ret = regmap_read(mt6397->regmap, reg, &status);
> +	if (ret) {
> +		dev_err(mt6397->dev, "Failed to read irq status: %d\n", ret);
> +		return;
> +	}
> +
> +	for (i = 0; i < 16; i++) {
> +		if (status & BIT(i)) {
> +			irq = irq_find_mapping(mt6397->irq_domain, irqbase + i);
> +			if (irq)
> +				handle_nested_irq(irq);
> +		}
> +	}
> +
> +	regmap_write(mt6397->regmap, reg, status);
> +}
> +
> +static irqreturn_t mt6397_irq_thread(int irq, void *data)
> +{
> +	struct mt6397_chip *mt6397 = data;
> +
> +	mt6397_irq_handle_reg(mt6397, mt6397->int_status[0], 0);
> +	mt6397_irq_handle_reg(mt6397, mt6397->int_status[1], 16);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mt6397_irq_domain_map(struct irq_domain *d, unsigned int irq,
> +				 irq_hw_number_t hw)
> +{
> +	struct mt6397_chip *mt6397 = d->host_data;
> +
> +	irq_set_chip_data(irq, mt6397);
> +	irq_set_chip_and_handler(irq, &mt6397_irq_chip, handle_level_irq);
> +	irq_set_nested_thread(irq, 1);
> +	irq_set_noprobe(irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops mt6397_irq_domain_ops = {
> +	.map = mt6397_irq_domain_map,
> +};
> +
> +int mt6397_irq_init(struct mt6397_chip *chip)
> +{
> +	int ret;
> +
> +	mutex_init(&chip->irqlock);
> +
> +	switch (chip->chip_id) {
> +	case MT6323_CHIP_ID:
> +		chip->int_con[0] = MT6323_INT_CON0;
> +		chip->int_con[1] = MT6323_INT_CON1;
> +		chip->int_status[0] = MT6323_INT_STATUS0;
> +		chip->int_status[1] = MT6323_INT_STATUS1;
> +		break;
> +
> +	case MT6391_CHIP_ID:
> +	case MT6397_CHIP_ID:
> +		chip->int_con[0] = MT6397_INT_CON0;
> +		chip->int_con[1] = MT6397_INT_CON1;
> +		chip->int_status[0] = MT6397_INT_STATUS0;
> +		chip->int_status[1] = MT6397_INT_STATUS1;
> +		break;
> +

Just stumbled over this in linux-next. I personally would prefer to have two
patches, one that moves the code and another one that adds the switch etc.

This way it would be much easier to realize this change.
Not sure if this is still possible, because it seems to be in one of Lee's
repositories already. (and if Lee thinks the same as I do, of course)

Regards,
Matthias


> +	default:
> +		dev_err(chip->dev, "unsupported chip: 0x%x\n", chip->chip_id);
> +		return -ENODEV;
> +	}
> +
> +	/* Mask all interrupt sources */
> +	regmap_write(chip->regmap, chip->int_con[0], 0x0);
> +	regmap_write(chip->regmap, chip->int_con[1], 0x0);
> +
> +	chip->irq_domain = irq_domain_add_linear(chip->dev->of_node,
> +						 MT6397_IRQ_NR,
> +						 &mt6397_irq_domain_ops,
> +						 chip);
> +	if (!chip->irq_domain) {
> +		dev_err(chip->dev, "could not create irq domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = devm_request_threaded_irq(chip->dev, chip->irq, NULL,
> +					mt6397_irq_thread, IRQF_ONESHOT,
> +					"mt6397-pmic", chip);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to register irq=%d; err: %d\n",
> +			chip->irq, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> diff --git a/include/linux/mfd/mt6397/core.h b/include/linux/mfd/mt6397/core.h
> index 25a95e7..9320c2a 100644
> --- a/include/linux/mfd/mt6397/core.h
> +++ b/include/linux/mfd/mt6397/core.h
> @@ -7,6 +7,12 @@
>  #ifndef __MFD_MT6397_CORE_H__
>  #define __MFD_MT6397_CORE_H__
>  
> +enum chip_id {
> +	MT6323_CHIP_ID = 0x23,
> +	MT6391_CHIP_ID = 0x91,
> +	MT6397_CHIP_ID = 0x97,
> +};
> +
>  enum mt6397_irq_numbers {
>  	MT6397_IRQ_SPKL_AB = 0,
>  	MT6397_IRQ_SPKR_AB,
> @@ -54,6 +60,9 @@ struct mt6397_chip {
>  	u16 irq_masks_cache[2];
>  	u16 int_con[2];
>  	u16 int_status[2];
> +	u16 chip_id;
>  };
>  
> +int mt6397_irq_init(struct mt6397_chip *chip);
> +
>  #endif /* __MFD_MT6397_CORE_H__ */
>
Matthias Brugger Aug. 23, 2019, 2:56 p.m. UTC | #3
On 23/08/2019 14:13, Frank Wunderlich wrote:
> Hi,
> 
> this commit breaks mt6323 pmic on BananaPi-R2
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=a4872e80ce7d2a1844328176dbf279d0a2b89bdb
> 
> resulting in this message in dmesg:
> 
> mt6397 1000d000.pwrap:mt6323: unsupported chip: 0x0
> and multiple
> mtk-cpufreq mtk-cpufreq: failed to initialize dvfs info for cpu0
> 
> see discussion here:
> http://lists.infradead.org/pipermail/linux-mediatek/2019-August/022505.html
> 
> after reverting this one the errors are gone, please provide a fix

are you sure that you provide the correct chip_id here? I saw 0x2023 (if I
remember correctly), while this switch checks for 0x23, 0x91 and 0x97, so I'm
not sure if the problem really lies here. I didn't dig into the code to find out
how the chip_id is created.

Regards,
Matthias

> 
> regards Frank
> 
> 
>> Gesendet: Freitag, 23. August 2019 um 05:45 Uhr
>> Von: "Hsin-Hsiung Wang" <hsin-hsiung.wang@mediatek.com>
>> Betreff: [PATCH v5 02/10] mfd: mt6397: extract irq related code from core driver
>>
>> In order to support different types of irq design, we decide to add
>> separate irq drivers for different design and keep mt6397 mfd core
>> simple and reusable to all generations of PMICs so far.
>>
>> Acked-for-mfd-by: Lee Jones <lee.jones@linaro.org>
>> Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
>> ---
>>  drivers/mfd/Makefile            |   3 +-
>>  drivers/mfd/mt6397-core.c       | 146 --------------------------------
>>  drivers/mfd/mt6397-irq.c        | 181 ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/mt6397/core.h |   9 ++
>>  4 files changed, 192 insertions(+), 147 deletions(-)
>>  create mode 100644 drivers/mfd/mt6397-irq.c
>
Frank Wunderlich Aug. 23, 2019, 3:26 p.m. UTC | #4
Am 23. August 2019 16:56:13 MESZ schrieb Matthias Brugger <matthias.bgg@gmail.com>:
>are you sure that you provide the correct chip_id here? I saw 0x2023
>(if I
>remember correctly), while this switch checks for 0x23, 0x91 and 0x97,
>so I'm
>not sure if the problem really lies here. I didn't dig into the code to
>find out
>how the chip_id is created.

The chip-id 0x2023 is reported with 5.3-rc5, next-code says 0x0. So i guess the chipid is read out/calculated the wrong way. If calculation is not changed the read is changed compared to 5.3
Frank Wunderlich Aug. 23, 2019, 3:35 p.m. UTC | #5
As far as i understand does old init-function not rely on the chip-id, so it seems that with this commit a prior bug is shown.
maybe the chip-id (should be 0x23 like constant) is set later after irq-request or completely missing for mt6323
Matthias Brugger Aug. 23, 2019, 3:42 p.m. UTC | #6
On 23/08/2019 17:26, Frank Wunderlich wrote:
> 
> 
> Am 23. August 2019 16:56:13 MESZ schrieb Matthias Brugger <matthias.bgg@gmail.com>:
>> are you sure that you provide the correct chip_id here? I saw 0x2023
>> (if I
>> remember correctly), while this switch checks for 0x23, 0x91 and 0x97,
>> so I'm
>> not sure if the problem really lies here. I didn't dig into the code to
>> find out
>> how the chip_id is created.
> 
> The chip-id 0x2023 is reported with 5.3-rc5, next-code says 0x0. So i guess the chipid is read out/calculated the wrong way. If calculation is not changed the read is changed compared to 5.3
> 

I suppose that's because 3/10 has code that should be in 2/10 and for some
reason 3/10 was not pushed for linux-next inclusion. Although it has the same
Acked-for-mfd-by tag.

@Frank, can you test if adding 3/10 to your code base fixes the issue?

Regards,
Matthias
Frank Wunderlich Aug. 23, 2019, 3:53 p.m. UTC | #7
Seems chip-id in 5.3 is read here

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/mt6397-core.c?h=v5.3-rc5#n282

It is before platform_get_irq which may call irq-init changed in the problematic commit.

I will add a dev_info here in next-code to see value of id
Frank Wunderlich Aug. 23, 2019, 5:16 p.m. UTC | #8
> Gesendet: Freitag, 23. August 2019 um 17:42 Uhr
> Von: "Matthias Brugger" <matthias.bgg@gmail.com>

> I suppose that's because 3/10 has code that should be in 2/10 and for some
> reason 3/10 was not pushed for linux-next inclusion. Although it has the same
> Acked-for-mfd-by tag.
>
> @Frank, can you test if adding 3/10 to your code base fixes the issue?

adding part 3 [1] seems to fix the issue too

[    4.960051] mt6323-regulator mt6323-regulator: Chip ID = 0x2023

thanks

[1] https://patchwork.kernel.org/patch/11110509/
Hsin-Hsiung Wang Aug. 29, 2019, 6:24 a.m. UTC | #9
Hi Frank/Matthias,

On Fri, 2019-08-23 at 19:16 +0200, Frank Wunderlich wrote:
> > Gesendet: Freitag, 23. August 2019 um 17:42 Uhr
> > Von: "Matthias Brugger" <matthias.bgg@gmail.com>
> 
> > I suppose that's because 3/10 has code that should be in 2/10 and for some
> > reason 3/10 was not pushed for linux-next inclusion. Although it has the same
> > Acked-for-mfd-by tag.
> >
> > @Frank, can you test if adding 3/10 to your code base fixes the issue?
> 
> adding part 3 [1] seems to fix the issue too
> 
> [    4.960051] mt6323-regulator mt6323-regulator: Chip ID = 0x2023
> 
> thanks
> 
> [1] https://patchwork.kernel.org/patch/11110509/
Thanks for your comments.
The root cause seems I didn't split the code well.
I will fix it in the next version.
Frank Wunderlich Sept. 19, 2019, 6:59 p.m. UTC | #10
Hi

When is new version ready? First 2 patches are still in next for 5.4 and i see no fix so i guess it is still broken.

Regards Frank

Am 29. August 2019 08:24:36 MESZ schrieb Hsin-hsiung Wang <hsin-hsiung.wang@mediatek.com>:
>Hi Frank/Matthias,
>
>Thanks for your comments.
>The root cause seems I didn't split the code well.
>I will fix it in the next version.
Frank Wunderlich Sept. 30, 2019, 7:44 p.m. UTC | #11
Hi,

bug is still present in 5.4-rc1

dmesg prints this line and at least switch is not inialized on bananapi-r2

mt6397 1000d000.pwrap:mt6323: unsupported chip: 0x0

regards Frank


> Gesendet: Donnerstag, 29. August 2019 um 08:24 Uhr
> Von: "Hsin-hsiung Wang" <hsin-hsiung.wang@mediatek.com>
> An: "Frank Wunderlich" <frank-w@public-files.de>, "Matthias Brugger" <matthias.bgg@gmail.com>
> Cc: linux-mediatek@lists.infradead.org, "Mark Rutland" <mark.rutland@arm.com>, "Alessandro Zummo" <a.zummo@towertech.it>, "Alexandre Belloni" <alexandre.belloni@bootlin.com>, srv_heupstream@mediatek.com, devicetree@vger.kernel.org, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Sean Wang" <sean.wang@mediatek.com>, "Liam Girdwood" <lgirdwood@gmail.com>, "Rob Herring" <robh+dt@kernel.org>, linux-kernel@vger.kernel.org, "Richard Fontana" <rfontana@redhat.com>, "Mark Brown" <broonie@kernel.org>, linux-arm-kernel@lists.infradead.org, "René van Dorst" <opensource@vdorst.com>, "Thomas Gleixner" <tglx@linutronix.de>, "Eddie Huang" <eddie.huang@mediatek.com>, "Lee Jones" <lee.jones@linaro.org>, "Kate Stewart" <kstewart@linuxfoundation.org>, linux-rtc@vger.kernel.org
> Betreff: Re: Aw: Re: [BUG] [PATCH v5 02/10] mfd: mt6397: extract irq related code from core driver
>
> Hi Frank/Matthias,
> 
> On Fri, 2019-08-23 at 19:16 +0200, Frank Wunderlich wrote:
> > > Gesendet: Freitag, 23. August 2019 um 17:42 Uhr
> > > Von: "Matthias Brugger" <matthias.bgg@gmail.com>
> > 
> > > I suppose that's because 3/10 has code that should be in 2/10 and for some
> > > reason 3/10 was not pushed for linux-next inclusion. Although it has the same
> > > Acked-for-mfd-by tag.
> > >
> > > @Frank, can you test if adding 3/10 to your code base fixes the issue?
> > 
> > adding part 3 [1] seems to fix the issue too
> > 
> > [    4.960051] mt6323-regulator mt6323-regulator: Chip ID = 0x2023
> > 
> > thanks
> > 
> > [1] https://patchwork.kernel.org/patch/11110509/
> Thanks for your comments.
> The root cause seems I didn't split the code well.
> I will fix it in the next version.
> 
>
diff mbox series

Patch

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index f026ada..9a96325 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -241,7 +241,8 @@  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
 obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC)	+= intel_soc_pmic_bxtwc.o
 obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC)	+= intel_soc_pmic_chtwc.o
 obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI)	+= intel_soc_pmic_chtdc_ti.o
-obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
+mt6397-objs	:= mt6397-core.o mt6397-irq.o
+obj-$(CONFIG_MFD_MT6397)	+= mt6397.o
 
 obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
 obj-$(CONFIG_MFD_ALTERA_SYSMGR) += altera-sysmgr.o
diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
index c070862..93c8881 100644
--- a/drivers/mfd/mt6397-core.c
+++ b/drivers/mfd/mt6397-core.c
@@ -18,10 +18,6 @@ 
 #define MT6397_RTC_BASE		0xe000
 #define MT6397_RTC_SIZE		0x3e
 
-#define MT6323_CHIP_ID		0x23
-#define MT6391_CHIP_ID		0x91
-#define MT6397_CHIP_ID		0x97
-
 static const struct resource mt6397_rtc_resources[] = {
 	{
 		.start = MT6397_RTC_BASE,
@@ -86,148 +82,6 @@ 
 	}
 };
 
-static void mt6397_irq_lock(struct irq_data *data)
-{
-	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
-
-	mutex_lock(&mt6397->irqlock);
-}
-
-static void mt6397_irq_sync_unlock(struct irq_data *data)
-{
-	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
-
-	regmap_write(mt6397->regmap, mt6397->int_con[0],
-		     mt6397->irq_masks_cur[0]);
-	regmap_write(mt6397->regmap, mt6397->int_con[1],
-		     mt6397->irq_masks_cur[1]);
-
-	mutex_unlock(&mt6397->irqlock);
-}
-
-static void mt6397_irq_disable(struct irq_data *data)
-{
-	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
-	int shift = data->hwirq & 0xf;
-	int reg = data->hwirq >> 4;
-
-	mt6397->irq_masks_cur[reg] &= ~BIT(shift);
-}
-
-static void mt6397_irq_enable(struct irq_data *data)
-{
-	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
-	int shift = data->hwirq & 0xf;
-	int reg = data->hwirq >> 4;
-
-	mt6397->irq_masks_cur[reg] |= BIT(shift);
-}
-
-#ifdef CONFIG_PM_SLEEP
-static int mt6397_irq_set_wake(struct irq_data *irq_data, unsigned int on)
-{
-	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(irq_data);
-	int shift = irq_data->hwirq & 0xf;
-	int reg = irq_data->hwirq >> 4;
-
-	if (on)
-		mt6397->wake_mask[reg] |= BIT(shift);
-	else
-		mt6397->wake_mask[reg] &= ~BIT(shift);
-
-	return 0;
-}
-#else
-#define mt6397_irq_set_wake NULL
-#endif
-
-static struct irq_chip mt6397_irq_chip = {
-	.name = "mt6397-irq",
-	.irq_bus_lock = mt6397_irq_lock,
-	.irq_bus_sync_unlock = mt6397_irq_sync_unlock,
-	.irq_enable = mt6397_irq_enable,
-	.irq_disable = mt6397_irq_disable,
-	.irq_set_wake = mt6397_irq_set_wake,
-};
-
-static void mt6397_irq_handle_reg(struct mt6397_chip *mt6397, int reg,
-		int irqbase)
-{
-	unsigned int status;
-	int i, irq, ret;
-
-	ret = regmap_read(mt6397->regmap, reg, &status);
-	if (ret) {
-		dev_err(mt6397->dev, "Failed to read irq status: %d\n", ret);
-		return;
-	}
-
-	for (i = 0; i < 16; i++) {
-		if (status & BIT(i)) {
-			irq = irq_find_mapping(mt6397->irq_domain, irqbase + i);
-			if (irq)
-				handle_nested_irq(irq);
-		}
-	}
-
-	regmap_write(mt6397->regmap, reg, status);
-}
-
-static irqreturn_t mt6397_irq_thread(int irq, void *data)
-{
-	struct mt6397_chip *mt6397 = data;
-
-	mt6397_irq_handle_reg(mt6397, mt6397->int_status[0], 0);
-	mt6397_irq_handle_reg(mt6397, mt6397->int_status[1], 16);
-
-	return IRQ_HANDLED;
-}
-
-static int mt6397_irq_domain_map(struct irq_domain *d, unsigned int irq,
-					irq_hw_number_t hw)
-{
-	struct mt6397_chip *mt6397 = d->host_data;
-
-	irq_set_chip_data(irq, mt6397);
-	irq_set_chip_and_handler(irq, &mt6397_irq_chip, handle_level_irq);
-	irq_set_nested_thread(irq, 1);
-	irq_set_noprobe(irq);
-
-	return 0;
-}
-
-static const struct irq_domain_ops mt6397_irq_domain_ops = {
-	.map = mt6397_irq_domain_map,
-};
-
-static int mt6397_irq_init(struct mt6397_chip *mt6397)
-{
-	int ret;
-
-	mutex_init(&mt6397->irqlock);
-
-	/* Mask all interrupt sources */
-	regmap_write(mt6397->regmap, mt6397->int_con[0], 0x0);
-	regmap_write(mt6397->regmap, mt6397->int_con[1], 0x0);
-
-	mt6397->irq_domain = irq_domain_add_linear(mt6397->dev->of_node,
-		MT6397_IRQ_NR, &mt6397_irq_domain_ops, mt6397);
-	if (!mt6397->irq_domain) {
-		dev_err(mt6397->dev, "could not create irq domain\n");
-		return -ENOMEM;
-	}
-
-	ret = devm_request_threaded_irq(mt6397->dev, mt6397->irq, NULL,
-		mt6397_irq_thread, IRQF_ONESHOT, "mt6397-pmic", mt6397);
-	if (ret) {
-		dev_err(mt6397->dev, "failed to register irq=%d; err: %d\n",
-			mt6397->irq, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
 #ifdef CONFIG_PM_SLEEP
 static int mt6397_irq_suspend(struct device *dev)
 {
diff --git a/drivers/mfd/mt6397-irq.c b/drivers/mfd/mt6397-irq.c
new file mode 100644
index 0000000..b2d3ce1
--- /dev/null
+++ b/drivers/mfd/mt6397-irq.c
@@ -0,0 +1,181 @@ 
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2019 MediaTek Inc.
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/mt6323/core.h>
+#include <linux/mfd/mt6323/registers.h>
+#include <linux/mfd/mt6397/core.h>
+#include <linux/mfd/mt6397/registers.h>
+
+static void mt6397_irq_lock(struct irq_data *data)
+{
+	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
+
+	mutex_lock(&mt6397->irqlock);
+}
+
+static void mt6397_irq_sync_unlock(struct irq_data *data)
+{
+	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
+
+	regmap_write(mt6397->regmap, mt6397->int_con[0],
+		     mt6397->irq_masks_cur[0]);
+	regmap_write(mt6397->regmap, mt6397->int_con[1],
+		     mt6397->irq_masks_cur[1]);
+
+	mutex_unlock(&mt6397->irqlock);
+}
+
+static void mt6397_irq_disable(struct irq_data *data)
+{
+	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
+	int shift = data->hwirq & 0xf;
+	int reg = data->hwirq >> 4;
+
+	mt6397->irq_masks_cur[reg] &= ~BIT(shift);
+}
+
+static void mt6397_irq_enable(struct irq_data *data)
+{
+	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(data);
+	int shift = data->hwirq & 0xf;
+	int reg = data->hwirq >> 4;
+
+	mt6397->irq_masks_cur[reg] |= BIT(shift);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mt6397_irq_set_wake(struct irq_data *irq_data, unsigned int on)
+{
+	struct mt6397_chip *mt6397 = irq_data_get_irq_chip_data(irq_data);
+	int shift = irq_data->hwirq & 0xf;
+	int reg = irq_data->hwirq >> 4;
+
+	if (on)
+		mt6397->wake_mask[reg] |= BIT(shift);
+	else
+		mt6397->wake_mask[reg] &= ~BIT(shift);
+
+	return 0;
+}
+#else
+#define mt6397_irq_set_wake NULL
+#endif
+
+static struct irq_chip mt6397_irq_chip = {
+	.name = "mt6397-irq",
+	.irq_bus_lock = mt6397_irq_lock,
+	.irq_bus_sync_unlock = mt6397_irq_sync_unlock,
+	.irq_enable = mt6397_irq_enable,
+	.irq_disable = mt6397_irq_disable,
+	.irq_set_wake = mt6397_irq_set_wake,
+};
+
+static void mt6397_irq_handle_reg(struct mt6397_chip *mt6397, int reg,
+				  int irqbase)
+{
+	unsigned int status;
+	int i, irq, ret;
+
+	ret = regmap_read(mt6397->regmap, reg, &status);
+	if (ret) {
+		dev_err(mt6397->dev, "Failed to read irq status: %d\n", ret);
+		return;
+	}
+
+	for (i = 0; i < 16; i++) {
+		if (status & BIT(i)) {
+			irq = irq_find_mapping(mt6397->irq_domain, irqbase + i);
+			if (irq)
+				handle_nested_irq(irq);
+		}
+	}
+
+	regmap_write(mt6397->regmap, reg, status);
+}
+
+static irqreturn_t mt6397_irq_thread(int irq, void *data)
+{
+	struct mt6397_chip *mt6397 = data;
+
+	mt6397_irq_handle_reg(mt6397, mt6397->int_status[0], 0);
+	mt6397_irq_handle_reg(mt6397, mt6397->int_status[1], 16);
+
+	return IRQ_HANDLED;
+}
+
+static int mt6397_irq_domain_map(struct irq_domain *d, unsigned int irq,
+				 irq_hw_number_t hw)
+{
+	struct mt6397_chip *mt6397 = d->host_data;
+
+	irq_set_chip_data(irq, mt6397);
+	irq_set_chip_and_handler(irq, &mt6397_irq_chip, handle_level_irq);
+	irq_set_nested_thread(irq, 1);
+	irq_set_noprobe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops mt6397_irq_domain_ops = {
+	.map = mt6397_irq_domain_map,
+};
+
+int mt6397_irq_init(struct mt6397_chip *chip)
+{
+	int ret;
+
+	mutex_init(&chip->irqlock);
+
+	switch (chip->chip_id) {
+	case MT6323_CHIP_ID:
+		chip->int_con[0] = MT6323_INT_CON0;
+		chip->int_con[1] = MT6323_INT_CON1;
+		chip->int_status[0] = MT6323_INT_STATUS0;
+		chip->int_status[1] = MT6323_INT_STATUS1;
+		break;
+
+	case MT6391_CHIP_ID:
+	case MT6397_CHIP_ID:
+		chip->int_con[0] = MT6397_INT_CON0;
+		chip->int_con[1] = MT6397_INT_CON1;
+		chip->int_status[0] = MT6397_INT_STATUS0;
+		chip->int_status[1] = MT6397_INT_STATUS1;
+		break;
+
+	default:
+		dev_err(chip->dev, "unsupported chip: 0x%x\n", chip->chip_id);
+		return -ENODEV;
+	}
+
+	/* Mask all interrupt sources */
+	regmap_write(chip->regmap, chip->int_con[0], 0x0);
+	regmap_write(chip->regmap, chip->int_con[1], 0x0);
+
+	chip->irq_domain = irq_domain_add_linear(chip->dev->of_node,
+						 MT6397_IRQ_NR,
+						 &mt6397_irq_domain_ops,
+						 chip);
+	if (!chip->irq_domain) {
+		dev_err(chip->dev, "could not create irq domain\n");
+		return -ENOMEM;
+	}
+
+	ret = devm_request_threaded_irq(chip->dev, chip->irq, NULL,
+					mt6397_irq_thread, IRQF_ONESHOT,
+					"mt6397-pmic", chip);
+	if (ret) {
+		dev_err(chip->dev, "failed to register irq=%d; err: %d\n",
+			chip->irq, ret);
+		return ret;
+	}
+
+	return 0;
+}
diff --git a/include/linux/mfd/mt6397/core.h b/include/linux/mfd/mt6397/core.h
index 25a95e7..9320c2a 100644
--- a/include/linux/mfd/mt6397/core.h
+++ b/include/linux/mfd/mt6397/core.h
@@ -7,6 +7,12 @@ 
 #ifndef __MFD_MT6397_CORE_H__
 #define __MFD_MT6397_CORE_H__
 
+enum chip_id {
+	MT6323_CHIP_ID = 0x23,
+	MT6391_CHIP_ID = 0x91,
+	MT6397_CHIP_ID = 0x97,
+};
+
 enum mt6397_irq_numbers {
 	MT6397_IRQ_SPKL_AB = 0,
 	MT6397_IRQ_SPKR_AB,
@@ -54,6 +60,9 @@  struct mt6397_chip {
 	u16 irq_masks_cache[2];
 	u16 int_con[2];
 	u16 int_status[2];
+	u16 chip_id;
 };
 
+int mt6397_irq_init(struct mt6397_chip *chip);
+
 #endif /* __MFD_MT6397_CORE_H__ */