Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Lee Jones <lee.jones@linaro.org>
To: Saravanan Sekar <sravanhome@gmail.com>
Cc: robh+dt@kernel.org, jic23@kernel.org, knaack.h@gmx.de,
	lars@metafoo.de, pmeerw@pmeerw.net, sre@kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-iio@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
Date: Fri, 27 Mar 2020 07:55:41 +0000
Message-ID: <20200327075541.GF603801@dell> (raw)
In-Reply-To: <20200322224626.13160-3-sravanhome@gmail.com>

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

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

  reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-22 22:46 [PATCH v4 0/5] Add battery charger driver support for MP2629 Saravanan Sekar
2020-03-22 22:46 ` [PATCH v4 1/5] dt-bindings: mfd: add document bindings for mp2629 Saravanan Sekar
2020-03-27  8:00   ` Lee Jones
2020-03-27 21:50     ` saravanan sekar
2020-03-30  6:46       ` Lee Jones
2020-03-22 22:46 ` [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger Saravanan Sekar
2020-03-27  7:55   ` Lee Jones [this message]
2020-03-27  8:32     ` saravanan sekar
2020-03-27 10:22       ` Lee Jones
2020-03-27 10:56         ` saravanan sekar
2020-03-27 11:25           ` Lee Jones
2020-03-27 12:40             ` saravanan sekar
2020-03-27 12:56               ` Andy Shevchenko
2020-03-27 13:03                 ` saravanan sekar
2020-03-27 13:17                   ` Andy Shevchenko
2020-03-28 14:36         ` Jonathan Cameron
2020-03-22 22:46 ` [PATCH v4 3/5] iio: adc: mp2629: Add support for mp2629 ADC driver Saravanan Sekar
2020-03-22 23:32   ` Andy Shevchenko
2020-03-28 14:42     ` Jonathan Cameron
2020-03-28 18:50       ` Andy Shevchenko
2020-03-29 10:37       ` saravanan sekar
2020-03-29 11:09         ` Andy Shevchenko
2020-03-22 22:46 ` [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger Saravanan Sekar
2020-03-22 23:26   ` Andy Shevchenko
2020-03-23 12:29     ` saravanan sekar
2020-03-22 22:46 ` [PATCH v4 5/5] MAINTAINERS: Add entry for mp2629 Battery Charger driver Saravanan Sekar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200327075541.GF603801@dell \
    --to=lee.jones@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=jic23@kernel.org \
    --cc=knaack.h@gmx.de \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=pmeerw@pmeerw.net \
    --cc=robh+dt@kernel.org \
    --cc=sravanhome@gmail.com \
    --cc=sre@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git