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 <and...@lunn.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 <and...@lunn.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
