[PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[Patrick Williams]

The i2c-slave-eeprom driver emulates at24 class eeprom devices,
which come initialized with all 1s.  Do the same in the software
emulation.

Signed-off-by: Patrick Williams <alpawi@amazon.com>
---
 drivers/i2c/i2c-slave-eeprom.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
index db9763cb4dae..efee56106251 100644
--- a/drivers/i2c/i2c-slave-eeprom.c
+++ b/drivers/i2c/i2c-slave-eeprom.c
@@ -131,6 +131,8 @@ static int i2c_slave_eeprom_probe(struct i2c_client *client, const struct i2c_de
 	if (!eeprom)
 		return -ENOMEM;
 
+	memset(eeprom->buffer, 0xFF, size);
+
 	eeprom->idx_write_cnt = 0;
 	eeprom->num_address_bytes = flag_addr16 ? 2 : 1;
 	eeprom->address_mask = size - 1;
-- 
2.17.2 (Apple Git-113)

[PATCH 2/2] i2c: slave-eeprom: support additional models

[Patrick Williams]

Add support for emulating the following EEPROMs:
    * 24c01  - 1024 bit
    * 24c128 - 128k bit
    * 24c256 - 256k bit
    * 24c512 - 512k bit

The flag bits in the device id were shifted up 1 bit to make
room for saving the 24c512's size.  24c512 uses the full 16-bit
address space of a 2-byte addressable EEPROM.

Signed-off-by: Patrick Williams <alpawi@amazon.com>
---
 drivers/i2c/i2c-slave-eeprom.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
index efee56106251..65419441313b 100644
--- a/drivers/i2c/i2c-slave-eeprom.c
+++ b/drivers/i2c/i2c-slave-eeprom.c
@@ -37,9 +37,9 @@ struct eeprom_data {
 	u8 buffer[];
 };
 
-#define I2C_SLAVE_BYTELEN GENMASK(15, 0)
-#define I2C_SLAVE_FLAG_ADDR16 BIT(16)
-#define I2C_SLAVE_FLAG_RO BIT(17)
+#define I2C_SLAVE_BYTELEN GENMASK(16, 0)
+#define I2C_SLAVE_FLAG_ADDR16 BIT(17)
+#define I2C_SLAVE_FLAG_RO BIT(18)
 #define I2C_SLAVE_DEVICE_MAGIC(_len, _flags) ((_flags) | (_len))
 
 static int i2c_slave_eeprom_slave_cb(struct i2c_client *client,
@@ -171,12 +171,20 @@ static int i2c_slave_eeprom_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id i2c_slave_eeprom_id[] = {
+	{ "slave-24c01", I2C_SLAVE_DEVICE_MAGIC(1024 / 8,  0) },
+	{ "slave-24c01ro", I2C_SLAVE_DEVICE_MAGIC(1024 / 8,  I2C_SLAVE_FLAG_RO) },
 	{ "slave-24c02", I2C_SLAVE_DEVICE_MAGIC(2048 / 8,  0) },
 	{ "slave-24c02ro", I2C_SLAVE_DEVICE_MAGIC(2048 / 8,  I2C_SLAVE_FLAG_RO) },
 	{ "slave-24c32", I2C_SLAVE_DEVICE_MAGIC(32768 / 8, I2C_SLAVE_FLAG_ADDR16) },
 	{ "slave-24c32ro", I2C_SLAVE_DEVICE_MAGIC(32768 / 8, I2C_SLAVE_FLAG_ADDR16 | I2C_SLAVE_FLAG_RO) },
 	{ "slave-24c64", I2C_SLAVE_DEVICE_MAGIC(65536 / 8, I2C_SLAVE_FLAG_ADDR16) },
 	{ "slave-24c64ro", I2C_SLAVE_DEVICE_MAGIC(65536 / 8, I2C_SLAVE_FLAG_ADDR16 | I2C_SLAVE_FLAG_RO) },
+	{ "slave-24c128", I2C_SLAVE_DEVICE_MAGIC(131072 / 8, I2C_SLAVE_FLAG_ADDR16) },
+	{ "slave-24c128ro", I2C_SLAVE_DEVICE_MAGIC(131072 / 8, I2C_SLAVE_FLAG_ADDR16 | I2C_SLAVE_FLAG_RO) },
+	{ "slave-24c256", I2C_SLAVE_DEVICE_MAGIC(262144 / 8, I2C_SLAVE_FLAG_ADDR16) },
+	{ "slave-24c256ro", I2C_SLAVE_DEVICE_MAGIC(262144 / 8, I2C_SLAVE_FLAG_ADDR16 | I2C_SLAVE_FLAG_RO) },
+	{ "slave-24c512", I2C_SLAVE_DEVICE_MAGIC(524288 / 8, I2C_SLAVE_FLAG_ADDR16) },
+	{ "slave-24c512ro", I2C_SLAVE_DEVICE_MAGIC(524288 / 8, I2C_SLAVE_FLAG_ADDR16 | I2C_SLAVE_FLAG_RO) },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, i2c_slave_eeprom_id);
-- 
2.17.2 (Apple Git-113)

Re: [PATCH 2/2] i2c: slave-eeprom: support additional models

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

On Tue, Oct 01, 2019 at 11:40:06AM -0500, Patrick Williams wrote:
> Add support for emulating the following EEPROMs:
>     * 24c01  - 1024 bit
>     * 24c128 - 128k bit
>     * 24c256 - 256k bit
>     * 24c512 - 512k bit
> 
> The flag bits in the device id were shifted up 1 bit to make
> room for saving the 24c512's size.  24c512 uses the full 16-bit
> address space of a 2-byte addressable EEPROM.
> 
> Signed-off-by: Patrick Williams <alpawi@amazon.com>

Do you really need them or is it just nice to have?

I am undecided. I definately don't want all the EEPROM types which
exist, but the full 16 bit address range makes sense...

More opinions welcome.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] i2c: slave-eeprom: support additional models

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

On Mon, Apr 20, 2020 at 06:46:19PM +0200, Wolfram Sang wrote:
> On Tue, Oct 01, 2019 at 11:40:06AM -0500, Patrick Williams wrote:
> > Add support for emulating the following EEPROMs:
> >     * 24c01  - 1024 bit
> >     * 24c128 - 128k bit
> >     * 24c256 - 256k bit
> >     * 24c512 - 512k bit
> > 
> > The flag bits in the device id were shifted up 1 bit to make
> > room for saving the 24c512's size.  24c512 uses the full 16-bit
> > address space of a 2-byte addressable EEPROM.
> > 
> > Signed-off-by: Patrick Williams <alpawi@amazon.com>
> 
> Do you really need them or is it just nice to have?
> 
> I am undecided. I definately don't want all the EEPROM types which
> exist, but the full 16 bit address range makes sense...
> 
> More opinions welcome.
> 

I don't remember exactly which ones we needed (and I am no longer at
Amazon), but it was pretty trivial to add them all to the table so I
went ahead and did it.  As long as we had one of the 2-byte addressable
EEPROMs, anything else necessary could be handleded as a small
out-of-tree patch.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[Bjorn Ardo]

Hi,


I sent in another patch earlier that added support for specifying a file 
in devicetree to initilize the eeprom from, corresponding to the case of 
pre-flashed eeprom. Maybe these two patches should be merged so this 
initialization is only done if no file is specified?


/BA

On 10/1/19 6:40 PM, Patrick Williams wrote:
> The i2c-slave-eeprom driver emulates at24 class eeprom devices,
> which come initialized with all 1s.  Do the same in the software
> emulation.
>
> Signed-off-by: Patrick Williams <alpawi@amazon.com>
> ---
>   drivers/i2c/i2c-slave-eeprom.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
> index db9763cb4dae..efee56106251 100644
> --- a/drivers/i2c/i2c-slave-eeprom.c
> +++ b/drivers/i2c/i2c-slave-eeprom.c
> @@ -131,6 +131,8 @@ static int i2c_slave_eeprom_probe(struct i2c_client *client, const struct i2c_de
>   	if (!eeprom)
>   		return -ENOMEM;
>   
> +	memset(eeprom->buffer, 0xFF, size);
> +
>   	eeprom->idx_write_cnt = 0;
>   	eeprom->num_address_bytes = flag_addr16 ? 2 : 1;
>   	eeprom->address_mask = size - 1;

Re: [PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]

On Wed, Oct 02, 2019 at 08:20:53AM +0200, Bjorn Ardo wrote:
> Hi,
> 
> 
> I sent in another patch earlier that added support for specifying a file in
> devicetree to initilize the eeprom from, corresponding to the case of
> pre-flashed eeprom. Maybe these two patches should be merged so this
> initialization is only done if no file is specified?

Yes, I agree.

> /BA
> 
> On 10/1/19 6:40 PM, Patrick Williams wrote:
> > The i2c-slave-eeprom driver emulates at24 class eeprom devices,
> > which come initialized with all 1s.  Do the same in the software
> > emulation.
> > 
> > Signed-off-by: Patrick Williams <alpawi@amazon.com>
> > ---
> >   drivers/i2c/i2c-slave-eeprom.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
> > index db9763cb4dae..efee56106251 100644
> > --- a/drivers/i2c/i2c-slave-eeprom.c
> > +++ b/drivers/i2c/i2c-slave-eeprom.c
> > @@ -131,6 +131,8 @@ static int i2c_slave_eeprom_probe(struct i2c_client *client, const struct i2c_de
> >   	if (!eeprom)
> >   		return -ENOMEM;
> > +	memset(eeprom->buffer, 0xFF, size);
> > +
> >   	eeprom->idx_write_cnt = 0;
> >   	eeprom->num_address_bytes = flag_addr16 ? 2 : 1;
> >   	eeprom->address_mask = size - 1;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

On Mon, Apr 20, 2020 at 06:43:49PM +0200, Wolfram Sang wrote:
> On Wed, Oct 02, 2019 at 08:20:53AM +0200, Bjorn Ardo wrote:
> > Hi,
> > 
> > 
> > I sent in another patch earlier that added support for specifying a file in
> > devicetree to initilize the eeprom from, corresponding to the case of
> > pre-flashed eeprom. Maybe these two patches should be merged so this
> > initialization is only done if no file is specified?
> 
> Yes, I agree.
> 

I've been meaning to get back to this...  I'll rebase the
commit on the latest tree.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On Mon, Apr 20, 2020 at 06:43:49PM +0200, Wolfram Sang wrote:
> On Wed, Oct 02, 2019 at 08:20:53AM +0200, Bjorn Ardo wrote:
> > Hi,
> > 
> > 
> > I sent in another patch earlier that added support for specifying a file in
> > devicetree to initilize the eeprom from, corresponding to the case of
> > pre-flashed eeprom. Maybe these two patches should be merged so this
> > initialization is only done if no file is specified?
> 
> Yes, I agree.
> 

It looks like Bjorn's referenced patches are still unmerged also?

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 741 bytes --]

On Mon, Apr 20, 2020 at 03:31:46PM -0500, Patrick Williams wrote:
> On Mon, Apr 20, 2020 at 06:43:49PM +0200, Wolfram Sang wrote:
> > On Wed, Oct 02, 2019 at 08:20:53AM +0200, Bjorn Ardo wrote:
> > > Hi,
> > > 
> > > 
> > > I sent in another patch earlier that added support for specifying a file in
> > > devicetree to initilize the eeprom from, corresponding to the case of
> > > pre-flashed eeprom. Maybe these two patches should be merged so this
> > > initialization is only done if no file is specified?
> > 
> > Yes, I agree.
> > 
> 
> It looks like Bjorn's referenced patches are still unmerged also?

Yes, it might be easiest if he merges your patch (with attribution) into
the else branch of his fw-load patch.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[Bjorn Ardo]

On 4/20/20 10:53 PM, Wolfram Sang wrote:

> On Mon, Apr 20, 2020 at 03:31:46PM -0500, Patrick Williams wrote:
>> On Mon, Apr 20, 2020 at 06:43:49PM +0200, Wolfram Sang wrote:
>>> On Wed, Oct 02, 2019 at 08:20:53AM +0200, Bjorn Ardo wrote:
>>>> Hi,
>>>>
>>>>
>>>> I sent in another patch earlier that added support for specifying a file in
>>>> devicetree to initilize the eeprom from, corresponding to the case of
>>>> pre-flashed eeprom. Maybe these two patches should be merged so this
>>>> initialization is only done if no file is specified?
>>> Yes, I agree.
>>>
>> It looks like Bjorn's referenced patches are still unmerged also?
> Yes, it might be easiest if he merges your patch (with attribution) into
> the else branch of his fw-load patch.
>

OK, so to summarize, I should update my patch to use 
device_property_read_string() instead and also init the memory to 0XFF 
if no file is present. And change name of the function to 
i2c_slave_init_eeprom_data.


I will look into that and let you know once I'm done.


/BA

Re: [PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]


> > Yes, it might be easiest if he merges your patch (with attribution) into
> > the else branch of his fw-load patch.
> > 
> 
> OK, so to summarize, I should update my patch to use
> device_property_read_string() instead and also init the memory to 0XFF if no
> file is present. And change name of the function to

Or something else went wrong.

> i2c_slave_init_eeprom_data.

Yes, that is my idea. You also need to replace checking for an of_node
with some equivalent for device properties maybe, but that should be
easy to find out.

> I will look into that and let you know once I'm done.

Thank you!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[Bjorn Ardo]

On 4/21/20 2:16 PM, Wolfram Sang wrote:
>>> Yes, it might be easiest if he merges your patch (with attribution) into
>>> the else branch of his fw-load patch.
>>>
>> OK, so to summarize, I should update my patch to use
>> device_property_read_string() instead and also init the memory to 0XFF if no
>> file is present. And change name of the function to
> Or something else went wrong.

I did like this now: If device_property_read_string() returns a firmware 
name, I use that, otherwise init to 0xFF. But if it returns a firmware 
name, and for some reason I get an error when trying to load that 
firmware I will not default to 0xFF, but rather fail the probe. The 
logic in that is that if you actively supply a firmware name, you should 
not silently get 0xFF in your eeprom. Does that sound good?


>> i2c_slave_init_eeprom_data.
> Yes, that is my idea. You also need to replace checking for an of_node
> with some equivalent for device properties maybe, but that should be
> easy to find out.

It appears to me that those kind of checks are done inside 
device_property_read_string() so I can just remove them and only look at 
the return value of that function.


>> I will look into that and let you know once I'm done.
> Thank you!
>

I have a patch now working on 4.14, will run some tests on it and then 
try to forward-port to latest kernel och see if it works there as well.


/BA

Re: [PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]


> I did like this now: If device_property_read_string() returns a firmware
> name, I use that, otherwise init to 0xFF. But if it returns a firmware name,
> and for some reason I get an error when trying to load that firmware I will
> not default to 0xFF, but rather fail the probe. The logic in that is that if
> you actively supply a firmware name, you should not silently get 0xFF in
> your eeprom. Does that sound good?

Sounds perfect to me.

> > Yes, that is my idea. You also need to replace checking for an of_node
> > with some equivalent for device properties maybe, but that should be
> > easy to find out.
> 
> It appears to me that those kind of checks are done inside
> device_property_read_string() so I can just remove them and only look at the
> return value of that function.

Even better!

> I have a patch now working on 4.14, will run some tests on it and then try
> to forward-port to latest kernel och see if it works there as well.

Looking forward to it. I will look at it right away then!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: slave-eeprom: initialize empty eeprom properly

[Bjorn Ardo]

On 4/22/20 11:36 AM, Wolfram Sang wrote:
>> I have a patch now working on 4.14, will run some tests on it and then try
>> to forward-port to latest kernel och see if it works there as well.
> Looking forward to it. I will look at it right away then!
>

Patch tested on latest kernel and submitted.


/BA