Re: [PATCH v16 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Le 2024-07-03 20:22, Marco Felsch a écrit :
Hi Kamel,

thank you for the updated version please see my comment inline.

Hi Marco,

Thanks for this new review :) !

[...]

+
+struct axiom_data {
+	struct axiom_devinfo devinfo;
+	struct device *dev;
+	struct gpio_desc *reset_gpio;
			    ^
The reset is done within the probe so this can be dropped.


Ack, thanks.

+	struct i2c_client *client;
			    ^
The client is never used. I would either store the client or the device
reference but not both.

Sure, nice catch, I'll just keep the device reference here.


+	struct input_dev *input_dev;
+	u32 max_report_len;
+	u8 rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];

Is there a reason for having the rx_buf within the driver priv data?
IMHO it's more error probe since we never set it to zero before we use
the buffer. I would rather move the rx-buffer to the functions which
perform the read.

Ok, there no real/technical reason to not move it to reading functions.


+	struct axiom_u41_target targets[AXIOM_U41_MAX_TARGETS];
+	struct axiom_usage_entry usage_table[AXIOM_U31_MAX_USAGES];
+	bool usage_table_populated;
		^
This can be an inline helper which checks the max_report_len e.g:

static inline bool axiom_usage_table_populated(struct axiom_data *ts)
{
	return ts->max_report_len;
}


Ack, just for my curiosity, is this for perfomance or clarity sake ?

+	struct regmap *regmap;
+	struct touchscreen_properties	prop;
+};
+

[...]

+static int axiom_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct input_dev *input_dev;
+	struct axiom_data *ts;
+	u32 poll_interval;
+	int target;
+	int error;
+
+	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, ts);
+	ts->client = client;
+	ts->dev = dev;
+
+	ts->regmap = devm_regmap_init_i2c(client, &axiom_i2c_regmap_config);
+	error = PTR_ERR_OR_ZERO(ts->regmap);
+	if (error) {
+		dev_err(dev, "Failed to initialize regmap: %d\n", error);
+		return error;
+	}
+
+	error = devm_regulator_get_enable(dev, "vddi");
+	if (error)
+		return dev_err_probe(&client->dev, error,
+				     "Failed to enable VDDI regulator\n");
+
+	error = devm_regulator_get_enable(dev, "vdda");
+	if (error)
+		return dev_err_probe(&client->dev, error,
+				     "Failed to enable VDDA regulator\n");
+
+ ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ts->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(ts->reset_gpio), "failed to get reset GPIO\n");
+
+	/* Make sure the time after a power ON sequence is meet */
+	if (ts->reset_gpio)
+		axiom_reset(ts->reset_gpio);
+	else

No else just:

Fixed, thanks !


+		msleep(AXIOM_STARTUP_TIME_MS);

	msleep(AXIOM_STARTUP_TIME_MS);

and drop the msleep within the axiom_reset().

Regards,
  Marco

[...]

--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux