[PATCH v3 0/1] at24: support eeproms that do not auto-rollover reads.

[PATCH v3 0/1] at24: support eeproms that do not auto-rollover reads.

[Sven Van Asbroeck]

From: Sven Van Asbroeck <svenv@arcx.com>

v3:
	rebased against at24 maintainer's devel staging branch:
	git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git at24/devel
	clarified some of the comments and wording

v2:
	kbuild test robot feedback: correct
		"warning: comparison of distinct pointer types lacks a cast"
	build warning on some compilers / architectures.

v1:
	original patch

Sven Van Asbroeck (1):
  at24: support eeproms that do not auto-rollover reads.

 .../devicetree/bindings/eeprom/eeprom.txt          |  5 +++
 drivers/misc/eeprom/at24.c                         | 37 +++++++++++++++-------
 include/linux/platform_data/at24.h                 |  2 ++
 3 files changed, 32 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH v3 1/1] at24: support eeproms that do not auto-rollover reads.

[Sven Van Asbroeck]

From: Sven Van Asbroeck <svenv@arcx.com>

Some multi-address eeproms in the at24 family may not automatically
roll-over reads to the next slave address. On those eeproms, reads
that straddle slave boundaries will not work correctly.

Solution:
Mark such eeproms with a flag that prevents reads straddling
slave boundaries. Add the AT24_FLAG_NO_RDROL flag to the eeprom
entry in the device_id table, or add 'no-read-rollover' to the
eeprom devicetree entry.

Note that I have not personally enountered an at24 chip that
does not support read rollovers. They may or may not exist.
However, my hardware requires this functionality because of
a quirk.

It's up to the Linux community to decide if this patch is useful/
general enough to warrant merging.

Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
---
 .../devicetree/bindings/eeprom/eeprom.txt          |  5 +++
 drivers/misc/eeprom/at24.c                         | 37 +++++++++++++++-------
 include/linux/platform_data/at24.h                 |  2 ++
 3 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/eeprom/eeprom.txt b/Documentation/devicetree/bindings/eeprom/eeprom.txt
index 27f2bc1..a1764f8 100644
--- a/Documentation/devicetree/bindings/eeprom/eeprom.txt
+++ b/Documentation/devicetree/bindings/eeprom/eeprom.txt
@@ -38,6 +38,11 @@ Optional properties:
 
   - size: total eeprom size in bytes
 
+  - no-read-rollover: supported on the at24 eeprom family only.
+			This parameterless property indicates that the multi-address
+			eeprom does not automatically roll over reads to the next
+			slave address. Please consult the manual of your device.
+
 Example:
 
 eeprom@52 {
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 625b001..33bca28 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -251,15 +251,6 @@ struct at24_data {
  * Slave address and byte offset derive from the offset. Always
  * set the byte address; on a multi-master board, another master
  * may have changed the chip's "current" address pointer.
- *
- * REVISIT some multi-address chips don't rollover page reads to
- * the next slave address, so we may need to truncate the count.
- * Those chips might need another quirk flag.
- *
- * If the real hardware used four adjacent 24c02 chips and that
- * were misconfigured as one 24c08, that would be a similar effect:
- * one "eeprom" file not four, but larger reads would fail when
- * they crossed certain pages.
  */
 static struct at24_client *at24_translate_offset(struct at24_data *at24,
 						 unsigned int *offset)
@@ -277,6 +268,28 @@ static struct at24_client *at24_translate_offset(struct at24_data *at24,
 	return &at24->client[i];
 }
 
+static size_t at24_adjust_read_count(struct at24_data *at24,
+				      unsigned int offset, size_t count)
+{
+	unsigned int bits;
+	size_t remainder;
+	/*
+	 * In case of multi-address chips that don't rollover reads to
+	 * the next slave address: truncate the count to the slave boundary,
+	 * so that the read never straddles slaves.
+	 */
+	if (at24->chip.flags & AT24_FLAG_NO_RDROL) {
+		bits = (at24->chip.flags & AT24_FLAG_ADDR16) ? 16 : 8;
+		remainder = BIT(bits) - offset;
+		if (count > remainder)
+			count = remainder;
+	}
+	if (count > io_limit)
+		count = io_limit;
+
+	return count;
+}
+
 static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
 				unsigned int offset, size_t count)
 {
@@ -289,9 +302,7 @@ static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
 	at24_client = at24_translate_offset(at24, &offset);
 	regmap = at24_client->regmap;
 	client = at24_client->client;
-
-	if (count > io_limit)
-		count = io_limit;
+	count = at24_adjust_read_count(at24, offset, count);
 
 	/* adjust offset for mac and serial read ops */
 	offset += at24->offset_adj;
@@ -457,6 +468,8 @@ static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
 
 	if (device_property_present(dev, "read-only"))
 		chip->flags |= AT24_FLAG_READONLY;
+	if (device_property_present(dev, "no-read-rollover"))
+		chip->flags |= AT24_FLAG_NO_RDROL;
 
 	err = device_property_read_u32(dev, "size", &val);
 	if (!err)
diff --git a/include/linux/platform_data/at24.h b/include/linux/platform_data/at24.h
index 271a4e2..841bb28 100644
--- a/include/linux/platform_data/at24.h
+++ b/include/linux/platform_data/at24.h
@@ -50,6 +50,8 @@ struct at24_platform_data {
 #define AT24_FLAG_TAKE8ADDR	BIT(4)	/* take always 8 addresses (24c00) */
 #define AT24_FLAG_SERIAL	BIT(3)	/* factory-programmed serial number */
 #define AT24_FLAG_MAC		BIT(2)	/* factory-programmed mac address */
+#define AT24_FLAG_NO_RDROL  BIT(1)	/* does not auto-rollover reads to */
+					/* the next slave address */
 
 	void		(*setup)(struct nvmem_device *nvmem, void *context);
 	void		*context;
-- 
1.9.1

Re: [PATCH v3 1/1] at24: support eeproms that do not auto-rollover reads.

[Sakari Ailus]

Hi Sven,

On Mon, Dec 04, 2017 at 10:36:18AM -0500, Sven Van Asbroeck wrote:
> From: Sven Van Asbroeck <svenv-fuHqz3Nb1YI@public.gmane.org>
> 
> Some multi-address eeproms in the at24 family may not automatically
> roll-over reads to the next slave address. On those eeproms, reads
> that straddle slave boundaries will not work correctly.
> 
> Solution:
> Mark such eeproms with a flag that prevents reads straddling
> slave boundaries. Add the AT24_FLAG_NO_RDROL flag to the eeprom
> entry in the device_id table, or add 'no-read-rollover' to the
> eeprom devicetree entry.
> 
> Note that I have not personally enountered an at24 chip that
> does not support read rollovers. They may or may not exist.
> However, my hardware requires this functionality because of
> a quirk.
> 
> It's up to the Linux community to decide if this patch is useful/
> general enough to warrant merging.
> 
> Signed-off-by: Sven Van Asbroeck <svendev-fuHqz3Nb1YI@public.gmane.org>
> ---
>  .../devicetree/bindings/eeprom/eeprom.txt          |  5 +++
>  drivers/misc/eeprom/at24.c                         | 37 +++++++++++++++-------
>  include/linux/platform_data/at24.h                 |  2 ++
>  3 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/eeprom/eeprom.txt b/Documentation/devicetree/bindings/eeprom/eeprom.txt
> index 27f2bc1..a1764f8 100644
> --- a/Documentation/devicetree/bindings/eeprom/eeprom.txt
> +++ b/Documentation/devicetree/bindings/eeprom/eeprom.txt
> @@ -38,6 +38,11 @@ Optional properties:
>  
>    - size: total eeprom size in bytes
>  
> +  - no-read-rollover: supported on the at24 eeprom family only.

If this is truly specific to at24, then vendor prefix would be appropriate,
plus it'd go to an at24 specific binding file. However if it isn't I'd just
remove the above sentence. I guess the latter?

Binding changes would be nicer in a separate patch, too.

> +			This parameterless property indicates that the multi-address
> +			eeprom does not automatically roll over reads to the next
> +			slave address. Please consult the manual of your device.
> +
>  Example:
>  
>  eeprom@52 {
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 625b001..33bca28 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -251,15 +251,6 @@ struct at24_data {
>   * Slave address and byte offset derive from the offset. Always
>   * set the byte address; on a multi-master board, another master
>   * may have changed the chip's "current" address pointer.
> - *
> - * REVISIT some multi-address chips don't rollover page reads to
> - * the next slave address, so we may need to truncate the count.
> - * Those chips might need another quirk flag.
> - *
> - * If the real hardware used four adjacent 24c02 chips and that
> - * were misconfigured as one 24c08, that would be a similar effect:
> - * one "eeprom" file not four, but larger reads would fail when
> - * they crossed certain pages.
>   */
>  static struct at24_client *at24_translate_offset(struct at24_data *at24,
>  						 unsigned int *offset)
> @@ -277,6 +268,28 @@ static struct at24_client *at24_translate_offset(struct at24_data *at24,
>  	return &at24->client[i];
>  }
>  
> +static size_t at24_adjust_read_count(struct at24_data *at24,
> +				      unsigned int offset, size_t count)
> +{
> +	unsigned int bits;
> +	size_t remainder;
> +	/*
> +	 * In case of multi-address chips that don't rollover reads to
> +	 * the next slave address: truncate the count to the slave boundary,
> +	 * so that the read never straddles slaves.
> +	 */
> +	if (at24->chip.flags & AT24_FLAG_NO_RDROL) {
> +		bits = (at24->chip.flags & AT24_FLAG_ADDR16) ? 16 : 8;
> +		remainder = BIT(bits) - offset;
> +		if (count > remainder)
> +			count = remainder;
> +	}
> +	if (count > io_limit)
> +		count = io_limit;
> +
> +	return count;
> +}
> +
>  static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
>  				unsigned int offset, size_t count)
>  {
> @@ -289,9 +302,7 @@ static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
>  	at24_client = at24_translate_offset(at24, &offset);
>  	regmap = at24_client->regmap;
>  	client = at24_client->client;
> -
> -	if (count > io_limit)
> -		count = io_limit;
> +	count = at24_adjust_read_count(at24, offset, count);
>  
>  	/* adjust offset for mac and serial read ops */
>  	offset += at24->offset_adj;
> @@ -457,6 +468,8 @@ static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
>  
>  	if (device_property_present(dev, "read-only"))
>  		chip->flags |= AT24_FLAG_READONLY;
> +	if (device_property_present(dev, "no-read-rollover"))
> +		chip->flags |= AT24_FLAG_NO_RDROL;
>  
>  	err = device_property_read_u32(dev, "size", &val);
>  	if (!err)
> diff --git a/include/linux/platform_data/at24.h b/include/linux/platform_data/at24.h
> index 271a4e2..841bb28 100644
> --- a/include/linux/platform_data/at24.h
> +++ b/include/linux/platform_data/at24.h
> @@ -50,6 +50,8 @@ struct at24_platform_data {
>  #define AT24_FLAG_TAKE8ADDR	BIT(4)	/* take always 8 addresses (24c00) */
>  #define AT24_FLAG_SERIAL	BIT(3)	/* factory-programmed serial number */
>  #define AT24_FLAG_MAC		BIT(2)	/* factory-programmed mac address */
> +#define AT24_FLAG_NO_RDROL  BIT(1)	/* does not auto-rollover reads to */
> +					/* the next slave address */
>  
>  	void		(*setup)(struct nvmem_device *nvmem, void *context);
>  	void		*context;
> -- 
> 1.9.1
> 

-- 
Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] at24: support eeproms that do not auto-rollover reads.

[Sven Van Asbroeck]

> If this is truly specific to at24, then vendor prefix would be appropriate,
> plus it'd go to an at24 specific binding file. However if it isn't I'd just
> remove the above sentence. I guess the latter?

Yes, no-read-rollover is truly specific to at24.c, because it applies only
to i2c multi-address chips. The at25 is spi based so cannot have multiple
addresses.

So yes, "at24,no-read-rollover" would perhaps be a better name.

Regarding an at24 specific binding file. You're saying I should create
Documentation/devicetree/bindings/eeprom/at24.txt ? Should I indicate
that at24.txt "inherits from" eeprom.txt? Note that at25.txt does not
currently do this.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] at24: support eeproms that do not auto-rollover reads.

[Sakari Ailus]

On Mon, Dec 04, 2017 at 05:24:33PM -0500, Sven Van Asbroeck wrote:
> > If this is truly specific to at24, then vendor prefix would be appropriate,
> > plus it'd go to an at24 specific binding file. However if it isn't I'd just
> > remove the above sentence. I guess the latter?
> 
> Yes, no-read-rollover is truly specific to at24.c, because it applies only
> to i2c multi-address chips. The at25 is spi based so cannot have multiple
> addresses.
> 
> So yes, "at24,no-read-rollover" would perhaps be a better name.
> 
> Regarding an at24 specific binding file. You're saying I should create
> Documentation/devicetree/bindings/eeprom/at24.txt ? Should I indicate
> that at24.txt "inherits from" eeprom.txt? Note that at25.txt does not
> currently do this.

Hmm. I actually missed we didn't have one to begin with. at25.txt exists
and it documents a number of properties specific to at25, so if at24 will
have an at24-specific property, then I think it should go to a separate
file.

Aren't there really other chips which need this? It'd be (a little bit)
easier to just remove the sentence. :-)

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v3 1/1] at24: support eeproms that do not auto-rollover reads.

[Bartosz Golaszewski]

2017-12-05 8:44 GMT+01:00 Sakari Ailus <sakari.ailus@linux.intel.com>:
> On Mon, Dec 04, 2017 at 05:24:33PM -0500, Sven Van Asbroeck wrote:
>> > If this is truly specific to at24, then vendor prefix would be appropriate,
>> > plus it'd go to an at24 specific binding file. However if it isn't I'd just
>> > remove the above sentence. I guess the latter?
>>
>> Yes, no-read-rollover is truly specific to at24.c, because it applies only
>> to i2c multi-address chips. The at25 is spi based so cannot have multiple
>> addresses.
>>
>> So yes, "at24,no-read-rollover" would perhaps be a better name.
>>
>> Regarding an at24 specific binding file. You're saying I should create
>> Documentation/devicetree/bindings/eeprom/at24.txt ? Should I indicate
>> that at24.txt "inherits from" eeprom.txt? Note that at25.txt does not
>> currently do this.
>
> Hmm. I actually missed we didn't have one to begin with. at25.txt exists
> and it documents a number of properties specific to at25, so if at24 will
> have an at24-specific property, then I think it should go to a separate
> file.

The eeprom.txt file in the bindings directory actually describes the
bindings for at24. There's a patch[1] from Wolfram waiting for Rob's
ack that renames it to at24.txt. I hope that clears any confusion.

@Sven: please split the patch into two: one for bindings and one for code.

As for the name: I would change it to at24,no-read-rollover and remove
the fragment saying it's only supported in at24 - as I said: this file
only concerns at24 and will be renamed.

>
> Aren't there really other chips which need this? It'd be (a little bit)
> easier to just remove the sentence. :-)
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

Thanks,
Bartosz

[1] http://patchwork.ozlabs.org/patch/842500/

Re: [PATCH v3 1/1] at24: support eeproms that do not auto-rollover reads.

[Rob Herring]

On Tue, Dec 05, 2017 at 09:14:22AM +0100, Bartosz Golaszewski wrote:
> 2017-12-05 8:44 GMT+01:00 Sakari Ailus <sakari.ailus@linux.intel.com>:
> > On Mon, Dec 04, 2017 at 05:24:33PM -0500, Sven Van Asbroeck wrote:
> >> > If this is truly specific to at24, then vendor prefix would be appropriate,
> >> > plus it'd go to an at24 specific binding file. However if it isn't I'd just
> >> > remove the above sentence. I guess the latter?
> >>
> >> Yes, no-read-rollover is truly specific to at24.c, because it applies only
> >> to i2c multi-address chips. The at25 is spi based so cannot have multiple
> >> addresses.
> >>
> >> So yes, "at24,no-read-rollover" would perhaps be a better name.
> >>
> >> Regarding an at24 specific binding file. You're saying I should create
> >> Documentation/devicetree/bindings/eeprom/at24.txt ? Should I indicate
> >> that at24.txt "inherits from" eeprom.txt? Note that at25.txt does not
> >> currently do this.
> >
> > Hmm. I actually missed we didn't have one to begin with. at25.txt exists
> > and it documents a number of properties specific to at25, so if at24 will
> > have an at24-specific property, then I think it should go to a separate
> > file.
> 
> The eeprom.txt file in the bindings directory actually describes the
> bindings for at24. There's a patch[1] from Wolfram waiting for Rob's
> ack that renames it to at24.txt. I hope that clears any confusion.

It's going to wait forever until it is sent to the DT list so 
patchwork picks it up and is in my queue.

> @Sven: please split the patch into two: one for bindings and one for code.
> 
> As for the name: I would change it to at24,no-read-rollover and remove

at24 is not a vendor.

> the fragment saying it's only supported in at24 - as I said: this file
> only concerns at24 and will be renamed.
> 
> >
> > Aren't there really other chips which need this? It'd be (a little bit)
> > easier to just remove the sentence. :-)
> >
> > --
> > Regards,
> >
> > Sakari Ailus
> > sakari.ailus@linux.intel.com
> 
> Thanks,
> Bartosz
> 
> [1] http://patchwork.ozlabs.org/patch/842500/