Thank you for your kind assistance!
I will carefully review the information you provided and send it back to you.
I have responded to a few points; could you please review them?"
Thank you!
>
> > + 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?
> > +static int sma1307_register_read(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
>
> > + ret = sma1307_regmap_read(sma1307, reg, &val);
> > + if (ret < 0)
> > + return -EINVAL;
> > +
> > + ucontrol->value.bytes.data[1] = val;
>
> We generally wouldn't offer controls like this, and since regmap has
> debugfs support we already have facilities for looking at the current
> device state.
> > +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.)
> > + /* 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.
>
> > + /* 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.
> > + 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.
Attachment:
signature.asc
Description: signature.asc
