RE: [PATCH] drivers:staging:ti-st: remove st_get_plat_device

[Savoy, Pavan]

Randy,
 

> -----Original Message-----
> From: Randy Dunlap [mailto:randy.dunlap@xxxxxxxxxx]
> Sent: Thursday, August 19, 2010 12:32 PM
> To: Savoy, Pavan
> Cc: gregkh@xxxxxxx; alan@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxx; Jain, Naveen
> Subject: Re: [PATCH] drivers:staging:ti-st: remove st_get_plat_device
> 
> On 08/19/10 11:08, pavan_savoy@xxxxxx wrote:
> > From: Pavan Savoy <pavan_savoy@xxxxxx>
> >
> > In order to support multiple ST platform devices, a new symbol
> > 'st_get_plat_device' earlier needed to be exported by the arch/XX/brd-XX.c
> > file which intends to add the ST platform device.
> >
> > On removing this dependency, now inside ST driver maintain the array of
> > ST platform devices that would be registered.
> > As of now let id=0, as and when we end up having such platforms
> > where mutliple ST devices can exist, id would come from
> > protocol drivers (BT, FM and GPS) as to on which platform device
> > they want to register to.
> >
> > Signed-off-by: Pavan Savoy <pavan_savoy@xxxxxx>
> 
> Thanks, that builds cleanly.  I'm OK with it if you are comfortable with a
> hard limit on the fixed number of devices that can be supported:

Yep, Thanks for pointing out, sort of cleaned up the code.

> +#define MAX_ST_DEVICES	3	/* Imagine 1 on each UART for now */
> +struct platform_device *st_kim_devices[MAX_ST_DEVICES];
> 
> We usually try not to have such limits nor use arrays like that,
> but if the nature of the device and its platform/environment is like
> that, so be it.
> 

Actually on all platforms that I have seen there's only 1 such device.
The device is basically a connectivity chip (with Bluetooth, FM and GPS working
on a single UART)

The number which I mentioned was out of imagination.
I don't think we are ever going to have multiple of them either...

> > ---
> >  drivers/staging/ti-st/st.h      |    1 -
> >  drivers/staging/ti-st/st_core.c |    9 ++++-----
> >  drivers/staging/ti-st/st_core.h |    2 +-
> >  drivers/staging/ti-st/st_kim.c  |   22 +++++++++++++++++++---
> >  4 files changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/staging/ti-st/st.h b/drivers/staging/ti-st/st.h
> > index 9952579..1b3060e 100644
> > --- a/drivers/staging/ti-st/st.h
> > +++ b/drivers/staging/ti-st/st.h
> > @@ -80,5 +80,4 @@ struct st_proto_s {
> >  extern long st_register(struct st_proto_s *);
> >  extern long st_unregister(enum proto_type);
> >
> > -extern struct platform_device *st_get_plat_device(void);
> >  #endif /* ST_H */
> > diff --git a/drivers/staging/ti-st/st_core.c b/drivers/staging/ti-
> st/st_core.c
> > index 063c9b1..b85d8bf 100644
> > --- a/drivers/staging/ti-st/st_core.c
> > +++ b/drivers/staging/ti-st/st_core.c
> > @@ -38,7 +38,6 @@
> >  #include "st_ll.h"
> >  #include "st.h"
> >
> > -#define VERBOSE
> >  /* strings to be used for rfkill entries and by
> >   * ST Core to be used for sysfs debug entry
> >   */
> > @@ -581,7 +580,7 @@ long st_register(struct st_proto_s *new_proto)
> >  	long err = 0;
> >  	unsigned long flags = 0;
> >
> > -	st_kim_ref(&st_gdata);
> > +	st_kim_ref(&st_gdata, 0);
> >  	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) {
> > @@ -713,7 +712,7 @@ long st_unregister(enum proto_type type)
> >
> >  	pr_debug("%s: %d ", __func__, type);
> >
> > -	st_kim_ref(&st_gdata);
> > +	st_kim_ref(&st_gdata, 0);
> >  	if (type < ST_BT || type >= ST_MAX) {
> >  		pr_err(" protocol %d not supported", type);
> >  		return -EPROTONOSUPPORT;
> > @@ -767,7 +766,7 @@ long st_write(struct sk_buff *skb)
> >  #endif
> >  	long len;
> >
> > -	st_kim_ref(&st_gdata);
> > +	st_kim_ref(&st_gdata, 0);
> >  	if (unlikely(skb == NULL || st_gdata == NULL
> >  		|| st_gdata->tty == NULL)) {
> >  		pr_err("data/tty unavailable to perform write");
> > @@ -818,7 +817,7 @@ static int st_tty_open(struct tty_struct *tty)
> >  	struct st_data_s *st_gdata;
> >  	pr_info("%s ", __func__);
> >
> > -	st_kim_ref(&st_gdata);
> > +	st_kim_ref(&st_gdata, 0);
> >  	st_gdata->tty = tty;
> >  	tty->disc_data = st_gdata;
> >
> > diff --git a/drivers/staging/ti-st/st_core.h b/drivers/staging/ti-
> st/st_core.h
> > index e0c32d1..8601320 100644
> > --- a/drivers/staging/ti-st/st_core.h
> > +++ b/drivers/staging/ti-st/st_core.h
> > @@ -117,7 +117,7 @@ int st_core_init(struct st_data_s **);
> >  void st_core_exit(struct st_data_s *);
> >
> >  /* ask for reference from KIM */
> > -void st_kim_ref(struct st_data_s **);
> > +void st_kim_ref(struct st_data_s **, int);
> >
> >  #define GPS_STUB_TEST
> >  #ifdef GPS_STUB_TEST
> > diff --git a/drivers/staging/ti-st/st_kim.c b/drivers/staging/ti-st/st_kim.c
> > index b4a6c7f..9e99463 100644
> > --- a/drivers/staging/ti-st/st_kim.c
> > +++ b/drivers/staging/ti-st/st_kim.c
> > @@ -72,11 +72,26 @@ const unsigned char *protocol_names[] = {
> >  	PROTO_ENTRY(ST_GPS, "GPS"),
> >  };
> >
> > +#define MAX_ST_DEVICES	3	/* Imagine 1 on each UART for now */
> > +struct platform_device *st_kim_devices[MAX_ST_DEVICES];
> >
> >  /**********************************************************************/
> >  /* internal functions */
> >
> >  /**
> > + * st_get_plat_device -
> > + *	function which returns the reference to the platform device
> > + *	requested by id. As of now only 1 such device exists (id=0)
> > + *	the context requesting for reference can get the id to be
> > + *	requested by a. The protocol driver which is registering or
> > + *	b. the tty device which is opened.
> > + */
> > +static struct platform_device *st_get_plat_device(int id)
> > +{
> > +	return st_kim_devices[id];
> > +}
> > +
> > +/**
> >   * validate_firmware_response -
> >   *	function to return whether the firmware response was proper
> >   *	in case of error don't complete so that waiting for proper
> > @@ -353,7 +368,7 @@ void st_kim_chip_toggle(enum proto_type type, enum
> kim_gpio_state state)
> >  	struct kim_data_s	*kim_gdata;
> >  	pr_info(" %s ", __func__);
> >
> > -	kim_pdev = st_get_plat_device();
> > +	kim_pdev = st_get_plat_device(0);
> >  	kim_gdata = dev_get_drvdata(&kim_pdev->dev);
> >
> >  	if (kim_gdata->gpios[type] == -1) {
> > @@ -574,12 +589,12 @@ static int kim_toggle_radio(void *data, bool blocked)
> >   *	This would enable multiple such platform devices to exist
> >   *	on a given platform
> >   */
> > -void st_kim_ref(struct st_data_s **core_data)
> > +void st_kim_ref(struct st_data_s **core_data, int id)
> >  {
> >  	struct platform_device	*pdev;
> >  	struct kim_data_s	*kim_gdata;
> >  	/* get kim_gdata reference from platform device */
> > -	pdev = st_get_plat_device();
> > +	pdev = st_get_plat_device(id);
> >  	kim_gdata = dev_get_drvdata(&pdev->dev);
> >  	*core_data = kim_gdata->core_data;
> >  }
> > @@ -623,6 +638,7 @@ static int kim_probe(struct platform_device *pdev)
> >  	long *gpios = pdev->dev.platform_data;
> >  	struct kim_data_s	*kim_gdata;
> >
> > +	st_kim_devices[pdev->id] = pdev;
> >  	kim_gdata = kzalloc(sizeof(struct kim_data_s), GFP_ATOMIC);
> >  	if (!kim_gdata) {
> >  		pr_err("no mem to allocate");
> 
> 
> --
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
--
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/