lkml.org 
[lkml]   [2017]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2] i2c: xgene-slimpro: Add ACPI support by using PCC mailbox
    Hi,

    have you tested these patches also without PCC? So, we can be sure there
    is no regression?

    > diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
    > index 96545aa..a5771c1 100644
    > --- a/drivers/i2c/busses/i2c-xgene-slimpro.c
    > +++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
    > @@ -32,6 +32,8 @@
    > #include <linux/platform_device.h>
    > #include <linux/version.h>
    >
    > +#include <acpi/pcc.h>
    > +

    Please keep it sorted alphabetically.

    > +#define PCC_CMD_GENERATE_DB_INT BIT(15)
    > +#define PCC_STS_CMD_COMPLETE BIT(0)
    > +#define PCC_STS_SCI_DOORBELL BIT(1)
    > +#define PCC_STS_ERR BIT(2)
    > +#define PCC_STS_PLAT_NOTIFY BIT(3)

    Please keep it sorted by number.

    > + if (!acpi_disabled) {
    > + mutex_lock(&ctx->tx_mutex);
    > + init_completion(&ctx->rd_complete);

    reinit_completion?

    > + slimpro_i2c_pcc_tx_prepare(ctx, msg);
    > + }
    > +
    > + mutex_init(&ctx->tx_mutex);

    Why this mutex, by the way? The i2c-core serializes access to the
    adapter. Maybe I am missing something?

    > + cppc_ss = ctx->mbox_chan->con_priv;
    > + if (!cppc_ss) {
    > + dev_err(&pdev->dev, "PPC subspace not found\n");
    > + rc = -ENODEV;
    > + goto mbox_err;
    > + }
    > +
    > + if (!ctx->mbox_chan->mbox->txdone_irq) {
    > + dev_err(&pdev->dev, "PCC IRQ not supported\n");
    > + rc = -ENODEV;
    > + goto mbox_err;
    > + }
    >
    > + /*
    > + * This is the shared communication region
    > + * for the OS and Platform to communicate over.
    > + */
    > + ctx->comm_base_addr = cppc_ss->base_address;
    > + if (ctx->comm_base_addr) {
    > + ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
    > + cppc_ss->length,
    > + MEMREMAP_WB);
    > + } else {
    > + dev_err(&pdev->dev, "Failed to get PCC comm region\n");
    > + rc = -ENODEV;
    > + goto mbox_err;
    > + }

    I think it doesn't make sense to print a dev_err and return ENODEV which
    is treated by the driver core as a non-error. It means "not present, but
    OK". You probably want other error codes here.

    Regards,

    Wolfram

    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2017-04-20 10:06    [W:2.368 / U:0.708 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site