Re: [PATCH 1/2] ASoC: codecs: twl4030: fix external mute GPIO lookup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ASoC: twl4030: broken ti,hs_extmute_gpio lookup in twl4030_get_board_params()
--

commit 0efa5663d0f46476c9f10223a843a170f4992c82
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

ASoC: codecs: twl4030: fix external mute GPIO lookup

This commit updates twl4030_get_board_params() to read properties from the
parent device's "codec" child fwnode and request the external mute gpio via
devm_fwnode_gpiod_get_optional().

> diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
> index 7117af5a1a569..9541c8a5a24e8 100644
> --- a/sound/soc/codecs/twl4030.c
> +++ b/sound/soc/codecs/twl4030.c

[ ... ]

> @@ -198,41 +199,53 @@ static void twl4030_codec_enable(struct snd_soc_component *component, int enable
>  	udelay(10);
>  }
>  
> -static void
> -twl4030_get_board_param_values(struct twl4030_board_params *board_params,
> -			       struct device_node *node)
> +static struct twl4030_board_params *twl4030_get_board_params(struct device *dev)
>  {
> -	of_property_read_u32(node, "ti,digimic_delay", &board_params->digimic_delay);
> -	of_property_read_u32(node, "ti,ramp_delay_value", &board_params->ramp_delay_value);
> -	of_property_read_u32(node, "ti,offset_cncl_path", &board_params->offset_cncl_path);
> -	board_params->hs_extmute = of_property_read_bool(node, "ti,hs_extmute");
> +	struct twl4030_board_params *params;
> +	int error;
>  
> -	if (of_property_present(node, "ti,hs_extmute_gpio"))
> -		board_params->hs_extmute = 1;
> -}
> +	struct fwnode_handle *codec_node __free(fwnode_handle) =
> +		device_get_named_child_node(dev->parent, "codec");
> +	if (!codec_node)
> +		return NULL;
>  
> -static struct twl4030_board_params*
> -twl4030_get_board_params(struct device *dev)
> -{
> -	struct twl4030_board_params *board_params = NULL;
> -	struct device_node *twl4030_codec_node = NULL;
> -
> -	twl4030_codec_node = of_get_child_by_name(dev->parent->of_node,
> -						  "codec");
> -
> -	if (twl4030_codec_node) {
> -		board_params = devm_kzalloc(dev,
> -					    sizeof(struct twl4030_board_params),
> -					    GFP_KERNEL);
> -		if (!board_params) {
> -			of_node_put(twl4030_codec_node);
> -			return ERR_PTR(-ENOMEM);
> -		}
> -		twl4030_get_board_param_values(board_params, twl4030_codec_node);
> -		of_node_put(twl4030_codec_node);
> +	params = devm_kzalloc(dev, sizeof(*params), GFP_KERNEL);
> +	if (!params)
> +		return ERR_PTR(-ENOMEM);
> +
> +	fwnode_property_read_u32(codec_node, "ti,digimic_delay",
> +				 &params->digimic_delay);
> +	fwnode_property_read_u32(codec_node, "ti,ramp_delay_value",
> +				 &params->ramp_delay_value);
> +	fwnode_property_read_u32(codec_node, "ti,offset_cncl_path",
> +				 &params->offset_cncl_path);
> +
> +	/*
> +	 * Headset external mute can be controlled either via TWL4030's
> +	 * dedicated GPIO6/EXTMUTE pin (enabled by the boolean "ti,hs_extmute"
> +	 * property) or via a separate GPIO line ("ti,hs_extmute-gpios" or
> +	 * legacy "ti,hs_extmute_gpio"), which implies external mute is active.
> +	 */
> +	params->hs_extmute = fwnode_property_read_bool(codec_node, "ti,hs_extmute");
> +
> +	/*
> +	 * This will attempt to parse "ti,hs_extmute-gpios" or
> +	 * "ti,hs_extmute_gpio".
> +	 */
> +	params->hs_extmute_gpio = devm_fwnode_gpiod_get_optional(dev, codec_node,
> +								 "ti,hs_extmute",
> +								 GPIOD_OUT_LOW,
> +								 "hs_extmute");
> +	error = PTR_ERR_OR_ZERO(params->hs_extmute_gpio);
> +	if (error) {
> +		dev_err_probe(dev, error, "Failed to get hs_extmute GPIO\n");
> +		return ERR_PTR(error);
> +	}
>  
> -	return board_params;
> +	if (params->hs_extmute_gpio)
> +		params->hs_extmute = 1;

[Severity: High]
Will devm_fwnode_gpiod_get_optional() actually match the legacy
"ti,hs_extmute_gpio" property (with an underscore) when called with
"ti,hs_extmute" as con_id in twl4030_get_board_params()?

In drivers/gpio/gpiolib.h, for_each_gpio_property_name() constructs candidate
property names using a hyphen:

	if (con_id)
		snprintf(propname, sizeof(propname), "%s-%s", con_id, __gs);

When of_find_gpio() in drivers/gpio/gpiolib-of.c iterates over gpio_suffixes
("gpios" and "gpio"), it only checks "ti,hs_extmute-gpios" and
"ti,hs_extmute-gpio", and of_find_gpio_rename() has no fallback entry for
"ti,hs_extmute_gpio".

However, Documentation/devicetree/bindings/sound/ti,twl4030-audio.yaml and
arch/arm/boot/dts/ti/omap/logicpd-som-lv.dtsi specify "ti,hs_extmute_gpio"
with an underscore:

	twl_audio: audio {
		compatible = "ti,twl4030-audio";
		codec {
			ti,hs_extmute_gpio = <&gpio2 25 GPIO_ACTIVE_HIGH>;
		};
	};

When probing a board whose "codec" node uses "ti,hs_extmute_gpio", won't
devm_fwnode_gpiod_get_optional() return NULL, leaving params->hs_extmute_gpio
unset (so params->hs_extmute stays 0 on boards like logicpd-som-lv.dtsi, or
twl4030_get_hw_params() configures the internal pin mux in TWL4030_PMBR1_REG
if "ti,hs_extmute" is also present)?

> +
> +	return params;
>  }

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-twl4030-extmute-v1-0-3d2dbe260df9@gmail.com?part=1