Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 6 Apr 2021 15:40:33 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        kernel@collabora.com, dafna3@gmail.com, airlied@linux.ie,
        daniel@ffwll.ch, enric.balletbo@collabora.com
Subject: Re: [PATCH] drm: bridge: rename the function drm_bridge_hpd_notify
 to drm_bridge_hpd_cb
Message-ID: <YGxWwTTjhFcVDELB@pendragon.ideasonboard.com>
References: <20210330115200.26006-1-dafna.hirschfeld@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20210330115200.26006-1-dafna.hirschfeld@collabora.com>
Precedence: bulk

Hi Dafna,

Thank you for the patch.

On Tue, Mar 30, 2021 at 01:52:00PM +0200, Dafna Hirschfeld wrote:
> drm_bridge_funcs has a function called 'hpd_notify'.
> The function drm_bridge_hpd_notify does not call
> 'hpd_notify' but it calls 'hpd_cb'. This is rather
> confusing. Rename the function to fix this confusion.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c        | 2 +-
>  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 4 ++--
>  drivers/gpu/drm/bridge/display-connector.c          | 2 +-
>  drivers/gpu/drm/bridge/lontium-lt9611uxc.c          | 8 ++++----
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c           | 2 +-
>  drivers/gpu/drm/bridge/ti-tpd12s015.c               | 2 +-
>  drivers/gpu/drm/drm_bridge.c                        | 8 ++++----
>  include/drm/drm_bridge.h                            | 8 ++++----
>  8 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 76555ae64e9c..748f82910f4f 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -449,7 +449,7 @@ static void adv7511_hpd_work(struct work_struct *work)
>  				cec_phys_addr_invalidate(adv7511->cec_adap);
>  			drm_kms_helper_hotplug_event(adv7511->connector.dev);
>  		} else {
> -			drm_bridge_hpd_notify(&adv7511->bridge, status);
> +			drm_bridge_hpd_cb(&adv7511->bridge, status);
>  		}
>  	}
>  }
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index d0c65610ebb5..682da288ff6d 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -794,7 +794,7 @@ static void cdns_mhdp_fw_cb(const struct firmware *fw, void *context)
>  		if (mhdp->connector.dev)
>  			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>  		else
> -			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
> +			drm_bridge_hpd_cb(&mhdp->bridge, cdns_mhdp_detect(mhdp));
>  	}
>  }
>  
> @@ -2314,7 +2314,7 @@ static irqreturn_t cdns_mhdp_irq_handler(int irq, void *data)
>  			else
>  				drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>  		} else {
> -			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
> +			drm_bridge_hpd_cb(&mhdp->bridge, cdns_mhdp_detect(mhdp));
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> index 05eb759da6fc..8ccd69d7fe34 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
> @@ -98,7 +98,7 @@ static irqreturn_t display_connector_hpd_irq(int irq, void *arg)
>  	struct display_connector *conn = arg;
>  	struct drm_bridge *bridge = &conn->bridge;
>  
> -	drm_bridge_hpd_notify(bridge, display_connector_detect(bridge));
> +	drm_bridge_hpd_cb(bridge, display_connector_detect(bridge));
>  
>  	return IRQ_HANDLED;
>  }
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> index fee27952ec6d..58f61b5da605 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> @@ -175,10 +175,10 @@ static void lt9611uxc_hpd_work(struct work_struct *work)
>  		connected = lt9611uxc->hdmi_connected;
>  		mutex_unlock(&lt9611uxc->ocm_lock);
>  
> -		drm_bridge_hpd_notify(&lt9611uxc->bridge,
> -				      connected ?
> -				      connector_status_connected :
> -				      connector_status_disconnected);
> +		drm_bridge_hpd_cb(&lt9611uxc->bridge,
> +				  connected ?
> +				  connector_status_connected :
> +				  connector_status_disconnected);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index dda4fa9a1a08..984ab5c4bc71 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -3026,7 +3026,7 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>  
>  		if (hdmi->bridge.dev) {
>  			drm_helper_hpd_irq_event(hdmi->bridge.dev);
> -			drm_bridge_hpd_notify(&hdmi->bridge, status);
> +			drm_bridge_hpd_cb(&hdmi->bridge, status);
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/bridge/ti-tpd12s015.c b/drivers/gpu/drm/bridge/ti-tpd12s015.c
> index e0e015243a60..2f079b6f51bc 100644
> --- a/drivers/gpu/drm/bridge/ti-tpd12s015.c
> +++ b/drivers/gpu/drm/bridge/ti-tpd12s015.c
> @@ -103,7 +103,7 @@ static irqreturn_t tpd12s015_hpd_isr(int irq, void *data)
>  	struct tpd12s015_device *tpd = data;
>  	struct drm_bridge *bridge = &tpd->bridge;
>  
> -	drm_bridge_hpd_notify(bridge, tpd12s015_detect(bridge));
> +	drm_bridge_hpd_cb(bridge, tpd12s015_detect(bridge));
>  
>  	return IRQ_HANDLED;
>  }
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 64f0effb52ac..653761a0d5f9 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1173,7 +1173,7 @@ void drm_bridge_hpd_disable(struct drm_bridge *bridge)
>  EXPORT_SYMBOL_GPL(drm_bridge_hpd_disable);
>  
>  /**
> - * drm_bridge_hpd_notify - notify hot plug detection events
> + * drm_bridge_hpd_cb - notify hot plug detection events

This function is still documented as notifying hot plug detection
events, so drm_bridge_hpd_cb() isn't a great name :-S I do agree there's
confusion with the current naming scheme though.

bridge->hpd_cb() is an internal callback, not part of bridge ops, so I'd
rather not expose its name in the public drm_bridge_hpd_notify() API.
Could we find a better naming scheme ?

>   * @bridge: bridge control structure
>   * @status: output connection status
>   *
> @@ -1183,15 +1183,15 @@ EXPORT_SYMBOL_GPL(drm_bridge_hpd_disable);
>   *
>   * This function shall be called in a context that can sleep.
>   */
> -void drm_bridge_hpd_notify(struct drm_bridge *bridge,
> -			   enum drm_connector_status status)
> +void drm_bridge_hpd_cb(struct drm_bridge *bridge,
> +		       enum drm_connector_status status)
>  {
>  	mutex_lock(&bridge->hpd_mutex);
>  	if (bridge->hpd_cb)
>  		bridge->hpd_cb(bridge->hpd_data, status);
>  	mutex_unlock(&bridge->hpd_mutex);
>  }
> -EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
> +EXPORT_SYMBOL_GPL(drm_bridge_hpd_cb);
>  
>  #ifdef CONFIG_OF
>  /**
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 2195daa289d2..ab54715eda8b 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -605,7 +605,7 @@ struct drm_bridge_funcs {
>  	 * @hpd_enable:
>  	 *
>  	 * Enable hot plug detection. From now on the bridge shall call
> -	 * drm_bridge_hpd_notify() each time a change is detected in the output
> +	 * drm_bridge_hpd_cb() each time a change is detected in the output
>  	 * connection status, until hot plug detection gets disabled with
>  	 * @hpd_disable.
>  	 *
> @@ -620,7 +620,7 @@ struct drm_bridge_funcs {
>  	 * @hpd_disable:
>  	 *
>  	 * Disable hot plug detection. Once this function returns the bridge
> -	 * shall not call drm_bridge_hpd_notify() when a change in the output
> +	 * shall not call drm_bridge_hpd_cb() when a change in the output
>  	 * connection status occurs.
>  	 *
>  	 * This callback is optional and shall only be implemented by bridges
> @@ -878,8 +878,8 @@ void drm_bridge_hpd_enable(struct drm_bridge *bridge,
>  				      enum drm_connector_status status),
>  			   void *data);
>  void drm_bridge_hpd_disable(struct drm_bridge *bridge);
> -void drm_bridge_hpd_notify(struct drm_bridge *bridge,
> -			   enum drm_connector_status status);
> +void drm_bridge_hpd_cb(struct drm_bridge *bridge,
> +		       enum drm_connector_status status);
>  
>  #ifdef CONFIG_DRM_PANEL_BRIDGE
>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);

-- 
Regards,

Laurent Pinchart