Re: [PATCH v2] mfd: tqmx86: IO controller with i2c, wachdog and gpio

From: Lee Jones
Date: Fri Dec 21 2018 - 09:35:12 EST

Next message: Nikolay Borisov: "[PATCH] mm: Refactor readahead defines in mm.h"
Previous message: Marek Szyprowski: "[PATCH v2 1/4] mfd: exynos-lpass: Enable UART module support"
In reply to: Andrew Lunn: "[PATCH v2] mfd: tqmx86: IO controller with i2c, wachdog and gpio"
Next in thread: Andrew Lunn: "Re: [PATCH v2] mfd: tqmx86: IO controller with i2c, wachdog and gpio"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 18 Dec 2018, Andrew Lunn wrote:

> The QMX86 is a PLD present on some TQ-Systems ComExpress modules. It
> provides 1 or 2 I2C bus masters, 8 GPIOs and a watchdog timer. Add an
> MFD which will instantiate the individual drivers.
>
> Signed-off-by: Andrew Lunn <andrew@xxxxxxx>
> ---
> v2:
>
> Drop setting i2c bus speed, which removes the build dependencies on
> the i2c ocores patches. This can be added back later.
> ---
> drivers/mfd/Kconfig | 8 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/tqmx86.c | 404 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 413 insertions(+)
> create mode 100644 drivers/mfd/tqmx86.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8c5dfdce4326..8c86a2a215e8 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1676,6 +1676,14 @@ config MFD_TC6393XB
> help
> Support for Toshiba Mobile IO Controller TC6393XB
>
> +config MFD_TQMX86
> + tristate "TQ-Systems IO controller TQMX86"
> + select MFD_CORE
> + help
> + Say yes here to enable support for various functions of the
> + TQ-Systems IO controller and watchdog device, found on their
> + ComExpress CPU modules

The help should be indented.

Nit: You're missing a full stop.

> config MFD_VX855
> tristate "VIA VX855/VX875 integrated south bridge"
> depends on PCI
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 12980a4ad460..7f4790662988 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_MFD_TC3589X) += tc3589x.o
> obj-$(CONFIG_MFD_T7L66XB) += t7l66xb.o tmio_core.o
> obj-$(CONFIG_MFD_TC6387XB) += tc6387xb.o tmio_core.o
> obj-$(CONFIG_MFD_TC6393XB) += tc6393xb.o tmio_core.o
> +obj-$(CONFIG_MFD_TQMX86) += tqmx86.o
>
> obj-$(CONFIG_MFD_ARIZONA) += arizona-core.o
> obj-$(CONFIG_MFD_ARIZONA) += arizona-irq.o
> diff --git a/drivers/mfd/tqmx86.c b/drivers/mfd/tqmx86.c
> new file mode 100644
> index 000000000000..4eca166db000
> --- /dev/null
> +++ b/drivers/mfd/tqmx86.c
> @@ -0,0 +1,404 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * TQ-Systems PLD MFD core driver
> + *
> + * Copyright (c) 2015 TQ-Systems GmbH

Copyright is out of date.

> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.

You shouldn't need this now that you have the SPDX above.

> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/dmi.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/platform_data/i2c-ocores.h>

Alphabetical.

> +#define TQMX86_IOBASE 0x160
> +#define TQMX86_IOSIZE 0x3f
> +#define TQMX86_CLK 33000 /* default */

Why don't you call this TQMX86_DEFAULT_CLK_RATE ?

Then drop the comment.

> +/* Registers offsets */

Register

> +#define TQMX86_BID 0x20 /* Board ID */
> +#define TQMX86_BREV 0x21 /* Board and PLD Revisions */
> +#define TQMX86_IOEIC 0x26 /* I/O Extension Interrupt Configuration */
> +#define TQMX86_I2C_DET 0x47 /* I2C controller detection register */
> +#define TQMX86_I2C_IEN 0x49 /* machxo2 I2C nterrupt enable register */

All them, TQMX86_REG_*, then drop the header comment.

If your #defines were named well, they should not need comments.

> +struct tqmx86_info {
> + u32 board_id;
> + u32 board_rev;
> + u32 pld_rev;
> + u32 i2c_type;
> +};

Why not just add these to ddata?

> +#define I2C_KIND_SOFT 1 /* Ocores soft controller */
> +#define I2C_KIND_HARD 2 /* Machxo2 hard controller */

What is a soft/hard controller?

These should be grouped with your other defines.

> +/**
> + * struct tqmx86_device_data - Internal representation of the PLD device
> + * @io_base: Pointer to the IO memory
> + * @pld_clock: PLD clock frequency

pid_clk_rate

> + * @dev: Pointer to kernel device structure
> + */
> +struct tqmx86_device_data {

s/data/ddata/

> + void __iomem *io_base;
> + u32 pld_clock;
> + struct device *dev;

You don't need this.

Just pass pdev as the first argument to tqmx86_detect_device().

> + struct tqmx86_info info;
> +};
> +
> +/**
> + * struct tqmx86_platform_data - PLD hardware configuration structure
> + * @pld_clock: PLD clock frequency
> + * @ioresource: IO addresses of the PLD
> + */
> +struct tqmx86_platform_data {
> + u32 pld_clock;
> + struct resource *ioresource;

Too many tabs.

> +};
> +
> +static uint gpio_irq;
> +module_param(gpio_irq, uint, 0);
> +MODULE_PARM_DESC(gpio_irq, "GPIO IRQ number (7, 9, 12)");

I have never seen anything like this.

This should be set by platform data, not a module parameter.

> +static u8 i2c_irq_ctl[16] = {
> + [7] = 1,
> + [9] = 2,
> + [12] = 3
> +};
> +
> +static u8 tqmx86_readb(struct tqmx86_device_data *pld, u32 off)
> +{
> + return ioread8(pld->io_base + off);
> +}
> +
> +static void tqmx86_writeb(struct tqmx86_device_data *pld, u8 val, u32 off)
> +{
> + iowrite8(val, pld->io_base + off);
> +}

Don't write needless abstraction layers.

Use the calls themselves.

Any reason for not using Regmap?

> +enum tqmx86_cells {
> + TQMX86_I2C_SOFT = 0,
> + TQMX86_WDT,
> + TQMX86_GPIO,
> + TQMX86_UART,
> +};

Why do you need to number the cells?

> +static struct resource tqmx_i2c_soft_resources[] = {
> + DEFINE_RES_IO(0x1a0, 10),

No magic numbers please. You need to define these.

> +};
> +
> +static struct resource tqmx_watchdog_resources[] = {
> + DEFINE_RES_IO(0x18b, 2),
> +};
> +
> +static struct resource tqmx_gpio_resources[] = {
> + DEFINE_RES_IO(0x18d, 4),
> + DEFINE_RES_IRQ(0)
> +};
> +
> +static struct i2c_board_info tqmx86_i2c_devices[] = {
> + /* 4K EEPROM at 0x50 */
> + {
> + .type = "24c32",
> + .addr = 0x50,
> + },
> +};
> +
> +static struct ocores_i2c_platform_data ocores_platfom_data = {
> + .clock_khz = TQMX86_CLK,
> + .num_devices = ARRAY_SIZE(tqmx86_i2c_devices),
> + .devices = tqmx86_i2c_devices,
> +};
> +
> +static const struct mfd_cell tqmx86_devs[] = {
> + [TQMX86_I2C_SOFT] = {
> + .name = "ocores-i2c",
> + .platform_data = &ocores_platfom_data,
> + .pdata_size = sizeof(ocores_platfom_data),
> + .resources = tqmx_i2c_soft_resources,
> + .num_resources = ARRAY_SIZE(tqmx_i2c_soft_resources),
> + },
> + [TQMX86_WDT] = {
> + .name = "tqmx86-wdt",
> + .resources = tqmx_watchdog_resources,
> + .num_resources = 1,
> + .ignore_resource_conflicts = 1,
> + },
> + [TQMX86_GPIO] = {
> + .name = "tqmx86-gpio",
> + .resources = tqmx_gpio_resources,
> + .num_resources = ARRAY_SIZE(tqmx_gpio_resources),
> + .ignore_resource_conflicts = 1,
> + },
> +};
> +
> +#define TQMX86_MAX_DEVS ARRAY_SIZE(tqmx86_devs)
> +
> +static int tqmx86_register_cells(struct tqmx86_device_data *pld)
> +{
> + struct mfd_cell devs[TQMX86_MAX_DEVS];

Why is it being done like this?

Registering MFD cells is a well trodden path.

No need to invent new ways to do it

> + int i = 0;
> + u8 ioeic_val = 0;
> +
> + ioeic_val |= (i2c_irq_ctl[gpio_irq] & 0x3) << 4;

What is IOEIC?

The magic numbers should be defined (*_SHIFT/*_MASK)

Side note: If I have to ask this many questions, it normally means the
code is not transparent enough. There could be many reasons for this;
variable/function nomenclature, code trying to be too clever or do too
many things at once, coding style, data structure hacks, etc etc.

> + dev_dbg(pld->dev, "ioeic %x\n", ioeic_val);

Are these really (still - after initial development) helpful to you?

Will they really be helpful to others?

> + if (ioeic_val) {
> + tqmx86_writeb(pld, ioeic_val, TQMX86_IOEIC);
> + if (tqmx86_readb(pld, TQMX86_IOEIC) != ioeic_val) {
> + dev_warn(pld->dev,
> + "i2c/gpio interrupts not supported.\n");
> + gpio_irq = 0;
> + }
> + }
> +
> + if (pld->info.i2c_type == I2C_KIND_SOFT) {
> + ocores_platfom_data.clock_khz = pld->pld_clock;
> + devs[i++] = tqmx86_devs[TQMX86_I2C_SOFT];
> + }

See other drivers to see how they handle optional cells.

> + tqmx_gpio_resources[1].start = gpio_irq;

What about end? This is a hack anyway.

> + devs[i++] = tqmx86_devs[TQMX86_WDT];
> + devs[i++] = tqmx86_devs[TQMX86_GPIO];
> +
> + return mfd_add_devices(pld->dev, -1, devs, i, NULL, 0, NULL);

Should not be -1. Check other drivers.

Can you use devm_*?

> +}
> +
> +static struct resource tqmx86_ioresource = {
> + .start = TQMX86_IOBASE,
> + .end = TQMX86_IOBASE + TQMX86_IOSIZE,
> + .flags = IORESOURCE_IO,
> +};

DEFINE_RES_*?

> +static const struct tqmx86_platform_data tqmx86_platform_data_generic = {
> + .pld_clock = TQMX86_CLK,
> + .ioresource = &tqmx86_ioresource,
> +};

Who will receive this platform data?

> +static struct platform_device *tqmx86_pdev;

Global?

> +static int tqmx86_create_platform_device(const struct dmi_system_id *id)

This blows my mind.

- The normal module_init() calls are initiated calling for a DMI scan
- Then the DMI device init()s
- You use the DMI init() to register this device as a platform device
- Then this platform device then probes

That seems very incestuous.

What is the reason for all the hoop jumping?

> +{
> + struct tqmx86_platform_data *pdata = id->driver_data;
> + int ret;
> +
> + tqmx86_pdev = platform_device_alloc("tqmx86", -1);
> + if (!tqmx86_pdev)
> + return -ENOMEM;
> +
> + ret = platform_device_add_data(tqmx86_pdev, pdata, sizeof(*pdata));
> + if (ret)
> + goto err;
> +
> + ret = platform_device_add_resources(tqmx86_pdev, pdata->ioresource, 1);
> + if (ret)
> + goto err;
> +
> + ret = platform_device_add(tqmx86_pdev);
> + if (ret)
> + goto err;
> +
> + return 0;
> +err:
> + platform_device_put(tqmx86_pdev);
> + return ret;
> +}
> +
> +static struct tq_board_info {
> + char *name;
> + u32 pld_clock;
> +} tq_board_info[] = {
> + {"", 0},
> + {"TQMxE38M", 33000},
> + {"TQMx50UC", 24000},
> + {"TQMxE38C", 33000},
> + {"TQMx60EB", 24000},
> + {"TQMxE39M", 25000},
> + {"TQMxE39C", 25000},
> + {"TQMxE39x", 25000},
> + {"TQMx70EB", 24000},
> + {"TQMx80UC", 24000},
> + {"TQMx90UC", 24000}
> +};

Better to write a look-up function I think.

What happens if the next released board ID is 0xFC?

> +static ssize_t board_id_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct tqmx86_device_data *pld = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "%s\n",
> + tq_board_info[pld->info.board_id].name);
> +}
> +
> +static ssize_t board_rev_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct tqmx86_device_data *pld = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "%d\n", pld->info.board_rev);
> +}
> +
> +static ssize_t pld_rev_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct tqmx86_device_data *pld = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "PLD Revision: %d",
> + pld->info.pld_rev);
> +}
> +
> +static DEVICE_ATTR_RO(board_id);
> +static DEVICE_ATTR_RO(board_rev);
> +static DEVICE_ATTR_RO(pld_rev);
> +
> +static struct attribute *pld_attributes[] = {
> + &dev_attr_board_id.attr,
> + &dev_attr_board_rev.attr,
> + &dev_attr_pld_rev.attr,
> + NULL
> +};
> +
> +static const struct attribute_group pld_attr_group = {
> + .attrs = pld_attributes,
> +};

What are you using sysfs for that requires this information?

> +static int tqmx86_detect_device(struct tqmx86_device_data *pld)
> +{
> + u8 board_id, rev, i2c_det, i2c_ien;
> + int ret;
> +
> +
> + board_id = tqmx86_readb(pld, TQMX86_BID);
> + if (board_id == 0 || board_id > ARRAY_SIZE(tq_board_info) - 1)
> + return -ENODEV;

This seems fragile. You should define the maximum board ID.

Also, you exit silently -- is that really what you want?

> + pld->pld_clock = tq_board_info[board_id].pld_clock;
> +
> + rev = tqmx86_readb(pld, TQMX86_BREV);

'\n' here.

> + pld->info.board_id = board_id;
> + pld->info.board_rev = rev >> 4;
> + pld->info.pld_rev = rev & 0xf;
> +
> + i2c_det = tqmx86_readb(pld, TQMX86_I2C_DET);
> + i2c_ien = tqmx86_readb(pld, TQMX86_I2C_IEN);

What are these values?

> + if (i2c_det == 0xa5 && (i2c_ien & 0xf0) == 0xf0)

More unreadable magic numbers.

> + pld->info.i2c_type = I2C_KIND_SOFT;
> + else
> + pld->info.i2c_type = I2C_KIND_HARD;

What are these?

> + dev_info(pld->dev,
> + "Found TQx86 PLD - Board ID %d, PCB Revision %d, PLD Revision %d\n",
> + board_id, rev >> 4, rev & 0xf);
> +
> + ret = sysfs_create_group(&pld->dev->kobj, &pld_attr_group);
> + if (ret)
> + return ret;
> +
> + ret = tqmx86_register_cells(pld);
> + if (ret)
> + sysfs_remove_group(&pld->dev->kobj, &pld_attr_group);
> +
> + return ret;
> +}
> +
> +static int tqmx86_probe(struct platform_device *pdev)
> +{
> + struct tqmx86_platform_data *pdata = dev_get_platdata(&pdev->dev);

Where was this previously set?

> + struct device *dev = &pdev->dev;
> + struct tqmx86_device_data *pld;
> + struct resource *ioport;
> +
> + pld = devm_kzalloc(dev, sizeof(*pld), GFP_KERNEL);
> + if (!pld)
> + return -ENOMEM;
> +
> + ioport = platform_get_resource(pdev, IORESOURCE_IO, 0);
> + if (!ioport)
> + return -EINVAL;
> +
> + pld->io_base = devm_ioport_map(dev, ioport->start,
> + resource_size(ioport));

This is used very little in the kernel.

What is it you're trying to do here?

Is Regmap a better alternative? Take a look at some other MFD drivers.

> + if (!pld->io_base)
> + return -ENOMEM;
> +
> + pld->pld_clock = pdata->pld_clock;
> + pld->dev = dev;
> +
> + platform_set_drvdata(pdev, pld);
> +
> + return tqmx86_detect_device(pld);
> +}
> +
> +static int tqmx86_remove(struct platform_device *pdev)
> +{
> + struct tqmx86_device_data *pld = dev_get_drvdata(&pdev->dev);
> +
> + sysfs_remove_group(&pld->dev->kobj, &pld_attr_group);
> + mfd_remove_devices(&pdev->dev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver tqmx86_driver = {
> + .driver = {
> + .name = "tqmx86",
> + },
> + .probe = tqmx86_probe,
> + .remove = tqmx86_remove,
> +};
> +
> +static struct dmi_system_id tqmx86_dmi_table[] __initdata = {
> + {
> + .ident = "TQMX86",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "TQ-Group"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "TQMx"),
> + },
> + .driver_data = (void *)&tqmx86_platform_data_generic,
> + .callback = tqmx86_create_platform_device,
> + },
> + {}
> +};
> +MODULE_DEVICE_TABLE(dmi, tqmx86_dmi_table);
> +
> +static int __init tqmx86_init(void)
> +{
> + if (gpio_irq > 15) {
> + pr_warn("tqmx86: Invalid GPIO IRQ (%d)\n", gpio_irq);
> + gpio_irq = 0;
> + } else if (i2c_irq_ctl[gpio_irq] == 0) {
> + pr_warn("tqmx86: GPIO IRQ %d not supported\n", gpio_irq);
> + gpio_irq = 0;
> + }
> +
> + if (!dmi_check_system(tqmx86_dmi_table))
> + return -ENODEV;
> +
> + return platform_driver_register(&tqmx86_driver);
> +}
> +
> +static void __exit tqmx86_exit(void)
> +{
> + if (tqmx86_pdev)
> + platform_device_unregister(tqmx86_pdev);
> +
> + platform_driver_unregister(&tqmx86_driver);
> +}
> +
> +module_init(tqmx86_init);
> +module_exit(tqmx86_exit);
> +
> +MODULE_DESCRIPTION("TQx86 PLD Core Driver");
> +MODULE_AUTHOR("Vadim V.Vlasov <vvlasov@xxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");

This does not match the file header.

Should be "GPL v2"

> +MODULE_ALIAS("platform:tqmx86");

--
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Next message: Nikolay Borisov: "[PATCH] mm: Refactor readahead defines in mm.h"
Previous message: Marek Szyprowski: "[PATCH v2 1/4] mfd: exynos-lpass: Enable UART module support"
In reply to: Andrew Lunn: "[PATCH v2] mfd: tqmx86: IO controller with i2c, wachdog and gpio"
Next in thread: Andrew Lunn: "Re: [PATCH v2] mfd: tqmx86: IO controller with i2c, wachdog and gpio"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]