On 27/02/2024 11:23, Alexandre Mergnat wrote:
>>> +
>>> +examples:
>>> + - |
>>> + sound {
>>> + compatible = "mediatek,mt8365-mt6357";
>>> + mediatek,platform = <&afe>;
>>
>> Please:
>>
>> https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
>
> Is it about the wrong pinctrl-names tab alignment ?
> Also, 2ND I2S BE => 2ND_I2S_BE ?
> Otherwise, I don't get it sorry.
Alignment of continued lines, order of properties.
Best regards,
Krzysztof
On 26/02/2024 15:01, Alexandre Mergnat wrote:
> Add Digital Micro Device Audio Interface support for MT8365 SoC.
>
> Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com>
> +
> +static int init_dmic_priv_data(struct mtk_base_afe *afe)
> +{
> + struct mt8365_afe_private *afe_priv = afe->platform_priv;
> + struct mt8365_dmic_data *dmic_priv;
> + struct device_node *np = afe->dev->of_node;
> + unsigned int temps[4];
> + int ret;
> +
> + dmic_priv = devm_kzalloc(afe->dev, sizeof(struct mt8365_dmic_data),
> + GFP_KERNEL);
You have very inconsistent style of coding. Some patches are done
correctly, some repeast known issues. All over. This is sizeof(*). This
comment (and all others) apply everywhere, just in case.
Best regards,
Krzysztof
On 26/02/2024 15:01, Alexandre Mergnat wrote:
> Add the codec property along with the mt6357.c codec driver support.
Describe the hardware, not the Linux drivers. There is no codec driver
support in the bindings.
https://elixir.bootlin.com/linux/v6.8-rc6/source/Documentation/devicetree/b…
>
> Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com>
> ---
> Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> index 37423c2e0fdf..d25a78070744 100644
> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> @@ -37,6 +37,17 @@ properties:
> "#interrupt-cells":
> const: 2
>
> + codec:
> + type: object
> + unevaluatedProperties: false
> + description:
> + MT6357 sound codec.
> + properties:
> + compatible:
> + const: mediatek,mt6357-sound
> + required:
> + - compatible
No resources? Then no need for this node.
We have it even documented (if my repeating every time is not enough)...
https://elixir.bootlin.com/linux/v6.8-rc6/source/Documentation/devicetree/b…
Best regards,
Krzysztof
> +static struct sk_buff *am65_cpsw_alloc_skb(struct net_device *ndev, unsigned int len)
> +{
> + struct page *page;
> + struct sk_buff *skb;
> +
> + page = dev_alloc_pages(0);
You are likely to get better performance if you use the page_pool.
When FEC added XDP support, the first set of changes was to make use
of page_pool. That improved the drivers performance. Then XDP was
added on top. Maybe you can follow that pattern.
Andrew
On Mon, Feb 26, 2024 at 03:01:50PM +0100, amergnat(a)baylibre.com wrote:
> index 000000000000..13e95c227114
> --- /dev/null
> +++ b/sound/soc/codecs/mt6357.c
> @@ -0,0 +1,1805 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MT6357 ALSA SoC audio codec driver
> + *
Please use a C++ comment for the whole comment to make it clearer that
this is intentional.
> +static void set_playback_gpio(struct mt6357_priv *priv, bool enable)
> +{
> + if (enable) {
> + /* set gpio mosi mode */
> + regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR, GPIO_MODE2_CLEAR_ALL);
> + regmap_write(priv->regmap, MT6357_GPIO_MODE2_SET, GPIO8_MODE_SET_AUD_CLK_MOSI |
> + GPIO9_MODE_SET_AUD_DAT_MOSI0 |
> + GPIO10_MODE_SET_AUD_DAT_MOSI1 |
> + GPIO11_MODE_SET_AUD_SYNC_MOSI);
This would be a lot more legible if you worked out the values to set and
then had a single set of writes, currently the indentation makes it very
hard to read. Similarly for other similar functions.
> +static int mt6357_put_volsw(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> + struct mt6357_priv *priv = snd_soc_component_get_drvdata(component);
> + struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value;
> + unsigned int reg;
> + int ret;
> +
> + ret = snd_soc_put_volsw(kcontrol, ucontrol);
> + if (ret < 0)
> + return ret;
> +
> + switch (mc->reg) {
> + case MT6357_ZCD_CON2:
> + regmap_read(priv->regmap, MT6357_ZCD_CON2, ®);
> + priv->ana_gain[ANALOG_VOLUME_HPOUTL] =
> + (reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT;
> + priv->ana_gain[ANALOG_VOLUME_HPOUTR] =
> + (reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT;
> + break;
It would probably be less code and would definitely be clearer and
simpler to just read the values when we need them rather than constatly
keeping a cache separate to the register cache.
> + /* ul channel swap */
> + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0),
On/off controls should end in Switch.
> +static const char * const hslo_mux_map[] = {
> + "Open", "DACR", "Playback", "Test mode"
> +};
> +
> +static int hslo_mux_map_value[] = {
> + 0x0, 0x1, 0x2, 0x3,
> +};
Why not just use a normal mux here, there's no missing values or
reordering? Similarly for other muxes.
> +static unsigned int mt6357_read(struct snd_soc_component *codec, unsigned int reg)
> +{
> + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec);
> + unsigned int val;
> +
> + pr_debug("%s() reg = 0x%x", __func__, reg);
> + regmap_read(priv->regmap, reg, &val);
> + return val;
> +}
Remove these, there are vastly more logging facilities as standard in
the regmap core.
> +/* Reg bit defines */
> +/* MT6357_GPIO_DIR0 */
> +#define GPIO8_DIR_MASK BIT(8)
> +#define GPIO8_DIR_INPUT 0
Please namespace your defines, these look very generic.