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@xxxxxxxxxxxx>
> Cc: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> Signed-off-by: Trent Piepho <tpiepho@xxxxxxxxxx>
> ---
> 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
