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

[Thomas Petazzoni]

Hello,

Couple more comments in addition to the more extensive review done by
Maxime.

On Mon, 29 May 2017 16:45:37 +0200, MylÃne Josserand wrote:
> +static int cyttsp5_startup(struct cyttsp5 *ts)
> +{
> +	int rc;
> +
> +	rc = cyttsp5_deassert_int(ts);
> +	if (rc) {
> +		dev_err(ts->dev, "%s: Error on deassert int r=%d\n",
> +			__func__, rc);

I don't think it's very common to put __func__ in error messages,
especially when dev_err() already prints the name of the dvice.

> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Launch the application as the device starts in bootloader mode
> +	 * because of a power-on-reset
> +	 */
> +	rc = cyttsp5_hid_output_bl_launch_app(ts);
> +	if (rc < 0) {
> +		dev_err(ts->dev, "%s: Error on launch app r=%d\n",
> +			__func__, rc);
> +		goto exit;

Replace "goto exit" by "return rc;" since anyway the exit label
doesn't do anything.


> +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
> +			 const char *name)
> +{
> +	struct cyttsp5 *ts;
> +	struct cyttsp5_sysinfo *si;
> +	int rc = 0, i;
> +
> +	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> +	if (!ts) {
> +		rc = -ENOMEM;
> +		goto error_alloc_data;

Just return directly here (because the of_node_put is useless, see
below).

> +	}
> +
> +	/* Initialize device info */
> +	ts->regmap = regmap;
> +	ts->dev = dev;
> +	si = &ts->sysinfo;

So "si" is never NULL.

> +	if (si) {

And therefore you can do this unconditionally.

> +		__set_bit(EV_KEY, ts->input->evbit);
> +		for (i = 0; i < si->num_btns; i++)
> +			__set_bit(si->btn[i].key_code, ts->input->keybit);
> +
> +		rc = cyttsp5_setup_input_device(dev);
> +		if (rc)
> +			goto error_init_input;
> +	}
> +
> +	return 0;
> +
> +error_init_input:
> +	input_free_device(ts->input);

What about using devm_input_allocate_device() ?

> +error_startup:
> +error_setup_irq:
> +	dev_set_drvdata(dev, NULL);
> +error_alloc_data:
> +	dev_err(dev, "%s failed.\n", __func__);
> +	if (dev->of_node)
> +		of_node_put(dev->of_node);

As Maxime said, this is not necessary.

> +static int cyttsp5_remove(struct device *dev)
> +{
> +	const struct of_device_id *match;
> +	struct cyttsp5 *ts = dev_get_drvdata(dev);
> +
> +	input_unregister_device(ts->input);
> +
> +	dev_set_drvdata(dev, NULL);
> +
> +	match = of_match_device(of_match_ptr(cyttsp5_of_match), dev);
> +	if (match && dev->of_node)
> +		of_node_put(dev->of_node);

Ditto.

I'm also not sure that doing a dev_set_drvdata(dev, NULL) is really
needed in ->remove() and in the ->probe() exit path.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com