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

[Pavan Savoy]

Greg,

On Thu, Aug 19, 2010 at 11:07 PM, Randy Dunlap <randy.dunlap@xxxxxxxxxx> wrote:
> On 08/19/10 10:35, Savoy, Pavan wrote:
>> 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...
>
> OK, thanks.
>
> Acked-by: Randy Dunlap <randy.dunlap@xxxxxxxxxx>


Can you please merge this patch ? Also please have a look at the driver and
suggest what needs to be done to move it out of staging.


>>
>>>> ---
>>>> Â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 ***
>
>
> --
> ~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/
>
>
>
--
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/