[PATCH 1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support

[PATCH 1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support

[Marek Vasut]

The ST M24256E behaves as a regular M24C256, except for the E variant
which uses up another I2C address for Additional Write lockable page.
This page is 64 Bytes long and can contain additional data. Add entry
for it, so users can describe that page in DT. Note that users still
have to describe the main M24C256 area separately as that is on separate
I2C address from this page.

Unlike M24C32-D and M24C64-D, this part is specifically ST and does not
have any comparable M24* counterparts from other vendors, hence the st,
vendor prefix. Furthermore, the part name is M24256E without C between
the 24 and 256, this is not a typo. Finally, there is M24C256-D part,
which does contain 32 Bytes long Additional Write lockable page, which
is a different part and not supported by this patch.

Datasheet: https://www.st.com/resource/en/datasheet/m24256e-f.pdf

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: kernel@dh-electronics.com
Cc: linux-i2c@vger.kernel.org
---
 Documentation/devicetree/bindings/eeprom/at24.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
index b6239ec3512b3..590ba0ef5fa26 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.yaml
+++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
@@ -141,6 +141,8 @@ properties:
           - const: microchip,24aa025e48
       - items:
           - const: microchip,24aa025e64
+      - items:
+          - const: st,24256e-wl
       - pattern: '^atmel,24c(32|64)d-wl$' # Actual vendor is st
 
   label:
-- 
2.45.2

[PATCH 2/2] eeprom: at24: add ST M24256E Additional Write lockable page support

[Marek Vasut]

The ST M24256E behaves as a regular M24C256, except for the E variant
which uses up another I2C address for Additional Write lockable page.
This page is 64 Bytes long and can contain additional data. Add entry
for it, so users can describe that page in DT. Note that users still
have to describe the main M24C256 area separately as that is on separate
I2C address from this page.

Unlike M24C32-D and M24C64-D, this part is specifically ST and does not
have any comparable M24* counterparts from other vendors, hence the st,
vendor prefix. Furthermore, the part name is M24256E without C between
the 24 and 256, this is not a typo. Finally, there is M24C256-D part,
which does contain 32 Bytes long Additional Write lockable page, which
is a different part and not supported by this patch.

Datasheet: https://www.st.com/resource/en/datasheet/m24256e-f.pdf

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: kernel@dh-electronics.com
Cc: linux-i2c@vger.kernel.org
---
 drivers/misc/eeprom/at24.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index ca872e3465ed9..0a7c7f29406c7 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -207,6 +207,8 @@ AT24_CHIP_DATA(at24_data_24cs64, 16,
 	AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
 AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16);
 AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16);
+/* M24256E Additional Write lockable page (M24256E-F order codes) */
+AT24_CHIP_DATA(at24_data_24256e_wlp, 64, AT24_FLAG_ADDR16);
 AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16);
 AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16);
 AT24_CHIP_DATA_BS(at24_data_24c1025, 1048576 / 8, AT24_FLAG_ADDR16, 2);
@@ -240,6 +242,7 @@ static const struct i2c_device_id at24_ids[] = {
 	{ "24cs64",	(kernel_ulong_t)&at24_data_24cs64 },
 	{ "24c128",	(kernel_ulong_t)&at24_data_24c128 },
 	{ "24c256",	(kernel_ulong_t)&at24_data_24c256 },
+	{ "24256e-wl",	(kernel_ulong_t)&at24_data_24256e_wlp },
 	{ "24c512",	(kernel_ulong_t)&at24_data_24c512 },
 	{ "24c1024",	(kernel_ulong_t)&at24_data_24c1024 },
 	{ "24c1025",	(kernel_ulong_t)&at24_data_24c1025 },
@@ -278,6 +281,7 @@ static const struct of_device_id __maybe_unused at24_of_match[] = {
 	{ .compatible = "atmel,24c2048",	.data = &at24_data_24c2048 },
 	{ .compatible = "microchip,24aa025e48",	.data = &at24_data_24aa025e48 },
 	{ .compatible = "microchip,24aa025e64",	.data = &at24_data_24aa025e64 },
+	{ .compatible = "st,24256e-wl",		.data = &at24_data_24256e_wlp },
 	{ /* END OF LIST */ },
 };
 MODULE_DEVICE_TABLE(of, at24_of_match);
-- 
2.45.2

Re: [PATCH 1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support

[Rob Herring]

On Thu, Oct 17, 2024 at 08:41:25PM +0200, Marek Vasut wrote:
> The ST M24256E behaves as a regular M24C256, except for the E variant
> which uses up another I2C address for Additional Write lockable page.
> This page is 64 Bytes long and can contain additional data. Add entry
> for it, so users can describe that page in DT. Note that users still
> have to describe the main M24C256 area separately as that is on separate
> I2C address from this page.

I think this should be modelled as 1 node having 2 addresses, not 2 
nodes.

> 
> Unlike M24C32-D and M24C64-D, this part is specifically ST and does not
> have any comparable M24* counterparts from other vendors, hence the st,
> vendor prefix. Furthermore, the part name is M24256E without C between
> the 24 and 256, this is not a typo. Finally, there is M24C256-D part,
> which does contain 32 Bytes long Additional Write lockable page, which
> is a different part and not supported by this patch.
> 
> Datasheet: https://www.st.com/resource/en/datasheet/m24256e-f.pdf
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: kernel@dh-electronics.com
> Cc: linux-i2c@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/eeprom/at24.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> index b6239ec3512b3..590ba0ef5fa26 100644
> --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> @@ -141,6 +141,8 @@ properties:
>            - const: microchip,24aa025e48
>        - items:
>            - const: microchip,24aa025e64
> +      - items:
> +          - const: st,24256e-wl
>        - pattern: '^atmel,24c(32|64)d-wl$' # Actual vendor is st
>  
>    label:
> -- 
> 2.45.2
> 

Re: [PATCH 1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support

[Marek Vasut]

On 10/18/24 3:27 PM, Rob Herring wrote:
> On Thu, Oct 17, 2024 at 08:41:25PM +0200, Marek Vasut wrote:
>> The ST M24256E behaves as a regular M24C256, except for the E variant
>> which uses up another I2C address for Additional Write lockable page.
>> This page is 64 Bytes long and can contain additional data. Add entry
>> for it, so users can describe that page in DT. Note that users still
>> have to describe the main M24C256 area separately as that is on separate
>> I2C address from this page.
> 
> I think this should be modelled as 1 node having 2 addresses, not 2
> nodes.
We had the exact same discussion regarding M24C32D, see:

https://lore.kernel.org/all/CAMRc=MdTu1gagX-L4_cHmN9aUCoKhN-b5i7yEeszKSdr+BuROg@mail.gmail.com/

Re: [PATCH 1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support

[Rob Herring]

On Sun, Oct 20, 2024 at 06:29:13AM +0200, Marek Vasut wrote:
> On 10/18/24 3:27 PM, Rob Herring wrote:
> > On Thu, Oct 17, 2024 at 08:41:25PM +0200, Marek Vasut wrote:
> > > The ST M24256E behaves as a regular M24C256, except for the E variant
> > > which uses up another I2C address for Additional Write lockable page.
> > > This page is 64 Bytes long and can contain additional data. Add entry
> > > for it, so users can describe that page in DT. Note that users still
> > > have to describe the main M24C256 area separately as that is on separate
> > > I2C address from this page.
> > 
> > I think this should be modelled as 1 node having 2 addresses, not 2
> > nodes.
> We had the exact same discussion regarding M24C32D, see:
> 
> https://lore.kernel.org/all/CAMRc=MdTu1gagX-L4_cHmN9aUCoKhN-b5i7yEeszKSdr+BuROg@mail.gmail.com/

Seems like kernel implementation details dictating the binding to me. 
Won't be a problem until there are shared resources on "both" devices.

I guess since it is inline with what we already have:

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support

[Bartosz Golaszewski]

On Mon, Oct 21, 2024 at 8:14 PM Rob Herring <robh@kernel.org> wrote:
>
> On Sun, Oct 20, 2024 at 06:29:13AM +0200, Marek Vasut wrote:
> > On 10/18/24 3:27 PM, Rob Herring wrote:
> > > On Thu, Oct 17, 2024 at 08:41:25PM +0200, Marek Vasut wrote:
> > > > The ST M24256E behaves as a regular M24C256, except for the E variant
> > > > which uses up another I2C address for Additional Write lockable page.
> > > > This page is 64 Bytes long and can contain additional data. Add entry
> > > > for it, so users can describe that page in DT. Note that users still
> > > > have to describe the main M24C256 area separately as that is on separate
> > > > I2C address from this page.
> > >
> > > I think this should be modelled as 1 node having 2 addresses, not 2
> > > nodes.
> > We had the exact same discussion regarding M24C32D, see:
> >
> > https://lore.kernel.org/all/CAMRc=MdTu1gagX-L4_cHmN9aUCoKhN-b5i7yEeszKSdr+BuROg@mail.gmail.com/
>
> Seems like kernel implementation details dictating the binding to me.

Yeah, that's on me. I would have known better today but 8 years ago
the DT situation was much more volatile.

> Won't be a problem until there are shared resources on "both" devices.
>

Fortunately, that could be addressed with the power sequencing subsystem. :)

Bart

Re: [PATCH 1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Thu, 17 Oct 2024 20:41:25 +0200, Marek Vasut wrote:
> The ST M24256E behaves as a regular M24C256, except for the E variant
> which uses up another I2C address for Additional Write lockable page.
> This page is 64 Bytes long and can contain additional data. Add entry
> for it, so users can describe that page in DT. Note that users still
> have to describe the main M24C256 area separately as that is on separate
> I2C address from this page.
> 
> [...]

Applied, thanks!

[1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support
      (no commit info)
[2/2] eeprom: at24: add ST M24256E Additional Write lockable page support
      (no commit info)

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support

[Bartosz Golaszewski]

On Tue, Oct 22, 2024 at 9:13 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
>
> On Thu, 17 Oct 2024 20:41:25 +0200, Marek Vasut wrote:
> > The ST M24256E behaves as a regular M24C256, except for the E variant
> > which uses up another I2C address for Additional Write lockable page.
> > This page is 64 Bytes long and can contain additional data. Add entry
> > for it, so users can describe that page in DT. Note that users still
> > have to describe the main M24C256 area separately as that is on separate
> > I2C address from this page.
> >
> > [...]
>
> Applied, thanks!
>
> [1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support
>       (no commit info)
> [2/2] eeprom: at24: add ST M24256E Additional Write lockable page support
>       (no commit info)
>

No worries about the "no commit info" I was on a different branch when
generating thankyou emails. Sorry.

Bart

> Best regards,
> --
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support

[Geert Uytterhoeven]

Hi Bartosz,

On Mon, Oct 21, 2024 at 8:36 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Mon, Oct 21, 2024 at 8:14 PM Rob Herring <robh@kernel.org> wrote:
> > On Sun, Oct 20, 2024 at 06:29:13AM +0200, Marek Vasut wrote:
> > > On 10/18/24 3:27 PM, Rob Herring wrote:
> > > > On Thu, Oct 17, 2024 at 08:41:25PM +0200, Marek Vasut wrote:
> > > > > The ST M24256E behaves as a regular M24C256, except for the E variant
> > > > > which uses up another I2C address for Additional Write lockable page.
> > > > > This page is 64 Bytes long and can contain additional data. Add entry
> > > > > for it, so users can describe that page in DT. Note that users still
> > > > > have to describe the main M24C256 area separately as that is on separate
> > > > > I2C address from this page.
> > > >
> > > > I think this should be modelled as 1 node having 2 addresses, not 2
> > > > nodes.
> > > We had the exact same discussion regarding M24C32D, see:
> > >
> > > https://lore.kernel.org/all/CAMRc=MdTu1gagX-L4_cHmN9aUCoKhN-b5i7yEeszKSdr+BuROg@mail.gmail.com/
> >
> > Seems like kernel implementation details dictating the binding to me.
>
> Yeah, that's on me. I would have known better today but 8 years ago
> the DT situation was much more volatile.

And there's no way we can fix that for new devices?  Perhaps even
for old devices, by counting the number of entries in the "reg"
compatible value?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] dt-bindings: at24: add ST M24256E Additional Write lockable page support

[Bartosz Golaszewski]

On Tue, 19 Nov 2024 12:01:54 +0100, Geert Uytterhoeven
<geert@linux-m68k.org> said:
> Hi Bartosz,
>
> On Mon, Oct 21, 2024 at 8:36 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> On Mon, Oct 21, 2024 at 8:14 PM Rob Herring <robh@kernel.org> wrote:
>> > On Sun, Oct 20, 2024 at 06:29:13AM +0200, Marek Vasut wrote:
>> > > On 10/18/24 3:27 PM, Rob Herring wrote:
>> > > > On Thu, Oct 17, 2024 at 08:41:25PM +0200, Marek Vasut wrote:
>> > > > > The ST M24256E behaves as a regular M24C256, except for the E variant
>> > > > > which uses up another I2C address for Additional Write lockable page.
>> > > > > This page is 64 Bytes long and can contain additional data. Add entry
>> > > > > for it, so users can describe that page in DT. Note that users still
>> > > > > have to describe the main M24C256 area separately as that is on separate
>> > > > > I2C address from this page.
>> > > >
>> > > > I think this should be modelled as 1 node having 2 addresses, not 2
>> > > > nodes.
>> > > We had the exact same discussion regarding M24C32D, see:
>> > >
>> > > https://lore.kernel.org/all/CAMRc=MdTu1gagX-L4_cHmN9aUCoKhN-b5i7yEeszKSdr+BuROg@mail.gmail.com/
>> >
>> > Seems like kernel implementation details dictating the binding to me.
>>
>> Yeah, that's on me. I would have known better today but 8 years ago
>> the DT situation was much more volatile.
>
> And there's no way we can fix that for new devices?  Perhaps even
> for old devices, by counting the number of entries in the "reg"
> compatible value?
>

For sure. We can always deprecate old bindings after upstreaming new ones but
we'd need to still carry the model ID in the compatible string and remain
backward compatible with existing DTs.

So an existing example of:

	at24c01@57 {
		compatible = "atmel,24c01";
		reg = <0x57>;
	};

	at24cs01@5f {
		compatible = "atmel,24cs01";
		reg = <0x5f>;
	};

Would become:

	at24cs01@57 {
		compatible = "atmel,24cs01";
		reg = <0x57>, <0x5f>;
	};

Now the driver takes the "atmel,24cs01" compatible and needs to count the 'reg'
items to determine whether to create one or two nvmem sub-devices.

It's nothing impossible but would be quite tedious and make the driver so much
more complex.

I, for sure, don't have the bandwidth right now to tackle it but I'd happily
help with testing and reviewing.

Bart