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@xxxxxxxxxxxxxxx/
[2]: https://lore.kernel.org/linux-acpi/20200813175729.15088-4-andriy.shevchenko@xxxxxxxxxxxxxxx/
...
> +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.
--
With Best Regards,
Andy Shevchenko
