| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251106164240.GW8064@google.com>
Date: Thu, 6 Nov 2025 16:42:40 +0000
From: Lee Jones <lee@...nel.org>
To: Matthias Schiffer <matthias.schiffer@...tq-group.com>
Cc: Andi Shyti <andi.shyti@...nel.org>, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org, linux@...tq-group.com
Subject: Re: [PATCH v3 2/4] mfd: tqmx86: refactor I2C setup
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@...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 [李琼斯]
Powered by blists - more mailing lists