Re: [PATCH] drivers:staging: sources for ST core

[Pavan Savoy <pavan_savoy@ti.com>]

--- 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 ?

> --
> 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/