On Mon, 21 Sep 2015, Andrew F. Davis wrote:

On 09/19/2015 11:16 PM, Lee Jones wrote:

quoted

On Tue, 15 Sep 2015, Andrew F. Davis wrote:

quoted

The old driver does not support DT. Rewrite the driver adding DT support
and use modern kernel features such as regmap and related helpers.

Signed-off-by: Andrew F. Davis <redacted>
---
 drivers/gpio/gpio-tps65912.c           | 291 ++++++------
 drivers/mfd/Kconfig                    |  20 +-
 drivers/mfd/Makefile                   |   3 +-
 drivers/mfd/tps65912-core.c            | 288 +++++-------
 drivers/mfd/tps65912-i2c.c             | 233 ++++------
 drivers/mfd/tps65912-irq.c             | 217 ---------
 drivers/mfd/tps65912-spi.c             | 236 ++++------
 drivers/regulator/tps65912-regulator.c | 783 ++++++++++-----------------------
 include/linux/mfd/tps65912.h           | 256 +++++++----
 9 files changed, 854 insertions(+), 1473 deletions(-)
 rewrite drivers/gpio/gpio-tps65912.c (68%)
 rewrite drivers/mfd/tps65912-core.c (96%)
 rewrite drivers/mfd/tps65912-i2c.c (92%)
 delete mode 100644 drivers/mfd/tps65912-irq.c
 rewrite drivers/mfd/tps65912-spi.c (91%)
 rewrite drivers/regulator/tps65912-regulator.c (94%)

Is there really no other way to split this up?

I don't think so, not without breaking stuff, all the files are strongly
connected.

I believe you've already had this conversation with Mark, so I'll not
labour it.

[...]

quoted

quoted

+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+

No need for the '\n'.

Is it prohibited? It seems a lot over files do it this way, I personally
think it makes it cleaner to see what headers are driver local. If you
really don't like it have have no problem removing it though.

It's not a blocker.

quoted

quoted

+#include <linux/mfd/tps65912.h>
+
+#define TPS65912_IRQ(_name, _reg, _offset)	\
+	[TPS65912_IRQ_ ## _name] = {	\
+		.mask = TPS65912_ ## _reg ## _ ## _name,	\
+		.reg_offset = _offset,	\
+	}

If you find this useful, then others will too.

Please submit this to Mark Brown.

This is a really horrible macro hack I made so I didn't have to type
out the whole register name each time, I'm not sure anyone else
could use this, a better solution for most would be to use less
verbose #defines for register names.

http://www.gossamer-threads.com/lists/linux/kernel/2261088

[...]

quoted

quoted

+	.name = "tps65912",
+	.irqs = tps65912_irqs,
+	.num_irqs = ARRAY_SIZE(tps65912_irqs),
+
+	.num_regs = 4,

Why do you have 2 num_regs entries?

Not sure what you mean, the other is num_irqs?

That's what no sleep on a plane gets you.

Please ignore.

[...]

quoted

quoted

+#include <linux/mfd/tps65912.h>
+
+static const struct of_device_id tps65912_i2c_of_match_table[] = {
+	{ .compatible = "ti,tps65912", },
+	{ /*sentinel*/ }

No need for this comment.  We know what { } does.

We do, but I didn't when I first saw this kind of thing, everything is
obvious to someone, I'm not sure trying to keep the kernel as comment
bare as it is is a good idea. Plus, how many chances do you get to
use the word "sentinel" :). But I'll remove it if you have a strong
opposition to it.

Again, it's not a blocker, but I believe there are enough of these now
and they have been around long enough that any non-noob will know what
they mean.  But granted, "sentinel" is a cool word.  Either remove it
completely, or at least conform to the Kernel's single line
commentating standards (HINT: Whitespace).

quoted

quoted

+};
+
+static int tps65912_i2c_probe(struct i2c_client *client,
+			      const struct i2c_device_id *ids)
+{
+	struct tps65912 *tps;
+	const struct of_device_id *match;
+
+	match = of_match_device(tps65912_i2c_of_match_table, &client->dev);
+	if (!match) {
+		dev_err(&client->dev, "Failed to find matching DT id\n");
+		return -EINVAL;
+	}

Why are you matching?

It looks like other drivers do this so they will fail early if they were
not instantiated with DT. Here we don't need the match, so I'll just
drop this check.

They should not be doing that either.

quoted

quoted

+	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
+	if (!tps)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, tps);
+	tps->dev = &client->dev;
+	tps->irq = client->irq;
+
+	tps->regmap = devm_regmap_init_i2c(client, &tps65912_regmap_config);
+	if (IS_ERR(tps->regmap)) {
+		int ret = PTR_ERR(tps->regmap);

Neater just to declare 'int ret' up the top like we always do.

Linus has decreed we should not mix code and declerations outside of C89,
and so I will not, but ret is at the top of its scope and so is conforment
to C89. Moving it further up might be overly overly pedantic.

But if you think it would be really neater I can move it :)

Again, it's not a 'standard' per say and not a blocker, but it would
be conforming to the way we normally do things.

[...]

quoted

quoted

+	{ "tps65912", TPS65912 },

This doesn't appear to be used?

The TPS65912 part is not, but it may someday get an extra device ID,
some use just 0 for the second field, but I have TPS65912 defined so
might as well use it.

I'm not into adding code for 'may some day's.  If it's not used,
please remove it.

quoted

quoted

+	{ /*sentinel*/ },

As before.

quoted

+};
+MODULE_DEVICE_TABLE(i2c, tps65912_i2c_id_table);
+
+static struct i2c_driver tps65912_i2c_driver = {
+	.driver		= {
+		.name	= "tps65912",
+		.of_match_table = tps65912_i2c_of_match_table,
+	},
+	.probe		= tps65912_i2c_probe,
+	.remove		= tps65912_i2c_remove,
+	.id_table       = tps65912_i2c_id_table,
+};
+
+module_i2c_driver(tps65912_i2c_driver);
+
+MODULE_AUTHOR("Andrew F. Davis [off-list ref]");
+MODULE_DESCRIPTION("TPS65912x I2C Interface Driver");
+MODULE_LICENSE("GPL v2");

diff --git a/drivers/mfd/tps65912-irq.c b/drivers/mfd/tps65912-irq.c
deleted file mode 100644
index db2c29c..0000000
--- a/drivers/mfd/tps65912-irq.c
+++ /dev/null

@@ -1,217 +0,0 @@
-/*
- * tps65912-irq.c  --  TI TPS6591x
- *
- * Copyright 2011 Texas Instruments Inc.
- *
- * Author: Margarita Olaya <magi-kDsPt+C1G03kYMGBc/C6ZA@public.gmane.org>
- *
- *  This program is free software; you can redistribute it and/or modify it
- *  under  the terms of the GNU General  Public License as published by the
- *  Free Software Foundation;  either version 2 of the License, or (at your
- *  option) any later version.
- *
- * This driver is based on wm8350 implementation.
- */
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/bug.h>
-#include <linux/device.h>
-#include <linux/interrupt.h>
-#include <linux/irq.h>
-#include <linux/gpio.h>
-#include <linux/mfd/tps65912.h>
-
-static inline int irq_to_tps65912_irq(struct tps65912 *tps65912,
-							int irq)
-{
-	return irq - tps65912->irq_base;
-}
-
-/*
- * This is a threaded IRQ handler so can access I2C/SPI.  Since the
- * IRQ handler explicitly clears the IRQ it handles the IRQ line
- * will be reasserted and the physical IRQ will be handled again if
- * another interrupt is asserted while we run - in the normal course
- * of events this is a rare occurrence so we save I2C/SPI reads. We're
- * also assuming that it's rare to get lots of interrupts firing
- * simultaneously so try to minimise I/O.
- */
-static irqreturn_t tps65912_irq(int irq, void *irq_data)
-{
-	struct tps65912 *tps65912 = irq_data;
-	u32 irq_sts;
-	u32 irq_mask;
-	u8 reg;
-	int i;
-
-
-	tps65912->read(tps65912, TPS65912_INT_STS, 1, &reg);
-	irq_sts = reg;
-	tps65912->read(tps65912, TPS65912_INT_STS2, 1, &reg);
-	irq_sts |= reg << 8;
-	tps65912->read(tps65912, TPS65912_INT_STS3, 1, &reg);
-	irq_sts |= reg << 16;
-	tps65912->read(tps65912, TPS65912_INT_STS4, 1, &reg);
-	irq_sts |= reg << 24;
-
-	tps65912->read(tps65912, TPS65912_INT_MSK, 1, &reg);
-	irq_mask = reg;
-	tps65912->read(tps65912, TPS65912_INT_MSK2, 1, &reg);
-	irq_mask |= reg << 8;
-	tps65912->read(tps65912, TPS65912_INT_MSK3, 1, &reg);
-	irq_mask |= reg << 16;
-	tps65912->read(tps65912, TPS65912_INT_MSK4, 1, &reg);
-	irq_mask |= reg << 24;
-
-	irq_sts &= ~irq_mask;
-	if (!irq_sts)
-		return IRQ_NONE;
-
-	for (i = 0; i < tps65912->irq_num; i++) {
-		if (!(irq_sts & (1 << i)))
-			continue;
-
-		handle_nested_irq(tps65912->irq_base + i);
-	}
-
-	/* Write the STS register back to clear IRQs we handled */
-	reg = irq_sts & 0xFF;
-	irq_sts >>= 8;
-	if (reg)
-		tps65912->write(tps65912, TPS65912_INT_STS, 1, &reg);
-	reg = irq_sts & 0xFF;
-	irq_sts >>= 8;
-	if (reg)
-		tps65912->write(tps65912, TPS65912_INT_STS2, 1, &reg);
-	reg = irq_sts & 0xFF;
-	irq_sts >>= 8;
-	if (reg)
-		tps65912->write(tps65912, TPS65912_INT_STS3, 1, &reg);
-	reg = irq_sts & 0xFF;
-	if (reg)
-		tps65912->write(tps65912, TPS65912_INT_STS4, 1, &reg);
-
-	return IRQ_HANDLED;
-}
-
-static void tps65912_irq_lock(struct irq_data *data)
-{
-	struct tps65912 *tps65912 = irq_data_get_irq_chip_data(data);
-
-	mutex_lock(&tps65912->irq_lock);
-}
-
-static void tps65912_irq_sync_unlock(struct irq_data *data)
-{
-	struct tps65912 *tps65912 = irq_data_get_irq_chip_data(data);
-	u32 reg_mask;
-	u8 reg;
-
-	tps65912->read(tps65912, TPS65912_INT_MSK, 1, &reg);
-	reg_mask = reg;
-	tps65912->read(tps65912, TPS65912_INT_MSK2, 1, &reg);
-	reg_mask |= reg << 8;
-	tps65912->read(tps65912, TPS65912_INT_MSK3, 1, &reg);
-	reg_mask |= reg << 16;
-	tps65912->read(tps65912, TPS65912_INT_MSK4, 1, &reg);
-	reg_mask |= reg << 24;
-
-	if (tps65912->irq_mask != reg_mask) {
-		reg = tps65912->irq_mask & 0xFF;
-		tps65912->write(tps65912, TPS65912_INT_MSK, 1, &reg);
-		reg = tps65912->irq_mask >> 8 & 0xFF;
-		tps65912->write(tps65912, TPS65912_INT_MSK2, 1, &reg);
-		reg = tps65912->irq_mask >> 16 & 0xFF;
-		tps65912->write(tps65912, TPS65912_INT_MSK3, 1, &reg);
-		reg = tps65912->irq_mask >> 24 & 0xFF;
-		tps65912->write(tps65912, TPS65912_INT_MSK4, 1, &reg);
-	}
-
-	mutex_unlock(&tps65912->irq_lock);
-}
-
-static void tps65912_irq_enable(struct irq_data *data)
-{
-	struct tps65912 *tps65912 = irq_data_get_irq_chip_data(data);
-
-	tps65912->irq_mask &= ~(1 << irq_to_tps65912_irq(tps65912, data->irq));
-}
-
-static void tps65912_irq_disable(struct irq_data *data)
-{
-	struct tps65912 *tps65912 = irq_data_get_irq_chip_data(data);
-
-	tps65912->irq_mask |= (1 << irq_to_tps65912_irq(tps65912, data->irq));
-}
-
-static struct irq_chip tps65912_irq_chip = {
-	.name = "tps65912",
-	.irq_bus_lock = tps65912_irq_lock,
-	.irq_bus_sync_unlock = tps65912_irq_sync_unlock,
-	.irq_disable = tps65912_irq_disable,
-	.irq_enable = tps65912_irq_enable,
-};
-
-int tps65912_irq_init(struct tps65912 *tps65912, int irq,
-			    struct tps65912_platform_data *pdata)
-{
-	int ret, cur_irq;
-	int flags = IRQF_ONESHOT;
-	u8 reg;
-
-	if (!irq) {
-		dev_warn(tps65912->dev, "No interrupt support, no core IRQ\n");
-		return 0;
-	}
-
-	if (!pdata || !pdata->irq_base) {
-		dev_warn(tps65912->dev, "No interrupt support, no IRQ base\n");
-		return 0;
-	}
-
-	/* Clear unattended interrupts */
-	tps65912->read(tps65912, TPS65912_INT_STS, 1, &reg);
-	tps65912->write(tps65912, TPS65912_INT_STS, 1, &reg);
-	tps65912->read(tps65912, TPS65912_INT_STS2, 1, &reg);
-	tps65912->write(tps65912, TPS65912_INT_STS2, 1, &reg);
-	tps65912->read(tps65912, TPS65912_INT_STS3, 1, &reg);
-	tps65912->write(tps65912, TPS65912_INT_STS3, 1, &reg);
-	tps65912->read(tps65912, TPS65912_INT_STS4, 1, &reg);
-	tps65912->write(tps65912, TPS65912_INT_STS4, 1, &reg);
-
-	/* Mask top level interrupts */
-	tps65912->irq_mask = 0xFFFFFFFF;
-
-	mutex_init(&tps65912->irq_lock);
-	tps65912->chip_irq = irq;
-	tps65912->irq_base = pdata->irq_base;
-
-	tps65912->irq_num = TPS65912_NUM_IRQ;
-
-	/* Register with genirq */
-	for (cur_irq = tps65912->irq_base;
-	     cur_irq < tps65912->irq_num + tps65912->irq_base;
-	     cur_irq++) {
-		irq_set_chip_data(cur_irq, tps65912);
-		irq_set_chip_and_handler(cur_irq, &tps65912_irq_chip,
-					 handle_edge_irq);
-		irq_set_nested_thread(cur_irq, 1);
-		irq_clear_status_flags(cur_irq, IRQ_NOREQUEST | IRQ_NOPROBE);
-	}
-
-	ret = request_threaded_irq(irq, NULL, tps65912_irq, flags,
-				   "tps65912", tps65912);
-
-	irq_set_irq_type(irq, IRQ_TYPE_LEVEL_LOW);
-	if (ret != 0)
-		dev_err(tps65912->dev, "Failed to request IRQ: %d\n", ret);
-
-	return ret;
-}
-
-int tps65912_irq_exit(struct tps65912 *tps65912)
-{
-	free_irq(tps65912->chip_irq, tps65912);
-	return 0;
-}

diff --git a/drivers/mfd/tps65912-spi.c b/drivers/mfd/tps65912-spi.c
dissimilarity index 91%
index de60ad9..d6ab929 100644
--- a/drivers/mfd/tps65912-spi.c
+++ b/drivers/mfd/tps65912-spi.c

@@ -1,141 +1,95 @@
-/*
- * tps65912-spi.c  --  SPI access for TI TPS65912x PMIC
- *
- * Copyright 2011 Texas Instruments Inc.
- *
- * Author: Margarita Olaya Cabrera <magi-kDsPt+C1G03kYMGBc/C6ZA@public.gmane.org>
- *
- *  This program is free software; you can redistribute it and/or modify it
- *  under  the terms of the GNU General  Public License as published by the
- *  Free Software Foundation;  either version 2 of the License, or (at your
- *  option) any later version.
- *
- *  This driver is based on wm8350 implementation.
- */
-
-#include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/init.h>
-#include <linux/slab.h>
-#include <linux/gpio.h>
-#include <linux/spi/spi.h>
-#include <linux/mfd/core.h>
-#include <linux/mfd/tps65912.h>
-
-static int tps65912_spi_write(struct tps65912 *tps65912, u8 addr,
-							int bytes, void *src)
-{
-	struct spi_device *spi = tps65912->control_data;
-	u8 *data = (u8 *) src;
-	int ret;
-	/* bit 23 is the read/write bit */
-	unsigned long spi_data = 1 << 23 | addr << 15 | *data;
-	struct spi_transfer xfer;
-	struct spi_message msg;
-	u32 tx_buf;
-
-	tx_buf = spi_data;
-
-	xfer.tx_buf	= &tx_buf;
-	xfer.rx_buf	= NULL;
-	xfer.len	= sizeof(unsigned long);
-	xfer.bits_per_word = 24;
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&xfer, &msg);
-
-	ret = spi_sync(spi, &msg);
-	return ret;
-}
-
-static int tps65912_spi_read(struct tps65912 *tps65912, u8 addr,
-							int bytes, void *dest)
-{
-	struct spi_device *spi = tps65912->control_data;
-	/* bit 23 is the read/write bit */
-	unsigned long spi_data = 0 << 23 | addr << 15;
-	struct spi_transfer xfer;
-	struct spi_message msg;
-	int ret;
-	u8 *data = (u8 *) dest;
-	u32 tx_buf, rx_buf;
-
-	tx_buf = spi_data;
-	rx_buf = 0;
-
-	xfer.tx_buf	= &tx_buf;
-	xfer.rx_buf	= &rx_buf;
-	xfer.len	= sizeof(unsigned long);
-	xfer.bits_per_word = 24;
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&xfer, &msg);
-
-	if (spi == NULL)
-		return 0;
-
-	ret = spi_sync(spi, &msg);
-	if (ret == 0)
-		*data = (u8) (rx_buf & 0xFF);
-	return ret;
-}
-
-static int tps65912_spi_probe(struct spi_device *spi)
-{
-	struct tps65912 *tps65912;
-
-	tps65912 = devm_kzalloc(&spi->dev,
-				sizeof(struct tps65912), GFP_KERNEL);
-	if (tps65912 == NULL)
-		return -ENOMEM;
-
-	tps65912->dev = &spi->dev;
-	tps65912->control_data = spi;
-	tps65912->read = tps65912_spi_read;
-	tps65912->write = tps65912_spi_write;
-
-	spi_set_drvdata(spi, tps65912);
-
-	return tps65912_device_init(tps65912);
-}
-
-static int tps65912_spi_remove(struct spi_device *spi)
-{
-	struct tps65912 *tps65912 = spi_get_drvdata(spi);
-
-	tps65912_device_exit(tps65912);
-
-	return 0;
-}
-
-static struct spi_driver tps65912_spi_driver = {
-	.driver = {
-		.name = "tps65912",
-		.owner = THIS_MODULE,
-	},
-	.probe	= tps65912_spi_probe,
-	.remove = tps65912_spi_remove,
-};
-
-static int __init tps65912_spi_init(void)
-{
-	int ret;
-
-	ret = spi_register_driver(&tps65912_spi_driver);
-	if (ret != 0)
-		pr_err("Failed to register TPS65912 SPI driver: %d\n", ret);
-
-	return 0;
-}
-/* init early so consumer devices can complete system boot */
-subsys_initcall(tps65912_spi_init);
-
-static void __exit tps65912_spi_exit(void)
-{
-	spi_unregister_driver(&tps65912_spi_driver);
-}
-module_exit(tps65912_spi_exit);
-
-MODULE_AUTHOR("Margarita Olaya	<magi-kDsPt+C1G03kYMGBc/C6ZA@public.gmane.org>");
-MODULE_DESCRIPTION("SPI support for TPS65912 chip family mfd");
-MODULE_LICENSE("GPL");
+/*
+ * tps65912-spi.c -- SPI access driver for TI TPS65912x PMIC

All the same comments as the other files -- I'll not labour them.

Same goes for the rest of the file.

ACK

quoted

quoted

+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Author: Andrew F. Davis [off-list ref]
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether expressed or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License version 2 for more details.
+ *
+ * Based on the TPS65218 driver and the previous TPS65912 driver by
+ * Margarita Olaya Cabrera [off-list ref]
+ */
+
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>

Alphabetical.

ACK

quoted

quoted

+#include <linux/mfd/tps65912.h>
+
+static const struct of_device_id tps65912_spi_of_match_table[] = {
+	{ .compatible = "ti,tps65912", },
+	{ /*sentinel*/ }
+};
+
+static int tps65912_spi_probe(struct spi_device *spi)
+{
+	struct tps65912 *tps;
+	const struct of_device_id *match;
+
+	match = of_match_device(tps65912_spi_of_match_table, &spi->dev);
+	if (!match) {
+		dev_err(&spi->dev, "Failed to find matching DT id\n");
+		return -EINVAL;
+	}

Why are you matching?

Same response as above.

quoted

quoted

+	tps = devm_kzalloc(&spi->dev, sizeof(*tps), GFP_KERNEL);
+	if (!tps)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, tps);
+	tps->dev = &spi->dev;
+	tps->irq = spi->irq;
+
+	tps->regmap = devm_regmap_init_spi(spi, &tps65912_regmap_config);
+	if (IS_ERR(tps->regmap)) {
+		int ret = PTR_ERR(tps->regmap);
+
+		dev_err(tps->dev, "Failed to allocate register map: %d\n",
+			ret);
+
+		return ret;
+	}
+
+	return tps65912_device_init(tps);
+}
+
+static int tps65912_spi_remove(struct spi_device *client)
+{
+	struct tps65912 *tps = spi_get_drvdata(client);
+
+	tps65912_device_exit(tps);
+
+	return 0;
+}
+
+static const struct spi_device_id tps65912_spi_id_table[] = {
+	{ "tps65912", TPS65912 },
+	{ /*sentinel*/ },
+};
+MODULE_DEVICE_TABLE(spi, tps65912_spi_id_table);
+
+static struct spi_driver tps65912_spi_driver = {
+	.driver		= {
+		.name	= "tps65912",
+		.owner	= THIS_MODULE,

Remove this line.

I wasn't sure about this one, all the SPI drivers I looked at still have
this, but if you are sure this is not needed then I'll remove it.

(also if it is not needed someone could probably remove this from all
these drivers like 816c44c36901 did in power)

Yes, they should all be removed.  It's taken care of elsewhere.  Some
effort has been put in and I believe a Coccinelle script even exists
now.

Fill your boots.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html