Message ID | 20200930163809.6978-1-dmurphy@ti.com |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | [1/2] dt-bindings: tas2764: Add the TAS2764 binding doc | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/dt-meta-schema | success |
On Wed, Sep 30, 2020 at 11:38:08AM -0500, Dan Murphy wrote: > + reset-gpio: > + description: GPIO used to reset the device. Even if only a single GPIO is allowed DT properties for GPIOs should be plural.
On Wed, Sep 30, 2020 at 11:38:09AM -0500, Dan Murphy wrote: This all looks good - a few very minor things below but nothing substantial: > + default: > + dev_err(tas2764->dev, "Not supported evevt\n"); > + return -EINVAL; evevt -> event > +static int tas2764_mute(struct snd_soc_dai *dai, int mute, int direction) > +{ > + struct snd_soc_component *component = dai->component; > + int ret = snd_soc_component_update_bits(component, TAS2764_PWR_CTRL, > + TAS2764_PWR_CTRL_MASK, > + mute ? TAS2764_PWR_CTRL_MUTE : 0); > + > + if (ret < 0) > + return ret; This looks weird with the ternery operator and extreme indentation - could you please at least split the declaration of ret from the call to make the line length a bit extreme? > + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { > + case SND_SOC_DAIFMT_I2S: > + case SND_SOC_DAIFMT_DSP_A: > + tdm_rx_start_slot = 1; > + break; > + case SND_SOC_DAIFMT_DSP_B: > + case SND_SOC_DAIFMT_LEFT_J: > + tdm_rx_start_slot = 0; > + break; I'm not seeing any other handling that distinguishes between the I2S and DSP modes anywhere - I'm guessing this is because the device is really only implementing the DSP modes but because it's mono this is compatible with the I2S modes? It'd be worth having a comment saying this since while that would be OK not distinguishing between modes properly is a common error in drivers so it'd help avoid cut'n'paste issues if someone uses this code as a reference. > +static int tas2764_register_codec(struct tas2764_priv *tas2764) > +{ > + return devm_snd_soc_register_component(tas2764->dev, > + &soc_component_driver_tas2764, > + tas2764_dai_driver, > + ARRAY_SIZE(tas2764_dai_driver)); > +} This is a bit odd - can we not just inline the component registration rather than having this function?
Mark Thanks for the review On 10/1/20 11:25 AM, Mark Brown wrote: > On Wed, Sep 30, 2020 at 11:38:09AM -0500, Dan Murphy wrote: > > This all looks good - a few very minor things below but nothing > substantial: > >> + default: >> + dev_err(tas2764->dev, "Not supported evevt\n"); >> + return -EINVAL; > evevt -> event OK > >> +static int tas2764_mute(struct snd_soc_dai *dai, int mute, int direction) >> +{ >> + struct snd_soc_component *component = dai->component; >> + int ret = snd_soc_component_update_bits(component, TAS2764_PWR_CTRL, >> + TAS2764_PWR_CTRL_MASK, >> + mute ? TAS2764_PWR_CTRL_MUTE : 0); >> + >> + if (ret < 0) >> + return ret; > This looks weird with the ternery operator and extreme indentation - > could you please at least split the declaration of ret from the call to > make the line length a bit extreme? I will fix it up >> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { >> + case SND_SOC_DAIFMT_I2S: >> + case SND_SOC_DAIFMT_DSP_A: >> + tdm_rx_start_slot = 1; >> + break; >> + case SND_SOC_DAIFMT_DSP_B: >> + case SND_SOC_DAIFMT_LEFT_J: >> + tdm_rx_start_slot = 0; >> + break; > I'm not seeing any other handling that distinguishes between the I2S and > DSP modes anywhere - I'm guessing this is because the device is really > only implementing the DSP modes but because it's mono this is compatible > with the I2S modes? It'd be worth having a comment saying this since > while that would be OK not distinguishing between modes properly is a > common error in drivers so it'd help avoid cut'n'paste issues if someone > uses this code as a reference. Ah it does do LEFT J and Right J so I will fix this >> +static int tas2764_register_codec(struct tas2764_priv *tas2764) >> +{ >> + return devm_snd_soc_register_component(tas2764->dev, >> + &soc_component_driver_tas2764, >> + tas2764_dai_driver, >> + ARRAY_SIZE(tas2764_dai_driver)); >> +} > This is a bit odd - can we not just inline the component registration > rather than having this function? I will eliminate this completely and move to i2c_probe Dan
diff --git a/Documentation/devicetree/bindings/sound/tas2764.yaml b/Documentation/devicetree/bindings/sound/tas2764.yaml new file mode 100644 index 000000000000..4e758ff81003 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/tas2764.yaml @@ -0,0 +1,74 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (C) 2020 Texas Instruments Incorporated +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/sound/tas2764.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Texas Instruments TAS2764 Smart PA + +maintainers: + - Dan Murphy <dmurphy@ti.com> + +description: | + The TAS2764 is a mono, digital input Class-D audio amplifier optimized for + efficiently driving high peak power into small loudspeakers. + Integrated speaker voltage and current sense provides for + real time monitoring of loudspeaker behavior. + +properties: + compatible: + enum: + - ti,tas2764 + + reg: + maxItems: 1 + description: | + I2C address of the device can be between 0x38 to 0x45. + + reset-gpio: + description: GPIO used to reset the device. + + shutdown-gpios: + description: GPIO used to control the state of the device. + + interrupts: + maxItems: 1 + + ti,imon-slot-no: + $ref: /schemas/types.yaml#/definitions/uint32 + description: TDM TX current sense time slot. + + ti,vmon-slot-no: + $ref: /schemas/types.yaml#/definitions/uint32 + description: TDM TX voltage sense time slot. + + '#sound-dai-cells': + const: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + i2c0 { + #address-cells = <1>; + #size-cells = <0>; + codec: codec@38 { + compatible = "ti,tas2764"; + reg = <0x38>; + #sound-dai-cells = <1>; + interrupt-parent = <&gpio1>; + interrupts = <14>; + reset-gpio = <&gpio1 15 0>; + shutdown-gpios = <&gpio1 15 0>; + ti,imon-slot-no = <0>; + ti,vmon-slot-no = <2>; + }; + }; + +...
Add the binding for the TAS2764 Smart Amplifier. Signed-off-by: Dan Murphy <dmurphy@ti.com> --- .../devicetree/bindings/sound/tas2764.yaml | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tas2764.yaml