On Mon, Aug 17, 2020 at 12:02:39PM +0200, Cezary Rojewski wrote:
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!
You're welcome!
...
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?
I hope Rafael accepts the v2 soon and thus may provide an immutable branch to be consumed by others. That's the way how usually we solve cross-subsystem series.
In any case we still have time for better review of the rest of the series.
...
+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:
- LPT/WPT
- BYT/CHT/BDW
- cAVS (SKL+)
- 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.
Some documentation says that Audio is using iDMA 32-bit in (some?) BSW/CHT, some documentation says otherwise (Synopsys DesignWare). Can you somewhere clarify what the actual state of affairs? I remember even some (android?) ASoC code used to have different type of DMA engines because of that.
...
- 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.
In case some will return positive code you may hide the (potential) issue. I prefer be explicit over implicit, means use ' < 0' only where it makes sense.
goto exit;
...
- 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.
Actually you may simplify by calling devm_add_action_or_reset() instead.
...
- 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.
Up to you but I consider cleaner code if we don't do heavier operation ahead when lighter ones can fail.
...
- /* 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..
Hmm... But isn't catpt_dmac_probe(), actually what is in the DMA engine driver beneath, should take care of the requesting *and* remapping resource?
...
.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.
Basically of_match_ptr / ACPI_PTR should go together with ugly ifdeffery, otherwise neither of them should be present. If the driver can be compiled but won't be functional w/o OF / ACPI dependency, then we do something like
depends on ACPI || COMPILE_TEST
to give a hint to the user.
...
+#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.
The rule of thumb is to include headers which we are direct users of. This is listed in Submit Patches Checklist document AFAIR.
...
+#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.
include/linux/pxa2xx_ssp.h
...
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.
I understand, but try your best to leave at least a little trail of these... Sometimes, btw, so called Production Kernel repository (patches there) may give a hint. Lately, during AtomISP v2 resurrection, it appears that Intel Aero platform has support there and a lot of quirks available somewhere.