Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: William Breathitt Gray <vilhelm.gray@gmail.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: linux-arch <linux-arch@vger.kernel.org>,
	Arnd Bergmann <arnd@arndb.de>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Linux PM mailing list <linux-pm@vger.kernel.org>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	morten.tiljeset@prevas.dk, sean.nyekjaer@prevas.dk,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Lukas Wunner <lukas@wunner.de>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Phil Reid <preid@electromag.com.au>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro
Date: Thu, 10 Oct 2019 09:12:42 -0400
Message-ID: <20191010131242.GA3187@icarus> (raw)
In-Reply-To: <CAMuHMdVv_pYq7rVLvjAPhkoADLgXF-AFGWUBNqLjaumDPBYGaQ@mail.gmail.com>

On Thu, Oct 10, 2019 at 10:21:45AM +0200, Geert Uytterhoeven wrote:
> Hi Andy,
> 
> On Thu, Oct 10, 2019 at 10:08 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Oct 10, 2019 at 09:49:51AM +0200, Geert Uytterhoeven wrote:
> > > On Thu, Oct 10, 2019 at 9:42 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Thu, Oct 10, 2019 at 9:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Thu, Oct 10, 2019 at 7:49 AM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > On Thu, Oct 10, 2019 at 5:31 AM Masahiro Yamada
> > > > > > <yamada.masahiro@socionext.com> wrote:
> > > > > > > On Thu, Oct 10, 2019 at 3:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > > On Wed, Oct 9, 2019 at 7:09 PM Andy Shevchenko
> > > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > > On Thu, Oct 10, 2019 at 01:28:08AM +0900, Masahiro Yamada wrote:
> > > > > > > > > > On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray
> > > > > > > > > > <vilhelm.gray@gmail.com> wrote:
> >
> > > > > > > > > > Why is the return type "unsigned long" where you know
> > > > > > > > > > it return the 8-bit value ?
> > > > > > > > >
> > > > > > > > > Because bitmap API operates on unsigned long type. This is not only
> > > > > > > > > consistency, but for sake of flexibility in case we would like to introduce
> > > > > > > > > more calls like clump16 or so.
> > > > > > > >
> > > > > > > > TBH, that doesn't convince me: those functions explicitly take/return an
> > > > > > > > 8-bit value, and have "8" in their name.  The 8-bit value is never
> > > > > > > > really related to, retrieved from, or stored in a full "unsigned long"
> > > > > > > > element of a bitmap, only to/from/in a part (byte) of it.
> > > > > > > >
> > > > > > > > Following your rationale, all of iowrite{8,16,32,64}*() should take an
> > > > > > > > "unsigned long" value, too.
> > > > > > >
> > > > > > > Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.
> > > > > > >
> > > > > > > void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned
> > > > > > > int nbits);
> > > > > > > void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits);
> > > > > > > static inline void bitmap_from_u64(unsigned long *dst, u64 mask);
> > > > > > >
> > > > > > > If you want to see more examples from other parts,
> > > > > >
> > > > > > Geert's and yours examples both are not related. They are about
> > > > > > fixed-width properies when we know that is the part of protocol.
> > > > > > Here we have no protocol which stricts us to the mentioned fixed-width types.
> > > > >
> > > > > Yes you have: they are functions to store/retrieve an 8-bit value from
> > > > > the middle of the bitmap, which is reflected in their names ("clump8",
> > > > > "value8").
> > > > > The input/output value is clearly separated from the actual bitmap,
> > > > > which is referenced by the "unsigned long *".
> > > > >
> > > > > If you add new "value16" functions, they will be intended to store/retrieve
> > > > > 16-bit values.
> > > >
> > > > And if I add 4-bit, 12-bit or 24-bit values, what should I use?
> > >
> > > Whatever is needed to store that?
> > > I agree "unsigned long" is appropriate for a generic function to extract a
> > > bit field of 1 to BITS_PER_LONG bits.
> > >
> > > > > Besides, if retrieving an 8-bit value requires passing an
> > > > > "unsigned long *", the caller needs two variables: one unsigned long to
> > > > > pass the address of, and one u8 to copy the returned value into.
> > > >
> > > > Why do you need a temporary variable? In some cases it might make
> > > > sense, but in general simple cases I don't see what you may achieve
> > > > with it.
> > >
> > > Because find_next_clump8() takes a pointer to store the output value.
> >
> > So does regmap_read().
> 
> I believe that one is different, as it is a generic function, and the
> width of the
> returned value depends on the regmap config.
> 
> > 8 appeared there during review when it has been proposed to optimize to 8-bit
> > clumps as most of the current users utilize it. The initial idea was to be
> > bit-width agnostic. And with current API it's possible to easy convert to other
> > formats later if we need.
> 
> "optimized for 8-bit clumps" and "out-of-line function that takes an
> unsigned long pointer for an output parameter" don't match well, IMHO.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

"Optimize" may not be the best way of describing it. I conceded to
introducing a restricted implementation (i.e. for_each_set_clump8) since
there were disagreements on the best approach for an implementation a
generic for_each_set_clump macro that could support any bit size. So I
settled for introducing just for_each_set_clump8 since it has an
implementation everyone could agree on and I didn't want to stall the
patchset for this introduction.

I'm hoping to propose the generic for_each_set_clump macro again in the
future after for_each_set_clump8 has had time to be utilized. There are
some files that I think might benefit from such a generic implementation
(e.g. gpio-thunderx with 64-bit ports and gpio-xilinx with variable size
channels). In such case, for_each_set_clump8 would likely be
reimplemented as a macro hardcoding an 8 passed to for_each_set_clump --
or perhaps just eliminated and replaced with for_each_set_clump directly
-- so maintaining clump as unsigned long pointer is useful since we
won't need to worry about redeclaring variables to match the datatype.

Though I admit that there are advantages in specifying the datatype as
u8 (or u16, u32, etc.). If we know the size then it's reasonable to
expect that the implementation can be optimized to not worry about
variable sizes and boundaries -- as exemplified by the simplicity of the
for_each_set_clump8 implementation. So that may be an argument for
keeping the for_each_set_clump8 implementation separate from the generic
for_each_set_clump implementation.

William Breathitt Gray

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-09 15:26 [PATCH v17 00/14] " William Breathitt Gray
2019-10-09 15:26 ` William Breathitt Gray
2019-10-09 15:26 ` [PATCH v17 01/14] bitops: " William Breathitt Gray
2019-10-09 15:26   ` William Breathitt Gray
2019-10-09 16:28   ` Masahiro Yamada
2019-10-09 16:28     ` Masahiro Yamada
2019-10-09 17:02     ` William Breathitt Gray
2019-10-09 17:09     ` Andy Shevchenko
2019-10-09 18:54       ` Geert Uytterhoeven
2019-10-10  2:28         ` Masahiro Yamada
2019-10-10  5:49           ` Andy Shevchenko
2019-10-10  6:29             ` Geert Uytterhoeven
2019-10-10  7:41               ` Andy Shevchenko
2019-10-10  7:49                 ` Geert Uytterhoeven
2019-10-10  8:07                   ` Andy Shevchenko
2019-10-10  8:21                     ` Geert Uytterhoeven
2019-10-10 13:12                       ` William Breathitt Gray [this message]
2019-10-09 15:27 ` [PATCH v17 02/14] lib/test_bitmap.c: Add for_each_set_clump8 test cases William Breathitt Gray
2019-10-09 15:27   ` William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 03/14] gpio: 104-dio-48e: Utilize for_each_set_clump8 macro William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 04/14] gpio: 104-idi-48: " William Breathitt Gray
2019-10-09 15:27   ` William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 05/14] gpio: gpio-mm: " William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 06/14] gpio: ws16c48: " William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 07/14] gpio: pci-idio-16: " William Breathitt Gray
2019-10-09 15:27   ` William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 08/14] gpio: pcie-idio-24: " William Breathitt Gray
2019-10-09 15:27   ` William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 09/14] gpio: uniphier: " William Breathitt Gray
2019-10-09 15:27   ` William Breathitt Gray
2019-10-09 16:33   ` Masahiro Yamada
2019-10-09 16:33     ` Masahiro Yamada
2019-10-09 17:05     ` William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 10/14] gpio: 74x164: Utilize the " William Breathitt Gray
2019-10-09 15:27   ` William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 11/14] thermal: intel: intel_soc_dts_iosf: Utilize " William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 12/14] gpio: pisosr: Utilize the " William Breathitt Gray
2019-10-09 15:27   ` William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 13/14] gpio: max3191x: " William Breathitt Gray
2019-10-09 15:27   ` William Breathitt Gray
2019-10-09 15:27 ` [PATCH v17 14/14] gpio: pca953x: " William Breathitt Gray
2019-10-09 15:27   ` William Breathitt Gray

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191010131242.GA3187@icarus \
    --to=vilhelm.gray@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=andy.shevchenko@gmail.com \
    --cc=arnd@arndb.de \
    --cc=bgolaszewski@baylibre.com \
    --cc=geert@linux-m68k.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=lukas@wunner.de \
    --cc=morten.tiljeset@prevas.dk \
    --cc=preid@electromag.com.au \
    --cc=sean.nyekjaer@prevas.dk \
    --cc=yamada.masahiro@socionext.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git