Re: [PATCH v3 2/4] mfd: tqmx86: refactor I2C setup

[Lee Jones <lee@kernel.org>]

On Thu, 06 Nov 2025, Matthias Schiffer wrote:

> On Thu, 2025-11-06 at 13:38 +0000, Lee Jones wrote:
> > On Wed, 22 Oct 2025, Matthias Schiffer wrote:
> > 
> > > Preparation for supporting the second I2C controller, and detecting both
> > > ocores and machxo2 controllers.
> > > 
> > > - Avoid the confusing "soft" I2C controller term - just call it the
> > >   ocores I2C
> > > - All non-const parts of the MFD cell are moved from global variables
> > >   into new functions tqmx86_setup_i2c_ocores() and tqmx86_setup_i2c()
> > > - Define TQMX86_REG_I2C_DETECT relative to I2C base register
> > > 
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > ---
> > > 
> > > v2: no changes
> > > v3: no changes
> > > 
> > >  drivers/mfd/tqmx86.c | 130 ++++++++++++++++++++++++-------------------
> > >  1 file changed, 74 insertions(+), 56 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/tqmx86.c b/drivers/mfd/tqmx86.c
> > > index 1cba3b67b0fb9..3c6f158bf1a45 100644
> > > --- a/drivers/mfd/tqmx86.c
> > > +++ b/drivers/mfd/tqmx86.c
> > > @@ -18,7 +18,7 @@
> > >  
> > >  #define TQMX86_IOBASE	0x180
> > >  #define TQMX86_IOSIZE	0x20
> > > -#define TQMX86_IOBASE_I2C	0x1a0
> > > +#define TQMX86_IOBASE_I2C1	0x1a0
> > >  #define TQMX86_IOSIZE_I2C	0xa
> > >  #define TQMX86_IOBASE_WATCHDOG	0x18b
> > >  #define TQMX86_IOSIZE_WATCHDOG	0x2
> > > @@ -54,8 +54,8 @@
> > >  #define TQMX86_REG_IO_EXT_INT_GPIO_SHIFT	4
> > >  #define TQMX86_REG_SAUC		0x17
> > >  
> > > -#define TQMX86_REG_I2C_DETECT	0x1a7
> > > -#define TQMX86_REG_I2C_DETECT_SOFT		0xa5
> > > +#define TQMX86_REG_I2C_DETECT	0x7
> > > +#define TQMX86_REG_I2C_DETECT_OCORES	0xa5
> > >  
> > >  static uint gpio_irq;
> > >  module_param(gpio_irq, uint, 0);
> > > @@ -65,17 +65,6 @@ static uint i2c1_irq;
> > >  module_param(i2c1_irq, uint, 0);
> > >  MODULE_PARM_DESC(i2c1_irq, "I2C1 IRQ number (valid parameters: 7, 9, 12)");
> > >  
> > > -enum tqmx86_i2c1_resource_type {
> > > -	TQMX86_I2C1_IO,
> > > -	TQMX86_I2C1_IRQ,
> > > -};
> > > -
> > > -static struct resource tqmx_i2c_soft_resources[] = {
> > > -	[TQMX86_I2C1_IO] = DEFINE_RES_IO(TQMX86_IOBASE_I2C, TQMX86_IOSIZE_I2C),
> > > -	/* Placeholder for IRQ resource */
> > > -	[TQMX86_I2C1_IRQ] = {},
> > > -};
> > > -
> > >  static const struct resource tqmx_watchdog_resources[] = {
> > >  	DEFINE_RES_IO(TQMX86_IOBASE_WATCHDOG, TQMX86_IOSIZE_WATCHDOG),
> > >  };
> > > @@ -91,28 +80,13 @@ static struct resource tqmx_gpio_resources[] = {
> > >  	[TQMX86_GPIO_IRQ] = {},
> > >  };
> > >  
> > > -static struct i2c_board_info tqmx86_i2c_devices[] = {
> > > +static const struct i2c_board_info tqmx86_i2c1_devices[] = {
> > >  	{
> > >  		/* 4K EEPROM at 0x50 */
> > >  		I2C_BOARD_INFO("24c32", 0x50),
> > >  	},
> > >  };
> > >  
> > > -static struct ocores_i2c_platform_data ocores_platform_data = {
> > > -	.num_devices = ARRAY_SIZE(tqmx86_i2c_devices),
> > > -	.devices = tqmx86_i2c_devices,
> > > -};
> > > -
> > > -static const struct mfd_cell tqmx86_i2c_soft_dev[] = {
> > > -	{
> > > -		.name = "ocores-i2c",
> > > -		.platform_data = &ocores_platform_data,
> > > -		.pdata_size = sizeof(ocores_platform_data),
> > > -		.resources = tqmx_i2c_soft_resources,
> > > -		.num_resources = ARRAY_SIZE(tqmx_i2c_soft_resources),
> > > -	},
> > > -};
> > > -
> > >  static const struct mfd_cell tqmx86_devs[] = {
> > >  	{
> > >  		.name = "tqmx86-wdt",
> > > @@ -238,13 +212,74 @@ static int tqmx86_setup_irq(struct device *dev, const char *label, u8 irq,
> > >  	return 0;
> > >  }
> > >  
> > > +static int tqmx86_setup_i2c(struct device *dev, const char *name,
> > > +			    unsigned long i2c_base, const void *platform_data,
> > > +			    size_t pdata_size, u8 irq)
> > > +{
> > > +	const struct resource resources[] = {
> > > +		DEFINE_RES_IO(i2c_base, TQMX86_IOSIZE_I2C),
> > > +		irq ? DEFINE_RES_IRQ(irq) : (struct resource) {},
> > > +	};
> > > +	const struct mfd_cell i2c_dev = {
> > > +		.name = name,
> > > +		.platform_data = platform_data,
> > > +		.pdata_size = pdata_size,
> > > +		.resources = resources,
> > > +		.num_resources = ARRAY_SIZE(resources),
> > > +	};
> > 
> > No, please don't do it this way.
> > 
> > Keep as much information as you can in easy to read, easy to reference,
> > easy to find, easy to follow, etc static data.  If you have to add a
> > couple more static structs above, sobeit, but all of this parameter
> > passing through abstracted functions is a regression in readability and
> > maintainability IMHO.
> 
> Hmm, my reasoning for this change was that non-const static data always feels
> yucky (and it can't be const because of the dynamic irq field); but course you
> could argue that it's fine for a platform driver because there can only be a
> single instance.
> 
> Maybe have a const static at the toplevel, and copy that to a stack variable to
> fill in the resources?

It's okay not to be const.  We have a bunch of drivers that dynamically
add platform_data and the like.  It's the lesser of 2 evils.  Adding all
of this cruft dynamically "at the stack level" is suboptimal.

-- 
Lee Jones [李琼斯]