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@xxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>> Â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);
>>>
>>