From: Gregor Herburger <gregor.herburger@tq-group.com>

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@tq-group.com>
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

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.
+	 */
+	{},
 	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/

On Mon, 07 Oct 2024, Matthias Schiffer wrote:

> From: Gregor Herburger <gregor.herburger@tq-group.com>
> 
> 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@tq-group.com>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
> 
> 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 [李琼斯]

On Tue, 2024-10-15 at 12:38 +0100, Lee Jones wrote:
> 
> On Mon, 07 Oct 2024, Matthias Schiffer wrote:
> 
> > From: Gregor Herburger <gregor.herburger@tq-group.com>
> > 
> > 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@tq-group.com>
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> > 
> > 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.

I chose my current solution because it resulted in more simple and maintainable code:

- No dynamic array access, the IRQ resource is always written to index 0
- No surprises regarding num_resources, it is always equal to ARRAY_SIZE(resources)

It also allowed to make all mfd_cell const; to make num_resources modifyable, either the const would
need to be removed, or each mfd_cell would need to be copied to a stack variable in the probe
function.

In my opinion, these benefits outweigh the 2*64 bytes saved for the additional resources allocated
by platform_device_add_resources() - and 128 bytes doesn't seem significant at all, in particular
considering that this driver is used on x86_64 only.

Best,
Matthias



> 
> >  	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/
> > 
> 

-- 
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/

On Wed, 16 Oct 2024, Matthias Schiffer wrote:

> On Tue, 2024-10-15 at 12:38 +0100, Lee Jones wrote:
> > 
> > On Mon, 07 Oct 2024, Matthias Schiffer wrote:
> > 
> > > From: Gregor Herburger <gregor.herburger@tq-group.com>
> > > 
> > > 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@tq-group.com>
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > ---
> > > 
> > > 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.
> 
> I chose my current solution because it resulted in more simple and maintainable code:
> 
> - No dynamic array access, the IRQ resource is always written to index 0

Which is fragile (even with the comment).  If you're going to directly
index array elements, please do so with a unique identifier rather than
relying on it happening to reside in the first.

> - No surprises regarding num_resources, it is always equal to ARRAY_SIZE(resources)

No surprises, but sometimes it's incorrect.

> It also allowed to make all mfd_cell const; to make num_resources modifyable, either the const would
> need to be removed, or each mfd_cell would need to be copied to a stack variable in the probe
> function.
> 
> In my opinion, these benefits outweigh the 2*64 bytes saved for the additional resources allocated
> by platform_device_add_resources() - and 128 bytes doesn't seem significant at all, in particular
> considering that this driver is used on x86_64 only.

But doesn't outweigh my disliking for placing a NULL element at the
start of the array.  If you _must_ do something like this, please place
it at the end of the array.

> > 
> > >  	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/
> > > 
> > 
> 
> -- 
> 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 [李琼斯]

On Wed, 2024-10-16 at 14:07 +0100, Lee Jones wrote:
> 
> On Wed, 16 Oct 2024, Matthias Schiffer wrote:
> 
> > On Tue, 2024-10-15 at 12:38 +0100, Lee Jones wrote:
> > > 
> > > On Mon, 07 Oct 2024, Matthias Schiffer wrote:
> > > 
> > > > From: Gregor Herburger <gregor.herburger@tq-group.com>
> > > > 
> > > > 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@tq-group.com>
> > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > > ---
> > > > 
> > > > 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.
> > 
> > I chose my current solution because it resulted in more simple and maintainable code:
> > 
> > - No dynamic array access, the IRQ resource is always written to index 0
> 
> Which is fragile (even with the comment).  If you're going to directly
> index array elements, please do so with a unique identifier rather than
> relying on it happening to reside in the first.
> 
> > - No surprises regarding num_resources, it is always equal to ARRAY_SIZE(resources)
> 
> No surprises, but sometimes it's incorrect.
> 
> > It also allowed to make all mfd_cell const; to make num_resources modifyable, either the const would
> > need to be removed, or each mfd_cell would need to be copied to a stack variable in the probe
> > function.
> > 
> > In my opinion, these benefits outweigh the 2*64 bytes saved for the additional resources allocated
> > by platform_device_add_resources() - and 128 bytes doesn't seem significant at all, in particular
> > considering that this driver is used on x86_64 only.
> 
> But doesn't outweigh my disliking for placing a NULL element at the
> start of the array.  If you _must_ do something like this, please place
> it at the end of the array.
> 

Ok, will move the placeholder to the end, and access it using a defined index instead of 0. I would
still prefer to keep num_resources constant instead of adjusting it. Does this sound acceptable to
you?

Best,
Matthias



> > > 
> > > >  	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/
> > > > 
> > > 
> > 
> > -- 
> > 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/

On Wed, 16 Oct 2024, Matthias Schiffer wrote:

> On Wed, 2024-10-16 at 14:07 +0100, Lee Jones wrote:
> > 
> > On Wed, 16 Oct 2024, Matthias Schiffer wrote:
> > 
> > > On Tue, 2024-10-15 at 12:38 +0100, Lee Jones wrote:
> > > > 
> > > > On Mon, 07 Oct 2024, Matthias Schiffer wrote:
> > > > 
> > > > > From: Gregor Herburger <gregor.herburger@tq-group.com>
> > > > > 
> > > > > 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@tq-group.com>
> > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > > > ---
> > > > > 
> > > > > 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.
> > > 
> > > I chose my current solution because it resulted in more simple and maintainable code:
> > > 
> > > - No dynamic array access, the IRQ resource is always written to index 0
> > 
> > Which is fragile (even with the comment).  If you're going to directly
> > index array elements, please do so with a unique identifier rather than
> > relying on it happening to reside in the first.
> > 
> > > - No surprises regarding num_resources, it is always equal to ARRAY_SIZE(resources)
> > 
> > No surprises, but sometimes it's incorrect.
> > 
> > > It also allowed to make all mfd_cell const; to make num_resources modifyable, either the const would
> > > need to be removed, or each mfd_cell would need to be copied to a stack variable in the probe
> > > function.
> > > 
> > > In my opinion, these benefits outweigh the 2*64 bytes saved for the additional resources allocated
> > > by platform_device_add_resources() - and 128 bytes doesn't seem significant at all, in particular
> > > considering that this driver is used on x86_64 only.
> > 
> > But doesn't outweigh my disliking for placing a NULL element at the
> > start of the array.  If you _must_ do something like this, please place
> > it at the end of the array.
> > 
> 
> Ok, will move the placeholder to the end, and access it using a defined index instead of 0. I would
> still prefer to keep num_resources constant instead of adjusting it. Does this sound acceptable to
> you?

It would allow me to sleep at night, yes. :)

-- 
Lee Jones [李琼斯]

On Wed, 16 Oct 2024, Lee Jones wrote:

> On Wed, 16 Oct 2024, Matthias Schiffer wrote:
> 
> > On Wed, 2024-10-16 at 14:07 +0100, Lee Jones wrote:
> > > 
> > > On Wed, 16 Oct 2024, Matthias Schiffer wrote:
> > > 
> > > > On Tue, 2024-10-15 at 12:38 +0100, Lee Jones wrote:
> > > > > 
> > > > > On Mon, 07 Oct 2024, Matthias Schiffer wrote:
> > > > > 
> > > > > > From: Gregor Herburger <gregor.herburger@tq-group.com>
> > > > > > 
> > > > > > 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@tq-group.com>
> > > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > > > > ---
> > > > > > 
> > > > > > 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.
> > > > 
> > > > I chose my current solution because it resulted in more simple and maintainable code:
> > > > 
> > > > - No dynamic array access, the IRQ resource is always written to index 0
> > > 
> > > Which is fragile (even with the comment).  If you're going to directly
> > > index array elements, please do so with a unique identifier rather than
> > > relying on it happening to reside in the first.
> > > 
> > > > - No surprises regarding num_resources, it is always equal to ARRAY_SIZE(resources)
> > > 
> > > No surprises, but sometimes it's incorrect.
> > > 
> > > > It also allowed to make all mfd_cell const; to make num_resources modifyable, either the const would
> > > > need to be removed, or each mfd_cell would need to be copied to a stack variable in the probe
> > > > function.
> > > > 
> > > > In my opinion, these benefits outweigh the 2*64 bytes saved for the additional resources allocated
> > > > by platform_device_add_resources() - and 128 bytes doesn't seem significant at all, in particular
> > > > considering that this driver is used on x86_64 only.
> > > 
> > > But doesn't outweigh my disliking for placing a NULL element at the
> > > start of the array.  If you _must_ do something like this, please place
> > > it at the end of the array.
> > > 
> > 
> > Ok, will move the placeholder to the end, and access it using a defined index instead of 0. I would
> > still prefer to keep num_resources constant instead of adjusting it. Does this sound acceptable to
> > you?
> 
> It would allow me to sleep at night, yes. :)

Place a neat, succinct single line comment on the same line please.

For example - not verbatim:

	[IRQ] = {}, /* Placeholder */

-- 
Lee Jones [李琼斯]