This series of patches adds an initial set of devicetree bindings to the
eeprom_93xx46 driver which mirror the configuration options previously
available as a platform device. These bindings are then extended to
include support for specific Atmel devices in this family and also to
support GPIO-based selection of the device (e.g. for use with an SPI bus
mux).
Additionally, an address aliasing issue with 16-bit read and write
accesses in the eeprom_93xx46 driver discovered during testing is fixed.
Changes since v1:
- Consolidated all Documentation/devictree additions into a single patch.
- Clarified compatible string shall be only one of the supported values.
- Renamed the 'select-gpio' binding to 'select-gpios'.
Cory Tusar (5):
misc: eeprom_93xx46: Fix 16-bit read and write accesses.
Documentation: devicetree: Add DT bindings to eeprom_93xx46 driver.
misc: eeprom_93xx46: Implement eeprom_93xx46 DT bindings.
misc: eeprom_93xx46: Add quirks to support Atmel AT93C46D device.
misc: eeprom_93xx46: Add support for a GPIO 'select' line.
.../devicetree/bindings/misc/eeprom-93xx46.txt | 25 +++
drivers/misc/eeprom/eeprom_93xx46.c | 216 +++++++++++++++++----
include/linux/eeprom_93xx46.h | 7 +
3 files changed, 212 insertions(+), 36 deletions(-)
create mode 100644 Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
--
2.4.10
Compatible at93xx46 devices from both Microchip and Atmel expect a
word-based address, regardless of whether the device is strapped for 8-
or 16-bit operation. However, the offset parameter passed in when
reading or writing at a specific location is always specified in terms
of bytes.
This commit fixes 16-bit read and write accesses by shifting the offset
parameter to account for this difference between a byte offset and a
word-based address.
Signed-off-by: Cory Tusar <[email protected]>
---
drivers/misc/eeprom/eeprom_93xx46.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index ff63f05..e1bf0a5 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -54,7 +54,7 @@ eeprom_93xx46_bin_read(struct file *filp, struct kobject *kobj,
cmd_addr |= off & 0x7f;
bits = 10;
} else {
- cmd_addr |= off & 0x3f;
+ cmd_addr |= (off >> 1) & 0x3f;
bits = 9;
}
@@ -155,7 +155,7 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *edev,
bits = 10;
data_len = 1;
} else {
- cmd_addr |= off & 0x3f;
+ cmd_addr |= (off >> 1) & 0x3f;
bits = 9;
data_len = 2;
}
--
2.4.10
This commit documents bindings to be added to the eeprom_93xx46 driver
which will allow:
- Device word size and read-only attributes to be specified.
- A device-specific compatible string for use with Atmel AT93C46D
EEPROMs.
- Specifying a GPIO line to function as a 'select' or 'enable' signal
prior to accessing the EEPROM.
Signed-off-by: Cory Tusar <[email protected]>
---
.../devicetree/bindings/misc/eeprom-93xx46.txt | 25 ++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
new file mode 100644
index 0000000..8e116a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
@@ -0,0 +1,25 @@
+EEPROMs (SPI) compatible with Microchip Technology 93xx46 family.
+
+Required properties:
+- compatible : shall be one of:
+ "atmel,at93c46d"
+ "eeprom-93xx46"
+- data-size : number of data bits per word (either 8 or 16)
+
+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)
+
+Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt
+apply. In particular, "reg" and "spi-max-frequency" properties must be given.
+
+Example:
+ 93c46c@0 {
+ compatible = "eeprom-93xx46";
+ reg = <0>;
+ spi-max-frequency = <1000000>;
+ spi-cs-high;
+ data-size = <8>;
+ select-gpios = <&gpio4 4 GPIO_ACTIVE_HIGH>;
+ };
--
2.4.10
This commit implements bindings in the eeprom_93xx46 driver allowing
device word size and read-only attributes to be specified via
devicetree.
Signed-off-by: Cory Tusar <[email protected]>
---
drivers/misc/eeprom/eeprom_93xx46.c | 62 +++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index e1bf0a5..1f29d9a 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -13,6 +13,8 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/slab.h>
#include <linux/spi/spi.h>
#include <linux/sysfs.h>
@@ -294,12 +296,71 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
}
static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
+#ifdef CONFIG_OF
+static const struct of_device_id eeprom_93xx46_of_table[] = {
+ { .compatible = "eeprom-93xx46", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
+
+static int eeprom_93xx46_probe_dt(struct spi_device *spi)
+{
+ struct device_node *np = spi->dev.of_node;
+ struct eeprom_93xx46_platform_data *pd;
+ u32 tmp;
+ int ret;
+
+ if (!of_match_device(eeprom_93xx46_of_table, &spi->dev))
+ return 0;
+
+ pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
+ if (!pd)
+ return -ENOMEM;
+
+ ret = of_property_read_u32(np, "data-size", &tmp);
+ if (ret < 0) {
+ dev_err(&spi->dev, "data-size property not found\n");
+ goto error_free;
+ }
+
+ if (tmp == 8) {
+ pd->flags |= EE_ADDR8;
+ } else if (tmp == 16) {
+ pd->flags |= EE_ADDR16;
+ } else {
+ dev_err(&spi->dev, "invalid data-size (%d)\n", tmp);
+ goto error_free;
+ }
+
+ if (of_property_read_bool(np, "read-only"))
+ pd->flags |= EE_READONLY;
+
+ spi->dev.platform_data = pd;
+
+ return 1;
+
+error_free:
+ devm_kfree(&spi->dev, pd);
+ return ret;
+}
+
+#else
+static inline int eeprom_93xx46_probe_dt(struct spi_device *spi)
+{
+ return 0;
+}
+#endif
+
static int eeprom_93xx46_probe(struct spi_device *spi)
{
struct eeprom_93xx46_platform_data *pd;
struct eeprom_93xx46_dev *edev;
int err;
+ err = eeprom_93xx46_probe_dt(spi);
+ if (err < 0)
+ return err;
+
pd = spi->dev.platform_data;
if (!pd) {
dev_err(&spi->dev, "missing platform data\n");
@@ -370,6 +431,7 @@ static int eeprom_93xx46_remove(struct spi_device *spi)
static struct spi_driver eeprom_93xx46_driver = {
.driver = {
.name = "93xx46",
+ .of_match_table = eeprom_93xx46_of_table,
},
.probe = eeprom_93xx46_probe,
.remove = eeprom_93xx46_remove,
--
2.4.10
Atmel devices in this family have some quirks not found in other similar
chips - they do not support a sequential read of the entire EEPROM
contents, and the control word sent at the start of each operation
varies in bit length.
This commit adds quirk support to the driver and modifies the read
implementation to support non-sequential reads for consistency with
other misc/eeprom drivers.
Tested on a custom Freescale VF610-based platform, with an AT93C46D
device attached via dspi2. The spi-gpio driver was used to allow the
necessary non-byte-sized transfers.
Signed-off-by: Cory Tusar <[email protected]>
---
drivers/misc/eeprom/eeprom_93xx46.c | 128 ++++++++++++++++++++++++++----------
include/linux/eeprom_93xx46.h | 6 ++
2 files changed, 98 insertions(+), 36 deletions(-)
diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index 1f29d9a..0386b03 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -27,6 +27,15 @@
#define ADDR_ERAL 0x20
#define ADDR_EWEN 0x30
+struct eeprom_93xx46_devtype_data {
+ unsigned int quirks;
+};
+
+static const struct eeprom_93xx46_devtype_data atmel_at93c46d_data = {
+ .quirks = EEPROM_93XX46_QUIRK_SINGLE_WORD_READ |
+ EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH,
+};
+
struct eeprom_93xx46_dev {
struct spi_device *spi;
struct eeprom_93xx46_platform_data *pdata;
@@ -35,6 +44,16 @@ struct eeprom_93xx46_dev {
int addrlen;
};
+static inline bool has_quirk_single_word_read(struct eeprom_93xx46_dev *edev)
+{
+ return !!(edev->pdata->quirks & EEPROM_93XX46_QUIRK_SINGLE_WORD_READ);
+}
+
+static inline bool has_quirk_instruction_length(struct eeprom_93xx46_dev *edev)
+{
+ return !!(edev->pdata->quirks & EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH);
+}
+
static ssize_t
eeprom_93xx46_bin_read(struct file *filp, struct kobject *kobj,
struct bin_attribute *bin_attr,
@@ -42,58 +61,73 @@ eeprom_93xx46_bin_read(struct file *filp, struct kobject *kobj,
{
struct eeprom_93xx46_dev *edev;
struct device *dev;
- struct spi_message m;
- struct spi_transfer t[2];
- int bits, ret;
- u16 cmd_addr;
+ ssize_t ret = 0;
dev = container_of(kobj, struct device, kobj);
edev = dev_get_drvdata(dev);
- cmd_addr = OP_READ << edev->addrlen;
+ mutex_lock(&edev->lock);
- if (edev->addrlen == 7) {
- cmd_addr |= off & 0x7f;
- bits = 10;
- } else {
- cmd_addr |= (off >> 1) & 0x3f;
- bits = 9;
- }
+ if (edev->pdata->prepare)
+ edev->pdata->prepare(edev);
- dev_dbg(&edev->spi->dev, "read cmd 0x%x, %d Hz\n",
- cmd_addr, edev->spi->max_speed_hz);
+ while (count) {
+ struct spi_message m;
+ struct spi_transfer t[2] = { { 0 } };
+ u16 cmd_addr = OP_READ << edev->addrlen;
+ size_t nbytes = count;
+ int bits;
+ int err;
+
+ if (edev->addrlen == 7) {
+ cmd_addr |= off & 0x7f;
+ bits = 10;
+ if (has_quirk_single_word_read(edev))
+ nbytes = 1;
+ } else {
+ cmd_addr |= (off >> 1) & 0x3f;
+ bits = 9;
+ if (has_quirk_single_word_read(edev))
+ nbytes = 2;
+ }
- spi_message_init(&m);
- memset(t, 0, sizeof(t));
+ dev_dbg(&edev->spi->dev, "read cmd 0x%x, %d Hz\n",
+ cmd_addr, edev->spi->max_speed_hz);
- t[0].tx_buf = (char *)&cmd_addr;
- t[0].len = 2;
- t[0].bits_per_word = bits;
- spi_message_add_tail(&t[0], &m);
+ spi_message_init(&m);
- t[1].rx_buf = buf;
- t[1].len = count;
- t[1].bits_per_word = 8;
- spi_message_add_tail(&t[1], &m);
+ t[0].tx_buf = (char *)&cmd_addr;
+ t[0].len = 2;
+ t[0].bits_per_word = bits;
+ spi_message_add_tail(&t[0], &m);
- mutex_lock(&edev->lock);
+ t[1].rx_buf = buf;
+ t[1].len = count;
+ t[1].bits_per_word = 8;
+ spi_message_add_tail(&t[1], &m);
- if (edev->pdata->prepare)
- edev->pdata->prepare(edev);
+ err = spi_sync(edev->spi, &m);
+ /* have to wait at least Tcsl ns */
+ ndelay(250);
- ret = spi_sync(edev->spi, &m);
- /* have to wait at least Tcsl ns */
- ndelay(250);
- if (ret) {
- dev_err(&edev->spi->dev, "read %zu bytes at %d: err. %d\n",
- count, (int)off, ret);
+ if (err) {
+ dev_err(&edev->spi->dev, "read %zu bytes at %d: err. %d\n",
+ nbytes, (int)off, err);
+ ret = err;
+ break;
+ }
+
+ buf += nbytes;
+ off += nbytes;
+ count -= nbytes;
+ ret += nbytes;
}
if (edev->pdata->finish)
edev->pdata->finish(edev);
mutex_unlock(&edev->lock);
- return ret ? : count;
+ return ret;
}
static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
@@ -112,7 +146,13 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
bits = 9;
}
- dev_dbg(&edev->spi->dev, "ew cmd 0x%04x\n", cmd_addr);
+ if (has_quirk_instruction_length(edev)) {
+ cmd_addr <<= 2;
+ bits += 2;
+ }
+
+ dev_dbg(&edev->spi->dev, "ew%s cmd 0x%04x, %d bits\n",
+ is_on ? "en" : "ds", cmd_addr, bits);
spi_message_init(&m);
memset(&t, 0, sizeof(t));
@@ -247,6 +287,13 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
bits = 9;
}
+ if (has_quirk_instruction_length(edev)) {
+ cmd_addr <<= 2;
+ bits += 2;
+ }
+
+ dev_dbg(&edev->spi->dev, "eral cmd 0x%04x, %d bits\n", cmd_addr, bits);
+
spi_message_init(&m);
memset(&t, 0, sizeof(t));
@@ -299,18 +346,21 @@ static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
#ifdef CONFIG_OF
static const struct of_device_id eeprom_93xx46_of_table[] = {
{ .compatible = "eeprom-93xx46", },
+ { .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
{}
};
MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
static int eeprom_93xx46_probe_dt(struct spi_device *spi)
{
+ const struct of_device_id *of_id =
+ of_match_device(eeprom_93xx46_of_table, &spi->dev);
struct device_node *np = spi->dev.of_node;
struct eeprom_93xx46_platform_data *pd;
u32 tmp;
int ret;
- if (!of_match_device(eeprom_93xx46_of_table, &spi->dev))
+ if (!of_id)
return 0;
pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
@@ -335,6 +385,12 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
if (of_property_read_bool(np, "read-only"))
pd->flags |= EE_READONLY;
+ if (of_id->data) {
+ const struct eeprom_93xx46_devtype_data *data = of_id->data;
+
+ pd->quirks = data->quirks;
+ }
+
spi->dev.platform_data = pd;
return 1;
diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
index 0679181..92fa4c3 100644
--- a/include/linux/eeprom_93xx46.h
+++ b/include/linux/eeprom_93xx46.h
@@ -9,6 +9,12 @@ struct eeprom_93xx46_platform_data {
#define EE_ADDR16 0x02 /* 16 bit addr. cfg */
#define EE_READONLY 0x08 /* forbid writing */
+ unsigned int quirks;
+/* Single word read transfers only; no sequential read. */
+#define EEPROM_93XX46_QUIRK_SINGLE_WORD_READ (1 << 0)
+/* Instructions such as EWEN are (addrlen + 2) in length. */
+#define EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH (1 << 1)
+
/*
* optional hooks to control additional logic
* before and after spi transfer.
--
2.4.10
This commit adds support to the eeprom_93x46 driver allowing a GPIO line
to function as a 'select' or 'enable' signal prior to accessing the
EEPROM.
Signed-off-by: Cory Tusar <[email protected]>
---
drivers/misc/eeprom/eeprom_93xx46.c | 26 ++++++++++++++++++++++++++
include/linux/eeprom_93xx46.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index 0386b03..375951f 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -10,11 +10,14 @@
#include <linux/delay.h>
#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/of_gpio.h>
#include <linux/slab.h>
#include <linux/spi/spi.h>
#include <linux/sysfs.h>
@@ -344,6 +347,20 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
#ifdef CONFIG_OF
+static void select_assert(void *context)
+{
+ struct eeprom_93xx46_dev *edev = context;
+
+ gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
+}
+
+static void select_deassert(void *context)
+{
+ struct eeprom_93xx46_dev *edev = context;
+
+ gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 0);
+}
+
static const struct of_device_id eeprom_93xx46_of_table[] = {
{ .compatible = "eeprom-93xx46", },
{ .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
@@ -385,6 +402,15 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
if (of_property_read_bool(np, "read-only"))
pd->flags |= EE_READONLY;
+ ret = of_get_named_gpio(np, "select-gpios", 0);
+ if (ret < 0) {
+ pd->select_gpio = -1;
+ } else {
+ pd->select_gpio = ret;
+ pd->prepare = select_assert;
+ pd->finish = select_deassert;
+ }
+
if (of_id->data) {
const struct eeprom_93xx46_devtype_data *data = of_id->data;
diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
index 92fa4c3..aa472c7 100644
--- a/include/linux/eeprom_93xx46.h
+++ b/include/linux/eeprom_93xx46.h
@@ -21,4 +21,5 @@ struct eeprom_93xx46_platform_data {
*/
void (*prepare)(void *);
void (*finish)(void *);
+ unsigned int select_gpio;
};
--
2.4.10
Hi Cory,
On 19.11.2015 05:29, Cory Tusar wrote:
> This commit implements bindings in the eeprom_93xx46 driver allowing
> device word size and read-only attributes to be specified via
> devicetree.
>
> Signed-off-by: Cory Tusar <[email protected]>
> ---
> drivers/misc/eeprom/eeprom_93xx46.c | 62 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index e1bf0a5..1f29d9a 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
> @@ -13,6 +13,8 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/slab.h>
> #include <linux/spi/spi.h>
> #include <linux/sysfs.h>
> @@ -294,12 +296,71 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
> }
> static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id eeprom_93xx46_of_table[] = {
> + { .compatible = "eeprom-93xx46", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
> +
Please move this declaration closer to struct spi_driver
eeprom_93xx46_driver below.
Also you can avoid #ifdef here, if you write
.of_match_table = of_match_ptr(eeprom_93xx46_of_table)
Whenever possible please avoid #ifdef's in .c files.
> +static int eeprom_93xx46_probe_dt(struct spi_device *spi)
> +{
> + struct device_node *np = spi->dev.of_node;
> + struct eeprom_93xx46_platform_data *pd;
> + u32 tmp;
> + int ret;
> +
> + if (!of_match_device(eeprom_93xx46_of_table, &spi->dev))
> + return 0;
> +
> + pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
> + if (!pd)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32(np, "data-size", &tmp);
> + if (ret < 0) {
> + dev_err(&spi->dev, "data-size property not found\n");
> + goto error_free;
Because you use devm_* resource allocation in .probe, just return error.
Plus I would suggest to change "data-size" property to an optional one,
here I mean that if it is omitted, then by default consider pd->flags |=
EE_ADDR8.
> + }
> +
> + if (tmp == 8) {
> + pd->flags |= EE_ADDR8;
> + } else if (tmp == 16) {
> + pd->flags |= EE_ADDR16;
> + } else {
> + dev_err(&spi->dev, "invalid data-size (%d)\n", tmp);
> + goto error_free;
Same here.
> + }
> +
> + if (of_property_read_bool(np, "read-only"))
> + pd->flags |= EE_READONLY;
> +
> + spi->dev.platform_data = pd;
> +
> + return 1;
On success please return 0.
> +error_free:
> + devm_kfree(&spi->dev, pd);
> + return ret;
> +}
> +
> +#else
> +static inline int eeprom_93xx46_probe_dt(struct spi_device *spi)
> +{
> + return 0;
> +}
> +#endif
> +
I actually don't see a point to have #ifdef CONFIG_OF here.
Instead please add a check for !spi->dev.of_node at the beginning of
eeprom_93xx46_probe_dt() or in .probe()
> static int eeprom_93xx46_probe(struct spi_device *spi)
> {
> struct eeprom_93xx46_platform_data *pd;
> struct eeprom_93xx46_dev *edev;
> int err;
>
> + err = eeprom_93xx46_probe_dt(spi);
> + if (err < 0)
> + return err;
> +
> pd = spi->dev.platform_data;
> if (!pd) {
> dev_err(&spi->dev, "missing platform data\n");
> @@ -370,6 +431,7 @@ static int eeprom_93xx46_remove(struct spi_device *spi)
> static struct spi_driver eeprom_93xx46_driver = {
> .driver = {
> .name = "93xx46",
> + .of_match_table = eeprom_93xx46_of_table,
> },
> .probe = eeprom_93xx46_probe,
> .remove = eeprom_93xx46_remove,
>
On 19.11.2015 05:29, Cory Tusar wrote:
> Atmel devices in this family have some quirks not found in other similar
> chips - they do not support a sequential read of the entire EEPROM
> contents, and the control word sent at the start of each operation
> varies in bit length.
>
> This commit adds quirk support to the driver and modifies the read
> implementation to support non-sequential reads for consistency with
> other misc/eeprom drivers.
>
> Tested on a custom Freescale VF610-based platform, with an AT93C46D
> device attached via dspi2. The spi-gpio driver was used to allow the
> necessary non-byte-sized transfers.
>
> Signed-off-by: Cory Tusar <[email protected]>
> ---
> drivers/misc/eeprom/eeprom_93xx46.c | 128 ++++++++++++++++++++++++++----------
> include/linux/eeprom_93xx46.h | 6 ++
> 2 files changed, 98 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index 1f29d9a..0386b03 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
> @@ -27,6 +27,15 @@
> #define ADDR_ERAL 0x20
> #define ADDR_EWEN 0x30
>
> +struct eeprom_93xx46_devtype_data {
> + unsigned int quirks;
> +};
> +
> +static const struct eeprom_93xx46_devtype_data atmel_at93c46d_data = {
> + .quirks = EEPROM_93XX46_QUIRK_SINGLE_WORD_READ |
> + EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH,
> +};
> +
> struct eeprom_93xx46_dev {
> struct spi_device *spi;
> struct eeprom_93xx46_platform_data *pdata;
> @@ -35,6 +44,16 @@ struct eeprom_93xx46_dev {
> int addrlen;
> };
>
> +static inline bool has_quirk_single_word_read(struct eeprom_93xx46_dev *edev)
> +{
> + return !!(edev->pdata->quirks & EEPROM_93XX46_QUIRK_SINGLE_WORD_READ);
bool return type will do the work, please remove !!.
> +}
> +
> +static inline bool has_quirk_instruction_length(struct eeprom_93xx46_dev *edev)
> +{
> + return !!(edev->pdata->quirks & EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH);
Same here.
> +}
> +
> static ssize_t
> eeprom_93xx46_bin_read(struct file *filp, struct kobject *kobj,
> struct bin_attribute *bin_attr,
> @@ -42,58 +61,73 @@ eeprom_93xx46_bin_read(struct file *filp, struct kobject *kobj,
> {
> struct eeprom_93xx46_dev *edev;
> struct device *dev;
> - struct spi_message m;
> - struct spi_transfer t[2];
> - int bits, ret;
> - u16 cmd_addr;
> + ssize_t ret = 0;
>
> dev = container_of(kobj, struct device, kobj);
> edev = dev_get_drvdata(dev);
>
> - cmd_addr = OP_READ << edev->addrlen;
> + mutex_lock(&edev->lock);
>
> - if (edev->addrlen == 7) {
> - cmd_addr |= off & 0x7f;
> - bits = 10;
> - } else {
> - cmd_addr |= (off >> 1) & 0x3f;
> - bits = 9;
> - }
> + if (edev->pdata->prepare)
> + edev->pdata->prepare(edev);
>
> - dev_dbg(&edev->spi->dev, "read cmd 0x%x, %d Hz\n",
> - cmd_addr, edev->spi->max_speed_hz);
> + while (count) {
> + struct spi_message m;
> + struct spi_transfer t[2] = { { 0 } };
Just { 0 };
> + u16 cmd_addr = OP_READ << edev->addrlen;
> + size_t nbytes = count;
> + int bits;
> + int err;
> +
> + if (edev->addrlen == 7) {
> + cmd_addr |= off & 0x7f;
> + bits = 10;
> + if (has_quirk_single_word_read(edev))
> + nbytes = 1;
> + } else {
> + cmd_addr |= (off >> 1) & 0x3f;
> + bits = 9;
> + if (has_quirk_single_word_read(edev))
> + nbytes = 2;
> + }
>
> - spi_message_init(&m);
> - memset(t, 0, sizeof(t));
> + dev_dbg(&edev->spi->dev, "read cmd 0x%x, %d Hz\n",
> + cmd_addr, edev->spi->max_speed_hz);
>
> - t[0].tx_buf = (char *)&cmd_addr;
> - t[0].len = 2;
> - t[0].bits_per_word = bits;
> - spi_message_add_tail(&t[0], &m);
> + spi_message_init(&m);
>
> - t[1].rx_buf = buf;
> - t[1].len = count;
> - t[1].bits_per_word = 8;
> - spi_message_add_tail(&t[1], &m);
> + t[0].tx_buf = (char *)&cmd_addr;
> + t[0].len = 2;
> + t[0].bits_per_word = bits;
> + spi_message_add_tail(&t[0], &m);
>
> - mutex_lock(&edev->lock);
> + t[1].rx_buf = buf;
> + t[1].len = count;
> + t[1].bits_per_word = 8;
> + spi_message_add_tail(&t[1], &m);
>
> - if (edev->pdata->prepare)
> - edev->pdata->prepare(edev);
> + err = spi_sync(edev->spi, &m);
> + /* have to wait at least Tcsl ns */
> + ndelay(250);
>
> - ret = spi_sync(edev->spi, &m);
> - /* have to wait at least Tcsl ns */
> - ndelay(250);
> - if (ret) {
> - dev_err(&edev->spi->dev, "read %zu bytes at %d: err. %d\n",
> - count, (int)off, ret);
> + if (err) {
> + dev_err(&edev->spi->dev, "read %zu bytes at %d: err. %d\n",
> + nbytes, (int)off, err);
> + ret = err;
> + break;
> + }
> +
> + buf += nbytes;
> + off += nbytes;
> + count -= nbytes;
> + ret += nbytes;
> }
>
> if (edev->pdata->finish)
> edev->pdata->finish(edev);
>
> mutex_unlock(&edev->lock);
> - return ret ? : count;
> + return ret;
> }
>
> static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
> @@ -112,7 +146,13 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
> bits = 9;
> }
>
> - dev_dbg(&edev->spi->dev, "ew cmd 0x%04x\n", cmd_addr);
> + if (has_quirk_instruction_length(edev)) {
> + cmd_addr <<= 2;
> + bits += 2;
> + }
> +
> + dev_dbg(&edev->spi->dev, "ew%s cmd 0x%04x, %d bits\n",
> + is_on ? "en" : "ds", cmd_addr, bits);
>
> spi_message_init(&m);
> memset(&t, 0, sizeof(t));
> @@ -247,6 +287,13 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
> bits = 9;
> }
>
> + if (has_quirk_instruction_length(edev)) {
> + cmd_addr <<= 2;
> + bits += 2;
> + }
> +
> + dev_dbg(&edev->spi->dev, "eral cmd 0x%04x, %d bits\n", cmd_addr, bits);
> +
> spi_message_init(&m);
> memset(&t, 0, sizeof(t));
>
> @@ -299,18 +346,21 @@ static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
> #ifdef CONFIG_OF
> static const struct of_device_id eeprom_93xx46_of_table[] = {
> { .compatible = "eeprom-93xx46", },
> + { .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
> {}
> };
> MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
>
> static int eeprom_93xx46_probe_dt(struct spi_device *spi)
> {
> + const struct of_device_id *of_id =
> + of_match_device(eeprom_93xx46_of_table, &spi->dev);
> struct device_node *np = spi->dev.of_node;
> struct eeprom_93xx46_platform_data *pd;
> u32 tmp;
> int ret;
>
> - if (!of_match_device(eeprom_93xx46_of_table, &spi->dev))
> + if (!of_id)
> return 0;
>
> pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
> @@ -335,6 +385,12 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
> if (of_property_read_bool(np, "read-only"))
> pd->flags |= EE_READONLY;
>
> + if (of_id->data) {
> + const struct eeprom_93xx46_devtype_data *data = of_id->data;
> +
> + pd->quirks = data->quirks;
> + }
> +
> spi->dev.platform_data = pd;
>
> return 1;
> diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
> index 0679181..92fa4c3 100644
> --- a/include/linux/eeprom_93xx46.h
> +++ b/include/linux/eeprom_93xx46.h
> @@ -9,6 +9,12 @@ struct eeprom_93xx46_platform_data {
> #define EE_ADDR16 0x02 /* 16 bit addr. cfg */
> #define EE_READONLY 0x08 /* forbid writing */
>
> + unsigned int quirks;
> +/* Single word read transfers only; no sequential read. */
> +#define EEPROM_93XX46_QUIRK_SINGLE_WORD_READ (1 << 0)
> +/* Instructions such as EWEN are (addrlen + 2) in length. */
> +#define EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH (1 << 1)
> +
I wonder do you really need this in platfrom data? Would it work for
you, if quirks are OF specific only? If yes, then please move macros to
.c file.
> /*
> * optional hooks to control additional logic
> * before and after spi transfer.
>
--
With best wishes,
Vladimir
On 19.11.2015 05:29, Cory Tusar wrote:
> This commit adds support to the eeprom_93x46 driver allowing a GPIO line
> to function as a 'select' or 'enable' signal prior to accessing the
> EEPROM.
>
> Signed-off-by: Cory Tusar <[email protected]>
> ---
> drivers/misc/eeprom/eeprom_93xx46.c | 26 ++++++++++++++++++++++++++
> include/linux/eeprom_93xx46.h | 1 +
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index 0386b03..375951f 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
> @@ -10,11 +10,14 @@
>
> #include <linux/delay.h>
> #include <linux/device.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/of_gpio.h>
Please double check, adding only linux/of_gpio.h header should work,
linux/gpio.h and linux/gpio/consumer.h are redundant.
> #include <linux/slab.h>
> #include <linux/spi/spi.h>
> #include <linux/sysfs.h>
> @@ -344,6 +347,20 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
> static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
>
> #ifdef CONFIG_OF
> +static void select_assert(void *context)
> +{
> + struct eeprom_93xx46_dev *edev = context;
> +
> + gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
I would suggest to use gpio_set_value()
> +}
> +
> +static void select_deassert(void *context)
> +{
> + struct eeprom_93xx46_dev *edev = context;
> +
> + gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 0);
Same here.
> +}
> +
> static const struct of_device_id eeprom_93xx46_of_table[] = {
> { .compatible = "eeprom-93xx46", },
> { .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
> @@ -385,6 +402,15 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
> if (of_property_read_bool(np, "read-only"))
> pd->flags |= EE_READONLY;
>
> + ret = of_get_named_gpio(np, "select-gpios", 0);
gpios or gpio? I see only one requested gpio.
> + if (ret < 0) {
> + pd->select_gpio = -1;
> + } else {
> + pd->select_gpio = ret;
> + pd->prepare = select_assert;
> + pd->finish = select_deassert;
> + }
> +
> if (of_id->data) {
> const struct eeprom_93xx46_devtype_data *data = of_id->data;
>
> diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
> index 92fa4c3..aa472c7 100644
> --- a/include/linux/eeprom_93xx46.h
> +++ b/include/linux/eeprom_93xx46.h
> @@ -21,4 +21,5 @@ struct eeprom_93xx46_platform_data {
> */
> void (*prepare)(void *);
> void (*finish)(void *);
> + unsigned int select_gpio;
Same questions as in v2 4/5.
> };
>
--
With best wishes,
Vladimir
On 11/18/2015 11:50 PM, Vladimir Zapolskiy wrote:
> Hi Cory,
>
> On 19.11.2015 05:29, Cory Tusar wrote:
>> This commit implements bindings in the eeprom_93xx46 driver allowing
>> device word size and read-only attributes to be specified via
>> devicetree.
>>
>> Signed-off-by: Cory Tusar <[email protected]>
>> ---
>> drivers/misc/eeprom/eeprom_93xx46.c | 62 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 62 insertions(+)
>>
>> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
>> index e1bf0a5..1f29d9a 100644
>> --- a/drivers/misc/eeprom/eeprom_93xx46.c
>> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
>> @@ -13,6 +13,8 @@
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> #include <linux/slab.h>
>> #include <linux/spi/spi.h>
>> #include <linux/sysfs.h>
>> @@ -294,12 +296,71 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
>> }
>> static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
>>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id eeprom_93xx46_of_table[] = {
>> + { .compatible = "eeprom-93xx46", },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
>> +
>
> Please move this declaration closer to struct spi_driver
> eeprom_93xx46_driver below.
>
It is referenced in the function below.
> Also you can avoid #ifdef here, if you write
>
> .of_match_table = of_match_ptr(eeprom_93xx46_of_table)
>
I think this is backwards, when eeprom_93xx46_of_table is #ifdef'd
off you need to use of_match_ptr so when CONFIG_OF is not defined
you wont get an undefined reference to eeprom_93xx46_of_table.
Without the #ifdef, eeprom_93xx46_of_table will always be defined
and you can just .of_match_table = eeprom_93xx46_of_table; safely.
> Whenever possible please avoid #ifdef's in .c files.
>
>> +static int eeprom_93xx46_probe_dt(struct spi_device *spi)
>> +{
>> + struct device_node *np = spi->dev.of_node;
>> + struct eeprom_93xx46_platform_data *pd;
>> + u32 tmp;
>> + int ret;
>> +
>> + if (!of_match_device(eeprom_93xx46_of_table, &spi->dev))
>> + return 0;
>> +
>> + pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
>> + if (!pd)
>> + return -ENOMEM;
>> +
>> + ret = of_property_read_u32(np, "data-size", &tmp);
>> + if (ret < 0) {
>> + dev_err(&spi->dev, "data-size property not found\n");
>> + goto error_free;
>
> Because you use devm_* resource allocation in .probe, just return error.
>
> Plus I would suggest to change "data-size" property to an optional one,
> here I mean that if it is omitted, then by default consider pd->flags |=
> EE_ADDR8.
>
>> + }
>> +
>> + if (tmp == 8) {
>> + pd->flags |= EE_ADDR8;
>> + } else if (tmp == 16) {
>> + pd->flags |= EE_ADDR16;
>> + } else {
>> + dev_err(&spi->dev, "invalid data-size (%d)\n", tmp);
>> + goto error_free;
>
> Same here.
>
>> + }
>> +
>> + if (of_property_read_bool(np, "read-only"))
>> + pd->flags |= EE_READONLY;
>> +
>> + spi->dev.platform_data = pd;
>> +
>> + return 1;
>
> On success please return 0.
>
>> +error_free:
>> + devm_kfree(&spi->dev, pd);
>> + return ret;
>> +}
>> +
>> +#else
>> +static inline int eeprom_93xx46_probe_dt(struct spi_device *spi)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>
> I actually don't see a point to have #ifdef CONFIG_OF here.
>
Usually to avoid a lot of dead code and data when OF is not enabled.
> Instead please add a check for !spi->dev.of_node at the beginning of
> eeprom_93xx46_probe_dt() or in .probe()
>
>> static int eeprom_93xx46_probe(struct spi_device *spi)
>> {
>> struct eeprom_93xx46_platform_data *pd;
>> struct eeprom_93xx46_dev *edev;
>> int err;
>>
>> + err = eeprom_93xx46_probe_dt(spi);
>> + if (err < 0)
>> + return err;
>> +
>> pd = spi->dev.platform_data;
>> if (!pd) {
>> dev_err(&spi->dev, "missing platform data\n");
>> @@ -370,6 +431,7 @@ static int eeprom_93xx46_remove(struct spi_device *spi)
>> static struct spi_driver eeprom_93xx46_driver = {
>> .driver = {
>> .name = "93xx46",
>> + .of_match_table = eeprom_93xx46_of_table,
>> },
>> .probe = eeprom_93xx46_probe,
>> .remove = eeprom_93xx46_remove,
>>
> > #ifdef CONFIG_OF
> > +static void select_assert(void *context)
> > +{
> > + struct eeprom_93xx46_dev *edev = context;
> > +
> > + gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
>
> I would suggest to use gpio_set_value()
Could you explain why?
Maybe this gpio is on an SPI GPIO expander?
> > static const struct of_device_id eeprom_93xx46_of_table[] = {
> > { .compatible = "eeprom-93xx46", },
> > { .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
> > @@ -385,6 +402,15 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
> > if (of_property_read_bool(np, "read-only"))
> > pd->flags |= EE_READONLY;
> >
> > + ret = of_get_named_gpio(np, "select-gpios", 0);
>
> gpios or gpio? I see only one requested gpio.
DT always uses the plural. Go read some bindins in Documentation/devicetree/bindings/
Andrew
On Wed, Nov 18, 2015 at 10:29:38PM -0500, Cory Tusar wrote:
> This commit documents bindings to be added to the eeprom_93xx46 driver
> which will allow:
>
> - Device word size and read-only attributes to be specified.
> - A device-specific compatible string for use with Atmel AT93C46D
> EEPROMs.
> - Specifying a GPIO line to function as a 'select' or 'enable' signal
> prior to accessing the EEPROM.
>
> Signed-off-by: Cory Tusar <[email protected]>
One nit, but:
Acked-by: Rob Herring <[email protected]>
> ---
> .../devicetree/bindings/misc/eeprom-93xx46.txt | 25 ++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
> new file mode 100644
> index 0000000..8e116a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
> @@ -0,0 +1,25 @@
> +EEPROMs (SPI) compatible with Microchip Technology 93xx46 family.
> +
> +Required properties:
> +- compatible : shall be one of:
> + "atmel,at93c46d"
> + "eeprom-93xx46"
> +- data-size : number of data bits per word (either 8 or 16)
> +
> +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)
> +
> +Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt
> +apply. In particular, "reg" and "spi-max-frequency" properties must be given.
> +
> +Example:
> + 93c46c@0 {
This should be eeprom@0.
> + compatible = "eeprom-93xx46";
> + reg = <0>;
> + spi-max-frequency = <1000000>;
> + spi-cs-high;
> + data-size = <8>;
> + select-gpios = <&gpio4 4 GPIO_ACTIVE_HIGH>;
> + };
> --
> 2.4.10
>
Hi Andrew,
On 19.11.2015 16:00, Andrew F. Davis wrote:
> On 11/18/2015 11:50 PM, Vladimir Zapolskiy wrote:
>> Hi Cory,
>>
>> On 19.11.2015 05:29, Cory Tusar wrote:
>>> This commit implements bindings in the eeprom_93xx46 driver allowing
>>> device word size and read-only attributes to be specified via
>>> devicetree.
>>>
>>> Signed-off-by: Cory Tusar <[email protected]>
>>> ---
[snip]
>>> +
>>> +#else
>>> +static inline int eeprom_93xx46_probe_dt(struct spi_device *spi)
>>> +{
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>
>> I actually don't see a point to have #ifdef CONFIG_OF here.
>>
>
> Usually to avoid a lot of dead code and data when OF is not enabled.
>
This is a premature optimization.
Also please see Documentation/CodingStyle, Chapter 20: Conditional
Compilation:
Wherever possible, don't use preprocessor conditionals
(#if, #ifdef) in .c files; doing so makes code harder to read
and logic harder to follow. Instead, use such conditionals in
a header file defining functions for use in those .c files,
providing no-op stub versions in the #else case, and then call
those functions unconditionally from .c files. The compiler
will avoid generating any code for the stub calls, producing
identical results, but the logic will remain easy to follow.
etc.
--
With best wishes,
Vladimir
On 19.11.2015 16:18, Andrew Lunn wrote:
>>> #ifdef CONFIG_OF
>>> +static void select_assert(void *context)
>>> +{
>>> + struct eeprom_93xx46_dev *edev = context;
>>> +
>>> + gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
>>
>> I would suggest to use gpio_set_value()
>
> Could you explain why?
>
> Maybe this gpio is on an SPI GPIO expander?
My point is that gpio_*() interface, gpio_set_value() or
gpio_set_value_cansleep(), might be preferred is this particular case.
I know it is legacy, but in this case there is no difference, if the
target have gpiolib, and the interface is broken, if the target does not
have gpiolib.
>
>>> static const struct of_device_id eeprom_93xx46_of_table[] = {
>>> { .compatible = "eeprom-93xx46", },
>>> { .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
>>> @@ -385,6 +402,15 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
>>> if (of_property_read_bool(np, "read-only"))
>>> pd->flags |= EE_READONLY;
>>>
>>> + ret = of_get_named_gpio(np, "select-gpios", 0);
>>
>> gpios or gpio? I see only one requested gpio.
>
> DT always uses the plural. Go read some bindins in Documentation/devicetree/bindings/
>
Ok.
--
With best wishes,
Vladimir
On Thu, Nov 19, 2015 at 06:52:57PM +0200, Vladimir Zapolskiy wrote:
> On 19.11.2015 16:18, Andrew Lunn wrote:
> >>> #ifdef CONFIG_OF
> >>> +static void select_assert(void *context)
> >>> +{
> >>> + struct eeprom_93xx46_dev *edev = context;
> >>> +
> >>> + gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
> >>
> >> I would suggest to use gpio_set_value()
> >
> > Could you explain why?
> >
> > Maybe this gpio is on an SPI GPIO expander?
>
> My point is that gpio_*() interface, gpio_set_value() or
> gpio_set_value_cansleep(), might be preferred is this particular case.
Ah, O.K, yes, avoid the gpio_to_desc() call.
Agreed.
Andrew
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 11/19/2015 09:59 AM, Rob Herring wrote:
> On Wed, Nov 18, 2015 at 10:29:38PM -0500, Cory Tusar wrote:
>> This commit documents bindings to be added to the eeprom_93xx46 driver
>> which will allow:
>>
>> - Device word size and read-only attributes to be specified.
>> - A device-specific compatible string for use with Atmel AT93C46D
>> EEPROMs.
>> - Specifying a GPIO line to function as a 'select' or 'enable' signal
>> prior to accessing the EEPROM.
>>
>> Signed-off-by: Cory Tusar <[email protected]>
>
> One nit, but:
>
> Acked-by: Rob Herring <[email protected]>
>
>> ---
>> .../devicetree/bindings/misc/eeprom-93xx46.txt | 25 ++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
>> new file mode 100644
>> index 0000000..8e116a1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
>> @@ -0,0 +1,25 @@
>> +EEPROMs (SPI) compatible with Microchip Technology 93xx46 family.
>> +
>> +Required properties:
>> +- compatible : shall be one of:
>> + "atmel,at93c46d"
>> + "eeprom-93xx46"
>> +- data-size : number of data bits per word (either 8 or 16)
>> +
>> +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)
>> +
>> +Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt
>> +apply. In particular, "reg" and "spi-max-frequency" properties must be given.
>> +
>> +Example:
>> + 93c46c@0 {
>
> This should be eeprom@0.
Easy enough. Will fix for v3. Thanks.
>> + compatible = "eeprom-93xx46";
>> + reg = <0>;
>> + spi-max-frequency = <1000000>;
>> + spi-cs-high;
>> + data-size = <8>;
>> + select-gpios = <&gpio4 4 GPIO_ACTIVE_HIGH>;
>> + };
>> --
>> 2.4.10
>>
- --
Cory Tusar
Principal
PID 1 Solutions, Inc.
"There are two ways of constructing a software design. One way is to
make it so simple that there are obviously no deficiencies, and the
other way is to make it so complicated that there are no obvious
deficiencies." --Sir Charles Anthony Richard Hoare
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEYEARECAAYFAlZOB0oACgkQHT1tsfGwHJ9EigCfUGD754C+nIpu5/bvnprFt1gF
DDYAn1KkIYfk6Tp5X1eqfn2OirGHVLTe
=PLGL
-----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 11/19/2015 12:50 AM, Vladimir Zapolskiy wrote:
> Hi Cory,
>
> On 19.11.2015 05:29, Cory Tusar wrote:
>> This commit implements bindings in the eeprom_93xx46 driver allowing
>> device word size and read-only attributes to be specified via
>> devicetree.
>>
>> Signed-off-by: Cory Tusar <[email protected]>
>> ---
>> drivers/misc/eeprom/eeprom_93xx46.c | 62 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 62 insertions(+)
>>
>> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
>> index e1bf0a5..1f29d9a 100644
>> --- a/drivers/misc/eeprom/eeprom_93xx46.c
>> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
>> @@ -13,6 +13,8 @@
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> #include <linux/slab.h>
>> #include <linux/spi/spi.h>
>> #include <linux/sysfs.h>
>> @@ -294,12 +296,71 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
>> }
>> static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
>>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id eeprom_93xx46_of_table[] = {
>> + { .compatible = "eeprom-93xx46", },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
>> +
>
> Please move this declaration closer to struct spi_driver
> eeprom_93xx46_driver below.
As Andrew noted in his follow-up, it's used in the function immediately
after this declaration. Seems logical to leave it here?
> Also you can avoid #ifdef here, if you write
>
> .of_match_table = of_match_ptr(eeprom_93xx46_of_table)
Will change this to use of_match_ptr().
> Whenever possible please avoid #ifdef's in .c files.
Agreed. #ifdef CONFIG_OF still seems to be fairly pervasive though...?
>> +static int eeprom_93xx46_probe_dt(struct spi_device *spi)
>> +{
>> + struct device_node *np = spi->dev.of_node;
>> + struct eeprom_93xx46_platform_data *pd;
>> + u32 tmp;
>> + int ret;
>> +
>> + if (!of_match_device(eeprom_93xx46_of_table, &spi->dev))
>> + return 0;
>> +
>> + pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
>> + if (!pd)
>> + return -ENOMEM;
>> +
>> + ret = of_property_read_u32(np, "data-size", &tmp);
>> + if (ret < 0) {
>> + dev_err(&spi->dev, "data-size property not found\n");
>> + goto error_free;
>
> Because you use devm_* resource allocation in .probe, just return error.
Will fix.
> Plus I would suggest to change "data-size" property to an optional one,
> here I mean that if it is omitted, then by default consider pd->flags |=
> EE_ADDR8.
I don't see such an assumption as safe...data word size is an inherent
property of the device (or the way it's strapped on a given platform),
and should be required for proper operation.
>> + }
>> +
>> + if (tmp == 8) {
>> + pd->flags |= EE_ADDR8;
>> + } else if (tmp == 16) {
>> + pd->flags |= EE_ADDR16;
>> + } else {
>> + dev_err(&spi->dev, "invalid data-size (%d)\n", tmp);
>> + goto error_free;
>
> Same here.
Will fix.
>> + }
>> +
>> + if (of_property_read_bool(np, "read-only"))
>> + pd->flags |= EE_READONLY;
>> +
>> + spi->dev.platform_data = pd;
>> +
>> + return 1;
>
> On success please return 0.
Fixed.
>> +error_free:
>> + devm_kfree(&spi->dev, pd);
>> + return ret;
>> +}
>> +
>> +#else
>> +static inline int eeprom_93xx46_probe_dt(struct spi_device *spi)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>
> I actually don't see a point to have #ifdef CONFIG_OF here.
>
> Instead please add a check for !spi->dev.of_node at the beginning of
> eeprom_93xx46_probe_dt() or in .probe()
How about...
if (IS_ENABLED(CONFIG_OF) && spi->dev.of_node) {
err = eeprom_93xx46_probe_dt(spi);
if (err < 0)
return err;
}
...at the beginning of eeprom_93xx46_probe() (as below)?
>> static int eeprom_93xx46_probe(struct spi_device *spi)
>> {
>> struct eeprom_93xx46_platform_data *pd;
>> struct eeprom_93xx46_dev *edev;
>> int err;
>>
>> + err = eeprom_93xx46_probe_dt(spi);
>> + if (err < 0)
>> + return err;
>> +
>> pd = spi->dev.platform_data;
>> if (!pd) {
>> dev_err(&spi->dev, "missing platform data\n");
>> @@ -370,6 +431,7 @@ static int eeprom_93xx46_remove(struct spi_device *spi)
>> static struct spi_driver eeprom_93xx46_driver = {
>> .driver = {
>> .name = "93xx46",
>> + .of_match_table = eeprom_93xx46_of_table,
>> },
>> .probe = eeprom_93xx46_probe,
>> .remove = eeprom_93xx46_remove,
>>
- --
Cory Tusar
Principal
PID 1 Solutions, Inc.
"There are two ways of constructing a software design. One way is to
make it so simple that there are obviously no deficiencies, and the
other way is to make it so complicated that there are no obvious
deficiencies." --Sir Charles Anthony Richard Hoare
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEYEARECAAYFAlZP9cUACgkQHT1tsfGwHJ9XxgCeM7ajaOt2OpGD5Is3+NOeUPTY
z00AoIbY5YFI885NL51XToLgfBNQUQzE
=HBsi
-----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 11/19/2015 09:00 AM, Andrew F. Davis wrote:
> On 11/18/2015 11:50 PM, Vladimir Zapolskiy wrote:
>> Hi Cory,
>>
>> On 19.11.2015 05:29, Cory Tusar wrote:
>>> This commit implements bindings in the eeprom_93xx46 driver allowing
>>> device word size and read-only attributes to be specified via
>>> devicetree.
>>>
>>> Signed-off-by: Cory Tusar <[email protected]>
[snip]
>>> +error_free:
>>> + devm_kfree(&spi->dev, pd);
>>> + return ret;
>>> +}
>>> +
>>> +#else
>>> +static inline int eeprom_93xx46_probe_dt(struct spi_device *spi)
>>> +{
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>
>> I actually don't see a point to have #ifdef CONFIG_OF here.
>>
>
> Usually to avoid a lot of dead code and data when OF is not enabled.
Hi Andrew,
I tend to agree, but I'm going to cross-check by building a couple
variants of this to see just how much gets optimized out automagically
when using Vladimir's suggestions.
>> Instead please add a check for !spi->dev.of_node at the beginning of
>> eeprom_93xx46_probe_dt() or in .probe()
>>
>>> static int eeprom_93xx46_probe(struct spi_device *spi)
>>> {
>>> struct eeprom_93xx46_platform_data *pd;
>>> struct eeprom_93xx46_dev *edev;
>>> int err;
>>>
>>> + err = eeprom_93xx46_probe_dt(spi);
>>> + if (err < 0)
>>> + return err;
>>> +
>>> pd = spi->dev.platform_data;
>>> if (!pd) {
>>> dev_err(&spi->dev, "missing platform data\n");
>>> @@ -370,6 +431,7 @@ static int eeprom_93xx46_remove(struct spi_device *spi)
>>> static struct spi_driver eeprom_93xx46_driver = {
>>> .driver = {
>>> .name = "93xx46",
>>> + .of_match_table = eeprom_93xx46_of_table,
>>> },
>>> .probe = eeprom_93xx46_probe,
>>> .remove = eeprom_93xx46_remove,
>>>
- --
Cory Tusar
Principal
PID 1 Solutions, Inc.
"There are two ways of constructing a software design. One way is to
make it so simple that there are obviously no deficiencies, and the
other way is to make it so complicated that there are no obvious
deficiencies." --Sir Charles Anthony Richard Hoare
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEYEARECAAYFAlZP+LYACgkQHT1tsfGwHJ8GjwCffZac5kr/MHgiLrBh0IxyT6UJ
3rMAn2a/bD5OdWXdxg+DscoHUQbtyHr9
=W3Rd
-----END PGP SIGNATURE-----
On 21.11.2015 06:40, Cory Tusar wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 11/19/2015 12:50 AM, Vladimir Zapolskiy wrote:
>> Hi Cory,
>>
>> On 19.11.2015 05:29, Cory Tusar wrote:
>>> This commit implements bindings in the eeprom_93xx46 driver allowing
>>> device word size and read-only attributes to be specified via
>>> devicetree.
>>>
>>> Signed-off-by: Cory Tusar <[email protected]>
>>> ---
>>> drivers/misc/eeprom/eeprom_93xx46.c | 62 +++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 62 insertions(+)
>>>
>>> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
>>> index e1bf0a5..1f29d9a 100644
>>> --- a/drivers/misc/eeprom/eeprom_93xx46.c
>>> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
>>> @@ -13,6 +13,8 @@
>>> #include <linux/kernel.h>
>>> #include <linux/module.h>
>>> #include <linux/mutex.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> #include <linux/slab.h>
>>> #include <linux/spi/spi.h>
>>> #include <linux/sysfs.h>
>>> @@ -294,12 +296,71 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
>>> }
>>> static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
>>>
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id eeprom_93xx46_of_table[] = {
>>> + { .compatible = "eeprom-93xx46", },
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
>>> +
>>
>> Please move this declaration closer to struct spi_driver
>> eeprom_93xx46_driver below.
>
> As Andrew noted in his follow-up, it's used in the function immediately
> after this declaration. Seems logical to leave it here?
IMO no, see my comment below.
>> Also you can avoid #ifdef here, if you write
>>
>> .of_match_table = of_match_ptr(eeprom_93xx46_of_table)
>
> Will change this to use of_match_ptr().
>
>> Whenever possible please avoid #ifdef's in .c files.
>
> Agreed. #ifdef CONFIG_OF still seems to be fairly pervasive though...?
>
In my opinion it is better to avoid it, and many nice drivers don't have
#ifdef CONFIG_OF.
>>> +static int eeprom_93xx46_probe_dt(struct spi_device *spi)
>>> +{
>>> + struct device_node *np = spi->dev.of_node;
>>> + struct eeprom_93xx46_platform_data *pd;
>>> + u32 tmp;
>>> + int ret;
>>> +
>>> + if (!of_match_device(eeprom_93xx46_of_table, &spi->dev))
>>> + return 0;
This check above is redundant, please remove it.
Imagine, how can you get here !of_match_device(..) condition, if you
have driver initialization from a valid device node?
>>> +
>>> + pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
>>> + if (!pd)
>>> + return -ENOMEM;
>>> +
>>> + ret = of_property_read_u32(np, "data-size", &tmp);
>>> + if (ret < 0) {
>>> + dev_err(&spi->dev, "data-size property not found\n");
>>> + goto error_free;
>>
>> Because you use devm_* resource allocation in .probe, just return error.
>
> Will fix.
>
>> Plus I would suggest to change "data-size" property to an optional one,
>> here I mean that if it is omitted, then by default consider pd->flags |=
>> EE_ADDR8.
>
> I don't see such an assumption as safe...data word size is an inherent
> property of the device (or the way it's strapped on a given platform),
> and should be required for proper operation.
>
Ok.
>>> + }
>>> +
>>> + if (tmp == 8) {
>>> + pd->flags |= EE_ADDR8;
>>> + } else if (tmp == 16) {
>>> + pd->flags |= EE_ADDR16;
>>> + } else {
>>> + dev_err(&spi->dev, "invalid data-size (%d)\n", tmp);
>>> + goto error_free;
>>
>> Same here.
>
> Will fix.
>
>>> + }
>>> +
>>> + if (of_property_read_bool(np, "read-only"))
>>> + pd->flags |= EE_READONLY;
>>> +
>>> + spi->dev.platform_data = pd;
>>> +
>>> + return 1;
>>
>> On success please return 0.
>
> Fixed.
>
>>> +error_free:
>>> + devm_kfree(&spi->dev, pd);
>>> + return ret;
>>> +}
>>> +
>>> +#else
>>> +static inline int eeprom_93xx46_probe_dt(struct spi_device *spi)
>>> +{
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>
>> I actually don't see a point to have #ifdef CONFIG_OF here.
>>
>> Instead please add a check for !spi->dev.of_node at the beginning of
>> eeprom_93xx46_probe_dt() or in .probe()
>
> How about...
>
> if (IS_ENABLED(CONFIG_OF) && spi->dev.of_node) {
> err = eeprom_93xx46_probe_dt(spi);
> if (err < 0)
> return err;
> }
>
> ...at the beginning of eeprom_93xx46_probe() (as below)?
>
if (spi->dev.of_node) {
err = eeprom_93xx46_probe_dt(spi);
if (err < 0)
return err;
}
is good enough.
Condition (!IS_ENABLED(CONFIG_OF) && spi->dev.of_node) is always false.
>>> static int eeprom_93xx46_probe(struct spi_device *spi)
>>> {
>>> struct eeprom_93xx46_platform_data *pd;
>>> struct eeprom_93xx46_dev *edev;
>>> int err;
>>>
>>> + err = eeprom_93xx46_probe_dt(spi);
>>> + if (err < 0)
>>> + return err;
>>> +
>>> pd = spi->dev.platform_data;
>>> if (!pd) {
>>> dev_err(&spi->dev, "missing platform data\n");
>>> @@ -370,6 +431,7 @@ static int eeprom_93xx46_remove(struct spi_device *spi)
>>> static struct spi_driver eeprom_93xx46_driver = {
>>> .driver = {
>>> .name = "93xx46",
>>> + .of_match_table = eeprom_93xx46_of_table,
>>> },
>>> .probe = eeprom_93xx46_probe,
>>> .remove = eeprom_93xx46_remove,
>>>
>
>
--
With best wishes,
Vladimir
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 11/19/2015 12:59 AM, Vladimir Zapolskiy wrote:
> On 19.11.2015 05:29, Cory Tusar wrote:
>> Atmel devices in this family have some quirks not found in other similar
>> chips - they do not support a sequential read of the entire EEPROM
>> contents, and the control word sent at the start of each operation
>> varies in bit length.
>>
>> This commit adds quirk support to the driver and modifies the read
>> implementation to support non-sequential reads for consistency with
>> other misc/eeprom drivers.
>>
>> Tested on a custom Freescale VF610-based platform, with an AT93C46D
>> device attached via dspi2. The spi-gpio driver was used to allow the
>> necessary non-byte-sized transfers.
>>
>> Signed-off-by: Cory Tusar <[email protected]>
>> ---
>> drivers/misc/eeprom/eeprom_93xx46.c | 128 ++++++++++++++++++++++++++----------
>> include/linux/eeprom_93xx46.h | 6 ++
>> 2 files changed, 98 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
>> index 1f29d9a..0386b03 100644
>> --- a/drivers/misc/eeprom/eeprom_93xx46.c
>> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
>> @@ -27,6 +27,15 @@
>> #define ADDR_ERAL 0x20
>> #define ADDR_EWEN 0x30
>>
>> +struct eeprom_93xx46_devtype_data {
>> + unsigned int quirks;
>> +};
>> +
>> +static const struct eeprom_93xx46_devtype_data atmel_at93c46d_data = {
>> + .quirks = EEPROM_93XX46_QUIRK_SINGLE_WORD_READ |
>> + EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH,
>> +};
>> +
>> struct eeprom_93xx46_dev {
>> struct spi_device *spi;
>> struct eeprom_93xx46_platform_data *pdata;
>> @@ -35,6 +44,16 @@ struct eeprom_93xx46_dev {
>> int addrlen;
>> };
>>
>> +static inline bool has_quirk_single_word_read(struct eeprom_93xx46_dev *edev)
>> +{
>> + return !!(edev->pdata->quirks & EEPROM_93XX46_QUIRK_SINGLE_WORD_READ);
>
> bool return type will do the work, please remove !!.
Will fix.
>> +}
>> +
>> +static inline bool has_quirk_instruction_length(struct eeprom_93xx46_dev *edev)
>> +{
>> + return !!(edev->pdata->quirks & EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH);
>
> Same here.
Will fix.
>> +}
>> +
>> static ssize_t
>> eeprom_93xx46_bin_read(struct file *filp, struct kobject *kobj,
>> struct bin_attribute *bin_attr,
>> @@ -42,58 +61,73 @@ eeprom_93xx46_bin_read(struct file *filp, struct kobject *kobj,
>> {
>> struct eeprom_93xx46_dev *edev;
>> struct device *dev;
>> - struct spi_message m;
>> - struct spi_transfer t[2];
>> - int bits, ret;
>> - u16 cmd_addr;
>> + ssize_t ret = 0;
>>
>> dev = container_of(kobj, struct device, kobj);
>> edev = dev_get_drvdata(dev);
>>
>> - cmd_addr = OP_READ << edev->addrlen;
>> + mutex_lock(&edev->lock);
>>
>> - if (edev->addrlen == 7) {
>> - cmd_addr |= off & 0x7f;
>> - bits = 10;
>> - } else {
>> - cmd_addr |= (off >> 1) & 0x3f;
>> - bits = 9;
>> - }
>> + if (edev->pdata->prepare)
>> + edev->pdata->prepare(edev);
>>
>> - dev_dbg(&edev->spi->dev, "read cmd 0x%x, %d Hz\n",
>> - cmd_addr, edev->spi->max_speed_hz);
>> + while (count) {
>> + struct spi_message m;
>> + struct spi_transfer t[2] = { { 0 } };
>
> Just { 0 };
No...just '{ 0 }' may be functional, but results in a compiler
warning.
>> + u16 cmd_addr = OP_READ << edev->addrlen;
>> + size_t nbytes = count;
>> + int bits;
>> + int err;
>> +
>> + if (edev->addrlen == 7) {
>> + cmd_addr |= off & 0x7f;
>> + bits = 10;
>> + if (has_quirk_single_word_read(edev))
>> + nbytes = 1;
>> + } else {
>> + cmd_addr |= (off >> 1) & 0x3f;
>> + bits = 9;
>> + if (has_quirk_single_word_read(edev))
>> + nbytes = 2;
>> + }
>>
>> - spi_message_init(&m);
>> - memset(t, 0, sizeof(t));
>> + dev_dbg(&edev->spi->dev, "read cmd 0x%x, %d Hz\n",
>> + cmd_addr, edev->spi->max_speed_hz);
>>
>> - t[0].tx_buf = (char *)&cmd_addr;
>> - t[0].len = 2;
>> - t[0].bits_per_word = bits;
>> - spi_message_add_tail(&t[0], &m);
>> + spi_message_init(&m);
>>
>> - t[1].rx_buf = buf;
>> - t[1].len = count;
>> - t[1].bits_per_word = 8;
>> - spi_message_add_tail(&t[1], &m);
>> + t[0].tx_buf = (char *)&cmd_addr;
>> + t[0].len = 2;
>> + t[0].bits_per_word = bits;
>> + spi_message_add_tail(&t[0], &m);
>>
>> - mutex_lock(&edev->lock);
>> + t[1].rx_buf = buf;
>> + t[1].len = count;
>> + t[1].bits_per_word = 8;
>> + spi_message_add_tail(&t[1], &m);
>>
>> - if (edev->pdata->prepare)
>> - edev->pdata->prepare(edev);
>> + err = spi_sync(edev->spi, &m);
>> + /* have to wait at least Tcsl ns */
>> + ndelay(250);
>>
>> - ret = spi_sync(edev->spi, &m);
>> - /* have to wait at least Tcsl ns */
>> - ndelay(250);
>> - if (ret) {
>> - dev_err(&edev->spi->dev, "read %zu bytes at %d: err. %d\n",
>> - count, (int)off, ret);
>> + if (err) {
>> + dev_err(&edev->spi->dev, "read %zu bytes at %d: err. %d\n",
>> + nbytes, (int)off, err);
>> + ret = err;
>> + break;
>> + }
>> +
>> + buf += nbytes;
>> + off += nbytes;
>> + count -= nbytes;
>> + ret += nbytes;
>> }
>>
>> if (edev->pdata->finish)
>> edev->pdata->finish(edev);
>>
>> mutex_unlock(&edev->lock);
>> - return ret ? : count;
>> + return ret;
>> }
>>
>> static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
>> @@ -112,7 +146,13 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
>> bits = 9;
>> }
>>
>> - dev_dbg(&edev->spi->dev, "ew cmd 0x%04x\n", cmd_addr);
>> + if (has_quirk_instruction_length(edev)) {
>> + cmd_addr <<= 2;
>> + bits += 2;
>> + }
>> +
>> + dev_dbg(&edev->spi->dev, "ew%s cmd 0x%04x, %d bits\n",
>> + is_on ? "en" : "ds", cmd_addr, bits);
>>
>> spi_message_init(&m);
>> memset(&t, 0, sizeof(t));
>> @@ -247,6 +287,13 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
>> bits = 9;
>> }
>>
>> + if (has_quirk_instruction_length(edev)) {
>> + cmd_addr <<= 2;
>> + bits += 2;
>> + }
>> +
>> + dev_dbg(&edev->spi->dev, "eral cmd 0x%04x, %d bits\n", cmd_addr, bits);
>> +
>> spi_message_init(&m);
>> memset(&t, 0, sizeof(t));
>>
>> @@ -299,18 +346,21 @@ static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
>> #ifdef CONFIG_OF
>> static const struct of_device_id eeprom_93xx46_of_table[] = {
>> { .compatible = "eeprom-93xx46", },
>> + { .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
>> {}
>> };
>> MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
>>
>> static int eeprom_93xx46_probe_dt(struct spi_device *spi)
>> {
>> + const struct of_device_id *of_id =
>> + of_match_device(eeprom_93xx46_of_table, &spi->dev);
>> struct device_node *np = spi->dev.of_node;
>> struct eeprom_93xx46_platform_data *pd;
>> u32 tmp;
>> int ret;
>>
>> - if (!of_match_device(eeprom_93xx46_of_table, &spi->dev))
>> + if (!of_id)
>> return 0;
>>
>> pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
>> @@ -335,6 +385,12 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
>> if (of_property_read_bool(np, "read-only"))
>> pd->flags |= EE_READONLY;
>>
>> + if (of_id->data) {
>> + const struct eeprom_93xx46_devtype_data *data = of_id->data;
>> +
>> + pd->quirks = data->quirks;
>> + }
>> +
>> spi->dev.platform_data = pd;
>>
>> return 1;
>> diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
>> index 0679181..92fa4c3 100644
>> --- a/include/linux/eeprom_93xx46.h
>> +++ b/include/linux/eeprom_93xx46.h
>> @@ -9,6 +9,12 @@ struct eeprom_93xx46_platform_data {
>> #define EE_ADDR16 0x02 /* 16 bit addr. cfg */
>> #define EE_READONLY 0x08 /* forbid writing */
>>
>> + unsigned int quirks;
>> +/* Single word read transfers only; no sequential read. */
>> +#define EEPROM_93XX46_QUIRK_SINGLE_WORD_READ (1 << 0)
>> +/* Instructions such as EWEN are (addrlen + 2) in length. */
>> +#define EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH (1 << 1)
>> +
>
> I wonder do you really need this in platfrom data? Would it work for
> you, if quirks are OF specific only? If yes, then please move macros to
> .c file.
The driver currently supports instantiation as a platform_device only.
Why not continue to support this, while also adding the capability to
instantiate via DT?
>
>> /*
>> * optional hooks to control additional logic
>> * before and after spi transfer.
>>
>
> --
> With best wishes,
> Vladimir
>
- --
Cory Tusar
Principal
PID 1 Solutions, Inc.
"There are two ways of constructing a software design. One way is to
make it so simple that there are obviously no deficiencies, and the
other way is to make it so complicated that there are no obvious
deficiencies." --Sir Charles Anthony Richard Hoare
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEYEARECAAYFAlZTUIwACgkQHT1tsfGwHJ+eaQCfUPrpmynJlsJArLgpooe5JVfO
AF4AoIG09XYhMuQ+YFjckwE3LNxVPHNf
=p86d
-----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 11/21/2015 01:36 PM, Vladimir Zapolskiy wrote:
> On 21.11.2015 06:40, Cory Tusar wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 11/19/2015 12:50 AM, Vladimir Zapolskiy wrote:
>>> Hi Cory,
>>>
>>> On 19.11.2015 05:29, Cory Tusar wrote:
>>>> This commit implements bindings in the eeprom_93xx46 driver allowing
>>>> device word size and read-only attributes to be specified via
>>>> devicetree.
>>>>
>>>> Signed-off-by: Cory Tusar <[email protected]>
>>>> ---
>>>> drivers/misc/eeprom/eeprom_93xx46.c | 62 +++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 62 insertions(+)
>>>>
>>>> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
>>>> index e1bf0a5..1f29d9a 100644
>>>> --- a/drivers/misc/eeprom/eeprom_93xx46.c
>>>> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
>>>> @@ -13,6 +13,8 @@
>>>> #include <linux/kernel.h>
>>>> #include <linux/module.h>
>>>> #include <linux/mutex.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>> #include <linux/slab.h>
>>>> #include <linux/spi/spi.h>
>>>> #include <linux/sysfs.h>
>>>> @@ -294,12 +296,71 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
>>>> }
>>>> static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
>>>>
>>>> +#ifdef CONFIG_OF
>>>> +static const struct of_device_id eeprom_93xx46_of_table[] = {
>>>> + { .compatible = "eeprom-93xx46", },
>>>> + {}
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
>>>> +
>>>
>>> Please move this declaration closer to struct spi_driver
>>> eeprom_93xx46_driver below.
>>
>> As Andrew noted in his follow-up, it's used in the function immediately
>> after this declaration. Seems logical to leave it here?
>
> IMO no, see my comment below.
...keep in mind also that it needs to be _here_ for quirk support (next
patch) as well.
>>> Also you can avoid #ifdef here, if you write
>>>
>>> .of_match_table = of_match_ptr(eeprom_93xx46_of_table)
>>
>> Will change this to use of_match_ptr().
>>
>>> Whenever possible please avoid #ifdef's in .c files.
>>
>> Agreed. #ifdef CONFIG_OF still seems to be fairly pervasive though...?
>>
>
> In my opinion it is better to avoid it, and many nice drivers don't have
> #ifdef CONFIG_OF.
>
>>>> +static int eeprom_93xx46_probe_dt(struct spi_device *spi)
>>>> +{
>>>> + struct device_node *np = spi->dev.of_node;
>>>> + struct eeprom_93xx46_platform_data *pd;
>>>> + u32 tmp;
>>>> + int ret;
>>>> +
>>>> + if (!of_match_device(eeprom_93xx46_of_table, &spi->dev))
>>>> + return 0;
>
> This check above is redundant, please remove it.
>
> Imagine, how can you get here !of_match_device(..) condition, if you
> have driver initialization from a valid device node?
Will fix.
>>>> +
>>>> + pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
>>>> + if (!pd)
>>>> + return -ENOMEM;
>>>> +
>>>> + ret = of_property_read_u32(np, "data-size", &tmp);
>>>> + if (ret < 0) {
>>>> + dev_err(&spi->dev, "data-size property not found\n");
>>>> + goto error_free;
>>>
>>> Because you use devm_* resource allocation in .probe, just return error.
>>
>> Will fix.
>>
>>> Plus I would suggest to change "data-size" property to an optional one,
>>> here I mean that if it is omitted, then by default consider pd->flags |=
>>> EE_ADDR8.
>>
>> I don't see such an assumption as safe...data word size is an inherent
>> property of the device (or the way it's strapped on a given platform),
>> and should be required for proper operation.
>>
>
> Ok.
>
>>>> + }
>>>> +
>>>> + if (tmp == 8) {
>>>> + pd->flags |= EE_ADDR8;
>>>> + } else if (tmp == 16) {
>>>> + pd->flags |= EE_ADDR16;
>>>> + } else {
>>>> + dev_err(&spi->dev, "invalid data-size (%d)\n", tmp);
>>>> + goto error_free;
>>>
>>> Same here.
>>
>> Will fix.
>>
>>>> + }
>>>> +
>>>> + if (of_property_read_bool(np, "read-only"))
>>>> + pd->flags |= EE_READONLY;
>>>> +
>>>> + spi->dev.platform_data = pd;
>>>> +
>>>> + return 1;
>>>
>>> On success please return 0.
>>
>> Fixed.
>>
>>>> +error_free:
>>>> + devm_kfree(&spi->dev, pd);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +#else
>>>> +static inline int eeprom_93xx46_probe_dt(struct spi_device *spi)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>
>>> I actually don't see a point to have #ifdef CONFIG_OF here.
>>>
>>> Instead please add a check for !spi->dev.of_node at the beginning of
>>> eeprom_93xx46_probe_dt() or in .probe()
>>
>> How about...
>>
>> if (IS_ENABLED(CONFIG_OF) && spi->dev.of_node) {
>> err = eeprom_93xx46_probe_dt(spi);
>> if (err < 0)
>> return err;
>> }
>>
>> ...at the beginning of eeprom_93xx46_probe() (as below)?
>>
>
> if (spi->dev.of_node) {
> err = eeprom_93xx46_probe_dt(spi);
> if (err < 0)
> return err;
> }
>
> is good enough.
>
> Condition (!IS_ENABLED(CONFIG_OF) && spi->dev.of_node) is always false.
Please re-read the above - there's no negation on the IS_ENABLED()
portion... :)
That stated, a quick build test shows that just the check for
spi->dev.of_node is sufficient to short-circuit (at compile-time) the
DT-specific probe.
This driver previously supported instantiation only as a
platform_device; I'm trying not to break that, just add to it...
Thanks for all the review.
>>>> static int eeprom_93xx46_probe(struct spi_device *spi)
>>>> {
>>>> struct eeprom_93xx46_platform_data *pd;
>>>> struct eeprom_93xx46_dev *edev;
>>>> int err;
>>>>
>>>> + err = eeprom_93xx46_probe_dt(spi);
>>>> + if (err < 0)
>>>> + return err;
>>>> +
>>>> pd = spi->dev.platform_data;
>>>> if (!pd) {
>>>> dev_err(&spi->dev, "missing platform data\n");
>>>> @@ -370,6 +431,7 @@ static int eeprom_93xx46_remove(struct spi_device *spi)
>>>> static struct spi_driver eeprom_93xx46_driver = {
>>>> .driver = {
>>>> .name = "93xx46",
>>>> + .of_match_table = eeprom_93xx46_of_table,
>>>> },
>>>> .probe = eeprom_93xx46_probe,
>>>> .remove = eeprom_93xx46_remove,
>>>>
>>
>>
> --
> With best wishes,
> Vladimir
>
- --
Cory Tusar
Principal
PID 1 Solutions, Inc.
"There are two ways of constructing a software design. One way is to
make it so simple that there are obviously no deficiencies, and the
other way is to make it so complicated that there are no obvious
deficiencies." --Sir Charles Anthony Richard Hoare
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEYEARECAAYFAlZTWc8ACgkQHT1tsfGwHJ/mKwCgnmCkDTbUPjusqHFCcE37D6qs
Ht0AnR1NEIQ+OT9eO5l8DaQSWs1mlOR2
=VVAx
-----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 11/19/2015 01:05 AM, Vladimir Zapolskiy wrote:
> On 19.11.2015 05:29, Cory Tusar wrote:
>> This commit adds support to the eeprom_93x46 driver allowing a GPIO line
>> to function as a 'select' or 'enable' signal prior to accessing the
>> EEPROM.
>>
>> Signed-off-by: Cory Tusar <[email protected]>
>> ---
>> drivers/misc/eeprom/eeprom_93xx46.c | 26 ++++++++++++++++++++++++++
>> include/linux/eeprom_93xx46.h | 1 +
>> 2 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
>> index 0386b03..375951f 100644
>> --- a/drivers/misc/eeprom/eeprom_93xx46.c
>> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
>> @@ -10,11 +10,14 @@
>>
>> #include <linux/delay.h>
>> #include <linux/device.h>
>> +#include <linux/gpio.h>
>> +#include <linux/gpio/consumer.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/mutex.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> +#include <linux/of_gpio.h>
>
> Please double check, adding only linux/of_gpio.h header should work,
> linux/gpio.h and linux/gpio/consumer.h are redundant.
There was an error which turned up on a 0-day build related to this:
tree: https://github.com/lunn/linux.git asl_v4.3-rc2-zii-stable-dsa-reset
head: c91bad95b39a98e0d06809c4c70c9c26747c874a
commit: a3e1b85039c722799102366b527b6bab9543e4ac [4/41] misc: eeprom: 93xx46: Add support for a GPIO 'select' line.
config: x86_64-randconfig-x007-11010710 (attached as .config)
reproduce:
git checkout a3e1b85039c722799102366b527b6bab9543e4ac
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/misc/eeprom/eeprom_93xx46.c: In function 'select_assert':
>> drivers/misc/eeprom/eeprom_93xx46.c:342:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
^
>> drivers/misc/eeprom/eeprom_93xx46.c:342:27: error: implicit declaration of function 'gpio_to_desc' [-Werror=implicit-function-declaration]
gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
I'll re-check with v3 (where everything uses the gpiod_*() interface) to
see if this can be eliminated...
>> #include <linux/slab.h>
>> #include <linux/spi/spi.h>
>> #include <linux/sysfs.h>
>> @@ -344,6 +347,20 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
>> static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
>>
>> #ifdef CONFIG_OF
>> +static void select_assert(void *context)
>> +{
>> + struct eeprom_93xx46_dev *edev = context;
>> +
>> + gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
>
> I would suggest to use gpio_set_value()
v3 uses gpiod_*() throughout. This also addresses an issue where flags
were not being tracked and used properly...
>> +}
>> +
>> +static void select_deassert(void *context)
>> +{
>> + struct eeprom_93xx46_dev *edev = context;
>> +
>> + gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 0);
>
> Same here.
As above.
>> +}
>> +
>> static const struct of_device_id eeprom_93xx46_of_table[] = {
>> { .compatible = "eeprom-93xx46", },
>> { .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
>> @@ -385,6 +402,15 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
>> if (of_property_read_bool(np, "read-only"))
>> pd->flags |= EE_READONLY;
>>
>> + ret = of_get_named_gpio(np, "select-gpios", 0);
>
> gpios or gpio? I see only one requested gpio.
gpios - for consistency.
>> + if (ret < 0) {
>> + pd->select_gpio = -1;
>> + } else {
>> + pd->select_gpio = ret;
>> + pd->prepare = select_assert;
>> + pd->finish = select_deassert;
>> + }
>> +
>> if (of_id->data) {
>> const struct eeprom_93xx46_devtype_data *data = of_id->data;
>>
>> diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
>> index 92fa4c3..aa472c7 100644
>> --- a/include/linux/eeprom_93xx46.h
>> +++ b/include/linux/eeprom_93xx46.h
>> @@ -21,4 +21,5 @@ struct eeprom_93xx46_platform_data {
>> */
>> void (*prepare)(void *);
>> void (*finish)(void *);
>> + unsigned int select_gpio;
>
> Same questions as in v2 4/5.
I simply see it as more straightforward to keep all platform-specific
data together, rather than mix-and-match between eeprom_93xx46_dev and
eeprom_93xx46_platform_data...
Also, the private eeprom_93xx46_dev structure has not been allocated
prior to parsing for DT bindings (without additional restructuring of
.probe() logic).
>> };
>>
>
> --
> With best wishes,
> Vladimir
>
- --
Cory Tusar
Principal
PID 1 Solutions, Inc.
"There are two ways of constructing a software design. One way is to
make it so simple that there are obviously no deficiencies, and the
other way is to make it so complicated that there are no obvious
deficiencies." --Sir Charles Anthony Richard Hoare
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEUEARECAAYFAlZVPrUACgkQHT1tsfGwHJ8qxQCdEc01RKpHTX2aQepam4J9AweJ
ODsAmKMxPN+ljW/4vBQ7dr9ZXHcj3HQ=
=XmRI
-----END PGP SIGNATURE-----