Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen

[Mylene Josserand]

Maxime,

On 06/06/2017 14:04, Maxime Ripard wrote:
> On Tue, Jun 06, 2017 at 10:03:48AM +0200, Mylene Josserand wrote:
> > > > +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts)
> > > > +{
> > > > +	int rc, t;
> > > > +	u8 cmd[HID_OUTPUT_BL_LAUNCH_APP];
> > > > +	u16 crc;
> > > > +
> > > > +	mutex_lock(&ts->system_lock);
> > > > +	ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1;
> > > > +	mutex_unlock(&ts->system_lock);
> > > > +
> > > > +	cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> > > > +	cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> > > > +	cmd[2] = HID_BL_OUTPUT_REPORT_ID;
> > > > +	cmd[3] = 0x0; /* Reserved */
> > > > +	cmd[4] = HID_OUTPUT_BL_SOP;
> > > > +	cmd[5] = HID_OUTPUT_BL_LAUNCH_APP;
> > > > +	cmd[6] = 0x0; /* Low bytes of data */
> > > > +	cmd[7] = 0x0; /* Hi bytes of data */
> > > > +	crc = cyttsp5_compute_crc(&cmd[4], 4);
> > > > +	cmd[8] = LOW_BYTE(crc);
> > > > +	cmd[9] = HI_BYTE(crc);
> > > > +	cmd[10] = HID_OUTPUT_BL_EOP;
> > > > +
> > > > +	rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
> > > > +			   HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> > >
> > > It seems like you'll send HID_OUTPUT_BL_LAUNCH_APP_SIZE twice here,
> > > once as setup in cyttsp5_write, and once in the buffer you want to
> > > send. Is that on purpose?
> >
> > I am not sure to see the second time it is sent. What do you mean by "as
> > setup in cyttsp5_write"? Anyway, the size is needed in the buffer of the
> > frame.
>
> Nevermind, I confused the HID_OUTPUT_BL_LAUNCH_APP_SIZE with the
> register, my bad.

No problem.

> > > > +	/* Call platform init function */
> > > > +	ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > > +	if (IS_ERR(ts->reset_gpio)) {
> > > > +		rc = PTR_ERR(ts->reset_gpio);
> > > > +		dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> > > > +		return rc;
> > > > +	}
> > > > +
> > > > +	/* Need a delay to have device up */
> > > > +	msleep(20);
> > >
> > > In the case where the device has already been out of reset, this means
> > > that this alone is not sufficient to reset it and bring it out of
> > > reset, which also means that you do not have any guarantee on the
> > > current state of the device. I'm not sure how it would impact the
> > > proper operations though.
> >
> > Okay. A reset frame can be sent to perform a "software
> > reset". Should I add it on startup to be in a "known behavior"?
>
> I guess you'd be safer just getting the GPIO with the low level by
> default, and then changing to high. That way you know that you went
> through a reset state, before bringing up the device.

Okay, I will update the driver, thanks!

> > > > +	ts->input = input_allocate_device();
> > > > +	if (!ts->input) {
> > > > +		dev_err(dev, "%s: Error, failed to allocate input device\n",
> > > > +			__func__);
> > > > +		rc = -ENODEV;
> > > > +		goto error_startup;
> > > > +	}
> > >
> > > In error_startup, you never uninitialize the device. Given your
> > > comment in cyttsp5_startup, this means that you might end up in a
> > > situation where your driver probes again with the device initialized
> > > and no longer in a bootloader mode, which is likely to cause some
> > > error.
> > >
> > > Putting the call to cyttsp5_startup later will make you less likely to
> > > fail (especially because of the DT parsing), and putting the device
> > > back into reset will probably address the issue once and for all.
> >
> > Hm, I am not sure to understand what you want to say, here.
> > Could you explain me more what you have in mind?
>
> I meant to say that there was still an issue with the reset here, and
> moving the DT parsing code further would ease the reset operation.

Okay

> > Notice that the DT parsing uses a sysinfo's variable (si->num_btns)
> > which is retrieved in the startup function (thanks to
> > get_sysinfo). So, currently, it is not possible to move the startup
> > function after the DT parsing.
>
> Can't that be made the other way around? You parse the number of
> buttons in the DT, and check the consistency with the hardware?

It could be possible but currently, it was designed to enable every 
button from hardware's configuration and DT binding was just a way to 
customize button's keycode (default is KEY_RESERVED).
If I base it on the DT, it means that some buttons could never be 
reported (such as deactivated) because they will not be present in the 
DT. I am not sure if it is the behavior we want. What do you think?


Best regards,

--
Mylène Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com