From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Dyer Subject: Re: [PATCH 2/2] Input: synaptics-rmi4 - add sysfs interfaces for hardware IDs Date: Sun, 22 Jan 2017 20:12:23 +0000 Message-ID: <20170122201223.GC11577@lava.h.shmanahar.org> References: <1483123398-27595-1-git-send-email-nick@shmanahar.org> <1483123398-27595-3-git-send-email-nick@shmanahar.org> <20170120134252.GI28187@mail.corp.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from avasout06.plus.net ([212.159.14.18]:37757 "EHLO avasout06.plus.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751047AbdAVUM1 (ORCPT ); Sun, 22 Jan 2017 15:12:27 -0500 Content-Disposition: inline In-Reply-To: <20170120134252.GI28187@mail.corp.redhat.com> Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Benjamin Tissoires Cc: Dmitry Torokhov , Andrew Duggan , Chris Healy , linux-input@vger.kernel.org On Fri, Jan 20, 2017 at 02:42:52PM +0100, Benjamin Tissoires wrote: > Subject: Re: [PATCH 2/2] Input: synaptics-rmi4 - add sysfs interfaces for > hardware IDs > > Hi Nick, > > Well, first, you need a changelog explaining why you need custom sysfs > files. It is for a firmware update system that works across a number of devices supported by the synaptics-rmi4 driver. It needs to manage multiple different versions of the firmware/hardware and which ones are compatible with which. I will put it in the commit message: although possibly the information would be useful even if you weren't trying to update firmware. > Then, I really wonder if this is the best solution. You are trying to > solve a general problem in a specific way, while other drivers/devices > could benefit from reporting such information in a generic way. > > I don't have a solution, but I know that we already had the case where > userspace wants to know which firmware is currently loaded on a > board[1]. > > A solution could be to add some ioctls to evdev to report those various > hardware IDs, but I am not so sure it will help in the RMI4 device while > in bootloader case. If we are trying to recover a broken device (eg corrupt firmware), we might not have the information to register an input device anyway. I agree that a generic solution to get this information would be nice. But we would have to map the complexities of the underlying hardware/firmware anyway - for example, an "input device" may have multiple components with separate firmwares having individual versions. So it would end up being a hierarchy with attributes of various types: isn't that just sysfs? > If Dmitry agrees to take this one, I have nothing against it. > > Cheers, > Benjamin > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=97912 > > On Dec 30 2016 or thereabouts, Nick Dyer wrote: > > Signed-off-by: Nick Dyer > > --- > > drivers/input/rmi4/rmi_driver.c | 2 +- > > drivers/input/rmi4/rmi_driver.h | 6 +- > > drivers/input/rmi4/rmi_f01.c | 45 ++++++++++++++- > > drivers/input/rmi4/rmi_f34.c | 123 ++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 172 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c > > index cb6efe6..e0359d6 100644 > > --- a/drivers/input/rmi4/rmi_driver.c > > +++ b/drivers/input/rmi4/rmi_driver.c > > @@ -365,7 +365,7 @@ static void rmi_driver_set_input_name(struct rmi_device *rmi_dev, > > struct input_dev *input) > > { > > struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev); > > - char *device_name = rmi_f01_get_product_ID(data->f01_container); > > + const char *device_name = rmi_f01_get_product_ID(data->f01_container); > > char *name; > > > > name = devm_kasprintf(&rmi_dev->dev, GFP_KERNEL, > > diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h > > index 24f8f76..48b3131 100644 > > --- a/drivers/input/rmi4/rmi_driver.h > > +++ b/drivers/input/rmi4/rmi_driver.h > > @@ -104,7 +104,11 @@ int rmi_init_functions(struct rmi_driver_data *data); > > int rmi_initial_reset(struct rmi_device *rmi_dev, void *ctx, > > const struct pdt_entry *pdt); > > > > -char *rmi_f01_get_product_ID(struct rmi_function *fn); > > +const char *rmi_f01_get_product_ID(struct rmi_function *fn); > > +u8 rmi_f01_get_manufacturer_ID(struct rmi_function *fn); > > +const char *rmi_f01_get_date_of_manufacture(struct rmi_function *fn); > > +u32 rmi_f01_get_firmware_ID(struct rmi_function *fn); > > +u32 rmi_f01_get_package_ID(struct rmi_function *fn); > > > > #ifdef CONFIG_RMI4_F34 > > int rmi_f34_create_sysfs(struct rmi_device *rmi_dev); > > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c > > index cae35c6..1b03f4d 100644 > > --- a/drivers/input/rmi4/rmi_f01.c > > +++ b/drivers/input/rmi4/rmi_f01.c > > @@ -13,6 +13,7 @@ > > #include > > #include > > #include > > +#include > > #include "rmi_driver.h" > > > > #define RMI_PRODUCT_ID_LENGTH 10 > > @@ -55,6 +56,7 @@ struct f01_basic_properties { > > u8 product_id[RMI_PRODUCT_ID_LENGTH + 1]; > > u16 productinfo; > > u32 firmware_id; > > + u32 package_id; > > }; > > > > /* F01 device status bits */ > > @@ -221,8 +223,19 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev, > > has_build_id_query = !!(queries[0] & BIT(1)); > > } > > > > - if (has_package_id_query) > > + if (has_package_id_query) { > > + ret = rmi_read_block(rmi_dev, prod_info_addr, > > + queries, sizeof(__le64)); > > + if (ret) { > > + dev_err(&rmi_dev->dev, > > + "Failed to read package info: %d\n", > > + ret); > > + return ret; > > + } > > + > > + props->package_id = get_unaligned_le64(queries); > > prod_info_addr++; > > + } > > > > if (has_build_id_query) { > > ret = rmi_read_block(rmi_dev, prod_info_addr, queries, > > @@ -242,13 +255,41 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev, > > return 0; > > } > > > > -char *rmi_f01_get_product_ID(struct rmi_function *fn) > > +const char *rmi_f01_get_product_ID(struct rmi_function *fn) > > { > > struct f01_data *f01 = dev_get_drvdata(&fn->dev); > > > > return f01->properties.product_id; > > } > > > > +u8 rmi_f01_get_manufacturer_ID(struct rmi_function *fn) > > +{ > > + struct f01_data *f01 = dev_get_drvdata(&fn->dev); > > + > > + return f01->properties.manufacturer_id; > > +} > > + > > +const char *rmi_f01_get_date_of_manufacture(struct rmi_function *fn) > > +{ > > + struct f01_data *f01 = dev_get_drvdata(&fn->dev); > > + > > + return f01->properties.dom; > > +} > > + > > +u32 rmi_f01_get_firmware_ID(struct rmi_function *fn) > > +{ > > + struct f01_data *f01 = dev_get_drvdata(&fn->dev); > > + > > + return f01->properties.firmware_id; > > +} > > + > > +u32 rmi_f01_get_package_ID(struct rmi_function *fn) > > +{ > > + struct f01_data *f01 = dev_get_drvdata(&fn->dev); > > + > > + return f01->properties.package_id; > > +} > > + > > #ifdef CONFIG_OF > > static int rmi_f01_of_probe(struct device *dev, > > struct rmi_device_platform_data *pdata) > > diff --git a/drivers/input/rmi4/rmi_f34.c b/drivers/input/rmi4/rmi_f34.c > > index d7709bc..b7b38c1 100644 > > --- a/drivers/input/rmi4/rmi_f34.c > > +++ b/drivers/input/rmi4/rmi_f34.c > > @@ -394,6 +394,122 @@ static int rmi_firmware_update(struct rmi_driver_data *data, > > return ret; > > } > > > > +static ssize_t rmi_driver_manufacturer_id_show(struct device *dev, > > + struct device_attribute *dattr, > > + char *buf) > > +{ > > + struct rmi_driver_data *data = dev_get_drvdata(dev); > > + struct rmi_function *fn = data->f01_container; > > + > > + return scnprintf(buf, PAGE_SIZE, "%d\n", > > + rmi_f01_get_manufacturer_ID(fn)); > > +} > > + > > +static DEVICE_ATTR(manufacturer_id, 0444, > > + rmi_driver_manufacturer_id_show, NULL); > > + > > +static ssize_t rmi_driver_dom_show(struct device *dev, > > + struct device_attribute *dattr, char *buf) > > +{ > > + struct rmi_driver_data *data = dev_get_drvdata(dev); > > + struct rmi_function *fn = data->f01_container; > > + > > + return scnprintf(buf, PAGE_SIZE, "%s\n", > > + rmi_f01_get_date_of_manufacture(fn)); > > +} > > + > > +static DEVICE_ATTR(date_of_manufacture, 0444, rmi_driver_dom_show, NULL); > > + > > +static ssize_t rmi_driver_product_id_show(struct device *dev, > > + struct device_attribute *dattr, > > + char *buf) > > +{ > > + struct rmi_driver_data *data = dev_get_drvdata(dev); > > + struct rmi_function *fn = data->f01_container; > > + > > + return scnprintf(buf, PAGE_SIZE, "%s\n", > > + rmi_f01_get_product_ID(fn)); > > +} > > + > > +static DEVICE_ATTR(product_id, 0444, > > + rmi_driver_product_id_show, NULL); > > + > > +static ssize_t rmi_driver_firmware_id_show(struct device *dev, > > + struct device_attribute *dattr, > > + char *buf) > > +{ > > + struct rmi_driver_data *data = dev_get_drvdata(dev); > > + struct rmi_function *fn = data->f01_container; > > + > > + return scnprintf(buf, PAGE_SIZE, "%d\n", > > + rmi_f01_get_firmware_ID(fn)); > > +} > > + > > +static DEVICE_ATTR(firmware_id, 0444, > > + rmi_driver_firmware_id_show, NULL); > > + > > +static ssize_t rmi_driver_bootloader_id_show(struct device *dev, > > + struct device_attribute *dattr, > > + char *buf) > > +{ > > + struct rmi_driver_data *data = dev_get_drvdata(dev); > > + struct rmi_function *fn = data->f34_container; > > + struct f34_data *f34; > > + > > + if (fn) { > > + f34 = dev_get_drvdata(&fn->dev); > > + > > + if (f34->bl_version == 5) > > + return scnprintf(buf, PAGE_SIZE, "%c%c\n", > > + f34->bootloader_id[0], > > + f34->bootloader_id[1]); > > + else > > + return scnprintf(buf, PAGE_SIZE, "V%d.%d\n", > > + f34->bootloader_id[1], > > + f34->bootloader_id[0]); > > + } > > + > > + return 0; > > +} > > + > > +static DEVICE_ATTR(bootloader_id, 0444, > > + rmi_driver_bootloader_id_show, NULL); > > + > > +static ssize_t rmi_driver_configuration_id_show(struct device *dev, > > + struct device_attribute *dattr, > > + char *buf) > > +{ > > + struct rmi_driver_data *data = dev_get_drvdata(dev); > > + struct rmi_function *fn = data->f34_container; > > + struct f34_data *f34; > > + > > + if (fn) { > > + f34 = dev_get_drvdata(&fn->dev); > > + > > + return scnprintf(buf, PAGE_SIZE, "%s\n", f34->configuration_id); > > + } > > + > > + return 0; > > +} > > + > > +static DEVICE_ATTR(configuration_id, 0444, > > + rmi_driver_configuration_id_show, NULL); > > + > > +static ssize_t rmi_driver_package_id_show(struct device *dev, > > + struct device_attribute *dattr, > > + char *buf) > > +{ > > + struct rmi_driver_data *data = dev_get_drvdata(dev); > > + struct rmi_function *fn = data->f01_container; > > + > > + u32 package_id = rmi_f01_get_package_ID(fn); > > + > > + return scnprintf(buf, PAGE_SIZE, "%04x.%04x\n", > > + package_id & 0xffff, (package_id >> 16) & 0xffff); > > +} > > + > > +static DEVICE_ATTR(package_id, 0444, rmi_driver_package_id_show, NULL); > > + > > static int rmi_firmware_update(struct rmi_driver_data *data, > > const struct firmware *fw); > > > > @@ -448,6 +564,13 @@ static DEVICE_ATTR(update_fw_status, 0444, > > rmi_driver_update_fw_status_show, NULL); > > > > static struct attribute *rmi_firmware_attrs[] = { > > + &dev_attr_bootloader_id.attr, > > + &dev_attr_configuration_id.attr, > > + &dev_attr_manufacturer_id.attr, > > + &dev_attr_date_of_manufacture.attr, > > + &dev_attr_product_id.attr, > > + &dev_attr_package_id.attr, > > + &dev_attr_firmware_id.attr, > > &dev_attr_update_fw.attr, > > &dev_attr_update_fw_status.attr, > > NULL > > -- > > 2.7.4 > >