i2c driver for Maxim max9485 audio clock generator chip

PATCH Superseded Jon Smirl 3 participants 6 messages Oct 21, 2008 – Oct 22, 2008 3 messages not in the archive I2C subsystem host drivers

lore

Share & tools

lore

Jon Smirl

author

2008-10-21 03:02 UTC #0 lore raw

PATCH 1/1 i2c driver for Maxim max9485 audio clock generator chip

Signed-off-by Jon Smirl <[email protected]>

---
 drivers/i2c/chips/Kconfig   |    9 ++++
 drivers/i2c/chips/Makefile  |    1 
 drivers/i2c/chips/max9485.c |  106 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/max9485.h |   38 +++++++++++++++
 4 files changed, 154 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/chips/max9485.c
 create mode 100644 include/linux/i2c/max9485.h

drivers/i2c/chips/Kconfig +9 -0

index 1735682..bc2a6d3 100644
@@ -40,6 +40,15 @@ config AT24
 	  This driver can also be built as a module.  If so, the module
 	  will be called at24.
 
+config MAX9485
+	tristate "Maxim MAX9485 Programmable Audio Clock Generator"
+	help
+	  If you say yes here you get support for Maxim MAX9485 
+	  Programmable Audio Clock Generator.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called max9485.
+
 config SENSORS_EEPROM
 	tristate "EEPROM reader"
 	depends on EXPERIMENTAL

drivers/i2c/chips/Makefile +1 -0

index ca520fa..1560baf 100644
@@ -11,6 +11,7 @@
 
 obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_AT24)		+= at24.o
+obj-$(CONFIG_MAX9485)		+= max9485.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o

drivers/i2c/chips/max9485.c new file +106 -0

106 lines not on this page · raw message

include/linux/i2c/max9485.h new file +38 -0

new file mode 100644
index 0000000..0c97450
@@ -0,0 +1,38 @@
+/*
+ * Maxim max9485 Programmable Audio Clock Generator driver
+ *
+ * Written by: Jon Smirl <[email protected]>
+ *
+ * Copyright (C) Digispeaker.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
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __LINUX_I2C_MAX9485_H
+#define __LINUX_I2C_MAX9485_H
+
+struct i2c_client;
+
+/* Defines for Maxim MAX9485 Audio Clock Generator */
+
+#define MAX9485_MCLK 0x80
+#define MAX9485_CLK_OUT_2 0x40
+#define MAX9485_CLK_OUT_1 0x20
+#define MAX9485_DOUBLED 0x10
+#define MAX9485_SCALE_256 0x0
+#define MAX9485_SCALE_384 0x2
+#define MAX9485_SCALE_768 0x6
+#define MAX9485_FREQUENCY_12 0x0
+#define MAX9485_FREQUENCY_32 0x1
+#define MAX9485_FREQUENCY_441 0x2
+#define MAX9485_FREQUENCY_48 0x3
+
+/* Combinations that minimize jitter */
+#define MAX9485_245760 MAX9485_SCALE_256 | MAX9485_FREQUENCY_48 | MAX9485_DOUBLED
+#define MAX9485_225792 MAX9485_SCALE_256 | MAX9485_FREQUENCY_441 | MAX9485_DOUBLED
+
+int max9485_set(struct i2c_client *max9485, u8 value);
+
+#endif /*  __LINUX_I2C_MAX9485_H */

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

Jean Delvare

2008-10-21 08:36 UTC #1 lore raw

Hi Jon,

Quoted 9 lines · Jon Smirl

Signed-off-by Jon Smirl <[email protected]>

---
 drivers/i2c/chips/Kconfig   |    9 ++++
 drivers/i2c/chips/Makefile  |    1 
 drivers/i2c/chips/max9485.c |  106 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/max9485.h |   38 +++++++++++++++
 4 files changed, 154 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/chips/max9485.c
 create mode 100644 include/linux/i2c/max9485.h

Nack. No new drivers in drivers/i2c/chips please, I'm trying hard to get rid of this directory. If you can't find a better place (drivers/clocksource? sound?) please put this new driver under drivers/misc.

Quoted 62 lines

drivers/i2c/chips/Kconfig +9 -0

index 1735682..bc2a6d3 100644
@@ -40,6 +40,15 @@ config AT24
 	  This driver can also be built as a module.  If so, the module
 	  will be called at24.
 
+config MAX9485
+	tristate "Maxim MAX9485 Programmable Audio Clock Generator"
+	help
+	  If you say yes here you get support for Maxim MAX9485 
+	  Programmable Audio Clock Generator.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called max9485.
+
 config SENSORS_EEPROM
 	tristate "EEPROM reader"
 	depends on EXPERIMENTAL

drivers/i2c/chips/Makefile +1 -0

index ca520fa..1560baf 100644
@@ -11,6 +11,7 @@
 
 obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_AT24)		+= at24.o
+obj-$(CONFIG_MAX9485)		+= max9485.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o

drivers/i2c/chips/max9485.c new file +23 -0

new file mode 100644
index 0000000..65058b4
@@ -0,0 +1,106 @@
+/*
+ * Maxim max9485 Programmable Audio Clock Generator driver
+ *
+ * Written by: Jon Smirl <[email protected]>
+ *
+ * Copyright (C) Digispeaker.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
+ * published by the Free Software Foundation.
+ */
+
+/*
+ * This device is under the control of ALSA and can not be changed from
+ * userspace. The main purpose of this driver is to locate the i2c address
+ * of where the chip is located.
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/sysfs.h>
+#include <linux/i2c/max9485.h>
+

drivers/i2c/chips/max9485.c line 33

Quoted 10 lines

drivers/i2c/chips/max9485.c new file +10 -0

+int max9485_set(struct i2c_client *max9485, u8 value)
+{
+	int rc;
+
+	if (!max9485)
+		return -ENODEV;
+
+	rc = i2c_smbus_write_byte(max9485, value);
+	if (rc < 0)
+		return -EIO;

Please return rc instead of hard-cording an error value.

drivers/i2c/chips/max9485.c line 37

Quoted 4 lines

drivers/i2c/chips/max9485.c new file +4 -0
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(max9485_set);

I don't quite get the point of exporting this function. It's hardly any better than i2c_smbus_write_byte() itself, which is already exported. In fact I wonder why you wrote this function in the first place.

drivers/i2c/chips/max9485.c line 50

Quoted 13 lines

drivers/i2c/chips/max9485.c new file +13 -0

+
+/*
+ * Display the only register
+ */
+static ssize_t max9485_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int rc;
+
+	rc = i2c_smbus_read_byte(client);
+	if (rc < 0)
+		return -EIO;

Please return rc instead of hard-cording an error value.

drivers/i2c/chips/max9485.c line 54

Quoted 4 lines

drivers/i2c/chips/max9485.c new file +4 -0

+
+	return sprintf(buf, "0x%02X\n", rc);
+}
+static DEVICE_ATTR(max9485, S_IRUGO, max9485_show, NULL);

Attributes in sysfs are supposed to be normalized. Returning raw register values there makes no sense to me. Users can simply use i2cget for the same result, no need to write a kernel driver.

drivers/i2c/chips/max9485.c line 63

Quoted 9 lines

drivers/i2c/chips/max9485.c new file +9 -0

+
+/*
+ * Called when a max9485 device is matched with this driver
+ */
+static int max9485_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	if (!i2c_check_functionality(client->adapter,
+				I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE)) {

I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE is better written I2C_FUNC_SMBUS_BYTE.

Quoted 69 lines

drivers/i2c/chips/max9485.c new file +43 -0

+		dev_err(&client->dev, "i2c bus does not support the max9485\n");
+		return -ENODEV;
+	}
+	return sysfs_create_file(&client->dev.kobj, &dev_attr_max9485.attr);
+}
+
+static int max9485_remove(struct i2c_client *client)
+{
+	sysfs_remove_file(&client->dev.kobj, &dev_attr_max9485.attr);
+	return 0;
+}
+
+static const struct i2c_device_id max9485_id[] = {
+	{ "max9485", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max9485_id);
+
+static struct i2c_driver max9485_driver = {
+	.driver = {
+		.name = "max9485",
+	},
+	.probe = max9485_probe,
+	.remove = max9485_remove,
+	.id_table = max9485_id,
+};
+
+static int __init max9485_init(void)
+{
+	return i2c_add_driver(&max9485_driver);
+}
+
+static void __exit max9485_exit(void)
+{
+	i2c_del_driver(&max9485_driver);
+}
+
+MODULE_AUTHOR("Jon Smirl <[email protected]");
+MODULE_DESCRIPTION("Maxim MAX9485 Programmable Audio Clock Generator driver");
+MODULE_LICENSE("GPL");
+
+module_init(max9485_init);
+module_exit(max9485_exit);

include/linux/i2c/max9485.h new file +20 -0

new file mode 100644
index 0000000..0c97450
@@ -0,0 +1,38 @@
+/*
+ * Maxim max9485 Programmable Audio Clock Generator driver
+ *
+ * Written by: Jon Smirl <[email protected]>
+ *
+ * Copyright (C) Digispeaker.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
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __LINUX_I2C_MAX9485_H
+#define __LINUX_I2C_MAX9485_H
+
+struct i2c_client;
+
+/* Defines for Maxim MAX9485 Audio Clock Generator */
+
+#define MAX9485_MCLK 0x80

include/linux/i2c/max9485.h line 30

Quoted 10 lines

include/linux/i2c/max9485.h new file +10 -0

+#define MAX9485_CLK_OUT_2 0x40
+#define MAX9485_CLK_OUT_1 0x20
+#define MAX9485_DOUBLED 0x10
+#define MAX9485_SCALE_256 0x0
+#define MAX9485_SCALE_384 0x2
+#define MAX9485_SCALE_768 0x6
+#define MAX9485_FREQUENCY_12 0x0
+#define MAX9485_FREQUENCY_32 0x1
+#define MAX9485_FREQUENCY_441 0x2
+#define MAX9485_FREQUENCY_48 0x3

It's rather suspicious that you have use bit 1 for 2 different purposes while you don't use bit 3. Didn't you shift some values accidentally?

include/linux/i2c/max9485.h line 34

Quoted 4 lines

include/linux/i2c/max9485.h new file +4 -0

+
+/* Combinations that minimize jitter */
+#define MAX9485_245760 MAX9485_SCALE_256 | MAX9485_FREQUENCY_48 | MAX9485_DOUBLED
+#define MAX9485_225792 MAX9485_SCALE_256 | MAX9485_FREQUENCY_441 | MAX9485_DOUBLED

Missing parentheses.

include/linux/i2c/max9485.h line 38

Quoted 4 lines

include/linux/i2c/max9485.h new file +4 -0

+
+int max9485_set(struct i2c_client *max9485, u8 value);
+
+#endif /*  __LINUX_I2C_MAX9485_H */

Honestly I don't see any value in this driver. There's nothing you can do with it that you couldn't already do without it.

Signature · 6 lines

Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jon Smirl

author

2008-10-21 23:57 UTC #3 lore raw

Quoted 2 lines · Jean Delvare

Honestly I don't see any value in this driver. There's nothing you can do with it that you couldn't already do without it.

I need a driver so that my device tree will bind and tell me the i2c address of the device.

We may also build a device in the future with two of these chips.

Signature · 7 lines

Jon Smirl
[email protected]

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): 9e4733910810211657s5e25e391p5ca6e6f672d115c5-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Jean Delvare

2008-10-22 06:38 UTC #5 lore raw

Quoted 6 lines · Jon Smirl

Quoted 2 lines · Jean Delvare
Honestly I don't see any value in this driver. There's nothing you can do with it that you couldn't already do without it.
I need a driver so that my device tree will bind and tell me the i2c address of the device.

This is rubbish. It's perfectly fine to let the I2C device be instantiated from the device tree without having a driver that binds to it. The device tree and the instantiated device, not the driver, tell you the I2C address of the device.

Quoted 1 line

We may also build a device in the future with two of these chips.

I fail to see how it matters.

Signature · 6 lines

Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jon Smirl

author

2008-10-22 16:38 UTC #7 lore raw

What you want me to do? Put in a bare .h file for the part? Bare .h files are much harder to find and reuse. It is also going to force anyone reusing this part into reading the datasheet and determining how the part works.

Quoted 20 lines · Jean Delvare

Quoted 6 lines · Jon Smirl
Quoted 2 lines · Jean Delvare
Honestly I don't see any value in this driver. There's nothing you can do with it that you couldn't already do without it.
I need a driver so that my device tree will bind and tell me the i2c address of the device.
This is rubbish. It's perfectly fine to let the I2C device be instantiated from the device tree without having a driver that binds to it. The device tree and the instantiated device, not the driver, tell you the I2C address of the device.
Quoted 1 line
We may also build a device in the future with two of these chips.
I fail to see how it matters.
--
Jean Delvare

Signature · 7 lines

Jon Smirl
[email protected]

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

Ben Dooks

2008-10-21 08:45 UTC #8 lore raw

In reply to Jon Smirl

Quoted 48 lines · Jon Smirl

Signed-off-by Jon Smirl <[email protected]>

---
 drivers/i2c/chips/Kconfig   |    9 ++++
 drivers/i2c/chips/Makefile  |    1 
 drivers/i2c/chips/max9485.c |  106 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/max9485.h |   38 +++++++++++++++
 4 files changed, 154 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/chips/max9485.c
 create mode 100644 include/linux/i2c/max9485.h

drivers/i2c/chips/Kconfig +9 -0

index 1735682..bc2a6d3 100644
@@ -40,6 +40,15 @@ config AT24
 	  This driver can also be built as a module.  If so, the module
 	  will be called at24.
 
+config MAX9485
+	tristate "Maxim MAX9485 Programmable Audio Clock Generator"
+	help
+	  If you say yes here you get support for Maxim MAX9485 
+	  Programmable Audio Clock Generator.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called max9485.
+
 config SENSORS_EEPROM
 	tristate "EEPROM reader"
 	depends on EXPERIMENTAL

drivers/i2c/chips/Makefile +1 -0

index ca520fa..1560baf 100644
@@ -11,6 +11,7 @@
 
 obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_AT24)		+= at24.o
+obj-$(CONFIG_MAX9485)		+= max9485.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o

drivers/i2c/chips/max9485.c new file +0 -0

new file mode 100644
index 0000000..65058b4

drivers/i2c/chips/max9485.c line 6

Quoted 6 lines

drivers/i2c/chips/max9485.c new file +6 -0

@@ -0,0 +1,106 @@
+/*
+ * Maxim max9485 Programmable Audio Clock Generator driver
+ *
+ * Written by: Jon Smirl <[email protected]>
+ *
+ * Copyright (C) Digispeaker.com

you need a date at which you are claiming the copyright. I'd also check that (C) in a big symbol is a legally recognised substitute for an small c in an circle.

Signature · 8 lines

Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c