Hi Dmitry,
Thank you for the quick review.
I've some comments to your comments on my driver
in 2 cases inline below. For the rest I agree and will
fix things for v2 of this patch.
On 07-04-18 00:35, Dmitry Torokhov wrote:
Hi Hans,
On Tue, Apr 03, 2018 at 08:03:48PM +0200, Hans de Goede wrote:
The ChipOne icn8505 is an i2c capacitive touchscreen controller typically
used in cheap x86 tablets, this commit adds a driver for it.
Note the icn8505 is somewhat similar to the icn8318 and I started with
modifying that driver to support both, but in the end the differences were
too large and I decided to write a new driver instead.
Cc: Antonio Davide Trogu <trogu.davide@xxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
<snip>
+static int icn8505_probe_acpi(struct icn8505_data *icn8505, struct device *dev)
+{
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ const char *subsys = "unknown";
+ struct acpi_device *adev;
+ union acpi_object *obj;
+ acpi_status status;
+
+ adev = ACPI_COMPANION(dev);
+ if (!adev)
+ return -ENODEV;
+
+ status = acpi_evaluate_object(adev->handle, "_SUB", NULL, &buffer);
+ if (ACPI_SUCCESS(status)) {
+ obj = buffer.pointer;
+ if (obj->type == ACPI_TYPE_STRING)
+ subsys = obj->string.pointer;
+ else
+ dev_warn(dev, "Warning ACPI _SUB did not return a string\n");
+ } else {
+ dev_warn(dev, "Warning ACPI _SUB failed: %#x\n", status);
+ buffer.pointer = NULL;
+ }
+
+ snprintf(icn8505->firmware_name, sizeof(icn8505->firmware_name),
+ "chipone/icn8505-%s.fw", subsys);
What if string is too long? How about using devm_kasprintf()?
The _SUB method returns an ACPI hwid type string which is always 8 chars,
so no need for kasprintf() here.
<snip>
+
+ ret = icn8505_upload_fw(icn8505);
+ if (ret < 0)
+ return ret;
It does not look like we need firmware to determine device capabilities,
consider moving it into ->open()?
Without firmware loaded the device nacks any i2c transfers on its
regular (non firmware upload) i2c address, so:
+
+ ret = icn8505_read_data(icn8505, ICN8505_REG_CONFIGDATA,
+ resolution, sizeof(resolution));
+ if (ret < 0) {
+ dev_err(dev, "Error reading resolution: %d\n", ret);
+ return ret;
+ }
This will fail and we cannot determine the resolution. Also at least
on the hardware I've when booted normally the EFI code has already
uploaded the firmware, so icn8505_upload_fw() hits the path where
it exits early because there is already firmware loaded and this
is (almost) free.
+
+ input_set_abs_params(input, ABS_MT_POSITION_X, 0,
+ le16_to_cpu(resolution[0]) - 1, 0, 0);
+ input_set_abs_params(input, ABS_MT_POSITION_Y, 0,
+ le16_to_cpu(resolution[1]) - 1, 0, 0);
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
