2021-05-07 21:25:54

by Stephen Boyd

Subject: [PATCH 1/3] drm/msm/dp: Simplify aux irq handling code

We don't need to stash away 'isr' in the aux structure to pass to two
functions. Let's use a local variable instead. And we can complete the
completion variable in one place instead of two to simplify the code.

Cc: Dmitry Baryshkov <[email protected]>
Cc: Abhinav Kumar <[email protected]>
Cc: Kuogee Hsieh <[email protected]>
Cc: [email protected]
Cc: Sean Paul <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_aux.c | 22 ++++++++--------------
drivers/gpu/drm/msm/dp/dp_catalog.c | 2 +-
drivers/gpu/drm/msm/dp/dp_catalog.h | 2 +-
3 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 7c22bfe0fc7d..91188466cece 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -27,7 +27,6 @@ struct dp_aux_private {
bool no_send_stop;
u32 offset;
u32 segment;
- u32 isr;

struct drm_dp_aux dp_aux;
};
@@ -181,10 +180,8 @@ static void dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
}
}

-static void dp_aux_native_handler(struct dp_aux_private *aux)
+static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
{
- u32 isr = aux->isr;
-
if (isr & DP_INTR_AUX_I2C_DONE)
aux->aux_error_num = DP_AUX_ERR_NONE;
else if (isr & DP_INTR_WRONG_ADDR)
@@ -197,14 +194,10 @@ static void dp_aux_native_handler(struct dp_aux_private *aux)
aux->aux_error_num = DP_AUX_ERR_PHY;
dp_catalog_aux_clear_hw_interrupts(aux->catalog);
}
-
- complete(&aux->comp);
}

-static void dp_aux_i2c_handler(struct dp_aux_private *aux)
+static void dp_aux_i2c_handler(struct dp_aux_private *aux, u32 isr)
{
- u32 isr = aux->isr;
-
if (isr & DP_INTR_AUX_I2C_DONE) {
if (isr & (DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER))
aux->aux_error_num = DP_AUX_ERR_NACK;
@@ -226,8 +219,6 @@ static void dp_aux_i2c_handler(struct dp_aux_private *aux)
dp_catalog_aux_clear_hw_interrupts(aux->catalog);
}
}
-
- complete(&aux->comp);
}

static void dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
@@ -412,6 +403,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,

void dp_aux_isr(struct drm_dp_aux *dp_aux)
{
+ u32 isr;
struct dp_aux_private *aux;

if (!dp_aux) {
@@ -421,15 +413,17 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)

aux = container_of(dp_aux, struct dp_aux_private, dp_aux);

- aux->isr = dp_catalog_aux_get_irq(aux->catalog);
+ isr = dp_catalog_aux_get_irq(aux->catalog);

if (!aux->cmd_busy)
return;

if (aux->native)
- dp_aux_native_handler(aux);
+ dp_aux_native_handler(aux, isr);
else
- dp_aux_i2c_handler(aux);
+ dp_aux_i2c_handler(aux, isr);
+
+ complete(&aux->comp);
}

void dp_aux_reconfig(struct drm_dp_aux *dp_aux)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index b1a9b1b98f5f..a70c238f34b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -292,7 +292,7 @@ void dp_catalog_dump_regs(struct dp_catalog *dp_catalog)
dump_regs(catalog->io->dp_controller.base + offset, len);
}

-int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
+u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 176a9020a520..502bc0dc7787 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -80,7 +80,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog);
void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
-int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
+u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);

/* DP Controller APIs */
void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 state);
--
https://chromeos.dev

2021-05-24 16:24:24

by Kuogee Hsieh

Subject: Re: [PATCH 1/3] drm/msm/dp: Simplify aux irq handling code

On 2021-05-07 14:25, Stephen Boyd wrote:
> We don't need to stash away 'isr' in the aux structure to pass to two
> functions. Let's use a local variable instead. And we can complete the
> completion variable in one place instead of two to simplify the code.
>
> Cc: Dmitry Baryshkov <[email protected]>
> Cc: Abhinav Kumar <[email protected]>
> Cc: Kuogee Hsieh <[email protected]>
> Cc: [email protected]
> Cc: Sean Paul <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>

Reviewed-by: Kuogee Hsieh <[email protected]>
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 22 ++++++++--------------
> drivers/gpu/drm/msm/dp/dp_catalog.c | 2 +-
> drivers/gpu/drm/msm/dp/dp_catalog.h | 2 +-
> 3 files changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 7c22bfe0fc7d..91188466cece 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -27,7 +27,6 @@ struct dp_aux_private {
> bool no_send_stop;
> u32 offset;
> u32 segment;
> - u32 isr;
>
> struct drm_dp_aux dp_aux;
> };
> @@ -181,10 +180,8 @@ static void dp_aux_cmd_fifo_rx(struct
> dp_aux_private *aux,
> }
> }
>
> -static void dp_aux_native_handler(struct dp_aux_private *aux)
> +static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
> {
> - u32 isr = aux->isr;
> -
> if (isr & DP_INTR_AUX_I2C_DONE)
> aux->aux_error_num = DP_AUX_ERR_NONE;
> else if (isr & DP_INTR_WRONG_ADDR)
> @@ -197,14 +194,10 @@ static void dp_aux_native_handler(struct
> dp_aux_private *aux)
> aux->aux_error_num = DP_AUX_ERR_PHY;
> dp_catalog_aux_clear_hw_interrupts(aux->catalog);
> }
> -
> - complete(&aux->comp);
> }
>
> -static void dp_aux_i2c_handler(struct dp_aux_private *aux)
> +static void dp_aux_i2c_handler(struct dp_aux_private *aux, u32 isr)
> {
> - u32 isr = aux->isr;
> -
> if (isr & DP_INTR_AUX_I2C_DONE) {
> if (isr & (DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER))
> aux->aux_error_num = DP_AUX_ERR_NACK;
> @@ -226,8 +219,6 @@ static void dp_aux_i2c_handler(struct
> dp_aux_private *aux)
> dp_catalog_aux_clear_hw_interrupts(aux->catalog);
> }
> }
> -
> - complete(&aux->comp);
> }
>
> static void dp_aux_update_offset_and_segment(struct dp_aux_private
> *aux,
> @@ -412,6 +403,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> *dp_aux,
>
> void dp_aux_isr(struct drm_dp_aux *dp_aux)
> {
> + u32 isr;
> struct dp_aux_private *aux;
>
> if (!dp_aux) {
> @@ -421,15 +413,17 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
>
> aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
>
> - aux->isr = dp_catalog_aux_get_irq(aux->catalog);
> + isr = dp_catalog_aux_get_irq(aux->catalog);
>
> if (!aux->cmd_busy)
> return;
>
> if (aux->native)
> - dp_aux_native_handler(aux);
> + dp_aux_native_handler(aux, isr);
> else
> - dp_aux_i2c_handler(aux);
> + dp_aux_i2c_handler(aux, isr);
> +
> + complete(&aux->comp);
> }
>
> void dp_aux_reconfig(struct drm_dp_aux *dp_aux)
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index b1a9b1b98f5f..a70c238f34b0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -292,7 +292,7 @@ void dp_catalog_dump_regs(struct dp_catalog
> *dp_catalog)
> dump_regs(catalog->io->dp_controller.base + offset, len);
> }
>
> -int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
> +u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
> {
> struct dp_catalog_private *catalog = container_of(dp_catalog,
> struct dp_catalog_private, dp_catalog);
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
> b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 176a9020a520..502bc0dc7787 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -80,7 +80,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct
> dp_catalog *dp_catalog);
> void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
> void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool
> enable);
> void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
> -int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
> +u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
>
> /* DP Controller APIs */
> void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32
> state);