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. > > > > [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/ > > > > 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: > 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. 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. -- With Best Regards, Andy Shevchenko
