| Message ID | 1438781947-7952-3-git-send-email-heikki.krogerus@linux.intel.com |
|---|---|
| State | Deferred |
| Headers | show |
On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote: > Helper for finding the type based on name. Useful if the > type needs to be determined based on device property. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > include/linux/rfkill.h | 15 +++++++++++++ > net/rfkill/core.c | 57 +++++++++++++++++++++++++--------------- > ---------- > 2 files changed, 44 insertions(+), 28 deletions(-) > > diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h > index d901078..02f563c 100644 > --- a/include/linux/rfkill.h > +++ b/include/linux/rfkill.h > @@ -212,6 +212,15 @@ void rfkill_set_states(struct rfkill *rfkill, > bool sw, bool hw); > * @rfkill: rfkill struct to query > */ > bool rfkill_blocked(struct rfkill *rfkill); > + > +/** > + * rfkill_find_type - Helpper for finding rfkill type by name > + * @name: the name of the type > + * > + * Returns enum rfkill_type that conrresponds the name. > + */ > +enum rfkill_type rfkill_find_type(const char *name); > + > #else /* !RFKILL */ > static inline struct rfkill * __must_check > rfkill_alloc(const char *name, > @@ -268,6 +277,12 @@ static inline bool rfkill_blocked(struct rfkill > *rfkill) > { > return false; > } > + > +static inline enum rfkill_type rfkill_find_type(const char *name) > +{ > + return 0; Hmm… Besides 0 is implicitly casted to enum type the issue with enums that you rather have to supply existing enum entry. I would suggest to add RFKILL_TYPE_UNKNOWN if _ALL is reserved for some use cases. > +} > + > #endif /* RFKILL || RFKILL_MODULE */ > > > diff --git a/net/rfkill/core.c b/net/rfkill/core.c > index f12149a..53d7a97e 100644 > --- a/net/rfkill/core.c > +++ b/net/rfkill/core.c > @@ -574,6 +574,33 @@ void rfkill_set_states(struct rfkill *rfkill, > bool sw, bool hw) > } > EXPORT_SYMBOL(rfkill_set_states); > > +static const char *rfkill_types[NUM_RFKILL_TYPES] = { > + [RFKILL_TYPE_WLAN] = "wlan", > + [RFKILL_TYPE_BLUETOOTH] = "bluetooth", > + [RFKILL_TYPE_UWB] = "ultrawideband", > + [RFKILL_TYPE_WIMAX] = "wimax", > + [RFKILL_TYPE_WWAN] = "wwan", > + [RFKILL_TYPE_GPS] = "gps", > + [RFKILL_TYPE_FM] = "fm", > + [RFKILL_TYPE_NFC] = "nfc", > +}; > + > +enum rfkill_type rfkill_find_type(const char *name) > +{ > + int i; > + > + BUILD_BUG_ON(NUM_RFKILL_TYPES != RFKILL_TYPE_NFC + 1); > + > + if (!name) > + return RFKILL_TYPE_ALL; > + > + for (i = 1; i < NUM_RFKILL_TYPES; i++) > + if (!strcmp(name, rfkill_types[i])) > + return i; > + return RFKILL_TYPE_ALL; > +} > +EXPORT_SYMBOL(rfkill_find_type); > + > static ssize_t name_show(struct device *dev, struct device_attribute > *attr, > char *buf) > { > @@ -583,38 +610,12 @@ static ssize_t name_show(struct device *dev, > struct device_attribute *attr, > } > static DEVICE_ATTR_RO(name); > > -static const char *rfkill_get_type_str(enum rfkill_type type) > -{ > - BUILD_BUG_ON(NUM_RFKILL_TYPES != RFKILL_TYPE_NFC + 1); > - > - switch (type) { > - case RFKILL_TYPE_WLAN: > - return "wlan"; > - case RFKILL_TYPE_BLUETOOTH: > - return "bluetooth"; > - case RFKILL_TYPE_UWB: > - return "ultrawideband"; > - case RFKILL_TYPE_WIMAX: > - return "wimax"; > - case RFKILL_TYPE_WWAN: > - return "wwan"; > - case RFKILL_TYPE_GPS: > - return "gps"; > - case RFKILL_TYPE_FM: > - return "fm"; > - case RFKILL_TYPE_NFC: > - return "nfc"; > - default: > - BUG(); > - } > -} > - > static ssize_t type_show(struct device *dev, struct device_attribute > *attr, > char *buf) > { > struct rfkill *rfkill = to_rfkill(dev); > > - return sprintf(buf, "%s\n", rfkill_get_type_str(rfkill > ->type)); > + return sprintf(buf, "%s\n", rfkill_types[rfkill->type]); > } > static DEVICE_ATTR_RO(type); > > @@ -760,7 +761,7 @@ static int rfkill_dev_uevent(struct device *dev, > struct kobj_uevent_env *env) > if (error) > return error; > error = add_uevent_var(env, "RFKILL_TYPE=%s", > - rfkill_get_type_str(rfkill->type)); > + rfkill_types[rfkill->type]); > if (error) > return error; > spin_lock_irqsave(&rfkill->lock, flags);
On Wed, Aug 05, 2015 at 05:07:29PM +0300, Andy Shevchenko wrote: > On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote: > > Helper for finding the type based on name. Useful if the > > type needs to be determined based on device property. > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > --- > > include/linux/rfkill.h | 15 +++++++++++++ > > net/rfkill/core.c | 57 +++++++++++++++++++++++++--------------- > > ---------- > > 2 files changed, 44 insertions(+), 28 deletions(-) > > > > diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h > > index d901078..02f563c 100644 > > --- a/include/linux/rfkill.h > > +++ b/include/linux/rfkill.h > > @@ -212,6 +212,15 @@ void rfkill_set_states(struct rfkill *rfkill, > > bool sw, bool hw); > > * @rfkill: rfkill struct to query > > */ > > bool rfkill_blocked(struct rfkill *rfkill); > > + > > +/** > > + * rfkill_find_type - Helpper for finding rfkill type by name > > + * @name: the name of the type > > + * > > + * Returns enum rfkill_type that conrresponds the name. > > + */ > > +enum rfkill_type rfkill_find_type(const char *name); > > + > > #else /* !RFKILL */ > > static inline struct rfkill * __must_check > > rfkill_alloc(const char *name, > > @@ -268,6 +277,12 @@ static inline bool rfkill_blocked(struct rfkill > > *rfkill) > > { > > return false; > > } > > + > > +static inline enum rfkill_type rfkill_find_type(const char *name) > > +{ > > + return 0; > > Hmm… Besides 0 is implicitly casted to enum type the issue with enums > that you rather have to supply existing enum entry. I would suggest to > add RFKILL_TYPE_UNKNOWN if _ALL is reserved for some use cases. Why would you add a new type just for this? You do realize it would require adding specific handling all over the place? RFKILL_TYPE_ALL (0) is already handled as an invalid type. Confused? I'll change this and return RFKILL_TYPE_ALL instead of 0. Thanks,
On Thu, 2015-08-06 at 11:30 +0300, Heikki Krogerus wrote: > > > > On Wed, Aug 05, 2015 at 05:07:29PM +0300, Andy Shevchenko wrote: > > On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote: > > > +static inline enum rfkill_type rfkill_find_type(const char > > > *name) > > > +{ > > > + return 0; > > > > Hmm… Besides 0 is implicitly casted to enum type the issue with > > enums > > that you rather have to supply existing enum entry. I would suggest > > to > > add RFKILL_TYPE_UNKNOWN if _ALL is reserved for some use cases. > > Why would you add a new type just for this? You do realize it would > require adding specific handling all over the place? RFKILL_TYPE_ALL > (0) is already handled as an invalid type. It was my thought as well (see *if* in my previous comment). > Confused? A bit, yes. > > I'll change this and return RFKILL_TYPE_ALL instead of 0. Excellent!
Hello. On 8/5/2015 4:39 PM, Heikki Krogerus wrote: > Helper for finding the type based on name. Useful if the > type needs to be determined based on device property. > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > include/linux/rfkill.h | 15 +++++++++++++ > net/rfkill/core.c | 57 +++++++++++++++++++++++++------------------------- > 2 files changed, 44 insertions(+), 28 deletions(-) > diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h > index d901078..02f563c 100644 > --- a/include/linux/rfkill.h > +++ b/include/linux/rfkill.h [...] > @@ -268,6 +277,12 @@ static inline bool rfkill_blocked(struct rfkill *rfkill) > { > return false; > } > + > +static inline enum rfkill_type rfkill_find_type(const char *name) > +{ > + return 0; > +} > + Shouldn't it return some member of 'enum rfkill_type' instead? [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote: > > +static const char *rfkill_types[NUM_RFKILL_TYPES] = { > + [RFKILL_TYPE_WLAN] = "wlan", > + [RFKILL_TYPE_BLUETOOTH] = "bluetooth", > + [RFKILL_TYPE_UWB] = "ultrawideband", > + [RFKILL_TYPE_WIMAX] = "wimax", > + [RFKILL_TYPE_WWAN] = "wwan", > + [RFKILL_TYPE_GPS] = "gps", > + [RFKILL_TYPE_FM] = "fm", > + [RFKILL_TYPE_NFC] = "nfc", > +}; > + > +enum rfkill_type rfkill_find_type(const char *name) > +{ > + int i; > + > + BUILD_BUG_ON(NUM_RFKILL_TYPES != RFKILL_TYPE_NFC + 1); > That BUILD_BUG_ON() is now less useful - previously it pointed to the code that needed to change, now you're left wondering if you don't look up since it isn't quite that obvious from the code what this does. Something like BUILD_BUG_ON(rfkill_types[NUM_RFKILL_TYPES - 1] == NULL); would be better. As we only add here, that would be safe enough - I've done something similar in the past that a bit more complicated. With that and the static inline fixed (which maybe you could even remove) I'm fine with all these rfkill patches, but I'm not sure how to merge them since they affect all kinds of other trees. If desired, I can apply them, but an ACK from the tegra maintainer would be good :) johannes -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Aug 13, 2015 at 11:27:46AM +0200, Johannes Berg wrote: > On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote: > > > > +static const char *rfkill_types[NUM_RFKILL_TYPES] = { > > + [RFKILL_TYPE_WLAN] = "wlan", > > + [RFKILL_TYPE_BLUETOOTH] = "bluetooth", > > + [RFKILL_TYPE_UWB] = "ultrawideband", > > + [RFKILL_TYPE_WIMAX] = "wimax", > > + [RFKILL_TYPE_WWAN] = "wwan", > > + [RFKILL_TYPE_GPS] = "gps", > > + [RFKILL_TYPE_FM] = "fm", > > + [RFKILL_TYPE_NFC] = "nfc", > > +}; > > + > > +enum rfkill_type rfkill_find_type(const char *name) > > +{ > > + int i; > > + > > + BUILD_BUG_ON(NUM_RFKILL_TYPES != RFKILL_TYPE_NFC + 1); > > > That BUILD_BUG_ON() is now less useful - previously it pointed to the > code that needed to change, now you're left wondering if you don't look > up since it isn't quite that obvious from the code what this does. > > Something like > > BUILD_BUG_ON(rfkill_types[NUM_RFKILL_TYPES - 1] == NULL); > > would be better. As we only add here, that would be safe enough - I've > done something similar in the past that a bit more complicated. OK, I'll change it. > With that and the static inline fixed (which maybe you could even > remove) I'm fine with all these rfkill patches, but I'm not sure how to > merge them since they affect all kinds of other trees. If desired, I > can apply them, but an ACK from the tegra maintainer would be good :) Andy and Mika are preparing some changes to the device property handling. I'll wait for their proposal and prepare next version these after that. Thanks,
diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h index d901078..02f563c 100644 --- a/include/linux/rfkill.h +++ b/include/linux/rfkill.h @@ -212,6 +212,15 @@ void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw); * @rfkill: rfkill struct to query */ bool rfkill_blocked(struct rfkill *rfkill); + +/** + * rfkill_find_type - Helpper for finding rfkill type by name + * @name: the name of the type + * + * Returns enum rfkill_type that conrresponds the name. + */ +enum rfkill_type rfkill_find_type(const char *name); + #else /* !RFKILL */ static inline struct rfkill * __must_check rfkill_alloc(const char *name, @@ -268,6 +277,12 @@ static inline bool rfkill_blocked(struct rfkill *rfkill) { return false; } + +static inline enum rfkill_type rfkill_find_type(const char *name) +{ + return 0; +} + #endif /* RFKILL || RFKILL_MODULE */ diff --git a/net/rfkill/core.c b/net/rfkill/core.c index f12149a..53d7a97e 100644 --- a/net/rfkill/core.c +++ b/net/rfkill/core.c @@ -574,6 +574,33 @@ void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw) } EXPORT_SYMBOL(rfkill_set_states); +static const char *rfkill_types[NUM_RFKILL_TYPES] = { + [RFKILL_TYPE_WLAN] = "wlan", + [RFKILL_TYPE_BLUETOOTH] = "bluetooth", + [RFKILL_TYPE_UWB] = "ultrawideband", + [RFKILL_TYPE_WIMAX] = "wimax", + [RFKILL_TYPE_WWAN] = "wwan", + [RFKILL_TYPE_GPS] = "gps", + [RFKILL_TYPE_FM] = "fm", + [RFKILL_TYPE_NFC] = "nfc", +}; + +enum rfkill_type rfkill_find_type(const char *name) +{ + int i; + + BUILD_BUG_ON(NUM_RFKILL_TYPES != RFKILL_TYPE_NFC + 1); + + if (!name) + return RFKILL_TYPE_ALL; + + for (i = 1; i < NUM_RFKILL_TYPES; i++) + if (!strcmp(name, rfkill_types[i])) + return i; + return RFKILL_TYPE_ALL; +} +EXPORT_SYMBOL(rfkill_find_type); + static ssize_t name_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -583,38 +610,12 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr, } static DEVICE_ATTR_RO(name); -static const char *rfkill_get_type_str(enum rfkill_type type) -{ - BUILD_BUG_ON(NUM_RFKILL_TYPES != RFKILL_TYPE_NFC + 1); - - switch (type) { - case RFKILL_TYPE_WLAN: - return "wlan"; - case RFKILL_TYPE_BLUETOOTH: - return "bluetooth"; - case RFKILL_TYPE_UWB: - return "ultrawideband"; - case RFKILL_TYPE_WIMAX: - return "wimax"; - case RFKILL_TYPE_WWAN: - return "wwan"; - case RFKILL_TYPE_GPS: - return "gps"; - case RFKILL_TYPE_FM: - return "fm"; - case RFKILL_TYPE_NFC: - return "nfc"; - default: - BUG(); - } -} - static ssize_t type_show(struct device *dev, struct device_attribute *attr, char *buf) { struct rfkill *rfkill = to_rfkill(dev); - return sprintf(buf, "%s\n", rfkill_get_type_str(rfkill->type)); + return sprintf(buf, "%s\n", rfkill_types[rfkill->type]); } static DEVICE_ATTR_RO(type); @@ -760,7 +761,7 @@ static int rfkill_dev_uevent(struct device *dev, struct kobj_uevent_env *env) if (error) return error; error = add_uevent_var(env, "RFKILL_TYPE=%s", - rfkill_get_type_str(rfkill->type)); + rfkill_types[rfkill->type]); if (error) return error; spin_lock_irqsave(&rfkill->lock, flags);
Helper for finding the type based on name. Useful if the type needs to be determined based on device property. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- include/linux/rfkill.h | 15 +++++++++++++ net/rfkill/core.c | 57 +++++++++++++++++++++++++------------------------- 2 files changed, 44 insertions(+), 28 deletions(-)