Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1372864541-10763-1-git-send-email-luiz.dentz@gmail.com> <1372864541-10763-2-git-send-email-luiz.dentz@gmail.com> Date: Thu, 4 Jul 2013 09:49:51 +0300 Message-ID: Subject: Re: [RFC 01/20] audio/sink: Use service user_data for private data From: Luiz Augusto von Dentz To: Mikel Astiz Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Mikel, On Thu, Jul 4, 2013 at 9:36 AM, Mikel Astiz wrote: > Hi Luiz, > > On Wed, Jul 3, 2013 at 5:15 PM, Luiz Augusto von Dentz > wrote: >> From: Luiz Augusto von Dentz >> >> This remove the need of forward declaration of struct sink and prepare >> for a complete removal of struct audio_device. >> --- >> profiles/audio/avdtp.c | 2 +- >> profiles/audio/device.c | 54 ++++++++++++++++++++++++++++++------------------- >> profiles/audio/device.h | 3 +-- >> profiles/audio/sink.c | 29 +++++++++++++++----------- >> profiles/audio/sink.h | 5 +++-- >> 5 files changed, 55 insertions(+), 38 deletions(-) >> >> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c >> index bd73572..0d67e67 100644 >> --- a/profiles/audio/avdtp.c >> +++ b/profiles/audio/avdtp.c >> @@ -1165,7 +1165,7 @@ static gboolean disconnect_timeout(gpointer user_data) >> dev = manager_get_audio_device(session->device, FALSE); >> >> if (dev && dev->sink && stream_setup) >> - sink_setup_stream(dev->sink, session); >> + sink_setup_stream(dev, session); >> else if (dev && dev->source && stream_setup) >> source_setup_stream(dev->source, session); >> else >> diff --git a/profiles/audio/device.c b/profiles/audio/device.c >> index 3fa49ad..f11c728 100644 >> --- a/profiles/audio/device.c >> +++ b/profiles/audio/device.c >> @@ -42,10 +42,12 @@ >> #include >> #include >> >> -#include "log.h" >> -#include "../src/adapter.h" >> -#include "../src/device.h" >> +#include "lib/uuid.h" >> +#include "src/adapter.h" >> +#include "src/device.h" >> +#include "src/service.h" >> >> +#include "log.h" >> #include "error.h" >> #include "dbus-common.h" >> #include "device.h" >> @@ -61,7 +63,7 @@ >> #define AVDTP_CONNECT_TIMEOUT_BOOST 1 >> >> struct dev_priv { >> - sink_state_t sink_state; >> + btd_service_state_t sink_state; >> avctp_state_t avctp_state; >> >> guint control_timer; >> @@ -69,9 +71,9 @@ struct dev_priv { >> >> gboolean disconnecting; >> >> + unsigned int service_cb_id; >> unsigned int avdtp_callback_id; >> unsigned int avctp_callback_id; >> - unsigned int sink_callback_id; >> }; >> >> static void device_free(struct audio_device *dev) >> @@ -87,7 +89,7 @@ static void device_free(struct audio_device *dev) >> >> avdtp_remove_state_cb(priv->avdtp_callback_id); >> avctp_remove_state_cb(priv->avctp_callback_id); >> - sink_remove_state_cb(priv->sink_callback_id); >> + btd_service_remove_state_cb(priv->service_cb_id); >> >> g_free(priv); >> } >> @@ -138,6 +140,7 @@ static void disconnect_cb(struct btd_device *btd_dev, gboolean removal, >> { >> struct audio_device *dev = user_data; >> struct dev_priv *priv = dev->priv; >> + struct btd_service *sink; >> >> if (priv->disconnecting) >> return; >> @@ -149,7 +152,8 @@ static void disconnect_cb(struct btd_device *btd_dev, gboolean removal, >> if (dev->control && priv->avctp_state != AVCTP_STATE_DISCONNECTED) >> avrcp_disconnect(dev); >> >> - if (dev->sink && priv->sink_state != SINK_STATE_DISCONNECTED) >> + sink = btd_device_get_service(btd_dev, A2DP_SINK_UUID); >> + if (sink) >> sink_disconnect(dev, TRUE); >> else >> priv->disconnecting = FALSE; >> @@ -171,30 +175,25 @@ static void device_avdtp_cb(struct audio_device *dev, struct avdtp *session, >> } >> >> static void device_sink_cb(struct audio_device *dev, >> - sink_state_t old_state, >> - sink_state_t new_state, >> - void *user_data) >> + btd_service_state_t old_state, >> + btd_service_state_t new_state) >> { >> struct dev_priv *priv = dev->priv; >> >> - if (!dev->sink) >> - return; >> - >> priv->sink_state = new_state; >> >> switch (new_state) { >> - case SINK_STATE_DISCONNECTED: >> + case BTD_SERVICE_STATE_UNAVAILABLE: >> + case BTD_SERVICE_STATE_DISCONNECTED: >> if (dev->control) { >> device_remove_control_timer(dev); >> if (priv->avctp_state != AVCTP_STATE_DISCONNECTED) >> avrcp_disconnect(dev); >> } >> break; >> - case SINK_STATE_CONNECTING: >> - break; >> - case SINK_STATE_CONNECTED: >> - break; >> - case SINK_STATE_PLAYING: >> + case BTD_SERVICE_STATE_CONNECTING: >> + case BTD_SERVICE_STATE_CONNECTED: >> + case BTD_SERVICE_STATE_DISCONNECTING: >> break; >> } >> } >> @@ -222,6 +221,20 @@ static void device_avctp_cb(struct audio_device *dev, avctp_state_t old_state, >> } >> } >> >> +static void service_cb(struct btd_service *service, >> + btd_service_state_t old_state, >> + btd_service_state_t new_state, >> + void *user_data) >> +{ >> + struct audio_device *dev = user_data; >> + >> + if (dev->btd_dev != btd_service_get_device(service)) >> + return; >> + >> + if (service == dev->sink) >> + device_sink_cb(dev, old_state, new_state); >> +} >> + >> struct audio_device *audio_device_register(struct btd_device *device) >> { >> struct audio_device *dev; >> @@ -236,8 +249,7 @@ struct audio_device *audio_device_register(struct btd_device *device) >> dev->priv->dc_id = device_add_disconnect_watch(dev->btd_dev, >> disconnect_cb, dev, >> NULL); >> - dev->priv->sink_callback_id = sink_add_state_cb(dev, device_sink_cb, >> - NULL); >> + dev->priv->service_cb_id = btd_service_add_state_cb(service_cb, dev); >> dev->priv->avdtp_callback_id = avdtp_add_state_cb(dev, device_avdtp_cb); >> dev->priv->avctp_callback_id = avctp_add_state_cb(dev, device_avctp_cb); >> >> diff --git a/profiles/audio/device.h b/profiles/audio/device.h >> index 286bcdd..e24bdf9 100644 >> --- a/profiles/audio/device.h >> +++ b/profiles/audio/device.h >> @@ -25,13 +25,12 @@ >> struct audio_device; >> struct source; >> struct control; >> -struct sink; >> struct dev_priv; >> >> struct audio_device { >> struct btd_device *btd_dev; >> >> - struct sink *sink; >> + struct btd_service *sink; >> struct source *source; >> struct control *control; >> >> diff --git a/profiles/audio/sink.c b/profiles/audio/sink.c >> index 3969417..f5b4e67 100644 >> --- a/profiles/audio/sink.c >> +++ b/profiles/audio/sink.c >> @@ -87,7 +87,7 @@ static char *str_state[] = { >> >> static void sink_set_state(struct audio_device *dev, sink_state_t new_state) >> { >> - struct sink *sink = dev->sink; >> + struct sink *sink = btd_service_get_user_data(dev->sink); >> sink_state_t old_state = sink->state; >> GSList *l; >> >> @@ -119,7 +119,7 @@ static void avdtp_state_callback(struct audio_device *dev, >> avdtp_session_state_t old_state, >> avdtp_session_state_t new_state) >> { >> - struct sink *sink = dev->sink; >> + struct sink *sink = btd_service_get_user_data(dev->sink); >> >> switch (new_state) { >> case AVDTP_SESSION_STATE_DISCONNECTED: >> @@ -142,7 +142,7 @@ static void stream_state_changed(struct avdtp_stream *stream, >> void *user_data) >> { >> struct audio_device *dev = user_data; >> - struct sink *sink = dev->sink; >> + struct sink *sink = btd_service_get_user_data(dev->sink); >> >> if (err) >> return; >> @@ -291,8 +291,10 @@ failed: >> sink->session = NULL; >> } >> >> -gboolean sink_setup_stream(struct sink *sink, struct avdtp *session) >> +gboolean sink_setup_stream(struct audio_device *dev, struct avdtp *session) >> { >> + struct sink *sink = btd_service_get_user_data(dev->sink); >> + >> if (sink->connect_id > 0 || sink->disconnect_id > 0) >> return FALSE; >> >> @@ -310,7 +312,7 @@ gboolean sink_setup_stream(struct sink *sink, struct avdtp *session) >> >> int sink_connect(struct audio_device *dev) >> { >> - struct sink *sink = dev->sink; >> + struct sink *sink = btd_service_get_user_data(dev->sink); >> >> if (!sink->session) >> sink->session = avdtp_get(dev); >> @@ -326,7 +328,7 @@ int sink_connect(struct audio_device *dev) >> if (sink->stream_state >= AVDTP_STATE_OPEN) >> return -EALREADY; >> >> - if (!sink_setup_stream(sink, NULL)) { >> + if (!sink_setup_stream(dev, NULL)) { >> DBG("Failed to create a stream"); >> return -EIO; >> } >> @@ -338,7 +340,7 @@ int sink_connect(struct audio_device *dev) >> >> static void sink_free(struct audio_device *dev) >> { >> - struct sink *sink = dev->sink; >> + struct sink *sink = btd_service_get_user_data(dev->sink); >> >> if (sink->cb_id) >> avdtp_stream_remove_cb(sink->session, sink->stream, >> @@ -375,7 +377,8 @@ void sink_unregister(struct audio_device *dev) >> sink_free(dev); >> } >> >> -struct sink *sink_init(struct audio_device *dev, struct btd_service *service) >> +struct btd_service *sink_init(struct audio_device *dev, >> + struct btd_service *service) > > Why not make the return type bool or even void? > > Another interesting alternative would be to return 'int', such that > the function eventually matches the btd_profile probe callback, and > therefore the manager can be bypassed. They idea here is that it would work as a ref so the return is stored in audio_device but you are right we can probably return int to match probe, gonna change this. > >> { >> struct sink *sink; >> >> @@ -388,12 +391,14 @@ struct sink *sink_init(struct audio_device *dev, struct btd_service *service) >> >> sink->avdtp_callback_id = avdtp_add_state_cb(dev, avdtp_state_callback); >> >> - return sink; >> + btd_service_set_user_data(service, sink); >> + >> + return service; >> } >> >> gboolean sink_is_active(struct audio_device *dev) >> { >> - struct sink *sink = dev->sink; >> + struct sink *sink = btd_service_get_user_data(dev->sink); >> >> if (sink->session) >> return TRUE; >> @@ -404,7 +409,7 @@ gboolean sink_is_active(struct audio_device *dev) >> gboolean sink_new_stream(struct audio_device *dev, struct avdtp *session, >> struct avdtp_stream *stream) >> { >> - struct sink *sink = dev->sink; >> + struct sink *sink = btd_service_get_user_data(dev->sink); >> >> if (sink->stream) >> return FALSE; >> @@ -422,7 +427,7 @@ gboolean sink_new_stream(struct audio_device *dev, struct avdtp *session, >> >> int sink_disconnect(struct audio_device *dev, gboolean shutdown) >> { >> - struct sink *sink = dev->sink; >> + struct sink *sink = btd_service_get_user_data(dev->sink); >> >> if (!sink->session) >> return -ENOTCONN; >> diff --git a/profiles/audio/sink.h b/profiles/audio/sink.h >> index 1a80756..e3f974b 100644 >> --- a/profiles/audio/sink.h >> +++ b/profiles/audio/sink.h >> @@ -40,11 +40,12 @@ unsigned int sink_add_state_cb(struct audio_device *dev, sink_state_cb cb, >> void *user_data); >> gboolean sink_remove_state_cb(unsigned int id); >> >> -struct sink *sink_init(struct audio_device *dev, struct btd_service *service); >> +struct btd_service *sink_init(struct audio_device *dev, >> + struct btd_service *service); >> void sink_unregister(struct audio_device *dev); >> gboolean sink_is_active(struct audio_device *dev); >> int sink_connect(struct audio_device *dev); >> gboolean sink_new_stream(struct audio_device *dev, struct avdtp *session, >> struct avdtp_stream *stream); >> -gboolean sink_setup_stream(struct sink *sink, struct avdtp *session); >> +gboolean sink_setup_stream(struct audio_device *dev, struct avdtp *session); > > Is there any reason not to use btd_service here directly, as later > changed in patch 4? The idea is to pack as little changes as possible, but I will take a look perhaps such a change can be done sooner rather than latter. -- Luiz Augusto von Dentz