Re: [PATCH v2 1/1] Input: add driver for Cypress APA I2C Trackpad

[Jean Delvare <khali@linux-fr.org>]

On Wed, 5 Dec 2012 23:48:17 -0800, Dmitry Torokhov wrote:
> Hi Benson,
> 
> On Wed, Dec 05, 2012 at 04:48:19PM -0800, Benson Leung wrote:
> > This patch introduces a driver for Cypress All Points Addressable
> > I2C Trackpad, including the ones in 2012 Samsung Chromebooks.
> > 
> > This device is compatible with MT protocol type B, providing identifiable
> > contacts.
> > 
> > Signed-off-by: Dudley Du <dudl@cypress.com>
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > Signed-off-by: Benson Leung <bleung@chromium.org>
> > ---
> >  Version history :
> > v2 : * Removed firmware update.
> >      * Removed sysfs properties related to firmware update and power mode.
> >      * Folded cyapa_detect into cyapa_probe.
> >      * Added support for middle and right mechanical buttons, if they exist.
> >      * Rearranged disable_irq/enable_irq in suspend and resume to prevent
> >          a power mode change from colliding with a read of tracking data.
> >      * Made cyapa_get_state more reliable.
> >      * Use IRQF_ONESHOT for threaded irq
> >      * Simplified cyapa_set_power_mode.
> >      * Removed extra kernel-doc style comments
> >      * Removed dev_dbg messages.
> >      * Cleaned up unused includes.
> >      * Cleaned up unused #defines
> > v1 : Initial
> 
> First of all - this version is excellent compared to the previous ones
> posted. I have just a few quesutions (and of course I'd like Henrik to
> look over MT handlingi and Jean for I2C bits).

I did not find any problem with the I2C bits.

> > (...)
> > +static ssize_t cyapa_i2c_reg_read_block(struct cyapa *cyapa, u8 reg, size_t len,
> > +					u8 *values)
> > +{
> > +	return i2c_smbus_read_i2c_block_data(cyapa->client, reg, len, values);
> 
> Does it have to be SMBUS? If it does you need to check the capabilities
> in probe(). 

Using SMBus calls is recommended as it increases portability of the
driver. I2C controllers can do SMBus but SMBus controllers can't do
I2C. Checking the capabilities is needed in both cases anyway, as you
can't assume anything from an i2c_adapter.

-- 
Jean Delvare