From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: pierre-louis.bossart@linux.intel.com,
alsa-devel@alsa-project.org, filip.kaczmarski@intel.com,
harshapriya.n@intel.com, marcin.barlik@intel.com,
zwisler@google.com, lgirdwood@gmail.com, tiwai@suse.com,
filip.proborszcz@intel.com, broonie@kernel.org,
amadeuszx.slawinski@linux.intel.com, michal.wasko@intel.com,
cujomalainey@chromium.org, krzysztof.hejmowski@intel.com,
ppapierkowski@habana.ai, vamshi.krishna.gopal@intel.com
Subject: Re: [PATCH v4 01/13] ASoC: Intel: Add catpt device
Date: Thu, 13 Aug 2020 21:29:08 +0300 [thread overview]
Message-ID: <20200813182908.GA1891694@smile.fi.intel.com> (raw)
In-Reply-To: <20200812205753.29115-2-cezary.rojewski@intel.com>
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@linux.intel.com/
[2]: https://lore.kernel.org/linux-acpi/20200813175729.15088-4-andriy.shevchenko@linux.intel.com/
...
> +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
next prev parent reply other threads:[~2020-08-13 18:32 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-12 20:57 [PATCH v4 00/13] ASoC: Intel: Catpt - Lynx and Wildcat point Cezary Rojewski
2020-08-12 20:57 ` [PATCH v4 01/13] ASoC: Intel: Add catpt device Cezary Rojewski
2020-08-13 18:29 ` Andy Shevchenko [this message]
2020-08-17 10:02 ` Cezary Rojewski
2020-08-18 10:07 ` Andy Shevchenko
2020-08-19 13:26 ` Cezary Rojewski
2020-08-19 13:43 ` Andy Shevchenko
2020-08-25 9:32 ` Cezary Rojewski
2020-08-25 13:18 ` Andy Shevchenko
2020-08-25 13:19 ` Andy Shevchenko
2020-08-25 20:43 ` Cezary Rojewski
2020-08-12 20:57 ` [PATCH v4 02/13] ASoC: Intel: catpt: Define DSP operations Cezary Rojewski
2020-08-13 18:51 ` Andy Shevchenko
2020-08-17 11:12 ` Cezary Rojewski
2020-08-18 11:50 ` Andy Shevchenko
2020-08-19 13:46 ` Cezary Rojewski
2020-08-19 14:21 ` Andy Shevchenko
2020-08-19 14:54 ` Cezary Rojewski
2020-08-20 7:30 ` Cezary Rojewski
2020-08-20 9:00 ` Andy Shevchenko
2020-08-24 16:33 ` Cezary Rojewski
2020-08-25 13:16 ` Andy Shevchenko
2020-08-25 13:23 ` Andy Shevchenko
2020-08-27 10:06 ` Cezary Rojewski
2020-08-12 20:57 ` [PATCH v4 03/13] ASoC: Intel: catpt: Firmware loading and context restore Cezary Rojewski
2020-08-12 20:57 ` [PATCH v4 04/13] ASoC: Intel: catpt: Implement IPC protocol Cezary Rojewski
2020-08-12 20:57 ` [PATCH v4 05/13] ASoC: Intel: catpt: Add IPC messages Cezary Rojewski
2020-08-12 20:57 ` [PATCH v4 06/13] ASoC: Intel: catpt: PCM operations Cezary Rojewski
2020-08-12 20:57 ` [PATCH v4 07/13] ASoC: Intel: catpt: Event tracing Cezary Rojewski
2020-08-12 20:57 ` [PATCH v4 08/13] ASoC: Intel: catpt: Simple sysfs attributes Cezary Rojewski
2020-08-12 20:57 ` [PATCH v4 09/13] ASoC: Intel: Select catpt and deprecate haswell Cezary Rojewski
2020-08-12 20:57 ` [PATCH v4 10/13] ASoC: Intel: haswell: Remove haswell-solution specific code Cezary Rojewski
2020-08-13 18:57 ` Andy Shevchenko
2020-08-12 20:57 ` [PATCH v4 11/13] ASoC: Intel: broadwell: " Cezary Rojewski
2020-08-13 18:56 ` Andy Shevchenko
2020-08-12 20:57 ` [PATCH v4 12/13] ASoC: Intel: bdw-5650: " Cezary Rojewski
2020-08-13 18:56 ` Andy Shevchenko
2020-08-12 20:57 ` [PATCH v4 13/13] ASoC: Intel: bdw-5677: " Cezary Rojewski
2020-08-13 18:57 ` Andy Shevchenko
2020-08-13 8:30 ` [PATCH v4 00/13] ASoC: Intel: Catpt - Lynx and Wildcat point Amadeusz Sławiński
2020-08-13 16:00 ` Liam Girdwood
2020-08-13 18:11 ` Cezary Rojewski
2020-08-13 19:03 ` Liam Girdwood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200813182908.GA1891694@smile.fi.intel.com \
--to=andriy.shevchenko@intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=amadeuszx.slawinski@linux.intel.com \
--cc=broonie@kernel.org \
--cc=cezary.rojewski@intel.com \
--cc=cujomalainey@chromium.org \
--cc=filip.kaczmarski@intel.com \
--cc=filip.proborszcz@intel.com \
--cc=harshapriya.n@intel.com \
--cc=krzysztof.hejmowski@intel.com \
--cc=lgirdwood@gmail.com \
--cc=marcin.barlik@intel.com \
--cc=michal.wasko@intel.com \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=ppapierkowski@habana.ai \
--cc=tiwai@suse.com \
--cc=vamshi.krishna.gopal@intel.com \
--cc=zwisler@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).