Re: [PATCH v4 5/5] mfd: tqmx86: add I2C IRQ support

[Lee Jones]

On Mon, 07 Oct 2024, Matthias Schiffer wrote:

> From: Gregor Herburger <gregor.herburger@xxxxxxxxxxxx>
> 
> The i2c-ocores controller can run in interrupt mode on tqmx86 modules.
> Add module parameter to allow configuring the IRQ number, similar to the
> handling of the GPIO IRQ.
> 
> Signed-off-by: Gregor Herburger <gregor.herburger@xxxxxxxxxxxx>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>
> ---
> 
> v2: improve module parameter description (was patch 4/4)
> v3: replace IRQ 0 resource with an empty placeholder to simplify error handling
> v4: no changes
> 
>  drivers/mfd/tqmx86.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/tqmx86.c b/drivers/mfd/tqmx86.c
> index e444fcd2a3e9d..057d035b71d33 100644
> --- a/drivers/mfd/tqmx86.c
> +++ b/drivers/mfd/tqmx86.c
> @@ -50,6 +50,7 @@
>  #define TQMX86_REG_IO_EXT_INT_9			2
>  #define TQMX86_REG_IO_EXT_INT_12		3
>  #define TQMX86_REG_IO_EXT_INT_MASK		0x3
> +#define TQMX86_REG_IO_EXT_INT_I2C_SHIFT		0
>  #define TQMX86_REG_IO_EXT_INT_GPIO_SHIFT	4
>  #define TQMX86_REG_SAUC		0x17
>  
> @@ -60,7 +61,16 @@ static uint gpio_irq;
>  module_param(gpio_irq, uint, 0);
>  MODULE_PARM_DESC(gpio_irq, "GPIO IRQ number (valid parameters: 7, 9, 12)");
>  
> -static const struct resource tqmx_i2c_soft_resources[] = {
> +static uint i2c_irq;
> +module_param(i2c_irq, uint, 0);
> +MODULE_PARM_DESC(i2c_irq, "I2C IRQ number (valid parameters: 7, 9, 12)");
> +
> +static struct resource tqmx_i2c_soft_resources[] = {
> +	/*
> +	 * Placeholder for IRQ resource - must come first to be filled in by the
> +	 * probe function.
> +	 */
> +	{},

Having a NULLed entry in the first slot doesn't sit well with me at all.

In order for us to avoid wasting memory, it would be better to place the
entry at the end of the array with a blank entry:

  DEFINE_RES_IRQ(0),

Later comes the matching code which updates the 0 value to something sane.

Before you call to the add the devices, check to see if the value has
changed.  If it hasn't, deprecate num_resources, effectively masking the
last entry in the array.  Then when platform_device_add_resources()
comes to duplicate the array, it will only copy the relevant entries.

>  	DEFINE_RES_IO(TQMX86_IOBASE_I2C, TQMX86_IOSIZE_I2C),
>  };
>  
> @@ -263,6 +273,14 @@ static int tqmx86_probe(struct platform_device *pdev)
>  	ocores_platform_data.clock_khz = tqmx86_board_id_to_clk_rate(dev, board_id);
>  
>  	if (i2c_det == TQMX86_REG_I2C_DETECT_SOFT) {
> +		if (i2c_irq) {
> +			err = tqmx86_setup_irq(dev, "I2C", i2c_irq, io_base,
> +					       TQMX86_REG_IO_EXT_INT_I2C_SHIFT);
> +			if (!err)
> +				/* Assumes the IRQ resource placeholder is first */
> +				tqmx_i2c_soft_resources[0] = DEFINE_RES_IRQ(i2c_irq);
> +		}
> +
>  		err = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
>  					   tqmx86_i2c_soft_dev,
>  					   ARRAY_SIZE(tqmx86_i2c_soft_dev),
> -- 
> 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 [李琼斯]