Re: Race related to e04a0442d33b "HID: core: remove the absolute need of hid_have_special_driver[]"

[Heiner Kallweit]

Am 14.05.2018 um 11:58 schrieb Benjamin Tissoires:
> Hi Heiner,
> 
> On Fri, May 11, 2018 at 12:11 AM, Heiner Kallweit <hkallwe...@gmail.com> 
> wrote:
>> Due to some other issue with one devices supported by hid-led I figured
>> out that it's no longer needed to list devices with own driver in
>> hid_have_special_driver[].
>>
>> So I removed the entries for the hid-led devices and got the following.
>> When I plugged in the device first the device-specific driver wasn't
>> loaded. After unplugging/re-plugging the device-specific driver was
>> loaded properly.
>>
>> It seems usbhid module wasn't fully loaded and active yet when
>> hid-generic checks for a device-specific driver for the first time.
>> This also explains why the second attempt was successful.
> 
> I don't think so. When you plugged in the device, the usb core stack
> emitted a uevent requesting usbhid to be loaded. Then udev loaded
> usbhid, which created the USB-HID transport layer device immediately
> and started probing devices attached to it. The kernel emitted then a
> uevent regarding 0003:0FC5:B080, and udev answered by loading
> hid-generic. hid-generic made its initialization of the HID device,
> and then only now usbhid finished its initialization where it emits
> "USB HID core driver".
> 
> On the second attempt, the uevent is still emitted by the kernel, but
> this time udev loaded hid-led, which is detected by the hid-sstack as
> a new candidate and it takes precedence over hid-generic.
> 
> Keep in mind that the kernel doesn't load external modules, but udev
> does. So here, it seems we expected udev to load hid-led the first
> time, but it didn't.
> 
>>
>> Did you come across similar races? At a first glance I'm not sure how
>> to prevent this race. Any idea?
> 
> This is related to udev and the way the uevents are emitted.
> Maybe if we see the logs from "udevadm monitor" I'll be able to tell a
> little bit more. Meanwhile, I'll try reproducing it locally.
> 
> Also, the v4.17-rc series has seen a little cleanup in the way
> hid-generic is unbound, so it is worth a try.
> 
I used the latest linux-next kernel, so I assume these changes should
be included. When trying to create the "udevadm monitor" log I found
that I can't reproduce the issue. Hmm, strange ..
I will keep an eye on it. Until then: sorry for the noise.

Heiner

> Cheers,
> Benjamin
> 
>>
>> [   15.785205] usb 2-5: new low-speed USB device number 4 using xhci_hcd
>> [   15.917766] usb 2-5: New USB device found, idVendor=0fc5, idProduct=b080, 
>> bcdDevice= 0.1f
>> [   15.917842] usb 2-5: New USB device strings: Mfr=1, Product=2, 
>> SerialNumber=0
>> [   15.917899] usb 2-5: Product: USB IO Controller
>> [   15.917941] usb 2-5: Manufacturer: Delcom Products Inc.
>> [   15.943667] hid-generic 0003:0FC5:B080.0001: hiddev96,hidraw0: USB HID 
>> v1.00 Device [Delcom Products Inc. USB IO Controller ] on 
>> usb-:00:14.0-5/input0
>> [   15.944171] usbcore: registered new interface driver usbhid
>> [   15.944204] usbhid: USB HID core driver
>> [  101.496255] usb 2-5: USB disconnect, device number 4
>> [  107.364402] usb 2-5: new low-speed USB device number 5 using xhci_hcd
>> [  107.496582] usb 2-5: New USB device found, idVendor=0fc5, idProduct=b080, 
>> bcdDevice= 0.1f
>> [  107.496659] usb 2-5: New USB device strings: Mfr=1, Product=2, 
>> SerialNumber=0
>> [  107.496716] usb 2-5: Product: USB IO Controller
>> [  107.496757] usb 2-5: Manufacturer: Delcom Products Inc.
>> [  107.510676] hid-led 0003:0FC5:B080.0002: hidraw0: USB HID v1.00 Device 
>> [Delcom Products Inc. USB IO Controller ] on usb-:00:14.0-5/input0
>> [  107.511975] hid-led 0003:0FC5:B080.0002: Delcom Visual Signal Indicator 
>> G2 initialized
> 

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

Race related to e04a0442d33b "HID: core: remove the absolute need of hid_have_special_driver[]"

[Heiner Kallweit]

Due to some other issue with one devices supported by hid-led I figured
out that it's no longer needed to list devices with own driver in
hid_have_special_driver[].

So I removed the entries for the hid-led devices and got the following.
When I plugged in the device first the device-specific driver wasn't
loaded. After unplugging/re-plugging the device-specific driver was
loaded properly.

It seems usbhid module wasn't fully loaded and active yet when
hid-generic checks for a device-specific driver for the first time.
This also explains why the second attempt was successful.

Did you come across similar races? At a first glance I'm not sure how
to prevent this race. Any idea?

[   15.785205] usb 2-5: new low-speed USB device number 4 using xhci_hcd
[   15.917766] usb 2-5: New USB device found, idVendor=0fc5, idProduct=b080, 
bcdDevice= 0.1f
[   15.917842] usb 2-5: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   15.917899] usb 2-5: Product: USB IO Controller
[   15.917941] usb 2-5: Manufacturer: Delcom Products Inc.
[   15.943667] hid-generic 0003:0FC5:B080.0001: hiddev96,hidraw0: USB HID v1.00 
Device [Delcom Products Inc. USB IO Controller ] on usb-:00:14.0-5/input0
[   15.944171] usbcore: registered new interface driver usbhid
[   15.944204] usbhid: USB HID core driver
[  101.496255] usb 2-5: USB disconnect, device number 4
[  107.364402] usb 2-5: new low-speed USB device number 5 using xhci_hcd
[  107.496582] usb 2-5: New USB device found, idVendor=0fc5, idProduct=b080, 
bcdDevice= 0.1f
[  107.496659] usb 2-5: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[  107.496716] usb 2-5: Product: USB IO Controller
[  107.496757] usb 2-5: Manufacturer: Delcom Products Inc.
[  107.510676] hid-led 0003:0FC5:B080.0002: hidraw0: USB HID v1.00 Device 
[Delcom Products Inc. USB IO Controller ] on usb-:00:14.0-5/input0
[  107.511975] hid-led 0003:0FC5:B080.0002: Delcom Visual Signal Indicator G2 
initialized
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: dwc2: skip regulator operations if supplies vusb_a and vusb_d are not available

[Heiner Kallweit]

Supplies for vusb_a and vusb_d are needed only on a minority of systems
supported by the dwc2 driver (AFAIK systems with Samsung SoCs).

On all other systems this results in harmless but annoying warnings:

c900.usb supply vusb_d not found, using dummy regulator
c900.usb supply vusb_a not found, using dummy regulator

Therefore introduce an upfront check whether the supplies are available.
If they are not skip all supply regulator operations.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- replace the config parameter with an upfront check whether
  the supplies are available and adjust commit message and
  patch subject accordingly
---
 drivers/usb/dwc2/core.h |  2 ++
 drivers/usb/dwc2/platform.c | 35 +++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 1a7e8300..47e32f0e 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -772,6 +772,7 @@ struct dwc2_hregs_backup {
  * @plat:   The platform specific configuration data. This can be
  *  removed once all SoCs support usb transceiver.
  * @supplies:   Definition of USB power supplies
+ * @supplies_available: Supplies are available (optional on most chips)
  * @phyif:  PHY interface width
  * @lock:  Spinlock that protects all the driver data structures
  * @priv:  Stores a pointer to the struct usb_hcd
@@ -908,6 +909,7 @@ struct dwc2_hsotg {
struct usb_phy *uphy;
struct dwc2_hsotg_plat *plat;
struct regulator_bulk_data supplies[DWC2_NUM_SUPPLIES];
+   bool supplies_available;
u32 phyif;
 
spinlock_t lock;
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 9564bc76..849f6bdb 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -125,10 +125,12 @@ static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg 
*hsotg)
struct platform_device *pdev = to_platform_device(hsotg->dev);
int ret;
 
-   ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
-   hsotg->supplies);
-   if (ret)
-   return ret;
+   if (hsotg->supplies_available) {
+   ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
+   hsotg->supplies);
+   if (ret)
+   return ret;
+   }
 
if (hsotg->clk) {
ret = clk_prepare_enable(hsotg->clk);
@@ -185,6 +187,9 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg 
*hsotg)
if (hsotg->clk)
clk_disable_unprepare(hsotg->clk);
 
+   if (!hsotg->supplies_available)
+   return 0;
+
ret = regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
 hsotg->supplies);
 
@@ -207,6 +212,25 @@ int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
return ret;
 }
 
+static bool dwc2_supplies_available(struct dwc2_hsotg *hsotg)
+{
+   struct regulator *reg;
+   int i;
+
+   for (i = 0; i < ARRAY_SIZE(dwc2_hsotg_supply_names); i++) {
+   reg = regulator_get_optional(hsotg->dev,
+dwc2_hsotg_supply_names[i]);
+   if (reg == ERR_PTR(-ENODEV)) {
+   hsotg->supplies_available = false;
+   return false;
+   }
+   regulator_put(reg);
+   }
+
+   hsotg->supplies_available = true;
+   return true;
+}
+
 static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
 {
int i, ret;
@@ -293,6 +317,9 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
for (i = 0; i < ARRAY_SIZE(hsotg->supplies); i++)
hsotg->supplies[i].supply = dwc2_hsotg_supply_names[i];
 
+   if (!dwc2_supplies_available(hsotg))
+   return 0;
+
ret = devm_regulator_bulk_get(hsotg->dev, ARRAY_SIZE(hsotg->supplies),
  hsotg->supplies);
if (ret) {
-- 
2.11.0


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

Re: [PATCH] usb: dwc2: introduce config parameter to ignore supplies vusb_a and vusb_d

[Heiner Kallweit]

Am 31.01.2017 um 20:23 schrieb Heiner Kallweit:
> Am 31.01.2017 um 19:48 schrieb John Youn:
>> On 1/30/2017 11:13 PM, Heiner Kallweit wrote:
>>> Am 31.01.2017 um 03:32 schrieb John Youn:
>>>> On 1/28/2017 2:06 PM, Heiner Kallweit wrote:
>>>>> Supplies for vusb_a and vusb_d are needed only on a minority of systems
>>>>> supported by the dwc2 driver (AFAIK systems with Samsung SoCs).
>>>>>
>>>>> On all other systems this results in these harmless but annoying
>>>>> warnings:
>>>>>
>>>>> c900.usb supply vusb_d not found, using dummy regulator
>>>>> c900.usb supply vusb_a not found, using dummy regulator
>>>>>
>>>>> Introduce a configuration parameter to ignore the supplies on
>>>>> systems not needing it.
>>>>
>>>> You can probably just check for the existence of those bindings. Or
>>>> possibly use devm_regulator_get_optional().
>>>>
>>> Right, that we could do to silence the warning. But then we would
>>> loose the warning on systems where these supplies are mandatory
>>> (in case somebody adds a system with such a SoC and forgets to define
>>> the supplies in the DT).
>>
>> I don't think the dwc2 driver should be doing that though. It should
>> treat these as optional properties and use them if available, just
>> like it does now for the reset control, phy, clocks, etc. Otherwise
>> you could make the case for adding the same flag for these other
>> components as well.
>>
>> But really, I just don't want to add new parameters, if anything I
>> want to remove more of them.
>>
> I see. Then most likely it's best to switch to
> devm_regulator_get_optional. I'll send a patch for it.
> 
Hmm, I just see that devm_regulator_bulk_get is used. And there's
no bulk function for optional regulators (yet).

> Heiner
> 
>> Regards,
>> John
>>
> 

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

Re: [PATCH] usb: dwc2: introduce config parameter to ignore supplies vusb_a and vusb_d

[Heiner Kallweit]

Am 31.01.2017 um 19:48 schrieb John Youn:
> On 1/30/2017 11:13 PM, Heiner Kallweit wrote:
>> Am 31.01.2017 um 03:32 schrieb John Youn:
>>> On 1/28/2017 2:06 PM, Heiner Kallweit wrote:
>>>> Supplies for vusb_a and vusb_d are needed only on a minority of systems
>>>> supported by the dwc2 driver (AFAIK systems with Samsung SoCs).
>>>>
>>>> On all other systems this results in these harmless but annoying
>>>> warnings:
>>>>
>>>> c900.usb supply vusb_d not found, using dummy regulator
>>>> c900.usb supply vusb_a not found, using dummy regulator
>>>>
>>>> Introduce a configuration parameter to ignore the supplies on
>>>> systems not needing it.
>>>
>>> You can probably just check for the existence of those bindings. Or
>>> possibly use devm_regulator_get_optional().
>>>
>> Right, that we could do to silence the warning. But then we would
>> loose the warning on systems where these supplies are mandatory
>> (in case somebody adds a system with such a SoC and forgets to define
>> the supplies in the DT).
> 
> I don't think the dwc2 driver should be doing that though. It should
> treat these as optional properties and use them if available, just
> like it does now for the reset control, phy, clocks, etc. Otherwise
> you could make the case for adding the same flag for these other
> components as well.
> 
> But really, I just don't want to add new parameters, if anything I
> want to remove more of them.
> 
I see. Then most likely it's best to switch to
devm_regulator_get_optional. I'll send a patch for it.

Heiner

> Regards,
> John
> 

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

Re: [PATCH] usb: dwc2: introduce config parameter to ignore supplies vusb_a and vusb_d

[Heiner Kallweit]

Am 31.01.2017 um 03:32 schrieb John Youn:
> On 1/28/2017 2:06 PM, Heiner Kallweit wrote:
>> Supplies for vusb_a and vusb_d are needed only on a minority of systems
>> supported by the dwc2 driver (AFAIK systems with Samsung SoCs).
>>
>> On all other systems this results in these harmless but annoying
>> warnings:
>>
>> c900.usb supply vusb_d not found, using dummy regulator
>> c900.usb supply vusb_a not found, using dummy regulator
>>
>> Introduce a configuration parameter to ignore the supplies on
>> systems not needing it.
> 
> You can probably just check for the existence of those bindings. Or
> possibly use devm_regulator_get_optional().
> 
Right, that we could do to silence the warning. But then we would
loose the warning on systems where these supplies are mandatory
(in case somebody adds a system with such a SoC and forgets to define
the supplies in the DT).

> Also, reviewing this code, it looks like vusb_d and vusb_a should have
> been documented as dwc2 bindings but never were...
> 
> John
> 

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

[PATCH] usb: dwc2: introduce config parameter to ignore supplies vusb_a and vusb_d

[Heiner Kallweit]

Supplies for vusb_a and vusb_d are needed only on a minority of systems
supported by the dwc2 driver (AFAIK systems with Samsung SoCs).

On all other systems this results in these harmless but annoying
warnings:

c900.usb supply vusb_d not found, using dummy regulator
c900.usb supply vusb_a not found, using dummy regulator

Introduce a configuration parameter to ignore the supplies on
systems not needing it.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/dwc2/core.h |  2 ++
 drivers/usb/dwc2/params.c   |  6 ++
 drivers/usb/dwc2/platform.c | 30 +++---
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index a708e4fa..42deeeb4 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -433,6 +433,7 @@ enum dwc2_ep0_state {
  * needed.
  * 0 - No (default)
  * 1 - Yes
+ * @needs_supplies:Chip needs supplies (for vusb_a and vusb_d)
  * @ahb_burst:  Specifies the AHB burst.
  *   0 - Single
  *   1 - INCR
@@ -506,6 +507,7 @@ struct dwc2_core_params {
int uframe_sched;
int external_id_pin_ctl;
int hibernation;
+   bool needs_supplies;
 
/*
 * The following parameters are *only* set via device
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 014e269f..baf8b821 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -67,6 +67,7 @@ static const struct dwc2_core_params params_hi6220 = {
.uframe_sched   = 0,
.external_id_pin_ctl= -1,
.hibernation= -1,
+   .needs_supplies = false,
 };
 
 static const struct dwc2_core_params params_bcm2835 = {
@@ -104,6 +105,7 @@ static const struct dwc2_core_params params_bcm2835 = {
.uframe_sched   = 0,
.external_id_pin_ctl= -1,
.hibernation= -1,
+   .needs_supplies = false,
 };
 
 static const struct dwc2_core_params params_rk3066 = {
@@ -135,6 +137,7 @@ static const struct dwc2_core_params params_rk3066 = {
.uframe_sched   = -1,
.external_id_pin_ctl= -1,
.hibernation= -1,
+   .needs_supplies = false,
 };
 
 static const struct dwc2_core_params params_ltq = {
@@ -166,6 +169,7 @@ static const struct dwc2_core_params params_ltq = {
.uframe_sched   = -1,
.external_id_pin_ctl= -1,
.hibernation= -1,
+   .needs_supplies = false,
 };
 
 static const struct dwc2_core_params params_amlogic = {
@@ -197,6 +201,7 @@ static const struct dwc2_core_params params_amlogic = {
.uframe_sched   = 0,
.external_id_pin_ctl= -1,
.hibernation= -1,
+   .needs_supplies = false,
 };
 
 static const struct dwc2_core_params params_default = {
@@ -234,6 +239,7 @@ static const struct dwc2_core_params params_default = {
.uframe_sched   = -1,
.external_id_pin_ctl= -1,
.hibernation= -1,
+   .needs_supplies = true,
 };
 
 const struct of_device_id dwc2_of_match_table[] = {
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 3f59a73d..b218a72e 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -125,10 +125,12 @@ static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg 
*hsotg)
struct platform_device *pdev = to_platform_device(hsotg->dev);
int ret;
 
-   ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
-   hsotg->supplies);
-   if (ret)
-   return ret;
+   if (hsotg->params.needs_supplies) {
+   ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
+   hsotg->supplies);
+   if (ret)
+   return ret;
+   }
 
if (hsotg->clk) {
ret = clk_prepare_enable(hsotg->clk);
@@ -185,8 +187,9 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg 
*hsotg)
if (hsotg->clk)
clk_disable_unprepare(hsotg->clk);
 
-   ret = regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
-hsotg->supplies);
+   if (hsotg->params.needs_supplies)
+   ret = regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
+hsotg->supplies);
 
return ret;
 }
@@ -293,12 +296,17 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
for (i = 0; i < A

[PATCH] usb: dwc2: remove deprecated comment in definition of struct dwc2_core_params

[Heiner Kallweit]

Since commit 0a7d0d7fa820 "usb: dwc2: Remove dwc2_set_all_params function"
this comment isn't applicable any longer.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/dwc2/core.h | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 570a8004..a708e4fa 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -461,10 +461,6 @@ enum dwc2_ep0_state {
  * default described above.
  */
 struct dwc2_core_params {
-   /*
-* Don't add any non-int members here, this will break
-* dwc2_set_all_params!
-*/
int otg_cap;
 #define DWC2_CAP_PARAM_HNP_SRP_CAPABLE 0
 #define DWC2_CAP_PARAM_SRP_ONLY_CAPABLE1
-- 
2.11.0

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

[PATCH] usb: dwc2: eliminate irq parameter from dwc2_hcd_init

[Heiner Kallweit]

The irq is available in hsotg already, so there's no need to
pass it as separate function parameter.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/dwc2/core.h | 2 +-
 drivers/usb/dwc2/hcd.c  | 4 ++--
 drivers/usb/dwc2/hcd.h  | 2 +-
 drivers/usb/dwc2/platform.c | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 570a8004..e89943ff 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1245,7 +1245,7 @@ static inline void dwc2_hcd_connect(struct dwc2_hsotg 
*hsotg) {}
 static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
 static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
 static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
-static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
+static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
 { return 0; }
 static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
 { return 0; }
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index fa8a9f27..ef568c46 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4965,7 +4965,7 @@ static void dwc2_hcd_release(struct dwc2_hsotg *hsotg)
  * USB bus with the core and calls the hc_driver->start() function. It returns
  * a negative error on failure.
  */
-int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
+int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
 {
struct platform_device *pdev = to_platform_device(hsotg->dev);
struct resource *res;
@@ -5168,7 +5168,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
 * allocates the DMA buffer pool, registers the USB bus, requests the
 * IRQ line, and calls hcd_start method.
 */
-   retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
+   retval = usb_add_hcd(hcd, hsotg->irq, IRQF_SHARED);
if (retval < 0)
goto error4;
 
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 58bfe9f5..11c3c145 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -521,7 +521,7 @@ static inline u8 dwc2_hcd_is_pipe_out(struct 
dwc2_hcd_pipe_info *pipe)
return !dwc2_hcd_is_pipe_in(pipe);
 }
 
-int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq);
+int dwc2_hcd_init(struct dwc2_hsotg *hsotg);
 void dwc2_hcd_remove(struct dwc2_hsotg *hsotg);
 
 /* Transaction Execution Functions */
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 3f59a73d..9564bc76 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -445,7 +445,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
}
 
if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) {
-   retval = dwc2_hcd_init(hsotg, hsotg->irq);
+   retval = dwc2_hcd_init(hsotg);
if (retval) {
if (hsotg->gadget_enabled)
dwc2_hsotg_remove(hsotg);
-- 
2.11.0

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

Re: [PATCH] usb: dwc2: fix "iomem 0x00000000" message by setting iomem parameters in usb_hcd

[Heiner Kallweit]

Am 25.01.2017 um 22:28 schrieb John Youn:
> On 1/15/2017 12:37 PM, Heiner Kallweit wrote:
>> Set the iomem parameters in the usb_hcd to fix this misleading
>> message during driver load:
>> dwc2 c910.usb: irq 22, io mem 0x
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>> ---
>>  drivers/usb/dwc2/core.h | 3 ++-
>>  drivers/usb/dwc2/hcd.c  | 5 -
>>  drivers/usb/dwc2/hcd.h  | 3 ++-
>>  drivers/usb/dwc2/platform.c | 2 +-
>>  4 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 9548d3e0..b66eaeea 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -1229,7 +1229,8 @@ static inline void dwc2_hcd_connect(struct dwc2_hsotg 
>> *hsotg) {}
>>  static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool 
>> force) {}
>>  static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>>  static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
>> -static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>> +static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>> +struct resource *res)
>>  { return 0; }
>>  static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
>>  { return 0; }
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 911c3b36..2cfbd10e 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -4964,7 +4964,7 @@ static void dwc2_hcd_release(struct dwc2_hsotg *hsotg)
>>   * USB bus with the core and calls the hc_driver->start() function. It 
>> returns
>>   * a negative error on failure.
>>   */
>> -int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>> +int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, struct resource *res)
>>  {
>>  struct usb_hcd *hcd;
>>  struct dwc2_host_chan *channel;
>> @@ -5021,6 +5021,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>
>>  hcd->has_tt = 1;
>>
>> +hcd->rsrc_start = res->start;
>> +hcd->rsrc_len = resource_size(res);
> 
> You should be able to get the same from hsotg->dev instead of adding a
> paramter.
> 
> In fact, looks like the irq parameter is not needed either since it is 
> already stored in hsotg and can anyway get it from dev as well.
> 
> John
> 
Indeed. I'll send a v2 for the patch and another one for getting rid of
the irq function parameter.

Heiner

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

[PATCH v2] usb: dwc2: fix "iomem 0x00000000" message

[Heiner Kallweit]

Set the iomem parameters in the usb_hcd to fix this misleading
message during driver load:
dwc2 c910.usb: irq 22, io mem 0x

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- get info from hsotg->dev instead of adding a function parameter
---
 drivers/usb/dwc2/hcd.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 98249be1..fa8a9f27 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -42,6 +42,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -4966,6 +4967,8 @@ static void dwc2_hcd_release(struct dwc2_hsotg *hsotg)
  */
 int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
 {
+   struct platform_device *pdev = to_platform_device(hsotg->dev);
+   struct resource *res;
struct usb_hcd *hcd;
struct dwc2_host_chan *channel;
u32 hcfg;
@@ -5021,6 +5024,10 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
 
hcd->has_tt = 1;
 
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   hcd->rsrc_start = res->start;
+   hcd->rsrc_len = resource_size(res);
+
((struct wrapper_priv_data *)>hcd_priv)->hsotg = hsotg;
hsotg->priv = hcd;
 
-- 
2.11.0

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

Re: [PATCH] usb: dwc2: fix "iomem 0x00000000" message by setting iomem parameters in usb_hcd

[Heiner Kallweit]

Am 24.01.2017 um 09:46 schrieb Felipe Balbi:
> 
> Hi,
> 
> John Youn  writes:
>>> John Youn  writes:
 @@ -1229,7 +1229,8 @@ static inline void dwc2_hcd_connect(struct 
 dwc2_hsotg *hsotg) {}
  static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool 
 force) {}
  static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
  static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
 -static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
 +static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
 +  struct resource *res)
  { return 0; }
  static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
  { return 0; }
 diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
 index 911c3b36..2cfbd10e 100644
 --- a/drivers/usb/dwc2/hcd.c
 +++ b/drivers/usb/dwc2/hcd.c
 @@ -4964,7 +4964,7 @@ static void dwc2_hcd_release(struct dwc2_hsotg 
 *hsotg)
   * USB bus with the core and calls the hc_driver->start() function. 
 It returns
   * a negative error on failure.
   */
 -int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
 +int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, struct resource 
 *res)
  {
struct usb_hcd *hcd;
struct dwc2_host_chan *channel;
 @@ -5021,6 +5021,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int 
 irq)

hcd->has_tt = 1;

 +  hcd->rsrc_start = res->start;
 +  hcd->rsrc_len = resource_size(res);
 +
((struct wrapper_priv_data *) >hcd_priv)->hsotg = hsotg;
hsotg->priv = hcd;

 diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
 index 1ed5fa2b..2305b5fb 100644
 --- a/drivers/usb/dwc2/hcd.h
 +++ b/drivers/usb/dwc2/hcd.h
 @@ -521,7 +521,8 @@ static inline u8 dwc2_hcd_is_pipe_out(struct 
 dwc2_hcd_pipe_info *pipe)
return !dwc2_hcd_is_pipe_in(pipe);
  }

 -extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq);
 +extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
 +   struct resource *res);
  extern void dwc2_hcd_remove(struct dwc2_hsotg *hsotg);

  /* Transaction Execution Functions */
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index 4fc8c603..5ddc8860 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -445,7 +445,7 @@ static int dwc2_driver_probe(struct 
 platform_device *dev)
}

if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) {
 -  retval = dwc2_hcd_init(hsotg, hsotg->irq);
 +  retval = dwc2_hcd_init(hsotg, hsotg->irq, res);
>>>
>>> This is a good idea, but there's more work that can come out of this, if
>>> you're interested:
>>>
>>> i) devm_ioremap_resource() could be called by the generic layer
>>> ii) devm_request_irq() could be move to the generic layer
>>> iii) dwc2_hcd_init() could also be called generically as long as dr_mode
>>>  is set properly.
>>> iv) dwc2_debugfs_init() could be called generically as well
>>>
>>> IOW, dwc2_driver_probe() could be as minimal as:
>>>
>>> static int dwc2_driver_probe(struct platform_device *dev)
>>> {
>>> struct dwc2_hsotg *hsotg;
>>> struct resource *res;
>>> int retval;
>>>
>>> hsotg = devm_kzalloc(>dev, sizeof(*hsotg), GFP_KERNEL);
>>> if (!hsotg)
>>> return -ENOMEM;
>>>
>>> hsotg->dev = >dev;
>>>
>>> if (!dev->dev.dma_mask)
>>> dev->dev.dma_mask = >dev.coherent_dma_mask;
>>>
>>> retval = dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
>>> if (retval)
>>> return retval;
>>>
>>> hsotg->res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>>> hsotg->irq = platform_get_irq(dev, 0);
>>>
>>> retval = dwc2_get_dr_mode(hsotg);
>>> if (retval)
>>> return retval;
>>>
>>> retval = dwc2_get_hwparams(hsotg);
>>> if (retval)
>>> return retval;
>>>
>>> platform_set_drvdata(dev, hsotg);
>>>
>>> retval = dwc2_core_init(hsotg);
>>> if (retval)
>>> return retval;
>>>
>>> return 0;
>>> }
>>>
>>> dwc2_core_init() needs to implemented, of course, but it could hide all
>>> 

Re: [PATCH] usb: dwc2: fix "iomem 0x00000000" message by setting iomem parameters in usb_hcd

[Heiner Kallweit]

Am 17.01.2017 um 21:08 schrieb Heiner Kallweit:
> Am 17.01.2017 um 09:11 schrieb Felipe Balbi:
>>
>> Hi,
>>
>> Heiner Kallweit <hkallwe...@gmail.com> writes:
>>> Am 16.01.2017 um 15:05 schrieb Felipe Balbi:
>>>>
>>>> Hi,
>>>>
>>>> Heiner Kallweit <hkallwe...@gmail.com> writes:
>>>>> Set the iomem parameters in the usb_hcd to fix this misleading
>>>>> message during driver load:
>>>>> dwc2 c910.usb: irq 22, io mem 0x
>>>>>
>>>>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>>>>> ---
>>>>>  drivers/usb/dwc2/core.h | 3 ++-
>>>>>  drivers/usb/dwc2/hcd.c  | 5 -
>>>>>  drivers/usb/dwc2/hcd.h  | 3 ++-
>>>>>  drivers/usb/dwc2/platform.c | 2 +-
>>>>>  4 files changed, 9 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>>>> index 9548d3e0..b66eaeea 100644
>>>>> --- a/drivers/usb/dwc2/core.h
>>>>> +++ b/drivers/usb/dwc2/core.h
>>>>> @@ -1229,7 +1229,8 @@ static inline void dwc2_hcd_connect(struct 
>>>>> dwc2_hsotg *hsotg) {}
>>>>>  static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool 
>>>>> force) {}
>>>>>  static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>>>>>  static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
>>>>> -static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>>>> +static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>>>>> + struct resource *res)
>>>>>  { return 0; }
>>>>>  static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
>>>>>  { return 0; }
>>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>>> index 911c3b36..2cfbd10e 100644
>>>>> --- a/drivers/usb/dwc2/hcd.c
>>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>>> @@ -4964,7 +4964,7 @@ static void dwc2_hcd_release(struct dwc2_hsotg 
>>>>> *hsotg)
>>>>>   * USB bus with the core and calls the hc_driver->start() function. It 
>>>>> returns
>>>>>   * a negative error on failure.
>>>>>   */
>>>>> -int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>>>> +int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, struct resource 
>>>>> *res)
>>>>>  {
>>>>>   struct usb_hcd *hcd;
>>>>>   struct dwc2_host_chan *channel;
>>>>> @@ -5021,6 +5021,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>>>>  
>>>>>   hcd->has_tt = 1;
>>>>>  
>>>>> + hcd->rsrc_start = res->start;
>>>>> + hcd->rsrc_len = resource_size(res);
>>>>> +
>>>>>   ((struct wrapper_priv_data *) >hcd_priv)->hsotg = hsotg;
>>>>>   hsotg->priv = hcd;
>>>>>  
>>>>> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
>>>>> index 1ed5fa2b..2305b5fb 100644
>>>>> --- a/drivers/usb/dwc2/hcd.h
>>>>> +++ b/drivers/usb/dwc2/hcd.h
>>>>> @@ -521,7 +521,8 @@ static inline u8 dwc2_hcd_is_pipe_out(struct 
>>>>> dwc2_hcd_pipe_info *pipe)
>>>>>   return !dwc2_hcd_is_pipe_in(pipe);
>>>>>  }
>>>>>  
>>>>> -extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq);
>>>>> +extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>>>>> +  struct resource *res);
>>>>>  extern void dwc2_hcd_remove(struct dwc2_hsotg *hsotg);
>>>>>  
>>>>>  /* Transaction Execution Functions */
>>>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>>>> index 4fc8c603..5ddc8860 100644
>>>>> --- a/drivers/usb/dwc2/platform.c
>>>>> +++ b/drivers/usb/dwc2/platform.c
>>>>> @@ -445,7 +445,7 @@ static int dwc2_driver_probe(struct platform_device 
>>>>> *dev)
>>>>>   }
>>>>>  
>>>>>   if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) {
>>>>> - retval = dwc2_hcd_init(hsotg, hsotg->irq);
>>>>> + retval = dwc2_hcd_init(hsotg, hsotg->i

[PATCH] usb: dwc2: remove dead function dwc2_pci_quirks

[Heiner Kallweit]

Commit 9962b62f1be9 "Deprecate g-use-dma binding" removed the only
property in dwc2_pci_quirks. This function is dead code now.

Maybe it was kept intentionally to be prepared for the case that
another quirk-related property needs to be added in future.
If not it can be removed.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/dwc2/pci.c | 18 --
 1 file changed, 18 deletions(-)

diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
index a23329e3..ae419615 100644
--- a/drivers/usb/dwc2/pci.c
+++ b/drivers/usb/dwc2/pci.c
@@ -62,20 +62,6 @@ struct dwc2_pci_glue {
struct platform_device *phy;
 };
 
-static int dwc2_pci_quirks(struct pci_dev *pdev, struct platform_device *dwc2)
-{
-   if (pdev->vendor == PCI_VENDOR_ID_SYNOPSYS &&
-   pdev->device == PCI_PRODUCT_ID_HAPS_HSOTG) {
-   struct property_entry properties[] = {
-   { },
-   };
-
-   return platform_device_add_properties(dwc2, properties);
-   }
-
-   return 0;
-}
-
 static void dwc2_pci_remove(struct pci_dev *pci)
 {
struct dwc2_pci_glue *glue = pci_get_drvdata(pci);
@@ -136,10 +122,6 @@ static int dwc2_pci_probe(struct pci_dev *pci,
return PTR_ERR(phy);
}
 
-   ret = dwc2_pci_quirks(pci, dwc2);
-   if (ret)
-   goto err;
-
ret = platform_device_add(dwc2);
if (ret) {
dev_err(dev, "failed to register dwc2 device\n");
-- 
2.11.0

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

Re: [PATCH] usb: dwc2: fix "iomem 0x00000000" message by setting iomem parameters in usb_hcd

[Heiner Kallweit]

Am 17.01.2017 um 09:11 schrieb Felipe Balbi:
> 
> Hi,
> 
> Heiner Kallweit <hkallwe...@gmail.com> writes:
>> Am 16.01.2017 um 15:05 schrieb Felipe Balbi:
>>>
>>> Hi,
>>>
>>> Heiner Kallweit <hkallwe...@gmail.com> writes:
>>>> Set the iomem parameters in the usb_hcd to fix this misleading
>>>> message during driver load:
>>>> dwc2 c910.usb: irq 22, io mem 0x
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>>>> ---
>>>>  drivers/usb/dwc2/core.h | 3 ++-
>>>>  drivers/usb/dwc2/hcd.c  | 5 -
>>>>  drivers/usb/dwc2/hcd.h  | 3 ++-
>>>>  drivers/usb/dwc2/platform.c | 2 +-
>>>>  4 files changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>>> index 9548d3e0..b66eaeea 100644
>>>> --- a/drivers/usb/dwc2/core.h
>>>> +++ b/drivers/usb/dwc2/core.h
>>>> @@ -1229,7 +1229,8 @@ static inline void dwc2_hcd_connect(struct 
>>>> dwc2_hsotg *hsotg) {}
>>>>  static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool 
>>>> force) {}
>>>>  static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>>>>  static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
>>>> -static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>>> +static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>>>> +  struct resource *res)
>>>>  { return 0; }
>>>>  static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
>>>>  { return 0; }
>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>> index 911c3b36..2cfbd10e 100644
>>>> --- a/drivers/usb/dwc2/hcd.c
>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>> @@ -4964,7 +4964,7 @@ static void dwc2_hcd_release(struct dwc2_hsotg 
>>>> *hsotg)
>>>>   * USB bus with the core and calls the hc_driver->start() function. It 
>>>> returns
>>>>   * a negative error on failure.
>>>>   */
>>>> -int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>>> +int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, struct resource *res)
>>>>  {
>>>>struct usb_hcd *hcd;
>>>>struct dwc2_host_chan *channel;
>>>> @@ -5021,6 +5021,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>>>  
>>>>hcd->has_tt = 1;
>>>>  
>>>> +  hcd->rsrc_start = res->start;
>>>> +  hcd->rsrc_len = resource_size(res);
>>>> +
>>>>((struct wrapper_priv_data *) >hcd_priv)->hsotg = hsotg;
>>>>hsotg->priv = hcd;
>>>>  
>>>> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
>>>> index 1ed5fa2b..2305b5fb 100644
>>>> --- a/drivers/usb/dwc2/hcd.h
>>>> +++ b/drivers/usb/dwc2/hcd.h
>>>> @@ -521,7 +521,8 @@ static inline u8 dwc2_hcd_is_pipe_out(struct 
>>>> dwc2_hcd_pipe_info *pipe)
>>>>return !dwc2_hcd_is_pipe_in(pipe);
>>>>  }
>>>>  
>>>> -extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq);
>>>> +extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>>>> +   struct resource *res);
>>>>  extern void dwc2_hcd_remove(struct dwc2_hsotg *hsotg);
>>>>  
>>>>  /* Transaction Execution Functions */
>>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>>> index 4fc8c603..5ddc8860 100644
>>>> --- a/drivers/usb/dwc2/platform.c
>>>> +++ b/drivers/usb/dwc2/platform.c
>>>> @@ -445,7 +445,7 @@ static int dwc2_driver_probe(struct platform_device 
>>>> *dev)
>>>>}
>>>>  
>>>>if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) {
>>>> -  retval = dwc2_hcd_init(hsotg, hsotg->irq);
>>>> +  retval = dwc2_hcd_init(hsotg, hsotg->irq, res);
>>>
>>> This is a good idea, but there's more work that can come out of this, if
>>> you're interested:
>>>
>>> i) devm_ioremap_resource() could be called by the generic layer
>>> ii) devm_request_irq() could be move to the generic layer
>>> iii) dwc2_hcd_init() could also be called generically as long as dr_mode
>>>  is set properly.
&

Re: [PATCH] usb: dwc2: fix "iomem 0x00000000" message by setting iomem parameters in usb_hcd

[Heiner Kallweit]

Am 16.01.2017 um 15:05 schrieb Felipe Balbi:
> 
> Hi,
> 
> Heiner Kallweit <hkallwe...@gmail.com> writes:
>> Set the iomem parameters in the usb_hcd to fix this misleading
>> message during driver load:
>> dwc2 c910.usb: irq 22, io mem 0x0000
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>> ---
>>  drivers/usb/dwc2/core.h | 3 ++-
>>  drivers/usb/dwc2/hcd.c  | 5 -
>>  drivers/usb/dwc2/hcd.h  | 3 ++-
>>  drivers/usb/dwc2/platform.c | 2 +-
>>  4 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 9548d3e0..b66eaeea 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -1229,7 +1229,8 @@ static inline void dwc2_hcd_connect(struct dwc2_hsotg 
>> *hsotg) {}
>>  static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool 
>> force) {}
>>  static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>>  static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
>> -static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>> +static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>> +struct resource *res)
>>  { return 0; }
>>  static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
>>  { return 0; }
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 911c3b36..2cfbd10e 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -4964,7 +4964,7 @@ static void dwc2_hcd_release(struct dwc2_hsotg *hsotg)
>>   * USB bus with the core and calls the hc_driver->start() function. It 
>> returns
>>   * a negative error on failure.
>>   */
>> -int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>> +int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, struct resource *res)
>>  {
>>  struct usb_hcd *hcd;
>>  struct dwc2_host_chan *channel;
>> @@ -5021,6 +5021,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>  
>>  hcd->has_tt = 1;
>>  
>> +hcd->rsrc_start = res->start;
>> +hcd->rsrc_len = resource_size(res);
>> +
>>  ((struct wrapper_priv_data *) >hcd_priv)->hsotg = hsotg;
>>  hsotg->priv = hcd;
>>  
>> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
>> index 1ed5fa2b..2305b5fb 100644
>> --- a/drivers/usb/dwc2/hcd.h
>> +++ b/drivers/usb/dwc2/hcd.h
>> @@ -521,7 +521,8 @@ static inline u8 dwc2_hcd_is_pipe_out(struct 
>> dwc2_hcd_pipe_info *pipe)
>>  return !dwc2_hcd_is_pipe_in(pipe);
>>  }
>>  
>> -extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq);
>> +extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>> + struct resource *res);
>>  extern void dwc2_hcd_remove(struct dwc2_hsotg *hsotg);
>>  
>>  /* Transaction Execution Functions */
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index 4fc8c603..5ddc8860 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -445,7 +445,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
>>  }
>>  
>>  if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) {
>> -retval = dwc2_hcd_init(hsotg, hsotg->irq);
>> +retval = dwc2_hcd_init(hsotg, hsotg->irq, res);
> 
> This is a good idea, but there's more work that can come out of this, if
> you're interested:
> 
> i) devm_ioremap_resource() could be called by the generic layer
> ii) devm_request_irq() could be move to the generic layer
> iii) dwc2_hcd_init() could also be called generically as long as dr_mode
>  is set properly.
> iv) dwc2_debugfs_init() could be called generically as well
> 
> IOW, dwc2_driver_probe() could be as minimal as:
> 
> static int dwc2_driver_probe(struct platform_device *dev)
> {
>   struct dwc2_hsotg *hsotg;
>   struct resource *res;
>   int retval;
> 
>   hsotg = devm_kzalloc(>dev, sizeof(*hsotg), GFP_KERNEL);
>   if (!hsotg)
>   return -ENOMEM;
> 
>   hsotg->dev = >dev;
> 
>   if (!dev->dev.dma_mask)
>   dev->dev.dma_mask = >dev.coherent_dma_mask;
> 
>   retval = dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
>   if (retval)
>   return retval;
> 
>   hsotg->res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>   hsotg->irq = platform_get_irq(dev, 0);
> 
>

[PATCH] usb: dwc2: fix "iomem 0x00000000" message by setting iomem parameters in usb_hcd

[Heiner Kallweit]

Set the iomem parameters in the usb_hcd to fix this misleading
message during driver load:
dwc2 c910.usb: irq 22, io mem 0x

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/dwc2/core.h | 3 ++-
 drivers/usb/dwc2/hcd.c  | 5 -
 drivers/usb/dwc2/hcd.h  | 3 ++-
 drivers/usb/dwc2/platform.c | 2 +-
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 9548d3e0..b66eaeea 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1229,7 +1229,8 @@ static inline void dwc2_hcd_connect(struct dwc2_hsotg 
*hsotg) {}
 static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
 static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
 static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
-static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
+static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
+   struct resource *res)
 { return 0; }
 static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
 { return 0; }
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 911c3b36..2cfbd10e 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4964,7 +4964,7 @@ static void dwc2_hcd_release(struct dwc2_hsotg *hsotg)
  * USB bus with the core and calls the hc_driver->start() function. It returns
  * a negative error on failure.
  */
-int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
+int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, struct resource *res)
 {
struct usb_hcd *hcd;
struct dwc2_host_chan *channel;
@@ -5021,6 +5021,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
 
hcd->has_tt = 1;
 
+   hcd->rsrc_start = res->start;
+   hcd->rsrc_len = resource_size(res);
+
((struct wrapper_priv_data *) >hcd_priv)->hsotg = hsotg;
hsotg->priv = hcd;
 
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 1ed5fa2b..2305b5fb 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -521,7 +521,8 @@ static inline u8 dwc2_hcd_is_pipe_out(struct 
dwc2_hcd_pipe_info *pipe)
return !dwc2_hcd_is_pipe_in(pipe);
 }
 
-extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq);
+extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
+struct resource *res);
 extern void dwc2_hcd_remove(struct dwc2_hsotg *hsotg);
 
 /* Transaction Execution Functions */
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 4fc8c603..5ddc8860 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -445,7 +445,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
}
 
if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) {
-   retval = dwc2_hcd_init(hsotg, hsotg->irq);
+   retval = dwc2_hcd_init(hsotg, hsotg->irq, res);
if (retval) {
if (hsotg->gadget_enabled)
dwc2_hsotg_remove(hsotg);
-- 
2.11.0

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

Re: [PATCH v2] hid: hid-led: fix issue with transfer buffer not being dma capable

[Heiner Kallweit]

Am 07.10.2016 um 18:22 schrieb Benjamin Tissoires:
> On Oct 03 2016 or thereabouts, Heiner Kallweit wrote:
>> The hid-led driver works fine under 4.8.0, however with the next
>> kernel from today I get this:
>>
>> [ cut here ]
>> WARNING: CPU: 0 PID: 2578 at drivers/usb/core/hcd.c:1584 
>> usb_hcd_map_urb_for_dma+0x373/0x550 [usbcore]
>> transfer buffer not dma capable
>> Modules linked in: hid_led(+) usbhid vfat fat ir_sony_decoder iwlmvm 
>> led_class mac80211 snd_hda_codec_realtek snd_hda_codec_generic 
>> x86_pkg_temp_thermal iwlwifi crc32c_intel snd_hda_codec_hdmi i2c_i801 
>> i2c_smbus snd_hda_intel cfg80211 snd_hda_codec snd_hda_core snd_pcm r8169 
>> snd_timer mei_me mii snd mei ir_lirc_codec lirc_dev nuvoton_cir rc_core 
>> btusb btintel bluetooth rfkill usb_storage efivarfs ipv6 ehci_pci ehci_hcd 
>> xhci_pci xhci_hcd usbcore usb_common ext4 jbd2 mbcache ahci libahci libata
>> CPU: 0 PID: 2578 Comm: systemd-udevd Not tainted 4.8.0-rc8-next-20161003 #1
>> Hardware name: ZOTAC ZBOX-CI321NANO/ZBOX-CI321NANO, BIOS B246P105 06/01/2015
>>  c90003dbb7e0 81280425 c90003dbb830 
>>  c90003dbb820 8105b086 063003dbb800 88006f374480
>>    0001 880079544000
>> Call Trace:
>>  [] dump_stack+0x68/0x93
>>  [] __warn+0xc6/0xe0
>>  [] warn_slowpath_fmt+0x4a/0x50
>>  [] usb_hcd_map_urb_for_dma+0x373/0x550 [usbcore]
>>  [] usb_hcd_submit_urb+0x316/0x9c0 [usbcore]
>>  [] ? rcu_read_lock_sched_held+0x40/0x80
>>  [] ? module_assert_mutex_or_preempt+0x13/0x50
>>  [] ? __module_address+0x27/0xf0
>>  [] usb_submit_urb+0x2c4/0x520 [usbcore]
>>  [] usb_start_wait_urb+0x5a/0xe0 [usbcore]
>>  [] usb_control_msg+0xbc/0xf0 [usbcore]
>>  [] ? __module_address+0x27/0xf0
>>  [] usbhid_raw_request+0xa4/0x180 [usbhid]
>>  [] hidled_recv+0x71/0xe0 [hid_led]
>>  [] thingm_init+0x2d/0x50 [hid_led]
>>  [] hidled_probe+0xcb/0x24a [hid_led]
>>  [] hid_device_probe+0xd2/0x150
>>  [] driver_probe_device+0x1fd/0x2c0
>>  [] __driver_attach+0x9a/0xa0
>>  [] ? driver_probe_device+0x2c0/0x2c0
>>  [] bus_for_each_dev+0x5d/0x90
>>  [] driver_attach+0x19/0x20
>>  [] bus_add_driver+0x11f/0x220
>>  [] ? 0xa07ac000
>>  [] driver_register+0x5b/0xd0
>>  [] ? 0xa07ac000
>>  [] __hid_register_driver+0x61/0xa0
>>  [] hidled_driver_init+0x1e/0x20 [hid_led]
>>  [] do_one_initcall+0x38/0x150
>>  [] ? rcu_read_lock_sched_held+0x40/0x80
>>  [] ? kmem_cache_alloc_trace+0x1d0/0x230
>>  [] do_init_module+0x5a/0x1cb
>>  [] load_module+0x1e42/0x2530
>>  [] ? __symbol_put+0x50/0x50
>>  [] ? show_coresize+0x30/0x30
>>  [] ? kernel_read_file+0x100/0x190
>>  [] ? kernel_read_file_from_fd+0x44/0x70
>>  [] SYSC_finit_module+0xba/0xc0
>>  [] SyS_finit_module+0x9/0x10
>>  [] entry_SYSCALL_64_fastpath+0x18/0xad
>> ---[ end trace c9e6ea27003ecf9e ]---
>>
>> Fix this by using a kmalloc'ed buffer when calling hid_hw_raw_request.
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>> ---
>> v2:
>> - Based on a comment from Alan Stern allocate the buffer to be provided to
>>   hid_hw_raw_request separately (and not as part of struct hidled_device).
>>   Alternative would have been to allocate the buffer dynamically in each
>>   function calling hidled_send/_recv. However this would mean more overhead
>>   IMHO, and we'd need an error path in callers to free the buffer in case
>>   of an error.
>>   In addition we have better control that a proper buffer is used in case
>>   the driver is extended by somebody for supporting another LED device.
>> ---
> 
> Looks like the receive function is only called from .probe(), so this
> should be safe.
> However, for the send function, is there a chance there can be a
> concurrent access of the buffer? (like 2 userspace processes writing a
> different values at the same time).
> 
Buffer ldev->buf is accessed with mutex ldev->lock held in hidled_send.
So the locking exists already.

> If so, then you'll need to add a locking mechanism (can't recall if the
> LED API provide one for us or not), or just alloc/free the buffers
> directly.
> 
> If no, the patch is:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoi...@redhat.com>
> 
> Cheers,
> Benjamin
> 
>>  drivers/hid/hid-led.c | 23 +++
>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hid/hid-led.c b/drivers/hid/hid-led.c
>> index d8d55f3..555c88

[PATCH v2] hid: hid-led: fix issue with transfer buffer not being dma capable

[Heiner Kallweit]

The hid-led driver works fine under 4.8.0, however with the next
kernel from today I get this:

[ cut here ]
WARNING: CPU: 0 PID: 2578 at drivers/usb/core/hcd.c:1584 
usb_hcd_map_urb_for_dma+0x373/0x550 [usbcore]
transfer buffer not dma capable
Modules linked in: hid_led(+) usbhid vfat fat ir_sony_decoder iwlmvm led_class 
mac80211 snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal 
iwlwifi crc32c_intel snd_hda_codec_hdmi i2c_i801 i2c_smbus snd_hda_intel 
cfg80211 snd_hda_codec snd_hda_core snd_pcm r8169 snd_timer mei_me mii snd mei 
ir_lirc_codec lirc_dev nuvoton_cir rc_core btusb btintel bluetooth rfkill 
usb_storage efivarfs ipv6 ehci_pci ehci_hcd xhci_pci xhci_hcd usbcore 
usb_common ext4 jbd2 mbcache ahci libahci libata
CPU: 0 PID: 2578 Comm: systemd-udevd Not tainted 4.8.0-rc8-next-20161003 #1
Hardware name: ZOTAC ZBOX-CI321NANO/ZBOX-CI321NANO, BIOS B246P105 06/01/2015
 c90003dbb7e0 81280425 c90003dbb830 
 c90003dbb820 8105b086 063003dbb800 88006f374480
   0001 880079544000
Call Trace:
 [] dump_stack+0x68/0x93
 [] __warn+0xc6/0xe0
 [] warn_slowpath_fmt+0x4a/0x50
 [] usb_hcd_map_urb_for_dma+0x373/0x550 [usbcore]
 [] usb_hcd_submit_urb+0x316/0x9c0 [usbcore]
 [] ? rcu_read_lock_sched_held+0x40/0x80
 [] ? module_assert_mutex_or_preempt+0x13/0x50
 [] ? __module_address+0x27/0xf0
 [] usb_submit_urb+0x2c4/0x520 [usbcore]
 [] usb_start_wait_urb+0x5a/0xe0 [usbcore]
 [] usb_control_msg+0xbc/0xf0 [usbcore]
 [] ? __module_address+0x27/0xf0
 [] usbhid_raw_request+0xa4/0x180 [usbhid]
 [] hidled_recv+0x71/0xe0 [hid_led]
 [] thingm_init+0x2d/0x50 [hid_led]
 [] hidled_probe+0xcb/0x24a [hid_led]
 [] hid_device_probe+0xd2/0x150
 [] driver_probe_device+0x1fd/0x2c0
 [] __driver_attach+0x9a/0xa0
 [] ? driver_probe_device+0x2c0/0x2c0
 [] bus_for_each_dev+0x5d/0x90
 [] driver_attach+0x19/0x20
 [] bus_add_driver+0x11f/0x220
 [] ? 0xa07ac000
 [] driver_register+0x5b/0xd0
 [] ? 0xa07ac000
 [] __hid_register_driver+0x61/0xa0
 [] hidled_driver_init+0x1e/0x20 [hid_led]
 [] do_one_initcall+0x38/0x150
 [] ? rcu_read_lock_sched_held+0x40/0x80
 [] ? kmem_cache_alloc_trace+0x1d0/0x230
 [] do_init_module+0x5a/0x1cb
 [] load_module+0x1e42/0x2530
 [] ? __symbol_put+0x50/0x50
 [] ? show_coresize+0x30/0x30
 [] ? kernel_read_file+0x100/0x190
 [] ? kernel_read_file_from_fd+0x44/0x70
 [] SYSC_finit_module+0xba/0xc0
 [] SyS_finit_module+0x9/0x10
 [] entry_SYSCALL_64_fastpath+0x18/0xad
---[ end trace c9e6ea27003ecf9e ]---

Fix this by using a kmalloc'ed buffer when calling hid_hw_raw_request.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- Based on a comment from Alan Stern allocate the buffer to be provided to
  hid_hw_raw_request separately (and not as part of struct hidled_device).
  Alternative would have been to allocate the buffer dynamically in each
  function calling hidled_send/_recv. However this would mean more overhead
  IMHO, and we'd need an error path in callers to free the buffer in case
  of an error.
  In addition we have better control that a proper buffer is used in case
  the driver is extended by somebody for supporting another LED device.
---
 drivers/hid/hid-led.c | 23 +++
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-led.c b/drivers/hid/hid-led.c
index d8d55f3..555c88e 100644
--- a/drivers/hid/hid-led.c
+++ b/drivers/hid/hid-led.c
@@ -100,6 +100,7 @@ struct hidled_device {
const struct hidled_config *config;
struct hid_device   *hdev;
struct hidled_rgb   *rgb;
+   u8  *buf;
struct mutexlock;
 };
 
@@ -118,13 +119,19 @@ static int hidled_send(struct hidled_device *ldev, __u8 
*buf)
 
mutex_lock(>lock);
 
+   /*
+* buffer provided to hid_hw_raw_request must not be on the stack
+* and must not be part of a data structure
+*/
+   memcpy(ldev->buf, buf, ldev->config->report_size);
+
if (ldev->config->report_type == RAW_REQUEST)
-   ret = hid_hw_raw_request(ldev->hdev, buf[0], buf,
+   ret = hid_hw_raw_request(ldev->hdev, buf[0], ldev->buf,
 ldev->config->report_size,
 HID_FEATURE_REPORT,
 HID_REQ_SET_REPORT);
else if (ldev->config->report_type == OUTPUT_REPORT)
-   ret = hid_hw_output_report(ldev->hdev, buf,
+   ret = hid_hw_output_report(ldev->hdev, ldev->buf,
   ldev->config->report_size);
else
ret = -EINVAL;
@@ -147,17 +154,21 @@ static int hidled_recv(struct hidled_device *ldev, __u8 
*buf)
 
mutex_lock(>lock);
 
-   ret = hid_hw_raw_request(ldev->hdev, buf

Re: [PATCH] hid: hid-led: fix issue with transfer buffer not being dma capable

[Heiner Kallweit]

Am 03.10.2016 um 20:22 schrieb Alan Stern:
> On Mon, 3 Oct 2016, Heiner Kallweit wrote:
> 
>> The hid-led driver works fine under 4.8.0, however with the next
>> kernel from today I get this:
>>
>> [ cut here ]
>> WARNING: CPU: 0 PID: 2578 at drivers/usb/core/hcd.c:1584 
>> usb_hcd_map_urb_for_dma+0x373/0x550 [usbcore]
>> transfer buffer not dma capable
>> Modules linked in: hid_led(+) usbhid vfat fat ir_sony_decoder iwlmvm 
>> led_class mac80211 snd_hda_codec_realtek snd_hda_codec_generic 
>> x86_pkg_temp_thermal iwlwifi crc32c_intel snd_hda_codec_hdmi i2c_i801 
>> i2c_smbus snd_hda_intel cfg80211 snd_hda_codec snd_hda_core snd_pcm r8169 
>> snd_timer mei_me mii snd mei ir_lirc_codec lirc_dev nuvoton_cir rc_core 
>> btusb btintel bluetooth rfkill usb_storage efivarfs ipv6 ehci_pci ehci_hcd 
>> xhci_pci xhci_hcd usbcore usb_common ext4 jbd2 mbcache ahci libahci libata
>> CPU: 0 PID: 2578 Comm: systemd-udevd Not tainted 4.8.0-rc8-next-20161003 #1
>> Hardware name: ZOTAC ZBOX-CI321NANO/ZBOX-CI321NANO, BIOS B246P105 06/01/2015
>>  c90003dbb7e0 81280425 c90003dbb830 
>>  c90003dbb820 8105b086 063003dbb800 88006f374480
>>    0001 880079544000
>> Call Trace:
>>  [] dump_stack+0x68/0x93
>>  [] __warn+0xc6/0xe0
>>  [] warn_slowpath_fmt+0x4a/0x50
>>  [] usb_hcd_map_urb_for_dma+0x373/0x550 [usbcore]
>>  [] usb_hcd_submit_urb+0x316/0x9c0 [usbcore]
>>  [] ? rcu_read_lock_sched_held+0x40/0x80
>>  [] ? module_assert_mutex_or_preempt+0x13/0x50
>>  [] ? __module_address+0x27/0xf0
>>  [] usb_submit_urb+0x2c4/0x520 [usbcore]
>>  [] usb_start_wait_urb+0x5a/0xe0 [usbcore]
>>  [] usb_control_msg+0xbc/0xf0 [usbcore]
>>  [] ? __module_address+0x27/0xf0
>>  [] usbhid_raw_request+0xa4/0x180 [usbhid]
>>  [] hidled_recv+0x71/0xe0 [hid_led]
>>  [] thingm_init+0x2d/0x50 [hid_led]
> 
> ...
> 
>> Fix this by using a kmalloc'ed buffer when calling hid_hw_raw_request.
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>> ---
>>  drivers/hid/hid-led.c | 20 ++--
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hid/hid-led.c b/drivers/hid/hid-led.c
>> index d8d55f3..6c2b8e0 100644
>> --- a/drivers/hid/hid-led.c
>> +++ b/drivers/hid/hid-led.c
>> @@ -67,6 +67,8 @@ union delcom_packet {
>>  #define DELCOM_RED_LED  1
>>  #define DELCOM_BLUE_LED 2
>>  
>> +#define MAX_REPORT_SIZE 16
>> +
>>  struct hidled_device;
>>  struct hidled_rgb;
>>  
>> @@ -100,11 +102,10 @@ struct hidled_device {
>>  const struct hidled_config *config;
>>  struct hid_device   *hdev;
>>  struct hidled_rgb   *rgb;
>> +u8  buf[MAX_REPORT_SIZE];
>>  struct mutexlock;
>>  };
>>  
>> -#define MAX_REPORT_SIZE 16
>> -
>>  #define to_hidled_led(arg) container_of(arg, struct hidled_led, cdev)
>>  
>>  static bool riso_kagaku_switch_green_blue;
>> @@ -118,13 +119,16 @@ static int hidled_send(struct hidled_device *ldev, 
>> __u8 *buf)
>>  
>>  mutex_lock(>lock);
>>  
>> +/* buffer provided to hid_hw_raw_request must not be on the stack */
>> +memcpy(ldev->buf, buf, ldev->config->report_size);
> 
> In fact, this is not the best way to solve the problem.  Not only must 
> the buffer be dynamically allocated, it also must not be part of a 
> larger structure (such as struct hidled_device).
> 
> Instead you should change the routines that call hidled_recv and 
> hidled_send.  Make each of them allocate a buffer using kmalloc rather 
> than use a buffer on the stack.
> 
I see, thanks for the hint. I'll provide a v2.

> Alan Stern
> 
> 

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

[PATCH] hid: hid-led: fix issue with transfer buffer not being dma capable

[Heiner Kallweit]

The hid-led driver works fine under 4.8.0, however with the next
kernel from today I get this:

[ cut here ]
WARNING: CPU: 0 PID: 2578 at drivers/usb/core/hcd.c:1584 
usb_hcd_map_urb_for_dma+0x373/0x550 [usbcore]
transfer buffer not dma capable
Modules linked in: hid_led(+) usbhid vfat fat ir_sony_decoder iwlmvm led_class 
mac80211 snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal 
iwlwifi crc32c_intel snd_hda_codec_hdmi i2c_i801 i2c_smbus snd_hda_intel 
cfg80211 snd_hda_codec snd_hda_core snd_pcm r8169 snd_timer mei_me mii snd mei 
ir_lirc_codec lirc_dev nuvoton_cir rc_core btusb btintel bluetooth rfkill 
usb_storage efivarfs ipv6 ehci_pci ehci_hcd xhci_pci xhci_hcd usbcore 
usb_common ext4 jbd2 mbcache ahci libahci libata
CPU: 0 PID: 2578 Comm: systemd-udevd Not tainted 4.8.0-rc8-next-20161003 #1
Hardware name: ZOTAC ZBOX-CI321NANO/ZBOX-CI321NANO, BIOS B246P105 06/01/2015
 c90003dbb7e0 81280425 c90003dbb830 
 c90003dbb820 8105b086 063003dbb800 88006f374480
   0001 880079544000
Call Trace:
 [] dump_stack+0x68/0x93
 [] __warn+0xc6/0xe0
 [] warn_slowpath_fmt+0x4a/0x50
 [] usb_hcd_map_urb_for_dma+0x373/0x550 [usbcore]
 [] usb_hcd_submit_urb+0x316/0x9c0 [usbcore]
 [] ? rcu_read_lock_sched_held+0x40/0x80
 [] ? module_assert_mutex_or_preempt+0x13/0x50
 [] ? __module_address+0x27/0xf0
 [] usb_submit_urb+0x2c4/0x520 [usbcore]
 [] usb_start_wait_urb+0x5a/0xe0 [usbcore]
 [] usb_control_msg+0xbc/0xf0 [usbcore]
 [] ? __module_address+0x27/0xf0
 [] usbhid_raw_request+0xa4/0x180 [usbhid]
 [] hidled_recv+0x71/0xe0 [hid_led]
 [] thingm_init+0x2d/0x50 [hid_led]
 [] hidled_probe+0xcb/0x24a [hid_led]
 [] hid_device_probe+0xd2/0x150
 [] driver_probe_device+0x1fd/0x2c0
 [] __driver_attach+0x9a/0xa0
 [] ? driver_probe_device+0x2c0/0x2c0
 [] bus_for_each_dev+0x5d/0x90
 [] driver_attach+0x19/0x20
 [] bus_add_driver+0x11f/0x220
 [] ? 0xa07ac000
 [] driver_register+0x5b/0xd0
 [] ? 0xa07ac000
 [] __hid_register_driver+0x61/0xa0
 [] hidled_driver_init+0x1e/0x20 [hid_led]
 [] do_one_initcall+0x38/0x150
 [] ? rcu_read_lock_sched_held+0x40/0x80
 [] ? kmem_cache_alloc_trace+0x1d0/0x230
 [] do_init_module+0x5a/0x1cb
 [] load_module+0x1e42/0x2530
 [] ? __symbol_put+0x50/0x50
 [] ? show_coresize+0x30/0x30
 [] ? kernel_read_file+0x100/0x190
 [] ? kernel_read_file_from_fd+0x44/0x70
 [] SYSC_finit_module+0xba/0xc0
 [] SyS_finit_module+0x9/0x10
 [] entry_SYSCALL_64_fastpath+0x18/0xad
---[ end trace c9e6ea27003ecf9e ]---

Fix this by using a kmalloc'ed buffer when calling hid_hw_raw_request.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/hid/hid-led.c | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-led.c b/drivers/hid/hid-led.c
index d8d55f3..6c2b8e0 100644
--- a/drivers/hid/hid-led.c
+++ b/drivers/hid/hid-led.c
@@ -67,6 +67,8 @@ union delcom_packet {
 #define DELCOM_RED_LED 1
 #define DELCOM_BLUE_LED2
 
+#define MAX_REPORT_SIZE16
+
 struct hidled_device;
 struct hidled_rgb;
 
@@ -100,11 +102,10 @@ struct hidled_device {
const struct hidled_config *config;
struct hid_device   *hdev;
struct hidled_rgb   *rgb;
+   u8  buf[MAX_REPORT_SIZE];
struct mutexlock;
 };
 
-#define MAX_REPORT_SIZE16
-
 #define to_hidled_led(arg) container_of(arg, struct hidled_led, cdev)
 
 static bool riso_kagaku_switch_green_blue;
@@ -118,13 +119,16 @@ static int hidled_send(struct hidled_device *ldev, __u8 
*buf)
 
mutex_lock(>lock);
 
+   /* buffer provided to hid_hw_raw_request must not be on the stack */
+   memcpy(ldev->buf, buf, ldev->config->report_size);
+
if (ldev->config->report_type == RAW_REQUEST)
-   ret = hid_hw_raw_request(ldev->hdev, buf[0], buf,
+   ret = hid_hw_raw_request(ldev->hdev, buf[0], ldev->buf,
 ldev->config->report_size,
 HID_FEATURE_REPORT,
 HID_REQ_SET_REPORT);
else if (ldev->config->report_type == OUTPUT_REPORT)
-   ret = hid_hw_output_report(ldev->hdev, buf,
+   ret = hid_hw_output_report(ldev->hdev, ldev->buf,
   ldev->config->report_size);
else
ret = -EINVAL;
@@ -147,17 +151,21 @@ static int hidled_recv(struct hidled_device *ldev, __u8 
*buf)
 
mutex_lock(>lock);
 
-   ret = hid_hw_raw_request(ldev->hdev, buf[0], buf,
+   memcpy(ldev->buf, buf, ldev->config->report_size);
+
+   ret = hid_hw_raw_request(ldev->hdev, buf[0], ldev->buf,
 ldev->config->report_size,
 

Re: [PATCH] usb: storage: fix runtime pm issue in usb_stor_probe2

[Heiner Kallweit]

Am 03.08.2016 um 23:25 schrieb Alan Stern:
> On Wed, 3 Aug 2016, Heiner Kallweit wrote:
> 
>> Since commit 71723f95463d "PM / runtime: print error when activating a
>> child to unactive parent" I see the following error message:
>>
>> scsi host2: usb-storage 1-3:1.0
>> scsi host2: runtime PM trying to activate child device host2 but parent
>>  (1-3:1.0) is not active
>>
>> Digging into it it seems to be related to the problem described in the
>> commit message for cd998ded5c12 "i2c: designware: Prevent runtime
>> suspend during adapter registration" as scsi_add_host also calls
>> device_add and after the call to device_add the parent device is
>> suspended.
>>
>> Fix this by using the approach from the mentioned commit and getting
>> the runtime pm reference before calling scsi_add_host.
> 
> Acked-by: Alan Stern <st...@rowland.harvard.edu>
> 
> There's nothing wrong with this patch; it's a worthwhile thing to do 
> because it can prevent an unnecessary runtime-suspend/resume cycle.
> 
> Still, it looks like the real problem here may lie in
> drivers/scsi/hosts.c.  Commit bc4f24014de5 ("[SCSI] implement runtime
> Power Management") added a call to pm_runtime_set_active() in
> scsi_add_host_with_dma() _after_ device_add() instead of _before_.
> 
> If that were changed, this problem would not have occurred.
> 
In parallel to this patch I sent another one to address exactly the
ordering issue in scsi_add_host_with_dma you mention.
The other patch went to Martin and the SCSI mailing list only, sorry.
I'll forward it to you for review.

Heiner

> Alan Stern
> 
> 

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

[PATCH] usb: storage: fix runtime pm issue in usb_stor_probe2

[Heiner Kallweit]

Since commit 71723f95463d "PM / runtime: print error when activating a
child to unactive parent" I see the following error message:

scsi host2: usb-storage 1-3:1.0
scsi host2: runtime PM trying to activate child device host2 but parent
(1-3:1.0) is not active

Digging into it it seems to be related to the problem described in the
commit message for cd998ded5c12 "i2c: designware: Prevent runtime
suspend during adapter registration" as scsi_add_host also calls
device_add and after the call to device_add the parent device is
suspended.

Fix this by using the approach from the mentioned commit and getting
the runtime pm reference before calling scsi_add_host.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/storage/usb.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index ef2d8cd..8c5f011 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -1070,17 +1070,17 @@ int usb_stor_probe2(struct us_data *us)
result = usb_stor_acquire_resources(us);
if (result)
goto BadDevice;
+   usb_autopm_get_interface_no_resume(us->pusb_intf);
snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s",
dev_name(>pusb_intf->dev));
result = scsi_add_host(us_to_host(us), dev);
if (result) {
dev_warn(dev,
"Unable to add the scsi host\n");
-   goto BadDevice;
+   goto HostAddErr;
}
 
/* Submit the delayed_work for SCSI-device scanning */
-   usb_autopm_get_interface_no_resume(us->pusb_intf);
set_bit(US_FLIDX_SCAN_PENDING, >dflags);
 
if (delay_use > 0)
@@ -1090,6 +1090,8 @@ int usb_stor_probe2(struct us_data *us)
return 0;
 
/* We come here if there are any problems */
+HostAddErr:
+   usb_autopm_put_interface_no_suspend(us->pusb_intf);
 BadDevice:
usb_stor_dbg(us, "storage_probe() failed\n");
release_everything(us);
-- 
2.9.2

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

[PATCH v2 1/2] hid: migrate USB LED driver from usb misc to hid

[Heiner Kallweit]

This patch migrates the USB LED driver to the HID subsystem.
Supported are Dream Cheeky Webmail Notifier / Friends Alert
and Riso Kagaku Webmail Notifier.

Benefits:
- Avoid using USB low-level calls and use the HID subsystem instead
  (as this device provides a USB HID interface)
- Use standard LED subsystem instead of proprietary sysfs entries,
  this allows e.g. to use the device with features like triggers

Successfully tested with a Dream Cheeky Webmail Notifier and a
Riso Kagaku Webmail Notifier compatible device.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- change usbled to hidled
- rename config symbol to HID_LED
- introduce enum hidled_type to be passed in driver_data instead
  of a pointer (facilitating manual driver binding via sysfs)
- rename struct hidled_type to hidled_config to avoid name conflict
- replace dev_info with hid_info
---
 drivers/hid/Kconfig|  12 +++
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   6 +-
 drivers/hid/hid-ids.h  |   2 +
 drivers/hid/hid-led.c  | 288 +
 5 files changed, 306 insertions(+), 3 deletions(-)
 create mode 100644 drivers/hid/hid-led.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 5646ca4..04bd203 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -388,6 +388,18 @@ config HID_LCPOWER
---help---
Support for LC-Power RC1000MCE RF remote control.
 
+config HID_LED
+   tristate "Simple USB RGB LED support"
+   depends on HID
+   depends on LEDS_CLASS
+   ---help---
+   Support for simple USB RGB LED devices. Currently supported are the
+   Riso Kagaku Webmail Notifier and the Dream Cheeky Webmail Notifier
+   and Friends Alert.
+
+   To compile this driver as a module, choose M here: the
+   module will be called hid-led.
+
 config HID_LENOVO
tristate "Lenovo / Thinkpad devices"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index a2fb562..86d71f6 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -96,6 +96,7 @@ obj-$(CONFIG_HID_TIVO)+= hid-tivo.o
 obj-$(CONFIG_HID_TOPSEED)  += hid-topseed.o
 obj-$(CONFIG_HID_TWINHAN)  += hid-twinhan.o
 obj-$(CONFIG_HID_UCLOGIC)  += hid-uclogic.o
+obj-$(CONFIG_HID_LED)  += hid-led.o
 obj-$(CONFIG_HID_XINMO)+= hid-xinmo.o
 obj-$(CONFIG_HID_ZEROPLUS) += hid-zpff.o
 obj-$(CONFIG_HID_ZYDACRON) += hid-zydacron.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8ea3a26..eb674ce 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1879,6 +1879,8 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_MOUSE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_WN) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_FA) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, 
USB_DEVICE_ID_ELECOM_BM084) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELO, 0x0009) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELO, 0x0030) },
@@ -2008,6 +2010,7 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, 
USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS, HID_ANY_ID) },
{ HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
USB_DEVICE_ID_RI_KA_WEBMAIL) },
 #if IS_ENABLED(CONFIG_HID_ROCCAT)
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ARVO) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ISKU) },
@@ -2348,8 +2351,6 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_DEALEXTREAME, 
USB_DEVICE_ID_DEALEXTREAME_RADIO_SI4701) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, 
USB_DEVICE_ID_DELORME_EARTHMATE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EM_LT20) 
},
-   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x0004) },
-   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x000a) },
{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0400) },
{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0401) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ESSENTIAL_REALITY, 
USB_DEVICE_ID_ESSENTIAL_REALITY_P5) },
@@ -2486,7 +2487,6 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_DPAD) 
},
 #endif
{ HID_USB_DEVICE(USB_VENDOR_ID_YEALINK, 
USB_DEVICE_ID_YEALINK_P1K_P4K_B2K) },
-   { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
USB_DEVICE_ID_RI_KA_WEBMAIL) },
{ }
 };
 
diff --git

Re: [PATCH 1/2] hid: migrate USB LED driver from usb misc to hid

[Heiner Kallweit]

Am 17.06.2016 um 10:53 schrieb Benjamin Tissoires:
> Hi Heiner,
> 
> On Jun 17 2016 or thereabouts, Heiner Kallweit wrote:
>> This patch migrates the USB LED driver to the HID subsystem.
>> Supported are Dream Cheeky Webmail Notifier / Friends Alert
>> and Riso Kagaku Webmail Notifier.
>>
>> Benefits:
>> - Avoid using USB low-level calls and use the HID subsystem instead
>>   (as this device provides a USB HID interface)
>> - Use standard LED subsystem instead of proprietary sysfs entries,
>>   this allows e.g. to use the device with features like triggers
>>
>> Successfully tested with a Dream Cheeky Webmail Notifier and a
>> Riso Kagaku Webmail Notifier compatible device.
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
> 
> Looks good, though I have a few comments (sorry, again...)
> 
>> ---
>>  drivers/hid/Kconfig   |  12 ++
>>  drivers/hid/Makefile  |   1 +
>>  drivers/hid/hid-core.c|   6 +-
>>  drivers/hid/hid-ids.h |   2 +
>>  drivers/hid/hid-usb-led.c | 276 
>> ++
> 
> There is nothing USB related now. How about just calling it hid-led.c?
> 
Sounds reasonable ..

>>  5 files changed, 294 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/hid/hid-usb-led.c
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 5646ca4..adc9ce6 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -841,6 +841,18 @@ config THRUSTMASTER_FF
>>a THRUSTMASTER Dual Trigger 3-in-1 or a THRUSTMASTER Ferrari GT
>>Rumble Force or Force Feedback Wheel.
>>  
>> +config HID_USB_LED
>> +tristate "Simple USB RGB LED support"
>> +depends on HID
>> +depends on LEDS_CLASS
>> +---help---
>> +Support for simple USB RGB LED devices. Currently supported are the
>> +Riso Kagaku Webmail Notifier and the Dream Cheeky Webmail Notifier
>> +and Friends Alert.
>> +
>> +To compile this driver as a module, choose M here: the
>> +module will be called hid-usb-led.
>> +
>>  config HID_WACOM
>>  tristate "Wacom Intuos/Graphire tablet support (USB)"
>>  depends on HID
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index a2fb562..3014bdd 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -96,6 +96,7 @@ obj-$(CONFIG_HID_TIVO) += hid-tivo.o
>>  obj-$(CONFIG_HID_TOPSEED)   += hid-topseed.o
>>  obj-$(CONFIG_HID_TWINHAN)   += hid-twinhan.o
>>  obj-$(CONFIG_HID_UCLOGIC)   += hid-uclogic.o
>> +obj-$(CONFIG_HID_USB_LED)   += hid-usb-led.o
>>  obj-$(CONFIG_HID_XINMO) += hid-xinmo.o
>>  obj-$(CONFIG_HID_ZEROPLUS)  += hid-zpff.o
>>  obj-$(CONFIG_HID_ZYDACRON)  += hid-zydacron.o
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 8ea3a26..eb674ce 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1879,6 +1879,8 @@ static const struct hid_device_id 
>> hid_have_special_driver[] = {
>>  { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_MOUSE) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
>> +{ HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
>> USB_DEVICE_ID_DREAM_CHEEKY_WN) },
>> +{ HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
>> USB_DEVICE_ID_DREAM_CHEEKY_FA) },
>>  { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, 
>> USB_DEVICE_ID_ELECOM_BM084) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_ELO, 0x0009) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_ELO, 0x0030) },
>> @@ -2008,6 +2010,7 @@ static const struct hid_device_id 
>> hid_have_special_driver[] = {
>>  { HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, 
>> USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS, HID_ANY_ID) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
>> +{ HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
>> USB_DEVICE_ID_RI_KA_WEBMAIL) },
>>  #if IS_ENABLED(CONFIG_HID_ROCCAT)
>>  { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ARVO) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ISKU) },
>> @@ -2348,8 +2351,6 @@ static const struct hid_device_id hid_ignore_list[] = {
>>  { HID_USB_DEVICE(USB_VENDOR_ID_DEALEXTREAME, 
>> USB_DEVICE_ID_DEALEXTREAME_RADIO_SI4701) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_DELORM

[PATCH 2/2] usb: misc: remove outdated USB LED driver

[Heiner Kallweit]

The USB LED driver exposes a undocumented sysfs interface and doesn't
use the standard kernel LED subsystem. It supports three devices:

Delcom Visual Signal Indicator
The driver supports generation 1 of the device only which was
manufactured until 2008. Remove support for this device completely.

Riso Kagaku RGB LED + Dream Cheeky Webmail Notifier
These devices are HID compliant and are supported by a new USB LED
driver under drivers/hid utilizing the kernel LED subsystem.

So let's remove the old USB LED driver.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
 drivers/usb/misc/Kconfig  |   9 --
 drivers/usb/misc/Makefile |   1 -
 drivers/usb/misc/usbled.c | 273 --
 3 files changed, 283 deletions(-)
 delete mode 100644 drivers/usb/misc/usbled.c

diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index f7a7fc2..e9c5458 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -79,15 +79,6 @@ config USB_LCD
  To compile this driver as a module, choose M here: the
  module will be called usblcd.
 
-config USB_LED
-   tristate "USB LED driver support"
-   help
- Say Y here if you want to connect an USBLED device to your 
- computer's USB port.
-
- To compile this driver as a module, choose M here: the
- module will be called usbled.
-
 config USB_CYPRESS_CY7C63
tristate "Cypress CY7C63xxx USB driver support"
help
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 45fd4ac..4bc755b 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -15,7 +15,6 @@ obj-$(CONFIG_USB_IOWARRIOR)   += iowarrior.o
 obj-$(CONFIG_USB_ISIGHTFW) += isight_firmware.o
 obj-$(CONFIG_USB_LCD)  += usblcd.o
 obj-$(CONFIG_USB_LD)   += ldusb.o
-obj-$(CONFIG_USB_LED)  += usbled.o
 obj-$(CONFIG_USB_LEGOTOWER)+= legousbtower.o
 obj-$(CONFIG_USB_RIO500)   += rio500.o
 obj-$(CONFIG_USB_TEST) += usbtest.o
diff --git a/drivers/usb/misc/usbled.c b/drivers/usb/misc/usbled.c
deleted file mode 100644
index bdef0d6..000
--- a/drivers/usb/misc/usbled.c
+++ /dev/null
@@ -1,273 +0,0 @@
-/*
- * USB LED driver
- *
- * Copyright (C) 2004 Greg Kroah-Hartman (g...@kroah.com)
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation, version 2.
- *
- */
-
-#include 
-#include 
-#include 
-#include 
-#include 
-
-
-#define DRIVER_AUTHOR "Greg Kroah-Hartman, g...@kroah.com"
-#define DRIVER_DESC "USB LED Driver"
-
-enum led_type {
-   DELCOM_VISUAL_SIGNAL_INDICATOR,
-   DREAM_CHEEKY_WEBMAIL_NOTIFIER,
-   RISO_KAGAKU_LED
-};
-
-/* the Webmail LED made by RISO KAGAKU CORP. decodes a color index
-   internally, we want to keep the red+green+blue sysfs api, so we decode
-   from 1-bit RGB to the riso kagaku color index according to this table... */
-
-static unsigned const char riso_kagaku_tbl[] = {
-/* R+2G+4B -> riso kagaku color index */
-   [0] = 0, /* black   */
-   [1] = 2, /* red */
-   [2] = 1, /* green   */
-   [3] = 5, /* yellow  */
-   [4] = 3, /* blue*/
-   [5] = 6, /* magenta */
-   [6] = 4, /* cyan*/
-   [7] = 7  /* white   */
-};
-
-#define RISO_KAGAKU_IX(r,g,b) riso_kagaku_tbl[((r)?1:0)+((g)?2:0)+((b)?4:0)]
-
-/* table of devices that work with this driver */
-static const struct usb_device_id id_table[] = {
-   { USB_DEVICE(0x0fc5, 0x1223),
-   .driver_info = DELCOM_VISUAL_SIGNAL_INDICATOR },
-   { USB_DEVICE(0x1d34, 0x0004),
-   .driver_info = DREAM_CHEEKY_WEBMAIL_NOTIFIER },
-   { USB_DEVICE(0x1d34, 0x000a),
-   .driver_info = DREAM_CHEEKY_WEBMAIL_NOTIFIER },
-   { USB_DEVICE(0x1294, 0x1320),
-   .driver_info = RISO_KAGAKU_LED },
-   { },
-};
-MODULE_DEVICE_TABLE(usb, id_table);
-
-struct usb_led {
-   struct usb_device   *udev;
-   unsigned char   blue;
-   unsigned char   red;
-   unsigned char   green;
-   enum led_type   type;
-};
-
-static void change_color(struct usb_led *led)
-{
-   int retval = 0;
-   unsigned char *buffer;
-   int actlength;
-
-   buffer = kmalloc(8, GFP_KERNEL);
-   if (!buffer) {
-   dev_err(>udev->dev, "out of memory\n");
-   return;
-   }
-
-   switch (led->type) {
-   case DELCOM_VISUAL_SIGNAL_INDICATOR: {
-   unsigned char color = 0x07;
-
-   if (led->blue)
-   color &= ~0x04;
-   if (led->red)
-   co

Re: [PATCH] usb: misc: remove outdated USB LED driver

[Heiner Kallweit]

Am 16.06.2016 um 21:13 schrieb Greg Kroah-Hartman:
> On Thu, Jun 16, 2016 at 09:00:50PM +0200, Heiner Kallweit wrote:
>> The USB LED driver exposes a undocumented sysfs interface and doesn't
>> use the standard kernel LED subsystem. It supports three devices:
>>
>> Delcom Visual Signal Indicator
>> The driver supports generation 1 of the device only which was
>> manufactured until 2008. Remove support for this device completely.
>>
>> Riso Kagaku RGB LED + Dream Cheeky Webmail Notifier
>> These devices are HID compliant and will be supported by a new driver
>> under drivers/hid utilizing the kernel LED subsystem.
>>
>> So let's remove the USB LED driver.
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
> 
> Has the HID driver been merged yet?  I'd like to have this go in as the
> 2nd patch in that series, so feel free to add my:
> 
Yes, I just submitted the patch set.

Regards, Heiner

> Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> 
> to this one.
> 
> thanks,
> 
> greg k-h
> 

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

[PATCH 1/2] hid: migrate USB LED driver from usb misc to hid

[Heiner Kallweit]

This patch migrates the USB LED driver to the HID subsystem.
Supported are Dream Cheeky Webmail Notifier / Friends Alert
and Riso Kagaku Webmail Notifier.

Benefits:
- Avoid using USB low-level calls and use the HID subsystem instead
  (as this device provides a USB HID interface)
- Use standard LED subsystem instead of proprietary sysfs entries,
  this allows e.g. to use the device with features like triggers

Successfully tested with a Dream Cheeky Webmail Notifier and a
Riso Kagaku Webmail Notifier compatible device.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/hid/Kconfig   |  12 ++
 drivers/hid/Makefile  |   1 +
 drivers/hid/hid-core.c|   6 +-
 drivers/hid/hid-ids.h |   2 +
 drivers/hid/hid-usb-led.c | 276 ++
 5 files changed, 294 insertions(+), 3 deletions(-)
 create mode 100644 drivers/hid/hid-usb-led.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 5646ca4..adc9ce6 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -841,6 +841,18 @@ config THRUSTMASTER_FF
  a THRUSTMASTER Dual Trigger 3-in-1 or a THRUSTMASTER Ferrari GT
  Rumble Force or Force Feedback Wheel.
 
+config HID_USB_LED
+   tristate "Simple USB RGB LED support"
+   depends on HID
+   depends on LEDS_CLASS
+   ---help---
+   Support for simple USB RGB LED devices. Currently supported are the
+   Riso Kagaku Webmail Notifier and the Dream Cheeky Webmail Notifier
+   and Friends Alert.
+
+   To compile this driver as a module, choose M here: the
+   module will be called hid-usb-led.
+
 config HID_WACOM
tristate "Wacom Intuos/Graphire tablet support (USB)"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index a2fb562..3014bdd 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -96,6 +96,7 @@ obj-$(CONFIG_HID_TIVO)+= hid-tivo.o
 obj-$(CONFIG_HID_TOPSEED)  += hid-topseed.o
 obj-$(CONFIG_HID_TWINHAN)  += hid-twinhan.o
 obj-$(CONFIG_HID_UCLOGIC)  += hid-uclogic.o
+obj-$(CONFIG_HID_USB_LED)  += hid-usb-led.o
 obj-$(CONFIG_HID_XINMO)+= hid-xinmo.o
 obj-$(CONFIG_HID_ZEROPLUS) += hid-zpff.o
 obj-$(CONFIG_HID_ZYDACRON) += hid-zydacron.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8ea3a26..eb674ce 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1879,6 +1879,8 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_MOUSE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_WN) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_FA) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, 
USB_DEVICE_ID_ELECOM_BM084) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELO, 0x0009) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELO, 0x0030) },
@@ -2008,6 +2010,7 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, 
USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS, HID_ANY_ID) },
{ HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
USB_DEVICE_ID_RI_KA_WEBMAIL) },
 #if IS_ENABLED(CONFIG_HID_ROCCAT)
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ARVO) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ISKU) },
@@ -2348,8 +2351,6 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_DEALEXTREAME, 
USB_DEVICE_ID_DEALEXTREAME_RADIO_SI4701) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, 
USB_DEVICE_ID_DELORME_EARTHMATE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EM_LT20) 
},
-   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x0004) },
-   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x000a) },
{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0400) },
{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0401) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ESSENTIAL_REALITY, 
USB_DEVICE_ID_ESSENTIAL_REALITY_P5) },
@@ -2486,7 +2487,6 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_DPAD) 
},
 #endif
{ HID_USB_DEVICE(USB_VENDOR_ID_YEALINK, 
USB_DEVICE_ID_YEALINK_P1K_P4K_B2K) },
-   { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
USB_DEVICE_ID_RI_KA_WEBMAIL) },
{ }
 };
 
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 3eec09a..e104aba 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -334,6 +334,8 @@
 #define USB_DEVICE_ID_ELECOM_B

Re: [PATCH v2] hid: migrate Riso Kagaku LED driver from USB misc to HID

[Heiner Kallweit]

Am 16.06.2016 um 09:08 schrieb Greg Kroah-Hartman:
> On Thu, Jun 16, 2016 at 07:45:35AM +0200, Heiner Kallweit wrote:
>> Am 15.06.2016 um 22:59 schrieb Heiner Kallweit:
>>> Am 15.06.2016 um 09:31 schrieb Benjamin Tissoires:
>>>> On Jun 15 2016 or thereabouts, Heiner Kallweit wrote:
>>>>> Am 14.06.2016 um 23:49 schrieb Benjamin Tissoires:
>>>>>> On Jun 12 2016 or thereabouts, Heiner Kallweit wrote:
>>>>>>> The Riso Kagaku Webmail Notifier (and its clones) is supported as part 
>>>>>>> of
>>>>>>> usb/misc/usbled driver currently. This patch migrates the driver for 
>>>>>>> this
>>>>>>> device to the HID subsystem.
>>>>>>>
>>>>>>> Benefits:
>>>>>>> - Avoid using USB low-level calls and use the HID subsystem instead
>>>>>>>   (as this device provides a USB HID interface)
>>>>>>> - Use standard LED subsystem instead of proprietary sysfs entries,
>>>>>>>   this allows e.g. to use the device with features like triggers
>>>>>>>
>>>>>>> I know at least one cheap clone coming with green and blue LED switched.
>>>>>>> There's a module paramater to deal with this.
>>>>>>>
>>>>>>> There might be users of the current module, therefore so far allow
>>>>>>> compilation of the new driver only if the old one is disabled.
>>>>>>>
>>>>>>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> - change config symbol from HID_RIKA to HID_RISO_KAGAKU
>>>>>>> - use full name Riso Kagaku instead of rika in several places
>>>>>>> - don't remove device from ignore list if CONFIG_USB_LED is defined
>>>>>>> - fix module parameter permissions
>>>>>>
>>>>>> Thanks for the respin. This version (and the other for hid-rika) looks
>>>>>> fine. Though I wonder if you should not call hid_hw_open() at the end of
>>>>>> probe (and hid_hw_close() during remove) to keep the device opened.
>>>>>> Otherwise I am unsure whether your usb commands will get treated (I
>>>>>> might be wrong).
>>>>>>
>>>>> When looking at other hid device drivers hid_hw_open seems to be needed
>>>>> only if the device needs special treatment.
>>>>
>>>> Not exactly. hid_hw_open is called whenever someone opens the hidraw or
>>>> input node (or any other resource attached to the HID device). This
>>>> starts the input reading, but also calls usb_autopm_get_interface()
>>>> which will prevent the device to be autosuspended. The drivers that call
>>>> hid_hw_open() in their probe are the ones where we know no one will open
>>>> the hidraw or input node but we need to poll for events.
>>>>
>>>>> My Riso Kagaku and Dream Cheeky devices work fine with the new drivers
>>>>> w/o calling hid_hw_open.
>>>>
>>>> Let's keep it that way then.
>>>>
>>>>>
>>>>>> There is also one high level comment I would like to do:
>>>>>> even if your driver is better than usbled.c, people won't use it because
>>>>>> distributions won't enable it as there is 2 issues:
>>>>>> - one is you are breaking kernel ABI (we already discussed it)
>>>>>> - the other one is that if people want to use your new hid drivers, they
>>>>>>   have to disable CONFIG_USB_LED, which removes support of the
>>>>>>   DELCOM_VISUAL_SIGNAL_INDICATOR
>>>>>>
>>>>> Yes, I think you're right.
>>>>> With regard to the Delcom Visual Signal Indicator:
>>>>> The old USB LED driver supports only generation 1 of these devices which
>>>>> have a not fully HID compliant interface.
>>>>> In 2008 they were replace with generation 2 which has fully a HID
>>>>> compliant interface. Generation 2 is not yet supported by the kernel.
>>>>> (Currently I'm trying to get hold of such a device.)
>>>>>
>>>>> Therefore it might be better to leave support for the Gen 1 Delcom device
>>>>> at USB misc. However this would mean that we have to change the config
>>>>> symbol for this driver so that it 

Re: [PATCH v2] hid: migrate Riso Kagaku LED driver from USB misc to HID

[Heiner Kallweit]

Am 15.06.2016 um 22:59 schrieb Heiner Kallweit:
> Am 15.06.2016 um 09:31 schrieb Benjamin Tissoires:
>> On Jun 15 2016 or thereabouts, Heiner Kallweit wrote:
>>> Am 14.06.2016 um 23:49 schrieb Benjamin Tissoires:
>>>> On Jun 12 2016 or thereabouts, Heiner Kallweit wrote:
>>>>> The Riso Kagaku Webmail Notifier (and its clones) is supported as part of
>>>>> usb/misc/usbled driver currently. This patch migrates the driver for this
>>>>> device to the HID subsystem.
>>>>>
>>>>> Benefits:
>>>>> - Avoid using USB low-level calls and use the HID subsystem instead
>>>>>   (as this device provides a USB HID interface)
>>>>> - Use standard LED subsystem instead of proprietary sysfs entries,
>>>>>   this allows e.g. to use the device with features like triggers
>>>>>
>>>>> I know at least one cheap clone coming with green and blue LED switched.
>>>>> There's a module paramater to deal with this.
>>>>>
>>>>> There might be users of the current module, therefore so far allow
>>>>> compilation of the new driver only if the old one is disabled.
>>>>>
>>>>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>>>>> ---
>>>>> v2:
>>>>> - change config symbol from HID_RIKA to HID_RISO_KAGAKU
>>>>> - use full name Riso Kagaku instead of rika in several places
>>>>> - don't remove device from ignore list if CONFIG_USB_LED is defined
>>>>> - fix module parameter permissions
>>>>
>>>> Thanks for the respin. This version (and the other for hid-rika) looks
>>>> fine. Though I wonder if you should not call hid_hw_open() at the end of
>>>> probe (and hid_hw_close() during remove) to keep the device opened.
>>>> Otherwise I am unsure whether your usb commands will get treated (I
>>>> might be wrong).
>>>>
>>> When looking at other hid device drivers hid_hw_open seems to be needed
>>> only if the device needs special treatment.
>>
>> Not exactly. hid_hw_open is called whenever someone opens the hidraw or
>> input node (or any other resource attached to the HID device). This
>> starts the input reading, but also calls usb_autopm_get_interface()
>> which will prevent the device to be autosuspended. The drivers that call
>> hid_hw_open() in their probe are the ones where we know no one will open
>> the hidraw or input node but we need to poll for events.
>>
>>> My Riso Kagaku and Dream Cheeky devices work fine with the new drivers
>>> w/o calling hid_hw_open.
>>
>> Let's keep it that way then.
>>
>>>
>>>> There is also one high level comment I would like to do:
>>>> even if your driver is better than usbled.c, people won't use it because
>>>> distributions won't enable it as there is 2 issues:
>>>> - one is you are breaking kernel ABI (we already discussed it)
>>>> - the other one is that if people want to use your new hid drivers, they
>>>>   have to disable CONFIG_USB_LED, which removes support of the
>>>>   DELCOM_VISUAL_SIGNAL_INDICATOR
>>>>
>>> Yes, I think you're right.
>>> With regard to the Delcom Visual Signal Indicator:
>>> The old USB LED driver supports only generation 1 of these devices which
>>> have a not fully HID compliant interface.
>>> In 2008 they were replace with generation 2 which has fully a HID
>>> compliant interface. Generation 2 is not yet supported by the kernel.
>>> (Currently I'm trying to get hold of such a device.)
>>>
>>> Therefore it might be better to leave support for the Gen 1 Delcom device
>>> at USB misc. However this would mean that we have to change the config
>>> symbol for this driver so that it doesn't conflict with the other part
>>> we're moving to hid.
>>> Would changing the config symbol be an issue? For now we could select
>>> the new config symbol automatically if CONFIG_USB_LED is set.
>>
>> Why would you change the old config symbol for the existing devices. If
>> (and I don't think that's the best option) you were to add a new symbol,
>> you could use this to select between HID and USB for the new drivers:
>> CONFIG_USB_LED_LEGACY_SUPPORT would depend on CONFIG_USB_LED, and you
>> choose to enable the HID driver based on CONFIG_USB_LED_LEGACY_SUPPORT.
>>
>>>
>>>> I really think it would not cost too much to do the whole change in 2
&g

Re: [PATCH v2] hid: migrate Riso Kagaku LED driver from USB misc to HID

[Heiner Kallweit]

Am 15.06.2016 um 09:31 schrieb Benjamin Tissoires:
> On Jun 15 2016 or thereabouts, Heiner Kallweit wrote:
>> Am 14.06.2016 um 23:49 schrieb Benjamin Tissoires:
>>> On Jun 12 2016 or thereabouts, Heiner Kallweit wrote:
>>>> The Riso Kagaku Webmail Notifier (and its clones) is supported as part of
>>>> usb/misc/usbled driver currently. This patch migrates the driver for this
>>>> device to the HID subsystem.
>>>>
>>>> Benefits:
>>>> - Avoid using USB low-level calls and use the HID subsystem instead
>>>>   (as this device provides a USB HID interface)
>>>> - Use standard LED subsystem instead of proprietary sysfs entries,
>>>>   this allows e.g. to use the device with features like triggers
>>>>
>>>> I know at least one cheap clone coming with green and blue LED switched.
>>>> There's a module paramater to deal with this.
>>>>
>>>> There might be users of the current module, therefore so far allow
>>>> compilation of the new driver only if the old one is disabled.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>>>> ---
>>>> v2:
>>>> - change config symbol from HID_RIKA to HID_RISO_KAGAKU
>>>> - use full name Riso Kagaku instead of rika in several places
>>>> - don't remove device from ignore list if CONFIG_USB_LED is defined
>>>> - fix module parameter permissions
>>>
>>> Thanks for the respin. This version (and the other for hid-rika) looks
>>> fine. Though I wonder if you should not call hid_hw_open() at the end of
>>> probe (and hid_hw_close() during remove) to keep the device opened.
>>> Otherwise I am unsure whether your usb commands will get treated (I
>>> might be wrong).
>>>
>> When looking at other hid device drivers hid_hw_open seems to be needed
>> only if the device needs special treatment.
> 
> Not exactly. hid_hw_open is called whenever someone opens the hidraw or
> input node (or any other resource attached to the HID device). This
> starts the input reading, but also calls usb_autopm_get_interface()
> which will prevent the device to be autosuspended. The drivers that call
> hid_hw_open() in their probe are the ones where we know no one will open
> the hidraw or input node but we need to poll for events.
> 
>> My Riso Kagaku and Dream Cheeky devices work fine with the new drivers
>> w/o calling hid_hw_open.
> 
> Let's keep it that way then.
> 
>>
>>> There is also one high level comment I would like to do:
>>> even if your driver is better than usbled.c, people won't use it because
>>> distributions won't enable it as there is 2 issues:
>>> - one is you are breaking kernel ABI (we already discussed it)
>>> - the other one is that if people want to use your new hid drivers, they
>>>   have to disable CONFIG_USB_LED, which removes support of the
>>>   DELCOM_VISUAL_SIGNAL_INDICATOR
>>>
>> Yes, I think you're right.
>> With regard to the Delcom Visual Signal Indicator:
>> The old USB LED driver supports only generation 1 of these devices which
>> have a not fully HID compliant interface.
>> In 2008 they were replace with generation 2 which has fully a HID
>> compliant interface. Generation 2 is not yet supported by the kernel.
>> (Currently I'm trying to get hold of such a device.)
>>
>> Therefore it might be better to leave support for the Gen 1 Delcom device
>> at USB misc. However this would mean that we have to change the config
>> symbol for this driver so that it doesn't conflict with the other part
>> we're moving to hid.
>> Would changing the config symbol be an issue? For now we could select
>> the new config symbol automatically if CONFIG_USB_LED is set.
> 
> Why would you change the old config symbol for the existing devices. If
> (and I don't think that's the best option) you were to add a new symbol,
> you could use this to select between HID and USB for the new drivers:
> CONFIG_USB_LED_LEGACY_SUPPORT would depend on CONFIG_USB_LED, and you
> choose to enable the HID driver based on CONFIG_USB_LED_LEGACY_SUPPORT.
> 
>>
>>> I really think it would not cost too much to do the whole change in 2
>>> passes:
>>> - move the entire usbled to hid, keeping the old sysfs API in place, and
>>>   eventually add a symlink from the old place (under the usb device) to
>>>   the HID device to keep path stable
>>
>> I guess you don't mean symlink literally but adding a comment in Kconfig
>> that the driver was moved?
&g

Re: [PATCH v2] hid: migrate Riso Kagaku LED driver from USB misc to HID

[Heiner Kallweit]

Am 14.06.2016 um 23:49 schrieb Benjamin Tissoires:
> On Jun 12 2016 or thereabouts, Heiner Kallweit wrote:
>> The Riso Kagaku Webmail Notifier (and its clones) is supported as part of
>> usb/misc/usbled driver currently. This patch migrates the driver for this
>> device to the HID subsystem.
>>
>> Benefits:
>> - Avoid using USB low-level calls and use the HID subsystem instead
>>   (as this device provides a USB HID interface)
>> - Use standard LED subsystem instead of proprietary sysfs entries,
>>   this allows e.g. to use the device with features like triggers
>>
>> I know at least one cheap clone coming with green and blue LED switched.
>> There's a module paramater to deal with this.
>>
>> There might be users of the current module, therefore so far allow
>> compilation of the new driver only if the old one is disabled.
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>> ---
>> v2:
>> - change config symbol from HID_RIKA to HID_RISO_KAGAKU
>> - use full name Riso Kagaku instead of rika in several places
>> - don't remove device from ignore list if CONFIG_USB_LED is defined
>> - fix module parameter permissions
> 
> Thanks for the respin. This version (and the other for hid-rika) looks
> fine. Though I wonder if you should not call hid_hw_open() at the end of
> probe (and hid_hw_close() during remove) to keep the device opened.
> Otherwise I am unsure whether your usb commands will get treated (I
> might be wrong).
> 
When looking at other hid device drivers hid_hw_open seems to be needed
only if the device needs special treatment.
My Riso Kagaku and Dream Cheeky devices work fine with the new drivers
w/o calling hid_hw_open.

> There is also one high level comment I would like to do:
> even if your driver is better than usbled.c, people won't use it because
> distributions won't enable it as there is 2 issues:
> - one is you are breaking kernel ABI (we already discussed it)
> - the other one is that if people want to use your new hid drivers, they
>   have to disable CONFIG_USB_LED, which removes support of the
>   DELCOM_VISUAL_SIGNAL_INDICATOR
> 
Yes, I think you're right.
With regard to the Delcom Visual Signal Indicator:
The old USB LED driver supports only generation 1 of these devices which
have a not fully HID compliant interface.
In 2008 they were replace with generation 2 which has fully a HID
compliant interface. Generation 2 is not yet supported by the kernel.
(Currently I'm trying to get hold of such a device.)

Therefore it might be better to leave support for the Gen 1 Delcom device
at USB misc. However this would mean that we have to change the config
symbol for this driver so that it doesn't conflict with the other part
we're moving to hid.
Would changing the config symbol be an issue? For now we could select
the new config symbol automatically if CONFIG_USB_LED is set.

> I really think it would not cost too much to do the whole change in 2
> passes:
> - move the entire usbled to hid, keeping the old sysfs API in place, and
>   eventually add a symlink from the old place (under the usb device) to
>   the HID device to keep path stable

I guess you don't mean symlink literally but adding a comment in Kconfig
that the driver was moved?

> - then add support for the classdev for the whole 3 types of devices
> 
> Plus that would means only one driver to maintain instead of 3 that are
> close enough.
> 
> Cheers,
> Benjamin
> 
Regards, Heiner

>> ---
>>  drivers/hid/Kconfig   |   9 +++
>>  drivers/hid/Makefile  |   1 +
>>  drivers/hid/hid-core.c|   3 +
>>  drivers/hid/hid-riso-kagaku.c | 171 
>> ++
>>  4 files changed, 184 insertions(+)
>>  create mode 100644 drivers/hid/hid-riso-kagaku.c
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 5646ca4..16fe27d 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -784,6 +784,15 @@ config HID_HYPERV_MOUSE
>>  ---help---
>>  Select this option to enable the Hyper-V mouse driver.
>>  
>> +config HID_RISO_KAGAKU
>> +tristate "Riso Kagaku Webmail Notifier USB LED"
>> +depends on HID
>> +depends on LEDS_CLASS
>> +depends on USB_LED = 'n'
>> +---help---
>> +Support for the Riso Kagaku Webmail Notifier. This driver registers
>> +a LED class instance for red, green, and blue color component.
>> +
>>  config HID_SMARTJOYPLUS
>>  tristate "SmartJoy PLUS PS2/USB adapter support"
>>  depends on HID
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index a2fb562

[PATCH v2] hid: migrate Dream Cheeky LED driver from USB misc to HID

[Heiner Kallweit]

The Dream Cheeky Webmail Notifier and Friends Alert are supported as part
of usb/misc/usbled driver currently. This patch migrates the driver for
these devices to the HID subsystem.

Benefits:
- Avoid using USB low-level calls and use the HID subsystem instead
  (as this device provides a USB HID interface)
- Use standard LED subsystem instead of proprietary sysfs entries,
  this allows e.g. to use the device with features like triggers

There might be users of the current driver, therefore so far allow
compilation of the new driver only if the old one is disabled.

Successfully tested with a Dream Cheeky Webmail Notifier.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- move hid_hw_start after dc_init_device to avoid a potential race
---
 drivers/hid/Kconfig|   8 ++
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   6 +-
 drivers/hid/hid-dream-cheeky.c | 175 +
 drivers/hid/hid-ids.h  |   2 +
 5 files changed, 190 insertions(+), 2 deletions(-)
 create mode 100644 drivers/hid/hid-dream-cheeky.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 16fe27d..5618a61 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -247,6 +247,14 @@ config DRAGONRISE_FF
Say Y here if you want to enable force feedback support for DragonRise 
Inc.
game controllers.
 
+config HID_DREAM_CHEEKY
+   tristate "Dream Cheeky Webmail Notifier USB RGB LED"
+   depends on HID
+   depends on USB_LED = 'n'
+   ---help---
+   Support for the Dream Cheeky Webmail Notifier. This driver registers
+   a LED class instance for red, green, and blue color component.
+
 config HID_EMS_FF
tristate "EMS Production Inc. force feedback support"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 90300f3..2878739 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_HID_CORSAIR) += hid-corsair.o
 obj-$(CONFIG_HID_CP2112)   += hid-cp2112.o
 obj-$(CONFIG_HID_CYPRESS)  += hid-cypress.o
 obj-$(CONFIG_HID_DRAGONRISE)   += hid-dr.o
+obj-$(CONFIG_HID_DREAM_CHEEKY) += hid-dream-cheeky.o
 obj-$(CONFIG_HID_EMS_FF)   += hid-emsff.o
 obj-$(CONFIG_HID_ELECOM)   += hid-elecom.o
 obj-$(CONFIG_HID_ELO)  += hid-elo.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 730589a..c24f758 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1879,6 +1879,8 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_MOUSE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_WN) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_FA) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, 
USB_DEVICE_ID_ELECOM_BM084) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELO, 0x0009) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELO, 0x0030) },
@@ -2349,8 +2351,6 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_DEALEXTREAME, 
USB_DEVICE_ID_DEALEXTREAME_RADIO_SI4701) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, 
USB_DEVICE_ID_DELORME_EARTHMATE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EM_LT20) 
},
-   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x0004) },
-   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x000a) },
{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0400) },
{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0401) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ESSENTIAL_REALITY, 
USB_DEVICE_ID_ESSENTIAL_REALITY_P5) },
@@ -2489,6 +2489,8 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_YEALINK, 
USB_DEVICE_ID_YEALINK_P1K_P4K_B2K) },
 #if IS_ENABLED(CONFIG_USB_LED)
{ HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
USB_DEVICE_ID_RI_KA_WEBMAIL) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_WN) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_FA) },
 #endif
{ }
 };
diff --git a/drivers/hid/hid-dream-cheeky.c b/drivers/hid/hid-dream-cheeky.c
new file mode 100644
index 000..9806de4
--- /dev/null
+++ b/drivers/hid/hid-dream-cheeky.c
@@ -0,0 +1,175 @@
+/*
+ * Dream Cheeka Webmail Notifier USB RGB LED driver
+ *
+ * Copyright 2016 Heiner Kallweit <hkallwe...@gmail.com>
+ * Based on drivers/hid/hid-thingm.c and
+ * drivers/usb/misc/usbled.c
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ */
+
+#include 
+#include 
+#include 
+#include 
+

Re: [PATCH] hid: migrate Dream Cheeky LED driver from USB misc to HID

[Heiner Kallweit]

Am 14.06.2016 um 21:57 schrieb Oliver Neukum:
> On Tue, 2016-06-14 at 21:34 +0200, Heiner Kallweit wrote:
>> Am 14.06.2016 um 10:05 schrieb Oliver Neukum:
>>> On Tue, 2016-06-14 at 07:51 +0200, Heiner Kallweit wrote:
>>>
>>>> +  ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>>>> +  if (ret)
>>>> +  return ret;
>>>> +
>>>> +  minor = ((struct hidraw *) hdev->hidraw)->minor;
>>>> +
>>>> +  ret = dc_init_device(dcdev);
>>>
>>> That is a race condition. You announced an uninitialized device to the
>>> rest of the system.
>>>
>> I don't see a race. The hdev and lock elements of dcdev are initialized
> 
> You have called hid_hw_start(). That implies that the device be ready.
> 
OK, now I see your point, thanks. And right, it's better to call hid_hw_start
later.

Rgds, Heiner

>> and dc_init_device just calls dc_send to send the init command to the device.
>> It doesn't announce anything.
> 
> You send the init command after the rest of the system already sees the
> device.
> 
>   Regards
>   Oliver
> 
> 
> 

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

Re: [PATCH] hid: migrate Dream Cheeky LED driver from USB misc to HID

[Heiner Kallweit]

Am 14.06.2016 um 10:05 schrieb Oliver Neukum:
> On Tue, 2016-06-14 at 07:51 +0200, Heiner Kallweit wrote:
> 
>> +ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>> +if (ret)
>> +return ret;
>> +
>> +minor = ((struct hidraw *) hdev->hidraw)->minor;
>> +
>> +ret = dc_init_device(dcdev);
> 
> That is a race condition. You announced an uninitialized device to the
> rest of the system.
> 
I don't see a race. The hdev and lock elements of dcdev are initialized
and dc_init_device just calls dc_send to send the init command to the device.
It doesn't announce anything.

>> +if (ret)
>> +goto err;
>> +
>> +ret = dc_init_rgb(dcdev, minor);
>> +if (ret)
>> +goto err;
>> +
>> +dev_info(>dev, "Dream Cheeky Webmail Notifier %d initialized\n", 
>> minor);
>> +
>> +return 0;
>> +
>> +err:
>> +hid_hw_stop(hdev);
>> +return ret;
>> +}
> 
>   Regards
>   Oliver
> 
> 
> 

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

[PATCH] hid: migrate Dream Cheeky LED driver from USB misc to HID

[Heiner Kallweit]

The Dream Cheeky Webmail Notifier and Friends Alert are supported as part
of usb/misc/usbled driver currently. This patch migrates the driver for
these devices to the HID subsystem.

Benefits:
- Avoid using USB low-level calls and use the HID subsystem instead
  (as this device provides a USB HID interface)
- Use standard LED subsystem instead of proprietary sysfs entries,
  this allows e.g. to use the device with features like triggers

There might be users of the current driver, therefore so far allow
compilation of the new driver only if the old one is disabled.

Successfully tested with a Dream Cheeky Webmail Notifier.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/hid/Kconfig|   8 ++
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   6 +-
 drivers/hid/hid-dream-cheeky.c | 177 +
 drivers/hid/hid-ids.h  |   2 +
 5 files changed, 192 insertions(+), 2 deletions(-)
 create mode 100644 drivers/hid/hid-dream-cheeky.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 16fe27d..5618a61 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -247,6 +247,14 @@ config DRAGONRISE_FF
Say Y here if you want to enable force feedback support for DragonRise 
Inc.
game controllers.
 
+config HID_DREAM_CHEEKY
+   tristate "Dream Cheeky Webmail Notifier USB RGB LED"
+   depends on HID
+   depends on USB_LED = 'n'
+   ---help---
+   Support for the Dream Cheeky Webmail Notifier. This driver registers
+   a LED class instance for red, green, and blue color component.
+
 config HID_EMS_FF
tristate "EMS Production Inc. force feedback support"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 90300f3..2878739 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_HID_CORSAIR) += hid-corsair.o
 obj-$(CONFIG_HID_CP2112)   += hid-cp2112.o
 obj-$(CONFIG_HID_CYPRESS)  += hid-cypress.o
 obj-$(CONFIG_HID_DRAGONRISE)   += hid-dr.o
+obj-$(CONFIG_HID_DREAM_CHEEKY) += hid-dream-cheeky.o
 obj-$(CONFIG_HID_EMS_FF)   += hid-emsff.o
 obj-$(CONFIG_HID_ELECOM)   += hid-elecom.o
 obj-$(CONFIG_HID_ELO)  += hid-elo.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 730589a..c24f758 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1879,6 +1879,8 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_MOUSE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_WN) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_FA) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, 
USB_DEVICE_ID_ELECOM_BM084) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELO, 0x0009) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELO, 0x0030) },
@@ -2349,8 +2351,6 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_DEALEXTREAME, 
USB_DEVICE_ID_DEALEXTREAME_RADIO_SI4701) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, 
USB_DEVICE_ID_DELORME_EARTHMATE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EM_LT20) 
},
-   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x0004) },
-   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x000a) },
{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0400) },
{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0401) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ESSENTIAL_REALITY, 
USB_DEVICE_ID_ESSENTIAL_REALITY_P5) },
@@ -2489,6 +2489,8 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_YEALINK, 
USB_DEVICE_ID_YEALINK_P1K_P4K_B2K) },
 #if IS_ENABLED(CONFIG_USB_LED)
{ HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
USB_DEVICE_ID_RI_KA_WEBMAIL) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_WN) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 
USB_DEVICE_ID_DREAM_CHEEKY_FA) },
 #endif
{ }
 };
diff --git a/drivers/hid/hid-dream-cheeky.c b/drivers/hid/hid-dream-cheeky.c
new file mode 100644
index 000..8c228f9
--- /dev/null
+++ b/drivers/hid/hid-dream-cheeky.c
@@ -0,0 +1,177 @@
+/*
+ * Dream Cheeka Webmail Notifier USB RGB LED driver
+ *
+ * Copyright 2016 Heiner Kallweit <hkallwe...@gmail.com>
+ * Based on drivers/hid/hid-thingm.c and
+ * drivers/usb/misc/usbled.c
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "hid-ids.h"
+
+#define REPORT_SIZE

[PATCH v2] hid: migrate Riso Kagaku LED driver from USB misc to HID

[Heiner Kallweit]

The Riso Kagaku Webmail Notifier (and its clones) is supported as part of
usb/misc/usbled driver currently. This patch migrates the driver for this
device to the HID subsystem.

Benefits:
- Avoid using USB low-level calls and use the HID subsystem instead
  (as this device provides a USB HID interface)
- Use standard LED subsystem instead of proprietary sysfs entries,
  this allows e.g. to use the device with features like triggers

I know at least one cheap clone coming with green and blue LED switched.
There's a module paramater to deal with this.

There might be users of the current module, therefore so far allow
compilation of the new driver only if the old one is disabled.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- change config symbol from HID_RIKA to HID_RISO_KAGAKU
- use full name Riso Kagaku instead of rika in several places
- don't remove device from ignore list if CONFIG_USB_LED is defined
- fix module parameter permissions
---
 drivers/hid/Kconfig   |   9 +++
 drivers/hid/Makefile  |   1 +
 drivers/hid/hid-core.c|   3 +
 drivers/hid/hid-riso-kagaku.c | 171 ++
 4 files changed, 184 insertions(+)
 create mode 100644 drivers/hid/hid-riso-kagaku.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 5646ca4..16fe27d 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -784,6 +784,15 @@ config HID_HYPERV_MOUSE
---help---
Select this option to enable the Hyper-V mouse driver.
 
+config HID_RISO_KAGAKU
+   tristate "Riso Kagaku Webmail Notifier USB LED"
+   depends on HID
+   depends on LEDS_CLASS
+   depends on USB_LED = 'n'
+   ---help---
+   Support for the Riso Kagaku Webmail Notifier. This driver registers
+   a LED class instance for red, green, and blue color component.
+
 config HID_SMARTJOYPLUS
tristate "SmartJoy PLUS PS2/USB adapter support"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index a2fb562..90300f3 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o
 obj-$(CONFIG_HID_SUNPLUS)  += hid-sunplus.o
 obj-$(CONFIG_HID_GREENASIA)+= hid-gaff.o
 obj-$(CONFIG_HID_THINGM)   += hid-thingm.o
+obj-$(CONFIG_HID_RISO_KAGAKU)  += hid-riso-kagaku.o
 obj-$(CONFIG_HID_THRUSTMASTER) += hid-tmff.o
 obj-$(CONFIG_HID_TIVO) += hid-tivo.o
 obj-$(CONFIG_HID_TOPSEED)  += hid-topseed.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8ea3a26..730589a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2008,6 +2008,7 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, 
USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS, HID_ANY_ID) },
{ HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
USB_DEVICE_ID_RI_KA_WEBMAIL) },
 #if IS_ENABLED(CONFIG_HID_ROCCAT)
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ARVO) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ISKU) },
@@ -2486,7 +2487,9 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_DPAD) 
},
 #endif
{ HID_USB_DEVICE(USB_VENDOR_ID_YEALINK, 
USB_DEVICE_ID_YEALINK_P1K_P4K_B2K) },
+#if IS_ENABLED(CONFIG_USB_LED)
{ HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
USB_DEVICE_ID_RI_KA_WEBMAIL) },
+#endif
{ }
 };
 
diff --git a/drivers/hid/hid-riso-kagaku.c b/drivers/hid/hid-riso-kagaku.c
new file mode 100644
index 000..0a37f18
--- /dev/null
+++ b/drivers/hid/hid-riso-kagaku.c
@@ -0,0 +1,171 @@
+/*
+ * Riso Kagaku Webmail Notifier USB RGB LED driver
+ *
+ * Copyright 2016 Heiner Kallweit <hkallwe...@gmail.com>
+ * Based on drivers/hid/hid-thingm.c and
+ * drivers/usb/misc/usbled.c
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "hid-ids.h"
+
+#define REPORT_SIZE6
+
+static unsigned const char riso_kagaku_tbl[] = {
+/* R+2G+4B -> riso kagaku color index */
+   [0] = 0, /* black   */
+   [1] = 2, /* red */
+   [2] = 1, /* green   */
+   [3] = 5, /* yellow  */
+   [4] = 3, /* blue*/
+   [5] = 6, /* magenta */
+   [6] = 4, /* cyan*/
+   [7] = 7  /* white   */
+};
+
+#define RISO_KAGAKU_IX(r, g, b) riso_kagaku_tbl[((r)?1:0)+((g)?2:0)+((b)?4:0)]
+
+struct rika_led {
+   struct led_classdev cdev;
+   struct rika_device  *rdev;
+   charname[32];
+};
+
+struct rika_device {
+ 

Re: [PATCH] hid: migrate Riso Kagaku LED driver from USB misc to HID

[Heiner Kallweit]

Am 03.06.2016 um 10:46 schrieb Benjamin Tissoires:
> On May 28 2016 or thereabouts, Heiner Kallweit wrote:
>> Am 27.05.2016 um 23:45 schrieb Benjamin Tissoires:
>>> On May 27 2016 or thereabouts, Heiner Kallweit wrote:
>>>> The Riso Kagaku Webmail Notifier (and its clones) is supported as part of
>>>> usb/misc/usbled driver currently. This patch migrates the driver for this
>>>> device to the HID subsystem.
>>>>
>>>> Benefits:
>>>> - Avoid using USB low-level calls and use the HID subsystem instead
>>>>   (as this device provides a USB HID interface)
>>>> - Use standard LED subsystem instead of proprietary sysfs entries,
>>>>   this allows e.g. to use the device with features like triggers
>>>>
>>>> I know at least one cheap clone coming with green and blue LED switched.
>>>> There's a module paramater to deal with this.
>>>>
>>>> There might be users of the current module, therefore so far allow
>>>> compilation of the new driver only if the old one is disabled.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>>>> ---
>>>>  drivers/hid/Kconfig|   9 +++
>>>>  drivers/hid/Makefile   |   1 +
>>>>  drivers/hid/hid-core.c |   2 +-
>>>>  drivers/hid/hid-rika.c | 171 
>>>> +
>>>>  4 files changed, 182 insertions(+), 1 deletion(-)
>>>>  create mode 100644 drivers/hid/hid-rika.c
>>>>
>>>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>>>> index 5646ca4..5ba75de 100644
>>>> --- a/drivers/hid/Kconfig
>>>> +++ b/drivers/hid/Kconfig
>>>> @@ -784,6 +784,15 @@ config HID_HYPERV_MOUSE
>>>>---help---
>>>>Select this option to enable the Hyper-V mouse driver.
>>>>  
>>>> +config HID_RIKA
>>>> +  tristate "Riso Kaguku Webmail Notifier USB LED"
>>>> +  depends on HIDRAW
>>>> +  depends on LEDS_CLASS
>>>> +  depends on USB_LED = 'n'
>>>> +  ---help---
>>>> +  Support for the Riso Kagaku Webmail Notifier. This driver registers
>>>> +  a LED class instance for red, green, and blue color component.
>>>> +
>>>>  config HID_SMARTJOYPLUS
>>>>tristate "SmartJoy PLUS PS2/USB adapter support"
>>>>depends on HID
>>>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>>>> index a2fb562..406f6d0 100644
>>>> --- a/drivers/hid/Makefile
>>>> +++ b/drivers/hid/Makefile
>>>> @@ -91,6 +91,7 @@ obj-$(CONFIG_HID_STEELSERIES)+= hid-steelseries.o
>>>>  obj-$(CONFIG_HID_SUNPLUS) += hid-sunplus.o
>>>>  obj-$(CONFIG_HID_GREENASIA)   += hid-gaff.o
>>>>  obj-$(CONFIG_HID_THINGM)  += hid-thingm.o
>>>> +obj-$(CONFIG_HID_RIKA)+= hid-rika.o
>>>>  obj-$(CONFIG_HID_THRUSTMASTER)+= hid-tmff.o
>>>>  obj-$(CONFIG_HID_TIVO)+= hid-tivo.o
>>>>  obj-$(CONFIG_HID_TOPSEED) += hid-topseed.o
>>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>>> index 8ea3a26..db929dc 100644
>>>> --- a/drivers/hid/hid-core.c
>>>> +++ b/drivers/hid/hid-core.c
>>>> @@ -2008,6 +2008,7 @@ static const struct hid_device_id 
>>>> hid_have_special_driver[] = {
>>>>{ HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, 
>>>> USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) },
>>>>{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS, HID_ANY_ID) },
>>>>{ HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
>>>> +  { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
>>>> USB_DEVICE_ID_RI_KA_WEBMAIL) },
>>>>  #if IS_ENABLED(CONFIG_HID_ROCCAT)
>>>>{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ARVO) },
>>>>{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ISKU) },
>>>> @@ -2486,7 +2487,6 @@ static const struct hid_device_id hid_ignore_list[] 
>>>> = {
>>>>{ HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_DPAD) 
>>>> },
>>>>  #endif
>>>>{ HID_USB_DEVICE(USB_VENDOR_ID_YEALINK, 
>>>> USB_DEVICE_ID_YEALINK_P1K_P4K_B2K) },
>>>> -  { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
>>>> USB_DEVICE_ID_RI_KA_WEBMAIL) },
>>>
>>> Hi Heiner,
>>>
>>> Just a quick remark before a

Re: [PATCH] hid: migrate Riso Kagaku LED driver from USB misc to HID

[Heiner Kallweit]

Am 27.05.2016 um 23:45 schrieb Benjamin Tissoires:
> On May 27 2016 or thereabouts, Heiner Kallweit wrote:
>> The Riso Kagaku Webmail Notifier (and its clones) is supported as part of
>> usb/misc/usbled driver currently. This patch migrates the driver for this
>> device to the HID subsystem.
>>
>> Benefits:
>> - Avoid using USB low-level calls and use the HID subsystem instead
>>   (as this device provides a USB HID interface)
>> - Use standard LED subsystem instead of proprietary sysfs entries,
>>   this allows e.g. to use the device with features like triggers
>>
>> I know at least one cheap clone coming with green and blue LED switched.
>> There's a module paramater to deal with this.
>>
>> There might be users of the current module, therefore so far allow
>> compilation of the new driver only if the old one is disabled.
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>> ---
>>  drivers/hid/Kconfig|   9 +++
>>  drivers/hid/Makefile   |   1 +
>>  drivers/hid/hid-core.c |   2 +-
>>  drivers/hid/hid-rika.c | 171 
>> +
>>  4 files changed, 182 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/hid/hid-rika.c
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 5646ca4..5ba75de 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -784,6 +784,15 @@ config HID_HYPERV_MOUSE
>>  ---help---
>>  Select this option to enable the Hyper-V mouse driver.
>>  
>> +config HID_RIKA
>> +tristate "Riso Kaguku Webmail Notifier USB LED"
>> +depends on HIDRAW
>> +depends on LEDS_CLASS
>> +depends on USB_LED = 'n'
>> +---help---
>> +Support for the Riso Kagaku Webmail Notifier. This driver registers
>> +a LED class instance for red, green, and blue color component.
>> +
>>  config HID_SMARTJOYPLUS
>>  tristate "SmartJoy PLUS PS2/USB adapter support"
>>  depends on HID
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index a2fb562..406f6d0 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -91,6 +91,7 @@ obj-$(CONFIG_HID_STEELSERIES)  += hid-steelseries.o
>>  obj-$(CONFIG_HID_SUNPLUS)   += hid-sunplus.o
>>  obj-$(CONFIG_HID_GREENASIA) += hid-gaff.o
>>  obj-$(CONFIG_HID_THINGM)+= hid-thingm.o
>> +obj-$(CONFIG_HID_RIKA)  += hid-rika.o
>>  obj-$(CONFIG_HID_THRUSTMASTER)  += hid-tmff.o
>>  obj-$(CONFIG_HID_TIVO)  += hid-tivo.o
>>  obj-$(CONFIG_HID_TOPSEED)   += hid-topseed.o
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 8ea3a26..db929dc 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -2008,6 +2008,7 @@ static const struct hid_device_id 
>> hid_have_special_driver[] = {
>>  { HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, 
>> USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS, HID_ANY_ID) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
>> +{ HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
>> USB_DEVICE_ID_RI_KA_WEBMAIL) },
>>  #if IS_ENABLED(CONFIG_HID_ROCCAT)
>>  { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ARVO) },
>>  { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ISKU) },
>> @@ -2486,7 +2487,6 @@ static const struct hid_device_id hid_ignore_list[] = {
>>  { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_DPAD) 
>> },
>>  #endif
>>  { HID_USB_DEVICE(USB_VENDOR_ID_YEALINK, 
>> USB_DEVICE_ID_YEALINK_P1K_P4K_B2K) },
>> -{ HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
>> USB_DEVICE_ID_RI_KA_WEBMAIL) },
> 
> Hi Heiner,
> 
> Just a quick remark before an actual review. If you remove
> unconditionally the driver from hid_ignore_list, the usb driver will
> never get a chance to be bound to it as hid core will keep a reference
> to it.
> 
> The solution is either to #ifdef this line or to port the full USB
> driver into a HID one (and so remove the USB one).

Right, thanks for the hint. For now I would enclose this line in
#if IS_ENABLED(CONFIG_USB_LED)
There might be users of the current USB driver (although the proprietary
sysfs interface isn't documented under Documentation/ABI) and it might
be a good idea to give them some time to migrate before removing the old
USB driver.

Before submitting a v2 of the patch I'll wait for more review feedback.

Rgds, Heiner
> 
> Cheers,
> Benjamin

[PATCH] hid: migrate Riso Kagaku LED driver from USB misc to HID

[Heiner Kallweit]

The Riso Kagaku Webmail Notifier (and its clones) is supported as part of
usb/misc/usbled driver currently. This patch migrates the driver for this
device to the HID subsystem.

Benefits:
- Avoid using USB low-level calls and use the HID subsystem instead
  (as this device provides a USB HID interface)
- Use standard LED subsystem instead of proprietary sysfs entries,
  this allows e.g. to use the device with features like triggers

I know at least one cheap clone coming with green and blue LED switched.
There's a module paramater to deal with this.

There might be users of the current module, therefore so far allow
compilation of the new driver only if the old one is disabled.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/hid/Kconfig|   9 +++
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   2 +-
 drivers/hid/hid-rika.c | 171 +
 4 files changed, 182 insertions(+), 1 deletion(-)
 create mode 100644 drivers/hid/hid-rika.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 5646ca4..5ba75de 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -784,6 +784,15 @@ config HID_HYPERV_MOUSE
---help---
Select this option to enable the Hyper-V mouse driver.
 
+config HID_RIKA
+   tristate "Riso Kaguku Webmail Notifier USB LED"
+   depends on HIDRAW
+   depends on LEDS_CLASS
+   depends on USB_LED = 'n'
+   ---help---
+   Support for the Riso Kagaku Webmail Notifier. This driver registers
+   a LED class instance for red, green, and blue color component.
+
 config HID_SMARTJOYPLUS
tristate "SmartJoy PLUS PS2/USB adapter support"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index a2fb562..406f6d0 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o
 obj-$(CONFIG_HID_SUNPLUS)  += hid-sunplus.o
 obj-$(CONFIG_HID_GREENASIA)+= hid-gaff.o
 obj-$(CONFIG_HID_THINGM)   += hid-thingm.o
+obj-$(CONFIG_HID_RIKA) += hid-rika.o
 obj-$(CONFIG_HID_THRUSTMASTER) += hid-tmff.o
 obj-$(CONFIG_HID_TIVO) += hid-tivo.o
 obj-$(CONFIG_HID_TOPSEED)  += hid-topseed.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8ea3a26..db929dc 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2008,6 +2008,7 @@ static const struct hid_device_id 
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, 
USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS, HID_ANY_ID) },
{ HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
+   { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
USB_DEVICE_ID_RI_KA_WEBMAIL) },
 #if IS_ENABLED(CONFIG_HID_ROCCAT)
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ARVO) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ISKU) },
@@ -2486,7 +2487,6 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_DPAD) 
},
 #endif
{ HID_USB_DEVICE(USB_VENDOR_ID_YEALINK, 
USB_DEVICE_ID_YEALINK_P1K_P4K_B2K) },
-   { HID_USB_DEVICE(USB_VENDOR_ID_RISO_KAGAKU, 
USB_DEVICE_ID_RI_KA_WEBMAIL) },
{ }
 };
 
diff --git a/drivers/hid/hid-rika.c b/drivers/hid/hid-rika.c
new file mode 100644
index 000..d4d2087
--- /dev/null
+++ b/drivers/hid/hid-rika.c
@@ -0,0 +1,171 @@
+/*
+ * Riso Kagaku Webmail Notifier USB RGB LED driver
+ *
+ * Copyright 2016 Heiner Kallweit <hkallwe...@gmail.com>
+ * Based on drivers/hid/hid-thingm.c and
+ * drivers/usb/misc/usbled.c
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "hid-ids.h"
+
+#define REPORT_SIZE6
+
+static unsigned const char riso_kagaku_tbl[] = {
+/* R+2G+4B -> riso kagaku color index */
+   [0] = 0, /* black   */
+   [1] = 2, /* red */
+   [2] = 1, /* green   */
+   [3] = 5, /* yellow  */
+   [4] = 3, /* blue*/
+   [5] = 6, /* magenta */
+   [6] = 4, /* cyan*/
+   [7] = 7  /* white   */
+};
+
+#define RISO_KAGAKU_IX(r, g, b) riso_kagaku_tbl[((r)?1:0)+((g)?2:0)+((b)?4:0)]
+
+struct rika_led {
+   struct led_classdev cdev;
+   struct rika_device  *rdev;
+   charname[32];
+};
+
+struct rika_device {
+   struct rika_led red;
+   struct rika_led green;
+   struct rika_led blue;
+   struct hid_device   *hdev;
+   struct mutexlock;
+};
+
+#define to_rika_led(arg) container_of(arg, struct rika_led, cdev)
+
+static bool switch_green_blue;
+module_param(s

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Heiner Kallweit]

Am 05.04.2016 um 21:45 schrieb Jacek Anaszewski:
> On 04/05/2016 11:01 AM, Pavel Machek wrote:
>> Hi!
>>
>>> It would have the same downsides as in case of having r, g and b in
>>> separate attributes, i.e. - problems with setting LED colour in
>>> a consistent way. This way LED blinking in whatever colour couldn't
>>> be supported reliably. It was one of your primary rationale standing
>>> behind this design, if I remember correctly. Second - what about
>>> triggers? We've had a long discussion about it and this design turned
>>> out to be most fitting.
>>
>> Are on/off triggers really that useful for a LED that can produce 16
>> million colors?
>>
>> I believe we should support patterns for RGB LEDs. Something like
>> [ (time, r, g, b), ... ] . Ok, what about this one?
>>
>> Lets say we have
>>
>> /sys/class/pattern/lp5533::0
>> /sys/class/pattern/software::0
>>
>> /sys/class/led/n900::red ; default trigger "lp5533::0:0"
>> /sys/class/led/n900::green ; default trigger "lp5533::0:1"
>> /sys/class/led/n900::blue ; default trigger "lp5533::0:2"
>>
>> Normally, pattern would correspond to one RGB LED. We could have
>> attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
>> this pattern.
>>>
>>> Could you give an example on how to set a color for RGB LED using
>>> this interface? Would it be compatible with LED triggers?
>>> Where the "pattern" class would be implemented?
>>
>> Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
>> set the color for the led. 'echo "trigger-name" > trigger' would set
>> the trigger, probably just toggling between LED off and set color for
>> the old triggers.
>>
>> Where to implement the patterns is different question, but for example
>> drivers/leds/pattern?
> 
> I'd rather leave the pattern issue for now, since it seems to be
> different from the problem Heiner was trying to solve with his LED RGB
> extension. Moreover, hardware patterns are device specific and it could
> be hard to propose a generic interface.
> Drivers can always expose their custom sysfs attributes for configuring
> the patterns.
> 
> Regardless of the above, some of your considerations brought me an idea
> on how to add generic and backwards compatible support for setting RGB
> color at one go.
> 
> Currently LED class drivers of RGB LED controllers expose three LED
> class devices - one per R, G and B color component. I propose that
> such drivers set LED_DEV_CAP_RGB flag for each LED class device they
> register. LED core, seeing the flag, would create a generic "color"
> sysfs attribute for each of the three LED class devices.
> 
> The "color" attribute would contain "R G B" values. Setting the "color"
> attribute of any of the three LED class devices would affect brightness
> properties (i.e. constituent colors) of the remaining two ones.
> It would result in disabling any active triggers and writing all the
> three color settings to the RGB LED controller at one go.
> 
> We would probably need additional op in the LED core : color_set.
> 
> Having the color set to nonzero value would signify the the three LED
> class devices are in sync and that setting a trigger on any of them
> applies to the remaining two ones. It would have to be considered
> whether existing triggers could be made compatible with synchronized
> RGB LED class devices.
> 
> I'm curious what do you think about the idea.
> 
> Pavel, Heiner, others?
> 
Exposing "coupled LED devices" as separate LED devices most likely is ok
when accessed from user space as the name of the led_classdev's indicates
that they belong together.
But how about a trigger wanting to set a RGB LED to a specific color?
(That's not available yet but one possible use case for RGB LED's)
A trigger is bound to a led_classdev currently. In addition we'd need
to introduce some kind of super_led_classdev having links to the respective
R/G/B led_classdev's (+ trigger functions dealing with this super_led_classdev).

These changes / extensions are not needed if a RGB LED is exposed as one
led_classdev, just with flag LED_DEV_CAP_RGB set.
OK, we'd still have to change the sysfs interface as obviously setting
hue/sat/brightness via one "brightness" attribute is not acceptable.
However this constraint might not affect the kernel-internal trigger API
(usage of parameter brightness in led_trigger_event).

I see Pavel's point that there might be different types of multi-color LED's.
At least we have:
- multi-color LED's where each single LED is visible even if all are switched on
- multi-color LED's like RGB LED's where you usually just see a uniform color

Last but not least regarding the patterns:
Something like proposed by Pavel is e.g. (partially) supported by the blink(1)
firmware. That would be an example of such a "hardware-accelerated" pattern.

As I see it the current blinking support then would be one special case of a 
pattern.
As 

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Heiner Kallweit]

On Wed, Mar 30, 2016 at 3:03 PM, Pavel Machek  wrote:
> Hi!
>
>> >Ok, so:
>> >
>> >a) Do we want RGB leds to be handled by existing subsystem, or do we
>> >need separate layer on top of that?
>> >
>> >b) Does RGB make sense, or HSV? RGB is quite widely used in graphics,
>> >and it is what hardware implements. (But we'd need to do gamma
>> >correction).
>> >
>> >c) My hardware has "acceleration engine", LED is independend from
>> >CPU. That's rather big deal. Does yours? It seems to be quite common,
>> >at least in cellphones.
>> >
>> >Ideally, I'd like to have "triggers", but different ones. As in: if
>> >charging, do yellow " .xX" pattern. If fully charged, do green steady
>> >light. If message is waiting, do blue " x x" pattern. If none of
>> >above, do slow white blinking. (Plus priorities of events). But that's
>> >quite different from existing support...)
>>
>> Please note that HSV colour scheme allows to neatly project monochrome
>> brightness semantics on the RGB realm. I.e. you can have fixed
>> hue and saturation, and by changing the brightness component a perceived
>> colour intensity can be altered.
>
> I see HSV has some advantages. But we already have LEDs with multiple
> colors, and kernel already handles them:
>
> pavel@duo:~$ ls -1 /sys/class/leds/
> tpacpi:green:batt
> tpacpi:orange:batt
>
> This is physically 2 leds but hidden under one indicator, so you got
> "off", "green", "orange" and "green+orange".

That's a good example. As long as you can recognize green+orange as
separate lights/colors
(w/o magnifying glass) I wouldn't call it "a LED with multiple colors"
but "multiple
LED devices".

In my use case we talk about RGB LEDs like the commonly used 5050 SMD RGB LEDs.
And it's not only about using a handful of discrete colors but about
displaying any arbitrary
color.
So far the kernel exposes the physical RGB LEDs as separate LEDs only
and I can't use
a trigger to e.g. set "magenta with 50% brightness".

Heiner

> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) 
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Heiner Kallweit]

Am 29.03.2016 um 23:43 schrieb Pavel Machek:
> Hi!
> 
>>> First, please Cc me on RGB color support.
>>>
 Add generic support for RGB Color LED's.

 Basic idea is to use enum led_brightness also for the hue and saturation
 color components.This allows to implement the color extension w/o
 changes to struct led_classdev.

 Select LEDS_RGB to enable building drivers using the RGB extension.

 Flag LED_SET_HUE_SAT allows to specify that hue / saturation
 should be overridden even if the provided values are zero.

 Some examples for writing values to /sys/class/leds//brightness:
 (now also hex notation can be used)

 255 -> set full brightness and keep existing color if set
 0 -> switch LED off but keep existing color so that it can be restored
  if the LED is switched on again later
 0x100 -> switch LED off and set also hue and saturation to 0
 0x00 -> set full brightness, full saturation and set hue to 0
 (red)
>>>
>>> Umm, that's rather strange interface -- and three values in single sysfs
>>> file is actually forbidden.
>>>
>>> Plus, it is very much unlike existing interfaces for RGB LEDs, which
>>> we already have supported in the tree. (At least nokia N900 and Sony
>>> motion controller already contain supported three-color LEDs).
>>>
>>> Now... yes, there's work to be done for the 3-color LEDs. Currently,
>>> they are treated as three different LEDs. (Which makes some sense, you
>>> can use "battery charging" trigger for LED, and CPU activity trigger
>>> for green, for example). It would be good to have some kind of
>>> grouping, so that userspace can tell "these 3 leds are actually
>>> combined into one light".
>>>
>> At first thanks for the review comments.
>> Treating the three physical LEDs of a RGB LED as separate LED devices
>> might have been implemented due to the lack of alternatives.
> 
> To be fair... they _are_ separate LED devices. In N900 case, you can
> even see light comming from slightly different places if you look closely.
> 
I mainly work with encapsulated USB HID LED devices like Thingm blink(1).
Due to the diffuse plastic cover you don't see the individual LEDs on the chip.

>> With one trigger controlling the red LED and another controlling the green
>> LED we may end up with a yellow light. Not sure whether this is what
>> we want.
> 
> Well, it should be understandable for most people.
> 
>> One driver for this extension was the idea of triggers using color
>> to visualize states etc.
>> Therefore it's not only about userspace controlling the color.
>> As a trigger is bound to a led_classdev we need a led_classdev
>> representing a RGB LED device.
>>
>> And ok: If required the sysfs interface can be splitted into separate
>> attributes for hue, saturation, and (existing) brightness.
> 
> Required.
> 
> Ok, so:
> 
> a) Do we want RGB leds to be handled by existing subsystem, or do we
> need separate layer on top of that?
> 
> b) Does RGB make sense, or HSV? RGB is quite widely used in graphics,
> and it is what hardware implements. (But we'd need to do gamma
> correction).
> 
HSV has the charme that the current monochrome V-only is a subset.
Therefore the current API can be used also with color LEDs.
However there might be good reasons for using RGB too.

> c) My hardware has "acceleration engine", LED is independend from
> CPU. That's rather big deal. Does yours? It seems to be quite common,
> at least in cellphones.
> 
Devices like blink(1) support storing and re-playing patterns, fading etc.

> Ideally, I'd like to have "triggers", but different ones. As in: if
> charging, do yellow " .xX" pattern. If fully charged, do green steady
> light. If message is waiting, do blue " x x" pattern. If none of
> above, do slow white blinking. (Plus priorities of events). But that's
> quite different from existing support...)
> 
I think for this a separate layer would be helpful.
Your primary intention is to e.g. display "charging" on the RGB LED
device. Most likely you don't want to split yellow into its red + green
component and then write to the respective (sub-)LEDs.
Also just think of the case that later you might decide that orange
is nicer than yellow.

>   Pavel
> 

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

Re: [PATCH v5 4/4] leds: core: add support for RGB LED's

[Heiner Kallweit]

Am 29.03.2016 um 12:05 schrieb Pavel Machek:
> On Tue 2016-03-01 22:36:12, Heiner Kallweit wrote:
>> Export a function to convert HSV color values to RGB.
>> It's intended to be called by drivers for RGB LEDs.
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>> ---
>> v2:
>> - move hsv -> rgb conversion to separate file
>> - remove flag LED_DEV_CAP_RGB
>> v3:
>> - call led_hsv_to_rgb only if LED_DEV_CAP_HSV is set
>>   This is needed in cases when we have monochrome and color LEDs
>>   as well in a system.
>> v4:
>> - Export led_hsv_to_rgb and let the device driver call it instead
>>   of doing the conversion in the core
>> v5:
>> - don't ignore led_cdev->brightness_get silently if LED_DEV_CAP_RGB
>>   is set but warn
>> ---
>>  drivers/leds/led-class.c|  7 +++
>>  drivers/leds/led-rgb-core.c | 36 
>>  include/linux/leds.h|  8 
>>  3 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 8a3748a..a4b144e 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -193,6 +193,13 @@ int led_classdev_register(struct device *parent, struct 
>> led_classdev *led_cdev)
>>  char name[64];
>>  int ret;
>>  
>> +/*
>> + * for now reading back the color is not supported as multiple
>> + * HSV -> RGB -> HSV conversions may distort the color due to
>> + * rounding issues in the conversion algorithm
>> + */
>> +WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get);
>> +
> 
> Backtrace is useless here, you may want to add some ()s and you don't
> really want user to be causing messages in syslog this easily.
> 
I agree that the backtrace doesn't provide a benefit here.

However I don't see how a user could create syslog entries.
The warn condition can be true only for drivers implementing the RGB extension
in a not supported way (by setting flag LED_DEV_CAP_RGB and implementing
brightness_get).

Pavel
> 

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

[PATCH v8 1/5] leds: core: add generic support for RGB LED's

[Heiner Kallweit]

Add generic support for RGB LED's.

Basic idea is to use enum led_brightness also for the hue and saturation
color components.This allows to implement the color extension w/o
changes to struct led_classdev.

Select LEDS_CLASS_RGB to enable building drivers using the RGB extension.

Flag LED_SET_HUE_SAT allows to specify that hue / saturation
should be overridden even if the provided values are zero.

Some examples for writing values to /sys/class/leds//brightness:
(now also hex notation can be used)

255 -> set full brightness and keep existing color if set
0 -> switch LED off but keep existing color so that it can be restored
 if the LED is switched on again later
0x100 -> switch LED off and set also hue and saturation to 0
0x00 -> set full brightness, full saturation and set hue to 0 (red)

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- move extension-specific code into a separate source file and
  introduce config symbol LEDS_HSV for it
- create separate patch for the extension to sysfs
- use variable name led_cdev as in the rest if the core
- rename to_hsv to led_validate_brightness
- rename LED_BRIGHTNESS_SET_COLOR to LED_SET_HSV
- introduce helper is_off for checking whether V part
  of a HSV value is zero
v3:
- change Kconfig to use depend instead of select, add help message,
  and change config symbol to LEDS_COLOR
- change LED core object file name in Makefile
- rename flag LED_SET_HSV to LED_SET_COLOR
- rename is_off to led_is_off
- rename led_validate-brightness to led_confine_brightness
- rename variable in led_confine_brightness
- add dummy enum led_brightness value to enforce 32bit enum
- rename led-hsv-core.c to led-color-core.c
- move check of provided brightness value to led_confine_brightness
v4:
- change config symbol name to LEDS_RGB
- change name of new file to led-rgb-core.c
- factor out part of led_confine_brightness
- change led_is_off to __is_set_brightness
- in led_set_software_blink pass led_cdev->max_brightness instead of LED_FULL
- rename LED_SET_COLOR to LED_SET_HUE_SAT
v5:
- change "RGB Color LED" to "RGB LED" in Kconfig
- move definition of LED_HUE_SAT_MASK to drivers/leds/leds.h
- rename LED_DEV_CAP_HSV to LED_DEV_CAP_RGB
v6:
- no change
v7:
- removed "Color" from RGB Color LED in commit message and title
- don't include linux/kernel.h in led-rgb-core.c
- keep definition of LED_DEV_CAP_[] flags together
v8:
- rename LEDS_RGB to LEDS_CLASS_RGB
---
 drivers/leds/Kconfig| 12 
 drivers/leds/Makefile   |  4 +++-
 drivers/leds/led-class.c|  2 +-
 drivers/leds/led-core.c | 16 +---
 drivers/leds/led-rgb-core.c | 39 +++
 drivers/leds/leds.h | 18 ++
 include/linux/leds.h| 11 ++-
 7 files changed, 92 insertions(+), 10 deletions(-)
 create mode 100644 drivers/leds/led-rgb-core.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 7f940c2..20697a2 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -19,6 +19,14 @@ config LEDS_CLASS
  This option enables the led sysfs class in /sys/class/leds.  You'll
  need this to do anything useful with LEDs.  If unsure, say N.
 
+config LEDS_CLASS_RGB
+   bool "RGB LED Class Support"
+   depends on LEDS_CLASS
+   help
+ This option enables support for RGB LED devices.
+ Sysfs attribute brightness is interpreted as a HSV color value.
+ For details see Documentation/leds/leds-class.txt.
+
 config LEDS_CLASS_FLASH
tristate "LED Flash Class Support"
depends on LEDS_CLASS
@@ -29,6 +37,10 @@ config LEDS_CLASS_FLASH
  for the flash related features of a LED device. It can be built
  as a module.
 
+if LEDS_CLASS_RGB
+comment "RGB LED drivers"
+endif # LEDS_CLASS_RGB
+
 comment "LED drivers"
 
 config LEDS_88PM860X
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e9d5309..7f1b2be 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -1,6 +1,8 @@
 
 # LED Core
-obj-$(CONFIG_NEW_LEDS) += led-core.o
+obj-$(CONFIG_NEW_LEDS) += led-core-objs.o
+led-core-objs-y:= led-core.o
+led-core-objs-$(CONFIG_LEDS_CLASS_RGB) += led-rgb-core.o
 obj-$(CONFIG_LEDS_CLASS)   += led-class.o
 obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o
 obj-$(CONFIG_LEDS_TRIGGERS)+= led-triggers.o
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index aa84e5b..007a5b3 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -53,7 +53,7 @@ static ssize_t brightness_store(struct device *dev,
if (ret)
goto unlock;
 
-   if (state == LED_OFF)
+   if (!__is_brightness_set(state))
led_trigger_remove(led_cdev);
led_set_brightness(led_cdev, stat

Re: [PATCH v7 1/5] leds: core: add generic support for RGB LED's

[Heiner Kallweit]

Am 11.03.2016 um 09:38 schrieb Jacek Anaszewski:
> Hi Heiner,
> 
> Thanks for the updated set. I've renamed the feature to RGB LED class
> and pushed out to devel branch of linux-leds.git. It will sit there
> till the end of the upcoming merge window. There have been some
> uncertainties raised related to overloading brightness syntax. so let's
> better have it in linux-next through the whole next development cycle
> for the people to comment on.

Thanks Jacek, fine with me. I think in the next days I can come up with
two or three follow-up patches using this RGB LED functionality in triggers.
They would also be candidates for the devel branch then.

Rgds, Heiner

> 
> Thanks,
> Jacek Anaszewski
> 
> [...]
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/5] leds: core: add generic support for RGB LED's

[Heiner Kallweit]

Am 06.03.2016 um 21:55 schrieb Karl Palsson:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
> 
> Heiner Kallweit <hkallwe...@gmail.com> wrote:
>> Add generic support for RGB LED's.
>>
>> Basic idea is to use enum led_brightness also for the hue and
>> saturation color components.This allows to implement the color
>> extension w/o changes to struct led_classdev.
>>
>> Select LEDS_RGB to enable building drivers using the RGB
>> extension.
>>
>> Flag LED_SET_HUE_SAT allows to specify that hue / saturation
>> should be overridden even if the provided values are zero.
>>
>> Some examples for writing values to
>> /sys/class/leds//brightness: (now also hex notation can be
>> used)
>>
>> 255 -> set full brightness and keep existing color if set 0 ->
>> switch LED off but keep existing color so that it can be
>> restored
>>  if the LED is switched on again later
>> 0x100 -> switch LED off and set also hue and saturation to
>> 0 0x00 -> set full brightness, full saturation and set hue
>> to 0 (red)
>>
> 
> 
> I admit I hadn't seen this earlier, and I didn't spend all day
> looking at previous attempts, but what's the motivation for
> putting all this overloaded syntax into the "brightness" file? I
> thought the point was to have a single value in each file, one of
> the references I did find was
> http://www.spinics.net/lists/linux-leds/msg02995.html Is there
> some thread where this was decided as advantageous? Surely 0-255
> for _brightness_ is what the brightness entry should do?
> 
The referenced mail thread refers to a proposed sysfs attribute
holding a list of space-separated entries. Here it's still about
one numeric value.
Advantage of the approach used now is the full backwards compatibilty.
You can still set brightness to 0..255 and only the brightness will
change (as expected). Or in other words: So far only V was supported,
now we support HSV as a superset.

> I'd like to set the rgb colour of a led (or hsv, that can be it's
> own file too) and separately ramp the brightness up and down. I
> also think it's substantially simpler and easier to use from the
> user's point of view, which is surely the place you want easy
> right?
> 
What you describe is perfectly possible with the new approach.

Regards, Heiner

> Sincerely,
> Karl Palsson
> 
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v1.4.11 (GNU/Linux)
> 
> iQIcBAEBAgAGBQJW3Jk5AAoJEBmotQ/U1cr2fxYP/AsuQ/x8ky86S9xf9Y8UdRrk
> IC7eouBpf07RsDTv2KobRwEH69tk12zxGKmOpNZ5SY8ozVT/VDXMA8Iw/cwKL2t4
> EBWTdBhORrOlfxw0sykp4SXYSYBm9n2Z+xZGK9b/fN+2g+XCv4B+W2iDejyvAsIt
> c/eH6dGR0PvYdovEh0Tq7qAflpXRAhU0ykRR0Ydq/HrF8Xfxi+MDHC99zTRrHIsV
> rPTbPh26cxZ3zyOoUxwgPLNmm4O1BvMsghxuQXV49A95gOlRet+ewDQxBgwWabEp
> AUh3fuOl53R/ODJSqjX/JjlO4ynXWgv/9kdCF8QwPUAl13gyhilPvIdI5O3gm3Nr
> beiW/rUnvHej3ZxbRUe/Q8ZlQ099WTVH4cEgSxLclC5hiWm4dCjsjskJA1acbnZV
> 4w5WSqrAqSyNP81Rhy7WV6k8kazDUrASSAl4JFnNJVRC4WNdHQJA4pKkH08mtYyo
> 5ls3ydMzU2eiTNKCFEze4/cH3MgUWM+L29rLRzev6rT7s32rPzR0JKaKv460pocd
> rjpKanbt+zgUVySprVzX4t4GsmDZtKjQkTGooz9BabZP5+WeVvDtEMK3kciZ1d/x
> ubtvcMXGbDpZ0FMcQkTQj44Sq3wMdr3P0CoMiDspDGk7XY67gSXsmUgSSh0JTLRL
> X4K67h/OUpH0A00XGZCO
> =86mG
> -END PGP SIGNATURE-
> 

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

[PATCH v7 1/5] leds: core: add generic support for RGB LED's

[Heiner Kallweit]

Add generic support for RGB LED's.

Basic idea is to use enum led_brightness also for the hue and saturation
color components.This allows to implement the color extension w/o
changes to struct led_classdev.

Select LEDS_RGB to enable building drivers using the RGB extension.

Flag LED_SET_HUE_SAT allows to specify that hue / saturation
should be overridden even if the provided values are zero.

Some examples for writing values to /sys/class/leds//brightness:
(now also hex notation can be used)

255 -> set full brightness and keep existing color if set
0 -> switch LED off but keep existing color so that it can be restored
 if the LED is switched on again later
0x100 -> switch LED off and set also hue and saturation to 0
0x00 -> set full brightness, full saturation and set hue to 0 (red)

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- move extension-specific code into a separate source file and
  introduce config symbol LEDS_HSV for it
- create separate patch for the extension to sysfs
- use variable name led_cdev as in the rest if the core
- rename to_hsv to led_validate_brightness
- rename LED_BRIGHTNESS_SET_COLOR to LED_SET_HSV
- introduce helper is_off for checking whether V part
  of a HSV value is zero
v3:
- change Kconfig to use depend instead of select, add help message,
  and change config symbol to LEDS_COLOR
- change LED core object file name in Makefile
- rename flag LED_SET_HSV to LED_SET_COLOR
- rename is_off to led_is_off
- rename led_validate-brightness to led_confine_brightness
- rename variable in led_confine_brightness
- add dummy enum led_brightness value to enforce 32bit enum
- rename led-hsv-core.c to led-color-core.c
- move check of provided brightness value to led_confine_brightness
v4:
- change config symbol name to LEDS_RGB
- change name of new file to led-rgb-core.c
- factor out part of led_confine_brightness
- change led_is_off to __is_set_brightness
- in led_set_software_blink pass led_cdev->max_brightness instead of LED_FULL
- rename LED_SET_COLOR to LED_SET_HUE_SAT
v5:
- change "RGB Color LED" to "RGB LED" in Kconfig
- move definition of LED_HUE_SAT_MASK to drivers/leds/leds.h
- rename LED_DEV_CAP_HSV to LED_DEV_CAP_RGB
v6:
- no change
v7:
- removed "Color" from RGB Color LED in commit message and title
- don't include linux/kernel.h in led-rgb-core.c
- keep definition of LED_DEV_CAP_[] flags together
---
 drivers/leds/Kconfig| 11 +++
 drivers/leds/Makefile   |  4 +++-
 drivers/leds/led-class.c|  2 +-
 drivers/leds/led-core.c | 16 +---
 drivers/leds/led-rgb-core.c | 39 +++
 drivers/leds/leds.h | 18 ++
 include/linux/leds.h| 11 ++-
 7 files changed, 91 insertions(+), 10 deletions(-)
 create mode 100644 drivers/leds/led-rgb-core.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 7f940c2..5b1c852 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -13,6 +13,13 @@ menuconfig NEW_LEDS
 
 if NEW_LEDS
 
+config LEDS_RGB
+   bool "RGB LED Support"
+   help
+This option enables support for RGB Color LED devices.
+Sysfs attribute brightness is interpreted as a HSV color value.
+For details see Documentation/leds/leds-class.txt.
+
 config LEDS_CLASS
tristate "LED Class Support"
help
@@ -29,6 +36,10 @@ config LEDS_CLASS_FLASH
  for the flash related features of a LED device. It can be built
  as a module.
 
+if LEDS_RGB
+comment "RGB LED drivers"
+endif # LEDS_RGB
+
 comment "LED drivers"
 
 config LEDS_88PM860X
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e9d5309..cc3676f 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -1,6 +1,8 @@
 
 # LED Core
-obj-$(CONFIG_NEW_LEDS) += led-core.o
+obj-$(CONFIG_NEW_LEDS) += led-core-objs.o
+led-core-objs-y:= led-core.o
+led-core-objs-$(CONFIG_LEDS_RGB)   += led-rgb-core.o
 obj-$(CONFIG_LEDS_CLASS)   += led-class.o
 obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o
 obj-$(CONFIG_LEDS_TRIGGERS)+= led-triggers.o
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index aa84e5b..007a5b3 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -53,7 +53,7 @@ static ssize_t brightness_store(struct device *dev,
if (ret)
goto unlock;
 
-   if (state == LED_OFF)
+   if (!__is_brightness_set(state))
led_trigger_remove(led_cdev);
led_set_brightness(led_cdev, state);
 
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 3495d5d..e75b0c8 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -62,7 +62,7 @@ static void led_timer_function(unsigned long data)
}
 
brightness 

Re: [PATCH v5 4/4] leds: core: add support for RGB LED's

[Heiner Kallweit]

Am 04.03.2016 um 10:05 schrieb Jacek Anaszewski:
> On 03/01/2016 10:36 PM, Heiner Kallweit wrote:
>> Export a function to convert HSV color values to RGB.
>> It's intended to be called by drivers for RGB LEDs.
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>> ---
>> v2:
>> - move hsv -> rgb conversion to separate file
>> - remove flag LED_DEV_CAP_RGB
>> v3:
>> - call led_hsv_to_rgb only if LED_DEV_CAP_HSV is set
>>This is needed in cases when we have monochrome and color LEDs
>>as well in a system.
>> v4:
>> - Export led_hsv_to_rgb and let the device driver call it instead
>>of doing the conversion in the core
>> v5:
>> - don't ignore led_cdev->brightness_get silently if LED_DEV_CAP_RGB
>>is set but warn
>> ---
>>   drivers/leds/led-class.c|  7 +++
>>   drivers/leds/led-rgb-core.c | 36 
>>   include/linux/leds.h|  8 
>>   3 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 8a3748a..a4b144e 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -193,6 +193,13 @@ int led_classdev_register(struct device *parent, struct 
>> led_classdev *led_cdev)
>>   char name[64];
>>   int ret;
>>
>> +/*
>> + * for now reading back the color is not supported as multiple
>> + * HSV -> RGB -> HSV conversions may distort the color due to
>> + * rounding issues in the conversion algorithm
>> + */
> 
> Does the "for now" mean that you plan on supporting it in the future?
> 
It was meant as "until somebody has an idea how to implement the conversion
HSV -> RGB -> HSV in a way that always results in the original values".
I'll remove the "for now".

>> +WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get);
>> +
> 
> Could you move this warning to the line 216?
> 
>>   ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>>   if (ret < 0)
>>   return ret;
>> diff --git a/drivers/leds/led-rgb-core.c b/drivers/leds/led-rgb-core.c
>> index f6591f1..2b18d4c 100644
>> --- a/drivers/leds/led-rgb-core.c
>> +++ b/drivers/leds/led-rgb-core.c
>> @@ -38,3 +38,39 @@ enum led_brightness led_confine_brightness(struct 
>> led_classdev *led_cdev,
>>   return brightness |
>>  min(value & LED_BRIGHTNESS_MASK, led_cdev->max_brightness);
>>   }
>> +
>> +enum led_brightness led_hsv_to_rgb(enum led_brightness hsv)
>> +{
>> +int h = min_t(int, (hsv >> 16) & 0xff, 251);
>> +int s = (hsv >> 8) & 0xff;
>> +int v = hsv & 0xff;
>> +int f, p, q, t, r, g, b;
>> +
>> +if (!v)
>> +return 0;
>> +if (!s)
>> +return (v << 16) + (v << 8) + v;
>> +
>> +f = DIV_ROUND_CLOSEST((h % 42) * 255, 42);
>> +p = v - DIV_ROUND_CLOSEST(s * v, 255);
>> +q = v - DIV_ROUND_CLOSEST(f * s * v, 255 * 255);
>> +t = v - DIV_ROUND_CLOSEST((255 - f) * s * v, 255 * 255);
>> +
>> +switch (h / 42) {
>> +case 0:
>> +r = v; g = t; b = p; break;
>> +case 1:
>> +r = q; g = v; b = p; break;
>> +case 2:
>> +r = p; g = v; b = t; break;
>> +case 3:
>> +r = p; g = q; b = v; break;
>> +case 4:
>> +r = t; g = p; b = v; break;
>> +case 5:
>> +r = v; g = p; b = q; break;
>> +}
>> +
>> +return (r << 16) + (g << 8) + b;
>> +}
>> +EXPORT_SYMBOL_GPL(led_hsv_to_rgb);
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index bbf24bb..82b3477 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -226,6 +226,14 @@ static inline bool led_sysfs_is_disabled(struct 
>> led_classdev *led_cdev)
>>   return led_cdev->flags & LED_SYSFS_DISABLE;
>>   }
>>
>> +/**
>> + * led_hsv_to_rgb - convert a hsv color value to rgb color model
>> + * @hsv: the hsv value to convert
>> + *
>> + * Returns: the resulting rgb value
>> + */
>> +enum led_brightness led_hsv_to_rgb(enum led_brightness hsv);
>> +
>>   /*
>>* LED Triggers
>>*/
>>
> 
> 

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

[PATCH v7 3/5] leds: core: add documentation for RGB extension

[Heiner Kallweit]

Document the RGB extension in Documentation/leds/leds-class.txt

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- introduced to patch series
v3:
- document extension in more detail
v4:
- Better explain why flag LED_SET_HUE_SAT is needed
v5:
- no changes
v6:
- no changes
v7:
- move Documentation/ABI change into separate patch
- change "colour" to "RGB" in more places
---
 Documentation/leds/leds-class.txt | 27 ++-
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/leds/leds-class.txt 
b/Documentation/leds/leds-class.txt
index d406d98..003c098 100644
--- a/Documentation/leds/leds-class.txt
+++ b/Documentation/leds/leds-class.txt
@@ -8,6 +8,22 @@ LED is defined in max_brightness file. The brightness file 
will set the brightne
 of the LED (taking a value 0-max_brightness). Most LEDs don't have hardware
 brightness support so will just be turned on for non-zero brightness settings.
 
+If a driver uses the RGB extension of the LED core then the brightness
+file can be used to set hue / saturation / value. The brightness value is
+interpreted as: <000F>
+Usage of the least byte is identical to monochrome mode. Saturation can be
+0-255 and hue 0-251 (Colour circle is mapped to 0-252).
+If hue and saturation both are 0 the current colour is preserved and only
+the brightness is set. This ensures backwards compatibility with monochrome
+mode, e.g. for led_set_brightness() calls from triggers.
+However we might want to have the option to set all HSV components, even
+if hue and saturation both are 0 (e.g. via brightness sysfs attribute).
+Use case: Set color to white (hue = 0 and saturation = 0).
+Therefore the default behaviour can be overridden with flag F 
(LED_SET_HUE_SAT).
+If this flag is set then hue and saturation are not checked for being 0 and
+the color components are set unconditionally. Example:
+0x01ff sets the LED to white color with full brightness.
+
 The class also introduces the optional concept of an LED trigger. A trigger
 is a kernel based source of led events. Triggers can either be simple or
 complex. A simple trigger isn't configurable and is designed to slot into
@@ -45,11 +61,12 @@ Is currently of the form:
 
 "devicename:colour:function"
 
-There have been calls for LED properties such as colour to be exported as
-individual led class attributes. As a solution which doesn't incur as much
-overhead, I suggest these become part of the device name. The naming scheme
-above leaves scope for further attributes should they be needed. If sections
-of the name don't apply, just leave that section blank.
+If the RGB extension is used hsv / rgb can be used instead of a specific
+colour.  There have been calls for LED properties such as colour to be
+exported as individual led class attributes. As a solution which doesn't
+incur as much overhead, I suggest these become part of the device name.
+The naming scheme above leaves scope for further attributes should they be
+needed. If sections of the name don't apply, just leave that section blank.
 
 
 Brightness setting API
-- 
2.7.1


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

[PATCH v7 2/5] leds: core: add color LED sysfs extension

[Heiner Kallweit]

Extend brightness sysfs property handling to deal with monochrome
and color mode as well.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- split from patch 1
v3:
- moved one change (led_is_off) to patch 1
v4:
- changed printf format string to %#.6x
v5:
- no changes
v6:
- no changes
v7:
- no changes
---
 drivers/leds/led-class.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 007a5b3..8a3748a 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -32,7 +32,10 @@ static ssize_t brightness_show(struct device *dev,
/* no lock needed for this */
led_update_brightness(led_cdev);
 
-   return sprintf(buf, "%u\n", led_cdev->brightness);
+   if (led_cdev->brightness > LED_FULL)
+   return sprintf(buf, "%#.6x\n", led_cdev->brightness);
+   else
+   return sprintf(buf, "%u\n", led_cdev->brightness);
 }
 
 static ssize_t brightness_store(struct device *dev,
@@ -49,7 +52,7 @@ static ssize_t brightness_store(struct device *dev,
goto unlock;
}
 
-   ret = kstrtoul(buf, 10, );
+   ret = kstrtoul(buf, 0, );
if (ret)
goto unlock;
 
-- 
2.7.1


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

[PATCH v7 4/5] leds: core: document ABI change for RGB extension

[Heiner Kallweit]

Document the ABI change in Documentation/ABI/testing/sysfs-class-led.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v7:
- separated from patch 3
---
 Documentation/ABI/testing/sysfs-class-led | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-led 
b/Documentation/ABI/testing/sysfs-class-led
index 3646ec8..20357d5 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -7,6 +7,17 @@ Description:
have hardware brightness support so will just be turned on for
non-zero brightness settings. The value is between 0 and
/sys/class/leds//max_brightness.
+   If a driver uses the RGB LED feature then this attribute is
+   interpreted as a HSV color model value:
+   <000F>
+   Usage of the least byte is identical to monochrome mode.
+   Saturation can be 0-255 and hue 0-251 (Colour circle is mapped
+   to 0-252). If hue and saturation both are 0 the current colour
+   is preserved and only the brightness is set. This ensures
+   backwards compatibility with monochrome mode, e.g. for
+   led_set_brightness() calls from triggers.
+   Flag F enables setting hue and saturation even if both are 0.
+   For details and examples see Documentation/leds/leds-class.txt
 
 What:  /sys/class/leds//max_brightness
 Date:  March 2006
-- 
2.7.1


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

[PATCH v7 5/5] leds: core: add support for RGB LED's

[Heiner Kallweit]

Export a function to convert HSV color values to RGB.
It's intended to be called by drivers for RGB LEDs.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- move hsv -> rgb conversion to separate file
- remove flag LED_DEV_CAP_RGB
v3:
- call led_hsv_to_rgb only if LED_DEV_CAP_HSV is set
  This is needed in cases when we have monochrome and color LEDs
  as well in a system.
v4:
- Export led_hsv_to_rgb and let the device driver call it instead
  of doing the conversion in the core
v5:
- don't ignore led_cdev->brightness_get silently if LED_DEV_CAP_RGB
  is set but warn
v6:
- no changes
v7:
- remove "for now" in WARN_ON comment
- move position of WARN_ON
---
 drivers/leds/led-class.c|  6 ++
 drivers/leds/led-rgb-core.c | 36 
 include/linux/leds.h|  8 
 3 files changed, 50 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 8a3748a..adcd8f6 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -205,6 +205,12 @@ int led_classdev_register(struct device *parent, struct 
led_classdev *led_cdev)
if (ret)
dev_warn(parent, "Led %s renamed to %s due to name collision",
led_cdev->name, dev_name(led_cdev->dev));
+   /*
+* Reading back the color is not supported as multiple
+* HSV -> RGB -> HSV conversions may distort the color due to
+* rounding issues in the conversion algorithm
+*/
+   WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get);
 
 #ifdef CONFIG_LEDS_TRIGGERS
init_rwsem(_cdev->trigger_lock);
diff --git a/drivers/leds/led-rgb-core.c b/drivers/leds/led-rgb-core.c
index cbd8b35..cdb7ba6 100644
--- a/drivers/leds/led-rgb-core.c
+++ b/drivers/leds/led-rgb-core.c
@@ -37,3 +37,39 @@ enum led_brightness led_confine_brightness(struct 
led_classdev *led_cdev,
return brightness |
   min(value & LED_BRIGHTNESS_MASK, led_cdev->max_brightness);
 }
+
+enum led_brightness led_hsv_to_rgb(enum led_brightness hsv)
+{
+   int h = min_t(int, (hsv >> 16) & 0xff, 251);
+   int s = (hsv >> 8) & 0xff;
+   int v = hsv & 0xff;
+   int f, p, q, t, r, g, b;
+
+   if (!v)
+   return 0;
+   if (!s)
+   return (v << 16) + (v << 8) + v;
+
+   f = DIV_ROUND_CLOSEST((h % 42) * 255, 42);
+   p = v - DIV_ROUND_CLOSEST(s * v, 255);
+   q = v - DIV_ROUND_CLOSEST(f * s * v, 255 * 255);
+   t = v - DIV_ROUND_CLOSEST((255 - f) * s * v, 255 * 255);
+
+   switch (h / 42) {
+   case 0:
+   r = v; g = t; b = p; break;
+   case 1:
+   r = q; g = v; b = p; break;
+   case 2:
+   r = p; g = v; b = t; break;
+   case 3:
+   r = p; g = q; b = v; break;
+   case 4:
+   r = t; g = p; b = v; break;
+   case 5:
+   r = v; g = p; b = q; break;
+   }
+
+   return (r << 16) + (g << 8) + b;
+}
+EXPORT_SYMBOL_GPL(led_hsv_to_rgb);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 58e8299..58e22e6 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -226,6 +226,14 @@ static inline bool led_sysfs_is_disabled(struct 
led_classdev *led_cdev)
return led_cdev->flags & LED_SYSFS_DISABLE;
 }
 
+/**
+ * led_hsv_to_rgb - convert a hsv color value to rgb color model
+ * @hsv: the hsv value to convert
+ *
+ * Returns: the resulting rgb value
+ */
+enum led_brightness led_hsv_to_rgb(enum led_brightness hsv);
+
 /*
  * LED Triggers
  */
-- 
2.7.1


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

[PATCH] hid: thingm: change driver to use RGB LED core extension

[Heiner Kallweit]

Based on the proposed RGB LED core extension the thingm driver was
changed to make use of this extension. It allows to simplify
the code a lot. For now primary purpose of this patch is to facilitate
testing of the RGB LED core extension.

I didn't find a way to split the patch into smaller parts as either
you use the RGB LED extension or not. However this shouldn't be really
an issue as the resulting code is relatively short and simple.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/hid/hid-thingm.c | 131 +--
 1 file changed, 36 insertions(+), 95 deletions(-)

diff --git a/drivers/hid/hid-thingm.c b/drivers/hid/hid-thingm.c
index 847a497..07b9f1b 100644
--- a/drivers/hid/hid-thingm.c
+++ b/drivers/hid/hid-thingm.c
@@ -19,6 +19,7 @@
 
 #define REPORT_ID  1
 #define REPORT_SIZE9
+#define NUMRGB_MAX 2
 
 /* Firmware major number of supported devices */
 #define THINGM_MAJOR_MK1   '1'
@@ -42,33 +43,26 @@ static const struct thingm_fwinfo thingm_fwinfo[] = {
}
 };
 
-/* A red, green or blue channel, part of an RGB chip */
 struct thingm_led {
-   struct thingm_rgb *rgb;
-   struct led_classdev ldev;
-   char name[32];
-};
-
-/* Basically a WS2812 5050 RGB LED chip */
-struct thingm_rgb {
-   struct thingm_device *tdev;
-   struct thingm_led red;
-   struct thingm_led green;
-   struct thingm_led blue;
-   u8 num;
+   struct led_classdev cdev;
+   charname[32];
+   unsignedled_num;
+   struct thingm_device*tdev;
 };
 
 struct thingm_device {
-   struct hid_device *hdev;
+   struct hid_device   *hdev;
struct {
char major;
char minor;
-   } version;
-   const struct thingm_fwinfo *fwinfo;
-   struct mutex lock;
-   struct thingm_rgb *rgb;
+   }   version;
+   const struct thingm_fwinfo  *fwinfo;
+   struct mutexlock;
+   struct thingm_led   tled[NUMRGB_MAX];
 };
 
+#define to_thingm_led(arg) container_of((arg), struct thingm_led, cdev)
+
 static int thingm_send(struct thingm_device *tdev, u8 buf[REPORT_SIZE])
 {
int ret;
@@ -133,86 +127,33 @@ static int thingm_version(struct thingm_device *tdev)
return 0;
 }
 
-static int thingm_write_color(struct thingm_rgb *rgb)
+static int thingm_brightness_set_blocking(struct led_classdev *cdev,
+ enum led_brightness value)
 {
-   u8 buf[REPORT_SIZE] = { REPORT_ID, 'c', 0, 0, 0, 0, 0, rgb->num, 0 };
-
-   buf[2] = rgb->red.ldev.brightness;
-   buf[3] = rgb->green.ldev.brightness;
-   buf[4] = rgb->blue.ldev.brightness;
-
-   return thingm_send(rgb->tdev, buf);
-}
+   struct thingm_led *tled = to_thingm_led(cdev);
+   u8 buf[REPORT_SIZE] = { REPORT_ID, 'c' };
 
-static int thingm_led_set(struct led_classdev *ldev,
- enum led_brightness brightness)
-{
-   struct thingm_led *led = container_of(ldev, struct thingm_led, ldev);
-   int ret;
+   value = led_hsv_to_rgb(value);
 
-   ret = thingm_write_color(led->rgb);
-   if (ret)
-   hid_err(led->rgb->tdev->hdev, "failed to write color\n");
+   buf[2] = (value >> 16) & 0xff;
+   buf[3] = (value >> 8) & 0xff;
+   buf[4] = value & 0xff;
+   buf[7] = tled->led_num;
 
-   return ret;
-}
-
-static int thingm_init_rgb(struct thingm_rgb *rgb)
-{
-   const int minor = ((struct hidraw *) rgb->tdev->hdev->hidraw)->minor;
-   int err;
-
-   /* Register the red diode */
-   snprintf(rgb->red.name, sizeof(rgb->red.name),
-   "thingm%d:red:led%d", minor, rgb->num);
-   rgb->red.ldev.name = rgb->red.name;
-   rgb->red.ldev.max_brightness = 255;
-   rgb->red.ldev.brightness_set_blocking = thingm_led_set;
-   rgb->red.rgb = rgb;
-
-   err = devm_led_classdev_register(>tdev->hdev->dev,
->red.ldev);
-   if (err)
-   return err;
-
-   /* Register the green diode */
-   snprintf(rgb->green.name, sizeof(rgb->green.name),
-   "thingm%d:green:led%d", minor, rgb->num);
-   rgb->green.ldev.name = rgb->green.name;
-   rgb->green.ldev.max_brightness = 255;
-   rgb->green.ldev.brightness_set_blocking = thingm_led_set;
-   rgb->green.rgb = rgb;
-
-   err = devm_led_classdev_register(>tdev->hdev->dev,
->green.ldev);
-   if (err)
-   return err;
-
-   /* Register the blue diode */
-   snprintf(rgb->blue.name, sizeof(rgb->

[PATCH v6 3/4] leds: core: add documentation for color extension

[Heiner Kallweit]

Document the color extension in Documentation/leds/leds-class.txt

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- introduced to patch series
v3:
- document extension in more detail
v4:
- Better explain why flag LED_SET_HUE_SAT is needed
v5:
- no changes
v6:
- add docu for this feature to Documentation/ABI/testing/sysfs-class-led
---
 Documentation/ABI/testing/sysfs-class-led | 11 +++
 Documentation/leds/leds-class.txt | 27 ++-
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-led 
b/Documentation/ABI/testing/sysfs-class-led
index 3646ec8..20357d5 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -7,6 +7,17 @@ Description:
have hardware brightness support so will just be turned on for
non-zero brightness settings. The value is between 0 and
/sys/class/leds//max_brightness.
+   If a driver uses the RGB LED feature then this attribute is
+   interpreted as a HSV color model value:
+   <000F>
+   Usage of the least byte is identical to monochrome mode.
+   Saturation can be 0-255 and hue 0-251 (Colour circle is mapped
+   to 0-252). If hue and saturation both are 0 the current colour
+   is preserved and only the brightness is set. This ensures
+   backwards compatibility with monochrome mode, e.g. for
+   led_set_brightness() calls from triggers.
+   Flag F enables setting hue and saturation even if both are 0.
+   For details and examples see Documentation/leds/leds-class.txt
 
 What:  /sys/class/leds//max_brightness
 Date:  March 2006
diff --git a/Documentation/leds/leds-class.txt 
b/Documentation/leds/leds-class.txt
index d406d98..b1e4cba 100644
--- a/Documentation/leds/leds-class.txt
+++ b/Documentation/leds/leds-class.txt
@@ -8,6 +8,22 @@ LED is defined in max_brightness file. The brightness file 
will set the brightne
 of the LED (taking a value 0-max_brightness). Most LEDs don't have hardware
 brightness support so will just be turned on for non-zero brightness settings.
 
+If a driver uses the colour extension of the LED core then the brightness
+file can be used to set hue / saturation / value. The brightness value is
+interpreted as: <000F>
+Usage of the least byte is identical to monochrome mode. Saturation can be
+0-255 and hue 0-251 (Colour circle is mapped to 0-252).
+If hue and saturation both are 0 the current colour is preserved and only
+the brightness is set. This ensures backwards compatibility with monochrome
+mode, e.g. for led_set_brightness() calls from triggers.
+However we might want to have the option to set all HSV components, even
+if hue and saturation both are 0 (e.g. via brightness sysfs attribute).
+Use case: Set color to white (hue = 0 and saturation = 0).
+Therefore the default behaviour can be overridden with flag F 
(LED_SET_HUE_SAT).
+If this flag is set then hue and saturation are not checked for being 0 and
+the color components are set unconditionally. Example:
+0x01ff sets the LED to white color with full brightness.
+
 The class also introduces the optional concept of an LED trigger. A trigger
 is a kernel based source of led events. Triggers can either be simple or
 complex. A simple trigger isn't configurable and is designed to slot into
@@ -45,11 +61,12 @@ Is currently of the form:
 
 "devicename:colour:function"
 
-There have been calls for LED properties such as colour to be exported as
-individual led class attributes. As a solution which doesn't incur as much
-overhead, I suggest these become part of the device name. The naming scheme
-above leaves scope for further attributes should they be needed. If sections
-of the name don't apply, just leave that section blank.
+If the colour extension is used hsv / rgb can be used instead of a specific
+colour.  There have been calls for LED properties such as colour to be
+exported as individual led class attributes. As a solution which doesn't
+incur as much overhead, I suggest these become part of the device name.
+The naming scheme above leaves scope for further attributes should they be
+needed. If sections of the name don't apply, just leave that section blank.
 
 
 Brightness setting API
-- 
2.7.1


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

Re: [PATCH v5 3/4] leds: core: add documentation for color extension

[Heiner Kallweit]

On Wed, Mar 2, 2016 at 9:38 AM, Jacek Anaszewski
<j.anaszew...@samsung.com> wrote:
> On 03/01/2016 10:41 PM, Greg KH wrote:
>>
>> On Tue, Mar 01, 2016 at 10:29:31PM +0100, Heiner Kallweit wrote:
>>>
>>> Document the color extension in Documentation/leds/leds-class.txt
>>>
>>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>>> ---
>>> v2:
>>> - introduced to patch series
>>> v3:
>>> - document extension in more detail
>>> v4:
>>> - Better explain why flag LED_SET_HUE_SAT is needed
>>> v5:
>>> - no changes
>>> ---
>>>   Documentation/leds/leds-class.txt | 27 ++-
>>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>
>>
>> What aboud Documentation/ABI/ ?
>>
Thanks for the hint. Will update also this part of the documentation.

>>
>
> Right, I haven't pushed for this so far, but now, as the functionality
> seems to be reaching its final shape, please update also:
>
> Documentation/ABI/testing/sysfs-class-led
>
>
> --
> Best regards,
> Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 4/4] leds: core: add support for RGB LED's

[Heiner Kallweit]

Export a function to convert HSV color values to RGB.
It's intended to be called by drivers for RGB LEDs.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- move hsv -> rgb conversion to separate file
- remove flag LED_DEV_CAP_RGB
v3:
- call led_hsv_to_rgb only if LED_DEV_CAP_HSV is set
  This is needed in cases when we have monochrome and color LEDs
  as well in a system.
v4:
- Export led_hsv_to_rgb and let the device driver call it instead
  of doing the conversion in the core
v5:
- don't ignore led_cdev->brightness_get silently if LED_DEV_CAP_RGB
  is set but warn
---
 drivers/leds/led-class.c|  7 +++
 drivers/leds/led-rgb-core.c | 36 
 include/linux/leds.h|  8 
 3 files changed, 51 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 8a3748a..a4b144e 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -193,6 +193,13 @@ int led_classdev_register(struct device *parent, struct 
led_classdev *led_cdev)
char name[64];
int ret;
 
+   /*
+* for now reading back the color is not supported as multiple
+* HSV -> RGB -> HSV conversions may distort the color due to
+* rounding issues in the conversion algorithm
+*/
+   WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get);
+
ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
if (ret < 0)
return ret;
diff --git a/drivers/leds/led-rgb-core.c b/drivers/leds/led-rgb-core.c
index f6591f1..2b18d4c 100644
--- a/drivers/leds/led-rgb-core.c
+++ b/drivers/leds/led-rgb-core.c
@@ -38,3 +38,39 @@ enum led_brightness led_confine_brightness(struct 
led_classdev *led_cdev,
return brightness |
   min(value & LED_BRIGHTNESS_MASK, led_cdev->max_brightness);
 }
+
+enum led_brightness led_hsv_to_rgb(enum led_brightness hsv)
+{
+   int h = min_t(int, (hsv >> 16) & 0xff, 251);
+   int s = (hsv >> 8) & 0xff;
+   int v = hsv & 0xff;
+   int f, p, q, t, r, g, b;
+
+   if (!v)
+   return 0;
+   if (!s)
+   return (v << 16) + (v << 8) + v;
+
+   f = DIV_ROUND_CLOSEST((h % 42) * 255, 42);
+   p = v - DIV_ROUND_CLOSEST(s * v, 255);
+   q = v - DIV_ROUND_CLOSEST(f * s * v, 255 * 255);
+   t = v - DIV_ROUND_CLOSEST((255 - f) * s * v, 255 * 255);
+
+   switch (h / 42) {
+   case 0:
+   r = v; g = t; b = p; break;
+   case 1:
+   r = q; g = v; b = p; break;
+   case 2:
+   r = p; g = v; b = t; break;
+   case 3:
+   r = p; g = q; b = v; break;
+   case 4:
+   r = t; g = p; b = v; break;
+   case 5:
+   r = v; g = p; b = q; break;
+   }
+
+   return (r << 16) + (g << 8) + b;
+}
+EXPORT_SYMBOL_GPL(led_hsv_to_rgb);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index bbf24bb..82b3477 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -226,6 +226,14 @@ static inline bool led_sysfs_is_disabled(struct 
led_classdev *led_cdev)
return led_cdev->flags & LED_SYSFS_DISABLE;
 }
 
+/**
+ * led_hsv_to_rgb - convert a hsv color value to rgb color model
+ * @hsv: the hsv value to convert
+ *
+ * Returns: the resulting rgb value
+ */
+enum led_brightness led_hsv_to_rgb(enum led_brightness hsv);
+
 /*
  * LED Triggers
  */
-- 
2.7.1

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

[PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Heiner Kallweit]

Add generic support for RGB Color LED's.

Basic idea is to use enum led_brightness also for the hue and saturation
color components.This allows to implement the color extension w/o
changes to struct led_classdev.

Select LEDS_RGB to enable building drivers using the RGB extension.

Flag LED_SET_HUE_SAT allows to specify that hue / saturation
should be overridden even if the provided values are zero.

Some examples for writing values to /sys/class/leds//brightness:
(now also hex notation can be used)

255 -> set full brightness and keep existing color if set
0 -> switch LED off but keep existing color so that it can be restored
 if the LED is switched on again later
0x100 -> switch LED off and set also hue and saturation to 0
0x00 -> set full brightness, full saturation and set hue to 0 (red)

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- move extension-specific code into a separate source file and
  introduce config symbol LEDS_HSV for it
- create separate patch for the extension to sysfs
- use variable name led_cdev as in the rest if the core
- rename to_hsv to led_validate_brightness
- rename LED_BRIGHTNESS_SET_COLOR to LED_SET_HSV
- introduce helper is_off for checking whether V part
  of a HSV value is zero
v3:
- change Kconfig to use depend instead of select, add help message,
  and change config symbol to LEDS_COLOR
- change LED core object file name in Makefile
- rename flag LED_SET_HSV to LED_SET_COLOR
- rename is_off to led_is_off
- rename led_validate-brightness to led_confine_brightness
- rename variable in led_confine_brightness
- add dummy enum led_brightness value to enforce 32bit enum
- rename led-hsv-core.c to led-color-core.c
- move check of provided brightness value to led_confine_brightness
v4:
- change config symbol name to LEDS_RGB
- change name of new file to led-rgb-core.c
- factor out part of led_confine_brightness
- change led_is_off to __is_set_brightness
- in led_set_software_blink pass led_cdev->max_brightness instead of LED_FULL
- rename LED_SET_COLOR to LED_SET_HUE_SAT
v5:
- change "RGB Color LED" to "RGB LED" in Kconfig
- move definition of LED_HUE_SAT_MASK to drivers/leds/leds.h
- rename LED_DEV_CAP_HSV to LED_DEV_CAP_RGB
---
 drivers/leds/Kconfig| 11 +++
 drivers/leds/Makefile   |  4 +++-
 drivers/leds/led-class.c|  2 +-
 drivers/leds/led-core.c | 16 +---
 drivers/leds/led-rgb-core.c | 40 
 drivers/leds/leds.h | 18 ++
 include/linux/leds.h|  9 +
 7 files changed, 91 insertions(+), 9 deletions(-)
 create mode 100644 drivers/leds/led-rgb-core.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 7f940c2..5b1c852 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -13,6 +13,13 @@ menuconfig NEW_LEDS
 
 if NEW_LEDS
 
+config LEDS_RGB
+   bool "RGB LED Support"
+   help
+This option enables support for RGB LED devices.
+Sysfs attribute brightness is interpreted as a HSV color value.
+For details see Documentation/leds/leds-class.txt.
+
 config LEDS_CLASS
tristate "LED Class Support"
help
@@ -29,6 +36,10 @@ config LEDS_CLASS_FLASH
  for the flash related features of a LED device. It can be built
  as a module.
 
+if LEDS_RGB
+comment "RGB LED drivers"
+endif # LEDS_RGB
+
 comment "LED drivers"
 
 config LEDS_88PM860X
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e9d5309..cc3676f 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -1,6 +1,8 @@
 
 # LED Core
-obj-$(CONFIG_NEW_LEDS) += led-core.o
+obj-$(CONFIG_NEW_LEDS) += led-core-objs.o
+led-core-objs-y:= led-core.o
+led-core-objs-$(CONFIG_LEDS_RGB)   += led-rgb-core.o
 obj-$(CONFIG_LEDS_CLASS)   += led-class.o
 obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o
 obj-$(CONFIG_LEDS_TRIGGERS)+= led-triggers.o
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index aa84e5b..007a5b3 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -53,7 +53,7 @@ static ssize_t brightness_store(struct device *dev,
if (ret)
goto unlock;
 
-   if (state == LED_OFF)
+   if (!__is_brightness_set(state))
led_trigger_remove(led_cdev);
led_set_brightness(led_cdev, state);
 
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 3495d5d..e75b0c8 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -62,7 +62,7 @@ static void led_timer_function(unsigned long data)
}
 
brightness = led_get_brightness(led_cdev);
-   if (!brightness) {
+   if (!__is_brightness_set(brightness)) {
/* Time to switch the LED on. */
brightness = led_cdev->

[PATCH v5 3/4] leds: core: add documentation for color extension

[Heiner Kallweit]

Document the color extension in Documentation/leds/leds-class.txt

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- introduced to patch series
v3:
- document extension in more detail
v4:
- Better explain why flag LED_SET_HUE_SAT is needed
v5:
- no changes
---
 Documentation/leds/leds-class.txt | 27 ++-
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/leds/leds-class.txt 
b/Documentation/leds/leds-class.txt
index d406d98..b1e4cba 100644
--- a/Documentation/leds/leds-class.txt
+++ b/Documentation/leds/leds-class.txt
@@ -8,6 +8,22 @@ LED is defined in max_brightness file. The brightness file 
will set the brightne
 of the LED (taking a value 0-max_brightness). Most LEDs don't have hardware
 brightness support so will just be turned on for non-zero brightness settings.
 
+If a driver uses the colour extension of the LED core then the brightness
+file can be used to set hue / saturation / value. The brightness value is
+interpreted as: <000F>
+Usage of the least byte is identical to monochrome mode. Saturation can be
+0-255 and hue 0-251 (Colour circle is mapped to 0-252).
+If hue and saturation both are 0 the current colour is preserved and only
+the brightness is set. This ensures backwards compatibility with monochrome
+mode, e.g. for led_set_brightness() calls from triggers.
+However we might want to have the option to set all HSV components, even
+if hue and saturation both are 0 (e.g. via brightness sysfs attribute).
+Use case: Set color to white (hue = 0 and saturation = 0).
+Therefore the default behaviour can be overridden with flag F 
(LED_SET_HUE_SAT).
+If this flag is set then hue and saturation are not checked for being 0 and
+the color components are set unconditionally. Example:
+0x01ff sets the LED to white color with full brightness.
+
 The class also introduces the optional concept of an LED trigger. A trigger
 is a kernel based source of led events. Triggers can either be simple or
 complex. A simple trigger isn't configurable and is designed to slot into
@@ -45,11 +61,12 @@ Is currently of the form:
 
 "devicename:colour:function"
 
-There have been calls for LED properties such as colour to be exported as
-individual led class attributes. As a solution which doesn't incur as much
-overhead, I suggest these become part of the device name. The naming scheme
-above leaves scope for further attributes should they be needed. If sections
-of the name don't apply, just leave that section blank.
+If the colour extension is used hsv / rgb can be used instead of a specific
+colour.  There have been calls for LED properties such as colour to be
+exported as individual led class attributes. As a solution which doesn't
+incur as much overhead, I suggest these become part of the device name.
+The naming scheme above leaves scope for further attributes should they be
+needed. If sections of the name don't apply, just leave that section blank.
 
 
 Brightness setting API
-- 
2.7.1


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

[PATCH v5 2/4] leds: core: add color LED sysfs extension

[Heiner Kallweit]

Extend brightness sysfs property handling to deal with monochrome
and color mode as well.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
v2:
- split from patch 1
v3:
- moved one change (led_is_off) to patch 1
v4:
- changed printf format string to %#.6x
v5:
- no changes
---
 drivers/leds/led-class.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 007a5b3..8a3748a 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -32,7 +32,10 @@ static ssize_t brightness_show(struct device *dev,
/* no lock needed for this */
led_update_brightness(led_cdev);
 
-   return sprintf(buf, "%u\n", led_cdev->brightness);
+   if (led_cdev->brightness > LED_FULL)
+   return sprintf(buf, "%#.6x\n", led_cdev->brightness);
+   else
+   return sprintf(buf, "%u\n", led_cdev->brightness);
 }
 
 static ssize_t brightness_store(struct device *dev,
@@ -49,7 +52,7 @@ static ssize_t brightness_store(struct device *dev,
goto unlock;
}
 
-   ret = kstrtoul(buf, 10, );
+   ret = kstrtoul(buf, 0, );
if (ret)
goto unlock;
 
-- 
2.7.1


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

Re: [PATCH v3 4/4] leds: core: add support for RGB LED's

[Heiner Kallweit]

Am 29.02.2016 um 10:57 schrieb Jacek Anaszewski:
> On 02/26/2016 11:36 PM, Heiner Kallweit wrote:
>> Am 25.02.2016 um 13:40 schrieb Jacek Anaszewski:
>>> On 02/23/2016 09:27 PM, Heiner Kallweit wrote:
>>>> Am 19.02.2016 um 14:59 schrieb Jacek Anaszewski:
>>>>> Hi Heiner,
>>>>>
>>>>> On 02/18/2016 10:31 PM, Heiner Kallweit wrote:
>>>>>> Add support for RGB LED's. Flag LED_DEV_CAP_HSV is used to instruct
>>>>>> the core to convert HSV to RGB on output.
>>>>>>
>>>>>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>>>>>> ---
>>>>>> v2:
>>>>>> - move hsv -> rgb conversion to separate file
>>>>>> - remove flag LED_DEV_CAP_RGB
>>>>>> v3:
>>>>>> - call led_hsv_to_rgb only if LED_DEV_CAP_HSV is set
>>>>>>  This is needed in cases when we have monochrome and color LEDs
>>>>>>  as well in a system.
>>>>>> ---
>>>>>> drivers/leds/led-color-core.c | 35 
>>>>>> +++
>>>>>> drivers/leds/led-core.c   | 17 -
>>>>>> drivers/leds/leds.h   |  1 +
>>>>>> 3 files changed, 52 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/leds/led-color-core.c 
>>>>>> b/drivers/leds/led-color-core.c
>>>>>> index b101f73..467beeb 100644
>>>>>> --- a/drivers/leds/led-color-core.c
>>>>>> +++ b/drivers/leds/led-color-core.c
>>>>>> @@ -31,3 +31,38 @@ enum led_brightness led_confine_brightness(struct 
>>>>>> led_classdev *led_cdev,
>>>>>> return brightness |
>>>>>>min(value & LED_BRIGHTNESS_MASK, 
>>>>>> led_cdev->max_brightness);
>>>>>> }
>>>>>> +
>>>>>> +enum led_brightness led_hsv_to_rgb(enum led_brightness hsv)
>>>>>> +{
>>>>>> +int h = min_t(int, (hsv >> 16) & 0xff, 251);
>>>>>> +int s = (hsv >> 8) & 0xff;
>>>>>> +int v = hsv & 0xff;
>>>>>> +int f, p, q, t, r, g, b;
>>>>>> +
>>>>>> +if (!v)
>>>>>> +return 0;
>>>>>> +if (!s)
>>>>>> +return (v << 16) + (v << 8) + v;
>>>>>> +
>>>>>> +f = DIV_ROUND_CLOSEST((h % 42) * 255, 42);
>>>>>> +p = v - DIV_ROUND_CLOSEST(s * v, 255);
>>>>>> +q = v - DIV_ROUND_CLOSEST(f * s * v, 255 * 255);
>>>>>> +t = v - DIV_ROUND_CLOSEST((255 - f) * s * v, 255 * 255);
>>>>>> +
>>>>>> +switch (h / 42) {
>>>>>> +case 0:
>>>>>> +r = v; g = t; b = p; break;
>>>>>> +case 1:
>>>>>> +r = q; g = v; b = p; break;
>>>>>> +case 2:
>>>>>> +r = p; g = v; b = t; break;
>>>>>> +case 3:
>>>>>> +r = p; g = q; b = v; break;
>>>>>> +case 4:
>>>>>> +r = t; g = p; b = v; break;
>>>>>> +case 5:
>>>>>> +r = v; g = p; b = q; break;
>>>>>> +}
>>>>>> +
>>>>>> +return (r << 16) + (g << 8) + b;
>>>>>> +}
>>>>>
>>>>> Do you plan to add a driver using Color LEDs Support?
>>>>> If so, it would be good to add it to this patch set,
>>>>> which would make testing this code easier.
>>>>>
>>>> I own a Thingm Blink(1) dual RGB LED USB device which I use for testing.
>>>> With the RGB extension in the kernel the driver could be significantly 
>>>> simplified
>>>> so that the changes are more or less a rewrite of the driver.
>>>> Because the USB device uses a HID interface the driver currently is located
>>>> under drivers/hid. IMHO that's questionable because the driver implements
>>>> LED functionality only. Moving this driver to drivers/leds could be an 
>>>> option.
>>>> When sending the patches for this driver to you for testing I'll set the 
>>>> maintainer
>>>> of the HID subsystem on cc.
>>

[PATCH RESEND2 2/3] usb: core: rename mutex usb_bus_list_lock to usb_bus_idr_lock

[Heiner Kallweit]

Now that usb_bus_list has been removed and switched to idr
rename the related mutex accordingly.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/core/devices.c |  6 +++---
 drivers/usb/core/hcd.c | 30 +++---
 drivers/usb/core/hub.c |  4 ++--
 drivers/usb/mon/mon_main.c |  4 ++--
 include/linux/usb/hcd.h|  2 +-
 5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
index 6118a04..ef04b50 100644
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -620,7 +620,7 @@ static ssize_t usb_device_read(struct file *file, char 
__user *buf,
if (!access_ok(VERIFY_WRITE, buf, nbytes))
return -EFAULT;
 
-   mutex_lock(_bus_list_lock);
+   mutex_lock(_bus_idr_lock);
/* print devices for all busses */
idr_for_each_entry(_bus_idr, bus, id) {
/* recurse through all children of the root hub */
@@ -631,12 +631,12 @@ static ssize_t usb_device_read(struct file *file, char 
__user *buf,
  bus->root_hub, bus, 0, 0, 0);
usb_unlock_device(bus->root_hub);
if (ret < 0) {
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
return ret;
}
total_written += ret;
}
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
return total_written;
 }
 
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index cf3eb22..0b8a910 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -97,8 +97,8 @@ EXPORT_SYMBOL_GPL (usb_bus_idr);
 #define USB_MAXBUS 64
 
 /* used when updating list of hcds */
-DEFINE_MUTEX(usb_bus_list_lock);   /* exported only for usbfs */
-EXPORT_SYMBOL_GPL (usb_bus_list_lock);
+DEFINE_MUTEX(usb_bus_idr_lock);/* exported only for usbfs */
+EXPORT_SYMBOL_GPL (usb_bus_idr_lock);
 
 /* used for controlling access to virtual root hubs */
 static DEFINE_SPINLOCK(hcd_root_hub_lock);
@@ -1014,14 +1014,14 @@ static int usb_register_bus(struct usb_bus *bus)
int result = -E2BIG;
int busnum;
 
-   mutex_lock(_bus_list_lock);
+   mutex_lock(_bus_idr_lock);
busnum = idr_alloc(_bus_idr, bus, 1, USB_MAXBUS, GFP_KERNEL);
if (busnum < 0) {
pr_err("%s: failed to get bus number\n", usbcore_name);
goto error_find_busnum;
}
bus->busnum = busnum;
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
 
usb_notify_add_bus(bus);
 
@@ -1030,7 +1030,7 @@ static int usb_register_bus(struct usb_bus *bus)
return 0;
 
 error_find_busnum:
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
return result;
 }
 
@@ -1051,9 +1051,9 @@ static void usb_deregister_bus (struct usb_bus *bus)
 * controller code, as well as having it call this when cleaning
 * itself up
 */
-   mutex_lock(_bus_list_lock);
+   mutex_lock(_bus_idr_lock);
idr_remove(_bus_idr, bus->busnum);
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
 
usb_notify_remove_bus(bus);
 }
@@ -1083,12 +1083,12 @@ static int register_root_hub(struct usb_hcd *hcd)
set_bit (devnum, usb_dev->bus->devmap.devicemap);
usb_set_device_state(usb_dev, USB_STATE_ADDRESS);
 
-   mutex_lock(_bus_list_lock);
+   mutex_lock(_bus_idr_lock);
 
usb_dev->ep0.desc.wMaxPacketSize = cpu_to_le16(64);
retval = usb_get_device_descriptor(usb_dev, USB_DT_DEVICE_SIZE);
if (retval != sizeof usb_dev->descriptor) {
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
dev_dbg (parent_dev, "can't read %s device descriptor %d\n",
dev_name(_dev->dev), retval);
return (retval < 0) ? retval : -EMSGSIZE;
@@ -1099,7 +1099,7 @@ static int register_root_hub(struct usb_hcd *hcd)
if (!retval) {
usb_dev->lpm_capable = usb_device_supports_lpm(usb_dev);
} else if (usb_dev->speed >= USB_SPEED_SUPER) {
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
dev_dbg(parent_dev, "can't read %s bos descriptor %d\n",
dev_name(_dev->dev), retval);
return retval;
@@ -1119,7 +1119,7 @@ static int register_root_hub(struct usb_hcd *hcd)
if (HCD_DEAD(hcd))
usb_hc_died (hcd);  /* This time clean up */
}
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
 
return retval;
 }
@@ -2

Re: [PATCH RESEND 2/3] usb: core: rename mutex usb_bus_list_lock to usb_bus_idr_lock

[Heiner Kallweit]

Am 03.02.2016 um 22:28 schrieb Greg Kroah-Hartman:
> On Mon, Jan 25, 2016 at 08:31:21PM +0100, Heiner Kallweit wrote:
>> Now that usb_bus_list has been removed and switched to idr
>> rename the related mutex accordingly.
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>> ---
>>  drivers/usb/core/devices.c |  6 +++---
>>  drivers/usb/core/hcd.c | 30 +++---
>>  drivers/usb/core/hub.c |  4 ++--
>>  drivers/usb/mon/mon_main.c |  4 ++--
>>  include/linux/usb/hcd.h|  2 +-
>>  5 files changed, 23 insertions(+), 23 deletions(-)
> 
> This patch fails to apply, so I didn't apply it, or the 3/3 patch.  Can
> you please rebase this and resend so that I can apply it?
> 
Sure, will resend them.

> thanks,
> 
> greg k-h
> 

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

[PATCH RESEND2 3/3] usb: r8a66597: add locking to r8a66597_check_detect_child

[Heiner Kallweit]

Use mutex usb_bus_idr_lock to protect idr_find.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/host/r8a66597-hcd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 1ef8873..bfa7fa3 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -2099,11 +2099,13 @@ static void r8a66597_check_detect_child(struct r8a66597 
*r8a66597,
 
memset(now_map, 0, sizeof(now_map));
 
+   mutex_lock(_bus_idr_lock);
bus = idr_find(_bus_idr, hcd->self.busnum);
if (bus && bus->root_hub) {
collect_usb_address_map(bus->root_hub, now_map);
update_usb_address_map(r8a66597, bus->root_hub, now_map);
}
+   mutex_unlock(_bus_idr_lock);
 }
 
 static int r8a66597_hub_status_data(struct usb_hcd *hcd, char *buf)
-- 
2.7.0


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

[PATCH RESEND 1/3] usb: core: switch bus numbering to using idr

[Heiner Kallweit]

USB bus numbering is based on directly dealing with bitmaps and
defines a separate list of busses.
This can be simplified and unified by using existing idr functionality.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/core/devices.c  | 10 ++
 drivers/usb/core/hcd.c  | 21 ++---
 drivers/usb/core/usb.c  |  1 +
 drivers/usb/host/r8a66597-hcd.c |  9 ++---
 drivers/usb/mon/mon_main.c  |  5 ++---
 include/linux/usb.h |  1 -
 include/linux/usb/hcd.h |  3 ++-
 7 files changed, 15 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
index cffa0a0..6f5476e 100644
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -110,13 +110,6 @@ static const char format_endpt[] =
 /* E:  Ad=xx(s) Atr=xx() MxPS= Ivl=D?s */
   "E:  Ad=%02x(%c) Atr=%02x(%-4s) MxPS=%4d Ivl=%d%cs\n";
 
-
-/*
- * Need access to the driver and USB bus lists.
- * extern struct list_head usb_bus_list;
- * However, these will come from functions that return ptrs to each of them.
- */
-
 /*
  * Wait for an connect/disconnect event to happen. We initialize
  * the event counter with an odd number, and each event will increment
@@ -616,6 +609,7 @@ static ssize_t usb_device_read(struct file *file, char 
__user *buf,
struct usb_bus *bus;
ssize_t ret, total_written = 0;
loff_t skip_bytes = *ppos;
+   int id;
 
if (*ppos < 0)
return -EINVAL;
@@ -626,7 +620,7 @@ static ssize_t usb_device_read(struct file *file, char 
__user *buf,
 
mutex_lock(_bus_list_lock);
/* print devices for all busses */
-   list_for_each_entry(bus, _bus_list, bus_list) {
+   idr_for_each_entry(_bus_idr, bus, id) {
/* recurse through all children of the root hub */
if (!bus_to_hcd(bus)->rh_registered)
continue;
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index df0e3b9..411d5c2 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -90,12 +90,11 @@ unsigned long usb_hcds_loaded;
 EXPORT_SYMBOL_GPL(usb_hcds_loaded);
 
 /* host controllers we manage */
-LIST_HEAD (usb_bus_list);
-EXPORT_SYMBOL_GPL (usb_bus_list);
+DEFINE_IDR (usb_bus_idr);
+EXPORT_SYMBOL_GPL (usb_bus_idr);
 
 /* used when allocating bus numbers */
 #define USB_MAXBUS 64
-static DECLARE_BITMAP(busmap, USB_MAXBUS);
 
 /* used when updating list of hcds */
 DEFINE_MUTEX(usb_bus_list_lock);   /* exported only for usbfs */
@@ -967,8 +966,6 @@ static void usb_bus_init (struct usb_bus *bus)
bus->bandwidth_int_reqs  = 0;
bus->bandwidth_isoc_reqs = 0;
mutex_init(>usb_address0_mutex);
-
-   INIT_LIST_HEAD (>bus_list);
 }
 
 /*-*/
@@ -989,16 +986,12 @@ static int usb_register_bus(struct usb_bus *bus)
int busnum;
 
mutex_lock(_bus_list_lock);
-   busnum = find_next_zero_bit(busmap, USB_MAXBUS, 1);
-   if (busnum >= USB_MAXBUS) {
-   printk (KERN_ERR "%s: too many buses\n", usbcore_name);
+   busnum = idr_alloc(_bus_idr, bus, 1, USB_MAXBUS, GFP_KERNEL);
+   if (busnum < 0) {
+   pr_err("%s: failed to get bus number\n", usbcore_name);
goto error_find_busnum;
}
-   set_bit(busnum, busmap);
bus->busnum = busnum;
-
-   /* Add it to the local list of buses */
-   list_add (>bus_list, _bus_list);
mutex_unlock(_bus_list_lock);
 
usb_notify_add_bus(bus);
@@ -1030,12 +1023,10 @@ static void usb_deregister_bus (struct usb_bus *bus)
 * itself up
 */
mutex_lock(_bus_list_lock);
-   list_del (>bus_list);
+   idr_remove(_bus_idr, bus->busnum);
mutex_unlock(_bus_list_lock);
 
usb_notify_remove_bus(bus);
-
-   clear_bit(bus->busnum, busmap);
 }
 
 /**
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index ebb29ca..aebbdb0 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -1115,6 +1115,7 @@ static void __exit usb_exit(void)
bus_unregister(_bus_type);
usb_acpi_unregister();
usb_debugfs_cleanup();
+   idr_destroy(_bus_idr);
 }
 
 subsys_initcall(usb_init);
diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 4cbd063..1ef8873 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -2099,13 +2099,8 @@ static void r8a66597_check_detect_child(struct r8a66597 
*r8a66597,
 
memset(now_map, 0, sizeof(now_map));
 
-   list_for_each_entry(bus, _bus_list, bus_list) {
-   if (!bus->root_hub)
-   continue;
-
-   if (bus->busnum != hcd->self.busnum)
-   continue;
-

[PATCH RESEND 3/3] usb: r8a66597: add locking to r8a66597_check_detect_child

[Heiner Kallweit]

Use mutex usb_bus_idr_lock to protect idr_find.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/host/r8a66597-hcd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 1ef8873..bfa7fa3 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -2099,11 +2099,13 @@ static void r8a66597_check_detect_child(struct r8a66597 
*r8a66597,
 
memset(now_map, 0, sizeof(now_map));
 
+   mutex_lock(_bus_idr_lock);
bus = idr_find(_bus_idr, hcd->self.busnum);
if (bus && bus->root_hub) {
collect_usb_address_map(bus->root_hub, now_map);
update_usb_address_map(r8a66597, bus->root_hub, now_map);
}
+   mutex_unlock(_bus_idr_lock);
 }
 
 static int r8a66597_hub_status_data(struct usb_hcd *hcd, char *buf)
-- 
2.7.0


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

Re: [PATCH 1/3] usb: core: switch bus numbering to using idr

[Heiner Kallweit]

Am 25.01.2016 um 06:02 schrieb Greg Kroah-Hartman:
> On Tue, Dec 22, 2015 at 09:22:09PM +0100, Heiner Kallweit wrote:
>> USB bus numbering is based on directly dealing with bitmaps and
>> defines a separate list of busses.
>> This can be simplified and unified by using existing idr functionality.
>>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>> ---
>>  drivers/usb/core/devices.c  | 10 ++
>>  drivers/usb/core/hcd.c  | 21 ++---
>>  drivers/usb/core/usb.c  |  1 +
>>  drivers/usb/host/r8a66597-hcd.c |  9 ++---
>>  drivers/usb/mon/mon_main.c  |  5 ++---
>>  include/linux/usb.h |  1 -
>>  include/linux/usb/hcd.h |  3 ++-
>>  7 files changed, 15 insertions(+), 35 deletions(-)
> 
> This series no longer applies to my tree, can you refresh it against the
> latest 4.5-rc1 and resend it?
> 
Sure, I resent it.

Regards, Heiner

> thanks,
> 
> greg k-h
> 

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

[PATCH 3/3] usb: r8a66597: add locking to r8a66597_check_detect_child

[Heiner Kallweit]

Use mutex usb_bus_idr_lock to protect idr_find.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/host/r8a66597-hcd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 1ef8873..bfa7fa3 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -2099,11 +2099,13 @@ static void r8a66597_check_detect_child(struct r8a66597 
*r8a66597,
 
memset(now_map, 0, sizeof(now_map));
 
+   mutex_lock(_bus_idr_lock);
bus = idr_find(_bus_idr, hcd->self.busnum);
if (bus && bus->root_hub) {
collect_usb_address_map(bus->root_hub, now_map);
update_usb_address_map(r8a66597, bus->root_hub, now_map);
}
+   mutex_unlock(_bus_idr_lock);
 }
 
 static int r8a66597_hub_status_data(struct usb_hcd *hcd, char *buf)
-- 
2.6.4


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

[PATCH 1/3] usb: core: switch bus numbering to using idr

[Heiner Kallweit]

USB bus numbering is based on directly dealing with bitmaps and
defines a separate list of busses.
This can be simplified and unified by using existing idr functionality.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/core/devices.c  | 10 ++
 drivers/usb/core/hcd.c  | 21 ++---
 drivers/usb/core/usb.c  |  1 +
 drivers/usb/host/r8a66597-hcd.c |  9 ++---
 drivers/usb/mon/mon_main.c  |  5 ++---
 include/linux/usb.h |  1 -
 include/linux/usb/hcd.h |  3 ++-
 7 files changed, 15 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
index 2a3bbdf..f4c5962 100644
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -110,13 +110,6 @@ static const char format_endpt[] =
 /* E:  Ad=xx(s) Atr=xx() MxPS= Ivl=D?s */
   "E:  Ad=%02x(%c) Atr=%02x(%-4s) MxPS=%4d Ivl=%d%cs\n";
 
-
-/*
- * Need access to the driver and USB bus lists.
- * extern struct list_head usb_bus_list;
- * However, these will come from functions that return ptrs to each of them.
- */
-
 /*
  * Wait for an connect/disconnect event to happen. We initialize
  * the event counter with an odd number, and each event will increment
@@ -616,6 +609,7 @@ static ssize_t usb_device_read(struct file *file, char 
__user *buf,
struct usb_bus *bus;
ssize_t ret, total_written = 0;
loff_t skip_bytes = *ppos;
+   int id;
 
if (*ppos < 0)
return -EINVAL;
@@ -626,7 +620,7 @@ static ssize_t usb_device_read(struct file *file, char 
__user *buf,
 
mutex_lock(_bus_list_lock);
/* print devices for all busses */
-   list_for_each_entry(bus, _bus_list, bus_list) {
+   idr_for_each_entry(_bus_idr, bus, id) {
/* recurse through all children of the root hub */
if (!bus_to_hcd(bus)->rh_registered)
continue;
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 1c102d6..08f5ea5 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -90,12 +90,11 @@ unsigned long usb_hcds_loaded;
 EXPORT_SYMBOL_GPL(usb_hcds_loaded);
 
 /* host controllers we manage */
-LIST_HEAD (usb_bus_list);
-EXPORT_SYMBOL_GPL (usb_bus_list);
+DEFINE_IDR (usb_bus_idr);
+EXPORT_SYMBOL_GPL (usb_bus_idr);
 
 /* used when allocating bus numbers */
 #define USB_MAXBUS 64
-static DECLARE_BITMAP(busmap, USB_MAXBUS);
 
 /* used when updating list of hcds */
 DEFINE_MUTEX(usb_bus_list_lock);   /* exported only for usbfs */
@@ -967,8 +966,6 @@ static void usb_bus_init (struct usb_bus *bus)
bus->bandwidth_int_reqs  = 0;
bus->bandwidth_isoc_reqs = 0;
mutex_init(>usb_address0_mutex);
-
-   INIT_LIST_HEAD (>bus_list);
 }
 
 /*-*/
@@ -989,16 +986,12 @@ static int usb_register_bus(struct usb_bus *bus)
int busnum;
 
mutex_lock(_bus_list_lock);
-   busnum = find_next_zero_bit(busmap, USB_MAXBUS, 1);
-   if (busnum >= USB_MAXBUS) {
-   printk (KERN_ERR "%s: too many buses\n", usbcore_name);
+   busnum = idr_alloc(_bus_idr, bus, 1, USB_MAXBUS, GFP_KERNEL);
+   if (busnum < 0) {
+   pr_err("%s: failed to get bus number\n", usbcore_name);
goto error_find_busnum;
}
-   set_bit(busnum, busmap);
bus->busnum = busnum;
-
-   /* Add it to the local list of buses */
-   list_add (>bus_list, _bus_list);
mutex_unlock(_bus_list_lock);
 
usb_notify_add_bus(bus);
@@ -1030,12 +1023,10 @@ static void usb_deregister_bus (struct usb_bus *bus)
 * itself up
 */
mutex_lock(_bus_list_lock);
-   list_del (>bus_list);
+   idr_remove(_bus_idr, bus->busnum);
mutex_unlock(_bus_list_lock);
 
usb_notify_remove_bus(bus);
-
-   clear_bit(bus->busnum, busmap);
 }
 
 /**
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index f8bbd0b..17f8230 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -1114,6 +1114,7 @@ static void __exit usb_exit(void)
bus_unregister(_bus_type);
usb_acpi_unregister();
usb_debugfs_cleanup();
+   idr_destroy(_bus_idr);
 }
 
 subsys_initcall(usb_init);
diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 4cbd063..1ef8873 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -2099,13 +2099,8 @@ static void r8a66597_check_detect_child(struct r8a66597 
*r8a66597,
 
memset(now_map, 0, sizeof(now_map));
 
-   list_for_each_entry(bus, _bus_list, bus_list) {
-   if (!bus->root_hub)
-   continue;
-
-   if (bus->busnum != hcd->self.busnum)
-   continue;
-

[PATCH 2/3] usb: core: rename mutex usb_bus_list_lock to usb_bus_idr_lock

[Heiner Kallweit]

Now that usb_bus_list has been removed and switched to idr
rename the related mutex accordingly.

Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
 drivers/usb/core/devices.c |  6 +++---
 drivers/usb/core/hcd.c | 30 +++---
 drivers/usb/core/hub.c |  4 ++--
 drivers/usb/mon/mon_main.c |  4 ++--
 include/linux/usb/hcd.h|  2 +-
 5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
index f4c5962..1f37a18 100644
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -618,7 +618,7 @@ static ssize_t usb_device_read(struct file *file, char 
__user *buf,
if (!access_ok(VERIFY_WRITE, buf, nbytes))
return -EFAULT;
 
-   mutex_lock(_bus_list_lock);
+   mutex_lock(_bus_idr_lock);
/* print devices for all busses */
idr_for_each_entry(_bus_idr, bus, id) {
/* recurse through all children of the root hub */
@@ -629,12 +629,12 @@ static ssize_t usb_device_read(struct file *file, char 
__user *buf,
  bus->root_hub, bus, 0, 0, 0);
usb_unlock_device(bus->root_hub);
if (ret < 0) {
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
return ret;
}
total_written += ret;
}
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
return total_written;
 }
 
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 08f5ea5..dc52405 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -97,8 +97,8 @@ EXPORT_SYMBOL_GPL (usb_bus_idr);
 #define USB_MAXBUS 64
 
 /* used when updating list of hcds */
-DEFINE_MUTEX(usb_bus_list_lock);   /* exported only for usbfs */
-EXPORT_SYMBOL_GPL (usb_bus_list_lock);
+DEFINE_MUTEX(usb_bus_idr_lock);/* exported only for usbfs */
+EXPORT_SYMBOL_GPL (usb_bus_idr_lock);
 
 /* used for controlling access to virtual root hubs */
 static DEFINE_SPINLOCK(hcd_root_hub_lock);
@@ -985,14 +985,14 @@ static int usb_register_bus(struct usb_bus *bus)
int result = -E2BIG;
int busnum;
 
-   mutex_lock(_bus_list_lock);
+   mutex_lock(_bus_idr_lock);
busnum = idr_alloc(_bus_idr, bus, 1, USB_MAXBUS, GFP_KERNEL);
if (busnum < 0) {
pr_err("%s: failed to get bus number\n", usbcore_name);
goto error_find_busnum;
}
bus->busnum = busnum;
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
 
usb_notify_add_bus(bus);
 
@@ -1001,7 +1001,7 @@ static int usb_register_bus(struct usb_bus *bus)
return 0;
 
 error_find_busnum:
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
return result;
 }
 
@@ -1022,9 +1022,9 @@ static void usb_deregister_bus (struct usb_bus *bus)
 * controller code, as well as having it call this when cleaning
 * itself up
 */
-   mutex_lock(_bus_list_lock);
+   mutex_lock(_bus_idr_lock);
idr_remove(_bus_idr, bus->busnum);
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
 
usb_notify_remove_bus(bus);
 }
@@ -1054,12 +1054,12 @@ static int register_root_hub(struct usb_hcd *hcd)
set_bit (devnum, usb_dev->bus->devmap.devicemap);
usb_set_device_state(usb_dev, USB_STATE_ADDRESS);
 
-   mutex_lock(_bus_list_lock);
+   mutex_lock(_bus_idr_lock);
 
usb_dev->ep0.desc.wMaxPacketSize = cpu_to_le16(64);
retval = usb_get_device_descriptor(usb_dev, USB_DT_DEVICE_SIZE);
if (retval != sizeof usb_dev->descriptor) {
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
dev_dbg (parent_dev, "can't read %s device descriptor %d\n",
dev_name(_dev->dev), retval);
return (retval < 0) ? retval : -EMSGSIZE;
@@ -1070,7 +1070,7 @@ static int register_root_hub(struct usb_hcd *hcd)
if (!retval) {
usb_dev->lpm_capable = usb_device_supports_lpm(usb_dev);
} else if (usb_dev->speed == USB_SPEED_SUPER) {
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
dev_dbg(parent_dev, "can't read %s bos descriptor %d\n",
dev_name(_dev->dev), retval);
return retval;
@@ -1090,7 +1090,7 @@ static int register_root_hub(struct usb_hcd *hcd)
if (HCD_DEAD(hcd))
usb_hc_died (hcd);  /* This time clean up */
}
-   mutex_unlock(_bus_list_lock);
+   mutex_unlock(_bus_idr_lock);
 
return retval;
 }
@@ -2854

[PATCH] usb: chipidea: replace udelay in loops with usleep_range

[Heiner Kallweit]

Both delays are at the lower end of where the use of usleep_range
is recommended. However as both udelay's occur in loops I think it
makes sense to replace them with sleeping equivalents to avoid
longer busy-waits.

Signed-off-by: Heiner Kallweit hkallwe...@gmail.com
---
 drivers/usb/chipidea/core.c | 2 +-
 drivers/usb/chipidea/udc.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 5b9825a..b9c645b 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -396,7 +396,7 @@ static int hw_controller_reset(struct ci_hdrc *ci)
 
hw_write(ci, OP_USBCMD, USBCMD_RST, USBCMD_RST);
while (hw_read(ci, OP_USBCMD, USBCMD_RST)) {
-   udelay(10);
+   usleep_range(10, 20);
if (count++  1000)
return -ETIMEDOUT;
}
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 4fe18ce..70ef2f2 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -326,7 +326,7 @@ static int hw_usb_reset(struct ci_hdrc *ci)
 
/* wait until all bits cleared */
while (hw_read(ci, OP_ENDPTPRIME, ~0))
-   udelay(10); /* not RTOS friendly */
+   usleep_range(10, 20);
 
/* reset all endpoints ? */
 
-- 
2.1.3

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