On 1/9/19 2:51 PM, Mark Brown wrote:

On Tue, Dec 11, 2018 at 03:23:12PM -0600, Pierre-Louis Bossart wrote:

...

+int snd_sof_pci_update_bits_unlocked(struct snd_sof_dev *sdev, u32 offset,

• 		     u32 mask, u32 value)
  

+{

• bool change;
• unsigned int old, new;
• u32 ret = ~0; /* explicit init to remove uninitialized use warnings */

This looks a lot like you want to write regmap_pci_config...

I think you made that note for the v2 a long time ago, not sure if I replied at the time.

We only use this for 4 cases (power/clock gating on/off, traffic class, etc) and only during the hardware initialization. This is similar to the legacy and Skylake driver, I don't see a significant benefit with a regmap?

intel/hda-ctrl.c:       snd_sof_pci_update_bits(sdev, PCI_CGCTL, PCI_CGCTL_MISCBDCGE_MASK, val); intel/hda-ctrl.c:       snd_sof_pci_update_bits(sdev, PCI_CGCTL, PCI_CGCTL_ADSPDCGE, val); intel/hda-ctrl.c:       snd_sof_pci_update_bits(sdev, PCI_PGCTL, PCI_PGCTL_ADSPPGD, val); intel/hda-dsp.c:        snd_sof_pci_update_bits(sdev, PCI_PGCTL, intel/hda-dsp.c:        snd_sof_pci_update_bits(sdev, PCI_TCSEL, 0x07, 0);

...

+/* control */ +static inline int snd_sof_dsp_run(struct snd_sof_dev *sdev) +{

• if (sdev->ops->run)

• return sdev->ops->run(sdev);
  

• return 0;

+}

Do we really want to return 0 for all these ops if they're not implemented? For some that seems sensible but there's others where it seems like the caller might want to know they got ignored and an error code like -ENOTSUPP might be better.

Good point indeed. There are a set of ops that are really mandatory, we should rework this instead. Thanks for the comment.