Re: [PATCH] MAINTAINERS: Remove Jarod Wilson and orphan LIRC drivers
Hi On 02/12/2013 10:20 PM, Joe Perches wrote: His email bounces jwil...@redhat.com should work I think. Note I think this may be the right call, but asking him directly is better :) Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
ACPI regression in 3.6, no more battery status, with Dell Latitude E6430
Hi, First of all, sorry for not reporting this earlier in the cycle, but I only got this (new) laptop yesterday ... With both the 3.5 kernels as well as with the 3.6 kernels, there is quite some unhappiness being reported in dmesg about the ACPI tables, esp. surrounding BAT0. But with 3.5 I do get battery status reported, where as with 3.6 I do not. Here is the dmesg output of booting with 3.5.4, resp. 3.6.0-rc7: http://people.fedoraproject.org/~jwrdegoede/dmesg-3.5 http://people.fedoraproject.org/~jwrdegoede/dmesg-3.6 Both boots were done with a standard Fedora kernel build. Please let me know what else I can do to help. I'm a kernel developer myself, so you can just throw a patch in my direction and I can build a test kernels without needing any hand-holding, etc. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 14/14] drivers/media/usb/gspca/cpia1.c: fix error return code
Hi,
Applied to my gspca tree and included in my pull-req for 3.7 which I just send
out.
Thanks,
Hans
On 09/06/2012 05:24 PM, Peter Senna Tschudin wrote:
From: Peter Senna Tschudin
Convert a nonnegative error return code to a negative one, as returned
elsewhere in the function.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
//
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
//
Signed-off-by: Peter Senna Tschudin
---
drivers/media/usb/gspca/cpia1.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/gspca/cpia1.c b/drivers/media/usb/gspca/cpia1.c
index 2499a88..b3ba47d 100644
--- a/drivers/media/usb/gspca/cpia1.c
+++ b/drivers/media/usb/gspca/cpia1.c
@@ -751,7 +751,7 @@ static int goto_high_power(struct gspca_dev *gspca_dev)
if (signal_pending(current))
return -EINTR;
- do_command(gspca_dev, CPIA_COMMAND_GetCameraStatus, 0, 0, 0, 0);
+ ret = do_command(gspca_dev, CPIA_COMMAND_GetCameraStatus, 0, 0, 0, 0);
if (ret)
return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup
Hi, On 06/26/2013 11:16 PM, Maxime Ripard wrote: Hi everyone, It also finally adds a clocksource from the free running counter found in the A10/A13 SoCs. Hmm, have you benchmarked this? There have been reports from linux-sunxi kernel users (xbmc project) that the waiting for the latch is quite slow. Note we don't have anything better yet in the linux-sunxi kernel. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup
Hi, On 06/27/2013 11:43 AM, Maxime Ripard wrote: On Thu, Jun 27, 2013 at 11:27:02AM +0200, Hans de Goede wrote: Hi, On 06/26/2013 11:16 PM, Maxime Ripard wrote: Hi everyone, It also finally adds a clocksource from the free running counter found in the A10/A13 SoCs. Hmm, have you benchmarked this? There have been reports from linux-sunxi kernel users (xbmc project) that the waiting for the latch is quite slow. Note we don't have anything better yet in the linux-sunxi kernel. No. I didn't. Do you have any pointers to these discussions? The original discussion should be somewhere here: https://groups.google.com/forum/#!forum/linux-sunxi But I could not find it (it is probably hidden under an unlogical subject). Looking at my own notes (a small TODO file), I've written down that the reporter reports: -current clocksource can cause us to run with interrupts disabled for 17% of the time, see "perf top" output This is with a workload which does a lot of gettimeofday calls. I notice that unlike the sunxi-3.4 code you don't do any locking, so how do you stop 2 clocksource calls from racing (and thus getting a possible wrong value because of things not being properly latched) ? Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup
Hi, On 06/27/2013 06:54 PM, Maxime Ripard wrote: On Thu, Jun 27, 2013 at 11:54:11AM +0200, Hans de Goede wrote: Hi, On 06/27/2013 11:43 AM, Maxime Ripard wrote: On Thu, Jun 27, 2013 at 11:27:02AM +0200, Hans de Goede wrote: Hi, On 06/26/2013 11:16 PM, Maxime Ripard wrote: Hi everyone, It also finally adds a clocksource from the free running counter found in the A10/A13 SoCs. Hmm, have you benchmarked this? There have been reports from linux-sunxi kernel users (xbmc project) that the waiting for the latch is quite slow. Note we don't have anything better yet in the linux-sunxi kernel. No. I didn't. Do you have any pointers to these discussions? The original discussion should be somewhere here: https://groups.google.com/forum/#!forum/linux-sunxi But I could not find it (it is probably hidden under an unlogical subject). I searched a bit and it seems to be that discussion: https://groups.google.com/d/msg/linux-sunxi/gaTDngPT7Is/oeLtWb1N1wIJ Looking at my own notes (a small TODO file), I've written down that the reporter reports: -current clocksource can cause us to run with interrupts disabled for 17% of the time, see "perf top" output This is with a workload which does a lot of gettimeofday calls. Siarhei however notes that even higher-end SoCs like the exynos5 have similar performances with that regard. So I'm not sure we can do something about it, except what is suggested in the above mail, which looks rather unsafe. Anyway, like you said, we have no easy other solution, and we lacked such support until now. So why not merge this code for now, and try to optimise it later if we find it's needed. That is fine with me, I just wanted to share that this has shown as a bottleneck in some benchmarks in case anyone has a clever idea to fix it ... Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup
Hi, On 06/27/2013 10:26 PM, Siarhei Siamashka wrote: On Thu, 27 Jun 2013 18:54:36 +0200 Maxime Ripard wrote: On Thu, Jun 27, 2013 at 11:54:11AM +0200, Hans de Goede wrote: I notice that unlike the sunxi-3.4 code you don't do any locking, so how do you stop 2 clocksource calls from racing (and thus getting a possible wrong value because of things not being properly latched) ? Hmm, right. I'll add a spinlock. I think the best would be to ask the Allwinner people (it's good to have them in CC, right?) whether anything wrong can happen because of "things not being properly latched". The A10 manual from http://free-electrons.com/~maxime/pub/datasheet/ does not seem to contain any details about what bad things may happen if we try to read CNT64_LO_REG while latching is still in progress and CNT64_RL_EN bit in CNT64_CTRL_REG has not changed to zero yet. I can imagine the following possible scenarios: 1. We read either the old stale CNT64_LO_REG value or the new correct value. 2. We read either the old stale CNT64_LO_REG value or the new correct value, or some random garbage. 3. The processor may deadlock, eat your dog, or do some other nasty thing. In the case of 1, we probably can get away without using any spinlocks? No, because if ie CNT64_LO_REG old is 0x and CNT64_LO_REG new is say 0x0001, and we do get the new CNT64_HI_REG things will break. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] gspca: fix dev_open() error path
Hi,
Thanks for the patch I've added this to my "gspca" tree, and this
will be included in my next pull-request to Mauro for 3.12
Regards,
Hans
On 08/05/2013 10:16 PM, Alexey Khoroshilov wrote:
If v4l2_fh_open() fails in dev_open(), gspca_dev->module left locked.
The patch adds module_put(gspca_dev->module) on this path.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov
---
drivers/media/usb/gspca/gspca.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index b7ae872..048507b 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -1266,6 +1266,7 @@ static void gspca_release(struct v4l2_device *v4l2_device)
static int dev_open(struct file *file)
{
struct gspca_dev *gspca_dev = video_drvdata(file);
+ int ret;
PDEBUG(D_STREAM, "[%s] open", current->comm);
@@ -1273,7 +1274,10 @@ static int dev_open(struct file *file)
if (!try_module_get(gspca_dev->module))
return -ENODEV;
- return v4l2_fh_open(file);
+ ret = v4l2_fh_open(file);
+ if (ret)
+ module_put(gspca_dev->module);
+ return ret;
}
static int dev_close(struct file *file)
-- 1.8.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] gspca: remove obsolete Kconfig macros
Mauro,
Can you pick this one up? I don't have anything pending for gspca,
and to create a tree + pullreq for just a trivial patch is not really
efficient.
Alternatively I can put it on my TODO for when there is more gspca work,
esp. since there is not really a need to hurry with merging this.
Regards,
Hans
On 03/28/2013 10:33 PM, Paul Bolle wrote:
The et61x251 driver was removed in v3.5. Remove the last references to
its Kconfig macro now.
Signed-off-by: Paul Bolle
---
Untested, as usual.
drivers/media/usb/gspca/etoms.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/media/usb/gspca/etoms.c b/drivers/media/usb/gspca/etoms.c
index 38f68e1..f165581 100644
--- a/drivers/media/usb/gspca/etoms.c
+++ b/drivers/media/usb/gspca/etoms.c
@@ -768,9 +768,7 @@ static const struct sd_desc sd_desc = {
/* -- module initialisation -- */
static const struct usb_device_id device_table[] = {
{USB_DEVICE(0x102c, 0x6151), .driver_info = SENSOR_PAS106},
-#if !defined CONFIG_USB_ET61X251 && !defined CONFIG_USB_ET61X251_MODULE
{USB_DEVICE(0x102c, 0x6251), .driver_info = SENSOR_TAS5130CXX},
-#endif
{}
};
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] gspca_touptek: Add support for ToupTek UCMOS series USB cameras
Hi John,
Thanks for the new driver! Unfortunately the driver is still
using gspca's own/deprecated control mechanism rather then
the new v4l2-control framework which all drivers now a days
use.
And we've just finished converting all the gspca sub-drivers
to using this new control framework, and removing gspca's
own custom control code.
Can you please convert the driver to use the new control code
(see other gspca drivers for examples how to do that, ie:
http://git.linuxtv.org/hgoede/gspca.git/commitdiff/4e02a3cd647940be6a9b4b71dfa88dbcb6fe1af9
http://git.linuxtv.org/hgoede/gspca.git/commitdiff/77f509f09754f46bcad35e13f1e6c95c1b8defdc
http://git.linuxtv.org/hgoede/gspca.git/commitdiff/c535784016480ef17778c285915492b581fa6a5f
And then resubmit the converted driver?
Thanks & Regards,
Hans
On 04/01/2013 02:53 AM, John McMaster wrote:
Adds support for AmScope MU800 / ToupTek UCMOS08000KPB USB microscope camera.
Signed-off-by: John McMaster
---
drivers/media/usb/gspca/Kconfig | 10 +
drivers/media/usb/gspca/Makefile |2 +
drivers/media/usb/gspca/touptek.c | 857 +
3 files changed, 869 insertions(+)
create mode 100644 drivers/media/usb/gspca/touptek.c
diff --git a/drivers/media/usb/gspca/Kconfig b/drivers/media/usb/gspca/Kconfig
index 6345f93..113f181 100644
--- a/drivers/media/usb/gspca/Kconfig
+++ b/drivers/media/usb/gspca/Kconfig
@@ -376,6 +376,16 @@ config USB_GSPCA_TOPRO
To compile this driver as a module, choose M here: the
module will be called gspca_topro.
+config USB_GSPCA_TOUPTEK
+ tristate "Touptek USB Camera Driver"
+ depends on VIDEO_V4L2 && USB_GSPCA
+ help
+ Say Y here if you want support for cameras based on the ToupTek UCMOS
+ / AmScope MU series camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called gspca_touptek.
+
config USB_GSPCA_TV8532
tristate "TV8532 USB Camera Driver"
depends on VIDEO_V4L2 && USB_GSPCA
diff --git a/drivers/media/usb/gspca/Makefile b/drivers/media/usb/gspca/Makefile
index c901da0..5f18e7f 100644
--- a/drivers/media/usb/gspca/Makefile
+++ b/drivers/media/usb/gspca/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_USB_GSPCA_STK014) += gspca_stk014.o
obj-$(CONFIG_USB_GSPCA_STV0680) += gspca_stv0680.o
obj-$(CONFIG_USB_GSPCA_T613) += gspca_t613.o
obj-$(CONFIG_USB_GSPCA_TOPRO)+= gspca_topro.o
+obj-$(CONFIG_USB_GSPCA_TOUPTEK) += gspca_touptek.o
obj-$(CONFIG_USB_GSPCA_TV8532) += gspca_tv8532.o
obj-$(CONFIG_USB_GSPCA_VC032X) += gspca_vc032x.o
obj-$(CONFIG_USB_GSPCA_VICAM)+= gspca_vicam.o
@@ -82,6 +83,7 @@ gspca_stv0680-objs := stv0680.o
gspca_sunplus-objs := sunplus.o
gspca_t613-objs := t613.o
gspca_topro-objs:= topro.o
+gspca_touptek-objs := touptek.o
gspca_tv8532-objs := tv8532.o
gspca_vc032x-objs := vc032x.o
gspca_vicam-objs:= vicam.o
diff --git a/drivers/media/usb/gspca/touptek.c
b/drivers/media/usb/gspca/touptek.c
new file mode 100644
index 000..9ce31f0
--- /dev/null
+++ b/drivers/media/usb/gspca/touptek.c
@@ -0,0 +1,857 @@
+/*
+ * ToupTek UCMOS / AmScope MU series camera driver
+ * TODO: contrast with ScopeTek / AmScope MDC cameras
+ *
+ * Copyright (C) 2012 John McMaster
+ *
+ * Special thanks to Bushing for helping with the decrypt algorithm and
+ * Sean O'Sullivan / the Rensselaer Center for Open Source
+ * Software (RCOS) for helping me learn kernel development
+ *
+ * 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; either version 2 of the License, or
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include "gspca.h"
+
+#define MODULE_NAME "touptek"
+
+MODULE_AUTHOR("John McMaster");
+MODULE_DESCRIPTION("ToupTek UCMOS / Amscope MU microscope camera driver");
+MODULE_LICENSE("GPL");
+
+/*
+Register value is linear with exposure time
+Exposure (sec), E (reg)
+0.000400, 0x0002
+0.001000, 0x0005
+0.005000, 0x0019
+0.02, 0x0064
+0.08, 0x0190
+0.40, 0x07D0
+1.00, 0x1388
+2.00, 0x2710
+*/
+#define INDEX_EXPOSURE 0x3012
+#define INDEX_WIDTH0x034C
+#define INDEX_HEIGHT 0x034E
+
+/*
+Gain does not vary with resolution (checked 640x480 vs 1600x1200)
+Range 0x1000 (nothing) to 0x11FF (highest)
+However, there appear to be gaps
+
+Suspect three gain stages
+0x1000: master channel enable bit
+0x007F: low gain bits
+0x0080: m
Re: linux-next: Tree for July 31 (media/radio-tea5777)
Thanks for fixing this for me!
Acked-by: Hans de Goede
On 07/31/2012 09:56 PM, Mauro Carvalho Chehab wrote:
Em 31-07-2012 14:22, Randy Dunlap escreveu:
drivers/built-in.o: In function `radio_tea5777_set_freq':
radio-tea5777.c:(.text+0x4d8704): undefined reference to `__udivdi3'
The patch below should fix it.
Thanks for reporting it!
Regards,
Mauro
[media] radio-tea5777: use library for 64bits div
From: Mauro Carvalho Chehab
drivers/built-in.o: In function `radio_tea5777_set_freq':
radio-tea5777.c:(.text+0x4d8704): undefined reference to `__udivdi3'
Reported-by: Randy Dunlap
Cc: Hans de Goede
Signed-off-by: Mauro Carvalho Chehab
diff --git a/drivers/media/radio/radio-tea5777.c
b/drivers/media/radio/radio-tea5777.c
index 3e12179..5bc9fa6 100644
--- a/drivers/media/radio/radio-tea5777.c
+++ b/drivers/media/radio/radio-tea5777.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include "radio-tea5777.h"
MODULE_AUTHOR("Hans de Goede ");
@@ -158,10 +159,11 @@ static int radio_tea5777_set_freq(struct radio_tea5777
*tea)
int res;
freq = clamp_t(u32, tea->freq,
- TEA5777_FM_RANGELOW, TEA5777_FM_RANGEHIGH);
- freq = (freq + 8) / 16; /* to kHz */
+ TEA5777_FM_RANGELOW, TEA5777_FM_RANGEHIGH) + 8;
+ do_div(freq, 16); /* to kHz */
- freq = (freq - TEA5777_FM_IF) / TEA5777_FM_FREQ_STEP;
+ freq -= TEA5777_FM_IF;
+ do_div(freq, TEA5777_FM_FREQ_STEP);
tea->write_reg &= ~(TEA5777_W_FM_PLL_MASK | TEA5777_W_FM_FREF_MASK);
tea->write_reg |= freq << TEA5777_W_FM_PLL_SHIFT;
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] media/radio/shark2: Fix build error caused by missing dependencies
Hi, I've a better fix for this here: http://git.linuxtv.org/hgoede/gspca.git/shortlog/refs/heads/media-for_v3.6 I already send a pull-req for this to Mauro a while ago, Mauro? Regards, Hans On 08/22/2012 05:16 PM, Guenter Roeck wrote: Fix build error: ERROR: "led_classdev_register" [drivers/media/radio/shark2.ko] undefined! ERROR: "led_classdev_unregister" [drivers/media/radio/shark2.ko] undefined! which is seen if RADIO_SHARK2 is enabled, but LEDS_CLASS is not. Since RADIO_SHARK2 depends on NEW_LEDS and LEDS_CLASS, select both if it is enabled. Cc: Hans de Goede Signed-off-by: Guenter Roeck --- drivers/media/radio/Kconfig |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/media/radio/Kconfig b/drivers/media/radio/Kconfig index 8090b87..bee4bee 100644 --- a/drivers/media/radio/Kconfig +++ b/drivers/media/radio/Kconfig @@ -77,6 +77,8 @@ config RADIO_SHARK config RADIO_SHARK2 tristate "Griffin radioSHARK2 USB radio receiver" depends on USB + select NEW_LEDS + select LEDS_CLASS ---help--- Choose Y here if you have this radio receiver. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] media/radio/shark2: Fix build error caused by missing dependencies
Hi, On 08/22/2012 08:57 PM, Arnd Bergmann wrote: On Wednesday 22 August 2012, Guenter Roeck wrote: On Wed, Aug 22, 2012 at 05:22:26PM +0200, Hans de Goede wrote: Hi, I've a better fix for this here: http://git.linuxtv.org/hgoede/gspca.git/shortlog/refs/heads/media-for_v3.6 I already send a pull-req for this to Mauro a while ago, Mauro? Looks like it found its way into mainline in the last couple of days. Should have updated my tree first. Sorry for the noise. I found another issue with the shark driver while doing randconfig tests. Here is my semi-automated log file for the problem. Has this also made it in already? No, Mauro, can you please add Arnd's fix for this to 3.6 ? Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL for 3.6-rc1] media updates part 2
Hi, My bad, sorry about this. Mauro's patch looks good. An alternative fix would be to #ifdefify the led code in the drivers themselves. Regards, Hans On 08/09/2012 01:38 PM, Mauro Carvalho Chehab wrote: Em 08-08-2012 19:28, David Rientjes escreveu: On Tue, 31 Jul 2012, Mauro Carvalho Chehab wrote: [media] radio-shark: New driver for the Griffin radioSHARK USB radio receiver This one gives me a build warning if CONFIG_LEDS_CLASS is disabled: ERROR: "led_classdev_register" [drivers/media/radio/shark2.ko] undefined! ERROR: "led_classdev_unregister" [drivers/media/radio/shark2.ko] undefined! Could you please test the enclosed patch? Thanks! Mauro - [media] radio-shark: make sure LEDS_CLASS is selected As reported by David: > ERROR: "led_classdev_register" [drivers/media/radio/shark2.ko] undefined! > ERROR: "led_classdev_unregister" [drivers/media/radio/shark2.ko] undefined! Reported-by: Dadiv Rientjes Cc: Hans de Goede Signed-off-by: Mauro Carvalho Chehab diff --git a/drivers/media/radio/Kconfig b/drivers/media/radio/Kconfig index 8090b87..be68ec2 100644 --- a/drivers/media/radio/Kconfig +++ b/drivers/media/radio/Kconfig @@ -60,6 +60,7 @@ config RADIO_MAXIRADIO config RADIO_SHARK tristate "Griffin radioSHARK USB radio receiver" depends on USB && SND + select LEDS_CLASS ---help--- Choose Y here if you have this radio receiver. @@ -77,6 +78,7 @@ config RADIO_SHARK config RADIO_SHARK2 tristate "Griffin radioSHARK2 USB radio receiver" depends on USB + select LEDS_CLASS ---help--- Choose Y here if you have this radio receiver. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL for 3.6-rc1] media updates part 2
Hi, On 08/09/2012 10:03 PM, David Rientjes wrote: On Thu, 9 Aug 2012, Mauro Carvalho Chehab wrote: Yeah, that would work as well, although the code would look uglier. IMHO, using select/depend is better. Agreed, I think it should be "depends on LEDS_CLASS" rather than select it if there is a hard dependency that cannot be fixed with extracting the led support in the driver to #ifdef CONFIG_LEDS_CLASS code. The led support could be #ifdef CONFIG_LEDS_CLASS, the problem with that approach is the whole module versus build-in thing: led-class shark enable-led-support build-inbuild-inyes build-inmodule yes module build-inno module module yes Now this can be coded into #ifdef magic, but it won't be pretty, of course we only need the non pretty version once at the top to set a SHARK_USE_LEDS define, but still. I'm fine with either solution (depends or ifdef magic), although I think that people will get unpleasantly surprised if they want to use the shark driver and they don't get to see it in the menu because they don't have leds enabled. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] NULL dereference BUG in sch56xx_init()
Hi, On 08/09/2012 04:42 PM, Guenter Roeck wrote: On Thu, Aug 09, 2012 at 08:55:26PM +0800, Fengguang Wu wrote: Hi Guenter, This commit triggered an oops which can be fixed by the attached diff. Should it be folded into the original one (preferable for me), or be resent as a standalone patch? I folded it into the original commit. Thanks a lot for the test and feedback! Fengguang, good catch, thanks! Guenter, 2 remarks: 1) The changing of the type of the address parameter of sch56xx_device_add is not necessary 2) A similar change is needed for the f71882fg, there the type of the address variable in f71882fg_init() needs to be changed to int too. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] radio-shark2: Only compile led support when CONFIG_LED_CLASS is set
Reported-by: Dadiv Rientjes
Signed-off-by: Hans de Goede
---
drivers/media/radio/radio-shark2.c | 27 ---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/media/radio/radio-shark2.c
b/drivers/media/radio/radio-shark2.c
index b9575de..e593d5a 100644
--- a/drivers/media/radio/radio-shark2.c
+++ b/drivers/media/radio/radio-shark2.c
@@ -27,7 +27,6 @@
#include
#include
-#include
#include
#include
#include
@@ -35,6 +34,12 @@
#include
#include "radio-tea5777.h"
+#if defined(CONFIG_LEDS_CLASS) || \
+(defined(CONFIG_LEDS_CLASS_MODULE) && defined(CONFIG_RADIO_SHARK2_MODULE))
+#include
+#define SHARK_USE_LEDS 1
+#endif
+
MODULE_AUTHOR("Hans de Goede ");
MODULE_DESCRIPTION("Griffin radioSHARK2, USB radio receiver driver");
MODULE_LICENSE("GPL");
@@ -43,7 +48,6 @@ static int debug;
module_param(debug, int, 0);
MODULE_PARM_DESC(debug, "Debug level (0-1)");
-
#define SHARK_IN_EP0x83
#define SHARK_OUT_EP 0x05
@@ -52,6 +56,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
#define v4l2_dev_to_shark(d) container_of(d, struct shark_device, v4l2_dev)
+#ifdef SHARK_USE_LEDS
enum { BLUE_LED, RED_LED, NO_LEDS };
static void shark_led_set_blue(struct led_classdev *led_cdev,
@@ -73,17 +78,20 @@ static const struct led_classdev
shark_led_templates[NO_LEDS] = {
.brightness_set = shark_led_set_red,
},
};
+#endif
struct shark_device {
struct usb_device *usbdev;
struct v4l2_device v4l2_dev;
struct radio_tea5777 tea;
+#ifdef SHARK_USE_LEDS
struct work_struct led_work;
struct led_classdev leds[NO_LEDS];
char led_names[NO_LEDS][32];
atomic_t brightness[NO_LEDS];
unsigned long brightness_new;
+#endif
u8 *transfer_buffer;
};
@@ -161,6 +169,7 @@ static struct radio_tea5777_ops shark_tea_ops = {
.read_reg = shark_read_reg,
};
+#ifdef SHARK_USE_LEDS
static void shark_led_work(struct work_struct *work)
{
struct shark_device *shark =
@@ -216,20 +225,25 @@ static void shark_led_set_red(struct led_classdev
*led_cdev,
set_bit(RED_LED, &shark->brightness_new);
schedule_work(&shark->led_work);
}
+#endif
static void usb_shark_disconnect(struct usb_interface *intf)
{
struct v4l2_device *v4l2_dev = usb_get_intfdata(intf);
struct shark_device *shark = v4l2_dev_to_shark(v4l2_dev);
+#ifdef SHARK_USE_LEDS
int i;
+#endif
mutex_lock(&shark->tea.mutex);
v4l2_device_disconnect(&shark->v4l2_dev);
radio_tea5777_exit(&shark->tea);
mutex_unlock(&shark->tea.mutex);
+#ifdef SHARK_USE_LEDS
for (i = 0; i < NO_LEDS; i++)
led_classdev_unregister(&shark->leds[i]);
+#endif
v4l2_device_put(&shark->v4l2_dev);
}
@@ -238,7 +252,9 @@ static void usb_shark_release(struct v4l2_device *v4l2_dev)
{
struct shark_device *shark = v4l2_dev_to_shark(v4l2_dev);
+#ifdef SHARK_USE_LEDS
cancel_work_sync(&shark->led_work);
+#endif
v4l2_device_unregister(&shark->v4l2_dev);
kfree(shark->transfer_buffer);
kfree(shark);
@@ -248,7 +264,10 @@ static int usb_shark_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct shark_device *shark;
- int i, retval = -ENOMEM;
+ int retval = -ENOMEM;
+#ifdef SHARK_USE_LEDS
+ int i;
+#endif
shark = kzalloc(sizeof(struct shark_device), GFP_KERNEL);
if (!shark)
@@ -292,6 +311,7 @@ static int usb_shark_probe(struct usb_interface *intf,
goto err_init_tea;
}
+#ifdef SHARK_USE_LEDS
INIT_WORK(&shark->led_work, shark_led_work);
for (i = 0; i < NO_LEDS; i++) {
shark->leds[i] = shark_led_templates[i];
@@ -312,6 +332,7 @@ static int usb_shark_probe(struct usb_interface *intf,
"couldn't register led: %s\n",
shark->led_names[i]);
}
+#endif
return 0;
--
1.7.11.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] radio-shark: Only compile led support when CONFIG_LED_CLASS is set
Reported-by: Dadiv Rientjes
Signed-off-by: Hans de Goede
---
drivers/media/radio/radio-shark.c | 26 --
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/media/radio/radio-shark.c
b/drivers/media/radio/radio-shark.c
index c2ead23..f746ed0 100644
--- a/drivers/media/radio/radio-shark.c
+++ b/drivers/media/radio/radio-shark.c
@@ -27,7 +27,6 @@
#include
#include
-#include
#include
#include
#include
@@ -35,6 +34,12 @@
#include
#include
+#if defined(CONFIG_LEDS_CLASS) || \
+(defined(CONFIG_LEDS_CLASS_MODULE) && defined(CONFIG_RADIO_SHARK_MODULE))
+#include
+#define SHARK_USE_LEDS 1
+#endif
+
/*
* Version Information
*/
@@ -54,6 +59,7 @@ MODULE_LICENSE("GPL");
#define v4l2_dev_to_shark(d) container_of(d, struct shark_device, v4l2_dev)
+#ifdef SHARK_USE_LEDS
enum { BLUE_LED, BLUE_PULSE_LED, RED_LED, NO_LEDS };
static void shark_led_set_blue(struct led_classdev *led_cdev,
@@ -83,17 +89,20 @@ static const struct led_classdev
shark_led_templates[NO_LEDS] = {
.brightness_set = shark_led_set_red,
},
};
+#endif
struct shark_device {
struct usb_device *usbdev;
struct v4l2_device v4l2_dev;
struct snd_tea575x tea;
+#ifdef SHARK_USE_LEDS
struct work_struct led_work;
struct led_classdev leds[NO_LEDS];
char led_names[NO_LEDS][32];
atomic_t brightness[NO_LEDS];
unsigned long brightness_new;
+#endif
u8 *transfer_buffer;
u32 last_val;
@@ -175,6 +184,7 @@ static struct snd_tea575x_ops shark_tea_ops = {
.read_val = shark_read_val,
};
+#ifdef SHARK_USE_LEDS
static void shark_led_work(struct work_struct *work)
{
struct shark_device *shark =
@@ -244,20 +254,25 @@ static void shark_led_set_red(struct led_classdev
*led_cdev,
set_bit(RED_LED, &shark->brightness_new);
schedule_work(&shark->led_work);
}
+#endif
static void usb_shark_disconnect(struct usb_interface *intf)
{
struct v4l2_device *v4l2_dev = usb_get_intfdata(intf);
struct shark_device *shark = v4l2_dev_to_shark(v4l2_dev);
+#ifdef SHARK_USE_LEDS
int i;
+#endif
mutex_lock(&shark->tea.mutex);
v4l2_device_disconnect(&shark->v4l2_dev);
snd_tea575x_exit(&shark->tea);
mutex_unlock(&shark->tea.mutex);
+#ifdef SHARK_USE_LEDS
for (i = 0; i < NO_LEDS; i++)
led_classdev_unregister(&shark->leds[i]);
+#endif
v4l2_device_put(&shark->v4l2_dev);
}
@@ -266,7 +281,9 @@ static void usb_shark_release(struct v4l2_device *v4l2_dev)
{
struct shark_device *shark = v4l2_dev_to_shark(v4l2_dev);
+#ifdef SHARK_USE_LEDS
cancel_work_sync(&shark->led_work);
+#endif
v4l2_device_unregister(&shark->v4l2_dev);
kfree(shark->transfer_buffer);
kfree(shark);
@@ -276,7 +293,10 @@ static int usb_shark_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct shark_device *shark;
- int i, retval = -ENOMEM;
+ int retval = -ENOMEM;
+#ifdef SHARK_USE_LEDS
+ int i;
+#endif
shark = kzalloc(sizeof(struct shark_device), GFP_KERNEL);
if (!shark)
@@ -321,6 +341,7 @@ static int usb_shark_probe(struct usb_interface *intf,
goto err_init_tea;
}
+#ifdef SHARK_USE_LEDS
INIT_WORK(&shark->led_work, shark_led_work);
for (i = 0; i < NO_LEDS; i++) {
shark->leds[i] = shark_led_templates[i];
@@ -341,6 +362,7 @@ static int usb_shark_probe(struct usb_interface *intf,
"couldn't register led: %s\n",
shark->led_names[i]);
}
+#endif
return 0;
--
1.7.11.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] radio-shark: Only compile led support when CONFIG_LED_CLASS is set
Hi,
On 08/10/2012 10:15 PM, Mauro Carvalho Chehab wrote:
Em 10-08-2012 16:58, Hans de Goede escreveu:
Reported-by: Dadiv Rientjes
Signed-off-by: Hans de Goede
---
drivers/media/radio/radio-shark.c | 26 --
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/media/radio/radio-shark.c
b/drivers/media/radio/radio-shark.c
index c2ead23..f746ed0 100644
--- a/drivers/media/radio/radio-shark.c
+++ b/drivers/media/radio/radio-shark.c
@@ -27,7 +27,6 @@
#include
#include
-#include
#include
#include
#include
@@ -35,6 +34,12 @@
#include
#include
+#if defined(CONFIG_LEDS_CLASS) || \
+(defined(CONFIG_LEDS_CLASS_MODULE) && defined(CONFIG_RADIO_SHARK_MODULE))
+#include
Conditionally including headers is not a good thing.
Ok, I will make it unconditional then.
...
static void usb_shark_disconnect(struct usb_interface *intf)
{
struct v4l2_device *v4l2_dev = usb_get_intfdata(intf);
struct shark_device *shark = v4l2_dev_to_shark(v4l2_dev);
+#ifdef SHARK_USE_LEDS
int i;
+#endif
mutex_lock(&shark->tea.mutex);
v4l2_device_disconnect(&shark->v4l2_dev);
snd_tea575x_exit(&shark->tea);
mutex_unlock(&shark->tea.mutex);
+#ifdef SHARK_USE_LEDS
for (i = 0; i < NO_LEDS; i++)
led_classdev_unregister(&shark->leds[i]);
+#endif
v4l2_device_put(&shark->v4l2_dev);
}
That looks ugly.
Agreed.
Maybe you could code it on a different way.
You could be move all shark_use_leds together into the same place at
the code, like:
Sounds good, although I will still need to set a SHARK_USE_LEDS define at
the top of the file, to avoid having unused members in struct shark_device, not
that the compiler will complain but having them there for no good reason seems
undesirable.
#if defined(CONFIG_LEDS_CLASS) || \
(defined(CONFIG_LEDS_CLASS_MODULE) && defined(CONFIG_RADIO_SHARK_MODULE))
static void shark_led_set_blue(struct led_classdev *led_cdev,
...
.brightness_set = shark_led_set_red,
},
};
static void shark_led_disconnect(...)
{
...
}
static void shark_led_release(...)
{
...
}
static void shark_led_register(...)
{
...
}
#else
static inline void shark_led_disconnect(...) { };
static inline void shark_led_release(...) { };
static inline void shark_led_register(...)
{
printk(KERN_WARN "radio-shark: CONFIG_LED_CLASS not enabled. LEDs won't
work\n");
}
#endif
And let the rest of the code to call the shark_led functions, as if LEDS aren't
enabled,
the function stubs won't produce any code (well, except for the above error
notice).
The same comment also applies to patch 2.
Ok new versions of the 2 patches coming up (should be there in 2 hours or so, I
want
to both compile and run-time test them both with / without leds).
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/4] radio-shark*: Only compile led support when CONFIG_LED_CLA
Hi All, Here is the second revision of my patch-set to fix the build breakage when the radio-shark* drivers are enabled and CONFIG_LED_CLASS is not enabled. This new version introduces 2 new cleanup / preparation patches, and take into account the remarks from Mauro's review of v1. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/4] radio-shark*: Remove work-around for dangling pointer in usb intfdata
Recent kernels properly clear the usb intfdata pointer when another driver fails to bind (in the radio-shark* case the usbhid driver would try to bind first. Signed-off-by: Hans de Goede --- drivers/media/radio/radio-shark.c | 9 - drivers/media/radio/radio-shark2.c | 9 - 2 files changed, 18 deletions(-) diff --git a/drivers/media/radio/radio-shark.c b/drivers/media/radio/radio-shark.c index c2ead23..6117132 100644 --- a/drivers/media/radio/radio-shark.c +++ b/drivers/media/radio/radio-shark.c @@ -286,15 +286,6 @@ static int usb_shark_probe(struct usb_interface *intf, if (!shark->transfer_buffer) goto err_alloc_buffer; - /* -* Work around a bug in usbhid/hid-core.c, where it leaves a dangling -* pointer in intfdata causing v4l2-device.c to not set it. Which -* results in usb_shark_disconnect() referencing the dangling pointer -* -* REMOVE (as soon as the above bug is fixed, patch submitted) -*/ - usb_set_intfdata(intf, NULL); - shark->v4l2_dev.release = usb_shark_release; v4l2_device_set_name(&shark->v4l2_dev, DRV_NAME, &shark_instance); retval = v4l2_device_register(&intf->dev, &shark->v4l2_dev); diff --git a/drivers/media/radio/radio-shark2.c b/drivers/media/radio/radio-shark2.c index b9575de..fc0289d 100644 --- a/drivers/media/radio/radio-shark2.c +++ b/drivers/media/radio/radio-shark2.c @@ -258,15 +258,6 @@ static int usb_shark_probe(struct usb_interface *intf, if (!shark->transfer_buffer) goto err_alloc_buffer; - /* -* Work around a bug in usbhid/hid-core.c, where it leaves a dangling -* pointer in intfdata causing v4l2-device.c to not set it. Which -* results in usb_shark_disconnect() referencing the dangling pointer -* -* REMOVE (as soon as the above bug is fixed, patch submitted) -*/ - usb_set_intfdata(intf, NULL); - shark->v4l2_dev.release = usb_shark_release; v4l2_device_set_name(&shark->v4l2_dev, DRV_NAME, &shark_instance); retval = v4l2_device_register(&intf->dev, &shark->v4l2_dev); -- 1.7.11.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/4] radio-shark: Only compile led support when CONFIG_LED_CLASS is set
Signed-off-by: Hans de Goede
---
drivers/media/radio/radio-shark.c | 135 ++
1 file changed, 79 insertions(+), 56 deletions(-)
diff --git a/drivers/media/radio/radio-shark.c
b/drivers/media/radio/radio-shark.c
index 05e12bf..e1970bf 100644
--- a/drivers/media/radio/radio-shark.c
+++ b/drivers/media/radio/radio-shark.c
@@ -35,6 +35,11 @@
#include
#include
+#if defined(CONFIG_LEDS_CLASS) || \
+(defined(CONFIG_LEDS_CLASS_MODULE) && defined(CONFIG_RADIO_SHARK_MODULE))
+#define SHARK_USE_LEDS 1
+#endif
+
/*
* Version Information
*/
@@ -56,44 +61,18 @@ MODULE_LICENSE("GPL");
enum { BLUE_LED, BLUE_PULSE_LED, RED_LED, NO_LEDS };
-static void shark_led_set_blue(struct led_classdev *led_cdev,
- enum led_brightness value);
-static void shark_led_set_blue_pulse(struct led_classdev *led_cdev,
-enum led_brightness value);
-static void shark_led_set_red(struct led_classdev *led_cdev,
- enum led_brightness value);
-
-static const struct led_classdev shark_led_templates[NO_LEDS] = {
- [BLUE_LED] = {
- .name = "%s:blue:",
- .brightness = LED_OFF,
- .max_brightness = 127,
- .brightness_set = shark_led_set_blue,
- },
- [BLUE_PULSE_LED] = {
- .name = "%s:blue-pulse:",
- .brightness = LED_OFF,
- .max_brightness = 255,
- .brightness_set = shark_led_set_blue_pulse,
- },
- [RED_LED] = {
- .name = "%s:red:",
- .brightness = LED_OFF,
- .max_brightness = 1,
- .brightness_set = shark_led_set_red,
- },
-};
-
struct shark_device {
struct usb_device *usbdev;
struct v4l2_device v4l2_dev;
struct snd_tea575x tea;
+#ifdef SHARK_USE_LEDS
struct work_struct led_work;
struct led_classdev leds[NO_LEDS];
char led_names[NO_LEDS][32];
atomic_t brightness[NO_LEDS];
unsigned long brightness_new;
+#endif
u8 *transfer_buffer;
u32 last_val;
@@ -175,6 +154,7 @@ static struct snd_tea575x_ops shark_tea_ops = {
.read_val = shark_read_val,
};
+#ifdef SHARK_USE_LEDS
static void shark_led_work(struct work_struct *work)
{
struct shark_device *shark =
@@ -235,21 +215,78 @@ static void shark_led_set_red(struct led_classdev
*led_cdev,
schedule_work(&shark->led_work);
}
+static const struct led_classdev shark_led_templates[NO_LEDS] = {
+ [BLUE_LED] = {
+ .name = "%s:blue:",
+ .brightness = LED_OFF,
+ .max_brightness = 127,
+ .brightness_set = shark_led_set_blue,
+ },
+ [BLUE_PULSE_LED] = {
+ .name = "%s:blue-pulse:",
+ .brightness = LED_OFF,
+ .max_brightness = 255,
+ .brightness_set = shark_led_set_blue_pulse,
+ },
+ [RED_LED] = {
+ .name = "%s:red:",
+ .brightness = LED_OFF,
+ .max_brightness = 1,
+ .brightness_set = shark_led_set_red,
+ },
+};
+
+static int shark_register_leds(struct shark_device *shark, struct device *dev)
+{
+ int i, retval;
+
+ INIT_WORK(&shark->led_work, shark_led_work);
+ for (i = 0; i < NO_LEDS; i++) {
+ shark->leds[i] = shark_led_templates[i];
+ snprintf(shark->led_names[i], sizeof(shark->led_names[0]),
+shark->leds[i].name, shark->v4l2_dev.name);
+ shark->leds[i].name = shark->led_names[i];
+ retval = led_classdev_register(dev, &shark->leds[i]);
+ if (retval) {
+ v4l2_err(&shark->v4l2_dev,
+"couldn't register led: %s\n",
+shark->led_names[i]);
+ return retval;
+ }
+ }
+ return 0;
+}
+
+static void shark_unregister_leds(struct shark_device *shark)
+{
+ int i;
+
+ for (i = 0; i < NO_LEDS; i++)
+ led_classdev_unregister(&shark->leds[i]);
+
+ cancel_work_sync(&shark->led_work);
+}
+#else
+static int shark_register_leds(struct shark_device *shark, struct device *dev)
+{
+ v4l2_warn(&shark->v4l2_dev,
+ "CONFIG_LED_CLASS not enabled, LED support disabled\n");
+ return 0;
+}
+static inline void shark_unregister_leds(struct shark_device *shark) { }
+#endif
+
static void usb_shark_disconnect(struct usb_interface *intf)
{
struct v4l2_device *v4l2_dev = usb_get_intfdata(intf);
struct shark_devic
[PATCH 4/4] radio-shark2: Only compile led support when CONFIG_LED_CLASS is set
Signed-off-by: Hans de Goede
---
drivers/media/radio/radio-shark2.c | 122 ++---
1 file changed, 73 insertions(+), 49 deletions(-)
diff --git a/drivers/media/radio/radio-shark2.c
b/drivers/media/radio/radio-shark2.c
index 217483c..7b4efdf 100644
--- a/drivers/media/radio/radio-shark2.c
+++ b/drivers/media/radio/radio-shark2.c
@@ -35,6 +35,11 @@
#include
#include "radio-tea5777.h"
+#if defined(CONFIG_LEDS_CLASS) || \
+(defined(CONFIG_LEDS_CLASS_MODULE) && defined(CONFIG_RADIO_SHARK2_MODULE))
+#define SHARK_USE_LEDS 1
+#endif
+
MODULE_AUTHOR("Hans de Goede ");
MODULE_DESCRIPTION("Griffin radioSHARK2, USB radio receiver driver");
MODULE_LICENSE("GPL");
@@ -43,7 +48,6 @@ static int debug;
module_param(debug, int, 0);
MODULE_PARM_DESC(debug, "Debug level (0-1)");
-
#define SHARK_IN_EP0x83
#define SHARK_OUT_EP 0x05
@@ -54,36 +58,18 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
enum { BLUE_LED, RED_LED, NO_LEDS };
-static void shark_led_set_blue(struct led_classdev *led_cdev,
- enum led_brightness value);
-static void shark_led_set_red(struct led_classdev *led_cdev,
- enum led_brightness value);
-
-static const struct led_classdev shark_led_templates[NO_LEDS] = {
- [BLUE_LED] = {
- .name = "%s:blue:",
- .brightness = LED_OFF,
- .max_brightness = 127,
- .brightness_set = shark_led_set_blue,
- },
- [RED_LED] = {
- .name = "%s:red:",
- .brightness = LED_OFF,
- .max_brightness = 1,
- .brightness_set = shark_led_set_red,
- },
-};
-
struct shark_device {
struct usb_device *usbdev;
struct v4l2_device v4l2_dev;
struct radio_tea5777 tea;
+#ifdef SHARK_USE_LEDS
struct work_struct led_work;
struct led_classdev leds[NO_LEDS];
char led_names[NO_LEDS][32];
atomic_t brightness[NO_LEDS];
unsigned long brightness_new;
+#endif
u8 *transfer_buffer;
};
@@ -161,6 +147,7 @@ static struct radio_tea5777_ops shark_tea_ops = {
.read_reg = shark_read_reg,
};
+#ifdef SHARK_USE_LEDS
static void shark_led_work(struct work_struct *work)
{
struct shark_device *shark =
@@ -208,21 +195,72 @@ static void shark_led_set_red(struct led_classdev
*led_cdev,
schedule_work(&shark->led_work);
}
+static const struct led_classdev shark_led_templates[NO_LEDS] = {
+ [BLUE_LED] = {
+ .name = "%s:blue:",
+ .brightness = LED_OFF,
+ .max_brightness = 127,
+ .brightness_set = shark_led_set_blue,
+ },
+ [RED_LED] = {
+ .name = "%s:red:",
+ .brightness = LED_OFF,
+ .max_brightness = 1,
+ .brightness_set = shark_led_set_red,
+ },
+};
+
+static int shark_register_leds(struct shark_device *shark, struct device *dev)
+{
+ int i, retval;
+
+ INIT_WORK(&shark->led_work, shark_led_work);
+ for (i = 0; i < NO_LEDS; i++) {
+ shark->leds[i] = shark_led_templates[i];
+ snprintf(shark->led_names[i], sizeof(shark->led_names[0]),
+shark->leds[i].name, shark->v4l2_dev.name);
+ shark->leds[i].name = shark->led_names[i];
+ retval = led_classdev_register(dev, &shark->leds[i]);
+ if (retval) {
+ v4l2_err(&shark->v4l2_dev,
+"couldn't register led: %s\n",
+shark->led_names[i]);
+ return retval;
+ }
+ }
+ return 0;
+}
+
+static void shark_unregister_leds(struct shark_device *shark)
+{
+ int i;
+
+ for (i = 0; i < NO_LEDS; i++)
+ led_classdev_unregister(&shark->leds[i]);
+
+ cancel_work_sync(&shark->led_work);
+}
+#else
+static int shark_register_leds(struct shark_device *shark, struct device *dev)
+{
+ v4l2_warn(&shark->v4l2_dev,
+ "CONFIG_LED_CLASS not enabled, LED support disabled\n");
+ return 0;
+}
+static inline void shark_unregister_leds(struct shark_device *shark) { }
+#endif
+
static void usb_shark_disconnect(struct usb_interface *intf)
{
struct v4l2_device *v4l2_dev = usb_get_intfdata(intf);
struct shark_device *shark = v4l2_dev_to_shark(v4l2_dev);
- int i;
mutex_lock(&shark->tea.mutex);
v4l2_device_disconnect(&shark->v4l2_dev);
radio_tea5777_exit(&shark->tea);
mutex_unlock(&shark->tea.mutex);
- for (i = 0
[PATCH 2/4] radio-shark*: Call cancel_work_sync from disconnect rather then release
This removes the need for shark_led_work to take the v4l2 lock.
Signed-off-by: Hans de Goede
---
drivers/media/radio/radio-shark.c | 13 ++---
drivers/media/radio/radio-shark2.c | 12 ++--
2 files changed, 4 insertions(+), 21 deletions(-)
diff --git a/drivers/media/radio/radio-shark.c
b/drivers/media/radio/radio-shark.c
index 6117132..05e12bf 100644
--- a/drivers/media/radio/radio-shark.c
+++ b/drivers/media/radio/radio-shark.c
@@ -181,14 +181,6 @@ static void shark_led_work(struct work_struct *work)
container_of(work, struct shark_device, led_work);
int i, res, brightness, actual_len;
- /*
-* We use the v4l2_dev lock and registered bit to ensure the device
-* does not get unplugged and unreffed while we're running.
-*/
- mutex_lock(&shark->tea.mutex);
- if (!video_is_registered(&shark->tea.vd))
- goto leave;
-
for (i = 0; i < 3; i++) {
if (!test_and_clear_bit(i, &shark->brightness_new))
continue;
@@ -208,8 +200,6 @@ static void shark_led_work(struct work_struct *work)
v4l2_err(&shark->v4l2_dev, "set LED %s error: %d\n",
shark->led_names[i], res);
}
-leave:
- mutex_unlock(&shark->tea.mutex);
}
static void shark_led_set_blue(struct led_classdev *led_cdev,
@@ -259,6 +249,8 @@ static void usb_shark_disconnect(struct usb_interface *intf)
for (i = 0; i < NO_LEDS; i++)
led_classdev_unregister(&shark->leds[i]);
+ cancel_work_sync(&shark->led_work);
+
v4l2_device_put(&shark->v4l2_dev);
}
@@ -266,7 +258,6 @@ static void usb_shark_release(struct v4l2_device *v4l2_dev)
{
struct shark_device *shark = v4l2_dev_to_shark(v4l2_dev);
- cancel_work_sync(&shark->led_work);
v4l2_device_unregister(&shark->v4l2_dev);
kfree(shark->transfer_buffer);
kfree(shark);
diff --git a/drivers/media/radio/radio-shark2.c
b/drivers/media/radio/radio-shark2.c
index fc0289d..217483c 100644
--- a/drivers/media/radio/radio-shark2.c
+++ b/drivers/media/radio/radio-shark2.c
@@ -166,13 +166,6 @@ static void shark_led_work(struct work_struct *work)
struct shark_device *shark =
container_of(work, struct shark_device, led_work);
int i, res, brightness, actual_len;
- /*
-* We use the v4l2_dev lock and registered bit to ensure the device
-* does not get unplugged and unreffed while we're running.
-*/
- mutex_lock(&shark->tea.mutex);
- if (!video_is_registered(&shark->tea.vd))
- goto leave;
for (i = 0; i < 2; i++) {
if (!test_and_clear_bit(i, &shark->brightness_new))
@@ -191,8 +184,6 @@ static void shark_led_work(struct work_struct *work)
v4l2_err(&shark->v4l2_dev, "set LED %s error: %d\n",
shark->led_names[i], res);
}
-leave:
- mutex_unlock(&shark->tea.mutex);
}
static void shark_led_set_blue(struct led_classdev *led_cdev,
@@ -231,6 +222,8 @@ static void usb_shark_disconnect(struct usb_interface *intf)
for (i = 0; i < NO_LEDS; i++)
led_classdev_unregister(&shark->leds[i]);
+ cancel_work_sync(&shark->led_work);
+
v4l2_device_put(&shark->v4l2_dev);
}
@@ -238,7 +231,6 @@ static void usb_shark_release(struct v4l2_device *v4l2_dev)
{
struct shark_device *shark = v4l2_dev_to_shark(v4l2_dev);
- cancel_work_sync(&shark->led_work);
v4l2_device_unregister(&shark->v4l2_dev);
kfree(shark->transfer_buffer);
kfree(shark);
--
1.7.11.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESION] Suspend hangs with 3.6-rc1 on Lenovo T60 notebook
Hi,
On 08/15/2012 07:13 AM, Miklos Szeredi wrote:
Suspend oopses in generic_ide_suspend() because dev_get_drvdata()
returns NULL (dev->p->driver_data == NULL) and this function is not
prepared for this.
I bisected it to 0998d063 (device-core: Ensure drvdata = NULL when no
driver is bound). Reverting it fixes suspend.
First of all, thanks for reporting and bisecting this. With that said,
I must say that this is very weird. The patch in question:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=0998d063
Only makes dev-drvdata NULL in 2 cases:
1) The probe method of the driver fails
2) The driver has been detached from the device by calling one of:
device_release_driver() or driver_detach()
Note that in both code paths dev->driver also gets set to NULL, and
other generic ide driver callbacks very much depend on that not being
NULL, ie:
static int generic_ide_remove(struct device *dev)
{
ide_drive_t *drive = to_ide_device(dev);
struct ide_driver *drv = to_ide_driver(dev->driver);
if (drv->remove)
drv->remove(drive);
return 0;
}
Also how can a drivers suspend callback get called if dev->driver is NULL,
since that callback would normally be "reached" through dev->driver, so
something weird is going on here ...
I hope one of the ide guys can shed some light on this.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESION] Suspend hangs with 3.6-rc1 on Lenovo T60 notebook
Hi,
On 08/15/2012 09:59 PM, Rafael J. Wysocki wrote:
On Wednesday, August 15, 2012, Hans de Goede wrote:
Hi,
On 08/15/2012 07:13 AM, Miklos Szeredi wrote:
Suspend oopses in generic_ide_suspend() because dev_get_drvdata()
returns NULL (dev->p->driver_data == NULL) and this function is not
prepared for this.
I bisected it to 0998d063 (device-core: Ensure drvdata = NULL when no
driver is bound). Reverting it fixes suspend.
First of all, thanks for reporting and bisecting this. With that said,
I must say that this is very weird. The patch in question:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=0998d063
Only makes dev-drvdata NULL in 2 cases:
1) The probe method of the driver fails
2) The driver has been detached from the device by calling one of:
device_release_driver() or driver_detach()
Note that in both code paths dev->driver also gets set to NULL, and
other generic ide driver callbacks very much depend on that not being
NULL, ie:
static int generic_ide_remove(struct device *dev)
{
ide_drive_t *drive = to_ide_device(dev);
struct ide_driver *drv = to_ide_driver(dev->driver);
if (drv->remove)
drv->remove(drive);
return 0;
}
Also how can a drivers suspend callback get called if dev->driver is NULL,
since that callback would normally be "reached" through dev->driver, so
something weird is going on here ...
No, it wouldn't, because it is a bus type callback and it is invoked for
all devices whose bus type is ide_bus_type, regardless of whether or not
their driver field is NULL.
Ah right, these are bus_driver operations. That explains some things, so I've
done some more research asking myself: "Why does generic_ide_suspend(), which
is a *bus* op, call dev_get_drvdata?", the answer to that seems to be that
the ide subsystem is abusing (IMHO) drvdata to store per device bus_driver
data. Which I believe is not how drvdata is intended to be used.
With that said, the above knowledge has allowed me to write an (ugly) fix for
the regression Miklos is seeing. Miklos can you give the attached patch a
try please?
It clearly should check if drive is not NULL before using that pointer.
I assume you mean drive*r*, yes I agree that generic_ide_remove should
check for that. So who is going to write a patch for that?
Regards,
Hans
>From 00f700ef4dd5fa335f725361aa683388c9b8ec4f Mon Sep 17 00:00:00 2001
From: Hans de Goede
Date: Thu, 16 Aug 2012 13:23:30 +0200
Subject: [PATCH] ide: Restore drvdata after device_attach failure
Since commit 0998d063: "device-core: Ensure drvdata = NULL when no
driver is bound", device_attach will clear a device's drvdata if the
driver failed to bind to it.
In the ide subsystem however drvdata is not used to store driver data,
but rather to store per device bus_driver data. So for now restore
drvdata after calling device_register(), so that drvdata will still point
to the per device bus_driver data after a driver probe failure.
In the long run the ide subsystem should probably be fixed to not abuse
drvdata in this way, as this clearly is not how drvdata is intended to be
used.
Signed-off-by: Hans de Goede
---
drivers/ide/ide-probe.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
index 068cef0..be1981b 100644
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -1015,6 +1015,12 @@ static void hwif_register_devices(ide_hwif_t *hwif)
if (ret < 0)
printk(KERN_WARNING "IDE: %s: device_register error: "
"%d\n", __func__, ret);
+ /*
+ * device_register() will have cleared drvdata on
+ * device_attach failure, but we use drvdata to store per
+ * device bus info, rather then for driver info, so restore it.
+ */
+ dev_set_drvdata(dev, drive);
}
}
--
1.7.11.4
Re: [REGRESION] Suspend hangs with 3.6-rc1 on Lenovo T60 notebook
Hi all, On 08/16/2012 10:02 PM, Rafael J. Wysocki wrote: On Thursday, August 16, 2012, Alan Stern wrote: On Thu, 16 Aug 2012, Miklos Szeredi wrote: Yes, this appears to work. Following patch fixes the suspend oops. Thanks, Miklos OK Miklos, can you please send that to Dave with a proper changelog and sign-off (please add my ACK too)? Please make it clear that this is a regression fix and which commit has introduced the regression. I think that Alan's suggest fix, as implemented by Milos is great!, but before moving forward with this someone should audit all the other (generic) ide code for other places using the drvdata, a simple grep for dev_get_drvdata should show these. And now you can get rid of the useless dev_set_drvdata() call. That was in the other patch I think. No my patch was a hack to undo the results of the commit causing the regression in the IDE case. But Alan's approach clearly is much better! Once we are sure drvdata is not used anywhere the dev_set_drvdata call could be removed in the place where my hack added a second call to it. Note that there are likely actual ide drivers using it, without setting it themselves since the ide core was setting it. So removing it will require even more auditing / checking. A 3th thing which should be audited is the generic ide code assuming that dev->driver != NULL, this is at least true for generic_ide_remove, where: if (drv->remove) Should be changed to: if (dev->driver && drv->remove) Following the way things are done in generic_ide_shutdown, a similar change could be needed for generic_ide_probe(), although I guess that may never get called with dev->driver == NULL ? Unfortunately I'm very busy with other stuff atm, so I cannot help here other then pointing out that such audits should be done :| Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESION] Suspend hangs with 3.6-rc1 on Lenovo T60 notebook
Hi, On 08/17/2012 04:27 PM, Alan Stern wrote: On Fri, 17 Aug 2012, Alan Stern wrote: On Fri, 17 Aug 2012, Hans de Goede wrote: No my patch was a hack to undo the results of the commit causing the regression in the IDE case. But Alan's approach clearly is much better! Once we are sure drvdata is not used anywhere the dev_set_drvdata call could be removed in the place where my hack added a second call to it. Note that there are likely actual ide drivers using it, without setting it themselves since the ide core was setting it. So removing it will require even more auditing / checking. I did search for dev_get_drvdata() calls in drivers/ide; there were no other calls that retrieved an ide_drive_t value. But I didn't check anywhere else in the kernel. In fact, I'm not sure where else to look. After a little more checking: Among all the .c files in the kernel which match the pattern '[^a-z]ide_', the only occurrences of dev_get_drvdata() which retrieve an ide_drive_t value are the two in ide-pm.c. Thanks, good work! That means we should move forward with submitting Miklos' patch to the ide subsystem maintainer asap. Also if someone feels like it we still could do the 2 follow up / clean up patches: 1) Removin the set_drvdata call from hwif_register_devices() in ide-prove.c 2) Fixup ide-generic.c ide_remove to not dereference a NULL dev->driver Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: video: USB webcam fails since kernel 3.2
Hi, On 07/09/2012 01:33 PM, Martin-Éric Racine wrote: Hmm, this is then likely caused by the new isoc bandwidth negotiation code in 3.2, unfortunately the vc032x driver is one of the few gspca drivers for which I don't have a cam to test with. Can you try to build your own kernel from source? Boot into your own kernel, and verify the regression is still there, then edit drivers/media/video/gspca/gspca.c and go to the which_bandwidth function, and at the beginning of this function add the following line: return 2000 * 2000 * 120; Then rebuild and re-install the kernel and try again. If that helps, remove the added return 2000 * 2000 * 120; line, and also remove the following lines from which_bandwidth: /* if the image is compressed, estimate its mean size */ if (!gspca_dev->cam.needs_full_bandwidth && bandwidth < gspca_dev->cam.cam_mode[i].width * gspca_dev->cam.cam_mode[i].height) bandwidth = bandwidth * 3 / 8; /* 0.375 */ And try again if things still work this way. Once you've tested this I can try to write a fix for this. Hans, Thank you for your reply. Just to eliminate the possibility of mistakes on my part while trying to perform the above changes, could you send me a patch against Linux 3.2.21 that I could apply as-is, before building myself a test kernel package? Erm, that is quite a bit of work from my side for something which you can easily do yourself, edit gspca.c, search for which_bandwidth and then under the following lines: u32 bandwidth; int i; Add a line like this: return 2000 * 2000 * 120; Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers
Hi, On 10/10/2012 10:31 PM, Henrik Rydberg wrote: Hi Hans, Alan, Greg, commit 3d97ff63f8997761f12c8fbe8082996c6eeaba1a Author: Hans de Goede Date: Wed Jul 4 09:18:03 2012 +0200 usbdevfs: Use scatter-gather lists for large bulk transfers breaks an usb programming cable over here. The problem is reported as "bulk tranfer failed" [sic] by the tool, and bisection leads to this commit. Reverting on top of 3.6 solves it for me. Oh what fun (not). The best way to figure out what really is going on is to get some usb level traces. Note my first hunch is that what you're seeing is a device firmware bug, as this patch together with a new libusb (which you seem to also have) will make bulk transfers run slightly faster, which might be just enough to overwhelm your device ... Can you please do the following: 1) unplug as many usb devices as possible 2) plug in the programmer 3) run lsusb, note the programmer bus number 4) if the programmer is one the same bus as another device (other then a hub), try a different port 5) ideally rinse repeat until it is on a bus of its own, or atleast a bus with a device which generate little trafic 6) Writedown the bus number, and keep using the same port for all further tests Then: 1) start wireshark as root, tell it to start capturing on the USB bus you've ended up using 2) Do what you always do with the programmer 3) When finished, or when things have failed stop wireshark capture and save the capture to a file Repeat 2 times once with a kernel with the problematic commit, once without. Then send me the 2 wireshark captures. Note: 1) Privacy warning: In theory I might be able to reconstruct what you're sending over the captured USB bus when you send me these captures 2) Less is more, so if you can find some mini mini program to send to the device you're programming that makes live easier (and should make 1 less of an issue too). Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers
Hi, On 10/11/2012 11:53 PM, Greg Kroah-Hartman wrote: On Thu, Oct 11, 2012 at 11:37:07PM +0200, Henrik Rydberg wrote: Hi Hans, Oh what fun (not). The best way to figure out what really is going on is to get some usb level traces. Note my first hunch is that whath you're seeing is a device firmware bug, as this patch together with a new libusb (which you seem to also have) will make bulk transfers run slightly faster, which might be just enough to overwhelm your device ... Or, the large bulk transfer actually never worked in the first place. Large input transfers certainly do, as they were part of my tests, but I must admit my test cases seem to not include large output transfers (my bad). Thanks for fixing this! The list you gave me seemed boringly long, so I read the patch more closely instead. The fix below is the result. Greg, will you please take it through your tree? Henrik, Very nice fix, thanks for debugging this. Hans, any objection to me taking this? No objections please take it, this patch is: Acked-by: Hans de Goede And stating the obvious: CC: sta...@vger.kernel.org To be backported to 3.6 only Thanks & Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers
Hi, On 10/12/2012 05:08 PM, Henrik Rydberg wrote: Hi Alan, Instead of introducing a new local variable, why not simply update uurb->buffer? That's what we do elsewhere in the code. It seemed fragile, due to these scary lines: if (is_in && uurb->buffer_length > 0) as->userbuffer = uurb->buffer; else as->userbuffer = NULL; I suppose it works in this particular case. How is the USBDEVFS_URB_TYPE_CONTROL case supposed to work here? I can certainly change the patch if preferred. Tested and attached below. Thanks, Henrik From 40b70394747eea51fdd07cc8213dd6afd24b1b30 Mon Sep 17 00:00:00 2001 From: Henrik Rydberg Date: Thu, 11 Oct 2012 23:27:04 +0200 Subject: [PATCH v2] usbdevfs: Fix broken scatter-gather transfer The handling of large output bulk transfers is broken; the same u page is read over and over again. Fixed with this patch. Signed-off-by: Henrik Rydberg --- drivers/usb/core/devio.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index ebb8a9d..6e58b59 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -1349,6 +1351,7 @@ goto error; } } + uurb->buffer += u; I know you've already fixed this in the next revision, but still: NAK, this will break large, split input transfers as it well set as->userbuffer to the end of the buffer instead of the beginning! Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] usbdevfs: Fix broken scatter-gather transfer
Hi, On 10/13/2012 12:20 PM, Henrik Rydberg wrote: The handling of large output bulk transfers is broken; the same user page is read over and over again. Fixed with this patch. Cc: sta...@kernel.org Acked-by: Peter Stuge Acked-by: Hans de Goede Acked-by: Alan Stern Signed-off-by: Henrik Rydberg --- Hi Greg, Here is the formal and third version of the patch. Version two still made me nervous, And it should, as as said before I believe it would have broken large input transfers ... so I moved the increment to where it is clear that the buffer pointer is never referenced again later in the function. I still kept the ACKS, hope that is alright with everyone. Unlike the previous one, this version should work, so yes it may keep my ACK :) Regards, Hans Thanks, Henrik drivers/usb/core/devio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index ebb8a9d..7f75343 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -1348,6 +1348,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb, ret = -EFAULT; goto error; } + uurb->buffer += u; } totlen -= u; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: video: USB webcam fails since kernel 3.2
Hi, On 07/08/2012 03:01 PM, Martin-Éric Racine wrote: 2012/6/17 Martin-Éric Racine : pe, 2012-06-15 kello 23:41 -0500, Jonathan Nieder kirjoitti: Martin-Éric Racine wrote: usb 1-7: new high-speed USB device number 3 using ehci_hcd [...] usb 1-7: New USB device found, idVendor=0ac8, idProduct=0321 usb 1-7: New USB device strings: Mfr=1, Product=2, SerialNumber=0 usb 1-7: Product: USB2.0 Web Camera usb 1-7: Manufacturer: Vimicro Corp. [...] Linux media interface: v0.10 Linux video capture interface: v2.00 gspca_main: v2.14.0 registered gspca_main: vc032x-2.14.0 probing 0ac8:0321 usbcore: registered new interface driver vc032x The device of interest is discovered. gspca_main: ISOC data error: [36] len=0, status=-71 gspca_main: ISOC data error: [65] len=0, status=-71 [...] gspca_main: ISOC data error: [48] len=0, status=-71 video_source:sr[3246]: segfault at 0 ip (null) sp ab36de1c error 14 in cheese[8048000+21000] gspca_main: ISOC data error: [17] len=0, status=-71 (The above data error spew starts around t=121 seconds and continues at a rate of about 15 messages per second. The segfault is around t=154.) The vc032x code hasn't changed since 3.4.1, so please report your symptoms to Jean-François Moine , cc-ing linux-me...@vger.kernel.org, linux-kernel@vger.kernel.org, and either me or this bug log so we can track it. Be sure to mention: - steps to reproduce, expected result, actual result, and how the difference indicates a bug (should be simple enough in this case) 1. Ensure that user 'myself' is a member of the 'video' group. 2. Launch the webcam application Cheese from the GNOME desktop. Expected result: Cheese displays whatever this laptop's camera sees. Actual result: Cheese crashes while attempting to access the camera. - how reproducible the bug is (100%?) 100% - which kernel versions you have tested and result with each (what is the newest kernel version that worked?) It probably was 3.1.0 or some earlier 3.2 release (the upcoming Debian will release with 3.2.x; 3.4 was only used here for testing purposes), but I wouldn't know for sure since I don't use my webcam too often. I finally found time to perform further testing, using kernel packages from snapshots.debian.org, and the last one that positively worked (at least using GNOME's webcam application Cheese) was: linux-image-3.1.0-1-686-pae 3.1.8-2 Linux 3.1 for modern PCs This loaded the following video modules: gspca_vc032x gspca_main videodev media Tests using 3.2.1-1 or more recent crashed as described before. This at least gives us a time frame for when the regression started. Hmm, this is then likely caused by the new isoc bandwidth negotiation code in 3.2, unfortunately the vc032x driver is one of the few gspca drivers for which I don't have a cam to test with. Can you try to build your own kernel from source? Boot into your own kernel, and verify the regression is still there, then edit drivers/media/video/gspca/gspca.c and go to the which_bandwidth function, and at the beginning of this function add the following line: return 2000 * 2000 * 120; Then rebuild and re-install the kernel and try again. If that helps, remove the added return 2000 * 2000 * 120; line, and also remove the following lines from which_bandwidth: /* if the image is compressed, estimate its mean size */ if (!gspca_dev->cam.needs_full_bandwidth && bandwidth < gspca_dev->cam.cam_mode[i].width * gspca_dev->cam.cam_mode[i].height) bandwidth = bandwidth * 3 / 8; /* 0.375 */ And try again if things still work this way. Once you've tested this I can try to write a fix for this. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [2.6 patch] make abituguru3_read_increment_offset() static
Adrian Bunk wrote:
abituguru3_read_increment_offset() can become static.
Signed-off-by: Adrian Bunk <[EMAIL PROTECTED]>
Looks good, good catch.
Acked-by: Hans de Goede <[EMAIL PROTECTED]>
---
--- linux-2.6.23-rc1-mm1/drivers/hwmon/abituguru3.c.old 2007-07-26
08:56:33.0 +0200
+++ linux-2.6.23-rc1-mm1/drivers/hwmon/abituguru3.c 2007-07-26
08:57:00.0 +0200
@@ -691,8 +691,9 @@
/* Sensor settings are stored 1 byte per offset with the bytes
placed add consecutive offsets. */
-int abituguru3_read_increment_offset(struct abituguru3_data *data, u8 bank,
- u8 offset, u8 count, u8 *buf, int offset_count)
+static int abituguru3_read_increment_offset(struct abituguru3_data *data,
+ u8 bank, u8 offset, u8 count,
+ u8 *buf, int offset_count)
{
int i, x;
___
lm-sensors mailing list
[EMAIL PROTECTED]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: 2.6.23.1 x86 hardware monitoring bug?
Mark M. Hoffman wrote: Hi Justin: (added some CCs) * Justin Piszcz <[EMAIL PROTECTED]> [2007-10-14 15:30:18 -0400]: As a regular user, I cannot see the sensors on the A-bit board, but I can see the CPU temperature, how come I can see one but not the other? What does "which sensors" say as user, and what as root? What does "ldd `which sensors`" say as user and what as root? I have the feeling you have 2 different versions of lm-sensors installed in root is using one, and the user the other. Regards, Hans Kernel: $ uname -a Linux mybox 2.6.23.1 #4 SMP PREEMPT Sun Oct 14 15:20:53 EDT 2007 i686 GNU/Linux Distribution: Debian Lenny $ sensors abituguru3-isa-00e0 Adapter: ISA adapter coretemp-isa- Adapter: ISA adapter temp1: +35°C (high = +85°C) coretemp-isa-0001 Adapter: ISA adapter temp1: +36°C (high = +85°C) As root: # sensors abituguru3-isa-00e0 Adapter: ISA adapter CPU Core: +1.35 V (min +0.00 V, max +1.60 V) DDR2: +2.02 V (min +1.60 V, max +2.40 V) DDR2 VTT: +1.01 V (min +0.80 V, max +1.20 V) CPU VTT 1.2V: +1.22 V (min +0.95 V, max +1.45 V) MCH & PCIE 1.5V:+1.50 V (min +1.20 V, max +1.80 V) MCH 2.5V: +2.58 V (min +2.00 V, max +3.00 V) ICH 1.05V: +1.04 V (min +0.85 V, max +1.25 V) ATX +12V (24-Pin): +12.18 V (min +9.60 V, max +14.40 V) ATX +12V (4-pin): +12.24 V (min +9.60 V, max +14.40 V) ATX +5V:+5.07 V (min +3.99 V, max +6.00 V) +3.3V: +3.40 V (min +2.64 V, max +3.94 V) 5VSB: +5.10 V (min +3.99 V, max +6.00 V) CPU: +43°C (high = +75°C, crit = +85°C) System : +38°C (high = +55°C, crit = +65°C) PWM1: +43°C (high = +80°C, crit = +90°C) PWM2: +43°C (high = +80°C, crit = +90°C) PWM3: +46°C (high = +80°C, crit = +90°C) PWM4: +40°C (high = +80°C, crit = +90°C) CPU Fan: 1380 RPM (min 300 RPM) NB Fan: 0 RPM (min 300 RPM) SYS Fan: 0 RPM (min 300 RPM) AUX1 Fan: 0 RPM (min 300 RPM) AUX2 Fan: 0 RPM (min 300 RPM) AUX3 Fan: 0 RPM (min 300 RPM) OTES1 Fan:0 RPM (min 300 RPM) coretemp-isa- Adapter: ISA adapter Core 0: +39°C (high = +85°C) coretemp-isa-0001 Adapter: ISA adapter Core 1: +39°C (high = +85°C) Strange. What does 'ls -lH /sys/class/hwmon/hwmon*/device' say? Regards, - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [ patch .24-rc0 0/5 ] SuperIO locks coordinator
Jim Cromie wrote: this patchset (on hwmon-git) re-introduces superio_locks module, previously RFC'd here, where I 'borrowed' another thread.. http://marc.info/?l=linux-kernel&m=115821759424601&w=2 The module shares out slots/shared-reservations containing a mutex, so that multiple modules can coordinate access to the sio-port. Im crossposting - LKML for more reviewers, lm-sensors for folks with the hardware and (perhaps) more interest. If its not too late, please consider for 2.6.24-rc0 01 - adds superio_locks module User-drivers specify the sio-port characteristics they can support device-ids, sio-port-addrs, enter & exit sequences, etc in a struct superio_search (in __devinit, preferably). superio_find() then searches existing slots/shared-reservations for a matching sio-port, and returns it if found. Otherwize it probes port-addrs, specified by find() user, and makes and returns a new reservation. superio_find() finds and reserves the slot, returned as ptr or null superio_release() relinguishes the slot (ref-counted) Once theyve got the reservation in struct superio * gate (as named in patches 2-5) they *may* use superio_lock(gate) superio_enter/exit(gate) superio_inb/w(gate, regaddr), superio_outb/w(gate, regaddr, val) or they can do it themselves with inb/outb, by using gate->sioaddr, etc. - Hans, I think Ive added all your suggestions, thanks. Your welcome, at a first look over things look pretty sane now. I'm willing to do a full review of this, but first I would like to see some more input from others. Regards, Hans - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] Could the k8temp driver be interfering with ACPI?
Jean Delvare wrote:
On Wed, 21 Feb 2007 15:19:44 -0500, Dave Jones wrote:
Ah, Fedora has this horror in its initscripts (which explains why I missed
it in my grep)..
# Initialize ACPI bits
if [ -d /proc/acpi ]; then
for module in /lib/modules/$unamer/kernel/drivers/acpi/* ; do
module=${module##*/}
module=${module%.ko}
modprobe $module >/dev/null 2>&1
done
fi
Ah, this also explains why the i2c_ec and sbs drivers were loaded on
Chuck's system, although they were not needed.
This is there because there's no clean way for userspace to know whether
to load the system specific stuff right now. Bill Nottingham pointed
out that we could add a /sys/class/dmi/modalias and appropriate MODULE_DMI
tags to the various modules like asus_acpi to make udev autoload them.
Something similiar should be doable for i2c_ec, as it's only useful if a
given ACPI object is present. sbs, in turn, is only useful if i2c_ec is
loaded.
I'm thinking that it might be an idea to also use this idea of udev autoloading
through DMI info for the abituguru and abituguru3 driver (review please). The
both only support about 12 motherboards. For the abituguru driver, dmi info
could also be used to automatically set the module options needed on the 2
oldest uguru featuring abit motherboards. What do you think about this?
Regards,
Hans
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
PATCH: change keycode for scancode e0 32 from 150 to 172
Dmitry Torokhov wrote: On 6/13/07, Jiri Kosina <[EMAIL PROTECTED]> wrote: On Wed, 13 Jun 2007, Hans de Goede wrote: > Good to hear, so as everyone smees to agree, shall I write a (massive, > complex, intrusive) patch to fix this, or are there until now silent > parties that object? Well, Dmitry is the one who has final word on this, as it is related to PS/2 keyboards. Just noticed he is not in CC, added. Yep, just send me the patch (don't forget to make change to drivers/char/keyboard.c that Vojtech mentioned in other e-mail). Here is a patch as promised, attached. Thanks & Regards, Hans As discussed by mail change keycode for scancode e0 32 from 150 to 172 Signed-off-by: Hans de Goede <[EMAIL PROTECTED]> diff -ur linux-2.6.22-rc4.orig/drivers/char/keyboard.c linux-2.6.22-rc4/drivers/char/keyboard.c --- linux-2.6.22-rc4.orig/drivers/char/keyboard.c 2007-06-16 22:54:47.0 +0200 +++ linux-2.6.22-rc4/drivers/char/keyboard.c 2007-06-18 23:15:27.0 +0200 @@ -1005,8 +1005,8 @@ 284,285,309, 0,312, 91,327,328,329,331,333,335,336,337,338,339, 367,288,302,304,350, 89,334,326,267,126,268,269,125,347,348,349, 360,261,262,263,268,376,100,101,321,316,373,286,289,102,351,355, - 103,104,105,275,287,279,306,106,274,107,294,364,358,363,362,361, - 291,108,381,281,290,272,292,305,280, 99,112,257,258,359,113,114, + 103,104,105,275,287,279,258,106,274,107,294,364,358,363,362,361, + 291,108,381,281,290,272,292,305,280, 99,112,257,306,359,113,114, 264,117,271,374,379,265,266, 93, 94, 95, 85,259,375,260, 90,116, 377,109,111,277,278,282,283,295,296,297,299,300,301,293,303,307, 308,310,313,314,315,317,318,319,320,357,322,323,324,325,276,330, diff -ur linux-2.6.22-rc4.orig/drivers/input/keyboard/atkbd.c linux-2.6.22-rc4/drivers/input/keyboard/atkbd.c --- linux-2.6.22-rc4.orig/drivers/input/keyboard/atkbd.c 2007-06-16 22:54:48.0 +0200 +++ linux-2.6.22-rc4/drivers/input/keyboard/atkbd.c 2007-06-18 23:07:24.0 +0200 @@ -89,7 +89,7 @@ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 217,100,255, 0, 97,165, 0, 0,156, 0, 0, 0, 0, 0, 0,125, 173,114, 0,113, 0, 0, 0,126,128, 0, 0,140, 0, 0, 0,127, - 159, 0,115, 0,164, 0, 0,116,158, 0,150,166, 0, 0, 0,142, + 159, 0,115, 0,164, 0, 0,116,158, 0,172,166, 0, 0, 0,142, 157, 0, 0, 0, 0, 0, 0, 0,155, 0, 98, 0, 0,163, 0, 0, 226, 0, 0, 0, 0, 0, 0, 0, 0,255, 96, 0, 0, 0,143, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,107, 0,105,102, 0, 0,112, @@ -111,7 +111,7 @@ 82, 83, 80, 76, 77, 72, 69, 98, 0, 96, 81, 0, 78, 73, 55,183, 184,185,186,187, 74, 94, 92, 93, 0, 0, 0,125,126,127,112, 0, - 0,139,150,163,165,115,152,150,166,140,160,154,113,114,167,168, + 0,139,172,163,165,115,152,172,166,140,160,154,113,114,167,168, 148,149,147,140 };
PATCH: fix mismatch between usb-hid.c HUT find/search mapping and the HUT reference doc
Hi all, As some of you might know from my earlier post/thread about atkbd and softraw, I'm currently working on getting keyboards with internet/easy access keys to work painlessly / plug and play. In order to be able to better test / develop this I've bought 2 cheap such keyboards today, one ps2 and one both usb and ps2 capable. When comparing usb vs ps2 / testing the keycodes generated for the easy access keys on my trust (microsoft compatible) keyboard. I noticed the search key generated the linux/input keycode for find when connected through USB. This lead me to check the consumer page mappings in hid-input.c . And it turns out the the mapping for ID 0x221 deviates from the HUT standard document: http://www.usb.org/developers/devclass_docs/Hut1_12.pdf Currently it is incorrectly mapped to find, whereas it should be mapped to search. I also added missing bindings for ID 0x21f, the real find and for 0x222, goto. Regards, Hans When comparing usb vs ps2 / testing the keycodes generated for the easy access keys on my trust (microsoft compatible) keyboard. I noticed the search key generated the keycode for find when connected through USB. This lead me to check the consumer page mappings in hid-input.c . And it turns out the the mapping for ID 0x221 deviates from the HUT standard document: http://www.usb.org/developers/devclass_docs/Hut1_12.pdf Currently it is incorrectly mapped to find, whereas it should be mapped to search. I also added missing bindings for ID 0x21f, the real find and for 0x222, goto. Signed-off-by: Hans de Goede <[EMAIL PROTECTED]> --- linux-2.6.21.x86_64/drivers/hid/hid-input.c.hut 2007-06-12 19:26:58.0 +0200 +++ linux-2.6.21.x86_64/drivers/hid/hid-input.c 2007-06-12 19:33:32.0 +0200 @@ -598,7 +598,9 @@ case 0x21b: map_key_clear(KEY_COPY); break; case 0x21c: map_key_clear(KEY_CUT); break; case 0x21d: map_key_clear(KEY_PASTE); break; -case 0x221: map_key_clear(KEY_FIND); break; +case 0x21f: map_key_clear(KEY_FIND); break; +case 0x221: map_key_clear(KEY_SEARCH); break; +case 0x222: map_key_clear(KEY_GOTO); break; case 0x223: map_key_clear(KEY_HOMEPAGE); break; case 0x224: map_key_clear(KEY_BACK); break; case 0x225: map_key_clear(KEY_FORWARD); break;
Proposal: change keycode for scancode e0 32 from 150 to 172
Hi all, As some of you might know from my earlier post/thread about atkbd and softraw, I'm currently working on getting keyboards with internet/easy access keys to work painlessly / plug and play. In order to be able to better test / develop this I've bought 2 cheap such keyboards today, one ps2 and one both usb and ps2 capable. When comparing usb vs ps2 / testing the keycodes generated for the easy access keys on my trust (microsoft compatible) keyboard. I noticed the homepage key sends keycode 150 with ps2 and 172 with USB, or for those who don't know the keycodes by head with ps2 it sends KEY_WWW and with usb it sends KEY_HOMEPAGE I personally believe that the usb behaviour is correct and that the ps/2 code should be modified to match for consistency. The ps/2 scancode to keycode mapping is set up to handle easy access / internet keys for microsoft compatible keyboards. So what is the right code to send here, tricky, see: http://www.s2.com.br/s2arquivos/361/Imagens/555Image.jpg http://www.keyboardco.com/keyboard_images/microsoft_ergonomic_keyboard_4000_black_usb_large.jpg The logo on the key is a homepage logo, the text below is www/homepage. So what to send? I believe that for consistency with the usb codes send it should be KEY_HOMEPAGE, but thats based on a sample of 1 usb keyboard. Input on what other usb keyboards send for the key with the homepage iocn is very much welcome. Regards, Hans - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Proposal: change keycode for scancode e0 32 from 150 to 172
H. Peter Anvin wrote: Hans de Goede wrote: In order to be able to better test / develop this I've bought 2 cheap such keyboards today, one ps2 and one both usb and ps2 capable. When comparing usb vs ps2 / testing the keycodes generated for the easy access keys on my trust (microsoft compatible) keyboard. I noticed the homepage key sends keycode 150 with ps2 and 172 with USB, or for those who don't know the keycodes by head with ps2 it sends KEY_WWW and with usb it sends KEY_HOMEPAGE I just tested this using Microsoft Natural Keyboard Pro, which is a dual-mode (USB-PS/2) keyboard. This key is labelled Web/Home and has a picture of a house on the keycap. In PS/2 mode it reports E0 32 which gets converted to keycode 150. In USB mode it reports E0 02 which gets converted to keycode 172. Thanks, that confirms that the ps/2 translation (which assumes a microsoft or compatible keyboard) is wrong. I don't know if it's the keyboard itself that's being inconsistent, or if it is the table in usbkbd.c that's broken (in which case it should be fixed to be consistent with the keyboard in PS/2 mode.) See below. I personally believe that the usb behaviour is correct and that the ps/2 code should be modified to match for consistency. The ps/2 scancode to keycode mapping is set up to handle easy access / internet keys for microsoft compatible keyboards. So what is the right code to send here, tricky, see: http://www.s2.com.br/s2arquivos/361/Imagens/555Image.jpg http://www.keyboardco.com/keyboard_images/microsoft_ergonomic_keyboard_4000_black_usb_large.jpg The logo on the key is a homepage logo, the text below is www/homepage. So what to send? I believe that for consistency with the usb codes send it should be KEY_HOMEPAGE, but thats based on a sample of 1 usb keyboard. Input on what other usb keyboards send for the key with the homepage iocn is very much welcome. You seem to be of the opinion that "usb behaviour is correct", but don't give any motivation why usb should take precedence. Offhand, I would expect there to be fewer translation layers for PS/2 and would therefore assume PS/2 is more inherently correct. I'm of the opinion that the USB behaviour is correct, because usb generates the 172 / KEY_HOMEPAGE in accordance with: http://www.usb.org/developers/devclass_docs/Hut1_12.pdf Page 84 Where as in the ps2 world there is no official scancode mapping for these special keys, hence I talk about microsoft and compatibles. Also the number of layers of translation in both cases is just 1: one table (ps2) versus one switch statement (usb) Regards, Hans - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Proposal: change keycode for scancode e0 32 from 150 to 172
Jiri Kosina wrote: On Tue, 12 Jun 2007, H. Peter Anvin wrote: In PS/2 mode it reports E0 32 which gets converted to keycode 150. In USB mode it reports E0 02 which gets converted to keycode 172. I don't know if it's the keyboard itself that's being inconsistent, or if it is the table in usbkbd.c that's broken (in which case it should be fixed to be consistent with the keyboard in PS/2 mode.) Hi Peter, First, usbkbd.c has very probably zero business with this - the mappings are being done in hid-input.c, usbkdb.c is only for embedded/debugging cases, and is almost never used on modern systems (see the corresponding Kconfig help text). You seem to be of the opinion that "usb behaviour is correct", but don't give any motivation why usb should take precedence. Offhand, I would expect there to be fewer translation layers for PS/2 and would therefore assume PS/2 is more inherently correct. For USB, we have Hid Usage Pages, which define this to be KEY_HOMEPAGE. There is no such specification for PS/2 though, so what Hans is proposing is to make it consistent with behavior of USB HID devices, which I agree with. Good to hear, so as everyone smees to agree, shall I write a (massive, complex, intrusive) patch to fix this, or are there until now silent parties that object? Regards, Hans - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: CONFIG_BREAK_MY_MACHINE
Jean Delvare wrote: On Tue, 15 May 2007 20:31:00 +, Pavel Machek wrote: Hi! Hardware Monitoring support ---> <*> Hardware Monitoring support <*> Abit uGuru VIA686A <*> IBM Hard Drive Active Protection System (hdaps) But I'm quite sure that the only module used is VIA686A (I'm rebuilding to confirm). This is a rather bad idea to build the abituguru and hdaps drivers into your kernel if you don't have these devices. Especially abituguru, as it does arbitrary port probing. hdaps should be safe (DMI based whitelist, no?) Correct. If abitguru breaks random machines, we probably should DMI whitelist, too. I never said it was breaking machines, just that it was accessing arbitrary I/O ports. This was already discussed with the driver's author (Hans de Goede, Cc'd) and I think we agreed on the principle, but it didn't happen yet. This device only exists on Abit motherboards so it would be easy enough to check the DMI vendor. A more detailed white list is also possible, but I'm not insisting on it. The driver could also be made to depend on X86, as this is the only architecture where it is useful. Hans, can you please submit a patch doing this? As you can see from the CC I've send you, I've just mailed my private army of testers / known uguru users, requesting them to send me the output of dmidecode on their uguru mobo's. Once I have this info, I'll see if there is some consistency in the manufacturer DMI field. I've mailed them, since the DMI info is needed anyways to put uguru detection in sensors-detect, but I'm not sure I want to add this to the driver too. Many other ISA drivers do io probe's too, if a users decides to start loading random drivers, he is asking for problems, should we really even try to protect against this? Regards, Hans - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
problem with softraw and keycodes > 128
Hi all, First a short intro I'm a Linux enthousiast and developer. I mainly write userspace code, but I've also written 2 kernel drivers of which one is in the mainline and the other is waiting for review. I've been experimenting with getting the internetkeys on several keyboards to work. My biggest problem with this currently is the following: Step 1: press key, dmesg says: atkbd.c: Unknown key released (translated set 2, code 0xa3 on isa0060/serio0). atkbd.c: Use 'setkeycodes e023 ' to make it known. Step 2: map key: setkeycodes e023 163 Step 3: run xev, press key. X-keycode is: 153 instead of 163 ? Problem, as the xkb files for these keyboards expect the X-keycode to be 163, as just it is under the console. Now I know that X-keycodes != console-keycodes, for example the A key is 30 on the console and 38 in X, but in the case of this special keys, both the xkb files for these internet keyboards (written by suse) and config files for special daemons like lineak, expect them to be identical. Doing: echo -n 0 >/sys/devices/platform/i8042/serio1/softraw However does make them identical. I don't know if this is an xorg or a kernel problem, but I do know that this behavior is rather annoying, and IMHO a bug. I've been reading the kernel code from input.c and atkbd.c but I cannot find anything explaining this there, so now I'm looking at the xorg kbd driver. I have the feeling though that this require someone with some more knowledge of the whole input subsystem, hence this mail. Thanks & Regards, Hans - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: problem with softraw and keycodes > 128
Vojtech Pavlik wrote: On Thu, Jun 07, 2007 at 10:21:33AM +0200, Hans de Goede wrote: Hans, I've been experimenting with getting the internetkeys on several keyboards to work. My biggest problem with this currently is the following: Step 1: press key, dmesg says: atkbd.c: Unknown key released (translated set 2, code 0xa3 on isa0060/serio0). atkbd.c: Use 'setkeycodes e023 ' to make it known. Step 2: map key: setkeycodes e023 163 Step 3: run xev, press key. X-keycode is: 153 instead of 163 ? Doing: echo -n 0 >/sys/devices/platform/i8042/serio1/softraw However does make them identical. this is a well known problem. It's not easy to explain and next to impossible to fix within the scope of operation of your test case - otherwsie it'd be fixed ages ago. The explanation why this happen will be a little longer, and will follow the life of a keystroke as it passes through the different layers. 1. First, the user presses a key. Since we're talking a 102-key PS/2-type keyboard here, the keyboard scans its matrix and figures out the position of the key. Then, since the connection to the PC is running in AT mode, it translates the key to a string (one to eight bytes) of AT-compatible scancodes. 2. The i8042 in the computer receives the scancodes, translates them to XT-compatible scancodes and makes them available to the OS. 3. In OS the i8042.c gets the scancodes and forwards them to atkbd.c 4. atkbd.c decodes the string of scancodes and turns them into a single Linux input event, and forwards it to the input core. 5. The Linux input core dispatches the events to char/keyboard.c. 6. keyboard.c, a part of the VT driver, sees that X told the VT that it wants the "raw mode", which means that the VT should give the keystrokes as the original string of XT scancodes. Hence (when softraw is 1), it consults a table and synthesizes what it thinks the original string of bytes could have been and gives that to X. For all keys that atkbd.c knows by default, this string is IDENTICAL to what atkbd.c has received. 7. X takes the string, and using an algorithm similar to, but DIFFERENT from the algoithm that atkbd.c uses, it converts them to an X scancode. The X scancodes are similar to, but different from the Linux event keycodes, for historical reasons going back to the 90's. 8. Using an xmodmap table compiled from xkb sources, X converts the X scancode to an X keysym. In the softraw == 0 case, the original string of XT scancodes is passed all along the path to X, so that X really gets what the keyboard sent. Since the table mapping Linux event keycodes to the synthesized XT scancodes in step 6. is constant and shared for all (even USB and other) keyboards, this means that the Linux kernel always presents a virtual "Linux keyboard" to X. The "Linux keyboard" always sends the same scancodes for keys with the same meaning, regardless of what the real hardware does. But doesn't it only know the meaning after doing a setkeycodes for easy-access keys? I find it somewhat counter inituitive that doing "setkeycodes e023 163" will give me a keycode 163 on the console (as reported by showkeys), but will send a scancode of 153 to the X-server. And this is why X sees a different code in softraw==1 mode than in softraw==0 mode: In the first it gets the virtual "Linux keyboard", in the other it gets access to the real hardware. Problem, as the xkb files for these keyboards expect the X-keycode to be 163, as just it is under the console. Now I know that X-keycodes != console-keycodes, for example the A key is 30 on the console and 38 in X, but in the case of this special keys, both the xkb files for these internet keyboards (written by suse) and config files for special daemons like lineak, expect them to be identical. This is wrong. Someone simply needs to write a xkb/lineak description for the "Linux keyboard", and then all these problems will go away. Well to be honest I'm looking for a somewhat more quick fix then that. Let me try to explain. Xorg comes with keyboard descriptions for many special-access keys having keyboards. These descriptions can be easily selected by the end user from the kde / gnome keyboard properties applets. This however only works with softraw=0 for 2 reasons: 1) without softraw=0 the key presses never reach the server when there hasn't been an setkeycodes command for the easy access key 2) even if a setkeycodes command was given, then the keycode given to setkeycodes doesn't match the scancode seen by xorg, messing things up. (unless one uses setkeycodes with a reverse mapping of the mapping done in the kernel) There are 3 ways
Re: problem with softraw and keycodes > 128
Vojtech Pavlik wrote: On Thu, Jun 07, 2007 at 04:55:23PM +0200, Hans de Goede wrote: 2) Somehow fix things so that selecting the right model in gnome/kde keyboard-preferences will make the keys work. Like it does now with softraw=0. Which leads me to asking what are the downsides of using softraw=0? It doesn't work with anything else but PS/2 keyboards. It's useless on eg. USB. This is an atkbd only setting, right, so indeed it doesn't affect USB, right? What I'm wondering if is there is any harm to setting softraw to 0, atleast until there is a better fix. With it set to 0, for ps/2 keyboards all the user addtionally needs todo is select the correct model, for which there are nice gui tools. Add a usb-keyboard (which in reality is the linuxkeyboard) model to the mix, for the user to select / to make default even, and things will work for usb users to, right? Since the xkb discriptions for most of the easy access keyboards have been written by Suse, I assume the tested them and it works for them. Does anyone know how suse does this? I don't think it does work. One of our guys is trying to fix it by 1) Converting the xkb multimedia keyboard descriptions to setkeycodes descriptions 2) Creating a "linux keyboard" X xkb description Then, after loading the setkeycodes at boot for the right keyboard ty[e, AT keyboard will work fine, and USB and other keyboards will not need *any* setup to have their multimedia keys working out of the box. Thats good news, but what about gui tools to select the model, as this cannot be autoprobed? And what about portability of said tools to for example freebsd? Using xkb models should work across different OS's, and is available now. All that is needed (for ps2 keyboards) is for the path of the special keys from the kernel to X-server to not change the codes. Regards, Hans - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] Hardware monitoring subsystem maintainer position is open
Jean Delvare wrote: Hi all, I am resigning from my role as hardware monitoring subsystem (drivers/hwmon) maintainer. This is too much work for me, I do not have the necessary bandwidth to review all the incoming patches, in particular new drivers, in a timely manner. Patch authors have been complaining about this repeatedly. This is no fun for them, and even less for me, so I'd rather let someone else with more spare time take care of it. If there are volunteers, this is the right time to speak up. I'm sorry to hear this and I can't help but think that my, erm, rant* yesterday is somehow involved in you making this decision. This was in no way my intention. My intention was to try and make you change how you handle these things, my intention was to make you less of a perfectionist and to distribute the work more, you cannot do everything yourself. I think that if you could change that you would be able to cope with the load better. I think you're experience / knowledge when it comes to hwmon stuff is very valuable and if you leave / quit doing hwmon stuff this would be a big loose. I also believe that with a looser management style, you could seriously reduce your workload coming from being the hwmon maintainer and at the same time get a better / faster flow of patches. But alas, I'm afraid I cannot change your mind, so that brings us to the question who will pickup the task. I've considered volunteering myself, but I'm to inexperienced when it comes to kernel stuff and lm-sensors for me is kinda a side project. I sink most of my spare time into Fedora where I maintain circa 120 packages, which really means I have little time left for lm-sensors (to be clear the average package maintainer maintains between 30 - 50 packages). However I do hereby promise to help the new "Jean" in anyway I can, and although not many know me very well here I think I can safely say I can help in many ways. I'm a (very) experienced c-programmer specialised in lowlevel code, embedded (linux) systems, etc. and I have a bachelor in electronics. So who-ever becomes the new "Jean" don't hesitate to ask for help, the worst thing that can happen is that I say no :) Jean, thanks for all your hard work the past years! Regards, Hans * for those not on the lmsensors list I am one of the complaining patch authors - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] Hardware monitoring subsystem maintainer position is open
Rudolf Marek wrote: Hello all, Maybe we could try following: 1) person post the driver 2) quick review could be done critical fixes only, driver goes to -mm 3) review that goes deeper - check for interface conformity and all the stuff which could break - fixes for non-critical stuff 4) after this driver goes to Linus tree at the very beginning for the merge cycle marked as EXPERIMENTAL 5) if the person agrees to be in MAINTAINER final review may be done and EXPERIMENTAL could be removed. (This is step which might involve all steps to make the driver really perfect ;) Well this is just an idea. Neither it does solve the maintainer problem, nor it is perfect solution. I like the general direction, but I don't really see a clear difference between step 2 and 3, IMO the border between these steps is vague. Also this doesn't address who can do (which kinda) reviews. I've been thinking about a solution for the slow path for new driver inclusion myself here is what I propose: 1a) We need a set of review guidelines / a review checklist. Here is a start: * Must follow kernel coding style guidelines * sysfs interface must be in accordance with Documentation/hwmon/sysfs * proper locking to avoid 2 simultaneous attempts to communicate with the device when for example multiple sysfs files get read at once. * check the code for any obvious programming errors, like: -not freeing resources (in error paths and in general) -overrunning an array -not checking return values of calls to other parts of the kernel -of by one errors / bad loops -etc. 1b) We need to decide somehow who can do reviews. For now I say anyone who already maintains atleast one hwmon driver. But thats just a wild shot better ideas are welcome. 2) We need a review process for new drivers, suggestion: 1. Person (the submitter) posts driver 2a. Someone (the reviewer) reviews it according to our checklist / guidelines and posts a detailed report of his findings and what needs fixing 2b. The submitter posts a fixed version 2c. The reviewer re-reviews the fixed version and either approves the fixed version, goto 3 or points out things that (still) need fixing, goto 2b. 3. The driver gets send to the -mm tree (marked as EXPERIMENTAL) 4. Another person (the second reviewer) reviews it, same process as with step 2, once approved goto 5 5. Send to Linus tree at the very beginning for the merge cycle still marked as EXPERIMENTAL 6. After quite some time and positive usage reports without any negative counter reports a patch gets send to Linux to remove EXPERIMENTAL status. 3) We need a review process for fixes to existing drivers, suggestion: 1. Person (the submitter) posts a patch to an existing driver 2. The driver maintainer is responsible for handling this patch and reviews it. If necessary he either modifies the patch himself or asks the submitter to make modifications. 3. Once the maintainer is happy with the patch, he sends it to Linus tree at the beginning for the merge cycle. The rationale of doing the new driver thing this way is to avoid having a single person who bears end responsibilities for all reviews and thus becomes a potential bottleneck. The problem with distributing reviews though is that most of us aren't as experienced as Jean is. Thus I decided to just do the review twice, to compensate (a bit) for this. The rationale of doing modifications of existing drivers through the maintainer is that he knows the code best, and thus can best judge if fixes are really fixes or more bandaids / workarounds. And if these fixes have any unwanted side effects. To give proper credit: the above is heavily inspired by how Fedora handles new packages, Fedora has thousands of packages written and maintained by 100's of contributers. Using a model where any contributer, once he has taken the first horde of becoming a contributer and thus proving (some) competence can review other contributers new packages. Problems with the above, I'm not completely happy with my current proposal for deciding who can do reviews. Also I guess that Andrew and Linus would like to have a single person to take hwmon patches from, thus we still need someone todo the actual collecting of approved patches and sending them to Andrew and/or Linus. Last but not least I want to say that we should not focus too much on review, review can only get us sofar. In my (abituguru) experience most real life hwmon problems do not come from things easily catched by a review, but rather from problems with (certain versions of) the hardware responding different then expected. No amount of review will catch these cases, thus we also need to concentrate on testing. For both abituguru drivers I've posted to the forums of the abit websites and the lm-sensors list as soon as I got something usable and that way managed to build a small team of circa 8 testers for each version, a team which I send eac
Re: [lm-sensors] Hardware monitoring subsystem maintainer positionis open
Krzysztof Helt wrote: This is comments from someone who is newbie to this list. 1a) We need a set of review guidelines / a review checklist. Here is a start: Maybe these guidelines can be described in more details and with links or names of documents with more description. Yes they should, this was just a quick list. Feel free to help writing a better list. And I guess we should put this list in the wiki somewhere. * Must follow kernel coding style guidelines Is there any tool to check this? If there is one, a basic instruction how to use it would be great. No tool. * sysfs interface must be in accordance with Documentation/hwmon/sysfs The documentation is still confusing to me. Can someone of you update it to answer these questions directly? A. What is meaning of 0 and 1 values in pwmX_enable ? B. How to stop the fan (pwmX_enable = 1 and pwmX = 0 was suggested to someone on the list)? C. How t o handle situation if the pwm register minimum value (0) does not stop the fan, but there is a separate bit to do it? (do stop emulate with the bit when 0 is written to pwmX?) I think this is best answered by Jean and/or Mark * proper locking to avoid 2 simultaneous attempts to communicate with the device when for example multiple sysfs files get read at once. An example or two common errors would be great help. Erm, if someone doesn't know what this means / how to look for this he shouldn't be reviewing a driver. * check the code for any obvious programming errors, like: -not freeing resources (in error paths and in general) -overrunning an array -not checking return values of calls to other parts of the kernel -of by one errors / bad loops -etc. List them, so a newbie can check the code against it. The etc. was because I'm sure there are more but I couldn't come up with any right there and then. And again a newbie shouldn't be reviewing, he should start reading some book in device drivers writing a driver or two himself get those reviewed and then he should no longer be a newbie :) 1b) We need to decide somehow who can do reviews. For now I say anyone who already maintains atleast one hwmon driver. But thats just a wild shot better ideas are welcome. There are volunteers already. In order to lessen their work you can require to follow the guidelines (if they got described) by authors of patches . Yes ofcourse authors should follow the guidelines, and check they did themselves before submitting. Also its great that there are volunteers, but reviewing does require a certain level of competence. There are many other ways to help out for those without the necessary skills todo the reviewing. For example you could check out the 3.0.0 branch: svn checkout http://lm-sensors.org/svn/lm-sensors/branches/lm-sensors-3.0.0 Edit lib/chips.c, goto the long array at the end and comment any entries for chips you have. Optionally edit prog/sensors/main.c goto the array near the end and again comment entries for chips you have. Build and install lm-sensors-3.0.0 and let us know how it works, despite the commenting it should still recognize your cips and print the same info as usual (it can now dynamicly recognize new/unknown chips as long as the kernel supports them). When you skipped the optional step run the sensors command as 'sensors -g', the normal sensors command will be borked if you skipped this step. Thanks & Regards, Hans - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] dt: bindings: add allwinner,otg-routed property for phy-sun4i-usb
Hi,
On 09/21/2016 10:04 AM, Icenowy Zheng wrote:
On some newer Allwinner SoCs (H3 or A64), the PHY0 can be either routed to
the MUSB controller (which is an OTG controller) or the OHCI/EHCI pair
(which is a Host-only controller, but more stable and easy to implement).
This property marks whether on a certain board which controller should be
attached to the PHY.
Signed-off-by: Icenowy Zheng
Erm, I think that the idea here is to dynamically switch the routing
based on the id-pin of the otg connector. IOW use the musb controller
for device mode, and the ehci/ohci proper for proper host support
when in host mode.
Regards,
Hans
---
Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt | 7 +++
1 file changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
index 287150d..5c11d57 100644
--- a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
+++ b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
@@ -36,6 +36,13 @@ Optional properties:
- usb1_vbus-supply : regulator phandle for controller usb1 vbus
- usb2_vbus-supply : regulator phandle for controller usb2 vbus
+Optional properties for H3 or A64 SoCs:
+- allwinner,otg-routed : USB0 (OTG) PHY is routed to OHCI/EHCI pair rather than
+MUSB. (boolean, if this property is set, the OHCI/EHCI
+controllers at PHY0 should be enabled and the MUSB
+controller must *NOT* be enabled, and thus the PHY can
+only work in host mode)
+
Example:
usbphy: phy@0x01c13400 {
#phy-cells = <1>;
Re: [PATCH 1/2] dt: bindings: add allwinner,otg-routed property for phy-sun4i-usb
Hi, On 09/21/2016 10:19 AM, Icenowy Zheng wrote: 21.09.2016, 15:10, "Hans de Goede" : Hi, On 09/21/2016 10:04 AM, Icenowy Zheng wrote: On some newer Allwinner SoCs (H3 or A64), the PHY0 can be either routed to the MUSB controller (which is an OTG controller) or the OHCI/EHCI pair (which is a Host-only controller, but more stable and easy to implement). This property marks whether on a certain board which controller should be attached to the PHY. Signed-off-by: Icenowy Zheng Erm, I think that the idea here is to dynamically switch the routing based on the id-pin of the otg connector. IOW use the musb controller for device mode, and the ehci/ohci proper for proper host support when in host mode. At least on some boards this implementation works... (I mean Pine64, which has two USB-A connectors) Right and I think it is great that you're working on this. But even with an A connector on the board, we can still use the device mode (e.g. the SoC's native FEL mode will be used this way). Notice that you can fake id-pin changes by echoing a mode to: /sys/devices/platform/soc@01c0/1c13000.usb/musb-hdrc.1.auto/mode Valid values to echo are: host, peripheral and otg. If you combine this with using either an USB A<->A cable, or using the port normally as a host you should be able to develop and test full otg support. Eventually we will need a full otg support rather then your current solution and I'm afraid that your solution may get in the way of full otg support. Regards, Hans
Re: [linux-sunxi] [PATCH v2] ARM: dts: sun8i: Add dts file for Olimex A33-OLinuXino
Hi,
On 20-06-16 08:42, Stefan Mavrodiev wrote:
A33-OLinuXino is A33 development board designed by Olimex LTD.
It has AXP233 PMU, 1GB DRAM, a micro SD card, one USB-OTG connector,
headphone and mic jacks, connector for LiPo battery and optional
4GB NAND Flash.
It has two 40-pin headers. One for LCD panel, and one for
additional modules. Also there is CSI/DSI connector.
Signed-off-by: Stefan Mavrodiev
The new looks good to me:
Reviewed-by: Hans de Goede
Regards,
Hans
---
Changes for v2:
- Removed unused power nodes
- Removed default-trigger for green led
- Removed "always-on" option for LCD power
arch/arm/boot/dts/Makefile| 1 +
arch/arm/boot/dts/sun8i-a33-olinuxino.dts | 207 ++
2 files changed, 208 insertions(+)
create mode 100644 arch/arm/boot/dts/sun8i-a33-olinuxino.dts
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 970e906..b78f363 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -760,6 +760,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
sun8i-a33-ippo-q8h-v1.2.dtb \
sun8i-a33-q8-tablet.dtb \
sun8i-a33-sinlinx-sina33.dtb \
+ sun8i-a33-olinuxino.dtb \
sun8i-a83t-allwinner-h8homlet-v2.dtb \
sun8i-a83t-cubietruck-plus.dtb \
sun8i-h3-orangepi-2.dtb \
diff --git a/arch/arm/boot/dts/sun8i-a33-olinuxino.dts
b/arch/arm/boot/dts/sun8i-a33-olinuxino.dts
new file mode 100644
index 000..14fa801
--- /dev/null
+++ b/arch/arm/boot/dts/sun8i-a33-olinuxino.dts
@@ -0,0 +1,207 @@
+/*
+ * Copyright 2016 - Stefan Mavrodiev
+ * Olimex LTD.
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This file 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; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This file is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+#include "sun8i-a33.dtsi"
+#include "sunxi-common-regulators.dtsi"
+
+#include
+#include
+#include
+
+/ {
+ model = "A33-OLinuXino";
+ compatible = "allwinner,sun8i-a33";
+
+ aliases {
+ serial0 = &uart0;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ leds {
+ compatible = "gpio-leds";
+ pinctrl-names = "default";
+ pinctrl-0 = <&led_pin_olinuxino>;
+
+ green {
+ label = "olinuxino:green:usr";
+ gpios = <&pio 1 7 GPIO_ACTIVE_HIGH>; /* LED2 */
+ };
+ };
+};
+
+&ehci0 {
+ status = "okay";
+};
+
+&ohci0 {
+ status = "okay";
+};
+
+&mmc0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_olinuxino>;
+ vmmc-supply = <®_dcdc1>;
+ bus-width = <4>;
+ cd-gpios = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4 */
+ cd-inverted;
+ status = "okay";
+};
+
+&pio {
+ mmc0_cd_pin_olinuxino: mmc0_cd_pin@0 {
+ a
Re: [PATCH] ARM: sun8i: Add Parrot Board DTS
Hi,
On 20-06-16 18:30, Chen-Yu Tsai wrote:
Hi,
On Mon, Jun 20, 2016 at 11:44 PM, Maxime Ripard
wrote:
Hi,
On Tue, Jun 14, 2016 at 09:19:58PM +0800, Chen-Yu Tsai wrote:
On Tue, Jun 14, 2016 at 8:59 PM, Quentin Schulz
wrote:
Hi,
On 13/06/2016 15:04, Chen-Yu Tsai wrote:
Hi,
On Mon, Jun 13, 2016 at 6:15 PM, Quentin Schulz
wrote:
The Parrot Board is an evaluation board with an Allwinner R16 (assumed
to be close to an Allwinner A33), 4GB of NAND, 512MB of RAM, USB host
You say NAND here, but you enable mmc2 for eMMC below. Please correct it.
ACK.
and OTG, a WiFi/Bluetooth combo chip, a micro SD Card reader, 2
controllable buttons, an LVDS port with separated backlight and
capacitive touch panel ports, an audio/microphone jack, a camera CSI
port, 2 sets of 22 GPIOs and an accelerometer.
I assume the board is this one:
https://world.taobao.com/item/530374411673.htm
Definitely looks like it.
Signed-off-by: Quentin Schulz
---
arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/sun8i-r16-parrot.dts | 333 +
2 files changed, 334 insertions(+)
create mode 100644 arch/arm/boot/dts/sun8i-r16-parrot.dts
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 06b6c2d..1149512 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -760,6 +760,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
sun8i-a33-ippo-q8h-v1.2.dtb \
sun8i-a33-q8-tablet.dtb \
sun8i-a33-sinlinx-sina33.dtb \
+ sun8i-r16-parrot.dtb \
sun8i-a83t-allwinner-h8homlet-v2.dtb \
sun8i-a83t-cubietruck-plus.dtb \
sun8i-h3-orangepi-2.dtb \
diff --git a/arch/arm/boot/dts/sun8i-r16-parrot.dts
b/arch/arm/boot/dts/sun8i-r16-parrot.dts
new file mode 100644
index 000..75e2420
--- /dev/null
+++ b/arch/arm/boot/dts/sun8i-r16-parrot.dts
@@ -0,0 +1,333 @@
+/*
+ * Copyright 2015 Quentin Schulz
+ *
+ * Quentin Schulz
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This file 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; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This file is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+#include "sun8i-a33.dtsi"
+#include "sunxi-common-regulators.dtsi"
+
+#include
+#include
+
+/ {
+ model = "Allwinner Parrot EVB R16";
+ compatible = "allwinner,parrot-evb-r16", "allwinner,sun8i-a33";
+
+ aliases {
+ serial0 = &uart0;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ leds {
+ compatible = "gpio-leds";
+ pinctrl-names = "default";
+ pinctrl-0 = <&led_pins_r16>;
IMO r16 is too generic. You may want to add parrot_ or parrot_evb_ to it.
Same goes for all the other r16 identifier names.
ACK.
+
+ led1 {
+ label = "r16:led1:usr";
+ gpio = <&pio 4 17 GPIO_ACTIVE_HIGH>; /* PE17 */
+ };
+
+ led2 {
+ label = "r16:led2:usr";
+ gpio = <&pio 4 16 GPIO_ACTIVE_HIGH>; /* PE16 */
+ };
+ };
+
+ wifi_pwrseq: wifi_pwrseq {
+ compatible = "mmc-pwrseq-simple";
+
Re: [PATCH 2/2] mmc: host: sunxi: add support for A64 mmc controller
Hi,
On 30-07-16 11:36, Icenowy Zheng wrote:
A64 SoC features a MMC controller which need only the mod clock, and can
calibrate delay by itself. This patch adds support for the new MMC
controller IP core.
Signed-off-by: Icenowy Zheng
Cool stuff, thanks for your work on this!
---
drivers/mmc/host/sunxi-mmc.c | 166 +++
1 file changed, 122 insertions(+), 44 deletions(-)
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 2ee4c21..ac56bcf 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -72,6 +72,14 @@
#define SDXC_REG_CHDA (0x90)
#define SDXC_REG_CBDA (0x94)
+/* New registers introduced in A64 */
+#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
+#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
+#define SDXC_REG_DRV_DL0x140 /* Drive Delay Control Register */
+#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
+#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
+
+
#define mmc_readl(host, reg) \
readl((host)->reg_base + SDXC_##reg)
#define mmc_writel(host, reg, value) \
@@ -217,6 +225,15 @@
#define SDXC_CLK_50M_DDR 3
#define SDXC_CLK_50M_DDR_8BIT 4
+#define SDXC_2X_TIMING_MODEBIT(31)
+
+#define SDXC_CAL_START BIT(15)
+#define SDXC_CAL_DONE BIT(14)
+#define SDXC_CAL_DL_SHIFT 8
+#define SDXC_CAL_DL_SW_EN BIT(7)
+#define SDXC_CAL_DL_SW_SHIFT 0
+#define SDXC_CAL_DL_MASK 0x3f
+
struct sunxi_mmc_clk_delay {
u32 output;
u32 sample;
@@ -261,6 +278,9 @@ struct sunxi_mmc_host {
/* vqmmc */
boolvqmmc_enabled;
+
+ /* does the IP block support autocalibration? */
+ boolcan_calibrate;
};
static int sunxi_mmc_reset_host(struct sunxi_mmc_host *host)
@@ -653,10 +673,66 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host
*host, u32 oclk_en)
return 0;
}
+static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
+ struct mmc_ios *ios, int reg_off)
+{
+ u32 reg = readl(host->reg_base + reg_off);
+ u32 delay;
+
+ reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
+ reg &= ~SDXC_CAL_DL_SW_EN;
+
+ writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration started\n");
+
+ while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
+ cpu_relax();
+
+ delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
+
+ reg &= ~SDXC_CAL_START;
+ reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
+
+ writel(reg, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
+
+ return 0;
+}
+
+static int sunxi_mmc_determine_delays(struct sunxi_mmc_host *host,
+ struct mmc_ios *ios, int rate)
+{
+ int index;
+
+ if (rate <= 40) {
+ index = SDXC_CLK_400K;
+ } else if (rate <= 2500) {
+ index = SDXC_CLK_25M;
+ } else if (rate <= 5200) {
+ if (ios->timing != MMC_TIMING_UHS_DDR50 &&
+ ios->timing != MMC_TIMING_MMC_DDR52) {
+ index = SDXC_CLK_50M;
+ } else if (ios->bus_width == MMC_BUS_WIDTH_8) {
+ index = SDXC_CLK_50M_DDR_8BIT;
+ } else {
+ index = SDXC_CLK_50M_DDR;
+ }
+ } else {
+ return -EINVAL;
+ }
+
+ clk_set_phase(host->clk_sample, host->clk_delays[index].sample);
+ clk_set_phase(host->clk_output, host->clk_delays[index].output);
+
+ return 0;
+}
+
The factoring out of this into a function really should be done in
a separate preparation patch, that will also make the patch making
the actual functional changes much easier to read.
Regards,
Hans
static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
struct mmc_ios *ios)
{
- u32 rate, oclk_dly, rval, sclk_dly;
+ u32 rate, rval;
u32 clock = ios->clock;
int ret;
@@ -692,32 +768,18 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host
*host,
}
mmc_writel(host, REG_CLKCR, rval);
- /* determine delays */
- if (rate <= 40) {
- oclk_dly = host->clk_delays[SDXC_CLK_400K].output;
- sclk_dly = host->clk_delays[SDXC_CLK_400K].sample;
- } else if (rate <= 2500) {
- oclk_dly = host->clk_delays[SDXC_CLK_25M].output;
- sclk_dly = host->clk_delays[SDXC_CLK_25M].sample;
- } else if (rate <= 5200) {
- if (ios->timing != MMC_TIMING_UHS_DDR50 &&
- ios->timing != MMC_TIMING_MMC_DDR52) {
- oclk_dly = host->clk_delays[SDXC_CLK_
Re: [PATCH 2/2] mmc: host: sunxi: add support for A64 mmc controller
Hi,
On 30-07-16 13:35, Icenowy Zheng wrote:
30.07.2016, 18:30, "Hans de Goede" :
Hi,
On 30-07-16 11:36, Icenowy Zheng wrote:
A64 SoC features a MMC controller which need only the mod clock, and can
calibrate delay by itself. This patch adds support for the new MMC
controller IP core.
Signed-off-by: Icenowy Zheng
Cool stuff, thanks for your work on this!
---
drivers/mmc/host/sunxi-mmc.c | 166 +++
1 file changed, 122 insertions(+), 44 deletions(-)
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 2ee4c21..ac56bcf 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -72,6 +72,14 @@
#define SDXC_REG_CHDA (0x90)
#define SDXC_REG_CBDA (0x94)
+/* New registers introduced in A64 */
+#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
+#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
+#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
+#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
+#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
+
+
#define mmc_readl(host, reg) \
readl((host)->reg_base + SDXC_##reg)
#define mmc_writel(host, reg, value) \
@@ -217,6 +225,15 @@
#define SDXC_CLK_50M_DDR 3
#define SDXC_CLK_50M_DDR_8BIT 4
+#define SDXC_2X_TIMING_MODE BIT(31)
+
+#define SDXC_CAL_START BIT(15)
+#define SDXC_CAL_DONE BIT(14)
+#define SDXC_CAL_DL_SHIFT 8
+#define SDXC_CAL_DL_SW_EN BIT(7)
+#define SDXC_CAL_DL_SW_SHIFT 0
+#define SDXC_CAL_DL_MASK 0x3f
+
struct sunxi_mmc_clk_delay {
u32 output;
u32 sample;
@@ -261,6 +278,9 @@ struct sunxi_mmc_host {
/* vqmmc */
bool vqmmc_enabled;
+
+ /* does the IP block support autocalibration? */
+ bool can_calibrate;
};
static int sunxi_mmc_reset_host(struct sunxi_mmc_host *host)
@@ -653,10 +673,66 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host
*host, u32 oclk_en)
return 0;
}
+static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
+ struct mmc_ios *ios, int reg_off)
+{
+ u32 reg = readl(host->reg_base + reg_off);
+ u32 delay;
+
+ reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
+ reg &= ~SDXC_CAL_DL_SW_EN;
+
+ writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration started\n");
+
+ while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
+ cpu_relax();
+
+ delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
+
+ reg &= ~SDXC_CAL_START;
+ reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
+
+ writel(reg, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
+
+ return 0;
+}
+
+static int sunxi_mmc_determine_delays(struct sunxi_mmc_host *host,
+ struct mmc_ios *ios, int rate)
+{
+ int index;
+
+ if (rate <= 40) {
+ index = SDXC_CLK_400K;
+ } else if (rate <= 2500) {
+ index = SDXC_CLK_25M;
+ } else if (rate <= 5200) {
+ if (ios->timing != MMC_TIMING_UHS_DDR50 &&
+ ios->timing != MMC_TIMING_MMC_DDR52) {
+ index = SDXC_CLK_50M;
+ } else if (ios->bus_width == MMC_BUS_WIDTH_8) {
+ index = SDXC_CLK_50M_DDR_8BIT;
+ } else {
+ index = SDXC_CLK_50M_DDR;
+ }
+ } else {
+ return -EINVAL;
+ }
+
+ clk_set_phase(host->clk_sample, host->clk_delays[index].sample);
+ clk_set_phase(host->clk_output, host->clk_delays[index].output);
+
+ return 0;
+}
+
The factoring out of this into a function really should be done in
a separate preparation patch, that will also make the patch making
the actual functional changes much easier to read.
Thanks.
And I forgot add the infomation that the patch is based on apritzel's
work...
I will soon send a PATCH v2.
If your A10/13 mmc clock driver can be merged ASAP, then I will be
able to drop some bits from the patch.
I need to do a v2 of that, I will hopefully send that out today.
Regards,
Hans
Regards,
Hans
static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
struct mmc_ios *ios)
{
- u32 rate, oclk_dly, rval, sclk_dly;
+ u32 rate, rval;
u32 clock = ios->clock;
int ret;
@@ -692,32 +768,18 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host
*host,
}
mmc_writel(host, REG_CLKCR, rval);
- /* determine delays */
- if (rate <= 40) {
- oclk_dly = host->clk_delays[SDXC_CLK_400K].output;
- sclk_dly = host->clk_delays[SDXC_CLK_400K].sample;
- } else if (rate <= 2500) {
- oclk_dly = host->clk_delays[SDXC_CLK_25M].output;
- sclk_dly = host->clk_delays[SDXC_CLK_25M].sample;
- } else if (rate <= 5200) {
- if (ios->timing != MMC_TIMING_UHS_DDR50 &&
- ios->timing != MMC_TIM
Re: [PATCH 2/2] mmc: host: sunxi: add support for A64 mmc controller
Hi,
On 30-07-16 13:35, Icenowy Zheng wrote:
30.07.2016, 18:30, "Hans de Goede" :
Hi,
On 30-07-16 11:36, Icenowy Zheng wrote:
A64 SoC features a MMC controller which need only the mod clock, and can
calibrate delay by itself. This patch adds support for the new MMC
controller IP core.
Signed-off-by: Icenowy Zheng
Cool stuff, thanks for your work on this!
---
drivers/mmc/host/sunxi-mmc.c | 166 +++
1 file changed, 122 insertions(+), 44 deletions(-)
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 2ee4c21..ac56bcf 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -72,6 +72,14 @@
#define SDXC_REG_CHDA (0x90)
#define SDXC_REG_CBDA (0x94)
+/* New registers introduced in A64 */
+#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
+#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
+#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
+#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
+#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
+
+
#define mmc_readl(host, reg) \
readl((host)->reg_base + SDXC_##reg)
#define mmc_writel(host, reg, value) \
@@ -217,6 +225,15 @@
#define SDXC_CLK_50M_DDR 3
#define SDXC_CLK_50M_DDR_8BIT 4
+#define SDXC_2X_TIMING_MODE BIT(31)
+
+#define SDXC_CAL_START BIT(15)
+#define SDXC_CAL_DONE BIT(14)
+#define SDXC_CAL_DL_SHIFT 8
+#define SDXC_CAL_DL_SW_EN BIT(7)
+#define SDXC_CAL_DL_SW_SHIFT 0
+#define SDXC_CAL_DL_MASK 0x3f
+
struct sunxi_mmc_clk_delay {
u32 output;
u32 sample;
@@ -261,6 +278,9 @@ struct sunxi_mmc_host {
/* vqmmc */
bool vqmmc_enabled;
+
+ /* does the IP block support autocalibration? */
+ bool can_calibrate;
};
static int sunxi_mmc_reset_host(struct sunxi_mmc_host *host)
@@ -653,10 +673,66 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host
*host, u32 oclk_en)
return 0;
}
+static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
+ struct mmc_ios *ios, int reg_off)
+{
+ u32 reg = readl(host->reg_base + reg_off);
+ u32 delay;
+
+ reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
+ reg &= ~SDXC_CAL_DL_SW_EN;
+
+ writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration started\n");
+
+ while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
+ cpu_relax();
+
+ delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
+
+ reg &= ~SDXC_CAL_START;
+ reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
+
+ writel(reg, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
+
+ return 0;
+}
+
+static int sunxi_mmc_determine_delays(struct sunxi_mmc_host *host,
+ struct mmc_ios *ios, int rate)
+{
+ int index;
+
+ if (rate <= 40) {
+ index = SDXC_CLK_400K;
+ } else if (rate <= 2500) {
+ index = SDXC_CLK_25M;
+ } else if (rate <= 5200) {
+ if (ios->timing != MMC_TIMING_UHS_DDR50 &&
+ ios->timing != MMC_TIMING_MMC_DDR52) {
+ index = SDXC_CLK_50M;
+ } else if (ios->bus_width == MMC_BUS_WIDTH_8) {
+ index = SDXC_CLK_50M_DDR_8BIT;
+ } else {
+ index = SDXC_CLK_50M_DDR;
+ }
+ } else {
+ return -EINVAL;
+ }
+
+ clk_set_phase(host->clk_sample, host->clk_delays[index].sample);
+ clk_set_phase(host->clk_output, host->clk_delays[index].output);
+
+ return 0;
+}
+
The factoring out of this into a function really should be done in
a separate preparation patch, that will also make the patch making
the actual functional changes much easier to read.
Thanks.
And I forgot add the infomation that the patch is based on apritzel's
work...
I will soon send a PATCH v2.
If your A10/13 mmc clock driver can be merged ASAP, then I will be
able to drop some bits from the patch.
Ok, I'm working on v2 now, and it looks like factoring out the
clk_delay / phase stuff is useful for my v2 too, so I'm going to
do a patch factoring this out myself. I'll send my v2 in 1 - 2
hours from now, you may want to base your v2 on my work (it contains
some other refactoring which should make things easier too).
Regards,
Hans
Re: [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
Hi,
On 31-07-16 13:02, Icenowy Zheng wrote:
A64 SoC features a MMC controller which need only the mod clock, and can
calibrate delay by itself. This patch adds support for the new MMC
controller IP core.
Based on work by Andre Przywara .
Signed-off-by: Icenowy Zheng
Looks good, some minor remarks (see comments inline) after those
are addressed this is ready to be merged IMHO.
---
drivers/mmc/host/sunxi-mmc.c | 106 ---
1 file changed, 90 insertions(+), 16 deletions(-)
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 2ec91ce..aa2abf3 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -72,6 +72,14 @@
#define SDXC_REG_CHDA (0x90)
#define SDXC_REG_CBDA (0x94)
+/* New registers introduced in A64 */
+#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
+#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
+#define SDXC_REG_DRV_DL0x140 /* Drive Delay Control Register */
+#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
+#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
+
+
Please drop 1 empty line here.
#define mmc_readl(host, reg) \
readl((host)->reg_base + SDXC_##reg)
#define mmc_writel(host, reg, value) \
@@ -217,6 +225,15 @@
#define SDXC_CLK_50M_DDR 3
#define SDXC_CLK_50M_DDR_8BIT 4
+#define SDXC_2X_TIMING_MODEBIT(31)
+
+#define SDXC_CAL_START BIT(15)
+#define SDXC_CAL_DONE BIT(14)
+#define SDXC_CAL_DL_SHIFT 8
+#define SDXC_CAL_DL_SW_EN BIT(7)
+#define SDXC_CAL_DL_SW_SHIFT 0
+#define SDXC_CAL_DL_MASK 0x3f
+
struct sunxi_mmc_clk_delay {
u32 output;
u32 sample;
@@ -232,6 +249,9 @@ struct sunxi_idma_des {
struct sunxi_mmc_cfg {
u32 idma_des_size_bits;
const struct sunxi_mmc_clk_delay *clk_delays;
+
I would not insert an empty line here.
+ /* does the IP block support autocalibration? */
+ bool can_calibrate;
};
struct sunxi_mmc_host {
@@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host
*host, u32 oclk_en)
return 0;
}
+static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
+ struct mmc_ios *ios, int reg_off)
+{
+ u32 reg = readl(host->reg_base + reg_off);
+ u32 delay;
+
I would add:
if (!host->cfg->can_calibrate)
return 0;
Here; and ...
+ reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
+ reg &= ~SDXC_CAL_DL_SW_EN;
+
+ writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration started\n");
+
+ while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
+ cpu_relax();
+
+ delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
+
+ reg &= ~SDXC_CAL_START;
+ reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
+
+ writel(reg, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
Add:
/* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
here; and ...
+
+ return 0;
+}
+
static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
struct mmc_ios *ios, u32 rate)
{
@@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host
*host,
}
mmc_writel(host, REG_CLKCR, rval);
- ret = sunxi_mmc_clk_set_phase(host, ios, rate);
- if (ret)
- return ret;
Keep this as is; and add:
ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
if (ret)
return ret;
Instead of:
+ if (host->cfg->can_calibrate) {
+ ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
+ if (ret)
+ return ret;
+
+ /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
+ } else {
+ ret = sunxi_mmc_clk_set_phase(host, ios, rate);
+ if (ret)
+ return ret;
+ }
return sunxi_mmc_oclk_onoff(host, 1);
}
@@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay
sun9i_mmc_clk_delays[] = {
static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
.idma_des_size_bits = 13,
.clk_delays = NULL,
+ .can_calibrate = false,
};
static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
.idma_des_size_bits = 16,
.clk_delays = NULL,
+ .can_calibrate = false,
};
static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
.idma_des_size_bits = 16,
.clk_delays = sunxi_mmc_clk_delays,
+ .can_calibrate = false,
};
static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
.idma_des_size_bits = 16,
.clk_delays = sun9i_mmc_clk_delays,
+ .can_calibrate = false,
+};
+
Re: [PATCH 2/3] phy: sun4i: add support for A64 usb phy
Hi,
On 31-07-16 13:25, Icenowy Zheng wrote:
There's something unknown in the pmu part.
Signed-off-by: Icenowy Zheng
Cool, I really like the work you're doing on A64 support,
keep up the good work!
---
drivers/phy/phy-sun4i-usb.c | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index 0a45bc6..6f94369 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -97,6 +97,7 @@ enum sun4i_usb_phy_type {
sun6i_a31_phy,
sun8i_a33_phy,
sun8i_h3_phy,
+ sun50i_a64_phy,
};
struct sun4i_usb_phy_cfg {
@@ -180,8 +181,9 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy,
u32 addr, u32 data,
mutex_lock(&phy_data->mutex);
- if (phy_data->cfg->type == sun8i_a33_phy) {
- /* A33 needs us to set phyctl to 0 explicitly */
+ if (phy_data->cfg->type == sun8i_a33_phy ||
+ phy_data->cfg->type == sun50i_a64_phy) {
+ /* A33 or A64 needs us to set phyctl to 0 explicitly */
writel(0, phyctl);
}
Maybe add a "bool explicitly_0_phyctl;" to sun4i_usb_phy_cfg ?
Note I'm not sure of this myself, feel free to keep this as is,
we can always introduce such a bool when we get a 3th SoC which
needs it.
@@ -264,6 +266,12 @@ static int sun4i_usb_phy_init(struct phy *_phy)
val = readl(phy->pmu + REG_PMU_UNK_H3);
writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
} else {
+ /* A64 needs also this unknown bit */
+ if (data->cfg->type == sun50i_a64_phy) {
+ val = readl(phy->pmu + REG_PMU_UNK_H3);
+ writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
+ }
+
/* Enable USB 45 Ohm resistor calibration */
if (phy->index == 0)
sun4i_usb_phy_write(phy, PHY_RES45_CAL_EN, 0x01, 1);
Erm, as pointed out thus duplicates code from the H3 code path, I believe
that you should add a "bool needs_h3_pmu_unk_poke;" to sun4i_usb_phy_cfg
and then change this bit of the code to:
if (data->cfg->needs_h3_pmu_unk_poke) {
val = readl(phy->pmu + REG_PMU_UNK_H3);
writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
}
if (data->cfg->type == sun8i_h3_phy) {
if (phy->index == 0) {
val = readl(data->base + REG_PHY_UNK_H3);
writel(val & ~1, data->base + REG_PHY_UNK_H3);
}
} else {
... (original code)
}
That seems like a cleaner solution to me.
And do not forget to set the needs_h3_pmu_unk_poke for the h3!
I would not add it to the sun4i_usb_phy_cfg structs for the
other type SoCs, if part of the struct is initialized the
rest will get set to 0 by the compiler and I believe that
it things will be more readable without an explicit:
.needs_h3_pmu_unk_poke = false
Everywhere.
Thanks & Regards,
Hans
@@ -762,6 +770,14 @@ static const struct sun4i_usb_phy_cfg sun8i_h3_cfg = {
.dedicated_clocks = true,
};
+static const struct sun4i_usb_phy_cfg sun50i_a64_cfg = {
+ .num_phys = 2,
+ .type = sun50i_a64_phy,
+ .disc_thresh = 3,
+ .phyctl_offset = REG_PHYCTL_A33,
+ .dedicated_clocks = true,
+};
+
static const struct of_device_id sun4i_usb_phy_of_match[] = {
{ .compatible = "allwinner,sun4i-a10-usb-phy", .data = &sun4i_a10_cfg },
{ .compatible = "allwinner,sun5i-a13-usb-phy", .data = &sun5i_a13_cfg },
@@ -770,6 +786,7 @@ static const struct of_device_id sun4i_usb_phy_of_match[] =
{
{ .compatible = "allwinner,sun8i-a23-usb-phy", .data = &sun8i_a23_cfg },
{ .compatible = "allwinner,sun8i-a33-usb-phy", .data = &sun8i_a33_cfg },
{ .compatible = "allwinner,sun8i-h3-usb-phy", .data = &sun8i_h3_cfg },
+ { .compatible = "allwinner,sun50i-a64-usb-phy", .data =
&sun50i_a64_cfg},
{ },
};
MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
Re: [PATCH 2/3] phy: sun4i: add support for A64 usb phy
Hi, On 31-07-16 16:50, Chen-Yu Tsai wrote: FYI: H3 USB PHY support is not complete. USB0 PHY is not supported, and it does not work. I did a preliminary comparison of this PHY driver and the code in Allwinner's SDK. There are some bits missing. Right that is a known issue, I believe someone was working on an otg support patch series for the H3 though ? Regards, Hans
Re: [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
Hi,
On 01-08-16 01:48, Icenowy Zheng wrote:
Hi,
31.07.2016, 22:30, "Hans de Goede" :
Hi,
On 31-07-16 13:02, Icenowy Zheng wrote:
A64 SoC features a MMC controller which need only the mod clock, and can
calibrate delay by itself. This patch adds support for the new MMC
controller IP core.
Based on work by Andre Przywara .
Signed-off-by: Icenowy Zheng
Looks good, some minor remarks (see comments inline) after those
are addressed this is ready to be merged IMHO.
---
drivers/mmc/host/sunxi-mmc.c | 106 ---
1 file changed, 90 insertions(+), 16 deletions(-)
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 2ec91ce..aa2abf3 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -72,6 +72,14 @@
#define SDXC_REG_CHDA (0x90)
#define SDXC_REG_CBDA (0x94)
+/* New registers introduced in A64 */
+#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
+#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
+#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
+#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
+#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
+
+
Please drop 1 empty line here.
Thanks for you notice... I forgot to checkout the empty line...
#define mmc_readl(host, reg) \
readl((host)->reg_base + SDXC_##reg)
#define mmc_writel(host, reg, value) \
@@ -217,6 +225,15 @@
#define SDXC_CLK_50M_DDR 3
#define SDXC_CLK_50M_DDR_8BIT 4
+#define SDXC_2X_TIMING_MODE BIT(31)
+
+#define SDXC_CAL_START BIT(15)
+#define SDXC_CAL_DONE BIT(14)
+#define SDXC_CAL_DL_SHIFT 8
+#define SDXC_CAL_DL_SW_EN BIT(7)
+#define SDXC_CAL_DL_SW_SHIFT 0
+#define SDXC_CAL_DL_MASK 0x3f
+
struct sunxi_mmc_clk_delay {
u32 output;
u32 sample;
@@ -232,6 +249,9 @@ struct sunxi_idma_des {
struct sunxi_mmc_cfg {
u32 idma_des_size_bits;
const struct sunxi_mmc_clk_delay *clk_delays;
+
I would not insert an empty line here.
I'm only feeling that a empty before such a comment line is a
kind of separator...
(Refer to struct sunxi_mmc_host)
Ok.
+ /* does the IP block support autocalibration? */
+ bool can_calibrate;
};
struct sunxi_mmc_host {
@@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host
*host, u32 oclk_en)
return 0;
}
+static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
+ struct mmc_ios *ios, int reg_off)
+{
+ u32 reg = readl(host->reg_base + reg_off);
+ u32 delay;
+
I would add:
if (!host->cfg->can_calibrate)
return 0;
Here; and ...
I agree with it. It's some kind of error checking.
+ reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
+ reg &= ~SDXC_CAL_DL_SW_EN;
+
+ writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration started\n");
+
+ while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
+ cpu_relax();
+
+ delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
+
+ reg &= ~SDXC_CAL_START;
+ reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
+
+ writel(reg, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
Add:
/* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
here; and ...
The function only received a register address as a parameter...
It do not know the difference between different regs to calibrate.
We can change the parameters the function gets, or even introduce
a new function once we fix the TODO.
For now we just need a place to put the TODO and this seems like
the best place.
+
+ return 0;
+}
+
static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
struct mmc_ios *ios, u32 rate)
{
@@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host
*host,
}
mmc_writel(host, REG_CLKCR, rval);
- ret = sunxi_mmc_clk_set_phase(host, ios, rate);
- if (ret)
- return ret;
Keep this as is; and add:
ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
if (ret)
return ret;
It's a good idea :-)
Instead of:
+ if (host->cfg->can_calibrate) {
+ ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
+ if (ret)
+ return ret;
+
+ /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
+ } else {
+ ret = sunxi_mmc_clk_set_phase(host, ios, rate);
+ if (ret)
+ return ret;
+ }
return sunxi_mmc_oclk_onoff(host, 1);
}
@@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay
sun9i_mmc_clk_delays[] = {
static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
.idma_des_size_bits =
Re: [PATCH 3/3] ehci-platform: add the max clock number to 4
Hi, On 01-08-16 09:05, Icenowy Zheng wrote: clocks = <&ccu CLK_A64_BUS_OHCI1>, <&ccu CLK_A64_BUS_EHCI1>, <&ccu CLK_A64_USB_OHCI0>, <&ccu CLK_A64_USB_OHCI1>; On A64, EHCI requires the matched OHCI to work. Ah, so just like on the H3 (where this also is needed and not documented). And OHCI1 clock requires OHCI0 clock to work. Hmm, that one is new, can you double check this ? Regards, Hans (But from the SoC's user manual we cannot get any infomation about the relationship between OHCI1 clock and OHCI0 clock, and in the manual OHCI0 clock is called OTG-OHCI) 01.08.2016, 15:01, "Arnd Bergmann" : On Sunday, July 31, 2016 7:25:36 PM CEST Icenowy Zheng wrote: Allwinner A64 EHCI requires 4 clocks to be enabled. Signed-off-by: Icenowy Zheng Can you say what those four clocks are? Are you sure that it's not just a case of a clock being incorrectly described in the clk driver, i.e. you reference one clock along with its parent here? Arnd
Re: [PATCH 3/3] ehci-platform: add the max clock number to 4
Hi, On 01-08-16 10:18, Icenowy Zheng wrote: 01.08.2016, 15:27, "Hans de Goede" : Hi, On 01-08-16 09:05, Icenowy Zheng wrote: clocks = <&ccu CLK_A64_BUS_OHCI1>, <&ccu CLK_A64_BUS_EHCI1>, <&ccu CLK_A64_USB_OHCI0>, <&ccu CLK_A64_USB_OHCI1>; On A64, EHCI requires the matched OHCI to work. Ah, so just like on the H3 (where this also is needed and not documented). Yes, I feeled that A64 is like a hybrid of H3 and A33. And OHCI1 clock requires OHCI0 clock to work. Hmm, that one is new, can you double check this ? SCLK_GATING_OHCI. Gating Special Clock For OHCI(48M and 12M) 00: Clock is OFF 01: OTG-OHCI Clock is ON 10: Clock is OFF 11:OTG-OHCI and OHCI0 Clock is ON P.113 of A64 user manual 1.0 Ah I see that looks weird, I assume that you're working on getting the regular usb host on the A64 to work, iow the "HCI0" block in the usb block diagram at p. 580, right ? It could be that the ohci clock for that somehow is tapped from the ohci clock for the "USB-OTG-HCI" block, but: P.113, other bits of the USBPHY_CFG_REG register: 23:22 R/W 0x0 OHCI1_12M_SRC_SEL. OHCI1 12M Source Select 00: 12M divided from 48M 01: 12M divided from 24M 10: LOSC 11: / 21:20 R/W 0x0 OHCI0_12M_SRC_SEL. OHCI0 12M Source Select 00: 12M divided from 48M 01: 12M divided from 24M 10: LOSC 11: / Suggests that they are independent... Have you tried to simply drop <&ccu CLK_A64_USB_OHCI0>, From the clocks list and check that usb-1 devices (e.g. a mouse / keyboard) plugged directly into the board still work ? If it does we can simply drop it, of it does not work then indeed we need 4 clocks because allwinner has done something weird again. ### Also it seems that the CLK_A64_USB_OHCI0 / CLK_A64_USB_OHCI1 names are wrong, the datasheet consistently (*) refers to "usb-otg-ohci" and an "usb-ohci0" rather then ohci0 and ohci1 (**) Except for the USBPHY_CFG_REG documentation for bits 20:23, which I believe is an error in the datasheet. So we should do the same in the dt-bindings IMHO. Regards, Hans *) In the system address map (p. 73), "Interrupt Source" list (p.210) in the "Bus Clock Gating Register0" doc (p. 100) and in the usb block diagram (p. 580). **) Unlike the H3 where usb-otg-ohci is called usb-ohci0 and the first non otg host controller is called usb-ohci1.
Re: [PATCH] usb: phy: add USB_SUPPORT dependency
Hi,
On 06-09-16 14:54, Arnd Bergmann wrote:
The driver now calls of_usb_get_dr_mode_by_phy, which is part of the
USB core layer, and it fails to build when that is not provided:
drivers/phy/phy-sun4i-usb.o: In function `sun4i_usb_phy_probe':
phy-sun4i-usb.c:(.text.sun4i_usb_phy_probe+0x140): undefined reference to
`of_usb_get_dr_mode_by_phy'
We already have a couple of other PHY drivers with a dependency
on USB_SUPPORT, so that seems to be the easiest fix here.
An alternative would be to adjust the #ifdef in include/linux/usb/of.h
to also check for CONFIG_USB_SUPPORT.
Signed-off-by: Arnd Bergmann
Fixes: b33ecca87df9 ("phy-sun4i-usb: Add support for peripheral-only mode")
Good catch, patch LGTM:
Reviewed-by: Hans de Goede
Regards,
Hans
---
drivers/phy/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index c42ddf3c8ed8..985dff8558e5 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -260,6 +260,7 @@ config PHY_SUN4I_USB
depends on RESET_CONTROLLER
depends on EXTCON
depends on POWER_SUPPLY
+ depends on USB_SUPPORT
select GENERIC_PHY
help
Enable this to support the transceiver that is part of Allwinner
Re: [PATCH] simplefb: Disable and release clocks and regulators in destroy callback
Hi,
On 07-09-16 11:09, Chen-Yu Tsai wrote:
simplefb gets unregister when a proper framebuffer driver comes in and
kicks it out. However the claimed clocks and regulators stay enabled
as they are only released in the platform device remove function, which
in theory would never get called.
Move the clock/regulator cleanup into the framebuffer destroy callback,
which gets called as part of the framebuffer unregister process.
Note this introduces asymmetry in how the resources are claimed and
released.
Signed-off-by: Chen-Yu Tsai
Good catch, patch LGTM:
Reviewed-by: Hans de Goede
Regards,
Hans
---
drivers/video/fbdev/simplefb.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index e9cf19977285..3620d10a0d00 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -74,8 +74,14 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int
green, u_int blue,
return 0;
}
+struct simplefb_par;
+static void simplefb_clocks_destroy(struct simplefb_par *par);
+static void simplefb_regulators_destroy(struct simplefb_par *par);
+
static void simplefb_destroy(struct fb_info *info)
{
+ simplefb_regulators_destroy(info->par);
+ simplefb_clocks_destroy(info->par);
if (info->screen_base)
iounmap(info->screen_base);
}
@@ -487,11 +493,8 @@ error_fb_release:
static int simplefb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
- struct simplefb_par *par = info->par;
unregister_framebuffer(info);
- simplefb_regulators_destroy(par);
- simplefb_clocks_destroy(par);
framebuffer_release(info);
return 0;
Re: [PATCH] simplefb: Disable and release clocks and regulators in destroy callback
Hi, On 07-09-16 13:12, Geert Uytterhoeven wrote: On Wed, Sep 7, 2016 at 11:09 AM, Chen-Yu Tsai wrote: simplefb gets unregister when a proper framebuffer driver comes in and kicks it out. However the claimed clocks and regulators stay enabled as they are only released in the platform device remove function, which in theory would never get called. Move the clock/regulator cleanup into the framebuffer destroy callback, which gets called as part of the framebuffer unregister process. Is this called before or after the new proper framebuffer driver kicks in? If before, it may cause glitches. It is called by the new proper framebuffer driver's probe method, so it can make sure that it has already claimed / enabled the clocks/regulators before it calls remove_conlicting_framebuffers, avoiding the glitch. Regards, Hans
Re: [PATCH] phy: sun4i-usb: Use spinlock to guard phyctl register access
Hi,
On 08-09-16 05:14, Chen-Yu Tsai wrote:
The musb driver calls into this phy driver to disable/enable squelch
detection. This function was introduced in 24fe86a617c5 ("phy: sun4i-usb:
Add a sunxi specific function for setting squelch-detect"). This
function in turn calls sun4i_usb_phy_write, which uses a mutex to
guard the common access register. Unfortunately musb does this
in atomic context, which results in the following warning with lock
debugging enabled:
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
in_atomic(): 1, irqs_disabled(): 128, pid: 96, name: kworker/0:2
CPU: 0 PID: 96 Comm: kworker/0:2 Not tainted 4.8.0-rc4-00181-gd502f8ad1c3e #13
Hardware name: Allwinner sun8i Family
Workqueue: events musb_deassert_reset
[] (unwind_backtrace) from [] (show_stack+0xb/0xc)
[] (show_stack) from [] (dump_stack+0x67/0x74)
[] (dump_stack) from [] (mutex_lock+0x15/0x2c)
[] (mutex_lock) from [] (sun4i_usb_phy_write+0x39/0xec)
[] (sun4i_usb_phy_write) from []
(musb_port_reset+0xfb/0x184)
[] (musb_port_reset) from [] (musb_deassert_reset+0x1f/0x2c)
[] (musb_deassert_reset) from []
(process_one_work+0x129/0x2b8)
[] (process_one_work) from [] (worker_thread+0xf3/0x424)
[] (worker_thread) from [] (kthread+0xa1/0xb8)
[] (kthread) from [] (ret_from_fork+0x11/0x20)
Since the register access is mmio, we can use a spinlock to guard this
specific access, rather than the mutex that guards the entire phy.
Fixes: ba4bdc9e1dc0 ("PHY: sunxi: Add driver for sunxi usb phy")
Cc: Hans de Goede
Cc: sta...@vger.kernel.org
Signed-off-by: Chen-Yu Tsai
Good catch, but you're actually replacing the locking calls in the only
user of sun4i_usb_phy_data.mutex, so after your patch it is no longer
used anywhere. Can you do a v2 just outright replacing it with a
spinlock please ?
Thanks,
Hans
---
drivers/phy/phy-sun4i-usb.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index fcf4d95ecc6d..ae4ac5457c64 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include
#include
@@ -115,6 +116,7 @@ struct sun4i_usb_phy_data {
const struct sun4i_usb_phy_cfg *cfg;
enum usb_dr_mode dr_mode;
struct mutex mutex;
+ spinlock_t reg_lock; /* guard access to phyctl reg */
struct sun4i_usb_phy {
struct phy *phy;
void __iomem *pmu;
@@ -183,7 +185,7 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy,
u32 addr, u32 data,
void __iomem *phyctl = phy_data->base + phy_data->cfg->phyctl_offset;
int i;
- mutex_lock(&phy_data->mutex);
+ spin_lock(&phy_data->reg_lock);
if (phy_data->cfg->type == sun8i_a33_phy ||
phy_data->cfg->type == sun50i_a64_phy) {
@@ -221,7 +223,8 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy,
u32 addr, u32 data,
data >>= 1;
}
- mutex_unlock(&phy_data->mutex);
+
+ spin_unlock(&phy_data->reg_lock);
}
static void sun4i_usb_phy_passby(struct sun4i_usb_phy *phy, int enable)
@@ -583,6 +586,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
return -ENOMEM;
mutex_init(&data->mutex);
+ spin_lock_init(&data->reg_lock);
INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
dev_set_drvdata(dev, data);
data->cfg = of_device_get_match_data(dev);
Re: [PATCH] phy: sun4i-usb: Use spinlock to guard phyctl register access
Hi,
On 08-09-16 12:24, Chen-Yu Tsai wrote:
On Thu, Sep 8, 2016 at 6:16 PM, Hans de Goede wrote:
Hi,
On 08-09-16 05:14, Chen-Yu Tsai wrote:
The musb driver calls into this phy driver to disable/enable squelch
detection. This function was introduced in 24fe86a617c5 ("phy: sun4i-usb:
Add a sunxi specific function for setting squelch-detect"). This
function in turn calls sun4i_usb_phy_write, which uses a mutex to
guard the common access register. Unfortunately musb does this
in atomic context, which results in the following warning with lock
debugging enabled:
BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:97
in_atomic(): 1, irqs_disabled(): 128, pid: 96, name: kworker/0:2
CPU: 0 PID: 96 Comm: kworker/0:2 Not tainted 4.8.0-rc4-00181-gd502f8ad1c3e
#13
Hardware name: Allwinner sun8i Family
Workqueue: events musb_deassert_reset
[] (unwind_backtrace) from [] (show_stack+0xb/0xc)
[] (show_stack) from [] (dump_stack+0x67/0x74)
[] (dump_stack) from [] (mutex_lock+0x15/0x2c)
[] (mutex_lock) from []
(sun4i_usb_phy_write+0x39/0xec)
[] (sun4i_usb_phy_write) from []
(musb_port_reset+0xfb/0x184)
[] (musb_port_reset) from []
(musb_deassert_reset+0x1f/0x2c)
[] (musb_deassert_reset) from []
(process_one_work+0x129/0x2b8)
[] (process_one_work) from []
(worker_thread+0xf3/0x424)
[] (worker_thread) from [] (kthread+0xa1/0xb8)
[] (kthread) from [] (ret_from_fork+0x11/0x20)
Since the register access is mmio, we can use a spinlock to guard this
specific access, rather than the mutex that guards the entire phy.
Fixes: ba4bdc9e1dc0 ("PHY: sunxi: Add driver for sunxi usb phy")
Cc: Hans de Goede
Cc: sta...@vger.kernel.org
Signed-off-by: Chen-Yu Tsai
Good catch, but you're actually replacing the locking calls in the only
user of sun4i_usb_phy_data.mutex, so after your patch it is no longer
used anywhere. Can you do a v2 just outright replacing it with a
spinlock please ?
Ah. I misread the other use of mutex_lock, which is actually using
the mutex in struct phy. Sure I'll just replace it.
After sending this patch I wondered if I should have used
spin_lock_irqsave though.
Good point, yes please use spin_lock_irqsave, I believe this
code path may get called outside of irq handling so using
spin_lock_irqsave is the right thing todo.
Regards,
Hans
ChenYu
Thanks,
Hans
---
drivers/phy/phy-sun4i-usb.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index fcf4d95ecc6d..ae4ac5457c64 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include
#include
@@ -115,6 +116,7 @@ struct sun4i_usb_phy_data {
const struct sun4i_usb_phy_cfg *cfg;
enum usb_dr_mode dr_mode;
struct mutex mutex;
+ spinlock_t reg_lock; /* guard access to phyctl reg */
struct sun4i_usb_phy {
struct phy *phy;
void __iomem *pmu;
@@ -183,7 +185,7 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy
*phy, u32 addr, u32 data,
void __iomem *phyctl = phy_data->base +
phy_data->cfg->phyctl_offset;
int i;
- mutex_lock(&phy_data->mutex);
+ spin_lock(&phy_data->reg_lock);
if (phy_data->cfg->type == sun8i_a33_phy ||
phy_data->cfg->type == sun50i_a64_phy) {
@@ -221,7 +223,8 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy
*phy, u32 addr, u32 data,
data >>= 1;
}
- mutex_unlock(&phy_data->mutex);
+
+ spin_unlock(&phy_data->reg_lock);
}
static void sun4i_usb_phy_passby(struct sun4i_usb_phy *phy, int enable)
@@ -583,6 +586,7 @@ static int sun4i_usb_phy_probe(struct platform_device
*pdev)
return -ENOMEM;
mutex_init(&data->mutex);
+ spin_lock_init(&data->reg_lock);
INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
dev_set_drvdata(dev, data);
data->cfg = of_device_get_match_data(dev);
Re: [PATCH] phy-sun4i-usb: select 'USB_COMMON'
Hi, On 09-09-16 11:25, Arnd Bergmann wrote: In commit "usb: phy: add USB_SUPPORT dependency", I tried to fix the dependency for this driver, but unfortunately I missed that we need both USB_COMMON and USB_SUPPORT here, and we can still get the same link error in the much rarer case that USB_COMMON is a loadable module and the phy driver is build-in: drivers/phy/phy-sun4i-usb.o: In function `sun4i_usb_phy_probe': phy-sun4i-usb.c:(.text.sun4i_usb_phy_probe+0x1a6): undefined reference to `of_usb_get_dr_mode_by_phy' This adds the select, hopefully fixing it properly this time. Signed-off-by: Arnd Bergmann Thanks for catching this: Reviewed-by: Hans de Goede --- I see the original patch hasn't made it into linux-next yet, so ideally just fold this patch into the first one. The original patch is in 4.8-rc# (it got there after rc1). Regards, Hans Sorry for not getting it right the first time around. diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index 985dff8558e5..c717f306d131 100644 --- a/drivers/phy/Kconfig +++ b/drivers/phy/Kconfig @@ -262,6 +262,7 @@ config PHY_SUN4I_USB depends on POWER_SUPPLY depends on USB_SUPPORT select GENERIC_PHY + select USB_COMMON help Enable this to support the transceiver that is part of Allwinner sunxi SoCs.
Re: [PATCH v2] phy: sun4i-usb: Use spinlock to guard phyctl register access
Hi,
On 09-09-16 05:58, Chen-Yu Tsai wrote:
The musb driver calls into this phy driver to disable/enable squelch
detection. This function was introduced in 24fe86a617c5 ("phy: sun4i-usb:
Add a sunxi specific function for setting squelch-detect"). This
function in turn calls sun4i_usb_phy_write, which uses a mutex to
guard the common access register. Unfortunately musb does this
in atomic context, which results in the following warning with lock
debugging enabled:
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
in_atomic(): 1, irqs_disabled(): 128, pid: 96, name: kworker/0:2
CPU: 0 PID: 96 Comm: kworker/0:2 Not tainted 4.8.0-rc4-00181-gd502f8ad1c3e #13
Hardware name: Allwinner sun8i Family
Workqueue: events musb_deassert_reset
[] (unwind_backtrace) from [] (show_stack+0xb/0xc)
[] (show_stack) from [] (dump_stack+0x67/0x74)
[] (dump_stack) from [] (mutex_lock+0x15/0x2c)
[] (mutex_lock) from [] (sun4i_usb_phy_write+0x39/0xec)
[] (sun4i_usb_phy_write) from []
(musb_port_reset+0xfb/0x184)
[] (musb_port_reset) from [] (musb_deassert_reset+0x1f/0x2c)
[] (musb_deassert_reset) from []
(process_one_work+0x129/0x2b8)
[] (process_one_work) from [] (worker_thread+0xf3/0x424)
[] (worker_thread) from [] (kthread+0xa1/0xb8)
[] (kthread) from [] (ret_from_fork+0x11/0x20)
Since the register access is mmio, we can use a spinlock to guard this
specific access, rather than the mutex that guards the entire phy.
Fixes: ba4bdc9e1dc0 ("PHY: sunxi: Add driver for sunxi usb phy")
Cc: Hans de Goede
Cc: sta...@vger.kernel.org
Signed-off-by: Chen-Yu Tsai
Looks good to me:
Reviewed-by: Hans de Goede
Regards,
Hans
---
Changes since v1:
- Remove the old mutex, since it is no longer used
- Use irqsave/irqrestore variant of spinlock ops
---
drivers/phy/phy-sun4i-usb.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index fcf4d95ecc6d..34b7f0c2e019 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include
#include
@@ -114,7 +115,7 @@ struct sun4i_usb_phy_data {
void __iomem *base;
const struct sun4i_usb_phy_cfg *cfg;
enum usb_dr_mode dr_mode;
- struct mutex mutex;
+ spinlock_t reg_lock; /* guard access to phyctl reg */
struct sun4i_usb_phy {
struct phy *phy;
void __iomem *pmu;
@@ -181,9 +182,10 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy,
u32 addr, u32 data,
struct sun4i_usb_phy_data *phy_data = to_sun4i_usb_phy_data(phy);
u32 temp, usbc_bit = BIT(phy->index * 2);
void __iomem *phyctl = phy_data->base + phy_data->cfg->phyctl_offset;
+ unsigned long flags;
int i;
- mutex_lock(&phy_data->mutex);
+ spin_lock_irqsave(&phy_data->reg_lock, flags);
if (phy_data->cfg->type == sun8i_a33_phy ||
phy_data->cfg->type == sun50i_a64_phy) {
@@ -221,7 +223,8 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy,
u32 addr, u32 data,
data >>= 1;
}
- mutex_unlock(&phy_data->mutex);
+
+ spin_unlock_irqrestore(&phy_data->reg_lock, flags);
}
static void sun4i_usb_phy_passby(struct sun4i_usb_phy *phy, int enable)
@@ -582,7 +585,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
if (!data)
return -ENOMEM;
- mutex_init(&data->mutex);
+ spin_lock_init(&data->reg_lock);
INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
dev_set_drvdata(dev, data);
data->cfg = of_device_get_match_data(dev);
Re: [linux-sunxi] Re: [PATCH v3] ARM: dts: sun8i: enable UART1 for iNet D978 Rev2 board
Hi,
On 31-08-16 18:22, Maxime Ripard wrote:
On Mon, Aug 29, 2016 at 10:18:32PM +0800, Icenowy Zheng wrote:
UART1 is connected to the bluetooth part of RTL8723BS WiFi/BT combo card
on iNet D978 Rev2 board.
Enable the UART1 to make it possible to use the modified hciattach by
Realtek to drive the BT part of RTL8723BS.
On the board no r_uart pins are found now (the onboard RX/TX pins are
wired to PF2/PF4, which is muxed with mmc0), so also disabled it.
Signed-off-by: Icenowy Zheng
I'll make the same comments than in the v2.
https://www.spinics.net/lists/arm-kernel/msg527001.html
I've a feeling there is a bit of miscommunication here,
let me try to clarify things:
Icenowy, Maxime wants you to split this into 2 patches:
1) Adding just the uart1_pins_a and uart1_pins_cts_rts_a nodes to
sun8i-a23-a33.dtsi; and
2) Another patch with the sun8i-a33-inet-d978-rev2.dts changes
And for 2. Maxime wants you to change:
aliases {
/delete-property/serial0;
serial1 = &uart1;
};
to:
aliases {
serial0 = &uart1;
};
There is no serial0 and Maxime wants the serial-s
to be numbered starting at 0 (iow there is no reason
to make the bluetooth uart serial1).
Regards,
Hans
Re: [PATCH 0/2] drm: add SimpleDRM driver
Hi, On 04-08-16 20:12, Luc Verhaegen wrote: On Thu, Aug 04, 2016 at 06:58:55PM +0200, Noralf Trønnes wrote: I didn't read the binding document[1], which I should have done. If simpledrm claims to be compatible with simple-framebuffer I assume it should support the entire binding doc which includes clocks, regulators and having the node under /chosen. I will lift the necessary code from simplefb.c and put it in the next version. Smashing, repeat of a massive pain avoided, thanks :) The binding doc also mentions an optional display phandle property, but I can't find any reference to this in simplefb.c. Ah yes, the display phandle, so the idea behind this is that the simplefb node would have a display phandle pointing to a node describing the "primary" node describing the actual display-pipe hardware. The primary language is there because a display pipeline typically consists of multiple blocks and thus has multiple nodes describing it. This way the hardware driver would be able to figure out which simplefb to disable if there is more then 1. In practice the remove_conflicting_framebuffers kernel API is used for this and that takes a framebuffer address, so that bit of the bindings is essentially unused. Either way that bit is only relevant to the actual display hardware driver (so that it can disable sumplefb when it takes over the display) and for simpledrm you can simply ignore it. Regards, Hans p.s. Noralf, I recognize your name from the ft6236 touchscreen driver, I've mailed you about this in the past because it is a duplicate driver, the edt-ft5x06 driver already speaks the same protocol. I see now that I made a copy and paste error in your email address, so you never got my mails on this. I'll resend my latest mail (a kernel patch removing the duplicate driver!) with a fixed email address.
Re: [PATCH 2/2] ARM: dts: sun8i: enable UART1 for iNet D978 Rev2 board
Hi,
On 25-08-16 16:12, Icenowy Zheng wrote:
25.08.2016, 16:03, "Maxime Ripard" :
Hi,
On Thu, Aug 25, 2016 at 02:57:24PM +0800, Icenowy Zheng wrote:
diff --git a/arch/arm/boot/dts/sun8i-a33-inet-d978-rev2.dts
b/arch/arm/boot/dts/sun8i-a33-inet-d978-rev2.dts
index 78823d8..3ac22d4 100644
--- a/arch/arm/boot/dts/sun8i-a33-inet-d978-rev2.dts
+++ b/arch/arm/boot/dts/sun8i-a33-inet-d978-rev2.dts
@@ -48,6 +48,10 @@
model = "INet-D978 Rev 02";
compatible = "primux,inet-d978-rev2", "allwinner,sun8i-a33";
+ aliases {
+ serial1 = &uart1;
+ };
+
>>>
>>> Is there any other UART in the system?
>>
>> serial0 is defined in sun8i-reference-design-tablet.dtsi, as r_uart.
>
> If your board does not have r_uart pads, then the right thing to do
> would be to disable it. You can then have uart1 as serial0.
I don't know it. The RX/TX is wired on PF2/PF4 on the board (UART0 muxed with
MMC)
Then you don't support it and you need to disable it.
But many codes will require a ttyS0 as console... So I think the
bluetooth should be located at ttyS1...
Most of them would use /dev/console anyway. What do you have in mind?
As a general configuration, /dev/ttyS0 will be indicated as the default console=
value... (Otherwise more boards will fail)
Or maybe we can specify uart0 as serial0, and leave it as disabled? (Thus if the
debug port is needed, we can easily apply a overlay with &mmc0 status as
disabled,
and &uart0 status as okay)
On all my A33 boards, the official boot0, u-boot all uses uart0 as debugging
port.
If no uart is available (and being muxed with the mmc counts as not being
available)
then you should not have any serial alias in the dts.
In u-boot you need to use:
# CONFIG_REQUIRE_SERIAL_CONSOLE is not set
In the defconfig in this case, otherwise u-boot is going to be unhappy about
not having a serial console.
Regards,
Hans
Re: [PATCH 2/2] ARM: dts: sun8i: enable UART1 for iNet D978 Rev2 board
HI,
On 25-08-16 16:53, Icenowy Zheng wrote:
25.08.2016, 22:18, "Hans de Goede" :
Hi,
On 25-08-16 16:12, Icenowy Zheng wrote:
25.08.2016, 16:03, "Maxime Ripard" :
Hi,
On Thu, Aug 25, 2016 at 02:57:24PM +0800, Icenowy Zheng wrote:
>>>> diff --git a/arch/arm/boot/dts/sun8i-a33-inet-d978-rev2.dts
b/arch/arm/boot/dts/sun8i-a33-inet-d978-rev2.dts
>>>> index 78823d8..3ac22d4 100644
>>>> --- a/arch/arm/boot/dts/sun8i-a33-inet-d978-rev2.dts
>>>> +++ b/arch/arm/boot/dts/sun8i-a33-inet-d978-rev2.dts
>>>> @@ -48,6 +48,10 @@
>>>> model = "INet-D978 Rev 02";
>>>> compatible = "primux,inet-d978-rev2", "allwinner,sun8i-a33";
>>>>
>>>> + aliases {
>>>> + serial1 = &uart1;
>>>> + };
>>>> +
>>>
>>> Is there any other UART in the system?
>>
>> serial0 is defined in sun8i-reference-design-tablet.dtsi, as r_uart.
>
> If your board does not have r_uart pads, then the right thing to do
> would be to disable it. You can then have uart1 as serial0.
I don't know it. The RX/TX is wired on PF2/PF4 on the board (UART0 muxed with
MMC)
Then you don't support it and you need to disable it.
But many codes will require a ttyS0 as console... So I think the
bluetooth should be located at ttyS1...
Most of them would use /dev/console anyway. What do you have in mind?
As a general configuration, /dev/ttyS0 will be indicated as the default
console=
value... (Otherwise more boards will fail)
Or maybe we can specify uart0 as serial0, and leave it as disabled? (Thus if
the
debug port is needed, we can easily apply a overlay with &mmc0 status as
disabled,
and &uart0 status as okay)
On all my A33 boards, the official boot0, u-boot all uses uart0 as debugging
port.
If no uart is available (and being muxed with the mmc counts as not being
available)
then you should not have any serial alias in the dts.
In u-boot you need to use:
# CONFIG_REQUIRE_SERIAL_CONSOLE is not set
In the defconfig in this case, otherwise u-boot is going to be unhappy about
not having a serial console.
It's not the problem.
Can I have no ttyS0 and just make bluetooth ttyS1?
Ah, yes having a serial1 alias should do that I believe and I agree that it
is probably better to not use ttyS0 for the bluetooth uart.
Regards,
Hans
Re: [PATCH v2 08/14] power: supply: Add power_supply_set_input_current_limit_from_supplier helper
Hi, On 16-08-17 17:54, Tony Lindgren wrote: * Hans de Goede [170815 13:06]: On some devices the USB Type-C port power (USB PD 2.0) negotiation is done by a separate port-controller IC, while the current limit is controlled through another (charger) IC. It has been decided to model this by modelling the external Type-C power brick (adapter/charger) as a power-supply class device which supplies the charger-IC, with its voltage-now and current-max representing the negotiated voltage and max current draw. This commit adds a power_supply_set_input_current_limit_from_supplier helper function which charger power-supply drivers can call to get the max-current from their supplier and have this applied through their set_property call-back to their input-current-limit. Hmm so can this also be used for the USB gadget subsystem to tell charge controller when it's OK to enable 500mA charging after enumeration? I'm not sure that that would be best modeled this way. Perhaps the phy-driver can directly control the gpio you have for that, that seems better then trying to solve this with cross subsystem calls which are always tricky. FYI, that's controlled by the bq24190 pin named OTG that should be only set high after enumeration. Any ideas how that is wired on your device? Does it connect to the USB PHY or to a GPIO line? I believe it is just hardwired to be logical high all the time on my board. Regards, Hans
Re: [PATCH v5 0/3] i2c: Hookup typec power-negotation to the PMIC and charger
Hi, On 14-10-17 12:45, Andy Shevchenko wrote: On Wed, Oct 11, 2017 at 12:41 PM, Hans de Goede wrote: Hi All, These are the last 3 patches from all my work to add support for USB PD charging to Intel Cherry Trail devices with a Whiskey Cove PMIC and fusb302 Type-C controller. With these 3 patches these devices change from slowly discharging at 5V 0.5A to actually charging at 12V 2A, so getting these upstream is important for users of these devices. Wolfram does not really have time to review the first 2 patches, so if someone else (from the people in the Cc) can review the first 2 patches and esp. the first patch, so that we can get this merged for 4.15, then that would be great. The person comes to my mind is Heikki, Though I dunno he has time either. The first patch, although it is an i2c-core change, is really quite simple. I believe anyone can review it. But it does need someone other then me to take a proper look (and not just do a code only review) because it is a core change. Maybe there is another way to solve the problem (persistent / predictable device-names for i2c devices instantiated through board-data), but I don't think so. Also note this is all kernel internal stuff, so we can always change it later. Although it would be better to get it right in one go. Regards, Hans
[PATCH] platform/x86: intel_cht_int33fe: Set supplied-from property on max17047 dev
Devices with the intel_cht_int33fe ACPI device use a max17047 fuel-gauge
combined with a bq24272i charger, in order for the fuel-gauge driver to
correctly display charging / discharging status it needs to know which
charger is supplying the battery.
This commit sets the supplied-from device property to the name of the
bq24272i charger for this.
Signed-off-by: Hans de Goede
---
drivers/platform/x86/intel_cht_int33fe.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/platform/x86/intel_cht_int33fe.c
b/drivers/platform/x86/intel_cht_int33fe.c
index 6a1b2ca5b6fe..da706e2c4232 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -34,6 +34,13 @@ struct cht_int33fe_data {
struct i2c_client *pi3usb30532;
};
+static const char * const max17047_suppliers[] = { "bq24190-charger" };
+
+static const struct property_entry max17047_props[] = {
+ PROPERTY_ENTRY_STRING_ARRAY("supplied-from", max17047_suppliers),
+ { }
+};
+
static int cht_int33fe_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
@@ -70,6 +77,7 @@ static int cht_int33fe_probe(struct i2c_client *client)
memset(&board_info, 0, sizeof(board_info));
strlcpy(board_info.type, "max17047", I2C_NAME_SIZE);
+ board_info.properties = max17047_props;
data->max17047 = i2c_acpi_new_device(dev, 1, &board_info);
if (!data->max17047)
--
2.13.0
Re: [PATCH v4 1/2] mfd: intel_soc_pmic: Select designware i2c-bus driver
Hi, On 22-05-17 12:57, Lee Jones wrote: On Mon, 15 May 2017, Hans de Goede wrote: The Crystal Cove PMIC provides an ACPI OPRegion handler, which must be available before other drivers using it are loaded, which is why INTEL_SOC_PMIC is a bool. Just having the driver is not enough, the driver for the i2c-bus must also be built in, to ensure this, this patch adds a select for it. This fixes errors like these during boot: mmc0: SDHCI controller on ACPI [80860F14:00] using ADMA ACPI Error: No handler for Region [REGS] (93543b0cc3a8) [UserDefinedRegion] (20170119/evregion-166) ACPI Error: Region UserDefinedRegion (ID=143) has no handler (20170119/exfldio-299) ACPI Error: Method parse/execution failed [\_SB.PCI0.I2C7.PMI5.GET] (Node 93543b0cde10), AE_NOT_EXIST (20170119/psparse-543) ACPI Error: Method parse/execution failed [\_SB.PCI0.SHC1._PS0] (Node 93543b0b5cd0), AE_NOT_EXIST (20170119/psparse-543) acpi 80860F14:02: Failed to change power state to D0 While at it this patch also changes the human readable name of the Kconfig option to make clear the INTEL_SOC_PMIC option selects support for the Intel Crystal Cove PMIC and documents why this is a bool. Cc: Andy Shevchenko Signed-off-by: Hans de Goede Would be good to get an Ack by the users/maintainers of this driver. --- Note this patch will partially conflicts with (contains the same changes as) a patch in Andy Shevchenko's tree. Has this been sorted now? Yes, sorry I should have dropped this bit. --- Changes in v2: -Fix Kconfig depends and selects to fix warning reported by kbuild test robot -Improve commit msg (add example of ACPI errors this avoids) Changes in v3: -No changes Changes in v4: -Add explanation why this is a bool and why it selects i2c-designwaree to the help text rather then as comments in the Kconfig --- drivers/mfd/Kconfig | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 1b6a83a4a114..61ebda81e134 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -448,17 +448,24 @@ config LPC_SCH config INTEL_SOC_PMIC bool "Support for Crystal Cove PMIC" - depends on GPIOLIB - depends on I2C=y + depends on HAS_IOMEM + select GPIOLIB + select I2C select MFD_CORE select REGMAP_I2C select REGMAP_IRQ + select COMMON_CLK + select I2C_DESIGNWARE_PLATFORM help Select this option to enable support for Crystal Cove PMIC on some Intel SoC systems. The PMIC provides ADC, GPIO, thermal, charger and related power management functions on these systems. + This option is a bool as it provides an ACPI Opregion which must be Nit: s/Opregion/OpRegion/ Fixed for v5 (which I will send in a minute). Regards, Hans
Re: [PATCH v4 2/2] mfd: axp20x-i2c: Document that this must be builtin on x86
Hi, On 22-05-17 12:56, Lee Jones wrote: On Mon, 15 May 2017, Hans de Goede wrote: On x86 the axp288 PMIC provides an ACPI OpRegion handler, which must be available before other drivers using it are loaded, which can only be ensured if the mfd, OpRegionr and i2c-bus drivers are built in. Since the axp20x mfd code is used on non X86 too we cannot simply change this into a bool, I've tried some Kconfig magic with if x86 but I could not get this working correctly, so this commit just documents that this should be builtin on x86, which fixes errors like these during boot: mmc0: SDHCI controller on ACPI [80860F14:00] using ADMA ACPI Error: No handler for Region [REGS] (93543b0cc3a8) [UserDefinedRegion] ACPI Error: Region UserDefinedRegion (ID=143) has no handler (20170119/exfldio-2 ACPI Error: Method parse/execution failed [\_SB.PCI0.I2C7.PMI5.GET] (Node 93 ACPI Error: Method parse/execution failed [\_SB.PCI0.SHC1._PS0] (Node 93543b acpi 80860F14:02: Failed to change power state to D0 Signed-off-by: Hans de Goede Would be good to get an Ack by the users/maintainers of this driver. If you look at recent commits to drivers for the mfd-subdevices for the AXP288 (which is the only AXP PMIC found on x86 systems) you will see that I've been the only one touch them / caring about them and in many cases fixing them from being non-functional to actually working. I do have quite a few people who are using the AXP288 code on Bay and Cherry Trail devices with this PMIC and who have send me thank you's and test success reports. So I guess I'm the closest thing to a MAINTAINER for the axp288 bits of this driver. --- Changes in v2: -Fix Kconfig depends and selects to fix warning reported by kbuild test robot -Improve commit msg (add example of ACPI errors this avoids) Changes in v3: -Since the axp20x mfd code is used on ARM too where it does not necessarily need to be builtin settle for simply documenting the need to have this builtin on x86 --- drivers/mfd/Kconfig | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 61ebda81e134..cfac1e8a0a20 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -160,6 +160,11 @@ config MFD_AXP20X_I2C components like regulators or the PEK (Power Enable Key) under the corresponding menus. + Note on x86 this provides an ACPI OpRegion, so this must be 'y' + (builtin) and not a module, as the OpRegion must be available as + soon as possible. For the same reason the i2c bus driver options Nit: s/i2c/I2C/ Fixed for v5 (which I will send in a minute). Regards, Hans
[PATCH v5 2/2] mfd: axp20x-i2c: Document that this must be builtin on x86
On x86 the axp288 PMIC provides an ACPI OpRegion handler, which must be available before other drivers using it are loaded, which can only be ensured if the mfd, OpRegionr and i2c-bus drivers are built in. Since the axp20x mfd code is used on non X86 too we cannot simply change this into a bool, I've tried some Kconfig magic with if x86 but I could not get this working correctly, so this commit just documents that this should be builtin on x86, which fixes errors like these during boot: mmc0: SDHCI controller on ACPI [80860F14:00] using ADMA ACPI Error: No handler for Region [REGS] (93543b0cc3a8) [UserDefinedRegion] ACPI Error: Region UserDefinedRegion (ID=143) has no handler (20170119/exfldio-2 ACPI Error: Method parse/execution failed [\_SB.PCI0.I2C7.PMI5.GET] (Node 93 ACPI Error: Method parse/execution failed [\_SB.PCI0.SHC1._PS0] (Node 93543b acpi 80860F14:02: Failed to change power state to D0 Signed-off-by: Hans de Goede --- Changes in v2: -Fix Kconfig depends and selects to fix warning reported by kbuild test robot -Improve commit msg (add example of ACPI errors this avoids) Changes in v3: -Since the axp20x mfd code is used on ARM too where it does not necessarily need to be builtin settle for simply documenting the need to have this builtin on x86 Changes in v5: -Fix I2C spelling --- drivers/mfd/Kconfig | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 4f71cab2e6e8..e03853affcc5 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -160,6 +160,11 @@ config MFD_AXP20X_I2C components like regulators or the PEK (Power Enable Key) under the corresponding menus. + Note on x86 this provides an ACPI OpRegion, so this must be 'y' + (builtin) and not a module, as the OpRegion must be available as + soon as possible. For the same reason the I2C bus driver options + I2C_DESIGNWARE_PLATFORM and I2C_DESIGNWARE_BAYTRAIL must be 'y' too. + config MFD_AXP20X_RSB tristate "X-Powers AXP series PMICs with RSB" select MFD_AXP20X -- 2.13.0
[PATCH v5 1/2] mfd: intel_soc_pmic: Select designware i2c-bus driver
The Crystal Cove PMIC provides an ACPI OPRegion handler, which must be available before other drivers using it are loaded, which is why INTEL_SOC_PMIC is a bool. Just having the driver is not enough, the driver for the i2c-bus must also be built in, to ensure this, this patch adds a select for it. This fixes errors like these during boot: mmc0: SDHCI controller on ACPI [80860F14:00] using ADMA ACPI Error: No handler for Region [REGS] (93543b0cc3a8) [UserDefinedRegion] (20170119/evregion-166) ACPI Error: Region UserDefinedRegion (ID=143) has no handler (20170119/exfldio-299) ACPI Error: Method parse/execution failed [\_SB.PCI0.I2C7.PMI5.GET] (Node 93543b0cde10), AE_NOT_EXIST (20170119/psparse-543) ACPI Error: Method parse/execution failed [\_SB.PCI0.SHC1._PS0] (Node 93543b0b5cd0), AE_NOT_EXIST (20170119/psparse-543) acpi 80860F14:02: Failed to change power state to D0 While at it this patch also changes the human readable name of the Kconfig option to make clear the INTEL_SOC_PMIC option selects support for the Intel Crystal Cove PMIC and documents why this is a bool. Cc: Andy Shevchenko Signed-off-by: Hans de Goede Reviewed-by: Andy Shevchenko --- Changes in v2: -Fix Kconfig depends and selects to fix warning reported by kbuild test robot -Improve commit msg (add example of ACPI errors this avoids) Changes in v3: -No changes Changes in v4: -Add explanation why this is a bool and why it selects i2c-designwaree to the help text rather then as comments in the Kconfig Changes in v5: -Add Andy's Reviewed-by -Fix OpRegion spelling --- drivers/mfd/Kconfig | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 1b6a83a4a114..4f71cab2e6e8 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -448,17 +448,24 @@ config LPC_SCH config INTEL_SOC_PMIC bool "Support for Crystal Cove PMIC" - depends on GPIOLIB - depends on I2C=y + depends on HAS_IOMEM + select GPIOLIB + select I2C select MFD_CORE select REGMAP_I2C select REGMAP_IRQ + select COMMON_CLK + select I2C_DESIGNWARE_PLATFORM help Select this option to enable support for Crystal Cove PMIC on some Intel SoC systems. The PMIC provides ADC, GPIO, thermal, charger and related power management functions on these systems. + This option is a bool as it provides an ACPI OpRegion which must be + available before any devices using it are probed. This option also + causes the designware-i2c driver to be builtin for the same reason. + config INTEL_SOC_PMIC_BXTWC tristate "Support for Intel Broxton Whiskey Cove PMIC" depends on INTEL_PMC_IPC -- 2.13.0
[PATCH v5 3/7] staging: atomisp: Set step to 0 for mt9m114 menu control
menu controls are not allowed to have a step size, set step to 0 to
fix an oops from the WARN_ON in v4l2_ctrl_new_custom() triggering
because of this.
Signed-off-by: Hans de Goede
---
drivers/staging/media/atomisp/i2c/mt9m114.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/atomisp/i2c/mt9m114.c
b/drivers/staging/media/atomisp/i2c/mt9m114.c
index ced175c268d1..3fa915313e53 100644
--- a/drivers/staging/media/atomisp/i2c/mt9m114.c
+++ b/drivers/staging/media/atomisp/i2c/mt9m114.c
@@ -1499,7 +1499,7 @@ static struct v4l2_ctrl_config mt9m114_controls[] = {
.type = V4L2_CTRL_TYPE_MENU,
.min = 0,
.max = 3,
-.step = 1,
+.step = 0,
.def = 1,
.flags = 0,
},
--
2.13.0
[PATCH v5 2/7] staging: atomisp: Do not call dev_warn with a NULL device
Do not call dev_warn with a NULL device, this silence the following 2
warnings:
[ 14.392194] (NULL device *): Failed to find gmin variable gmin_V2P8GPIO
[ 14.392257] (NULL device *): Failed to find gmin variable gmin_V1P8GPIO
We could switch to using pr_warn for dev == NULL instead, but as comments
in the source indicate, the check for these 2 special gmin variables with
a NULL device is a workaround for 2 specific evaluation boards, so
completely silencing the missing warning for these actually is a good
thing.
Signed-off-by: Hans de Goede
---
.../staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c| 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git
a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index 104fea2f8697..3fea81ea5dbd 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -689,7 +689,7 @@ int gmin_get_config_var(struct device *dev, const char
*var, char *out, size_t *
if (ret == 0) {
memcpy(out, ev->var.Data, ev->var.DataSize);
*out_len = ev->var.DataSize;
- } else {
+ } else if (dev) {
dev_warn(dev, "Failed to find gmin variable %s\n", var8);
}
--
2.13.0
[PATCH v5 1/7] staging: atomisp: Fix calling efivar_entry_get() with unaligned arguments
efivar_entry_get has certain alignment requirements and the atomisp
platform code was not honoring these, causing an oops by triggering the
WARN_ON in arch/x86/platform/efi/efi_64.c: virt_to_phys_or_null_size().
This commit fixes this by using the members of the efivar struct embedded
in the efivar_entry struct we kzalloc as arguments to efivar_entry_get(),
which is how all the other callers of efivar_entry_get() do this.
Signed-off-by: Hans de Goede
---
.../atomisp/platform/intel-mid/atomisp_gmin_platform.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git
a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index 5b4506a71126..104fea2f8697 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -623,9 +623,7 @@ int gmin_get_config_var(struct device *dev, const char
*var, char *out, size_t *
char var8[CFG_VAR_NAME_MAX];
efi_char16_t var16[CFG_VAR_NAME_MAX];
struct efivar_entry *ev;
- u32 efiattr_dummy;
int i, j, ret;
- unsigned long efilen;
if (dev && ACPI_COMPANION(dev))
dev = &ACPI_COMPANION(dev)->dev;
@@ -684,15 +682,18 @@ int gmin_get_config_var(struct device *dev, const char
*var, char *out, size_t *
return -ENOMEM;
memcpy(&ev->var.VariableName, var16, sizeof(var16));
ev->var.VendorGuid = GMIN_CFG_VAR_EFI_GUID;
+ ev->var.DataSize = *out_len;
- efilen = *out_len;
- ret = efivar_entry_get(ev, &efiattr_dummy, &efilen, out);
+ ret = efivar_entry_get(ev, &ev->var.Attributes,
+ &ev->var.DataSize, ev->var.Data);
+ if (ret == 0) {
+ memcpy(out, ev->var.Data, ev->var.DataSize);
+ *out_len = ev->var.DataSize;
+ } else {
+ dev_warn(dev, "Failed to find gmin variable %s\n", var8);
+ }
kfree(ev);
- *out_len = efilen;
-
- if (ret)
- dev_warn(dev, "Failed to find gmin variable %s\n", var8);
return ret;
}
--
2.13.0
Re: [PATCH v5 2/7] staging: atomisp: Do not call dev_warn with a NULL device
Hi, On 28-05-17 19:08, Alan Cox wrote: On Sun, 28 May 2017 14:30:35 +0200 Hans de Goede wrote: Do not call dev_warn with a NULL device, this silence the following 2 warnings: [ 14.392194] (NULL device *): Failed to find gmin variable gmin_V2P8GPIO [ 14.392257] (NULL device *): Failed to find gmin variable gmin_V1P8GPIO We could switch to using pr_warn for dev == NULL instead, but as comments in the source indicate, the check for these 2 special gmin variables with a NULL device is a workaround for 2 specific evaluation boards, so completely silencing the missing warning for these actually is a good thing. At which point real missing variables won't get reported so NAK. I think the right fix is to make the offending callers pass subdev->dev The code for the special v1p8 / v2p8 gpios is ugly as sin, it operates on a global v2p8_gpio value rather then storing info in the gmin_subdev struct, as such passing the subdev->dev pointer would be simply wrong. AFAICT the v1p8 / v2p8 gpio code is the only caller passing in a NULL pointer and as said since thisv1p8 / v2p8 gpio code is only for some special evaluation boards, silencing the error when these variables are not present actually is the right thing to do. which if my understanding of the subdevices is correct should pass the right valid device field from the atomisp. Please also cc me if you are proposing patches this driver - and also linux-media. Sorry about that, I messed up my git send-email foo and send this to a wrong set of addresses (and also added v5 in the subject which should not be there) I did send out a fresh-copy with the full 7 patch patch-set directly after CTRL+c-ing this wrong send-email (which only got the first 3 patches send). Regards, Hans
Re: [PATCH v11] drm: Unplug drm device when unregistering it (v8)
Hi,
On 29-05-17 22:25, Chris Wilson wrote:
On Fri, Apr 14, 2017 at 11:15:04AM -0400, Sean Paul wrote:
On Thu, Apr 13, 2017 at 03:32:44PM +0800, Jeffy Chen wrote:
After unbinding drm, the user space may still owns the drm dev fd, and
may still be able to call drm ioctl.
We're using an unplugged state to prevent something like that, so let's
reuse it here.
Also drop drm_unplug_dev, because it would be unused after other changes.
Verified on rk3399 chromebook kevin(with cros 4.4 kernel), no more crashes
when unbinding drm with ui service still running.
v2: Fix some commit messages.
v3: Reuse unplug status.
v4: Add drm_device_set_plug_state helper.
v5: Fix hang when unregistering drm dev with open_count 0.
v6: Move drm_device_set_plug_state into drm_drv.
v7: Add missing drm_dev_unref in udl_drv.
v8: Fix compiler errors after enable udl.
Signed-off-by: Jeffy Chen
---
Hi Jeffy,
Given the trouble we've had with this patch already, coupled with the fact that
unbinding while userspace is active is a contrived/pathological case, I don't
think it's worth picking this patch upstream.
If it's really causing issues downstream, you can add my Reviewed-by for a
CHROMIUM
patch, but I'd rather not carry patches in the CrOS repo if we don't need to.
Would a
Fixes: a39be606f99d ("drm: Do a full device unregister when unplugging")
Reported-by: Marco Diego Aurélio Mesquita
Cc: Hans de Goede
convince us to look into this patch again?
The problem is this patch is wrong, see below.
drivers/gpu/drm/drm_drv.c | 26 ++
drivers/gpu/drm/udl/udl_drv.c | 3 ++-
include/drm/drmP.h| 6 --
include/drm/drm_drv.h | 1 -
4 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index b5c6bb4..e1da4d1 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -355,22 +355,6 @@ void drm_put_dev(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_put_dev);
-void drm_unplug_dev(struct drm_device *dev)
-{
- /* for a USB device */
- drm_dev_unregister(dev);
-
- mutex_lock(&drm_global_mutex);
-
- drm_device_set_unplugged(dev);
-
- if (dev->open_count == 0) {
- drm_put_dev(dev);
- }
- mutex_unlock(&drm_global_mutex);
-}
-EXPORT_SYMBOL(drm_unplug_dev);
-
/*
* DRM internal mount
* We want to be able to allocate our own "struct address_space" to control
@@ -733,6 +717,13 @@ static void remove_compat_control_link(struct drm_device
*dev)
kfree(name);
}
+static inline void drm_device_set_plug_state(struct drm_device *dev,
+bool plugged)
+{
+ smp_wmb();
+ atomic_set(&dev->unplugged, !plugged);
+}
+
/**
* drm_dev_register - Register DRM device
* @dev: Device to register
@@ -787,6 +778,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long
flags)
if (drm_core_check_feature(dev, DRIVER_MODESET))
drm_modeset_register_all(dev);
+ drm_device_set_plug_state(dev, true);
+
ret = 0;
DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
@@ -826,6 +819,7 @@ void drm_dev_unregister(struct drm_device *dev)
drm_lastclose(dev);
dev->registered = false;
+ drm_device_set_plug_state(dev, false);
if (drm_core_check_feature(dev, DRIVER_MODESET))
drm_modeset_unregister_all(dev);
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index cd8b017..fc73e24 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -108,7 +108,8 @@ static void udl_usb_disconnect(struct usb_interface
*interface)
drm_kms_helper_poll_disable(dev);
udl_fbdev_unplug(dev);
udl_drop_usb(dev);
- drm_unplug_dev(dev);
+ drm_dev_unregister(dev);
+ drm_dev_unref(dev);
The unref here will cause the device struct to get free-ed even if
userspace still holds references to it through the drm_dev. To fix
this we would need to call drm_dev_ref on a open from userspace and
drm_dev_unref from drm_release.
Regards,
Hans
}
/*
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3bfafcd..980a204 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -488,12 +488,6 @@ static __inline__ int drm_core_check_feature(struct
drm_device *dev,
return ((dev->driver->driver_features & feature) ? 1 : 0);
}
-static inline void drm_device_set_unplugged(struct drm_device *dev)
-{
- smp_wmb();
- atomic_set(&dev->unplugged, 1);
-}
-
static inline int drm_device_is_unplugged(struct drm_device *dev)
{
int ret = atomic_read(&dev->unplugged);
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 0fefc3f..eb63078 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/
Re: [PATCH] iio/accel/bmc150: Improve unlocking of a mutex in two functions
Hi,
On 25-10-17 16:33, SF Markus Elfring wrote:
From: Markus Elfring
Date: Wed, 25 Oct 2017 16:26:29 +0200
Add a jump target so that a call of the function "mutex_unlock" is mostly
stored at the end of these function implementations.
Replace five calls by goto statements.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/iio/accel/bmc150-accel-core.c | 32 ++--
1 file changed, 14 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/accel/bmc150-accel-core.c
b/drivers/iio/accel/bmc150-accel-core.c
index 870f92ef61c2..f2a85a11a5e4 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -554,18 +554,15 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data
*data,
mutex_lock(&data->mutex);
ret = bmc150_accel_set_power_state(data, true);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
- return ret;
- }
+ if (ret < 0)
+ goto unlock_after_failure;
ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_AXIS_TO_REG(axis),
&raw_val, sizeof(raw_val));
if (ret < 0) {
dev_err(dev, "Error reading axis %d\n", axis);
bmc150_accel_set_power_state(data, false);
- mutex_unlock(&data->mutex);
- return ret;
+ goto unlock_after_failure;
}
*val = sign_extend32(le16_to_cpu(raw_val) >> chan->scan_type.shift,
chan->scan_type.realbits - 1);
@@ -575,6 +572,10 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data
*data,
return ret;
return IIO_VAL_INT;
+
+unlock_after_failure:
+ mutex_unlock(&data->mutex);
+ return ret;
}
static int bmc150_accel_read_raw(struct iio_dev *indio_dev,
IMHO, if you do this, you should rework the function so that there is a single
unlock call
at the end, not a separate one in in error label.
Could e.g. change this:
ret = bmc150_accel_set_power_state(data, false);
mutex_unlock(&data->mutex);
if (ret < 0)
return ret;
return IIO_VAL_INT;
}
To:
ret = bmc150_accel_set_power_state(data, false);
if (ret < 0)
goto unlock;
ret = IIO_VAL_INT;
unlock:
mutex_unlock(&data->mutex);
return ret;
}
And also use the unlock label in the other cases, this is actually
quite a normal pattern. I see little use in a patch like this if there
are still 2 unlock paths after the patch.
Regards,
Hans
@@ -1170,28 +1171,23 @@ static int bmc150_accel_trigger_set_state(struct
iio_trigger *trig,
mutex_lock(&data->mutex);
if (t->enabled == state) {
- mutex_unlock(&data->mutex);
- return 0;
+ ret = 0;
+ goto unlock;
}
if (t->setup) {
ret = t->setup(t, state);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
}
ret = bmc150_accel_set_interrupt(data, t->intr, state);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
t->enabled = state;
-
+unlock:
mutex_unlock(&data->mutex);
-
return ret;
}
Re: [PATCH] iio/accel/bmc150: Improve unlocking of a mutex in two functions
Hi, On 25-10-17 18:15, SF Markus Elfring wrote: IMHO, if you do this, you should rework the function so that there is a single unlock call at the end, not a separate one in in error label. Thanks for your update suggestion. Does it indicate that I may propose similar source code adjustments in this software area? Could e.g. change this: ret = bmc150_accel_set_power_state(data, false); mutex_unlock(&data->mutex); if (ret < 0) return ret; return IIO_VAL_INT; } To: ret = bmc150_accel_set_power_state(data, false); if (ret < 0) goto unlock; ret = IIO_VAL_INT; If that is the only unlock in the function, then it is probably best to keep things as is. In general gotos are considered better then multiple unlocks, but not having either is even better. How do you think about to use the following code variant then? if (!ret) ret = IIO_VAL_INT; I believe the goto unlock variant and setting ret = IIO_VAL_INT; directly above the unlock label variant is better, because that way the error handling is consistent between all steps and if another step is later added at the end, the last step will not require modification. unlock: mutex_unlock(&data->mutex); return ret; } And also use the unlock label in the other cases, this is actually quite a normal pattern. I see little use in a patch like this if there are still 2 unlock paths after the patch. How long should I wait for corresponding feedback before another small source code adjustment will be appropriate? That is hard to say. I usually just do a new version when I've time, seldomly someone complains I should have waited longer for feedback (when I'm quite quick) but usually sending out a new version as soon as you've time to work on a new version is best, since if you wait you may then not have time for the entire next week or so, at least that is my experience :) There is really no clear rule here. Regards, Hans
Re: iio/accel/bmc150: Improve unlocking of a mutex in two functions
Hi,
On 25-10-17 18:58, SF Markus Elfring wrote:
If that is the only unlock in the function, then it is probably
best to keep things as is. In general gotos are considered
better then multiple unlocks, but not having either is
even better.
Thanks for your quick feedback.
How do you think about to use the following code variant then?
if (!ret)
ret = IIO_VAL_INT;
I believe the goto unlock variant and setting ret = IIO_VAL_INT;
directly above the unlock label variant is better,
I would prefer the approach above so that an extra goto statement
could also be avoided before.
Usually code in a function follows a pattern of:
err = step1()
if (err)
handle-err
err = step2()
if (err)
handle-err
err = step3()
if (err)
handle-err
What you are suggesting breaks this pattern (not
using a goto in the last if (err) case) which makes
the code harder to read and makes things harder
(and potentially leads to introducing bugs) when
a step4() gets added.
because that way the error handling is consistent between all steps
and if another step is later added at the end, the last step will
not require modification.
Do any more contributors insist on such a code layout?
There definitely is some personal preference involved here,
but I do believe that consistency is more important then
saving a goto here.
How long should I wait for corresponding feedback before another small
source code adjustment will be appropriate?
That is hard to say.
I am just curious on how we can achieve progress here.
I usually just do a new version when I've time,
This is generally fine.
seldomly someone complains I should have waited longer for feedback
(when I'm quite quick) but usually sending out a new version as soon
as you've time to work on a new version is best, since if you wait
you may then not have time for the entire next week or so, at least
that is my experience :) There is really no clear rule here.
I asked also because other well-known contributors showed strong
reactions for this change pattern (with the help of a script
for the semantic patch language).
Would you care for similar updates in source files like the following?
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id=1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760
So I just checked this one, this one is tricky because the
lock is taking inside a switch-case and doing gotos inside
the case is not pretty. If I were to refactor this, I would
add an "if (mask == IIO_CHAN_INFO_SCALE) {}" block to
handle the unlocked scale case and then take the lock around
the entire switch-case, using breaks on error to jump to
the unlock after the switch-case without needing gotos.
To me this seems the right thing here, since the scale is
special here in that it does not need locking. Or optionally
one can just take the lock for scale regardless, it won't
hurt (much).
Basically I believe there is no one-size fits all solution
here and refactoring like this may introduce bugs, so one
needs to weight the amount of work + regression risk vs
the benefits of the code being cleaner going forward.
Regards,
Hans
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/stk8312.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n432
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/adc/palmas_gpadc.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n304
Regards,
Markus
Re: linux-next: build warning after merge of the bluetooth tree
Hi,
On 14-11-17 01:56, Stephen Rothwell wrote:
Hi all,
After merging the bluetooth tree, today's linux-next build (arm
multi_v7_defconfig) produced this warning:
drivers/bluetooth/Kconfig:35:warning: multi-line strings not supported
Introduced by commit
86be3c232877 ("Bluetooth: btusb: Add a Kconfig option to enable USB autosuspend
by default")
There is a missing close double quote ...
Ugh, sorry about that, a patch (on top of the original) fixing this
is coming up right away.
Regards,
Hans
Re: [PATCH v3 10/14] extcon: intel-int3496: Add support for controlling the USB-role mux
Hi, On 18-10-17 04:33, Chanwoo Choi wrote: Hi Hans, On 2017년 09월 23일 03:37, Hans de Goede wrote: Cherry Trail SoCs have a built-in USB-role mux for switching between the host and device controllers, rather then using an external mux controller by a GPIO. There is a driver using the mux-subsys to control this mux, this commit adds support to the intel-int3496 driver to get a mux_controller handle for the mux and set the mux through the mux-subsys rather then through a GPIO. Signed-off-by: Hans de Goede --- Changes in v2: -Drop || COMPILE_TEST from Kconfig depends on, as we will now fail to compile on !X86 -Minor code style tweaks --- drivers/extcon/Kconfig| 3 +- drivers/extcon/extcon-intel-int3496.c | 59 +++ 2 files changed, 61 insertions(+), 1 deletion(-) Acked-by: Chanwoo Choi Note that there have been some comments on this series, and it is not sure yet how we are going to end up handling this. So please do not merge this yet, as we may end up with another solution. Regards, Hans
[PATCH v3 2/2] platform/x86: silead_dmi: Add silead,home-button property to some tablets
Add "silead,home-button" property to entries for tablets which have
a capacitive home button (typically a windows logo on the front).
This new property is checked for by the new capacitive home button
support in the silead touchscreen driver.
Signed-off-by: Hans de Goede
---
drivers/platform/x86/silead_dmi.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/platform/x86/silead_dmi.c
b/drivers/platform/x86/silead_dmi.c
index fadfce9b3f5f..ac304f2318cc 100644
--- a/drivers/platform/x86/silead_dmi.c
+++ b/drivers/platform/x86/silead_dmi.c
@@ -58,6 +58,7 @@ static const struct property_entry dexp_ursus_7w_props[] = {
PROPERTY_ENTRY_U32("touchscreen-size-y", 630),
PROPERTY_ENTRY_STRING("firmware-name", "gsl1686-dexp-ursus-7w.fw"),
PROPERTY_ENTRY_U32("silead,max-fingers", 10),
+ PROPERTY_ENTRY_BOOL("silead,home-button"),
{ }
};
@@ -72,6 +73,7 @@ static const struct property_entry
surftab_wintron70_st70416_6_props[] = {
PROPERTY_ENTRY_STRING("firmware-name",
"gsl1686-surftab-wintron70-st70416-6.fw"),
PROPERTY_ENTRY_U32("silead,max-fingers", 10),
+ PROPERTY_ENTRY_BOOL("silead,home-button"),
{ }
};
@@ -116,6 +118,7 @@ static const struct property_entry
pov_mobii_wintab_p800w_props[] = {
PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
PROPERTY_ENTRY_STRING("firmware-name",
"gsl3692-pov-mobii-wintab-p800w.fw"),
+ PROPERTY_ENTRY_BOOL("silead,home-button"),
{ }
};
@@ -143,6 +146,7 @@ static const struct property_entry chuwi_hi8_pro_props[] = {
PROPERTY_ENTRY_U32("touchscreen-size-y", 1148),
PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
PROPERTY_ENTRY_STRING("firmware-name", "gsl3680-chuwi-hi8-pro.fw"),
+ PROPERTY_ENTRY_BOOL("silead,home-button"),
{ }
};
--
2.14.2
[PATCH v3 1/2] Input: silead - Add support for capactive home button found on some x86 tablets
On some x86 tablets with a silead touchscreen the windows logo on the
front is a capacitive home button. Touching this button results in a touch
with bits 12-15 of the Y coordinates set, while normally only the lower 12
are used.
Detect this and report a KEY_LEFTMETA press when this happens. Note for
now we only respond to the Y coordinate bits 12-15 containing 0x01, on some
tablets *without* a capacative button I've noticed these bits containing
0x04 when crossing the edges of the screen.
Cc: Rob Herring
Signed-off-by: Hans de Goede
---
Changes in v2:
-Only enable support for the home-button if a "silead,home-button"
boolean device-property is set on the device
Changes in v3:
-Document the new silead,home-button property in:
Documentation/devicetree/bindings/input/touchscreen/goodix.txt
---
.../bindings/input/touchscreen/silead_gsl1680.txt | 2 +
drivers/input/touchscreen/silead.c | 46 --
2 files changed, 37 insertions(+), 11 deletions(-)
diff --git
a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
index 6aa625e0cb8d..84752de12412 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
@@ -23,6 +23,8 @@ Optional properties:
- touchscreen-inverted-y : See touchscreen.txt
- touchscreen-swapped-x-y : See touchscreen.txt
- silead,max-fingers : maximum number of fingers the touchscreen can detect
+- silead,home-button : Boolean, set to true on devices which have a
+ capacitive home-button build into the touchscreen
- vddio-supply : regulator phandle for controller VDDIO
- avdd-supply: regulator phandle for controller AVDD
diff --git a/drivers/input/touchscreen/silead.c
b/drivers/input/touchscreen/silead.c
index 0dbcf105f7db..646b1e768e6b 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c
@@ -56,7 +56,7 @@
#define SILEAD_POINT_Y_MSB_OFF 0x01
#define SILEAD_POINT_X_OFF 0x02
#define SILEAD_POINT_X_MSB_OFF 0x03
-#define SILEAD_TOUCH_ID_MASK 0xF0
+#define SILEAD_EXTRA_DATA_MASK 0xF0
#define SILEAD_CMD_SLEEP_MIN 1
#define SILEAD_CMD_SLEEP_MAX 2
@@ -109,6 +109,9 @@ static int silead_ts_request_input_dev(struct
silead_ts_data *data)
INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED |
INPUT_MT_TRACK);
+ if (device_property_read_bool(dev, "silead,home-button"))
+ input_set_capability(data->input, EV_KEY, KEY_LEFTMETA);
+
data->input->name = SILEAD_TS_NAME;
data->input->phys = "input/ts";
data->input->id.bustype = BUS_I2C;
@@ -139,7 +142,8 @@ static void silead_ts_read_data(struct i2c_client *client)
struct input_dev *input = data->input;
struct device *dev = &client->dev;
u8 *bufp, buf[SILEAD_TS_DATA_LEN];
- int touch_nr, error, i;
+ int touch_nr, softbutton, error, i;
+ bool softbutton_pressed = false;
error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
SILEAD_TS_DATA_LEN, buf);
@@ -148,21 +152,40 @@ static void silead_ts_read_data(struct i2c_client *client)
return;
}
- touch_nr = buf[0];
- if (touch_nr > data->max_fingers) {
+ if (buf[0] > data->max_fingers) {
dev_warn(dev, "More touches reported then supported %d > %d\n",
-touch_nr, data->max_fingers);
- touch_nr = data->max_fingers;
+buf[0], data->max_fingers);
+ buf[0] = data->max_fingers;
}
+ touch_nr = 0;
bufp = buf + SILEAD_POINT_DATA_LEN;
- for (i = 0; i < touch_nr; i++, bufp += SILEAD_POINT_DATA_LEN) {
- /* Bits 4-7 are the touch id */
- data->id[i] = (bufp[SILEAD_POINT_X_MSB_OFF] &
- SILEAD_TOUCH_ID_MASK) >> 4;
- touchscreen_set_mt_pos(&data->pos[i], &data->prop,
+ for (i = 0; i < buf[0]; i++, bufp += SILEAD_POINT_DATA_LEN) {
+ softbutton = (bufp[SILEAD_POINT_Y_MSB_OFF] &
+ SILEAD_EXTRA_DATA_MASK) >> 4;
+
+ if (softbutton) {
+ /*
+* For now only respond to softbutton == 0x01, some
+* tablets *without* a capacative button send 0x04
+* when crossing the edges of the screen.
+*/
+ if (softbutton == 0x01)
+ softbutton_pressed = true;
+
+ continue;
+ }
+
+
[PATCH] platform/x86: silead_dmi: Fix GP-electronic T701 entry
The GP-electronic T701 has its LCD panel mounted upside-down, initially
my plan was to fix this by transparently rotating the image in the i915
driver (my "drm/i915: Deal with upside-down mounted LCD" patch), but
that approach has been rejected instead the kernel will now export
a "panel orientation" property on the drm-connector for the panel and
let userspace deal with it.
But userspace expects the touchscreen coordinates to match the panel
coordinates *before* applying any rotation, so now that we no longer hide
the upside-down-ness of the LCD panel from userspace the coordinates being
generated are wrong and we need to apply a rotation of 180 degrees to the
coordinates to fix this.
Signed-off-by: Hans de Goede
---
drivers/platform/x86/silead_dmi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/platform/x86/silead_dmi.c
b/drivers/platform/x86/silead_dmi.c
index a0f80f971081..ac304f2318cc 100644
--- a/drivers/platform/x86/silead_dmi.c
+++ b/drivers/platform/x86/silead_dmi.c
@@ -85,6 +85,8 @@ static const struct silead_ts_dmi_data
surftab_wintron70_st70416_6_data = {
static const struct property_entry gp_electronic_t701_props[] = {
PROPERTY_ENTRY_U32("touchscreen-size-x", 960),
PROPERTY_ENTRY_U32("touchscreen-size-y", 640),
+ PROPERTY_ENTRY_BOOL("touchscreen-inverted-x"),
+ PROPERTY_ENTRY_BOOL("touchscreen-inverted-y"),
PROPERTY_ENTRY_STRING("firmware-name",
"gsl1680-gp-electronic-t701.fw"),
{ }
--
2.14.2
[PATCH v5 1/2] x86 / i915 iosf_mbi / PMIC bus access fixes
Hi All, Here is a split-up version of my "drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()" patch, this time with the arch/x86/platform/intel/iosf_mbi.c split out into a separate patch. Since the iosf_mbi changes are fairly isolated, ideally this entire series would go upstream through the drm tree. X86 maintainers, can we have an ack for that from one of you please ? Regards, Hans
[PATCH v5 2/2] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()
intel_uncore_forcewake_reset() does forcewake puts and gets as such
we need to make sure that no-one tries to access the PUNIT->PMIC bus
(on systems where this bus is shared) while it runs, otherwise bad
things happen.
Normally this is taken care of by the i915_pmic_bus_access_notifier()
which does an intel_uncore_forcewake_get(FORCEWAKE_ALL) when some other
driver tries to access the PMIC bus, so that later forcewake gets are
no-ops (for the duration of the bus access).
But intel_uncore_forcewake_reset gets called in 3 cases:
1) Before registering the pmic_bus_access_notifier
2) After unregistering the pmic_bus_access_notifier
3) To reset forcewake state on a GPU reset
In all 3 cases the i915_pmic_bus_access_notifier() protection is
insufficient.
This commit fixes this race by calling iosf_mbi_punit_acquire() before
calling intel_uncore_forcewake_reset(). In the case where it is called
directly after unregistering the pmic_bus_access_notifier, we need to
hold the punit-lock over both calls to avoid a race where
intel_uncore_fw_release_timer() may execute between the 2 calls.
Signed-off-by: Hans de Goede
Reviewed-by: Imre Deak
---
Changes in v2:
-Rebase on current (July 6th 2017) drm-next
Changes in v3:
-Keep punit acquired / locked over the unregister + forcewake_reset
call combo to avoid a race hitting between the 2 calls
-This requires modifying iosf_mbi_unregister_pmic_bus_access_notifier
to not take the lock itself, since we are the only users this is done
in this same commit
Changes in v4:
-Fix missing rename in doc-comment
-Add and use iosf_mbi_assert_punit_acquired() helper
-Add missing acquire surrounding intel_uncore_forcewake_reset() inside
intel_uncore_check_forcewake_domains()
-Add Imre's Reviewed-by
Changes in v5:
-Separate out arch/x86 iosf_mbi changes into a separate patch
---
drivers/gpu/drm/i915/intel_uncore.c | 17 +
drivers/gpu/drm/i915/selftests/intel_uncore.c | 3 +++
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c
b/drivers/gpu/drm/i915/intel_uncore.c
index 8c2ce81f01c2..0da81faf3981 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -229,6 +229,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
return HRTIMER_NORESTART;
}
+/* Note callers must have acquired the PUNIT->PMIC bus, before calling this. */
static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
bool restore)
{
@@ -237,6 +238,8 @@ static void intel_uncore_forcewake_reset(struct
drm_i915_private *dev_priv,
int retry_count = 100;
enum forcewake_domains fw, active_domains;
+ iosf_mbi_assert_punit_acquired();
+
/* Hold uncore.lock across reset to prevent any register access
* with forcewake not set correctly. Wait until all pending
* timers are run before holding.
@@ -416,14 +419,18 @@ static void __intel_uncore_early_sanitize(struct
drm_i915_private *dev_priv,
GT_FIFO_CTL_RC6_POLICY_STALL);
}
+ iosf_mbi_punit_acquire();
intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
+ iosf_mbi_punit_release();
}
void intel_uncore_suspend(struct drm_i915_private *dev_priv)
{
- iosf_mbi_unregister_pmic_bus_access_notifier(
+ iosf_mbi_punit_acquire();
+ iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
&dev_priv->uncore.pmic_bus_access_nb);
intel_uncore_forcewake_reset(dev_priv, false);
+ iosf_mbi_punit_release();
}
void intel_uncore_resume_early(struct drm_i915_private *dev_priv)
@@ -1315,12 +1322,14 @@ void intel_uncore_init(struct drm_i915_private
*dev_priv)
void intel_uncore_fini(struct drm_i915_private *dev_priv)
{
- iosf_mbi_unregister_pmic_bus_access_notifier(
- &dev_priv->uncore.pmic_bus_access_nb);
-
/* Paranoia: make sure we have disabled everything before we exit. */
intel_uncore_sanitize(dev_priv);
+
+ iosf_mbi_punit_acquire();
+ iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
+ &dev_priv->uncore.pmic_bus_access_nb);
intel_uncore_forcewake_reset(dev_priv, false);
+ iosf_mbi_punit_release();
}
static const struct reg_whitelist {
diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c
b/drivers/gpu/drm/i915/selftests/intel_uncore.c
index 3cac22eb47ce..733d87fe7737 100644
--- a/drivers/gpu/drm/i915/selftests/intel_uncore.c
+++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c
@@ -148,7 +148,10 @@ static int intel_uncore_check_forcewake_domains(struct
drm_i915_private *dev_pri
for_each_set_bit(offset, valid, FW_RANGE) {
i915_reg_t reg = { offset };
+ iosf_mbi_punit_acquire();
intel_uncore_forcewake_reset(dev_priv, false);
+
[PATCH v5 1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
For race free unregistration drivers may need to acquire PMIC bus access
through iosf_mbi_punit_acquire() and then (un)register the notifier without
dropping the lock.
This commit adds an unlocked variant of
iosf_mbi_unregister_pmic_bus_access_notifier for this use case.
Signed-off-by: Hans de Goede
---
arch/x86/include/asm/iosf_mbi.h| 25 +
arch/x86/platform/intel/iosf_mbi.c | 19 +--
2 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index c313cac36f56..eb959cd77994 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -145,6 +145,18 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct
notifier_block *nb);
*/
int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb);
+/**
+ * iosf_mbi_unregister_pmic_bus_access_notifier_unlocked - Unregister PMIC bus
+ * notifier, unlocked
+ *
+ * Like iosf_mbi_unregister_pmic_bus_access_notifier(), but for use when the
+ * caller has already called iosf_mbi_punit_acquire() itself.
+ *
+ * @nb: notifier_block to unregister
+ */
+int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
+ struct notifier_block *nb);
+
/**
* iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier chain
*
@@ -153,6 +165,11 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct
notifier_block *nb);
*/
int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v);
+/**
+ * iosf_mbi_assert_punit_acquired - Assert that the P-Unit has been acquired.
+ */
+void iosf_mbi_assert_punit_acquired(void);
+
#else /* CONFIG_IOSF_MBI is not enabled */
static inline
bool iosf_mbi_available(void)
@@ -196,12 +213,20 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct
notifier_block *nb)
return 0;
}
+static inline int
+iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(struct notifier_block
*nb)
+{
+ return 0;
+}
+
static inline
int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v)
{
return 0;
}
+static inline void iosf_mbi_assert_punit_acquired(void) {}
+
#endif /* CONFIG_IOSF_MBI */
#endif /* IOSF_MBI_SYMS_H */
diff --git a/arch/x86/platform/intel/iosf_mbi.c
b/arch/x86/platform/intel/iosf_mbi.c
index a952ac199741..6f37a2137a79 100644
--- a/arch/x86/platform/intel/iosf_mbi.c
+++ b/arch/x86/platform/intel/iosf_mbi.c
@@ -218,14 +218,23 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct
notifier_block *nb)
}
EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier);
+int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
+ struct notifier_block *nb)
+{
+ iosf_mbi_assert_punit_acquired();
+
+ return blocking_notifier_chain_unregister(
+ &iosf_mbi_pmic_bus_access_notifier, nb);
+}
+EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier_unlocked);
+
int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
{
int ret;
/* Wait for the bus to go inactive before unregistering */
mutex_lock(&iosf_mbi_punit_mutex);
- ret = blocking_notifier_chain_unregister(
- &iosf_mbi_pmic_bus_access_notifier, nb);
+ ret = iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(nb);
mutex_unlock(&iosf_mbi_punit_mutex);
return ret;
@@ -239,6 +248,12 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned
long val, void *v)
}
EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain);
+void iosf_mbi_assert_punit_acquired(void)
+{
+ WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex));
+}
+EXPORT_SYMBOL(iosf_mbi_assert_punit_acquired);
+
#ifdef CONFIG_IOSF_MBI_DEBUG
static u32 dbg_mdr;
static u32 dbg_mcr;
--
2.14.2
Re: [PATCH v5 0/3] i2c: Hookup typec power-negotation to the PMIC and charger
Hi, On 19-10-17 16:42, Wolfram Sang wrote: The first patch, although it is an i2c-core change, is really quite simple. I believe anyone can review it. But it does need someone other then me to take a proper look (and not just do a code only review) because it is a core change. Maybe there is another way to solve the problem (persistent / predictable device-names for i2c devices instantiated through board-data), but I don't think so. My plan is to hopefully meet Mark Brown (SPI and regulator maintainer) at ELCE next week and have a chat with him about this series. If all goes well, I'll merge it for v4.15. Great, thank you for looking into this. Regards, Hans
Re: [PATCH resend v5 3/3] platform/x86: intel_cht_int33fe: Update fusb302 type string, add properties
Hi, On 26-10-17 22:33, Wolfram Sang wrote: On Wed, Oct 11, 2017 at 11:41:21AM +0200, Hans de Goede wrote: The fusb302 driver as merged in staging uses "typec_fusb302" as i2c-id rather then just "fusb302" and needs us to set a number of device- properties, adjust the intel_cht_int33fe driver accordingly. One of the properties set is max-snk-mv which makes the fusb302 driver negotiate up to 12V charging voltage, which is a bad idea on boards which are not setup to handle this, so this commit also adds 2 extra sanity checks to make sure that the expected Whiskey Cove PMIC + TI bq24292i charger combo, which can handle 12V, is present. Signed-off-by: Hans de Goede Acked-by: Andy Shevchenko I can't apply this one. Is there an immutable branch I need to pick up? Or shall this go via another tree? My base is v4.14-rc5. It should be applied on top of this patch: http://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git/commitdiff/5c003458db40cf3c89aeddd22c6e934c28b5a565 From linux-platform-drivers-x86.git/for-next. So either we are going to need an immutable branch from you with the first patch of this series so that the platform/x86 maintainers can merge this, or the other way around :| Regards, Hans
[PATCH] platform/x86: silead_dmi: Add entry for the Digma e200 tablet
From: Sergey Tshovrebov
Add touchscreen platform data for the Digma e200 tablet.
Signed-off-by: Sergey Tshovrebov
Signed-off-by: Hans de Goede
---
drivers/platform/x86/silead_dmi.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/drivers/platform/x86/silead_dmi.c
b/drivers/platform/x86/silead_dmi.c
index ac304f2318cc..266535c2a72f 100644
--- a/drivers/platform/x86/silead_dmi.c
+++ b/drivers/platform/x86/silead_dmi.c
@@ -155,6 +155,22 @@ static const struct silead_ts_dmi_data chuwi_hi8_pro_data
= {
.properties = chuwi_hi8_pro_props,
};
+static const struct property_entry digma_citi_e200_props[] = {
+ PROPERTY_ENTRY_U32("touchscreen-size-x", 1980),
+ PROPERTY_ENTRY_U32("touchscreen-size-y", 1500),
+ PROPERTY_ENTRY_BOOL("touchscreen-inverted-y"),
+ PROPERTY_ENTRY_STRING("firmware-name",
+ "gsl1686-digma_citi_e200.fw"),
+ PROPERTY_ENTRY_U32("silead,max-fingers", 10),
+ PROPERTY_ENTRY_BOOL("silead,home-button"),
+ { }
+};
+
+static const struct silead_ts_dmi_data digma_citi_e200_data = {
+ .acpi_name = "MSSL1680:00",
+ .properties = digma_citi_e200_props,
+};
+
static const struct dmi_system_id silead_ts_dmi_table[] = {
{
/* CUBE iwork8 Air */
@@ -246,6 +262,15 @@ static const struct dmi_system_id silead_ts_dmi_table[] = {
DMI_MATCH(DMI_PRODUCT_NAME, "X1D3_C806N"),
},
},
+ {
+ /* Digma Citi E200 */
+ .driver_data = (void *)&digma_citi_e200_data,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Digma"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "CITI E200"),
+ DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
+ },
+ },
{ },
};
--
2.14.2
