Messages in this thread | | | Date | Thu, 7 Feb 2019 10:23:56 +0000 | From | Lee Jones <> | 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@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 <andrew@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
| |