[04/20] W1-EEPROM: Add an W1-EEPROM uclass for 1 wire EEPROMs

Message ID 1531994288-19423-5-git-send-email-eugen.hristev@microchip.com
State New
Headers show
Series
  • [01/20] w1: Add 1-Wire uclass
Related show

Commit Message

Eugen Hristev July 19, 2018, 9:57 a.m.
From: Maxime Ripard <maxime.ripard@free-electrons.com>

We might want to access data stored onto one wire EEPROMs.
Create a framework to provide a consistent API.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
[eugen.hristev@microchip.com: reworked patch]
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/Kconfig                      |  2 ++
 drivers/Makefile                     |  1 +
 drivers/w1-eeprom/Kconfig            | 17 +++++++++++
 drivers/w1-eeprom/Makefile           |  2 ++
 drivers/w1-eeprom/w1-eeprom-uclass.c | 56 ++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h               |  1 +
 include/w1-eeprom.h                  | 28 ++++++++++++++++++
 7 files changed, 107 insertions(+)
 create mode 040000 drivers/w1-eeprom
 create mode 100644 drivers/w1-eeprom/Kconfig
 create mode 100644 drivers/w1-eeprom/Makefile
 create mode 100644 drivers/w1-eeprom/w1-eeprom-uclass.c
 create mode 100644 include/w1-eeprom.h

Comments

Maxime Ripard July 20, 2018, 2:28 p.m. | #1
Hi Eugen,

Thanks for giving those patches another shot.

On Thu, Jul 19, 2018 at 12:57:52PM +0300, Eugen Hristev wrote:
> From: Maxime Ripard <maxime.ripard@free-electrons.com>

> 

> We might want to access data stored onto one wire EEPROMs.

> Create a framework to provide a consistent API.

> 

> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

> [eugen.hristev@microchip.com: reworked patch]

> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

> ---

>  drivers/Kconfig                      |  2 ++

>  drivers/Makefile                     |  1 +

>  drivers/w1-eeprom/Kconfig            | 17 +++++++++++

>  drivers/w1-eeprom/Makefile           |  2 ++

>  drivers/w1-eeprom/w1-eeprom-uclass.c | 56 ++++++++++++++++++++++++++++++++++++

>  include/dm/uclass-id.h               |  1 +

>  include/w1-eeprom.h                  | 28 ++++++++++++++++++

>  7 files changed, 107 insertions(+)

>  create mode 040000 drivers/w1-eeprom

>  create mode 100644 drivers/w1-eeprom/Kconfig

>  create mode 100644 drivers/w1-eeprom/Makefile

>  create mode 100644 drivers/w1-eeprom/w1-eeprom-uclass.c

>  create mode 100644 include/w1-eeprom.h


I believe that we shouldn't have a framework solely for 1-wire
EEPROMs, but for EEPROMs, connected to any bus.

The 1-Wire EEPROMs all behave pretty much the same, so we'll probably
only see a single driver within that framework. And at the same time,
we'll want to have a consistent interface to access all the EEPROMs,
no matter on which bus they sit on.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Eugen Hristev July 30, 2018, 8:54 a.m. | #2
On 20.07.2018 17:28, Maxime Ripard wrote:
> Hi Eugen,
> 
> Thanks for giving those patches another shot.
> 
> On Thu, Jul 19, 2018 at 12:57:52PM +0300, Eugen Hristev wrote:
>> From: Maxime Ripard <maxime.ripard@free-electrons.com>
>>
>> We might want to access data stored onto one wire EEPROMs.
>> Create a framework to provide a consistent API.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> [eugen.hristev@microchip.com: reworked patch]
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>>   drivers/Kconfig                      |  2 ++
>>   drivers/Makefile                     |  1 +
>>   drivers/w1-eeprom/Kconfig            | 17 +++++++++++
>>   drivers/w1-eeprom/Makefile           |  2 ++
>>   drivers/w1-eeprom/w1-eeprom-uclass.c | 56 ++++++++++++++++++++++++++++++++++++
>>   include/dm/uclass-id.h               |  1 +
>>   include/w1-eeprom.h                  | 28 ++++++++++++++++++
>>   7 files changed, 107 insertions(+)
>>   create mode 040000 drivers/w1-eeprom
>>   create mode 100644 drivers/w1-eeprom/Kconfig
>>   create mode 100644 drivers/w1-eeprom/Makefile
>>   create mode 100644 drivers/w1-eeprom/w1-eeprom-uclass.c
>>   create mode 100644 include/w1-eeprom.h
> 
> I believe that we shouldn't have a framework solely for 1-wire
> EEPROMs, but for EEPROMs, connected to any bus.
> 
> The 1-Wire EEPROMs all behave pretty much the same, so we'll probably
> only see a single driver within that framework. And at the same time,
> we'll want to have a consistent interface to access all the EEPROMs,
> no matter on which bus they sit on.

Hello,

I worked this series using the original implementation as a starting 
point: having the eeproms on different subsystems (i2c and onewire).

The different types of eeproms have only the name in common as I see it, 
and the way to access them is totally different: two different type of 
buses, so uniting them is just for the namesake ?

One option is to have them separately, as we have spi, i2c memories , etc;
Or, unite them under a single subsystem for eeproms, and have one driver 
for i2c eeproms and one for w1 eeproms, trying to make the same API to 
access them, and hide the bus specific differences.

Question for maintainers: which is the best direction to go, so I can 
rework the series accordingly ?

Thanks,
Eugen
> 
> Maxime
>
Tom Rini July 31, 2018, 2:06 a.m. | #3
On Mon, Jul 30, 2018 at 11:54:51AM +0300, Eugen Hristev wrote:
> 

> 

> On 20.07.2018 17:28, Maxime Ripard wrote:

> >Hi Eugen,

> >

> >Thanks for giving those patches another shot.

> >

> >On Thu, Jul 19, 2018 at 12:57:52PM +0300, Eugen Hristev wrote:

> >>From: Maxime Ripard <maxime.ripard@free-electrons.com>

> >>

> >>We might want to access data stored onto one wire EEPROMs.

> >>Create a framework to provide a consistent API.

> >>

> >>Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

> >>[eugen.hristev@microchip.com: reworked patch]

> >>Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

> >>---

> >>  drivers/Kconfig                      |  2 ++

> >>  drivers/Makefile                     |  1 +

> >>  drivers/w1-eeprom/Kconfig            | 17 +++++++++++

> >>  drivers/w1-eeprom/Makefile           |  2 ++

> >>  drivers/w1-eeprom/w1-eeprom-uclass.c | 56 ++++++++++++++++++++++++++++++++++++

> >>  include/dm/uclass-id.h               |  1 +

> >>  include/w1-eeprom.h                  | 28 ++++++++++++++++++

> >>  7 files changed, 107 insertions(+)

> >>  create mode 040000 drivers/w1-eeprom

> >>  create mode 100644 drivers/w1-eeprom/Kconfig

> >>  create mode 100644 drivers/w1-eeprom/Makefile

> >>  create mode 100644 drivers/w1-eeprom/w1-eeprom-uclass.c

> >>  create mode 100644 include/w1-eeprom.h

> >

> >I believe that we shouldn't have a framework solely for 1-wire

> >EEPROMs, but for EEPROMs, connected to any bus.

> >

> >The 1-Wire EEPROMs all behave pretty much the same, so we'll probably

> >only see a single driver within that framework. And at the same time,

> >we'll want to have a consistent interface to access all the EEPROMs,

> >no matter on which bus they sit on.

> 

> Hello,

> 

> I worked this series using the original implementation as a starting point:

> having the eeproms on different subsystems (i2c and onewire).

> 

> The different types of eeproms have only the name in common as I see it, and

> the way to access them is totally different: two different type of buses, so

> uniting them is just for the namesake ?

> 

> One option is to have them separately, as we have spi, i2c memories , etc;

> Or, unite them under a single subsystem for eeproms, and have one driver for

> i2c eeproms and one for w1 eeproms, trying to make the same API to access

> them, and hide the bus specific differences.

> 

> Question for maintainers: which is the best direction to go, so I can rework

> the series accordingly ?


It would be nice if we had a generic eeprom command that wasn't
bus-centric.  Unfortunately we have an eeprom command that IS bus
centric and not easily extended to working on all appropriate buses.  So
to me, starting out by handing w1 eeproms under w1 seems OK.  And we can
put it on the TODO list to make cmd/eeprom.c parse <bus> as perhaps
"BUSTYPE:number" so we could do eeprom w1:0 ... or eeprom i2c:0 ... and
so forth.

-- 
Tom
Simon Glass Aug. 2, 2018, 4:56 p.m. | #4
Hi,

On 30 July 2018 at 20:06, Tom Rini <trini@konsulko.com> wrote:
> On Mon, Jul 30, 2018 at 11:54:51AM +0300, Eugen Hristev wrote:
>>
>>
>> On 20.07.2018 17:28, Maxime Ripard wrote:
>> >Hi Eugen,
>> >
>> >Thanks for giving those patches another shot.
>> >
>> >On Thu, Jul 19, 2018 at 12:57:52PM +0300, Eugen Hristev wrote:
>> >>From: Maxime Ripard <maxime.ripard@free-electrons.com>
>> >>
>> >>We might want to access data stored onto one wire EEPROMs.
>> >>Create a framework to provide a consistent API.
>> >>
>> >>Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> >>[eugen.hristev@microchip.com: reworked patch]
>> >>Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> >>---
>> >>  drivers/Kconfig                      |  2 ++
>> >>  drivers/Makefile                     |  1 +
>> >>  drivers/w1-eeprom/Kconfig            | 17 +++++++++++
>> >>  drivers/w1-eeprom/Makefile           |  2 ++
>> >>  drivers/w1-eeprom/w1-eeprom-uclass.c | 56 ++++++++++++++++++++++++++++++++++++
>> >>  include/dm/uclass-id.h               |  1 +
>> >>  include/w1-eeprom.h                  | 28 ++++++++++++++++++
>> >>  7 files changed, 107 insertions(+)
>> >>  create mode 040000 drivers/w1-eeprom
>> >>  create mode 100644 drivers/w1-eeprom/Kconfig
>> >>  create mode 100644 drivers/w1-eeprom/Makefile
>> >>  create mode 100644 drivers/w1-eeprom/w1-eeprom-uclass.c
>> >>  create mode 100644 include/w1-eeprom.h
>> >
>> >I believe that we shouldn't have a framework solely for 1-wire
>> >EEPROMs, but for EEPROMs, connected to any bus.
>> >
>> >The 1-Wire EEPROMs all behave pretty much the same, so we'll probably
>> >only see a single driver within that framework. And at the same time,
>> >we'll want to have a consistent interface to access all the EEPROMs,
>> >no matter on which bus they sit on.
>>
>> Hello,
>>
>> I worked this series using the original implementation as a starting point:
>> having the eeproms on different subsystems (i2c and onewire).
>>
>> The different types of eeproms have only the name in common as I see it, and
>> the way to access them is totally different: two different type of buses, so
>> uniting them is just for the namesake ?
>>
>> One option is to have them separately, as we have spi, i2c memories , etc;
>> Or, unite them under a single subsystem for eeproms, and have one driver for
>> i2c eeproms and one for w1 eeproms, trying to make the same API to access
>> them, and hide the bus specific differences.
>>
>> Question for maintainers: which is the best direction to go, so I can rework
>> the series accordingly ?
>
> It would be nice if we had a generic eeprom command that wasn't
> bus-centric.  Unfortunately we have an eeprom command that IS bus
> centric and not easily extended to working on all appropriate buses.  So
> to me, starting out by handing w1 eeproms under w1 seems OK.  And we can
> put it on the TODO list to make cmd/eeprom.c parse <bus> as perhaps
> "BUSTYPE:number" so we could do eeprom w1:0 ... or eeprom i2c:0 ... and
> so forth.

That makes sense to me.

We could provide some sort of read/write device supported by SPI, I2C,
1wire, etc. in principle. I don't think anyone has attempted it yet.

Regards,
Simon

Patch

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 2cae829..386af75 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -104,6 +104,8 @@  source "drivers/video/Kconfig"
 
 source "drivers/w1/Kconfig"
 
+source "drivers/w1-eeprom/Kconfig"
+
 source "drivers/watchdog/Kconfig"
 
 config PHYS_TO_BUS
diff --git a/drivers/Makefile b/drivers/Makefile
index 728380b..de67a17 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -102,6 +102,7 @@  obj-y += soc/
 obj-$(CONFIG_REMOTEPROC) += remoteproc/
 obj-y += thermal/
 obj-$(CONFIG_W1) += w1/
+obj-$(CONFIG_W1_EEPROM) += w1-eeprom/
 
 obj-$(CONFIG_MACH_PIC32) += ddr/microchip/
 endif
diff --git a/drivers/w1-eeprom/Kconfig b/drivers/w1-eeprom/Kconfig
new file mode 100644
index 0000000..d5ddc80
--- /dev/null
+++ b/drivers/w1-eeprom/Kconfig
@@ -0,0 +1,17 @@ 
+#
+# EEPROM subsystem configuration
+#
+
+menu "1-wire EEPROM support"
+
+config W1_EEPROM
+	bool "Enable support for EEPROMs on 1wire interface"
+	depends on DM
+	help
+	  Support for the EEPROMs connected on 1-wire Dallas protocol interface
+
+if W1_EEPROM
+
+endif
+
+endmenu
diff --git a/drivers/w1-eeprom/Makefile b/drivers/w1-eeprom/Makefile
new file mode 100644
index 0000000..b72950e
--- /dev/null
+++ b/drivers/w1-eeprom/Makefile
@@ -0,0 +1,2 @@ 
+obj-$(CONFIG_W1_EEPROM) += w1-eeprom-uclass.o
+
diff --git a/drivers/w1-eeprom/w1-eeprom-uclass.c b/drivers/w1-eeprom/w1-eeprom-uclass.c
new file mode 100644
index 0000000..4176baf
--- /dev/null
+++ b/drivers/w1-eeprom/w1-eeprom-uclass.c
@@ -0,0 +1,56 @@ 
+/* SPDX-License-Identifier:	GPL-2.0+
+ *
+ * Copyright (c) 2015 Free Electrons
+ * Copyright (c) 2015 NextThing Co.
+ *
+ * Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <w1-eeprom.h>
+
+#include <dm/device-internal.h>
+
+int w1_eeprom_read_buf(struct udevice *dev, unsigned int offset,
+		       u8 *buf, unsigned int count)
+{
+	const struct w1_eeprom_ops *ops = device_get_ops(dev);
+
+	if (!ops->read_buf)
+		return -ENOSYS;
+
+	return ops->read_buf(dev, offset, buf, count);
+}
+
+UCLASS_DRIVER(w1_eeprom) = {
+	.name		= "w1_eeprom",
+	.id		= UCLASS_W1_EEPROM,
+};
+
+int w1_eeprom_dm_init(void)
+{
+	struct udevice *bus;
+	struct uclass *uc;
+	int ret;
+
+	ret = uclass_get(UCLASS_W1_EEPROM, &uc);
+	if (ret)
+		return ret;
+
+	uclass_foreach_dev(bus, uc) {
+		ret = device_probe(bus);
+		if (ret == -ENODEV) {	/* No such device. */
+			debug("W1_EEPROM not available.\n");
+			continue;
+		}
+
+		if (ret) {		/* Other error. */
+			printf("W1_EEPROM probe failed, error %d\n", ret);
+			continue;
+		}
+	}
+
+	return 0;
+}
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index 8eca9dc..06ff339 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -90,6 +90,7 @@  enum uclass_id {
 	UCLASS_VIDEO_BRIDGE,	/* Video bridge, e.g. DisplayPort to LVDS */
 	UCLASS_VIDEO_CONSOLE,	/* Text console driver for video device */
 	UCLASS_W1,		/* Dallas 1-Wire bus */
+	UCLASS_W1_EEPROM,	/* one-wire EEPROMs */
 	UCLASS_WDT,		/* Watchdot Timer driver */
 
 	UCLASS_COUNT,
diff --git a/include/w1-eeprom.h b/include/w1-eeprom.h
new file mode 100644
index 0000000..7c9dd96
--- /dev/null
+++ b/include/w1-eeprom.h
@@ -0,0 +1,28 @@ 
+/* SPDX-License-Identifier:	GPL-2.0+
+ *
+ * Copyright (c) 2015 Free Electrons
+ * Copyright (c) 2015 NextThing Co
+ * Copyright (c) 2018 Microchip Technology, Inc.
+ *
+ */
+
+#ifndef __W1_EEPROM_H
+#define __W1_EEPROM_H
+
+struct udevice;
+
+struct w1_eeprom_ops {
+	/*
+	 * Reads a buff from the given EEPROM memory, starting at
+	 * given offset and place the results into the given buffer.
+	 * Should read given count of bytes.
+	 * Should return 0 on success, and normal error.h on error
+	 */
+	int	(*read_buf)(struct udevice *dev, unsigned int offset,
+			    u8 *buf, unsigned int count);
+};
+
+int w1_eeprom_read_buf(struct udevice *dev, unsigned int offset,
+		       u8 *buf, unsigned int count);
+
+#endif