On 2020-08-13 8:29 PM, Andy Shevchenko wrote:
On Wed, Aug 12, 2020 at 10:57:41PM +0200, Cezary Rojewski wrote:
Thanks for good review Andy!
+static inline bool catpt_resource_overlapping(struct resource *r1,
struct resource *r2,struct resource *ret)+{
- if (!resource_overlaps(r1, r2))
return false;- ret->start = max(r1->start, r2->start);
- ret->end = min(r1->end, r2->end);
- return true;
+}
JFYI, I have just submitted a series [1] that includes this helper [2] to be available for all.
Well, I'm happy that catpt somewhat impacted resource-API getting more flexble, although it would be nice to get --cc'ed as _overlapping/_intersecting got moved into general part of kernel without changes, basically.
This raises a dependancy issue, am I right? i.e. until this gets merged, catpt will cause compilation errors on Mark's for-next. -or- perhaps you want me to leave things as they are for current release while removing said function later, once your PR get's merged?
+struct catpt_dev {
- struct device *dev;
- struct dw_dma_chip *dmac;
Is it possible to use opaque pointer here? It will be better if in the future (I think unlikely, but still) somebody decides to use this with another DMA engine.
Any opaque structure comes at a cost -> requires higher level of understanding from developers maintaining given piece of code (that includes architecture knowledge too, to get a grasp of why such decision was even made) == higher maintaince cost.
One could device ADSP architectures into: 1) LPT/WPT 2) BYT/CHT/BDW 3) cAVS (SKL+) 4) new (which I won't be elaborating here for obvious reasons)
To my knowledge, except for 1), none of them makes use of dmaengine.h when loading FW or doing any other action for that matter. As such, I don't see any reason to convert something explicit into something implicit. Don't believe either of options would be reusing struct catpt_dev too. In general, to make that happen you'd have to start with conversion of existing HDAudio transport (cAVS+) into dmaengine model and then do the same with SoundWire (cAVS+) - haven't seen sdw code in a while but still pretty sure it's not dmaengine-friendly.
+#define CATPT_IPC_ERROR(err) ((err < 0) ? err : -EREMOTEIO)
err -> (err) in all cases of right side.
Ack.
+struct catpt_stream_runtime {
- struct snd_pcm_substream *substream;
- struct catpt_stream_template *template;
- struct catpt_stream_info info;
- struct resource *persistent;
- struct snd_dma_buffer pgtbl;
- bool allocated:1;
- bool prepared:1;
Does this ':1' make any sense?
In current state it does not, really. Playing internally with segments which are not part of this release (as noted in cover-letter) where some of these did. Will remove in v5.
- struct list_head node;
+};
...
+#ifdef CONFIG_PM
Perhaps __maybe_unused?
Sure, removal of #ifdefs is always nice.
- ret = catpt_dsp_stall(cdev, true);
- if (ret < 0)
I'm wondering if all these ' < 0' all over the code make sense? What do you expect out of positive returned values if any?
Isn't this more of a preference? Please note I'm basing many of my decisions on code that's around me - /sound/core/ and sound/soc/ *.c.
Except for IPCs, basically all catpt rets retrieved from functions called will be returning either 0 (success) or <0 (error). No objections, but I don't see much difference either.
goto exit;
+#ifdef CONFIG_PM_SLEEP
Perhaps __maybe_unused?
Same as above~1, ack.
- board = platform_device_register_data(NULL, mach->drv_name,
PLATFORM_DEVID_NONE,(const void *)mach, sizeof(*mach));- if (!board) {
Here obviously not correct check.
Indeed, ack.
dev_err(cdev->dev, "board register failed\n");return PTR_ERR(board);- }
- ret = devm_add_action(cdev->dev, board_pdev_unregister, board);
- if (ret < 0) {
platform_device_unregister(board);
return ret;- }
- return 0;
return ret;
Similarly, to comment~2 regarding preferences, don't mind the change (in fact, I'm a fan) but in the past got messaged to leave things explicit - leave last 'if' with return ret, while return 0 marking success outside.
- mutex_init(&cdev->clk_mutex);
- blank line.
Thought wide comment makes enough distance already.
- /*
* Mark both device formats as uninitialized. Once corresponding* cpu_dai's pcm is created, proper values are assignedPlease, use period(s) in multi-line comments.
Used to: all-but-last sentence with period(s). Will update as requested in v5.
+static int catpt_acpi_probe(struct platform_device *pdev) +{
- struct device *dev = &pdev->dev;
- struct catpt_dev *cdev;
- struct resource *res;
- int ret;
- cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
- if (!cdev)
return -ENOMEM;
- cdev->spec = device_get_match_data(dev);
- if (!cdev->spec)
return -ENODEV;You may save some cycles if you do this before memory allocations.
i.e. define a local for spec, assign and begin the init process only once it's found? Isn't that a loss in most cases? Comes down to:
declare local + later cdev->spec = spec assignment vs unlikely -ENODEV with memory being unnecessarily allocated
Perhaps I'm unaware of what's going on with device_get_match_data, but I believe .probe() won't get called until one of .acpi_match_table ids matches device available on the bus. Existing list of ids won't ever get changed as there are only two platforms available for 2011-2013 ADSP architecture.
- catpt_dev_init(cdev, dev);
- /* map DSP bar address */
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res)
return -ENODEV;- cdev->lpe_ba = devm_ioremap(dev, res->start, resource_size(res));
- if (!cdev->lpe_ba)
return -EIO;- cdev->lpe_base = res->start;
Why the region is not get requested?
- /* map PCI bar address */
- res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
- if (!res)
return -ENODEV;- cdev->pci_ba = devm_ioremap(dev, res->start, resource_size(res));
- if (!cdev->pci_ba)
return -EIO;Ditto.
Comes from catpt_dmac_probe() (dsp.c) making use of devm_ioremap_resource(). If you _get_ requested resource there, the function called in catpt_dmac_probe() will yielrd -EBUSY.
This is based on existing code: /sound/soc/intel/common/sst-acpi.c ::sst_acpi_probe() see mmio assignments. /sound/soc/intel/common/sst-firmate.ce ::dw_probe() see chip->regs assignment.
Perhaps you've found even more problems in existing code than I did..
- /* alloc buffer for storing DRAM context during dx transitions */
- cdev->dxbuf_vaddr = dma_alloc_coherent(dev, catpt_dram_size(cdev),
dmam_alloc_coherent() ?
Nice! Wasn't aware of this helper. Simplifies error-path too.
&cdev->dxbuf_paddr, GFP_KERNEL);- if (!cdev->dxbuf_vaddr)
return -ENOMEM;
- ret = platform_get_irq(pdev, 0);
- if (ret < 0)
goto irq_err;- cdev->irq = ret;
But you may return directly if you get IRQ resource before allocation (despite previous comment).
Indeed, reordering irq-request and dxbuf allocation would alloc for s/goto irq_err/return <err>/ Error-path wouldn't be removed though, as final operation - catpt_probe_components - must be verified before leaving scope.
- platform_set_drvdata(pdev, cdev);
- ret = devm_request_threaded_irq(dev, cdev->irq, catpt_dsp_irq_handler,
catpt_dsp_irq_thread,IRQF_SHARED, "AudioDSP", cdev);- if (ret < 0)
goto irq_err;- ret = catpt_probe_components(cdev);
return ...
With dmam_xxx helper, true.
- if (ret < 0)
goto irq_err;- return 0;
+irq_err:
- dma_free_coherent(cdev->dev, catpt_dram_size(cdev),
cdev->dxbuf_vaddr, cdev->dxbuf_paddr);- return ret;
This will be gone...
Ditto. Thanks!
+}
...
+static const struct acpi_device_id catpt_ids[] = {
- { "INT33C8", (unsigned long)&lpt_desc },
- { "INT3438", (unsigned long)&wpt_desc },
- { },
No need to have comma in terminator line.
Well, that's a habbit to add a ',' at the end of each enumeration line and I bet it's a good one. No problem removing this one though.
+};
...
+static struct platform_driver catpt_acpi_driver = {
- .probe = catpt_acpi_probe,
- .remove = catpt_acpi_remove,
- .driver = {
.name = "catpt_adsp",
.acpi_match_table = ACPI_PTR(catpt_ids),ACPI_PTR() either bogus (when you have depends on ACPI) or mistake that brings you compiler warning (unused variable).
I highly recommend in new code avoid completely ACPI_PTR() and of_match_ptr() macros.
That's something new for me. Thanks for a good advice.
.pm = &catpt_dev_pm,- },
+};
...
+#include <linux/iopoll.h>
Missed headers: bits.h (note, the below guarantees to provide this one) bitops.h io.h (writel(), readl(), etc)
Removed these as registers.h always gets included with other files which already inhering them via nesting. Will update in v5 as requested.
+/* DSP Shim registers */
+#define CATPT_SHIM_CS1 0x0
Please, keep all register definitions of the same width, like 0x00 here or so.
Ack.
+#define CATPT_CS_SFCR(ssp) BIT(27 + ssp)
In all macros, try to be a little bit defensive, e.g. here ssp -> (ssp).
...
+#define CATPT_HMDC_HDDA(e, ch) BIT(8 * e + ch)
...or e -> (e) and ch -> (ch) here.
Agreed, will update in v5.
+#define CATPT_CS_DEFAULT 0x8480040E +#define CATPT_IMC_DEFAULT 0x7FFF0003 +#define CATPT_IMD_DEFAULT 0x7FFF0003 +#define CATPT_CLKCTL_DEFAULT 0x7FF
These looks like set of bit fields, can we describe them either in comments or in the values like GENMASK(x, y) | BIT(z) ?
Let's go with the latter. As explained below, I don't have much info in regard to re-setting registers to their defaults. This knowldge might come in time (and a ton of testing) but certainly, won't be part of this release.
One issue might arise when describing the "reserved" regions as some bits should not be modified by sw normally, but are part of "recommended sequence" anyway. I'll see if there are any among '1's.
+/* PCI Configuration registers */
+#define CATPT_PCI_PMCS 0x84
Why?! We have PCI capability and entire infrastructure for that in PCI core.
...
+#define CATPT_PMCS_PS GENMASK(1, 0) +#define CATPT_PMCS_PS_D3HOT (0x3 << 0)
Ditto.
No need for astonishment : ) Wasn't aware of this, in fact, I count on more experienced kernel developers - like you Andy - to help others in learning about such improvements. Certainly, this isn't knowledge one is going to inherit from developing drivers in Windows environment.
Ack.
+#define CATPT_SSP_SSC0 0x0 +#define CATPT_SSP_SSC1 0x4 +#define CATPT_SSP_SSS 0x8 +#define CATPT_SSP_SSIT 0xC +#define CATPT_SSP_SSD 0x10 +#define CATPT_SSP_SSTO 0x28 +#define CATPT_SSP_SSPSP 0x2C +#define CATPT_SSP_SSTSA 0x30 +#define CATPT_SSP_SSRSA 0x34 +#define CATPT_SSP_SSTSS 0x38 +#define CATPT_SSP_SSC2 0x40 +#define CATPT_SSP_SSPSP2 0x44
Isn't it PXA2xx register set? Can you use their definitions?
Could you be more specific? Wasn't able to find anything useful in /include.
+#define CATPT_SSP_SSC0_DEFAULT 0x0 +#define CATPT_SSP_SSC1_DEFAULT 0x0 +#define CATPT_SSP_SSS_DEFAULT 0xF004 +#define CATPT_SSP_SSIT_DEFAULT 0x0 +#define CATPT_SSP_SSD_DEFAULT 0xC43893A3 +#define CATPT_SSP_SSTO_DEFAULT 0x0 +#define CATPT_SSP_SSPSP_DEFAULT 0x0 +#define CATPT_SSP_SSTSA_DEFAULT 0x0 +#define CATPT_SSP_SSRSA_DEFAULT 0x0 +#define CATPT_SSP_SSTSS_DEFAULT 0x0 +#define CATPT_SSP_SSC2_DEFAULT 0x0 +#define CATPT_SSP_SSPSP2_DEFAULT 0x0
These defaults lack of comments.
Because there aren't any available to choose from. While these are part of "recommended sequence", the only comment attached is: bring hw to their defaults as hw won't reset itself
catpt is an effort of sw and fw guys, no hw input is included as I've found only one guy still @ intel but he is busy with different projects and honestly, even if he would agree, him digging now why was this needed might take weeks. That's 2011 ADSP architecture, not some cutting-edge stuff.