On Thu, Aug 04, 2022 at 08:28:28PM +0200, Andy Shevchenko wrote: > > > > +static const struct { > > > > + int val; > > > > + int val2; > > > > +} msa311_fs_table[] = { > > > > + {0, 9580}, {0, 19160}, {0, 38320}, {0, 76641} > > > > +}; > > > > > > Besides that this struct is defined not only once in the file, this is > > > very well NIH struct s32_fract. Why not use the latter? > > > Good point, but looks like this struct is not so popular in the other > > kernel drivers: > > It's simply new, it is not about popularity. I would put it as it's > not _yet_ so popular. Okay, no problem. I've already reworked it to s32_fract locally. > > ... > > > grep "s32_fract" -r -l . | wc -l > > 3 > > Hint: `git grep` much much faster on Git repositories (it goes via > index and not working copy) and see > `git grep -lw s32_fract` > Thank you for the hint. Actually I'm using ripgrep for the kernel fuzzysearching, it's faster than git grep. Here I gave an example based on grep command, because it's general shell command for the searching substrings I suppose. > ... > > > > > + .cache_type = REGCACHE_RBTREE, > > > > > > Tree hash is good for sparse data, do you really have it sparse (a lot > > > of big gaps in between)? > > > > I suppose so. MSA311 regmap has ~6 gaps. > > Yes and how long is each gap in comparison to the overall space? The longest gap is 0xb bytes. > > ... > > > > > + for (fs = 0; fs < ARRAY_SIZE(msa311_fs_table); ++fs) > > > > > > fs++ will work as well. > > > > I would prefer ++fs if you don't mind :) > > Why? It's a non-standard pattern, and needs an explanation. > >From statistics point of view, you are right :) $ rg "for" | rg "\+\+[a-z]" | wc -l 7271 $ rg "for" | rg "[a-z]\+\+" | wc -l 81247 [...] > > > > + dev_err(dev, "cannot update freq (%d)\n", err); > > > > > > frequency > > > > It will be more ugly due 80 symbols restriction. > > Nope, it will be understandable by the user. You do code for the user > and then for the developer, right? Sure, that's good point. [...] > > ... > > > > > + dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid); > > > > > > Useless message. > > > > Why? It's under dynamic debug, so I will see it if I really want to. > > So what the point of the _successful_ detection? You already know from > the code, that partid, you know by other means that probe was > successful, why this message is needed? Especially for debugging when > we have initcall_debug option. > Agreed [...] -- Thank you, Dmitry