| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <80af3fca-f71b-c118-e5d8-fde8b7d21705@ti.com>
Date: Mon, 1 Jul 2019 10:25:04 -0400
From: "Andrew F. Davis" <afd@...com>
To: Nikolaus Voss <nikolaus.voss@...wensteinmedical.de>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Andreas Dannenberg <dannenberg@...com>,
Kate Stewart <kstewart@...uxfoundation.org>
CC: <alsa-devel@...a-project.org>, <linux-kernel@...r.kernel.org>,
<linux-acpi@...r.kernel.org>, <nv@...n.de>
Subject: Re: [PATCH v2 1/2] ASoC: tas5720.c: cleanup variant management
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@...wensteinmedical.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);
>
Powered by blists - more mailing lists