Messages in this thread | | | Subject | Re: [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control | From | Cezary Rojewski <> | Date | Fri, 19 Jul 2019 09:19:10 +0200 |
| |
On 2019-07-19 09:09, Oleksandr Suvorov wrote: > On Thu, 18 Jul 2019 at 21:49, Cezary Rojewski <cezary.rojewski@intel.com> wrote: >> >> On 2019-07-18 20:42, Cezary Rojewski wrote: >>> On 2019-07-18 11:02, Oleksandr Suvorov wrote: >>>> +enum { >>>> + HP_POWER_EVENT, >>>> + DAC_POWER_EVENT, >>>> + ADC_POWER_EVENT, >>>> + LAST_POWER_EVENT >>>> +}; >>>> + >>>> +static u16 mute_mask[] = { >>>> + SGTL5000_HP_MUTE, >>>> + SGTL5000_OUTPUTS_MUTE, >>>> + SGTL5000_OUTPUTS_MUTE >>>> +}; >>> >>> If mute_mask[] is only used within common handler, you may consider >>> declaring const array within said handler instead (did not check that >>> myself). >>> Otherwise, simple comment for the second _OUTPUTS_MUTE should suffice - >>> its not self explanatory why you doubled that mask. > > Ok, I'll add a comment to explain doubled mask. > >>> >>>> + >>>> /* sgtl5000 private structure in codec */ >>>> struct sgtl5000_priv { >>>> int sysclk; /* sysclk rate */ >>>> @@ -137,8 +157,109 @@ struct sgtl5000_priv { >>>> u8 micbias_voltage; >>>> u8 lrclk_strength; >>>> u8 sclk_strength; >>>> + u16 mute_state[LAST_POWER_EVENT]; >>>> }; >>> >>> When I spoke of LAST enum constant, I did not really had this specific >>> usage in mind. >>> >>> From design perspective, _LAST_ does not exist and should never be >>> referred to as "the next option" i.e.: new enum constant. > > By its nature, LAST_POWER_EVENT is actually a size of the array, but I > couldn't come up with a better name. > >>> That is way preferred usage is: >>> u16 mute_state[ADC_POWER_EVENT+1; >>> -or- >>> u16 mute_state[LAST_POWER_EVENT+1]; >>> >>> Maybe I'm just being radical here :) > > Maybe :) I don't like first variant (ADC_POWER_EVENT+1): somewhen in > future, someone can add a new event to this enum and we've got a > possible situation with "out of array indexing". > >>> >>> Czarek >> >> Forgive me for double posting. Comment above is targeted towards: >> >> >> +enum { >> >> + HP_POWER_EVENT, >> >> + DAC_POWER_EVENT, >> >> + ADC_POWER_EVENT, >> >> + LAST_POWER_EVENT >> >> +}; >> >> as LAST_POWER_EVENT is not assigned explicitly to ADC_POWER_EVENT and >> thus generates implicit "new option" of value 3. > > So will you be happy with the following variant? > ... > ADC_POWER_EVENT, > LAST_POWER_EVENT = ADC_POWER_EVENT, > ... > u16 mute_state[LAST_POWER_EVENT+1]; > ... >
It's not about being happy - I'm a happy man in general ;p
As stated already, declaring _LAST_ as the "new option" is misleading and not advised. And yeah, [_LAST_ + 1] is usually the one you should go with.
Czarek
| |