Re: [RFC PATCH 06/06] input/rmi4: F11 - 2D touch interface
From: Linus Walleij
Date: Tue Oct 09 2012 - 06:02:53 EST
On Sat, Oct 6, 2012 at 6:10 AM, Christopher Heiny <cheiny@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/