Re: [PATCH v4 4/4] mfd: tqmx86: add detection for MachXO2 I2C controller

[Lee Jones <lee@kernel.org>]

On Wed, 10 Dec 2025, Matthias Schiffer wrote:

> The TQMx86 PLD may contain two kinds of I2C controllers as its secondary
> instance: the previously supported OpenCores I2C, or the MachXO2 I2C. Add
> support for the latter.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
> 
> v2: no changes
> 
> v3: no changes
> 
> v4:
> - Reorder patch (was 3/4) - the new order makes more sense with the changed (less generic) approach
> - Platform data and mfd_cell are toplevel variables now
> - With the new less generic approach i2c-machxo2 detection is only done for the secondary I2C controller
>   (as the primary one is always i2c-ocores anyways)
> 
> 
>  drivers/mfd/tqmx86.c | 44 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mfd/tqmx86.c b/drivers/mfd/tqmx86.c
> index 708b33c3e5724..a4e3ecfccb8ab 100644
> --- a/drivers/mfd/tqmx86.c
> +++ b/drivers/mfd/tqmx86.c
> @@ -13,6 +13,7 @@
>  #include <linux/io.h>
>  #include <linux/mfd/core.h>
>  #include <linux/module.h>
> +#include <linux/platform_data/i2c-machxo2.h>
>  #include <linux/platform_data/i2c-ocores.h>
>  #include <linux/platform_device.h>
>  
> @@ -61,6 +62,8 @@
>  #define TQMX86_REG_I2C_DETECT	0x7
>  #define TQMX86_REG_I2C_DETECT_OCORES	0xa5
>  
> +#define TQMX86_REG_I2C_IEN	0x9
> +
>  static uint gpio_irq;
>  module_param(gpio_irq, uint, 0);
>  MODULE_PARM_DESC(gpio_irq, "GPIO IRQ number (valid parameters: 7, 9, 12)");
> @@ -130,6 +133,8 @@ static const struct mfd_cell tqmx86_i2c1_dev_ocores = {
>  	.num_resources = ARRAY_SIZE(tqmx_i2c1_resources),
>  };
>  
> +/* Only one of the tqmx86_i2c2_* instances is registered, depending on which is detected */
> +
>  static struct ocores_i2c_platform_data tqmx86_i2c2_platform_data_ocores = {};
>  
>  static const struct mfd_cell tqmx86_i2c2_dev_ocores = {
> @@ -140,6 +145,18 @@ static const struct mfd_cell tqmx86_i2c2_dev_ocores = {
>  	.num_resources = ARRAY_SIZE(tqmx_i2c2_resources),
>  };
>  
> +struct machxo2_i2c_platform_data tqmx86_i2c2_platform_data_machxo2 = {
> +	.bus_khz = 100,
> +};
> +
> +static const struct mfd_cell tqmx86_i2c2_dev_machxo2 = {
> +	.name = "i2c-machxo2",
> +	.platform_data = &tqmx86_i2c2_platform_data_machxo2,
> +	.pdata_size = sizeof(tqmx86_i2c2_platform_data_machxo2),
> +	.resources = tqmx_i2c2_resources,
> +	.num_resources = ARRAY_SIZE(tqmx_i2c2_resources),
> +};
> +
>  static const struct mfd_cell tqmx86_devs[] = {
>  	{
>  		.name = "tqmx86-wdt",
> @@ -282,9 +299,9 @@ static int tqmx86_detect_i2c1(struct device *dev, void __iomem *io_base, int clo
>  	}
>  
>  	/*
> -	 * The I2C_DETECT register is in the range assigned to the I2C driver
> -	 * later, so we don't extend TQMX86_IOSIZE. Use inb() for this one-off
> -	 * access instead of ioport_map + unmap.
> +	 * These registers are in the range assigned to the I2C driver
> +	 * later, so we don't extend TQMX86_IOSIZE. Use inb() for these one-off
> +	 * accesses instead of ioport_map + unmap.
>  	 */
>  	i2c_det = inb(TQMX86_IOBASE_I2C1 + TQMX86_REG_I2C_DETECT);
>  
> @@ -300,7 +317,7 @@ static int tqmx86_detect_i2c1(struct device *dev, void __iomem *io_base, int clo
>  
>  static int tqmx86_detect_i2c2(struct device *dev, void __iomem *io_base, int clock_khz)
>  {
> -	u8 i2c_det;
> +	u8 i2c_det, i2c_ien;
>  
>  	if (i2c2_irq) {
>  		if (!tqmx86_setup_irq(dev, "I2C2", i2c2_irq, io_base,
> @@ -312,16 +329,33 @@ static int tqmx86_detect_i2c2(struct device *dev, void __iomem *io_base, int clo
>  	 * The I2C_DETECT register is in the range assigned to the I2C driver
>  	 * later, so we don't extend TQMX86_IOSIZE. Use inb() for this one-off
>  	 * access instead of ioport_map + unmap.
> +	 *
> +	 * There are 3 cases to distinguish for the secondary controller:
> +	 *
> +	 * - ocores: i2c_det is a TQMx86-specific register that always contains
> +	 *   the value 0xa5. i2c_ien is unused and reads as 0xff.
> +	 * - machxo2: i2c_det is the data register can read as any value.
> +	 *   i2c_ien is the interrupt enable register; the upper nibble is
> +	 *   reserved and always reads as 0.
> +	 * - none: both i2c_det and i2c_ien read as 0xff if no I2C controller
> +	 *   exists at a given base address.
>  	 */
>  	i2c_det = inb(TQMX86_IOBASE_I2C2 + TQMX86_REG_I2C_DETECT);
> +	i2c_ien = inb(TQMX86_IOBASE_I2C2 + TQMX86_REG_I2C_IEN);
>  
> -	if (i2c_det == TQMX86_REG_I2C_DETECT_OCORES) {
> +	if (i2c_det == TQMX86_REG_I2C_DETECT_OCORES && i2c_ien == 0xff) {

What is 0xff?  No magic numbers - please define.

>  		tqmx86_i2c2_platform_data_ocores.clock_khz = clock_khz;
>  		return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
>  					    &tqmx86_i2c2_dev_ocores, 1, NULL, 0,
>  					    NULL);
> +	} else if ((i2c_ien & 0xf0) == 0x00) {

As above - 0x00 means nothing to me.

> +		tqmx86_i2c2_platform_data_machxo2.clock_khz = clock_khz;
> +		return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> +					    &tqmx86_i2c2_dev_machxo2, 1, NULL, 0,
> +					    NULL);
>  	}
>  
> +
>  	return 0;
>  }
>  
> -- 
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> https://www.tq-group.com/
> 

-- 
Lee Jones [李琼斯]