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

[Ki-Seok Jo <kiseok.jo@xxxxxxxxxxxxxx>]

Apologies for the delay in responding. Thank you for the great feedback. I will incorporate your suggestions into the code and submit an updated patch.


> > > > +		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?


Yes, that's correct. Essentially, we can change to default values through a reset, but we also load and apply settings files from the firmware. This allows us to modify the settings file even after the probe and reapply values using that control.



> > > > +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?
> 


When applied to actual smartphones, debugfs is blocked, so regmap settings cannot be modified, but tinymix might still be available. Therefore, it was added to change device settings when needed.
The contol has been removed.



> > > > +	/* 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.
> 

Well, you're right. I should handle errors if the size of the data is smaller than the size of the header I specified. Thanks for the good feedback.