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

[Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>]

On 6/11/2026 3:09 PM, Krzysztof Kozlowski wrote:
> 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.

Hi Krzysztof,

Thanks for reviewing and comments on patch.

Agree. The NULL check is unnecessary, and the helper does not add any 
meaningful abstraction.

I'll remove the wrapper and use the GPIO API directly in the next revision.

>
>> +		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?

No, I will remove all the unnecessary checks in the next version of patch.


>
>> +
>> +	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?
Ack, will use.
>
>> +	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.
Ack, Will update
>
>> +
>> +	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.
Ack, Will update
>
>> +	{}
>> +};
>> +
>> +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.

Agreed. I'll place each MODULE_DEVICE_TABLE() immediately after its 
associated table to match the existing kernel style.

Thanks,

Prasad


>
> Best regards,
> Krzysztof
>