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

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

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

>  sound/soc/codecs/Kconfig      |  14 +
>  sound/soc/codecs/Makefile     |   4 +
>  sound/soc/codecs/cs530x-i2c.c |  89 +++
>  sound/soc/codecs/cs530x.c     | 991 ++++++++++++++++++++++++++++++++++
>  sound/soc/codecs/cs530x.h     | 223 ++++++++
>  5 files changed, 1321 insertions(+)
>  create mode 100644 sound/soc/codecs/cs530x-i2c.c
>  create mode 100644 sound/soc/codecs/cs530x.c
>  create mode 100644 sound/soc/codecs/cs530x.h
> 
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 3621781d63c4..8745ba193595 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -99,6 +99,7 @@ config SND_SOC_ALL_CODECS
>  	imply SND_SOC_CS47L90
>  	imply SND_SOC_CS47L92
>  	imply SND_SOC_CS53L30
> +	imply SND_SOC_CS530X_I2C
>  	imply SND_SOC_CX20442
>  	imply SND_SOC_CX2072X
>  	imply SND_SOC_DA7210
> @@ -998,6 +999,19 @@ config SND_SOC_CS53L30
>  	tristate "Cirrus Logic CS53L30 CODEC"
>  	depends on I2C
>  
> +config SND_SOC_CS530X
> +	tristate
> +
> +config SND_SOC_CS530X_I2C
> +	tristate "Cirrus Logic CS530x ADCs (I2C)"
> +	depends on I2C
> +	select REGMAP
> +	select REGMAP_I2C
> +	select SND_SOC_CS530X
> +	help
> +	  Enable support for Cirrus Logic CS530X ADCs
> +	  with I2C control.
> +
>  config SND_SOC_CX20442
>  	tristate
>  	depends on TTY
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index d343d23c8f0f..3e08862d10e9 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -107,6 +107,8 @@ snd-soc-cs47l85-y := cs47l85.o
>  snd-soc-cs47l90-y := cs47l90.o
>  snd-soc-cs47l92-y := cs47l92.o
>  snd-soc-cs53l30-y := cs53l30.o
> +snd-soc-cs530x-y := cs530x.o
> +snd-soc-cs530x-i2c-y := cs530x-i2c.o
>  snd-soc-cx20442-y := cx20442.o
>  snd-soc-cx2072x-y := cx2072x.o
>  snd-soc-da7210-y := da7210.o
> @@ -506,6 +508,8 @@ obj-$(CONFIG_SND_SOC_CS47L85)	+= snd-soc-cs47l85.o
>  obj-$(CONFIG_SND_SOC_CS47L90)	+= snd-soc-cs47l90.o
>  obj-$(CONFIG_SND_SOC_CS47L92)	+= snd-soc-cs47l92.o
>  obj-$(CONFIG_SND_SOC_CS53L30)	+= snd-soc-cs53l30.o
> +obj-$(CONFIG_SND_SOC_CS530X)	+= snd-soc-cs530x.o
> +obj-$(CONFIG_SND_SOC_CS530X_I2C)	+= snd-soc-cs530x-i2c.o
>  obj-$(CONFIG_SND_SOC_CX20442)	+= snd-soc-cx20442.o
>  obj-$(CONFIG_SND_SOC_CX2072X)	+= snd-soc-cx2072x.o
>  obj-$(CONFIG_SND_SOC_DA7210)	+= snd-soc-da7210.o
> diff --git a/sound/soc/codecs/cs530x-i2c.c b/sound/soc/codecs/cs530x-i2c.c
> new file mode 100644
> index 000000000000..b2f808ee6f37
> --- /dev/null
> +++ b/sound/soc/codecs/cs530x-i2c.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// CS530x CODEC driver
> +//
> +// Copyright (C) 2024 Cirrus Logic, Inc. and
> +//                    Cirrus Logic International Semiconductor Ltd.
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +
> +#include "cs530x.h"
> +
> +static const struct of_device_id cs530x_of_match[] = {
> +	{
> +		.compatible = "cirrus,cs5302",
> +		.data = (void *)CS5302,
> +	}, {
> +		.compatible = "cirrus,cs5304",
> +		.data = (void *)CS5304,
> +	}, {
> +		.compatible = "cirrus,cs5308",
> +		.data = (void *)CS5308,
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cs530x_of_match);
> +
> +static const struct i2c_device_id cs530x_i2c_id[] = {
> +	{ "cs5302", CS5302 },
> +	{ "cs5304", CS5304 },
> +	{ "cs5308", CS5308 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, cs530x_i2c_id);
> +
> +static int cs530x_i2c_probe(struct i2c_client *client)
> +{
> +	struct cs530x_priv *cs530x;
> +	int ret;
> +
> +	cs530x = devm_kzalloc(&client->dev, sizeof(struct cs530x_priv),

sizeof(*)

I think you try to upstream some older code... please always use some
new kernel drivers, which are cleaned up from trivialities, for new work.

> +			       GFP_KERNEL);
> +	if (!cs530x)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, cs530x);
> +
> +	cs530x->regmap = devm_regmap_init_i2c(client, &cs530x_regmap);
> +	if (IS_ERR(cs530x->regmap)) {
> +		ret = PTR_ERR(cs530x->regmap);
> +		dev_err(&client->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	if (client->dev.of_node) {
> +		const struct of_device_id *match;
> +
> +		match = of_match_node(cs530x_of_match, client->dev.of_node);
> +		if (match == NULL)
> +			return -EINVAL;
> +		cs530x->devtype = (enum cs530x_type)match->data;

I think you open-coded device_get_match_data


> +	} else {
> +		const struct i2c_device_id *id =
> +			i2c_match_id(cs530x_i2c_id, client);
> +
> +		cs530x->devtype = id->driver_data;
> +	}

and for all of this there is a helper i2c_get_device_match_data.

> +
> +	cs530x->dev = &client->dev;
> +
> +	return cs530x_probe(cs530x);
> +}
> +
> +static struct i2c_driver cs530x_i2c_driver = {
> +	.driver = {
> +		.name = "cs530x",
> +		.of_match_table = cs530x_of_match,
> +	},
> +	.probe = cs530x_i2c_probe,
> +	.id_table = cs530x_i2c_id,
> +};
> +module_i2c_driver(cs530x_i2c_driver);
> +


> +
> +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) {
> +		dev_err(dev, "Failed to request supplies: %d\n", ret);

return dev_err_probe()

> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(cs530x->supplies),
> +				    cs530x->supplies);
> +	if (ret != 0) {
> +		dev_err(dev, "Failed to enable supplies: %d\n", ret);

return dev_err_probe()

> +		return ret;
> +	}
> +
> +	cs530x->reset_gpio = devm_gpiod_get_optional(dev, "reset-gpios",
> +						      GPIOD_OUT_LOW);
> +	if (IS_ERR(cs530x->reset_gpio)) {
> +		dev_err(dev, "Reset gpio not available\n");

return dev_err_probe()

> +		ret = PTR_ERR(cs530x->reset_gpio);
> +		goto err_regulator;
> +	}
> +
> +	if (cs530x->reset_gpio) {
> +		usleep_range(2000, 2100);
> +		gpiod_set_value_cansleep(cs530x->reset_gpio, 1);

1 is to keep device in reset? This looks like you are using logical
values inverted.

> +	}
> +
> +	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(dev, "Soft Reset write failed %d\n", ret);
> +			goto err_reset;
> +		}
> +	}
> +
> +	ret = cs530x_parse_device_properties(cs530x);
> +	if (ret)
> +		goto err_reset;
> +
> +	cs530x->dev_dai = devm_kmemdup(dev, &cs530x_dai,
> +					sizeof(struct snd_soc_dai_driver),

sizeof(*)

> +					GFP_KERNEL);
> +	if (!cs530x->dev_dai) {
> +		ret = -ENOMEM;
> +		goto err_reset;
> +	}
> +



Best regards,
Krzysztof