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

[Nikolaus Voss]

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@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.
> >
> > 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