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

[Christopher Heiny]

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/