At Fri, 09 Mar 2012 09:19:05 +0100, David Henningsson wrote:
I'm a little surprised by the implementation though; partly because the functions added to hda_codec.c does not seem to be HDA specific, partly by the need to mess around with set_fs. I trust you to know what you're doing w r t to the set_fs stuff, but maybe it would be more elegant if these functions could be in the core and either using, or together with, other functions that need to do set_fs to read/write TLV information?
The volume initialization is a very sensitive topic. It's really depending on the hardware which value to be set. In the case of vmaster, it's relatively easy. At least, the slave output volumes can be set to 0dB just to follow the master volume.
Then I thought of implementing it in the common vmaster code, but found that it's not so trivial. The vmaster slaves may contain also input volumes like line-in. Raising such a control automatically is obviously wrong. Thus the selection of slaves must be selective. Also the handling of dB TLV would be more complicated when we allow all dB TLV types, to be more generic. That's why I decided to start from the HD-audio specific code.
About set_fs: yes, it's ugly and I want to get rid of it, too. Currently it's needed because the TLV callback is supposed to handle the user-space pointer directly. Handling the user-space pointer allows us to access to a large data without too much copying.
OTOH, it's true that the direct access would be easier for the lowlevel driver no matter with our without set_fs. Since the TLV data size is limited (practically in 4kB or such), we may pass it to kmalloc'ed buffer and let the core copying to user-space. It's not so frequently accessed type, anyway.
So, both your points are correct. They are to be improved in future.
I think at least get_kctl_0dB_offset should move to control.c at this step (and renamed to start with snd_ ), just because it belongs there and seems useful enough to other drivers.
Well, I still don't want to put it to the common place yet until any user in other drivers comes up. This function is still not generic enough. It doesn't handle all dB TLV types. It doesn't even check the element type, assuming it's either an integer or a boolean control. Exporting it as a common API function needs such brush-ups.
In control.c you can check if tlv callback has been overridden and if not, use struct user_element directly, to avoid set_fs. Btw, not many drivers override tlv.c anyway and those who do just do it to provide dB min/max. Seems easy enough to change the tlv.c to return dB min/max instead of copying data to userspace.
Err, no, user_element is only for user-space control elements. It has nothing to do with the kernel-space controls. The static TLV pointers are used as is already in the current patch. But the callback function needs to be evaluated at each time, and set_fs() is inevitable for faking copy_to_user() to kerne-space unless we change the callback not to handle user-space pointers.
Hence, the steps ahead of us are:
- Change the all tlv callbacks to handle kerne-pointers; the allocation of the temporary buffer and the user-space copy are done in control.c
- Add some sanity checks in get_kctl_0dB_offset() not to allow everything
Takashi