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

[Kamel BOUHARA <kamel.bouhara@bootlin.com>]

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