Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
On Thu, Nov 14, 2013 at 11:53:51AM +0100, Marek Szyprowski wrote: > On 2013-11-14 11:24, Mark Brown wrote: > >Modern systems should be able to use deferred probing to resolve these > >dependencies, subsys_initcall() is mostly there for legacy reasons and > >new systems ought to be able to move away from it. > The main problem here is usb gadget subsystem, which simply doesn't support > deferred probe yet. UDC driver (which uses those regulators) need to be > initialized before usb gadget driver (i.e. mass storage or cdc ethernet) > has been registered. OK, that makes sense - OK for the time being. signature.asc Description: Digital signature
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
Hello,
On 2013-11-14 11:24, Mark Brown wrote:
On Thu, Nov 14, 2013 at 10:33:22AM +0900, Kyungmin Park wrote:
> On Wed, Nov 13, 2013 at 10:13 PM, Mark Brown wrote:
> > Are you *positive* this is a falling triggered IRQ? All the code to do
> > with spinning reading the GPIO state during handling makes it look like
> > this is in fact an active low interrupt and a lot of the code in here is
> > working around trying to handle that as the wrong kind of IRQ.
> It's not work with level triggering. as wm8994, it requires edge
> triggering. previous time I send RFC patch to handle edge triggering
> at regmap.
No, wm8994 is level triggered only - the edge triggering stuff there is
to work around some SoCs that could only support edge triggering and not
level triggering. Is this a similar issue or is there something else
going on, what's the differeence with a level triggered scheme?
> >> +static int __init max14577_i2c_init(void)
> >> +{
> >> + return i2c_add_driver(_i2c_driver);
> >> +}
> >> +subsys_initcall(max14577_i2c_init);
> > Why not module_i2c_driver?
> there's ordering issue, it should provide regulator which is used
> others before USB probe. if not, it failed to use USB.
> Other PMICs use also subsys_initcall for this reason.
Modern systems should be able to use deferred probing to resolve these
dependencies, subsys_initcall() is mostly there for legacy reasons and
new systems ought to be able to move away from it.
The main problem here is usb gadget subsystem, which simply doesn't support
deferred probe yet. UDC driver (which uses those regulators) need to be
initialized before usb gadget driver (i.e. mass storage or cdc ethernet)
has been registered.
Best regards
--
Marek Szyprowski
Samsung R Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
On Thu, Nov 14, 2013 at 10:33:22AM +0900, Kyungmin Park wrote:
> On Wed, Nov 13, 2013 at 10:13 PM, Mark Brown wrote:
> > Are you *positive* this is a falling triggered IRQ? All the code to do
> > with spinning reading the GPIO state during handling makes it look like
> > this is in fact an active low interrupt and a lot of the code in here is
> > working around trying to handle that as the wrong kind of IRQ.
> It's not work with level triggering. as wm8994, it requires edge
> triggering. previous time I send RFC patch to handle edge triggering
> at regmap.
No, wm8994 is level triggered only - the edge triggering stuff there is
to work around some SoCs that could only support edge triggering and not
level triggering. Is this a similar issue or is there something else
going on, what's the differeence with a level triggered scheme?
> >> +static int __init max14577_i2c_init(void)
> >> +{
> >> + return i2c_add_driver(_i2c_driver);
> >> +}
> >> +subsys_initcall(max14577_i2c_init);
> > Why not module_i2c_driver?
> there's ordering issue, it should provide regulator which is used
> others before USB probe. if not, it failed to use USB.
> Other PMICs use also subsys_initcall for this reason.
Modern systems should be able to use deferred probing to resolve these
dependencies, subsys_initcall() is mostly there for legacy reasons and
new systems ought to be able to move away from it.
signature.asc
Description: Digital signature
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
Hi,
On Wed, 2013-11-13 at 11:50 +, Mark Rutland wrote:
> I see some of_* calls in the code, but no documentation of the binding.
> As this has more than just a compatible, interrupts, and reg, it needs
> its own binding document.
>
> [...]
>
> > +static struct mfd_cell max14577_devs[] = {
> > + { .name = "max14577-muic",
> > + .of_compatible = "maxim,max14577-muic", },
>
> The compatible string looks fine to me, but should be documented.
Sure, I will add documentation.
> [...]
>
> > +#ifdef CONFIG_OF
> > +static struct max14577_platform_data *max14577_i2c_parse_dt(struct device
> > *dev)
> > +{
> > + struct max14577_platform_data *pdata;
> > + struct device_node *np = dev->of_node;
> > +
> > + pdata = devm_kzalloc(dev, sizeof(struct max14577_platform_data),
> > +GFP_KERNEL);
> > + if (!pdata)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + pdata->irq_gpio = of_get_named_gpio(np, "irq_gpio", 0);
>
> In general, '-' is preferred to '_' in property names. This should be
> irq-gpio.
OK.
> What exactly is this used for? I know that others have strong opinions
> about how gpio/irq interaction should be handled.
Later also Mark Brown raised this. I'll leave this to discussion with
Kyungmin Park as I'm not entirely the author of the code.
>
> > + if (!gpio_is_valid(pdata->irq_gpio)) {
> > + dev_err(dev, "Invalid irq_gpio in DT\n");
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + if (of_property_read_bool(np, "wakeup"))
> > + pdata->wakeup = true;
>
> What does this mean? It seems like a remarkably generic name for
> something that I'd guess is _very_ specific to this particular binding.
>
> It might be worth prefixing it, and is certainly worth having a more
> precise name.
It is used for marking device as wake up source. This is same code as in
other max drivers used on Exynos boards (max77693 and max77686).
Thanks for review,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
Hi,
On Wed, 2013-11-13 at 13:13 +, Mark Brown wrote:
> On Wed, Nov 13, 2013 at 08:40:54AM +0100, Krzysztof Kozlowski wrote:
>
> > +/**
> > + * After resuming from suspend it may happen that IRQ is signalled but
> > + * IRQ GPIO is not high. Also the interrupt registers won't have any data
> > + * (all of them equal to 0x00).
> > + *
> > + * In such case retry few times reading the interrupt registers.
> > + */
> > +#define IRQ_READ_REG_RETRY_CNT 5
>
> What is the cause here? This smells like an unreliable workaround for
> some other behaviour. In general this all looks very like standard
> regmap code.
>
> > + for (i = 0; i < MAX14577_IRQ_REGS_NUM; i++) {
> > + u8 mask_reg = max14577_mask_reg[i];
> > +
> > + if (mask_reg == MAX14577_REG_INVALID ||
> > + IS_ERR_OR_NULL(max14577->regmap))
> > + continue;
>
> Why would this code even be running if you don't have a register map?
>
> > + dev_info(max14577->dev, "Got interrupts [1:0x%02x, 2:0x%02x,
> > 3:0x%02x]\n",
> > + irq_reg[MAX14577_IRQ_INT1], irq_reg[MAX14577_IRQ_INT2],
> > + irq_reg[MAX14577_IRQ_INT3]);
>
> This is far too noisy, dev_dbg() at most.
>
> > + gpio_val = gpio_get_value(pdata->irq_gpio);
> > +
> > + if (gpio_get_value(pdata->irq_gpio) == 0)
> > + dev_warn(max14577->dev, "IRQ GPIO is not high, retry
> > reading interrupt registers\n");
> > + } while (gpio_val == 0 && --retry > 0);
>
> This looks very strange...
>
> > + max14577->irq = gpio_to_irq(pdata->irq_gpio);
> > + ret = gpio_request(pdata->irq_gpio, "max14577_irq");
> > + if (ret) {
> > + dev_err(max14577->dev, "Failed requesting GPIO %d: %d\n",
> > + pdata->irq_gpio, ret);
> > + goto err;
> > + }
> > + gpio_direction_input(pdata->irq_gpio);
> > + gpio_free(pdata->irq_gpio);
>
> This means the GPIO handling code that was present in the handling is
> broken, it's trying to use the GPIO after it was freed.
>
> > + ret = request_threaded_irq(max14577->irq, NULL, max14577_irq_thread,
> > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > + "max14577-irq", max14577);
>
> Are you *positive* this is a falling triggered IRQ? All the code to do
> with spinning reading the GPIO state during handling makes it look like
> this is in fact an active low interrupt and a lot of the code in here is
> working around trying to handle that as the wrong kind of IRQ.
>
> > +int max14577_bulk_write(struct regmap *map, u8 reg, u8 *buf, int count)
> > +{
> > + return regmap_bulk_write(map, reg, buf, count);
> > +}
> > +EXPORT_SYMBOL_GPL(max14577_bulk_write);
>
> Given that these are basically all trivial wrappers around regmap they
> probably ought to be static inlines in the header.
>
> > +static struct max14577_platform_data *max14577_i2c_parse_dt(struct device
> > *dev)
> > +{
>
> There's no DT binding document?
>
> > +const struct dev_pm_ops max14577_pm = {
> > + .suspend = max14577_suspend,
> > + .resume = max14577_resume,
> > +};
>
> SET_SYSTEM_SLEEP_PM_OPS().
>
> > +static int __init max14577_i2c_init(void)
> > +{
> > + return i2c_add_driver(_i2c_driver);
> > +}
> > +subsys_initcall(max14577_i2c_init);
>
> Why not module_i2c_driver?
Thanks for review. I'll the fix issues and send later new version of
patch.
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
Hi,
On Wed, 2013-11-13 at 13:13 +, Mark Brown wrote:
On Wed, Nov 13, 2013 at 08:40:54AM +0100, Krzysztof Kozlowski wrote:
+/**
+ * After resuming from suspend it may happen that IRQ is signalled but
+ * IRQ GPIO is not high. Also the interrupt registers won't have any data
+ * (all of them equal to 0x00).
+ *
+ * In such case retry few times reading the interrupt registers.
+ */
+#define IRQ_READ_REG_RETRY_CNT 5
What is the cause here? This smells like an unreliable workaround for
some other behaviour. In general this all looks very like standard
regmap code.
+ for (i = 0; i MAX14577_IRQ_REGS_NUM; i++) {
+ u8 mask_reg = max14577_mask_reg[i];
+
+ if (mask_reg == MAX14577_REG_INVALID ||
+ IS_ERR_OR_NULL(max14577-regmap))
+ continue;
Why would this code even be running if you don't have a register map?
+ dev_info(max14577-dev, Got interrupts [1:0x%02x, 2:0x%02x,
3:0x%02x]\n,
+ irq_reg[MAX14577_IRQ_INT1], irq_reg[MAX14577_IRQ_INT2],
+ irq_reg[MAX14577_IRQ_INT3]);
This is far too noisy, dev_dbg() at most.
+ gpio_val = gpio_get_value(pdata-irq_gpio);
+
+ if (gpio_get_value(pdata-irq_gpio) == 0)
+ dev_warn(max14577-dev, IRQ GPIO is not high, retry
reading interrupt registers\n);
+ } while (gpio_val == 0 --retry 0);
This looks very strange...
+ max14577-irq = gpio_to_irq(pdata-irq_gpio);
+ ret = gpio_request(pdata-irq_gpio, max14577_irq);
+ if (ret) {
+ dev_err(max14577-dev, Failed requesting GPIO %d: %d\n,
+ pdata-irq_gpio, ret);
+ goto err;
+ }
+ gpio_direction_input(pdata-irq_gpio);
+ gpio_free(pdata-irq_gpio);
This means the GPIO handling code that was present in the handling is
broken, it's trying to use the GPIO after it was freed.
+ ret = request_threaded_irq(max14577-irq, NULL, max14577_irq_thread,
+ IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ max14577-irq, max14577);
Are you *positive* this is a falling triggered IRQ? All the code to do
with spinning reading the GPIO state during handling makes it look like
this is in fact an active low interrupt and a lot of the code in here is
working around trying to handle that as the wrong kind of IRQ.
+int max14577_bulk_write(struct regmap *map, u8 reg, u8 *buf, int count)
+{
+ return regmap_bulk_write(map, reg, buf, count);
+}
+EXPORT_SYMBOL_GPL(max14577_bulk_write);
Given that these are basically all trivial wrappers around regmap they
probably ought to be static inlines in the header.
+static struct max14577_platform_data *max14577_i2c_parse_dt(struct device
*dev)
+{
There's no DT binding document?
+const struct dev_pm_ops max14577_pm = {
+ .suspend = max14577_suspend,
+ .resume = max14577_resume,
+};
SET_SYSTEM_SLEEP_PM_OPS().
+static int __init max14577_i2c_init(void)
+{
+ return i2c_add_driver(max14577_i2c_driver);
+}
+subsys_initcall(max14577_i2c_init);
Why not module_i2c_driver?
Thanks for review. I'll the fix issues and send later new version of
patch.
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
Hi,
On Wed, 2013-11-13 at 11:50 +, Mark Rutland wrote:
I see some of_* calls in the code, but no documentation of the binding.
As this has more than just a compatible, interrupts, and reg, it needs
its own binding document.
[...]
+static struct mfd_cell max14577_devs[] = {
+ { .name = max14577-muic,
+ .of_compatible = maxim,max14577-muic, },
The compatible string looks fine to me, but should be documented.
Sure, I will add documentation.
[...]
+#ifdef CONFIG_OF
+static struct max14577_platform_data *max14577_i2c_parse_dt(struct device
*dev)
+{
+ struct max14577_platform_data *pdata;
+ struct device_node *np = dev-of_node;
+
+ pdata = devm_kzalloc(dev, sizeof(struct max14577_platform_data),
+GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+
+ pdata-irq_gpio = of_get_named_gpio(np, irq_gpio, 0);
In general, '-' is preferred to '_' in property names. This should be
irq-gpio.
OK.
What exactly is this used for? I know that others have strong opinions
about how gpio/irq interaction should be handled.
Later also Mark Brown raised this. I'll leave this to discussion with
Kyungmin Park as I'm not entirely the author of the code.
+ if (!gpio_is_valid(pdata-irq_gpio)) {
+ dev_err(dev, Invalid irq_gpio in DT\n);
+ return ERR_PTR(-EINVAL);
+ }
+
+ if (of_property_read_bool(np, wakeup))
+ pdata-wakeup = true;
What does this mean? It seems like a remarkably generic name for
something that I'd guess is _very_ specific to this particular binding.
It might be worth prefixing it, and is certainly worth having a more
precise name.
It is used for marking device as wake up source. This is same code as in
other max drivers used on Exynos boards (max77693 and max77686).
Thanks for review,
Krzysztof
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
On Thu, Nov 14, 2013 at 10:33:22AM +0900, Kyungmin Park wrote:
On Wed, Nov 13, 2013 at 10:13 PM, Mark Brown broo...@kernel.org wrote:
Are you *positive* this is a falling triggered IRQ? All the code to do
with spinning reading the GPIO state during handling makes it look like
this is in fact an active low interrupt and a lot of the code in here is
working around trying to handle that as the wrong kind of IRQ.
It's not work with level triggering. as wm8994, it requires edge
triggering. previous time I send RFC patch to handle edge triggering
at regmap.
No, wm8994 is level triggered only - the edge triggering stuff there is
to work around some SoCs that could only support edge triggering and not
level triggering. Is this a similar issue or is there something else
going on, what's the differeence with a level triggered scheme?
+static int __init max14577_i2c_init(void)
+{
+ return i2c_add_driver(max14577_i2c_driver);
+}
+subsys_initcall(max14577_i2c_init);
Why not module_i2c_driver?
there's ordering issue, it should provide regulator which is used
others before USB probe. if not, it failed to use USB.
Other PMICs use also subsys_initcall for this reason.
Modern systems should be able to use deferred probing to resolve these
dependencies, subsys_initcall() is mostly there for legacy reasons and
new systems ought to be able to move away from it.
signature.asc
Description: Digital signature
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
Hello,
On 2013-11-14 11:24, Mark Brown wrote:
On Thu, Nov 14, 2013 at 10:33:22AM +0900, Kyungmin Park wrote:
On Wed, Nov 13, 2013 at 10:13 PM, Mark Brown broo...@kernel.org wrote:
Are you *positive* this is a falling triggered IRQ? All the code to do
with spinning reading the GPIO state during handling makes it look like
this is in fact an active low interrupt and a lot of the code in here is
working around trying to handle that as the wrong kind of IRQ.
It's not work with level triggering. as wm8994, it requires edge
triggering. previous time I send RFC patch to handle edge triggering
at regmap.
No, wm8994 is level triggered only - the edge triggering stuff there is
to work around some SoCs that could only support edge triggering and not
level triggering. Is this a similar issue or is there something else
going on, what's the differeence with a level triggered scheme?
+static int __init max14577_i2c_init(void)
+{
+ return i2c_add_driver(max14577_i2c_driver);
+}
+subsys_initcall(max14577_i2c_init);
Why not module_i2c_driver?
there's ordering issue, it should provide regulator which is used
others before USB probe. if not, it failed to use USB.
Other PMICs use also subsys_initcall for this reason.
Modern systems should be able to use deferred probing to resolve these
dependencies, subsys_initcall() is mostly there for legacy reasons and
new systems ought to be able to move away from it.
The main problem here is usb gadget subsystem, which simply doesn't support
deferred probe yet. UDC driver (which uses those regulators) need to be
initialized before usb gadget driver (i.e. mass storage or cdc ethernet)
has been registered.
Best regards
--
Marek Szyprowski
Samsung RD Institute Poland
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
On Thu, Nov 14, 2013 at 11:53:51AM +0100, Marek Szyprowski wrote: On 2013-11-14 11:24, Mark Brown wrote: Modern systems should be able to use deferred probing to resolve these dependencies, subsys_initcall() is mostly there for legacy reasons and new systems ought to be able to move away from it. The main problem here is usb gadget subsystem, which simply doesn't support deferred probe yet. UDC driver (which uses those regulators) need to be initialized before usb gadget driver (i.e. mass storage or cdc ethernet) has been registered. OK, that makes sense - OK for the time being. signature.asc Description: Digital signature
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
On Wed, Nov 13, 2013 at 10:13 PM, Mark Brown wrote:
> On Wed, Nov 13, 2013 at 08:40:54AM +0100, Krzysztof Kozlowski wrote:
>
>> +/**
>> + * After resuming from suspend it may happen that IRQ is signalled but
>> + * IRQ GPIO is not high. Also the interrupt registers won't have any data
>> + * (all of them equal to 0x00).
>> + *
>> + * In such case retry few times reading the interrupt registers.
>> + */
>> +#define IRQ_READ_REG_RETRY_CNT 5
>
> What is the cause here? This smells like an unreliable workaround for
> some other behaviour. In general this all looks very like standard
> regmap code.
>
>> + for (i = 0; i < MAX14577_IRQ_REGS_NUM; i++) {
>> + u8 mask_reg = max14577_mask_reg[i];
>> +
>> + if (mask_reg == MAX14577_REG_INVALID ||
>> + IS_ERR_OR_NULL(max14577->regmap))
>> + continue;
>
> Why would this code even be running if you don't have a register map?
>
>> + dev_info(max14577->dev, "Got interrupts [1:0x%02x, 2:0x%02x,
>> 3:0x%02x]\n",
>> + irq_reg[MAX14577_IRQ_INT1], irq_reg[MAX14577_IRQ_INT2],
>> + irq_reg[MAX14577_IRQ_INT3]);
>
> This is far too noisy, dev_dbg() at most.
>
>> + gpio_val = gpio_get_value(pdata->irq_gpio);
>> +
>> + if (gpio_get_value(pdata->irq_gpio) == 0)
>> + dev_warn(max14577->dev, "IRQ GPIO is not high, retry
>> reading interrupt registers\n");
>> + } while (gpio_val == 0 && --retry > 0);
>
> This looks very strange...
>
>> + max14577->irq = gpio_to_irq(pdata->irq_gpio);
>> + ret = gpio_request(pdata->irq_gpio, "max14577_irq");
>> + if (ret) {
>> + dev_err(max14577->dev, "Failed requesting GPIO %d: %d\n",
>> + pdata->irq_gpio, ret);
>> + goto err;
>> + }
>> + gpio_direction_input(pdata->irq_gpio);
>> + gpio_free(pdata->irq_gpio);
>
> This means the GPIO handling code that was present in the handling is
> broken, it's trying to use the GPIO after it was freed.
>
>> + ret = request_threaded_irq(max14577->irq, NULL, max14577_irq_thread,
>> +IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +"max14577-irq", max14577);
>
> Are you *positive* this is a falling triggered IRQ? All the code to do
> with spinning reading the GPIO state during handling makes it look like
> this is in fact an active low interrupt and a lot of the code in here is
> working around trying to handle that as the wrong kind of IRQ.
It's not work with level triggering. as wm8994, it requires edge
triggering. previous time I send RFC patch to handle edge triggering
at regmap.
>
>> +int max14577_bulk_write(struct regmap *map, u8 reg, u8 *buf, int count)
>> +{
>> + return regmap_bulk_write(map, reg, buf, count);
>> +}
>> +EXPORT_SYMBOL_GPL(max14577_bulk_write);
>
> Given that these are basically all trivial wrappers around regmap they
> probably ought to be static inlines in the header.
>
>> +static struct max14577_platform_data *max14577_i2c_parse_dt(struct device
>> *dev)
>> +{
>
> There's no DT binding document?
>
>> +const struct dev_pm_ops max14577_pm = {
>> + .suspend = max14577_suspend,
>> + .resume = max14577_resume,
>> +};
>
> SET_SYSTEM_SLEEP_PM_OPS().
>
>> +static int __init max14577_i2c_init(void)
>> +{
>> + return i2c_add_driver(_i2c_driver);
>> +}
>> +subsys_initcall(max14577_i2c_init);
>
> Why not module_i2c_driver?
there's ordering issue, it should provide regulator which is used
others before USB probe. if not, it failed to use USB.
Other PMICs use also subsys_initcall for this reason.
Thank you,
Kyungmin Park
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
On Wed, Nov 13, 2013 at 08:40:54AM +0100, Krzysztof Kozlowski wrote:
> +/**
> + * After resuming from suspend it may happen that IRQ is signalled but
> + * IRQ GPIO is not high. Also the interrupt registers won't have any data
> + * (all of them equal to 0x00).
> + *
> + * In such case retry few times reading the interrupt registers.
> + */
> +#define IRQ_READ_REG_RETRY_CNT 5
What is the cause here? This smells like an unreliable workaround for
some other behaviour. In general this all looks very like standard
regmap code.
> + for (i = 0; i < MAX14577_IRQ_REGS_NUM; i++) {
> + u8 mask_reg = max14577_mask_reg[i];
> +
> + if (mask_reg == MAX14577_REG_INVALID ||
> + IS_ERR_OR_NULL(max14577->regmap))
> + continue;
Why would this code even be running if you don't have a register map?
> + dev_info(max14577->dev, "Got interrupts [1:0x%02x, 2:0x%02x,
> 3:0x%02x]\n",
> + irq_reg[MAX14577_IRQ_INT1], irq_reg[MAX14577_IRQ_INT2],
> + irq_reg[MAX14577_IRQ_INT3]);
This is far too noisy, dev_dbg() at most.
> + gpio_val = gpio_get_value(pdata->irq_gpio);
> +
> + if (gpio_get_value(pdata->irq_gpio) == 0)
> + dev_warn(max14577->dev, "IRQ GPIO is not high, retry
> reading interrupt registers\n");
> + } while (gpio_val == 0 && --retry > 0);
This looks very strange...
> + max14577->irq = gpio_to_irq(pdata->irq_gpio);
> + ret = gpio_request(pdata->irq_gpio, "max14577_irq");
> + if (ret) {
> + dev_err(max14577->dev, "Failed requesting GPIO %d: %d\n",
> + pdata->irq_gpio, ret);
> + goto err;
> + }
> + gpio_direction_input(pdata->irq_gpio);
> + gpio_free(pdata->irq_gpio);
This means the GPIO handling code that was present in the handling is
broken, it's trying to use the GPIO after it was freed.
> + ret = request_threaded_irq(max14577->irq, NULL, max14577_irq_thread,
> +IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +"max14577-irq", max14577);
Are you *positive* this is a falling triggered IRQ? All the code to do
with spinning reading the GPIO state during handling makes it look like
this is in fact an active low interrupt and a lot of the code in here is
working around trying to handle that as the wrong kind of IRQ.
> +int max14577_bulk_write(struct regmap *map, u8 reg, u8 *buf, int count)
> +{
> + return regmap_bulk_write(map, reg, buf, count);
> +}
> +EXPORT_SYMBOL_GPL(max14577_bulk_write);
Given that these are basically all trivial wrappers around regmap they
probably ought to be static inlines in the header.
> +static struct max14577_platform_data *max14577_i2c_parse_dt(struct device
> *dev)
> +{
There's no DT binding document?
> +const struct dev_pm_ops max14577_pm = {
> + .suspend = max14577_suspend,
> + .resume = max14577_resume,
> +};
SET_SYSTEM_SLEEP_PM_OPS().
> +static int __init max14577_i2c_init(void)
> +{
> + return i2c_add_driver(_i2c_driver);
> +}
> +subsys_initcall(max14577_i2c_init);
Why not module_i2c_driver?
signature.asc
Description: Digital signature
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
On Wed, Nov 13, 2013 at 07:40:54AM +, Krzysztof Kozlowski wrote:
> From: Chanwoo Choi
>
> This patch adds max14577 core/irq driver to support MUIC(Micro USB IC)
> device and charger device and support irq domain method to control
> internal interrupt of max14577 device. Also, this patch supports DT
> binding with max14577_i2c_parse_dt().
>
> The MAXIM 14577 chip contains Micro-USB Interface Circuit and Li+ Battery
> Charger. It contains accessory and USB charger detection logic. It supports
> USB 2.0 Hi-Speed, UART and stereo audio signals over Micro-USB connector.
>
> The battery charger is compliant with the USB Battery Charging Specification
> Revision 1.1. It has also SFOUT LDO output for powering USB devices.
>
> Signed-off-by: Chanwoo Choi
> Signed-off-by: Krzysztof Kozlowski
> Signed-off-by: Kyungmin Park
> ---
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile |1 +
> drivers/mfd/max14577-irq.c | 283 +
> drivers/mfd/max14577.c | 268 +++
> include/linux/mfd/max14577-private.h | 291
> ++
> include/linux/mfd/max14577.h | 76 +
> 6 files changed, 932 insertions(+)
> create mode 100644 drivers/mfd/max14577-irq.c
> create mode 100644 drivers/mfd/max14577.c
> create mode 100644 include/linux/mfd/max14577-private.h
> create mode 100644 include/linux/mfd/max14577.h
I see some of_* calls in the code, but no documentation of the binding.
As this has more than just a compatible, interrupts, and reg, it needs
its own binding document.
[...]
> +static struct mfd_cell max14577_devs[] = {
> + { .name = "max14577-muic",
> + .of_compatible = "maxim,max14577-muic", },
The compatible string looks fine to me, but should be documented.
[...]
> +#ifdef CONFIG_OF
> +static struct max14577_platform_data *max14577_i2c_parse_dt(struct device
> *dev)
> +{
> + struct max14577_platform_data *pdata;
> + struct device_node *np = dev->of_node;
> +
> + pdata = devm_kzalloc(dev, sizeof(struct max14577_platform_data),
> +GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> +
> + pdata->irq_gpio = of_get_named_gpio(np, "irq_gpio", 0);
In general, '-' is preferred to '_' in property names. This should be
irq-gpio.
What exactly is this used for? I know that others have strong opinions
about how gpio/irq interaction should be handled.
> + if (!gpio_is_valid(pdata->irq_gpio)) {
> + dev_err(dev, "Invalid irq_gpio in DT\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (of_property_read_bool(np, "wakeup"))
> + pdata->wakeup = true;
What does this mean? It seems like a remarkably generic name for
something that I'd guess is _very_ specific to this particular binding.
It might be worth prefixing it, and is certainly worth having a more
precise name.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
On Wed, Nov 13, 2013 at 07:40:54AM +, Krzysztof Kozlowski wrote:
From: Chanwoo Choi cw00.c...@samsung.com
This patch adds max14577 core/irq driver to support MUIC(Micro USB IC)
device and charger device and support irq domain method to control
internal interrupt of max14577 device. Also, this patch supports DT
binding with max14577_i2c_parse_dt().
The MAXIM 14577 chip contains Micro-USB Interface Circuit and Li+ Battery
Charger. It contains accessory and USB charger detection logic. It supports
USB 2.0 Hi-Speed, UART and stereo audio signals over Micro-USB connector.
The battery charger is compliant with the USB Battery Charging Specification
Revision 1.1. It has also SFOUT LDO output for powering USB devices.
Signed-off-by: Chanwoo Choi cw00.c...@samsung.com
Signed-off-by: Krzysztof Kozlowski k.kozlow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile |1 +
drivers/mfd/max14577-irq.c | 283 +
drivers/mfd/max14577.c | 268 +++
include/linux/mfd/max14577-private.h | 291
++
include/linux/mfd/max14577.h | 76 +
6 files changed, 932 insertions(+)
create mode 100644 drivers/mfd/max14577-irq.c
create mode 100644 drivers/mfd/max14577.c
create mode 100644 include/linux/mfd/max14577-private.h
create mode 100644 include/linux/mfd/max14577.h
I see some of_* calls in the code, but no documentation of the binding.
As this has more than just a compatible, interrupts, and reg, it needs
its own binding document.
[...]
+static struct mfd_cell max14577_devs[] = {
+ { .name = max14577-muic,
+ .of_compatible = maxim,max14577-muic, },
The compatible string looks fine to me, but should be documented.
[...]
+#ifdef CONFIG_OF
+static struct max14577_platform_data *max14577_i2c_parse_dt(struct device
*dev)
+{
+ struct max14577_platform_data *pdata;
+ struct device_node *np = dev-of_node;
+
+ pdata = devm_kzalloc(dev, sizeof(struct max14577_platform_data),
+GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+
+ pdata-irq_gpio = of_get_named_gpio(np, irq_gpio, 0);
In general, '-' is preferred to '_' in property names. This should be
irq-gpio.
What exactly is this used for? I know that others have strong opinions
about how gpio/irq interaction should be handled.
+ if (!gpio_is_valid(pdata-irq_gpio)) {
+ dev_err(dev, Invalid irq_gpio in DT\n);
+ return ERR_PTR(-EINVAL);
+ }
+
+ if (of_property_read_bool(np, wakeup))
+ pdata-wakeup = true;
What does this mean? It seems like a remarkably generic name for
something that I'd guess is _very_ specific to this particular binding.
It might be worth prefixing it, and is certainly worth having a more
precise name.
Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
On Wed, Nov 13, 2013 at 08:40:54AM +0100, Krzysztof Kozlowski wrote:
+/**
+ * After resuming from suspend it may happen that IRQ is signalled but
+ * IRQ GPIO is not high. Also the interrupt registers won't have any data
+ * (all of them equal to 0x00).
+ *
+ * In such case retry few times reading the interrupt registers.
+ */
+#define IRQ_READ_REG_RETRY_CNT 5
What is the cause here? This smells like an unreliable workaround for
some other behaviour. In general this all looks very like standard
regmap code.
+ for (i = 0; i MAX14577_IRQ_REGS_NUM; i++) {
+ u8 mask_reg = max14577_mask_reg[i];
+
+ if (mask_reg == MAX14577_REG_INVALID ||
+ IS_ERR_OR_NULL(max14577-regmap))
+ continue;
Why would this code even be running if you don't have a register map?
+ dev_info(max14577-dev, Got interrupts [1:0x%02x, 2:0x%02x,
3:0x%02x]\n,
+ irq_reg[MAX14577_IRQ_INT1], irq_reg[MAX14577_IRQ_INT2],
+ irq_reg[MAX14577_IRQ_INT3]);
This is far too noisy, dev_dbg() at most.
+ gpio_val = gpio_get_value(pdata-irq_gpio);
+
+ if (gpio_get_value(pdata-irq_gpio) == 0)
+ dev_warn(max14577-dev, IRQ GPIO is not high, retry
reading interrupt registers\n);
+ } while (gpio_val == 0 --retry 0);
This looks very strange...
+ max14577-irq = gpio_to_irq(pdata-irq_gpio);
+ ret = gpio_request(pdata-irq_gpio, max14577_irq);
+ if (ret) {
+ dev_err(max14577-dev, Failed requesting GPIO %d: %d\n,
+ pdata-irq_gpio, ret);
+ goto err;
+ }
+ gpio_direction_input(pdata-irq_gpio);
+ gpio_free(pdata-irq_gpio);
This means the GPIO handling code that was present in the handling is
broken, it's trying to use the GPIO after it was freed.
+ ret = request_threaded_irq(max14577-irq, NULL, max14577_irq_thread,
+IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+max14577-irq, max14577);
Are you *positive* this is a falling triggered IRQ? All the code to do
with spinning reading the GPIO state during handling makes it look like
this is in fact an active low interrupt and a lot of the code in here is
working around trying to handle that as the wrong kind of IRQ.
+int max14577_bulk_write(struct regmap *map, u8 reg, u8 *buf, int count)
+{
+ return regmap_bulk_write(map, reg, buf, count);
+}
+EXPORT_SYMBOL_GPL(max14577_bulk_write);
Given that these are basically all trivial wrappers around regmap they
probably ought to be static inlines in the header.
+static struct max14577_platform_data *max14577_i2c_parse_dt(struct device
*dev)
+{
There's no DT binding document?
+const struct dev_pm_ops max14577_pm = {
+ .suspend = max14577_suspend,
+ .resume = max14577_resume,
+};
SET_SYSTEM_SLEEP_PM_OPS().
+static int __init max14577_i2c_init(void)
+{
+ return i2c_add_driver(max14577_i2c_driver);
+}
+subsys_initcall(max14577_i2c_init);
Why not module_i2c_driver?
signature.asc
Description: Digital signature
Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
On Wed, Nov 13, 2013 at 10:13 PM, Mark Brown broo...@kernel.org wrote:
On Wed, Nov 13, 2013 at 08:40:54AM +0100, Krzysztof Kozlowski wrote:
+/**
+ * After resuming from suspend it may happen that IRQ is signalled but
+ * IRQ GPIO is not high. Also the interrupt registers won't have any data
+ * (all of them equal to 0x00).
+ *
+ * In such case retry few times reading the interrupt registers.
+ */
+#define IRQ_READ_REG_RETRY_CNT 5
What is the cause here? This smells like an unreliable workaround for
some other behaviour. In general this all looks very like standard
regmap code.
+ for (i = 0; i MAX14577_IRQ_REGS_NUM; i++) {
+ u8 mask_reg = max14577_mask_reg[i];
+
+ if (mask_reg == MAX14577_REG_INVALID ||
+ IS_ERR_OR_NULL(max14577-regmap))
+ continue;
Why would this code even be running if you don't have a register map?
+ dev_info(max14577-dev, Got interrupts [1:0x%02x, 2:0x%02x,
3:0x%02x]\n,
+ irq_reg[MAX14577_IRQ_INT1], irq_reg[MAX14577_IRQ_INT2],
+ irq_reg[MAX14577_IRQ_INT3]);
This is far too noisy, dev_dbg() at most.
+ gpio_val = gpio_get_value(pdata-irq_gpio);
+
+ if (gpio_get_value(pdata-irq_gpio) == 0)
+ dev_warn(max14577-dev, IRQ GPIO is not high, retry
reading interrupt registers\n);
+ } while (gpio_val == 0 --retry 0);
This looks very strange...
+ max14577-irq = gpio_to_irq(pdata-irq_gpio);
+ ret = gpio_request(pdata-irq_gpio, max14577_irq);
+ if (ret) {
+ dev_err(max14577-dev, Failed requesting GPIO %d: %d\n,
+ pdata-irq_gpio, ret);
+ goto err;
+ }
+ gpio_direction_input(pdata-irq_gpio);
+ gpio_free(pdata-irq_gpio);
This means the GPIO handling code that was present in the handling is
broken, it's trying to use the GPIO after it was freed.
+ ret = request_threaded_irq(max14577-irq, NULL, max14577_irq_thread,
+IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+max14577-irq, max14577);
Are you *positive* this is a falling triggered IRQ? All the code to do
with spinning reading the GPIO state during handling makes it look like
this is in fact an active low interrupt and a lot of the code in here is
working around trying to handle that as the wrong kind of IRQ.
It's not work with level triggering. as wm8994, it requires edge
triggering. previous time I send RFC patch to handle edge triggering
at regmap.
+int max14577_bulk_write(struct regmap *map, u8 reg, u8 *buf, int count)
+{
+ return regmap_bulk_write(map, reg, buf, count);
+}
+EXPORT_SYMBOL_GPL(max14577_bulk_write);
Given that these are basically all trivial wrappers around regmap they
probably ought to be static inlines in the header.
+static struct max14577_platform_data *max14577_i2c_parse_dt(struct device
*dev)
+{
There's no DT binding document?
+const struct dev_pm_ops max14577_pm = {
+ .suspend = max14577_suspend,
+ .resume = max14577_resume,
+};
SET_SYSTEM_SLEEP_PM_OPS().
+static int __init max14577_i2c_init(void)
+{
+ return i2c_add_driver(max14577_i2c_driver);
+}
+subsys_initcall(max14577_i2c_init);
Why not module_i2c_driver?
there's ordering issue, it should provide regulator which is used
others before USB probe. if not, it failed to use USB.
Other PMICs use also subsys_initcall for this reason.
Thank you,
Kyungmin Park
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
From: Chanwoo Choi
This patch adds max14577 core/irq driver to support MUIC(Micro USB IC)
device and charger device and support irq domain method to control
internal interrupt of max14577 device. Also, this patch supports DT
binding with max14577_i2c_parse_dt().
The MAXIM 14577 chip contains Micro-USB Interface Circuit and Li+ Battery
Charger. It contains accessory and USB charger detection logic. It supports
USB 2.0 Hi-Speed, UART and stereo audio signals over Micro-USB connector.
The battery charger is compliant with the USB Battery Charging Specification
Revision 1.1. It has also SFOUT LDO output for powering USB devices.
Signed-off-by: Chanwoo Choi
Signed-off-by: Krzysztof Kozlowski
Signed-off-by: Kyungmin Park
---
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile |1 +
drivers/mfd/max14577-irq.c | 283 +
drivers/mfd/max14577.c | 268 +++
include/linux/mfd/max14577-private.h | 291 ++
include/linux/mfd/max14577.h | 76 +
6 files changed, 932 insertions(+)
create mode 100644 drivers/mfd/max14577-irq.c
create mode 100644 drivers/mfd/max14577.c
create mode 100644 include/linux/mfd/max14577-private.h
create mode 100644 include/linux/mfd/max14577.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 914c3d1..f2a1a76 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -309,6 +309,19 @@ config MFD_88PM860X
select individual components like voltage regulators, RTC and
battery-charger under the corresponding menus.
+config MFD_MAX14577
+ bool "Maxim Semiconductor MAX14577 MUIC + Charger Support"
+ depends on I2C=y
+ select MFD_CORE
+ select REGMAP_I2C
+ select IRQ_DOMAIN
+ help
+ Say yes here to support for Maxim Semiconductor MAX14577.
+ This is a Micro-USB IC with Charger controls on chip.
+ This driver provides common support for accessing the device;
+ additional drivers must be enabled in order to use the functionality
+ of the device.
+
config MFD_MAX77686
bool "Maxim Semiconductor MAX77686 PMIC Support"
depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 15b905c..548c87d 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -110,6 +110,7 @@ obj-$(CONFIG_MFD_DA9055)+= da9055.o
da9063-objs:= da9063-core.o da9063-irq.o da9063-i2c.o
obj-$(CONFIG_MFD_DA9063) += da9063.o
+obj-$(CONFIG_MFD_MAX14577) += max14577.o max14577-irq.o
obj-$(CONFIG_MFD_MAX77686) += max77686.o max77686-irq.o
obj-$(CONFIG_MFD_MAX77693) += max77693.o max77693-irq.o
obj-$(CONFIG_MFD_MAX8907) += max8907.o
diff --git a/drivers/mfd/max14577-irq.c b/drivers/mfd/max14577-irq.c
new file mode 100644
index 000..9cd8012
--- /dev/null
+++ b/drivers/mfd/max14577-irq.c
@@ -0,0 +1,283 @@
+/*
+ * max14577-irq.c - MFD Interrupt controller support for MAX14577
+ *
+ * Copyright (C) 2013 Samsung Electronics Co.Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * This driver is based on max8997-irq.c
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+/**
+ * After resuming from suspend it may happen that IRQ is signalled but
+ * IRQ GPIO is not high. Also the interrupt registers won't have any data
+ * (all of them equal to 0x00).
+ *
+ * In such case retry few times reading the interrupt registers.
+ */
+#define IRQ_READ_REG_RETRY_CNT 5
+
+static const u8 max14577_mask_reg[] = {
+ [MAX14577_IRQ_INT1] = MAX14577_REG_INTMASK1,
+ [MAX14577_IRQ_INT2] = MAX14577_REG_INTMASK2,
+ [MAX14577_IRQ_INT3] = MAX14577_REG_INTMASK3,
+};
+
+struct max14577_irq_data {
+ int mask;
+ enum max14577_irq_source group;
+};
+
+#define DECLARE_IRQ(idx, _group, _mask)\
+ [(idx)] = { .group = (_group), .mask = (_mask) }
+static const struct max14577_irq_data max14577_irqs[] = {
+ DECLARE_IRQ(MAX14577_IRQ_INT1_ADC, MAX14577_IRQ_INT1, 1 <<
0),
+ DECLARE_IRQ(MAX14577_IRQ_INT1_ADCLOW, MAX14577_IRQ_INT1, 1 <<
1),
+ DECLARE_IRQ(MAX14577_IRQ_INT1_ADCERR,
[PATCH 1/4] mfd: max14577: Add max14577 MFD driver core
From: Chanwoo Choi cw00.c...@samsung.com
This patch adds max14577 core/irq driver to support MUIC(Micro USB IC)
device and charger device and support irq domain method to control
internal interrupt of max14577 device. Also, this patch supports DT
binding with max14577_i2c_parse_dt().
The MAXIM 14577 chip contains Micro-USB Interface Circuit and Li+ Battery
Charger. It contains accessory and USB charger detection logic. It supports
USB 2.0 Hi-Speed, UART and stereo audio signals over Micro-USB connector.
The battery charger is compliant with the USB Battery Charging Specification
Revision 1.1. It has also SFOUT LDO output for powering USB devices.
Signed-off-by: Chanwoo Choi cw00.c...@samsung.com
Signed-off-by: Krzysztof Kozlowski k.kozlow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile |1 +
drivers/mfd/max14577-irq.c | 283 +
drivers/mfd/max14577.c | 268 +++
include/linux/mfd/max14577-private.h | 291 ++
include/linux/mfd/max14577.h | 76 +
6 files changed, 932 insertions(+)
create mode 100644 drivers/mfd/max14577-irq.c
create mode 100644 drivers/mfd/max14577.c
create mode 100644 include/linux/mfd/max14577-private.h
create mode 100644 include/linux/mfd/max14577.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 914c3d1..f2a1a76 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -309,6 +309,19 @@ config MFD_88PM860X
select individual components like voltage regulators, RTC and
battery-charger under the corresponding menus.
+config MFD_MAX14577
+ bool Maxim Semiconductor MAX14577 MUIC + Charger Support
+ depends on I2C=y
+ select MFD_CORE
+ select REGMAP_I2C
+ select IRQ_DOMAIN
+ help
+ Say yes here to support for Maxim Semiconductor MAX14577.
+ This is a Micro-USB IC with Charger controls on chip.
+ This driver provides common support for accessing the device;
+ additional drivers must be enabled in order to use the functionality
+ of the device.
+
config MFD_MAX77686
bool Maxim Semiconductor MAX77686 PMIC Support
depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 15b905c..548c87d 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -110,6 +110,7 @@ obj-$(CONFIG_MFD_DA9055)+= da9055.o
da9063-objs:= da9063-core.o da9063-irq.o da9063-i2c.o
obj-$(CONFIG_MFD_DA9063) += da9063.o
+obj-$(CONFIG_MFD_MAX14577) += max14577.o max14577-irq.o
obj-$(CONFIG_MFD_MAX77686) += max77686.o max77686-irq.o
obj-$(CONFIG_MFD_MAX77693) += max77693.o max77693-irq.o
obj-$(CONFIG_MFD_MAX8907) += max8907.o
diff --git a/drivers/mfd/max14577-irq.c b/drivers/mfd/max14577-irq.c
new file mode 100644
index 000..9cd8012
--- /dev/null
+++ b/drivers/mfd/max14577-irq.c
@@ -0,0 +1,283 @@
+/*
+ * max14577-irq.c - MFD Interrupt controller support for MAX14577
+ *
+ * Copyright (C) 2013 Samsung Electronics Co.Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * This driver is based on max8997-irq.c
+ */
+
+#include linux/irq.h
+#include linux/interrupt.h
+#include linux/gpio.h
+#include linux/irqdomain.h
+#include linux/mfd/max14577.h
+#include linux/mfd/max14577-private.h
+
+/**
+ * After resuming from suspend it may happen that IRQ is signalled but
+ * IRQ GPIO is not high. Also the interrupt registers won't have any data
+ * (all of them equal to 0x00).
+ *
+ * In such case retry few times reading the interrupt registers.
+ */
+#define IRQ_READ_REG_RETRY_CNT 5
+
+static const u8 max14577_mask_reg[] = {
+ [MAX14577_IRQ_INT1] = MAX14577_REG_INTMASK1,
+ [MAX14577_IRQ_INT2] = MAX14577_REG_INTMASK2,
+ [MAX14577_IRQ_INT3] = MAX14577_REG_INTMASK3,
+};
+
+struct max14577_irq_data {
+ int mask;
+ enum max14577_irq_source group;
+};
+
+#define DECLARE_IRQ(idx, _group, _mask)\
+ [(idx)] = { .group = (_group), .mask = (_mask) }
+static const struct max14577_irq_data max14577_irqs[] = {
+ DECLARE_IRQ(MAX14577_IRQ_INT1_ADC,
