lkml.org 
[lkml]   [2010]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] drivers:staging: sources for ST core


--- On Thu, 1/4/10, Pavan Savoy <pavan_savoy@ti.com> wrote:

> From: Pavan Savoy <pavan_savoy@ti.com>
> Subject: Re: [PATCH] drivers:staging: sources for ST core
> To: "Alan Cox" <alan@lxorguk.ukuu.org.uk>
> Cc: "Greg KH" <gregkh@suse.de>, "Marcel Holtmann" <marcel@holtmann.org>, linux-kernel@vger.kernel.org
> Date: Thursday, 1 April, 2010, 10:50 PM
> --- On Thu, 1/4/10, Alan Cox <alan@lxorguk.ukuu.org.uk>
> wrote:
>
> > From: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > Subject: Re: [PATCH] drivers:staging: sources for ST
> core
> > To: pavan_savoy@ti.com
> > Cc: "Greg KH" <gregkh@suse.de>,
> "Marcel Holtmann" <marcel@holtmann.org>,
> linux-kernel@vger.kernel.org
> > 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 <pavan_savoy@ti.com>");
+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 majordomo@vger.kernel.org
> > 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 majordomo@vger.kernel.org
> 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2010-04-02 00:45    [W:0.057 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site