On Wed, Aug 12, 2020 at 10:57:41PM +0200, Cezary Rojewski wrote:
Declare base structures, registers and device routines for the catpt solution. Catpt deprecates and is a direct replacement for sound/soc/intel/haswell. Supports Lynxpoint and Wildcat Point both.
Before doing v5 give a chance to review entire series, please!
...
+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.
[1]: https://lore.kernel.org/linux-acpi/20200813175729.15088-1-andriy.shevchenko@... [2]: https://lore.kernel.org/linux-acpi/20200813175729.15088-4-andriy.shevchenko@...
...
+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.
- struct catpt_ipc ipc;
- void __iomem *pci_ba;
- void __iomem *lpe_ba;
- u32 lpe_base;
- int irq;
- const struct catpt_spec *spec;
- struct completion fw_ready;
- struct resource dram;
- struct resource iram;
- struct resource *scratch;
- struct catpt_mixer_stream_info mixer;
- struct catpt_module_type modules[CATPT_MODULE_COUNT];
- struct catpt_ssp_device_format devfmt[CATPT_SSP_COUNT];
- struct list_head stream_list;
- spinlock_t list_lock;
- struct mutex clk_mutex;
- struct catpt_dx_context dx_ctx;
- void *dxbuf_vaddr;
- dma_addr_t dxbuf_paddr;
+};
...
+#define CATPT_IPC_ERROR(err) ((err < 0) ? err : -EREMOTEIO)
err -> (err) in all cases of right side.
...
+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?
- struct list_head node;
+};
...
+#ifdef CONFIG_PM
Perhaps __maybe_unused?
...
- 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?
goto exit;
...
+#ifdef CONFIG_PM_SLEEP
Perhaps __maybe_unused?
...
- board = platform_device_register_data(NULL, mach->drv_name,
PLATFORM_DEVID_NONE,(const void *)mach, sizeof(*mach));- if (!board) {
Here obviously not correct check.
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;
...
- mutex_init(&cdev->clk_mutex);
+ blank line.
- /*
* Mark both device formats as uninitialized. Once corresponding* cpu_dai's pcm is created, proper values are assigned
Please, use period(s) in multi-line comments.
*/
...
+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.
- 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.
- /* alloc buffer for storing DRAM context during dx transitions */
- cdev->dxbuf_vaddr = dma_alloc_coherent(dev, catpt_dram_size(cdev),
dmam_alloc_coherent() ?
&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).
- 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 ...
- 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...
+}
...
+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.
+};
...
+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.
.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)
...
+/* DSP Shim registers */
+#define CATPT_SHIM_CS1 0x0
Please, keep all register definitions of the same width, like 0x00 here or so.
...
+#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.
...
+#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) ?
...
+/* 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.
...
+#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?
...
+#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.