Re: [PATCH 2/2] Input: synaptics-rmi4 - add sysfs interfaces for hardware IDs

[Nick Dyer <nick@shmanahar.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 <nick@shmanahar.org>
> > ---
> >  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 <linux/slab.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/of.h>
> > +#include <asm/unaligned.h>
> >  #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
> >