[PATCH v2 1/1] regmap-irq: Place kernel doc of struct regmap_irq_chip in order

[PATCH v2 1/1] regmap-irq: Place kernel doc of struct regmap_irq_chip in order

[Andy Shevchenko]

It seems that a couple of members got lost theirorder, put them back.
Besides that, split field descriptions into groups in the same way
as it's done in the structure definition.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: rebased on the latest Linux Next (Mark)
 include/linux/regmap.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 4d10790adeb0..1c777566fb7d 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1551,6 +1551,7 @@ struct regmap_irq_chip_data;
  * @use_ack:     Use @ack register even if it is zero.
  * @ack_invert:  Inverted ack register: cleared bits for ack.
  * @clear_ack:  Use this to set 1 and 0 or vice-versa to clear interrupts.
+ * @status_invert: Inverted status register: cleared bits are active interrupts.
  * @wake_invert: Inverted wake register: cleared bits are wake enabled.
  * @type_in_mask: Use the mask registers for controlling irq type. Use this if
  *		  the hardware provides separate bits for rising/falling edge
@@ -1560,18 +1561,19 @@ struct regmap_irq_chip_data;
  * @clear_on_unmask: For chips with interrupts cleared on read: read the status
  *                   registers before unmasking interrupts to clear any bits
  *                   set when they were masked.
+ * @runtime_pm:  Hold a runtime PM lock on the device when accessing it.
  * @not_fixed_stride: Used when chip peripherals are not laid out with fixed
  *		      stride. Must be used with sub_reg_offsets containing the
  *		      offsets to each peripheral. Deprecated; the same thing
  *		      can be accomplished with a @get_irq_reg callback, without
  *		      the need for a @sub_reg_offsets table.
- * @status_invert: Inverted status register: cleared bits are active interrupts.
- * @runtime_pm:  Hold a runtime PM lock on the device when accessing it.
  *
  * @num_regs:    Number of registers in each control bank.
+ *
  * @irqs:        Descriptors for individual IRQs.  Interrupt numbers are
  *               assigned based on the index in the array of the interrupt.
  * @num_irqs:    Number of descriptors.
+ *
  * @num_type_reg:    Number of type registers. Deprecated, use config registers
  *		     instead.
  * @num_virt_regs:   Number of non-standard irq configuration registers.
@@ -1579,6 +1581,7 @@ struct regmap_irq_chip_data;
  *		     instead.
  * @num_config_bases:	Number of config base registers.
  * @num_config_regs:	Number of config registers for each config base register.
+ *
  * @handle_pre_irq:  Driver specific callback to handle interrupt from device
  *		     before regmap_irq_handler process the interrupts.
  * @handle_post_irq: Driver specific callback to handle interrupt from device
@@ -1625,12 +1628,12 @@ struct regmap_irq_chip {
 	unsigned int use_ack:1;
 	unsigned int ack_invert:1;
 	unsigned int clear_ack:1;
+	unsigned int status_invert:1;
 	unsigned int wake_invert:1;
-	unsigned int runtime_pm:1;
 	unsigned int type_in_mask:1;
 	unsigned int clear_on_unmask:1;
+	unsigned int runtime_pm:1;
 	unsigned int not_fixed_stride:1;
-	unsigned int status_invert:1;
 
 	int num_regs;
 
-- 
2.39.1

Re: [PATCH v2 1/1] regmap-irq: Place kernel doc of struct regmap_irq_chip in order

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 3625 bytes --]

On Mon, Feb 20, 2023 at 05:33:34PM +0200, Andy Shevchenko wrote:
> It seems that a couple of members got lost theirorder, put them back.

Looks like a typographical error here: "theirorder".

> Besides that, split field descriptions into groups in the same way
> as it's done in the structure definition.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: rebased on the latest Linux Next (Mark)
>  include/linux/regmap.h | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index 4d10790adeb0..1c777566fb7d 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -1551,6 +1551,7 @@ struct regmap_irq_chip_data;
>   * @use_ack:     Use @ack register even if it is zero.
>   * @ack_invert:  Inverted ack register: cleared bits for ack.
>   * @clear_ack:  Use this to set 1 and 0 or vice-versa to clear interrupts.
> + * @status_invert: Inverted status register: cleared bits are active interrupts.
>   * @wake_invert: Inverted wake register: cleared bits are wake enabled.
>   * @type_in_mask: Use the mask registers for controlling irq type. Use this if
>   *		  the hardware provides separate bits for rising/falling edge
> @@ -1560,18 +1561,19 @@ struct regmap_irq_chip_data;
>   * @clear_on_unmask: For chips with interrupts cleared on read: read the status
>   *                   registers before unmasking interrupts to clear any bits
>   *                   set when they were masked.
> + * @runtime_pm:  Hold a runtime PM lock on the device when accessing it.
>   * @not_fixed_stride: Used when chip peripherals are not laid out with fixed
>   *		      stride. Must be used with sub_reg_offsets containing the
>   *		      offsets to each peripheral. Deprecated; the same thing
>   *		      can be accomplished with a @get_irq_reg callback, without
>   *		      the need for a @sub_reg_offsets table.
> - * @status_invert: Inverted status register: cleared bits are active interrupts.
> - * @runtime_pm:  Hold a runtime PM lock on the device when accessing it.
>   *
>   * @num_regs:    Number of registers in each control bank.
> + *
>   * @irqs:        Descriptors for individual IRQs.  Interrupt numbers are
>   *               assigned based on the index in the array of the interrupt.
>   * @num_irqs:    Number of descriptors.
> + *
>   * @num_type_reg:    Number of type registers. Deprecated, use config registers
>   *		     instead.
>   * @num_virt_regs:   Number of non-standard irq configuration registers.
> @@ -1579,6 +1581,7 @@ struct regmap_irq_chip_data;
>   *		     instead.
>   * @num_config_bases:	Number of config base registers.
>   * @num_config_regs:	Number of config registers for each config base register.
> + *
>   * @handle_pre_irq:  Driver specific callback to handle interrupt from device
>   *		     before regmap_irq_handler process the interrupts.
>   * @handle_post_irq: Driver specific callback to handle interrupt from device
> @@ -1625,12 +1628,12 @@ struct regmap_irq_chip {
>  	unsigned int use_ack:1;
>  	unsigned int ack_invert:1;
>  	unsigned int clear_ack:1;
> +	unsigned int status_invert:1;
>  	unsigned int wake_invert:1;
> -	unsigned int runtime_pm:1;
>  	unsigned int type_in_mask:1;
>  	unsigned int clear_on_unmask:1;
> +	unsigned int runtime_pm:1;
>  	unsigned int not_fixed_stride:1;
> -	unsigned int status_invert:1;

These don't look alphabetical, so what is the order for these?

William Breathitt Gray

>  
>  	int num_regs;
>  
> -- 
> 2.39.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/1] regmap-irq: Place kernel doc of struct regmap_irq_chip in order

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]

On Thu, Feb 23, 2023 at 08:52:36PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 22, 2023 at 11:43:28AM -0500, William Breathitt Gray wrote:
> > On Mon, Feb 20, 2023 at 05:33:34PM +0200, Andy Shevchenko wrote:
> > >  	unsigned int use_ack:1;
> > >  	unsigned int ack_invert:1;
> > >  	unsigned int clear_ack:1;
> > > +	unsigned int status_invert:1;
> > >  	unsigned int wake_invert:1;
> > > -	unsigned int runtime_pm:1;
> > >  	unsigned int type_in_mask:1;
> > >  	unsigned int clear_on_unmask:1;
> > > +	unsigned int runtime_pm:1;
> > >  	unsigned int not_fixed_stride:1;
> > > -	unsigned int status_invert:1;
> > 
> > These don't look alphabetical, so what is the order for these?
> 
> Nope, the order is to follow:
> a) kernel doc
> b) semantics of each of the groups
> 
> Do you think the order can be improved? Can you point out how?

No, I don't have any particular improvement suggestions, I'm just want
to understand the current order for when I introduce something new here
(e.g. no_status). If I understand correctly, status_invert was moved up
to be with the other "*_invert" flags, but why was runtime_pm moved
to above not_fixed_stride rather than below it?

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/1] regmap-irq: Place kernel doc of struct regmap_irq_chip in order

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]

On Fri, Feb 24, 2023 at 05:01:09PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 22, 2023 at 12:46:05PM -0500, William Breathitt Gray wrote:
> > On Thu, Feb 23, 2023 at 08:52:36PM +0200, Andy Shevchenko wrote:
> > > On Wed, Feb 22, 2023 at 11:43:28AM -0500, William Breathitt Gray wrote:
> > > > On Mon, Feb 20, 2023 at 05:33:34PM +0200, Andy Shevchenko wrote:
> > > > >  	unsigned int use_ack:1;
> > > > >  	unsigned int ack_invert:1;
> > > > >  	unsigned int clear_ack:1;
> > > > > +	unsigned int status_invert:1;
> > > > >  	unsigned int wake_invert:1;
> > > > > -	unsigned int runtime_pm:1;
> > > > >  	unsigned int type_in_mask:1;
> > > > >  	unsigned int clear_on_unmask:1;
> > > > > +	unsigned int runtime_pm:1;
> > > > >  	unsigned int not_fixed_stride:1;
> > > > > -	unsigned int status_invert:1;
> > > > 
> > > > These don't look alphabetical, so what is the order for these?
> > > 
> > > Nope, the order is to follow:
> > > a) kernel doc
> > > b) semantics of each of the groups
> > > 
> > > Do you think the order can be improved? Can you point out how?
> > 
> > No, I don't have any particular improvement suggestions, I'm just want
> > to understand the current order for when I introduce something new here
> > (e.g. no_status). If I understand correctly, status_invert was moved up
> > to be with the other "*_invert" flags,
> 
> As far as I read these there are IRQ related flags, and some others.

Okay, that makes sense to me too.

> > but why was runtime_pm moved
> > to above not_fixed_stride rather than below it?
> 
> Do you think that not_fixed_stride belongs to "*mask" group?

However, I don't think runtime_pm belongs to a "*mask" group either
because it's a lock for power management if I'm not mistaken. So if we
can move runtime_pm below not_fixed_stride then we can avoid the
respective runtime_pm kernel doc line move which makes the diff for this
patch slightly simpler.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/1] regmap-irq: Place kernel doc of struct regmap_irq_chip in order

[Andy Shevchenko]

On Wed, Feb 22, 2023 at 11:43:28AM -0500, William Breathitt Gray wrote:
> On Mon, Feb 20, 2023 at 05:33:34PM +0200, Andy Shevchenko wrote:
> > It seems that a couple of members got lost theirorder, put them back.
> 
> Looks like a typographical error here: "theirorder".

Ah, thanks!

...

> >  	unsigned int use_ack:1;
> >  	unsigned int ack_invert:1;
> >  	unsigned int clear_ack:1;
> > +	unsigned int status_invert:1;
> >  	unsigned int wake_invert:1;
> > -	unsigned int runtime_pm:1;
> >  	unsigned int type_in_mask:1;
> >  	unsigned int clear_on_unmask:1;
> > +	unsigned int runtime_pm:1;
> >  	unsigned int not_fixed_stride:1;
> > -	unsigned int status_invert:1;
> 
> These don't look alphabetical, so what is the order for these?

Nope, the order is to follow:
a) kernel doc
b) semantics of each of the groups

Do you think the order can be improved? Can you point out how?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] regmap-irq: Place kernel doc of struct regmap_irq_chip in order

[Andy Shevchenko]

On Wed, Feb 22, 2023 at 12:46:05PM -0500, William Breathitt Gray wrote:
> On Thu, Feb 23, 2023 at 08:52:36PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 22, 2023 at 11:43:28AM -0500, William Breathitt Gray wrote:
> > > On Mon, Feb 20, 2023 at 05:33:34PM +0200, Andy Shevchenko wrote:
> > > >  	unsigned int use_ack:1;
> > > >  	unsigned int ack_invert:1;
> > > >  	unsigned int clear_ack:1;
> > > > +	unsigned int status_invert:1;
> > > >  	unsigned int wake_invert:1;
> > > > -	unsigned int runtime_pm:1;
> > > >  	unsigned int type_in_mask:1;
> > > >  	unsigned int clear_on_unmask:1;
> > > > +	unsigned int runtime_pm:1;
> > > >  	unsigned int not_fixed_stride:1;
> > > > -	unsigned int status_invert:1;
> > > 
> > > These don't look alphabetical, so what is the order for these?
> > 
> > Nope, the order is to follow:
> > a) kernel doc
> > b) semantics of each of the groups
> > 
> > Do you think the order can be improved? Can you point out how?
> 
> No, I don't have any particular improvement suggestions, I'm just want
> to understand the current order for when I introduce something new here
> (e.g. no_status). If I understand correctly, status_invert was moved up
> to be with the other "*_invert" flags,

As far as I read these there are IRQ related flags, and some others.

> but why was runtime_pm moved
> to above not_fixed_stride rather than below it?

Do you think that not_fixed_stride belongs to "*mask" group?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] regmap-irq: Place kernel doc of struct regmap_irq_chip in order

[Andy Shevchenko]

On Wed, Feb 22, 2023 at 02:24:55PM -0500, William Breathitt Gray wrote:
> On Fri, Feb 24, 2023 at 05:01:09PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 22, 2023 at 12:46:05PM -0500, William Breathitt Gray wrote:
> > > On Thu, Feb 23, 2023 at 08:52:36PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Feb 22, 2023 at 11:43:28AM -0500, William Breathitt Gray wrote:
> > > > > On Mon, Feb 20, 2023 at 05:33:34PM +0200, Andy Shevchenko wrote:
> > > > > >  	unsigned int use_ack:1;
> > > > > >  	unsigned int ack_invert:1;
> > > > > >  	unsigned int clear_ack:1;
> > > > > > +	unsigned int status_invert:1;
> > > > > >  	unsigned int wake_invert:1;
> > > > > > -	unsigned int runtime_pm:1;
> > > > > >  	unsigned int type_in_mask:1;
> > > > > >  	unsigned int clear_on_unmask:1;
> > > > > > +	unsigned int runtime_pm:1;
> > > > > >  	unsigned int not_fixed_stride:1;
> > > > > > -	unsigned int status_invert:1;
> > > > > 
> > > > > These don't look alphabetical, so what is the order for these?
> > > > 
> > > > Nope, the order is to follow:
> > > > a) kernel doc
> > > > b) semantics of each of the groups
> > > > 
> > > > Do you think the order can be improved? Can you point out how?
> > > 
> > > No, I don't have any particular improvement suggestions, I'm just want
> > > to understand the current order for when I introduce something new here
> > > (e.g. no_status). If I understand correctly, status_invert was moved up
> > > to be with the other "*_invert" flags,
> > 
> > As far as I read these there are IRQ related flags, and some others.
> 
> Okay, that makes sense to me too.
> 
> > > but why was runtime_pm moved
> > > to above not_fixed_stride rather than below it?
> > 
> > Do you think that not_fixed_stride belongs to "*mask" group?
> 
> However, I don't think runtime_pm belongs to a "*mask" group either
> because it's a lock for power management if I'm not mistaken. So if we
> can move runtime_pm below not_fixed_stride then we can avoid the
> respective runtime_pm kernel doc line move which makes the diff for this
> patch slightly simpler.

Good point! I will address this in v3.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] regmap-irq: Place kernel doc of struct regmap_irq_chip in order

[Mark Brown]

On Mon, 20 Feb 2023 17:33:34 +0200, Andy Shevchenko wrote:
> It seems that a couple of members got lost theirorder, put them back.
> Besides that, split field descriptions into groups in the same way
> as it's done in the structure definition.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[1/1] regmap-irq: Place kernel doc of struct regmap_irq_chip in order
      commit: 9b400171a69d2487c3196cc3b6de60de3b08e1ee

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark