RE: [PATCH v4 4/14] input: cyapa: add cyapa key function interfaces in sysfs system

[Dudley Du]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
> Sent: Friday, August 22, 2014 2:29 AM
> To: Dudley Du
> Cc: Rafael J. Wysocki; Benson Leung; Patrik Fimml; linux-input@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Dudley Du
> Subject: Re: [PATCH v4 4/14] input: cyapa: add cyapa key function interfaces
> in sysfs system
> 
> On Thu, Jul 17, 2014 at 02:51:04PM +0800, Dudley Du wrote:
> > Add key basic function interfaces in cyapa driver in sysfs system,
> > these interfaces are commonly used in pre- and after production, and
> > for trackpad device state checking, manage and firmware image updating.
> > These interfaces including firmware_version and product_id interfaces
> > for reading firmware version and trackpad device product id values,
> > and including update_fw interface to command firmware image update
> > process. Also including baseline and calibrate interfaces, so can
> > read and check the trackpad device states. If the baseline values are
> > invalid, then can use calibrate interface to recover it.
> > TEST=test on Chromebooks.
> >
> > Signed-off-by: Dudley Du <dudl@xxxxxxxxxxx>
> > ---
> >  drivers/input/mouse/cyapa.c |  190
> +++++++++++++++++++++++++++++++++++++++++++
> >  drivers/input/mouse/cyapa.h |   27 ++++++
> >  2 files changed, 217 insertions(+)
> >
> > diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> > index 6a2783b..53c9d59 100644
> > --- a/drivers/input/mouse/cyapa.c
> > +++ b/drivers/input/mouse/cyapa.c
> > @@ -388,6 +388,78 @@ static void cyapa_detect(struct cyapa *cyapa)
> >  	}
> >  }
> >
> > +static int cyapa_firmware(struct cyapa *cyapa, const char *fw_name)
> > +{
> > +	struct device *dev = &cyapa->client->dev;
> > +	int ret;
> > +	const struct firmware *fw;
> > +
> > +	ret = request_firmware(&fw, fw_name, dev);
> > +	if (ret) {
> > +		dev_err(dev, "Could not load firmware from %s, %d\n",
> > +			fw_name, ret);
> > +		return ret;
> > +	}
> > +
> > +	if (cyapa->ops->check_fw) {
> > +		ret = cyapa->ops->check_fw(cyapa, fw);
> > +		if (ret) {
> > +			dev_err(dev, "Invalid CYAPA firmware image: %s\n",
> > +					fw_name);
> > +			goto done;
> > +		}
> > +	} else {
> > +		dev_err(dev, "Unknown status, operation forbidden, gen=%d\n",
> > +			cyapa->gen);
> > +		ret = -ENOTSUPP;
> 
> I'd say EIO or EINVAL.

Thanks, I will set it to EINVAL in next release.


> 
> > +		goto done;
> > +	}
> > +
> > +	/*
> > +	 * Resume the potentially suspended device because doing FW
> > +	 * update on a device not in the FULL mode has a chance to
> > +	 * fail.
> > +	 */
> > +	pm_runtime_get_sync(dev);
> > +
> > +	if (cyapa->ops->bl_enter) {
> > +		ret = cyapa->ops->bl_enter(cyapa);
> > +		if (ret)
> > +			goto err_detect;
> > +	}
> > +
> > +	if (cyapa->ops->bl_activate) {
> > +		ret = cyapa->ops->bl_activate(cyapa);
> > +		if (ret)
> > +			goto err_detect;
> > +	}
> > +
> > +	if (cyapa->ops->bl_initiate) {
> > +		ret = cyapa->ops->bl_initiate(cyapa, fw);
> > +		if (ret)
> > +			goto err_detect;
> > +	}
> > +
> > +	if (cyapa->ops->update_fw) {
> > +		ret = cyapa->ops->update_fw(cyapa, fw);
> > +		if (ret)
> > +			goto err_detect;
> > +	}
> > +
> > +	if (cyapa->ops->bl_verify_app_integrity) {
> > +		ret = cyapa->ops->bl_verify_app_integrity(cyapa);
> > +		if (ret)
> > +			goto err_detect;
> > +	}
> > +
> > +err_detect:
> > +	pm_runtime_put_noidle(dev);
> > +
> > +done:
> > +	release_firmware(fw);
> > +	return ret;
> > +}
> > +
> >  /*
> >   * Sysfs Interface.
> >   */
> > @@ -584,6 +656,120 @@ static void cyapa_start_runtime(struct cyapa *cyapa)
> >  static void cyapa_start_runtime(struct cyapa *cyapa) {}
> >  #endif /* CONFIG_PM_RUNTIME */
> >
> > +static ssize_t cyapa_show_fm_ver(struct device *dev,
> > +				 struct device_attribute *attr, char *buf)
> > +{
> > +	int ret;
> > +	struct cyapa *cyapa = dev_get_drvdata(dev);
> > +
> > +	mutex_lock(&cyapa->state_sync_lock);
> > +	ret = scnprintf(buf, PAGE_SIZE, "%d.%d\n", cyapa->fw_maj_ver,
> > +			 cyapa->fw_min_ver);
> > +	mutex_unlock(&cyapa->state_sync_lock);
> > +	return ret;
> > +}
> > +
> > +static ssize_t cyapa_show_product_id(struct device *dev,
> > +				     struct device_attribute *attr, char *buf)
> > +{
> > +	int ret;
> > +	struct cyapa *cyapa = dev_get_drvdata(dev);
> > +
> > +	mutex_lock(&cyapa->state_sync_lock);
> > +	ret = scnprintf(buf, PAGE_SIZE, "%s\n", cyapa->product_id);
> > +	mutex_unlock(&cyapa->state_sync_lock);
> > +	return ret;
> > +}
> > +
> > +static ssize_t cyapa_update_fw_store(struct device *dev,
> > +				     struct device_attribute *attr,
> > +				     const char *buf, size_t count)
> > +{
> > +	struct cyapa *cyapa = dev_get_drvdata(dev);
> > +	const char *fw_name;
> > +	int ret;
> > +
> > +	/* Do not allow paths that step out of /lib/firmware  */
> > +	if (strstr(buf, "../") != NULL)
> > +		return -EINVAL;
> 
> This should go away, it does not help anything.

Thanks. It will be removed in next release.

> 
> > +
> > +	if (!strncmp(buf, "1", count) || !strncmp(buf, "1\n", count))
> > +		fw_name = CYAPA_FW_NAME;
> > +	else
> > +		fw_name = buf;
> 
> I'd rather you either required firmware name to be passed always or settle on
> given name. Otherwise why can't I call my firmware file '1'?
> 

Thanks. I will modify to require firmware name to be passed always.

> > +
> > +	mutex_lock(&cyapa->state_sync_lock);
> > +
> > +	ret = cyapa_firmware(cyapa, fw_name);
> > +	if (ret)
> > +		dev_err(dev, "firmware update failed, %d\n", ret);
> > +	else
> > +		dev_dbg(dev, "firmware update succeeded\n");
> > +
> > +	mutex_unlock(&cyapa->state_sync_lock);
> > +
> > +	/* Redetect trackpad device states. */
> > +	cyapa_detect_async(cyapa, 0);
> > +
> > +	return ret ? ret : count;
> > +}
> > +
> > +static ssize_t cyapa_calibrate_store(struct device *dev,
> > +				     struct device_attribute *attr,
> > +				     const char *buf, size_t count)
> > +{
> > +	struct cyapa *cyapa = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	mutex_lock(&cyapa->state_sync_lock);
> > +
> > +	if (!cyapa->ops->calibrate_store) {
> > +		dev_err(dev, "Calibrate operation not permitted.\n");
> > +		ret = -ENOTSUPP;
> 
> Permitted as in supported or forbidden?

The "permitted" means not supported, because device state is unknown.
I will modify the word "permitted" to "supported".
Thanks.

> 
> > +	} else
> > +		ret = cyapa->ops->calibrate_store(dev, attr, buf, count);
> > +
> > +	mutex_unlock(&cyapa->state_sync_lock);
> > +	return ret < 0 ? ret : count;
> > +}
> > +
> > +static ssize_t cyapa_show_baseline(struct device *dev,
> > +				   struct device_attribute *attr, char *buf)
> > +{
> > +	struct cyapa *cyapa = dev_get_drvdata(dev);
> > +	ssize_t ret;
> > +
> > +	mutex_lock(&cyapa->state_sync_lock);
> > +
> > +	if (!cyapa->ops->show_baseline) {
> > +		dev_err(dev, "Calibrate operation not permitted.\n");
> > +		ret = -ENOTSUPP;
> > +	} else
> > +		ret = cyapa->ops->show_baseline(dev, attr, buf);
> > +
> > +	mutex_unlock(&cyapa->state_sync_lock);
> > +	return ret;
> > +}
> > +
> > +static DEVICE_ATTR(firmware_version, S_IRUGO, cyapa_show_fm_ver, NULL);
> > +static DEVICE_ATTR(product_id, S_IRUGO, cyapa_show_product_id, NULL);
> > +static DEVICE_ATTR(update_fw, S_IWUSR, NULL, cyapa_update_fw_store);
> > +static DEVICE_ATTR(baseline, S_IRUGO, cyapa_show_baseline, NULL);
> > +static DEVICE_ATTR(calibrate, S_IWUSR, NULL, cyapa_calibrate_store);
> > +
> > +static struct attribute *cyapa_sysfs_entries[] = {
> > +	&dev_attr_firmware_version.attr,
> > +	&dev_attr_product_id.attr,
> > +	&dev_attr_update_fw.attr,
> > +	&dev_attr_baseline.attr,
> > +	&dev_attr_calibrate.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group cyapa_sysfs_group = {
> > +	.attrs = cyapa_sysfs_entries,
> > +};
> > +
> >  void cyapa_detect_async(void *data, async_cookie_t cookie)
> >  {
> >  	struct cyapa *cyapa = (struct cyapa *)data;
> > @@ -680,6 +866,9 @@ static int cyapa_probe(struct i2c_client *client,
> >  		goto err_uninit_tp_modules;
> >  	}
> >
> > +	if (sysfs_create_group(&client->dev.kobj, &cyapa_sysfs_group))
> > +		dev_warn(dev, "error creating sysfs entries.\n");
> > +
> >  #ifdef CONFIG_PM_SLEEP
> >  	if (device_can_wakeup(dev) &&
> >  	    sysfs_merge_group(&client->dev.kobj, &cyapa_power_wakeup_group))
> > @@ -704,6 +893,7 @@ static int cyapa_remove(struct i2c_client *client)
> >  	struct cyapa *cyapa = i2c_get_clientdata(client);
> >
> >  	pm_runtime_disable(&client->dev);
> > +	sysfs_remove_group(&client->dev.kobj, &cyapa_sysfs_group);
> >
> >  #ifdef CONFIG_PM_SLEEP
> >  	sysfs_unmerge_group(&client->dev.kobj, &cyapa_power_wakeup_group);
> > diff --git a/drivers/input/mouse/cyapa.h b/drivers/input/mouse/cyapa.h
> > index 9c1f0b91..567ab08 100644
> > --- a/drivers/input/mouse/cyapa.h
> > +++ b/drivers/input/mouse/cyapa.h
> > @@ -171,6 +171,22 @@ struct cyapa;
> >  typedef bool (*cb_sort)(struct cyapa *, u8 *, int);
> >
> >  struct cyapa_dev_ops {
> > +	int (*check_fw)(struct cyapa *, const struct firmware *);
> > +	int (*bl_enter)(struct cyapa *);
> > +	int (*bl_activate)(struct cyapa *);
> > +	int (*bl_initiate)(struct cyapa *, const struct firmware *);
> > +	int (*update_fw)(struct cyapa *, const struct firmware *);
> > +	int (*bl_verify_app_integrity)(struct cyapa *);
> > +	int (*bl_deactivate)(struct cyapa *);
> > +
> > +	ssize_t (*show_baseline)(struct device *,
> > +			struct device_attribute *, char *);
> > +	ssize_t (*calibrate_store)(struct device *,
> > +			struct device_attribute *, const char *, size_t);
> > +
> > +	int (*read_fw)(struct cyapa *);
> > +	int (*read_raw_data)(struct cyapa *);
> > +
> >  	int (*initialize)(struct cyapa *cyapa);
> >  	int (*uninitialize)(struct cyapa *cyapa);
> >
> > @@ -243,6 +259,17 @@ struct cyapa {
> >  	 */
> >  	struct mutex state_sync_lock;
> >
> > +	/* Per-instance debugfs root */
> > +	struct dentry *dentry_dev;
> > +
> > +	/* Buffer to store firmware read using debugfs */
> > +	struct mutex debugfs_mutex;
> > +	u8 *fw_image;
> > +	size_t fw_image_size;
> > +	/* Buffer to store sensors' raw data */
> > +	u8 *tp_raw_data;
> > +	size_t tp_raw_data_size;
> > +
> >  	const struct cyapa_dev_ops *ops;
> >  };
> >
> > --
> > 1.7.9.5
> >
> >
> 
> Thanks.
> 
> --
> Dmitry

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/