Re: [PATCH 2/5] net: rfkill: add rfkill_find_type function

2015-08-13 Thread Johannes Berg
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 netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/5] net: rfkill: add rfkill_find_type function

2015-08-13 Thread Heikki Krogerus
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,

-- 
heikki
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/5] net: rfkill: add rfkill_find_type function

2015-08-06 Thread Heikki Krogerus
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.kroge...@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,

-- 
heikki
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/5] net: rfkill: add rfkill_find_type function

2015-08-06 Thread Andy Shevchenko
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!

-- 
Andy Shevchenko andriy.shevche...@linux.intel.com
Intel Finland Oy
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/5] net: rfkill: add rfkill_find_type function

2015-08-06 Thread Sergei Shtylyov

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.kroge...@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 netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/5] net: rfkill: add rfkill_find_type function

2015-08-05 Thread Andy Shevchenko
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.kroge...@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);

-- 
Andy Shevchenko andriy.shevche...@linux.intel.com
Intel Finland Oy
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html