Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1627179 > unrolled thread
| Started by | Wolfram Sang <wsa@the-dreams.de> |
|---|---|
| First post | 2017-04-20 10:10 +0200 |
| Last post | 2017-04-21 08:40 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 2/2] i2c: xgene-slimpro: Add ACPI support by using PCC mailbox Wolfram Sang <wsa@the-dreams.de> - 2017-04-20 10:10 +0200
Re: [PATCH 2/2] i2c: xgene-slimpro: Add ACPI support by using PCC mailbox Hoan Tran <hotran@apm.com> - 2017-04-20 22:40 +0200
Re: [PATCH 2/2] i2c: xgene-slimpro: Add ACPI support by using PCC mailbox Wolfram Sang <wsa@the-dreams.de> - 2017-04-21 08:40 +0200
| From | Wolfram Sang <wsa@the-dreams.de> |
|---|---|
| Date | 2017-04-20 10:10 +0200 |
| Subject | Re: [PATCH 2/2] i2c: xgene-slimpro: Add ACPI support by using PCC mailbox |
| Message-ID | <tyfk6-6tb-11@gated-at.bofh.it> |
[Multipart message — attachments visible in raw view] — view raw
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
[toc] | [next] | [standalone]
| From | Hoan Tran <hotran@apm.com> |
|---|---|
| Date | 2017-04-20 22:40 +0200 |
| Subject | Re: [PATCH 2/2] i2c: xgene-slimpro: Add ACPI support by using PCC mailbox |
| Message-ID | <tyr1U-565-15@gated-at.bofh.it> |
| In reply to | #1627179 |
Hi Wolfram,
On Thu, Apr 20, 2017 at 1:05 AM, Wolfram Sang <wsa@the-dreams.de> 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 <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?
OK,
>
>> + 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?
That's a good point, let me remove this mutex.
>
>> + 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.
How about -EINVAL for these -ENODEV error codes? Do you have any suggestion?
Thank for your comments!
Hoan
>
> Regards,
>
> Wolfram
>
[toc] | [prev] | [next] | [standalone]
| From | Wolfram Sang <wsa@the-dreams.de> |
|---|---|
| Date | 2017-04-21 08:40 +0200 |
| Message-ID | <tyAox-2um-5@gated-at.bofh.it> |
| In reply to | #1627801 |
[Multipart message — attachments visible in raw view] — view raw
> > 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.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web