Re: [PATCH v1 2/2] ASoC: codecs: add Qualcomm WSA885X I2C codec driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On Wed, Jun 10, 2026 at 09:27:08PM +0530, Prasad Kumpatla wrote:
> +};
> +
> +static void wsa885x_gpio_set(struct wsa885x_i2c_priv *wsa885x, bool val)
> +{
> +	if (!wsa885x || !wsa885x->sd_n)

How wsa885x can be NULL?

This wrapper is pointless. Avoid creating abstraction layers over single
call to standard kernel interfaces.

> +		return;
> +
> +	gpiod_set_value_cansleep(wsa885x->sd_n, val);
> +}
> +

...

> +
> +static void wsa885x_gpio_powerdown(void *data)
> +{
> +	struct wsa885x_i2c_priv *wsa885x = data;
> +
> +	if (!wsa885x)
> +		return;

How is this possible?

> +
> +	wsa885x_gpio_set(wsa885x, true);
> +}
> +

...

> +	if (count > 0) {
> +		if (count % 2) {
> +			dev_err(dev, "%s: Invalid number of elements in %s (%d)\n",
> +				__func__, init_table_prop, count);
> +			return -EINVAL;
> +		}
> +		if (count > WSA885X_INIT_TABLE_MAX_ITEMS) {
> +			dev_err(dev, "%s: %s has too many elements (%d > %u)\n",
> +				__func__, init_table_prop, count,
> +				WSA885X_INIT_TABLE_MAX_ITEMS);
> +			return -EINVAL;
> +		}
> +		wsa885x->init_table_size = count;
> +
> +		wsa885x->init_table = devm_kcalloc(dev, wsa885x->init_table_size,
> +						   sizeof(*wsa885x->init_table), GFP_KERNEL);
> +		if (!wsa885x->init_table)
> +			return -ENOMEM;
> +
> +		if (device_property_read_u32_array(dev, init_table_prop,
> +						   wsa885x->init_table,
> +						   wsa885x->init_table_size)) {
> +			dev_err(dev, "%s: Failed to read %s\n",
> +				__func__, init_table_prop);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	ret = device_property_read_u32(dev, "qcom,battery-config",
> +				       &wsa885x->batt_conf);
> +	if (ret) {
> +		wsa885x->batt_conf = WSA885X_BATT_1S;
> +	} else if (wsa885x->batt_conf != WSA885X_BATT_1S &&
> +		   wsa885x->batt_conf != WSA885X_BATT_2S) {
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid battery config %u (expected 1S or 2S)\n",
> +				     wsa885x->batt_conf);
> +	}
> +
> +	for (i = 0; i < WSA885X_SUPPLIES_NUM; i++)
> +		wsa885x->supplies[i].supply = wsa885x_supply_name[i];
> +
> +	ret = devm_regulator_bulk_get(dev, WSA885X_SUPPLIES_NUM, wsa885x->supplies);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulators\n");
> +
> +	ret = regulator_bulk_enable(WSA885X_SUPPLIES_NUM, wsa885x->supplies);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> +	ret = devm_add_action_or_reset(dev, wsa885x_regulator_disable, wsa885x);

Why you cannot simply use devm_regulator_get_enable?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "devm_add_action_or_reset failed\n");
> +
> +	wsa885x->sd_n = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH);
> +	if (IS_ERR(wsa885x->sd_n))
> +		return dev_err_probe(dev, PTR_ERR(wsa885x->sd_n),
> +							 "Shutdown Control GPIO not found\n");

Messed/misaligned indentation.

> +
> +	wsa885x_gpio_set(wsa885x, false);
> +
> +	ret = devm_add_action_or_reset(dev, wsa885x_gpio_powerdown, wsa885x);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "devm_add_action_or_reset failed\n");
> +
> +	i2c_set_clientdata(client, wsa885x);
> +
> +	wsa885x->intr_pin = devm_gpiod_get(dev, "interrupt", GPIOD_IN);
> +	if (IS_ERR(wsa885x->intr_pin))
> +		return dev_err_probe(dev, PTR_ERR(wsa885x->intr_pin),
> +							 "Interrupt GPIO not found\n");
> +
> +	ret = wsa885x_register_irq(wsa885x);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "wsa885x irq registration failed\n");
> +
> +	ret = devm_snd_soc_register_component(dev, component_driver,
> +					      wsa885x_i2c_dai,
> +					      ARRAY_SIZE(wsa885x_i2c_dai));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Codec component registration failed\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id wsa885x_i2c_dt_match[] = {
> +	{
> +		.compatible = "qcom,wsa885x-i2c",
> +	},
> +	{}
> +};
> +
> +static const struct i2c_device_id wsa885x_id_i2c[] = {
> +	{"wsa885x_i2c", 0},

Used named initializers.

> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, wsa885x_id_i2c);
> +MODULE_DEVICE_TABLE(of, wsa885x_i2c_dt_match);

Don't come with own coding style. Each above goes IMMEDIATELY after the table.

Best regards,
Krzysztof