| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150921230445.GA11284@x1>
Date: Tue, 22 Sep 2015 00:04:45 +0100
From: Lee Jones <lee.jones@...aro.org>
To: "Andrew F. Davis" <afd@...com>
Cc: Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Linus Walleij <linus.walleij@...aro.org>,
Alexandre Courbot <gnurou@...il.com>,
Samuel Ortiz <sameo@...ux.intel.com>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, devicetree@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and
using regmap
On Mon, 21 Sep 2015, Andrew F. Davis wrote:
> On 09/19/2015 11:16 PM, Lee Jones wrote:
> >On Tue, 15 Sep 2015, Andrew F. Davis wrote:
> >
> >>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 <afd@...com>
> >>---
> >> 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.
[...]
> >>+#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.
> >>+#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
[...]
> >>+ .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.
[...]
> >>+#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).
> >>+};
> >>+
> >>+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.
> >>+ 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.
[...]
> >>+ { "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.
> >>+ { /*sentinel*/ },
> >
> >As before.
> >
> >>+};
> >>+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 <afd@...com>");
> >>+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@...mlogic.co.uk>
> >>- *
> >>- * 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, ®);
> >>- irq_sts = reg;
> >>- tps65912->read(tps65912, TPS65912_INT_STS2, 1, ®);
> >>- irq_sts |= reg << 8;
> >>- tps65912->read(tps65912, TPS65912_INT_STS3, 1, ®);
> >>- irq_sts |= reg << 16;
> >>- tps65912->read(tps65912, TPS65912_INT_STS4, 1, ®);
> >>- irq_sts |= reg << 24;
> >>-
> >>- tps65912->read(tps65912, TPS65912_INT_MSK, 1, ®);
> >>- irq_mask = reg;
> >>- tps65912->read(tps65912, TPS65912_INT_MSK2, 1, ®);
> >>- irq_mask |= reg << 8;
> >>- tps65912->read(tps65912, TPS65912_INT_MSK3, 1, ®);
> >>- irq_mask |= reg << 16;
> >>- tps65912->read(tps65912, TPS65912_INT_MSK4, 1, ®);
> >>- 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 = irq_sts & 0xFF;
> >>- irq_sts >>= 8;
> >>- if (reg)
> >>- tps65912->write(tps65912, TPS65912_INT_STS2, 1, ®);
> >>- reg = irq_sts & 0xFF;
> >>- irq_sts >>= 8;
> >>- if (reg)
> >>- tps65912->write(tps65912, TPS65912_INT_STS3, 1, ®);
> >>- reg = irq_sts & 0xFF;
> >>- if (reg)
> >>- tps65912->write(tps65912, TPS65912_INT_STS4, 1, ®);
> >>-
> >>- 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_mask = reg;
> >>- tps65912->read(tps65912, TPS65912_INT_MSK2, 1, ®);
> >>- reg_mask |= reg << 8;
> >>- tps65912->read(tps65912, TPS65912_INT_MSK3, 1, ®);
> >>- reg_mask |= reg << 16;
> >>- tps65912->read(tps65912, TPS65912_INT_MSK4, 1, ®);
> >>- reg_mask |= reg << 24;
> >>-
> >>- if (tps65912->irq_mask != reg_mask) {
> >>- reg = tps65912->irq_mask & 0xFF;
> >>- tps65912->write(tps65912, TPS65912_INT_MSK, 1, ®);
> >>- reg = tps65912->irq_mask >> 8 & 0xFF;
> >>- tps65912->write(tps65912, TPS65912_INT_MSK2, 1, ®);
> >>- reg = tps65912->irq_mask >> 16 & 0xFF;
> >>- tps65912->write(tps65912, TPS65912_INT_MSK3, 1, ®);
> >>- reg = tps65912->irq_mask >> 24 & 0xFF;
> >>- tps65912->write(tps65912, TPS65912_INT_MSK4, 1, ®);
> >>- }
> >>-
> >>- 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, ®);
> >>- tps65912->write(tps65912, TPS65912_INT_STS, 1, ®);
> >>- tps65912->read(tps65912, TPS65912_INT_STS2, 1, ®);
> >>- tps65912->write(tps65912, TPS65912_INT_STS2, 1, ®);
> >>- tps65912->read(tps65912, TPS65912_INT_STS3, 1, ®);
> >>- tps65912->write(tps65912, TPS65912_INT_STS3, 1, ®);
> >>- tps65912->read(tps65912, TPS65912_INT_STS4, 1, ®);
> >>- tps65912->write(tps65912, TPS65912_INT_STS4, 1, ®);
> >>-
> >>- /* 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@...mlogic.co.uk>
> >>- *
> >>- * 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@...mlogic.co.uk>");
> >>-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
>
> >>+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
> >>+ *
> >>+ * Author: Andrew F. Davis <afd@...com>
> >>+ *
> >>+ * 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 <magi@...mlogic.co.uk>
> >>+ */
> >>+
> >>+#include <linux/spi/spi.h>
> >>+#include <linux/module.h>
> >>+#include <linux/of_device.h>
> >>+#include <linux/regmap.h>
> >
> >Alphabetical.
> >
>
> ACK
>
> >>+#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.
>
> >>+ 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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists