Message-ID: <CACRpkdbjgnH=FCyKfQEYo+fNOj0aDM7z+CX6z3SKpRRUGyx3tQ@mail.gmail.com>
Date:	Tue, 9 Oct 2012 12:02:57 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Christopher Heiny <cheiny@...aptics.com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Jean Delvare <khali@...ux-fr.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Linux Input <linux-input@...r.kernel.org>,
	Allie Xiong <axiong@...aptics.com>,
	Vivian Ly <vly@...aptics.com>,
	Daniel Rosenberg <daniel.rosenberg@...aptics.com>,
	Alexandra Chen <alexandra.chen@...synaptics.com>,
	Joerie de Gram <j.de.gram@...il.com>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Mathieu Poirier <mathieu.poirier@...aro.org>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	Naveen Kumar Gaddipati <naveen.gaddipati@...ricsson.com>,
	Henrik Rydberg <rydberg@...omail.se>
Subject: Re: [RFC PATCH 06/06] input/rmi4: F11 - 2D touch interface

On Sat, Oct 6, 2012 at 6:10 AM, Christopher Heiny <cheiny@...aptics.com> wrote:

So looking closer at this one since we will use it. Maybe it's in such a
good shape now that I should be able to actually test it with the hardware?

(...)
> diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c
(...)
> +#ifdef CONFIG_RMI4_DEBUG
> +#include <linux/debugfs.h>
> +#include <linux/fs.h>
> +#include <linux/uaccess.h>
> +#endif

Skip the #ifdef

> +#define F11_CTRL_SENSOR_MAX_X_POS_OFFSET       6
> +#define F11_CTRL_SENSOR_MAX_Y_POS_OFFSET       8
> +
> +#define F11_CEIL(x, y) (((x) + ((y)-1)) / (y))

Use existing kernel macros in <linux/kernel.h>

In this case:
#define F11_CEIL(x, y) DIV_ROUND_UP(x, y)

Or just use DIV_ROUND_UP() directly in the code, your choice.

> +#define MAX_NAME_LENGTH 256

Really? Are you sure there is not a null terminator or length byte
included so it's actually 255?

(...)
> +static int sensor_debug_open(struct inode *inodep, struct file *filp)
> +{
> +       struct sensor_debugfs_data *data;
> +       struct f11_2d_sensor *sensor = inodep->i_private;
> +       struct rmi_function_container *fc = sensor->fc;
> +
> +       data = devm_kzalloc(&fc->dev, sizeof(struct sensor_debugfs_data),
> +               GFP_KERNEL);

Again I may have lead you astray. Check if this leaks memory, in that
case use kzalloc()/kfree(). Sorry :-(

(...)
> +static int f11_debug_open(struct inode *inodep, struct file *filp)
> +{
> +       struct f11_debugfs_data *data;
> +       struct rmi_function_container *fc = inodep->i_private;
> +
> +       data = devm_kzalloc(&fc->dev, sizeof(struct f11_debugfs_data),
> +               GFP_KERNEL);

Dito. :-(

(...)
> +static void rmi_f11_abs_pos_report(struct f11_data *f11,
> +                                  struct f11_2d_sensor *sensor,
> +                                  u8 finger_state, u8 n_finger)
(...)
> +               if (axis_align->flip_y)
> +                       y = max(sensor->max_y - y, 0);
> +
> +               /*
> +               ** here checking if X offset or y offset are specified is
> +               **  redundant.  We just add the offsets or, clip the values
> +               **
> +               ** note: offsets need to be done before clipping occurs,
> +               ** or we could get funny values that are outside
> +               ** clipping boundaries.
> +               */

This is a weird commenting style, what's wrong with a single star?
(No big deal but it stands out...)

(...)
> +static int f11_allocate_control_regs(struct rmi_device *rmi_dev,
> +                               struct f11_2d_device_query *device_query,
> +                               struct f11_2d_sensor_query *sensor_query,
> +                               struct f11_2d_ctrl *ctrl,
> +                               u16 ctrl_base_addr) {
> +
> +       struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> +       struct rmi_function_container *fc = driver_data->f01_container;
> +
> +       ctrl->ctrl0_9 = devm_kzalloc(&fc->dev, sizeof(union f11_2d_ctrl0_9),
> +                                      GFP_KERNEL);

If this is called from .probe() only, this is correct.

So the rule is: use devm_* for anything that is allocated at .probe()
and released on .remove(). Any other dynamic buffers etc need to
use common kzalloc()/kfree().

> +       if (!ctrl->ctrl0_9)
> +               return -ENOMEM;
> +       if (sensor_query->f11_2d_query7__8[0]) {
> +               ctrl->ctrl10 = devm_kzalloc(&fc->dev,
> +                       sizeof(union f11_2d_ctrl10), GFP_KERNEL);
> +               if (!ctrl->ctrl10)
> +                       return -ENOMEM;
> +       }
> +
> +       if (sensor_query->f11_2d_query7__8[1]) {
> +               ctrl->ctrl11 = devm_kzalloc(&fc->dev,
> +                       sizeof(union f11_2d_ctrl11), GFP_KERNEL);
> +               if (!ctrl->ctrl11)
> +                       return -ENOMEM;
> +       }
> +
> +       if (device_query->has_query9 && sensor_query->query9.has_pen) {
> +               ctrl->ctrl20_21 = devm_kzalloc(&fc->dev,
> +                       sizeof(union f11_2d_ctrl20_21), GFP_KERNEL);
> +               if (!ctrl->ctrl20_21)
> +                       return -ENOMEM;
> +       }
> +
> +       if (device_query->has_query9 && sensor_query->query9.has_proximity) {
> +               ctrl->ctrl22_26 = devm_kzalloc(&fc->dev,
> +                       sizeof(union f11_2d_ctrl22_26), GFP_KERNEL);
> +               if (!ctrl->ctrl22_26)
> +                       return -ENOMEM;
> +       }
> +
> +       if (device_query->has_query9 &&
> +               (sensor_query->query9.has_palm_det_sensitivity ||
> +               sensor_query->query9.has_suppress_on_palm_detect)) {
> +               ctrl->ctrl27 = devm_kzalloc(&fc->dev,
> +                       sizeof(union f11_2d_ctrl27), GFP_KERNEL);
> +               if (!ctrl->ctrl27)
> +                       return -ENOMEM;
> +       }
> +
> +       if (sensor_query->has_multi_finger_scroll) {
> +               ctrl->ctrl28 = devm_kzalloc(&fc->dev,
> +                       sizeof(union f11_2d_ctrl28), GFP_KERNEL);
> +               if (!ctrl->ctrl28)
> +                       return -ENOMEM;
> +       }
> +
> +       if (device_query->has_query11 && device_query->has_z_tuning) {
> +               ctrl->ctrl29_30 = devm_kzalloc(&fc->dev,
> +                       sizeof(union f11_2d_ctrl29_30), GFP_KERNEL);
> +               if (!ctrl->ctrl29_30)
> +                       return -ENOMEM;
> +       }

All of these are probably correct as well.

> +
> +       return f11_read_control_regs(rmi_dev, ctrl, ctrl_base_addr);

Hey why are you ending with a call to that function?
The function name gets misleading.

Instead call both functions in succession at the call site on
.probe().

(...)
> +static int f11_device_init(struct rmi_function_container *fc)
> +{
> +       int rc;
> +
> +       rc = rmi_f11_initialize(fc);
> +       if (rc < 0)
> +               goto err_free_data;
> +
> +       rc = rmi_f11_register_devices(fc);
> +       if (rc < 0)
> +               goto err_free_data;
> +
> +       rc = rmi_f11_create_sysfs(fc);
> +       if (rc < 0)
> +               goto err_free_data;
> +
> +       return 0;
> +
> +err_free_data:
> +       rmi_f11_free_memory(fc);
> +
> +       return rc;
> +}
> +
> +static void rmi_f11_free_memory(struct rmi_function_container *fc)
> +{
> +       struct f11_data *f11 = fc->data;
> +       int i;
> +
> +       if (f11) {
> +               for (i = 0; i < f11->dev_query.nbr_of_sensors + 1; i++)
> +                       kfree(f11->sensors[i].button_map);
> +       }

This is wrong. The button_map was allocated with devm_kzalloc()
so it will get automatically freed. Just skip this step.

(...)
> +static int rmi_f11_initialize(struct rmi_function_container *fc)
> +{
(...)
> +               rc = f11_allocate_control_regs(rmi_dev,
> +                               &f11->dev_query, &sensor->sens_query,
> +                               &f11->dev_controls, control_base_addr);
> +               if (rc < 0) {
> +                       dev_err(&fc->dev,
> +                               "Failed to initialize F11 control params.\n");

"failed to allocate F11 control params"

> +                       return rc;
> +               }

So after this call the read regs explicitly instead as described above.

(...)
> +static void register_virtual_buttons(struct rmi_function_container *fc,
> +                                    struct f11_2d_sensor *sensor) {
> +       int j;
> +
> +       if (!sensor->sens_query.has_gestures)
> +               return;
> +       if (!sensor->virtual_buttons.buttons) {
> +               dev_warn(&fc->dev, "No virtual button platform data for 2D sensor %d.\n",
> +                        sensor->sensor_index);
> +               return;
> +       }
> +       /* call devm_kcalloc when it will be defined in kernel */
> +       sensor->button_map = devm_kzalloc(&fc->dev,
> +                       sensor->virtual_buttons.buttons,
> +                       GFP_KERNEL);
> +       if (!sensor->button_map) {
> +               dev_err(&fc->dev, "Failed to allocate the virtual button map.\n");
> +               return;
> +       }

So as noted above, since it's using devm_kzalloc(), don't free() it.

(...)
> +                       sensor->mouse_input = input_dev_mouse;
> +                       input_dev_mouse->name = "rmi_mouse";
> +                       input_dev_mouse->phys = "rmi_f11/input0";
> +
> +                       input_dev_mouse->id.vendor  = 0x18d1;
> +                       input_dev_mouse->id.product = 0x0210;
> +                       input_dev_mouse->id.version = 0x0100;

As noted in previous review, 0x18d1 is Google's vendor ID. Please use
a Synaptics Vendor ID, Product ID and version!

Hint: synaptics Vendor ID is 0x06cb.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists