[PATCH RFC 0/2] eeprom: eeprom_93xx46: Add support for sending zero bits after address during read transfer

[PATCH RFC 0/2] eeprom: eeprom_93xx46: Add support for sending zero bits after address during read transfer

[Aswath Govindraju]

The following series of patches adds support for sending zero bits after
address during read transfer, through a device tree property.


Aswath Govindraju (2):
  Documentation: devicetree: Add property for ignoring the dummy bits
    sent before read transfer
  eeprom: eeprom_93xx46: Add support for ignoring the dummy bit
    preceding data during read transfer

 .../bindings/misc/eeprom-93xx46.txt           |  4 +++-
 drivers/misc/eeprom/eeprom_93xx46.c           | 23 +++++++++++++++++++
 include/linux/eeprom_93xx46.h                 |  6 +++++
 3 files changed, 32 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH RFC 1/2] Documentation: devicetree: Add property for ignoring the dummy bits sent before read transfer

[Aswath Govindraju]

Dummy zero bits are sent before data during a read transfer. This causes
the data read to be shifted to the right. To fix this send zero bits after
the address during a read transfer.

Add property to send zero bits after the address during a read transfer.

Suggested-by: Vignesh Raghavendra <vigneshr@ti.com>
Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 Documentation/devicetree/bindings/misc/eeprom-93xx46.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
index a8ebb4621f79..09fb1cec54f0 100644
--- a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
+++ b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
@@ -10,7 +10,9 @@ Optional properties:
 - read-only : parameter-less property which disables writes to the EEPROM
 - select-gpios : if present, specifies the GPIO that will be asserted prior to
   each access to the EEPROM (e.g. for SPI bus multiplexing)
-
+- read-op-dummy-cycles: If present specifies the number of dummy zero-bits to
+  be sent after the address during a read transfer to ignore any bits sent
+  preceding the data.
 Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt
 apply.  In particular, "reg" and "spi-max-frequency" properties must be given.
 
-- 
2.17.1

Re: [PATCH RFC 1/2] Documentation: devicetree: Add property for ignoring the dummy bits sent before read transfer

[Rob Herring]

On Wed, Dec 09, 2020 at 11:27:07PM +0530, Aswath Govindraju wrote:
> Dummy zero bits are sent before data during a read transfer. This causes
> the data read to be shifted to the right. To fix this send zero bits after
> the address during a read transfer.
> 
> Add property to send zero bits after the address during a read transfer.

When is this necessary? Why can't it be implied by the compatible 
string which should be specific to the chip model?

> 
> Suggested-by: Vignesh Raghavendra <vigneshr@ti.com>
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  Documentation/devicetree/bindings/misc/eeprom-93xx46.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
> index a8ebb4621f79..09fb1cec54f0 100644
> --- a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
> +++ b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
> @@ -10,7 +10,9 @@ Optional properties:
>  - read-only : parameter-less property which disables writes to the EEPROM
>  - select-gpios : if present, specifies the GPIO that will be asserted prior to
>    each access to the EEPROM (e.g. for SPI bus multiplexing)
> -
> +- read-op-dummy-cycles: If present specifies the number of dummy zero-bits to
> +  be sent after the address during a read transfer to ignore any bits sent
> +  preceding the data.
>  Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt
>  apply.  In particular, "reg" and "spi-max-frequency" properties must be given.
>  
> -- 
> 2.17.1
> 

Re: [PATCH RFC 1/2] Documentation: devicetree: Add property for ignoring the dummy bits sent before read transfer

[Aswath Govindraju]

Hi,
On 11/12/20 9:03 am, Rob Herring wrote:
> On Wed, Dec 09, 2020 at 11:27:07PM +0530, Aswath Govindraju wrote:
>> Dummy zero bits are sent before data during a read transfer. This causes
>> the data read to be shifted to the right. To fix this send zero bits after
>> the address during a read transfer.
>>
>> Add property to send zero bits after the address during a read transfer.
> 
> When is this necessary? Why can't it be implied by the compatible 
> string which should be specific to the chip model?
> 

This is necessary for 93AA46A/B/C, 93LC46A/B/C, 93C46A/B/C eeproms, as
it can be seen in section 2.7 of [1]. We were not sure if these were the
only devices supported by the driver(eeprom_93xx46.c). So, in order to
apply this only to the above listed devices, we thought that it would be
better to apply this change when required by introducing a DT property.

May I know how has this case been handled till now ??

If this is required by all the devices then we can drop the property and
include the zero bit by default.

[1]- https://www.mouser.com/datasheet/2/268/20001749K-277859.pdf

Thanks,
Aswath
>>
>> Suggested-by: Vignesh Raghavendra <vigneshr@ti.com>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>  Documentation/devicetree/bindings/misc/eeprom-93xx46.txt | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
>> index a8ebb4621f79..09fb1cec54f0 100644
>> --- a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
>> +++ b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
>> @@ -10,7 +10,9 @@ Optional properties:
>>  - read-only : parameter-less property which disables writes to the EEPROM
>>  - select-gpios : if present, specifies the GPIO that will be asserted prior to
>>    each access to the EEPROM (e.g. for SPI bus multiplexing)
>> -
>> +- read-op-dummy-cycles: If present specifies the number of dummy zero-bits to
>> +  be sent after the address during a read transfer to ignore any bits sent
>> +  preceding the data.
>>  Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt
>>  apply.  In particular, "reg" and "spi-max-frequency" properties must be given.
>>  
>> -- 
>> 2.17.1
>>

Re: [PATCH RFC 1/2] Documentation: devicetree: Add property for ignoring the dummy bits sent before read transfer

[Rob Herring]

On Fri, Dec 11, 2020 at 08:34:57PM +0530, Aswath Govindraju wrote:
> Hi,
> On 11/12/20 9:03 am, Rob Herring wrote:
> > On Wed, Dec 09, 2020 at 11:27:07PM +0530, Aswath Govindraju wrote:
> >> Dummy zero bits are sent before data during a read transfer. This causes
> >> the data read to be shifted to the right. To fix this send zero bits after
> >> the address during a read transfer.
> >>
> >> Add property to send zero bits after the address during a read transfer.
> > 
> > When is this necessary? Why can't it be implied by the compatible 
> > string which should be specific to the chip model?
> > 
> 
> This is necessary for 93AA46A/B/C, 93LC46A/B/C, 93C46A/B/C eeproms, as
> it can be seen in section 2.7 of [1]. We were not sure if these were the
> only devices supported by the driver(eeprom_93xx46.c). So, in order to
> apply this only to the above listed devices, we thought that it would be
> better to apply this change when required by introducing a DT property.
> 
> May I know how has this case been handled till now ??
> 

No idea. From the at93c46d (which has a compatible string) datasheet it 
looks like it has the same thing.

> If this is required by all the devices then we can drop the property and
> include the zero bit by default.

Looks like you need a combination of compatible strings for the above 
devices and a property for the ORG pin state on the C devices. I assume 
s/w needs to know if x8 or x16?

Rob

Re: [PATCH RFC 1/2] Documentation: devicetree: Add property for ignoring the dummy bits sent before read transfer

[Aswath Govindraju]

Hi Rob,
On 15/12/20 3:53 am, Rob Herring wrote:
> On Fri, Dec 11, 2020 at 08:34:57PM +0530, Aswath Govindraju wrote:
>> Hi,
>> On 11/12/20 9:03 am, Rob Herring wrote:
>>> On Wed, Dec 09, 2020 at 11:27:07PM +0530, Aswath Govindraju wrote:
>>>> Dummy zero bits are sent before data during a read transfer. This causes
>>>> the data read to be shifted to the right. To fix this send zero bits after
>>>> the address during a read transfer.
>>>>
>>>> Add property to send zero bits after the address during a read transfer.
>>>
>>> When is this necessary? Why can't it be implied by the compatible 
>>> string which should be specific to the chip model?
>>>
>>
>> This is necessary for 93AA46A/B/C, 93LC46A/B/C, 93C46A/B/C eeproms, as
>> it can be seen in section 2.7 of [1]. We were not sure if these were the
>> only devices supported by the driver(eeprom_93xx46.c). So, in order to
>> apply this only to the above listed devices, we thought that it would be
>> better to apply this change when required by introducing a DT property.
>>
>> May I know how has this case been handled till now ??
>>
> 
> No idea. From the at93c46d (which has a compatible string) datasheet it 
> looks like it has the same thing.
> 
>> If this is required by all the devices then we can drop the property and
>> include the zero bit by default.
> 
> Looks like you need a combination of compatible strings for the above  
> devices and a property for the ORG pin state on the C devices. I assume 
> s/w needs to know if x8 or x16?
> 
Yes, there are separate properties for indicating different types of
types of eeproms.

So, do you think that it is better to add it as a seperate property??

Thanks,
Aswath
> Rob
> 

Re: [PATCH RFC 1/2] Documentation: devicetree: Add property for ignoring the dummy bits sent before read transfer

[Aswath Govindraju]

Hi Rob,

On 15/12/20 9:42 pm, Aswath Govindraju wrote:
> Hi Rob,
> On 15/12/20 3:53 am, Rob Herring wrote:
>> On Fri, Dec 11, 2020 at 08:34:57PM +0530, Aswath Govindraju wrote:
>>> Hi,
>>> On 11/12/20 9:03 am, Rob Herring wrote:
>>>> On Wed, Dec 09, 2020 at 11:27:07PM +0530, Aswath Govindraju wrote:
>>>>> Dummy zero bits are sent before data during a read transfer. This causes
>>>>> the data read to be shifted to the right. To fix this send zero bits after
>>>>> the address during a read transfer.
>>>>>
>>>>> Add property to send zero bits after the address during a read transfer.
>>>>
>>>> When is this necessary? Why can't it be implied by the compatible 
>>>> string which should be specific to the chip model?
>>>>
>>>
>>> This is necessary for 93AA46A/B/C, 93LC46A/B/C, 93C46A/B/C eeproms, as
>>> it can be seen in section 2.7 of [1]. We were not sure if these were the
>>> only devices supported by the driver(eeprom_93xx46.c). So, in order to
>>> apply this only to the above listed devices, we thought that it would be
>>> better to apply this change when required by introducing a DT property.
>>>
>>> May I know how has this case been handled till now ??
>>>
>>
>> No idea. From the at93c46d (which has a compatible string) datasheet it 
>> looks like it has the same thing.
>>
>>> If this is required by all the devices then we can drop the property and
>>> include the zero bit by default.
>>
>> Looks like you need a combination of compatible strings for the above  
>> devices and a property for the ORG pin state on the C devices. I assume 
>> s/w needs to know if x8 or x16?
>>
> Yes, there are separate properties for indicating different types of
> types of eeproms.
> 

Here I was saying about x8 or x16 using the data-size property. ORG pin
state is implied through data-size property and an additional property
is not required for ORG pin state.

> So, do you think that it is better to add it as a seperate property??
> 


These are the available options to my knowledge,

1) As you mentioned earlier all the eeprom's supported by the driver
send a dummy bit before the read data. This can be thought of a bug and
add this change as a fix for it. This might a problem for users who are
already using this driver and working around it using user space tools.

2) Add a special compatible string "eeprom-93xx46B", to add the extra
dummy cycle and not add an additional property.

3) Add an additional property as proposed in this patch and use when
required.

Are there any other suggestions on solving this issue??

Thanks,
Aswath

Re: [PATCH RFC 1/2] Documentation: devicetree: Add property for ignoring the dummy bits sent before read transfer

[Rob Herring]

On Thu, Dec 17, 2020 at 7:48 AM Aswath Govindraju <a-govindraju@ti.com> wrote:
>
> Hi Rob,
>
> On 15/12/20 9:42 pm, Aswath Govindraju wrote:
> > Hi Rob,
> > On 15/12/20 3:53 am, Rob Herring wrote:
> >> On Fri, Dec 11, 2020 at 08:34:57PM +0530, Aswath Govindraju wrote:
> >>> Hi,
> >>> On 11/12/20 9:03 am, Rob Herring wrote:
> >>>> On Wed, Dec 09, 2020 at 11:27:07PM +0530, Aswath Govindraju wrote:
> >>>>> Dummy zero bits are sent before data during a read transfer. This causes
> >>>>> the data read to be shifted to the right. To fix this send zero bits after
> >>>>> the address during a read transfer.
> >>>>>
> >>>>> Add property to send zero bits after the address during a read transfer.
> >>>>
> >>>> When is this necessary? Why can't it be implied by the compatible
> >>>> string which should be specific to the chip model?
> >>>>
> >>>
> >>> This is necessary for 93AA46A/B/C, 93LC46A/B/C, 93C46A/B/C eeproms, as
> >>> it can be seen in section 2.7 of [1]. We were not sure if these were the
> >>> only devices supported by the driver(eeprom_93xx46.c). So, in order to
> >>> apply this only to the above listed devices, we thought that it would be
> >>> better to apply this change when required by introducing a DT property.
> >>>
> >>> May I know how has this case been handled till now ??
> >>>
> >>
> >> No idea. From the at93c46d (which has a compatible string) datasheet it
> >> looks like it has the same thing.
> >>
> >>> If this is required by all the devices then we can drop the property and
> >>> include the zero bit by default.
> >>
> >> Looks like you need a combination of compatible strings for the above
> >> devices and a property for the ORG pin state on the C devices. I assume
> >> s/w needs to know if x8 or x16?
> >>
> > Yes, there are separate properties for indicating different types of
> > types of eeproms.
> >
>
> Here I was saying about x8 or x16 using the data-size property. ORG pin
> state is implied through data-size property and an additional property
> is not required for ORG pin state.

Ah, I missed that property.

>
> > So, do you think that it is better to add it as a seperate property??
> >
>
>
> These are the available options to my knowledge,
>
> 1) As you mentioned earlier all the eeprom's supported by the driver
> send a dummy bit before the read data. This can be thought of a bug and
> add this change as a fix for it. This might a problem for users who are
> already using this driver and working around it using user space tools.
>
> 2) Add a special compatible string "eeprom-93xx46B", to add the extra
> dummy cycle and not add an additional property.

No. Genericish compatible strings are what cause the problem and this
whole discussion.

> 3) Add an additional property as proposed in this patch and use when
> required.
>
> Are there any other suggestions on solving this issue??

You need a compatible string for each vendor+model. Period.

Rob

Re: [PATCH RFC 1/2] Documentation: devicetree: Add property for ignoring the dummy bits sent before read transfer

[Aswath Govindraju]

Hi Rob,

On 17/12/20 9:18 pm, Rob Herring wrote:
> On Thu, Dec 17, 2020 at 7:48 AM Aswath Govindraju <a-govindraju@ti.com> wrote:
>>
>> Hi Rob,
>>
>> On 15/12/20 9:42 pm, Aswath Govindraju wrote:
>>> Hi Rob,
>>> On 15/12/20 3:53 am, Rob Herring wrote:
>>>> On Fri, Dec 11, 2020 at 08:34:57PM +0530, Aswath Govindraju wrote:
>>>>> Hi,
>>>>> On 11/12/20 9:03 am, Rob Herring wrote:
>>>>>> On Wed, Dec 09, 2020 at 11:27:07PM +0530, Aswath Govindraju wrote:
>>>>>>> Dummy zero bits are sent before data during a read transfer. This causes
>>>>>>> the data read to be shifted to the right. To fix this send zero bits after
>>>>>>> the address during a read transfer.
>>>>>>>
>>>>>>> Add property to send zero bits after the address during a read transfer.
>>>>>>
>>>>>> When is this necessary? Why can't it be implied by the compatible
>>>>>> string which should be specific to the chip model?
>>>>>>
>>>>>
>>>>> This is necessary for 93AA46A/B/C, 93LC46A/B/C, 93C46A/B/C eeproms, as
>>>>> it can be seen in section 2.7 of [1]. We were not sure if these were the
>>>>> only devices supported by the driver(eeprom_93xx46.c). So, in order to
>>>>> apply this only to the above listed devices, we thought that it would be
>>>>> better to apply this change when required by introducing a DT property.
>>>>>
>>>>> May I know how has this case been handled till now ??
>>>>>
>>>>
>>>> No idea. From the at93c46d (which has a compatible string) datasheet it
>>>> looks like it has the same thing.
>>>>
>>>>> If this is required by all the devices then we can drop the property and
>>>>> include the zero bit by default.
>>>>
>>>> Looks like you need a combination of compatible strings for the above
>>>> devices and a property for the ORG pin state on the C devices. I assume
>>>> s/w needs to know if x8 or x16?
>>>>
>>> Yes, there are separate properties for indicating different types of
>>> types of eeproms.
>>>
>>
>> Here I was saying about x8 or x16 using the data-size property. ORG pin
>> state is implied through data-size property and an additional property
>> is not required for ORG pin state.
> 
> Ah, I missed that property.
> 
>>
>>> So, do you think that it is better to add it as a seperate property??
>>>
>>
>>
>> These are the available options to my knowledge,
>>
>> 1) As you mentioned earlier all the eeprom's supported by the driver
>> send a dummy bit before the read data. This can be thought of a bug and
>> add this change as a fix for it. This might a problem for users who are
>> already using this driver and working around it using user space tools.
>>
>> 2) Add a special compatible string "eeprom-93xx46B", to add the extra
>> dummy cycle and not add an additional property.
> 
> No. Genericish compatible strings are what cause the problem and this
> whole discussion.
> 
>> 3) Add an additional property as proposed in this patch and use when
>> required.
>>
>> Are there any other suggestions on solving this issue??
> 
> You need a compatible string for each vendor+model. Period.
>

Thank you for the comments.

This change is required for microchip "93LC46B" model . I will add a new
compatible string "microchip,93LC46B" and use it to implement the driver
changes.

Thanks,
Aswath

[PATCH RFC 2/2] eeprom: eeprom_93xx46: Add support for ignoring the dummy bit preceding data during read transfer

[Aswath Govindraju]

A dummy zero bit is sent by eeprom preceding the data of every read
transfer. This results in right shift of data during a read. In order to
ignore the dummy bits preceding the data, extra zero bits are transferred
after the read address.

This feature can be added by including the property read-op-dummy-cycles
with the number of zero bits to be transferred as the value, in device tree
node.

Fix read by sending extra zero bits after the read address to ignore the
zero bits sent by eeprom before data.

Suggested-by: Vignesh Raghavendra <vigneshr@ti.com>
Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 drivers/misc/eeprom/eeprom_93xx46.c | 23 +++++++++++++++++++++++
 include/linux/eeprom_93xx46.h       |  6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index 7c45f82b4302..e778ae54a6f1 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -55,6 +55,16 @@ static inline bool has_quirk_instruction_length(struct eeprom_93xx46_dev *edev)
 	return edev->pdata->quirks & EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH;
 }
 
+static inline bool needs_extra_read_cycle(struct eeprom_93xx46_dev *edev)
+{
+	return edev->pdata->flags & EE_EXTRA_CYCLE_READ;
+}
+
+static inline u32 get_read_op_dummy_cycles(struct eeprom_93xx46_dev *edev)
+{
+	return edev->pdata->read_op_dummy_cycles;
+}
+
 static int eeprom_93xx46_read(void *priv, unsigned int off,
 			      void *val, size_t count)
 {
@@ -80,6 +90,7 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
 		u16 cmd_addr = OP_READ << edev->addrlen;
 		size_t nbytes = count;
 		int bits;
+		u32 read_op_dummy_cycles;
 
 		if (edev->addrlen == 7) {
 			cmd_addr |= off & 0x7f;
@@ -93,6 +104,12 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
 				nbytes = 2;
 		}
 
+		if (needs_extra_read_cycle(edev)) {
+			read_op_dummy_cycles = get_read_op_dummy_cycles(edev);
+			cmd_addr = cmd_addr << read_op_dummy_cycles;
+			bits += read_op_dummy_cycles;
+		}
+
 		dev_dbg(&edev->spi->dev, "read cmd 0x%x, %d Hz\n",
 			cmd_addr, edev->spi->max_speed_hz);
 
@@ -398,6 +415,12 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
 	if (of_property_read_bool(np, "read-only"))
 		pd->flags |= EE_READONLY;
 
+	ret = of_property_read_u32(np, "read-op-dummy-cycles", &tmp);
+	if (ret == 0) {
+		pd->flags |= EE_EXTRA_CYCLE_READ;
+		pd->read_op_dummy_cycles = tmp;
+	}
+
 	pd->select = devm_gpiod_get_optional(&spi->dev, "select",
 					     GPIOD_OUT_LOW);
 	if (IS_ERR(pd->select))
diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
index eec7928ff8fe..f0d37e921aba 100644
--- a/include/linux/eeprom_93xx46.h
+++ b/include/linux/eeprom_93xx46.h
@@ -11,6 +11,12 @@ struct eeprom_93xx46_platform_data {
 #define EE_ADDR16	0x02		/* 16 bit addr. cfg */
 #define EE_READONLY	0x08		/* forbid writing */
 
+/* Add extra zero bits of data after the address during read transfer
+ * to ignore the dummy bits sent before data
+ */
+#define EE_EXTRA_CYCLE_READ 0x04
+	u32 read_op_dummy_cycles;
+
 	unsigned int	quirks;
 /* Single word read transfers only; no sequential read. */
 #define EEPROM_93XX46_QUIRK_SINGLE_WORD_READ		(1 << 0)
-- 
2.17.1