Re: [RFC PATCH 13/16] ALSA: mixer_audient: Add enumerated controls

[Will Porter <mrwillporter@gmail.com>]

Hi Christian,

Thank you for squashing the fix and for testing it on 7.3.3.

> patch 12:
> - I would declare snd_audient_int_max, snd_audient_int_min and
>   snd_audient_int_from_ctl as inline [...]

Done.

> patch 13:
> - I would have snd_audient_enum_item return the negative errno
>   instead of e->items and then propagate it.

Done. It returns the item or -EIO, and get propagates the error.

> - I wonder if it would make the code more readable if we refactor all
>   the if-statements snd_ctl_audient_info has grown into something
>   like DIFF 1 below?

Done. snd_ctl_audient_info() now uses a switch.

> patch 15:
> - snd_ctl_audient_enum_get uses ctype->init which is not set by the
>   ID14_ENUM() macro. Is this intentionally always constant 0? In this
>   case, could the logic be removed?

I think the logic is still needed, because init is not always 0.
ID14_ENUM() builds only the readable enums: Mono Mode and the four
routing controls. They have no cache entry, so
snd_audient_raw_get_one() never returns -ENODATA for them, and no code
reads init for them. The three write-only enums (iD key function,
talkback source, optical mode) do not use the macro. They set .init
explicitly, to 0xffff, 0xff and 1. A comment on the macro now says
this.

> - Not sure if this is relevant for the iD14 but we might want to
>   apply something like DIFF 2 to make update events work on the
>   replaced controls 11 and possibly 60 [...]

I left DIFF 2 out of my patches. I have no objection to it, but I
cannot test it. The audio control interface of the iD14 MkI has no
interrupt endpoint (bNumEndpoints is 0), so
snd_usb_mixer_status_create() returns before it creates the URB. The
device sends no status interrupts, and a knob change shows only by a
read. On the iD14 the diff only moves the table-created controls from
id 0 to their unit in id_elems and in the usbmixer proc file.

Two things in sound/usb/mixer.c may matter for your EVO4 events:

- snd_usb_mixer_interrupt_v2() skips an element when the unit has more
  than one element and info->control differs from the control selector
  in the message. Our elements leave control at 0. Would the event
  reach them?
- The notify paths clear info->cached of the generic mixer, not the
  mixer_audient cache. Would a cached control report its old value
  after the event?

Could you carry DIFF 2 with your EVO4 event change?

> - In mixer_quirks.c: Fallthrough case statements seem to be used
>   almost everywhere else, [...]

Done. The two case labels share one call.

> patch 16:
> - can we rename 'create' to something which makes it obvious from the
>   name that the callback is optional, e.g. 'extra_init' or similar?

Done. It is now extra_init, with a comment that it is optional.

With these changes each of the five patches builds with W=1 without
warnings, and checkpatch --strict reports nothing new.
On the iD14 (stable 7.2.15) the read and write checks of the controls
pass with the new version.

For the next version: unless you would like another RFC round, I
would send it as a normal series, on top of your four mixer_evo fixes
as before. Does that work for you?

Thanks,
Will