Re: [PATCH 3/6] drivers:misc: sources for ST core

[Pavan Savoy <pavan_savoy@ti.com>]

comments inline...
 > +/* all debug macros go in here */
 > +#define ST_DRV_ERR(fmt, arg...)  printk(KERN_ERR "(stc):"fmt"\n" , 
## arg)
 > +#if defined(DEBUG)           /* limited debug messages */
 > +#define ST_DRV_DBG(fmt, arg...)  printk(KERN_INFO "(stc):"fmt"\n" , 
## arg)
 > +#define ST_DRV_VER(fmt, arg...)
 > +#elif defined(VERBOSE)               /* very verbose */
 > +#define ST_DRV_DBG(fmt, arg...)  printk(KERN_INFO "(stc):"fmt"\n" , 
## arg)
 > +#define ST_DRV_VER(fmt, arg...)  printk(KERN_INFO "(stc):"fmt"\n" , 
## arg)
 > +#else /* error msgs only */
 > +#define ST_DRV_DBG(fmt, arg...)
 > +#define ST_DRV_VER(fmt, arg...)
 > +#endif

 >Use the existing debug macros

[pavan]
Wanted a module level debugging code - and hence the usage of these
debug macros. Like printing out of all UART data, etc..
Apparently most UART problems surfaced during firmware download in 
st_kim.c so the module level debug macros.
All printk's also do have their own KERN_ level already.
Will change it if you want it.


 > +/* function pointer pointing to either,
 > + * st_kim_recv during registration to receive fw download responses
 > + * st_int_recv after registration to receive proto stack responses
 > + */
 > +void (*st_recv) (const unsigned char *data, long count);

[alan]
 >What if you have multiple devices at once one in each state ?
 >Why is this global ?

[pavan]
st_gdata required in non-tty calls as well.

[pavan]
Can't happen, The chip on doing a UART write will either write whole of 
BT data or whole of FM, so it can't be in multiple states.

 > +     if (unlikely(st_gdata == NULL || st_gdata->tty == NULL)) {

[alan]
 >Again shouldn't be using globals and needs to support multiple devices.
 >See the tty_struct - there is a field for an ldisc pointer, stuff
 >st_gdata in there at open time and pass tty around ?

[pavan]
st_gdata not in tty priv data or ldisc_data because there are entry 
points like st_register/unregister which are EXPORTed requires it.



 > +             ST_DRV_ERR("tty unavailable to perform write");
 > +             return ST_ERR_FAILURE;
 > +     }
 > +     tty = st_gdata->tty;

[alan]
 >Explain the locking on this NULL test - what stops it becoming NULL
 >between the if and the assignment ?

[pavan]
Calls to int_write in itself are safe, locked before being called.
Will recheck.
Up until now the same code has worked fine on UP and SMP.

[alan]
 >I think this code needs a fair bit of work at this point - locking,
 >supporting multiple devices at once etc.

 >Staging perhaps ?

[pavan]
If the rest seems fine, please consider it for staging,
Will keep reworking on it.


On Mon, 22 Mar 2010 14:35:30 -0700
Greg KH <gregkh@suse.de> wrote:

 > On Mon, Mar 22, 2010 at 04:19:12PM -0500, pavan_savoy@ti.com wrote:
 > > From: Pavan Savoy <pavan_savoy@ti.com>
 > >
 > > This change adds the Kconfig and Make file for TI's
 > > ST line discipline driver and the BlueZ driver for BT
 > > core of the TI BT/FM/GPS combo chip.
 > >
 > > Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
 > > ---
 > >  drivers/misc/Kconfig        |    1 +
 >
 > Why 'misc'?  Why not 'char' like all the other ldiscs?
 >
 > Or 'drivers/ldisc' to be more specific?

We've discussed having /tty or drivers/tty for a while. The ldiscs are
currently everywhere - drivers/net, isdn, char ....

I am not sure an ldisc directory helps though - slip and ppp are in
drivers/net for example and clearly belong there.
 >  #define N_V253               19      /* Codec control over voice 
modem */
 > +#define N_TI_SHARED  20      /* for TI's WL7 connectivity chips */

Be more specific or some future TI shared bus protocol might cause
confusion N_TI_WL7 sounds fine.

Alan