Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1686358 > unrolled thread

Re: [PATCH v2 01/18] mfd: madera: Add register definitions for Cirrus Logic Madera codecs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 01/18] mfd: madera: Add register definitions for  Cirrus Logic Madera codecs Lee Jones <lee.jones@linaro.org> - 2017-07-13 10:10 +0200
    Re: [PATCH v2 01/18] mfd: madera: Add register definitions for  Cirrus Logic Madera codecs Mark Brown <broonie@kernel.org> - 2017-07-13 12:10 +0200
      Re: [PATCH v2 01/18] mfd: madera: Add register definitions for  Cirrus Logic Madera codecs Richard Fitzgerald <rf@opensource.wolfsonmicro.com> - 2017-07-13 14:50 +0200
        Re: [PATCH v2 01/18] mfd: madera: Add register definitions for  Cirrus Logic Madera codecs Mark Brown <broonie@kernel.org> - 2017-07-13 15:10 +0200
          Re: [PATCH v2 01/18] mfd: madera: Add register definitions for  Cirrus Logic Madera codecs Richard Fitzgerald <rf@opensource.wolfsonmicro.com> - 2017-07-14 14:00 +0200
            Re: [alsa-devel] [PATCH v2 01/18] mfd: madera: Add register definitions for Cirrus Logic Madera codecs Takashi Iwai <tiwai@suse.de> - 2017-07-14 14:10 +0200

#1686358 — Re: [PATCH v2 01/18] mfd: madera: Add register definitions for Cirrus Logic Madera codecs

On Mon, 24 Apr 2017, Richard Fitzgerald wrote:

> This patch adds a header file of register definitions for Cirrus
> Logic "Madera" class codecs. These codecs are all based off a common
> set of hardware IP so have a common register map (with a few minor
> device-to-device variations). These are complex devices with a large
> number of features and so have a correspondingly large register set.
> The registers.h file has been auto-generated from the hardware register
> definitions, stripped down to only registers we need to access from
> the driver.
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> ---
> 
> No changes since V1.
> 
>  MAINTAINERS                          |   10 +
>  include/linux/mfd/madera/registers.h | 8832 ++++++++++++++++++++++++++++++++++
>  2 files changed, 8842 insertions(+)
>  create mode 100644 include/linux/mfd/madera/registers.h

This patch has been rejected by Linus.

https://lkml.org/lkml/2017/7/7/579

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [next] | [standalone]

#1686424

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 13, 2017 at 09:02:10AM +0100, Lee Jones wrote:

> This patch has been rejected by Linus.

> https://lkml.org/lkml/2017/7/7/579

Hrm, when I used to push the register definition patches I did elide all
the obviously repeated register banks like the write sequencer one that
Linus is calling out there.  I'm surprised by the "every single line"
bit though...

[toc] | [prev] | [next] | [standalone]

#1686511

On Thu, 2017-07-13 at 11:05 +0100, Mark Brown wrote:
> On Thu, Jul 13, 2017 at 09:02:10AM +0100, Lee Jones wrote:
> 
> > This patch has been rejected by Linus.
> 
> > https://lkml.org/lkml/2017/7/7/579
> 
> Hrm, when I used to push the register definition patches I did elide all
> the obviously repeated register banks like the write sequencer one that
> Linus is calling out there.  I'm surprised by the "every single line"
> bit though...

Linux doesn't say what he wants us to do about it. I could manually
strip out a few more definitions but seriously, it makes the code a lot
harder to maintain if we can't grep it for use of registers and register
fields. They are big chips, they have a lot of stuff in the registers.

[toc] | [prev] | [next] | [standalone]

#1686518

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 13, 2017 at 01:44:10PM +0100, Richard Fitzgerald wrote:
> On Thu, 2017-07-13 at 11:05 +0100, Mark Brown wrote:
> > On Thu, Jul 13, 2017 at 09:02:10AM +0100, Lee Jones wrote:

> > > This patch has been rejected by Linus.

> > > https://lkml.org/lkml/2017/7/7/579

> > Hrm, when I used to push the register definition patches I did elide all
> > the obviously repeated register banks like the write sequencer one that
> > Linus is calling out there.  I'm surprised by the "every single line"
> > bit though...

> Linux doesn't say what he wants us to do about it. I could manually
> strip out a few more definitions but seriously, it makes the code a lot
> harder to maintain if we can't grep it for use of registers and register
> fields. They are big chips, they have a lot of stuff in the registers.

I'd take it up with him, if you can explain why it looks very
repetitive but isn't actually that'd help...

Building up copies of the repeated IPs with macros would help too (the
AIF and especially frame control registers stick out like a sore thumb
here), as would removing the individual registers for the write
sequencer block.

[toc] | [prev] | [next] | [standalone]

#1687286

On Thu, 2017-07-13 at 14:03 +0100, Mark Brown wrote:
> On Thu, Jul 13, 2017 at 01:44:10PM +0100, Richard Fitzgerald wrote:
> > On Thu, 2017-07-13 at 11:05 +0100, Mark Brown wrote:
> > > On Thu, Jul 13, 2017 at 09:02:10AM +0100, Lee Jones wrote:
> 
> > > > This patch has been rejected by Linus.
> 
> > > > https://lkml.org/lkml/2017/7/7/579
> 
> > > Hrm, when I used to push the register definition patches I did elide all
> > > the obviously repeated register banks like the write sequencer one that
> > > Linus is calling out there.  I'm surprised by the "every single line"
> > > bit though...
> 
> > Linux doesn't say what he wants us to do about it. I could manually
> > strip out a few more definitions but seriously, it makes the code a lot
> > harder to maintain if we can't grep it for use of registers and register
> > fields. They are big chips, they have a lot of stuff in the registers.
> 
> I'd take it up with him, if you can explain why it looks very
> repetitive but isn't actually that'd help...
> 

I would but I haven't subscribed to receive LKML email because of the
high bandwidth, and I'm not sure how thrilled Linus would be if my reply
isn't a reply to his thread.

> Building up copies of the repeated IPs with macros would help too (the
> AIF and especially frame control registers stick out like a sore thumb
> here), as would removing the individual registers for the write
> sequencer block.

Mark: I know you already know what's below so apologies for this. But
for others on this thread interested in why our registers.h is the way
it is...

We really quite like to keep all the registers defined because it makes
the source highly greppable, which is extremely useful. A type of
question which is often asked is "what does the driver do with register
bits XYZ" or "where does it use register bits XYZ". Eliminating
definitions into generator macros or block-indexing code makes these
questions more difficult to answer. Another convenience is that it makes
it easier for people to translate raw hardware configurations (which are
a list of register values) into ALSA and pdata settings if they can
easily grep the source for those registers to see how they are
controlled. It also helps when adapting the driver for another similar
codec.

We do use block-indexing code where it's clearly the only sensible way
to implement a feature. But a lot of the source that uses
repetitive-looking definitions is ASoC tables, not functions, so there's
no code to do block-indexing, and macroing out the register definitions
just makes the table definitions more clunky and less readable but has
no effect on its "quality". I did already strip a lot out of the file to
remove stuff we don't need or we implement as repeated blocks (the
original file is >44000 lines).

As for the naming and the 1-indexed counting, these are the datasheet
names of the registers and we want to use the same name in the source as
is in the datasheet and the hardware design, otherwise it gets
confusing. In fact, the majority are the names of actual wires and
blocks in the hardware design, generated directly from it. Renaming them
is a big deal for the hardware engineers because it's a design change
and you can't patch silicon if it introduces a bug. The 0-indexed
counting is a very C-centric view, hardware designers have other
considerations, as well as avoiding the risk of unnecessary changes.

The WSEQ registers can go but I was hoping to avoid manually stripping
too much more because when there are updated register maps for new chips
or new revisions of existing silicon it must be possible to see what's
changed. As we strip more and more it gets harder work for people to see
in a diff the difference between genuinely removed register bits and
spurious changes from manual edits. However, I can strip out definitions
of bits that we aren't using _right now_ to reduce file size and patch
them back later when we need to use them, though that does mean anyone
debugging (via regmap debugfs for example) has to go to the datasheet to
look for the definitions of all those bits because they are missing from
the source.

[toc] | [prev] | [next] | [standalone]

#1687294 — Re: [alsa-devel] [PATCH v2 01/18] mfd: madera: Add register definitions for Cirrus Logic Madera codecs

On Fri, 14 Jul 2017 13:50:59 +0200,
Richard Fitzgerald wrote:
> 
> On Thu, 2017-07-13 at 14:03 +0100, Mark Brown wrote:
> > On Thu, Jul 13, 2017 at 01:44:10PM +0100, Richard Fitzgerald wrote:
> > > On Thu, 2017-07-13 at 11:05 +0100, Mark Brown wrote:
> > > > On Thu, Jul 13, 2017 at 09:02:10AM +0100, Lee Jones wrote:
> > 
> > > > > This patch has been rejected by Linus.
> > 
> > > > > https://lkml.org/lkml/2017/7/7/579
> > 
> > > > Hrm, when I used to push the register definition patches I did elide all
> > > > the obviously repeated register banks like the write sequencer one that
> > > > Linus is calling out there.  I'm surprised by the "every single line"
> > > > bit though...
> > 
> > > Linux doesn't say what he wants us to do about it. I could manually
> > > strip out a few more definitions but seriously, it makes the code a lot
> > > harder to maintain if we can't grep it for use of registers and register
> > > fields. They are big chips, they have a lot of stuff in the registers.
> > 
> > I'd take it up with him, if you can explain why it looks very
> > repetitive but isn't actually that'd help...
> > 
> 
> I would but I haven't subscribed to receive LKML email because of the
> high bandwidth, and I'm not sure how thrilled Linus would be if my reply
> isn't a reply to his thread.
> 
> > Building up copies of the repeated IPs with macros would help too (the
> > AIF and especially frame control registers stick out like a sore thumb
> > here), as would removing the individual registers for the write
> > sequencer block.
> 
> Mark: I know you already know what's below so apologies for this. But
> for others on this thread interested in why our registers.h is the way
> it is...
> 
> We really quite like to keep all the registers defined because it makes
> the source highly greppable, which is extremely useful. A type of
> question which is often asked is "what does the driver do with register
> bits XYZ" or "where does it use register bits XYZ". Eliminating
> definitions into generator macros or block-indexing code makes these
> questions more difficult to answer. Another convenience is that it makes
> it easier for people to translate raw hardware configurations (which are
> a list of register values) into ALSA and pdata settings if they can
> easily grep the source for those registers to see how they are
> controlled. It also helps when adapting the driver for another similar
> codec.
> 
> We do use block-indexing code where it's clearly the only sensible way
> to implement a feature. But a lot of the source that uses
> repetitive-looking definitions is ASoC tables, not functions, so there's
> no code to do block-indexing, and macroing out the register definitions
> just makes the table definitions more clunky and less readable but has
> no effect on its "quality". I did already strip a lot out of the file to
> remove stuff we don't need or we implement as repeated blocks (the
> original file is >44000 lines).
> 
> As for the naming and the 1-indexed counting, these are the datasheet
> names of the registers and we want to use the same name in the source as
> is in the datasheet and the hardware design, otherwise it gets
> confusing. In fact, the majority are the names of actual wires and
> blocks in the hardware design, generated directly from it. Renaming them
> is a big deal for the hardware engineers because it's a design change
> and you can't patch silicon if it introduces a bug. The 0-indexed
> counting is a very C-centric view, hardware designers have other
> considerations, as well as avoiding the risk of unnecessary changes.
> 
> The WSEQ registers can go but I was hoping to avoid manually stripping
> too much more because when there are updated register maps for new chips
> or new revisions of existing silicon it must be possible to see what's
> changed. As we strip more and more it gets harder work for people to see
> in a diff the difference between genuinely removed register bits and
> spurious changes from manual edits. However, I can strip out definitions
> of bits that we aren't using _right now_ to reduce file size and patch
> them back later when we need to use them, though that does mean anyone
> debugging (via regmap debugfs for example) has to go to the datasheet to
> look for the definitions of all those bits because they are missing from
> the source.

Well, you can generate a header file dynamically from the minimal
definitions in Makefile by some scripting, too.  It's another hack,
and not always recommended, but if it really helps reducing the size
significantly, it's worth to consider.


Takashi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web