Re: patch "drivers: thermal: st: remove several sparse warnings" added to thermal-soc tree

Re: patch "drivers: thermal: st: remove several sparse warnings" added to thermal-soc tree

[Lee Jones]

On Tue, 07 Apr 2015, Eduardo Valentin wrote:

> 
> This is a simple automated notification to let you know that 
> I've just added the patch titled
> 
>     drivers: thermal: st: remove several sparse warnings
> 
> to my thermal-soc git tree which can be found at
>     git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git
> in the fixes branch.
> 
> The patch will show up in the next release of the linux-next tree
> (usually sometime within the next 24 hours during the week.)
> 
> The patch will hopefully also be merged in Linus's tree for the
> next -rc kernel release.
> 
> If you have any questions about this process, please let me know.
> 
> All the best,
> 
> Eduardo Valentin
> 
> ----------
> From 541d529f9845d249d1cb84f1b395e48f0a117e3f Mon Sep 17 00:00:00 2001
> From: Eduardo Valentin <edubezval@gmail.com>
> Date: Tue, 7 Apr 2015 13:42:12 -0700
> Subject: drivers: thermal: st: remove several sparse warnings
> 
> Simple patch to make symbols static. Symbols that are not
> shared with other parts of the kernel can be made static.
> This change also removes several sparse complains.
> 
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Ajit Pal Singh <ajitpal.singh@st.com>
> Cc: linux-pm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Hold on a second, you can't do that.

Adding these Cc: tags means "this person has been given the opportunity
to comment on the patch but has chosen not to".  However, you've
applied this patch ~1min after sending it to the lists.

Please leave this patch on the lists for some time (I suggest at least
one week) before applying.  This process can be sped-up if the correct
Acks are received.

> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> ---
>  drivers/thermal/st/st_thermal.c        |  2 +-
>  drivers/thermal/st/st_thermal_memmap.c |  8 ++++----
>  drivers/thermal/st/st_thermal_syscfg.c | 12 ++++++------
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/thermal/st/st_thermal.c b/drivers/thermal/st/st_thermal.c
> index d1ec580..76c515d 100644
> --- a/drivers/thermal/st/st_thermal.c
> +++ b/drivers/thermal/st/st_thermal.c
> @@ -25,7 +25,7 @@
>   * Function to allocate regfields which are common
>   * between syscfg and memory mapped based sensors
>   */
> -int st_thermal_alloc_regfields(struct st_thermal_sensor *sensor)
> +static int st_thermal_alloc_regfields(struct st_thermal_sensor *sensor)
>  {
>  	struct device *dev = sensor->dev;
>  	struct regmap *regmap = sensor->regmap;
> diff --git a/drivers/thermal/st/st_thermal_memmap.c b/drivers/thermal/st/st_thermal_memmap.c
> index bd22339..fc0c9e1 100644
> --- a/drivers/thermal/st/st_thermal_memmap.c
> +++ b/drivers/thermal/st/st_thermal_memmap.c
> @@ -157,7 +157,7 @@ static const struct st_thermal_sensor_ops st_mmap_sensor_ops = {
>  };
>  
>  /* Compatible device data stih416 mpe thermal sensor */
> -const struct st_thermal_compat_data st_416mpe_cdata = {
> +static const struct st_thermal_compat_data st_416mpe_cdata = {
>  	.reg_fields		= st_mmap_thermal_regfields,
>  	.ops			= &st_mmap_sensor_ops,
>  	.calibration_val	= 14,
> @@ -166,7 +166,7 @@ const struct st_thermal_compat_data st_416mpe_cdata = {
>  };
>  
>  /* Compatible device data stih407 thermal sensor */
> -const struct st_thermal_compat_data st_407_cdata = {
> +static const struct st_thermal_compat_data st_407_cdata = {
>  	.reg_fields		= st_mmap_thermal_regfields,
>  	.ops			= &st_mmap_sensor_ops,
>  	.calibration_val	= 16,
> @@ -181,12 +181,12 @@ static const struct of_device_id st_mmap_thermal_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, st_mmap_thermal_of_match);
>  
> -int st_mmap_probe(struct platform_device *pdev)
> +static int st_mmap_probe(struct platform_device *pdev)
>  {
>  	return st_thermal_register(pdev,  st_mmap_thermal_of_match);
>  }
>  
> -int st_mmap_remove(struct platform_device *pdev)
> +static int st_mmap_remove(struct platform_device *pdev)
>  {
>  	return st_thermal_unregister(pdev);
>  }
> diff --git a/drivers/thermal/st/st_thermal_syscfg.c b/drivers/thermal/st/st_thermal_syscfg.c
> index 65eab72..3df5b789 100644
> --- a/drivers/thermal/st/st_thermal_syscfg.c
> +++ b/drivers/thermal/st/st_thermal_syscfg.c
> @@ -104,7 +104,7 @@ static const struct st_thermal_sensor_ops st_syscfg_sensor_ops = {
>  };
>  
>  /* Compatible device data for stih415 sas thermal sensor */
> -const struct st_thermal_compat_data st_415sas_cdata = {
> +static const struct st_thermal_compat_data st_415sas_cdata = {
>  	.sys_compat		= "st,stih415-front-syscfg",
>  	.reg_fields		= st_415sas_regfields,
>  	.ops			= &st_syscfg_sensor_ops,
> @@ -114,7 +114,7 @@ const struct st_thermal_compat_data st_415sas_cdata = {
>  };
>  
>  /* Compatible device data for stih415 mpe thermal sensor */
> -const struct st_thermal_compat_data st_415mpe_cdata = {
> +static const struct st_thermal_compat_data st_415mpe_cdata = {
>  	.sys_compat		= "st,stih415-system-syscfg",
>  	.reg_fields		= st_415mpe_regfields,
>  	.ops			= &st_syscfg_sensor_ops,
> @@ -124,7 +124,7 @@ const struct st_thermal_compat_data st_415mpe_cdata = {
>  };
>  
>  /* Compatible device data for stih416 sas thermal sensor */
> -const struct st_thermal_compat_data st_416sas_cdata = {
> +static const struct st_thermal_compat_data st_416sas_cdata = {
>  	.sys_compat		= "st,stih416-front-syscfg",
>  	.reg_fields		= st_416sas_regfields,
>  	.ops			= &st_syscfg_sensor_ops,
> @@ -134,7 +134,7 @@ const struct st_thermal_compat_data st_416sas_cdata = {
>  };
>  
>  /* Compatible device data for stid127 thermal sensor */
> -const struct st_thermal_compat_data st_127_cdata = {
> +static const struct st_thermal_compat_data st_127_cdata = {
>  	.sys_compat		= "st,stid127-cpu-syscfg",
>  	.reg_fields		= st_127_regfields,
>  	.ops			= &st_syscfg_sensor_ops,
> @@ -152,12 +152,12 @@ static const struct of_device_id st_syscfg_thermal_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, st_syscfg_thermal_of_match);
>  
> -int st_syscfg_probe(struct platform_device *pdev)
> +static int st_syscfg_probe(struct platform_device *pdev)
>  {
>  	return st_thermal_register(pdev, st_syscfg_thermal_of_match);
>  }
>  
> -int st_syscfg_remove(struct platform_device *pdev)
> +static int st_syscfg_remove(struct platform_device *pdev)
>  {
>  	return st_thermal_unregister(pdev);
>  }

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: patch "drivers: thermal: st: remove several sparse warnings" added to thermal-soc tree

[Pavel Machek]

On Wed 2015-04-08 08:26:34, Lee Jones wrote:
> On Tue, 07 Apr 2015, Eduardo Valentin wrote:
> 
> > 
> > This is a simple automated notification to let you know that 
> > I've just added the patch titled
> > 
> >     drivers: thermal: st: remove several sparse warnings
> > 
> > to my thermal-soc git tree which can be found at
> >     git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git
> > in the fixes branch.
> > 
> > The patch will show up in the next release of the linux-next tree
> > (usually sometime within the next 24 hours during the week.)
> > 
> > The patch will hopefully also be merged in Linus's tree for the
> > next -rc kernel release.
> > 
> > If you have any questions about this process, please let me know.
> > 
> > All the best,
> > 
> > Eduardo Valentin
> > 
> > ----------
> > From 541d529f9845d249d1cb84f1b395e48f0a117e3f Mon Sep 17 00:00:00 2001
> > From: Eduardo Valentin <edubezval@gmail.com>
> > Date: Tue, 7 Apr 2015 13:42:12 -0700
> > Subject: drivers: thermal: st: remove several sparse warnings
> > 
> > Simple patch to make symbols static. Symbols that are not
> > shared with other parts of the kernel can be made static.
> > This change also removes several sparse complains.
> > 
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Ajit Pal Singh <ajitpal.singh@st.com>
> > Cc: linux-pm@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> 
> Hold on a second, you can't do that.
> 
> Adding these Cc: tags means "this person has been given the opportunity
> to comment on the patch but has chosen not to".  However, you've
> applied this patch ~1min after sending it to the lists.

Cc means -- that person was Cced. And there's no harm in having patch
available in git for easy testing.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: patch "drivers: thermal: st: remove several sparse warnings" added to thermal-soc tree

[Lee Jones]

On Wed, 08 Apr 2015, Pavel Machek wrote:

> On Wed 2015-04-08 08:26:34, Lee Jones wrote:
> > On Tue, 07 Apr 2015, Eduardo Valentin wrote:
> > 
> > > 
> > > This is a simple automated notification to let you know that 
> > > I've just added the patch titled
> > > 
> > >     drivers: thermal: st: remove several sparse warnings
> > > 
> > > to my thermal-soc git tree which can be found at
> > >     git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git
> > > in the fixes branch.
> > > 
> > > The patch will show up in the next release of the linux-next tree
> > > (usually sometime within the next 24 hours during the week.)
> > > 
> > > The patch will hopefully also be merged in Linus's tree for the
> > > next -rc kernel release.
> > > 
> > > If you have any questions about this process, please let me know.
> > > 
> > > All the best,
> > > 
> > > Eduardo Valentin
> > > 
> > > ----------
> > > From 541d529f9845d249d1cb84f1b395e48f0a117e3f Mon Sep 17 00:00:00 2001
> > > From: Eduardo Valentin <edubezval@gmail.com>
> > > Date: Tue, 7 Apr 2015 13:42:12 -0700
> > > Subject: drivers: thermal: st: remove several sparse warnings
> > > 
> > > Simple patch to make symbols static. Symbols that are not
> > > shared with other parts of the kernel can be made static.
> > > This change also removes several sparse complains.
> > > 
> > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > Cc: Lee Jones <lee.jones@linaro.org>
> > > Cc: Pavel Machek <pavel@ucw.cz>
> > > Cc: Ajit Pal Singh <ajitpal.singh@st.com>
> > > Cc: linux-pm@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > 
> > Hold on a second, you can't do that.
> > 
> > Adding these Cc: tags means "this person has been given the opportunity
> > to comment on the patch but has chosen not to".  However, you've
> > applied this patch ~1min after sending it to the lists.
> 
> Cc means -- that person was Cced.

You just made that up out of thin air.  That's not what it means at all.

Documentation/SubmittingPatches:

	"If a person has had the opportunity to comment on a patch,
	but has not provided such comments, you may optionally add a
	"Cc:" tag to the patch. This is the only tag which might be
	added without an explicit action by the person it names - but
	it should indicate that this person was copied on the patch.
	This tag documents that potentially interested parties have
	been included in the discussion."

To add people on Cc, then immediately apply the patch is fundamentally
wrong.

> And there's no harm in having patch available in git for easy testing.

If we weren't so deep into the release cycle, I'd agree with you.  But
even if we weren't so close to the merge-window, I'd still expect a
note to the tune of "tentatively applying this for reason X for early
soak testing in -next".  Although X would have to be a pretty good
reason, as that's not usually how we do things.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: patch "drivers: thermal: st: remove several sparse warnings" added to thermal-soc tree

[Pavel Machek]

On Wed 2015-04-08 10:45:07, Lee Jones wrote:
> On Wed, 08 Apr 2015, Pavel Machek wrote:
> 
> > On Wed 2015-04-08 08:26:34, Lee Jones wrote:
> > > On Tue, 07 Apr 2015, Eduardo Valentin wrote:
> > > 
> > > > 
> > > > This is a simple automated notification to let you know that 
> > > > I've just added the patch titled
> > > > 
> > > >     drivers: thermal: st: remove several sparse warnings
> > > > 
> > > > to my thermal-soc git tree which can be found at
> > > >     git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git
> > > > in the fixes branch.
> > > > 
> > > > The patch will show up in the next release of the linux-next tree
> > > > (usually sometime within the next 24 hours during the week.)
> > > > 
> > > > The patch will hopefully also be merged in Linus's tree for the
> > > > next -rc kernel release.
> > > > 
> > > > If you have any questions about this process, please let me know.
> > > > 
> > > > All the best,
> > > > 
> > > > Eduardo Valentin
> > > > 
> > > > ----------
> > > > From 541d529f9845d249d1cb84f1b395e48f0a117e3f Mon Sep 17 00:00:00 2001
> > > > From: Eduardo Valentin <edubezval@gmail.com>
> > > > Date: Tue, 7 Apr 2015 13:42:12 -0700
> > > > Subject: drivers: thermal: st: remove several sparse warnings
> > > > 
> > > > Simple patch to make symbols static. Symbols that are not
> > > > shared with other parts of the kernel can be made static.
> > > > This change also removes several sparse complains.
> > > > 
> > > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > > Cc: Lee Jones <lee.jones@linaro.org>
> > > > Cc: Pavel Machek <pavel@ucw.cz>
> > > > Cc: Ajit Pal Singh <ajitpal.singh@st.com>
> > > > Cc: linux-pm@vger.kernel.org
> > > > Cc: linux-kernel@vger.kernel.org
> > > 
> > > Hold on a second, you can't do that.
> > > 
> > > Adding these Cc: tags means "this person has been given the opportunity
> > > to comment on the patch but has chosen not to".  However, you've
> > > applied this patch ~1min after sending it to the lists.
> > 
> > Cc means -- that person was Cced.
> 
> You just made that up out of thin air.  That's not what it means at all.
> 
> Documentation/SubmittingPatches:
> 
> 	"If a person has had the opportunity to comment on a patch,
> 	but has not provided such comments, you may optionally add a
> 	"Cc:" tag to the patch. This is the only tag which might be
> 	added without an explicit action by the person it names - but
> 	it should indicate that this person was copied on the patch.
> 	This tag documents that potentially interested parties have
> 	been included in the discussion."
> 
> To add people on Cc, then immediately apply the patch is fundamentally
> wrong.

That is not how I seen it working before.

> > And there's no harm in having patch available in git for easy testing.
> 
> If we weren't so deep into the release cycle, I'd agree with you.  But
> even if we weren't so close to the merge-window, I'd still expect a
> note to the tune of "tentatively applying this for reason X for early
> soak testing in -next".  Although X would have to be a pretty good
> reason, as that's not usually how we do things.

Patches should spend a week or so in -next (that's documented
somewhere), so people in the Cc list will have their chance to
comment.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: patch "drivers: thermal: st: remove several sparse warnings" added to thermal-soc tree

[Lee Jones]

On Wed, 08 Apr 2015, Pavel Machek wrote:

> On Wed 2015-04-08 10:45:07, Lee Jones wrote:
> > On Wed, 08 Apr 2015, Pavel Machek wrote:
> > 
> > > On Wed 2015-04-08 08:26:34, Lee Jones wrote:
> > > > On Tue, 07 Apr 2015, Eduardo Valentin wrote:
> > > > 
> > > > > 
> > > > > This is a simple automated notification to let you know that 
> > > > > I've just added the patch titled
> > > > > 
> > > > >     drivers: thermal: st: remove several sparse warnings
> > > > > 
> > > > > to my thermal-soc git tree which can be found at
> > > > >     git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git
> > > > > in the fixes branch.
> > > > > 
> > > > > The patch will show up in the next release of the linux-next tree
> > > > > (usually sometime within the next 24 hours during the week.)
> > > > > 
> > > > > The patch will hopefully also be merged in Linus's tree for the
> > > > > next -rc kernel release.
> > > > > 
> > > > > If you have any questions about this process, please let me know.
> > > > > 
> > > > > All the best,
> > > > > 
> > > > > Eduardo Valentin
> > > > > 
> > > > > ----------
> > > > > From 541d529f9845d249d1cb84f1b395e48f0a117e3f Mon Sep 17 00:00:00 2001
> > > > > From: Eduardo Valentin <edubezval@gmail.com>
> > > > > Date: Tue, 7 Apr 2015 13:42:12 -0700
> > > > > Subject: drivers: thermal: st: remove several sparse warnings
> > > > > 
> > > > > Simple patch to make symbols static. Symbols that are not
> > > > > shared with other parts of the kernel can be made static.
> > > > > This change also removes several sparse complains.
> > > > > 
> > > > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > > > Cc: Lee Jones <lee.jones@linaro.org>
> > > > > Cc: Pavel Machek <pavel@ucw.cz>
> > > > > Cc: Ajit Pal Singh <ajitpal.singh@st.com>
> > > > > Cc: linux-pm@vger.kernel.org
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > 
> > > > Hold on a second, you can't do that.
> > > > 
> > > > Adding these Cc: tags means "this person has been given the opportunity
> > > > to comment on the patch but has chosen not to".  However, you've
> > > > applied this patch ~1min after sending it to the lists.
> > > 
> > > Cc means -- that person was Cced.
> > 
> > You just made that up out of thin air.  That's not what it means at all.
> > 
> > Documentation/SubmittingPatches:
> > 
> > 	"If a person has had the opportunity to comment on a patch,
> > 	but has not provided such comments, you may optionally add a
> > 	"Cc:" tag to the patch. This is the only tag which might be
> > 	added without an explicit action by the person it names - but
> > 	it should indicate that this person was copied on the patch.
> > 	This tag documents that potentially interested parties have
> > 	been included in the discussion."
> > 
> > To add people on Cc, then immediately apply the patch is fundamentally
> > wrong.
> 
> That is not how I seen it working before.

But now you know what they are meant to be for and you can spread the
word appropriately.

> > > And there's no harm in having patch available in git for easy testing.
> > 
> > If we weren't so deep into the release cycle, I'd agree with you.  But
> > even if we weren't so close to the merge-window, I'd still expect a
> > note to the tune of "tentatively applying this for reason X for early
> > soak testing in -next".  Although X would have to be a pretty good
> > reason, as that's not usually how we do things.
> 
> Patches should spend a week or so in -next (that's documented
> somewhere), so people in the Cc list will have their chance to
> comment.

Yes they should, _after_ reviewing.  Chucking patches into -next
before others have been given a chance to review is _not_ the right
thing to do.  If everyone did that -next would be unbuildable and
unusable.  Patches should only be sucked into -next when a certain
level of quality has been reached.  That level is obtained in our case
by peer review.

I've said my piece on this now and shall not be discussing it further,
as I have proper work to be getting on with.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: patch "drivers: thermal: st: remove several sparse warnings" added to thermal-soc tree

[Pavel Machek]

> > > > And there's no harm in having patch available in git for easy testing.
> > > 
> > > If we weren't so deep into the release cycle, I'd agree with you.  But
> > > even if we weren't so close to the merge-window, I'd still expect a
> > > note to the tune of "tentatively applying this for reason X for early
> > > soak testing in -next".  Although X would have to be a pretty good
> > > reason, as that's not usually how we do things.
> > 
> > Patches should spend a week or so in -next (that's documented
> > somewhere), so people in the Cc list will have their chance to
> > comment.
> 
> Yes they should, _after_ reviewing.  Chucking patches into -next
> before others have been given a chance to review is _not_ the right
> thing to do.  If everyone did that -next would be unbuildable and

This is up to the maintainer, and given how trivial patch was, he
did the right thing.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html