[PATCH v2 0/2] nvmem: lan9662-otpc: add support

[PATCH v2 0/2] nvmem: lan9662-otpc: add support

[Horatiu Vultur]

Add support for lan9662 OTP controller that is available on lan9662.
The driver gives access to non-volatile memory. It allows both write
and read access to it.

v1->V2:
- remove unneed quotes for $ref: nvmem.yaml#
- rename lan966x to lan9662 as not to have any wildcards
- remove compatible string syscon

Horatiu Vultur (2):
  dt-bindings: lan9662-otpc: document Lan9662 OTPC
  nvmem: lan9662-otp: add support.

 .../nvmem/microchip,lan9662-otpc.yaml         |  42 +++
 drivers/nvmem/Kconfig                         |   8 +
 drivers/nvmem/Makefile                        |   2 +
 drivers/nvmem/lan9662-otpc.c                  | 249 ++++++++++++++++++
 4 files changed, 301 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml
 create mode 100644 drivers/nvmem/lan9662-otpc.c

-- 
2.33.0

[PATCH v2 1/2] dt-bindings: lan9662-otpc: document Lan9662 OTPC

[Horatiu Vultur]

Document Lan9662 OTP controller.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../nvmem/microchip,lan9662-otpc.yaml         | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml b/Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml
new file mode 100644
index 000000000000..3307f6a7a373
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/microchip,lan9662-otpc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip LAN9662 OTP Controller (OTPC)
+
+maintainers:
+  - Horatiu Vultur <horatiu.vultur@microchip.com>
+
+description: |
+  OTP controller drives a NVMEM memory where system specific data
+  (e.g. hardware configuration settings, chip identifiers) or
+  user specific data could be stored.
+
+allOf:
+  - $ref: nvmem.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: microchip,lan9662-otpc
+      - const: microchip,lan9668-otpc
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    otpc: otp@e0021000 {
+        compatible = "microchip,lan9662-otpc";
+        reg = <0xe0021000 0x300>;
+    };
+
+...
-- 
2.33.0

[PATCH v2 2/2] nvmem: lan9662-otp: add support.

[Horatiu Vultur]

Add support for OTP controller available on LAN9662. The OTPC controls
the access to a non-volatile memory. The size of the memory is 8KB.
The OTPC can access the memory based on an offset.
Implement both the read and the write functionality.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/nvmem/Kconfig        |   8 ++
 drivers/nvmem/Makefile       |   2 +
 drivers/nvmem/lan9662-otpc.c | 249 +++++++++++++++++++++++++++++++++++
 3 files changed, 259 insertions(+)
 create mode 100644 drivers/nvmem/lan9662-otpc.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 967d0084800e..c9929ec35a39 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -84,6 +84,14 @@ config NVMEM_LPC18XX_OTP
 	  To compile this driver as a module, choose M here: the module
 	  will be called nvmem_lpc18xx_otp.
 
+config NVMEM_LAN9662_OTPC
+	tristate "Microchip LAN9662 OTP controller support"
+	depends on SOC_LAN966 || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  This driver enables the OTP controller available on Microchip LAN9662
+	  SoCs. It controlls the access to the OTP memory connected to it.
+
 config NVMEM_MXS_OCOTP
 	tristate "Freescale MXS On-Chip OTP Memory Support"
 	depends on ARCH_MXS || COMPILE_TEST
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 00e136a0a123..e1baface2c53 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -21,6 +21,8 @@ obj-$(CONFIG_NVMEM_LPC18XX_EEPROM)	+= nvmem_lpc18xx_eeprom.o
 nvmem_lpc18xx_eeprom-y	:= lpc18xx_eeprom.o
 obj-$(CONFIG_NVMEM_LPC18XX_OTP)	+= nvmem_lpc18xx_otp.o
 nvmem_lpc18xx_otp-y		:= lpc18xx_otp.o
+obj-$(CONFIG_NVMEM_LAN9662_OTPC)	+= nvmem-lan9662-otpc.o
+nvmem-lan9662-otpc-y		:= lan9662-otpc.o
 obj-$(CONFIG_NVMEM_MXS_OCOTP)	+= nvmem-mxs-ocotp.o
 nvmem-mxs-ocotp-y		:= mxs-ocotp.o
 obj-$(CONFIG_NVMEM_NINTENDO_OTP)	+= nvmem-nintendo-otp.o
diff --git a/drivers/nvmem/lan9662-otpc.c b/drivers/nvmem/lan9662-otpc.c
new file mode 100644
index 000000000000..302a5bae04dc
--- /dev/null
+++ b/drivers/nvmem/lan9662-otpc.c
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#define OTP_OTP_PWR_DN(t)			(t + 0x00)
+#define OTP_OTP_PWR_DN_OTP_PWRDN_N		BIT(0)
+#define OTP_OTP_ADDR_HI(t)			(t + 0x04)
+#define OTP_OTP_ADDR_LO(t)			(t + 0x08)
+#define OTP_OTP_PRGM_DATA(t)			(t + 0x10)
+#define OTP_OTP_PRGM_MODE(t)			(t + 0x14)
+#define OTP_OTP_PRGM_MODE_OTP_PGM_MODE_BYTE	BIT(0)
+#define OTP_OTP_RD_DATA(t)			(t + 0x18)
+#define OTP_OTP_FUNC_CMD(t)			(t + 0x20)
+#define OTP_OTP_FUNC_CMD_OTP_PROGRAM		BIT(1)
+#define OTP_OTP_FUNC_CMD_OTP_READ		BIT(0)
+#define OTP_OTP_CMD_GO(t)			(t + 0x28)
+#define OTP_OTP_CMD_GO_OTP_GO			BIT(0)
+#define OTP_OTP_PASS_FAIL(t)			(t + 0x2c)
+#define OTP_OTP_PASS_FAIL_OTP_READ_PROHIBITED	BIT(3)
+#define OTP_OTP_PASS_FAIL_OTP_WRITE_PROHIBITED	BIT(2)
+#define OTP_OTP_PASS_FAIL_OTP_FAIL		BIT(0)
+#define OTP_OTP_STATUS(t)			(t + 0x30)
+#define OTP_OTP_STATUS_OTP_CPUMPEN		BIT(1)
+#define OTP_OTP_STATUS_OTP_BUSY			BIT(0)
+
+#define OTP_MEM_SIZE 8192
+#define OTP_SLEEP_US 10
+#define OTP_TIMEOUT_US 500000
+
+struct lan9662_otp {
+	struct device *dev;
+	void __iomem *base;
+};
+
+static inline void lan9662_writel(void __iomem *addr, u32 val)
+{
+	writel(val, addr);
+}
+
+static inline u32 lan9662_readl(void __iomem *addr)
+{
+	return readl(addr);
+}
+
+static inline void lan9662_clrbits(void __iomem *addr, u32 clear)
+{
+	writel(readl(addr) & ~clear, addr);
+}
+
+static inline void lan9662_setbits(void __iomem *addr, u32 set)
+{
+	writel(readl(addr) | set, addr);
+}
+
+static bool lan9662_otp_wait_flag_clear(void __iomem *reg, u32 flag)
+{
+	u32 val;
+
+	return readl_poll_timeout(reg, val, !(val & flag),
+				  OTP_SLEEP_US, OTP_TIMEOUT_US);
+}
+
+static int lan9662_otp_power(struct lan9662_otp *otp, bool up)
+{
+	if (up) {
+		lan9662_clrbits(OTP_OTP_PWR_DN(otp->base),
+				OTP_OTP_PWR_DN_OTP_PWRDN_N);
+		if (lan9662_otp_wait_flag_clear(OTP_OTP_STATUS(otp->base),
+						OTP_OTP_STATUS_OTP_CPUMPEN))
+			return -ETIMEDOUT;
+	} else {
+		lan9662_setbits(OTP_OTP_PWR_DN(otp->base),
+				OTP_OTP_PWR_DN_OTP_PWRDN_N);
+	}
+
+	return 0;
+}
+
+static int lan9662_otp_execute(struct lan9662_otp *otp)
+{
+	if (lan9662_otp_wait_flag_clear(OTP_OTP_CMD_GO(otp->base),
+					OTP_OTP_CMD_GO_OTP_GO))
+		return -ETIMEDOUT;
+
+	if (lan9662_otp_wait_flag_clear(OTP_OTP_STATUS(otp->base),
+					OTP_OTP_STATUS_OTP_BUSY))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static void lan9662_otp_set_address(struct lan9662_otp *otp, u32 offset)
+{
+	WARN_ON(offset >= OTP_MEM_SIZE);
+
+	lan9662_writel(OTP_OTP_ADDR_HI(otp->base), 0xff & (offset >> 8));
+	lan9662_writel(OTP_OTP_ADDR_LO(otp->base), 0xff & offset);
+}
+
+static int lan9662_otp_read_byte(struct lan9662_otp *otp, u32 offset, u8 *dst)
+{
+	u32 pass;
+	int rc;
+
+	lan9662_otp_set_address(otp, offset);
+	lan9662_writel(OTP_OTP_FUNC_CMD(otp->base),
+		       OTP_OTP_FUNC_CMD_OTP_READ);
+	lan9662_writel(OTP_OTP_CMD_GO(otp->base),
+		       OTP_OTP_CMD_GO_OTP_GO);
+	rc = lan9662_otp_execute(otp);
+	if (!rc) {
+		pass = lan9662_readl(OTP_OTP_PASS_FAIL(otp->base));
+		if (pass & OTP_OTP_PASS_FAIL_OTP_READ_PROHIBITED)
+			return -EACCES;
+		*dst = (u8) lan9662_readl(OTP_OTP_RD_DATA(otp->base));
+	}
+	return rc;
+}
+
+static int lan9662_otp_write_byte(struct lan9662_otp *otp, u32 offset, u8 data)
+{
+	u32 pass;
+	int rc;
+
+	lan9662_otp_set_address(otp, offset);
+	lan9662_writel(OTP_OTP_PRGM_MODE(otp->base),
+		       OTP_OTP_PRGM_MODE_OTP_PGM_MODE_BYTE);
+	lan9662_writel(OTP_OTP_PRGM_DATA(otp->base), data);
+	lan9662_writel(OTP_OTP_FUNC_CMD(otp->base),
+		       OTP_OTP_FUNC_CMD_OTP_PROGRAM);
+	lan9662_writel(OTP_OTP_CMD_GO(otp->base), OTP_OTP_CMD_GO_OTP_GO);
+
+	rc = lan9662_otp_execute(otp);
+	if (!rc) {
+		pass = lan9662_readl(OTP_OTP_PASS_FAIL(otp->base));
+		if (pass & OTP_OTP_PASS_FAIL_OTP_WRITE_PROHIBITED)
+			return -EACCES;
+		if (pass & OTP_OTP_PASS_FAIL_OTP_FAIL)
+			return -EIO;
+	}
+	return rc;
+}
+
+static int lan9662_otp_read(void *context, unsigned int offset,
+			    void *_val, size_t bytes)
+{
+	struct lan9662_otp *otp = context;
+	u8 *val = _val;
+	uint8_t data;
+	int i, rc = 0;
+
+	lan9662_otp_power(otp, true);
+	for (i = 0; i < bytes; i++) {
+		rc = lan9662_otp_read_byte(otp, offset + i, &data);
+		if (rc < 0)
+			break;
+		*val++ = data;
+	}
+	lan9662_otp_power(otp, false);
+
+	return rc;
+}
+
+static int lan9662_otp_write(void *context, unsigned int offset,
+			     void *_val, size_t bytes)
+{
+	struct lan9662_otp *otp = context;
+	u8 *val = _val;
+	u8 data, newdata;
+	int i, rc = 0;
+
+	lan9662_otp_power(otp, true);
+	for (i = 0; i < bytes; i++) {
+		/* Skip zero bytes */
+		if (val[i]) {
+			rc = lan9662_otp_read_byte(otp, offset + i, &data);
+			if (rc < 0)
+				break;
+
+			newdata = data | val[i];
+			if (newdata == data)
+				continue;
+
+			rc = lan9662_otp_write_byte(otp, offset + i,
+						      newdata);
+			if (rc < 0)
+				break;
+		}
+	}
+	lan9662_otp_power(otp, false);
+
+	return rc;
+}
+
+static struct nvmem_config otp_config = {
+	.name = "lan9662-otp",
+	.stride = 1,
+	.word_size = 1,
+	.reg_read = lan9662_otp_read,
+	.reg_write = lan9662_otp_write,
+	.size = OTP_MEM_SIZE,
+};
+
+static int lan9662_otp_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct nvmem_device *nvmem;
+	struct lan9662_otp *otp;
+
+	otp = devm_kzalloc(&pdev->dev, sizeof(*otp), GFP_KERNEL);
+	if (!otp)
+		return -ENOMEM;
+
+	otp->dev = dev;
+	otp->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(otp->base))
+		return PTR_ERR(otp->base);
+
+	otp_config.priv = otp;
+	otp_config.dev = dev;
+
+	nvmem = devm_nvmem_register(dev, &otp_config);
+
+	return PTR_ERR_OR_ZERO(nvmem);
+}
+
+static const struct of_device_id lan9662_otp_match[] = {
+	{ .compatible = "microchip,lan9662-otp", },
+	{ .compatible = "microchip,lan9668-otp", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, lan9662_otp_match);
+
+static struct platform_driver lan9662_otp_driver = {
+	.probe = lan9662_otp_probe,
+	.driver = {
+		.name = "lan9662-otp",
+		.of_match_table = lan9662_otp_match,
+	},
+};
+module_platform_driver(lan9662_otp_driver);
+
+MODULE_AUTHOR("Horatiu Vultur <horatiu.vultur@microchip.com>");
+MODULE_DESCRIPTION("lan9662 OTP driver");
+MODULE_LICENSE("GPL");
-- 
2.33.0

Re: [PATCH v2 1/2] dt-bindings: lan9662-otpc: document Lan9662 OTPC

[Krzysztof Kozlowski]

On 25/08/2022 23:40, Horatiu Vultur wrote:
> Document Lan9662 OTP controller.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../nvmem/microchip,lan9662-otpc.yaml         | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml b/Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml
> new file mode 100644
> index 000000000000..3307f6a7a373
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/microchip,lan9662-otpc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip LAN9662 OTP Controller (OTPC)
> +
> +maintainers:
> +  - Horatiu Vultur <horatiu.vultur@microchip.com>
> +
> +description: |
> +  OTP controller drives a NVMEM memory where system specific data
> +  (e.g. hardware configuration settings, chip identifiers) or
> +  user specific data could be stored.
> +
> +allOf:
> +  - $ref: nvmem.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: microchip,lan9662-otpc
> +      - const: microchip,lan9668-otpc

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

This won't work...

Best regards,
Krzysztof

Re: [PATCH v2 2/2] nvmem: lan9662-otp: add support.

[Krzysztof Kozlowski]

On 25/08/2022 23:40, Horatiu Vultur wrote:
> +static int lan9662_otp_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct nvmem_device *nvmem;
> +	struct lan9662_otp *otp;
> +
> +	otp = devm_kzalloc(&pdev->dev, sizeof(*otp), GFP_KERNEL);
> +	if (!otp)
> +		return -ENOMEM;
> +
> +	otp->dev = dev;
> +	otp->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(otp->base))
> +		return PTR_ERR(otp->base);
> +
> +	otp_config.priv = otp;
> +	otp_config.dev = dev;
> +
> +	nvmem = devm_nvmem_register(dev, &otp_config);
> +
> +	return PTR_ERR_OR_ZERO(nvmem);
> +}
> +
> +static const struct of_device_id lan9662_otp_match[] = {
> +	{ .compatible = "microchip,lan9662-otp", },
> +	{ .compatible = "microchip,lan9668-otp", },

Why do you need two compatibles here? Your bindings are saying only one
is needed...

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: lan9662-otpc: document Lan9662 OTPC

[Horatiu Vultur]

The 08/26/2022 09:42, Krzysztof Kozlowski wrote:

Hi Krzysztof,

> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: microchip,lan9662-otpc
> > +      - const: microchip,lan9668-otpc
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> 
> This won't work...

You are right. That was a silly mistake on my side.

It should be:
---
properties:
  compatible:
    enum:
      - microchip,lan9662-otpc
      - microchip,lan9668-otpc
---
Because what I want to achive is to be able to use any of
string(microchip,lan9662-otpc or microchip,lan9668-otpc) as compatible
string.

Or this is not the correct change?
At least with this change dt_binding_check is happy.

> 
> Best regards,
> Krzysztof

-- 
/Horatiu

Re: [PATCH v2 1/2] dt-bindings: lan9662-otpc: document Lan9662 OTPC

[Rob Herring]

On Thu, 25 Aug 2022 22:40:40 +0200, Horatiu Vultur wrote:
> Document Lan9662 OTP controller.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../nvmem/microchip,lan9662-otpc.yaml         | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.example.dtb: otp@e0021000: compatible: ['microchip,lan9662-otpc'] is too short
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.example.dtb: otp@e0021000: Unevaluated properties are not allowed ('compatible' was unexpected)
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v2 1/2] dt-bindings: lan9662-otpc: document Lan9662 OTPC

[Krzysztof Kozlowski]

On 26/08/2022 10:31, Horatiu Vultur wrote:
> The 08/26/2022 09:42, Krzysztof Kozlowski wrote:
> 
> Hi Krzysztof,
> 
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - const: microchip,lan9662-otpc
>>> +      - const: microchip,lan9668-otpc
>>
>> Does not look like you tested the bindings. Please run `make
>> dt_binding_check` (see
>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>
>> This won't work...
> 
> You are right. That was a silly mistake on my side.
> 
> It should be:
> ---
> properties:
>   compatible:
>     enum:
>       - microchip,lan9662-otpc
>       - microchip,lan9668-otpc
> ---
> Because what I want to achive is to be able to use any of
> string(microchip,lan9662-otpc or microchip,lan9668-otpc) as compatible
> string.
> 
> Or this is not the correct change?
> At least with this change dt_binding_check is happy.

This would be correct from syntax point of view, however maybe not the
best choice from functional point of view. How you wrote the driver and
bindings, these devices are compatible, so why this is not expressed as
compatible devices?

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: lan9662-otpc: document Lan9662 OTPC

[Horatiu Vultur]

The 08/26/2022 20:37, Krzysztof Kozlowski wrote:
> 
> On 26/08/2022 10:31, Horatiu Vultur wrote:
> > The 08/26/2022 09:42, Krzysztof Kozlowski wrote:
> >
> > Hi Krzysztof,
> >
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>> +      - const: microchip,lan9662-otpc
> >>> +      - const: microchip,lan9668-otpc
> >>
> >> Does not look like you tested the bindings. Please run `make
> >> dt_binding_check` (see
> >> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> >>
> >> This won't work...
> >
> > You are right. That was a silly mistake on my side.
> >
> > It should be:
> > ---
> > properties:
> >   compatible:
> >     enum:
> >       - microchip,lan9662-otpc
> >       - microchip,lan9668-otpc
> > ---
> > Because what I want to achive is to be able to use any of
> > string(microchip,lan9662-otpc or microchip,lan9668-otpc) as compatible
> > string.
> >
> > Or this is not the correct change?
> > At least with this change dt_binding_check is happy.
> 
> This would be correct from syntax point of view, however maybe not the
> best choice from functional point of view. How you wrote the driver and
> bindings, these devices are compatible, so why this is not expressed as
> compatible devices?

OK, so then it should be something like this?
---
properties:
  compatible:
    items:
       - const: microchip,lan9662-otpc
       - const: microchip,lan9668-otpc
---

I have tried to look at the following yaml files[1],[2] to see how they
have done it.

[1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
[2] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/nvmem/imx-ocotp.yaml

> 
> Best regards,
> Krzysztof

-- 
/Horatiu

Re: [PATCH v2 1/2] dt-bindings: lan9662-otpc: document Lan9662 OTPC

[Krzysztof Kozlowski]

On 29/08/2022 09:35, Horatiu Vultur wrote:
> The 08/26/2022 20:37, Krzysztof Kozlowski wrote:
>>
>> On 26/08/2022 10:31, Horatiu Vultur wrote:
>>> The 08/26/2022 09:42, Krzysztof Kozlowski wrote:
>>>
>>> Hi Krzysztof,
>>>
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    items:
>>>>> +      - const: microchip,lan9662-otpc
>>>>> +      - const: microchip,lan9668-otpc
>>>>
>>>> Does not look like you tested the bindings. Please run `make
>>>> dt_binding_check` (see
>>>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>>>
>>>> This won't work...
>>>
>>> You are right. That was a silly mistake on my side.
>>>
>>> It should be:
>>> ---
>>> properties:
>>>   compatible:
>>>     enum:
>>>       - microchip,lan9662-otpc
>>>       - microchip,lan9668-otpc
>>> ---
>>> Because what I want to achive is to be able to use any of
>>> string(microchip,lan9662-otpc or microchip,lan9668-otpc) as compatible
>>> string.
>>>
>>> Or this is not the correct change?
>>> At least with this change dt_binding_check is happy.
>>
>> This would be correct from syntax point of view, however maybe not the
>> best choice from functional point of view. How you wrote the driver and
>> bindings, these devices are compatible, so why this is not expressed as
>> compatible devices?
> 
> OK, so then it should be something like this?
> ---
> properties:
>   compatible:
>     items:
>        - const: microchip,lan9662-otpc
>        - const: microchip,lan9668-otpc
> ---
> 

I would expect:

oneOf:
  - items:
       - const: microchip,lan9668-otpc
       - const: microchip,lan9662-otpc
  - enum:
       - microchip,lan9662-otpc

(but you need to fix indentation)

Best regards,
Krzysztof

Re: [PATCH v2 2/2] nvmem: lan9662-otp: add support.

[Srinivas Kandagatla]

On 25/08/2022 21:40, Horatiu Vultur wrote:
> Add support for OTP controller available on LAN9662. The OTPC controls
> the access to a non-volatile memory. The size of the memory is 8KB.
> The OTPC can access the memory based on an offset.
> Implement both the read and the write functionality.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>   drivers/nvmem/Kconfig        |   8 ++
>   drivers/nvmem/Makefile       |   2 +
>   drivers/nvmem/lan9662-otpc.c | 249 +++++++++++++++++++++++++++++++++++
>   3 files changed, 259 insertions(+)
>   create mode 100644 drivers/nvmem/lan9662-otpc.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 967d0084800e..c9929ec35a39 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -84,6 +84,14 @@ config NVMEM_LPC18XX_OTP
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called nvmem_lpc18xx_otp.
>   
> +config NVMEM_LAN9662_OTPC
> +	tristate "Microchip LAN9662 OTP controller support"
> +	depends on SOC_LAN966 || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  This driver enables the OTP controller available on Microchip LAN9662
> +	  SoCs. It controlls the access to the OTP memory connected to it.
> +

s/controlls/controls/


>   config NVMEM_MXS_OCOTP
>   	tristate "Freescale MXS On-Chip OTP Memory Support"
>   	depends on ARCH_MXS || COMPILE_TEST
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index 00e136a0a123..e1baface2c53 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -21,6 +21,8 @@ obj-$(CONFIG_NVMEM_LPC18XX_EEPROM)	+= nvmem_lpc18xx_eeprom.o
>   nvmem_lpc18xx_eeprom-y	:= lpc18xx_eeprom.o
>   obj-$(CONFIG_NVMEM_LPC18XX_OTP)	+= nvmem_lpc18xx_otp.o
>   nvmem_lpc18xx_otp-y		:= lpc18xx_otp.o
> +obj-$(CONFIG_NVMEM_LAN9662_OTPC)	+= nvmem-lan9662-otpc.o
> +nvmem-lan9662-otpc-y		:= lan9662-otpc.o
>   obj-$(CONFIG_NVMEM_MXS_OCOTP)	+= nvmem-mxs-ocotp.o
>   nvmem-mxs-ocotp-y		:= mxs-ocotp.o
>   obj-$(CONFIG_NVMEM_NINTENDO_OTP)	+= nvmem-nintendo-otp.o
> diff --git a/drivers/nvmem/lan9662-otpc.c b/drivers/nvmem/lan9662-otpc.c
> new file mode 100644
> index 000000000000..302a5bae04dc
> --- /dev/null
> +++ b/drivers/nvmem/lan9662-otpc.c
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define OTP_OTP_PWR_DN(t)			(t + 0x00)
> +#define OTP_OTP_PWR_DN_OTP_PWRDN_N		BIT(0)
> +#define OTP_OTP_ADDR_HI(t)			(t + 0x04)
> +#define OTP_OTP_ADDR_LO(t)			(t + 0x08)
> +#define OTP_OTP_PRGM_DATA(t)			(t + 0x10)
> +#define OTP_OTP_PRGM_MODE(t)			(t + 0x14)
> +#define OTP_OTP_PRGM_MODE_OTP_PGM_MODE_BYTE	BIT(0)
> +#define OTP_OTP_RD_DATA(t)			(t + 0x18)
> +#define OTP_OTP_FUNC_CMD(t)			(t + 0x20)
> +#define OTP_OTP_FUNC_CMD_OTP_PROGRAM		BIT(1)
> +#define OTP_OTP_FUNC_CMD_OTP_READ		BIT(0)
> +#define OTP_OTP_CMD_GO(t)			(t + 0x28)
> +#define OTP_OTP_CMD_GO_OTP_GO			BIT(0)
> +#define OTP_OTP_PASS_FAIL(t)			(t + 0x2c)
> +#define OTP_OTP_PASS_FAIL_OTP_READ_PROHIBITED	BIT(3)
> +#define OTP_OTP_PASS_FAIL_OTP_WRITE_PROHIBITED	BIT(2)
> +#define OTP_OTP_PASS_FAIL_OTP_FAIL		BIT(0)
> +#define OTP_OTP_STATUS(t)			(t + 0x30)
> +#define OTP_OTP_STATUS_OTP_CPUMPEN		BIT(1)
> +#define OTP_OTP_STATUS_OTP_BUSY			BIT(0)
> +
> +#define OTP_MEM_SIZE 8192
> +#define OTP_SLEEP_US 10
> +#define OTP_TIMEOUT_US 500000
> +
> +struct lan9662_otp {
> +	struct device *dev;
> +	void __iomem *base;
> +};
> +
> +static inline void lan9662_writel(void __iomem *addr, u32 val)
> +{
> +	writel(val, addr);
> +}
> +
> +static inline u32 lan9662_readl(void __iomem *addr)
> +{
> +	return readl(addr);
> +}
> +

Why these boiler plate functions?

> +static inline void lan9662_clrbits(void __iomem *addr, u32 clear)
> +{
> +	writel(readl(addr) & ~clear, addr);
> +}
> +
> +static inline void lan9662_setbits(void __iomem *addr, u32 set)
> +{
> +	writel(readl(addr) | set, addr);
> +}

These two functions are called just once and I see no point in having a 
wrapper function for this, instead you could use them directly or use 
./include/linux/bitfield.h helper macros.

> +
> +static bool lan9662_otp_wait_flag_clear(void __iomem *reg, u32 flag)
> +{
> +	u32 val;
> +
> +	return readl_poll_timeout(reg, val, !(val & flag),
> +				  OTP_SLEEP_US, OTP_TIMEOUT_US);
> +}
> +
> +static int lan9662_otp_power(struct lan9662_otp *otp, bool up)
> +{
> +	if (up) {
> +		lan9662_clrbits(OTP_OTP_PWR_DN(otp->base),
> +				OTP_OTP_PWR_DN_OTP_PWRDN_N);
> +		if (lan9662_otp_wait_flag_clear(OTP_OTP_STATUS(otp->base),
> +						OTP_OTP_STATUS_OTP_CPUMPEN))
> +			return -ETIMEDOUT;
> +	} else {
> +		lan9662_setbits(OTP_OTP_PWR_DN(otp->base),
> +				OTP_OTP_PWR_DN_OTP_PWRDN_N);
> +	}
> +
> +	return 0;
> +}
> +
> +static int lan9662_otp_execute(struct lan9662_otp *otp)
> +{
> +	if (lan9662_otp_wait_flag_clear(OTP_OTP_CMD_GO(otp->base),
> +					OTP_OTP_CMD_GO_OTP_GO))
> +		return -ETIMEDOUT;
> +
> +	if (lan9662_otp_wait_flag_clear(OTP_OTP_STATUS(otp->base),
> +					OTP_OTP_STATUS_OTP_BUSY))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static void lan9662_otp_set_address(struct lan9662_otp *otp, u32 offset)
> +{
> +	WARN_ON(offset >= OTP_MEM_SIZE);
> +
would we ever hit this condition? looks like unecessary check.



> +	lan9662_writel(OTP_OTP_ADDR_HI(otp->base), 0xff & (offset >> 8));
> +	lan9662_writel(OTP_OTP_ADDR_LO(otp->base), 0xff & offset);
> +}
> +
> +static int lan9662_otp_read_byte(struct lan9662_otp *otp, u32 offset, u8 *dst)
> +{
> +	u32 pass;
> +	int rc;
> +
> +	lan9662_otp_set_address(otp, offset);
> +	lan9662_writel(OTP_OTP_FUNC_CMD(otp->base),
> +		       OTP_OTP_FUNC_CMD_OTP_READ);
> +	lan9662_writel(OTP_OTP_CMD_GO(otp->base),
> +		       OTP_OTP_CMD_GO_OTP_GO);
Can be wrapped into single line.

> +	rc = lan9662_otp_execute(otp);
> +	if (!rc) {
> +		pass = lan9662_readl(OTP_OTP_PASS_FAIL(otp->base));
> +		if (pass & OTP_OTP_PASS_FAIL_OTP_READ_PROHIBITED)
> +			return -EACCES;
> +		*dst = (u8) lan9662_readl(OTP_OTP_RD_DATA(otp->base));
> +	}
> +	return rc;
> +}
> +

...

thanks,
srini
> +
> +static const struct of_device_id lan9662_otp_match[] = {
> +	{ .compatible = "microchip,lan9662-otp", },
> +	{ .compatible = "microchip,lan9668-otp", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, lan9662_otp_match);
> +
> +static struct platform_driver lan9662_otp_driver = {
> +	.probe = lan9662_otp_probe,
> +	.driver = {
> +		.name = "lan9662-otp",
> +		.of_match_table = lan9662_otp_match,
> +	},
> +};
> +module_platform_driver(lan9662_otp_driver);
> +
> +MODULE_AUTHOR("Horatiu Vultur <horatiu.vultur@microchip.com>");
> +MODULE_DESCRIPTION("lan9662 OTP driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v2 2/2] nvmem: lan9662-otp: add support.

[Horatiu Vultur]

The 08/30/2022 13:08, Srinivas Kandagatla wrote:
> 
> > +static inline void lan9662_writel(void __iomem *addr, u32 val)
> > +{
> > +     writel(val, addr);
> > +}
> > +
> > +static inline u32 lan9662_readl(void __iomem *addr)
> > +{
> > +     return readl(addr);
> > +}
> > +
> 
> Why these boiler plate functions?

It was more for the style purpose. I will remove these ones.

> 
> > +static inline void lan9662_clrbits(void __iomem *addr, u32 clear)
> > +{
> > +     writel(readl(addr) & ~clear, addr);
> > +}
> > +
> > +static inline void lan9662_setbits(void __iomem *addr, u32 set)
> > +{
> > +     writel(readl(addr) | set, addr);
> > +}
> 
> These two functions are called just once and I see no point in having a
> wrapper function for this, instead you could use them directly or use
> ./include/linux/bitfield.h helper macros.

I will remove also these ones and use them directly.

> 
> > +
> > +static bool lan9662_otp_wait_flag_clear(void __iomem *reg, u32 flag)
> > +{
> > +     u32 val;
> > +
> > +     return readl_poll_timeout(reg, val, !(val & flag),
> > +                               OTP_SLEEP_US, OTP_TIMEOUT_US);
> > +}
> > +
> > +static int lan9662_otp_power(struct lan9662_otp *otp, bool up)
> > +{
> > +     if (up) {
> > +             lan9662_clrbits(OTP_OTP_PWR_DN(otp->base),
> > +                             OTP_OTP_PWR_DN_OTP_PWRDN_N);
> > +             if (lan9662_otp_wait_flag_clear(OTP_OTP_STATUS(otp->base),
> > +                                             OTP_OTP_STATUS_OTP_CPUMPEN))
> > +                     return -ETIMEDOUT;
> > +     } else {
> > +             lan9662_setbits(OTP_OTP_PWR_DN(otp->base),
> > +                             OTP_OTP_PWR_DN_OTP_PWRDN_N);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int lan9662_otp_execute(struct lan9662_otp *otp)
> > +{
> > +     if (lan9662_otp_wait_flag_clear(OTP_OTP_CMD_GO(otp->base),
> > +                                     OTP_OTP_CMD_GO_OTP_GO))
> > +             return -ETIMEDOUT;
> > +
> > +     if (lan9662_otp_wait_flag_clear(OTP_OTP_STATUS(otp->base),
> > +                                     OTP_OTP_STATUS_OTP_BUSY))
> > +             return -ETIMEDOUT;
> > +
> > +     return 0;
> > +}
> > +
> > +static void lan9662_otp_set_address(struct lan9662_otp *otp, u32 offset)
> > +{
> > +     WARN_ON(offset >= OTP_MEM_SIZE);
> > +
> would we ever hit this condition? looks like unecessary check.

That is not the case. I will remove it.

> 
> 
> 

-- 
/Horatiu