Re: [PATCH v2 2/2] ASoC: cs530x: Support for cs530x ADCs

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On 14/06/2024 20:36, Paul Handrigan wrote:
> Add support for the cs530x family of high performance
> ADCs.
> 
> Signed-off-by: Paul Handrigan <paulha@xxxxxxxxxxxxxxxxxxxxx>

...

> +
> +	dev_dbg(dev, "Device ID 0x%x\n", dev_id);
> +
> +	ret = regmap_read(cs530x->regmap, CS530X_REVID, &rev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Can't read REV ID\n");
> +
> +	switch (dev_id) {
> +	case CS530X_2CH_ADC_DEV_ID:
> +		cs530x->num_adcs = 2;
> +		break;
> +	case CS530X_4CH_ADC_DEV_ID:
> +		cs530x->num_adcs = 4;
> +		break;
> +	case CS530X_8CH_ADC_DEV_ID:
> +		cs530x->num_adcs = 8;
> +		break;
> +	default:
> +		return dev_err_probe(dev, -EINVAL, "Invalid device ID 0x%x\n",
> +				     dev_id);
> +	}
> +
> +	dev_info(dev, "CS5308 %d Channel Audio ADC Rev 0x%x\n",
> +		 cs530x->num_adcs, rev & 0xff);

dev_dbg or just drop. You already have dev_dbg with rev before.

> +
> +	return 0;
> +}
> +
> +static int cs530x_parse_device_properties(struct cs530x_priv *cs530x)
> +{
> +	struct regmap *regmap = cs530x->regmap;
> +	struct device *dev = cs530x->dev;
> +	unsigned int val = 0;
> +
> +	switch (cs530x->num_adcs) {
> +	case 8:
> +		if (device_property_read_bool(dev, "cirrus,in-hiz-pin78"))
> +			val = CS530X_IN78_HIZ;
> +
> +		if (device_property_read_bool(dev, "cirrus,in-hiz-pin56"))
> +			val |= CS530X_IN56_HIZ;
> +
> +		fallthrough;
> +	case 4:
> +		if (device_property_read_bool(dev, "cirrus,in-hiz-pin34"))
> +			val |= CS530X_IN34_HIZ;
> +
> +		fallthrough;
> +	case 2:
> +		if (device_property_read_bool(dev, "cirrus,in-hiz-pin12"))
> +			val |= CS530X_IN12_HIZ;
> +
> +		return regmap_set_bits(regmap, CS530X_IN_HIZ, val);
> +	default:
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid number of adcs %d\n",
> +				     cs530x->num_adcs);
> +	}
> +}
> +
> +int cs530x_probe(struct cs530x_priv *cs530x)
> +{
> +	struct device *dev = cs530x->dev;
> +	int ret, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(cs530x->supplies); i++)
> +		cs530x->supplies[i].supply = cs530x_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(cs530x->supplies),
> +				      cs530x->supplies);
> +	if (ret != 0)
> +		return dev_err_probe(dev, ret, "Failed to request supplies");
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(cs530x->supplies),
> +				    cs530x->supplies);
> +	if (ret != 0)
> +		return dev_err_probe(dev, ret, "Failed to enable supplies");
> +
> +	cs530x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						      GPIOD_OUT_HIGH);
> +	if (IS_ERR(cs530x->reset_gpio)) {
> +		ret = PTR_ERR(cs530x->reset_gpio);
> +		dev_err_probe(dev, ret, "Reset gpio not available\n");

My previous comments about syntax being return dev_err_probe() should
lead you to understand how dev_err_probe works.

ret = dev_err_probe()


> +		goto err_regulator;
> +	}
> +
> +	if (cs530x->reset_gpio) {
> +		usleep_range(2000, 2100);
> +		gpiod_set_value_cansleep(cs530x->reset_gpio, 0);
> +	}
> +
> +	usleep_range(5000, 5100);
> +	ret = cs530x_check_device_id(cs530x);
> +	if (ret)
> +		goto err_reset;
> +
> +	if (!cs530x->reset_gpio) {
> +		ret = regmap_write(cs530x->regmap, CS530X_SW_RESET,
> +				   CS530X_SW_RST_VAL);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "Soft Reset Failed\n");
> +			goto err_reset;
> +		}
> +	}
> +
> +	ret = cs530x_parse_device_properties(cs530x);
> +	if (ret)
> +		goto err_reset;
> +
> +	cs530x->dev_dai = devm_kmemdup(dev, &cs530x_dai,
> +					sizeof(*(cs530x->dev_dai)),
> +					GFP_KERNEL);
> +	if (!cs530x->dev_dai) {
> +		ret = -ENOMEM;
> +		goto err_reset;
> +	}

Allocations are usually placed at beginning of probe(). This simplifies
error paths.

> +
> +	cs530x->dev_dai->capture.channels_max = cs530x->num_adcs;
> +
> +	ret = devm_snd_soc_register_component(dev,
> +			&soc_component_dev_cs530x, cs530x->dev_dai, 1);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Can't register cs530x component\n");
> +		goto err_reset;
> +	}
> +
> +	return 0;
> +
> +err_reset:
> +	gpiod_set_value_cansleep(cs530x->reset_gpio, 1);
> +
> +err_regulator:
> +	regulator_bulk_disable(ARRAY_SIZE(cs530x->supplies),
> +			       cs530x->supplies);
> +
> +	return ret;
> +}

Best regards,
Krzysztof