Hi Jeff,
On 26/10/2023 07:05, Jeff LaBundy wrote:
Hi Neil,
On Mon, Oct 23, 2023 at 05:03:46PM +0200, Neil Armstrong wrote:
[...]
+static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
+{
+ __le16 length_raw;
+ u8 *afe_data;
+ u16 length;
+ int error;
+
+ afe_data = kzalloc(GOODIX_BERLIN_IC_INFO_MAX_LEN, GFP_KERNEL);
+ if (!afe_data)
+ return -ENOMEM;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ &length_raw, sizeof(length_raw));
+ if (error) {
+ dev_info(cd->dev, "failed get ic info length, %d\n", error);
This should be dev_err().
Ack
+ goto free_afe_data;
+ }
+
+ length = le16_to_cpu(length_raw);
+ if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
+ dev_info(cd->dev, "invalid ic info length %d\n", length);
And here.
Ack
+ error = -EINVAL;
+ goto free_afe_data;
+ }
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ afe_data, length);
+ if (error) {
+ dev_info(cd->dev, "failed get ic info data, %d\n", error);
+ return error;
+ goto free_afe_data;
+ }
This return statement is left over from v9; the print should also be dev_err().
Ack
+
+ /* check whether the data is valid (ex. bus default values) */
+ if (goodix_berlin_is_dummy_data(cd, afe_data, length)) {
+ dev_err(cd->dev, "fw info data invalid\n");
+ error = -EINVAL;
+ goto free_afe_data;
+ }
+
+ if (!goodix_berlin_checksum_valid(afe_data, length)) {
+ dev_info(cd->dev, "fw info checksum error\n");
And here.
Ack
+ error = -EINVAL;
+ goto free_afe_data;
+ }
+
+ error = goodix_berlin_convert_ic_info(cd, afe_data, length);
+ if (error)
+ goto free_afe_data;
+
+ /* check some key info */
+ if (!cd->touch_data_addr) {
+ dev_err(cd->dev, "touch_data_addr is null\n");
+ error = -EINVAL;
+ goto free_afe_data;
+ }
+
+ return 0;
+
+free_afe_data:
+ kfree(afe_data);
+
+ return error;
+}
[...]
+static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
+{
+ gpiod_set_value(cd->reset_gpio, 1);
+ usleep_range(2000, 2100);
+ gpiod_set_value(cd->reset_gpio, 0);
I see that now, this function is only called if the reset GPIO is defined,
but there used to be another msleep() here that has since been dropped. Is
that intentional?
Indeed, it was dropped, I'll add it back thx for noticing !
+
+ return 0;
+}
Kind regards,
Jeff LaBundy
Thanks,
Neil
