Message-ID: <20190207102356.GB20638@dell>
Date:   Thu, 7 Feb 2019 10:23:56 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     linux-kernel@...r.kernel.org,
        Emeric Dupont <emeric.dupont@....aero>
Subject: Re: [PATCH v5] mfd: tqmx86: IO controller with i2c, wachdog and gpio

On Thu, 07 Feb 2019, 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.
> 
> v3
> 
> Fix indentation of help
> Fix SPDX to match MODULE_LICENSE
> Remove warranty text
> Add my Copyright
> Sort include files
> Add _REG_ to register defines
> Add #defines for all resources
> Replace magic numbers with #defines
> Rename pld_clock to pld_clock_rate
> Remove wrapper around ioread8()/iowrite8()
> Scatter const keyword in a few places
> Remove enum for calls
> Implement lookup for board in tq_board_if
> Rename ioeic to io_ext_int_val to make is more readable
> Handle optional calls in a different way
> Better group code and structures
> Kill all the sysfs attributes
> Use devm_mfd_add_devices()
> Don't exist silently for unknown board ID
> 
> Not addressed, waiting for answers:
> MODULE_PARM for GPIO interrupts
> Not using regmap, intel IO not supported by regmap
> Setting GPIO irq on resource structure
> Global tqmx86_pdev
> Jumping through hoops
> 
> v4
> 
> Remove -- to avoid broken mailer
> 
> v5
> 
> Remove device data since it is only used in the probe function
> Remove platform data, since it only contains well know values
> Move GPIO IRQ resource first and comment about this assumption
> Fixup platform data naming
> Use true for ignore_resource_conflicts
> Use ARRAY_SIZE for num_resources
> Replace tq_board_info with two functions using switch
> Error out for invalid GPIO IRQ configuration
> Error out if GPIO IRQ cannot be configured in the hardware
> Split registering cells into two calls
> Validate GPIO IRQ using a switch statement, and derive the HW configuration
> ---
>  drivers/mfd/Kconfig  |   8 ++
>  drivers/mfd/Makefile |   1 +
>  drivers/mfd/tqmx86.c | 289 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 298 insertions(+)
>  create mode 100644 drivers/mfd/tqmx86.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index f461460a2aeb..226f7eeb2093 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1677,6 +1677,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.
> +
>  config MFD_VX855
>  	tristate "VIA VX855/VX875 integrated south bridge"
>  	depends on PCI
[...]

> +static int tqmx86_board_id_to_clk_rate(u8 board_id)
> +{
> +	switch (board_id) {
> +	case TQMX86_REG_BOARD_ID_E38M:
> +		return 33000;
> +	case TQMX86_REG_BOARD_ID_50UC:
> +		return 24000;
> +	case TQMX86_REG_BOARD_ID_E38C:
> +		return 33000;
> +	case TQMX86_REG_BOARD_ID_60EB:
> +		return 24000;
> +	case TQMX86_REG_BOARD_ID_E39M:
> +		return 25000;
> +	case TQMX86_REG_BOARD_ID_E39C:
> +		return 25000;
> +	case TQMX86_REG_BOARD_ID_E39x:
> +		return 25000;
> +	case TQMX86_REG_BOARD_ID_70EB:
> +		return 24000;
> +	case TQMX86_REG_BOARD_ID_80UC:
> +		return 24000;
> +	case TQMX86_REG_BOARD_ID_90UC:
> +		return 24000;

Nit: What do you think of:

	switch (board_id) {
	case TQMX86_REG_BOARD_ID_E38M:
	case TQMX86_REG_BOARD_ID_E38C:
		return 33000;
	case TQMX86_REG_BOARD_ID_50UC:
	case TQMX86_REG_BOARD_ID_60EB:
	case TQMX86_REG_BOARD_ID_70EB:
	case TQMX86_REG_BOARD_ID_80UC:
	case TQMX86_REG_BOARD_ID_90UC:
		return 24000;
	case TQMX86_REG_BOARD_ID_E39M:
	case TQMX86_REG_BOARD_ID_E39C:
	case TQMX86_REG_BOARD_ID_E39x:
		return 25000;

> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int tqmx86_probe(struct platform_device *pdev)
> +{
> +	u8 board_id, rev, i2c_det, i2c_ien, io_ext_int_val;
> +	struct device *dev = &pdev->dev;
> +	const char *board_name;
> +	void __iomem *io_base;
> +	int err;
> +
> +	io_base = devm_ioport_map(dev, TQMX86_IOBASE, TQMX86_IOSIZE);
> +	if (!io_base)
> +		return -ENOMEM;
> +
> +	board_id = ioread8(io_base + TQMX86_REG_BOARD_ID);
> +	board_name = tqmx86_board_id_to_name(board_id);
> +	rev = ioread8(io_base + TQMX86_REG_BOARD_REV);
> +
> +	dev_info(dev,
> +		 "Found %s - Board ID %d, PCB Revision %d, PLD Revision %d\n",
> +		 board_name, board_id, rev >> 4, rev & 0xf);
> +
> +	i2c_det = ioread8(io_base + TQMX86_REG_I2C_DETECT);
> +	i2c_ien = ioread8(io_base + TQMX86_REG_I2C_INT_EN);
> +
> +	if (gpio_irq_cfg) {
> +		io_ext_int_val = gpio_irq_cfg <<
> +			TQMX86_REG_IO_EXT_INT_GPIO_SHIFT;

		io_ext_int_val =
			gpio_irq_cfg << TQMX86_REG_IO_EXT_INT_GPIO_SHIFT;

Reads better, don't you think?

> +		iowrite8(io_ext_int_val, io_base + TQMX86_REG_IO_EXT_INT);
> +		if (ioread8(io_base + TQMX86_REG_IO_EXT_INT) !=
> +		    io_ext_int_val) {

		iowrite8(io_ext_int_val, io_base + TQMX86_REG_IO_EXT_INT);
		readback = ioread8(io_base + TQMX86_REG_IO_EXT_INT);
		if (readback != io_ext_int_val) {

Never been a fan of function calls inside if statements.

This method also prevents the line break, which makes it slightly
harder to read.

> +			dev_warn(dev, "GPIO interrupts not supported.\n");

Do you know why they wouldn't be supported?

Isn't that the point of the device?

> +			return -EINVAL;
> +		}
> +
> +		/* Assumes the IRQ resource is first. */
> +		tqmx_gpio_resources[0].start = gpio_irq;
> +	}
> +
> +	ocores_platfom_data.clock_khz = tqmx86_board_id_to_clk_rate(board_id);
> +
> +	if (i2c_det == TQMX86_REG_I2C_DETECT_SOFT) {
> +		err = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> +					   tqmx86_i2c_soft_dev,
> +					   ARRAY_SIZE(tqmx86_i2c_soft_dev),
> +					   NULL, 0, NULL);
> +		if (err)
> +			return err;
> +	}
> +
> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> +				    tqmx86_devs,
> +				    ARRAY_SIZE(tqmx86_devs),
> +				    NULL, 0, NULL);

That's better, thanks.

> +}
> +
> +static int tqmx86_create_platform_device(const struct dmi_system_id *id)
> +{
> +	struct platform_device *pdev;
> +	int err;
> +
> +	pdev = platform_device_alloc("tqmx86", -1);
> +	if (!pdev)
> +		return -ENOMEM;
> +
> +	err = platform_device_add(pdev);
> +	if (err)
> +		platform_device_put(pdev);
> +
> +	return err;
> +}
> +
> +static const struct dmi_system_id tqmx86_dmi_table[] __initconst = {
> +	{
> +		.ident = "TQMX86",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "TQ-Group"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "TQMx"),
> +		},
> +		.callback = tqmx86_create_platform_device,
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(dmi, tqmx86_dmi_table);
> +
> +static struct platform_driver tqmx86_driver = {
> +	.driver		= {
> +		.name	= "tqmx86",
> +	},
> +	.probe		= tqmx86_probe,
> +};
> +
> +static int __init tqmx86_init(void)
> +{
> +	switch (gpio_irq) {
> +	case 0:
> +		gpio_irq_cfg = TQMX86_REG_IO_EXT_INT_NONE;
> +		break;
> +	case 7:
> +		gpio_irq_cfg = TQMX86_REG_IO_EXT_INT_7;
> +		break;
> +	case 9:
> +		gpio_irq_cfg = TQMX86_REG_IO_EXT_INT_9;
> +		break;
> +	case 12:
> +		gpio_irq_cfg = TQMX86_REG_IO_EXT_INT_12;
> +		break;
> +	default:
> +		pr_err("tqmx86: Invalid GPIO IRQ (%d)\n", gpio_irq);
> +		return -EINVAL;
> +	}

Is there anything stopping you doing this checking in .probe()?

Then you can drop the gpio_irq_cfg global variable.

> +	if (!dmi_check_system(tqmx86_dmi_table))
> +		return -ENODEV;
> +
> +	return platform_driver_register(&tqmx86_driver);
> +}
> +
> +module_init(tqmx86_init);
> +
> +MODULE_DESCRIPTION("TQx86 PLD Core Driver");
> +MODULE_AUTHOR("Andrew Lunn <andrew@...n.ch>");
> +MODULE_LICENSE("GPL");
> +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