Fundamental Design Flaw of the Device Driver Model?
===================================================
Sorry for the misleading subject, its purpose is to draw your attention :-)
The ideas below are preliminary and I hope I'm not making serious mistakes
here.
This question has actually been around in my mind for several months, when
I started to work on some devices with multiple functions. Specifically, a
Power Management IC (PMIC in short in the following text) usually includes
LEDs support (charging, indication...) audio, touch screen, power monitoring,
LDOs, DC-DC bucks, and possibly some others.
The initial two ideas came into my mind were:
1. separate the functions into multiple devices, write a driver for each
of these devices
2. a big all-in-one driver
Pros and Cons
Solution (1) is obviously more attractive: better modularization and
structure, work can be incrementally done, individual patches can go
through each subsystem's git tree, reviewed by dedicated people, and
so on.
One thing I'm not so happy with solution (1) is: you have to create
intermediate devices in order that the dedicated sub-device drivers
to match up. E.g.
==== pmic.c =============================================================
...
pmic_add_subdevs(void)
{
for all sub-devices on this pmic:
pdev = platform_device_alloc(sub-device-name, sub-device-id)
platform_device_add(pdev);
...
}
pmic_probe(struct i2c_client *client)
{
...
pmic_add_subdevs();
...
}
struct i2c_driver pmic_driver = {
.probe = pmic_probe,
...
}
==== pmic.c =============================================================
==== pmic-subdev.c ======================================================
static int sub_device_driver_probe(struct platform_device *sub_device)
{
... ...
}
static struct platform_driver pmic_subdev_driver = {
.name = sub-device-name,
.probe = sub_device_driver_probe,
... ...
}
==== pmic-subdev.c ======================================================
This, however, creates many questions you have to face with:
1. on what bus shall these sub-devices be?
** this is the reason I choose to use "platform_device", at least they
can reside on the platform_bus_type, thus platform_driver can be used
for this sub-device
2. these devices are actually useless except for linking the sub-device
to it's sub-device driver and of course, wasting memory. Normally in
the driver, another dedicated device will be created. E.g. let's take
a typical simple LED driver as an example:
static int __devinit xxxx_led_probe(struct BUS_device *dev)
{
struct led_classdev led;
/* initialize led */
...
led_classdev_register(dev, &led);
...
}
static struct BUS_driver xxxx_led_driver = {
.probe = xxxx_led_probe,
...
};
While led_classdev_register() is implemented something like:
int led_classdev_register(struct device *parent, struct led_classdev *led)
{
1. allocate led device
2. initialize led device
3. assign "struct class leds_class"
4. add device specific attributes
5. add device to a global list (LED subsystem specific)
return OK or ERR
}
As you notice, a new LED device is created. If there are multiple
LEDs on the chip, (yes, we have a PMIC with 5 LEDs on-chip, each
differs only at the offset of its controlling register), then you
probably will have to create 5 LED devices here, and manage them
in a whole. In recent terminology of the device-model, such device
is called a virtual device, it has no bus, no driver ..., it's used
so that class, attributes can attach.
Hence, here comes the 3rd and 4th questions:
3. Who should be the correct parent of these LED devices, the intermediate
sub-device we created just now? Or the pmic device on the I2C bus?
My answer is the the latter, obviously. However, writing code like:
led_classdev_register(pdev->dev.parent, &led_cdev)
is apparantly funny and misleading.
4. An intermediate device with no bus, no driver, no many other things
is really not something deserving a "struct device", that's a waste
of memory.
I'd really like to blame the device model that prevent me from writing a
perfect driver for such multi-function devices, otherwise I'd like to
blame the semiconductor manufacturers to come up with such devices making
us write ugly drivers. But I couldn't:
1. the PMIC design is nature, if I were the PMIC designer, with so many
analog circuitry already built-in, I would also add ADC, Touch, Audio
PWM, and many others to make them squeeze into the tiny chip. (No,
not including using I2C bus, which I'd prefer to avoid)
2. There's no fundamental flaw in the device model (sorry for the title),
of course, after weeks of reviewing the underlying code.
So probably, we are writing drivers in an incorrect way, that we have
ignored for the following reasons:
1. Most Linux developers are working on the PC world, which usually don't
have to care about the multi-function devices, one PCI card, one PCI
device, and one PCI device one function, we have a PCI driver to handle
that, in that PCI driver, implement the function we need, that's all.
2. Even if there are multiple sub-devices, they are probably of the same
functionality, and creating an array is easy, managing them in a whole
in the driver is really not a big deal.
3. We don't strictly distinguish between a physical device and a virtual
device, a physical device driver and a virtual device driver. E.g. a
virtual device could be a "net_device", "mtd_device", "led_classdev",
and many others
A normal device layout would be:
device specific
virtual bus type
|
platform_bus_type i2c_bus_type | virtual devices
| | | |
(device) V V V V
Platform BUS ---> I2C Controller ---> PMIC -+-> LED device (1)
|
+-> LED device (2)
|
+-> LED device (3)
|
+-> DC-DC Buck1
|
+-> DC-DC Buck2
|
+-> LDO1
|
+-> LDO2
|
+-> Backlight PWM1
|
+-> Backlight PWM2
|
...
the startup sequence would be:
1. bus controller being initialized
2. each bus_device on this bus controller is identified
3. registration of these bus_device[] triggers the match of the
bus_driver[] on this bus type
4. the bus_driver for this device creates all the virtual devices
found, (device can be registered to a virtual bus type)
5. virtual device driver matches the virtual device and implement
the function of this virtual device
Now it can be seen that:
1. virtual devices are no different than physical devices, instead
of being registered on a physical bus, they can be registered
on a device specific virtual bus, or a function specific bus.
(Let's say a net-bus, led-bus, mtd-bus, probably within
/sys/devices/virtual/net/, /sys/devices/virtual/mtd, and so on)
Or even on a global somewhat virtual_bus, which will probably
increase the iteration match time.
2. no intermediate device required for the device/driver match-up,
the parent of the virtual device is the physical device, and
this sounds reasonable
3. one virtual driver for many virtual devices of the same type,
driver doesn't have to managing them by array or link-list.
4. everything else still works
The only inconvenience would be for those physical device with only
_one_ virtual device, that by using a virtual device the driver, it
(we really want just a single driver for the physical device and the
virtual device) will possibly look like:
static int virtual_driver_probe ( struct device *virtual_device )
{
...
}
static virtual_device_driver {
.probe = virtual_driver_probe,
...
}
static int physical_driver_probe ( struct device *physical_device )
{
...
add_one_virtual_device(physical_device_as_parent);
...
return 0;
}
static physical_device_driver {
.probe = physical_driver_probe,
...
}
module_init:
physical_driver_register (&physical_device_driver);
virtual_driver_register (&virtual_device_driver);
This doesn't look nice, a generic virtual device driver is needed here.
If we define the network device as something like:
struct net_device {
struct device dev;
struct net_device_ops *ops;
...
}
static int generic_net_probe(struct device *dev)
{
struct net_device *net_dev = to_net_device(dev);
net_dev->ops->init();
add_to_net_device_list(net_dev);
register_to_network_layer(net_dev);
...
}
struct device_driver generic_net_device_driver = {
.name = "generic_virtual_net_device_driver",
.probe = generic_net_probe,
....
}
Then probably, each physical network device driver will be written like:
static int physical_driver_probe ( struct device *physical_device )
{
/* net_dev as the virtual device of this physical one */
struct net_device *net_dev;
...
net_dev->ops = xxxx;
add_one_virtual_device(physical_device_as_parent);
...
return 0;
}
static physical_device_driver {
.probe = physical_driver_probe,
...
}
module_init:
physical_driver_register (&physical_device_driver);
virtual_driver_register (&virtual_device_driver);
This is same as what we are doing now, the difference is that: instead of
calling register_netdev() by the physical driver itself, it's now creating
a virtual device and using the device-driver bus model for the generic
virtual network driver to match. As said, on such 1 physical device + 1
virtual device case, the benefit isn't significant. (that's why our driver
is written like it is today)
Finally comes an example of how LED device driver shall be implemented with
the above concept change, this is illustration _only_, no guarantee to work.
==== board.c ==============================================================
... ...
static struct gpio_led_platform_data board_leds[] = {
[0] = {
.name = "gpio-led",
.gpio = 10,
.active_low = 1,
},
[1] = {
.name = "gpio-led",
.gpio = 11,
.active_low = 1,
},
};
static void __init board_init_leds(void)
{
gpio_led_add(gpio_dev, 0, &board_debug_leds[0]);
gpio_led_add(gpio_dev, 1, &board_debug_leds[1]);
}
... ...
==== board.c ==============================================================
==== include/linux/leds/led.h =============================================
#ifndef __LED_H
#define __LED_H
#include <linux/device.h>
enum {
LED_OFF = 0,
LED_HALF = 127,
LED_FULL = 255,
};
struct led_device;
struct led_device_ops {
void (*set_brightness)(struct led_device *, int brightness);
int (*get_brightness)(struct led_device *);
};
struct led_device {
struct device dev;
const char *name;
int id;
int brightness;
struct led_device_ops *ops;
};
#define to_led_device(dev) container_of((dev), struct led_device, dev)
extern struct bus_type led_bus_type;
extern struct class led_class;
extern struct led_device *led_device_alloc(const char *name, int id);
extern int led_device_add(struct led_device *ldev);
extern int led_device_register(struct led_device *ldev);
extern void led_device_unregister(struct led_device *ldev);
#endif /* __LED_H */
==== include/linux/leds/led.h =============================================
==== drivers/leds/core.c ==================================================
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/leds/led.h>
static struct device led_bus_device = {
.bus_id = "leds",
};
static void led_device_release(struct device *dev)
{
kfree(to_led_device(dev));
}
struct led_device *led_device_alloc(const char *name, int id)
{
struct led_device *ldev;
ldev = kzalloc(sizeof(struct led_device), GFP_KERNEL);
if (ldev == NULL)
return NULL;
ldev->name = name;
ldev->id = id;
ldev->brightness = 0;
device_initialize(&ldev->dev);
ldev->dev.release = led_device_release;
return ldev;
}
EXPORT_SYMBOL_GPL(led_device_alloc);
int led_device_add(struct led_device *ldev)
{
if (!ldev)
return -EINVAL;
if (!ldev->dev.parent)
ldev->dev.parent = &led_bus_device;
ldev->dev.bus = &led_bus_type;
ldev->dev.class = &led_class;
if (ldev->id != -1)
snprintf(ldev->dev.bus_id, BUS_ID_SIZE, "%s.%d",
ldev->name, ldev->id);
else
strlcpy(ldev->dev.bus_id, ldev->name, BUS_ID_SIZE);
pr_debug("Registering led device '%s'. Parent at %s\n",
ldev->dev.bus_id, ldev->dev.parent->bus_id);
return device_add(&ldev->dev);
}
EXPORT_SYMBOL_GPL(led_device_add);
int led_device_register(struct led_device *ldev)
{
device_initialize(&ldev->dev);
return led_device_add(ldev);
}
EXPORT_SYMBOL_GPL(led_device_register);
void led_device_unregister(struct led_device *ldev)
{
if (ldev) {
device_del(&ldev->dev);
put_device(&ldev->dev);
}
}
EXPORT_SYMBOL_GPL(led_device_unregister);
static void led_set_brightness(struct led_device *ldev, int brightness)
{
if (ldev->ops && ldev->ops->set_brightness)
ldev->ops->set_brightness(ldev, brightness);
ldev->brightness = brightness;
}
static int led_get_brightness(struct led_device *ldev)
{
if (ldev->ops && ldev->ops->get_brightness)
return ldev->ops->get_brightness(ldev);
return ldev->brightness;
}
static ssize_t led_brightness_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
return sprintf(buf, "%d\n", led_get_brightness(to_led_device(dev)));
}
static ssize_t led_brightness_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)
{
struct led_device *ldev = to_led_device(dev);
int brightness = (int) simple_strtol(buf, NULL, 0);
led_set_brightness(ldev, brightness);
return count;
}
static struct device_attribute led_device_attrs[] = {
__ATTR(brightness, 0644, led_brightness_show, led_brightness_store),
__ATTR_NULL,
};
#ifdef CONFIG_PM
static int led_class_suspend(struct device *dev, pm_message_t state)
{
struct led_device *ldev = to_led_device(dev);
if (ldev->ops && ldev->ops->set_brightness)
ldev->ops->set_brightness(ldev, 0);
return 0;
}
static int led_class_resume(struct device *dev)
{
struct led_device *ldev = to_led_device(dev);
if (ldev->ops && ldev->ops->set_brightness)
ldev->ops->set_brightness(ldev, ldev->brightness);
return 0;
}
#else
#define led_class_suspend NULL
#define led_class_resume NULL
#endif /* CONFIG_PM */
struct class led_class = {
.name = "led-class",
.owner = THIS_MODULE,
.dev_attrs = led_device_attrs,
.suspend = led_class_suspend,
.resume = led_class_resume,
};
EXPORT_SYMBOL_GPL(led_class);
static int led_bus_match(struct device *dev, struct device_driver *drv)
{
struct led_device *ldev = to_led_device(dev);
return (strncmp(ldev->name, drv->name, BUS_ID_SIZE) == 0);
}
static int led_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
{
struct led_device *ldev = to_led_device(dev);
add_uevent_var(env, "MODALIAS=led:%s", ldev->name);
return 0;
}
struct bus_type led_bus_type = {
.name = "led-bus",
.match = led_bus_match,
.uevent = led_bus_uevent,
};
EXPORT_SYMBOL_GPL(led_bus_type);
static int __init led_core_init(void)
{
int err;
err = device_register(&led_bus_device);
if (err)
return err;
err = bus_register(&led_bus_type);
if (err)
return err;
err = class_register(&led_class);
if (err)
return err;
return 0;
}
subsys_initcall(led_core_init);
static void __exit led_core_exit(void)
{
class_unregister(&led_class);
bus_unregister(&led_bus_type);
device_unregister(&led_bus_device);
}
module_exit(led_core_exit);
==== drivers/leds/core.c ==================================================
==== include/linux/leds/led-gpio.h ========================================
#ifndef __LED_GPIO_H
#define __LED_GPIO_H
#include <linux/leds/led.h>
struct gpio_led_platform_data {
unsigned gpio;
unsigned active_low : 1;
};
extern struct led_device *gpio_led_add(struct device *parent, int id,
struct gpio_led_platform_data *pdata);
extern void gpio_led_del(struct led_device *ldev);
#endif /* __LED_GPIO_H */
==== include/linux/leds/led-gpio.h ========================================
==== drivers/leds/led-gpio.c ==============================================
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/gpio.h>
#include <linux/leds/led.h>
#include <linux/leds/led-gpio.h>
struct led_device *gpio_led_add(struct device *parent, int id,
struct gpio_led_platform_data *pdata)
{
struct led_device *ldev;
struct gpio_led_platform_data *p;
int err;
ldev = led_device_alloc("led-gpio", id);
if (ldev == NULL)
return NULL;
p = kmalloc(sizeof(struct gpio_led_platform_data), GFP_KERNEL);
if (p == NULL) {
kfree(ldev);
return NULL;
}
memcpy(p, pdata, sizeof(*pdata));
ldev->dev.platform_data = p;
err = led_device_add(ldev);
if (err) {
kfree(p);
put_device(&ldev->dev);
return NULL;
}
return ldev;
}
EXPORT_SYMBOL_GPL(gpio_led_add);
void gpio_led_del(struct led_device *ldev)
{
led_device_unregister(ldev);
}
EXPORT_SYMBOL_GPL(gpio_led_del);
struct gpio_led_data {
unsigned gpio;
unsigned active_low : 1;
unsigned can_sleep : 1;
int new_level;
struct work_struct work;
};
static void gpio_led_work(struct work_struct *work)
{
struct gpio_led_data *data;
data = container_of(work, struct gpio_led_data, work);
gpio_set_value_cansleep(data->gpio, data->new_level);
}
static void gpio_led_set_brightness(struct led_device *ldev, int brightness)
{
struct gpio_led_data *data = dev_get_drvdata(&ldev->dev);
int level;
level = (brightness == LED_OFF) ? 0 : 1;
if (data->active_low)
level = !level;
if (data->can_sleep) {
data->new_level = level;
schedule_work(&data->work);
} else
gpio_set_value(data->gpio, level);
}
static struct led_device_ops gpio_led_ops = {
.set_brightness = gpio_led_set_brightness,
};
static int __devinit gpio_led_probe(struct device *dev)
{
struct gpio_led_platform_data *pdata = dev->platform_data;
struct led_device *ldev = to_led_device(dev);
struct gpio_led_data *data;
int err;
ldev->ops = &gpio_led_ops;
err = gpio_request(pdata->gpio, dev->bus_id);
if (err) {
dev_err(dev, "failed to request GPIO%d\n", pdata->gpio);
return -EBUSY;
}
data = kzalloc(sizeof(struct gpio_led_data), GFP_KERNEL);
if (data == NULL) {
dev_err(dev, "failed to allocate memory\n");
return -ENOMEM;
}
data->gpio = pdata->gpio;
data->active_low = pdata->active_low;
data->can_sleep = gpio_cansleep(data->gpio);
data->new_level = 0;
INIT_WORK(&data->work, gpio_led_work);
dev_set_drvdata(dev, data);
return 0;
}
static int __devexit gpio_led_remove(struct device *dev)
{
struct gpio_led_data *data = dev_get_drvdata(dev);
kfree(data);
return 0;
}
static struct device_driver gpio_led_driver = {
.name = "led",
.bus = &led_bus_type,
.owner = THIS_MODULE,
.probe = gpio_led_probe,
.remove = __devexit_p(gpio_led_remove),
};
static int __init gpio_led_init(void)
{
return driver_register(&gpio_led_driver);
}
module_init(gpio_led_init);
static void __exit gpio_led_exit(void)
{
driver_unregister(&gpio_led_driver);
}
module_exit(gpio_led_exit);
==== drivers/leds/led-gpio.c ==============================================
Cheers
- eric
> Fundamental Design Flaw of the Device Driver Model?
> ===================================================
>
> Sorry for the misleading subject, its purpose is to draw your attention :-)
> The ideas below are preliminary and I hope I'm not making serious mistakes
> here.
>
> This question has actually been around in my mind for several months, when
> I started to work on some devices with multiple functions. Specifically, a
> Power Management IC (PMIC in short in the following text) usually includes
> LEDs support (charging, indication...) audio, touch screen, power monitoring,
> LDOs, DC-DC bucks, and possibly some others.
>
> The initial two ideas came into my mind were:
>
> 1. separate the functions into multiple devices, write a driver for each
> of these devices
Go for 1.
> 4. An intermediate device with no bus, no driver, no many other things
> is really not something deserving a "struct device", that's a waste
> of memory.
Memory is not _that_ expensive, and struct device is not that
big. Adding infrastructure to driver model for supporting this would
also cost you memory, this time in .text segment.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri, Aug 22, 2008 at 5:03 PM, Pavel Machek <[email protected]> wrote:
>> Fundamental Design Flaw of the Device Driver Model?
>> ===================================================
>>
>> Sorry for the misleading subject, its purpose is to draw your attention :-)
>> The ideas below are preliminary and I hope I'm not making serious mistakes
>> here.
>>
>> This question has actually been around in my mind for several months, when
>> I started to work on some devices with multiple functions. Specifically, a
>> Power Management IC (PMIC in short in the following text) usually includes
>> LEDs support (charging, indication...) audio, touch screen, power monitoring,
>> LDOs, DC-DC bucks, and possibly some others.
>>
>> The initial two ideas came into my mind were:
>>
>> 1. separate the functions into multiple devices, write a driver for each
>> of these devices
>
> Go for 1.
>
>> 4. An intermediate device with no bus, no driver, no many other things
>> is really not something deserving a "struct device", that's a waste
>> of memory.
>
> Memory is not _that_ expensive, and struct device is not that
> big. Adding infrastructure to driver model for supporting this would
> also cost you memory, this time in .text segment.
>
Actually I don't mind wasting additional memory for a better structure,
but option (1) isn't. And I just assume you have read to the end of thi
mail, then you will see it's more like a concept change,no fundamental
change to the code itself.
Actually the reason to have two different kind of devices are obvious:
1. physical device - handles the bus related operations (read, write,
locking, I/O)
2. virtual device - handles the functionality (interface with the
upper framework)
With this separation, the same IP on different semiconductor can be
shared naturally. There are lot of such products esp in embedded market,
with multiple shared IPs within a single die.
Attached is a patch series to get you guys a feeling how the option
(1) will look like, and think again about the device-driver relationship,
the correctness of introducing an intermediate platform_device.
On Fri, 2008-08-22 at 14:25 +0800, Eric Miao wrote:
> Fundamental Design Flaw of the Device Driver Model?
> ===================================================
>
> Sorry for the misleading subject, its purpose is to draw your attention :-)
> The ideas below are preliminary and I hope I'm not making serious mistakes
> here.
>
> This question has actually been around in my mind for several months, when
> I started to work on some devices with multiple functions. Specifically, a
> Power Management IC (PMIC in short in the following text) usually includes
> LEDs support (charging, indication...) audio, touch screen, power monitoring,
> LDOs, DC-DC bucks, and possibly some others.
>
> The initial two ideas came into my mind were:
>
> 1. separate the functions into multiple devices, write a driver for each
> of these devices
>
I've opted for this option with the WM8350 PMIC driver.
> This, however, creates many questions you have to face with:
>
> 1. on what bus shall these sub-devices be?
> ** this is the reason I choose to use "platform_device", at least they
> can reside on the platform_bus_type, thus platform_driver can be used
> for this sub-device
My WM8350 clients are all platform_devices too.
>
> 2. these devices are actually useless except for linking the sub-device
> to it's sub-device driver and of course, wasting memory. Normally in
> the driver, another dedicated device will be created. E.g. let's take
> a typical simple LED driver as an example:
>
They are not useless as most of my client devices _need_ some sort of
platform data to be passed on for their probe() e.g. my LED driver needs
to know it's brightness values, trigger etc. They also all need a PMIC
reference (passed in the platform_data) which they will need to call any
core PMIC functions e.g. chip read/write
>
> 3. Who should be the correct parent of these LED devices, the intermediate
> sub-device we created just now? Or the pmic device on the I2C bus?
> My answer is the the latter, obviously. However, writing code like:
>
> led_classdev_register(pdev->dev.parent, &led_cdev)
Fwiw, I've made the WM8350 I2C driver the parent. The I2C driver also
contains the core PMIC services e.g. IRQs, GPIO's, IO, etc
> A normal device layout would be:
>
> device specific
> virtual bus type
> |
> platform_bus_type i2c_bus_type | virtual devices
> | | | |
> (device) V V V V
> Platform BUS ---> I2C Controller ---> PMIC -+-> LED device (1)
> |
> +-> LED device (2)
> |
> +-> LED device (3)
> |
> +-> DC-DC Buck1
> |
> +-> DC-DC Buck2
> |
> +-> LDO1
> |
> +-> LDO2
> |
> +-> Backlight PWM1
> |
> +-> Backlight PWM2
> |
> ...
>
Ok, I basically have :-
Platform devices
| |
V V
Platform Bus --> I2C controller --> PMIC +-> LED1
+-> LED2
+-> Audio
+-> DCDC1
+-> Backlight
I originally had a PMIC bus (like above) but decided it was easier just
to use the existing kernel infrastructure. My I2C PMIC driver just
registers the client platform_devices when it probes.
Cheers
Liam
On Fri, Aug 22, 2008 at 11:32 AM, Eric Miao <[email protected]> wrote:
> On Fri, Aug 22, 2008 at 5:03 PM, Pavel Machek <[email protected]> wrote:
>>> Fundamental Design Flaw of the Device Driver Model?
>>> ===================================================
>>>
>>> Sorry for the misleading subject, its purpose is to draw your attention :-)
>>> The ideas below are preliminary and I hope I'm not making serious mistakes
>>> here.
>>>
>>> This question has actually been around in my mind for several months, when
>>> I started to work on some devices with multiple functions. Specifically, a
>>> Power Management IC (PMIC in short in the following text) usually includes
>>> LEDs support (charging, indication...) audio, touch screen, power monitoring,
>>> LDOs, DC-DC bucks, and possibly some others.
>>>
>>> The initial two ideas came into my mind were:
>>>
>>> 1. separate the functions into multiple devices, write a driver for each
>>> of these devices
>>
>> Go for 1.
>>
>>> 4. An intermediate device with no bus, no driver, no many other things
>>> is really not something deserving a "struct device", that's a waste
>>> of memory.
>>
>> Memory is not _that_ expensive, and struct device is not that
>> big. Adding infrastructure to driver model for supporting this would
>> also cost you memory, this time in .text segment.
>>
>
> Actually I don't mind wasting additional memory for a better structure,
> but option (1) isn't. And I just assume you have read to the end of thi
> mail, then you will see it's more like a concept change,no fundamental
> change to the code itself.
>
> Actually the reason to have two different kind of devices are obvious:
>
> 1. physical device - handles the bus related operations (read, write,
> locking, I/O)
> 2. virtual device - handles the functionality (interface with the
> upper framework)
>
> With this separation, the same IP on different semiconductor can be
> shared naturally. There are lot of such products esp in embedded market,
> with multiple shared IPs within a single die.
>
> Attached is a patch series to get you guys a feeling how the option
> (1) will look like, and think again about the device-driver relationship,
> the correctness of introducing an intermediate platform_device.
Instead of allocating/adding the platform subdevices manually in
da903x_add_subdevs, couldn't you use the MFD (multi function device)
infrastructure that seems to be intended for this case? Also, I think
that the da9030 base driver would then better fit in drivers/mfd, not
drivers/i2c/chips (didn't Jean Delvare himself state that ideally
drivers/i2c/chips should be empty?).
regards
Philipp
Eric Miao wrote:
> I'd really like to blame the device model that prevent me from writing a
> perfect driver for such multi-function devices, otherwise I'd like to
> blame the semiconductor manufacturers to come up with such devices making
> us write ugly drivers. But I couldn't:
>
> 1. the PMIC design is nature, if I were the PMIC designer, with so many
> analog circuitry already built-in, I would also add ADC, Touch, Audio
> PWM, and many others to make them squeeze into the tiny chip. (No,
> not including using I2C bus, which I'd prefer to avoid)
>
> 2. There's no fundamental flaw in the device model (sorry for the title),
> of course, after weeks of reviewing the underlying code.
>
> So probably, we are writing drivers in an incorrect way, that we have
> ignored for the following reasons:
>
> 1. Most Linux developers are working on the PC world, which usually don't
> have to care about the multi-function devices, one PCI card, one PCI
> device, and one PCI device one function, we have a PCI driver to handle
> that, in that PCI driver, implement the function we need, that's all.
>
> 2. Even if there are multiple sub-devices, they are probably of the same
> functionality, and creating an array is easy, managing them in a whole
> in the driver is really not a big deal.
>
> 3. We don't strictly distinguish between a physical device and a virtual
> device, a physical device driver and a virtual device driver. E.g. a
> virtual device could be a "net_device", "mtd_device", "led_classdev",
> and many others
I didn't read and understand all of your post. Nevertheless I dare to
comment on these last 3 points here, because they don't fully match what
we already have been dealing with for a long time now. For example,
SCSI (including newer transport types over serial buses or networks)
deals with target devices which have children called logical units.
Logical units of the same target can have very different functionality
(= implement different command sets) while there is still a dependency
of the logical units on the target which provides them. Other example:
USB presumably deals with multi-function devices with some inner
structure and dependency too. (I don't know details.)
Ditto FireWire: There are "nodes" on a bus, and each node can have 0..n
"units". The units can implement various different protocols. During
the presence of a node on the bus, units on it can come and go at any
time. (Our drivers handle this in a simplified manner though.) The
ieee1394 driver stack and its future replacement, the firewire stack,
instantiate a struct device for each node, and a struct device for each
unit. There are appropriate parent-child relationships between them.
(Furthermore, node devices are children of the respective bus controller
device.)
All these devices have the same bus_type. The older ieee1394 core
distinguishes between node devices and unit devices by means of
different class devices. The newer, leaner firewire core distinguishes
between them by means of different device_type.
There are high-level drivers which are bound to unit devices, but not to
node devices. These drivers have the necessary knowledge of the
hierarchy though because their business with a unit device typically
also involves some dealings with the parent and grandparent.
(Types of the new stack: struct fw_device and struct fw_unit in
drivers/firewire/fw-device.[hc],
types of the old stack: struct node_entry and struct unit_directory in
drivers/ieee1394/nodemgr.[hc])
Anyway; there are several approaches to multi-function devices possible.
Wherever you go, the goal should be to keep it simple. (The ieee1394
stack is not a good example for this; the firewire stack is better in
this regard.) It typically makes a lot of sense to create struct device
based objects _not_ based on which boundaries the silicon draws, but
which functionality is there. E.g. if you can operate the various
functions of a hardware by means of separate drivers that don't
interfere a lot with each other (a core layer may control such
interferences), then separate struct device embedding objects for each
functionality are a good idea.
And you may not even have to resort to the term "virtual" when talking
about devices or device types or bus types if you structure it all
nicely and reasonably simple.
--
Stefan Richter
-=====-==--- =--- =-==-
http://arcgraph.de/sr/
On 8/22/08, Eric Miao <[email protected]> wrote:
======================================================
> This, however, creates many questions you have to face with:
>
> 1. on what bus shall these sub-devices be?
> ** this is the reason I choose to use "platform_device", at least they
> can reside on the platform_bus_type, thus platform_driver can be used
> for this sub-device
Another option is making your own bus. If I understand your hardware
it effectively has an internal bus.
--
Jon Smirl
[email protected]
> Instead of allocating/adding the platform subdevices manually in
> da903x_add_subdevs, couldn't you use the MFD (multi function device)
> infrastructure that seems to be intended for this case? Also, I think
> that the da9030 base driver would then better fit in drivers/mfd, not
> drivers/i2c/chips (didn't Jean Delvare himself state that ideally
> drivers/i2c/chips should be empty?).
Actually, I don't mind keep it anywhere. The intermediate
platform_device is what I'd like to avoid mostly. Using my
own simple logic allows me to do that later.
On Fri, Aug 22, 2008 at 10:33 AM, Jon Smirl <[email protected]> wrote:
> On 8/22/08, Eric Miao <[email protected]> wrote:
> ======================================================
>> This, however, creates many questions you have to face with:
>>
>> 1. on what bus shall these sub-devices be?
>> ** this is the reason I choose to use "platform_device", at least they
>> can reside on the platform_bus_type, thus platform_driver can be used
>> for this sub-device
>
> Another option is making your own bus. If I understand your hardware
> it effectively has an internal bus.
>
That's another option around, but it didn't solve my fundamental question
of, (e.g. an PCI card with multiple network interfaces and other functionality):
Why should I have to create an intermediate device provided that a
"struct net_device" already contains a "struct device"? And that
device-driver binding, parameter passing (platform_data), bus and
other functionalities of this "struct net_device" is not used while
that's used solely by that intermediate device (platform_device maybe)?
They should have perfectly been combined into a single virtual device.
On 8/22/08, Eric Miao <[email protected]> wrote:
> On Fri, Aug 22, 2008 at 10:33 AM, Jon Smirl <[email protected]> wrote:
> > On 8/22/08, Eric Miao <[email protected]> wrote:
> > ======================================================
> >> This, however, creates many questions you have to face with:
> >>
> >> 1. on what bus shall these sub-devices be?
> >> ** this is the reason I choose to use "platform_device", at least they
> >> can reside on the platform_bus_type, thus platform_driver can be used
> >> for this sub-device
> >
> > Another option is making your own bus. If I understand your hardware
> > it effectively has an internal bus.
> >
>
>
> That's another option around, but it didn't solve my fundamental question
> of, (e.g. an PCI card with multiple network interfaces and other functionality):
>
> Why should I have to create an intermediate device provided that a
> "struct net_device" already contains a "struct device"? And that
> device-driver binding, parameter passing (platform_data), bus and
> other functionalities of this "struct net_device" is not used while
> that's used solely by that intermediate device (platform_device maybe)?
>
> They should have perfectly been combined into a single virtual device.
>
First device represent the hardware with the PCI interface for the
card. Instantiating it creates a bus for the card. You then auto add
devices to this bus for each of the sub-devices on the card. Auto add
is fine in this case since the sub-devices aren't optional.
A multifunction card really is a local private bus with multiple
devices on it. First device is a real device - it's the bus controller
for the multifunction card.
A stranger problem is encountered with audio hardware. The SOC CPU
code loads an i2s/ac97 driver. A generic driver for the audio codec is
also loaded. Now you have to create a strange "fabric" device which
represent the specific PCB the generic codec and SOC have been
soldered into. The fabric driver describes how the codec and CPU are
wired together and if all of the codec functions are brought out to
jacks. For example, you don't want to display an ALSA capture device
if the codec supports it but no mic-in jack has been soldered to the
PCB. Is the fabric device a real device? You can't program it but you
can't get rid of it either.
--
Jon Smirl
[email protected]