| 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: <eb1b752b3584d27e4d5e38544e54d7d1b5faf4ab.camel@ew.tq-group.com>
Date: Thu, 06 Nov 2025 14:51:04 +0100
From: Matthias Schiffer <matthias.schiffer@...tq-group.com>
To: Lee Jones <lee@...nel.org>
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, 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?
Best,
Matthias
>
> > + return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, &i2c_dev, 1,
> > + NULL, 0, NULL);
> > +
> > +}
> > +
> > +static int tqmx86_setup_i2c_ocores(struct device *dev, const char *label,
> > + unsigned long i2c_base, int clock_khz, u8 irq,
> > + const struct i2c_board_info *devices,
> > + size_t num_devices)
> > +{
> > + const struct ocores_i2c_platform_data platform_data = {
> > + .clock_khz = clock_khz,
> > + .num_devices = num_devices,
> > + .devices = devices,
> > + };
> > +
> > + return tqmx86_setup_i2c(dev, "ocores-i2c", i2c_base, &platform_data,
> > + sizeof(platform_data), irq);
> > +}
> > +
> > +static int tqmx86_detect_i2c(struct device *dev, const char *label,
> > + unsigned long i2c_base, int clock_khz, u8 irq,
> > + const struct i2c_board_info *devices,
> > + size_t num_devices, void __iomem *io_base,
> > + u8 irq_reg_shift)
> > +{
> > + u8 i2c_det;
> > +
> > + if (tqmx86_setup_irq(dev, label, irq, io_base, irq_reg_shift))
> > + irq = 0;
> > +
> > + /*
> > + * 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.
> > + */
> > + i2c_det = inb(i2c_base + TQMX86_REG_I2C_DETECT);
> > +
> > + if (i2c_det == TQMX86_REG_I2C_DETECT_OCORES)
> > + return tqmx86_setup_i2c_ocores(dev, label, i2c_base, clock_khz,
> > + irq, devices, num_devices);
> > +
> > + return 0;
> > +}
> > +
> > static int tqmx86_probe(struct platform_device *pdev)
> > {
> > - u8 board_id, sauc, rev, i2c_det;
> > + u8 board_id, sauc, rev;
> > struct device *dev = &pdev->dev;
> > const char *board_name;
> > void __iomem *io_base;
> > - int err;
> > + int err, clock_khz;
> >
> > io_base = devm_ioport_map(dev, TQMX86_IOBASE, TQMX86_IOSIZE);
> > if (!io_base)
> > @@ -259,13 +294,6 @@ static int tqmx86_probe(struct platform_device *pdev)
> > "Found %s - Board ID %d, PCB Revision %d, PLD Revision %d\n",
> > board_name, board_id, rev >> 4, rev & 0xf);
> >
> > - /*
> > - * 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.
> > - */
> > - i2c_det = inb(TQMX86_REG_I2C_DETECT);
> > -
> > if (gpio_irq) {
> > err = tqmx86_setup_irq(dev, "GPIO", gpio_irq, io_base,
> > TQMX86_REG_IO_EXT_INT_GPIO_SHIFT);
> > @@ -273,23 +301,13 @@ static int tqmx86_probe(struct platform_device *pdev)
> > tqmx_gpio_resources[TQMX86_GPIO_IRQ] = DEFINE_RES_IRQ(gpio_irq);
> > }
> >
> > - ocores_platform_data.clock_khz = tqmx86_board_id_to_clk_rate(dev, board_id);
> > -
> > - if (i2c_det == TQMX86_REG_I2C_DETECT_SOFT) {
> > - if (i2c1_irq) {
> > - err = tqmx86_setup_irq(dev, "I2C1", i2c1_irq, io_base,
> > - TQMX86_REG_IO_EXT_INT_I2C1_SHIFT);
> > - if (!err)
> > - tqmx_i2c_soft_resources[TQMX86_I2C1_IRQ] = DEFINE_RES_IRQ(i2c1_irq);
> > - }
> > -
> > - 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;
> > - }
> > + clock_khz = tqmx86_board_id_to_clk_rate(dev, board_id);
> > +
> > + err = tqmx86_detect_i2c(dev, "I2C1", TQMX86_IOBASE_I2C1, clock_khz, i2c1_irq,
> > + tqmx86_i2c1_devices, ARRAY_SIZE(tqmx86_i2c1_devices),
> > + io_base, TQMX86_REG_IO_EXT_INT_I2C1_SHIFT);
> > + if (err)
> > + return err;
> >
> > return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> > tqmx86_devs,
> > --
> > 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/
>
--
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/
Powered by blists - more mailing lists