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

From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Johan Hovold <johan@kernel.org>
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: Fri, 27 Jan 2023 17:05:03 +0100	[thread overview]
Message-ID: <Y9P2L9sNiHIZt3On@mail.local> (raw)
In-Reply-To: <Y9Py/+GpI8x8ldDG@hovoldconsulting.com>

On 27/01/2023 16:51:27+0100, Johan Hovold wrote:
> > > +static int pm8xxx_rtc_read_nvmem_offset(struct pm8xxx_rtc *rtc_dd)
> > > +{
> > > +	size_t len;
> > > +	void *buf;
> > > +	int rc;
> > > +
> > > +	buf = nvmem_cell_read(rtc_dd->nvmem_cell, &len);
> > > +	if (IS_ERR(buf)) {
> > > +		rc = PTR_ERR(buf);
> > > +		dev_err(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);
> > 
> > You removed many dev_err strings in your previous patch and now this is
> > verbose. Honestly, there is not much to do apart from reying the
> > operation so I don't think the strings are worth it.
> 
> There's a difference. The SPMI ones are basically equivalent to mmio
> reads, which we also don't expect to fail (and other spmi drivers also
> ignore them).
> 
> These nvmem error paths I actually hit during development and it could
> help someone trying to enable this feature on a new platform.
>  

then consider using dev_dbg, the end user will never see or act on those
error messages anyway. I'm on a quest to cut down the number of strings
in the kernel, at least in the RTC subsystem ;)

> > > +		return rc;
> > > +	}
> > > +
> > > +	if (len != sizeof(u32)) {
> > > +		dev_err(rtc_dd->dev, "unexpected nvmem cell size %zu\n", len);
> > > +		kfree(buf);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	rtc_dd->offset = get_unaligned_le32(buf);
> > > +
> > > +	kfree(buf);
> > > +
> > > +	return 0;
> > > +}
> 
> > > @@ -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).

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com