linux-kernel - Re: [PATCH 1/3] dt-bindings: misc: Add binding for ST M24LR control interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <852ef4fd-6c26-4f79-a1d4-b3e37926ed7a@kernel.org>
Date: Sat, 31 May 2025 15:37:21 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Abd-Alrhman Masalkhi <abd.masalkhi@...il.com>,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, arnd@...db.de,
 gregkh@...uxfoundation.org
Subject: Re: [PATCH 1/3] dt-bindings: misc: Add binding for ST M24LR control
 interface

On 31/05/2025 10:11, Abd-Alrhman Masalkhi wrote:
> Add a Device Tree binding for the STMicroelectronics M24LR series
> RFID/NFC EEPROM chips (e.g., M24LR04E-R), which support a separate
> I2C interface for control and configuration.
> 
> This binding documents the control interface that is managed by
> a dedicated driver exposing sysfs attributes. The EEPROM memory
> interface is handled by the standard 'at24' driver and is
> represented as a child node in the Device Tree.
> 
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@...il.com>
> ---
>  .../devicetree/bindings/misc/st,m24lr.yaml    | 70 +++++++++++++++++++

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

A nit, subject: drop second/last, redundant "binding for". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


>  1 file changed, 70 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..5a8f5aef13ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/st,m24lr.yaml
> @@ -0,0 +1,70 @@
> +# 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 Series NFC/RFID EEPROM Control Interface
> +
> +maintainers:
> +  - name: Abd-Alrhman Masalkhi
> +    email: abd.masalkhi@...il.com
> +
> +description: |
> +  This binding describes the control interface for STMicroelectronics

Describe the hardware, not the binding.

> +  M24LR series RFID/NFC EEPROM chips (e.g., M24LR04E-R, M24LR16E-R).
> +  This driver provides sysfs access to device-specific control registers
> +  (authentication, UID, etc.) over the I2C interface. It act as a

Describe hardware, not drivers.

> +  I2C gate for the EEPROM. Therefore, The EEPROM is represented as a
> +  child node under a port and is accessed through a separate driver
> +  (the standard 'at24' driver). This implementation is possible because

again, describe hardware not driver

> +  the M24LR chips uses two I2C addresses: one for accessing the
> +  system parameter sector and another for the EEPROM.

This suggests you have two I2C addresses for one device, not two devices
with parent child relationship.

> +
> +allOf:
> +  - $ref: "i2c-mux.yaml#"

Drop quotes. So this is I2C mux or EEPROM?

> +
> +properties:
> +  compatible:
> +    enum:
> +    - st,m24lr04e-r
> +    - st,m24lr16e-r
> +    - st,m24lr64e-r
> +
> +  reg:
> +    maxItems: 1
> +    description: I2C address of the device.

Drop description, redundant.


This device is not compatible with AT24?

> +
> +  pagesize:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: >
> +      Maximum number of bytes that can be written in a single I2C
> +      transaction. the default is 1.

enum:
default:

and drop redundant free form text.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false

unevaluatedProperties instead

> +
> +examples:
> +  - |
> +    i2c {
> +      m24lr@57 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +        compatible = "st,m24lr04e-r";
> +        reg = <0x57>;
> +
> +        i2c-gate {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          m24lr_eeprom@53 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +            compatible = "atmel,24c04";
> +            reg = <0x53>;
> +            address-width = <16>;
> +            pagesize = <4>;
> +          };
> +        };
> +      };
> +    };
> +...
> \ No newline at end of file


Best regards,
Krzysztof