Message-ID: <20181221143505.GQ13248@dell>
Date:   Fri, 21 Dec 2018 14:35:05 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mfd: tqmx86: IO controller with i2c, wachdog and gpio

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@...n.ch>
> ---
> 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@....rtsoft.ru>");
> +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

Powered by blists - more mailing lists