rtc: pcf2127: add support for accessing internal static RAM

Message ID 20180520133723.30547-1-u.kleine-koenig@pengutronix.de
State Accepted
Headers show
Series
  • rtc: pcf2127: add support for accessing internal static RAM
Related show

Commit Message

Uwe Kleine-König May 20, 2018, 1:37 p.m.
The PCF2127 has 512 bytes of internal static RAM and this patch expands
the driver to access this memory.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/rtc/rtc-pcf2127.c | 68 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 62 insertions(+), 6 deletions(-)

Comments

Alexandre Belloni May 20, 2018, 6:05 p.m. | #1
Hi,

On 20/05/2018 15:37:23+0200, Uwe Kleine-König wrote:
>  static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> -			const char *name)
> +			const char *name, bool has_nvmem)
>  {
>  	struct pcf2127 *pcf2127;
> +	int ret = 0;
>  
>  	dev_dbg(dev, "%s\n", __func__);
>  
> @@ -200,8 +242,21 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  
>  	pcf2127->rtc = devm_rtc_device_register(dev, name, &pcf2127_rtc_ops,
>  						THIS_MODULE);
> +	if (IS_ERR(pcf2127->rtc))
> +		return PTR_ERR(pcf2127->rtc);
> +
> +	if (has_nvmem) {
> +		struct nvmem_config nvmem_cfg = {
> +			.priv = pcf2127,
> +			.reg_read = pcf2127_nvmem_read,
> +			.reg_write = pcf2127_nvmem_write,
> +			.size = 512,
> +		};
> +
> +		ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
> +	}
>  
> -	return PTR_ERR_OR_ZERO(pcf2127->rtc);
> +	return ret;

You must not return an error here once devm_rtc_device_register has
succeeded. You have the choice between ignoring the nvmem registration
error or switching to devm_rtc_allocate_device/rtc_register_device().
Uwe Kleine-König May 20, 2018, 6:40 p.m. | #2
Hello Alexandre,

On Sun, May 20, 2018 at 08:05:20PM +0200, Alexandre Belloni wrote:
> On 20/05/2018 15:37:23+0200, Uwe Kleine-König wrote:
> > @@ -200,8 +242,21 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> >  
> >  	pcf2127->rtc = devm_rtc_device_register(dev, name, &pcf2127_rtc_ops,
> >  						THIS_MODULE);
> > +	if (IS_ERR(pcf2127->rtc))
> > +		return PTR_ERR(pcf2127->rtc);
> > +
> > +	if (has_nvmem) {
> > +		struct nvmem_config nvmem_cfg = {
> > +			.priv = pcf2127,
> > +			.reg_read = pcf2127_nvmem_read,
> > +			.reg_write = pcf2127_nvmem_write,
> > +			.size = 512,
> > +		};
> > +
> > +		ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
> > +	}
> >  
> > -	return PTR_ERR_OR_ZERO(pcf2127->rtc);
> > +	return ret;
> 
> You must not return an error here once devm_rtc_device_register has
> succeeded.

Why? Sounds like something that should be fixed.

Best regards
Uwe
Alexandre Belloni May 20, 2018, 8:08 p.m. | #3
On 20/05/2018 20:40:01+0200, Uwe Kleine-König wrote:
> Hello Alexandre,
> 
> On Sun, May 20, 2018 at 08:05:20PM +0200, Alexandre Belloni wrote:
> > On 20/05/2018 15:37:23+0200, Uwe Kleine-König wrote:
> > > @@ -200,8 +242,21 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> > >  
> > >  	pcf2127->rtc = devm_rtc_device_register(dev, name, &pcf2127_rtc_ops,
> > >  						THIS_MODULE);
> > > +	if (IS_ERR(pcf2127->rtc))
> > > +		return PTR_ERR(pcf2127->rtc);
> > > +
> > > +	if (has_nvmem) {
> > > +		struct nvmem_config nvmem_cfg = {
> > > +			.priv = pcf2127,
> > > +			.reg_read = pcf2127_nvmem_read,
> > > +			.reg_write = pcf2127_nvmem_write,
> > > +			.size = 512,
> > > +		};
> > > +
> > > +		ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
> > > +	}
> > >  
> > > -	return PTR_ERR_OR_ZERO(pcf2127->rtc);
> > > +	return ret;
> > 
> > You must not return an error here once devm_rtc_device_register has
> > succeeded.
> 
> Why? Sounds like something that should be fixed.
> 

This is the explanation:
http://patchwork.ozlabs.org/patch/324263/
Uwe Kleine-König May 21, 2018, 8:39 a.m. | #4
Hello,

On Sun, May 20, 2018 at 10:08:09PM +0200, Alexandre Belloni wrote:
> On 20/05/2018 20:40:01+0200, Uwe Kleine-König wrote:
> > On Sun, May 20, 2018 at 08:05:20PM +0200, Alexandre Belloni wrote:
> > > You must not return an error here once devm_rtc_device_register has
> > > succeeded.
> > 
> > Why? Sounds like something that should be fixed.
> > 
> 
> This is the explanation:
> http://patchwork.ozlabs.org/patch/324263/

So open() should get a reference to the module to ensure it is not freed
while the dev is accessed. The same can happen when the driver is
manually unbound while the device is accessed (which is also fixed when
open() takes a reference).

Best regards
Uwe

Patch

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index e83be1852c2f..9f99a0966550 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -36,6 +36,11 @@ 
 #define PCF2127_REG_MO          (0x08)
 #define PCF2127_REG_YR          (0x09)
 
+/* the pcf2127 has 512 bytes nvmem, pcf2129 doesn't */
+#define PCF2127_REG_RAM_addr_MSB       0x1a
+#define PCF2127_REG_RAM_wrt_cmd        0x1c
+#define PCF2127_REG_RAM_rd_cmd         0x1d
+
 #define PCF2127_OSF             BIT(7)  /* Oscillator Fail flag */
 
 struct pcf2127 {
@@ -183,10 +188,47 @@  static const struct rtc_class_ops pcf2127_rtc_ops = {
 	.set_time	= pcf2127_rtc_set_time,
 };
 
+static int pcf2127_nvmem_read(void *priv, unsigned int offset,
+			      void *val, size_t bytes)
+{
+	struct pcf2127 *pcf2127 = priv;
+	int ret;
+	unsigned char offsetbuf[] = { offset >> 8, offset };
+
+	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_addr_MSB,
+				offsetbuf, 2);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_RAM_rd_cmd,
+			       val, bytes);
+
+	return ret ?: bytes;
+}
+
+static int pcf2127_nvmem_write(void *priv, unsigned int offset,
+			       void *val, size_t bytes)
+{
+	struct pcf2127 *pcf2127 = priv;
+	int ret;
+	unsigned char offsetbuf[] = { offset >> 8, offset };
+
+	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_addr_MSB,
+				offsetbuf, 2);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_wrt_cmd,
+				val, bytes);
+
+	return ret ?: bytes;
+}
+
 static int pcf2127_probe(struct device *dev, struct regmap *regmap,
-			const char *name)
+			const char *name, bool has_nvmem)
 {
 	struct pcf2127 *pcf2127;
+	int ret = 0;
 
 	dev_dbg(dev, "%s\n", __func__);
 
@@ -200,8 +242,21 @@  static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 
 	pcf2127->rtc = devm_rtc_device_register(dev, name, &pcf2127_rtc_ops,
 						THIS_MODULE);
+	if (IS_ERR(pcf2127->rtc))
+		return PTR_ERR(pcf2127->rtc);
+
+	if (has_nvmem) {
+		struct nvmem_config nvmem_cfg = {
+			.priv = pcf2127,
+			.reg_read = pcf2127_nvmem_read,
+			.reg_write = pcf2127_nvmem_write,
+			.size = 512,
+		};
+
+		ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
+	}
 
-	return PTR_ERR_OR_ZERO(pcf2127->rtc);
+	return ret;
 }
 
 #ifdef CONFIG_OF
@@ -309,11 +364,11 @@  static int pcf2127_i2c_probe(struct i2c_client *client,
 	}
 
 	return pcf2127_probe(&client->dev, regmap,
-				pcf2127_i2c_driver.driver.name);
+			     pcf2127_i2c_driver.driver.name, id->driver_data);
 }
 
 static const struct i2c_device_id pcf2127_i2c_id[] = {
-	{ "pcf2127", 0 },
+	{ "pcf2127", 1 },
 	{ "pcf2129", 0 },
 	{ }
 };
@@ -372,11 +427,12 @@  static int pcf2127_spi_probe(struct spi_device *spi)
 		return PTR_ERR(regmap);
 	}
 
-	return pcf2127_probe(&spi->dev, regmap, pcf2127_spi_driver.driver.name);
+	return pcf2127_probe(&spi->dev, regmap, pcf2127_spi_driver.driver.name,
+			     spi_get_device_id(spi)->driver_data);
 }
 
 static const struct spi_device_id pcf2127_spi_id[] = {
-	{ "pcf2127", 0 },
+	{ "pcf2127", 1 },
 	{ "pcf2129", 0 },
 	{ }
 };