| Message ID | 20191009085108.4950-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | ASoC: codecs: Add WSA881x Smart Speaker amplifier support | expand |
On Wed, Oct 09, 2019 at 09:51:08AM +0100, Srinivas Kandagatla wrote: > +static const u8 wsa881x_reg_readable[WSA881X_CACHE_SIZE] = { > +static bool wsa881x_readable_register(struct device *dev, unsigned int reg) > +{ > + return wsa881x_reg_readable[reg]; u There's no bounds check and that array size is not... > +static struct regmap_config wsa881x_regmap_config = { > + .reg_bits = 32, > + .val_bits = 8, > + .cache_type = REGCACHE_RBTREE, > + .reg_defaults = wsa881x_defaults, > + .num_reg_defaults = ARRAY_SIZE(wsa881x_defaults), > + .max_register = WSA881X_MAX_REGISTER, ...what regmap has as max_register. Uusually you'd render as a switch statement (as you did for volatile) and let the compiler figure out a sensible way to do the lookup. > +static void wsa881x_init(struct wsa881x_priv *wsa881x) > +{ > + struct regmap *rm = wsa881x->regmap; > + unsigned int val = 0; > + > + regmap_read(rm, WSA881X_CHIP_ID1, &wsa881x->version); > + regcache_cache_only(rm, true); > + regmap_multi_reg_write(rm, wsa881x_rev_2_0, > + ARRAY_SIZE(wsa881x_rev_2_0)); > + regcache_cache_only(rm, false); This looks broken, what is it supposed to be doing? It looks like it should be a register patch but it's not documented. > +static const struct snd_kcontrol_new wsa881x_snd_controls[] = { > + SOC_ENUM("Smart Boost Level", smart_boost_lvl_enum), > + WSA881X_PA_GAIN_TLV("PA Gain", WSA881X_SPKR_DRV_GAIN, > + 4, 0xC, 1, pa_gain), As covered in control-names.rst all volume controls should end in Volume. > +static void wsa881x_clk_ctrl(struct snd_soc_component *comp, bool enable) > +{ > + struct wsa881x_priv *wsa881x = snd_soc_component_get_drvdata(comp); > + > + mutex_lock(&wsa881x->res_lock); What is this lock supposed to be protecting? As far as I can tell this function is the only place it is used and this function has exactly one caller which itself has only one caller which is a DAPM widget and hence needs no locking. It looks awfully like it should just be a widget itself, or inlined into the single caller. > +static void wsa881x_bandgap_ctrl(struct snd_soc_component *comp, bool enable) > +{ > + struct wsa881x_priv *wsa881x = snd_soc_component_get_drvdata(comp); Similarly here. > +static int32_t wsa881x_resource_acquire(struct snd_soc_component *comp, > + bool enable) > +{ > + wsa881x_clk_ctrl(comp, enable); > + wsa881x_bandgap_ctrl(comp, enable); > + > + return 0; > +} There's no corresponding disables.
Thanks Mark for taking time to review this patch. On 09/10/2019 17:35, Mark Brown wrote: > On Wed, Oct 09, 2019 at 09:51:08AM +0100, Srinivas Kandagatla wrote: > >> +static const u8 wsa881x_reg_readable[WSA881X_CACHE_SIZE] = { > >> +static bool wsa881x_readable_register(struct device *dev, unsigned int reg) >> +{ >> + return wsa881x_reg_readable[reg]; > u > There's no bounds check and that array size is not... > I converted this now to a proper switch statement as other drivers do. >> +static struct regmap_config wsa881x_regmap_config = { >> + .reg_bits = 32, >> + .val_bits = 8, >> + .cache_type = REGCACHE_RBTREE, >> + .reg_defaults = wsa881x_defaults, >> + .num_reg_defaults = ARRAY_SIZE(wsa881x_defaults), >> + .max_register = WSA881X_MAX_REGISTER, > > ...what regmap has as max_register. Uusually you'd render as a > switch statement (as you did for volatile) and let the compiler > figure out a sensible way to do the lookup. Sorry, I did not get your point here. Are you saying that we can skip max_register in this regmap config ? Then how would max_register in regmap be set? > >> +static void wsa881x_init(struct wsa881x_priv *wsa881x) >> +{ >> + struct regmap *rm = wsa881x->regmap; >> + unsigned int val = 0; >> + >> + regmap_read(rm, WSA881X_CHIP_ID1, &wsa881x->version); >> + regcache_cache_only(rm, true); >> + regmap_multi_reg_write(rm, wsa881x_rev_2_0, >> + ARRAY_SIZE(wsa881x_rev_2_0)); >> + regcache_cache_only(rm, false); > > This looks broken, what is it supposed to be doing? It looks > like it should be a register patch but it's not documented. > Yep, it makes sense to move this to patch, its done in new version. >> +static const struct snd_kcontrol_new wsa881x_snd_controls[] = { >> + SOC_ENUM("Smart Boost Level", smart_boost_lvl_enum), >> + WSA881X_PA_GAIN_TLV("PA Gain", WSA881X_SPKR_DRV_GAIN, >> + 4, 0xC, 1, pa_gain), > > As covered in control-names.rst all volume controls should end in > Volume. > Fixed this in next version. >> +static void wsa881x_clk_ctrl(struct snd_soc_component *comp, bool enable) >> +{ >> + struct wsa881x_priv *wsa881x = snd_soc_component_get_drvdata(comp); >> + >> + mutex_lock(&wsa881x->res_lock); > > What is this lock supposed to be protecting? As far as I can > tell this function is the only place it is used and this function > has exactly one caller which itself has only one caller which is > a DAPM widget and hence needs no locking. It looks awfully like > it should just be a widget itself, or inlined into the single > caller. > This was done for temperature sensor reads which can happen in parallel. But for now I will remove it and add back once we add tsens support. >> +static void wsa881x_bandgap_ctrl(struct snd_soc_component *comp, bool enable) >> +{ >> + struct wsa881x_priv *wsa881x = snd_soc_component_get_drvdata(comp); > > Similarly here. > This one was over done! its now removed in next version. >> +static int32_t wsa881x_resource_acquire(struct snd_soc_component *comp, >> + bool enable) >> +{ >> + wsa881x_clk_ctrl(comp, enable); >> + wsa881x_bandgap_ctrl(comp, enable); >> + >> + return 0; >> +} > > There's no corresponding disables. both wsa881x_clk_ctrl() and wsa881x_bandgap_ctrl() have corresponding disables in that functions. thanks, srini >
On Thu, Oct 10, 2019 at 10:28:04AM +0100, Srinivas Kandagatla wrote: > On 09/10/2019 17:35, Mark Brown wrote: > > On Wed, Oct 09, 2019 at 09:51:08AM +0100, Srinivas Kandagatla wrote: > > > +static const u8 wsa881x_reg_readable[WSA881X_CACHE_SIZE] = { > > > +static bool wsa881x_readable_register(struct device *dev, unsigned int reg) > > > +{ > > > + return wsa881x_reg_readable[reg]; > > There's no bounds check and that array size is not... > I converted this now to a proper switch statement as other drivers do. > > > +static struct regmap_config wsa881x_regmap_config = { > > > + .reg_bits = 32, > > > + .val_bits = 8, > > > + .cache_type = REGCACHE_RBTREE, > > > + .reg_defaults = wsa881x_defaults, > > > + .num_reg_defaults = ARRAY_SIZE(wsa881x_defaults), > > > + .max_register = WSA881X_MAX_REGISTER, > > ...what regmap has as max_register. Uusually you'd render as a > > switch statement (as you did for volatile) and let the compiler > > figure out a sensible way to do the lookup. > Sorry, I did not get your point here. > Are you saying that we can skip max_register in this regmap config ? > Then how would max_register in regmap be set? I'm saying that you appear to be relying on max_register to verify that you're not overflowing the array bounds but you max_register is not set to the same thing as the array size.
On 10/10/2019 14:23, Mark Brown wrote: > On Thu, Oct 10, 2019 at 10:28:04AM +0100, Srinivas Kandagatla wrote: >> On 09/10/2019 17:35, Mark Brown wrote: >>> On Wed, Oct 09, 2019 at 09:51:08AM +0100, Srinivas Kandagatla wrote: >>>> +static const u8 wsa881x_reg_readable[WSA881X_CACHE_SIZE] = { > >>>> +static bool wsa881x_readable_register(struct device *dev, unsigned int reg) >>>> +{ >>>> + return wsa881x_reg_readable[reg]; > >>> There's no bounds check and that array size is not... > >> I converted this now to a proper switch statement as other drivers do. > >>>> +static struct regmap_config wsa881x_regmap_config = { >>>> + .reg_bits = 32, >>>> + .val_bits = 8, >>>> + .cache_type = REGCACHE_RBTREE, >>>> + .reg_defaults = wsa881x_defaults, >>>> + .num_reg_defaults = ARRAY_SIZE(wsa881x_defaults), >>>> + .max_register = WSA881X_MAX_REGISTER, > >>> ...what regmap has as max_register. Uusually you'd render as a >>> switch statement (as you did for volatile) and let the compiler >>> figure out a sensible way to do the lookup. > >> Sorry, I did not get your point here. > >> Are you saying that we can skip max_register in this regmap config ? >> Then how would max_register in regmap be set? > > I'm saying that you appear to be relying on max_register to > verify that you're not overflowing the array bounds but you > max_register is not set to the same thing as the array size. Thanks for clarification, I have removed the readable array in new version, removed multiple macros for max register and fixed max_register to point to last register. --srini >
This patchset adds support to WSA8810/WSA8815 Class-D Smart Speaker Amplifier which is SoundWire interfaced. This patchset along with DB845c machine driver and WCD934x codec driver has been tested on SDM845 SoC based DragonBoard DB845c with two WSA8810 and Lenovo YOGA C630 Laptop based on SDM850 with WSA8815 speaker amplifiers. Most of the code in this driver is rework of Qualcomm downstream drivers used in Andriod. Credits to Banajit Goswami and Patrick Lai's Team. TODO: Add thermal sensor support in WSA881x. Thanks, srini Changes since v6: - Added reviewed-by tag from Pierre and Rob - Removed udelay argument as suggested by Pierre. - Added device id for WSA8815 - rebased on sound-next Srinivas Kandagatla (2): dt-bindings: ASoC: Add WSA881x bindings ASoC: codecs: add wsa881x amplifier support .../bindings/sound/qcom,wsa881x.yaml | 62 + sound/soc/codecs/Kconfig | 10 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/wsa881x.c | 1113 +++++++++++++++++ 4 files changed, 1187 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/qcom,wsa881x.yaml create mode 100644 sound/soc/codecs/wsa881x.c -- 2.21.0