RE: [PATCH v4 6/14] input: cyapa: add gen3 trackpad device basic functions support

[Dudley Du]

Hi Dmitry,

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
> Sent: Saturday, August 23, 2014 7:55 AM
> To: Dudley Du
> Cc: Rafael J. Wysocki; Benson Leung; Patrik Fimml; linux-input@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Dudley Du
> Subject: Re: [PATCH v4 6/14] input: cyapa: add gen3 trackpad device basic
> functions support
> 
> On Thu, Jul 17, 2014 at 02:53:48PM +0800, Dudley Du wrote:
> > Based on the cyapa core, add the gen3 trackpad device's basic functions
> > supported, so gen3 trackpad device can work with kernel input system.
> > The basic function is absolutely same as previous cyapa driver only
> > support gen3 trackpad device.
> > TEST=test on Chromebooks.
> >
> > Signed-off-by: Dudley Du <dudl@xxxxxxxxxxx>
> > ---
> >  drivers/input/mouse/Makefile     |    2 +-
> >  drivers/input/mouse/cyapa.c      |   96 ++++-
> >  drivers/input/mouse/cyapa.h      |    1 +
> >  drivers/input/mouse/cyapa_gen3.c |  784
> ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 881 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/input/mouse/cyapa_gen3.c
> >
> > diff --git a/drivers/input/mouse/Makefile b/drivers/input/mouse/Makefile
> > index 8608eb7..63b42e0 100644
> > --- a/drivers/input/mouse/Makefile
> > +++ b/drivers/input/mouse/Makefile
> > @@ -35,4 +35,4 @@ psmouse-$(CONFIG_MOUSE_PS2_TRACKPOINT)	+= trackpoint.o
> >  psmouse-$(CONFIG_MOUSE_PS2_TOUCHKIT)	+= touchkit_ps2.o
> >  psmouse-$(CONFIG_MOUSE_PS2_CYPRESS)	+= cypress_ps2.o
> >
> > -cyapatp-y := cyapa.o
> > +cyapatp-y := cyapa.o cyapa_gen3.o
> > diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> > index ae24b02..5c62503 100644
> > --- a/drivers/input/mouse/cyapa.c
> > +++ b/drivers/input/mouse/cyapa.c
> > @@ -113,6 +113,15 @@ ssize_t cyapa_i2c_write(struct cyapa *cyapa, u8 reg,
> >
> >  void cyapa_default_irq_handler(struct cyapa *cyapa)
> >  {
> > +	bool cont;
> > +
> > +	/* Interrupt triggerred by command response in detecting. */
> > +	cont = true;
> > +	if (cyapa_gen3_ops.irq_cmd_handler)
> > +		cont = cyapa_gen3_ops.irq_cmd_handler(cyapa);
> 
> Why not simply
> 
> 	cont = cyapa->ops->irq_cmd_handler(cyapa)?


When this the default irq handler is called, at this time, means the device
haven't been initialized yet, the cyapa->ops hasn't been setup correctly, and
the cyapa->ops->irq_cmd_handler is NULL.
So currently, maybe the driver is still on detecting, which device module should
be setup to cyapa->ops is unknown yet. But like gen5 device, it will execute commands
that based on interrupt signals, so the irq_cmd_handler() must be executed
for each module to complete the possible command response when an interrupt comes.
So, here, for each device module, their irq_cmd_handler() should be called directly.


> 
> 
> > +	if (!cont)
> > +		return;
> > +
> >  	/*
> >  	 * Do redetecting when device states is still unknown and
> >  	 * interrupt envent is received from device.
> > @@ -252,6 +261,9 @@ static int cyapa_check_is_operational(struct cyapa
> *cyapa)
> >  		return ret;
> >
> >  	switch (cyapa->gen) {
> > +	case CYAPA_GEN3:
> > +		cyapa->ops = &cyapa_gen3_ops;
> > +		break;
> >  	default:
> >  		cyapa->ops = &cyapa_default_ops;
> >  		cyapa->gen = CYAPA_GEN_UNKNOWN;
> > @@ -314,9 +326,85 @@ out:
> >   */
> >  static int cyapa_get_state(struct cyapa *cyapa)
> >  {
> > +	int ret;
> > +	u8 status[BL_STATUS_SIZE];
> > +	u8 cmd[32];
> > +	/* The i2c address of gen4 and gen5 trackpad device must be even. */
> > +	bool even_addr = ((cyapa->client->addr & 0x0001) == 0);
> > +	bool smbus = false;
> > +	int retries = 2;
> > +
> >  	cyapa->state = CYAPA_STATE_NO_DEVICE;
> >
> > -	return -ENODEV;
> > +	/*
> > +	 * Get trackpad status by reading 3 registers starting from 0.
> > +	 * If the device is in the bootloader, this will be BL_HEAD.
> > +	 * If the device is in operation mode, this will be the DATA regs.
> > +	 *
> > +	 */
> > +	ret = cyapa_i2c_reg_read_block(cyapa, BL_HEAD_OFFSET, BL_STATUS_SIZE,
> > +				       status);
> > +
> > +	/*
> > +	 * On smbus systems in OP mode, the i2c_reg_read will fail with
> > +	 * -ETIMEDOUT.  In this case, try again using the smbus equivalent
> > +	 * command.  This should return a BL_HEAD indicating CYAPA_STATE_OP.
> > +	 */
> > +	if (cyapa->smbus && (ret == -ETIMEDOUT || ret == -ENXIO)) {
> > +		if (!even_addr)
> > +			ret = cyapa_read_block(cyapa,
> > +					CYAPA_CMD_BL_STATUS, status);
> > +		smbus = true;
> > +	}
> > +	if (ret != BL_STATUS_SIZE)
> > +		goto error;
> > +
> > +	/*
> > +	 * Detect trackpad protocol based on characristic registers and bits.
> > +	 */
> > +	do {
> > +		cyapa->status[REG_OP_STATUS] = status[REG_OP_STATUS];
> > +		cyapa->status[REG_BL_STATUS] = status[REG_BL_STATUS];
> > +		cyapa->status[REG_BL_ERROR] = status[REG_BL_ERROR];
> > +
> > +		if (cyapa->gen == CYAPA_GEN_UNKNOWN ||
> > +				cyapa->gen == CYAPA_GEN3) {
> > +			ret = cyapa_gen3_ops.state_parse(cyapa,
> > +					status, BL_STATUS_SIZE);
> > +			if (ret == 0)
> > +				goto out_detected;
> > +		}
> > +
> > +		/*
> > +		 * Cannot detect communication protocol based on current
> > +		 * charateristic registers and bits.
> > +		 * So write error command to do further detection.
> > +		 * this method only valid on I2C bus.
> > +		 * for smbus interface, it won't have overwrite issue.
> > +		 */
> > +		if (!smbus) {
> > +			cmd[0] = 0x00;
> > +			cmd[1] = 0x00;
> > +			ret = cyapa_i2c_write(cyapa, 0, 2, cmd);
> > +			if (ret)
> > +				goto error;
> > +
> > +			msleep(50);
> > +
> > +			ret = cyapa_i2c_read(cyapa, BL_HEAD_OFFSET,
> > +					BL_STATUS_SIZE, status);
> > +			if (ret < 0)
> > +				goto error;
> > +		}
> > +	} while (--retries > 0 && !smbus);
> > +
> > +	goto error;
> > +
> > +out_detected:
> > +	return 0;
> > +
> > +error:
> > +	return (ret < 0) ? ret : -EAGAIN;
> >  }
> >
> >  /*
> > @@ -994,11 +1082,17 @@ static void cyapa_detect_and_start(void *data,
> async_cookie_t cookie)
> >
> >  static int cyapa_tp_modules_init(struct cyapa *cyapa)
> >  {
> > +	if (cyapa_gen3_ops.initialize)
> > +		return cyapa_gen3_ops.initialize(cyapa);
> 
> Same here.

Same reason as above.

> 
> > +
> >  	return 0;
> >  }
> >
> >  static int cyapa_tp_modules_uninit(struct cyapa *cyapa)
> >  {
> > +	if (cyapa_gen3_ops.uninitialize)
> > +		return cyapa_gen3_ops.uninitialize(cyapa);
> 
> And here.
> 
> Thanks.
> 
> --
> Dmitry

--
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/