Re: [PATCH 2/2] i2c: xgene-slimpro: Add ACPI support by using PCC mailbox
> > 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. > > How about -EINVAL for these -ENODEV error codes? Do you have any suggestion? -EINVAL will do, I would go for -ENOENT, probably. But it doesn't matter much. signature.asc Description: PGP signature
Re: [PATCH 2/2] i2c: xgene-slimpro: Add ACPI support by using PCC mailbox
> > 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. > > How about -EINVAL for these -ENODEV error codes? Do you have any suggestion? -EINVAL will do, I would go for -ENOENT, probably. But it doesn't matter much. signature.asc Description: PGP signature
Re: [PATCH 2/2] i2c: xgene-slimpro: Add ACPI support by using PCC mailbox
Hi Wolfram, On Thu, Apr 20, 2017 at 1:05 AM, Wolfram Sangwrote: > Hi, > > have you tested these patches also without PCC? So, we can be sure there > is no regression? These patches are tested with/without PCC. > >> 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 >> #include >> >> +#include >> + > > 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(>tx_mutex); >> + init_completion(>rd_complete); > > reinit_completion? OK, > >> + slimpro_i2c_pcc_tx_prepare(ctx, msg); >> + } >> + >> + mutex_init(>tx_mutex); > > Why this mutex, by the way? The i2c-core serializes access to the > adapter. Maybe I am missing something? That's a good point, let me remove this mutex. > >> + cppc_ss = ctx->mbox_chan->con_priv; >> + if (!cppc_ss) { >> + dev_err(>dev, "PPC subspace not found\n"); >> + rc = -ENODEV; >> + goto mbox_err; >> + } >> + >> + if (!ctx->mbox_chan->mbox->txdone_irq) { >> + dev_err(>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(>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. How about -EINVAL for these -ENODEV error codes? Do you have any suggestion? Thank for your comments! Hoan > > Regards, > >Wolfram >
Re: [PATCH 2/2] i2c: xgene-slimpro: Add ACPI support by using PCC mailbox
Hi Wolfram, On Thu, Apr 20, 2017 at 1:05 AM, Wolfram Sang wrote: > Hi, > > have you tested these patches also without PCC? So, we can be sure there > is no regression? These patches are tested with/without PCC. > >> 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 >> #include >> >> +#include >> + > > 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(>tx_mutex); >> + init_completion(>rd_complete); > > reinit_completion? OK, > >> + slimpro_i2c_pcc_tx_prepare(ctx, msg); >> + } >> + >> + mutex_init(>tx_mutex); > > Why this mutex, by the way? The i2c-core serializes access to the > adapter. Maybe I am missing something? That's a good point, let me remove this mutex. > >> + cppc_ss = ctx->mbox_chan->con_priv; >> + if (!cppc_ss) { >> + dev_err(>dev, "PPC subspace not found\n"); >> + rc = -ENODEV; >> + goto mbox_err; >> + } >> + >> + if (!ctx->mbox_chan->mbox->txdone_irq) { >> + dev_err(>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(>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. How about -EINVAL for these -ENODEV error codes? Do you have any suggestion? Thank for your comments! Hoan > > Regards, > >Wolfram >
Re: [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 > #include > > +#include > + 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(>tx_mutex); > + init_completion(>rd_complete); reinit_completion? > + slimpro_i2c_pcc_tx_prepare(ctx, msg); > + } > + > + mutex_init(>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(>dev, "PPC subspace not found\n"); > + rc = -ENODEV; > + goto mbox_err; > + } > + > + if (!ctx->mbox_chan->mbox->txdone_irq) { > + dev_err(>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(>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 signature.asc Description: PGP signature
Re: [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 > #include > > +#include > + 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(>tx_mutex); > + init_completion(>rd_complete); reinit_completion? > + slimpro_i2c_pcc_tx_prepare(ctx, msg); > + } > + > + mutex_init(>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(>dev, "PPC subspace not found\n"); > + rc = -ENODEV; > + goto mbox_err; > + } > + > + if (!ctx->mbox_chan->mbox->txdone_irq) { > + dev_err(>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(>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 signature.asc Description: PGP signature