Messages in this thread | | | Date | Mon, 17 Jul 2017 09:56:50 +0100 | From | Charles Keepax <> | Subject | Re: [GIT PULL] MFD for v4.13 |
| |
On Fri, Jul 07, 2017 at 11:50:14PM +0100, Lee Jones wrote: > On Fri, 07 Jul 2017, Linus Torvalds wrote: > > > On Fri, Jul 7, 2017 at 2:50 AM, Lee Jones <lee.jones@linaro.org> wrote: > > > > > > include/linux/mfd/madera/registers.h | 8832 ++++++++++++++++++++ > > > > So I've pulled everything but this, because honestly, that file looks > > like utter garbage. > > > > Why are there all those _hundreds_ of odd defines for > > > > MADERA_WSEQ_SEQUENCE_xx > > > > when it looks like you could just do one single one: > > > > // The sequence is one-based because somebody doesn't > > // know that indices start at 0. Thus the "-2". > > #define MADERA_WSEQ_SEQUENCE(x) (0x3000 + (x)*2 - 2) > > > > and similar things go for for pretty much EVERY SINGLE LINE in that > > 8-thousand line piece of nasty horrible crud. > > > > Being auto-generated doesn't really make this kind of thing any > > better. In fact, it makes it worse, because those stupid hardcoded > > names are often *harder* to use, because you cannot use a variable to > > index into things (which you may often want). > > > > So we have eight thousand lines of garbage that is > > > > (a) probably closer to 200x too many lines > > (b) less flexible than doing it right > > > > Honestly, tell me why would I want to merge something monstrous like that? > > I'm inclined to agree and had my reservations. However based on > a previous conversation [0], I was convinced that it's actually the > right thing to do. > > https://lkml.org/lkml/2017/4/7/229
We are happy to look at other ways to implement the file but there are a few reasons it looks the way it does, although admittedly the write sequencer defines should probably go.
Copying in Richard's reply from the other thread below, as he doesn't subscribe to LKML, so missed this thread.
Thanks, Charles
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.
| |