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

[Pavan Savoy]

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@xxxxxxx> wrote:

> On Mon, Mar 22, 2010 at 04:19:12PM -0500, pavan_savoy@xxxxxx wrote:
> > From: Pavan Savoy <pavan_savoy@xxxxxx>
> >
> > 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@xxxxxx>
> > ---
> >  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/