[PATCH] sound/soc/codecs/tas5720.c: add ACPI support

[PATCH] sound/soc/codecs/tas5720.c: add ACPI support

[Nikolaus Voss]

Add support for ACPI enumeration for tas5720 and tas5722.
Use device_match API to unify access to driver data for DT and ACPI.
Aggregate variant stuff into its own struct and directly reference
it in variant data for i2c/of/acpi_device_id.

Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
---
 sound/soc/codecs/tas5720.c | 215 +++++++++++++++++--------------------
 1 file changed, 99 insertions(+), 116 deletions(-)

diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c
index 37fab8f22800..ea973764c745 100644
--- a/sound/soc/codecs/tas5720.c
+++ b/sound/soc/codecs/tas5720.c
@@ -7,6 +7,7 @@
  * Author: Andreas Dannenberg <dannenberg@ti.com>
  */
 
+#include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/errno.h>
 #include <linux/device.h>
@@ -28,9 +29,10 @@
 /* Define how often to check (and clear) the fault status register (in ms) */
 #define TAS5720_FAULT_CHECK_INTERVAL		200
 
-enum tas572x_type {
-	TAS5720,
-	TAS5722,
+struct tas5720_variant {
+	const int device_id;
+	const struct regmap_config reg_config;
+	const struct snd_soc_component_driver comp_drv;
 };
 
 static const char * const tas5720_supply_names[] = {
@@ -44,7 +46,7 @@ struct tas5720_data {
 	struct snd_soc_component *component;
 	struct regmap *regmap;
 	struct i2c_client *tas5720_client;
-	enum tas572x_type devtype;
+	const struct tas5720_variant *variant;
 	struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
 	struct delayed_work fault_check_work;
 	unsigned int last_fault;
@@ -179,17 +181,13 @@ static int tas5720_set_dai_tdm_slot(struct snd_soc_dai *dai,
 		goto error_snd_soc_component_update_bits;
 
 	/* Configure TDM slot width. This is only applicable to TAS5722. */
-	switch (tas5720->devtype) {
-	case TAS5722:
+	if (tas5720->variant->device_id == TAS5722_DEVICE_ID) {
 		ret = snd_soc_component_update_bits(component, TAS5722_DIGITAL_CTRL2_REG,
 						    TAS5722_TDM_SLOT_16B,
 						    slot_width == 16 ?
 						    TAS5722_TDM_SLOT_16B : 0);
 		if (ret < 0)
 			goto error_snd_soc_component_update_bits;
-		break;
-	default:
-		break;
 	}
 
 	return 0;
@@ -277,7 +275,7 @@ static void tas5720_fault_check_work(struct work_struct *work)
 static int tas5720_codec_probe(struct snd_soc_component *component)
 {
 	struct tas5720_data *tas5720 = snd_soc_component_get_drvdata(component);
-	unsigned int device_id, expected_device_id;
+	unsigned int device_id;
 	int ret;
 
 	tas5720->component = component;
@@ -301,21 +299,9 @@ static int tas5720_codec_probe(struct snd_soc_component *component)
 		goto probe_fail;
 	}
 
-	switch (tas5720->devtype) {
-	case TAS5720:
-		expected_device_id = TAS5720_DEVICE_ID;
-		break;
-	case TAS5722:
-		expected_device_id = TAS5722_DEVICE_ID;
-		break;
-	default:
-		dev_err(component->dev, "unexpected private driver data\n");
-		return -EINVAL;
-	}
-
-	if (device_id != expected_device_id)
+	if (device_id != tas5720->variant->device_id)
 		dev_warn(component->dev, "wrong device ID. expected: %u read: %u\n",
-			 expected_device_id, device_id);
+			 tas5720->variant->device_id, device_id);
 
 	/* Set device to mute */
 	ret = snd_soc_component_update_bits(component, TAS5720_DIGITAL_CTRL2_REG,
@@ -462,24 +448,6 @@ static bool tas5720_is_volatile_reg(struct device *dev, unsigned int reg)
 	}
 }
 
-static const struct regmap_config tas5720_regmap_config = {
-	.reg_bits = 8,
-	.val_bits = 8,
-
-	.max_register = TAS5720_MAX_REG,
-	.cache_type = REGCACHE_RBTREE,
-	.volatile_reg = tas5720_is_volatile_reg,
-};
-
-static const struct regmap_config tas5722_regmap_config = {
-	.reg_bits = 8,
-	.val_bits = 8,
-
-	.max_register = TAS5722_MAX_REG,
-	.cache_type = REGCACHE_RBTREE,
-	.volatile_reg = tas5720_is_volatile_reg,
-};
-
 /*
  * DAC analog gain. There are four discrete values to select from, ranging
  * from 19.2 dB to 26.3dB.
@@ -558,40 +526,6 @@ static const struct snd_soc_dapm_route tas5720_audio_map[] = {
 	{ "OUT", NULL, "DAC" },
 };
 
-static const struct snd_soc_component_driver soc_component_dev_tas5720 = {
-	.probe			= tas5720_codec_probe,
-	.remove			= tas5720_codec_remove,
-	.suspend		= tas5720_suspend,
-	.resume			= tas5720_resume,
-	.controls		= tas5720_snd_controls,
-	.num_controls		= ARRAY_SIZE(tas5720_snd_controls),
-	.dapm_widgets		= tas5720_dapm_widgets,
-	.num_dapm_widgets	= ARRAY_SIZE(tas5720_dapm_widgets),
-	.dapm_routes		= tas5720_audio_map,
-	.num_dapm_routes	= ARRAY_SIZE(tas5720_audio_map),
-	.idle_bias_on		= 1,
-	.use_pmdown_time	= 1,
-	.endianness		= 1,
-	.non_legacy_dai_naming	= 1,
-};
-
-static const struct snd_soc_component_driver soc_component_dev_tas5722 = {
-	.probe = tas5720_codec_probe,
-	.remove = tas5720_codec_remove,
-	.suspend = tas5720_suspend,
-	.resume = tas5720_resume,
-	.controls = tas5722_snd_controls,
-	.num_controls = ARRAY_SIZE(tas5722_snd_controls),
-	.dapm_widgets = tas5720_dapm_widgets,
-	.num_dapm_widgets = ARRAY_SIZE(tas5720_dapm_widgets),
-	.dapm_routes = tas5720_audio_map,
-	.num_dapm_routes = ARRAY_SIZE(tas5720_audio_map),
-	.idle_bias_on		= 1,
-	.use_pmdown_time	= 1,
-	.endianness		= 1,
-	.non_legacy_dai_naming	= 1,
-};
-
 /* PCM rates supported by the TAS5720 driver */
 #define TAS5720_RATES	(SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\
 			 SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)
@@ -637,29 +571,25 @@ static int tas5720_probe(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	struct tas5720_data *data;
-	const struct regmap_config *regmap_config;
+	const struct tas5720_variant *type;
 	int ret;
 	int i;
 
+	type = device_get_match_data(&client->dev);
+	if (!type && id)
+		type = (const struct tas5720_variant *)id->driver_data;
+
+	if (!type)
+		return -EINVAL;
+
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
 	data->tas5720_client = client;
-	data->devtype = id->driver_data;
+	data->variant = type;
 
-	switch (id->driver_data) {
-	case TAS5720:
-		regmap_config = &tas5720_regmap_config;
-		break;
-	case TAS5722:
-		regmap_config = &tas5722_regmap_config;
-		break;
-	default:
-		dev_err(dev, "unexpected private driver data\n");
-		return -EINVAL;
-	}
-	data->regmap = devm_regmap_init_i2c(client, regmap_config);
+	data->regmap = devm_regmap_init_i2c(client, &type->reg_config);
 	if (IS_ERR(data->regmap)) {
 		ret = PTR_ERR(data->regmap);
 		dev_err(dev, "failed to allocate register map: %d\n", ret);
@@ -678,51 +608,104 @@ static int tas5720_probe(struct i2c_client *client,
 
 	dev_set_drvdata(dev, data);
 
-	switch (id->driver_data) {
-	case TAS5720:
-		ret = devm_snd_soc_register_component(&client->dev,
-					&soc_component_dev_tas5720,
-					tas5720_dai,
-					ARRAY_SIZE(tas5720_dai));
-		break;
-	case TAS5722:
-		ret = devm_snd_soc_register_component(&client->dev,
-					&soc_component_dev_tas5722,
-					tas5720_dai,
-					ARRAY_SIZE(tas5720_dai));
-		break;
-	default:
-		dev_err(dev, "unexpected private driver data\n");
-		return -EINVAL;
-	}
-	if (ret < 0) {
-		dev_err(dev, "failed to register component: %d\n", ret);
-		return ret;
-	}
+	ret = devm_snd_soc_register_component(&client->dev,
+					      &type->comp_drv,
+					      tas5720_dai,
+					      ARRAY_SIZE(tas5720_dai));
 
-	return 0;
+	if (ret < 0)
+		dev_err(dev, "failed to register component: %d\n", ret);
+
+	return ret;
 }
 
+static const struct tas5720_variant variant[] = {
+	{
+		.device_id = TAS5720_DEVICE_ID,
+		.reg_config = {
+			.reg_bits = 8,
+			.val_bits = 8,
+
+			.max_register = TAS5720_MAX_REG,
+			.cache_type = REGCACHE_RBTREE,
+			.volatile_reg = tas5720_is_volatile_reg,
+		},
+		.comp_drv = {
+			.probe = tas5720_codec_probe,
+			.remove = tas5720_codec_remove,
+			.suspend = tas5720_suspend,
+			.resume = tas5720_resume,
+			.controls = tas5720_snd_controls,
+			.num_controls = ARRAY_SIZE(tas5720_snd_controls),
+			.dapm_widgets = tas5720_dapm_widgets,
+			.num_dapm_widgets = ARRAY_SIZE(tas5720_dapm_widgets),
+			.dapm_routes = tas5720_audio_map,
+			.num_dapm_routes = ARRAY_SIZE(tas5720_audio_map),
+			.idle_bias_on = 1,
+			.use_pmdown_time = 1,
+			.endianness = 1,
+			.non_legacy_dai_naming = 1
+		},
+	},
+	{
+		.device_id = TAS5722_DEVICE_ID,
+		.reg_config = {
+			.reg_bits = 8,
+			.val_bits = 8,
+
+			.max_register = TAS5722_MAX_REG,
+			.cache_type = REGCACHE_RBTREE,
+			.volatile_reg = tas5720_is_volatile_reg,
+		},
+		.comp_drv = {
+			.probe = tas5720_codec_probe,
+			.remove = tas5720_codec_remove,
+			.suspend = tas5720_suspend,
+			.resume = tas5720_resume,
+			.controls = tas5722_snd_controls,
+			.num_controls = ARRAY_SIZE(tas5722_snd_controls),
+			.dapm_widgets = tas5720_dapm_widgets,
+			.num_dapm_widgets = ARRAY_SIZE(tas5720_dapm_widgets),
+			.dapm_routes = tas5720_audio_map,
+			.num_dapm_routes = ARRAY_SIZE(tas5720_audio_map),
+			.idle_bias_on = 1,
+			.use_pmdown_time = 1,
+			.endianness = 1,
+			.non_legacy_dai_naming = 1,
+		},
+	},
+};
+
 static const struct i2c_device_id tas5720_id[] = {
-	{ "tas5720", TAS5720 },
-	{ "tas5722", TAS5722 },
+	{ "tas5720", (kernel_ulong_t)&variant[0] },
+	{ "tas5722", (kernel_ulong_t)&variant[1] },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tas5720_id);
 
 #if IS_ENABLED(CONFIG_OF)
 static const struct of_device_id tas5720_of_match[] = {
-	{ .compatible = "ti,tas5720", },
-	{ .compatible = "ti,tas5722", },
+	{ .compatible = "ti,tas5720", .data = &variant[0], },
+	{ .compatible = "ti,tas5722", .data = &variant[1], },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, tas5720_of_match);
 #endif
 
+#if IS_ENABLED(CONFIG_ACPI)
+static const struct acpi_device_id tas5720_acpi_match[] = {
+	{ "10TI5720", (kernel_ulong_t)&variant[0] },
+	{ "10TI5722", (kernel_ulong_t)&variant[1] },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, tas5720_acpi_match);
+#endif
+
 static struct i2c_driver tas5720_i2c_driver = {
 	.driver = {
 		.name = "tas5720",
 		.of_match_table = of_match_ptr(tas5720_of_match),
+		.acpi_match_table = ACPI_PTR(tas5720_acpi_match),
 	},
 	.probe = tas5720_probe,
 	.id_table = tas5720_id,
-- 
2.17.1

Re: [PATCH] sound/soc/codecs/tas5720.c: add ACPI support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

On Fri, Jun 28, 2019 at 02:34:16PM +0200, Nikolaus Voss wrote:
> Add support for ACPI enumeration for tas5720 and tas5722.
> Use device_match API to unify access to driver data for DT and ACPI.
> Aggregate variant stuff into its own struct and directly reference
> it in variant data for i2c/of/acpi_device_id.

Please use subject lines matching the style for the subsystem.  This
makes it easier for people to identify relevant patches.

There's a huge amount of refactoring in here as well as the enumeration
changes, please split this up into a patch series as covered in
submitting-patches.rst for review.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] sound/soc/codecs/tas5720.c: add ACPI support

[Andrew F. Davis]

On 6/28/19 8:34 AM, Nikolaus Voss wrote:
> Add support for ACPI enumeration for tas5720 and tas5722.
> Use device_match API to unify access to driver data for DT and ACPI.
> Aggregate variant stuff into its own struct and directly reference
> it in variant data for i2c/of/acpi_device_id.
> 
> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
> ---
>  sound/soc/codecs/tas5720.c | 215 +++++++++++++++++--------------------
>  1 file changed, 99 insertions(+), 116 deletions(-)
> 
> diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c
> index 37fab8f22800..ea973764c745 100644
> --- a/sound/soc/codecs/tas5720.c
> +++ b/sound/soc/codecs/tas5720.c
> @@ -7,6 +7,7 @@
>   * Author: Andreas Dannenberg <dannenberg@ti.com>
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/module.h>
>  #include <linux/errno.h>
>  #include <linux/device.h>
> @@ -28,9 +29,10 @@
>  /* Define how often to check (and clear) the fault status register (in ms) */
>  #define TAS5720_FAULT_CHECK_INTERVAL		200
>  
> -enum tas572x_type {
> -	TAS5720,
> -	TAS5722,
> +struct tas5720_variant {
> +	const int device_id;
> +	const struct regmap_config reg_config;
> +	const struct snd_soc_component_driver comp_drv;
>  };
>  
>  static const char * const tas5720_supply_names[] = {
> @@ -44,7 +46,7 @@ struct tas5720_data {
>  	struct snd_soc_component *component;
>  	struct regmap *regmap;
>  	struct i2c_client *tas5720_client;
> -	enum tas572x_type devtype;
> +	const struct tas5720_variant *variant;
>  	struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
>  	struct delayed_work fault_check_work;
>  	unsigned int last_fault;
> @@ -179,17 +181,13 @@ static int tas5720_set_dai_tdm_slot(struct snd_soc_dai *dai,
>  		goto error_snd_soc_component_update_bits;
>  
>  	/* Configure TDM slot width. This is only applicable to TAS5722. */
> -	switch (tas5720->devtype) {
> -	case TAS5722:
> +	if (tas5720->variant->device_id == TAS5722_DEVICE_ID) {
>  		ret = snd_soc_component_update_bits(component, TAS5722_DIGITAL_CTRL2_REG,
>  						    TAS5722_TDM_SLOT_16B,
>  						    slot_width == 16 ?
>  						    TAS5722_TDM_SLOT_16B : 0);
>  		if (ret < 0)
>  			goto error_snd_soc_component_update_bits;
> -		break;
> -	default:
> -		break;
>  	}
>  
>  	return 0;
> @@ -277,7 +275,7 @@ static void tas5720_fault_check_work(struct work_struct *work)
>  static int tas5720_codec_probe(struct snd_soc_component *component)
>  {
>  	struct tas5720_data *tas5720 = snd_soc_component_get_drvdata(component);
> -	unsigned int device_id, expected_device_id;
> +	unsigned int device_id;
>  	int ret;
>  
>  	tas5720->component = component;
> @@ -301,21 +299,9 @@ static int tas5720_codec_probe(struct snd_soc_component *component)
>  		goto probe_fail;
>  	}
>  
> -	switch (tas5720->devtype) {
> -	case TAS5720:
> -		expected_device_id = TAS5720_DEVICE_ID;
> -		break;
> -	case TAS5722:
> -		expected_device_id = TAS5722_DEVICE_ID;
> -		break;
> -	default:
> -		dev_err(component->dev, "unexpected private driver data\n");
> -		return -EINVAL;
> -	}
> -
> -	if (device_id != expected_device_id)
> +	if (device_id != tas5720->variant->device_id)
>  		dev_warn(component->dev, "wrong device ID. expected: %u read: %u\n",
> -			 expected_device_id, device_id);
> +			 tas5720->variant->device_id, device_id);
>  
>  	/* Set device to mute */
>  	ret = snd_soc_component_update_bits(component, TAS5720_DIGITAL_CTRL2_REG,
> @@ -462,24 +448,6 @@ static bool tas5720_is_volatile_reg(struct device *dev, unsigned int reg)
>  	}
>  }
>  
> -static const struct regmap_config tas5720_regmap_config = {
> -	.reg_bits = 8,
> -	.val_bits = 8,
> -
> -	.max_register = TAS5720_MAX_REG,
> -	.cache_type = REGCACHE_RBTREE,
> -	.volatile_reg = tas5720_is_volatile_reg,
> -};
> -
> -static const struct regmap_config tas5722_regmap_config = {
> -	.reg_bits = 8,
> -	.val_bits = 8,
> -
> -	.max_register = TAS5722_MAX_REG,
> -	.cache_type = REGCACHE_RBTREE,
> -	.volatile_reg = tas5720_is_volatile_reg,
> -};
> -
>  /*
>   * DAC analog gain. There are four discrete values to select from, ranging
>   * from 19.2 dB to 26.3dB.
> @@ -558,40 +526,6 @@ static const struct snd_soc_dapm_route tas5720_audio_map[] = {
>  	{ "OUT", NULL, "DAC" },
>  };
>  
> -static const struct snd_soc_component_driver soc_component_dev_tas5720 = {
> -	.probe			= tas5720_codec_probe,
> -	.remove			= tas5720_codec_remove,
> -	.suspend		= tas5720_suspend,
> -	.resume			= tas5720_resume,
> -	.controls		= tas5720_snd_controls,
> -	.num_controls		= ARRAY_SIZE(tas5720_snd_controls),
> -	.dapm_widgets		= tas5720_dapm_widgets,
> -	.num_dapm_widgets	= ARRAY_SIZE(tas5720_dapm_widgets),
> -	.dapm_routes		= tas5720_audio_map,
> -	.num_dapm_routes	= ARRAY_SIZE(tas5720_audio_map),
> -	.idle_bias_on		= 1,
> -	.use_pmdown_time	= 1,
> -	.endianness		= 1,
> -	.non_legacy_dai_naming	= 1,
> -};
> -
> -static const struct snd_soc_component_driver soc_component_dev_tas5722 = {
> -	.probe = tas5720_codec_probe,
> -	.remove = tas5720_codec_remove,
> -	.suspend = tas5720_suspend,
> -	.resume = tas5720_resume,
> -	.controls = tas5722_snd_controls,
> -	.num_controls = ARRAY_SIZE(tas5722_snd_controls),
> -	.dapm_widgets = tas5720_dapm_widgets,
> -	.num_dapm_widgets = ARRAY_SIZE(tas5720_dapm_widgets),
> -	.dapm_routes = tas5720_audio_map,
> -	.num_dapm_routes = ARRAY_SIZE(tas5720_audio_map),
> -	.idle_bias_on		= 1,
> -	.use_pmdown_time	= 1,
> -	.endianness		= 1,
> -	.non_legacy_dai_naming	= 1,
> -};
> -
>  /* PCM rates supported by the TAS5720 driver */
>  #define TAS5720_RATES	(SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\
>  			 SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)
> @@ -637,29 +571,25 @@ static int tas5720_probe(struct i2c_client *client,
>  {
>  	struct device *dev = &client->dev;
>  	struct tas5720_data *data;
> -	const struct regmap_config *regmap_config;
> +	const struct tas5720_variant *type;
>  	int ret;
>  	int i;
>  
> +	type = device_get_match_data(&client->dev);
> +	if (!type && id)
> +		type = (const struct tas5720_variant *)id->driver_data;
> +
> +	if (!type)
> +		return -EINVAL;
> +
>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
>  	data->tas5720_client = client;
> -	data->devtype = id->driver_data;
> +	data->variant = type;
>  
> -	switch (id->driver_data) {
> -	case TAS5720:
> -		regmap_config = &tas5720_regmap_config;
> -		break;
> -	case TAS5722:
> -		regmap_config = &tas5722_regmap_config;
> -		break;
> -	default:
> -		dev_err(dev, "unexpected private driver data\n");
> -		return -EINVAL;
> -	}
> -	data->regmap = devm_regmap_init_i2c(client, regmap_config);
> +	data->regmap = devm_regmap_init_i2c(client, &type->reg_config);
>  	if (IS_ERR(data->regmap)) {
>  		ret = PTR_ERR(data->regmap);
>  		dev_err(dev, "failed to allocate register map: %d\n", ret);
> @@ -678,51 +608,104 @@ static int tas5720_probe(struct i2c_client *client,
>  
>  	dev_set_drvdata(dev, data);
>  
> -	switch (id->driver_data) {
> -	case TAS5720:
> -		ret = devm_snd_soc_register_component(&client->dev,
> -					&soc_component_dev_tas5720,
> -					tas5720_dai,
> -					ARRAY_SIZE(tas5720_dai));
> -		break;
> -	case TAS5722:
> -		ret = devm_snd_soc_register_component(&client->dev,
> -					&soc_component_dev_tas5722,
> -					tas5720_dai,
> -					ARRAY_SIZE(tas5720_dai));
> -		break;
> -	default:
> -		dev_err(dev, "unexpected private driver data\n");
> -		return -EINVAL;
> -	}
> -	if (ret < 0) {
> -		dev_err(dev, "failed to register component: %d\n", ret);
> -		return ret;
> -	}
> +	ret = devm_snd_soc_register_component(&client->dev,
> +					      &type->comp_drv,
> +					      tas5720_dai,
> +					      ARRAY_SIZE(tas5720_dai));
>  
> -	return 0;
> +	if (ret < 0)
> +		dev_err(dev, "failed to register component: %d\n", ret);
> +
> +	return ret;
>  }
>  
> +static const struct tas5720_variant variant[] = {
> +	{
> +		.device_id = TAS5720_DEVICE_ID,
> +		.reg_config = {


This patch would be a lot more simple if you leave the regmap_config and
snd_soc_component_driver definitions where they are above and just store
a pointer to them down here in this new struct. That also would allow
for new devices to use them in this list should they ever match.

If you really want to move the data down here for some reason, do it in
a separate patch at least, this isn't needed as part of adding ACPI support.

Andrew

> +			.reg_bits = 8,
> +			.val_bits = 8,
> +
> +			.max_register = TAS5720_MAX_REG,
> +			.cache_type = REGCACHE_RBTREE,
> +			.volatile_reg = tas5720_is_volatile_reg,
> +		},
> +		.comp_drv = {
> +			.probe = tas5720_codec_probe,
> +			.remove = tas5720_codec_remove,
> +			.suspend = tas5720_suspend,
> +			.resume = tas5720_resume,
> +			.controls = tas5720_snd_controls,
> +			.num_controls = ARRAY_SIZE(tas5720_snd_controls),
> +			.dapm_widgets = tas5720_dapm_widgets,
> +			.num_dapm_widgets = ARRAY_SIZE(tas5720_dapm_widgets),
> +			.dapm_routes = tas5720_audio_map,
> +			.num_dapm_routes = ARRAY_SIZE(tas5720_audio_map),
> +			.idle_bias_on = 1,
> +			.use_pmdown_time = 1,
> +			.endianness = 1,
> +			.non_legacy_dai_naming = 1
> +		},
> +	},
> +	{
> +		.device_id = TAS5722_DEVICE_ID,
> +		.reg_config = {
> +			.reg_bits = 8,
> +			.val_bits = 8,
> +
> +			.max_register = TAS5722_MAX_REG,
> +			.cache_type = REGCACHE_RBTREE,
> +			.volatile_reg = tas5720_is_volatile_reg,
> +		},
> +		.comp_drv = {
> +			.probe = tas5720_codec_probe,
> +			.remove = tas5720_codec_remove,
> +			.suspend = tas5720_suspend,
> +			.resume = tas5720_resume,
> +			.controls = tas5722_snd_controls,
> +			.num_controls = ARRAY_SIZE(tas5722_snd_controls),
> +			.dapm_widgets = tas5720_dapm_widgets,
> +			.num_dapm_widgets = ARRAY_SIZE(tas5720_dapm_widgets),
> +			.dapm_routes = tas5720_audio_map,
> +			.num_dapm_routes = ARRAY_SIZE(tas5720_audio_map),
> +			.idle_bias_on = 1,
> +			.use_pmdown_time = 1,
> +			.endianness = 1,
> +			.non_legacy_dai_naming = 1,
> +		},
> +	},
> +};
> +
>  static const struct i2c_device_id tas5720_id[] = {
> -	{ "tas5720", TAS5720 },
> -	{ "tas5722", TAS5722 },
> +	{ "tas5720", (kernel_ulong_t)&variant[0] },
> +	{ "tas5722", (kernel_ulong_t)&variant[1] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, tas5720_id);
>  
>  #if IS_ENABLED(CONFIG_OF)
>  static const struct of_device_id tas5720_of_match[] = {
> -	{ .compatible = "ti,tas5720", },
> -	{ .compatible = "ti,tas5722", },
> +	{ .compatible = "ti,tas5720", .data = &variant[0], },
> +	{ .compatible = "ti,tas5722", .data = &variant[1], },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, tas5720_of_match);
>  #endif
>  
> +#if IS_ENABLED(CONFIG_ACPI)
> +static const struct acpi_device_id tas5720_acpi_match[] = {
> +	{ "10TI5720", (kernel_ulong_t)&variant[0] },
> +	{ "10TI5722", (kernel_ulong_t)&variant[1] },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, tas5720_acpi_match);
> +#endif
> +
>  static struct i2c_driver tas5720_i2c_driver = {
>  	.driver = {
>  		.name = "tas5720",
>  		.of_match_table = of_match_ptr(tas5720_of_match),
> +		.acpi_match_table = ACPI_PTR(tas5720_acpi_match),
>  	},
>  	.probe = tas5720_probe,
>  	.id_table = tas5720_id,
> 

Re: [PATCH] sound/soc/codecs/tas5720.c: add ACPI support

[Nikolaus Voss]

On Fri, 28 Jun 2019, Mark Brown wrote:
> On Fri, Jun 28, 2019 at 02:34:16PM +0200, Nikolaus Voss wrote:
>> Add support for ACPI enumeration for tas5720 and tas5722.
>> Use device_match API to unify access to driver data for DT and ACPI.
>> Aggregate variant stuff into its own struct and directly reference
>> it in variant data for i2c/of/acpi_device_id.
>
> Please use subject lines matching the style for the subsystem.  This
> makes it easier for people to identify relevant patches.
>
> There's a huge amount of refactoring in here as well as the enumeration
> changes, please split this up into a patch series as covered in
> submitting-patches.rst for review.
>

Ok, thanks for the feedback, I'll prepare a series.

Nikolaus

Re: [PATCH] sound/soc/codecs/tas5720.c: add ACPI support

[Nikolaus Voss]

On Fri, 28 Jun 2019, Andrew F. Davis wrote:
> On 6/28/19 8:34 AM, Nikolaus Voss wrote:
>> Add support for ACPI enumeration for tas5720 and tas5722.
>> Use device_match API to unify access to driver data for DT and ACPI.
>> Aggregate variant stuff into its own struct and directly reference
>> it in variant data for i2c/of/acpi_device_id.
>>
>> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
>> ---
>>  sound/soc/codecs/tas5720.c | 215 +++++++++++++++++--------------------
>>  1 file changed, 99 insertions(+), 116 deletions(-)
>>
>> diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c
>> index 37fab8f22800..ea973764c745 100644
>> --- a/sound/soc/codecs/tas5720.c
>> +++ b/sound/soc/codecs/tas5720.c
>> @@ -7,6 +7,7 @@
>>   * Author: Andreas Dannenberg <dannenberg@ti.com>
>>   */
>>
>> +#include <linux/acpi.h>
>>  #include <linux/module.h>
>>  #include <linux/errno.h>
>>  #include <linux/device.h>
>> @@ -28,9 +29,10 @@
>>  /* Define how often to check (and clear) the fault status register (in ms) */
>>  #define TAS5720_FAULT_CHECK_INTERVAL		200
>>
>> -enum tas572x_type {
>> -	TAS5720,
>> -	TAS5722,
>> +struct tas5720_variant {
>> +	const int device_id;
>> +	const struct regmap_config reg_config;
>> +	const struct snd_soc_component_driver comp_drv;
>>  };
>>
>>  static const char * const tas5720_supply_names[] = {
>> @@ -44,7 +46,7 @@ struct tas5720_data {
>>  	struct snd_soc_component *component;
>>  	struct regmap *regmap;
>>  	struct i2c_client *tas5720_client;
>> -	enum tas572x_type devtype;
>> +	const struct tas5720_variant *variant;
>>  	struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
>>  	struct delayed_work fault_check_work;
>>  	unsigned int last_fault;
>> @@ -179,17 +181,13 @@ static int tas5720_set_dai_tdm_slot(struct snd_soc_dai *dai,
>>  		goto error_snd_soc_component_update_bits;
>>
>>  	/* Configure TDM slot width. This is only applicable to TAS5722. */
>> -	switch (tas5720->devtype) {
>> -	case TAS5722:
>> +	if (tas5720->variant->device_id == TAS5722_DEVICE_ID) {
>>  		ret = snd_soc_component_update_bits(component, TAS5722_DIGITAL_CTRL2_REG,
>>  						    TAS5722_TDM_SLOT_16B,
>>  						    slot_width == 16 ?
>>  						    TAS5722_TDM_SLOT_16B : 0);
>>  		if (ret < 0)
>>  			goto error_snd_soc_component_update_bits;
>> -		break;
>> -	default:
>> -		break;
>>  	}
>>
>>  	return 0;
>> @@ -277,7 +275,7 @@ static void tas5720_fault_check_work(struct work_struct *work)
>>  static int tas5720_codec_probe(struct snd_soc_component *component)
>>  {
>>  	struct tas5720_data *tas5720 = snd_soc_component_get_drvdata(component);
>> -	unsigned int device_id, expected_device_id;
>> +	unsigned int device_id;
>>  	int ret;
>>
>>  	tas5720->component = component;
>> @@ -301,21 +299,9 @@ static int tas5720_codec_probe(struct snd_soc_component *component)
>>  		goto probe_fail;
>>  	}
>>
>> -	switch (tas5720->devtype) {
>> -	case TAS5720:
>> -		expected_device_id = TAS5720_DEVICE_ID;
>> -		break;
>> -	case TAS5722:
>> -		expected_device_id = TAS5722_DEVICE_ID;
>> -		break;
>> -	default:
>> -		dev_err(component->dev, "unexpected private driver data\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	if (device_id != expected_device_id)
>> +	if (device_id != tas5720->variant->device_id)
>>  		dev_warn(component->dev, "wrong device ID. expected: %u read: %u\n",
>> -			 expected_device_id, device_id);
>> +			 tas5720->variant->device_id, device_id);
>>
>>  	/* Set device to mute */
>>  	ret = snd_soc_component_update_bits(component, TAS5720_DIGITAL_CTRL2_REG,
>> @@ -462,24 +448,6 @@ static bool tas5720_is_volatile_reg(struct device *dev, unsigned int reg)
>>  	}
>>  }
>>
>> -static const struct regmap_config tas5720_regmap_config = {
>> -	.reg_bits = 8,
>> -	.val_bits = 8,
>> -
>> -	.max_register = TAS5720_MAX_REG,
>> -	.cache_type = REGCACHE_RBTREE,
>> -	.volatile_reg = tas5720_is_volatile_reg,
>> -};
>> -
>> -static const struct regmap_config tas5722_regmap_config = {
>> -	.reg_bits = 8,
>> -	.val_bits = 8,
>> -
>> -	.max_register = TAS5722_MAX_REG,
>> -	.cache_type = REGCACHE_RBTREE,
>> -	.volatile_reg = tas5720_is_volatile_reg,
>> -};
>> -
>>  /*
>>   * DAC analog gain. There are four discrete values to select from, ranging
>>   * from 19.2 dB to 26.3dB.
>> @@ -558,40 +526,6 @@ static const struct snd_soc_dapm_route tas5720_audio_map[] = {
>>  	{ "OUT", NULL, "DAC" },
>>  };
>>
>> -static const struct snd_soc_component_driver soc_component_dev_tas5720 = {
>> -	.probe			= tas5720_codec_probe,
>> -	.remove			= tas5720_codec_remove,
>> -	.suspend		= tas5720_suspend,
>> -	.resume			= tas5720_resume,
>> -	.controls		= tas5720_snd_controls,
>> -	.num_controls		= ARRAY_SIZE(tas5720_snd_controls),
>> -	.dapm_widgets		= tas5720_dapm_widgets,
>> -	.num_dapm_widgets	= ARRAY_SIZE(tas5720_dapm_widgets),
>> -	.dapm_routes		= tas5720_audio_map,
>> -	.num_dapm_routes	= ARRAY_SIZE(tas5720_audio_map),
>> -	.idle_bias_on		= 1,
>> -	.use_pmdown_time	= 1,
>> -	.endianness		= 1,
>> -	.non_legacy_dai_naming	= 1,
>> -};
>> -
>> -static const struct snd_soc_component_driver soc_component_dev_tas5722 = {
>> -	.probe = tas5720_codec_probe,
>> -	.remove = tas5720_codec_remove,
>> -	.suspend = tas5720_suspend,
>> -	.resume = tas5720_resume,
>> -	.controls = tas5722_snd_controls,
>> -	.num_controls = ARRAY_SIZE(tas5722_snd_controls),
>> -	.dapm_widgets = tas5720_dapm_widgets,
>> -	.num_dapm_widgets = ARRAY_SIZE(tas5720_dapm_widgets),
>> -	.dapm_routes = tas5720_audio_map,
>> -	.num_dapm_routes = ARRAY_SIZE(tas5720_audio_map),
>> -	.idle_bias_on		= 1,
>> -	.use_pmdown_time	= 1,
>> -	.endianness		= 1,
>> -	.non_legacy_dai_naming	= 1,
>> -};
>> -
>>  /* PCM rates supported by the TAS5720 driver */
>>  #define TAS5720_RATES	(SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\
>>  			 SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)
>> @@ -637,29 +571,25 @@ static int tas5720_probe(struct i2c_client *client,
>>  {
>>  	struct device *dev = &client->dev;
>>  	struct tas5720_data *data;
>> -	const struct regmap_config *regmap_config;
>> +	const struct tas5720_variant *type;
>>  	int ret;
>>  	int i;
>>
>> +	type = device_get_match_data(&client->dev);
>> +	if (!type && id)
>> +		type = (const struct tas5720_variant *)id->driver_data;
>> +
>> +	if (!type)
>> +		return -EINVAL;
>> +
>>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>  	if (!data)
>>  		return -ENOMEM;
>>
>>  	data->tas5720_client = client;
>> -	data->devtype = id->driver_data;
>> +	data->variant = type;
>>
>> -	switch (id->driver_data) {
>> -	case TAS5720:
>> -		regmap_config = &tas5720_regmap_config;
>> -		break;
>> -	case TAS5722:
>> -		regmap_config = &tas5722_regmap_config;
>> -		break;
>> -	default:
>> -		dev_err(dev, "unexpected private driver data\n");
>> -		return -EINVAL;
>> -	}
>> -	data->regmap = devm_regmap_init_i2c(client, regmap_config);
>> +	data->regmap = devm_regmap_init_i2c(client, &type->reg_config);
>>  	if (IS_ERR(data->regmap)) {
>>  		ret = PTR_ERR(data->regmap);
>>  		dev_err(dev, "failed to allocate register map: %d\n", ret);
>> @@ -678,51 +608,104 @@ static int tas5720_probe(struct i2c_client *client,
>>
>>  	dev_set_drvdata(dev, data);
>>
>> -	switch (id->driver_data) {
>> -	case TAS5720:
>> -		ret = devm_snd_soc_register_component(&client->dev,
>> -					&soc_component_dev_tas5720,
>> -					tas5720_dai,
>> -					ARRAY_SIZE(tas5720_dai));
>> -		break;
>> -	case TAS5722:
>> -		ret = devm_snd_soc_register_component(&client->dev,
>> -					&soc_component_dev_tas5722,
>> -					tas5720_dai,
>> -					ARRAY_SIZE(tas5720_dai));
>> -		break;
>> -	default:
>> -		dev_err(dev, "unexpected private driver data\n");
>> -		return -EINVAL;
>> -	}
>> -	if (ret < 0) {
>> -		dev_err(dev, "failed to register component: %d\n", ret);
>> -		return ret;
>> -	}
>> +	ret = devm_snd_soc_register_component(&client->dev,
>> +					      &type->comp_drv,
>> +					      tas5720_dai,
>> +					      ARRAY_SIZE(tas5720_dai));
>>
>> -	return 0;
>> +	if (ret < 0)
>> +		dev_err(dev, "failed to register component: %d\n", ret);
>> +
>> +	return ret;
>>  }
>>
>> +static const struct tas5720_variant variant[] = {
>> +	{
>> +		.device_id = TAS5720_DEVICE_ID,
>> +		.reg_config = {
>
>
> This patch would be a lot more simple if you leave the regmap_config and
> snd_soc_component_driver definitions where they are above and just store
> a pointer to them down here in this new struct. That also would allow
> for new devices to use them in this list should they ever match.
>
> If you really want to move the data down here for some reason, do it in
> a separate patch at least, this isn't needed as part of adding ACPI support.

Ok, thanks for the feedback, I'll prepare a series.

Nikolaus

>
> Andrew

[...]

[PATCH v2 0/2] ASoC: tas5720.c: add ACPI enumeration support

[Nikolaus Voss]

Patch 1 aggregates variant specific stuff in a struct which is
directly referenced in the id tables (no functional change).

Patch 2 actually adds ACPI IDs for the two DAC variants and uses the
device match API to get the variant in a firmware agnostic manner.

v2:
- split patch into series as suggested by Mark Brown and Andrew F. Davis.
- don't integrate variant data into variant struct but reference it
  (suggested by Andrew F. Davis).

Nikolaus Voss (2):
  ASoC: tas5720.c: cleanup variant management
  ASoC: tas5720.c: add ACPI enumeration support

 sound/soc/codecs/tas5720.c | 111 +++++++++++++++----------------------
 1 file changed, 46 insertions(+), 65 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] ASoC: tas5720.c: cleanup variant management

[Nikolaus Voss]

Replace enum tas572x_type with struct tas5720_variant which aggregates
variant specific stuff and can be directly referenced from an id table.

Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
---
 sound/soc/codecs/tas5720.c | 98 +++++++++++++-------------------------
 1 file changed, 33 insertions(+), 65 deletions(-)

diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c
index 37fab8f22800..b2e897f094b4 100644
--- a/sound/soc/codecs/tas5720.c
+++ b/sound/soc/codecs/tas5720.c
@@ -28,9 +28,10 @@
 /* Define how often to check (and clear) the fault status register (in ms) */
 #define TAS5720_FAULT_CHECK_INTERVAL		200
 
-enum tas572x_type {
-	TAS5720,
-	TAS5722,
+struct tas5720_variant {
+	const int device_id;
+	const struct regmap_config *reg_config;
+	const struct snd_soc_component_driver *comp_drv;
 };
 
 static const char * const tas5720_supply_names[] = {
@@ -44,7 +45,7 @@ struct tas5720_data {
 	struct snd_soc_component *component;
 	struct regmap *regmap;
 	struct i2c_client *tas5720_client;
-	enum tas572x_type devtype;
+	const struct tas5720_variant *variant;
 	struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
 	struct delayed_work fault_check_work;
 	unsigned int last_fault;
@@ -179,17 +180,13 @@ static int tas5720_set_dai_tdm_slot(struct snd_soc_dai *dai,
 		goto error_snd_soc_component_update_bits;
 
 	/* Configure TDM slot width. This is only applicable to TAS5722. */
-	switch (tas5720->devtype) {
-	case TAS5722:
+	if (tas5720->variant->device_id == TAS5722_DEVICE_ID) {
 		ret = snd_soc_component_update_bits(component, TAS5722_DIGITAL_CTRL2_REG,
 						    TAS5722_TDM_SLOT_16B,
 						    slot_width == 16 ?
 						    TAS5722_TDM_SLOT_16B : 0);
 		if (ret < 0)
 			goto error_snd_soc_component_update_bits;
-		break;
-	default:
-		break;
 	}
 
 	return 0;
@@ -277,7 +274,7 @@ static void tas5720_fault_check_work(struct work_struct *work)
 static int tas5720_codec_probe(struct snd_soc_component *component)
 {
 	struct tas5720_data *tas5720 = snd_soc_component_get_drvdata(component);
-	unsigned int device_id, expected_device_id;
+	unsigned int device_id;
 	int ret;
 
 	tas5720->component = component;
@@ -301,21 +298,9 @@ static int tas5720_codec_probe(struct snd_soc_component *component)
 		goto probe_fail;
 	}
 
-	switch (tas5720->devtype) {
-	case TAS5720:
-		expected_device_id = TAS5720_DEVICE_ID;
-		break;
-	case TAS5722:
-		expected_device_id = TAS5722_DEVICE_ID;
-		break;
-	default:
-		dev_err(component->dev, "unexpected private driver data\n");
-		return -EINVAL;
-	}
-
-	if (device_id != expected_device_id)
+	if (device_id != tas5720->variant->device_id)
 		dev_warn(component->dev, "wrong device ID. expected: %u read: %u\n",
-			 expected_device_id, device_id);
+			 tas5720->variant->device_id, device_id);
 
 	/* Set device to mute */
 	ret = snd_soc_component_update_bits(component, TAS5720_DIGITAL_CTRL2_REG,
@@ -637,7 +622,6 @@ static int tas5720_probe(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	struct tas5720_data *data;
-	const struct regmap_config *regmap_config;
 	int ret;
 	int i;
 
@@ -646,20 +630,10 @@ static int tas5720_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	data->tas5720_client = client;
-	data->devtype = id->driver_data;
 
-	switch (id->driver_data) {
-	case TAS5720:
-		regmap_config = &tas5720_regmap_config;
-		break;
-	case TAS5722:
-		regmap_config = &tas5722_regmap_config;
-		break;
-	default:
-		dev_err(dev, "unexpected private driver data\n");
-		return -EINVAL;
-	}
-	data->regmap = devm_regmap_init_i2c(client, regmap_config);
+	data->variant = (const struct tas5720_variant *)id->driver_data;
+
+	data->regmap = devm_regmap_init_i2c(client, data->variant->reg_config);
 	if (IS_ERR(data->regmap)) {
 		ret = PTR_ERR(data->regmap);
 		dev_err(dev, "failed to allocate register map: %d\n", ret);
@@ -678,42 +652,36 @@ static int tas5720_probe(struct i2c_client *client,
 
 	dev_set_drvdata(dev, data);
 
-	switch (id->driver_data) {
-	case TAS5720:
-		ret = devm_snd_soc_register_component(&client->dev,
-					&soc_component_dev_tas5720,
-					tas5720_dai,
-					ARRAY_SIZE(tas5720_dai));
-		break;
-	case TAS5722:
-		ret = devm_snd_soc_register_component(&client->dev,
-					&soc_component_dev_tas5722,
-					tas5720_dai,
-					ARRAY_SIZE(tas5720_dai));
-		break;
-	default:
-		dev_err(dev, "unexpected private driver data\n");
-		return -EINVAL;
-	}
-	if (ret < 0) {
-		dev_err(dev, "failed to register component: %d\n", ret);
-		return ret;
-	}
-
-	return 0;
+	ret = devm_snd_soc_register_component(&client->dev,
+					      data->variant->comp_drv,
+					      tas5720_dai,
+					      ARRAY_SIZE(tas5720_dai));
+	return ret;
 }
 
+static const struct tas5720_variant tas5720 = {
+	.device_id = TAS5720_DEVICE_ID,
+	.reg_config = &tas5720_regmap_config,
+	.comp_drv = &soc_component_dev_tas5720,
+};
+
+static const struct tas5720_variant tas5722 = {
+	.device_id = TAS5722_DEVICE_ID,
+	.reg_config = &tas5722_regmap_config,
+	.comp_drv = &soc_component_dev_tas5722,
+};
+
 static const struct i2c_device_id tas5720_id[] = {
-	{ "tas5720", TAS5720 },
-	{ "tas5722", TAS5722 },
+	{ "tas5720", (kernel_ulong_t)&tas5720 },
+	{ "tas5722", (kernel_ulong_t)&tas5722 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tas5720_id);
 
 #if IS_ENABLED(CONFIG_OF)
 static const struct of_device_id tas5720_of_match[] = {
-	{ .compatible = "ti,tas5720", },
-	{ .compatible = "ti,tas5722", },
+	{ .compatible = "ti,tas5720", .data = &tas5720, },
+	{ .compatible = "ti,tas5722", .data = &tas5722, },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, tas5720_of_match);
-- 
2.17.1

[PATCH v2 2/2] ASoC: tas5720.c: add ACPI enumeration support

[Nikolaus Voss]

Add ACPI IDs for tas5720 and tas5722 and use device match API
to determine the variant.

Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
---
 sound/soc/codecs/tas5720.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c
index b2e897f094b4..e3f85926cac4 100644
--- a/sound/soc/codecs/tas5720.c
+++ b/sound/soc/codecs/tas5720.c
@@ -7,6 +7,7 @@
  * Author: Andreas Dannenberg <dannenberg@ti.com>
  */
 
+#include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/errno.h>
 #include <linux/device.h>
@@ -631,7 +632,9 @@ static int tas5720_probe(struct i2c_client *client,
 
 	data->tas5720_client = client;
 
-	data->variant = (const struct tas5720_variant *)id->driver_data;
+	data->variant = device_get_match_data(&client->dev);
+	if (!data->variant)
+		data->variant = (const struct tas5720_variant *)id->driver_data;
 
 	data->regmap = devm_regmap_init_i2c(client, data->variant->reg_config);
 	if (IS_ERR(data->regmap)) {
@@ -687,10 +690,20 @@ static const struct of_device_id tas5720_of_match[] = {
 MODULE_DEVICE_TABLE(of, tas5720_of_match);
 #endif
 
+#if IS_ENABLED(CONFIG_ACPI)
+static const struct acpi_device_id tas5720_acpi_match[] = {
+	{ "10TI5720", (kernel_ulong_t)&tas5720 },
+	{ "10TI5722", (kernel_ulong_t)&tas5722 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, tas5720_acpi_match);
+#endif
+
 static struct i2c_driver tas5720_i2c_driver = {
 	.driver = {
 		.name = "tas5720",
 		.of_match_table = of_match_ptr(tas5720_of_match),
+		.acpi_match_table = ACPI_PTR(tas5720_acpi_match),
 	},
 	.probe = tas5720_probe,
 	.id_table = tas5720_id,
-- 
2.17.1

Re: [PATCH v2 1/2] ASoC: tas5720.c: cleanup variant management

[Andrew F. Davis]

On 7/1/19 9:42 AM, Nikolaus Voss wrote:
> Replace enum tas572x_type with struct tas5720_variant which aggregates
> variant specific stuff and can be directly referenced from an id table.
> 
> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
> ---
>  sound/soc/codecs/tas5720.c | 98 +++++++++++++-------------------------
>  1 file changed, 33 insertions(+), 65 deletions(-)
> 
> diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c
> index 37fab8f22800..b2e897f094b4 100644
> --- a/sound/soc/codecs/tas5720.c
> +++ b/sound/soc/codecs/tas5720.c
> @@ -28,9 +28,10 @@
>  /* Define how often to check (and clear) the fault status register (in ms) */
>  #define TAS5720_FAULT_CHECK_INTERVAL		200
>  
> -enum tas572x_type {
> -	TAS5720,
> -	TAS5722,
> +struct tas5720_variant {
> +	const int device_id;
> +	const struct regmap_config *reg_config;
> +	const struct snd_soc_component_driver *comp_drv;
>  };
>  
>  static const char * const tas5720_supply_names[] = {
> @@ -44,7 +45,7 @@ struct tas5720_data {
>  	struct snd_soc_component *component;
>  	struct regmap *regmap;
>  	struct i2c_client *tas5720_client;
> -	enum tas572x_type devtype;
> +	const struct tas5720_variant *variant;

Why add a new struct? Actually I don't see the need for this patch at
all, the commit message only explains the 'what' not the 'why'. We can
and do already build this info from the tas572x_type.

Also below are several functional changes, the cover letter says this is
not a functional change, yet the driver behaves differently now.

Andrew

>  	struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
>  	struct delayed_work fault_check_work;
>  	unsigned int last_fault;
> @@ -179,17 +180,13 @@ static int tas5720_set_dai_tdm_slot(struct snd_soc_dai *dai,
>  		goto error_snd_soc_component_update_bits;
>  
>  	/* Configure TDM slot width. This is only applicable to TAS5722. */
> -	switch (tas5720->devtype) {
> -	case TAS5722:
> +	if (tas5720->variant->device_id == TAS5722_DEVICE_ID) {
>  		ret = snd_soc_component_update_bits(component, TAS5722_DIGITAL_CTRL2_REG,
>  						    TAS5722_TDM_SLOT_16B,
>  						    slot_width == 16 ?
>  						    TAS5722_TDM_SLOT_16B : 0);
>  		if (ret < 0)
>  			goto error_snd_soc_component_update_bits;
> -		break;
> -	default:
> -		break;
>  	}
>  
>  	return 0;
> @@ -277,7 +274,7 @@ static void tas5720_fault_check_work(struct work_struct *work)
>  static int tas5720_codec_probe(struct snd_soc_component *component)
>  {
>  	struct tas5720_data *tas5720 = snd_soc_component_get_drvdata(component);
> -	unsigned int device_id, expected_device_id;
> +	unsigned int device_id;
>  	int ret;
>  
>  	tas5720->component = component;
> @@ -301,21 +298,9 @@ static int tas5720_codec_probe(struct snd_soc_component *component)
>  		goto probe_fail;
>  	}
>  
> -	switch (tas5720->devtype) {
> -	case TAS5720:
> -		expected_device_id = TAS5720_DEVICE_ID;
> -		break;
> -	case TAS5722:
> -		expected_device_id = TAS5722_DEVICE_ID;
> -		break;
> -	default:
> -		dev_err(component->dev, "unexpected private driver data\n");
> -		return -EINVAL;
> -	}
> -
> -	if (device_id != expected_device_id)
> +	if (device_id != tas5720->variant->device_id)
>  		dev_warn(component->dev, "wrong device ID. expected: %u read: %u\n",
> -			 expected_device_id, device_id);
> +			 tas5720->variant->device_id, device_id);
>  
>  	/* Set device to mute */
>  	ret = snd_soc_component_update_bits(component, TAS5720_DIGITAL_CTRL2_REG,
> @@ -637,7 +622,6 @@ static int tas5720_probe(struct i2c_client *client,
>  {
>  	struct device *dev = &client->dev;
>  	struct tas5720_data *data;
> -	const struct regmap_config *regmap_config;
>  	int ret;
>  	int i;
>  
> @@ -646,20 +630,10 @@ static int tas5720_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  
>  	data->tas5720_client = client;
> -	data->devtype = id->driver_data;
>  
> -	switch (id->driver_data) {
> -	case TAS5720:
> -		regmap_config = &tas5720_regmap_config;
> -		break;
> -	case TAS5722:
> -		regmap_config = &tas5722_regmap_config;
> -		break;
> -	default:
> -		dev_err(dev, "unexpected private driver data\n");
> -		return -EINVAL;
> -	}
> -	data->regmap = devm_regmap_init_i2c(client, regmap_config);
> +	data->variant = (const struct tas5720_variant *)id->driver_data;
> +
> +	data->regmap = devm_regmap_init_i2c(client, data->variant->reg_config);
>  	if (IS_ERR(data->regmap)) {
>  		ret = PTR_ERR(data->regmap);
>  		dev_err(dev, "failed to allocate register map: %d\n", ret);
> @@ -678,42 +652,36 @@ static int tas5720_probe(struct i2c_client *client,
>  
>  	dev_set_drvdata(dev, data);
>  
> -	switch (id->driver_data) {
> -	case TAS5720:
> -		ret = devm_snd_soc_register_component(&client->dev,
> -					&soc_component_dev_tas5720,
> -					tas5720_dai,
> -					ARRAY_SIZE(tas5720_dai));
> -		break;
> -	case TAS5722:
> -		ret = devm_snd_soc_register_component(&client->dev,
> -					&soc_component_dev_tas5722,
> -					tas5720_dai,
> -					ARRAY_SIZE(tas5720_dai));
> -		break;
> -	default:
> -		dev_err(dev, "unexpected private driver data\n");
> -		return -EINVAL;
> -	}
> -	if (ret < 0) {
> -		dev_err(dev, "failed to register component: %d\n", ret);
> -		return ret;
> -	}
> -
> -	return 0;
> +	ret = devm_snd_soc_register_component(&client->dev,
> +					      data->variant->comp_drv,
> +					      tas5720_dai,
> +					      ARRAY_SIZE(tas5720_dai));
> +	return ret;
>  }
>  
> +static const struct tas5720_variant tas5720 = {
> +	.device_id = TAS5720_DEVICE_ID,
> +	.reg_config = &tas5720_regmap_config,
> +	.comp_drv = &soc_component_dev_tas5720,
> +};
> +
> +static const struct tas5720_variant tas5722 = {
> +	.device_id = TAS5722_DEVICE_ID,
> +	.reg_config = &tas5722_regmap_config,
> +	.comp_drv = &soc_component_dev_tas5722,
> +};
> +
>  static const struct i2c_device_id tas5720_id[] = {
> -	{ "tas5720", TAS5720 },
> -	{ "tas5722", TAS5722 },
> +	{ "tas5720", (kernel_ulong_t)&tas5720 },
> +	{ "tas5722", (kernel_ulong_t)&tas5722 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, tas5720_id);
>  
>  #if IS_ENABLED(CONFIG_OF)
>  static const struct of_device_id tas5720_of_match[] = {
> -	{ .compatible = "ti,tas5720", },
> -	{ .compatible = "ti,tas5722", },
> +	{ .compatible = "ti,tas5720", .data = &tas5720, },
> +	{ .compatible = "ti,tas5722", .data = &tas5722, },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, tas5720_of_match);
> 

Re: [PATCH v2 1/2] ASoC: tas5720.c: cleanup variant management

[Nikolaus Voss]

On Mon, 1 Jul 2019, Andrew F. Davis wrote:
> On 7/1/19 9:42 AM, Nikolaus Voss wrote:
>> Replace enum tas572x_type with struct tas5720_variant which aggregates
>> variant specific stuff and can be directly referenced from an id table.
>>
>> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
>> ---
>>  sound/soc/codecs/tas5720.c | 98 +++++++++++++-------------------------
>>  1 file changed, 33 insertions(+), 65 deletions(-)
>>
>> diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c
>> index 37fab8f22800..b2e897f094b4 100644
>> --- a/sound/soc/codecs/tas5720.c
>> +++ b/sound/soc/codecs/tas5720.c
>> @@ -28,9 +28,10 @@
>>  /* Define how often to check (and clear) the fault status register (in ms) */
>>  #define TAS5720_FAULT_CHECK_INTERVAL		200
>>
>> -enum tas572x_type {
>> -	TAS5720,
>> -	TAS5722,
>> +struct tas5720_variant {
>> +	const int device_id;
>> +	const struct regmap_config *reg_config;
>> +	const struct snd_soc_component_driver *comp_drv;
>>  };
>>
>>  static const char * const tas5720_supply_names[] = {
>> @@ -44,7 +45,7 @@ struct tas5720_data {
>>  	struct snd_soc_component *component;
>>  	struct regmap *regmap;
>>  	struct i2c_client *tas5720_client;
>> -	enum tas572x_type devtype;
>> +	const struct tas5720_variant *variant;
>
> Why add a new struct? Actually I don't see the need for this patch at
> all, the commit message only explains the 'what' not the 'why'. We can
> and do already build this info from the tas572x_type.

As the commit message says, the purpose is to aggregate the variant 
specifics and make it accessible via one pointer. This is a standard 
approach for of/acpi_device_id tables and thus makes the code simpler and 
improves readability. This is a maintenance patch to prepare using the 
device match API in a proper way.

>
> Also below are several functional changes, the cover letter says this is
> not a functional change, yet the driver behaves differently now.

Can you be a little bit more specific? The code should behave exactly as 
before.

Niko

>
> Andrew
>
>>  	struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
>>  	struct delayed_work fault_check_work;
>>  	unsigned int last_fault;
>> @@ -179,17 +180,13 @@ static int tas5720_set_dai_tdm_slot(struct snd_soc_dai *dai,
>>  		goto error_snd_soc_component_update_bits;
>>
>>  	/* Configure TDM slot width. This is only applicable to TAS5722. */
>> -	switch (tas5720->devtype) {
>> -	case TAS5722:
>> +	if (tas5720->variant->device_id == TAS5722_DEVICE_ID) {
>>  		ret = snd_soc_component_update_bits(component, TAS5722_DIGITAL_CTRL2_REG,
>>  						    TAS5722_TDM_SLOT_16B,
>>  						    slot_width == 16 ?
>>  						    TAS5722_TDM_SLOT_16B : 0);
>>  		if (ret < 0)
>>  			goto error_snd_soc_component_update_bits;
>> -		break;
>> -	default:
>> -		break;
>>  	}
>>
>>  	return 0;
>> @@ -277,7 +274,7 @@ static void tas5720_fault_check_work(struct work_struct *work)
>>  static int tas5720_codec_probe(struct snd_soc_component *component)
>>  {
>>  	struct tas5720_data *tas5720 = snd_soc_component_get_drvdata(component);
>> -	unsigned int device_id, expected_device_id;
>> +	unsigned int device_id;
>>  	int ret;
>>
>>  	tas5720->component = component;
>> @@ -301,21 +298,9 @@ static int tas5720_codec_probe(struct snd_soc_component *component)
>>  		goto probe_fail;
>>  	}
>>
>> -	switch (tas5720->devtype) {
>> -	case TAS5720:
>> -		expected_device_id = TAS5720_DEVICE_ID;
>> -		break;
>> -	case TAS5722:
>> -		expected_device_id = TAS5722_DEVICE_ID;
>> -		break;
>> -	default:
>> -		dev_err(component->dev, "unexpected private driver data\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	if (device_id != expected_device_id)
>> +	if (device_id != tas5720->variant->device_id)
>>  		dev_warn(component->dev, "wrong device ID. expected: %u read: %u\n",
>> -			 expected_device_id, device_id);
>> +			 tas5720->variant->device_id, device_id);
>>
>>  	/* Set device to mute */
>>  	ret = snd_soc_component_update_bits(component, TAS5720_DIGITAL_CTRL2_REG,
>> @@ -637,7 +622,6 @@ static int tas5720_probe(struct i2c_client *client,
>>  {
>>  	struct device *dev = &client->dev;
>>  	struct tas5720_data *data;
>> -	const struct regmap_config *regmap_config;
>>  	int ret;
>>  	int i;
>>
>> @@ -646,20 +630,10 @@ static int tas5720_probe(struct i2c_client *client,
>>  		return -ENOMEM;
>>
>>  	data->tas5720_client = client;
>> -	data->devtype = id->driver_data;
>>
>> -	switch (id->driver_data) {
>> -	case TAS5720:
>> -		regmap_config = &tas5720_regmap_config;
>> -		break;
>> -	case TAS5722:
>> -		regmap_config = &tas5722_regmap_config;
>> -		break;
>> -	default:
>> -		dev_err(dev, "unexpected private driver data\n");
>> -		return -EINVAL;
>> -	}
>> -	data->regmap = devm_regmap_init_i2c(client, regmap_config);
>> +	data->variant = (const struct tas5720_variant *)id->driver_data;
>> +
>> +	data->regmap = devm_regmap_init_i2c(client, data->variant->reg_config);
>>  	if (IS_ERR(data->regmap)) {
>>  		ret = PTR_ERR(data->regmap);
>>  		dev_err(dev, "failed to allocate register map: %d\n", ret);
>> @@ -678,42 +652,36 @@ static int tas5720_probe(struct i2c_client *client,
>>
>>  	dev_set_drvdata(dev, data);
>>
>> -	switch (id->driver_data) {
>> -	case TAS5720:
>> -		ret = devm_snd_soc_register_component(&client->dev,
>> -					&soc_component_dev_tas5720,
>> -					tas5720_dai,
>> -					ARRAY_SIZE(tas5720_dai));
>> -		break;
>> -	case TAS5722:
>> -		ret = devm_snd_soc_register_component(&client->dev,
>> -					&soc_component_dev_tas5722,
>> -					tas5720_dai,
>> -					ARRAY_SIZE(tas5720_dai));
>> -		break;
>> -	default:
>> -		dev_err(dev, "unexpected private driver data\n");
>> -		return -EINVAL;
>> -	}
>> -	if (ret < 0) {
>> -		dev_err(dev, "failed to register component: %d\n", ret);
>> -		return ret;
>> -	}
>> -
>> -	return 0;
>> +	ret = devm_snd_soc_register_component(&client->dev,
>> +					      data->variant->comp_drv,
>> +					      tas5720_dai,
>> +					      ARRAY_SIZE(tas5720_dai));
>> +	return ret;
>>  }
>>
>> +static const struct tas5720_variant tas5720 = {
>> +	.device_id = TAS5720_DEVICE_ID,
>> +	.reg_config = &tas5720_regmap_config,
>> +	.comp_drv = &soc_component_dev_tas5720,
>> +};
>> +
>> +static const struct tas5720_variant tas5722 = {
>> +	.device_id = TAS5722_DEVICE_ID,
>> +	.reg_config = &tas5722_regmap_config,
>> +	.comp_drv = &soc_component_dev_tas5722,
>> +};
>> +
>>  static const struct i2c_device_id tas5720_id[] = {
>> -	{ "tas5720", TAS5720 },
>> -	{ "tas5722", TAS5722 },
>> +	{ "tas5720", (kernel_ulong_t)&tas5720 },
>> +	{ "tas5722", (kernel_ulong_t)&tas5722 },
>>  	{ }
>>  };
>>  MODULE_DEVICE_TABLE(i2c, tas5720_id);
>>
>>  #if IS_ENABLED(CONFIG_OF)
>>  static const struct of_device_id tas5720_of_match[] = {
>> -	{ .compatible = "ti,tas5720", },
>> -	{ .compatible = "ti,tas5722", },
>> +	{ .compatible = "ti,tas5720", .data = &tas5720, },
>> +	{ .compatible = "ti,tas5722", .data = &tas5722, },
>>  	{ },
>>  };
>>  MODULE_DEVICE_TABLE(of, tas5720_of_match);
>>
>

Re: [PATCH v2 1/2] ASoC: tas5720.c: cleanup variant management

[Andrew F. Davis]

On 7/1/19 11:35 AM, Nikolaus Voss wrote:
> On Mon, 1 Jul 2019, Andrew F. Davis wrote:
>> On 7/1/19 9:42 AM, Nikolaus Voss wrote:
>>> Replace enum tas572x_type with struct tas5720_variant which aggregates
>>> variant specific stuff and can be directly referenced from an id table.
>>>
>>> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
>>> ---
>>>  sound/soc/codecs/tas5720.c | 98 +++++++++++++-------------------------
>>>  1 file changed, 33 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c
>>> index 37fab8f22800..b2e897f094b4 100644
>>> --- a/sound/soc/codecs/tas5720.c
>>> +++ b/sound/soc/codecs/tas5720.c
>>> @@ -28,9 +28,10 @@
>>>  /* Define how often to check (and clear) the fault status register
>>> (in ms) */
>>>  #define TAS5720_FAULT_CHECK_INTERVAL        200
>>>
>>> -enum tas572x_type {
>>> -    TAS5720,
>>> -    TAS5722,
>>> +struct tas5720_variant {
>>> +    const int device_id;
>>> +    const struct regmap_config *reg_config;
>>> +    const struct snd_soc_component_driver *comp_drv;
>>>  };
>>>
>>>  static const char * const tas5720_supply_names[] = {
>>> @@ -44,7 +45,7 @@ struct tas5720_data {
>>>      struct snd_soc_component *component;
>>>      struct regmap *regmap;
>>>      struct i2c_client *tas5720_client;
>>> -    enum tas572x_type devtype;
>>> +    const struct tas5720_variant *variant;
>>
>> Why add a new struct? Actually I don't see the need for this patch at
>> all, the commit message only explains the 'what' not the 'why'. We can
>> and do already build this info from the tas572x_type.
> 
> As the commit message says, the purpose is to aggregate the variant
> specifics and make it accessible via one pointer. This is a standard
> approach for of/acpi_device_id tables and thus makes the code simpler
> and improves readability. This is a maintenance patch to prepare using
> the device match API in a proper way.
> 


"make it accessible via one pointer" is again a "what", the "why" is:

"This is a standard approach"
"makes the code simpler and improves readability"

Those are valid reasons and should be what you put in the commit message.


>>
>> Also below are several functional changes, the cover letter says this is
>> not a functional change, yet the driver behaves differently now.
> 
> Can you be a little bit more specific? The code should behave exactly as
> before.
> 


Sure, for instance the line "unexpected private driver data" is removed,
this can now never happen, that is a functional change. The phrase "no
functional change", should be reserved for only changes to spelling,
formatting, code organizing, etc..


> Niko
> 
>>
>> Andrew
>>
>>>      struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
>>>      struct delayed_work fault_check_work;
>>>      unsigned int last_fault;
>>> @@ -179,17 +180,13 @@ static int tas5720_set_dai_tdm_slot(struct
>>> snd_soc_dai *dai,
>>>          goto error_snd_soc_component_update_bits;
>>>
>>>      /* Configure TDM slot width. This is only applicable to TAS5722. */
>>> -    switch (tas5720->devtype) {
>>> -    case TAS5722:
>>> +    if (tas5720->variant->device_id == TAS5722_DEVICE_ID) {


I also don't like this, TAS5722_DEVICE_ID is the expected contents of a
register, you are using it like the enum tas572x_type that you removed.
I'd leave that enum, the device ID register itself is not guaranteed to
be correct or unique, which is why we warn about mismatches below but
then continue to use the user provided device type anyway.

Andrew


>>>          ret = snd_soc_component_update_bits(component,
>>> TAS5722_DIGITAL_CTRL2_REG,
>>>                              TAS5722_TDM_SLOT_16B,
>>>                              slot_width == 16 ?
>>>                              TAS5722_TDM_SLOT_16B : 0);
>>>          if (ret < 0)
>>>              goto error_snd_soc_component_update_bits;
>>> -        break;
>>> -    default:
>>> -        break;
>>>      }
>>>
>>>      return 0;
>>> @@ -277,7 +274,7 @@ static void tas5720_fault_check_work(struct
>>> work_struct *work)
>>>  static int tas5720_codec_probe(struct snd_soc_component *component)
>>>  {
>>>      struct tas5720_data *tas5720 =
>>> snd_soc_component_get_drvdata(component);
>>> -    unsigned int device_id, expected_device_id;
>>> +    unsigned int device_id;
>>>      int ret;
>>>
>>>      tas5720->component = component;
>>> @@ -301,21 +298,9 @@ static int tas5720_codec_probe(struct
>>> snd_soc_component *component)
>>>          goto probe_fail;
>>>      }
>>>
>>> -    switch (tas5720->devtype) {
>>> -    case TAS5720:
>>> -        expected_device_id = TAS5720_DEVICE_ID;
>>> -        break;
>>> -    case TAS5722:
>>> -        expected_device_id = TAS5722_DEVICE_ID;
>>> -        break;
>>> -    default:
>>> -        dev_err(component->dev, "unexpected private driver data\n");
>>> -        return -EINVAL;
>>> -    }
>>> -
>>> -    if (device_id != expected_device_id)
>>> +    if (device_id != tas5720->variant->device_id)
>>>          dev_warn(component->dev, "wrong device ID. expected: %u
>>> read: %u\n",
>>> -             expected_device_id, device_id);
>>> +             tas5720->variant->device_id, device_id);
>>>
>>>      /* Set device to mute */
>>>      ret = snd_soc_component_update_bits(component,
>>> TAS5720_DIGITAL_CTRL2_REG,
>>> @@ -637,7 +622,6 @@ static int tas5720_probe(struct i2c_client *client,
>>>  {
>>>      struct device *dev = &client->dev;
>>>      struct tas5720_data *data;
>>> -    const struct regmap_config *regmap_config;
>>>      int ret;
>>>      int i;
>>>
>>> @@ -646,20 +630,10 @@ static int tas5720_probe(struct i2c_client
>>> *client,
>>>          return -ENOMEM;
>>>
>>>      data->tas5720_client = client;
>>> -    data->devtype = id->driver_data;
>>>
>>> -    switch (id->driver_data) {
>>> -    case TAS5720:
>>> -        regmap_config = &tas5720_regmap_config;
>>> -        break;
>>> -    case TAS5722:
>>> -        regmap_config = &tas5722_regmap_config;
>>> -        break;
>>> -    default:
>>> -        dev_err(dev, "unexpected private driver data\n");
>>> -        return -EINVAL;
>>> -    }
>>> -    data->regmap = devm_regmap_init_i2c(client, regmap_config);
>>> +    data->variant = (const struct tas5720_variant *)id->driver_data;
>>> +
>>> +    data->regmap = devm_regmap_init_i2c(client,
>>> data->variant->reg_config);
>>>      if (IS_ERR(data->regmap)) {
>>>          ret = PTR_ERR(data->regmap);
>>>          dev_err(dev, "failed to allocate register map: %d\n", ret);
>>> @@ -678,42 +652,36 @@ static int tas5720_probe(struct i2c_client
>>> *client,
>>>
>>>      dev_set_drvdata(dev, data);
>>>
>>> -    switch (id->driver_data) {
>>> -    case TAS5720:
>>> -        ret = devm_snd_soc_register_component(&client->dev,
>>> -                    &soc_component_dev_tas5720,
>>> -                    tas5720_dai,
>>> -                    ARRAY_SIZE(tas5720_dai));
>>> -        break;
>>> -    case TAS5722:
>>> -        ret = devm_snd_soc_register_component(&client->dev,
>>> -                    &soc_component_dev_tas5722,
>>> -                    tas5720_dai,
>>> -                    ARRAY_SIZE(tas5720_dai));
>>> -        break;
>>> -    default:
>>> -        dev_err(dev, "unexpected private driver data\n");
>>> -        return -EINVAL;
>>> -    }
>>> -    if (ret < 0) {
>>> -        dev_err(dev, "failed to register component: %d\n", ret);
>>> -        return ret;
>>> -    }
>>> -
>>> -    return 0;
>>> +    ret = devm_snd_soc_register_component(&client->dev,
>>> +                          data->variant->comp_drv,
>>> +                          tas5720_dai,
>>> +                          ARRAY_SIZE(tas5720_dai));
>>> +    return ret;
>>>  }
>>>
>>> +static const struct tas5720_variant tas5720 = {
>>> +    .device_id = TAS5720_DEVICE_ID,
>>> +    .reg_config = &tas5720_regmap_config,
>>> +    .comp_drv = &soc_component_dev_tas5720,
>>> +};
>>> +
>>> +static const struct tas5720_variant tas5722 = {
>>> +    .device_id = TAS5722_DEVICE_ID,
>>> +    .reg_config = &tas5722_regmap_config,
>>> +    .comp_drv = &soc_component_dev_tas5722,
>>> +};
>>> +
>>>  static const struct i2c_device_id tas5720_id[] = {
>>> -    { "tas5720", TAS5720 },
>>> -    { "tas5722", TAS5722 },
>>> +    { "tas5720", (kernel_ulong_t)&tas5720 },
>>> +    { "tas5722", (kernel_ulong_t)&tas5722 },
>>>      { }
>>>  };
>>>  MODULE_DEVICE_TABLE(i2c, tas5720_id);
>>>
>>>  #if IS_ENABLED(CONFIG_OF)
>>>  static const struct of_device_id tas5720_of_match[] = {
>>> -    { .compatible = "ti,tas5720", },
>>> -    { .compatible = "ti,tas5722", },
>>> +    { .compatible = "ti,tas5720", .data = &tas5720, },
>>> +    { .compatible = "ti,tas5722", .data = &tas5722, },
>>>      { },
>>>  };
>>>  MODULE_DEVICE_TABLE(of, tas5720_of_match);
>>>
>>

Re: [PATCH v2 1/2] ASoC: tas5720.c: cleanup variant management

[Nikolaus Voss]

[-- Attachment #1: Type: text/plain, Size: 10098 bytes --]

On Mon, 1 Jul 2019, Andrew F. Davis wrote:
> On 7/1/19 11:35 AM, Nikolaus Voss wrote:
>> On Mon, 1 Jul 2019, Andrew F. Davis wrote:
>>> On 7/1/19 9:42 AM, Nikolaus Voss wrote:
>>>> Replace enum tas572x_type with struct tas5720_variant which aggregates
>>>> variant specific stuff and can be directly referenced from an id table.
>>>>
>>>> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
>>>> ---
>>>>  sound/soc/codecs/tas5720.c | 98 +++++++++++++-------------------------
>>>>  1 file changed, 33 insertions(+), 65 deletions(-)
>>>>
>>>> diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c
>>>> index 37fab8f22800..b2e897f094b4 100644
>>>> --- a/sound/soc/codecs/tas5720.c
>>>> +++ b/sound/soc/codecs/tas5720.c
>>>> @@ -28,9 +28,10 @@
>>>>  /* Define how often to check (and clear) the fault status register
>>>> (in ms) */
>>>>  #define TAS5720_FAULT_CHECK_INTERVAL        200
>>>>
>>>> -enum tas572x_type {
>>>> -    TAS5720,
>>>> -    TAS5722,
>>>> +struct tas5720_variant {
>>>> +    const int device_id;
>>>> +    const struct regmap_config *reg_config;
>>>> +    const struct snd_soc_component_driver *comp_drv;
>>>>  };
>>>>
>>>>  static const char * const tas5720_supply_names[] = {
>>>> @@ -44,7 +45,7 @@ struct tas5720_data {
>>>>      struct snd_soc_component *component;
>>>>      struct regmap *regmap;
>>>>      struct i2c_client *tas5720_client;
>>>> -    enum tas572x_type devtype;
>>>> +    const struct tas5720_variant *variant;
>>>
>>> Why add a new struct? Actually I don't see the need for this patch at
>>> all, the commit message only explains the 'what' not the 'why'. We can
>>> and do already build this info from the tas572x_type.
>>
>> As the commit message says, the purpose is to aggregate the variant
>> specifics and make it accessible via one pointer. This is a standard
>> approach for of/acpi_device_id tables and thus makes the code simpler
>> and improves readability. This is a maintenance patch to prepare using
>> the device match API in a proper way.
>>
>
>
> "make it accessible via one pointer" is again a "what", the "why" is:
>
> "This is a standard approach"
> "makes the code simpler and improves readability"
>
> Those are valid reasons and should be what you put in the commit message.

ok

>
>
>>>
>>> Also below are several functional changes, the cover letter says this is
>>> not a functional change, yet the driver behaves differently now.
>>
>> Can you be a little bit more specific? The code should behave exactly as
>> before.
>>
>
>
> Sure, for instance the line "unexpected private driver data" is removed,
> this can now never happen, that is a functional change. The phrase "no
> functional change", should be reserved for only changes to spelling,
> formatting, code organizing, etc..

"unexpected private driver data" was unreachable code before, but you're 
right, debug output has changed a little, but the functional part is 
exactly the same.

>
>
>> Niko
>>
>>>
>>> Andrew
>>>
>>>>      struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
>>>>      struct delayed_work fault_check_work;
>>>>      unsigned int last_fault;
>>>> @@ -179,17 +180,13 @@ static int tas5720_set_dai_tdm_slot(struct
>>>> snd_soc_dai *dai,
>>>>          goto error_snd_soc_component_update_bits;
>>>>
>>>>      /* Configure TDM slot width. This is only applicable to TAS5722. */
>>>> -    switch (tas5720->devtype) {
>>>> -    case TAS5722:
>>>> +    if (tas5720->variant->device_id == TAS5722_DEVICE_ID) {
>
>
> I also don't like this, TAS5722_DEVICE_ID is the expected contents of a
> register, you are using it like the enum tas572x_type that you removed.
> I'd leave that enum, the device ID register itself is not guaranteed to
> be correct or unique, which is why we warn about mismatches below but
> then continue to use the user provided device type anyway.

Strange, with me it's the other way round, I don't like the enum. The 
mismatch behavior hasn't changed a bit, the same warning is printed. 
If the device ID is no longer unique in the future (apparently it is 
for now) the driver should explicitly handle this instead of printing a 
warning, because warnings should be reserved for an indication of any 
kind of misconfiguration and not of expected behavior.

That said the variant struct can of course replace the enum in every 
aspect, even for what you describe above. The enum was an ordinal 
representation of the user-selected i2c_device_id, the variant struct* is
a pointer representation of the user-selected i2c/of/acpi_device_id. The 
only difference is that it directly points to the variant specific parts 
of the driver instead of resolving those via multiple switch/case 
statements.

Niko

>
> Andrew
>
>
>>>>          ret = snd_soc_component_update_bits(component,
>>>> TAS5722_DIGITAL_CTRL2_REG,
>>>>                              TAS5722_TDM_SLOT_16B,
>>>>                              slot_width == 16 ?
>>>>                              TAS5722_TDM_SLOT_16B : 0);
>>>>          if (ret < 0)
>>>>              goto error_snd_soc_component_update_bits;
>>>> -        break;
>>>> -    default:
>>>> -        break;
>>>>      }
>>>>
>>>>      return 0;
>>>> @@ -277,7 +274,7 @@ static void tas5720_fault_check_work(struct
>>>> work_struct *work)
>>>>  static int tas5720_codec_probe(struct snd_soc_component *component)
>>>>  {
>>>>      struct tas5720_data *tas5720 =
>>>> snd_soc_component_get_drvdata(component);
>>>> -    unsigned int device_id, expected_device_id;
>>>> +    unsigned int device_id;
>>>>      int ret;
>>>>
>>>>      tas5720->component = component;
>>>> @@ -301,21 +298,9 @@ static int tas5720_codec_probe(struct
>>>> snd_soc_component *component)
>>>>          goto probe_fail;
>>>>      }
>>>>
>>>> -    switch (tas5720->devtype) {
>>>> -    case TAS5720:
>>>> -        expected_device_id = TAS5720_DEVICE_ID;
>>>> -        break;
>>>> -    case TAS5722:
>>>> -        expected_device_id = TAS5722_DEVICE_ID;
>>>> -        break;
>>>> -    default:
>>>> -        dev_err(component->dev, "unexpected private driver data\n");
>>>> -        return -EINVAL;
>>>> -    }
>>>> -
>>>> -    if (device_id != expected_device_id)
>>>> +    if (device_id != tas5720->variant->device_id)
>>>>          dev_warn(component->dev, "wrong device ID. expected: %u
>>>> read: %u\n",
>>>> -             expected_device_id, device_id);
>>>> +             tas5720->variant->device_id, device_id);
>>>>
>>>>      /* Set device to mute */
>>>>      ret = snd_soc_component_update_bits(component,
>>>> TAS5720_DIGITAL_CTRL2_REG,
>>>> @@ -637,7 +622,6 @@ static int tas5720_probe(struct i2c_client *client,
>>>>  {
>>>>      struct device *dev = &client->dev;
>>>>      struct tas5720_data *data;
>>>> -    const struct regmap_config *regmap_config;
>>>>      int ret;
>>>>      int i;
>>>>
>>>> @@ -646,20 +630,10 @@ static int tas5720_probe(struct i2c_client
>>>> *client,
>>>>          return -ENOMEM;
>>>>
>>>>      data->tas5720_client = client;
>>>> -    data->devtype = id->driver_data;
>>>>
>>>> -    switch (id->driver_data) {
>>>> -    case TAS5720:
>>>> -        regmap_config = &tas5720_regmap_config;
>>>> -        break;
>>>> -    case TAS5722:
>>>> -        regmap_config = &tas5722_regmap_config;
>>>> -        break;
>>>> -    default:
>>>> -        dev_err(dev, "unexpected private driver data\n");
>>>> -        return -EINVAL;
>>>> -    }
>>>> -    data->regmap = devm_regmap_init_i2c(client, regmap_config);
>>>> +    data->variant = (const struct tas5720_variant *)id->driver_data;
>>>> +
>>>> +    data->regmap = devm_regmap_init_i2c(client,
>>>> data->variant->reg_config);
>>>>      if (IS_ERR(data->regmap)) {
>>>>          ret = PTR_ERR(data->regmap);
>>>>          dev_err(dev, "failed to allocate register map: %d\n", ret);
>>>> @@ -678,42 +652,36 @@ static int tas5720_probe(struct i2c_client
>>>> *client,
>>>>
>>>>      dev_set_drvdata(dev, data);
>>>>
>>>> -    switch (id->driver_data) {
>>>> -    case TAS5720:
>>>> -        ret = devm_snd_soc_register_component(&client->dev,
>>>> -                    &soc_component_dev_tas5720,
>>>> -                    tas5720_dai,
>>>> -                    ARRAY_SIZE(tas5720_dai));
>>>> -        break;
>>>> -    case TAS5722:
>>>> -        ret = devm_snd_soc_register_component(&client->dev,
>>>> -                    &soc_component_dev_tas5722,
>>>> -                    tas5720_dai,
>>>> -                    ARRAY_SIZE(tas5720_dai));
>>>> -        break;
>>>> -    default:
>>>> -        dev_err(dev, "unexpected private driver data\n");
>>>> -        return -EINVAL;
>>>> -    }
>>>> -    if (ret < 0) {
>>>> -        dev_err(dev, "failed to register component: %d\n", ret);
>>>> -        return ret;
>>>> -    }
>>>> -
>>>> -    return 0;
>>>> +    ret = devm_snd_soc_register_component(&client->dev,
>>>> +                          data->variant->comp_drv,
>>>> +                          tas5720_dai,
>>>> +                          ARRAY_SIZE(tas5720_dai));
>>>> +    return ret;
>>>>  }
>>>>
>>>> +static const struct tas5720_variant tas5720 = {
>>>> +    .device_id = TAS5720_DEVICE_ID,
>>>> +    .reg_config = &tas5720_regmap_config,
>>>> +    .comp_drv = &soc_component_dev_tas5720,
>>>> +};
>>>> +
>>>> +static const struct tas5720_variant tas5722 = {
>>>> +    .device_id = TAS5722_DEVICE_ID,
>>>> +    .reg_config = &tas5722_regmap_config,
>>>> +    .comp_drv = &soc_component_dev_tas5722,
>>>> +};
>>>> +
>>>>  static const struct i2c_device_id tas5720_id[] = {
>>>> -    { "tas5720", TAS5720 },
>>>> -    { "tas5722", TAS5722 },
>>>> +    { "tas5720", (kernel_ulong_t)&tas5720 },
>>>> +    { "tas5722", (kernel_ulong_t)&tas5722 },
>>>>      { }
>>>>  };
>>>>  MODULE_DEVICE_TABLE(i2c, tas5720_id);
>>>>
>>>>  #if IS_ENABLED(CONFIG_OF)
>>>>  static const struct of_device_id tas5720_of_match[] = {
>>>> -    { .compatible = "ti,tas5720", },
>>>> -    { .compatible = "ti,tas5722", },
>>>> +    { .compatible = "ti,tas5720", .data = &tas5720, },
>>>> +    { .compatible = "ti,tas5722", .data = &tas5722, },
>>>>      { },
>>>>  };
>>>>  MODULE_DEVICE_TABLE(of, tas5720_of_match);
>>>>
>>>
>

Re: [PATCH v2 1/2] ASoC: tas5720.c: cleanup variant management

[Andrew F. Davis]

On 7/2/19 6:12 AM, Nikolaus Voss wrote:
> On Mon, 1 Jul 2019, Andrew F. Davis wrote:
>> On 7/1/19 11:35 AM, Nikolaus Voss wrote:
>>> On Mon, 1 Jul 2019, Andrew F. Davis wrote:
>>>> On 7/1/19 9:42 AM, Nikolaus Voss wrote:
>>>>> Replace enum tas572x_type with struct tas5720_variant which aggregates
>>>>> variant specific stuff and can be directly referenced from an id
>>>>> table.
>>>>>
>>>>> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
>>>>> ---
>>>>>  sound/soc/codecs/tas5720.c | 98
>>>>> +++++++++++++-------------------------
>>>>>  1 file changed, 33 insertions(+), 65 deletions(-)
>>>>>
>>>>> diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c
>>>>> index 37fab8f22800..b2e897f094b4 100644
>>>>> --- a/sound/soc/codecs/tas5720.c
>>>>> +++ b/sound/soc/codecs/tas5720.c
>>>>> @@ -28,9 +28,10 @@
>>>>>  /* Define how often to check (and clear) the fault status register
>>>>> (in ms) */
>>>>>  #define TAS5720_FAULT_CHECK_INTERVAL        200
>>>>>
>>>>> -enum tas572x_type {
>>>>> -    TAS5720,
>>>>> -    TAS5722,
>>>>> +struct tas5720_variant {
>>>>> +    const int device_id;
>>>>> +    const struct regmap_config *reg_config;
>>>>> +    const struct snd_soc_component_driver *comp_drv;
>>>>>  };
>>>>>
>>>>>  static const char * const tas5720_supply_names[] = {
>>>>> @@ -44,7 +45,7 @@ struct tas5720_data {
>>>>>      struct snd_soc_component *component;
>>>>>      struct regmap *regmap;
>>>>>      struct i2c_client *tas5720_client;
>>>>> -    enum tas572x_type devtype;
>>>>> +    const struct tas5720_variant *variant;
>>>>
>>>> Why add a new struct? Actually I don't see the need for this patch at
>>>> all, the commit message only explains the 'what' not the 'why'. We can
>>>> and do already build this info from the tas572x_type.
>>>
>>> As the commit message says, the purpose is to aggregate the variant
>>> specifics and make it accessible via one pointer. This is a standard
>>> approach for of/acpi_device_id tables and thus makes the code simpler
>>> and improves readability. This is a maintenance patch to prepare using
>>> the device match API in a proper way.
>>>
>>
>>
>> "make it accessible via one pointer" is again a "what", the "why" is:
>>
>> "This is a standard approach"
>> "makes the code simpler and improves readability"
>>
>> Those are valid reasons and should be what you put in the commit message.
> 
> ok
> 
>>
>>
>>>>
>>>> Also below are several functional changes, the cover letter says
>>>> this is
>>>> not a functional change, yet the driver behaves differently now.
>>>
>>> Can you be a little bit more specific? The code should behave exactly as
>>> before.
>>>
>>
>>
>> Sure, for instance the line "unexpected private driver data" is removed,
>> this can now never happen, that is a functional change. The phrase "no
>> functional change", should be reserved for only changes to spelling,
>> formatting, code organizing, etc..
> 
> "unexpected private driver data" was unreachable code before, but you're
> right, debug output has changed a little, but the functional part is
> exactly the same.
> 
>>
>>
>>> Niko
>>>
>>>>
>>>> Andrew
>>>>
>>>>>      struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
>>>>>      struct delayed_work fault_check_work;
>>>>>      unsigned int last_fault;
>>>>> @@ -179,17 +180,13 @@ static int tas5720_set_dai_tdm_slot(struct
>>>>> snd_soc_dai *dai,
>>>>>          goto error_snd_soc_component_update_bits;
>>>>>
>>>>>      /* Configure TDM slot width. This is only applicable to
>>>>> TAS5722. */
>>>>> -    switch (tas5720->devtype) {
>>>>> -    case TAS5722:
>>>>> +    if (tas5720->variant->device_id == TAS5722_DEVICE_ID) {
>>
>>
>> I also don't like this, TAS5722_DEVICE_ID is the expected contents of a
>> register, you are using it like the enum tas572x_type that you removed.
>> I'd leave that enum, the device ID register itself is not guaranteed to
>> be correct or unique, which is why we warn about mismatches below but
>> then continue to use the user provided device type anyway.
> 
> Strange, with me it's the other way round, I don't like the enum. The
> mismatch behavior hasn't changed a bit, the same warning is printed. If
> the device ID is no longer unique in the future (apparently it is for
> now) the driver should explicitly handle this instead of printing a
> warning, because warnings should be reserved for an indication of any
> kind of misconfiguration and not of expected behavior.
> 
> That said the variant struct can of course replace the enum in every
> aspect, even for what you describe above. The enum was an ordinal
> representation of the user-selected i2c_device_id, the variant struct* is
> a pointer representation of the user-selected i2c/of/acpi_device_id. The
> only difference is that it directly points to the variant specific parts
> of the driver instead of resolving those via multiple switch/case
> statements.

The enum identifies the device type, easy as that, if you want to
instead do all the logic switching on some internal ID register value
code then make a patch for just that and explain what is gained. Don't
do that into this one.

Andrew

> 
> Niko
> 
>>
>> Andrew
>>
>>
>>>>>          ret = snd_soc_component_update_bits(component,
>>>>> TAS5722_DIGITAL_CTRL2_REG,
>>>>>                              TAS5722_TDM_SLOT_16B,
>>>>>                              slot_width == 16 ?
>>>>>                              TAS5722_TDM_SLOT_16B : 0);
>>>>>          if (ret < 0)
>>>>>              goto error_snd_soc_component_update_bits;
>>>>> -        break;
>>>>> -    default:
>>>>> -        break;
>>>>>      }
>>>>>
>>>>>      return 0;
>>>>> @@ -277,7 +274,7 @@ static void tas5720_fault_check_work(struct
>>>>> work_struct *work)
>>>>>  static int tas5720_codec_probe(struct snd_soc_component *component)
>>>>>  {
>>>>>      struct tas5720_data *tas5720 =
>>>>> snd_soc_component_get_drvdata(component);
>>>>> -    unsigned int device_id, expected_device_id;
>>>>> +    unsigned int device_id;
>>>>>      int ret;
>>>>>
>>>>>      tas5720->component = component;
>>>>> @@ -301,21 +298,9 @@ static int tas5720_codec_probe(struct
>>>>> snd_soc_component *component)
>>>>>          goto probe_fail;
>>>>>      }
>>>>>
>>>>> -    switch (tas5720->devtype) {
>>>>> -    case TAS5720:
>>>>> -        expected_device_id = TAS5720_DEVICE_ID;
>>>>> -        break;
>>>>> -    case TAS5722:
>>>>> -        expected_device_id = TAS5722_DEVICE_ID;
>>>>> -        break;
>>>>> -    default:
>>>>> -        dev_err(component->dev, "unexpected private driver data\n");
>>>>> -        return -EINVAL;
>>>>> -    }
>>>>> -
>>>>> -    if (device_id != expected_device_id)
>>>>> +    if (device_id != tas5720->variant->device_id)
>>>>>          dev_warn(component->dev, "wrong device ID. expected: %u
>>>>> read: %u\n",
>>>>> -             expected_device_id, device_id);
>>>>> +             tas5720->variant->device_id, device_id);
>>>>>
>>>>>      /* Set device to mute */
>>>>>      ret = snd_soc_component_update_bits(component,
>>>>> TAS5720_DIGITAL_CTRL2_REG,
>>>>> @@ -637,7 +622,6 @@ static int tas5720_probe(struct i2c_client
>>>>> *client,
>>>>>  {
>>>>>      struct device *dev = &client->dev;
>>>>>      struct tas5720_data *data;
>>>>> -    const struct regmap_config *regmap_config;
>>>>>      int ret;
>>>>>      int i;
>>>>>
>>>>> @@ -646,20 +630,10 @@ static int tas5720_probe(struct i2c_client
>>>>> *client,
>>>>>          return -ENOMEM;
>>>>>
>>>>>      data->tas5720_client = client;
>>>>> -    data->devtype = id->driver_data;
>>>>>
>>>>> -    switch (id->driver_data) {
>>>>> -    case TAS5720:
>>>>> -        regmap_config = &tas5720_regmap_config;
>>>>> -        break;
>>>>> -    case TAS5722:
>>>>> -        regmap_config = &tas5722_regmap_config;
>>>>> -        break;
>>>>> -    default:
>>>>> -        dev_err(dev, "unexpected private driver data\n");
>>>>> -        return -EINVAL;
>>>>> -    }
>>>>> -    data->regmap = devm_regmap_init_i2c(client, regmap_config);
>>>>> +    data->variant = (const struct tas5720_variant *)id->driver_data;
>>>>> +
>>>>> +    data->regmap = devm_regmap_init_i2c(client,
>>>>> data->variant->reg_config);
>>>>>      if (IS_ERR(data->regmap)) {
>>>>>          ret = PTR_ERR(data->regmap);
>>>>>          dev_err(dev, "failed to allocate register map: %d\n", ret);
>>>>> @@ -678,42 +652,36 @@ static int tas5720_probe(struct i2c_client
>>>>> *client,
>>>>>
>>>>>      dev_set_drvdata(dev, data);
>>>>>
>>>>> -    switch (id->driver_data) {
>>>>> -    case TAS5720:
>>>>> -        ret = devm_snd_soc_register_component(&client->dev,
>>>>> -                    &soc_component_dev_tas5720,
>>>>> -                    tas5720_dai,
>>>>> -                    ARRAY_SIZE(tas5720_dai));
>>>>> -        break;
>>>>> -    case TAS5722:
>>>>> -        ret = devm_snd_soc_register_component(&client->dev,
>>>>> -                    &soc_component_dev_tas5722,
>>>>> -                    tas5720_dai,
>>>>> -                    ARRAY_SIZE(tas5720_dai));
>>>>> -        break;
>>>>> -    default:
>>>>> -        dev_err(dev, "unexpected private driver data\n");
>>>>> -        return -EINVAL;
>>>>> -    }
>>>>> -    if (ret < 0) {
>>>>> -        dev_err(dev, "failed to register component: %d\n", ret);
>>>>> -        return ret;
>>>>> -    }
>>>>> -
>>>>> -    return 0;
>>>>> +    ret = devm_snd_soc_register_component(&client->dev,
>>>>> +                          data->variant->comp_drv,
>>>>> +                          tas5720_dai,
>>>>> +                          ARRAY_SIZE(tas5720_dai));
>>>>> +    return ret;
>>>>>  }
>>>>>
>>>>> +static const struct tas5720_variant tas5720 = {
>>>>> +    .device_id = TAS5720_DEVICE_ID,
>>>>> +    .reg_config = &tas5720_regmap_config,
>>>>> +    .comp_drv = &soc_component_dev_tas5720,
>>>>> +};
>>>>> +
>>>>> +static const struct tas5720_variant tas5722 = {
>>>>> +    .device_id = TAS5722_DEVICE_ID,
>>>>> +    .reg_config = &tas5722_regmap_config,
>>>>> +    .comp_drv = &soc_component_dev_tas5722,
>>>>> +};
>>>>> +
>>>>>  static const struct i2c_device_id tas5720_id[] = {
>>>>> -    { "tas5720", TAS5720 },
>>>>> -    { "tas5722", TAS5722 },
>>>>> +    { "tas5720", (kernel_ulong_t)&tas5720 },
>>>>> +    { "tas5722", (kernel_ulong_t)&tas5722 },
>>>>>      { }
>>>>>  };
>>>>>  MODULE_DEVICE_TABLE(i2c, tas5720_id);
>>>>>
>>>>>  #if IS_ENABLED(CONFIG_OF)
>>>>>  static const struct of_device_id tas5720_of_match[] = {
>>>>> -    { .compatible = "ti,tas5720", },
>>>>> -    { .compatible = "ti,tas5722", },
>>>>> +    { .compatible = "ti,tas5720", .data = &tas5720, },
>>>>> +    { .compatible = "ti,tas5722", .data = &tas5722, },
>>>>>      { },
>>>>>  };
>>>>>  MODULE_DEVICE_TABLE(of, tas5720_of_match);
>>>>>
>>>>
>>

Re: [PATCH v2 1/2] ASoC: tas5720.c: cleanup variant management

[Nikolaus Voss]

[-- Attachment #1: Type: text/plain, Size: 5506 bytes --]

On Wed, 3 Jul 2019, Andrew F. Davis wrote:
> On 7/2/19 6:12 AM, Nikolaus Voss wrote:
>> On Mon, 1 Jul 2019, Andrew F. Davis wrote:
>>> On 7/1/19 11:35 AM, Nikolaus Voss wrote:
>>>> On Mon, 1 Jul 2019, Andrew F. Davis wrote:
>>>>> On 7/1/19 9:42 AM, Nikolaus Voss wrote:
>>>>>> Replace enum tas572x_type with struct tas5720_variant which aggregates
>>>>>> variant specific stuff and can be directly referenced from an id
>>>>>> table.
>>>>>>
>>>>>> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
>>>>>> ---
>>>>>>  sound/soc/codecs/tas5720.c | 98
>>>>>> +++++++++++++-------------------------
>>>>>>  1 file changed, 33 insertions(+), 65 deletions(-)
>>>>>>
>>>>>> diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c
>>>>>> index 37fab8f22800..b2e897f094b4 100644
>>>>>> --- a/sound/soc/codecs/tas5720.c
>>>>>> +++ b/sound/soc/codecs/tas5720.c
>>>>>> @@ -28,9 +28,10 @@
>>>>>>  /* Define how often to check (and clear) the fault status register
>>>>>> (in ms) */
>>>>>>  #define TAS5720_FAULT_CHECK_INTERVAL        200
>>>>>>
>>>>>> -enum tas572x_type {
>>>>>> -    TAS5720,
>>>>>> -    TAS5722,
>>>>>> +struct tas5720_variant {
>>>>>> +    const int device_id;
>>>>>> +    const struct regmap_config *reg_config;
>>>>>> +    const struct snd_soc_component_driver *comp_drv;
>>>>>>  };
>>>>>>
>>>>>>  static const char * const tas5720_supply_names[] = {
>>>>>> @@ -44,7 +45,7 @@ struct tas5720_data {
>>>>>>      struct snd_soc_component *component;
>>>>>>      struct regmap *regmap;
>>>>>>      struct i2c_client *tas5720_client;
>>>>>> -    enum tas572x_type devtype;
>>>>>> +    const struct tas5720_variant *variant;
>>>>>
>>>>> Why add a new struct? Actually I don't see the need for this patch at
>>>>> all, the commit message only explains the 'what' not the 'why'. We can
>>>>> and do already build this info from the tas572x_type.
>>>>
>>>> As the commit message says, the purpose is to aggregate the variant
>>>> specifics and make it accessible via one pointer. This is a standard
>>>> approach for of/acpi_device_id tables and thus makes the code simpler
>>>> and improves readability. This is a maintenance patch to prepare using
>>>> the device match API in a proper way.
>>>>
>>>
>>>
>>> "make it accessible via one pointer" is again a "what", the "why" is:
>>>
>>> "This is a standard approach"
>>> "makes the code simpler and improves readability"
>>>
>>> Those are valid reasons and should be what you put in the commit message.
>>
>> ok
>>
>>>
>>>
>>>>>
>>>>> Also below are several functional changes, the cover letter says
>>>>> this is
>>>>> not a functional change, yet the driver behaves differently now.
>>>>
>>>> Can you be a little bit more specific? The code should behave exactly as
>>>> before.
>>>>
>>>
>>>
>>> Sure, for instance the line "unexpected private driver data" is removed,
>>> this can now never happen, that is a functional change. The phrase "no
>>> functional change", should be reserved for only changes to spelling,
>>> formatting, code organizing, etc..
>>
>> "unexpected private driver data" was unreachable code before, but you're
>> right, debug output has changed a little, but the functional part is
>> exactly the same.
>>
>>>
>>>
>>>> Niko
>>>>
>>>>>
>>>>> Andrew
>>>>>
>>>>>>      struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
>>>>>>      struct delayed_work fault_check_work;
>>>>>>      unsigned int last_fault;
>>>>>> @@ -179,17 +180,13 @@ static int tas5720_set_dai_tdm_slot(struct
>>>>>> snd_soc_dai *dai,
>>>>>>          goto error_snd_soc_component_update_bits;
>>>>>>
>>>>>>      /* Configure TDM slot width. This is only applicable to
>>>>>> TAS5722. */
>>>>>> -    switch (tas5720->devtype) {
>>>>>> -    case TAS5722:
>>>>>> +    if (tas5720->variant->device_id == TAS5722_DEVICE_ID) {
>>>
>>>
>>> I also don't like this, TAS5722_DEVICE_ID is the expected contents of a
>>> register, you are using it like the enum tas572x_type that you removed.
>>> I'd leave that enum, the device ID register itself is not guaranteed to
>>> be correct or unique, which is why we warn about mismatches below but
>>> then continue to use the user provided device type anyway.
>>
>> Strange, with me it's the other way round, I don't like the enum. The
>> mismatch behavior hasn't changed a bit, the same warning is printed. If
>> the device ID is no longer unique in the future (apparently it is for
>> now) the driver should explicitly handle this instead of printing a
>> warning, because warnings should be reserved for an indication of any
>> kind of misconfiguration and not of expected behavior.
>>
>> That said the variant struct can of course replace the enum in every
>> aspect, even for what you describe above. The enum was an ordinal
>> representation of the user-selected i2c_device_id, the variant struct* is
>> a pointer representation of the user-selected i2c/of/acpi_device_id. The
>> only difference is that it directly points to the variant specific parts
>> of the driver instead of resolving those via multiple switch/case
>> statements.
>
> The enum identifies the device type, easy as that, if you want to
> instead do all the logic switching on some internal ID register value
> code then make a patch for just that and explain what is gained. Don't
> do that into this one.

I don't do and I don't want to. The struct pointer identifies the device 
type the same way as the enum does. I guess we better leave things as they 
are. Anyway, thanks for your time and effort.

Nikolaus