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
