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

[Nikolaus Voss <nv@vosn.de>]

[-- 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