Re: [PATCH v3 2/5] drm/stm: dsi: Adjust dw_mipi_dsi_probe and remove

[Brian Norris]

Hi Nickey,

On Tue, Nov 28, 2017 at 07:20:03PM +0800, Nickey Yang wrote:
> Bridge drivers/helpers shouldn't be clobbering the drvdata, since a
> parent driver might need to own this. Instead, let's return our

Other reviews have suggested this might be described as "SoC glue
driver", not "parent driver". Also, the subject probably should include
either "mipi" or "dw-mipi-dsi".

> 'dw_mipi_dsi' object and have callers pass that back to us for removal.
> So adjust it.
> 
> Signed-off-by: Nickey Yang <nickey.yang@xxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

So, you've split my patch in 2 and called it your own (see patch 1,
which still has the 'From' (i.e., author) line as "Nickey Yang", not
"Brian Norris"). You need to keep the author accurate. (It's good to add
an additional 'Signed-off-by' of your own, but keep the author.)

When developing in your local git tree, make sure that the 'Author' line
is accurate in 'git log'. Then 'git format-patch + git send-email' will
format things correctly, such that your email headers will say "From:
Nickey Yang", but then git-send-email will make sure there's an
additional line within the body of the email to attribute the author.

For example, see how this patch was sent by Doug in a later series, but has my
authorship:

https://patchwork.kernel.org/patch/9188419/

The email header says:

From: Douglas Anderson <dianders@xxxxxxxxxxxx>

but the body of the mail includes:

From: Brian Norris <briannorris@xxxxxxxxxxxx>

This ends up correctly-attributed in the mainline git tree:

commit 36b5d460261f16563f9196c49c936b3e17d237e3
Author: Brian Norris <briannorris@xxxxxxxxxxxx>
Date:   Mon Jun 20 10:56:42 2016 -0700

    phy: rockchip-emmc: configure default output tap delay
...
    Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx>
    Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>


But more importantly: you can't split my patch in 2, because it makes
the series non-bisectable. As you split it, patch 1 is making an API
change, but it doesn't update the drivers that use that API. So the STM
DSI driver doesn't compile if you apply only patch 1. That's discouraged
within the Linux kernel.

So, let's make sure to keep this patch in 1 piece.

Brian

> 
> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> index e5b6310..80f9950 100644
> --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> @@ -66,6 +66,7 @@ enum dsi_color {
>  struct dw_mipi_dsi_stm {
>  	void __iomem *base;
>  	struct clk *pllref_clk;
> +	struct dw_mipi_dsi *dmd;
>  };
>  
>  static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg, u32 val)
> @@ -318,10 +319,11 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>  	dw_mipi_dsi_stm_plat_data.base = dsi->base;
>  	dw_mipi_dsi_stm_plat_data.priv_data = dsi;
>  
> -	ret = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
> -	if (ret) {
> +	dsi->dmd = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
> +	if (IS_ERR(dsi->dmd)) {
>  		DRM_ERROR("Failed to initialize mipi dsi host\n");
>  		clk_disable_unprepare(dsi->pllref_clk);
> +		return PTR_ERR(dsi->dmd);
>  	}
>  
>  	return ret;
> @@ -332,7 +334,7 @@ static int dw_mipi_dsi_stm_remove(struct platform_device *pdev)
>  	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
>  
>  	clk_disable_unprepare(dsi->pllref_clk);
> -	dw_mipi_dsi_remove(pdev);
> +	dw_mipi_dsi_remove(dsi->dmd);
>  
>  	return 0;
>  }
> -- 
> 1.9.1
>