[v2] ASoC: cs42xx8-i2c.c: add module device table for of

Commit Message

Peter Bergin Oct. 28, 2022, 7:50 a.m. UTC

When trying to connect the device with the driver through
device-tree it is not working. The of_device_id is defined in
cs42xx8.c but is not correctly included in cs42xx8-i2c.c.

Move of_device_id table to cs42xx8-i2c.c. Get cs42xx8_driver_data
in cs42xx8_i2c_probe() and pass as argument to cs42xx8_probe(). Move
error check if no driver data found to cs42xx8_i2c_probe().

Matching of device in cs42xx8_i2c_probe() is coded with inspiration
from tlv320aic32x4-i2c.c.

Signed-off-by: Peter Bergin <[e-mail]>
---
v2: reworked and removed duplication of cs42xx8_of_match

 sound/soc/codecs/cs42xx8-i2c.c | 49 +++++++++++++++++++++++++++++++---
 sound/soc/codecs/cs42xx8.c     | 33 +++--------------------
 sound/soc/codecs/cs42xx8.h     |  5 +---
 3 files changed, 50 insertions(+), 37 deletions(-)

Comments

Charles Keepax Oct. 28, 2022, 9:55 a.m. UTC | #1

On Fri, Oct 28, 2022 at 09:50:44AM +0200, Peter Bergin wrote:
> Matching of device in cs42xx8_i2c_probe() is coded with inspiration
> from tlv320aic32x4-i2c.c.
> ---
> +	if (i2c->dev.of_node) {
> +		const struct of_device_id *oid;
> +
> +		oid = of_match_node(cs42xx8_of_match, i2c->dev.of_node);
> +		if (!oid)
> +			goto err_not_found;
> +		drvdata = (struct cs42xx8_driver_data *)oid->data;
> +	} else {
> +		const struct i2c_device_id *id;
> +
> +		id = i2c_match_id(cs42xx8_i2c_id, i2c);
> +		if (!id)
> +			goto err_not_found;
> +		drvdata = (struct cs42xx8_driver_data *)id->driver_data;
> +	}

Be worth noting a little more explicitly in the commit message
you updated this logic as part of the move. I would even be
tempted to put it as a separate patch personally.

> +static const struct cs42xx8_driver_data cs42448_data = {
> +	.name = "cs42448",
> +	.num_adcs = 3,
> +};
> +
> +static const struct cs42xx8_driver_data cs42888_data = {
> +	.name = "cs42888",
> +	.num_adcs = 2,
> +};

It is probably better to leave these two structures exported from
the primary module. These devices could technically have SPI
support added in the future and it might as well reuse these same
data structures.

Thanks,
Charles

Mark Brown Oct. 28, 2022, 10:55 a.m. UTC | #2

On Fri, Oct 28, 2022 at 09:50:44AM +0200, Peter Bergin wrote:
> When trying to connect the device with the driver through
> device-tree it is not working. The of_device_id is defined in
> cs42xx8.c but is not correctly included in cs42xx8-i2c.c.

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

Peter Bergin Oct. 28, 2022, 10:57 a.m. UTC | #3

On 2022-10-28 12:55, Mark Brown wrote:
> On Fri, Oct 28, 2022 at 09:50:44AM +0200, Peter Bergin wrote:
>> When trying to connect the device with the driver through
>> device-tree it is not working. The of_device_id is defined in
>> cs42xx8.c but is not correctly included in cs42xx8-i2c.c.
> Please don't send new patches in reply to old patches or serieses, this
> makes it harder for both people and tools to understand what is going
> on - it can bury things in mailboxes and make it difficult to keep track
> of what current patches are, both for the new patches and the old ones.

Thanks for the instructions! Will follow on the next update. I'm new to 
this and trying to learn, pointers like this are very welcome. :-)

Best regards,
/Peter

Patch

diff --git a/sound/soc/codecs/cs42xx8-i2c.c b/sound/soc/codecs/cs42xx8-i2c.c
index bd80e9fc907f..9f9e57398272 100644
--- a/sound/soc/codecs/cs42xx8-i2c.c
+++ b/sound/soc/codecs/cs42xx8-i2c.c
@@ -17,10 +17,32 @@ 
 
 #include "cs42xx8.h"
 
+static const struct i2c_device_id cs42xx8_i2c_id[];
+static const struct of_device_id cs42xx8_of_match[];
+
 static int cs42xx8_i2c_probe(struct i2c_client *i2c)
 {
-	int ret = cs42xx8_probe(&i2c->dev,
-			devm_regmap_init_i2c(i2c, &cs42xx8_regmap_config));
+	int ret;
+	struct cs42xx8_driver_data *drvdata;
+
+	if (i2c->dev.of_node) {
+		const struct of_device_id *oid;
+
+		oid = of_match_node(cs42xx8_of_match, i2c->dev.of_node);
+		if (!oid)
+			goto err_not_found;
+		drvdata = (struct cs42xx8_driver_data *)oid->data;
+	} else {
+		const struct i2c_device_id *id;
+
+		id = i2c_match_id(cs42xx8_i2c_id, i2c);
+		if (!id)
+			goto err_not_found;
+		drvdata = (struct cs42xx8_driver_data *)id->driver_data;
+	}
+
+	ret = cs42xx8_probe(&i2c->dev,
+		devm_regmap_init_i2c(i2c, &cs42xx8_regmap_config), drvdata);
 	if (ret)
 		return ret;
 
@@ -28,6 +50,10 @@  static int cs42xx8_i2c_probe(struct i2c_client *i2c)
 	pm_request_idle(&i2c->dev);
 
 	return 0;
+
+err_not_found:
+	dev_err(&i2c->dev, "failed to find driver data\n");
+	return -EINVAL;
 }
 
 static void cs42xx8_i2c_remove(struct i2c_client *i2c)
@@ -35,7 +61,24 @@  static void cs42xx8_i2c_remove(struct i2c_client *i2c)
 	pm_runtime_disable(&i2c->dev);
 }
 
-static struct i2c_device_id cs42xx8_i2c_id[] = {
+static const struct cs42xx8_driver_data cs42448_data = {
+	.name = "cs42448",
+	.num_adcs = 3,
+};
+
+static const struct cs42xx8_driver_data cs42888_data = {
+	.name = "cs42888",
+	.num_adcs = 2,
+};
+
+static const struct of_device_id cs42xx8_of_match[] = {
+	{ .compatible = "cirrus,cs42448", .data = &cs42448_data, },
+	{ .compatible = "cirrus,cs42888", .data = &cs42888_data, },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, cs42xx8_of_match);
+
+static const struct i2c_device_id cs42xx8_i2c_id[] = {
 	{"cs42448", (kernel_ulong_t)&cs42448_data},
 	{"cs42888", (kernel_ulong_t)&cs42888_data},
 	{}
diff --git a/sound/soc/codecs/cs42xx8.c b/sound/soc/codecs/cs42xx8.c
index d14eb2f6e1dd..957ae08bcf7c 100644
--- a/sound/soc/codecs/cs42xx8.c
+++ b/sound/soc/codecs/cs42xx8.c
@@ -499,29 +499,8 @@  static const struct snd_soc_component_driver cs42xx8_driver = {
 	.endianness		= 1,
 };
 
-const struct cs42xx8_driver_data cs42448_data = {
-	.name = "cs42448",
-	.num_adcs = 3,
-};
-EXPORT_SYMBOL_GPL(cs42448_data);
-
-const struct cs42xx8_driver_data cs42888_data = {
-	.name = "cs42888",
-	.num_adcs = 2,
-};
-EXPORT_SYMBOL_GPL(cs42888_data);
-
-const struct of_device_id cs42xx8_of_match[] = {
-	{ .compatible = "cirrus,cs42448", .data = &cs42448_data, },
-	{ .compatible = "cirrus,cs42888", .data = &cs42888_data, },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, cs42xx8_of_match);
-EXPORT_SYMBOL_GPL(cs42xx8_of_match);
-
-int cs42xx8_probe(struct device *dev, struct regmap *regmap)
+int cs42xx8_probe(struct device *dev, struct regmap *regmap, struct cs42xx8_driver_data *drvdata)
 {
-	const struct of_device_id *of_id;
 	struct cs42xx8_priv *cs42xx8;
 	int ret, val, i;
 
@@ -535,17 +514,11 @@  int cs42xx8_probe(struct device *dev, struct regmap *regmap)
 	if (cs42xx8 == NULL)
 		return -ENOMEM;
 
-	cs42xx8->regmap = regmap;
 	dev_set_drvdata(dev, cs42xx8);
 
-	of_id = of_match_device(cs42xx8_of_match, dev);
-	if (of_id)
-		cs42xx8->drvdata = of_id->data;
+	cs42xx8->regmap = regmap;
 
-	if (!cs42xx8->drvdata) {
-		dev_err(dev, "failed to find driver data\n");
-		return -EINVAL;
-	}
+	cs42xx8->drvdata = drvdata;
 
 	cs42xx8->gpiod_reset = devm_gpiod_get_optional(dev, "reset",
 							GPIOD_OUT_HIGH);
diff --git a/sound/soc/codecs/cs42xx8.h b/sound/soc/codecs/cs42xx8.h
index d36c61b6df74..938e21d92ac2 100644
--- a/sound/soc/codecs/cs42xx8.h
+++ b/sound/soc/codecs/cs42xx8.h
@@ -19,11 +19,8 @@  struct cs42xx8_driver_data {
 };
 
 extern const struct dev_pm_ops cs42xx8_pm;
-extern const struct cs42xx8_driver_data cs42448_data;
-extern const struct cs42xx8_driver_data cs42888_data;
 extern const struct regmap_config cs42xx8_regmap_config;
-extern const struct of_device_id cs42xx8_of_match[];
-int cs42xx8_probe(struct device *dev, struct regmap *regmap);
+int cs42xx8_probe(struct device *dev, struct regmap *regmap, struct cs42xx8_driver_data *drvdata);
 
 /* CS42888 register map */
 #define CS42XX8_CHIPID				0x01	/* Chip ID */