Re: FW: [PATCH 1/2] ASoC: sma1307: Add driver for Iron Device SMA1307

[Mark Brown <broonie@xxxxxxxxxx>]

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.

Attachment: signature.asc
Description: PGP signature