Re: [RFC PATCH 5/17] input: rmidev character driver for RMI4 sensors

From: Christopher Heiny
Date: Tue Sep 04 2012 - 20:26:17 EST


On 08/27/2012 11:49 AM, Linus Walleij wrote:
On Fri, Aug 17, 2012 at 3:17 PM, Christopher Heiny <cheiny@xxxxxxxxxxxxx> wrote:

Driver for Synaptics touchscreens using RMI4 protocol.

Really? This looks more like some custom char driver to get a pipe
into the device from userspace. Put in a proper description of what this
is for.

If the purpose is to read/write arbitrary addresses in the device,
you should use regmap's debugfs interface for this instead,
it is much better suited for the task.

We'll look into using regmap not only for that, but possibly for general register access. It has the potential to greatly simply both the arbitrary register access, as well as the driver itself.


(...)
+#define RMI_CHAR_DEV_TMPBUF_SZ 128
+#define RMI_REG_ADDR_PAGE_SELECT 0xFF
+#define REG_ADDR_LIMIT 0xFFFF
+
+struct rmidev_data {
+ /* mutex for file operation*/
+ struct mutex file_mutex;
+ /* main char dev structure */
+ struct cdev main_dev;
+
+ /* pointer to the corresponding RMI4 device. We use this to do */
+ /* read, write, etc. */
+ struct rmi_device *rmi_dev;
+ /* reference count */
+ int ref_count;

Something tells me you should atleast use <linux/kref.h> for this.
It also solves a few atomicity problems in a good standard way.
See Documentation/kref.txt

Roger.


+/*store dynamically allocated major number of char device*/
+static int rmidev_major_num;

You need to patch your desired major number into
Documentation/devices.txt'

We were going by the recommendation in Linux Device Drivers (3rd edition) to use dynamic major number allocation via alloc_chrdev_region. In particular in section 3.2.3 it says "new numbers are not being assigned". I guess at this point we need to know whether the info in LDD3 is authoritative or not. We can always add a number to Documentation/devices.txt if that's the right thing to do, but I'd like to make sure our next submission isn't bounced because we did that, turning the process into Patch Ping-Pong :-).

In any case, it's likely that switching to the regmap interface would make this question irrelevant.


+static struct class *rmidev_device_class;

Last time discussed with Greg, class devices were deprecated,
and you should just use a bus instead. (But not sure.)

The references I found online weren't clear on this, so more investigation is required. We'll defer that till we find out if the regmap changes eliminate the need for this.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/