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
