All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: [PATCH 000/100] rtc: remove cargo culted code
@ 2018-03-02  8:46 Steve Twiss
  2018-03-02  8:56 ` Alexandre Belloni
  0 siblings, 1 reply; 6+ messages in thread
From: Steve Twiss @ 2018-03-02  8:46 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: linux-kernel, linux-rtc

On Wed, Feb 21, 2018 at 8:54 PM, Alexandre Belloni wrote:
 
> subject:	[PATCH 000/100] rtc: remove cargo culted code
> mailing list:	linux-kernel@vger.kernel.org Filter messages from this mailing list
>
> Hello,
>
> This series:
>  - removes useless calls to rtc_valid_tm in .read_time, .set_time and
>    .set_alarm
>  - removes code setting default values for RTCs (and lets the core
>    handle it)
>  - removes useless "time is invalid" messages at probe time
>  - removes useless indirect calls
>
> Those were mostly copy pasted from other drivers

Hi Alexandre,

Acked for:
  rtc: da9063: stop validating rtc_time in .read_time
  rtc: da9052: stop validating rtc_time in .read_time
  rtc: da9055: stop validating rtc_time in .read_time

Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>

Agreed -- rtc_valid_tm() call is cargo cult for the above.

(By definition) for DA9063 I was trying to be rigorous.
The .read_time function is slightly different here because I can make a copy the alarm time into the RTC time
structure to solve an RTC synchronisation problem internally to the DA9063.
https://elixir.bootlin.com/linux/v4.5.6/source/drivers/rtc/rtc-da9063.c#L253

But after some further looking, I have not got any explicit case of how the time read directly from the DA9063
registers can be incorrectly represented. So there should be no need to check this.

Regards,
Steve

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 000/100] rtc: remove cargo culted code
  2018-03-02  8:46 [PATCH 000/100] rtc: remove cargo culted code Steve Twiss
@ 2018-03-02  8:56 ` Alexandre Belloni
  2018-03-02  9:06   ` Steve Twiss
  0 siblings, 1 reply; 6+ messages in thread
From: Alexandre Belloni @ 2018-03-02  8:56 UTC (permalink / raw)
  To: Steve Twiss; +Cc: linux-kernel, linux-rtc

On 02/03/2018 at 08:46:48 +0000, Steve Twiss wrote:
> On Wed, Feb 21, 2018 at 8:54 PM, Alexandre Belloni wrote:
>  
> > subject:	[PATCH 000/100] rtc: remove cargo culted code
> > mailing list:	linux-kernel@vger.kernel.org Filter messages from this mailing list
> >
> > Hello,
> >
> > This series:
> >  - removes useless calls to rtc_valid_tm in .read_time, .set_time and
> >    .set_alarm
> >  - removes code setting default values for RTCs (and lets the core
> >    handle it)
> >  - removes useless "time is invalid" messages at probe time
> >  - removes useless indirect calls
> >
> > Those were mostly copy pasted from other drivers
> 
> Hi Alexandre,
> 
> Acked for:
>   rtc: da9063: stop validating rtc_time in .read_time
>   rtc: da9052: stop validating rtc_time in .read_time
>   rtc: da9055: stop validating rtc_time in .read_time
> 
> Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>
> 
> Agreed -- rtc_valid_tm() call is cargo cult for the above.
> 
> (By definition) for DA9063 I was trying to be rigorous.
> The .read_time function is slightly different here because I can make a copy the alarm time into the RTC time
> structure to solve an RTC synchronisation problem internally to the DA9063.
> https://elixir.bootlin.com/linux/v4.5.6/source/drivers/rtc/rtc-da9063.c#L253
> 
> But after some further looking, I have not got any explicit case of how the time read directly from the DA9063
> registers can be incorrectly represented. So there should be no need to check this.
> 

My point is that it is checked later in the core anyway so you end up
doing:

da9063_rtc_read_time()
	return rtc_valid_tm(tm);

__rtc_read_time()
	if (err < 0)
		return err;
	err = rtc_valid_tm(tm);

	return err;

So the check in da9063_rtc_read_time is always pointless.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH 000/100] rtc: remove cargo culted code
  2018-03-02  8:56 ` Alexandre Belloni
@ 2018-03-02  9:06   ` Steve Twiss
  0 siblings, 0 replies; 6+ messages in thread
From: Steve Twiss @ 2018-03-02  9:06 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: linux-kernel, linux-rtc

On 02 March 2018 08:57, Alexandre Belloni wrote:

> To: Steve Twiss
> Cc: linux-kernel@vger.kernel.org; linux-rtc@vger.kernel.org
> Subject: Re: [PATCH 000/100] rtc: remove cargo culted code
> 
> On 02/03/2018 at 08:46:48 +0000, Steve Twiss wrote:
> > Hi Alexandre,
> >
> > Acked for:
> >   rtc: da9063: stop validating rtc_time in .read_time
> >   rtc: da9052: stop validating rtc_time in .read_time
> >   rtc: da9055: stop validating rtc_time in .read_time
> >
> > Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>
> >

[...]

> > But after some further looking, I have not got any explicit case of how the
> > time read directly from the DA9063
> > registers can be incorrectly represented. So there should be no need to
> > check this.
> 
> My point is that it is checked later in the core anyway so you end up
> doing:
> 
> da9063_rtc_read_time()
> 	return rtc_valid_tm(tm);
> 
> __rtc_read_time()
> 	if (err < 0)
> 		return err;
> 	err = rtc_valid_tm(tm);
> 
> 	return err;
> 
> So the check in da9063_rtc_read_time is always pointless.

Ahh. I see. Thanks!
Regards,
Steve

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 000/100] rtc: remove cargo culted code
  2018-02-22 15:48 ` Christoph Hellwig
@ 2018-02-22 16:02   ` Alexandre Belloni
  0 siblings, 0 replies; 6+ messages in thread
From: Alexandre Belloni @ 2018-02-22 16:02 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-rtc, linux-kernel

On 22/02/2018 at 07:48:03 -0800, Christoph Hellwig wrote:
> On Wed, Feb 21, 2018 at 09:54:55PM +0100, Alexandre Belloni wrote:
> > Hello,
> > 
> > This series:
> >  - removes useless calls to rtc_valid_tm in .read_time, .set_time and
> >    .set_alarm
> >  - removes code setting default values for RTCs (and lets the core
> >    handle it)
> >  - removes useless "time is invalid" messages at probe time
> >  - removes useless indirect calls
> 
> Seems like this could be just one patch for each of the issues..

I'll probably squash the simpler ones before sending to Linus.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 000/100] rtc: remove cargo culted code
  2018-02-21 20:54 Alexandre Belloni
@ 2018-02-22 15:48 ` Christoph Hellwig
  2018-02-22 16:02   ` Alexandre Belloni
  0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2018-02-22 15:48 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: linux-rtc, linux-kernel

On Wed, Feb 21, 2018 at 09:54:55PM +0100, Alexandre Belloni wrote:
> Hello,
> 
> This series:
>  - removes useless calls to rtc_valid_tm in .read_time, .set_time and
>    .set_alarm
>  - removes code setting default values for RTCs (and lets the core
>    handle it)
>  - removes useless "time is invalid" messages at probe time
>  - removes useless indirect calls

Seems like this could be just one patch for each of the issues..

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 000/100] rtc: remove cargo culted code
@ 2018-02-21 20:54 Alexandre Belloni
  2018-02-22 15:48 ` Christoph Hellwig
  0 siblings, 1 reply; 6+ messages in thread
From: Alexandre Belloni @ 2018-02-21 20:54 UTC (permalink / raw)
  To: linux-rtc; +Cc: linux-kernel, Alexandre Belloni

Hello,

This series:
 - removes useless calls to rtc_valid_tm in .read_time, .set_time and
   .set_alarm
 - removes code setting default values for RTCs (and lets the core
   handle it)
 - removes useless "time is invalid" messages at probe time
 - removes useless indirect calls

Those were mostly copy pasted from other drivers

Alexandre Belloni (100):
  rtc: ds1511: let the core handle invalid time
  rtc: ds1553: let the core handle invalid time
  rtc: cmos: let the core handle invalid time
  rtc: rs5c348: let the core handle invalid time
  rtc: stk17ta8: let the core handle invalid time
  rtc: ab-b5ze-s3: stop validating rtc_time in .read_time
  rtc: ab3100: stop validating rtc_time in .read_time
  rtc: ab8500: stop validating rtc_time in .read_time
  rtc: ac100: stop validating rtc_time in .read_time
  rtc: au1xxx: stop validating rtc_time in .read_time
  rtc: bq32k: stop validating rtc_time in .read_time
  rtc: coh901331: stop validating rtc_time in .read_time
  rtc: cpcap: stop validating rtc_time in .read_time
  rtc: da9052: stop validating rtc_time in .read_time
  rtc: da9055: stop validating rtc_time in .read_time
  rtc: da9063: stop validating rtc_time in .read_time
  rtc: ds1216: stop validating rtc_time in .read_time
  rtc: ds1286: stop validating rtc_time in .read_time
  rtc: ds1302: stop validating rtc_time in .read_time
  rtc: ds1305: stop validating rtc_time in .read_time
  rtc: ds1307: stop validating rtc_time in .read_time
  rtc: ds1343: stop validating rtc_time in .read_time
  rtc: ds1347: stop validating rtc_time in .read_time
  rtc: ds1390: stop validating rtc_time in .read_time
  rtc: ds1685: stop validating rtc_time in .read_time
  rtc: ds1742: stop validating rtc_time in .read_time
  rtc: ds2404: stop validating rtc_time in .read_time
  rtc: ds3232: stop validating rtc_time in .read_time
  rtc: efi: stop validating rtc_time in .read_time
  rtc: fm3130: stop validating rtc_time in .read_time
  rtc: jz4740: stop validating rtc_time in .read_time
  rtc: lpc24xx: stop validating rtc_time in .read_time
  rtc: lpc32xx: stop validating rtc_time in .read_time
  rtc: ls1x: stop validating rtc_time in .read_time
  rtc: m41t94: stop validating rtc_time in .read_time
  rtc: m48t35: stop validating rtc_time in .read_time
  rtc: m48t59: stop validating rtc_time in .read_time
  rtc: m48t86: stop validating rtc_time in .read_time
  rtc: max6902: stop validating rtc_time in .read_time
  rtc: max6916: stop validating rtc_time in .read_time
  rtc: max8997: stop validating rtc_time in .read_time
  rtc: max8998: stop validating rtc_time in .read_time
  rtc: mc13xxx: stop validating rtc_time in .read_time
  rtc: mcp795: stop validating rtc_time in .read_time
  rtc: mpc5121: stop validating rtc_time in .read_time
  rtc: mrst: stop validating rtc_time in .read_time
  rtc: msm6242: stop validating rtc_time in .read_time
  rtc: mt7622: stop validating rtc_time in .read_time
  rtc: mv: stop validating rtc_time in .read_time
  rtc: nuc900: stop validating rtc_time in .read_time
  rtc: pcap: stop validating rtc_time in .read_time
  rtc: pcf2123: stop validating rtc_time in .read_time
  rtc: pcf2127: stop validating rtc_time in .read_time
  rtc: pcf50633: stop validating rtc_time in .read_time
  rtc: pcf8523: stop validating rtc_time in .read_time
  rtc: pic32: stop validating rtc_time in .read_time
  rtc: ps3: stop validating rtc_time in .read_time
  rtc: r7301: stop validating rtc_time in .read_time
  rtc: r9701: stop validating rtc_time in .read_time
  rtc: rp5c01: stop validating rtc_time in .read_time
  rtc: rx6110: stop validating rtc_time in .read_time
  rtc: rx8010: stop validating rtc_time in .read_time
  rtc: rx8025: stop validating rtc_time in .read_time
  rtc: s3c: stop validating rtc_time in .read_time
  rtc: s5m: stop validating rtc_time in .read_time
  rtc: sc27xx: stop validating rtc_time in .read_time
  rtc: sh: stop validating rtc_time in .read_time
  rtc: starfire: stop validating rtc_time in .read_time
  rtc: sun6i: stop validating rtc_time in .read_time
  rtc: sunxi: stop validating rtc_time in .read_time
  rtc: tps6586x: stop validating rtc_time in .read_time
  rtc: tx4939: stop validating rtc_time in .read_time
  rtc: wm831x: stop validating rtc_time in .read_time
  rtc: xgene: stop validating rtc_time in .read_time
  rtc: zynqmp: stop validating rtc_time in .read_time
  rtc: isl12022: stop validating rtc_time in .read_time
  rtc: isl12022: remove useless indirection
  rtc: m41t80: stop validating rtc_time in .read_time
  rtc: m41t93: stop validating rtc_time in .read_time
  rtc: max77686: stop validating rtc_time in .read_time
  rtc: omap: stop validating rtc_time in .set_time and .set_alarm
  rtc: spear: stop validating rtc_time in .set_time and .set_alarm
  rtc: tegra: stop validating rtc_time in .set_time
  rtc: abx80x: remove useless message
  rtc: pm8xxx: remove useless message
  rtc: rx4581: remove useless message
  rtc: rx8581: remove useless message
  rtc: tile: remove useless message
  rtc: rk808: remove useless debug message
  rtc: rk808: fix possible race condition
  rtc: s35390a: remove useless message
  rtc: s35390a: stop validating rtc_time in .read_time
  rtc: s35390a: remove useless indirection
  rtc: rs5c372:  remove useless message
  rtc: rs5c372: stop validating rtc_time in .read_time
  rtc: rs5c372: remove useless indirection
  rtc: max6900: stop validating rtc_time in .read_time
  rtc: max6900: remove useless indirection
  rtc: pcf85063: stop validating rtc_time in .read_time
  rtc: pcf85063: remove useless indirection

 drivers/rtc/rtc-ab-b5ze-s3.c |  4 +---
 drivers/rtc/rtc-ab3100.c     |  2 +-
 drivers/rtc/rtc-ab8500.c     |  2 +-
 drivers/rtc/rtc-abx80x.c     |  6 +-----
 drivers/rtc/rtc-ac100.c      |  2 +-
 drivers/rtc/rtc-au1xxx.c     |  2 +-
 drivers/rtc/rtc-bq32k.c      |  2 +-
 drivers/rtc/rtc-cmos.c       | 12 ------------
 drivers/rtc/rtc-coh901331.c  |  2 +-
 drivers/rtc/rtc-cpcap.c      |  2 +-
 drivers/rtc/rtc-da9052.c     |  3 +--
 drivers/rtc/rtc-da9055.c     |  2 +-
 drivers/rtc/rtc-da9063.c     |  2 +-
 drivers/rtc/rtc-ds1216.c     |  2 +-
 drivers/rtc/rtc-ds1286.c     |  2 +-
 drivers/rtc/rtc-ds1302.c     |  3 +--
 drivers/rtc/rtc-ds1305.c     |  3 +--
 drivers/rtc/rtc-ds1307.c     |  3 +--
 drivers/rtc/rtc-ds1343.c     |  2 +-
 drivers/rtc/rtc-ds1347.c     |  2 +-
 drivers/rtc/rtc-ds1390.c     |  2 +-
 drivers/rtc/rtc-ds1511.c     |  4 ----
 drivers/rtc/rtc-ds1553.c     |  4 ----
 drivers/rtc/rtc-ds1685.c     |  2 +-
 drivers/rtc/rtc-ds1742.c     |  2 +-
 drivers/rtc/rtc-ds2404.c     |  2 +-
 drivers/rtc/rtc-ds3232.c     |  2 +-
 drivers/rtc/rtc-efi.c        |  2 +-
 drivers/rtc/rtc-fm3130.c     |  3 +--
 drivers/rtc/rtc-isl12022.c   | 18 +++++-------------
 drivers/rtc/rtc-jz4740.c     |  2 +-
 drivers/rtc/rtc-lpc24xx.c    |  2 +-
 drivers/rtc/rtc-lpc32xx.c    |  2 +-
 drivers/rtc/rtc-ls1x.c       |  2 +-
 drivers/rtc/rtc-m41t80.c     |  2 +-
 drivers/rtc/rtc-m41t93.c     |  2 +-
 drivers/rtc/rtc-m41t94.c     |  3 +--
 drivers/rtc/rtc-m48t35.c     |  2 +-
 drivers/rtc/rtc-m48t59.c     |  2 +-
 drivers/rtc/rtc-m48t86.c     |  2 +-
 drivers/rtc/rtc-max6900.c    | 19 +++++--------------
 drivers/rtc/rtc-max6902.c    |  2 +-
 drivers/rtc/rtc-max6916.c    |  2 +-
 drivers/rtc/rtc-max77686.c   |  4 +---
 drivers/rtc/rtc-max8997.c    |  2 +-
 drivers/rtc/rtc-max8998.c    |  2 +-
 drivers/rtc/rtc-mc13xxx.c    |  2 +-
 drivers/rtc/rtc-mcp795.c     |  2 +-
 drivers/rtc/rtc-mpc5121.c    |  2 +-
 drivers/rtc/rtc-mrst.c       |  2 +-
 drivers/rtc/rtc-msm6242.c    |  2 +-
 drivers/rtc/rtc-mt7622.c     |  2 +-
 drivers/rtc/rtc-mv.c         |  2 +-
 drivers/rtc/rtc-nuc900.c     | 14 ++++++++------
 drivers/rtc/rtc-omap.c       |  3 ---
 drivers/rtc/rtc-pcap.c       |  2 +-
 drivers/rtc/rtc-pcf2123.c    |  2 +-
 drivers/rtc/rtc-pcf2127.c    |  2 +-
 drivers/rtc/rtc-pcf50633.c   |  2 +-
 drivers/rtc/rtc-pcf85063.c   | 18 +++++-------------
 drivers/rtc/rtc-pcf8523.c    |  2 +-
 drivers/rtc/rtc-pic32.c      |  2 +-
 drivers/rtc/rtc-pm8xxx.c     |  6 ------
 drivers/rtc/rtc-ps3.c        |  2 +-
 drivers/rtc/rtc-r7301.c      |  2 +-
 drivers/rtc/rtc-r9701.c      |  2 +-
 drivers/rtc/rtc-rk808.c      | 25 +++++++------------------
 drivers/rtc/rtc-rp5c01.c     |  2 +-
 drivers/rtc/rtc-rs5c348.c    |  5 -----
 drivers/rtc/rtc-rs5c372.c    | 24 +++++-------------------
 drivers/rtc/rtc-rx4581.c     |  6 +-----
 drivers/rtc/rtc-rx6110.c     |  2 +-
 drivers/rtc/rtc-rx8010.c     |  2 +-
 drivers/rtc/rtc-rx8025.c     |  2 +-
 drivers/rtc/rtc-rx8581.c     |  6 +-----
 drivers/rtc/rtc-s35390a.c    | 38 +++++++++-----------------------------
 drivers/rtc/rtc-s3c.c        |  2 +-
 drivers/rtc/rtc-s5m.c        |  2 +-
 drivers/rtc/rtc-sc27xx.c     |  2 +-
 drivers/rtc/rtc-sh.c         |  2 +-
 drivers/rtc/rtc-spear.c      | 12 +++---------
 drivers/rtc/rtc-starfire.c   |  2 +-
 drivers/rtc/rtc-stk17ta8.c   |  4 ----
 drivers/rtc/rtc-sun6i.c      |  2 +-
 drivers/rtc/rtc-sunxi.c      |  2 +-
 drivers/rtc/rtc-tegra.c      |  4 ----
 drivers/rtc/rtc-tile.c       |  3 ---
 drivers/rtc/rtc-tps6586x.c   |  2 +-
 drivers/rtc/rtc-tx4939.c     |  2 +-
 drivers/rtc/rtc-wm831x.c     |  2 +-
 drivers/rtc/rtc-xgene.c      |  2 +-
 drivers/rtc/rtc-zynqmp.c     |  2 +-
 92 files changed, 122 insertions(+), 263 deletions(-)

-- 
2.16.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-03-02  9:06 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-02  8:46 [PATCH 000/100] rtc: remove cargo culted code Steve Twiss
2018-03-02  8:56 ` Alexandre Belloni
2018-03-02  9:06   ` Steve Twiss
  -- strict thread matches above, loose matches on Subject: below --
2018-02-21 20:54 Alexandre Belloni
2018-02-22 15:48 ` Christoph Hellwig
2018-02-22 16:02   ` Alexandre Belloni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.