[PATCH v3 0/3] Add support for STMicroelectronics M24LR EEPROM/NFC chips

[PATCH v3 0/3] Add support for STMicroelectronics M24LR EEPROM/NFC chips

[Abd-Alrhman Masalkhi]

This patch series adds support for the STMicroelectronics M24LR series
RFID/NFC EEPROM devices. These chips expose two I2C addresses: the primary
one provides access to system control and configuration registers, while
the secondary address is used for EEPROM access.

The driver implements both functionalities:
 - A sysfs-based interface for the control and system parameter registers.
 - EEPROM access via the nvmem subsystem using a secondary I2C dummy client.

Brief summary of changes in v3:
 - Full support for ST M24LR chips, including integrated EEPROM access
   within the same driver-no need for the at24 driver.
 - Dropped usage of the I2C mux API.
 - Switched to using the NVMEM subsystem for EEPROM access.
 - Removed reference to the i2c-mux binding.
 - Added reference to the nvmem binding to reflect EEPROM integration.
 - Updated the reg property to support devices exposing two I2C
   addresses, one for control and one for EEPROM.

Tested on: M24LR04E-R using Yocto on Raspberry Pi 4

Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>

Abd-Alrhman Masalkhi (3):
  dt-bindings: eeprom: Add ST M24LR support
  misc: add driver for ST M24LR series RFID/NFC EEPROM chips
  ABI: sysfs: add documentation for ST M24LR EEPROM and control
    interface

 .../ABI/testing/sysfs-bus-i2c-devices-m24lr   |  96 +++
 .../devicetree/bindings/misc/st,m24lr.yaml    |  54 ++
 drivers/misc/Kconfig                          |  17 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/m24lr.c                          | 705 ++++++++++++++++++
 5 files changed, 873 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-m24lr
 create mode 100644 Documentation/devicetree/bindings/misc/st,m24lr.yaml
 create mode 100644 drivers/misc/m24lr.c

-- 
2.43.0

[PATCH v3 1/3] dt-bindings: eeprom: Add ST M24LR support

[Abd-Alrhman Masalkhi]

Add support for STMicroelectronics M24LR RFID/NFC EEPROM chips.
These devices use two I2C addresses: the primary address provides
access to control and system parameter registers, while the
secondary address is used for EEPROM access.

Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v3:
 - Dropped reference to the i2c-mux binding.
 - Added reference to the nvmem binding to reflect EEPROM usage.
 - Updated 'reg' property to represent the device using two I2C addresses.
 - Fixed DT schema errors and yamllint warnings.
 - Removed the unused 'pagesize' property.
---
 .../devicetree/bindings/misc/st,m24lr.yaml    | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/st,m24lr.yaml

diff --git a/Documentation/devicetree/bindings/misc/st,m24lr.yaml b/Documentation/devicetree/bindings/misc/st,m24lr.yaml
new file mode 100644
index 000000000000..775d218381b7
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/st,m24lr.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/misc/st,m24lr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STMicroelectronics M24LR NFC/RFID EEPROM
+
+maintainers:
+  - Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
+
+description:
+  STMicroelectronics M24LR series are dual-interface (RF + I2C)
+  EEPROM chips. These devices support I2C-based access to both
+  memory and a system area that controls authentication and configuration.
+  They expose two I2C addresses, one for the system parameter sector and
+  one for the EEPROM.
+
+allOf:
+  - $ref: /schemas/nvmem/nvmem.yaml#
+
+properties:
+  compatible:
+    enum:
+      - st,m24lr04e-r
+      - st,m24lr16e-r
+      - st,m24lr64e-r
+
+  reg:
+    description:
+      Two I2C address, the primary for control registers, the secondary
+      for EEPROM access.
+    minItems: 2
+    maxItems: 2
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      eeprom@57 {
+        compatible = "st,m24lr04e-r";
+        reg = <0x57>, /* primary-device */
+              <0x53>; /* secondary-device */
+      };
+    };
+...
-- 
2.43.0

Re: [PATCH v3 1/3] dt-bindings: eeprom: Add ST M24LR support

[Krzysztof Kozlowski]

On 06/06/2025 14:06, Abd-Alrhman Masalkhi wrote:
> Add support for STMicroelectronics M24LR RFID/NFC EEPROM chips.
> These devices use two I2C addresses: the primary address provides
> access to control and system parameter registers, while the
> secondary address is used for EEPROM access.
> 
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> Changes in v3:
>  - Dropped reference to the i2c-mux binding.
>  - Added reference to the nvmem binding to reflect EEPROM usage.
>  - Updated 'reg' property to represent the device using two I2C addresses.
>  - Fixed DT schema errors and yamllint warnings.
>  - Removed the unused 'pagesize' property.
> ---
>  .../devicetree/bindings/misc/st,m24lr.yaml    | 54 +++++++++++++++++++


How did you implement this feedback:

"That's not a misc device, but eeprom. Place it in appropriate directory."
?

There is no such device as a misc device.

>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/st,m24lr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/st,m24lr.yaml b/Documentation/devicetree/bindings/misc/st,m24lr.yaml
> new file mode 100644
> index 000000000000..775d218381b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/st,m24lr.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/st,m24lr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STMicroelectronics M24LR NFC/RFID EEPROM
> +
> +maintainers:
> +  - Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> +
> +description:
> +  STMicroelectronics M24LR series are dual-interface (RF + I2C)
> +  EEPROM chips. These devices support I2C-based access to both
> +  memory and a system area that controls authentication and configuration.
> +  They expose two I2C addresses, one for the system parameter sector and
> +  one for the EEPROM.
> +
> +allOf:
> +  - $ref: /schemas/nvmem/nvmem.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - st,m24lr04e-r
> +      - st,m24lr16e-r
> +      - st,m24lr64e-r
> +
> +  reg:
> +    description:
> +      Two I2C address, the primary for control registers, the secondary
> +      for EEPROM access.
> +    minItems: 2
> +    maxItems: 2

Replace this all with items and description:
items:
  - description: foo
  - description: bar


> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      eeprom@57 {
> +        compatible = "st,m24lr04e-r";
> +        reg = <0x57>, /* primary-device */
> +              <0x53>; /* secondary-device */

Where is the rest of at24 properties? Not relevant? Not correct? I had
impression this is fully at24 compatible.


Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: eeprom: Add ST M24LR support

[Abd-Alrhman Masalkhi]

Hi Krzysztof,

thank you for the feedback.

> How did you implement this feedback:
>
> "That's not a misc device, but eeprom. Place it in appropriate directory."
> ?
>
> There is no such device as a misc device.

I will move it to the eeprom dirctory 

>> +  reg:
>> +    description:
>> +      Two I2C address, the primary for control registers, the secondary
>> +      for EEPROM access.
>> +    minItems: 2
>> +    maxItems: 2
>
> Replace this all with items and description:
> items:
>   - description: foo
>   - description: bar

I will do that.


>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      eeprom@57 {
>> +        compatible = "st,m24lr04e-r";
>> +        reg = <0x57>, /* primary-device */
>> +              <0x53>; /* secondary-device */
>
> Where is the rest of at24 properties? Not relevant? Not correct? I had
> impression this is fully at24 compatible.

The driver does not currently accept at24-style configuration properties
(e.g., pagesize, address-width) because the EEPROM access is handled
internally using fixed parameters per device variant. Let me know if
adding support for these via DT is preferred, I'm happy to extend it
for flexibility.

Best regards,
Abd-Alrhman Masalkhi

[PATCH v3 2/3] misc: add driver for ST M24LR series RFID/NFC EEPROM chips

[Abd-Alrhman Masalkhi]

adds support for STMicroelectronics M24LRxx devices, which expose
two separate I2C addresses: one for system control and one for EEPROM
access. The driver implements both a sysfs-based interface for control
registers (e.g. UID, password authentication) and an nvmem provider
for EEPROM access.

Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v3:
 - Fully support the M24LR chips, including EEPROM access, no need for
   the standard at24 driver to handle EEPROM separately.
 - Rename the driver file from m24lr_ctl.c to m24lr.c.
 - Rename all identifiers from the m24lr_ctl prefix to m24lr.
 - Retain the m24lr_ctl prefix for control-related routines to distinguish
   them from EEPROM-related logic.
 - Drop usage of the I2C mux API.
 - Use the NVMEM subsystem to handle EEPROM access.
 - Add REGMAP support for EEPROM register access.
 - Update Kconfig entry to reflect that the driver now supports both
   control and EEPROM functionality.

Changes in v2:
 - Fix compiling Errors and Warnings
 - Replace scnprintf with sysfs_emit
 - Drop success log message from probe.

Comment:
 - Running checkpatch emit a warning for non-const regmap_config.
   The variable must remain auto and mutable due to runtime manipulation.
---
 drivers/misc/Kconfig  |  17 +
 drivers/misc/Makefile |   1 +
 drivers/misc/m24lr.c  | 705 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 723 insertions(+)
 create mode 100644 drivers/misc/m24lr.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index c161546d728f..96dc2bf2ad45 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -644,6 +644,23 @@ config MCHP_LAN966X_PCI
 	    - lan966x-miim (MDIO_MSCC_MIIM)
 	    - lan966x-switch (LAN966X_SWITCH)
 
+config M24LR
+	tristate "STMicroelectronics M24LR RFID/NFC EEPROM support"
+	depends on SYSFS
+	select REGMAP_I2C
+	select NVMEM
+	help
+	  This enables support for STMicroelectronics M24LR RFID/NFC EEPROM
+	  chips. These dual-interface devices expose two I2C addresses:
+	  one for EEPROM memory access and another for control and system
+	  configuration (e.g. UID, password handling).
+
+	  This driver provides a sysfs interface for control functions and
+	  integrates with the nvmem subsystem for EEPROM access.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called m24lr.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 054cee9b08a4..689b68fad9fd 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -75,3 +75,4 @@ lan966x-pci-objs		:= lan966x_pci.o
 lan966x-pci-objs		+= lan966x_pci.dtbo.o
 obj-$(CONFIG_MCHP_LAN966X_PCI)	+= lan966x-pci.o
 obj-y				+= keba/
+obj-$(CONFIG_M24LR)		+= m24lr.o
diff --git a/drivers/misc/m24lr.c b/drivers/misc/m24lr.c
new file mode 100644
index 000000000000..46a3c8ef7b0f
--- /dev/null
+++ b/drivers/misc/m24lr.c
@@ -0,0 +1,705 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * m24lr.c - Sysfs control interface for ST M24LR series RFID/NFC chips
+ *
+ * Copyright (c) 2025 Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
+ *
+ * This driver implements both the sysfs-based control interface and EEPROM
+ * access for STMicroelectronics M24LR series chips (e.g., M24LR04E-R).
+ * It provides access to control registers for features such as password
+ * authentication, memory protection, and device configuration. In addition,
+ * it manages read and write operations to the EEPROM region of the chip.
+ */
+
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/nvmem-provider.h>
+
+#define M24LR_PAGESIZE_DEFAULT	  1u
+
+#define M24LR_WRITE_TIMEOUT	  25u
+#define M24LR_READ_TIMEOUT	  (M24LR_WRITE_TIMEOUT)
+
+#define to_sys_entry(attrp)   container_of(attrp, struct m24lr_sys_entry, attr)
+
+/**
+ * struct m24lr_sys_entry - describes a sysfs entry for M24LR device access
+ * @reg_addr:  register address in the M24LR device
+ * @reg_size:  size of the register in bytes
+ * @attr:      sysfs attribute used for exposing register access to userspace
+ *
+ * Used to define readable/writable register entries through sysfs.
+ */
+struct m24lr_sys_entry {
+	unsigned int reg_addr;
+	unsigned int reg_size;
+	struct device_attribute attr;
+};
+
+/**
+ * struct m24lr_chip - describes chip-specific sysfs layout
+ * @page_size:	   chip-specific limit on the maximum number of bytes allowed
+ *		   in a single write operation.
+ * @eeprom_size:   size of the EEPROM in byte
+ * @n_entries:	   number of entries in the array
+ * @n_sss_entries: number of sss entries required for the chip
+ * @entries:	   array of sysfs entries specific to the chip variant
+ *
+ * Supports multiple M24LR chip variants (e.g., M24LRxx) by allowing each
+ * to define its own set of sysfs attributes, depending on its available
+ * registers and features.
+ */
+struct m24lr_chip {
+	unsigned int page_size;
+	unsigned int eeprom_size;
+	unsigned int n_entries;
+	unsigned int n_sss_entries;
+	const struct m24lr_sys_entry *entries;
+};
+
+/**
+ * struct m24lr - core driver data for M24LR chip control
+ * @page_size:	   chip-specific limit on the maximum number of bytes allowed
+ *		   in a single write operation.
+ * @eeprom_size:   size of the EEPROM in byte
+ * @ctl_regmap:	   regmap interface for accessing the system parameter sector
+ * @eeprom_regmap: regmap interface for accessing the EEPROM
+ * @lock:	   mutex to synchronize operations to the device
+ * @sss_entries:   array of sssc sysfs entries specific to the chip variant
+ * @n_sss_entries: number of entries in the sss entries array
+ *
+ * Central data structure holding the state and resources used by the
+ * M24LR device driver.
+ */
+struct m24lr {
+	unsigned int page_size;
+	unsigned int eeprom_size;
+	struct regmap *ctl_regmap;
+	struct regmap *eeprom_regmap;
+	struct mutex lock;	 /* synchronize operations to the device */
+	struct m24lr_sys_entry *sss_entries;
+	unsigned int n_sss_entries;
+};
+
+static ssize_t m24lr_ctl_show(struct device *dev,
+			      struct device_attribute *attr, char *buf);
+static ssize_t m24lr_ctl_store(struct device *dev, struct device_attribute *attr,
+			       const char *buf, size_t count);
+static ssize_t m24lr_ctl_unlock_command(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count);
+static ssize_t m24lr_ctl_newpass_command(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count);
+
+static const struct regmap_range m24lr_ctl_vo_ranges[] = {
+	regmap_reg_range(0, 63),
+};
+
+static const struct regmap_access_table m24lr_ctl_vo_table = {
+	.yes_ranges = m24lr_ctl_vo_ranges,
+	.n_yes_ranges = ARRAY_SIZE(m24lr_ctl_vo_ranges),
+};
+
+static const struct regmap_config m24lr_ctl_regmap_conf = {
+	.name = "m24lr_ctl",
+	.reg_stride = 1,
+	.reg_bits = 16,
+	.val_bits = 8,
+	.disable_locking = false,
+	.cache_type = REGCACHE_RBTREE,/* Flat can't be used, there's huge gap */
+	.volatile_table = &m24lr_ctl_vo_table,
+};
+
+/* define the default sysfs entries for M24LR chips */
+static const struct m24lr_sys_entry m24lr_ctl_sys_entry_default_table[] = {
+	{.attr = __ATTR(unlock, 0200, NULL, m24lr_ctl_unlock_command)},
+	{.attr = __ATTR(new_pass, 0200, NULL, m24lr_ctl_newpass_command)},
+	{.reg_addr = 2324, .reg_size = 8,
+	 .attr = __ATTR(uid, 0444, m24lr_ctl_show, NULL)},
+	{.reg_addr = 2334, .reg_size = 1,
+	 .attr = __ATTR(mem_size, 0400, m24lr_ctl_show, NULL)},
+};
+
+/* Chip descriptor for M24LR04E-R variant */
+static const struct m24lr_chip m24lr04e_r_chip = {
+	.page_size = 4,
+	.eeprom_size = 512,
+	.n_sss_entries = 4,
+	.n_entries = ARRAY_SIZE(m24lr_ctl_sys_entry_default_table),
+	.entries = m24lr_ctl_sys_entry_default_table,
+};
+
+/* Chip descriptor for M24LR16E-R variant */
+static const struct m24lr_chip m24lr16e_r_chip = {
+	.page_size = 4,
+	.eeprom_size = 2048,
+	.n_sss_entries = 16,
+	.n_entries = ARRAY_SIZE(m24lr_ctl_sys_entry_default_table),
+	.entries = m24lr_ctl_sys_entry_default_table,
+};
+
+/* Chip descriptor for M24LR64E-R variant */
+static const struct m24lr_chip m24lr64e_r_chip = {
+	.page_size = 4,
+	.eeprom_size = 8192,
+	.n_sss_entries = 64,
+	.n_entries = ARRAY_SIZE(m24lr_ctl_sys_entry_default_table),
+	.entries = m24lr_ctl_sys_entry_default_table,
+};
+
+static const struct i2c_device_id m24lr_ids[] = {
+	{ "m24lr04e-r", (kernel_ulong_t)&m24lr04e_r_chip},
+	{ "m24lr16e-r", (kernel_ulong_t)&m24lr16e_r_chip},
+	{ "m24lr64e-r", (kernel_ulong_t)&m24lr64e_r_chip},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, m24lr_ids);
+
+static const struct of_device_id m24lr_of_match[] = {
+	{ .compatible = "st,m24lr04e-r", .data = &m24lr04e_r_chip},
+	{ .compatible = "st,m24lr16e-r", .data = &m24lr16e_r_chip},
+	{ .compatible = "st,m24lr64e-r", .data = &m24lr64e_r_chip},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, m24lr_of_match);
+
+/**
+ * m24lr_parse_le_value - Parse hex string and convert to little-endian binary
+ * @buf:	Input string buffer (hex format)
+ * @reg_size:	Size of the register in bytes (must be 1, 2, 4, or 8)
+ * @output:	Output buffer to store the value in little-endian format
+ *
+ * Converts a hexadecimal string to a numeric value of the given register size
+ * and writes it in little-endian byte order into the provided buffer.
+ *
+ * Return: 0 on success, or negative error code on failure
+ */
+static int m24lr_parse_le_value(const char *buf, u32 reg_size, u8 *output)
+{
+	int err;
+
+	switch (reg_size) {
+	case 1: {
+		u8 tmp;
+
+		err = kstrtou8(buf, 16, &tmp);
+		if (!err)
+			*output = tmp;
+		break;
+	}
+	case 2: {
+		u16 tmp;
+
+		err = kstrtou16(buf, 16, &tmp);
+		if (!err)
+			*(__le16 *)output = cpu_to_le16(tmp);
+		break;
+	}
+	case 4: {
+		u32 tmp;
+
+		err = kstrtou32(buf, 16, &tmp);
+		if (!err)
+			*(__le32 *)output = cpu_to_le32(tmp);
+		break;
+	}
+	case 8: {
+		u64 tmp;
+
+		err = kstrtou64(buf, 16, &tmp);
+		if (!err)
+			*(__le64 *)output = cpu_to_le64(tmp);
+		break;
+	}
+	default:
+		err = -EINVAL;
+	}
+
+	return err;
+}
+
+/**
+ * m24lr_regmap_read - read data using regmap with retry on failure
+ * @regmap:  regmap instance for the device
+ * @buf:     buffer to store the read data
+ * @size:    number of bytes to read
+ * @offset:  starting register address
+ *
+ * Attempts to read a block of data from the device with retries and timeout.
+ * Some M24LR chips may transiently NACK reads (e.g., during internal write
+ * cycles), so this function retries with a short sleep until the timeout
+ * expires.
+ *
+ * Returns:
+ *	 Number of bytes read on success,
+ *	 -ETIMEDOUT if the read fails within the timeout window.
+ */
+static ssize_t m24lr_regmap_read(struct regmap *regmap, u8 *buf,
+				 size_t size, unsigned int offset)
+{
+	int err;
+	unsigned long timeout, read_time;
+	ssize_t ret = -ETIMEDOUT;
+
+	timeout = jiffies + msecs_to_jiffies(M24LR_READ_TIMEOUT);
+	do {
+		read_time = jiffies;
+
+		err = regmap_bulk_read(regmap, offset, buf, size);
+		if (!err) {
+			ret = size;
+			break;
+		}
+
+		usleep_range(1000, 2000);
+	} while (time_before(read_time, timeout));
+
+	return ret;
+}
+
+/**
+ * m24lr_regmap_write - write data using regmap with retry on failure
+ * @regmap: regmap instance for the device
+ * @buf:    buffer containing the data to write
+ * @size:   number of bytes to write
+ * @offset: starting register address
+ *
+ * Attempts to write a block of data to the device with retries and a timeout.
+ * Some M24LR devices may NACK I2C writes while an internal write operation
+ * is in progress. This function retries the write operation with a short delay
+ * until it succeeds or the timeout is reached.
+ *
+ * Returns:
+ *	 Number of bytes written on success,
+ *	 -ETIMEDOUT if the write fails within the timeout window.
+ */
+static ssize_t m24lr_regmap_write(struct regmap *regmap, const u8 *buf,
+				  size_t size, unsigned int offset)
+{
+	int err;
+	unsigned long timeout, write_time;
+	ssize_t ret = -ETIMEDOUT;
+
+	timeout = jiffies + msecs_to_jiffies(M24LR_WRITE_TIMEOUT);
+
+	do {
+		write_time = jiffies;
+
+		err = regmap_bulk_write(regmap, offset, buf, size);
+		if (!err) {
+			ret = size;
+			break;
+		}
+
+		usleep_range(1000, 2000);
+	} while (time_before(write_time, timeout));
+
+	return ret;
+}
+
+static ssize_t m24lr_read(struct m24lr *m24lr, u8 *buf, size_t size,
+			  unsigned int offset, bool is_eeprom)
+{
+	struct regmap *regmap;
+	long ret;
+
+	if (is_eeprom)
+		regmap = m24lr->eeprom_regmap;
+	else
+		regmap = m24lr->ctl_regmap;
+
+	mutex_lock(&m24lr->lock);
+	ret = m24lr_regmap_read(regmap, buf, size, offset);
+	mutex_unlock(&m24lr->lock);
+
+	return ret;
+}
+
+/**
+ * m24lr_write - write buffer to M24LR device with page alignment handling
+ * @m24lr:  pointer to driver context
+ * @buf:    data buffer to write
+ * @size:   number of bytes to write
+ * @offset: target register address in the device
+ *
+ * Writes data to the M24LR device using regmap, split into chunks no larger
+ * than page_size to respect device-specific write limitations (e.g., page
+ * size or I2C hold-time concerns). Each chunk is aligned to the page boundary
+ * defined by page_size.
+ *
+ * Returns:
+ *	 Total number of bytes written on success,
+ *	 A negative error code if any write fails.
+ */
+static ssize_t m24lr_write(struct m24lr *m24lr, const u8 *buf, size_t size,
+			   unsigned int offset, bool is_eeprom)
+{
+	unsigned int n, next_sector;
+	struct regmap *regmap;
+	ssize_t ret = 0;
+	long err;
+
+	if (is_eeprom)
+		regmap = m24lr->eeprom_regmap;
+	else
+		regmap = m24lr->ctl_regmap;
+
+	n = min(size, m24lr->page_size);
+	next_sector = roundup(offset + 1, m24lr->page_size);
+	if (offset + n > next_sector)
+		n = next_sector - offset;
+
+	mutex_lock(&m24lr->lock);
+	while (n) {
+		err = m24lr_regmap_write(regmap, buf, n, offset);
+		if (IS_ERR_VALUE(err)) {
+			mutex_unlock(&m24lr->lock);
+			if (ret)
+				return ret;
+			else
+				return err;
+		}
+
+		offset += n;
+		size -= n;
+		ret += n;
+		n = min(size, m24lr->page_size);
+	}
+	mutex_unlock(&m24lr->lock);
+
+	return ret;
+}
+
+/**
+ * m24lr_write_pass - Write password to M24LR043-R using secure format
+ * @m24lr: Pointer to device control structure
+ * @buf:   Input buffer containing hex-encoded password
+ * @count: Number of bytes in @buf
+ * @code:  Operation code to embed between password copies
+ *
+ * This function parses a 4-byte password, encodes it in  big-endian format,
+ * and constructs a 9-byte sequence of the form:
+ *
+ *	  [BE(password), code, BE(password)]
+ *
+ * The result is written to register 0x0900 (2304), which is the password
+ * register in M24LR04E-R chip.
+ *
+ * Return: Number of bytes written on success, or negative error code on failure
+ */
+static ssize_t m24lr_write_pass(struct m24lr *m24lr, const char *buf,
+				size_t count, u8 code)
+{
+	__be32 be_pass;
+	u8 output[9];
+	long ret;
+	u32 pass;
+	int err;
+
+	if (unlikely(!count))
+		return -EINVAL;
+
+	if (count > 8)
+		return -EINVAL;
+
+	err = kstrtou32(buf, 16, &pass);
+	if (err)
+		return err;
+
+	be_pass = cpu_to_be32(pass);
+
+	memcpy(output, &be_pass, sizeof(be_pass));
+	output[4] = code;
+	memcpy(output + 5, &be_pass, sizeof(be_pass));
+
+	mutex_lock(&m24lr->lock);
+	ret = m24lr_regmap_write(m24lr->ctl_regmap, output, 9, 2304);
+	mutex_unlock(&m24lr->lock);
+
+	return ret;
+}
+
+static ssize_t m24lr_ctl_newpass_command(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct m24lr *m24lr = i2c_get_clientdata(to_i2c_client(dev));
+
+	return m24lr_write_pass(m24lr, buf, count, 7);
+}
+
+static ssize_t m24lr_ctl_unlock_command(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	struct m24lr *m24lr = i2c_get_clientdata(to_i2c_client(dev));
+
+	return m24lr_write_pass(m24lr, buf, count, 9);
+}
+
+static ssize_t m24lr_ctl_store(struct device *dev, struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct m24lr *m24lr = i2c_get_clientdata(to_i2c_client(dev));
+	struct m24lr_sys_entry *entry = to_sys_entry(attr);
+	unsigned int reg_size = entry->reg_size;
+	unsigned int reg_addr = entry->reg_addr;
+	u8 output[8];
+	int err = 0;
+
+	if (unlikely(!count))
+		return -EINVAL;
+
+	if (count > (reg_size << 1))
+		return -EINVAL;
+
+	if (unlikely(!is_power_of_2(reg_size) || reg_size > 8)) {
+		dev_dbg(dev,
+			"Invalid register size: must be a power of 2 and <= 8 bytes (%u)\n",
+			reg_size);
+		return -EIO;
+	}
+
+	err = m24lr_parse_le_value(buf, reg_size, output);
+	if (err)
+		return err;
+
+	return m24lr_write(m24lr, (u8 *)&output, reg_size, reg_addr, false);
+}
+
+static ssize_t m24lr_ctl_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	long ret;
+	u64 val;
+	__le64 input = 0;
+	struct m24lr *m24lr = i2c_get_clientdata(to_i2c_client(dev));
+	struct m24lr_sys_entry *entry = to_sys_entry(attr);
+	unsigned int reg_addr = entry->reg_addr;
+	unsigned int reg_size = entry->reg_size;
+
+	if (unlikely(!is_power_of_2(reg_size) || reg_size > 8)) {
+		dev_dbg(dev,
+			"Invalid register size: must be a power of 2 and <= 8 bytes (%u)\n",
+			reg_size);
+		return -EIO;
+	}
+
+	ret = m24lr_read(m24lr, (u8 *)&input, reg_size, reg_addr, false);
+	if (IS_ERR_VALUE(ret))
+		return ret;
+
+	if (ret != reg_size)
+		return -EIO;
+
+	switch (reg_size) {
+	case 1:
+		val = *(u8 *)&input;
+		break;
+	case 2:
+		val = le16_to_cpu((__le16)input);
+		break;
+	case 4:
+		val = le32_to_cpu((__le32)input);
+		break;
+	case 8:
+		val = le64_to_cpu((__le64)input);
+		break;
+	};
+
+	return sysfs_emit(buf, "%llx\n", val);
+}
+
+static int m24lr_nvmem_read(void *priv, unsigned int offset, void *val,
+			    size_t bytes)
+{
+	struct m24lr *m24lr = priv;
+
+	if (unlikely(!bytes))
+		return bytes;
+
+	if (offset + bytes > m24lr->eeprom_size)
+		return -EINVAL;
+
+	return m24lr_read(m24lr, val, bytes, offset, true);
+}
+
+static int m24lr_nvmem_write(void *priv, unsigned int offset, void *val,
+			     size_t bytes)
+{
+	struct m24lr *m24lr = priv;
+
+	if (unlikely(!bytes))
+		return -EINVAL;
+
+	if (offset + bytes > m24lr->eeprom_size)
+		return -EINVAL;
+
+	return m24lr_write(m24lr, val, bytes, offset, true);
+}
+
+static const struct m24lr_chip *m24lr_get_chip(struct device *dev)
+{
+	const struct m24lr_chip *ret;
+	const struct i2c_device_id *id;
+
+	id = i2c_match_id(m24lr_ids, to_i2c_client(dev));
+
+	if (dev->of_node && of_match_device(m24lr_of_match, dev))
+		ret = of_device_get_match_data(dev);
+	else if (id)
+		ret = (void *)id->driver_data;
+	else
+		ret = acpi_device_get_match_data(dev);
+
+	return ret;
+}
+
+static int m24lr_probe(struct i2c_client *client)
+{
+	struct regmap_config eeprom_regmap_conf = {0};
+	struct nvmem_config nvmem_conf = {0};
+	struct m24lr_sys_entry *sss = NULL;
+	struct device *dev = &client->dev;
+	struct i2c_client *eeprom_client;
+	const struct m24lr_chip *chip;
+	struct regmap *eeprom_regmap;
+	struct nvmem_device *nvmem;
+	struct regmap *ctl_regmap;
+	unsigned int n_sss;
+	struct m24lr *m24lr;
+	u32 regs[2];
+	long err;
+	u8 test;
+	int i;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -EOPNOTSUPP;
+
+	chip = m24lr_get_chip(dev);
+	if (!chip)
+		return -ENODEV;
+
+	m24lr = devm_kzalloc(dev, sizeof(struct m24lr), GFP_KERNEL);
+	if (!m24lr)
+		return -ENOMEM;
+
+	err = device_property_read_u32_array(dev, "reg", regs, ARRAY_SIZE(regs));
+	if (err) {
+		dev_err(dev, "device_property_read_u32_array\n");
+		return err;
+	}
+	/* Create a second I2C client for the eeprom interface */
+	eeprom_client = devm_i2c_new_dummy_device(dev, client->adapter, regs[1]);
+	if (IS_ERR(eeprom_client)) {
+		dev_err(dev,
+			"Failed to create dummy I2C client for the EEPROM\n");
+		return PTR_ERR(eeprom_client);
+	}
+
+	for (i = 0; i < chip->n_entries; i++) {
+		const struct device_attribute *attr = &chip->entries[i].attr;
+
+		err = device_create_file(dev, attr);
+		if (err)
+			dev_warn(dev,
+				 "Failed to create sysfs entry '%s'\n",
+				 attr->attr.name);
+	}
+
+	n_sss = chip->n_sss_entries;
+	if (n_sss) {
+		sss = devm_kzalloc(dev, n_sss * sizeof(struct m24lr_sys_entry),
+				   GFP_KERNEL);
+		if (!sss)
+			return -ENOMEM;
+
+		for (i = 0; i < n_sss; i++) {
+			char *name = devm_kasprintf(dev, GFP_KERNEL, "sss%d", i);
+
+			sss[i].reg_size = 1;
+			sss[i].reg_addr = i;
+			sss[i].attr.attr.name = name;
+			sss[i].attr.attr.mode = 0600;
+			sss[i].attr.show = m24lr_ctl_show;
+			sss[i].attr.store = m24lr_ctl_store;
+
+			err = device_create_file(dev, &sss[i].attr);
+			if (err)
+				dev_warn(dev,
+					 "Failed to create sysfs entry '%s'\n",
+					 name);
+		}
+	}
+
+	ctl_regmap = devm_regmap_init_i2c(client, &m24lr_ctl_regmap_conf);
+	if (IS_ERR(ctl_regmap)) {
+		err = PTR_ERR(ctl_regmap);
+		dev_err(dev, "Failed to init regmap\n");
+		return err;
+	}
+
+	eeprom_regmap_conf.name = "m24lr_eeprom";
+	eeprom_regmap_conf.reg_bits = 16;
+	eeprom_regmap_conf.val_bits = 8;
+	eeprom_regmap_conf.disable_locking = true;
+	eeprom_regmap_conf.max_register = chip->eeprom_size - 1;
+
+	eeprom_regmap = devm_regmap_init_i2c(eeprom_client,
+					     &eeprom_regmap_conf);
+	if (IS_ERR(eeprom_regmap)) {
+		err = PTR_ERR(eeprom_regmap);
+		dev_err(dev, "Failed to init regmap\n");
+		return err;
+	}
+
+	mutex_init(&m24lr->lock);
+	m24lr->page_size = chip->page_size;
+	m24lr->eeprom_size = chip->eeprom_size;
+	m24lr->eeprom_regmap = eeprom_regmap;
+	m24lr->ctl_regmap = ctl_regmap;
+	m24lr->n_sss_entries = n_sss;
+	m24lr->sss_entries = sss;
+
+	nvmem_conf.dev = &eeprom_client->dev;
+	nvmem_conf.owner = THIS_MODULE;
+	nvmem_conf.type = NVMEM_TYPE_EEPROM;
+	nvmem_conf.reg_read = m24lr_nvmem_read;
+	nvmem_conf.reg_write = m24lr_nvmem_write;
+	nvmem_conf.size = chip->eeprom_size;
+	nvmem_conf.word_size = 1;
+	nvmem_conf.stride = 1;
+	nvmem_conf.priv = m24lr;
+
+	nvmem = devm_nvmem_register(dev, &nvmem_conf);
+	if (IS_ERR(nvmem))
+		return PTR_ERR(nvmem);
+
+	i2c_set_clientdata(client, m24lr);
+	i2c_set_clientdata(eeprom_client, m24lr);
+
+	err = m24lr_read(m24lr, &test, 1, 0, true);
+	if (IS_ERR_VALUE(err))
+		return -ENODEV;
+
+	return 0;
+}
+
+static struct i2c_driver m24lr_driver = {
+	.driver = {
+		.name = "m24lr",
+		.of_match_table = m24lr_of_match,
+	},
+	.probe	  = m24lr_probe,
+	.id_table = m24lr_ids,
+};
+module_i2c_driver(m24lr_driver);
+
+MODULE_AUTHOR("Abd-Alrhman Masalkhi");
+MODULE_DESCRIPTION("st m24lr control driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v3 2/3] misc: add driver for ST M24LR series RFID/NFC EEPROM chips

[Greg KH]

On Fri, Jun 06, 2025 at 12:06:30PM +0000, Abd-Alrhman Masalkhi wrote:
> adds support for STMicroelectronics M24LRxx devices, which expose
> two separate I2C addresses: one for system control and one for EEPROM
> access. The driver implements both a sysfs-based interface for control
> registers (e.g. UID, password authentication) and an nvmem provider
> for EEPROM access.
> 
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> Changes in v3:
>  - Fully support the M24LR chips, including EEPROM access, no need for
>    the standard at24 driver to handle EEPROM separately.

Why isn't this under drivers/misc/eeprom/ instead?

thanks,

greg k-h

Re: [PATCH v3 2/3] misc: add driver for ST M24LR series RFID/NFC EEPROM chips

[Abd-Alrhman Masalkhi]

Hi greg,

Thanks for the feedback.

>> adds support for STMicroelectronics M24LRxx devices, which expose
>> two separate I2C addresses: one for system control and one for EEPROM
>> access. The driver implements both a sysfs-based interface for control
>> registers (e.g. UID, password authentication) and an nvmem provider
>> for EEPROM access.
>> 
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> Changes in v3:
>>  - Fully support the M24LR chips, including EEPROM access, no need for
>>    the standard at24 driver to handle EEPROM separately.
>
> Why isn't this under drivers/misc/eeprom/ instead?

The M24LR series is a dual-interface EEPROM with both I2C and ISO/IEC 15693
RF support. While it is technically an EEPROM, it also exposes a control
interface over I2C via a seprated address, which is used to manage features
such as password protection, energy harvesting configuration, and UID access.
This control interface is not memory-mapped like traditional EEPROMs, which
is why I did not place it under the eeprom

Best regards,
Abd-Alrhman Masalkhi

Re: [PATCH v3 2/3] misc: add driver for ST M24LR series RFID/NFC EEPROM chips

[Greg KH]

On Fri, Jun 06, 2025 at 12:06:30PM +0000, Abd-Alrhman Masalkhi wrote:
> +// SPDX-License-Identifier: GPL-2.0-or-later

Are you sure "or-later" is what you want?  Sorry, I have to ask.

> +/*
> + * m24lr.c - Sysfs control interface for ST M24LR series RFID/NFC chips
> + *
> + * Copyright (c) 2025 Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> + *
> + * This driver implements both the sysfs-based control interface and EEPROM
> + * access for STMicroelectronics M24LR series chips (e.g., M24LR04E-R).
> + * It provides access to control registers for features such as password
> + * authentication, memory protection, and device configuration. In addition,
> + * it manages read and write operations to the EEPROM region of the chip.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/nvmem-provider.h>
> +
> +#define M24LR_PAGESIZE_DEFAULT	  1u
> +
> +#define M24LR_WRITE_TIMEOUT	  25u
> +#define M24LR_READ_TIMEOUT	  (M24LR_WRITE_TIMEOUT)
> +
> +#define to_sys_entry(attrp)   container_of(attrp, struct m24lr_sys_entry, attr)

This shouldn't be needed, something seems odd...

> +static ssize_t m24lr_ctl_store(struct device *dev, struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct m24lr *m24lr = i2c_get_clientdata(to_i2c_client(dev));
> +	struct m24lr_sys_entry *entry = to_sys_entry(attr);

Why isn't this just going off of the device?  Are you using single
show/store callbacks for multiple attribute types?

> +	unsigned int reg_size = entry->reg_size;
> +	unsigned int reg_addr = entry->reg_addr;

Ah, you are.  Are you sure you need/want to do that?

> +	u8 output[8];
> +	int err = 0;
> +
> +	if (unlikely(!count))

likely/unlikely can ONLY be used when you can benchmark the difference
in the speed of not having it.  For a sysfs file, that's not needed at
all, please remove all of these.

> +		return -EINVAL;
> +
> +	if (count > (reg_size << 1))
> +		return -EINVAL;
> +
> +	if (unlikely(!is_power_of_2(reg_size) || reg_size > 8)) {
> +		dev_dbg(dev,
> +			"Invalid register size: must be a power of 2 and <= 8 bytes (%u)\n",
> +			reg_size);
> +		return -EIO;

Not -EINVAL?  This isn't an I/O error.

> +	}
> +
> +	err = m24lr_parse_le_value(buf, reg_size, output);
> +	if (err)
> +		return err;
> +
> +	return m24lr_write(m24lr, (u8 *)&output, reg_size, reg_addr, false);
> +}
> +
> +static ssize_t m24lr_ctl_show(struct device *dev, struct device_attribute *attr,
> +			      char *buf)
> +{
> +	long ret;
> +	u64 val;
> +	__le64 input = 0;
> +	struct m24lr *m24lr = i2c_get_clientdata(to_i2c_client(dev));
> +	struct m24lr_sys_entry *entry = to_sys_entry(attr);
> +	unsigned int reg_addr = entry->reg_addr;
> +	unsigned int reg_size = entry->reg_size;
> +
> +	if (unlikely(!is_power_of_2(reg_size) || reg_size > 8)) {
> +		dev_dbg(dev,
> +			"Invalid register size: must be a power of 2 and <= 8 bytes (%u)\n",
> +			reg_size);
> +		return -EIO;
> +	}
> +
> +	ret = m24lr_read(m24lr, (u8 *)&input, reg_size, reg_addr, false);
> +	if (IS_ERR_VALUE(ret))
> +		return ret;
> +
> +	if (ret != reg_size)
> +		return -EIO;
> +
> +	switch (reg_size) {
> +	case 1:
> +		val = *(u8 *)&input;
> +		break;
> +	case 2:
> +		val = le16_to_cpu((__le16)input);
> +		break;
> +	case 4:
> +		val = le32_to_cpu((__le32)input);
> +		break;
> +	case 8:
> +		val = le64_to_cpu((__le64)input);
> +		break;
> +	};
> +
> +	return sysfs_emit(buf, "%llx\n", val);

Why are "raw" read/writes happening from sysfs to the chip?  That feels
wrong, and an abuse of the sysfs api.

If you really want "raw" access, make it a binary file that userspace
can mmap or read/write from directly, with the kernel getting out of the
way entirely.

> +	n_sss = chip->n_sss_entries;
> +	if (n_sss) {
> +		sss = devm_kzalloc(dev, n_sss * sizeof(struct m24lr_sys_entry),
> +				   GFP_KERNEL);
> +		if (!sss)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < n_sss; i++) {
> +			char *name = devm_kasprintf(dev, GFP_KERNEL, "sss%d", i);
> +
> +			sss[i].reg_size = 1;
> +			sss[i].reg_addr = i;
> +			sss[i].attr.attr.name = name;
> +			sss[i].attr.attr.mode = 0600;
> +			sss[i].attr.show = m24lr_ctl_show;
> +			sss[i].attr.store = m24lr_ctl_store;
> +
> +			err = device_create_file(dev, &sss[i].attr);

You just raced with userspace and lost.  This is not how to do this,
please do not dynamically create attributes (hint, this should have
errored out as you didn't correctly initialize them), but also:

> +			if (err)
> +				dev_warn(dev,
> +					 "Failed to create sysfs entry '%s'\n",
> +					 name);

You do not unwind properly if an error happens.

Just use a default attribute group attached to the driver and the driver
core will handle all of that logic for you automatically.  Making the
code smaller and even better yet, correct :)

thanks,

greg k-h

Re: [PATCH v3 2/3] misc: add driver for ST M24LR series RFID/NFC EEPROM chips

[Abd-Alrhman Masalkhi]

Hi greg,

Thank you for the detailed feedback.

>> +// SPDX-License-Identifier: GPL-2.0-or-later
>
> Are you sure "or-later" is what you want?  Sorry, I have to ask.
I will remove the or-later part, lol

>> +
>> +#define M24LR_PAGESIZE_DEFAULT	  1u
>> +
>> +#define M24LR_WRITE_TIMEOUT	  25u
>> +#define M24LR_READ_TIMEOUT	  (M24LR_WRITE_TIMEOUT)
>> +
>> +#define to_sys_entry(attrp)   container_of(attrp, struct m24lr_sys_entry, attr)
>
> This shouldn't be needed, something seems odd...

I will remove the M24LR_PAGESIZE_DEFAULT, i do not needed any more
and about the to_sys_entry, i am using it in show and store callbacks

>> +static ssize_t m24lr_ctl_store(struct device *dev, struct device_attribute *attr,
>> +			       const char *buf, size_t count)
>> +{
>> +	struct m24lr *m24lr = i2c_get_clientdata(to_i2c_client(dev));
>> +	struct m24lr_sys_entry *entry = to_sys_entry(attr);
>
> Why isn't this just going off of the device?  Are you using single
> show/store callbacks for multiple attribute types?
>
>> +	unsigned int reg_size = entry->reg_size;
>> +	unsigned int reg_addr = entry->reg_addr;

> Ah, you are.  Are you sure you need/want to do that?

For registers that do not require any special processing, it's sufficient
to directly pass the value to the device. In such cases, a generic store
callback is appropriate. Other registers that require specific handling
have dedicated store callbacks. For example, the unlock attribute uses
its own specialized implementation.

>> +	u8 output[8];
>> +	int err = 0;
>> +
>> +	if (unlikely(!count))
>
> likely/unlikely can ONLY be used when you can benchmark the difference
> in the speed of not having it.  For a sysfs file, that's not needed at
> all, please remove all of these.

Alright, i will do that

>> +		return -EINVAL;
>> +
>> +	if (count > (reg_size << 1))
>> +		return -EINVAL;
>> +
>> +	if (unlikely(!is_power_of_2(reg_size) || reg_size > 8)) {
>> +		dev_dbg(dev,
>> +			"Invalid register size: must be a power of 2 and <= 8 bytes (%u)\n",
>> +			reg_size);
>> +		return -EIO;
>
> Not -EINVAL?  This isn't an I/O error.

The last if statement is primarily for debugging purposes. The reg_size
value is specified internally by the driver (not user-controlled), so
this check helps catch potential mistakes in the driver's sysfs entry
definitions. That's why I used -EIO instead of -EINVAL, as it's not due
to invalid user input but rather an internal misconfiguration.

>> +	n_sss = chip->n_sss_entries;
>> +	if (n_sss) {
>> +		sss = devm_kzalloc(dev, n_sss * sizeof(struct m24lr_sys_entry),
>> +				   GFP_KERNEL);
>> +		if (!sss)
>> +			return -ENOMEM;
>> +
>> +		for (i = 0; i < n_sss; i++) {
>> +			char *name = devm_kasprintf(dev, GFP_KERNEL, "sss%d", i);
>> +
>> +			sss[i].reg_size = 1;
>> +			sss[i].reg_addr = i;
>> +			sss[i].attr.attr.name = name;
>> +			sss[i].attr.attr.mode = 0600;
>> +			sss[i].attr.show = m24lr_ctl_show;
>> +			sss[i].attr.store = m24lr_ctl_store;
>> +
>> +			err = device_create_file(dev, &sss[i].attr);
>
> You just raced with userspace and lost. This is not how to do this,
> please do not dynamically create attributes (hint, this should have
> errored out as you didn't correctly initialize them), but also:

I didn't fully understand where the race condition comes from. Is
the issue caused by calling device_create_file() from within the
probe() function, or is it due to the fact that the attributes
are being allocated dynamically rather than defined statically?

>
>> +			if (err)
>> +				dev_warn(dev,
>> +					 "Failed to create sysfs entry '%s'\n",
>> +					 name);
>
> You do not unwind properly if an error happens.
>
> Just use a default attribute group attached to the driver and the driver
> core will handle all of that logic for you automatically. Making the
> code smaller and even better yet, correct :)

Thanks for clarifying this point. I'll rework the implementation
to use a default attribute_group

Best regards,
Abd-Alrhman Masalkhi

Re: [PATCH v3 2/3] misc: add driver for ST M24LR series RFID/NFC EEPROM chips

[Greg KH]

On Fri, Jun 06, 2025 at 02:24:56PM +0000, Abd-Alrhman Masalkhi wrote:
> >> +	if (unlikely(!is_power_of_2(reg_size) || reg_size > 8)) {
> >> +		dev_dbg(dev,
> >> +			"Invalid register size: must be a power of 2 and <= 8 bytes (%u)\n",
> >> +			reg_size);
> >> +		return -EIO;
> >
> > Not -EINVAL?  This isn't an I/O error.
> 
> The last if statement is primarily for debugging purposes. The reg_size
> value is specified internally by the driver (not user-controlled), so
> this check helps catch potential mistakes in the driver's sysfs entry
> definitions. That's why I used -EIO instead of -EINVAL, as it's not due
> to invalid user input but rather an internal misconfiguration.

But you just leaked your internal check to userspace in a potential
error code, so you must justify why userspace would ever see it and what
it should do with it :)

Just make it EINVAL please.  And if this is something that can't
actually happen, don't check it at all.

> >> +		for (i = 0; i < n_sss; i++) {
> >> +			char *name = devm_kasprintf(dev, GFP_KERNEL, "sss%d", i);
> >> +
> >> +			sss[i].reg_size = 1;
> >> +			sss[i].reg_addr = i;
> >> +			sss[i].attr.attr.name = name;
> >> +			sss[i].attr.attr.mode = 0600;
> >> +			sss[i].attr.show = m24lr_ctl_show;
> >> +			sss[i].attr.store = m24lr_ctl_store;
> >> +
> >> +			err = device_create_file(dev, &sss[i].attr);
> >
> > You just raced with userspace and lost. This is not how to do this,
> > please do not dynamically create attributes (hint, this should have
> > errored out as you didn't correctly initialize them), but also:
> 
> I didn't fully understand where the race condition comes from. Is
> the issue caused by calling device_create_file() from within the
> probe() function, or is it due to the fact that the attributes
> are being allocated dynamically rather than defined statically?

From calling device_create_file() at any point in time.  The driver core
should be doing that, not individual drivers.

thanks,

greg k-h

Re: [PATCH v3 2/3] misc: add driver for ST M24LR series RFID/NFC EEPROM chips

[kernel test robot]

Hi Abd-Alrhman,

kernel test robot noticed the following build warnings:

[auto build test WARNING on char-misc/char-misc-linus]
[also build test WARNING on soc/for-next robh/for-next driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus v6.15]
[cannot apply to char-misc/char-misc-testing char-misc/char-misc-next linus/master next-20250606]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abd-Alrhman-Masalkhi/dt-bindings-eeprom-Add-ST-M24LR-support/20250606-201000
base:   char-misc/char-misc-linus
patch link:    https://lore.kernel.org/r/20250606120631.3140054-3-abd.masalkhi%40gmail.com
patch subject: [PATCH v3 2/3] misc: add driver for ST M24LR series RFID/NFC EEPROM chips
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250607/202506071933.QIYSWX1a-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250607/202506071933.QIYSWX1a-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506071933.QIYSWX1a-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/misc/m24lr.c:341: warning: Function parameter or struct member 'is_eeprom' not described in 'm24lr_write'


vim +341 drivers/misc/m24lr.c

   322	
   323	/**
   324	 * m24lr_write - write buffer to M24LR device with page alignment handling
   325	 * @m24lr:  pointer to driver context
   326	 * @buf:    data buffer to write
   327	 * @size:   number of bytes to write
   328	 * @offset: target register address in the device
   329	 *
   330	 * Writes data to the M24LR device using regmap, split into chunks no larger
   331	 * than page_size to respect device-specific write limitations (e.g., page
   332	 * size or I2C hold-time concerns). Each chunk is aligned to the page boundary
   333	 * defined by page_size.
   334	 *
   335	 * Returns:
   336	 *	 Total number of bytes written on success,
   337	 *	 A negative error code if any write fails.
   338	 */
   339	static ssize_t m24lr_write(struct m24lr *m24lr, const u8 *buf, size_t size,
   340				   unsigned int offset, bool is_eeprom)
 > 341	{
   342		unsigned int n, next_sector;
   343		struct regmap *regmap;
   344		ssize_t ret = 0;
   345		long err;
   346	
   347		if (is_eeprom)
   348			regmap = m24lr->eeprom_regmap;
   349		else
   350			regmap = m24lr->ctl_regmap;
   351	
   352		n = min(size, m24lr->page_size);
   353		next_sector = roundup(offset + 1, m24lr->page_size);
   354		if (offset + n > next_sector)
   355			n = next_sector - offset;
   356	
   357		mutex_lock(&m24lr->lock);
   358		while (n) {
   359			err = m24lr_regmap_write(regmap, buf, n, offset);
   360			if (IS_ERR_VALUE(err)) {
   361				mutex_unlock(&m24lr->lock);
   362				if (ret)
   363					return ret;
   364				else
   365					return err;
   366			}
   367	
   368			offset += n;
   369			size -= n;
   370			ret += n;
   371			n = min(size, m24lr->page_size);
   372		}
   373		mutex_unlock(&m24lr->lock);
   374	
   375		return ret;
   376	}
   377	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 3/3] ABI: sysfs: add documentation for ST M24LR EEPROM and control interface

[Abd-Alrhman Masalkhi]

Add sysfs ABI documentation for the STMicroelectronics M24LR device,
covering both the control interface (e.g., unlock, password update, UID,
memory size, and SSS entries) and EEPROM access via the nvmem subsystem.

Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v3:
 - Updated sysfs entry paths to use <busnum>-<primary-addr> to reflect the
   control address.

Changes in v2:
 - Added initial sysfs ABI documentation.
---
 .../ABI/testing/sysfs-bus-i2c-devices-m24lr   | 96 +++++++++++++++++++
 1 file changed, 96 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-m24lr

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-m24lr b/Documentation/ABI/testing/sysfs-bus-i2c-devices-m24lr
new file mode 100644
index 000000000000..53b6fe39162c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-m24lr
@@ -0,0 +1,96 @@
+What:           /sys/bus/i2c/devices/<busnum>-<primary-addr>/unlock
+Date:           2025-05-31
+KernelVersion:  6.16
+Contact:        Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
+Description:
+                Write-only attribute used to present a password and unlock
+                access to protected areas of the M24LR chip, including
+                configuration registers such as the Sector Security Status
+                (SSS) bytes. A valid password must be written to enable write
+                access to these regions via the I2C interface.
+
+                Format:
+                  - Hexadecimal string representing a 32-bit (4-byte) password
+                  - Accepts 1 to 8 hex digits (e.g., "c", "1F", "a1b2c3d4")
+                  - No "0x" prefix, whitespace, or trailing newline
+                  - Case-insensitive
+
+                Behavior:
+                  - If the password matches the internal stored value,
+                    access to protected memory/configuration is granted
+                  - If the password does not match the internally stored value,
+                    it will fail silently
+
+What:           /sys/bus/i2c/devices/<busnum>-<primary-addr>/new_pass
+Date:           2025-05-31
+KernelVersion:  6.16
+Contact:        Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
+Description:
+                Write-only attribute used to update the password required to
+                unlock the M24LR chip.
+
+                Format:
+                  - Hexadecimal string representing a new 32-bit password
+                  - Accepts 1 to 8 hex digits (e.g., "1A", "ffff", "c0ffee00")
+                  - No "0x" prefix, whitespace, or trailing newline
+                  - Case-insensitive
+
+                Behavior:
+                  - Overwrites the current password stored in the I2C password
+                    register
+                  - Requires the device to be unlocked before changing the
+                    password
+                  - If the device is locked, the write silently fails
+
+What:           /sys/bus/i2c/devices/<busnum>-<primary-addr>/uid
+Date:           2025-05-31
+KernelVersion:  6.16
+Contact:        Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
+Description:
+                Read-only attribute that exposes the 8-byte unique identifier
+                programmed into the M24LR chip at the factory.
+
+                Format:
+                  - Lowercase hexadecimal string representing a 64-bit value
+                  - 1 to 16 hex digits (e.g., "e00204f12345678")
+                  - No "0x" prefix
+                  - Includes a trailing newline
+
+What:           /sys/bus/i2c/devices/<busnum>-<primary-addr>/mem_size
+Date:           2025-05-31
+KernelVersion:  6.16
+Contact:        Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
+Description:
+                Read-only attribute that exposes the internal memory size code
+                of the M24LR device, as stored in the system register area.
+
+                Format:
+                  - Unsigned 8-bit integer
+                  - Includes a trailing newline
+
+                Notes:
+                  - Value is encoded by the chip and corresponds to the EEPROM
+                    size (e.g., 3 = 4 kbit for M24LR04E-R)
+
+What:           /sys/bus/i2c/devices/<busnum>-<primary-addr>/sss<N>
+Date:           2025-05-31
+KernelVersion:  6.16
+Contact:        Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
+Description:
+                Read/write attribute representing the Sector Security Status
+                (SSS) byte for EEPROM sector <N> in the M24LR chips. Each sector
+                has one SSS byte, which defines I2c and RF access control via a
+                combination of protection and password settings.
+
+                Format:
+                  - Read: returns a 8-bit hexadecimal value followed by a
+                          newline
+                  - Write: requires exactly one or two hexadecimal digits
+                      - No "0x" prefix, whitespace, or trailing newline
+                      - Case-insensitive
+
+                Notes:
+                  - Refer to the M24LR chip datasheet for full bit definitions
+                    and usage
+                  - Write access requires prior password authentication in I2C
+                    mode
-- 
2.43.0

Re: [PATCH v3 3/3] ABI: sysfs: add documentation for ST M24LR EEPROM and control interface

[Greg KH]

On Fri, Jun 06, 2025 at 12:06:31PM +0000, Abd-Alrhman Masalkhi wrote:
> Add sysfs ABI documentation for the STMicroelectronics M24LR device,
> covering both the control interface (e.g., unlock, password update, UID,
> memory size, and SSS entries) and EEPROM access via the nvmem subsystem.
> 
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> Changes in v3:
>  - Updated sysfs entry paths to use <busnum>-<primary-addr> to reflect the
>    control address.
> 
> Changes in v2:
>  - Added initial sysfs ABI documentation.
> ---
>  .../ABI/testing/sysfs-bus-i2c-devices-m24lr   | 96 +++++++++++++++++++
>  1 file changed, 96 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-m24lr
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-m24lr b/Documentation/ABI/testing/sysfs-bus-i2c-devices-m24lr
> new file mode 100644
> index 000000000000..53b6fe39162c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-m24lr
> @@ -0,0 +1,96 @@
> +What:           /sys/bus/i2c/devices/<busnum>-<primary-addr>/unlock
> +Date:           2025-05-31

It's later than this.

> +KernelVersion:  6.16

This will not be showing up in 6.16, sorry, it's too late for that.

> +Contact:        Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> +Description:
> +                Write-only attribute used to present a password and unlock
> +                access to protected areas of the M24LR chip, including
> +                configuration registers such as the Sector Security Status
> +                (SSS) bytes. A valid password must be written to enable write
> +                access to these regions via the I2C interface.
> +
> +                Format:
> +                  - Hexadecimal string representing a 32-bit (4-byte) password
> +                  - Accepts 1 to 8 hex digits (e.g., "c", "1F", "a1b2c3d4")
> +                  - No "0x" prefix, whitespace, or trailing newline
> +                  - Case-insensitive
> +
> +                Behavior:
> +                  - If the password matches the internal stored value,
> +                    access to protected memory/configuration is granted
> +                  - If the password does not match the internally stored value,
> +                    it will fail silently

Why is the kernel in the business of adding passwords to devices?  That
feels wrong, and a way to just flood the device with a "try all the
values" attempt if needed.

> +What:           /sys/bus/i2c/devices/<busnum>-<primary-addr>/sss<N>
> +Date:           2025-05-31
> +KernelVersion:  6.16
> +Contact:        Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> +Description:
> +                Read/write attribute representing the Sector Security Status
> +                (SSS) byte for EEPROM sector <N> in the M24LR chips. Each sector
> +                has one SSS byte, which defines I2c and RF access control via a
> +                combination of protection and password settings.
> +
> +                Format:
> +                  - Read: returns a 8-bit hexadecimal value followed by a
> +                          newline
> +                  - Write: requires exactly one or two hexadecimal digits
> +                      - No "0x" prefix, whitespace, or trailing newline
> +                      - Case-insensitive
> +
> +                Notes:
> +                  - Refer to the M24LR chip datasheet for full bit definitions
> +                    and usage
> +                  - Write access requires prior password authentication in I2C
> +                    mode

How "deep" does this sysfs tree get here?  This feels like the wrong api
for read/write to the device, just do it with a single binary file if
you really want a "passthrough" way to get to the hardware.

thanks,

greg k-h

Re: [PATCH v3 3/3] ABI: sysfs: add documentation for ST M24LR EEPROM and control interface

[Abd-Alrhman Masalkhi]

Hi greg,

Thanks for the feedback.

>> +                Behavior:
>> +                  - If the password matches the internal stored value,
>> +                    access to protected memory/configuration is granted
>> +                  - If the password does not match the internally stored value,
>> +                    it will fail silently
>
> Why is the kernel in the business of adding passwords to devices?  That
> feels wrong, and a way to just flood the device with a "try all the
> values" attempt if needed.

You're absolutely right, implementing password-based access in kernel
space isn't ideal. However, this behavior is defined by the hardware
itself. The M24LR chips require the user to "unlock" the device by writing
a password before certain registers become writable (such as the Sector
Security Status registors) and unfortunately, the chip does not provide
any status or feedback to indicate whether the unlock was successful,
which limits what the driver can safely report or validate.

>> +What:           /sys/bus/i2c/devices/<busnum>-<primary-addr>/sss<N>
>> +Date:           2025-05-31
>> +KernelVersion:  6.16
>> +Contact:        Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> +Description:
>> +                Read/write attribute representing the Sector Security Status
>> +                (SSS) byte for EEPROM sector <N> in the M24LR chips. Each sector
>> +                has one SSS byte, which defines I2c and RF access control via a
>> +                combination of protection and password settings.
>> +                Format:
>> +                  - Read: returns a 8-bit hexadecimal value followed by a
>> +                          newline
>> +                  - Write: requires exactly one or two hexadecimal digits
>> +                      - No "0x" prefix, whitespace, or trailing newline
>> +                      - Case-insensitive
>> +
>> +                Notes:
>> +                  - Refer to the M24LR chip datasheet for full bit definitions
>> +                    and usage
>> +                  - Write access requires prior password authentication in I2C
>> +                    mode
>
> How "deep" does this sysfs tree get here?  This feels like the wrong api
> for read/write to the device, just do it with a single binary file if
> you really want a "passthrough" way to get to the hardware.

The depth of the sysfs tree depends on the M24LR variant. For example,
the M24LR04E-R has 4 sectors, resulting in 4 entries: sss0 through sss3.

I understand the concern about exposing multiple sysfs entries. The
reason for this design is that each sector has its own SSS byte, and
separating them helps reflect the per-sector nature of the access
control. That said, I'm open to refactoring this to expose the SSS
area via a single binary file if that's more in line with expected
kernel interfaces.

Best regards,
Abd-Alrhman Masalkhi

Re: [PATCH v3 3/3] ABI: sysfs: add documentation for ST M24LR EEPROM and control interface

[Greg KH]

On Fri, Jun 06, 2025 at 02:46:57PM +0000, Abd-Alrhman Masalkhi wrote:
> Hi greg,
> 
> Thanks for the feedback.
> 
> >> +                Behavior:
> >> +                  - If the password matches the internal stored value,
> >> +                    access to protected memory/configuration is granted
> >> +                  - If the password does not match the internally stored value,
> >> +                    it will fail silently
> >
> > Why is the kernel in the business of adding passwords to devices?  That
> > feels wrong, and a way to just flood the device with a "try all the
> > values" attempt if needed.
> 
> You're absolutely right, implementing password-based access in kernel
> space isn't ideal. However, this behavior is defined by the hardware
> itself. The M24LR chips require the user to "unlock" the device by writing
> a password before certain registers become writable (such as the Sector
> Security Status registors) and unfortunately, the chip does not provide
> any status or feedback to indicate whether the unlock was successful,
> which limits what the driver can safely report or validate.
> 
> >> +What:           /sys/bus/i2c/devices/<busnum>-<primary-addr>/sss<N>
> >> +Date:           2025-05-31
> >> +KernelVersion:  6.16
> >> +Contact:        Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> >> +Description:
> >> +                Read/write attribute representing the Sector Security Status
> >> +                (SSS) byte for EEPROM sector <N> in the M24LR chips. Each sector
> >> +                has one SSS byte, which defines I2c and RF access control via a
> >> +                combination of protection and password settings.
> >> +                Format:
> >> +                  - Read: returns a 8-bit hexadecimal value followed by a
> >> +                          newline
> >> +                  - Write: requires exactly one or two hexadecimal digits
> >> +                      - No "0x" prefix, whitespace, or trailing newline
> >> +                      - Case-insensitive
> >> +
> >> +                Notes:
> >> +                  - Refer to the M24LR chip datasheet for full bit definitions
> >> +                    and usage
> >> +                  - Write access requires prior password authentication in I2C
> >> +                    mode
> >
> > How "deep" does this sysfs tree get here?  This feels like the wrong api
> > for read/write to the device, just do it with a single binary file if
> > you really want a "passthrough" way to get to the hardware.
> 
> The depth of the sysfs tree depends on the M24LR variant. For example,
> the M24LR04E-R has 4 sectors, resulting in 4 entries: sss0 through sss3.
> 
> I understand the concern about exposing multiple sysfs entries. The
> reason for this design is that each sector has its own SSS byte, and
> separating them helps reflect the per-sector nature of the access
> control. That said, I'm open to refactoring this to expose the SSS
> area via a single binary file if that's more in line with expected
> kernel interfaces.

Who and what is going to be talking to this device through this
interface?  Is this unique and special to ONLY this one chip/device or
does it fit in with all other types of this device (i.e. eeproms)?  You
can't create a userspace api without actually having a user at all, so
if there is no userspace code using this, why even have this?

thanks,

greg k-h

Re: [PATCH v3 1/3] dt-bindings: eeprom: Add ST M24LR support

[Abd-Alrhman Masalkhi]

Hi Greg,

> > >> +What:           /sys/bus/i2c/devices/<busnum>-<primary-addr>/sss<N>
> > >> +Date:           2025-05-31
> > >> +KernelVersion:  6.16
> > >> +Contact:        Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> > >> +Description:
> > >> +                Read/write attribute representing the Sector Security Status
> > >> +                (SSS) byte for EEPROM sector <N> in the M24LR chips. Each sector
> > >> +                has one SSS byte, which defines I2c and RF access control via a
> > >> +                combination of protection and password settings.
> > >> +                Format:
> > >> +                  - Read: returns a 8-bit hexadecimal value followed by a
> > >> +                          newline
> > >> +                  - Write: requires exactly one or two hexadecimal digits
> > >> +                      - No "0x" prefix, whitespace, or trailing newline
> > >> +                      - Case-insensitive
> > >> +
> > >> +                Notes:
> > >> +                  - Refer to the M24LR chip datasheet for full bit definitions
> > >> +                    and usage
> > >> +                  - Write access requires prior password authentication in I2C
> > >> +                    mode
> > >
> > > How "deep" does this sysfs tree get here?  This feels like the wrong api
> > > for read/write to the device, just do it with a single binary file if
> > > you really want a "passthrough" way to get to the hardware.
> > 
> > The depth of the sysfs tree depends on the M24LR variant. For example,
> > the M24LR04E-R has 4 sectors, resulting in 4 entries: sss0 through sss3.
> > 
> > I understand the concern about exposing multiple sysfs entries. The
> > reason for this design is that each sector has its own SSS byte, and
> > separating them helps reflect the per-sector nature of the access
> > control. That said, I'm open to refactoring this to expose the SSS
> > area via a single binary file if that's more in line with expected
> > kernel interfaces.
> 
> Who and what is going to be talking to this device through this
> interface?  Is this unique and special to ONLY this one chip/device or
> does it fit in with all other types of this device (i.e. eeproms)?  You
> can't create a userspace api without actually having a user at all, so
> if there is no userspace code using this, why even have this?

A userspace application specific to the M24LR series is intended to
interface with this driver. The M24LR devices support dual access
to the EEPROM: via I2C and over RFID. The purpose of exposing the
Sector Security Status (SSS) registers to userspace is to provide
dynamic control over when and how the EEPROM is accessible to an
RFID reader. This allows userspace to decide whether to permit or
deny EEPROM reads/writes via RFID, or to configure protection for
specific memory sectors.

The SSS registers define per-sector read/write permissions and
password protection, directly determining how external RFID readers
interact with the tag. By exposing this configuration through sysfs,
userspace software can modify RFID access behavior at runtime for
example, to enable secure provisioning workflows, implement time-based
access, or prevent unauthorized access after setup.

Given that this is a per-sector control mechanism unique to the M24LR
series, would exposing the entire SSS region via a single binary sysfs
file be considered more appropriate than individual attributes per sector?

Best regards,
Abd-Alrhman Masalkhi

Re: [PATCH v3 3/3] ABI: sysfs: add documentation for ST M24LR EEPROM and control interface

[Abd-Alrhman Masalkhi]

Hi Greg,

> > >> +What:           /sys/bus/i2c/devices/<busnum>-<primary-addr>/sss<N>
> > >> +Date:           2025-05-31
> > >> +KernelVersion:  6.16
> > >> +Contact:        Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> > >> +Description:
> > >> +                Read/write attribute representing the Sector Security Status
> > >> +                (SSS) byte for EEPROM sector <N> in the M24LR chips. Each sector
> > >> +                has one SSS byte, which defines I2c and RF access control via a
> > >> +                combination of protection and password settings.
> > >> +                Format:
> > >> +                  - Read: returns a 8-bit hexadecimal value followed by a
> > >> +                          newline
> > >> +                  - Write: requires exactly one or two hexadecimal digits
> > >> +                      - No "0x" prefix, whitespace, or trailing newline
> > >> +                      - Case-insensitive
> > >> +
> > >> +                Notes:
> > >> +                  - Refer to the M24LR chip datasheet for full bit definitions
> > >> +                    and usage
> > >> +                  - Write access requires prior password authentication in I2C
> > >> +                    mode
> > >
> > > How "deep" does this sysfs tree get here?  This feels like the wrong api
> > > for read/write to the device, just do it with a single binary file if
> > > you really want a "passthrough" way to get to the hardware.
> > 
> > The depth of the sysfs tree depends on the M24LR variant. For example,
> > the M24LR04E-R has 4 sectors, resulting in 4 entries: sss0 through sss3.
> > 
> > I understand the concern about exposing multiple sysfs entries. The
> > reason for this design is that each sector has its own SSS byte, and
> > separating them helps reflect the per-sector nature of the access
> > control. That said, I'm open to refactoring this to expose the SSS
> > area via a single binary file if that's more in line with expected
> > kernel interfaces.
> 
> Who and what is going to be talking to this device through this
> interface?  Is this unique and special to ONLY this one chip/device or
> does it fit in with all other types of this device (i.e. eeproms)?  You
> can't create a userspace api without actually having a user at all, so
> if there is no userspace code using this, why even have this?

A userspace application specific to the M24LR series is intended to
interface with this driver. The M24LR devices support dual access
to the EEPROM: via I2C and over RFID. The purpose of exposing the
Sector Security Status (SSS) registers to userspace is to provide
dynamic control over when and how the EEPROM is accessible to an
RFID reader. This allows userspace to decide whether to permit or
deny EEPROM reads/writes via RFID, or to configure protection for
specific memory sectors.

The SSS registers define per-sector read/write permissions and
password protection, directly determining how external RFID readers
interact with the tag. By exposing this configuration through sysfs,
userspace software can modify RFID access behavior at runtime for
example, to enable secure provisioning workflows, implement time-based
access, or prevent unauthorized access after setup.

Given that this is a per-sector control mechanism unique to the M24LR
series, would exposing the entire SSS region via a single binary sysfs
file be considered more appropriate than individual attributes per sector?

Best regards,
Abd-Alrhman Masalkhi