On Wed, Aug 14, 2024 at 05:54:19AM +0000, Ki-Seok Jo wrote:
return false;- }
- sma1307_regmap_update_bits(sma1307, SMA1307_00_SYSTEM_CTRL,
SMA1307_RESET_MASK, SMA1307_RESET_ON);- sma1307_reset(component);
This is a very unusual thing to support - what's the motivation for it? If we were going to support it then we'd need to handle what happens to the controls, either generating events saying where they've changed or rsyncing existing values.
The 'sma1307_reset' function is responsible for configuring the amplifier to operate properly. Sometimes users may want to reset the mixer control to its initial state after having used it. I have added a reset control for this purpose. Will it be problematic to initialize the chip values and return it to a usable state?
Yeah, so we don't usually have a control for that - most drivers could have one after all. The usual thing with ALSA would be to do this at the userspace level, saving the configuration on startup and then rewriting the controls from that saved configuration to reset everything back to the initial state. I guess you're partly using this to reload the configuration from firmware?
+static int sma1307_register_write(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol) {
Again, this seems better provided by the regmap debugging features.
Occasionally, regmap writes are restricted in debugfs, so I added it. (Reading is still allowed, so a read function shouldn't be necessary.)
Yes, the regmap debugfs interface requires modification of the kernel to support writes which is generally fine for that application where you're doing things during development. Do you have a use case for this in normal operation?
- /* HEADER */
- sma1307->set.header_size = SMA1307_SETTING_HEADER_SIZE;
- sma1307->set.checksum = data[sma1307->set.header_size - 2];
- sma1307->set.num_mode = data[sma1307->set.header_size - 1];
We didn't verify that the firmware is big enough to contain the header (eg, if there's some filesystem corruption) or any of the other sizes or counts.
The file format is fixed. The size isn't very large, but up to what extent is it acceptable? The header size is predefined, and the header contains the file size value.
It's just about making sure that we're not reading beyond the end of the data we got from request_firmware(), or doing something like dereferencing a NULL pointer. The main issue is being robust against loading a corrupted firmware file, so no real need to enforce specific limits other than what's needed to make sure that reads are in bounds. Consider what would happen if there were a 0 byte file instead of an actual firmware for example.
- /* Register Initial Value Setting */
- switch (sma1307->amp_set) {
- case SMA1307_DEFAULT_SET:
sma1307_set_default(component);break;
Why aren't we just using the chip defaults here?
The values input to this function are the default values for the chip to operate. The chip won't function with just power applied; it requires specific values to be provided to operate.
Sure, but IIRC some of them looked like things which would normally be runtime configured through ALSA controls rather than fixed at startup - for ALSA controls we generally leave them at chip defaults and let each userspace pick what makes sense for it. This is just a way of making sure that we don't have issues with different userspaces wanting different configurations, the hardware provides a convenient default.
- case SMA1307_BINARY_FILE_SET:
sma1307_setting_loaded(sma1307, setting_file);if (sma1307->set.status) {sma1307_set_binary(component);} else {sma1307->force_mute_status = true;sma1307_set_default(component);}break;
What's this for? Usually any default setup would just be done through the usual control interface.
Generally, the amplifier operates correctly with default settings. Occasionally, when users want different output levels, we provide a binary file to adjust the settings.
Since the settings may vary depending on the speakers used and other external factors, and because these settings cannot be adjusted by the user, the purpose is to provide a configuration file upon request.
So really this is loading a firmware file with parameters? That's probably fine. It might be better to just try to load the firmware unconditionally and then fall back to defaults if there's no firmware, though possibly there's safety issues.
The setting_file should probably be named based on the machine compatible or something, that way people can ship a single firmware image with multiple machines supported. Possibly also with an option for extra configuration if it's at all plausible that there might be more than one of these devices in a single system.