| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 19 Nov 2015 07:59:40 +0200
From: Vladimir Zapolskiy <vz@...ia.com>
To: Cory Tusar <cory.tusar@...1solutions.com>, robh+dt@...nel.org,
pawel.moll@....com, mark.rutland@....com,
ijc+devicetree@...lion.org.uk, galak@...eaurora.org, agust@...x.de,
gregkh@...uxfoundation.org
CC: jic23@...nel.org, broonie@...nel.org, afd@...com, andrew@...n.ch,
Chris.Healy@....aero, Keith.Vennel@....aero,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/5] misc: eeprom_93xx46: Add quirks to support Atmel
AT93C46D device.
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 <cory.tusar@...1solutions.com>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists