[PATCH 1/3] rtc: isl1208: Introduce driver state struct

[PATCH 1/3] rtc: isl1208: Introduce driver state struct

[Trent Piepho]

This driver has no state of its own, depending entirely on what is in
the generic rtc device.

Intoduce a state struct.  For now it only contains a pointer to the rtc
device struct, but future patches will add more data.

Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 drivers/rtc/rtc-isl1208.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index 263af3d8cd9f..d8e0670e12fc 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -79,6 +79,11 @@ enum {
 	TYPE_ISL1219,
 };
 
+/* Device state */
+struct isl1208_state {
+	struct rtc_device *rtc;
+};
+
 /* block read */
 static int
 isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
@@ -557,7 +562,7 @@ isl1208_rtc_interrupt(int irq, void *data)
 {
 	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
 	struct i2c_client *client = data;
-	struct rtc_device *rtc = i2c_get_clientdata(client);
+	struct isl1208_state *isl1208 = i2c_get_clientdata(client);
 	int handled = 0, sr, err;
 
 	/*
@@ -580,7 +585,7 @@ isl1208_rtc_interrupt(int irq, void *data)
 	if (sr & ISL1208_REG_SR_ALM) {
 		dev_dbg(&client->dev, "alarm!\n");
 
-		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
+		rtc_update_irq(isl1208->rtc, 1, RTC_IRQF | RTC_AF);
 
 		/* Clear the alarm */
 		sr &= ~ISL1208_REG_SR_ALM;
@@ -598,7 +603,7 @@ isl1208_rtc_interrupt(int irq, void *data)
 	}
 
 	if (sr & ISL1208_REG_SR_EVT) {
-		sysfs_notify(&rtc->dev.kobj, NULL,
+		sysfs_notify(&isl1208->rtc->dev.kobj, NULL,
 			     dev_attr_timestamp0.attr.name);
 		dev_warn(&client->dev, "event detected");
 		handled = 1;
@@ -723,7 +728,7 @@ static int
 isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	int rc = 0;
-	struct rtc_device *rtc;
+	struct isl1208_state *isl1208;
 	int evdet_irq = -1;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
@@ -732,13 +737,17 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (isl1208_i2c_validate_client(client) < 0)
 		return -ENODEV;
 
-	rtc = devm_rtc_allocate_device(&client->dev);
-	if (IS_ERR(rtc))
-		return PTR_ERR(rtc);
+	/* Allocate driver state, point i2c client data to it */
+	isl1208 = devm_kzalloc(&client->dev, sizeof(*isl1208), GFP_KERNEL);
+	if (!isl1208)
+		return -ENOMEM;
+	i2c_set_clientdata(client, isl1208);
 
-	rtc->ops = &isl1208_rtc_ops;
+	isl1208->rtc = devm_rtc_allocate_device(&client->dev);
+	if (IS_ERR(isl1208->rtc))
+		return PTR_ERR(isl1208->rtc);
 
-	i2c_set_clientdata(client, rtc);
+	isl1208->rtc->ops = &isl1208_rtc_ops;
 
 	rc = isl1208_i2c_get_sr(client);
 	if (rc < 0) {
@@ -771,13 +780,13 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			dev_err(&client->dev, "could not enable tamper detection\n");
 			return rc;
 		}
-		rc = rtc_add_group(rtc, &isl1219_rtc_sysfs_files);
+		rc = rtc_add_group(isl1208->rtc, &isl1219_rtc_sysfs_files);
 		if (rc)
 			return rc;
 		evdet_irq = of_irq_get_byname(np, "evdet");
 	}
 
-	rc = rtc_add_group(rtc, &isl1208_rtc_sysfs_files);
+	rc = rtc_add_group(isl1208->rtc, &isl1208_rtc_sysfs_files);
 	if (rc)
 		return rc;
 
@@ -791,7 +800,7 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (rc)
 		return rc;
 
-	return rtc_register_device(rtc);
+	return rtc_register_device(isl1208->rtc);
 }
 
 static const struct i2c_device_id isl1208_id[] = {
-- 
2.14.4

[PATCH 2/3] rtc: isl1208: Support more chip variations

[Trent Piepho]

Add more support in the driver for dealing with differences in is1208
compatible chips.  Put the 1208, 1209, 1218, and 1219 in the list and
encode information about nvram size, tamper, and timestamp features.

This adds support for the isl1209, which has a tamper detect but no
timestamp feature.

Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 drivers/rtc/rtc-isl1208.c | 78 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 56 insertions(+), 22 deletions(-)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index d8e0670e12fc..77912ee58011 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -73,15 +73,49 @@
 static struct i2c_driver isl1208_driver;
 
 /* ISL1208 various variants */
-enum {
+enum isl1208_id {
 	TYPE_ISL1208 = 0,
 	TYPE_ISL1218,
 	TYPE_ISL1219,
+	TYPE_ISL1209,
+	ISL_LAST_ID
 };
 
+/* Chip capabilities table */
+static const struct isl1208_config {
+	const char	name[8];
+	unsigned int	nvmem_length;
+	unsigned	has_tamper:1;
+	unsigned	has_timestamp:1;
+} isl1208_configs[] = {
+	[TYPE_ISL1208] = { "isl1208", 2, false, false },
+	[TYPE_ISL1209] = { "isl1209", 2, true,  false },
+	[TYPE_ISL1218] = { "isl1218", 8, false, false },
+	[TYPE_ISL1219] = { "isl1219", 2, true,  true },
+};
+
+static const struct i2c_device_id isl1208_id[] = {
+	{ "isl1208", TYPE_ISL1208 },
+	{ "isl1209", TYPE_ISL1209 },
+	{ "isl1218", TYPE_ISL1218 },
+	{ "isl1219", TYPE_ISL1219 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, isl1208_id);
+
+static const struct of_device_id isl1208_of_match[] = {
+	{ .compatible = "isil,isl1208", .data = &isl1208_configs[TYPE_ISL1208] },
+	{ .compatible = "isil,isl1209", .data = &isl1208_configs[TYPE_ISL1209] },
+	{ .compatible = "isil,isl1218", .data = &isl1208_configs[TYPE_ISL1218] },
+	{ .compatible = "isil,isl1219", .data = &isl1208_configs[TYPE_ISL1219] },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, isl1208_of_match);
+
 /* Device state */
 struct isl1208_state {
 	struct rtc_device *rtc;
+	const struct isl1208_config *config;
 };
 
 /* block read */
@@ -602,11 +636,12 @@ isl1208_rtc_interrupt(int irq, void *data)
 			return err;
 	}
 
-	if (sr & ISL1208_REG_SR_EVT) {
-		sysfs_notify(&isl1208->rtc->dev.kobj, NULL,
-			     dev_attr_timestamp0.attr.name);
+	if (isl1208->config->has_tamper && (sr & ISL1208_REG_SR_EVT)) {
 		dev_warn(&client->dev, "event detected");
 		handled = 1;
+		if (isl1208->config->has_timestamp)
+			sysfs_notify(&isl1208->rtc->dev.kobj, NULL,
+				     dev_attr_timestamp0.attr.name);
 	}
 
 	return handled ? IRQ_HANDLED : IRQ_NONE;
@@ -743,6 +778,19 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return -ENOMEM;
 	i2c_set_clientdata(client, isl1208);
 
+	/* Determine which chip we have */
+	if (client->dev.of_node) {
+		const struct of_device_id *match =
+			of_match_node(isl1208_of_match, client->dev.of_node);
+		if (!match)
+			return -EINVAL;
+		isl1208->config = match->data;
+	} else {
+		if (id->driver_data >= ISL_LAST_ID)
+			return -EINVAL;
+		isl1208->config = &isl1208_configs[id->driver_data];
+	}
+
 	isl1208->rtc = devm_rtc_allocate_device(&client->dev);
 	if (IS_ERR(isl1208->rtc))
 		return PTR_ERR(isl1208->rtc);
@@ -759,7 +807,7 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		dev_warn(&client->dev, "rtc power failure detected, "
 			 "please set clock.\n");
 
-	if (id->driver_data == TYPE_ISL1219) {
+	if (isl1208->config->has_tamper) {
 		struct device_node *np = client->dev.of_node;
 		u32 evienb;
 
@@ -780,10 +828,12 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			dev_err(&client->dev, "could not enable tamper detection\n");
 			return rc;
 		}
+		evdet_irq = of_irq_get_byname(np, "evdet");
+	}
+	if (isl1208->config->has_timestamp) {
 		rc = rtc_add_group(isl1208->rtc, &isl1219_rtc_sysfs_files);
 		if (rc)
 			return rc;
-		evdet_irq = of_irq_get_byname(np, "evdet");
 	}
 
 	rc = rtc_add_group(isl1208->rtc, &isl1208_rtc_sysfs_files);
@@ -803,22 +853,6 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	return rtc_register_device(isl1208->rtc);
 }
 
-static const struct i2c_device_id isl1208_id[] = {
-	{ "isl1208", TYPE_ISL1208 },
-	{ "isl1218", TYPE_ISL1218 },
-	{ "isl1219", TYPE_ISL1219 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, isl1208_id);
-
-static const struct of_device_id isl1208_of_match[] = {
-	{ .compatible = "isil,isl1208" },
-	{ .compatible = "isil,isl1218" },
-	{ .compatible = "isil,isl1219" },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, isl1208_of_match);
-
 static struct i2c_driver isl1208_driver = {
 	.driver = {
 		.name = "rtc-isl1208",
-- 
2.14.4

[PATCH 3/3] rtc: isl1208: Add new style nvmem support to driver

[Trent Piepho]

Add support for the RTC's NVRAM using the standard nvmem support that is
part of the Linux RTC framework.

This driver already has a sysfs attribute that provides access to the
RTC's NVRAM as a single 16-bit value.  Some chips have more than two
bytes of NVRAM, so this will not work for them.  It's also non-standard.

This sysfs attribute is left in for backward compatibility, but will only
be able to read the first two bytes of NVRAM.  The nvmem interface will
allow access to all NVRAM, e.g. eight bytes on the isl1218.

Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 drivers/rtc/rtc-isl1208.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index 77912ee58011..1be1156aef23 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -114,6 +114,7 @@ MODULE_DEVICE_TABLE(of, isl1208_of_match);
 
 /* Device state */
 struct isl1208_state {
+	struct nvmem_config nvmem_config;
 	struct rtc_device *rtc;
 	const struct isl1208_config *config;
 };
@@ -741,6 +742,46 @@ static const struct attribute_group isl1219_rtc_sysfs_files = {
 	.attrs	= isl1219_rtc_attrs,
 };
 
+static int isl1208_nvmem_read(void *priv, unsigned int off, void *buf,
+			      size_t count)
+{
+	struct isl1208_state *isl1208 = priv;
+	struct i2c_client *client = to_i2c_client(isl1208->rtc->dev.parent);
+	int ret;
+
+	/* nvmem sanitizes offset/count for us, but count==0 is possible */
+	if (!count)
+		return count;
+	ret = isl1208_i2c_read_regs(client, ISL1208_REG_USR1 + off, buf,
+				    count);
+	return ret == 0 ? count : ret;
+}
+
+static int isl1208_nvmem_write(void *priv, unsigned int off, void *buf,
+			       size_t count)
+{
+	struct isl1208_state *isl1208 = priv;
+	struct i2c_client *client = to_i2c_client(isl1208->rtc->dev.parent);
+	int ret;
+
+	/* nvmem sanitizes off/count for us, but count==0 is possible */
+	if (!count)
+		return count;
+	ret = isl1208_i2c_set_regs(client, ISL1208_REG_USR1 + off, buf,
+				   count);
+
+	return ret == 0 ? count : ret;
+}
+
+static const struct nvmem_config isl1208_nvmem_config = {
+	.name = "isl1208_nvram",
+	.word_size = 1,
+	.stride = 1,
+	/* .size from chip specific config */
+	.reg_read = isl1208_nvmem_read,
+	.reg_write = isl1208_nvmem_write,
+};
+
 static int isl1208_setup_irq(struct i2c_client *client, int irq)
 {
 	int rc = devm_request_threaded_irq(&client->dev, irq, NULL,
@@ -797,6 +838,11 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	isl1208->rtc->ops = &isl1208_rtc_ops;
 
+	/* Setup nvmem configuration in driver state struct */
+	isl1208->nvmem_config = isl1208_nvmem_config;
+	isl1208->nvmem_config.size = isl1208->config->nvmem_length;
+	isl1208->nvmem_config.priv = isl1208;
+
 	rc = isl1208_i2c_get_sr(client);
 	if (rc < 0) {
 		dev_err(&client->dev, "reading status failed\n");
-- 
2.14.4

Re: [PATCH 2/3] rtc: isl1208: Support more chip variations

[Alexandre Belloni]

Hi,

Very few comments below:

On 01/02/2019 19:42:12+0000, Trent Piepho wrote:
> Add more support in the driver for dealing with differences in is1208
> compatible chips.  Put the 1208, 1209, 1218, and 1219 in the list and
> encode information about nvram size, tamper, and timestamp features.
> 
> This adds support for the isl1209, which has a tamper detect but no
> timestamp feature.
> 
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>
> ---
>  drivers/rtc/rtc-isl1208.c | 78 ++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 56 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> index d8e0670e12fc..77912ee58011 100644
> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c
> @@ -73,15 +73,49 @@
>  static struct i2c_driver isl1208_driver;
>  
>  /* ISL1208 various variants */
> -enum {
> +enum isl1208_id {
>  	TYPE_ISL1208 = 0,
>  	TYPE_ISL1218,
>  	TYPE_ISL1219,
> +	TYPE_ISL1209,

I would keep that list ordered.

> +	ISL_LAST_ID
>  };
>  
> +/* Chip capabilities table */
> +static const struct isl1208_config {
> +	const char	name[8];
> +	unsigned int	nvmem_length;
> +	unsigned	has_tamper:1;
> +	unsigned	has_timestamp:1;
> +} isl1208_configs[] = {
> +	[TYPE_ISL1208] = { "isl1208", 2, false, false },
> +	[TYPE_ISL1209] = { "isl1209", 2, true,  false },
> +	[TYPE_ISL1218] = { "isl1218", 8, false, false },
> +	[TYPE_ISL1219] = { "isl1219", 2, true,  true },
> +};
> +
> +static const struct i2c_device_id isl1208_id[] = {
> +	{ "isl1208", TYPE_ISL1208 },
> +	{ "isl1209", TYPE_ISL1209 },
> +	{ "isl1218", TYPE_ISL1218 },
> +	{ "isl1219", TYPE_ISL1219 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, isl1208_id);
> +
> +static const struct of_device_id isl1208_of_match[] = {
> +	{ .compatible = "isil,isl1208", .data = &isl1208_configs[TYPE_ISL1208] },
> +	{ .compatible = "isil,isl1209", .data = &isl1208_configs[TYPE_ISL1209] },

This compatible needs to be documented.

> +	{ .compatible = "isil,isl1218", .data = &isl1208_configs[TYPE_ISL1218] },
> +	{ .compatible = "isil,isl1219", .data = &isl1208_configs[TYPE_ISL1219] },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, isl1208_of_match);
> +
>  /* Device state */
>  struct isl1208_state {
>  	struct rtc_device *rtc;
> +	const struct isl1208_config *config;
>  };
>  
>  /* block read */
> @@ -602,11 +636,12 @@ isl1208_rtc_interrupt(int irq, void *data)
>  			return err;
>  	}
>  
> -	if (sr & ISL1208_REG_SR_EVT) {
> -		sysfs_notify(&isl1208->rtc->dev.kobj, NULL,
> -			     dev_attr_timestamp0.attr.name);
> +	if (isl1208->config->has_tamper && (sr & ISL1208_REG_SR_EVT)) {
>  		dev_warn(&client->dev, "event detected");
>  		handled = 1;
> +		if (isl1208->config->has_timestamp)
> +			sysfs_notify(&isl1208->rtc->dev.kobj, NULL,
> +				     dev_attr_timestamp0.attr.name);
>  	}
>  
>  	return handled ? IRQ_HANDLED : IRQ_NONE;
> @@ -743,6 +778,19 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		return -ENOMEM;
>  	i2c_set_clientdata(client, isl1208);
>  
> +	/* Determine which chip we have */
> +	if (client->dev.of_node) {
> +		const struct of_device_id *match =
> +			of_match_node(isl1208_of_match, client->dev.of_node);
> +		if (!match)

This will never happen, else the driver would not be probed.

> +			return -EINVAL;
> +		isl1208->config = match->data;

Maybe you should use of_device_get_match_data anyway.

> +	} else {
> +		if (id->driver_data >= ISL_LAST_ID)
> +			return -EINVAL;

This should be -ENODEV.

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

Re: [PATCH 2/3] rtc: isl1208: Support more chip variations

[Trent Piepho]

On Sun, 2019-02-10 at 22:12 +0100, Alexandre Belloni wrote:
> +};
> > +MODULE_DEVICE_TABLE(i2c, isl1208_id);
> > +
> > +static const struct of_device_id isl1208_of_match[] = {
> > +	{ .compatible = "isil,isl1208", .data = &isl1208_configs[TYPE_ISL1208] },
> > +	{ .compatible = "isil,isl1209", .data = &isl1208_configs[TYPE_ISL1209] },
> 
> This compatible needs to be documented.

I see isil,isl1219 escaped documentation when it was added too.  I'll
add both to rtc.txt.

> >  
> >  	return handled ? IRQ_HANDLED : IRQ_NONE;
> > @@ -743,6 +778,19 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  		return -ENOMEM;
> >  	i2c_set_clientdata(client, isl1208);
> >  
> > +	/* Determine which chip we have */
> > +	if (client->dev.of_node) {
> > +		const struct of_device_id *match =
> > +			of_match_node(isl1208_of_match, client->dev.of_node);
> > +		if (!match)
> 
> This will never happen, else the driver would not be probed.

If there was a driver_override ability with i2c, like PCI and SPI, then
it would be possible.

> 
> > +			return -EINVAL;
> > +		isl1208->config = match->data;
> 
> Maybe you should use of_device_get_match_data anyway.

Yes, of_device_get_match_data() would be better.  Not sure why I didn't
find that.

Interestingly, I see could have just depended on id->driver_data being
set even in the OF case, as the i2c-core code always sets it.  But it
look to be deprecated and i2c clients should provide a probe_new method
that does not provide an i2c_device_id.  I'm not sure if that's
entirely better, as every i2c client that cares will need to re-
implement the code to look up by device name and/or OF node and/or
ACPI.

> 
> > +	} else {
> > +		if (id->driver_data >= ISL_LAST_ID)
> > +			return -EINVAL;
> 
> This should be -ENODEV.

There's quite a few drivers that return EINVAL when the driver_data
appears to be incorrect.  But some also return ENODEV.