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
