--- On Thu, 1/4/10, Alan Cox <[email protected]> wrote:
> From: Alan Cox <[email protected]>
> Subject: Re: [PATCH] drivers:staging: sources for ST core
> To: [email protected]
> Cc: "Greg KH" <[email protected]>, "Marcel Holtmann" <[email protected]>, [email protected]
> Date: Thursday, 1 April, 2010, 2:50 PM
> > +/*
> > + * function to return whether the firmware response
> was proper
> > + * in case of error don't complete so that waiting
> for proper
> > + * response times out
> > + */
> > +void validate_firmware_response(struct sk_buff *skb)
> > +{
> > +??? if (unlikely(skb->data[5] !=
> 0)) {
> > +??? ??? pr_err("no
> proper response during fw download");
> > +??? ??? pr_err("data6
> %x", skb->data[5]);
>
> In this driver you do know the device so you need to be
> using dev_ and
> passing around dev (or something that gives you dev).
>
> > +static int kim_probe(struct platform_device *pdev)
> > +{
> > +??? long status;
> > +??? long proto;
> > +??? long *gpios =
> pdev->dev.platform_data;
> > +
> > +??? status =
> st_core_init(&kim_gdata->core_data);
>
> I would expect any truely global data to be configured in
> the module init
> and then device specific data you want to do something like
> this
>
> ??? kim_data = kzalloc(sizeof(something),
> GFP_KERNEL);
>
> ??? ..
>
> ??? kim_data_init(&pdev->dev,
> kim_data);
> ??? dev_set_drvdata(&pdev->dev,
> kim_data);
>
> Elsewhere you can now do
>
> ??? kim_data =
> dev_get_drvdata(&pdev->dev);
>
> to get it back
There are 2 sets of data structures here (after removing the un-necessary 3rd one),
1. st_gdata - which I would want to tie to tty
2. kim_gdata - which I "would" like to tie to the pdev.
Now the problem being, I have reference of st_gdata in kim_gdata, and there are about 4 functions in the st_core where I would need the st_gdata, as in,
EXPORTED symbol st_register - because I need to add in entries as to who registered,
+long st_register(struct st_proto_s *new_proto)
+{
+??? struct st_data_s??? *st_gdata;
+??? long err = ST_SUCCESS;
+??? unsigned long flags = 0;
+
+??? st_kim_ref(&st_gdata);
+??? pr_info("%s(%d) ", __func__, new_proto->type);
+??? if (st_gdata == NULL || new_proto == NULL || new_proto->recv == NULL
+??? ? ? || new_proto->reg_complete_cb == NULL) {
+??? ??? pr_err("gdata/new_proto/recv or reg_complete_cb not ready");
+??? ??? return ST_ERR_FAILURE;
+??? }
Also in st_unregister and st_write for similar purposes,
But I also need it in tty_open, to link it to the disc_data,
+static int st_tty_open(struct tty_struct *tty)
+{
+??? int err = ST_SUCCESS;
+??? struct st_data_s *st_gdata;
+??? pr_info("%s ", __func__);
+
+??? st_kim_ref(&st_gdata);
+??? st_gdata->tty = tty;
+??? tty->disc_data = st_gdata;
So, shouldn't some function like st_kim_ref be enough ?
+void st_kim_ref(struct st_data_s **core_data)
+{
+??? *core_data = kim_gdata->core_data;
+}
So Now st_gdata is tied to tty, and kim_gdata being purely global, as in only 1 platform device of such kind can exist.
Suppose 2 platform devices want to exist - then who's EXPORT of st_register is considered ?
And If bluetooth or FM wants to use this transport then, how would it tell onto which platform device it wants to attach to ?
Why should I tie kim_gdata to a pdev ?
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at? http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at? http://www.tux.org/lkml/
>
The INTERNET now has a personality. YOURS! See your Yahoo! Homepage. http://in.yahoo.com/
--- On Thu, 1/4/10, Pavan Savoy <[email protected]> wrote:
> From: Pavan Savoy <[email protected]>
> Subject: Re: [PATCH] drivers:staging: sources for ST core
> To: "Alan Cox" <[email protected]>
> Cc: "Greg KH" <[email protected]>, "Marcel Holtmann" <[email protected]>, [email protected]
> Date: Thursday, 1 April, 2010, 10:50 PM
> --- On Thu, 1/4/10, Alan Cox <[email protected]>
> wrote:
>
> > From: Alan Cox <[email protected]>
> > Subject: Re: [PATCH] drivers:staging: sources for ST
> core
> > To: [email protected]
> > Cc: "Greg KH" <[email protected]>,
> "Marcel Holtmann" <[email protected]>,
> [email protected]
> > Date: Thursday, 1 April, 2010, 2:50 PM
> > > +/*
> > > + * function to return whether the firmware
> response
> > was proper
> > > + * in case of error don't complete so that
> waiting
> > for proper
> > > + * response times out
> > > + */
> > > +void validate_firmware_response(struct sk_buff
> *skb)
> > > +{
> > > +??? if (unlikely(skb->data[5] !=
> > 0)) {
> > > +??? ??? pr_err("no
> > proper response during fw download");
> > > +??? ??? pr_err("data6
> > %x", skb->data[5]);
> >
> > In this driver you do know the device so you need to
> be
> > using dev_ and
> > passing around dev (or something that gives you dev).
> >
> > > +static int kim_probe(struct platform_device
> *pdev)
> > > +{
> > > +??? long status;
> > > +??? long proto;
> > > +??? long *gpios =
> > pdev->dev.platform_data;
> > > +
> > > +??? status =
> > st_core_init(&kim_gdata->core_data);
> >
> > I would expect any truely global data to be configured
> in
> > the module init
> > and then device specific data you want to do something
> like
> > this
> >
> > ??? kim_data = kzalloc(sizeof(something),
> > GFP_KERNEL);
> >
> > ??? ..
> >
> > ??? kim_data_init(&pdev->dev,
> > kim_data);
> > ??? dev_set_drvdata(&pdev->dev,
> > kim_data);
> >
> > Elsewhere you can now do
> >
> > ??? kim_data =
> > dev_get_drvdata(&pdev->dev);
> >
> > to get it back
>
> There are 2 sets of data structures here (after removing
> the un-necessary 3rd one),
> 1. st_gdata - which I would want to tie to tty
> 2. kim_gdata - which I "would" like to tie to the pdev.
>
> Now the problem being, I have reference of st_gdata in
> kim_gdata, and there are about 4 functions in the st_core
> where I would need the st_gdata, as in,
>
> EXPORTED symbol st_register - because I need to add in
> entries as to who registered,
> +long st_register(struct st_proto_s *new_proto)
> +{
> +??? struct st_data_s??? *st_gdata;
> +??? long err = ST_SUCCESS;
> +??? unsigned long flags = 0;
> +
> +??? st_kim_ref(&st_gdata);
> +??? pr_info("%s(%d) ", __func__, new_proto->type);
> +??? if (st_gdata == NULL || new_proto == NULL ||
> new_proto->recv == NULL
> +??? ? ? || new_proto->reg_complete_cb == NULL) {
> +??? ??? pr_err("gdata/new_proto/recv or
> reg_complete_cb not ready");
> +??? ??? return ST_ERR_FAILURE;
> +??? }
> Also in st_unregister and st_write for similar purposes,
> But I also need it in tty_open, to link it to the
> disc_data,
>
> +static int st_tty_open(struct tty_struct *tty)
> +{
> +??? int err = ST_SUCCESS;
> +??? struct st_data_s *st_gdata;
> +??? pr_info("%s ", __func__);
> +
> +??? st_kim_ref(&st_gdata);
> +??? st_gdata->tty = tty;
> +??? tty->disc_data = st_gdata;
>
> So, shouldn't some function like st_kim_ref be enough ?
>
> +void st_kim_ref(struct st_data_s **core_data)
> +{
> +??? *core_data = kim_gdata->core_data;
> +}
>
> So Now st_gdata is tied to tty, and kim_gdata being purely
> global, as in only 1 platform device of such kind can
> exist.
>
> Suppose 2 platform devices want to exist - then who's
> EXPORT of st_register is considered ?
> And If bluetooth or FM wants to use this transport then,
> how would it tell onto which platform device it wants to
> attach to ?
>
> Why should I tie kim_gdata to a pdev ?
Well, because of the comments on the architecture of this driver, I tried out the bus_ driver method, where I register a new bus type as ST (which also registers the N_TI_WL line discipline) and the different protocols on it, are registered to as devices with the N_TI_WL as bus type.
However, I ended up in some similar mess there on too, where during an st_register when I need to register devices to the bus, status-es like how many devices currently registered, and allowing other devices to register during firmware download all would then on require a data which isn't bound to any device ...
So any ideas ? What should be done ? Find below the test patch...
---
drivers/staging/ti-st/bus_drv/bt_test_bus.c | 55 +++++++++++
drivers/staging/ti-st/bus_drv/fm_test_bus.c | 55 +++++++++++
drivers/staging/ti-st/bus_drv/test_bus.c | 134 +++++++++++++++++++++++++++
3 files changed, 244 insertions(+), 0 deletions(-)
create mode 100644 drivers/staging/ti-st/bus_drv/bt_test_bus.c
create mode 100644 drivers/staging/ti-st/bus_drv/fm_test_bus.c
create mode 100644 drivers/staging/ti-st/bus_drv/test_bus.c
diff --git a/drivers/staging/ti-st/bus_drv/bt_test_bus.c b/drivers/staging/ti-st/bus_drv/bt_test_bus.c
new file mode 100644
index 0000000..02ae812
--- /dev/null
+++ b/drivers/staging/ti-st/bus_drv/bt_test_bus.c
@@ -0,0 +1,55 @@
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+
+#include "st_bus.h"
+
+static int __init st_user_probe(struct device *dev, int cur_bus, int cur_slot)
+{
+ pr_info("%s\n", __func__);
+ return 0;
+}
+static int __exit st_user_remove(struct device *dev, int cur_bus, int cur_slot)
+{
+ pr_info("%s\n", __func__);
+ return 0;
+}
+
+void bt_release(struct device *dev)
+{
+ pr_info("%s\n", __func__);
+}
+
+static struct st_driver st_user_driver = {
+ .dev = {
+ .release = bt_release,
+ },
+ .id = ST_BT,
+ .name = "bluetooth",
+};
+
+static int __init st_user_init(void)
+{
+ int retval;
+ retval = st_register(&st_user_driver);
+ if (retval != 0)
+ pr_err("%s: error registering driver\n", __func__);
+
+ return retval;
+
+}
+
+static void __exit st_user_exit(void)
+{
+ pr_info("%s\n", __func__);
+ st_unregister(&st_user_driver);
+}
+
+
+
+MODULE_LICENSE("GPL");
+
+module_init(st_user_init);
+module_exit(st_user_exit);
+
diff --git a/drivers/staging/ti-st/bus_drv/fm_test_bus.c b/drivers/staging/ti-st/bus_drv/fm_test_bus.c
new file mode 100644
index 0000000..75d9980
--- /dev/null
+++ b/drivers/staging/ti-st/bus_drv/fm_test_bus.c
@@ -0,0 +1,55 @@
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+
+#include "st_bus.h"
+
+static int __init st_user_probe(struct device *dev, int cur_bus, int cur_slot)
+{
+ pr_info("%s\n", __func__);
+ return 0;
+}
+static int __exit st_user_remove(struct device *dev, int cur_bus, int cur_slot)
+{
+ pr_info("%s\n", __func__);
+ return 0;
+}
+
+void fm_release(struct device *dev)
+{
+ pr_info("%s\n", __func__);
+}
+
+static struct st_driver st_user_driver = {
+ .dev = {
+ .release = fm_release,
+ },
+ .id = ST_FM,
+ .name = "fm",
+};
+
+static int __init st_user_init(void)
+{
+ int retval;
+ retval = st_register(&st_user_driver);
+ if (retval != 0)
+ pr_err("%s: error registering driver\n", __func__);
+
+ return retval;
+
+}
+
+static void __exit st_user_exit(void)
+{
+ pr_info("%s\n", __func__);
+ st_unregister(&st_user_driver);
+}
+
+
+
+MODULE_LICENSE("GPL");
+
+module_init(st_user_init);
+module_exit(st_user_exit);
+
diff --git a/drivers/staging/ti-st/bus_drv/test_bus.c b/drivers/staging/ti-st/bus_drv/test_bus.c
new file mode 100644
index 0000000..87aaa94
--- /dev/null
+++ b/drivers/staging/ti-st/bus_drv/test_bus.c
@@ -0,0 +1,134 @@
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+#include "st_bus.h"
+#include "st_core.h"
+
+struct bus_type st_bus_type;
+
+static int check_if_registered(struct device *dev, void *data)
+{
+ struct st_driver *drv;
+
+ drv = dev_get_drvdata(dev);
+ pr_info("device is %s, type %d\n", drv->name, drv->id);
+ return 1;
+}
+
+int st_register(struct st_driver *drv)
+{
+ int retval;
+ struct device *dev = &drv->dev;
+
+ if (bus_find_device_by_name(&st_bus_type, NULL,
+ drv->name) != NULL) {
+ pr_err("already registered\n");
+ }
+
+ retval = bus_for_each_dev(&st_bus_type, NULL, NULL,
+ check_if_registered);
+ if (retval != 1) { /* as returned by check_if_registered */
+ /* download_firmware here */
+ pr_info("downloading firmware..\n");
+ /* HOWTO carry on with device_register ? */
+ }
This is 1 place, I would be totally lost because, I should return the flow back to the driver which called the st_register, and allow other driver to do an st_register and return them as pending - and when firmware download completes (takes around 10secs - worst case), I then have to notify all the drivers(devices) registered about the completion of firmware download.
+ dev->bus = &(st_bus_type);
+ dev_set_name(dev, drv->name);
+ dev_set_drvdata(dev, drv);
+
+ /* add device here */
+ retval = device_register(dev);
+ if (retval) {
+ pr_err("device register failed\n");
+ return -1;
+ }
+
+ pr_info("%s: %s\n", __func__, drv->name);
+ return 0;
+}
+EXPORT_SYMBOL(st_register);
+
+void st_unregister(struct st_driver *drv)
+{
+ pr_info("%s\n", __func__);
+ device_unregister(&drv->dev);
+}
+EXPORT_SYMBOL(st_unregister);
+
+
+static int st_bus_probe(struct platform_device *pdev)
+{
+ struct st_data_s *core_data;
+ pr_info("%s\n", __func__);
+ bus_register(&st_bus_type);
+
+ st_core_init(&core_data);
+ dev_set_drvdata(&pdev->dev, core_data);
+ return 0;
+}
+
+static int st_bus_remove(struct platform_device *pdev)
+{
+ struct st_data_s *core_data;
+ pr_info("%s\n", __func__);
+
+ core_data = dev_get_drvdata(&pdev->dev);
+ st_core_exit(core_data);
+ bus_unregister(&st_bus_type);
+ return 0;
+}
+
+#if 0
+static int st_bus_match(struct device *dev, struct device_driver *drv)
+{
+ return 1;
+}
+
+int st_notify(struct notifier_block *notify, unsigned long a, void *b)
+{
+ pr_info("%s \n", __func__);
+ return 0;
+}
+#endif
+
+struct bus_type st_bus_type = {
+ .name = "ti-st",
+};
+EXPORT_SYMBOL(st_bus_type);
+
+struct platform_driver st_bus_driver = {
+ .driver = {
+ .name = "kim",
+ .owner = THIS_MODULE,
+ },
+ .probe = st_bus_probe,
+ .remove = st_bus_remove,
+};
+
+#if 0
+struct notifier_block st_bus_notify = {
+ .notifier_call = st_notify,
+};
+#endif
+
+static int __init test_bus_init(void)
+{
+ platform_driver_register(&st_bus_driver);
+ return 0;
+}
+
+static void __exit test_bus_exit(void)
+{
+ pr_info("%s\n", __func__);
+ platform_driver_unregister(&st_bus_driver);
+}
+
+module_init(test_bus_init);
+module_exit(test_bus_exit);
+MODULE_AUTHOR("Pavan Savoy <[email protected]>");
+MODULE_DESCRIPTION("Shared Transport Driver for TI BT/FM/GPS combo chips ");
+MODULE_LICENSE("GPL");
--
1.5.4.3
> > --
> > To unsubscribe from this list: send the line
> "unsubscribe
> > linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at? http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at? http://www.tux.org/lkml/
> >
>
>
>
> ? ? ? The INTERNET now has a personality.
> YOURS! See your Yahoo! Homepage. http://in.yahoo.com/
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at? http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at? http://www.tux.org/lkml/
>
Your Mail works best with the New Yahoo Optimized IE8. Get it NOW! http://downloads.yahoo.com/in/internetexplorer/
Sorry but I can't make head or tail of this and the code flow you are
trying to achieve.
The usual way you do stuff is to put per device stuff in a per device
struct, driver wide stuff in a driver struct (or static variables) and
then run everything from the device end.
I'd expect a low level driver to do something like
probe()
create device entry
initialise device (eg download firmware)
register itself with anything higher level
--- On Fri, 2/4/10, Alan Cox <[email protected]> wrote:
> From: Alan Cox <[email protected]>
> Subject: Re: [PATCH] drivers:staging: sources for ST core
> To: "Pavan Savoy" <[email protected]>
> Cc: "Greg KH" <[email protected]>, "Marcel Holtmann" <[email protected]>, [email protected]
> Date: Friday, 2 April, 2010, 4:57 AM
> Sorry but I can't make head or tail
> of this and the code flow you are
> trying to achieve.
>
> The usual way you do stuff is to put per device stuff in a
> per device
> struct, driver wide stuff in a driver struct (or static
> variables) and
> then run everything from the device end.
>
> I'd expect a low level driver to do something like
>
>
> ??? probe()
> ??? ??? create device entry
> ??? ??? initialise device (eg
> download firmware)
> ??? ??? register itself with
> anything higher level
>
What I am trying to achieve is something like this,
HCI-core V4L2-radio Char-device=/dev/tigps for fops
^ ^ ^
| | |
| | |
BT FM GPS [these register themselves to ST]
\ | /
\ | /
\ | /
Shared Transport Ldisc driver
|
TTY Layer <-- UART driver has already registered.
So, when a BT device try and registers itself to ST (shared transport) driver, I need to toggle chip enable line and 'download_firmware', and in case another _register from FM or GPS happens at the same time, I need to signal it pending, and call a callback upon completion of firmware download.
Now which are to be identified as per-device or bus or driver ?
Because when an st_register is called, I need to do some tty_* operations from the ldisc driver itself - i.e I cannot embed tty into any of BT, FM or GPS per-device structures.
I also expose a st_write function, where in any of BT, FM and GPS driver upon being ready (fw download completed) sends across an SKB, which I queue up and write.
All of what I wanted to do, could not be done when I tried ST ldisc as a bus and each of BT, FM and GPS as per-device structs registering with type ST as bus.
So again, Now which are to be identified as per-device or bus or driver ?
The INTERNET now has a personality. YOURS! See your Yahoo! Homepage. http://in.yahoo.com/