diff mbox series

[v4,2/5] mfd: mp2629: Add support for mps battery charger

Message ID 20200322224626.13160-3-sravanhome@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add battery charger driver support for MP2629 | expand

Commit Message

saravanan sekar March 22, 2020, 10:46 p.m. UTC 
mp2629 is a highly-integrated switching-mode battery charge management
device for single-cell Li-ion or Li-polymer battery.

Add MFD core enables chip access for ADC driver for battery readings,
and a power supply battery-charger driver

Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
---
 drivers/mfd/Kconfig        |   9 +++
 drivers/mfd/Makefile       |   2 +
 drivers/mfd/mp2629.c       | 116 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/mp2629.h |  22 +++++++
 4 files changed, 149 insertions(+)
 create mode 100644 drivers/mfd/mp2629.c
 create mode 100644 include/linux/mfd/mp2629.h

Comments

Lee Jones March 27, 2020, 7:55 a.m. UTC | #1 
On Sun, 22 Mar 2020, Saravanan Sekar wrote:

> mp2629 is a highly-integrated switching-mode battery charge management
> device for single-cell Li-ion or Li-polymer battery.
> 
> Add MFD core enables chip access for ADC driver for battery readings,
> and a power supply battery-charger driver
> 
> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> ---
>  drivers/mfd/Kconfig        |   9 +++
>  drivers/mfd/Makefile       |   2 +
>  drivers/mfd/mp2629.c       | 116 +++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/mp2629.h |  22 +++++++
>  4 files changed, 149 insertions(+)
>  create mode 100644 drivers/mfd/mp2629.c
>  create mode 100644 include/linux/mfd/mp2629.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3c547ed575e6..6614e5cff881 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -434,6 +434,15 @@ config MFD_MC13XXX_I2C
>  	help
>  	  Select this if your MC13xxx is connected via an I2C bus.
>  
> +config MFD_MP2629
> +	bool "Monolithic power system MP2629 ADC and Battery charger"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Select this option to enable support for monolithic power system
> +	  battery charger. This provides ADC, thermal, battery charger power
> +	  management functions on the systems.
> +
>  config MFD_MXS_LRADC
>  	tristate "Freescale i.MX23/i.MX28 LRADC"
>  	depends on ARCH_MXS || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f935d10cbf0f..d6c210f96d02 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -170,6 +170,8 @@ obj-$(CONFIG_MFD_MAX8925)	+= max8925.o
>  obj-$(CONFIG_MFD_MAX8997)	+= max8997.o max8997-irq.o
>  obj-$(CONFIG_MFD_MAX8998)	+= max8998.o max8998-irq.o
>  
> +obj-$(CONFIG_MFD_MP2629)	+= mp2629.o
> +
>  pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
> diff --git a/drivers/mfd/mp2629.c b/drivers/mfd/mp2629.c
> new file mode 100644
> index 000000000000..41a4082387ce
> --- /dev/null
> +++ b/drivers/mfd/mp2629.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * MP2629 MFD Driver for ADC and battery charger

s/MFD Driver/parent driver/

> + * Copyright 2020 Monolithic Power Systems, Inc
> + *
> + * Author: Saravanan Sekar <sravanhome@gmail.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/mp2629.h>

Alphabetical please.

> +enum {
> +	MP2629_MFD_ADC,
> +	MP2629_MFD_CHARGER,
> +	MP2629_MFD_MAX
> +};
> +
> +static struct resource mp2629_irq_rsrc[] = {
> +	{
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct mfd_cell mp2629mfd[] = {
> +	[MP2629_MFD_ADC] = {
> +		.name = "mp2629_adc",
> +		.of_compatible = "mps,mp2629_adc",
> +	},
> +	[MP2629_MFD_CHARGER] = {
> +		.name = "mp2629_charger",
> +		.of_compatible = "mps,mp2629_charger",
> +		.resources = mp2629_irq_rsrc,
> +		.num_resources = ARRAY_SIZE(mp2629_irq_rsrc),
> +	}
> +};
> +
> +static const struct regmap_config mp2629_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0x17,
> +};
> +
> +static int mp2629_probe(struct i2c_client *client)
> +{
> +	struct mp2629_info *info;

All this ddata instead of info.

> +	struct resource	*resources;
> +	int ret;
> +	int i;
> +
> +	info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->dev = &client->dev;
> +	i2c_set_clientdata(client, info);
> +
> +	info->regmap = devm_regmap_init_i2c(client, &mp2629_regmap_config);
> +	if (IS_ERR(info->regmap)) {
> +		dev_err(info->dev, "Failed to allocate regmap!\n");
> +		return PTR_ERR(info->regmap);
> +	}
> +
> +	for (i = 0; i < MP2629_MFD_MAX; i++) {
> +		mp2629mfd[i].platform_data = &info->regmap;
> +		mp2629mfd[i].pdata_size = sizeof(info->regmap);

You don't need to store this in platform data as well.

You already have it in device data (ddata [currently 'info']).

> +		resources = (struct resource *)mp2629mfd[i].resources;
> +		if (resources) {
> +			resources[0].start = client->irq;
> +			resources[0].end = client->irq;
> +		}

You don't need to store this separately either.

Just fetch it from the parent in the child device driver.

It will look something like (untested, off the top of my head):

  platform_get_irq(to_platform_device(pdev->dev.parent), 0);

> +	}
> +
> +	ret = devm_mfd_add_devices(info->dev, PLATFORM_DEVID_NONE, mp2629mfd,
> +				ARRAY_SIZE(mp2629mfd), NULL,
> +				0, NULL);
> +	if (ret)
> +		dev_err(info->dev, "Failed to add mfd %d\n", ret);

"Failed to register sub-devices"

> +	return ret;
> +}
> +
> +static const struct of_device_id mp2629_of_match[] = {
> +	{ .compatible = "mps,mp2629"},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, mp2629_of_match);
> +
> +static const struct i2c_device_id mp2629_id[] = {
> +	{ "mp2629", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, mp2629_id);

You're already using .probe_new - this can be removed.

> +static struct i2c_driver mp2629_driver = {
> +	.driver = {
> +		.name = "mp2629",
> +		.of_match_table = mp2629_of_match,
> +	},
> +	.probe_new	= mp2629_probe,
> +	.id_table	= mp2629_id,
> +};
> +module_i2c_driver(mp2629_driver);
> +
> +MODULE_AUTHOR("Saravanan Sekar <sravanhome@gmail.com>");
> +MODULE_DESCRIPTION("MP2629 Battery charger mfd driver");

"parent driver"

> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/mp2629.h b/include/linux/mfd/mp2629.h
> new file mode 100644
> index 000000000000..371e44330ba8
> --- /dev/null
> +++ b/include/linux/mfd/mp2629.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * mp2629.h  - register definitions for mp2629 charger

Remove the filename.

s/mp2629/MP2629/

> + * Copyright 2020 Monolithic Power Systems, Inc
> + *

Superfluous '\n'.

> + */
> +
> +#ifndef __MP2629_H__
> +#define __MP2629_H__
> +
> +#include <linux/types.h>
> +
> +struct device;
> +struct regmap;

Why not just add the includes?

> +struct mp2629_info {
> +	struct device *dev;
> +	struct regmap *regmap;
> +};
> +
> +#endif
saravanan sekar March 27, 2020, 8:32 a.m. UTC | #2 
Hi Lee,

On 27/03/20 8:55 am, Lee Jones wrote:
> On Sun, 22 Mar 2020, Saravanan Sekar wrote:
>
>> mp2629 is a highly-integrated switching-mode battery charge management
>> device for single-cell Li-ion or Li-polymer battery.
>>
>> Add MFD core enables chip access for ADC driver for battery readings,
>> and a power supply battery-charger driver
>>
>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>> ---
>>   drivers/mfd/Kconfig        |   9 +++
>>   drivers/mfd/Makefile       |   2 +
>>   drivers/mfd/mp2629.c       | 116 +++++++++++++++++++++++++++++++++++++
>>   include/linux/mfd/mp2629.h |  22 +++++++
>>   4 files changed, 149 insertions(+)
>>   create mode 100644 drivers/mfd/mp2629.c
>>   create mode 100644 include/linux/mfd/mp2629.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 3c547ed575e6..6614e5cff881 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -434,6 +434,15 @@ config MFD_MC13XXX_I2C
>>   	help
>>   	  Select this if your MC13xxx is connected via an I2C bus.
>>   
>> +config MFD_MP2629
>> +	bool "Monolithic power system MP2629 ADC and Battery charger"
>> +	depends on I2C
>> +	select REGMAP_I2C
>> +	help
>> +	  Select this option to enable support for monolithic power system
>> +	  battery charger. This provides ADC, thermal, battery charger power
>> +	  management functions on the systems.
>> +
>>   config MFD_MXS_LRADC
>>   	tristate "Freescale i.MX23/i.MX28 LRADC"
>>   	depends on ARCH_MXS || COMPILE_TEST
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index f935d10cbf0f..d6c210f96d02 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -170,6 +170,8 @@ obj-$(CONFIG_MFD_MAX8925)	+= max8925.o
>>   obj-$(CONFIG_MFD_MAX8997)	+= max8997.o max8997-irq.o
>>   obj-$(CONFIG_MFD_MAX8998)	+= max8998.o max8998-irq.o
>>   
>> +obj-$(CONFIG_MFD_MP2629)	+= mp2629.o
>> +
>>   pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>>   obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>>   obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>> diff --git a/drivers/mfd/mp2629.c b/drivers/mfd/mp2629.c
>> new file mode 100644
>> index 000000000000..41a4082387ce
>> --- /dev/null
>> +++ b/drivers/mfd/mp2629.c
>> @@ -0,0 +1,116 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * MP2629 MFD Driver for ADC and battery charger
> s/MFD Driver/parent driver/
>
>> + * Copyright 2020 Monolithic Power Systems, Inc
>> + *
>> + * Author: Saravanan Sekar <sravanhome@gmail.com>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/i2c.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/irq.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/mp2629.h>
> Alphabetical please.
>
>> +enum {
>> +	MP2629_MFD_ADC,
>> +	MP2629_MFD_CHARGER,
>> +	MP2629_MFD_MAX
>> +};
>> +
>> +static struct resource mp2629_irq_rsrc[] = {
>> +	{
>> +		.flags = IORESOURCE_IRQ,
>> +	},
>> +};
>> +
>> +static struct mfd_cell mp2629mfd[] = {
>> +	[MP2629_MFD_ADC] = {
>> +		.name = "mp2629_adc",
>> +		.of_compatible = "mps,mp2629_adc",
>> +	},
>> +	[MP2629_MFD_CHARGER] = {
>> +		.name = "mp2629_charger",
>> +		.of_compatible = "mps,mp2629_charger",
>> +		.resources = mp2629_irq_rsrc,
>> +		.num_resources = ARRAY_SIZE(mp2629_irq_rsrc),
>> +	}
>> +};
>> +
>> +static const struct regmap_config mp2629_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.max_register = 0x17,
>> +};
>> +
>> +static int mp2629_probe(struct i2c_client *client)
>> +{
>> +	struct mp2629_info *info;
> All this ddata instead of info.
Not sure the reason, I will do.
>
>> +	struct resource	*resources;
>> +	int ret;
>> +	int i;
>> +
>> +	info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	info->dev = &client->dev;
>> +	i2c_set_clientdata(client, info);
>> +
>> +	info->regmap = devm_regmap_init_i2c(client, &mp2629_regmap_config);
>> +	if (IS_ERR(info->regmap)) {
>> +		dev_err(info->dev, "Failed to allocate regmap!\n");
>> +		return PTR_ERR(info->regmap);
>> +	}
>> +
>> +	for (i = 0; i < MP2629_MFD_MAX; i++) {
>> +		mp2629mfd[i].platform_data = &info->regmap;
>> +		mp2629mfd[i].pdata_size = sizeof(info->regmap);
> You don't need to store this in platform data as well.
>
> You already have it in device data (ddata [currently 'info']).

"The IIO parts seems fine (minor comments inline) but I'm not keep on
directly accessing the internals of the mfd device info structure.
To my mind that should be opaque to the child drivers so as to provide
clear structure to any such accesses.

This mess in layering with the children directly using the parents
regmap is a little concerning. It means that the 3 drivers
really aren't very well separated and can't really be reviewed
independently (not a good thing)."

This is the review comments form Jonathan on V2, not to access parent 
data structure directly.
Am I misunderstood his review comments? please suggest the better option 
to follow as like in V2
or V2 + some improvements or V4 + improvements?

>> +		resources = (struct resource *)mp2629mfd[i].resources;
>> +		if (resources) {
>> +			resources[0].start = client->irq;
>> +			resources[0].end = client->irq;
>> +		}
> You don't need to store this separately either.
>
> Just fetch it from the parent in the child device driver.
>
> It will look something like (untested, off the top of my head):
>
>    platform_get_irq(to_platform_device(pdev->dev.parent), 0);
I have tested child got irq number correctly. Ok I will change it
>
>> +	}
>> +
>> +	ret = devm_mfd_add_devices(info->dev, PLATFORM_DEVID_NONE, mp2629mfd,
>> +				ARRAY_SIZE(mp2629mfd), NULL,
>> +				0, NULL);
>> +	if (ret)
>> +		dev_err(info->dev, "Failed to add mfd %d\n", ret);
> "Failed to register sub-devices"
>
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id mp2629_of_match[] = {
>> +	{ .compatible = "mps,mp2629"},
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, mp2629_of_match);
>> +
>> +static const struct i2c_device_id mp2629_id[] = {
>> +	{ "mp2629", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, mp2629_id);
> You're already using .probe_new - this can be removed.
>
>> +static struct i2c_driver mp2629_driver = {
>> +	.driver = {
>> +		.name = "mp2629",
>> +		.of_match_table = mp2629_of_match,
>> +	},
>> +	.probe_new	= mp2629_probe,
>> +	.id_table	= mp2629_id,
>> +};
>> +module_i2c_driver(mp2629_driver);
>> +
>> +MODULE_AUTHOR("Saravanan Sekar <sravanhome@gmail.com>");
>> +MODULE_DESCRIPTION("MP2629 Battery charger mfd driver");
> "parent driver"
>
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/mfd/mp2629.h b/include/linux/mfd/mp2629.h
>> new file mode 100644
>> index 000000000000..371e44330ba8
>> --- /dev/null
>> +++ b/include/linux/mfd/mp2629.h
>> @@ -0,0 +1,22 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * mp2629.h  - register definitions for mp2629 charger
> Remove the filename.
>
> s/mp2629/MP2629/
>
>> + * Copyright 2020 Monolithic Power Systems, Inc
>> + *
> Superfluous '\n'.
>
>> + */
>> +
>> +#ifndef __MP2629_H__
>> +#define __MP2629_H__
>> +
>> +#include <linux/types.h>
>> +
>> +struct device;
>> +struct regmap;
> Why not just add the includes?
Some more shared enum added in ADC driver
>> +struct mp2629_info {
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +};
>> +
>> +#endif
Thanks,
Saravanan
Lee Jones March 27, 2020, 10:22 a.m. UTC | #3 
Saravanan, Jonathan,

On Fri, 27 Mar 2020, saravanan sekar wrote:
> On 27/03/20 8:55 am, Lee Jones wrote:
> > On Sun, 22 Mar 2020, Saravanan Sekar wrote:
> > 
> > > mp2629 is a highly-integrated switching-mode battery charge management
> > > device for single-cell Li-ion or Li-polymer battery.
> > > 
> > > Add MFD core enables chip access for ADC driver for battery readings,
> > > and a power supply battery-charger driver
> > > 
> > > Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> > > ---
> > >   drivers/mfd/Kconfig        |   9 +++
> > >   drivers/mfd/Makefile       |   2 +
> > >   drivers/mfd/mp2629.c       | 116 +++++++++++++++++++++++++++++++++++++
> > >   include/linux/mfd/mp2629.h |  22 +++++++
> > >   4 files changed, 149 insertions(+)
> > >   create mode 100644 drivers/mfd/mp2629.c
> > >   create mode 100644 include/linux/mfd/mp2629.h

[...]

> > > +static int mp2629_probe(struct i2c_client *client)
> > > +{
> > > +	struct mp2629_info *info;
> > Call this ddata instead of info.
> Not sure the reason, I will do.

Because this is device data.  Info is too loose of a definition.

> > > +	struct resource	*resources;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
> > > +	if (!info)
> > > +		return -ENOMEM;
> > > +
> > > +	info->dev = &client->dev;
> > > +	i2c_set_clientdata(client, info);
> > > +
> > > +	info->regmap = devm_regmap_init_i2c(client, &mp2629_regmap_config);
> > > +	if (IS_ERR(info->regmap)) {
> > > +		dev_err(info->dev, "Failed to allocate regmap!\n");
> > > +		return PTR_ERR(info->regmap);
> > > +	}
> > > +
> > > +	for (i = 0; i < MP2629_MFD_MAX; i++) {
> > > +		mp2629mfd[i].platform_data = &info->regmap;
> > > +		mp2629mfd[i].pdata_size = sizeof(info->regmap);
> > You don't need to store this in platform data as well.
> > 
> > You already have it in device data (ddata [currently 'info']).
> 
> "The IIO parts seems fine (minor comments inline) but I'm not keep on
> directly accessing the internals of the mfd device info structure.
> To my mind that should be opaque to the child drivers so as to provide
> clear structure to any such accesses.
> 
> This mess in layering with the children directly using the parents
> regmap is a little concerning. It means that the 3 drivers
> really aren't very well separated and can't really be reviewed
> independently (not a good thing)."
> 
> This is the review comments form Jonathan on V2, not to access parent data
> structure directly.
> Am I misunderstood his review comments? please suggest the better option to
> follow as like in V2
> or V2 + some improvements or V4 + improvements?

I will take this up with Jonathan separately if necessary.

For your FYI (and Jonathan if he's Cc'ed), it's very common for a
child of an MFD to acquire resources from their parent.  That is the
point of a lot of MFDs, to obtain and register shared resources and
pass them onto their offspring.  There are 10's of examples of this.

Things like Regmaps aren't platform data, they are device/driver data,
which is usually passed though platform_set_drvdata().

[...]

> > > + */
> > > +
> > > +#ifndef __MP2629_H__
> > > +#define __MP2629_H__
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +struct device;
> > > +struct regmap;
> > Why not just add the includes?
> Some more shared enum added in ADC driver

Sorry?

> > > +struct mp2629_info {
> > > +	struct device *dev;
> > > +	struct regmap *regmap;
> > > +};
> > > +
> > > +#endif
saravanan sekar March 27, 2020, 10:56 a.m. UTC | #4 
Hi Lee,


On 27/03/20 11:22 am, Lee Jones wrote:
> Saravanan, Jonathan,
>
> On Fri, 27 Mar 2020, saravanan sekar wrote:
>> On 27/03/20 8:55 am, Lee Jones wrote:
>>> On Sun, 22 Mar 2020, Saravanan Sekar wrote:
>>>
>>>> mp2629 is a highly-integrated switching-mode battery charge management
>>>> device for single-cell Li-ion or Li-polymer battery.
>>>>
>>>> Add MFD core enables chip access for ADC driver for battery readings,
>>>> and a power supply battery-charger driver
>>>>
>>>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>>>> ---
>>>>    drivers/mfd/Kconfig        |   9 +++
>>>>    drivers/mfd/Makefile       |   2 +
>>>>    drivers/mfd/mp2629.c       | 116 +++++++++++++++++++++++++++++++++++++
>>>>    include/linux/mfd/mp2629.h |  22 +++++++
>>>>    4 files changed, 149 insertions(+)
>>>>    create mode 100644 drivers/mfd/mp2629.c
>>>>    create mode 100644 include/linux/mfd/mp2629.h
> [...]
>
>>>> +static int mp2629_probe(struct i2c_client *client)
>>>> +{
>>>> +	struct mp2629_info *info;
>>> Call this ddata instead of info.
>> Not sure the reason, I will do.
> Because this is device data.  Info is too loose of a definition.


Ok, noted

>>>> +	struct resource	*resources;
>>>> +	int ret;
>>>> +	int i;
>>>> +
>>>> +	info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
>>>> +	if (!info)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	info->dev = &client->dev;
>>>> +	i2c_set_clientdata(client, info);
>>>> +
>>>> +	info->regmap = devm_regmap_init_i2c(client, &mp2629_regmap_config);
>>>> +	if (IS_ERR(info->regmap)) {
>>>> +		dev_err(info->dev, "Failed to allocate regmap!\n");
>>>> +		return PTR_ERR(info->regmap);
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < MP2629_MFD_MAX; i++) {
>>>> +		mp2629mfd[i].platform_data = &info->regmap;
>>>> +		mp2629mfd[i].pdata_size = sizeof(info->regmap);
>>> You don't need to store this in platform data as well.
>>>
>>> You already have it in device data (ddata [currently 'info']).
>> "The IIO parts seems fine (minor comments inline) but I'm not keep on
>> directly accessing the internals of the mfd device info structure.
>> To my mind that should be opaque to the child drivers so as to provide
>> clear structure to any such accesses.
>>
>> This mess in layering with the children directly using the parents
>> regmap is a little concerning. It means that the 3 drivers
>> really aren't very well separated and can't really be reviewed
>> independently (not a good thing)."
>>
>> This is the review comments form Jonathan on V2, not to access parent data
>> structure directly.
>> Am I misunderstood his review comments? please suggest the better option to
>> follow as like in V2
>> or V2 + some improvements or V4 + improvements?
> I will take this up with Jonathan separately if necessary.
>
> For your FYI (and Jonathan if he's Cc'ed), it's very common for a
> child of an MFD to acquire resources from their parent.  That is the
> point of a lot of MFDs, to obtain and register shared resources and
> pass them onto their offspring.  There are 10's of examples of this.
>
> Things like Regmaps aren't platform data, they are device/driver data,
> which is usually passed though platform_set_drvdata().

Thanks for clarification, I will go as like in V2 sharing mfd struct to 
the childs.


> [...]
>
>>>> + */
>>>> +
>>>> +#ifndef __MP2629_H__
>>>> +#define __MP2629_H__
>>>> +
>>>> +#include <linux/types.h>
>>>> +
>>>> +struct device;
>>>> +struct regmap;
>>> Why not just add the includes?
>> Some more shared enum added in ADC driver
> Sorry?

I misunderstood your previous question that you are asking to remove 
this mp2629.h file

"No user here. (Hint: Use forward declaration of struct device instead)" 
- review comments on V1 from Andy Shevchenko.
So remove the includes


>>>> +struct mp2629_info {
>>>> +	struct device *dev;
>>>> +	struct regmap *regmap;
>>>> +};
>>>> +
>>>> +#endif


Thanks,

Saravanan
Lee Jones March 27, 2020, 11:25 a.m. UTC | #5 
On Fri, 27 Mar 2020, saravanan sekar wrote:
> On 27/03/20 11:22 am, Lee Jones wrote:
> > Saravanan, Jonathan,
> > 
> > On Fri, 27 Mar 2020, saravanan sekar wrote:
> > > On 27/03/20 8:55 am, Lee Jones wrote:
> > > > On Sun, 22 Mar 2020, Saravanan Sekar wrote:
> > > > 
> > > > > mp2629 is a highly-integrated switching-mode battery charge management
> > > > > device for single-cell Li-ion or Li-polymer battery.
> > > > > 
> > > > > Add MFD core enables chip access for ADC driver for battery readings,
> > > > > and a power supply battery-charger driver
> > > > > 
> > > > > Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> > > > > ---
> > > > >    drivers/mfd/Kconfig        |   9 +++
> > > > >    drivers/mfd/Makefile       |   2 +
> > > > >    drivers/mfd/mp2629.c       | 116 +++++++++++++++++++++++++++++++++++++
> > > > >    include/linux/mfd/mp2629.h |  22 +++++++
> > > > >    4 files changed, 149 insertions(+)
> > > > >    create mode 100644 drivers/mfd/mp2629.c
> > > > >    create mode 100644 include/linux/mfd/mp2629.h

[...]

> > > > > +#ifndef __MP2629_H__
> > > > > +#define __MP2629_H__
> > > > > +
> > > > > +#include <linux/types.h>
> > > > > +
> > > > > +struct device;
> > > > > +struct regmap;
> > > > Why not just add the includes?
> > > Some more shared enum added in ADC driver
> > Sorry?
> 
> I misunderstood your previous question that you are asking to remove this
> mp2629.h file
> 
> "No user here. (Hint: Use forward declaration of struct device instead)" -
> review comments on V1 from Andy Shevchenko.
> So remove the includes

So Andy has reviewed, but you still don't have him on Cc?

How are we meant to continue the discussion?

As a general rule I'm not a fan of forward declarations.

I think they should be avoided if at all possible.

> > > > > +struct mp2629_info {
> > > > > +	struct device *dev;
> > > > > +	struct regmap *regmap;
> > > > > +};
> > > > > +
> > > > > +#endif
saravanan sekar March 27, 2020, 12:40 p.m. UTC | #6 
Hi Lee, Andy

On 27/03/20 12:25 pm, Lee Jones wrote:
> On Fri, 27 Mar 2020, saravanan sekar wrote:
>> On 27/03/20 11:22 am, Lee Jones wrote:
>>> Saravanan, Jonathan,
>>>
>>> On Fri, 27 Mar 2020, saravanan sekar wrote:
>>>> On 27/03/20 8:55 am, Lee Jones wrote:
>>>>> On Sun, 22 Mar 2020, Saravanan Sekar wrote:
>>>>>
>>>>>> mp2629 is a highly-integrated switching-mode battery charge management
>>>>>> device for single-cell Li-ion or Li-polymer battery.
>>>>>>
>>>>>> Add MFD core enables chip access for ADC driver for battery readings,
>>>>>> and a power supply battery-charger driver
>>>>>>
>>>>>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>>>>>> ---
>>>>>>     drivers/mfd/Kconfig        |   9 +++
>>>>>>     drivers/mfd/Makefile       |   2 +
>>>>>>     drivers/mfd/mp2629.c       | 116 +++++++++++++++++++++++++++++++++++++
>>>>>>     include/linux/mfd/mp2629.h |  22 +++++++
>>>>>>     4 files changed, 149 insertions(+)
>>>>>>     create mode 100644 drivers/mfd/mp2629.c
>>>>>>     create mode 100644 include/linux/mfd/mp2629.h
> [...]
>
>>>>>> +#ifndef __MP2629_H__
>>>>>> +#define __MP2629_H__
>>>>>> +
>>>>>> +#include <linux/types.h>
>>>>>> +
>>>>>> +struct device;
>>>>>> +struct regmap;
>>>>> Why not just add the includes?
>>>> Some more shared enum added in ADC driver
>>> Sorry?
>> I misunderstood your previous question that you are asking to remove this
>> mp2629.h file
>>
>> "No user here. (Hint: Use forward declaration of struct device instead)" -
>> review comments on V1 from Andy Shevchenko.
>> So remove the includes
> So Andy has reviewed, but you still don't have him on Cc?
Sorry one of his hint made me removed him in CC unknowingly.

"For the future, hint:
         scripts/get_maintainer.pl --git --git-min-percent=67 ..."

My fault, added him in CC

> How are we meant to continue the discussion?
>
> As a general rule I'm not a fan of forward declarations.
>
> I think they should be avoided if at all possible.
Ok will add includes
>>>>>> +struct mp2629_info {
>>>>>> +	struct device *dev;
>>>>>> +	struct regmap *regmap;
>>>>>> +};
>>>>>> +
>>>>>> +#endif
Andy Shevchenko March 27, 2020, 12:56 p.m. UTC | #7 
On Fri, Mar 27, 2020 at 2:41 PM saravanan sekar <sravanhome@gmail.com> wrote:
> On 27/03/20 12:25 pm, Lee Jones wrote:
> > On Fri, 27 Mar 2020, saravanan sekar wrote:
> >> On 27/03/20 11:22 am, Lee Jones wrote:

> > So Andy has reviewed, but you still don't have him on Cc?
> Sorry one of his hint made me removed him in CC unknowingly.
>
> "For the future, hint:
>          scripts/get_maintainer.pl --git --git-min-percent=67 ..."
>

Perhaps I have to elaborate. The above hint for the initial submit.
Then the rule of thumb: Include reviewers into Cc (to the patches they
have reviewed), because you can't know their intention and maybe they
want to follow a development.

> My fault, added him in CC

No problem.

--
With Best Regards,
Andy Shevchenko
saravanan sekar March 27, 2020, 1:03 p.m. UTC | #8 
Hi Andy,

On 27/03/20 1:56 pm, Andy Shevchenko wrote:
> On Fri, Mar 27, 2020 at 2:41 PM saravanan sekar <sravanhome@gmail.com> wrote:
>> On 27/03/20 12:25 pm, Lee Jones wrote:
>>> On Fri, 27 Mar 2020, saravanan sekar wrote:
>>>> On 27/03/20 11:22 am, Lee Jones wrote:
>>> So Andy has reviewed, but you still don't have him on Cc?
>> Sorry one of his hint made me removed him in CC unknowingly.
>>
>> "For the future, hint:
>>           scripts/get_maintainer.pl --git --git-min-percent=67 ..."
>>
> Perhaps I have to elaborate. The above hint for the initial submit.
> Then the rule of thumb: Include reviewers into Cc (to the patches they
> have reviewed), because you can't know their intention and maybe they
> want to follow a development.
Thanks for more details. Between Lee has not recommended "forward 
declarations", the review comment
you gave on my V1 patch (more info on previous email). Hope you agree 
with the same.
>> My fault, added him in CC
> No problem.
>
> --
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko March 27, 2020, 1:17 p.m. UTC | #9 
On Fri, Mar 27, 2020 at 02:03:30PM +0100, saravanan sekar wrote:
> On 27/03/20 1:56 pm, Andy Shevchenko wrote:
> > On Fri, Mar 27, 2020 at 2:41 PM saravanan sekar <sravanhome@gmail.com> wrote:
> > > On 27/03/20 12:25 pm, Lee Jones wrote:
> > > > On Fri, 27 Mar 2020, saravanan sekar wrote:
> > > > > On 27/03/20 11:22 am, Lee Jones wrote:
> > > > So Andy has reviewed, but you still don't have him on Cc?
> > > Sorry one of his hint made me removed him in CC unknowingly.
> > > 
> > > "For the future, hint:
> > >           scripts/get_maintainer.pl --git --git-min-percent=67 ..."
> > > 
> > Perhaps I have to elaborate. The above hint for the initial submit.
> > Then the rule of thumb: Include reviewers into Cc (to the patches they
> > have reviewed), because you can't know their intention and maybe they
> > want to follow a development.
> Thanks for more details. Between Lee has not recommended "forward
> declarations", the review comment
> you gave on my V1 patch (more info on previous email). Hope you agree with
> the same.

I think so.
Jonathan Cameron March 28, 2020, 2:36 p.m. UTC | #10 
On Fri, 27 Mar 2020 10:22:21 +0000
Lee Jones <lee.jones@linaro.org> wrote:

> Saravanan, Jonathan,
> 
> On Fri, 27 Mar 2020, saravanan sekar wrote:
> > On 27/03/20 8:55 am, Lee Jones wrote:  
> > > On Sun, 22 Mar 2020, Saravanan Sekar wrote:
> > >   
> > > > mp2629 is a highly-integrated switching-mode battery charge management
> > > > device for single-cell Li-ion or Li-polymer battery.
> > > > 
> > > > Add MFD core enables chip access for ADC driver for battery readings,
> > > > and a power supply battery-charger driver
> > > > 
> > > > Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> > > > ---
> > > >   drivers/mfd/Kconfig        |   9 +++
> > > >   drivers/mfd/Makefile       |   2 +
> > > >   drivers/mfd/mp2629.c       | 116 +++++++++++++++++++++++++++++++++++++
> > > >   include/linux/mfd/mp2629.h |  22 +++++++
> > > >   4 files changed, 149 insertions(+)
> > > >   create mode 100644 drivers/mfd/mp2629.c
> > > >   create mode 100644 include/linux/mfd/mp2629.h  
> 
> [...]
> 
> > > > +static int mp2629_probe(struct i2c_client *client)
> > > > +{
> > > > +	struct mp2629_info *info;  
> > > Call this ddata instead of info.  
> > Not sure the reason, I will do.  
> 
> Because this is device data.  Info is too loose of a definition.
> 
> > > > +	struct resource	*resources;
> > > > +	int ret;
> > > > +	int i;
> > > > +
> > > > +	info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
> > > > +	if (!info)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	info->dev = &client->dev;
> > > > +	i2c_set_clientdata(client, info);
> > > > +
> > > > +	info->regmap = devm_regmap_init_i2c(client, &mp2629_regmap_config);
> > > > +	if (IS_ERR(info->regmap)) {
> > > > +		dev_err(info->dev, "Failed to allocate regmap!\n");
> > > > +		return PTR_ERR(info->regmap);
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < MP2629_MFD_MAX; i++) {
> > > > +		mp2629mfd[i].platform_data = &info->regmap;
> > > > +		mp2629mfd[i].pdata_size = sizeof(info->regmap);  
> > > You don't need to store this in platform data as well.
> > > 
> > > You already have it in device data (ddata [currently 'info']).  
> > 
> > "The IIO parts seems fine (minor comments inline) but I'm not keep on
> > directly accessing the internals of the mfd device info structure.
> > To my mind that should be opaque to the child drivers so as to provide
> > clear structure to any such accesses.
> > 
> > This mess in layering with the children directly using the parents
> > regmap is a little concerning. It means that the 3 drivers
> > really aren't very well separated and can't really be reviewed
> > independently (not a good thing)."
> > 
> > This is the review comments form Jonathan on V2, not to access parent data
> > structure directly.
> > Am I misunderstood his review comments? please suggest the better option to
> > follow as like in V2
> > or V2 + some improvements or V4 + improvements?  
> 
> I will take this up with Jonathan separately if necessary.
> 
> For your FYI (and Jonathan if he's Cc'ed), it's very common for a
> child of an MFD to acquire resources from their parent.  That is the
> point of a lot of MFDs, to obtain and register shared resources and
> pass them onto their offspring.  There are 10's of examples of this.
> 
> Things like Regmaps aren't platform data, they are device/driver data,
> which is usually passed though platform_set_drvdata().
> 

Fair enough.  It seemed a bit messy to have full visibility of the
parent driver structures just to access one element.

What I was actually meaning to suggest was a couple of wrapper functions,
not passing the regmap separately, but I guess that doesn't really make
any difference.  

So a read / write wrapper that just takes an abstract ddata pointer.

Fair enough if you think that's an unnecessary bit of abstraction.

Jonathan

> [...]
> 
> > > > + */
> > > > +
> > > > +#ifndef __MP2629_H__
> > > > +#define __MP2629_H__
> > > > +
> > > > +#include <linux/types.h>
> > > > +
> > > > +struct device;
> > > > +struct regmap;  
> > > Why not just add the includes?  
> > Some more shared enum added in ADC driver  
> 
> Sorry?
> 
> > > > +struct mp2629_info {
> > > > +	struct device *dev;
> > > > +	struct regmap *regmap;
> > > > +};
> > > > +
> > > > +#endif  
>
diff mbox series

Patch

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3c547ed575e6..6614e5cff881 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -434,6 +434,15 @@  config MFD_MC13XXX_I2C
 	help
 	  Select this if your MC13xxx is connected via an I2C bus.
 
+config MFD_MP2629
+	bool "Monolithic power system MP2629 ADC and Battery charger"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Select this option to enable support for monolithic power system
+	  battery charger. This provides ADC, thermal, battery charger power
+	  management functions on the systems.
+
 config MFD_MXS_LRADC
 	tristate "Freescale i.MX23/i.MX28 LRADC"
 	depends on ARCH_MXS || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index f935d10cbf0f..d6c210f96d02 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -170,6 +170,8 @@  obj-$(CONFIG_MFD_MAX8925)	+= max8925.o
 obj-$(CONFIG_MFD_MAX8997)	+= max8997.o max8997-irq.o
 obj-$(CONFIG_MFD_MAX8998)	+= max8998.o max8998-irq.o
 
+obj-$(CONFIG_MFD_MP2629)	+= mp2629.o
+
 pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
 obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
 obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
diff --git a/drivers/mfd/mp2629.c b/drivers/mfd/mp2629.c
new file mode 100644
index 000000000000..41a4082387ce
--- /dev/null
+++ b/drivers/mfd/mp2629.c
@@ -0,0 +1,116 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * MP2629 MFD Driver for ADC and battery charger
+ *
+ * Copyright 2020 Monolithic Power Systems, Inc
+ *
+ * Author: Saravanan Sekar <sravanhome@gmail.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/mp2629.h>
+
+enum {
+	MP2629_MFD_ADC,
+	MP2629_MFD_CHARGER,
+	MP2629_MFD_MAX
+};
+
+static struct resource mp2629_irq_rsrc[] = {
+	{
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
+static struct mfd_cell mp2629mfd[] = {
+	[MP2629_MFD_ADC] = {
+		.name = "mp2629_adc",
+		.of_compatible = "mps,mp2629_adc",
+	},
+	[MP2629_MFD_CHARGER] = {
+		.name = "mp2629_charger",
+		.of_compatible = "mps,mp2629_charger",
+		.resources = mp2629_irq_rsrc,
+		.num_resources = ARRAY_SIZE(mp2629_irq_rsrc),
+	}
+};
+
+static const struct regmap_config mp2629_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x17,
+};
+
+static int mp2629_probe(struct i2c_client *client)
+{
+	struct mp2629_info *info;
+	struct resource	*resources;
+	int ret;
+	int i;
+
+	info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->dev = &client->dev;
+	i2c_set_clientdata(client, info);
+
+	info->regmap = devm_regmap_init_i2c(client, &mp2629_regmap_config);
+	if (IS_ERR(info->regmap)) {
+		dev_err(info->dev, "Failed to allocate regmap!\n");
+		return PTR_ERR(info->regmap);
+	}
+
+	for (i = 0; i < MP2629_MFD_MAX; i++) {
+		mp2629mfd[i].platform_data = &info->regmap;
+		mp2629mfd[i].pdata_size = sizeof(info->regmap);
+
+		resources = (struct resource *)mp2629mfd[i].resources;
+		if (resources) {
+			resources[0].start = client->irq;
+			resources[0].end = client->irq;
+		}
+	}
+
+	ret = devm_mfd_add_devices(info->dev, PLATFORM_DEVID_NONE, mp2629mfd,
+				ARRAY_SIZE(mp2629mfd), NULL,
+				0, NULL);
+	if (ret)
+		dev_err(info->dev, "Failed to add mfd %d\n", ret);
+
+	return ret;
+}
+
+static const struct of_device_id mp2629_of_match[] = {
+	{ .compatible = "mps,mp2629"},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mp2629_of_match);
+
+static const struct i2c_device_id mp2629_id[] = {
+	{ "mp2629", },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mp2629_id);
+
+static struct i2c_driver mp2629_driver = {
+	.driver = {
+		.name = "mp2629",
+		.of_match_table = mp2629_of_match,
+	},
+	.probe_new	= mp2629_probe,
+	.id_table	= mp2629_id,
+};
+module_i2c_driver(mp2629_driver);
+
+MODULE_AUTHOR("Saravanan Sekar <sravanhome@gmail.com>");
+MODULE_DESCRIPTION("MP2629 Battery charger mfd driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/mp2629.h b/include/linux/mfd/mp2629.h
new file mode 100644
index 000000000000..371e44330ba8
--- /dev/null
+++ b/include/linux/mfd/mp2629.h
@@ -0,0 +1,22 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * mp2629.h  - register definitions for mp2629 charger
+ *
+ * Copyright 2020 Monolithic Power Systems, Inc
+ *
+ */
+
+#ifndef __MP2629_H__
+#define __MP2629_H__
+
+#include <linux/types.h>
+
+struct device;
+struct regmap;
+
+struct mp2629_info {
+	struct device *dev;
+	struct regmap *regmap;
+};
+
+#endif