RE: [EXTERNAL] Re: [PATCH v6 1/4] ASoc: PCM6240: Create PCM6240 Family driver code

[Ding, Shenghao]

> -----Original Message-----
> From: Mark Brown <broonie@xxxxxxxxxx>
> Sent: Tuesday, February 27, 2024 11:46 PM
> To: Ding, Shenghao <shenghao-ding@xxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; lgirdwood@xxxxxxxxx;
> robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; linux-sound@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx;
> 13916275206@xxxxxxx; Chawla, Mohit <mohit.chawla@xxxxxx>;
> soyer@xxxxxx; Huang, Jonathan <jkhuang3@xxxxxx>; tiwai@xxxxxxx; Djuandi,
> Peter <pdjuandi@xxxxxx>; Agrawal, Manisha <manisha.agrawal@xxxxxx>;
> Hari, Raj <s-hari@xxxxxx>; Yashar, Avi <aviel@xxxxxx>; Nagalla, Hari
> <hnagalla@xxxxxx>; Bajjuri, Praneeth <praneeth@xxxxxx>
> Subject: [EXTERNAL] Re: [PATCH v6 1/4] ASoc: PCM6240: Create PCM6240
> Family driver code
> 
> On Fri, Feb 23, 2024 at 08:38:04PM +0800, Shenghao Ding wrote:
> 
> > +static const char *const pcmdev_ctrl_name[] = {
> > +	"%s-i2c-%d-dev%d-ch%d-ana-gain",
> > +	"%s-i2c-%d-dev%d-ch%d-digi-gain",
> > +	"%s-i2c-%d-dev%d-ch%d-fine-gain",
> > +};
> 
> So, I see why you're doing this naming thing for the per-device controls
> - the device can (and is designed to) allow controlling multiple amps with a
> single I2C write.  However this is resulting in something that's really awkward
> from an ALSA point of view, the names that are being generated are very
> much not idiomatic for control names and probably aren't going to be
> terribly meaningful for end users since they're not associated with where the
> relevant outputs physically are.  We could require machines to provide
> names for everything but that's probably going to be a miserable experience
> on x86 and is unhelpful even with DT so having a default seems to make
> sense.
> 
> I think these should look more like ALSA names, so "I2C %d Device %d..."
> though that does make things a bit longer.
Hi Mark
For only single one device, all these seems redundant and makes uncomfortable
Many of my customers will have such cases as
Four pieces of Pcm6240s and three pieces of pcm6360s in the same i2c bus.
The chip name can help them identify the chip and set the right chip and the right channel.
> 
> > +static const char *const pcmdev_ctrl_name_with_prefix[] = {
> > +	"%s-dev%d-ch%d-ana-gain",
> > +	"%s-dev%d-ch%d-digi-gain",
> > +	"%s-dev%d-ch%d-fine-gain",
> > +};
> 
> For these I'm not clear why we don't just use the name, what's the goal in
> having the device number in there?
> 
> I think it would make sense to do a version the driver with no user visible
> controls (or at least none that need this name generation stuff) and then
> make the controls an incremental patch, the driver would obviously need
> both bits to be properly useful but it'd mean that the simple bit could get
> reviewed and possibly merged separately to the complicated bit which would
> probably be easier.

You means to remove the kcontrols first. After the patch was accepted, add these
Kcontrols, right?
> 
> Please also send the output of mixer-test from a machine with this driver in it
> as part of the cover letter, it'll make it easier to tell what's going on.
> 
> > +	if (comp->name_prefix) {
> > +	/* There's name_prefix defined in DTS. Bin file name will be
> > +	 * name_prefix.bin stores the firmware including register setting and
> > +	 * params for different filters inside chips, it must be copied into
> 
> The comment should be indented like it's inside the if ().
> 
> > +	} else {
> > +	/* There's NO name_prefix defined in DTS. Bin file name will be
> > +	 * device-name[defined in pcmdevice_i2c_id]-i2c-bus_id[0,1,...,N]-
> > +	 * sum[1,...,4]dev.bin stores the firmware including register setting
> > +	 * and params for different filters inside chips, it must be copied
> > +	 * into firmware folder. The same types of pcmdevices sitting on the
> > +	 * same i2c bus will be aggregated as one single codec, all of them
> > +	 * share the same bin file.
> > +	 */
> > +		scnprintf(pcm_dev->bin_name,
> PCMDEVICE_BIN_FILENAME_LEN,
> > +			"%s-i2c-%d-%udev.bin", pcm_dev->dev_name, adap-
> >nr,
> > +			pcm_dev->ndev);
> > +	}
> 
> You could also do this as a fallback (ie, try the specified name first then fall
> bakc to the bus based numbering), that way if a name is added later it won't
> break people's installs on upgrade.
> 
> > +static const struct regmap_config pcmdevice_i2c_regmap = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.cache_type = REGCACHE_MAPLE,
> > +	.ranges = pcmdevice_ranges,
> > +	.num_ranges = ARRAY_SIZE(pcmdevice_ranges),
> > +	.max_register = 256 * 128,
> > +};
> 
> Shouldn't there be some volatile registers if we have interrupts?