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

[Kamel BOUHARA]

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