Re: [lm-sensors] [PATCH 6/8] hwmon: add max1111/max1110 Low-power

Re: [lm-sensors] [PATCH 6/8] hwmon: add max1111/max1110 Low-power

[Eric Miao]

Driver based on corgi_ssp.c and sharpsl_pm.c, previously done by Richard
Purdie and many others.

Now changed to generic HWMON device and expose all the ADC input value
through sysfs.

Signed-off-by: Eric Miao <eric.miao@marvell.com>
---

Updated and incorporated most feedback. Jean, could you please take a
look and Ack, or let me know know who's the right guy to ack this, thanks.

 drivers/hwmon/Kconfig   |    9 ++
 drivers/hwmon/Makefile  |    1 +
 drivers/hwmon/max1111.c |  235 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 245 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/max1111.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index d402e8d..3309e86 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -540,6 +540,15 @@ config SENSORS_LM93
 	  This driver can also be built as a module.  If so, the module
 	  will be called lm93.

+config SENSORS_MAX1111
+	tristate "Maxim MAX1111 Multichannel, Serial 8-bit ADC chip"
+	depends on SPI_MASTER
+	help
+	  Say y here to support Maxim's MAX1111 ADC chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called max1111.
+
 config SENSORS_MAX1619
 	tristate "Maxim MAX1619 sensor chip"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 950134a..6babc80 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_SENSORS_LM87)	+= lm87.o
 obj-$(CONFIG_SENSORS_LM90)	+= lm90.o
 obj-$(CONFIG_SENSORS_LM92)	+= lm92.o
 obj-$(CONFIG_SENSORS_LM93)	+= lm93.o
+obj-$(CONFIG_SENSORS_MAX1111)	+= max1111.o
 obj-$(CONFIG_SENSORS_MAX1619)	+= max1619.o
 obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
new file mode 100644
index 0000000..5a974f8
--- /dev/null
+++ b/drivers/hwmon/max1111.c
@@ -0,0 +1,235 @@
+/*
+ * max1111.c - +2.7V, Low-Power, Multichannel, Serial 8-bit ADCs
+ *
+ * Based on arch/arm/mach-pxa/corgi_ssp.c
+ *
+ * Copyright (C) 2004-2005 Richard Purdie
+ *
+ * Copyright (C) 2008 Marvell International Ltd.
+ * 	Eric Miao <eric.miao@marvell.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  publishhed by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/spi/spi.h>
+
+#define MAX1111_TX_BUF_SIZE	(1)
+#define MAX1111_RX_BUF_SIZE	(2)
+
+/* MAX1111 Commands */
+#define MAX1111_CTRL_PD0      (1u << 0)
+#define MAX1111_CTRL_PD1      (1u << 1)
+#define MAX1111_CTRL_SGL      (1u << 2)
+#define MAX1111_CTRL_UNI      (1u << 3)
+#define MAX1111_CTRL_SEL_SH   (4)
+#define MAX1111_CTRL_STR      (1u << 7)
+
+static ssize_t show_adc(struct device *, struct device_attribute *, char *);
+static ssize_t show_name(struct device *, struct device_attribute *, char *);
+static int max1111_read(struct device *, int);
+
+#define MAX1111_ADC_ATTR(_id)		\
+	SENSOR_DEVICE_ATTR(adc##_id##_in, S_IRUGO, show_adc, NULL, _id)
+
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+static MAX1111_ADC_ATTR(0);
+static MAX1111_ADC_ATTR(1);
+static MAX1111_ADC_ATTR(2);
+static MAX1111_ADC_ATTR(3);
+
+static struct attribute *max1111_attributes[] = {
+	&dev_attr_name.attr,
+	&sensor_dev_attr_adc0_in.dev_attr.attr,
+	&sensor_dev_attr_adc1_in.dev_attr.attr,
+	&sensor_dev_attr_adc2_in.dev_attr.attr,
+	&sensor_dev_attr_adc3_in.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group max1111_attr_group = {
+	.attrs	= max1111_attributes,
+};
+
+/*
+ * NOTE: SPI devices do not have a default 'name' attribute, which is
+ * likely to be used by hwmon applications to distinguish between
+ * different devices, explicitly add a name attribute here.
+ */
+static ssize_t show_name(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "max1111\n");
+}
+
+static ssize_t show_adc(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	int channel = to_sensor_dev_attr(attr)->index;
+	int ret;
+
+	ret = max1111_read(dev, channel);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", ret);
+}
+
+struct max1111_data {
+	struct spi_device	*spi;
+	struct device		*hwmon_dev;
+	struct spi_message	msg;
+	struct spi_transfer	xfer[2];
+	uint8_t *tx_buf;
+	uint8_t *rx_buf;
+};
+
+static int max1111_read(struct device *dev, int channel)
+{
+	struct max1111_data *data = dev_get_drvdata(dev);
+	uint8_t v1, v2;
+	int err;
+
+	data->tx_buf[0] = (channel << MAX1111_CTRL_SEL_SH) |
+		MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 |
+		MAX1111_CTRL_SGL | MAX1111_CTRL_UNI | MAX1111_CTRL_STR;
+
+	err = spi_sync(data->spi, &data->msg);
+	if (err < 0) {
+		dev_err(dev, "spi_sync failed with %d\n", err);
+		return err;
+	}
+
+	v1 = data->rx_buf[0];
+	v2 = data->rx_buf[1];
+
+	if ((v1 & 0xc0) || (v2 & 0x3f))
+		return -EINVAL;
+
+	return ((v1 << 2) & 0xfc) | ((v2 >> 6) & 0x03);
+}
+
+static int setup_transfer(struct max1111_data *data)
+{
+	struct spi_message *m;
+	struct spi_transfer *x;
+
+	data->tx_buf = kmalloc(MAX1111_TX_BUF_SIZE, GFP_KERNEL);
+	if (!data->tx_buf)
+		return -ENOMEM;
+
+	data->rx_buf = kmalloc(MAX1111_RX_BUF_SIZE, GFP_KERNEL);
+	if (!data->rx_buf) {
+		kfree(data->tx_buf);
+		return -ENOMEM;
+	}
+
+	m = &data->msg;
+	x = &data->xfer[0];
+
+	spi_message_init(m);
+
+	x->tx_buf = &data->tx_buf[0];
+	x->len = 1;
+	spi_message_add_tail(x, m);
+
+	x++;
+	x->rx_buf = &data->rx_buf[0];
+	x->len = 2;
+	spi_message_add_tail(x, m);
+
+	return 0;
+}
+
+static int __devinit max1111_probe(struct spi_device *spi)
+{
+	struct max1111_data *data;
+	int err;
+
+	spi->bits_per_word = 8;
+	spi->mode = SPI_MODE_0;
+	err = spi_setup(spi);
+	if (err < 0)
+		return err;
+
+	data = kzalloc(sizeof(struct max1111_data), GFP_KERNEL);
+	if (data = NULL) {
+		dev_err(&spi->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	err = setup_transfer(data);
+	if (err)
+		goto err_free_data;
+
+	data->spi = spi;
+	spi_set_drvdata(spi, data);
+
+	err = sysfs_create_group(&spi->dev.kobj, &max1111_attr_group);
+	if (err) {
+		dev_err(&spi->dev, "failed to create attribute group\n");
+		goto err_free_all;
+	}
+
+	data->hwmon_dev = hwmon_device_register(&spi->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		dev_err(&spi->dev, "failed to create hwmon device\n");
+		err = PTR_ERR(data->hwmon_dev);
+		goto err_remove;
+	}
+
+	return 0;
+
+err_remove:
+	sysfs_remove_group(&spi->dev.kobj, &max1111_attr_group);
+err_free_all:
+	kfree(data->rx_buf);
+	kfree(data->tx_buf);
+err_free_data:
+	kfree(data);
+	return err;
+}
+
+static int __devexit max1111_remove(struct spi_device *spi)
+{
+	struct max1111_data *data = spi_get_drvdata(spi);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&spi->dev.kobj, &max1111_attr_group);
+	kfree(data->rx_buf);
+	kfree(data->tx_buf);
+	kfree(data);
+	return 0;
+}
+
+static struct spi_driver max1111_driver = {
+	.driver		= {
+		.name	= "max1111",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= max1111_probe,
+	.remove		= __devexit_p(max1111_remove),
+};
+
+static int __init max1111_init(void)
+{
+	return spi_register_driver(&max1111_driver);
+}
+module_init(max1111_init);
+
+static void __exit max1111_exit(void)
+{
+	spi_unregister_driver(&max1111_driver);
+}
+module_exit(max1111_exit);
+
+MODULE_AUTHOR("Eric Miao <eric.miao@marvell.com>");
+MODULE_DESCRIPTION("MAX1111 ADC Driver");
+MODULE_LICENSE("GPL");
-- 
1.5.4.3

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 6/8] hwmon: add max1111/max1110 Low-power

[Jean Delvare]

Hi Eric,

Sorry for the late answer.

On Wed, 3 Sep 2008 14:12:26 +0800, Eric Miao wrote:
> Driver based on corgi_ssp.c and sharpsl_pm.c, previously done by Richard
> Purdie and many others.
> 
> Now changed to generic HWMON device and expose all the ADC input value
> through sysfs.
> 
> Signed-off-by: Eric Miao <eric.miao@marvell.com>
> ---
> 
> Updated and incorporated most feedback. Jean, could you please take a
> look and Ack, or let me know know who's the right guy to ack this, thanks.

Code looks reasonable, so you get my:

Acked-by: Jean Delvare <khali@linux-fr.org>

However, please read my few suggestions below to make the driver even
better.

> 
>  drivers/hwmon/Kconfig   |    9 ++
>  drivers/hwmon/Makefile  |    1 +
>  drivers/hwmon/max1111.c |  235 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 245 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hwmon/max1111.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index d402e8d..3309e86 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -540,6 +540,15 @@ config SENSORS_LM93
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called lm93.
> 
> +config SENSORS_MAX1111
> +	tristate "Maxim MAX1111 Multichannel, Serial 8-bit ADC chip"
> +	depends on SPI_MASTER
> +	help
> +	  Say y here to support Maxim's MAX1111 ADC chips.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called max1111.
> +
>  config SENSORS_MAX1619
>  	tristate "Maxim MAX1619 sensor chip"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 950134a..6babc80 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_SENSORS_LM87)	+= lm87.o
>  obj-$(CONFIG_SENSORS_LM90)	+= lm90.o
>  obj-$(CONFIG_SENSORS_LM92)	+= lm92.o
>  obj-$(CONFIG_SENSORS_LM93)	+= lm93.o
> +obj-$(CONFIG_SENSORS_MAX1111)	+= max1111.o
>  obj-$(CONFIG_SENSORS_MAX1619)	+= max1619.o
>  obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
>  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
> diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
> new file mode 100644
> index 0000000..5a974f8
> --- /dev/null
> +++ b/drivers/hwmon/max1111.c
> @@ -0,0 +1,235 @@
> +/*
> + * max1111.c - +2.7V, Low-Power, Multichannel, Serial 8-bit ADCs
> + *
> + * Based on arch/arm/mach-pxa/corgi_ssp.c
> + *
> + * Copyright (C) 2004-2005 Richard Purdie
> + *
> + * Copyright (C) 2008 Marvell International Ltd.
> + * 	Eric Miao <eric.miao@marvell.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  publishhed by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/spi/spi.h>
> +
> +#define MAX1111_TX_BUF_SIZE	(1)
> +#define MAX1111_RX_BUF_SIZE	(2)

Note that parentheses aren't needed here.

> +
> +/* MAX1111 Commands */
> +#define MAX1111_CTRL_PD0      (1u << 0)
> +#define MAX1111_CTRL_PD1      (1u << 1)
> +#define MAX1111_CTRL_SGL      (1u << 2)
> +#define MAX1111_CTRL_UNI      (1u << 3)
> +#define MAX1111_CTRL_SEL_SH   (4)
> +#define MAX1111_CTRL_STR      (1u << 7)
> +
> +static ssize_t show_adc(struct device *, struct device_attribute *, char *);
> +static ssize_t show_name(struct device *, struct device_attribute *, char *);

You could easily get rid of these forward declarations by moving the
attribute declarations below after the functions themselves.

> +static int max1111_read(struct device *, int);
> +
> +#define MAX1111_ADC_ATTR(_id)		\
> +	SENSOR_DEVICE_ATTR(adc##_id##_in, S_IRUGO, show_adc, NULL, _id)
> +
> +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> +static MAX1111_ADC_ATTR(0);
> +static MAX1111_ADC_ATTR(1);
> +static MAX1111_ADC_ATTR(2);
> +static MAX1111_ADC_ATTR(3);
> +
> +static struct attribute *max1111_attributes[] = {
> +	&dev_attr_name.attr,
> +	&sensor_dev_attr_adc0_in.dev_attr.attr,
> +	&sensor_dev_attr_adc1_in.dev_attr.attr,
> +	&sensor_dev_attr_adc2_in.dev_attr.attr,
> +	&sensor_dev_attr_adc3_in.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group max1111_attr_group = {
> +	.attrs	= max1111_attributes,
> +};
> +
> +/*
> + * NOTE: SPI devices do not have a default 'name' attribute, which is
> + * likely to be used by hwmon applications to distinguish between
> + * different devices, explicitly add a name attribute here.
> + */
> +static ssize_t show_name(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "max1111\n");
> +}
> +
> +static ssize_t show_adc(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	int channel = to_sensor_dev_attr(attr)->index;
> +	int ret;
> +
> +	ret = max1111_read(dev, channel);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", ret);
> +}
> +
> +struct max1111_data {
> +	struct spi_device	*spi;
> +	struct device		*hwmon_dev;
> +	struct spi_message	msg;
> +	struct spi_transfer	xfer[2];
> +	uint8_t *tx_buf;
> +	uint8_t *rx_buf;
> +};
> +
> +static int max1111_read(struct device *dev, int channel)
> +{
> +	struct max1111_data *data = dev_get_drvdata(dev);
> +	uint8_t v1, v2;
> +	int err;
> +
> +	data->tx_buf[0] = (channel << MAX1111_CTRL_SEL_SH) |
> +		MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 |
> +		MAX1111_CTRL_SGL | MAX1111_CTRL_UNI | MAX1111_CTRL_STR;
> +
> +	err = spi_sync(data->spi, &data->msg);
> +	if (err < 0) {
> +		dev_err(dev, "spi_sync failed with %d\n", err);
> +		return err;
> +	}
> +
> +	v1 = data->rx_buf[0];
> +	v2 = data->rx_buf[1];
> +
> +	if ((v1 & 0xc0) || (v2 & 0x3f))
> +		return -EINVAL;
> +
> +	return ((v1 << 2) & 0xfc) | ((v2 >> 6) & 0x03);

The masking is not needed: you have explicitly checked right above that
the bits you're masking out are 0s.

> +}
> +
> +static int setup_transfer(struct max1111_data *data)
> +{
> +	struct spi_message *m;
> +	struct spi_transfer *x;
> +
> +	data->tx_buf = kmalloc(MAX1111_TX_BUF_SIZE, GFP_KERNEL);
> +	if (!data->tx_buf)
> +		return -ENOMEM;
> +
> +	data->rx_buf = kmalloc(MAX1111_RX_BUF_SIZE, GFP_KERNEL);
> +	if (!data->rx_buf) {
> +		kfree(data->tx_buf);
> +		return -ENOMEM;
> +	}

Allocating such small buffers using kmalloc seems pretty inefficient.
At the very least, I would allocate both buffers at once. But quite
frankly I don't get why you don't just make these buffers part of
struct max1111_data. This would even make the structure smaller!

> +
> +	m = &data->msg;
> +	x = &data->xfer[0];
> +
> +	spi_message_init(m);
> +
> +	x->tx_buf = &data->tx_buf[0];
> +	x->len = 1;
> +	spi_message_add_tail(x, m);
> +
> +	x++;
> +	x->rx_buf = &data->rx_buf[0];
> +	x->len = 2;
> +	spi_message_add_tail(x, m);
> +
> +	return 0;
> +}
> +
> +static int __devinit max1111_probe(struct spi_device *spi)
> +{
> +	struct max1111_data *data;
> +	int err;
> +
> +	spi->bits_per_word = 8;
> +	spi->mode = SPI_MODE_0;
> +	err = spi_setup(spi);
> +	if (err < 0)
> +		return err;
> +
> +	data = kzalloc(sizeof(struct max1111_data), GFP_KERNEL);
> +	if (data = NULL) {
> +		dev_err(&spi->dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	err = setup_transfer(data);
> +	if (err)
> +		goto err_free_data;
> +
> +	data->spi = spi;
> +	spi_set_drvdata(spi, data);
> +
> +	err = sysfs_create_group(&spi->dev.kobj, &max1111_attr_group);
> +	if (err) {
> +		dev_err(&spi->dev, "failed to create attribute group\n");
> +		goto err_free_all;
> +	}
> +
> +	data->hwmon_dev = hwmon_device_register(&spi->dev);
> +	if (IS_ERR(data->hwmon_dev)) {
> +		dev_err(&spi->dev, "failed to create hwmon device\n");
> +		err = PTR_ERR(data->hwmon_dev);
> +		goto err_remove;
> +	}
> +
> +	return 0;
> +
> +err_remove:
> +	sysfs_remove_group(&spi->dev.kobj, &max1111_attr_group);
> +err_free_all:
> +	kfree(data->rx_buf);
> +	kfree(data->tx_buf);
> +err_free_data:
> +	kfree(data);
> +	return err;
> +}
> +
> +static int __devexit max1111_remove(struct spi_device *spi)
> +{
> +	struct max1111_data *data = spi_get_drvdata(spi);
> +
> +	hwmon_device_unregister(data->hwmon_dev);
> +	sysfs_remove_group(&spi->dev.kobj, &max1111_attr_group);
> +	kfree(data->rx_buf);
> +	kfree(data->tx_buf);
> +	kfree(data);
> +	return 0;
> +}
> +
> +static struct spi_driver max1111_driver = {
> +	.driver		= {
> +		.name	= "max1111",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= max1111_probe,
> +	.remove		= __devexit_p(max1111_remove),
> +};
> +
> +static int __init max1111_init(void)
> +{
> +	return spi_register_driver(&max1111_driver);
> +}
> +module_init(max1111_init);
> +
> +static void __exit max1111_exit(void)
> +{
> +	spi_unregister_driver(&max1111_driver);
> +}
> +module_exit(max1111_exit);
> +
> +MODULE_AUTHOR("Eric Miao <eric.miao@marvell.com>");
> +MODULE_DESCRIPTION("MAX1111 ADC Driver");
> +MODULE_LICENSE("GPL");


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 6/8] hwmon: add max1111/max1110 Low-power

[Eric Miao]

>
> Code looks reasonable, so you get my:
>
> Acked-by: Jean Delvare <khali@linux-fr.org>

Thanks.

>
> However, please read my few suggestions below to make the driver even
> better.
>> +
>> +     data->rx_buf = kmalloc(MAX1111_RX_BUF_SIZE, GFP_KERNEL);
>> +     if (!data->rx_buf) {
>> +             kfree(data->tx_buf);
>> +             return -ENOMEM;
>> +     }
>
> Allocating such small buffers using kmalloc seems pretty inefficient.
> At the very least, I would allocate both buffers at once. But quite
> frankly I don't get why you don't just make these buffers part of
> struct max1111_data. This would even make the structure smaller!
>

I originally place the buffer within "struct max1111_data" but received
a mail from David Brownell suggesting using a kmalloc() buffer, so that
DMA mode will work better with the cache alignment and trailing bytes,
though PIO can just work happily. I don't know the specific reason for
this, honestly.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 6/8] hwmon: add max1111/max1110 Low-power

[Eric Miao]

On Tue, Sep 23, 2008 at 9:31 AM, Eric Miao <eric.y.miao@gmail.com> wrote:
>>
>> Code looks reasonable, so you get my:
>>
>> Acked-by: Jean Delvare <khali@linux-fr.org>
>
> Thanks.
>

OK, I've got all your comments incorporated and queued the patch,
except for the kmalloc() one, which I'd like to get feedback from
David Brownell, and may subject to change in the future.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 6/8] hwmon: add max1111/max1110 Low-power

[David Brownell]

On Monday 22 September 2008, Eric Miao wrote:
> >> +     data->rx_buf = kmalloc(MAX1111_RX_BUF_SIZE, GFP_KERNEL);
> >> +     if (!data->rx_buf) {
> >> +             kfree(data->tx_buf);
> >> +             return -ENOMEM;
> >> +     }
> >
> > Allocating such small buffers using kmalloc seems pretty inefficient.
> > At the very least, I would allocate both buffers at once. But quite
> > frankly I don't get why you don't just make these buffers part of
> > struct max1111_data. This would even make the structure smaller!
> >
> 
> I originally place the buffer within "struct max1111_data" but received
> a mail from David Brownell suggesting using a kmalloc() buffer, so that
> DMA mode will work better with the cache alignment and trailing bytes,
> though PIO can just work happily. I don't know the specific reason for
> this, honestly.

The phrase is "cache line sharing".  If you make sure the buffer doesn't
share its cache line with something the CPU may modify while the DMA is
happening, you're OK ... and using a dedicated kmalloc buffer guarantees
that.  (So will sticking buffers at the end of a struct, like max1111_data,
annotated as "____cacheline_aligned", in many cases.)

On most ARMs, L1 and L2 caches are not DMA-coherent.  So when both DMA
and CPU write to some memory, one can overwrite the other if you're
sloppy about buffer allocation.


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 6/8] hwmon: add max1111/max1110 Low-power

[Russell King - ARM Linux]

On Tue, Sep 23, 2008 at 09:31:09AM +0800, Eric Miao wrote:
> > Allocating such small buffers using kmalloc seems pretty inefficient.
> > At the very least, I would allocate both buffers at once. But quite
> > frankly I don't get why you don't just make these buffers part of
> > struct max1111_data. This would even make the structure smaller!
> >
> 
> I originally place the buffer within "struct max1111_data" but received
> a mail from David Brownell suggesting using a kmalloc() buffer, so that
> DMA mode will work better with the cache alignment and trailing bytes,
> though PIO can just work happily. I don't know the specific reason for
> this, honestly.

Having cachelines overlap with other data which may be modified during
the DMA causes problems on non-cache coherent hardware.  It's much
safer to ensure that DMA buffers don't share cache lines with anything
else.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 6/8] hwmon: add max1111/max1110 Low-power

[Jean Delvare]

On Tue, 23 Sep 2008 08:23:25 +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 23, 2008 at 09:31:09AM +0800, Eric Miao wrote:
> > > Allocating such small buffers using kmalloc seems pretty inefficient.
> > > At the very least, I would allocate both buffers at once. But quite
> > > frankly I don't get why you don't just make these buffers part of
> > > struct max1111_data. This would even make the structure smaller!
> > >
> > 
> > I originally place the buffer within "struct max1111_data" but received
> > a mail from David Brownell suggesting using a kmalloc() buffer, so that
> > DMA mode will work better with the cache alignment and trailing bytes,
> > though PIO can just work happily. I don't know the specific reason for
> > this, honestly.
> 
> Having cachelines overlap with other data which may be modified during
> the DMA causes problems on non-cache coherent hardware.  It's much
> safer to ensure that DMA buffers don't share cache lines with anything
> else.

Ah, OK. I had not realized that these buffers were used for DMA, sorry.

Still, can't we allocate the transmit and receive buffers in one go? I
guess transmit and receive can't happen concurrently, so that would be
safe?

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 6/8] hwmon: add max1111/max1110 Low-power

[David Brownell]

On Tuesday 23 September 2008, Jean Delvare wrote:
> Still, can't we allocate the transmit and receive buffers in one go?

I'd think so ...


> I guess transmit and receive can't happen concurrently, so that would be
> safe?

This is SPI, not I2C, so full duplex transfers work just fine.

Have a look at figure 12a in the MAX1110/MAX1111 data sheet to
observe continuous sampling using full duplex transfers.



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 6/8] hwmon: add max1111/max1110 Low-power

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed, Size: 1773 bytes --]

On Mon, 22 Sep 2008, David Brownell wrote:
> On Monday 22 September 2008, Eric Miao wrote:
>>>> +     data->rx_buf = kmalloc(MAX1111_RX_BUF_SIZE, GFP_KERNEL);
>>>> +     if (!data->rx_buf) {
>>>> +             kfree(data->tx_buf);
>>>> +             return -ENOMEM;
>>>> +     }
>>>
>>> Allocating such small buffers using kmalloc seems pretty inefficient.
>>> At the very least, I would allocate both buffers at once. But quite
>>> frankly I don't get why you don't just make these buffers part of
>>> struct max1111_data. This would even make the structure smaller!
>>>
>>
>> I originally place the buffer within "struct max1111_data" but received
>> a mail from David Brownell suggesting using a kmalloc() buffer, so that
>> DMA mode will work better with the cache alignment and trailing bytes,
>> though PIO can just work happily. I don't know the specific reason for
>> this, honestly.
>
> The phrase is "cache line sharing".  If you make sure the buffer doesn't
> share its cache line with something the CPU may modify while the DMA is
> happening, you're OK ... and using a dedicated kmalloc buffer guarantees
> that.  (So will sticking buffers at the end of a struct, like max1111_data,
> annotated as "____cacheline_aligned", in many cases.)

Another alternative:

typedef ____cacheline_aligned char cacheline_barrier_t[0];

struct max1111_data {
 	u8 buffer[BUF_SIZE];
 	cacheline_barrier_t barrier;

 	int whatever;
 	...
 	long something_else;

 	cacheline_barrier_t barrier2;
 	u8 also_gets_its_own_cacheline[BUF_SIZE];
};

One could also have ARM define:
typedef ____cacheline_aligned char dma_padding_t[0];

And x86, etc. would define:
typedef char dma_padding_t[0];

Then other arches don't get wasted space.

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors