On Mon, 2015-08-31 at 12:38 -0700, Darren Hart wrote:
> On Thu, Aug 27, 2015 at 12:38:37AM +0800, Qipeng Zha wrote:
> > This driver provides support for P-Unit mailbox IPC on Intel
> > platforms.
> > The heart of the P-Unit is the Foxton microcontroller and its
> > firmware,
> > which provide mailbox interface for power management usage.
> >
> > Signed-off-by: Qipeng Zha <qipeng.zha@xxxxxxxxx>
> >
>
> Thanks Qipeng. In the future, please remember to Cc the people who
> have reviewed
> previous versions so they can provide their Reviewed-by if they are
> satisfied
> with the changes.
It would be nice to see the full version before pushing anywhere.
>
> > ---
> > change in v3
> > Fix compile issue in intel_punit_ipc.h, it happened when built-in
> > and the header file is included in other source file.
> >
> > change in v2
> > Fix issues in code style and comments;
> > Remove "default y" in Kconfig;
> > Remove some header files;
> > Replace request_mem_region with devm_request_mem_region,
> > and same for request_irq;
> > Change to use prefix of IPC_PUNIT_ to define commands;
>
> Thanks for including the changelog.
>
> ...
>
> > IOC3 ETHERNET DRIVER
> > M: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> > diff --git a/arch/x86/include/asm/intel_punit_ipc.h
> > b/arch/x86/include/asm/intel_punit_ipc.h
> > new file mode 100644
> > index 0000000..0537434
> > --- /dev/null
> > +++ b/arch/x86/include/asm/intel_punit_ipc.h
> > @@ -0,0 +1,101 @@
> > +#ifndef _ASM_X86_INTEL_PUNIT_IPC_H_
> > +#define _ASM_X86_INTEL_PUNIT_IPC_H_
> > +
> > +/* Commands supported on CPU core, are handled by different bars,
> > + * unify these commands together here
> > + */
>
> Always include a leading /* empty line for multi-line comments
> please, this is
> in CodingStyle, but checkpatch missed it - likely because /net has
> its own
> rules. I've fixed this in the version I queued to "testing".
>
> ...
>
> > +static int intel_punit_get_bars(struct platform_device *pdev)
> > +{
> > + struct resource *res0, *res1;
> > + void __iomem *addr;
> > + int size;
> > +
> > + res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res0) {
> > + dev_err(&pdev->dev, "Failed to get iomem
> > resource\n");
> > + return -EINVAL;
> > + }
> > + size = resource_size(res0);
> > + if (!devm_request_mem_region(&pdev->dev, res0->start,
> > + size, pdev->name)) {
Shouldn't be devm_ioremap_resource() in both cases?
> > + dev_err(&pdev->dev, "Failed to request iomem
> > resouce\n");
> > + return -EBUSY;
> > + }
> > +
> > + res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (!res1) {
> > + dev_err(&pdev->dev, "Failed to get iomem
> > resource1\n");
> > + return -EINVAL;
> > + }
> > + size = resource_size(res1);
> > + if (!devm_request_mem_region(&pdev->dev, res1->start,
> > + size, pdev->name)) {
> > + dev_err(&pdev->dev, "Failed to request iomem
> > resouce1\n");
> > + return -EBUSY;
> > + }
>
> These return codes, -EINVAL and -EBUSY caught my eye. -EINVAL in
> particular
> seemed odd, I would have expected -ENXIO or -EIO. Checking
> intel_pmc_ipc.c, I
> see it uses -ENXIO for the comparable routine. Is there a
> justification for the
> variability here? If not, I'd prefer to see continuity across these
> similar
> drivers.
>
> > +
> > + addr = ioremap_nocache(res0->start,
> > + resource_size(res0) +
> > resource_size(res1));
> > + if (!addr) {
> > + dev_err(&pdev->dev, "Failed to ioremap ipc
> > base\n");
> > + return -ENOMEM;
> > + }
> > + ipcdev.base[BIOS_MAILBOX] = addr;
> > + addr += MAILBOX_REGISTER_SPACE;
> > + ipcdev.base[GTDRIVER_MAILBOX] = addr;
> > + addr += MAILBOX_REGISTER_SPACE;
> > + ipcdev.base[ISPDRIVER_MAILBOX] = addr;
> > +
> > + return 0;
> > +}
> > +
> > +static int intel_punit_ipc_probe(struct platform_device *pdev)
> > +{
> > + int irq, ret;
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + ipcdev.irq = 0;
> > + dev_warn(&pdev->dev, "Invalid IRQ\n");
> > + } else {
> > + if (devm_request_irq(&pdev->dev, irq,
> > intel_punit_ioc,
> > + IRQF_NO_SUSPEND,
> > "intel_punit_ipc",
> > + &ipcdev)) {
> > + dev_err(&pdev->dev, "Failed to request
> > irq: %d\n", irq);
> > + return -EBUSY;
> > + }
> > + ipcdev.irq = irq;
> > + }
> > +
> > + ret = intel_punit_get_bars(pdev);
> > + if (ret)
> > + return ret;
> > +
>
> This white space usage of the ret, if(ret) return ret block is
> typical and
> nicely separates the logical blocks of the function. There are many
> throughout
> this file that do not use any whitespace. This is fairly minor, but
> if we're
> respinning, please do a pass and add some newlines around similar
> blocks.
>
>
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
��.n��������+%������w��{.n������_���v��z����n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
