| Message ID | 1392724308-13375-2-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Mark Brown |
| Headers | show |
On Tue, Feb 18, 2014 at 12:51:28PM +0100, Takashi Iwai wrote: > Add new helper macros for defining the enum elements with a constant > array. With these macros, redundant ARRAY_SIZE() can be removed in > the code. So, this doesn't seem like the best way to do things. For one thing it's not really that the arrays are const, it's the fact that they are a fixed size that makes the difference with these macros. An array where we fill in a fixed number of strings at runtime would work just as well for this. I'd also expect this to be more what the default naming does since we want to try to steer people towards using the simplest and least error prone mechanism for doing this.
At Thu, 20 Feb 2014 11:33:39 +0900, Mark Brown wrote: > > On Tue, Feb 18, 2014 at 12:51:28PM +0100, Takashi Iwai wrote: > > Add new helper macros for defining the enum elements with a constant > > array. With these macros, redundant ARRAY_SIZE() can be removed in > > the code. > > So, this doesn't seem like the best way to do things. For one thing > it's not really that the arrays are const, it's the fact that they are a > fixed size that makes the difference with these macros. An array where > we fill in a fixed number of strings at runtime would work just as well > for this. Hm, in theory, yes. I thought of _FIXED instead of _CONST, too, but the former sounds also obscure, so I picked the latter in the end. > I'd also expect this to be more what the default naming does > since we want to try to steer people towards using the simplest and > least error prone mechanism for doing this. If changing the existing API is fine for you (not adding the new one), it's trivial to convert the patches to do so. Looking at the end result, all SOC_ENUM_SINGLE() and SOC_ENUM_DOUBLE() usages are covered. For any specific usage in future, we may provide lowlevel macros, __SOC_ENUM_SINGLE() with the number of items, too. Takashi
At Thu, 20 Feb 2014 08:08:51 +0100, Takashi Iwai wrote: > > At Thu, 20 Feb 2014 11:33:39 +0900, > Mark Brown wrote: > > > > On Tue, Feb 18, 2014 at 12:51:28PM +0100, Takashi Iwai wrote: > > > Add new helper macros for defining the enum elements with a constant > > > array. With these macros, redundant ARRAY_SIZE() can be removed in > > > the code. > > > > So, this doesn't seem like the best way to do things. For one thing > > it's not really that the arrays are const, it's the fact that they are a > > fixed size that makes the difference with these macros. An array where > > we fill in a fixed number of strings at runtime would work just as well > > for this. > > Hm, in theory, yes. I thought of _FIXED instead of _CONST, too, but > the former sounds also obscure, so I picked the latter in the end. > > > I'd also expect this to be more what the default naming does > > since we want to try to steer people towards using the simplest and > > least error prone mechanism for doing this. > > If changing the existing API is fine for you (not adding the new one), > it's trivial to convert the patches to do so. Looking at the end > result, all SOC_ENUM_SINGLE() and SOC_ENUM_DOUBLE() usages are > covered. For any specific usage in future, we may provide lowlevel > macros, __SOC_ENUM_SINGLE() with the number of items, too. OTOH, changing the API means that we need to fix all callers at one shot. (Otherwise you'll hit bisection problems.) The volume would become big as a single patch. If you prefer this way, let me know. Takashi
On Thu, Feb 20, 2014 at 08:08:51AM +0100, Takashi Iwai wrote: > Mark Brown wrote: > > I'd also expect this to be more what the default naming does > > since we want to try to steer people towards using the simplest and > > least error prone mechanism for doing this. > If changing the existing API is fine for you (not adding the new one), > it's trivial to convert the patches to do so. Looking at the end Yes, I think that's a better approach. It's fairly obvious that the existing interface isn't awesome so it shouldn't be the default one. > result, all SOC_ENUM_SINGLE() and SOC_ENUM_DOUBLE() usages are > covered. For any specific usage in future, we may provide lowlevel > macros, __SOC_ENUM_SINGLE() with the number of items, too. Indeed, I was intending to go through and look at renaming the legacy macros so they're implementaton details once I'd got through all the patches.
At Thu, 20 Feb 2014 18:09:38 +0900, Mark Brown wrote: > > On Thu, Feb 20, 2014 at 08:08:51AM +0100, Takashi Iwai wrote: > > Mark Brown wrote: > > > > I'd also expect this to be more what the default naming does > > > since we want to try to steer people towards using the simplest and > > > least error prone mechanism for doing this. > > > If changing the existing API is fine for you (not adding the new one), > > it's trivial to convert the patches to do so. Looking at the end > > Yes, I think that's a better approach. It's fairly obvious that the > existing interface isn't awesome so it shouldn't be the default one. > > > result, all SOC_ENUM_SINGLE() and SOC_ENUM_DOUBLE() usages are > > covered. For any specific usage in future, we may provide lowlevel > > macros, __SOC_ENUM_SINGLE() with the number of items, too. > > Indeed, I was intending to go through and look at renaming the legacy > macros so they're implementaton details once I'd got through all the > patches. OK, but still a question is whether to split patches or not. By splitting patches, we'll break the build in between, so the bisection won't work well. OTOH, a big single patch would be too big. I myself am inclined to have split patches despite bisection breakage in this case. Takashi
On Thu, Feb 20, 2014 at 03:56:11PM +0100, Takashi Iwai wrote: > OK, but still a question is whether to split patches or not. > By splitting patches, we'll break the build in between, so the > bisection won't work well. OTOH, a big single patch would be too > big. I myself am inclined to have split patches despite bisection > breakage in this case. I'd go with a single patch - if it's big and repetitive it's not going to be that hard to review - but I think either way is probably OK in the end so whatever you feel comfortable with.
diff --git a/include/sound/soc.h b/include/sound/soc.h index cc891387e7ac..11966c1cd33a 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -285,9 +285,13 @@ * Simplified versions of above macros, declaring a struct and calculating * ARRAY_SIZE internally */ +#define SOC_ENUM_DOUBLE_CONST(xreg, xshift_l, xshift_r, xtexts) \ + SOC_ENUM_DOUBLE(xreg, xshift_l, xshift_r, ARRAY_SIZE(xtexts), xtexts) +#define SOC_ENUM_SINGLE_CONST(xreg, xshift, xtexts) \ + SOC_ENUM_DOUBLE_CONST(xreg, xshift, xshift, xtexts) + #define SOC_ENUM_DOUBLE_DECL(name, xreg, xshift_l, xshift_r, xtexts) \ - const struct soc_enum name = SOC_ENUM_DOUBLE(xreg, xshift_l, xshift_r, \ - ARRAY_SIZE(xtexts), xtexts) + const struct soc_enum name = SOC_ENUM_DOUBLE_CONST(xreg, xshift_l, xshift_r, xtexts) #define SOC_ENUM_SINGLE_DECL(name, xreg, xshift, xtexts) \ SOC_ENUM_DOUBLE_DECL(name, xreg, xshift, xshift, xtexts) #define SOC_ENUM_SINGLE_EXT_DECL(name, xtexts) \
Add new helper macros for defining the enum elements with a constant array. With these macros, redundant ARRAY_SIZE() can be removed in the code. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- include/sound/soc.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)