ASoC: SOF: fix return values for mandatory ops #539

ranj063 · 2019-01-14T20:28:48Z

Fix return value for mandatory ops.

Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com

ranj063 · 2019-01-14T20:29:41Z

@plbossart this leaves only the PCM ops but I'm not sure whether they are mandotory or not. Could you please comment?

plbossart

this looks ok but I'd like error messages to reflect the name of the function with the _ included. That helps for searches.
Also it'd be good to tag the callbacks in the header file as mandatory/optional so that the information is in one place.

sound/soc/sof/ops.h

ranj063 · 2019-01-16T03:17:27Z

@plbossart fixed now. Could you please cross-check if the list of mandatory and optional ops is correct?

plbossart

@lgirdwood @keyonjie can you look at the list of mandatory ops and comment? I am afraid we have no documentation of what is expected in a minimal case?

plbossart · 2019-01-16T20:06:03Z

sound/soc/sof/sof-priv.h

@@ -67,6 +67,9 @@ struct snd_sof_pdata;
 * and DSP device(s).
 */
 struct snd_sof_dsp_ops {
+
+	/* The following ops are mandatory */


it's not very easy to figure out where the list stops. maybe for each op so that is can be searched whether it's optional or mandatory?

keyonjie

we don't have .remove yet for skl- platforms(please fix that).
stall, core_power_up/down should be optional.
write, read, write64, read64, block_read, block_write are same for all our platforms so let's remove them from struct snd_sof_dsp_ops.

Others looks fine.

keyonjie · 2019-01-17T04:59:04Z

sound/soc/sof/sof-priv.h

@@ -67,6 +67,9 @@ struct snd_sof_pdata;
 * and DSP device(s).
 */
 struct snd_sof_dsp_ops {
+
+	/* The following ops are mandatory */
+
 	/* probe and remove */
 	int (*remove)(struct snd_sof_dev *sof_dev);


I agree we should make .remove mandatory, but the fact is that we don't have it on skl- platforms yet, better to fix them together at one shot(e.g. define an empty one at least).

why should .remove be mandatory? if all your driver uses devm_ stuff and the hardware is reset correctly in the .probe, why would we care?

@plbossart going by what remove() does for SKL+, we also disable the DSP in remove(). Given that we are not really unloading the topology from the DSP like we do in the case of load, shouldnt we also be turning off the DSP here?

@plbossart i believe for module load/unload to work in SOF, we should make remove() mandatory so that the DSP can be turned OFF here. Please let me know if you disagree and I'll make changes accordingly.

sound/soc/sof/sof-priv.h

ranj063 · 2019-01-17T06:33:28Z

we don't have .remove yet for skl- platforms(please fix that).

stall, core_power_up/down should be optional.

@keyonjie With SMP enabled in the driver, they are no long optional.

write, read, write64, read64, block_read, block_write are same for all our platforms so let's remove them from struct snd_sof_dsp_ops.

Others looks fine.

keyonjie · 2019-01-17T06:44:10Z

we don't have .remove yet for skl- platforms(please fix that).

stall, core_power_up/down should be optional.

@keyonjie With SMP enabled in the driver, they are no long optional.

for SKL- platforms, we don't have SMP there, I meant those callbacks are not mandatory for snd_sof_dsp_ops (e.g. no need for byt), but they are mandatory for apl/cnl?

write, read, write64, read64, block_read, block_write are same for all our platforms so let's remove them from struct snd_sof_dsp_ops.

Others looks fine.

ranj063 · 2019-01-17T06:52:35Z

we don't have .remove yet for skl- platforms(please fix that).

stall, core_power_up/down should be optional.

@keyonjie With SMP enabled in the driver, they are no long optional.

for SKL- platforms, we don't have SMP there, I meant those callbacks are not mandatory for snd_sof_dsp_ops (e.g. no need for byt), but they are mandatory for apl/cnl?

write, read, write64, read64, block_read, block_write are same for all our platforms so let's remove them from struct snd_sof_dsp_ops.

Others looks fine.

@keyonjie ok Makes sense

ranj063 · 2019-01-17T06:57:21Z

we don't have .remove yet for skl- platforms(please fix that).

stall, core_power_up/down should be optional.

write, read, write64, read64, block_read, block_write are same for all our platforms so let's remove them from struct snd_sof_dsp_ops.

@keyonjie hsw-spi.c has a different set of ops!

Others looks fine.

ranj063 · 2019-01-17T07:32:38Z

@keyonjie @plbossart PR updated now.

plbossart · 2019-01-17T17:03:12Z

@ranj063 can you rebase, there is conflict. Also please mark each function as mandatory/optional, you are mixing the two styles (per function and per block) which will make searches more complicated.
Thanks!

ranj063 · 2019-01-22T19:54:28Z

@plbossart this one has been updated too. There are 2 lines over 80 chars which checkpatch complains about but I think there's no simple way to address it while keeping the style consistent for marking the ops are madatory/optional.

plbossart

Humm, I am not sure I agree with the mandatory support for PCM (SueCreek being the counter example) and there are some inconsistencies between the errors and tags. Maybe we should shoot for a lower bar of what is really mandatory?

plbossart · 2019-01-24T02:42:51Z

sound/soc/sof/ops.h

@@ -330,7 +345,8 @@ snd_sof_pcm_platform_pointer(struct snd_sof_dev *sdev,
 	if (sof_ops(sdev) && sof_ops(sdev)->pcm_pointer)
 		return sof_ops(sdev)->pcm_pointer(sdev, substream);

-	return 0;
+	dev_err(sdev->dev, "error: %s not defined\n", __func__);
+	return -ENOTSUPP;


Are PCM operations actually required? Take the example of Sue Creek, you may have all the processing done on the device itself with no PCM being passed back to the host?

@plbossart ok i will make these optional

plbossart · 2019-01-24T02:44:41Z

sound/soc/sof/sof-priv.h

-	u64 (*read64)(struct snd_sof_dev *sof_dev, void __iomem *addr);
+			u64 value); /* mandatory */
+	u64 (*read64)(struct snd_sof_dev *sof_dev,
+		      void __iomem *addr); /* mandatory */


this is inconsistent with the fact that those functions return 0 if not supported?

ranj063 · 2019-01-26T01:25:30Z

@plbossart i've updated the PR based on your comments.

wenqingfu · 2019-01-29T03:51:00Z

ping @plbossart

plbossart · 2019-01-29T04:30:36Z

ping @plbossart <https://github.com/plbossart>

Ranjani and I are working on PM stuff, this can wait.

…

ranj063 · 2019-02-05T02:16:02Z

@plbossart I've updated this PR now to address your comment about the is_ready() op. There will be conflicts between this one and my other PR where I have changed the name for this op. I will update them depending on which one gets merged first.

ranj063 · 2019-02-05T04:02:34Z

@plbossart please dont merge this. with the return value of undefined values change to < 0, I need to ensure the check is done correctly.

ranj063 · 2019-02-06T02:56:01Z

@plbossart this one has been updated now.

plbossart

Sorry, this still needs more work, some of the mandatory parts are not used so something's wrong.

plbossart · 2019-02-06T14:47:12Z

sound/soc/sof/ipc.c

+	if (is_dsp_ipc_ready < 0) {
+		spin_unlock_irq(&sdev->ipc_lock);
+		return -EBUSY;
+	}


sorry, what does this do?
The is_ready op is marked a mandatory
int (*is_ready)(struct snd_sof_dev sof_dev); / mandatory */

The comment seems wrong?

plbossart · 2019-02-06T14:51:03Z

sound/soc/sof/sof-priv.h

+	/* FW loading */
+	int (*load_firmware)(struct snd_sof_dev *sof_dev); /* mandatory */
+	int (*load_module)(struct snd_sof_dev *sof_dev,
+			   struct snd_sof_mod_hdr *hdr); /* mandatory */


meh, this is not defined for HDAudio.

plbossart · 2019-02-06T14:54:17Z

sound/soc/sof/sof-priv.h

-	int (*reset)(struct snd_sof_dev *sof_dev);
+	int (*run)(struct snd_sof_dev *sof_dev); /* mandatory */
+	int (*stall)(struct snd_sof_dev *sof_dev); /* optional */
+	int (*reset)(struct snd_sof_dev *sof_dev); /* mandatory */


meh. not defined for HDaudio.

@plbossart all 3 comments addressed and PR updated now.

plbossart

minor nit-pick on dev_err when we return void, looks ok otherwise

plbossart · 2019-02-07T19:04:33Z

sound/soc/sof/ops.h

+		return;
+	}
+
+	dev_err(sdev->dev, "error: %s not defined\n", __func__);


Ah sorry, I just thought about this one. Since this will not interrupt the flow, we may want to add a _once or _ratelimited qualifier to avoid polluting dmesg. Same remark for all cases where we return void.

@plbossart done

Clarify the mandatory and optional dsp ops and fix return value for mandatory ops. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

plbossart

Thanks, merging now.

ranj063 requested a review from plbossart January 14, 2019 20:29

ranj063 force-pushed the upstream/ops_fixes branch from 78173e0 to cce1f76 Compare January 14, 2019 20:31

ranj063 mentioned this pull request Jan 14, 2019

Upstream: fix ops (comments from Mark Brown) #525

Closed

plbossart requested changes Jan 15, 2019

View reviewed changes

sound/soc/sof/ops.h Show resolved Hide resolved

ranj063 force-pushed the upstream/ops_fixes branch from cce1f76 to 63c49e4 Compare January 16, 2019 03:14

plbossart reviewed Jan 16, 2019

View reviewed changes

keyonjie reviewed Jan 17, 2019

View reviewed changes

ranj063 closed this Jan 17, 2019

ranj063 deleted the upstream/ops_fixes branch January 17, 2019 07:27

ranj063 restored the upstream/ops_fixes branch January 17, 2019 07:29

ranj063 reopened this Jan 17, 2019

ranj063 closed this Jan 17, 2019

ranj063 deleted the upstream/ops_fixes branch January 17, 2019 07:30

ranj063 restored the upstream/ops_fixes branch January 17, 2019 07:30

ranj063 reopened this Jan 17, 2019

ranj063 force-pushed the upstream/ops_fixes branch from 63c49e4 to 55b9699 Compare January 17, 2019 07:31

ranj063 force-pushed the upstream/ops_fixes branch from 55b9699 to f8ab4fc Compare January 22, 2019 19:52

plbossart reviewed Jan 24, 2019

View reviewed changes

ranj063 force-pushed the upstream/ops_fixes branch from f8ab4fc to 84e8463 Compare January 26, 2019 01:24

ranj063 force-pushed the upstream/ops_fixes branch from 84e8463 to 960860b Compare February 5, 2019 02:14

ranj063 force-pushed the upstream/ops_fixes branch from 960860b to 75d52e6 Compare February 5, 2019 04:08

plbossart reviewed Feb 6, 2019

View reviewed changes

ranj063 force-pushed the upstream/ops_fixes branch from 75d52e6 to a88576e Compare February 7, 2019 17:53

plbossart reviewed Feb 7, 2019

View reviewed changes

ASoC: SOF: fix return values for mandatory ops

421e6e5

Clarify the mandatory and optional dsp ops and fix return value for mandatory ops. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the upstream/ops_fixes branch from a88576e to 421e6e5 Compare February 7, 2019 19:15

plbossart approved these changes Feb 7, 2019

View reviewed changes

plbossart merged commit 4066063 into thesofproject:topic/sof-dev Feb 7, 2019

ranj063 deleted the upstream/ops_fixes branch February 8, 2019 06:36

ASoC: SOF: fix return values for mandatory ops #539

ASoC: SOF: fix return values for mandatory ops #539

Conversation

ranj063 commented Jan 14, 2019

ranj063 commented Jan 14, 2019

plbossart left a comment

Choose a reason for hiding this comment

ranj063 commented Jan 16, 2019

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keyonjie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 commented Jan 17, 2019

keyonjie commented Jan 17, 2019

ranj063 commented Jan 17, 2019

ranj063 commented Jan 17, 2019

ranj063 commented Jan 17, 2019

plbossart commented Jan 17, 2019

ranj063 commented Jan 22, 2019

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 commented Jan 26, 2019

wenqingfu commented Jan 29, 2019

plbossart commented Jan 29, 2019 via email

ranj063 commented Feb 5, 2019

ranj063 commented Feb 5, 2019

ranj063 commented Feb 6, 2019

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment