| Message ID | 1531994288-19423-5-git-send-email-eugen.hristev@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/20] w1: Add 1-Wire uclass | expand |
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
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 >
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
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
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