| Message ID | 20221115022423.6437-1-wangweidong.a@awinic.com |
|---|---|
| Headers | show |
| Series | ASoC: codecs: Add Awinic AW883XX audio amplifier driver | expand |
On 15/11/2022 03:24, wangweidong.a@awinic.com wrote: > From: Weidong Wang <wangweidong.a@awinic.com> > > Add a DT schema for describing Awinic AW883xx audio amplifiers. They are > controlled using I2C. > > Signed-off-by: Weidong Wang <wangweidong.a@awinic.com> > --- > .../bindings/sound/awinic,aw883xx.yaml | 62 +++++++++++++++++++ > 1 file changed, 62 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/awinic,aw883xx.yaml > > diff --git a/Documentation/devicetree/bindings/sound/awinic,aw883xx.yaml b/Documentation/devicetree/bindings/sound/awinic,aw883xx.yaml > new file mode 100644 > index 000000000000..04cdcf25a6d4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/awinic,aw883xx.yaml > @@ -0,0 +1,62 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/awinic,aw883xx.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Awinic AW883xx Smart Audio Amplifier > + > +maintainers: > + - Stephan Gerhold <stephan@gerhold.net> > + > +description: > + The Awinic AW883XX is an I2S/TDM input, high efficiency > + digital Smart K audio amplifier with an integrated 10.25V > + smart boost convert. > + > +allOf: > + - $ref: name-prefix.yaml# > + > +properties: > + compatible: > + const: awinic,aw883xx_smartpa > + > + reg: > + description: > + The I2C address of the device for I2C What happened here? This was not present before. Drop description. > + maxItems: 1 > + > + reset-gpios: > + description: > + Reset pin of aw883xx chip The same case. Drop description. I commented only for irq-gpios that they need description. > + maxItems: 1 > + > + sound-channel: > + description: > + Number of sound channels of the aw883xx chip Your description does not explain me much. Number of supported sound channels is usually fixed in the hardware, thus coming from compatible. Therefore this might mean something else... but anyway your driver does not use it really, so just drop it. Best regards, Krzysztof
On Tue, Nov 15, 2022 at 10:24:18AM +0800, wangweidong.a@awinic.com wrote: > +/* > + * aw883xx distinguish between codecs and components by version > + */ > +static struct aw_componet_codec_ops aw_componet_codec_ops = { > + .kcontrol_codec = snd_soc_kcontrol_component, > + .codec_get_drvdata = snd_soc_component_get_drvdata, > + .add_codec_controls = snd_soc_add_component_controls, > + .unregister_codec = snd_soc_unregister_component, > + .register_codec = snd_soc_register_component, > +}; I've started looking at this a few times but keep stopping due to this bit. CODECs and components (note the spelling BTW) are both ASoC level concepts but this looks like the driver is trying to define it's own abstraction using the same terms. There's nothing in the commit log or anything that explains this so it's a bit of work to try to figure out what's going on which makes it hard to follow. It would really help to have some explanation of what's going on rather than having to reverse engineer it from the patches. > + mutex_lock(&aw883xx->dsp_lock); > + if (data_type == AW_DSP_16_DATA) { > + } else if (data_type == AW_DSP_32_DATA) { > + } else { > + } This looks like it should be written as a switch statement. > + if ((ucontrol->value.integer.value[0] > FADE_TIME_MAX) || > + (ucontrol->value.integer.value[0] < FADE_TIME_MIN)) { > + pr_debug("set val %ld overflow %d or less than :%d", > + ucontrol->value.integer.value[0], FADE_TIME_MAX, FADE_TIME_MAX); > + return 0; > + } Invalid values should report an error. > + pr_debug("step time %ld", ucontrol->value.integer.value[0]); Use dev_ prints where possible. > + if (!aw883xx->dbg_en_prof) { > + dev_info(codec->dev, "profile close"); > + return 0; > + } This should be a debug print at most. > + /* check value valid */ > + ret = aw883xx_dev_check_profile_index(aw883xx->aw_pa, ucontrol->value.integer.value[0]); > + if (ret) { > + dev_warn(codec->dev, "unsupported index %ld", > + ucontrol->value.integer.value[0]); > + return 0; > + } No error messages from control sets, an application could make a lot of noise in the logs. > +static int aw883xx_volume_set(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + vol_desc->ctl_volume = value; > + > + /*get smaller dB*/ > + compared_vol = AW_GET_MAX_VALUE(vol_desc->ctl_volume, > + vol_desc->monitor_volume); > + > + aw883xx_dev_set_volume(aw883xx->aw_pa, compared_vol); Why is there this extra soft limit on volume? This looks confusing. > +static void aw883xx_fw_wrok(struct work_struct *work) > +{ wrok? > +static int aw883xx_gpio_request(struct aw883xx *aw883xx) > +{ > + int ret = 0; > + > + if (gpio_is_valid(aw883xx->reset_gpio)) { > + ret = devm_gpio_request_one(aw883xx->dev, aw883xx->reset_gpio, > + GPIOF_OUT_INIT_LOW, "aw883xx_rst"); > + if (ret) { > + dev_err(aw883xx->dev, "rst request failed"); > + return ret; > + } > + } > + > + return 0; > +} Use gpiod_ APIs please for new code, the numeric GPIO API is being phased out. > +/* > + * sys group attribute: reg > + */ > +static ssize_t reg_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct aw883xx *aw883xx = dev_get_drvdata(dev); > + int reg_num = aw883xx->aw_pa->ops.aw_get_reg_num(); > + ssize_t len = 0; > + uint8_t i = 0; > + unsigned int reg_val = 0; > + > + for (i = 0; i < reg_num; i++) { > + if (aw883xx->aw_pa->ops.aw_check_rd_access(i)) { > + regmap_read(aw883xx->regmap, i, ®_val); > + len += snprintf(buf + len, PAGE_SIZE - len, > + "reg:0x%02x=0x%04x\n", i, reg_val); > + } > + } > + > + return len; > +} regmap already provides a debugfs interface for you. > +static ssize_t reg_store(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t count) > +{ > + struct aw883xx *aw883xx = dev_get_drvdata(dev); > + unsigned int databuf[2] = { 0 }; > + > + if (sscanf(buf, "%x %x", &databuf[0], &databuf[1]) == 2) > + regmap_write(aw883xx->regmap, databuf[0], databuf[1]); > + > + return count; > +} It's not OK to provide a raw register write interface to userspace, this allows userspace to just go around the back of the driver and do whatever which makes it impossible to guarantee that the state of the hardware matches what the driver thinks is going on. Needed functionality should go via some abstracted kernel interface. For debug use there is a regmap interface which can do register writes for debug purposes if the kernel is specially built for it. Just remove all this debugfs code. > +static ssize_t fade_step_store(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t count) > +{ Controls should go through the ALSA APIs, not sysfs.
From: Weidong Wang <wangweidong.a@awinic.com> The Awinic AW883XX is an I2S/TDM input, high efficiency digital Smart K audio amplifier with an integrated 10.25V smart boost convert Add a DT schema for describing Awinic AW883xx audio amplifiers. They are controlled using I2C. v3 -> v4: Specification awinic,aw883xx.yaml pin naming Add a description of the awinic,aw883xx.yaml file properties Change the name of the node in the awinic,aw883xx.yaml file Change the warning: this 'if' clause does not guard on line 1095 of aw883xx.c Change the error: initialization of 'void (*)(struct i2c_client *)' from incompatible pointer type 'int (*)(struct i2c_client *)' on line 1796 of aw883xx.c Change the warning: 'aw_pid_2049_reg_access' defined but not used on line 37 of aw883xx_init.c Change the warning: no previous prototype for 'aw883xx_dev_get_int_status' on line 634 of aw883xx_device.c Change the warning: no previous prototype for 'aw_dev_get_ra' on line 1000 of aw883xx_device.c Change the warning: no previous prototype for 'aw_dev_dsp_fw_update' on line 1062 of aw883xx_device.c Weidong Wang (6): ASoC: codecs: Add i2c and codec registration for aw883xx and their associated operation functions ASoC: codecs: Added configuration file parsing for aw883xx ASoC: codecs: Add aw883xx chip control logic, such as power-on and power-off ASoC: codecs: Realize aw883xx register configuration and register address file ASoC: dt-bindings: Add schema for "awinic,aw883xx" ASoC:codecs:aw883xx corresponds to the modified Makefile and Kconfig .../bindings/sound/awinic,aw883xx.yaml | 62 + sound/soc/codecs/Kconfig | 10 + sound/soc/codecs/Makefile | 7 + sound/soc/codecs/aw883xx/aw883xx.c | 1803 +++++++++++++ sound/soc/codecs/aw883xx/aw883xx.h | 155 ++ sound/soc/codecs/aw883xx/aw883xx_bin_parse.c | 1294 ++++++++++ sound/soc/codecs/aw883xx/aw883xx_bin_parse.h | 145 ++ sound/soc/codecs/aw883xx/aw883xx_data_type.h | 148 ++ sound/soc/codecs/aw883xx/aw883xx_device.c | 1618 ++++++++++++ sound/soc/codecs/aw883xx/aw883xx_device.h | 544 ++++ sound/soc/codecs/aw883xx/aw883xx_init.c | 635 +++++ .../soc/codecs/aw883xx/aw883xx_pid_2049_reg.h | 2300 +++++++++++++++++ 12 files changed, 8721 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/awinic,aw883xx.yaml create mode 100644 sound/soc/codecs/aw883xx/aw883xx.c create mode 100644 sound/soc/codecs/aw883xx/aw883xx.h create mode 100644 sound/soc/codecs/aw883xx/aw883xx_bin_parse.c create mode 100644 sound/soc/codecs/aw883xx/aw883xx_bin_parse.h create mode 100644 sound/soc/codecs/aw883xx/aw883xx_data_type.h create mode 100644 sound/soc/codecs/aw883xx/aw883xx_device.c create mode 100644 sound/soc/codecs/aw883xx/aw883xx_device.h create mode 100644 sound/soc/codecs/aw883xx/aw883xx_init.c create mode 100644 sound/soc/codecs/aw883xx/aw883xx_pid_2049_reg.h base-commit: 094226ad94f471a9f19e8f8e7140a09c2625abaa