| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 6 Jan 2012 19:26:05 -0800
From: Christopher Heiny <cheiny@...aptics.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
CC: Jean Delvare <khali@...ux-fr.org>,
Linux Kernel <linux-kernel@...r.kernel.org>,
Linux Input <linux-input@...r.kernel.org>,
Joerie de Gram <j.de.gram@...il.com>,
Linus Walleij <linus.walleij@...ricsson.com>,
Naveen Kumar Gaddipati <naveen.gaddipati@...ricsson.com>
Subject: Re: [RFC PATCH 2/11] input: RMI4 core bus and sensor drivers.
On 01/05/2012 06:34 PM, Dmitry Torokhov wrote:
> Hi Christopher,
>
> Please find my [incomplete] comments below.
And my responses. Most of them are just ACKing your input, but there's
a few point of disagreement, which I've called out.
>
> On Wed, Dec 21, 2011 at 06:09:53PM -0800, Christopher Heiny wrote:
>> Signed-off-by: Christopher Heiny<cheiny@...aptics.com>
>>
>> Cc: Dmitry Torokhov<dmitry.torokhov@...il.com>
>> Cc: Linus Walleij<linus.walleij@...ricsson.com>
>> Cc: Naveen Kumar Gaddipati<naveen.gaddipati@...ricsson.com>
>> Cc: Joeri de Gram<j.de.gram@...il.com>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_bus.c | 436 ++++++++++++
>> drivers/input/rmi4/rmi_driver.c | 1488 +++++++++++++++++++++++++++++++++++++++
>> drivers/input/rmi4/rmi_driver.h | 97 +++
>> 3 files changed, 2021 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
[snip]
>> +static int rmi_bus_match(struct device *dev, struct device_driver *driver)
>> +{
>> + struct rmi_driver *rmi_driver;
>> + struct rmi_device *rmi_dev;
>> + struct rmi_device_platform_data *pdata;
>> +
>> + rmi_driver = to_rmi_driver(driver);
>> + rmi_dev = to_rmi_device(dev);
>> + pdata = to_rmi_platform_data(rmi_dev);
>> + dev_dbg(dev, "Matching %s.\n", pdata->sensor_name);
>> +
>> + if (!strcmp(pdata->driver_name, rmi_driver->driver.name)) {
>> + rmi_dev->driver = rmi_driver;
>> + dev_dbg(dev, "%s: Match %s to %s succeeded.\n", __func__,
>> + pdata->driver_name, rmi_driver->driver.name);
>> + return 1;
>> + }
>> +
>> + dev_err(dev, "%s: Match %s to %s failed.\n", __func__,
>> + pdata->driver_name, rmi_driver->driver.name);
>
> Why is this an error? dev_vdbg() at most, better yet just remove it.
It's useful when helping new customers bring up the driver on their
product. However, dev_dbg or dev_vdbg is a better choice for
production, so we'll change to that.
>
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int rmi_bus_suspend(struct device *dev)
>> +{
>> +#ifdef GENERIC_SUBSYS_PM_OPS
>> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> +
>> + if (pm&& pm->suspend)
>> + return pm->suspend(dev);
>> +#endif
>> +
>> + return 0;
>> +}
>> +
>> +static int rmi_bus_resume(struct device *dev)
>> +{
>> +#ifdef GENERIC_SUBSYS_PM_OPS
>> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> +
>> + if (pm&& pm->resume)
>> + return pm->resume(dev);
>> +#endif
>> +
>> + return 0;
>> +}
>> +#endif
>
> These are not needed if you switch to generic_subsys_pm_ops.
Unfortunately, we've got some customers on kernels that don't support
generic_subsys_pm_ops. We'll probably drop support for that later this
year, but in the meantime we'd like to retain it.
[snip]
>> +int rmi_register_phys_device(struct rmi_phys_device *phys)
>> +{
>> + struct rmi_device_platform_data *pdata = phys->dev->platform_data;
>> + struct rmi_device *rmi_dev;
>> +
>> + if (!pdata) {
>> + dev_err(phys->dev, "no platform data!\n");
>> + return -EINVAL;
>> + }
>> +
>> + rmi_dev = kzalloc(sizeof(struct rmi_device), GFP_KERNEL);
>> + if (!rmi_dev)
>> + return -ENOMEM;
>> +
>> + rmi_dev->phys = phys;
>> + rmi_dev->dev.bus =&rmi_bus_type;
>> +
>> + mutex_lock(&rmi_bus_mutex);
>> + rmi_dev->number = physical_device_count;
>> + physical_device_count++;
>> + mutex_unlock(&rmi_bus_mutex);
>
> Do
> rmi_dev->number = atomic_inc_return(&rmi_no) - 1;
>
> and stick "static atomic_t rmi_no = ATOMIC_INIT(0)"; at the beginning
> of the function. Then you don't need to take mutex here. Do you need
> rmi_dev->number?
Yes, it's used elsewhere. atomic_inc is a tidier way to manage this, so
we'll switch to that.
>
>> +
>> + dev_set_name(&rmi_dev->dev, "sensor%02d", rmi_dev->number);
>> + pr_debug("%s: Registered %s as %s.\n", __func__, pdata->sensor_name,
>> + dev_name(&rmi_dev->dev));
>> +
>> + phys->rmi_dev = rmi_dev;
>> + return device_register(&rmi_dev->dev);
>> +}
>> +EXPORT_SYMBOL(rmi_register_phys_device);
>> +
>> +void rmi_unregister_phys_device(struct rmi_phys_device *phys)
>> +{
>> + struct rmi_device *rmi_dev = phys->rmi_dev;
>> +
>> + device_unregister(&rmi_dev->dev);
>> + kfree(rmi_dev);
>
> This is lifetime rules violation; rmi_dev->dev might still be referenced
> and you are freeing it. Please provide proper release function.
Agreed. Thanks for catching this.
>
>> +}
>> +EXPORT_SYMBOL(rmi_unregister_phys_device);
>> +
>> +int rmi_register_driver(struct rmi_driver *driver)
>> +{
>> + driver->driver.bus =&rmi_bus_type;
>> + return driver_register(&driver->driver);
>> +}
>> +EXPORT_SYMBOL(rmi_register_driver);
>> +
>> +static int __rmi_driver_remove(struct device *dev, void *data)
>> +{
>> + struct rmi_driver *driver = data;
>> + struct rmi_device *rmi_dev = to_rmi_device(dev);
>> +
>> + if (rmi_dev->driver == driver)
>> + rmi_dev->driver = NULL;
>
> No cleanup whatsoever?
That certainly looks like a bug.
>
>> +
>> + return 0;
>> +}
>> +
>> +void rmi_unregister_driver(struct rmi_driver *driver)
>> +{
>> + bus_for_each_dev(&rmi_bus_type, NULL, driver, __rmi_driver_remove);
>
> Why don't you rely on driver core to unbind devices upon driver removal
> instead of rolling your own (and highly likely broken) implementation.
We'll look into that.
>
>> + driver_unregister(&driver->driver);
>> +}
>> +EXPORT_SYMBOL(rmi_unregister_driver);
>> +
[snip]
>> +
>> +void rmi_unregister_function_driver(struct rmi_function_handler *fh)
>> +{
>> + struct rmi_function_list *entry, *n;
>> +
>> + /* notify devices of the removal of the function handler */
>> + bus_for_each_dev(&rmi_bus_type, NULL, fh, __rmi_bus_fh_remove);
>> +
>> + list_for_each_entry_safe(entry, n,&rmi_supported_functions.list,
>> + list) {
>> + if (entry->fh->func == fh->func) {
>> + list_del(&entry->list);
>> + kfree(entry);
>> + }
>> + }
>
> You are still rolling partly your own infrastructure. It looks like you
> need 2 types of devices on rmi bus - composite RMI device and sensor
> device, see struct device_type. Make 2 of those and match drivers
> depending on the device type. Then driver core will maintain all the
> lists for you.
Much better. We'll look into that.
>
>> +
>> +}
>> +EXPORT_SYMBOL(rmi_unregister_function_driver);
>> +
>> +struct rmi_function_handler *rmi_get_function_handler(int id)
>> +{
>> + struct rmi_function_list *entry;
>> +
>> + list_for_each_entry(entry,&rmi_supported_functions.list, list)
>> + if (entry->fh->func == id)
>> + return entry->fh;
>> +
>> + return NULL;
>> +}
>> +EXPORT_SYMBOL(rmi_get_function_handler);
>> +
>> +static void rmi_release_character_device(struct device *dev)
>> +{
>> + dev_dbg(dev, "%s: Called.\n", __func__);
>> + return;
>
> No, no, no. Do not try to shut up warnings from device core; they are
> there for a reason.
I don't think that's what this is trying to do. But certainly it should
be doing something other than quietly saying "here".
>
>> +}
[snip]
>> +
>> +int rmi_register_character_driver(struct rmi_char_driver *char_driver)
>> +{
>> + struct rmi_character_driver_list *entry;
>> + int retval;
>> +
>> + pr_debug("%s: Registering character driver %s.\n", __func__,
>> + char_driver->driver.name);
>> +
>> + char_driver->driver.bus =&rmi_bus_type;
>> + INIT_LIST_HEAD(&char_driver->devices);
>> + retval = driver_register(&char_driver->driver);
>> + if (retval) {
>> + pr_err("%s: Failed to register %s, code: %d.\n", __func__,
>> + char_driver->driver.name, retval);
>> + return retval;
>> + }
>> +
>> + entry = kzalloc(sizeof(struct rmi_character_driver_list), GFP_KERNEL);
>> + if (!entry)
>> + return -ENOMEM;
>> + entry->cd = char_driver;
>> +
>> + mutex_lock(&rmi_bus_mutex);
>> + list_add_tail(&entry->list,&rmi_character_drivers.list);
>> + mutex_unlock(&rmi_bus_mutex);
>> +
>> + /* notify devices of the removal of the function handler */
>> + bus_for_each_dev(&rmi_bus_type, NULL, char_driver,
>> + rmi_register_character_device);
>
> Hmm, thisi is very roundabout way of attaching RMI chardevice... Does it
> even work if driver [re]appears after rmi_dev module was loaded?
>
> IFF we agree on keeping rmi_dev interface then I think something more
> elegant could be cooked via bus's blocking notifier.
It does appear to work in that situation, at least on the bench. We'll
look into the blocking notifier, though - like you say, better to avoid
re-inventing the functionality.
[snip]
>> +
>> +static int __init rmi_bus_init(void)
>> +{
>> + int error;
>> +
>> + mutex_init(&rmi_bus_mutex);
>> + INIT_LIST_HEAD(&rmi_supported_functions.list);
>> + INIT_LIST_HEAD(&rmi_character_drivers.list);
>> +
>> + error = bus_register(&rmi_bus_type);
>> + if (error< 0) {
>> + pr_err("%s: error registering the RMI bus: %d\n", __func__,
>> + error);
>> + return error;
>> + }
>> + pr_info("%s: successfully registered RMI bus.\n", __func__);
>
> This is all useless noise. Just do:
>
> return bus_register(&rmi_bus_type);
Not entirely useless, as it helps customers debug failures when first
integrating the driver. However, we'll switch pr_info to pr_debug.
>
>> +
>> + return 0;
>> +}
>> +
>> +static void __exit rmi_bus_exit(void)
>> +{
>> + struct rmi_function_list *entry, *n;
>> +
>> + list_for_each_entry_safe(entry, n,&rmi_supported_functions.list,
>> + list) {
>> + list_del(&entry->list);
>> + kfree(entry);
>> + }
>
> How can this list be non-free? Your bus code is reference by function
> drivers so module count is non zero until all such drivers are unloaded,
> and therefore rmi_bus_exit() can not be called.
Agreed. Will address this.
>
>> +
>> + bus_unregister(&rmi_bus_type);
>> +}
>> +
>> +module_init(rmi_bus_init);
>> +module_exit(rmi_bus_exit);
>> +
>> +MODULE_AUTHOR("Eric Andersson<eric.andersson@...xphere.com>");
>> +MODULE_DESCRIPTION("RMI bus");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>> new file mode 100644
>> index 0000000..07097bb
>> --- /dev/null
>> +++ b/drivers/input/rmi4/rmi_driver.c
>> @@ -0,0 +1,1488 @@
>> +/*
>> + * Copyright (c) 2011 Synaptics Incorporated
>> + * Copyright (c) 2011 Unixphere
>> + *
>> + * This driver adds support for generic RMI4 devices from Synpatics. It
>> + * implements the mandatory f01 RMI register and depends on the presence of
>> + * other required RMI functions.
>> + *
>> + * The RMI4 specification can be found here (URL split after files/ for
>> + * style reasons):
>> + * http://www.synaptics.com/sites/default/files/
>> + * 511-000136-01-Rev-E-RMI4%20Intrfacing%20Guide.pdf
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>> + */
>> +
>> +#include<linux/kernel.h>
>> +#include<linux/delay.h>
>> +#include<linux/device.h>
>> +#include<linux/module.h>
>> +#include<linux/device.h>
>> +#include<linux/slab.h>
>> +#include<linux/list.h>
>> +#include<linux/pm.h>
>> +#include<linux/rmi.h>
>> +#include "rmi_driver.h"
>> +
>> +#define DELAY_DEBUG 0
>> +#define REGISTER_DEBUG 0
>> +
>> +#define PDT_END_SCAN_LOCATION 0x0005
>> +#define PDT_PROPERTIES_LOCATION 0x00EF
>> +#define BSR_LOCATION 0x00FE
>> +#define HAS_BSR_MASK 0x20
>> +#define HAS_NONSTANDARD_PDT_MASK 0x40
>> +#define RMI4_END_OF_PDT(id) ((id) == 0x00 || (id) == 0xff)
>> +#define RMI4_MAX_PAGE 0xff
>> +#define RMI4_PAGE_SIZE 0x100
>> +
>> +#define RMI_DEVICE_RESET_CMD 0x01
>> +#define DEFAULT_RESET_DELAY_MS 20
>> +
>> +#ifdef CONFIG_HAS_EARLYSUSPEND
>> +static void rmi_driver_early_suspend(struct early_suspend *h);
>> +static void rmi_driver_late_resume(struct early_suspend *h);
>> +#endif
>
> Does not appear to be in mainline; please trim.
Understood, but we're trying to support Android kernels without forking
the heck out of our codebase.
>
>> +
>> +/* sysfs files for attributes for driver values. */
>> +static ssize_t rmi_driver_hasbsr_show(struct device *dev,
>> + struct device_attribute *attr, char *buf);
>
> Well, if it does not have bsr why do you even create bsr attribute?
We've been asking that very same question, and plan to change that.
>
>> +
>> +static ssize_t rmi_driver_bsr_show(struct device *dev,
>> + struct device_attribute *attr, char *buf);
>> +
>> +static ssize_t rmi_driver_bsr_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count);
>> +
>> +static ssize_t rmi_driver_enabled_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf);
>> +
>> +static ssize_t rmi_driver_enabled_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count);
>
> Should rely on load/unload or bind/unbind; no need for yet another
> mechanism.
This is needed as part of the support for the RED/Design Studio tools.
The variable name is misleading, though - it doesn't enable/disable the
driver, but enables/disables touch data processing for a single sensor.
We'll change the name of this.
>
>> +
>> +static ssize_t rmi_driver_phys_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf);
>> +
>> +static ssize_t rmi_driver_version_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf);
>
> Should be /sys/module/xxx/version already.
I don't find that. I'll look into why not.
>> +
>> +#if REGISTER_DEBUG
>> +static ssize_t rmi_driver_reg_store(struct device *dev,
>> + struct device_attribute *attr,
>> +
> const char *buf, size_t count);
> debugfs
Agree.
>> +#endif
>> +
>> +#if DELAY_DEBUG
>> +static ssize_t rmi_delay_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf);
>> +
>> +static ssize_t rmi_delay_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count);
>
> debugfs
Agree.
[snip]
[snip]
>> + dev_dbg(dev, "%s: Creating sysfs files.", __func__);
>> + for (attr_count = 0; attr_count< ARRAY_SIZE(attrs); attr_count++) {
>> + error = device_create_file(dev,&attrs[attr_count]);
>> + if (error< 0) {
>> + dev_err(dev, "%s: Failed to create sysfs file %s.\n",
>> + __func__, attrs[attr_count].attr.name);
>> + goto err_free_data;
>> + }
>> + }
>
> Use attribute group or driver infrastructure for registering common
> attributes.
Agree. We're looking into this.
>
>> +
>> + __mutex_init(&data->irq_mutex, "irq_mutex",&data->irq_key);
>
> Why not standard mutex_init()?
Not sure. Will correct this.
>
>
>> + data->current_irq_mask = kzalloc(sizeof(u8)*data->num_of_irq_regs,
>
> Spaces around arithmetic and other operations are appreciated.
Agree.
>
>> + GFP_KERNEL);
>> + if (!data->current_irq_mask) {
>> + dev_err(dev, "Failed to allocate current_irq_mask.\n");
>> + error = -ENOMEM;
>> + goto err_free_data;
>> + }
>> + error = rmi_read_block(rmi_dev,
>> + data->f01_container->fd.control_base_addr+1,
>> + data->current_irq_mask, data->num_of_irq_regs);
>> + if (error< 0) {
>> + dev_err(dev, "%s: Failed to read current IRQ mask.\n",
>> + __func__);
>> + goto err_free_data;
>> + }
>> + data->irq_mask_store = kzalloc(sizeof(u8)*data->num_of_irq_regs,
>> + GFP_KERNEL);
>> + if (!data->irq_mask_store) {
>> + dev_err(dev, "Failed to allocate mask store.\n");
>> + error = -ENOMEM;
>> + goto err_free_data;
>> + }
>> +
>> +#ifdef CONFIG_PM
>> + data->pm_data = pdata->pm_data;
>> + data->pre_suspend = pdata->pre_suspend;
>> + data->post_resume = pdata->post_resume;
>
> Is it really platform dependent?
Yes. Some platforms have special things they want to do before/after
the touchpad suspends/resumes.
>
>> +
>> + mutex_init(&data->suspend_mutex);
>> +
>> +#ifdef CONFIG_HAS_EARLYSUSPEND
>> + rmi_dev->early_suspend_handler.level =
>> + EARLY_SUSPEND_LEVEL_BLANK_SCREEN + 1;
>> + rmi_dev->early_suspend_handler.suspend = rmi_driver_early_suspend;
>> + rmi_dev->early_suspend_handler.resume = rmi_driver_late_resume;
>> + register_early_suspend(&rmi_dev->early_suspend_handler);
>
> Not in mainline.
Yes, but as noted before we need to support Android without forking our
driver codebase.
>
>> +#endif /* CONFIG_HAS_EARLYSUSPEND */
>> +#endif /* CONFIG_PM */
>> + data->enabled = true;
>> +
>> + dev_info(dev, "connected RMI device manufacturer: %s product: %s\n",
>> + data->manufacturer_id == 1 ? "synaptics" : "unknown",
>> + data->product_id);
>> +
>> + return 0;
>> +
>> + err_free_data:
>> + rmi_free_function_list(rmi_dev);
>> + for (attr_count--; attr_count>= 0; attr_count--)
>> + device_remove_file(dev,&attrs[attr_count]);
>> + if (data) {
>
> You exit earlier if data is NULL.
Agree.
>
>> + if (data->f01_container)
>> + kfree(data->f01_container->irq_mask);
>> + kfree(data->irq_mask_store);
>> + kfree(data->current_irq_mask);
>> + kfree(data);
>> + rmi_set_driverdata(rmi_dev, NULL);
>> + }
>> + return error;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int rmi_driver_suspend(struct device *dev)
>> +{
>> + struct rmi_device *rmi_dev;
>> + struct rmi_driver_data *data;
>> + struct rmi_function_container *entry;
>> + int retval = 0;
>> +
>> + rmi_dev = to_rmi_device(dev);
>> + data = rmi_get_driverdata(rmi_dev);
>> +
>> + mutex_lock(&data->suspend_mutex);
>> + if (data->suspended)
>> + goto exit;
>> +
>> +#ifndef CONFIG_HAS_EARLYSUSPEND
>> + if (data->pre_suspend) {
>> + retval = data->pre_suspend(data->pm_data);
>> + if (retval)
>> + goto exit;
>> + }
>> +#endif /* !CONFIG_HAS_EARLYSUSPEND */
>> +
>> + list_for_each_entry(entry,&data->rmi_functions.list, list)
>> + if (entry->fh&& entry->fh->suspend) {
>> + retval = entry->fh->suspend(entry);
>> + if (retval< 0)
>> + goto exit;
>> + }
>> +
>> + if (data->f01_container&& data->f01_container->fh
>> + && data->f01_container->fh->suspend) {
>> + retval = data->f01_container->fh->suspend(data->f01_container);
>> + if (retval< 0)
>> + goto exit;
>> + }
>> + data->suspended = true;
>> +
>> +exit:
>> + mutex_unlock(&data->suspend_mutex);
>> + return retval;
>
> Once it is better integrated in driver core this will be much simpler.
OK.
>
>> +}
>> +
>> +static int rmi_driver_resume(struct device *dev)
>> +{
>> + struct rmi_device *rmi_dev;
>> + struct rmi_driver_data *data;
>> + struct rmi_function_container *entry;
>> + int retval = 0;
>> +
>> + rmi_dev = to_rmi_device(dev);
>> + data = rmi_get_driverdata(rmi_dev);
>> +
>> + mutex_lock(&data->suspend_mutex);
>> + if (!data->suspended)
>> + goto exit;
>> +
>> + if (data->f01_container&& data->f01_container->fh
>> + && data->f01_container->fh->resume) {
>> + retval = data->f01_container->fh->resume(data->f01_container);
>> + if (retval< 0)
>> + goto exit;
>> + }
>> +
>> + list_for_each_entry(entry,&data->rmi_functions.list, list)
>> + if (entry->fh&& entry->fh->resume) {
>> + retval = entry->fh->resume(entry);
>> + if (retval< 0)
>> + goto exit;
>> + }
>> +
>> +#ifndef CONFIG_HAS_EARLYSUSPEND
>> + if (data->post_resume) {
>> + retval = data->post_resume(data->pm_data);
>> + if (retval)
>> + goto exit;
>> + }
>> +#endif /* !CONFIG_HAS_EARLYSUSPEND */
>> +
>> + data->suspended = false;
>> +
>> +exit:
>> + mutex_unlock(&data->suspend_mutex);
>> + return retval;
>
> This one too.
OK
[snip]
>> +
>> +static ssize_t rmi_delay_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct rmi_device *rmi_dev;
>> + struct rmi_device_platform_data *pdata;
>> +
>> + rmi_dev = to_rmi_device(dev);
>> + pdata = rmi_dev->phys->dev->platform_data;
>> +
>> + return snprintf(buf, PAGE_SIZE, "%d %d %d %d %d\n",
>> + pdata->spi_data.read_delay_us, pdata->spi_data.write_delay_us,
>> + pdata->spi_data.block_delay_us,
>> + pdata->spi_data.pre_delay_us, pdata->spi_data.post_delay_us);
>
> This violates "one value per attribute" principle. Also it does not look
> like it is essential for device operation but rather diagnostic
> facility. Switch to debugfs?
Agree with debugfs. However, these are values that really ought to be
managed as a group, rather than one at a time.
>
>> +}
>> +#endif
>> +
>> +static ssize_t rmi_driver_phys_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct rmi_device *rmi_dev;
>> + struct rmi_phys_info *info;
>> +
>> + rmi_dev = to_rmi_device(dev);
>> + info =&rmi_dev->phys->info;
>> +
>> + return snprintf(buf, PAGE_SIZE, "%-5s %ld %ld %ld %ld %ld %ld %ld\n",
>> + info->proto ? info->proto : "unk",
>> + info->tx_count, info->tx_bytes, info->tx_errs,
>> + info->rx_count, info->rx_bytes, info->rx_errs,
>> + info->attn_count);
>
> Same as delay.
Agree with the debugfs part. However, this is a bunch of related
numbers that capture the state of the system at particular point in
time, and it doesn't make sense to me to spread it across a bunch of
different files. We used things like /proc/loadavg for the model here,
for good or ill.
[snip]
Thanks for all the input!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists