Re: [PATCH 16/24] rtc: pm8xxx: add support for nvmem offset

From: Johan Hovold <johan@kernel.org>
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Johan Hovold <johan+linaro@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Andy Gross <agross@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Alessandro Zummo <a.zummo@towertech.it>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Maximilian Luz <luzmaximilian@gmail.com>,
	linux-arm-msm@vger.kernel.org, linux-rtc@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 16/24] rtc: pm8xxx: add support for nvmem offset
Date: Thu, 2 Feb 2023 16:12:33 +0100	[thread overview]
Message-ID: <Y9vS4TpVHDnGN0G/@hovoldconsulting.com> (raw)
In-Reply-To: <Y9P2L9sNiHIZt3On@mail.local>

On Fri, Jan 27, 2023 at 05:05:03PM +0100, Alexandre Belloni wrote:
> On 27/01/2023 16:51:27+0100, Johan Hovold wrote:

> > > > @@ -380,9 +478,23 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
> > > >  	rtc_dd->allow_set_time = of_property_read_bool(pdev->dev.of_node,
> > > >  						      "allow-set-time");
> > > >  
> > > > +	rtc_dd->nvmem_cell = devm_nvmem_cell_get(&pdev->dev, "offset");
> > > 
> > > Maybe we should get something more specific than just "offset" so this
> > > could be parsed in the RTC core at some point (this is the second RTC to
> > > behave like this)
> > 
> > Yes, that thought crossed my mind, but it's an nvmem cell name (label)
> > and not a generic devicetree property. If you look at the binding
> > document I think the name makes sense given the current description, and
> > I'm not sure changing to something like 'base' would be much of an
> > improvement.
> > 
> > I also don't expect there to be more broken RTCs out there like these
> > ones. Hopefully Qualcomm will even get this fixed at some point
> > themselves.
> > 
> > And I assume you were think of the old Atmel driver which uses a timer
> > counter and a scratch register as a base? That one is also a bit
> > different in that the timer can be reset, just not set.
> 
> Nope, I'm thinking about the gamecube one and probably the nintendo
> switch one which seems to behave similarly (no driver in the kernel
> though).

Found the gamecube one now (misread you comment above to imply that it
was also out of tree).

That one is also different in that the timer in that RTC can also be
set (e.g. like the atmel one), but for consistency with some firmware an
offset also needs to be read from SRAM (not NVRAM) and applied. That
offset is also never updated by Linux.

Johan