* [PATCH v4 0/5] Add battery charger driver support for MP2629
@ 2020-03-22 22:46 Saravanan Sekar
2020-03-22 22:46 ` [PATCH v4 1/5] dt-bindings: mfd: add document bindings for mp2629 Saravanan Sekar
` (4 more replies)
0 siblings, 5 replies; 33+ messages in thread
From: Saravanan Sekar @ 2020-03-22 22:46 UTC (permalink / raw)
To: lee.jones, robh+dt, jic23, knaack.h, lars, pmeerw, sre
Cc: devicetree, linux-kernel, linux-iio, linux-pm, Saravanan Sekar
changes in v4:
- fixed capitalization in mfg Kconfig and documentation
changes in v3:
- regmap for children passed using platform data and remove mfd driver info
access directly from children
changes in v2:
- removed EXPORT_SYMBOL of register set/get helper
- regmap bit filed used, fixed other review comments
This patch series add support for Battery charger control driver for Monolithic
Power System's MP2629 chipset, includes MFD driver for ADC battery & input
power supply measurement and battery charger control driver.
Thanks,
Saravanan
Saravanan Sekar (5):
dt-bindings: mfd: add document bindings for mp2629
mfd: mp2629: Add support for mps battery charger
iio: adc: mp2629: Add support for mp2629 ADC driver
power: supply: Add support for mps mp2629 battery charger
MAINTAINERS: Add entry for mp2629 Battery Charger driver
.../devicetree/bindings/mfd/mps,mp2629.yaml | 60 ++
MAINTAINERS | 5 +
drivers/iio/adc/Kconfig | 10 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mp2629_adc.c | 214 ++++++
drivers/mfd/Kconfig | 9 +
drivers/mfd/Makefile | 2 +
drivers/mfd/mp2629.c | 116 +++
drivers/power/supply/Kconfig | 10 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/mp2629_charger.c | 697 ++++++++++++++++++
include/linux/mfd/mp2629.h | 31 +
12 files changed, 1156 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
create mode 100644 drivers/iio/adc/mp2629_adc.c
create mode 100644 drivers/mfd/mp2629.c
create mode 100644 drivers/power/supply/mp2629_charger.c
create mode 100644 include/linux/mfd/mp2629.h
--
2.17.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v4 1/5] dt-bindings: mfd: add document bindings for mp2629
2020-03-22 22:46 [PATCH v4 0/5] Add battery charger driver support for MP2629 Saravanan Sekar
@ 2020-03-22 22:46 ` Saravanan Sekar
2020-03-27 8:00 ` Lee Jones
2020-03-22 22:46 ` [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger Saravanan Sekar
` (3 subsequent siblings)
4 siblings, 1 reply; 33+ messages in thread
From: Saravanan Sekar @ 2020-03-22 22:46 UTC (permalink / raw)
To: lee.jones, robh+dt, jic23, knaack.h, lars, pmeerw, sre
Cc: devicetree, linux-kernel, linux-iio, linux-pm, Saravanan Sekar
Add device tree binding information for mp2629 mfd driver.
Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
---
.../devicetree/bindings/mfd/mps,mp2629.yaml | 60 +++++++++++++++++++
1 file changed, 60 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
diff --git a/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
new file mode 100644
index 000000000000..314309ea91ac
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/mps,mp2629.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MP2629 Battery Charger PMIC from Monolithic Power System.
+
+maintainers:
+ - Saravanan Sekar <sravanhome@gmail.com>
+
+description: |
+ MP2629 is an PMIC providing battery charging and power supply for smartphones,
+ wireless camera and portable devices. Chip is contrlled over I2C.
+
+ The MFD device handles battery charger controller and ADC IIO device for
+ battery, system voltage
+
+properties:
+ compatible:
+ const: mps,mp2629
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ interrupt-controller: true
+
+ "#interrupt-cells":
+ const: 2
+ description:
+ The first cell is the IRQ number, the second cell is the trigger type.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - interrupt-controller
+ - "#interrupt-cells"
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/input/linux-event-codes.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pmic@4b {
+ compatible = "mps,mp2629";
+ reg = <0x4b>;
+
+ interrupt-controller;
+ interrupt-parent = <&gpio2>;
+ #interrupt-cells = <2>;
+ interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
+ };
+ };
--
2.17.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
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-22 22:46 ` Saravanan Sekar
2020-03-23 6:08 ` kbuild test robot
` (2 more replies)
2020-03-22 22:46 ` [PATCH v4 3/5] iio: adc: mp2629: Add support for mp2629 ADC driver Saravanan Sekar
` (2 subsequent siblings)
4 siblings, 3 replies; 33+ messages in thread
From: Saravanan Sekar @ 2020-03-22 22:46 UTC (permalink / raw)
To: lee.jones, robh+dt, jic23, knaack.h, lars, pmeerw, sre
Cc: devicetree, linux-kernel, linux-iio, linux-pm, Saravanan Sekar
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
+ *
+ * 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
--
2.17.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 3/5] iio: adc: mp2629: Add support for mp2629 ADC driver
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-22 22:46 ` [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger Saravanan Sekar
@ 2020-03-22 22:46 ` Saravanan Sekar
2020-03-22 23:32 ` 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 22:46 ` [PATCH v4 5/5] MAINTAINERS: Add entry for mp2629 Battery Charger driver Saravanan Sekar
4 siblings, 1 reply; 33+ messages in thread
From: Saravanan Sekar @ 2020-03-22 22:46 UTC (permalink / raw)
To: lee.jones, robh+dt, jic23, knaack.h, lars, pmeerw, sre
Cc: devicetree, linux-kernel, linux-iio, linux-pm, Saravanan Sekar
Add support for 8-bit resolution ADC readings for input power
supply and battery charging measurement. Provides voltage, current
readings to mp2629 power supply driver.
Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
---
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mp2629_adc.c | 214 +++++++++++++++++++++++++++++++++++
include/linux/mfd/mp2629.h | 9 ++
4 files changed, 234 insertions(+)
create mode 100644 drivers/iio/adc/mp2629_adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 82e33082958c..ef0c0cd31855 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -680,6 +680,16 @@ config MESON_SARADC
To compile this driver as a module, choose M here: the
module will be called meson_saradc.
+config MP2629_ADC
+ tristate "Monolithic MP2629 ADC driver"
+ depends on MFD_MP2629
+ help
+ Say yes to have support for battery charger IC MP2629 ADC device
+ accessed over I2C.
+
+ This driver provides ADC conversion of system, input power supply
+ and battery voltage & current information.
+
config NAU7802
tristate "Nuvoton NAU7802 ADC driver"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 919228900df9..f14416c245a6 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_MCP3911) += mcp3911.o
obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
+obj-$(CONFIG_MP2629_ADC) += mp2629_adc.o
obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o
obj-$(CONFIG_NAU7802) += nau7802.o
obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
diff --git a/drivers/iio/adc/mp2629_adc.c b/drivers/iio/adc/mp2629_adc.c
new file mode 100644
index 000000000000..6967c695d177
--- /dev/null
+++ b/drivers/iio/adc/mp2629_adc.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MP2629 Driver for ADC
+ *
+ * Copyright 2020 Monolithic Power Systems, Inc
+ *
+ * Author: Saravanan Sekar <sravanhome@gmail.com>
+ */
+
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sysfs.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/machine.h>
+#include <linux/iio/driver.h>
+#include <linux/mfd/mp2629.h>
+
+#define MP2629_REG_ADC_CTRL 0x03
+#define MP2629_REG_BATT_VOLT 0x0e
+#define MP2629_REG_SYSTEM_VOLT 0x0f
+#define MP2629_REG_INPUT_VOLT 0x11
+#define MP2629_REG_BATT_CURRENT 0x12
+#define MP2629_REG_INPUT_CURRENT 0x13
+
+#define MP2629_ADC_START BIT(7)
+#define MP2629_ADC_CONTINUOUS BIT(6)
+
+#define MP2629_MAP(_mp, _mpc) IIO_MAP(#_mp, "mp2629_charger", "mp2629-"_mpc)
+
+#define MP2629_ADC_CHAN(_ch, _type) { \
+ .type = _type, \
+ .indexed = 1, \
+ .datasheet_name = #_ch, \
+ .channel = MP2629_ ## _ch, \
+ .address = MP2629_REG_ ## _ch, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+}
+
+struct mp2629_adc {
+ struct regmap *regmap;
+ struct device *dev;
+};
+
+static struct iio_chan_spec mp2629_channels[] = {
+ MP2629_ADC_CHAN(BATT_VOLT, IIO_VOLTAGE),
+ MP2629_ADC_CHAN(SYSTEM_VOLT, IIO_VOLTAGE),
+ MP2629_ADC_CHAN(INPUT_VOLT, IIO_VOLTAGE),
+ MP2629_ADC_CHAN(BATT_CURRENT, IIO_CURRENT),
+ MP2629_ADC_CHAN(INPUT_CURRENT, IIO_CURRENT)
+};
+
+static struct iio_map mp2629_adc_maps[] = {
+ MP2629_MAP(BATT_VOLT, "batt-volt"),
+ MP2629_MAP(SYSTEM_VOLT, "system-volt"),
+ MP2629_MAP(INPUT_VOLT, "input-volt"),
+ MP2629_MAP(BATT_CURRENT, "batt-current"),
+ MP2629_MAP(INPUT_CURRENT, "input-current")
+};
+
+static int mp2629_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct mp2629_adc *info = iio_priv(indio_dev);
+ unsigned int rval;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = regmap_read(info->regmap, chan->address, &rval);
+ if (ret < 0)
+ return ret;
+
+ if (chan->address == MP2629_INPUT_VOLT)
+ rval &= 0x7f;
+ *val = rval;
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->channel) {
+ case MP2629_BATT_VOLT:
+ case MP2629_SYSTEM_VOLT:
+ *val = 20;
+ return IIO_VAL_INT;
+
+ case MP2629_INPUT_VOLT:
+ *val = 60;
+ return IIO_VAL_INT;
+
+ case MP2629_BATT_CURRENT:
+ *val = 175;
+ *val2 = 10;
+ return IIO_VAL_FRACTIONAL;
+
+ case MP2629_INPUT_CURRENT:
+ *val = 133;
+ *val2 = 10;
+ return IIO_VAL_FRACTIONAL;
+
+ default:
+ return -EINVAL;
+ }
+
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static const struct iio_info mp2629_adc_info = {
+ .read_raw = &mp2629_read_raw,
+};
+
+static int mp2629_adc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ void **pdata = pdev->dev.platform_data;
+ struct mp2629_adc *info;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ info = iio_priv(indio_dev);
+ info->regmap = *pdata;
+ info->dev = dev;
+ platform_set_drvdata(pdev, indio_dev);
+
+ ret = iio_map_array_register(indio_dev, mp2629_adc_maps);
+ if (ret) {
+ dev_err(dev, "IIO maps register fail: %d\n", ret);
+ return ret;
+ }
+
+ indio_dev->name = dev_name(dev);
+ indio_dev->dev.parent = dev;
+ indio_dev->dev.of_node = pdev->dev.of_node;
+ indio_dev->channels = mp2629_channels;
+ indio_dev->num_channels = ARRAY_SIZE(mp2629_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->info = &mp2629_adc_info;
+
+ ret = regmap_update_bits(info->regmap, MP2629_REG_ADC_CTRL,
+ MP2629_ADC_START | MP2629_ADC_CONTINUOUS,
+ MP2629_ADC_START | MP2629_ADC_CONTINUOUS);
+ if (ret) {
+ dev_err(dev, "adc enable fail: %d\n", ret);
+ goto fail_unmap;
+ }
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret) {
+ dev_err(dev, "IIO device register fail: %d\n", ret);
+ goto fail_disable;
+ }
+
+ return 0;
+
+fail_disable:
+ regmap_update_bits(info->regmap, MP2629_REG_ADC_CTRL,
+ MP2629_ADC_CONTINUOUS, 0);
+ regmap_update_bits(info->regmap, MP2629_REG_ADC_CTRL,
+ MP2629_ADC_START, 0);
+
+fail_unmap:
+ iio_map_array_unregister(indio_dev);
+
+ return ret;
+}
+
+static int mp2629_adc_remove(struct platform_device *pdev)
+{
+ struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+ struct mp2629_adc *info = iio_priv(indio_dev);
+
+ regmap_update_bits(info->regmap, MP2629_REG_ADC_CTRL,
+ MP2629_ADC_CONTINUOUS, 0);
+ regmap_update_bits(info->regmap, MP2629_REG_ADC_CTRL,
+ MP2629_ADC_START, 0);
+
+ iio_map_array_unregister(indio_dev);
+ iio_device_unregister(indio_dev);
+
+ return 0;
+}
+
+static const struct of_device_id mp2629_adc_of_match[] = {
+ { .compatible = "mps,mp2629_adc"},
+ {}
+};
+MODULE_DEVICE_TABLE(of, mp2629_adc_of_match);
+
+static struct platform_driver mp2629_adc_driver = {
+ .driver = {
+ .name = "mp2629_adc",
+ .of_match_table = mp2629_adc_of_match,
+ },
+ .probe = mp2629_adc_probe,
+ .remove = mp2629_adc_remove,
+};
+module_platform_driver(mp2629_adc_driver);
+
+MODULE_AUTHOR("Saravanan Sekar <sravanhome@gmail.com>");
+MODULE_DESCRIPTION("MP2629 ADC driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/mp2629.h b/include/linux/mfd/mp2629.h
index 371e44330ba8..8b9c717e1fb0 100644
--- a/include/linux/mfd/mp2629.h
+++ b/include/linux/mfd/mp2629.h
@@ -19,4 +19,13 @@ struct mp2629_info {
struct regmap *regmap;
};
+enum mp2629_adc_chan {
+ MP2629_BATT_VOLT,
+ MP2629_SYSTEM_VOLT,
+ MP2629_INPUT_VOLT,
+ MP2629_BATT_CURRENT,
+ MP2629_INPUT_CURRENT,
+ MP2629_ADC_CHAN_END
+};
+
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger
2020-03-22 22:46 [PATCH v4 0/5] Add battery charger driver support for MP2629 Saravanan Sekar
` (2 preceding siblings ...)
2020-03-22 22:46 ` [PATCH v4 3/5] iio: adc: mp2629: Add support for mp2629 ADC driver Saravanan Sekar
@ 2020-03-22 22:46 ` Saravanan Sekar
2020-03-22 23:26 ` Andy Shevchenko
` (3 more replies)
2020-03-22 22:46 ` [PATCH v4 5/5] MAINTAINERS: Add entry for mp2629 Battery Charger driver Saravanan Sekar
4 siblings, 4 replies; 33+ messages in thread
From: Saravanan Sekar @ 2020-03-22 22:46 UTC (permalink / raw)
To: lee.jones, robh+dt, jic23, knaack.h, lars, pmeerw, sre
Cc: devicetree, linux-kernel, linux-iio, linux-pm, Saravanan Sekar
The mp2629 provides switching-mode battery charge management for
single-cell Li-ion or Li-polymer battery. Driver supports the
access/control input source and battery charging parameters.
Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
---
drivers/power/supply/Kconfig | 10 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/mp2629_charger.c | 697 ++++++++++++++++++++++++++
3 files changed, 708 insertions(+)
create mode 100644 drivers/power/supply/mp2629_charger.c
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index f3424fdce341..f45409d26d54 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -541,6 +541,16 @@ config CHARGER_MAX8998
Say Y to enable support for the battery charger control sysfs and
platform data of MAX8998/LP3974 PMICs.
+config CHARGER_MP2629
+ bool "Monolithic power system MP2629 Battery charger"
+ depends on MFD_MP2629
+ depends on MP2629_ADC
+ depends on IIO
+ help
+ Select this option to enable support for Monolithic power system
+ Battery charger. This driver provies Battery charger power management
+ functions on the systems.
+
config CHARGER_QCOM_SMBB
tristate "Qualcomm Switch-Mode Battery Charger and Boost"
depends on MFD_SPMI_PMIC || COMPILE_TEST
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 6c7da920ea83..41cb64f09e49 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_CHARGER_MAX77650) += max77650-charger.o
obj-$(CONFIG_CHARGER_MAX77693) += max77693_charger.o
obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
+obj-$(CONFIG_CHARGER_MP2629) += mp2629_charger.o
obj-$(CONFIG_CHARGER_QCOM_SMBB) += qcom_smbb.o
obj-$(CONFIG_CHARGER_BQ2415X) += bq2415x_charger.o
obj-$(CONFIG_CHARGER_BQ24190) += bq24190_charger.o
diff --git a/drivers/power/supply/mp2629_charger.c b/drivers/power/supply/mp2629_charger.c
new file mode 100644
index 000000000000..62a3ae4d1df8
--- /dev/null
+++ b/drivers/power/supply/mp2629_charger.c
@@ -0,0 +1,697 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MP2629 battery charger driver
+ *
+ * Copyright 2020 Monolithic Power Systems, Inc
+ *
+ * Author: Saravanan Sekar <sravanhome@gmail.com>
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/interrupt.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/types.h>
+#include <linux/power_supply.h>
+#include <linux/workqueue.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/core.h>
+#include <linux/mfd/mp2629.h>
+
+#define MP2629_REG_INPUT_ILIM 0x00
+#define MP2629_REG_INPUT_VLIM 0x01
+#define MP2629_REG_CHARGE_CTRL 0x04
+#define MP2629_REG_CHARGE_ILIM 0x05
+#define MP2629_REG_PRECHARGE 0x06
+#define MP2629_REG_TERM_CURRENT 0x06
+#define MP2629_REG_CHARGE_VLIM 0x07
+#define MP2629_REG_TIMER_CTRL 0x08
+#define MP2629_REG_IMPEDANCE_COMP 0x09
+#define MP2629_REG_INTERRUPT 0x0b
+#define MP2629_REG_STATUS 0x0c
+#define MP2629_REG_FAULT 0x0d
+
+#define MP2629_MASK_INPUT_TYPE 0xe0
+#define MP2629_MASK_CHARGE_TYPE 0x18
+#define MP2629_MASK_CHARGE_CTRL 0x30
+#define MP2629_MASK_WDOG_CTRL 0x30
+#define MP2629_MASK_IMPEDANCE 0xf0
+
+#define MP2629_INPUTSOURCE_CHANGE GENMASK(7, 5)
+#define MP2629_CHARGING_CHANGE GENMASK(4, 3)
+#define MP2629_FAULT_BATTERY BIT(3)
+#define MP2629_FAULT_THERMAL BIT(4)
+#define MP2629_FAULT_INPUT BIT(5)
+#define MP2629_FAULT_OTG BIT(6)
+
+#define MP2629_MAX_BATT_CAPACITY 100
+
+#define MP2629_PROPS(_idx, _min, _max, _step) \
+ [_idx] = { \
+ .min = _min, \
+ .max = _max, \
+ .step = _step, \
+}
+
+enum mp2629_source_type {
+ MP2629_SOURCE_TYPE_NO_INPUT,
+ MP2629_SOURCE_TYPE_NON_STD,
+ MP2629_SOURCE_TYPE_SDP,
+ MP2629_SOURCE_TYPE_CDP,
+ MP2629_SOURCE_TYPE_DCP,
+ MP2629_SOURCE_TYPE_OTG = 7,
+};
+
+enum mp2629_field {
+ INPUT_ILIM,
+ INPUT_VLIM,
+ CHARGE_ILIM,
+ CHARGE_VLIM,
+ PRECHARGE,
+ TERM_CURRENT,
+};
+
+struct mp2629_charger {
+ struct device *dev;
+ struct work_struct charger_work;
+ int status;
+ int fault;
+ int irq;
+
+ struct regmap *regmap;
+ struct regmap_field *regmap_fields[TERM_CURRENT + 1];
+ struct mutex lock;
+ struct power_supply *usb;
+ struct power_supply *battery;
+ struct iio_channel *iiochan[MP2629_ADC_CHAN_END];
+};
+
+struct mp2629_prop {
+ int reg;
+ int mask;
+ int min;
+ int max;
+ int step;
+ int shift;
+};
+
+static enum power_supply_usb_type mp2629_usb_types[] = {
+ POWER_SUPPLY_USB_TYPE_SDP,
+ POWER_SUPPLY_USB_TYPE_DCP,
+ POWER_SUPPLY_USB_TYPE_CDP,
+ POWER_SUPPLY_USB_TYPE_PD_DRP,
+ POWER_SUPPLY_USB_TYPE_UNKNOWN
+};
+
+static enum power_supply_property mp2629_charger_usb_props[] = {
+ POWER_SUPPLY_PROP_ONLINE,
+ POWER_SUPPLY_PROP_USB_TYPE,
+ POWER_SUPPLY_PROP_VOLTAGE_NOW,
+ POWER_SUPPLY_PROP_CURRENT_NOW,
+ POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+ POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
+};
+
+static enum power_supply_property mp2629_charger_bat_props[] = {
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_HEALTH,
+ POWER_SUPPLY_PROP_CHARGE_TYPE,
+ POWER_SUPPLY_PROP_VOLTAGE_NOW,
+ POWER_SUPPLY_PROP_CURRENT_NOW,
+ POWER_SUPPLY_PROP_CAPACITY,
+ POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
+ POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
+};
+
+static struct mp2629_prop props[] = {
+ MP2629_PROPS(INPUT_ILIM, 100000, 3250000, 50000),
+ MP2629_PROPS(INPUT_VLIM, 3800000, 5300000, 100000),
+ MP2629_PROPS(CHARGE_ILIM, 320000, 4520000, 40000),
+ MP2629_PROPS(CHARGE_VLIM, 3400000, 4670000, 10000),
+ MP2629_PROPS(PRECHARGE, 120000, 720000, 40000),
+ MP2629_PROPS(TERM_CURRENT, 80000, 680000, 40000),
+};
+
+static const struct reg_field mp2629_reg_fields[] = {
+ [INPUT_ILIM] = REG_FIELD(MP2629_REG_INPUT_ILIM, 0, 5),
+ [INPUT_VLIM] = REG_FIELD(MP2629_REG_INPUT_VLIM, 0, 3),
+ [CHARGE_ILIM] = REG_FIELD(MP2629_REG_CHARGE_ILIM, 0, 6),
+ [CHARGE_VLIM] = REG_FIELD(MP2629_REG_CHARGE_VLIM, 1, 7),
+ [PRECHARGE] = REG_FIELD(MP2629_REG_PRECHARGE, 4, 7),
+ [TERM_CURRENT] = REG_FIELD(MP2629_REG_TERM_CURRENT, 0, 3),
+};
+
+static char *adc_chan_name[] = {
+ "mp2629-batt-volt",
+ "mp2629-system-volt",
+ "mp2629-input-volt",
+ "mp2629-batt-current",
+ "mp2629-input-current",
+};
+
+static int mp2629_read_adc(struct mp2629_charger *charger,
+ enum mp2629_adc_chan ch,
+ union power_supply_propval *val)
+{
+ int ret;
+ int chval;
+
+ ret = iio_read_channel_processed(charger->iiochan[ch], &chval);
+ if (ret < 0)
+ return ret;
+
+ val->intval = chval * 1000;
+
+ return 0;
+}
+
+static int mp2629_get_prop(struct mp2629_charger *charger,
+ enum mp2629_field fld,
+ union power_supply_propval *val)
+{
+ int ret;
+ unsigned int rval;
+
+ ret = regmap_field_read(charger->regmap_fields[fld], &rval);
+ if (!ret)
+ val->intval = (rval * props[fld].step) + props[fld].min;
+
+ return ret;
+}
+
+static int mp2629_set_prop(struct mp2629_charger *charger,
+ enum mp2629_field fld,
+ const union power_supply_propval *val)
+{
+ unsigned int rval;
+
+ if (val->intval < props[fld].min || val->intval > props[fld].max)
+ return -EINVAL;
+
+ rval = (val->intval - props[fld].min) / props[fld].step;
+ return regmap_field_write(charger->regmap_fields[fld], rval);
+}
+
+static int mp2629_get_battery_capacity(struct mp2629_charger *charger,
+ union power_supply_propval *val)
+{
+ union power_supply_propval vnow, vlim;
+ int ret;
+
+ ret = mp2629_read_adc(charger, MP2629_BATT_VOLT, &vnow);
+ if (ret)
+ return ret;
+
+ ret = mp2629_get_prop(charger, CHARGE_VLIM, &vlim);
+ if (ret)
+ return ret;
+
+ val->intval = (vnow.intval * 100) / vlim.intval;
+ val->intval = min(val->intval, MP2629_MAX_BATT_CAPACITY);
+
+ return 0;
+}
+
+static int mp2629_charger_battery_get_prop(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent);
+ unsigned int rval;
+ int ret = 0;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ ret = mp2629_read_adc(charger, MP2629_BATT_VOLT, val);
+ break;
+
+ case POWER_SUPPLY_PROP_CURRENT_NOW:
+ ret = mp2629_read_adc(charger, MP2629_BATT_CURRENT, val);
+ break;
+
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+ val->intval = 4520000;
+ break;
+
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
+ val->intval = 4670000;
+ break;
+
+ case POWER_SUPPLY_PROP_CAPACITY:
+ ret = mp2629_get_battery_capacity(charger, val);
+ break;
+
+ case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
+ ret = mp2629_get_prop(charger, TERM_CURRENT, val);
+ break;
+
+ case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
+ ret = mp2629_get_prop(charger, PRECHARGE, val);
+ break;
+
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
+ ret = mp2629_get_prop(charger, CHARGE_VLIM, val);
+ break;
+
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
+ ret = mp2629_get_prop(charger, CHARGE_ILIM, val);
+ break;
+
+ case POWER_SUPPLY_PROP_HEALTH:
+ if (!charger->fault)
+ val->intval = POWER_SUPPLY_HEALTH_GOOD;
+ if (MP2629_FAULT_BATTERY & charger->fault)
+ val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+ else if (MP2629_FAULT_THERMAL & charger->fault)
+ val->intval = POWER_SUPPLY_HEALTH_OVERHEAT;
+ else if (MP2629_FAULT_INPUT & charger->fault)
+ val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+ break;
+
+ case POWER_SUPPLY_PROP_STATUS:
+ ret = regmap_read(charger->regmap, MP2629_REG_STATUS, &rval);
+ if (ret)
+ break;
+
+ rval = (rval & MP2629_MASK_CHARGE_TYPE) >> 3;
+ switch (rval) {
+ case 0x00:
+ val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+ break;
+ case 0x01:
+ case 0x10:
+ val->intval = POWER_SUPPLY_STATUS_CHARGING;
+ break;
+ case 0x11:
+ val->intval = POWER_SUPPLY_STATUS_FULL;
+ }
+ break;
+
+ case POWER_SUPPLY_PROP_CHARGE_TYPE:
+ ret = regmap_read(charger->regmap, MP2629_REG_STATUS, &rval);
+ if (ret)
+ break;
+
+ rval = (rval & MP2629_MASK_CHARGE_TYPE) >> 3;
+ switch (rval) {
+ case 0x00:
+ val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
+ break;
+ case 0x01:
+ val->intval = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
+ break;
+ case 0x10:
+ val->intval = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
+ break;
+ default:
+ val->intval = POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
+ }
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+static int mp2629_charger_battery_set_prop(struct power_supply *psy,
+ enum power_supply_property psp,
+ const union power_supply_propval *val)
+{
+ struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent);
+ int ret;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
+ ret = mp2629_set_prop(charger, TERM_CURRENT, val);
+ break;
+
+ case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
+ ret = mp2629_set_prop(charger, PRECHARGE, val);
+ break;
+
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
+ ret = mp2629_set_prop(charger, CHARGE_VLIM, val);
+ break;
+
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
+ ret = mp2629_set_prop(charger, CHARGE_ILIM, val);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+static int mp2629_charger_usb_get_prop(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent);
+ unsigned int rval;
+ int ret;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_ONLINE:
+ ret = regmap_read(charger->regmap, MP2629_REG_STATUS, &rval);
+ if (!ret)
+ val->intval = !!(rval & MP2629_MASK_INPUT_TYPE);
+ break;
+
+ case POWER_SUPPLY_PROP_USB_TYPE:
+ ret = regmap_read(charger->regmap, MP2629_REG_STATUS, &rval);
+ if (ret)
+ break;
+
+ rval = (rval & MP2629_MASK_INPUT_TYPE) >> 5;
+ switch (rval) {
+ case MP2629_SOURCE_TYPE_SDP:
+ val->intval = POWER_SUPPLY_USB_TYPE_SDP;
+ break;
+ case MP2629_SOURCE_TYPE_CDP:
+ val->intval = POWER_SUPPLY_USB_TYPE_CDP;
+ break;
+ case MP2629_SOURCE_TYPE_DCP:
+ val->intval = POWER_SUPPLY_USB_TYPE_DCP;
+ break;
+ case MP2629_SOURCE_TYPE_OTG:
+ val->intval = POWER_SUPPLY_USB_TYPE_PD_DRP;
+ break;
+ default:
+ val->intval = POWER_SUPPLY_USB_TYPE_UNKNOWN;
+ break;
+ }
+ break;
+
+ case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ ret = mp2629_read_adc(charger, MP2629_INPUT_VOLT, val);
+ break;
+
+ case POWER_SUPPLY_PROP_CURRENT_NOW:
+ ret = mp2629_read_adc(charger, MP2629_INPUT_CURRENT, val);
+ break;
+
+ case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
+ ret = mp2629_get_prop(charger, INPUT_VLIM, val);
+ break;
+
+ case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+ ret = mp2629_get_prop(charger, INPUT_ILIM, val);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+static int mp2629_charger_usb_set_prop(struct power_supply *psy,
+ enum power_supply_property psp,
+ const union power_supply_propval *val)
+{
+ struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent);
+ int ret;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
+ ret = mp2629_set_prop(charger, INPUT_VLIM, val);
+ break;
+
+ case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+ ret = mp2629_set_prop(charger, INPUT_ILIM, val);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+static int mp2629_charger_battery_prop_writeable(struct power_supply *psy,
+ enum power_supply_property psp)
+{
+ return (psp == POWER_SUPPLY_PROP_PRECHARGE_CURRENT ||
+ psp == POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT ||
+ psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT ||
+ psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE);
+}
+
+static int mp2629_charger_usb_prop_writeable(struct power_supply *psy,
+ enum power_supply_property psp)
+{
+ return (psp == POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT ||
+ psp == POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT);
+}
+
+static void mp2629_charger_work(struct work_struct *work)
+{
+ struct mp2629_charger *charger;
+ unsigned int rval;
+ int ret;
+
+ charger = container_of(work, struct mp2629_charger, charger_work);
+
+ mutex_lock(&charger->lock);
+
+ ret = regmap_read(charger->regmap, MP2629_REG_FAULT, &rval);
+ if (ret)
+ goto unlock;
+
+ if (rval) {
+ charger->fault = rval;
+ if (MP2629_FAULT_BATTERY & rval)
+ dev_err(charger->dev, "Battery fault OVP");
+ else if (MP2629_FAULT_THERMAL & rval)
+ dev_err(charger->dev, "Thermal shutdown fault");
+ else if (MP2629_FAULT_INPUT & rval)
+ dev_err(charger->dev, "no input or input OVP");
+ else if (MP2629_FAULT_OTG & rval)
+ dev_err(charger->dev, "VIN overloaded");
+
+ goto unlock;
+ }
+
+ ret = regmap_read(charger->regmap, MP2629_REG_STATUS, &rval);
+ if (ret)
+ goto unlock;
+
+ if (rval & MP2629_INPUTSOURCE_CHANGE)
+ power_supply_changed(charger->usb);
+ else if (rval & MP2629_CHARGING_CHANGE)
+ power_supply_changed(charger->battery);
+
+unlock:
+ mutex_unlock(&charger->lock);
+}
+
+static irqreturn_t mp2629_irq_handler(int irq, void *dev_id)
+{
+ struct mp2629_charger *charger = dev_id;
+
+ schedule_work(&charger->charger_work);
+ return IRQ_HANDLED;
+}
+
+static const struct power_supply_desc mp2629_usb_desc = {
+ .name = "mp2629_usb",
+ .type = POWER_SUPPLY_TYPE_USB,
+ .usb_types = mp2629_usb_types,
+ .num_usb_types = ARRAY_SIZE(mp2629_usb_types),
+ .properties = mp2629_charger_usb_props,
+ .num_properties = ARRAY_SIZE(mp2629_charger_usb_props),
+ .get_property = mp2629_charger_usb_get_prop,
+ .set_property = mp2629_charger_usb_set_prop,
+ .property_is_writeable = mp2629_charger_usb_prop_writeable,
+};
+
+static const struct power_supply_desc mp2629_battery_desc = {
+ .name = "mp2629_battery",
+ .type = POWER_SUPPLY_TYPE_BATTERY,
+ .properties = mp2629_charger_bat_props,
+ .num_properties = ARRAY_SIZE(mp2629_charger_bat_props),
+ .get_property = mp2629_charger_battery_get_prop,
+ .set_property = mp2629_charger_battery_set_prop,
+ .property_is_writeable = mp2629_charger_battery_prop_writeable,
+};
+
+static ssize_t batt_impedance_compensation_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct mp2629_charger *charger = dev_get_drvdata(dev->parent);
+ unsigned int rval;
+ int ret;
+
+ ret = regmap_read(charger->regmap, MP2629_REG_IMPEDANCE_COMP, &rval);
+ if (ret < 0)
+ return ret;
+
+ rval = (rval >> 4) * 10;
+
+ return scnprintf(buf, PAGE_SIZE, "%d mohm\n", rval);
+}
+
+static ssize_t batt_impedance_compensation_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t count)
+{
+ struct mp2629_charger *charger = dev_get_drvdata(dev->parent);
+ long val;
+ int ret;
+
+ ret = kstrtol(buf, 10, &val);
+ if (ret < 0)
+ return ret;
+
+ if (val < 0 && val > 140)
+ return -ERANGE;
+
+ /* multiples of 10 mohm so round off */
+ val = val / 10;
+ ret = regmap_update_bits(charger->regmap, MP2629_REG_IMPEDANCE_COMP,
+ MP2629_MASK_IMPEDANCE, val << 4);
+ if (ret < 0)
+ return ret;
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(batt_impedance_compensation);
+
+static struct attribute *mp2629_charger_sysfs_attrs[] = {
+ &dev_attr_batt_impedance_compensation.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(mp2629_charger_sysfs);
+
+static int mp2629_charger_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ void **pdata = pdev->dev.platform_data;
+ struct mp2629_charger *charger;
+ struct power_supply_config psy_cfg = {0};
+ int ret, i;
+
+ charger = devm_kzalloc(dev, sizeof(*charger), GFP_KERNEL);
+ if (!charger)
+ return -ENOMEM;
+
+ charger->regmap = *pdata;
+ charger->dev = dev;
+ charger->irq = platform_get_irq(pdev, 0);
+ platform_set_drvdata(pdev, charger);
+
+ for (i = 0; i <= TERM_CURRENT; i++) {
+ charger->regmap_fields[i] = devm_regmap_field_alloc(dev,
+ charger->regmap, mp2629_reg_fields[i]);
+ if (IS_ERR(charger->regmap_fields[i])) {
+ dev_err(dev, "regmap field alloc fail %d\n", i);
+ return PTR_ERR(charger->regmap_fields[i]);
+ }
+ }
+
+ for (i = 0; i < MP2629_ADC_CHAN_END; i++) {
+ charger->iiochan[i] = iio_channel_get(dev, adc_chan_name[i]);
+ if (IS_ERR(charger->iiochan[i])) {
+ ret = PTR_ERR(charger->iiochan[i]);
+ goto iio_fail;
+ }
+ }
+
+ charger->usb = devm_power_supply_register(dev, &mp2629_usb_desc, NULL);
+ if (IS_ERR(charger->usb)) {
+ ret = PTR_ERR(charger->usb);
+ goto iio_fail;
+ }
+
+ psy_cfg.drv_data = charger;
+ psy_cfg.attr_grp = mp2629_charger_sysfs_groups;
+ charger->battery = devm_power_supply_register(dev,
+ &mp2629_battery_desc, &psy_cfg);
+ if (IS_ERR(charger->battery)) {
+ ret = PTR_ERR(charger->battery);
+ goto iio_fail;
+ }
+
+ ret = regmap_update_bits(charger->regmap, MP2629_REG_CHARGE_CTRL,
+ MP2629_MASK_CHARGE_CTRL, BIT(4));
+ if (ret) {
+ dev_err(dev, "enable charge fail: %d\n", ret);
+ goto iio_fail;
+ }
+
+ regmap_update_bits(charger->regmap, MP2629_REG_TIMER_CTRL,
+ MP2629_MASK_WDOG_CTRL, 0);
+
+ INIT_WORK(&charger->charger_work, mp2629_charger_work);
+ mutex_init(&charger->lock);
+
+ if (charger->irq) {
+ ret = devm_request_irq(dev, charger->irq, mp2629_irq_handler,
+ IRQF_TRIGGER_RISING, "mp2629-charger",
+ charger);
+ if (ret) {
+ dev_err(dev, "failed to request gpio IRQ\n");
+ goto iio_fail;
+ }
+ }
+
+ regmap_update_bits(charger->regmap, MP2629_REG_INTERRUPT,
+ GENMASK(6, 5), (BIT(6) | BIT(5)));
+
+ return 0;
+
+iio_fail:
+ while (i--)
+ iio_channel_release(charger->iiochan[i]);
+
+ dev_err(dev, "driver register fail: %d\n", ret);
+ return ret;
+}
+
+static int mp2629_charger_remove(struct platform_device *pdev)
+{
+ struct mp2629_charger *charger = platform_get_drvdata(pdev);
+ int i;
+
+ cancel_work_sync(&charger->charger_work);
+
+ for (i = 0; i < MP2629_ADC_CHAN_END; i++)
+ iio_channel_release(charger->iiochan[i]);
+
+ regmap_update_bits(charger->regmap, MP2629_REG_CHARGE_CTRL,
+ MP2629_MASK_CHARGE_CTRL, 0);
+ return 0;
+}
+
+static const struct of_device_id mp2629_charger_of_match[] = {
+ { .compatible = "mps,mp2629_charger"},
+ {}
+};
+MODULE_DEVICE_TABLE(of, mp2629_charger_of_match);
+
+static struct platform_driver mp2629_charger_driver = {
+ .driver = {
+ .name = "mp2629_charger",
+ .of_match_table = mp2629_charger_of_match,
+ },
+ .probe = mp2629_charger_probe,
+ .remove = mp2629_charger_remove,
+};
+module_platform_driver(mp2629_charger_driver);
+
+MODULE_AUTHOR("Saravanan Sekar <sravanhome@gmail.com>");
+MODULE_DESCRIPTION("MP2629 Charger driver");
+MODULE_LICENSE("GPL");
--
2.17.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 5/5] MAINTAINERS: Add entry for mp2629 Battery Charger driver
2020-03-22 22:46 [PATCH v4 0/5] Add battery charger driver support for MP2629 Saravanan Sekar
` (3 preceding siblings ...)
2020-03-22 22:46 ` [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger Saravanan Sekar
@ 2020-03-22 22:46 ` Saravanan Sekar
4 siblings, 0 replies; 33+ messages in thread
From: Saravanan Sekar @ 2020-03-22 22:46 UTC (permalink / raw)
To: lee.jones, robh+dt, jic23, knaack.h, lars, pmeerw, sre
Cc: devicetree, linux-kernel, linux-iio, linux-pm, Saravanan Sekar
Add MAINTAINERS entry for Monolithic Power Systems mp2629 Charger driver.
Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
---
MAINTAINERS | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 32a95d162f06..0f82d5a7a614 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11358,10 +11358,15 @@ F: drivers/tty/mxser.*
MONOLITHIC POWER SYSTEM PMIC DRIVER
M: Saravanan Sekar <sravanhome@gmail.com>
S: Maintained
+F: Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
F: Documentation/devicetree/bindings/regulator/mps,mp*.yaml
+F: drivers/iio/adc/mp2629_adc.c
+F: drivers/mfd/mp2629.c
+F: drivers/power/supply/mp2629_charger.c
F: drivers/regulator/mp5416.c
F: drivers/regulator/mpq7920.c
F: drivers/regulator/mpq7920.h
+F: include/linux/mfd/mp2629.h
MR800 AVERMEDIA USB FM RADIO DRIVER
M: Alexey Klimov <klimov.linux@gmail.com>
--
2.17.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger
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-23 4:11 ` kbuild test robot
` (2 subsequent siblings)
3 siblings, 1 reply; 33+ messages in thread
From: Andy Shevchenko @ 2020-03-22 23:26 UTC (permalink / raw)
To: Saravanan Sekar
Cc: Lee Jones, Rob Herring, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Sebastian Reichel,
devicetree, Linux Kernel Mailing List, linux-iio, Linux PM
On Mon, Mar 23, 2020 at 12:47 AM Saravanan Sekar <sravanhome@gmail.com> wrote:
>
> The mp2629 provides switching-mode battery charge management for
> single-cell Li-ion or Li-polymer battery. Driver supports the
> access/control input source and battery charging parameters.
...
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
Do you need this one?
> +#include <linux/interrupt.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/types.h>
> +#include <linux/power_supply.h>
> +#include <linux/workqueue.h>
> +#include <linux/regmap.h>
Perhaps put them in order?
> +#include <linux/mfd/core.h>
How this is being used?
> +#include <linux/mfd/mp2629.h>
...
> +#define MP2629_MASK_INPUT_TYPE 0xe0
> +#define MP2629_MASK_CHARGE_TYPE 0x18
> +#define MP2629_MASK_CHARGE_CTRL 0x30
> +#define MP2629_MASK_WDOG_CTRL 0x30
> +#define MP2629_MASK_IMPEDANCE 0xf0
GENMASK()?
...
> + struct regmap_field *regmap_fields[TERM_CURRENT + 1];
Hmm... Why not to have a definition to cover + 1?
...
> +static int mp2629_get_prop(struct mp2629_charger *charger,
> + enum mp2629_field fld,
> + union power_supply_propval *val)
> +{
> + int ret;
> + unsigned int rval;
> +
> + ret = regmap_field_read(charger->regmap_fields[fld], &rval);
> + if (!ret)
> + val->intval = (rval * props[fld].step) + props[fld].min;
> +
> + return ret;
Why not to use standard pattern, i.e.
if (ret)
return ret;
...
return 0;
?
> +}
...
> +static int mp2629_charger_battery_set_prop(struct power_supply *psy,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent);
> + int ret;
You may replace it with in-place return statements.
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
> + ret = mp2629_set_prop(charger, TERM_CURRENT, val);
> + break;
> +
> + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
> + ret = mp2629_set_prop(charger, PRECHARGE, val);
> + break;
> +
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> + ret = mp2629_set_prop(charger, CHARGE_VLIM, val);
> + break;
> +
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> + ret = mp2629_set_prop(charger, CHARGE_ILIM, val);
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return ret;
...and drop this completely.
> +}
...
> + case POWER_SUPPLY_PROP_ONLINE:
> + ret = regmap_read(charger->regmap, MP2629_REG_STATUS, &rval);
> + if (!ret)
> + val->intval = !!(rval & MP2629_MASK_INPUT_TYPE);
> + break;
Traditional pattern?
...
> +static int mp2629_charger_usb_set_prop(struct power_supply *psy,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent);
> + int ret;
No need to have it.
> + switch (psp) {
> + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
> + ret = mp2629_set_prop(charger, INPUT_VLIM, val);
> + break;
> +
> + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> + ret = mp2629_set_prop(charger, INPUT_ILIM, val);
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return ret;
> +}
...
> + return (psp == POWER_SUPPLY_PROP_PRECHARGE_CURRENT ||
> + psp == POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT ||
> + psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT ||
> + psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE);
Redundant parentheses.
Ditto for similar cases in the driver.
...
> +static ssize_t batt_impedance_compensation_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct mp2629_charger *charger = dev_get_drvdata(dev->parent);
> + unsigned int rval;
> + int ret;
> +
> + ret = regmap_read(charger->regmap, MP2629_REG_IMPEDANCE_COMP, &rval);
> + if (ret < 0)
' < 0' is not needed.
Ditto for other cases.
> + return ret;
> +
> + rval = (rval >> 4) * 10;
> +
> + return scnprintf(buf, PAGE_SIZE, "%d mohm\n", rval);
Simple sprintf().
> +}
...
> +static ssize_t batt_impedance_compensation_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + struct mp2629_charger *charger = dev_get_drvdata(dev->parent);
> + long val;
> + int ret;
> +
> + ret = kstrtol(buf, 10, &val);
> + if (ret < 0)
No need to check for negative only.
> + return ret;
> +
> + if (val < 0 && val > 140)
> + return -ERANGE;
And what the point then to use l instead of ul or even uint variant of
the conversion above?
> + /* multiples of 10 mohm so round off */
> + val = val / 10;
> + ret = regmap_update_bits(charger->regmap, MP2629_REG_IMPEDANCE_COMP,
> + MP2629_MASK_IMPEDANCE, val << 4);
> + if (ret < 0)
> + return ret;
> +
> + return count;
> +}
...
> +static int mp2629_charger_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + void **pdata = pdev->dev.platform_data;
Why void?
Why **?
Why not to use dev_get_platdata()?
Why do we need platform data at all?
> + struct mp2629_charger *charger;
> + struct power_supply_config psy_cfg = {0};
> + int ret, i;
> + charger->regmap = *pdata;
> + regmap_update_bits(charger->regmap, MP2629_REG_INTERRUPT,
> + GENMASK(6, 5), (BIT(6) | BIT(5)));
Too many parentheses.
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 3/5] iio: adc: mp2629: Add support for mp2629 ADC driver
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
0 siblings, 1 reply; 33+ messages in thread
From: Andy Shevchenko @ 2020-03-22 23:32 UTC (permalink / raw)
To: Saravanan Sekar
Cc: Lee Jones, Rob Herring, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Sebastian Reichel,
devicetree, Linux Kernel Mailing List, linux-iio, Linux PM
On Mon, Mar 23, 2020 at 12:47 AM Saravanan Sekar <sravanhome@gmail.com> wrote:
>
> Add support for 8-bit resolution ADC readings for input power
> supply and battery charging measurement. Provides voltage, current
> readings to mp2629 power supply driver.
...
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
Don't see users of it.
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/sysfs.h>
Any users?
> +#include <linux/regmap.h>
Perhaps ordered?
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/driver.h>
+ blank line?
> +#include <linux/mfd/mp2629.h>
...
> +static int mp2629_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct mp2629_adc *info = iio_priv(indio_dev);
> + unsigned int rval;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = regmap_read(info->regmap, chan->address, &rval);
> + if (ret < 0)
> + return ret;
> +
> + if (chan->address == MP2629_INPUT_VOLT)
> + rval &= 0x7f;
GENMASK() ?
> + *val = rval;
> + return IIO_VAL_INT;
> + return 0;
> +}
...
> + void **pdata = pdev->dev.platform_data;
Same Qs as per other patch.
...
> + indio_dev->dev.of_node = pdev->dev.of_node;
Jonathan, doesn't IIO core do this for all?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger
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 4:11 ` kbuild test robot
2020-03-23 7:21 ` kbuild test robot
2020-03-23 7:30 ` kbuild test robot
3 siblings, 0 replies; 33+ messages in thread
From: kbuild test robot @ 2020-03-23 4:11 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2531 bytes --]
Hi Saravanan,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20200320]
[also build test ERROR on v5.6-rc7]
[cannot apply to ljones-mfd/for-mfd-next iio/togreg power-supply/for-next v5.6-rc6 v5.6-rc5 v5.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Saravanan-Sekar/Add-battery-charger-driver-support-for-MP2629/20200323-064940
base: d8f289c16fbb72e17b783175c1fb0942816264fd
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.2.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
sh4-linux-ld: drivers/mfd/mp2629.o: in function `mp2629_probe':
mp2629.c:(.text+0xdc): undefined reference to `__devm_regmap_init_i2c'
sh4-linux-ld: drivers/mfd/mp2629.o: in function `mp2629_driver_init':
mp2629.c:(.init.text+0x28): undefined reference to `i2c_register_driver'
sh4-linux-ld: drivers/mfd/mp2629.o: in function `mp2629_driver_exit':
mp2629.c:(.exit.text+0x18): undefined reference to `i2c_del_driver'
sh4-linux-ld: drivers/power/supply/mp2629_charger.o: in function `mp2629_charger_remove':
mp2629_charger.c:(.text+0xcc): undefined reference to `iio_channel_release'
sh4-linux-ld: drivers/power/supply/mp2629_charger.o: in function `mp2629_charger_usb_get_prop':
mp2629_charger.c:(.text+0x3dc): undefined reference to `iio_read_channel_processed'
sh4-linux-ld: drivers/power/supply/mp2629_charger.o: in function `mp2629_charger_probe':
mp2629_charger.c:(.text+0x6d4): undefined reference to `iio_channel_get'
>> sh4-linux-ld: mp2629_charger.c:(.text+0x7f8): undefined reference to `iio_channel_release'
sh4-linux-ld: drivers/power/supply/mp2629_charger.o: in function `mp2629_charger_battery_get_prop':
mp2629_charger.c:(.text+0xb9c): undefined reference to `iio_read_channel_processed'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 54342 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-22 22:46 ` [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger Saravanan Sekar
@ 2020-03-23 6:08 ` kbuild test robot
2020-03-23 9:15 ` kbuild test robot
2020-03-27 7:55 ` Lee Jones
2 siblings, 0 replies; 33+ messages in thread
From: kbuild test robot @ 2020-03-23 6:08 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]
Hi Saravanan,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20200320]
[also build test ERROR on v5.6-rc7]
[cannot apply to ljones-mfd/for-mfd-next iio/togreg power-supply/for-next v5.6-rc6 v5.6-rc5 v5.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Saravanan-Sekar/Add-battery-charger-driver-support-for-MP2629/20200323-064940
base: d8f289c16fbb72e17b783175c1fb0942816264fd
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.2.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
sh4-linux-ld: drivers/mfd/mp2629.o: in function `mp2629_probe':
mp2629.c:(.text+0xdc): undefined reference to `__devm_regmap_init_i2c'
sh4-linux-ld: drivers/mfd/mp2629.o: in function `mp2629_driver_init':
mp2629.c:(.init.text+0x28): undefined reference to `i2c_register_driver'
sh4-linux-ld: drivers/mfd/mp2629.o: in function `mp2629_driver_exit':
>> mp2629.c:(.exit.text+0x18): undefined reference to `i2c_del_driver'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 54332 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger
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 4:11 ` kbuild test robot
@ 2020-03-23 7:21 ` kbuild test robot
2020-03-23 7:30 ` kbuild test robot
3 siblings, 0 replies; 33+ messages in thread
From: kbuild test robot @ 2020-03-23 7:21 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2651 bytes --]
Hi Saravanan,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20200320]
[also build test ERROR on v5.6-rc7]
[cannot apply to ljones-mfd/for-mfd-next iio/togreg power-supply/for-next v5.6-rc6 v5.6-rc5 v5.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Saravanan-Sekar/Add-battery-charger-driver-support-for-MP2629/20200323-064940
base: d8f289c16fbb72e17b783175c1fb0942816264fd
config: s390-allmodconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 9.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.2.0 make.cross ARCH=s390
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
s390-linux-ld: drivers/mfd/mp2629.o: in function `mp2629_probe':
mp2629.c:(.text+0x86): undefined reference to `__devm_regmap_init_i2c'
s390-linux-ld: drivers/mfd/mp2629.o: in function `mp2629_driver_init':
mp2629.c:(.init.text+0x64): undefined reference to `i2c_register_driver'
s390-linux-ld: drivers/mfd/mp2629.o: in function `mp2629_driver_exit':
mp2629.c:(.exit.text+0x2a): undefined reference to `i2c_del_driver'
s390-linux-ld: drivers/power/supply/mp2629_charger.o: in function `mp2629_charger_remove':
mp2629_charger.c:(.text+0x154): undefined reference to `iio_channel_release'
s390-linux-ld: drivers/power/supply/mp2629_charger.o: in function `mp2629_charger_usb_get_prop':
mp2629_charger.c:(.text+0x810): undefined reference to `iio_read_channel_processed'
s390-linux-ld: drivers/power/supply/mp2629_charger.o: in function `mp2629_charger_probe':
mp2629_charger.c:(.text+0xee4): undefined reference to `iio_channel_get'
>> s390-linux-ld: mp2629_charger.c:(.text+0x1270): undefined reference to `iio_channel_release'
s390-linux-ld: drivers/power/supply/mp2629_charger.o: in function `mp2629_charger_battery_get_prop':
mp2629_charger.c:(.text+0x179e): undefined reference to `iio_read_channel_processed'
>> s390-linux-ld: mp2629_charger.c:(.text+0x1844): undefined reference to `iio_read_channel_processed'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 58988 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger
2020-03-22 22:46 ` [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger Saravanan Sekar
` (2 preceding siblings ...)
2020-03-23 7:21 ` kbuild test robot
@ 2020-03-23 7:30 ` kbuild test robot
3 siblings, 0 replies; 33+ messages in thread
From: kbuild test robot @ 2020-03-23 7:30 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 4200 bytes --]
Hi Saravanan,
I love your patch! Perhaps something to improve:
[auto build test WARNING on next-20200320]
[also build test WARNING on v5.6-rc7]
[cannot apply to ljones-mfd/for-mfd-next iio/togreg power-supply/for-next v5.6-rc6 v5.6-rc5 v5.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Saravanan-Sekar/Add-battery-charger-driver-support-for-MP2629/20200323-064940
base: d8f289c16fbb72e17b783175c1fb0942816264fd
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-187-gbff9b106-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/power/supply/mp2629_charger.c:584:47: sparse: sparse: Using plain integer as NULL pointer
vim +584 drivers/power/supply/mp2629_charger.c
578
579 static int mp2629_charger_probe(struct platform_device *pdev)
580 {
581 struct device *dev = &pdev->dev;
582 void **pdata = pdev->dev.platform_data;
583 struct mp2629_charger *charger;
> 584 struct power_supply_config psy_cfg = {0};
585 int ret, i;
586
587 charger = devm_kzalloc(dev, sizeof(*charger), GFP_KERNEL);
588 if (!charger)
589 return -ENOMEM;
590
591 charger->regmap = *pdata;
592 charger->dev = dev;
593 charger->irq = platform_get_irq(pdev, 0);
594 platform_set_drvdata(pdev, charger);
595
596 for (i = 0; i <= TERM_CURRENT; i++) {
597 charger->regmap_fields[i] = devm_regmap_field_alloc(dev,
598 charger->regmap, mp2629_reg_fields[i]);
599 if (IS_ERR(charger->regmap_fields[i])) {
600 dev_err(dev, "regmap field alloc fail %d\n", i);
601 return PTR_ERR(charger->regmap_fields[i]);
602 }
603 }
604
605 for (i = 0; i < MP2629_ADC_CHAN_END; i++) {
606 charger->iiochan[i] = iio_channel_get(dev, adc_chan_name[i]);
607 if (IS_ERR(charger->iiochan[i])) {
608 ret = PTR_ERR(charger->iiochan[i]);
609 goto iio_fail;
610 }
611 }
612
613 charger->usb = devm_power_supply_register(dev, &mp2629_usb_desc, NULL);
614 if (IS_ERR(charger->usb)) {
615 ret = PTR_ERR(charger->usb);
616 goto iio_fail;
617 }
618
619 psy_cfg.drv_data = charger;
620 psy_cfg.attr_grp = mp2629_charger_sysfs_groups;
621 charger->battery = devm_power_supply_register(dev,
622 &mp2629_battery_desc, &psy_cfg);
623 if (IS_ERR(charger->battery)) {
624 ret = PTR_ERR(charger->battery);
625 goto iio_fail;
626 }
627
628 ret = regmap_update_bits(charger->regmap, MP2629_REG_CHARGE_CTRL,
629 MP2629_MASK_CHARGE_CTRL, BIT(4));
630 if (ret) {
631 dev_err(dev, "enable charge fail: %d\n", ret);
632 goto iio_fail;
633 }
634
635 regmap_update_bits(charger->regmap, MP2629_REG_TIMER_CTRL,
636 MP2629_MASK_WDOG_CTRL, 0);
637
638 INIT_WORK(&charger->charger_work, mp2629_charger_work);
639 mutex_init(&charger->lock);
640
641 if (charger->irq) {
642 ret = devm_request_irq(dev, charger->irq, mp2629_irq_handler,
643 IRQF_TRIGGER_RISING, "mp2629-charger",
644 charger);
645 if (ret) {
646 dev_err(dev, "failed to request gpio IRQ\n");
647 goto iio_fail;
648 }
649 }
650
651 regmap_update_bits(charger->regmap, MP2629_REG_INTERRUPT,
652 GENMASK(6, 5), (BIT(6) | BIT(5)));
653
654 return 0;
655
656 iio_fail:
657 while (i--)
658 iio_channel_release(charger->iiochan[i]);
659
660 dev_err(dev, "driver register fail: %d\n", ret);
661 return ret;
662 }
663
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-22 22:46 ` [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger Saravanan Sekar
2020-03-23 6:08 ` kbuild test robot
@ 2020-03-23 9:15 ` kbuild test robot
2020-03-27 7:55 ` Lee Jones
2 siblings, 0 replies; 33+ messages in thread
From: kbuild test robot @ 2020-03-23 9:15 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]
Hi Saravanan,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20200320]
[also build test ERROR on v5.6-rc7]
[cannot apply to ljones-mfd/for-mfd-next iio/togreg power-supply/for-next v5.6-rc6 v5.6-rc5 v5.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Saravanan-Sekar/Add-battery-charger-driver-support-for-MP2629/20200323-064940
base: d8f289c16fbb72e17b783175c1fb0942816264fd
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.2.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
m68k-linux-ld: drivers/mfd/mp2629.o: in function `mp2629_probe':
>> mp2629.c:(.text+0x40): undefined reference to `__devm_regmap_init_i2c'
m68k-linux-ld: drivers/mfd/mp2629.o: in function `mp2629_driver_init':
>> mp2629.c:(.init.text+0xe): undefined reference to `i2c_register_driver'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 53610 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger
2020-03-22 23:26 ` Andy Shevchenko
@ 2020-03-23 12:29 ` saravanan sekar
0 siblings, 0 replies; 33+ messages in thread
From: saravanan sekar @ 2020-03-23 12:29 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, Rob Herring, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Sebastian Reichel,
devicetree, Linux Kernel Mailing List, linux-iio, Linux PM
Hi Andy,
On 23/03/20 12:26 am, Andy Shevchenko wrote:
> On Mon, Mar 23, 2020 at 12:47 AM Saravanan Sekar <sravanhome@gmail.com> wrote:
>> The mp2629 provides switching-mode battery charge management for
>> single-cell Li-ion or Li-polymer battery. Driver supports the
>> access/control input source and battery charging parameters.
> ...
>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of_device.h>
> Do you need this one?
>
>> +#include <linux/interrupt.h>
>> +#include <linux/iio/consumer.h>
>> +#include <linux/iio/types.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/regmap.h>
> Perhaps put them in order?
>
>> +#include <linux/mfd/core.h>
> How this is being used?
>
>> +#include <linux/mfd/mp2629.h>
> ...
>
>> +#define MP2629_MASK_INPUT_TYPE 0xe0
>> +#define MP2629_MASK_CHARGE_TYPE 0x18
>> +#define MP2629_MASK_CHARGE_CTRL 0x30
>> +#define MP2629_MASK_WDOG_CTRL 0x30
>> +#define MP2629_MASK_IMPEDANCE 0xf0
> GENMASK()?
>
> ...
>
>> + struct regmap_field *regmap_fields[TERM_CURRENT + 1];
> Hmm... Why not to have a definition to cover + 1?
>
> ...
>
>> +static int mp2629_get_prop(struct mp2629_charger *charger,
>> + enum mp2629_field fld,
>> + union power_supply_propval *val)
>> +{
>> + int ret;
>> + unsigned int rval;
>> +
>> + ret = regmap_field_read(charger->regmap_fields[fld], &rval);
>> + if (!ret)
>> + val->intval = (rval * props[fld].step) + props[fld].min;
>> +
>> + return ret;
> Why not to use standard pattern, i.e.
>
> if (ret)
> return ret;
> ...
> return 0;
>
> ?
>
>> +}
> ...
>
>> +static int mp2629_charger_battery_set_prop(struct power_supply *psy,
>> + enum power_supply_property psp,
>> + const union power_supply_propval *val)
>> +{
>> + struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent);
>> + int ret;
> You may replace it with in-place return statements.
>
>> +
>> + switch (psp) {
>> + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
>> + ret = mp2629_set_prop(charger, TERM_CURRENT, val);
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
>> + ret = mp2629_set_prop(charger, PRECHARGE, val);
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>> + ret = mp2629_set_prop(charger, CHARGE_VLIM, val);
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>> + ret = mp2629_set_prop(charger, CHARGE_ILIM, val);
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return ret;
> ...and drop this completely.
>
>> +}
> ...
>
>> + case POWER_SUPPLY_PROP_ONLINE:
>> + ret = regmap_read(charger->regmap, MP2629_REG_STATUS, &rval);
>> + if (!ret)
>> + val->intval = !!(rval & MP2629_MASK_INPUT_TYPE);
>> + break;
> Traditional pattern?
>
> ...
>
>> +static int mp2629_charger_usb_set_prop(struct power_supply *psy,
>> + enum power_supply_property psp,
>> + const union power_supply_propval *val)
>> +{
>> + struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent);
>> + int ret;
> No need to have it.
>
>> + switch (psp) {
>> + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
>> + ret = mp2629_set_prop(charger, INPUT_VLIM, val);
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>> + ret = mp2629_set_prop(charger, INPUT_ILIM, val);
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return ret;
>> +}
> ...
>
>> + return (psp == POWER_SUPPLY_PROP_PRECHARGE_CURRENT ||
>> + psp == POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT ||
>> + psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT ||
>> + psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE);
> Redundant parentheses.
> Ditto for similar cases in the driver.
>
> ...
>
>> +static ssize_t batt_impedance_compensation_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct mp2629_charger *charger = dev_get_drvdata(dev->parent);
>> + unsigned int rval;
>> + int ret;
>> +
>> + ret = regmap_read(charger->regmap, MP2629_REG_IMPEDANCE_COMP, &rval);
>> + if (ret < 0)
> ' < 0' is not needed.
> Ditto for other cases.
>
>> + return ret;
>> +
>> + rval = (rval >> 4) * 10;
>> +
>> + return scnprintf(buf, PAGE_SIZE, "%d mohm\n", rval);
> Simple sprintf().
>
>> +}
> ...
>
>> +static ssize_t batt_impedance_compensation_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf,
>> + size_t count)
>> +{
>> + struct mp2629_charger *charger = dev_get_drvdata(dev->parent);
>> + long val;
>> + int ret;
>> +
>> + ret = kstrtol(buf, 10, &val);
>> + if (ret < 0)
> No need to check for negative only.
>
>> + return ret;
>> +
>> + if (val < 0 && val > 140)
>> + return -ERANGE;
> And what the point then to use l instead of ul or even uint variant of
> the conversion above?
>
>> + /* multiples of 10 mohm so round off */
>> + val = val / 10;
>> + ret = regmap_update_bits(charger->regmap, MP2629_REG_IMPEDANCE_COMP,
>> + MP2629_MASK_IMPEDANCE, val << 4);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return count;
>> +}
> ...
>
>> +static int mp2629_charger_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + void **pdata = pdev->dev.platform_data;
> Why void?
> Why **?
> Why not to use dev_get_platdata()?
> Why do we need platform data at all?
I have to pass regmap (address) from mfd driver to its children, as per
review comment previous patch, mfd driver structure should not be
directly accessed in children.
Please suggest if any other better way
>> + struct mp2629_charger *charger;
>> + struct power_supply_config psy_cfg = {0};
>> + int ret, i;
>> + charger->regmap = *pdata;
>> + regmap_update_bits(charger->regmap, MP2629_REG_INTERRUPT,
>> + GENMASK(6, 5), (BIT(6) | BIT(5)));
> Too many parentheses.
>
>> +}
> --
> With Best Regards,
> Andy Shevchenko
Thanks,
Saravanan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-22 22:46 ` [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger Saravanan Sekar
2020-03-23 6:08 ` kbuild test robot
2020-03-23 9:15 ` kbuild test robot
@ 2020-03-27 7:55 ` Lee Jones
2020-03-27 8:32 ` saravanan sekar
2 siblings, 1 reply; 33+ messages in thread
From: Lee Jones @ 2020-03-27 7:55 UTC (permalink / raw)
To: Saravanan Sekar
Cc: robh+dt, jic23, knaack.h, lars, pmeerw, sre, devicetree,
linux-kernel, linux-iio, linux-pm
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/5] dt-bindings: mfd: add document bindings for mp2629
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
0 siblings, 1 reply; 33+ messages in thread
From: Lee Jones @ 2020-03-27 8:00 UTC (permalink / raw)
To: Saravanan Sekar
Cc: robh+dt, jic23, knaack.h, lars, pmeerw, sre, devicetree,
linux-kernel, linux-iio, linux-pm
On Sun, 22 Mar 2020, Saravanan Sekar wrote:
> Add device tree binding information for mp2629 mfd driver.
>
> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> ---
> .../devicetree/bindings/mfd/mps,mp2629.yaml | 60 +++++++++++++++++++
> 1 file changed, 60 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
> new file mode 100644
> index 000000000000..314309ea91ac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/mps,mp2629.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
Are these links supposed to work?
> +title: MP2629 Battery Charger PMIC from Monolithic Power System.
> +
> +maintainers:
> + - Saravanan Sekar <sravanhome@gmail.com>
> +
> +description: |
> + MP2629 is an PMIC providing battery charging and power supply for smartphones,
s/an/a/
> + wireless camera and portable devices. Chip is contrlled over I2C.
Spell check!
> + The MFD device handles battery charger controller and ADC IIO device for
> + battery, system voltage
MFD isn't a thing. We made it up. Please describe it as it is in the datasheet.
> +properties:
> + compatible:
> + const: mps,mp2629
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + interrupt-controller: true
> +
> + "#interrupt-cells":
> + const: 2
> + description:
> + The first cell is the IRQ number, the second cell is the trigger type.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - interrupt-controller
> + - "#interrupt-cells"
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/input/linux-event-codes.h>
> + i2c {
i2c@0x????????
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pmic@4b {
> + compatible = "mps,mp2629";
> + reg = <0x4b>;
> +
> + interrupt-controller;
> + interrupt-parent = <&gpio2>;
> + #interrupt-cells = <2>;
> + interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
> + };
> + };
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-27 7:55 ` Lee Jones
@ 2020-03-27 8:32 ` saravanan sekar
2020-03-27 10:22 ` Lee Jones
0 siblings, 1 reply; 33+ messages in thread
From: saravanan sekar @ 2020-03-27 8:32 UTC (permalink / raw)
To: Lee Jones
Cc: robh+dt, jic23, knaack.h, lars, pmeerw, sre, devicetree,
linux-kernel, linux-iio, linux-pm
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-27 8:32 ` saravanan sekar
@ 2020-03-27 10:22 ` Lee Jones
2020-03-27 10:56 ` saravanan sekar
2020-03-28 14:36 ` Jonathan Cameron
0 siblings, 2 replies; 33+ messages in thread
From: Lee Jones @ 2020-03-27 10:22 UTC (permalink / raw)
To: saravanan sekar
Cc: robh+dt, jic23, knaack.h, lars, pmeerw, sre, devicetree,
linux-kernel, linux-iio, linux-pm
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
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-27 10:22 ` Lee Jones
@ 2020-03-27 10:56 ` saravanan sekar
2020-03-27 11:25 ` Lee Jones
2020-03-28 14:36 ` Jonathan Cameron
1 sibling, 1 reply; 33+ messages in thread
From: saravanan sekar @ 2020-03-27 10:56 UTC (permalink / raw)
To: Lee Jones
Cc: robh+dt, jic23, knaack.h, lars, pmeerw, sre, devicetree,
linux-kernel, linux-iio, linux-pm
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-27 10:56 ` saravanan sekar
@ 2020-03-27 11:25 ` Lee Jones
2020-03-27 12:40 ` saravanan sekar
0 siblings, 1 reply; 33+ messages in thread
From: Lee Jones @ 2020-03-27 11:25 UTC (permalink / raw)
To: saravanan sekar
Cc: robh+dt, jic23, knaack.h, lars, pmeerw, sre, devicetree,
linux-kernel, linux-iio, linux-pm
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
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-27 11:25 ` Lee Jones
@ 2020-03-27 12:40 ` saravanan sekar
2020-03-27 12:56 ` Andy Shevchenko
0 siblings, 1 reply; 33+ messages in thread
From: saravanan sekar @ 2020-03-27 12:40 UTC (permalink / raw)
To: Lee Jones
Cc: robh+dt, jic23, knaack.h, lars, pmeerw, sre, devicetree,
linux-kernel, linux-iio, linux-pm, andriy.shevchenko
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-27 12:40 ` saravanan sekar
@ 2020-03-27 12:56 ` Andy Shevchenko
2020-03-27 13:03 ` saravanan sekar
0 siblings, 1 reply; 33+ messages in thread
From: Andy Shevchenko @ 2020-03-27 12:56 UTC (permalink / raw)
To: saravanan sekar
Cc: Lee Jones, Rob Herring, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Sebastian Reichel,
devicetree, Linux Kernel Mailing List, linux-iio, Linux PM,
Andy Shevchenko
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-27 12:56 ` Andy Shevchenko
@ 2020-03-27 13:03 ` saravanan sekar
2020-03-27 13:17 ` Andy Shevchenko
0 siblings, 1 reply; 33+ messages in thread
From: saravanan sekar @ 2020-03-27 13:03 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, Rob Herring, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Sebastian Reichel,
devicetree, Linux Kernel Mailing List, linux-iio, Linux PM,
Andy Shevchenko
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-27 13:03 ` saravanan sekar
@ 2020-03-27 13:17 ` Andy Shevchenko
0 siblings, 0 replies; 33+ messages in thread
From: Andy Shevchenko @ 2020-03-27 13:17 UTC (permalink / raw)
To: saravanan sekar
Cc: Lee Jones, Rob Herring, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Sebastian Reichel,
devicetree, Linux Kernel Mailing List, linux-iio, Linux PM
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/5] dt-bindings: mfd: add document bindings for mp2629
2020-03-27 8:00 ` Lee Jones
@ 2020-03-27 21:50 ` saravanan sekar
2020-03-30 6:46 ` Lee Jones
0 siblings, 1 reply; 33+ messages in thread
From: saravanan sekar @ 2020-03-27 21:50 UTC (permalink / raw)
To: Lee Jones
Cc: robh+dt, jic23, knaack.h, lars, pmeerw, sre, devicetree,
linux-kernel, linux-iio, linux-pm
Hi Lee,
On 27/03/20 9:00 am, Lee Jones wrote:
> On Sun, 22 Mar 2020, Saravanan Sekar wrote:
>
>> Add device tree binding information for mp2629 mfd driver.
>>
>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>> ---
>> .../devicetree/bindings/mfd/mps,mp2629.yaml | 60 +++++++++++++++++++
>> 1 file changed, 60 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
>> new file mode 100644
>> index 000000000000..314309ea91ac
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
>> @@ -0,0 +1,60 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/mps,mp2629.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> Are these links supposed to work?
Not really, but as far my understanding needed for dt-bindings check
>> +title: MP2629 Battery Charger PMIC from Monolithic Power System.
>> +
>> +maintainers:
>> + - Saravanan Sekar <sravanhome@gmail.com>
>> +
>> +description: |
>> + MP2629 is an PMIC providing battery charging and power supply for smartphones,
> s/an/a/
>
>> + wireless camera and portable devices. Chip is contrlled over I2C.
> Spell check!
>
>> + The MFD device handles battery charger controller and ADC IIO device for
>> + battery, system voltage
> MFD isn't a thing. We made it up. Please describe it as it is in the datasheet.
>
>> +properties:
>> + compatible:
>> + const: mps,mp2629
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + interrupt-controller: true
>> +
>> + "#interrupt-cells":
>> + const: 2
>> + description:
>> + The first cell is the IRQ number, the second cell is the trigger type.
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> + - interrupt-controller
>> + - "#interrupt-cells"
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/interrupt-controller/irq.h>
>> + #include <dt-bindings/input/linux-event-codes.h>
>> + i2c {
> i2c@0x????????
Its a I2C controller node, I don't think address is needed. Mention like
this my previous other driver patches,
dt_binding_check is also passed
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + pmic@4b {
>> + compatible = "mps,mp2629";
>> + reg = <0x4b>;
>> +
>> + interrupt-controller;
>> + interrupt-parent = <&gpio2>;
>> + #interrupt-cells = <2>;
>> + interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> + };
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger
2020-03-27 10:22 ` Lee Jones
2020-03-27 10:56 ` saravanan sekar
@ 2020-03-28 14:36 ` Jonathan Cameron
1 sibling, 0 replies; 33+ messages in thread
From: Jonathan Cameron @ 2020-03-28 14:36 UTC (permalink / raw)
To: Lee Jones
Cc: saravanan sekar, robh+dt, knaack.h, lars, pmeerw, sre,
devicetree, linux-kernel, linux-iio, linux-pm
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 3/5] iio: adc: mp2629: Add support for mp2629 ADC driver
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
0 siblings, 2 replies; 33+ messages in thread
From: Jonathan Cameron @ 2020-03-28 14:42 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Saravanan Sekar, Lee Jones, Rob Herring, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Sebastian Reichel,
devicetree, Linux Kernel Mailing List, linux-iio, Linux PM
On Mon, 23 Mar 2020 01:32:34 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Mar 23, 2020 at 12:47 AM Saravanan Sekar <sravanhome@gmail.com> wrote:
> >
> > Add support for 8-bit resolution ADC readings for input power
> > supply and battery charging measurement. Provides voltage, current
> > readings to mp2629 power supply driver.
>
> ...
>
> > +#include <linux/platform_device.h>
>
> > +#include <linux/of_device.h>
>
> Don't see users of it.
>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regulator/consumer.h>
>
> > +#include <linux/sysfs.h>
>
> Any users?
>
> > +#include <linux/regmap.h>
>
> Perhaps ordered?
>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/machine.h>
> > +#include <linux/iio/driver.h>
>
> + blank line?
>
> > +#include <linux/mfd/mp2629.h>
>
> ...
>
> > +static int mp2629_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct mp2629_adc *info = iio_priv(indio_dev);
> > + unsigned int rval;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = regmap_read(info->regmap, chan->address, &rval);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (chan->address == MP2629_INPUT_VOLT)
>
> > + rval &= 0x7f;
>
> GENMASK() ?
>
> > + *val = rval;
> > + return IIO_VAL_INT;
>
> > + return 0;
> > +}
>
> ...
>
> > + void **pdata = pdev->dev.platform_data;
>
> Same Qs as per other patch.
>
> ...
>
> > + indio_dev->dev.of_node = pdev->dev.of_node;
>
> Jonathan, doesn't IIO core do this for all?
>
Nope. I'm not totally sure it's always safe to do so
as we have some weird parent structures in some cases.
A quick grep suggests that we may be fine though, or
alternatively be able to get away with a set it if not
already set approach.
I'll take a look when I get some time. It would be nice
to clean this up.
Jonathan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 3/5] iio: adc: mp2629: Add support for mp2629 ADC driver
2020-03-28 14:42 ` Jonathan Cameron
@ 2020-03-28 18:50 ` Andy Shevchenko
2020-03-29 10:37 ` saravanan sekar
1 sibling, 0 replies; 33+ messages in thread
From: Andy Shevchenko @ 2020-03-28 18:50 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Saravanan Sekar, Lee Jones, Rob Herring, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Sebastian Reichel,
devicetree, Linux Kernel Mailing List, linux-iio, Linux PM
On Sat, Mar 28, 2020 at 4:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Mon, 23 Mar 2020 01:32:34 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Mon, Mar 23, 2020 at 12:47 AM Saravanan Sekar <sravanhome@gmail.com> wrote:
...
> > > + indio_dev->dev.of_node = pdev->dev.of_node;
> >
> > Jonathan, doesn't IIO core do this for all?
> >
>
> Nope. I'm not totally sure it's always safe to do so
> as we have some weird parent structures in some cases.
> A quick grep suggests that we may be fine though, or
> alternatively be able to get away with a set it if not
> already set approach.
>
> I'll take a look when I get some time. It would be nice
> to clean this up.
We may follow the GPIO subsystem's approach, i.e. if there is no node
provided take the one from the supplied struct device.
But it may have side effects. So, it's completely up to you.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 3/5] iio: adc: mp2629: Add support for mp2629 ADC driver
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
1 sibling, 1 reply; 33+ messages in thread
From: saravanan sekar @ 2020-03-29 10:37 UTC (permalink / raw)
To: Jonathan Cameron, Andy Shevchenko
Cc: Lee Jones, Rob Herring, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Sebastian Reichel, devicetree,
Linux Kernel Mailing List, linux-iio, Linux PM
Hi Andy,
On 28/03/20 3:42 pm, Jonathan Cameron wrote:
> On Mon, 23 Mar 2020 01:32:34 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>> On Mon, Mar 23, 2020 at 12:47 AM Saravanan Sekar <sravanhome@gmail.com> wrote:
>>> Add support for 8-bit resolution ADC readings for input power
>>> supply and battery charging measurement. Provides voltage, current
>>> readings to mp2629 power supply driver.
>> ...
>>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/of_device.h>
>> Don't see users of it.
Forgot to reply, its needed since I used struct of_device
"error: field name not in record or union initializer
{ .compatible = "mps,mp2629_charger"},"
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/sysfs.h>
>> Any users?
>>
>>> +#include <linux/regmap.h>
>> Perhaps ordered?
>>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/machine.h>
>>> +#include <linux/iio/driver.h>
>> + blank line?
>>
>>> +#include <linux/mfd/mp2629.h>
>> ...
>>
>>> +static int mp2629_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val, int *val2, long mask)
>>> +{
>>> + struct mp2629_adc *info = iio_priv(indio_dev);
>>> + unsigned int rval;
>>> + int ret;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + ret = regmap_read(info->regmap, chan->address, &rval);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (chan->address == MP2629_INPUT_VOLT)
>>> + rval &= 0x7f;
>> GENMASK() ?
>>
>>> + *val = rval;
>>> + return IIO_VAL_INT;
>>> + return 0;
>>> +}
>> ...
>>
>>> + void **pdata = pdev->dev.platform_data;
>> Same Qs as per other patch.
>>
>> ...
>>
>>> + indio_dev->dev.of_node = pdev->dev.of_node;
>> Jonathan, doesn't IIO core do this for all?
>>
> Nope. I'm not totally sure it's always safe to do so
> as we have some weird parent structures in some cases.
> A quick grep suggests that we may be fine though, or
> alternatively be able to get away with a set it if not
> already set approach.
>
> I'll take a look when I get some time. It would be nice
> to clean this up.
>
> Jonathan
>
>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 3/5] iio: adc: mp2629: Add support for mp2629 ADC driver
2020-03-29 10:37 ` saravanan sekar
@ 2020-03-29 11:09 ` Andy Shevchenko
0 siblings, 0 replies; 33+ messages in thread
From: Andy Shevchenko @ 2020-03-29 11:09 UTC (permalink / raw)
To: saravanan sekar
Cc: Jonathan Cameron, Lee Jones, Rob Herring, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Sebastian Reichel,
devicetree, Linux Kernel Mailing List, linux-iio, Linux PM
On Sun, Mar 29, 2020 at 1:37 PM saravanan sekar <sravanhome@gmail.com> wrote:
> On 28/03/20 3:42 pm, Jonathan Cameron wrote:
> > On Mon, 23 Mar 2020 01:32:34 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >> On Mon, Mar 23, 2020 at 12:47 AM Saravanan Sekar <sravanhome@gmail.com> wrote:
...
> >>> +#include <linux/of_device.h>
> >> Don't see users of it.
> Forgot to reply, its needed since I used struct of_device
> "error: field name not in record or union initializer
> { .compatible = "mps,mp2629_charger"},"
Yes, and my comment still stays. The error you get due to lack of
proper header, i.e. mod_devicetable.h.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/5] dt-bindings: mfd: add document bindings for mp2629
2020-03-27 21:50 ` saravanan sekar
@ 2020-03-30 6:46 ` Lee Jones
2020-04-07 16:29 ` saravanan sekar
0 siblings, 1 reply; 33+ messages in thread
From: Lee Jones @ 2020-03-30 6:46 UTC (permalink / raw)
To: saravanan sekar
Cc: robh+dt, jic23, knaack.h, lars, pmeerw, sre, devicetree,
linux-kernel, linux-iio, linux-pm
On Fri, 27 Mar 2020, saravanan sekar wrote:
> On 27/03/20 9:00 am, Lee Jones wrote:
> > On Sun, 22 Mar 2020, Saravanan Sekar wrote:
> >
> > > Add device tree binding information for mp2629 mfd driver.
> > >
> > > Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> > > ---
> > > .../devicetree/bindings/mfd/mps,mp2629.yaml | 60 +++++++++++++++++++
> > > 1 file changed, 60 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
> > > new file mode 100644
> > > index 000000000000..314309ea91ac
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
> > > @@ -0,0 +1,60 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mfd/mps,mp2629.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > Are these links supposed to work?
> Not really, but as far my understanding needed for dt-bindings check
Rob, why are these here if they just result in 404s?
> > > +examples:
> > > + - |
> > > + #include <dt-bindings/interrupt-controller/irq.h>
> > > + #include <dt-bindings/input/linux-event-codes.h>
> > > + i2c {
> > i2c@0x????????
>
> Its a I2C controller node, I don't think address is needed. Mention like
> this my previous other driver patches,
This doesn't sound right.
How do you control/operate the controller?
Surely you read/write from/to registers?
> dt_binding_check is also passed
>
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + pmic@4b {
> > > + compatible = "mps,mp2629";
> > > + reg = <0x4b>;
> > > +
> > > + interrupt-controller;
> > > + interrupt-parent = <&gpio2>;
> > > + #interrupt-cells = <2>;
> > > + interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
> > > + };
> > > + };
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/5] dt-bindings: mfd: add document bindings for mp2629
2020-03-30 6:46 ` Lee Jones
@ 2020-04-07 16:29 ` saravanan sekar
2020-04-08 10:15 ` Lee Jones
0 siblings, 1 reply; 33+ messages in thread
From: saravanan sekar @ 2020-04-07 16:29 UTC (permalink / raw)
To: Lee Jones
Cc: robh+dt, jic23, knaack.h, lars, pmeerw, sre, devicetree,
linux-kernel, linux-iio, linux-pm
Hi Lee,
On 30/03/20 8:46 am, Lee Jones wrote:
> On Fri, 27 Mar 2020, saravanan sekar wrote:
>> On 27/03/20 9:00 am, Lee Jones wrote:
>>> On Sun, 22 Mar 2020, Saravanan Sekar wrote:
>>>
>>>> Add device tree binding information for mp2629 mfd driver.
>>>>
>>>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>>>> ---
>>>> .../devicetree/bindings/mfd/mps,mp2629.yaml | 60 +++++++++++++++++++
>>>> 1 file changed, 60 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
>>>> new file mode 100644
>>>> index 000000000000..314309ea91ac
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
>>>> @@ -0,0 +1,60 @@
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/mfd/mps,mp2629.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> Are these links supposed to work?
>> Not really, but as far my understanding needed for dt-bindings check
> Rob, why are these here if they just result in 404s?
>
>>>> +examples:
>>>> + - |
>>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>>> + #include <dt-bindings/input/linux-event-codes.h>
>>>> + i2c {
>>> i2c@0x????????
>> Its a I2C controller node, I don't think address is needed. Mention like
>> this my previous other driver patches,
> This doesn't sound right.
>
> How do you control/operate the controller?
>
> Surely you read/write from/to registers?
Indeed, but each SoC will have different address so which address to
mention here.
For me it should be like &i2c {}, anyhow I respect maintainers review (I
can give RPi I2c bus address used for testing)
and wait for Rob's reply
>
>> dt_binding_check is also passed
>>
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> +
>>>> + pmic@4b {
>>>> + compatible = "mps,mp2629";
>>>> + reg = <0x4b>;
>>>> +
>>>> + interrupt-controller;
>>>> + interrupt-parent = <&gpio2>;
>>>> + #interrupt-cells = <2>;
>>>> + interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
>>>> + };
>>>> + };
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/5] dt-bindings: mfd: add document bindings for mp2629
2020-04-07 16:29 ` saravanan sekar
@ 2020-04-08 10:15 ` Lee Jones
0 siblings, 0 replies; 33+ messages in thread
From: Lee Jones @ 2020-04-08 10:15 UTC (permalink / raw)
To: saravanan sekar
Cc: robh+dt, jic23, knaack.h, lars, pmeerw, sre, devicetree,
linux-kernel, linux-iio, linux-pm
On Tue, 07 Apr 2020, saravanan sekar wrote:
> Hi Lee,
>
> On 30/03/20 8:46 am, Lee Jones wrote:
> > On Fri, 27 Mar 2020, saravanan sekar wrote:
> > > On 27/03/20 9:00 am, Lee Jones wrote:
> > > > On Sun, 22 Mar 2020, Saravanan Sekar wrote:
> > > >
> > > > > Add device tree binding information for mp2629 mfd driver.
> > > > >
> > > > > Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> > > > > ---
> > > > > .../devicetree/bindings/mfd/mps,mp2629.yaml | 60 +++++++++++++++++++
> > > > > 1 file changed, 60 insertions(+)
> > > > > create mode 100644 Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..314309ea91ac
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
> > > > > @@ -0,0 +1,60 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/mfd/mps,mp2629.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > Are these links supposed to work?
> > > Not really, but as far my understanding needed for dt-bindings check
> > Rob, why are these here if they just result in 404s?
> >
> > > > > +examples:
> > > > > + - |
> > > > > + #include <dt-bindings/interrupt-controller/irq.h>
> > > > > + #include <dt-bindings/input/linux-event-codes.h>
> > > > > + i2c {
> > > > i2c@0x????????
> > > Its a I2C controller node, I don't think address is needed. Mention like
> > > this my previous other driver patches,
> > This doesn't sound right.
> >
> > How do you control/operate the controller?
> >
> > Surely you read/write from/to registers?
>
>
> Indeed, but each SoC will have different address so which address to mention
> here.
> For me it should be like &i2c {}, anyhow I respect maintainers review (I can
> give RPi I2c bus address used for testing)
> and wait for Rob's reply
It's only an example. It doesn't have to match any real H/W.
> > > dt_binding_check is also passed
> > >
> > > > > + #address-cells = <1>;
> > > > > + #size-cells = <0>;
> > > > > +
> > > > > + pmic@4b {
> > > > > + compatible = "mps,mp2629";
> > > > > + reg = <0x4b>;
> > > > > +
> > > > > + interrupt-controller;
> > > > > + interrupt-parent = <&gpio2>;
> > > > > + #interrupt-cells = <2>;
> > > > > + interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
> > > > > + };
> > > > > + };
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2020-04-08 10:14 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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-04-07 16:29 ` saravanan sekar
2020-04-08 10:15 ` Lee Jones
2020-03-22 22:46 ` [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger Saravanan Sekar
2020-03-23 6:08 ` kbuild test robot
2020-03-23 9:15 ` kbuild test robot
2020-03-27 7:55 ` Lee Jones
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-23 4:11 ` kbuild test robot
2020-03-23 7:21 ` kbuild test robot
2020-03-23 7:30 ` kbuild test robot
2020-03-22 22:46 ` [PATCH v4 5/5] MAINTAINERS: Add entry for mp2629 Battery Charger driver Saravanan Sekar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.