* Re: [PATCH -next] backlight: ipaq_micro: Fix sparse non static symbol warning
From: Bryan Wu @ 2014-07-29 23:25 UTC (permalink / raw)
To: linux-fbdev
On Tue, Jul 29, 2014 at 4:20 PM, <weiyj_lk@163.com> wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>
> Fixes the following sparse warnings:
>
> drivers/video/backlight/ipaq_micro_bl.c:73:24: warning:
> symbol 'micro_backlight_device_driver' was not declared. Should it be static?
>
I've merged a similar fix for led-ipaq-micro. So this one looks good to me:
Acked-by: Bryan Wu <cooloney@gmail.com>
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> ---
> drivers/video/backlight/ipaq_micro_bl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/backlight/ipaq_micro_bl.c b/drivers/video/backlight/ipaq_micro_bl.c
> index feab29c..347dc11 100644
> --- a/drivers/video/backlight/ipaq_micro_bl.c
> +++ b/drivers/video/backlight/ipaq_micro_bl.c
> @@ -70,7 +70,7 @@ static int micro_backlight_probe(struct platform_device *pdev)
> return 0;
> }
>
> -struct platform_driver micro_backlight_device_driver = {
> +static struct platform_driver micro_backlight_device_driver = {
> .driver = {
> .name = "ipaq-micro-backlight",
> },
>
^ permalink raw reply
* Re: [PATCH] video: fbdev: intelfb: intelfb_i2c.c: Cleaning up variable that is never used
From: Pavel Machek @ 2014-07-30 6:50 UTC (permalink / raw)
To: Rickard Strandqvist
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Rickard Strandqvist, Jingoo Han, Laurent Pinchart, Rob Clark,
Daniel Vetter, linux-fbdev, linux-kernel
In-Reply-To: <1404563048-19690-1-git-send-email-rickard_strandqvist@spectrumdigital.se>
On Sat 2014-07-05 14:24:08, Rickard Strandqvist wrote:
> From: Rickard Strandqvist <rickard.strandqvist@sonymobile.com>
>
> Variable ar assigned a value that is never used.
> I have also removed all the code that thereby serves no purpose.
>
> This was found using a static code analysis program called cppcheck
Are you sure this is the right fix?
Do we need to read the register in case we don't care about return
value?
> index 3300bd3..4df2f1f 100644
> --- a/drivers/video/fbdev/intelfb/intelfb_i2c.c
> +++ b/drivers/video/fbdev/intelfb/intelfb_i2c.c
> @@ -55,22 +55,20 @@ static void intelfb_gpio_setscl(void *data, int state)
> {
> struct intelfb_i2c_chan *chan = data;
> struct intelfb_info *dinfo = chan->dinfo;
> - u32 val;
>
> OUTREG(chan->reg, (state ? SCL_VAL_OUT : 0) |
> SCL_DIR | SCL_DIR_MASK | SCL_VAL_MASK);
> - val = INREG(chan->reg);
> + INREG(chan->reg);
> }
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH -next] backlight: ipaq_micro: Fix sparse non static symbol warning
From: Lee Jones @ 2014-07-30 7:42 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <CAK5ve-K6WpvN++xdKsHw=-BipV-mYh1PP63G78K=ExynBD9VGQ@mail.gmail.com>
On Wed, 30 Jul 2014, weiyj_lk@163.com wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>
> Fixes the following sparse warnings:
>
> drivers/video/backlight/ipaq_micro_bl.c:73:24: warning:
> symbol 'micro_backlight_device_driver' was not declared. Should it be static?
>
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> ---
> drivers/video/backlight/ipaq_micro_bl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied with Bryan's Ack.
> diff --git a/drivers/video/backlight/ipaq_micro_bl.c b/drivers/video/backlight/ipaq_micro_bl.c
> index feab29c..347dc11 100644
> --- a/drivers/video/backlight/ipaq_micro_bl.c
> +++ b/drivers/video/backlight/ipaq_micro_bl.c
> @@ -70,7 +70,7 @@ static int micro_backlight_probe(struct platform_device *pdev)
> return 0;
> }
>
> -struct platform_driver micro_backlight_device_driver = {
> +static struct platform_driver micro_backlight_device_driver = {
> .driver = {
> .name = "ipaq-micro-backlight",
> },
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] drivers: video: fbdev: atmel_lcdfb.c: Add ability to inverted backlight PWM.
From: Tomi Valkeinen @ 2014-07-30 11:14 UTC (permalink / raw)
To: Michael Welling, Nicolas Ferre, Jean-Christophe Plagniol-Villard,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1404440796-11090-1-git-send-email-mwelling-EkmVulN54Sk@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 518 bytes --]
On 04/07/14 05:26, Michael Welling wrote:
> The code has a variable to change the polarity of the PWM backlight control but
> it was not being initialized. This patch adds a devicetree entry to set the
> variable if required.
>
> Signed-off-by: Michael Welling <mwelling@ieee.org>
> ---
> .../devicetree/bindings/video/atmel,lcdc.txt | 1 +
> drivers/video/fbdev/atmel_lcdfb.c | 3 ++-
> 2 files changed, 3 insertions(+), 1 deletion(-)
Thanks, queued for 3.17.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* fbdev patches for 3.17
From: Tomi Valkeinen @ 2014-07-30 13:12 UTC (permalink / raw)
To: linux-fbdev
[-- Attachment #1: Type: text/plain, Size: 303 bytes --]
Hi,
I just came back from vacation, and 3.17 merge window is getting close.
I have tons of emails in my inbox, so if you have important fbdev
patches for 3.17, please resend or ping.
I'll try to find them myself but I most likely don't have time to go
through all of them for 3.17.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v9] video: ARM CLCD: Add DT support
From: Pawel Moll @ 2014-07-30 13:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53D7825C.2020900@ti.com>
On Tue, 2014-07-29 at 14:15 +0300, Tomi Valkeinen wrote:
> Yep, I've queued it now for 3.17.
Awesome, thanks!
Pawel
^ permalink raw reply
* Re: [PATCH v3] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic
From: Tomi Valkeinen @ 2014-07-30 14:24 UTC (permalink / raw)
To: Dexuan Cui
Cc: linux-fbdev, gregkh, jasowang, driverdev-devel, linux-kernel,
olaf, apw, plagnioj, haiyangz, dan.carpenter
In-Reply-To: <1404875063-27424-1-git-send-email-decui@microsoft.com>
[-- Attachment #1: Type: text/plain, Size: 2157 bytes --]
On 09/07/14 06:04, Dexuan Cui wrote:
> Currently the VSC has no chance to notify the VSP of the dirty rectangle on VM
> panic because the notification work is done in a workqueue, and in panic() the
> kernel typically ends up in an infinite loop, and a typical kernel config has
> CONFIG_PREEMPT_VOLUNTARY=y and CONFIG_PREEMPT is not set, so a context switch
> can't happen in panic() and the workqueue won't have a chance to run. As a
> result, the VM Connection window can't refresh until it's closed and we
> re-connect to the VM.
>
> We can register a handler on panic_notifier_list: the handler can notify
> the VSC and switch the framebuffer driver to a "synchronous mode", meaning
> the VSC flushes any future framebuffer change to the VSP immediately.
>
> v2: removed the MS-TFS line in the commit message
> v3: remove some 'unlikely' markings
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/video/fbdev/hyperv_fb.c | 58 ++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index e23392e..a7b98e1 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -226,11 +226,16 @@ struct hvfb_par {
> u8 recv_buf[MAX_VMBUS_PKT_SIZE];
> };
>
> +static struct fb_info *hvfb_info;
Static variables like these are usually a no-no. This prevents you from
having multiple device instances.
> static uint screen_width = HVFB_WIDTH;
> static uint screen_height = HVFB_HEIGHT;
> static uint screen_depth;
> static uint screen_fb_size;
>
> +/* If true, the VSC notifies the VSP on every framebuffer change */
> +static bool synchronous_fb;
> +
Same comment here.
However, if (and only if) the driver is already designed to work only
with single device instance, then this patch is probably ok. But even
then, I'd prefer this to be handled without static variables so that the
driver could eventually be changed to support multiple device instances.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* RE: [PATCH v3] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic
From: Dexuan Cui @ 2014-07-31 10:11 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-fbdev@vger.kernel.org, gregkh@linuxfoundation.org,
jasowang@redhat.com, driverdev-devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org, olaf@aepfle.de, apw@canonical.com,
plagnioj@jcrosoft.com, Haiyang Zhang, dan.carpenter@oracle.com
In-Reply-To: <53D90008.4060908@ti.com>
> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkeinen@ti.com]
> Sent: Wednesday, July 30, 2014 22:24 PM
> > +static struct fb_info *hvfb_info;
>
> Static variables like these are usually a no-no. This prevents you from
> having multiple device instances.
I agree.
> > static uint screen_width = HVFB_WIDTH;
> > static uint screen_height = HVFB_HEIGHT;
> > static uint screen_depth;
> > static uint screen_fb_size;
> >
> > +/* If true, the VSC notifies the VSP on every framebuffer change */
> > +static bool synchronous_fb;
> > +
>
> Same comment here.
>
> However, if (and only if) the driver is already designed to work only
> with single device instance, then this patch is probably ok. But even
IMO the host should only provide at most 1 synthetic video device to a VM. :-)
> then, I'd prefer this to be handled without static variables so that the
> driver could eventually be changed to support multiple device instances.
Hi Tomi,
Maybe we can remove these static stuff:
+static struct fb_info *hvfb_info;
+static struct notifier_block hvfb_panic_nb = {
+ .notifier_call = hvfb_on_panic,
+};
by kmalloc()-ing a new struct:
struct hv_fb_panic_nb {
struct fb_info *hvfb_info;
struct notifier_block nb;
}
?
I think in hvfb_on_panic() we should be able to get the
hvfb_info pointer by
hvfb_info = container_of(nb, struct hv_fb_panic_nb, nb).
If you like that or have a better idea, please let me know so
I can make a new patch.
Thanks,
-- Dexuan
^ permalink raw reply
* [RFC] pwm-backlight: Allow backlight to remain disabled on boot
From: Thierry Reding @ 2014-07-31 11:42 UTC (permalink / raw)
To: linux-arm-kernel
From: Thierry Reding <treding@nvidia.com>
The default for backlight devices is to be enabled immediately when
registering with the backlight core. This can be useful for setups that
use a simple framebuffer device and where the backlight cannot otherwise
be hooked up to the panel.
However, when dealing with more complex setups, such as those of recent
ARM SoCs, this can be problematic. Since the backlight is usually setup
separately from the display controller, the probe order is not usually
deterministic. That can lead to situations where the backlight will be
powered up and the panel will show an uninitialized framebuffer.
Furthermore, subsystems such as DRM have advanced functionality to set
the power mode of a panel. In order to allow such setups to power up the
panel at exactly the right moment, a way is needed to prevent the
backlight core from powering the backlight up automatically when it is
registered.
This commit introduces a new boot_off field in the platform data (and
also implements getting the same information from device tree). When set
the initial backlight power mode will be set to "off".
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
I've been meaning to send this for a while but was always holding back
because of the indoctrination that this type of configuration shouldn't
be part of device tree. However this issue was recently raised again in
the context of power up sequences for display panels. As described above
the issue is that panel datasheets recommend that the backlight attached
to a panel be turned on at the very last step to avoid visual glitches
during the panel's power up sequence. With the current implementation it
is typical for the backlight to be probed before the display panel. That
has, in many cases, the side-effect of enabling the backlight, therefore
making the screen content visible before it's actually initialized.
Some panels come up with random garbage when uninitialized, others show
all white. With some luck the panel will be all black and users won't
really notice.
This patch is an attempt to enable boards to override the default of
turning on the backlight for the pwm-backlight driver. I'm not sure if
there was a specific reason to turn on the backlight by default when
this driver was initially written, but the fact is that since it has
pretty much always been like this we can't really go and change the
default, otherwise a lot of people may end up with no backlight and no
clue as to how to enable it. So the only reasonable thing we can do is
to keep the old behaviour and give new boards a way to override it if
they know that some other part of the stack will enable it at the right
moment.
.../devicetree/bindings/video/backlight/pwm-backlight.txt | 1 +
drivers/video/backlight/pwm_bl.c | 8 ++++++++
include/linux/pwm_backlight.h | 2 ++
3 files changed, 11 insertions(+)
diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 764db86d441a..65e001a1733d 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -17,6 +17,7 @@ Optional properties:
"pwms" property (see PWM binding[0])
- enable-gpios: contains a single GPIO specifier for the GPIO which enables
and disables the backlight (see GPIO binding[1])
+ - backlight-boot-off: keep the backlight disabled on boot
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
[1]: Documentation/devicetree/bindings/gpio/gpio.txt
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index d7a3d13e72ec..62adfc9d37a7 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -173,6 +173,8 @@ static int pwm_backlight_parse_dt(struct device *dev,
data->max_brightness--;
}
+ data->boot_off = of_property_read_bool(node, "backlight-boot-off");
+
return 0;
}
@@ -317,6 +319,12 @@ static int pwm_backlight_probe(struct platform_device *pdev)
}
bl->props.brightness = data->dft_brightness;
+
+ if (data->boot_off)
+ bl->props.power = FB_BLANK_POWERDOWN;
+ else
+ bl->props.power = FB_BLANK_UNBLANK;
+
backlight_update_status(bl);
platform_set_drvdata(pdev, bl);
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index efdd9227a49c..1fc14989da4a 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -15,6 +15,8 @@ struct platform_pwm_backlight_data {
unsigned int *levels;
/* TODO remove once all users are switched to gpiod_* API */
int enable_gpio;
+ bool boot_off;
+
int (*init)(struct device *dev);
int (*notify)(struct device *dev, int brightness);
void (*notify_after)(struct device *dev, int brightness);
--
2.0.3
^ permalink raw reply related
* Re: [RFC] pwm-backlight: Allow backlight to remain disabled on boot
From: Thierry Reding @ 2014-07-31 11:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1406806970-12561-1-git-send-email-thierry.reding@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]
On Thu, Jul 31, 2014 at 01:42:50PM +0200, Thierry Reding wrote:
[...]
> @@ -317,6 +319,12 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> }
>
> bl->props.brightness = data->dft_brightness;
> +
> + if (data->boot_off)
> + bl->props.power = FB_BLANK_POWERDOWN;
> + else
> + bl->props.power = FB_BLANK_UNBLANK;
> +
> backlight_update_status(bl);
Looking at this again, perhaps a more sensible thing to do would be to
not call backlight_update_status() in the first place. For example if
the board defines that backlight should be kept off at boot, but the
bootloader had already enabled it, then this would effectively turn off
the backlight again.
I think it's safe to assume that if the bootloader sets up the backlight
then it would also set up the display. Therefore not touching the
backlight state at all at probe time seems like the safest default.
Of course that doesn't help people who use some dumb framebuffer driver
and therefore nothing explicitly enables the backlight. So it would
still be changing behaviour for people for whom the bootloader doesn't
set up the backlight at all and who therefore rely on the kernel to turn
it on.
We could perhaps alleviate that pain a little by making this dependent
on whether or not the board is booted using DT on the assumption that
anything that uses DT would be "modern" enough to provide a means to
automatically enable the backlight at the right moment.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC] pwm-backlight: Allow backlight to remain disabled on boot
From: Thierry Reding @ 2014-07-31 11:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1406806970-12561-1-git-send-email-thierry.reding@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5464 bytes --]
Cc'ing Ajay (who raised this on a different thread recently), therefore
quoting all of the original email.
Thierry
On Thu, Jul 31, 2014 at 01:42:50PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> The default for backlight devices is to be enabled immediately when
> registering with the backlight core. This can be useful for setups that
> use a simple framebuffer device and where the backlight cannot otherwise
> be hooked up to the panel.
>
> However, when dealing with more complex setups, such as those of recent
> ARM SoCs, this can be problematic. Since the backlight is usually setup
> separately from the display controller, the probe order is not usually
> deterministic. That can lead to situations where the backlight will be
> powered up and the panel will show an uninitialized framebuffer.
>
> Furthermore, subsystems such as DRM have advanced functionality to set
> the power mode of a panel. In order to allow such setups to power up the
> panel at exactly the right moment, a way is needed to prevent the
> backlight core from powering the backlight up automatically when it is
> registered.
>
> This commit introduces a new boot_off field in the platform data (and
> also implements getting the same information from device tree). When set
> the initial backlight power mode will be set to "off".
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> I've been meaning to send this for a while but was always holding back
> because of the indoctrination that this type of configuration shouldn't
> be part of device tree. However this issue was recently raised again in
> the context of power up sequences for display panels. As described above
> the issue is that panel datasheets recommend that the backlight attached
> to a panel be turned on at the very last step to avoid visual glitches
> during the panel's power up sequence. With the current implementation it
> is typical for the backlight to be probed before the display panel. That
> has, in many cases, the side-effect of enabling the backlight, therefore
> making the screen content visible before it's actually initialized.
>
> Some panels come up with random garbage when uninitialized, others show
> all white. With some luck the panel will be all black and users won't
> really notice.
>
> This patch is an attempt to enable boards to override the default of
> turning on the backlight for the pwm-backlight driver. I'm not sure if
> there was a specific reason to turn on the backlight by default when
> this driver was initially written, but the fact is that since it has
> pretty much always been like this we can't really go and change the
> default, otherwise a lot of people may end up with no backlight and no
> clue as to how to enable it. So the only reasonable thing we can do is
> to keep the old behaviour and give new boards a way to override it if
> they know that some other part of the stack will enable it at the right
> moment.
>
> .../devicetree/bindings/video/backlight/pwm-backlight.txt | 1 +
> drivers/video/backlight/pwm_bl.c | 8 ++++++++
> include/linux/pwm_backlight.h | 2 ++
> 3 files changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> index 764db86d441a..65e001a1733d 100644
> --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> @@ -17,6 +17,7 @@ Optional properties:
> "pwms" property (see PWM binding[0])
> - enable-gpios: contains a single GPIO specifier for the GPIO which enables
> and disables the backlight (see GPIO binding[1])
> + - backlight-boot-off: keep the backlight disabled on boot
>
> [0]: Documentation/devicetree/bindings/pwm/pwm.txt
> [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index d7a3d13e72ec..62adfc9d37a7 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -173,6 +173,8 @@ static int pwm_backlight_parse_dt(struct device *dev,
> data->max_brightness--;
> }
>
> + data->boot_off = of_property_read_bool(node, "backlight-boot-off");
> +
> return 0;
> }
>
> @@ -317,6 +319,12 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> }
>
> bl->props.brightness = data->dft_brightness;
> +
> + if (data->boot_off)
> + bl->props.power = FB_BLANK_POWERDOWN;
> + else
> + bl->props.power = FB_BLANK_UNBLANK;
> +
> backlight_update_status(bl);
>
> platform_set_drvdata(pdev, bl);
> diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
> index efdd9227a49c..1fc14989da4a 100644
> --- a/include/linux/pwm_backlight.h
> +++ b/include/linux/pwm_backlight.h
> @@ -15,6 +15,8 @@ struct platform_pwm_backlight_data {
> unsigned int *levels;
> /* TODO remove once all users are switched to gpiod_* API */
> int enable_gpio;
> + bool boot_off;
> +
> int (*init)(struct device *dev);
> int (*notify)(struct device *dev, int brightness);
> void (*notify_after)(struct device *dev, int brightness);
> --
> 2.0.3
>
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic
From: Tomi Valkeinen @ 2014-07-31 13:38 UTC (permalink / raw)
To: Dexuan Cui
Cc: linux-fbdev@vger.kernel.org, gregkh@linuxfoundation.org,
jasowang@redhat.com, driverdev-devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org, olaf@aepfle.de, apw@canonical.com,
plagnioj@jcrosoft.com, Haiyang Zhang, dan.carpenter@oracle.com
In-Reply-To: <EE124450C0AAF944A40DD71E61F878C98F6F78@SINEX14MBXC417.southpacific.corp.microsoft.com>
[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]
On 31/07/14 13:11, Dexuan Cui wrote:
>> -----Original Message-----
>> From: Tomi Valkeinen [mailto:tomi.valkeinen@ti.com]
>> Sent: Wednesday, July 30, 2014 22:24 PM
>>> +static struct fb_info *hvfb_info;
>>
>> Static variables like these are usually a no-no. This prevents you from
>> having multiple device instances.
> I agree.
>
>>> static uint screen_width = HVFB_WIDTH;
>>> static uint screen_height = HVFB_HEIGHT;
>>> static uint screen_depth;
>>> static uint screen_fb_size;
>>>
>>> +/* If true, the VSC notifies the VSP on every framebuffer change */
>>> +static bool synchronous_fb;
>>> +
>>
>> Same comment here.
>>
>> However, if (and only if) the driver is already designed to work only
>> with single device instance, then this patch is probably ok. But even
> IMO the host should only provide at most 1 synthetic video device to a VM. :-)
>
>> then, I'd prefer this to be handled without static variables so that the
>> driver could eventually be changed to support multiple device instances.
>
> Hi Tomi,
> Maybe we can remove these static stuff:
> +static struct fb_info *hvfb_info;
> +static struct notifier_block hvfb_panic_nb = {
> + .notifier_call = hvfb_on_panic,
> +};
> by kmalloc()-ing a new struct:
> struct hv_fb_panic_nb {
> struct fb_info *hvfb_info;
> struct notifier_block nb;
> }
> ?
> I think in hvfb_on_panic() we should be able to get the
> hvfb_info pointer by
> hvfb_info = container_of(nb, struct hv_fb_panic_nb, nb).
>
> If you like that or have a better idea, please let me know so
> I can make a new patch.
To be honest, I haven't been using notifiers much. But the above looks
ok to me.
Or maybe you can add the notifier_block and the synchronous_fb to
hvfb_par? From the notifier_block pointer you could then get hvfp_par,
and from there hvfb_info.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* RE: [PATCH v3] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic
From: Dexuan Cui @ 2014-08-01 10:22 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-fbdev@vger.kernel.org, gregkh@linuxfoundation.org,
jasowang@redhat.com, driverdev-devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org, olaf@aepfle.de, apw@canonical.com,
plagnioj@jcrosoft.com, Haiyang Zhang, dan.carpenter@oracle.com
In-Reply-To: <53DA46C3.9080905@ti.com>
> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkeinen@ti.com]
> Sent: Thursday, July 31, 2014 21:38 PM
> > I think in hvfb_on_panic() we should be able to get the
> > hvfb_info pointer by
> > hvfb_info = container_of(nb, struct hv_fb_panic_nb, nb).
> >
> > If you like that or have a better idea, please let me know so
> > I can make a new patch.
>
> Or maybe you can add the notifier_block and the synchronous_fb to
> hvfb_par? From the notifier_block pointer you could then get hvfp_par,
> and from there hvfb_info.
> Tomi
Hi Tomi,
Thanks a lot for the suggestion! It's better.
I'll send out a v4 patch for your review.
Thanks,
-- Dexuan
^ permalink raw reply
* [PATCH v4] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic
From: Dexuan Cui @ 2014-08-01 10:26 UTC (permalink / raw)
To: gregkh, tomi.valkeinen, plagnioj, dan.carpenter, linux-kernel,
driverdev-devel, linux-fbdev
Cc: apw, olaf, haiyangz, jasowang
Currently the VSC has no chance to notify the VSP of the dirty rectangle on VM
panic because the notification work is done in a workqueue, and in panic() the
kernel typically ends up in an infinite loop, and a typical kernel config has
CONFIG_PREEMPT_VOLUNTARY=y and CONFIG_PREEMPT is not set, so a context switch
can't happen in panic() and the workqueue won't have a chance to run. As a
result, the VM Connection window can't refresh until it's closed and we
re-connect to the VM.
We can register a handler on panic_notifier_list: the handler can notify
the VSC and switch the framebuffer driver to a "synchronous mode", meaning
the VSC flushes any future framebuffer change to the VSP immediately.
v2: removed the MS-TFS line in the commit message
v3: remove some 'unlikely' markings
v4: avoid global variables as Tomi Valkeinen suggested
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
drivers/video/fbdev/hyperv_fb.c | 62 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 59 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index e23392e..569e756 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -224,6 +224,11 @@ struct hvfb_par {
u32 pseudo_palette[16];
u8 init_buf[MAX_VMBUS_PKT_SIZE];
u8 recv_buf[MAX_VMBUS_PKT_SIZE];
+
+ /* If true, the VSC notifies the VSP on every framebuffer change */
+ bool synchronous_fb;
+
+ struct notifier_block hvfb_panic_nb;
};
static uint screen_width = HVFB_WIDTH;
@@ -532,6 +537,19 @@ static void hvfb_update_work(struct work_struct *w)
schedule_delayed_work(&par->dwork, HVFB_UPDATE_DELAY);
}
+static int hvfb_on_panic(struct notifier_block *nb,
+ unsigned long e, void *p)
+{
+ struct hvfb_par *par;
+ struct fb_info *info;
+
+ par = container_of(nb, struct hvfb_par, hvfb_panic_nb);
+ par->synchronous_fb = true;
+ info = par->info;
+ synthvid_update(info);
+
+ return NOTIFY_DONE;
+}
/* Framebuffer operation handlers */
@@ -582,14 +600,44 @@ static int hvfb_blank(int blank, struct fb_info *info)
return 1; /* get fb_blank to set the colormap to all black */
}
+static void hvfb_cfb_fillrect(struct fb_info *p,
+ const struct fb_fillrect *rect)
+{
+ struct hvfb_par *par = p->par;
+
+ cfb_fillrect(p, rect);
+ if (par->synchronous_fb)
+ synthvid_update(p);
+}
+
+static void hvfb_cfb_copyarea(struct fb_info *p,
+ const struct fb_copyarea *area)
+{
+ struct hvfb_par *par = p->par;
+
+ cfb_copyarea(p, area);
+ if (par->synchronous_fb)
+ synthvid_update(p);
+}
+
+static void hvfb_cfb_imageblit(struct fb_info *p,
+ const struct fb_image *image)
+{
+ struct hvfb_par *par = p->par;
+
+ cfb_imageblit(p, image);
+ if (par->synchronous_fb)
+ synthvid_update(p);
+}
+
static struct fb_ops hvfb_ops = {
.owner = THIS_MODULE,
.fb_check_var = hvfb_check_var,
.fb_set_par = hvfb_set_par,
.fb_setcolreg = hvfb_setcolreg,
- .fb_fillrect = cfb_fillrect,
- .fb_copyarea = cfb_copyarea,
- .fb_imageblit = cfb_imageblit,
+ .fb_fillrect = hvfb_cfb_fillrect,
+ .fb_copyarea = hvfb_cfb_copyarea,
+ .fb_imageblit = hvfb_cfb_imageblit,
.fb_blank = hvfb_blank,
};
@@ -801,6 +849,11 @@ static int hvfb_probe(struct hv_device *hdev,
par->fb_ready = true;
+ par->synchronous_fb = false;
+ par->hvfb_panic_nb.notifier_call = hvfb_on_panic;
+ atomic_notifier_chain_register(&panic_notifier_list,
+ &par->hvfb_panic_nb);
+
return 0;
error:
@@ -820,6 +873,9 @@ static int hvfb_remove(struct hv_device *hdev)
struct fb_info *info = hv_get_drvdata(hdev);
struct hvfb_par *par = info->par;
+ atomic_notifier_chain_unregister(&panic_notifier_list,
+ &par->hvfb_panic_nb);
+
par->update = false;
par->fb_ready = false;
--
1.9.1
^ permalink raw reply related
* -- Re: Very Urgent............
From: Fabian Morision @ 2014-08-01 10:28 UTC (permalink / raw)
To: linux-fbdev
.
Greetings from gulf region
Thanks for the e-mail. I am very interested on funding lucrative
business partnership with you acting as the manager and sole
controller of the investment while i remain a silent investor for a
period of ten yrs , though I am only looking at investment
opportunities within the range you specified for a start. You can
reply me here (fmorision@yahoo.com)
Let me know your thought asap
Regards
Financial Consultant
Mr.Fabian Morision
^ permalink raw reply
* [PATCH] video: ARM CLCD: Fix DT-related build problems
From: Pawel Moll @ 2014-08-01 14:43 UTC (permalink / raw)
To: linux-fbdev
This patch fixes the following error when !CONFIG_OF:
drivers/video/fbdev/amba-clcd.c:800:54: warning: ‘struct amba_dev’ declared inside parameter list [enabled by default]
static struct clcd_board *clcdfb_of_get_board(struct amba_dev *dev)
^
and adds a missing Kconfig select causing this
when CONFIG_OF && !CONFIG_FB_MODE_HELPERS:
drivers/video/fbdev/amba-clcd.c:567: undefined reference to `fb_videomode_from_videomode'
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
Hi Tomi,
This fixes two small problems detected in next by the kbuild test robot
(thanks, Fengguang!) Hope you can apply it before the merge window.
Cheers!
Pawel
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/amba-clcd.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index a518fe5..7eed249 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -280,6 +280,7 @@ config FB_ARMCLCD
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
+ select FB_MODE_HELPERS if OF
select VIDEOMODE_HELPERS if OF
help
This framebuffer device driver is for the ARM PrimeCell PL110
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 23b3519..beadd3e 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -797,7 +797,7 @@ static struct clcd_board *clcdfb_of_get_board(struct amba_device *dev)
return board;
}
#else
-static struct clcd_board *clcdfb_of_get_board(struct amba_dev *dev)
+static struct clcd_board *clcdfb_of_get_board(struct amba_device *dev)
{
return NULL;
}
--
1.9.1
^ permalink raw reply related
* [PATCH] video: amba-clcd: Select FB_MODE_HELPERS
From: Mark Brown @ 2014-08-01 15:53 UTC (permalink / raw)
To: linux-fbdev
From: Mark Brown <broonie@linaro.org>
The AMBA CLCD driver uses fb_videomode_from_videomode() so it needs to
select DB_MODE_HELPERS as well as VIDEOMODE_HELPERS.
Signed-off-by: Mark Brown <broonie@linaro.org>
---
drivers/video/fbdev/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 37d473f..e222048 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -280,6 +280,7 @@ config FB_ARMCLCD
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
+ select FB_MODE_HELPERS
select VIDEOMODE_HELPERS if OF
help
This framebuffer device driver is for the ARM PrimeCell PL110
--
2.0.0
^ permalink raw reply related
* Re: [PATCH] video: amba-clcd: Select FB_MODE_HELPERS
From: Russell King - ARM Linux @ 2014-08-01 18:27 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1406908430-18487-1-git-send-email-broonie@kernel.org>
On Fri, Aug 01, 2014 at 04:53:50PM +0100, Mark Brown wrote:
> From: Mark Brown <broonie@linaro.org>
>
> The AMBA CLCD driver uses fb_videomode_from_videomode() so it needs to
> select DB_MODE_HELPERS as well as VIDEOMODE_HELPERS.
It doesn't, not as I designed it at least. It depends what other people
have done to my driver...
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH] video: amba-clcd: Select FB_MODE_HELPERS
From: Mark Brown @ 2014-08-01 18:48 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1406908430-18487-1-git-send-email-broonie@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 575 bytes --]
On Fri, Aug 01, 2014 at 07:27:03PM +0100, Russell King - ARM Linux wrote:
> On Fri, Aug 01, 2014 at 04:53:50PM +0100, Mark Brown wrote:
> > The AMBA CLCD driver uses fb_videomode_from_videomode() so it needs to
> > select DB_MODE_HELPERS as well as VIDEOMODE_HELPERS.
> It doesn't, not as I designed it at least. It depends what other people
> have done to my driver...
Ah, sorry - I didn't notice that this is as a result of an out of tree
patch I'm carrying adding DT support (which I'd forgotten all about as
I've needed it for so long). Please disregard this patch.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Mailbox Warning
From: Webmail Admin @ 2014-08-02 3:12 UTC (permalink / raw)
To: Recipients
In-Reply-To: <20140729121801.E5D1B4D1577@zirius.ipresspublicas.gob.pe>
Email Account Warning !!!
This mail is from Administrator; we wish to bring to your notice the Condition of your email account.
We have just noticed that you have exceeded your email Database limit of 500 MB quota and your email IP is causing conflict because it is been accessed in different server location. You need to Upgrade and expand your email quota limit before you can continue to use your email. Provide details or click the link below :
Update your email quota limit to 2.6 GB, use the below web link:
http://workouttoday.webs.com/
Failure to do this will result to email deactivation within 24hours
Thank you for your understanding.
Copyright 2014 Help Desk
Email Upgrade
^ permalink raw reply
* Question on s3fb DDC support
From: Jean Delvare @ 2014-08-03 12:42 UTC (permalink / raw)
To: linux-fbdev
Hi Onderj,
Are you still involved in the s3fb driver? So you still have the
hardware to test it?
I am looking at the DDC / I2C implementation in that driver and I'm not
sure I understand how it works. Specifically I am wondering about bit
DDC_DRIVE_EN. It is set unconditionally in s3fb_ddc_setscl() and
s3fb_ddc_setsda() and never cleared explicitly. If this bit is a
regular bit then I don't understand why it is not just set at driver
initialization time. Or is this bit self-clearing and/or not-sticking?
The reason why I am asking is that I don't think the code in these
functions is completely correct. For the I2C protocol, 1 is the natural
state of both lines and the devices on the bus should only ever pull
the lines low to force a 0 state. They should never force a 1 state, as
this breaks some protocol features (specifically arbitration and clock
stretching.) My interpretation of the current code is that the lines
are forced to both 0 and 1 by the master, which is not correct. But as
I don't know how DDC_DRIVE_EN works, I'm not sure, and if it's indeed
broken [1], I'm also not sure how to fix it.
So if you could clarify how bit DDC_DRIVE_EN works, that would be great.
[1] A broken implementation would work fine in most cases and fail only
seldom and randomly, so it could easily go unnoticed.
Thanks,
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply
* Re: Question on s3fb DDC support
From: Geert Uytterhoeven @ 2014-08-03 14:47 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20140803144236.0494b47a@endymion.delvare>
On Sun, Aug 3, 2014 at 2:42 PM, Jean Delvare <jdelvare@suse.de> wrote:
> I am looking at the DDC / I2C implementation in that driver and I'm not
> sure I understand how it works. Specifically I am wondering about bit
> DDC_DRIVE_EN. It is set unconditionally in s3fb_ddc_setscl() and
> s3fb_ddc_setsda() and never cleared explicitly. If this bit is a
> regular bit then I don't understand why it is not just set at driver
> initialization time. Or is this bit self-clearing and/or not-sticking?
>
> The reason why I am asking is that I don't think the code in these
> functions is completely correct. For the I2C protocol, 1 is the natural
> state of both lines and the devices on the bus should only ever pull
> the lines low to force a 0 state. They should never force a 1 state, as
> this breaks some protocol features (specifically arbitration and clock
> stretching.) My interpretation of the current code is that the lines
> are forced to both 0 and 1 by the master, which is not correct. But as
> I don't know how DDC_DRIVE_EN works, I'm not sure, and if it's indeed
> broken [1], I'm also not sure how to fix it.
>
> So if you could clarify how bit DDC_DRIVE_EN works, that would be great.
http://www.vgamuseum.info/images/stories/doc/s3/virge.pdf, p. 24-11:
Bit 0 SCW - Serial Clock Write
0 = Pin 205 is driven low
1 = Pin 205 is tri-stated
Pin 205 carries the DDC/I 2 C clock, depending on the operational
mode. When pin 205
is tri-stated, other devices may drive this line. The actual state of
the pin is read via
bit 2 of this register.
Bit 1 SDW - Serial Data Write
0 = Pin 206 is driven low
1 = Pin 206 is tri-stated
Pin 206 carries the DDC/I 2 C data, depending on the operational mode.
When pin 206
is tri-stated, other devices may drive this line. The actual state of
the pin is read via
bit 3 of this register.
Bit 2 SCR - Serial Clock Read (Read Only)
0 = Pin 205 is low
1 = Pin 205 is tri-stated (no device is driving this line)
Bit 3 SDR - Serial Data Read (Read Only)
0 = Pin 206 is low
1 = Pin 206 is tri-stated (no device is driving this line)
Bit 4 SPE - Serial Port Enable
0 = Use of bits 1-0 of this register disabled
1 = Use of bits 1-0 of this register enabled
Still doesn't explain well the real meaning of bit 4, but it does confirm that
writing a 1 to bit 0 or bit 1 doesn't drive the line high.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: Question on s3fb DDC support
From: Ondrej Zary @ 2014-08-03 15:15 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20140803144236.0494b47a@endymion.delvare>
On Sunday 03 August 2014 16:47:17 Geert Uytterhoeven wrote:
> On Sun, Aug 3, 2014 at 2:42 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > I am looking at the DDC / I2C implementation in that driver and I'm not
> > sure I understand how it works. Specifically I am wondering about bit
> > DDC_DRIVE_EN. It is set unconditionally in s3fb_ddc_setscl() and
> > s3fb_ddc_setsda() and never cleared explicitly. If this bit is a
> > regular bit then I don't understand why it is not just set at driver
> > initialization time. Or is this bit self-clearing and/or not-sticking?
> >
> > The reason why I am asking is that I don't think the code in these
> > functions is completely correct. For the I2C protocol, 1 is the natural
> > state of both lines and the devices on the bus should only ever pull
> > the lines low to force a 0 state. They should never force a 1 state, as
> > this breaks some protocol features (specifically arbitration and clock
> > stretching.) My interpretation of the current code is that the lines
> > are forced to both 0 and 1 by the master, which is not correct. But as
> > I don't know how DDC_DRIVE_EN works, I'm not sure, and if it's indeed
> > broken [1], I'm also not sure how to fix it.
> >
> > So if you could clarify how bit DDC_DRIVE_EN works, that would be great.
>
> http://www.vgamuseum.info/images/stories/doc/s3/virge.pdf, p. 24-11:
>
> Bit 0 SCW - Serial Clock Write
> 0 = Pin 205 is driven low
> 1 = Pin 205 is tri-stated
> Pin 205 carries the DDC/I 2 C clock, depending on the operational
> mode. When pin 205
> is tri-stated, other devices may drive this line. The actual state of
> the pin is read via
> bit 2 of this register.
> Bit 1 SDW - Serial Data Write
> 0 = Pin 206 is driven low
> 1 = Pin 206 is tri-stated
> Pin 206 carries the DDC/I 2 C data, depending on the operational mode.
> When pin 206
> is tri-stated, other devices may drive this line. The actual state of
> the pin is read via
> bit 3 of this register.
> Bit 2 SCR - Serial Clock Read (Read Only)
> 0 = Pin 205 is low
> 1 = Pin 205 is tri-stated (no device is driving this line)
> Bit 3 SDR - Serial Data Read (Read Only)
> 0 = Pin 206 is low
> 1 = Pin 206 is tri-stated (no device is driving this line)
> Bit 4 SPE - Serial Port Enable
> 0 = Use of bits 1-0 of this register disabled
> 1 = Use of bits 1-0 of this register enabled
>
> Still doesn't explain well the real meaning of bit 4, but it does confirm
> that writing a 1 to bit 0 or bit 1 doesn't drive the line high.
Thanks for explanation and datasheet. I wrote that code without docs, just
observing what card's BIOS does. "DRIVE_EN" name was just a guess.
The bit is set always because other code (especially X11) can mess with the HW
behind our back.
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that. -- Linus Torvalds
--
Ondrej Zary
^ permalink raw reply
* Re: Question on s3fb DDC support
From: Jean Delvare @ 2014-08-03 16:03 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20140803144236.0494b47a@endymion.delvare>
Hi Geert,
On Sun, 3 Aug 2014 16:47:17 +0200, Geert Uytterhoeven wrote:
> On Sun, Aug 3, 2014 at 2:42 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > I am looking at the DDC / I2C implementation in that driver and I'm not
> > sure I understand how it works. Specifically I am wondering about bit
> > DDC_DRIVE_EN. It is set unconditionally in s3fb_ddc_setscl() and
> > s3fb_ddc_setsda() and never cleared explicitly. If this bit is a
> > regular bit then I don't understand why it is not just set at driver
> > initialization time. Or is this bit self-clearing and/or not-sticking?
> >
> > The reason why I am asking is that I don't think the code in these
> > functions is completely correct. For the I2C protocol, 1 is the natural
> > state of both lines and the devices on the bus should only ever pull
> > the lines low to force a 0 state. They should never force a 1 state, as
> > this breaks some protocol features (specifically arbitration and clock
> > stretching.) My interpretation of the current code is that the lines
> > are forced to both 0 and 1 by the master, which is not correct. But as
> > I don't know how DDC_DRIVE_EN works, I'm not sure, and if it's indeed
> > broken [1], I'm also not sure how to fix it.
> >
> > So if you could clarify how bit DDC_DRIVE_EN works, that would be great.
>
> http://www.vgamuseum.info/images/stories/doc/s3/virge.pdf, p. 24-11:
>
> Bit 0 SCW - Serial Clock Write
> 0 = Pin 205 is driven low
> 1 = Pin 205 is tri-stated
> Pin 205 carries the DDC/I 2 C clock, depending on the operational
> mode. When pin 205
> is tri-stated, other devices may drive this line. The actual state of
> the pin is read via
> bit 2 of this register.
> Bit 1 SDW - Serial Data Write
> 0 = Pin 206 is driven low
> 1 = Pin 206 is tri-stated
> Pin 206 carries the DDC/I 2 C data, depending on the operational mode.
> When pin 206
> is tri-stated, other devices may drive this line. The actual state of
> the pin is read via
> bit 3 of this register.
> Bit 2 SCR - Serial Clock Read (Read Only)
> 0 = Pin 205 is low
> 1 = Pin 205 is tri-stated (no device is driving this line)
> Bit 3 SDR - Serial Data Read (Read Only)
> 0 = Pin 206 is low
> 1 = Pin 206 is tri-stated (no device is driving this line)
> Bit 4 SPE - Serial Port Enable
> 0 = Use of bits 1-0 of this register disabled
> 1 = Use of bits 1-0 of this register enabled
>
> Still doesn't explain well the real meaning of bit 4, but it does confirm that
> writing a 1 to bit 0 or bit 1 doesn't drive the line high.
Very nice, thanks for the documentation. So the driver code looks
correct and I can move on to the remaining two suspect drivers (intelfb
and i740fb.)
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply
* Re: Question on s3fb DDC support
From: Ondrej Zary @ 2014-08-03 16:13 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20140803144236.0494b47a@endymion.delvare>
On Sunday 03 August 2014 18:03:31 Jean Delvare wrote:
> Hi Geert,
>
> On Sun, 3 Aug 2014 16:47:17 +0200, Geert Uytterhoeven wrote:
> > On Sun, Aug 3, 2014 at 2:42 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > > I am looking at the DDC / I2C implementation in that driver and I'm not
> > > sure I understand how it works. Specifically I am wondering about bit
> > > DDC_DRIVE_EN. It is set unconditionally in s3fb_ddc_setscl() and
> > > s3fb_ddc_setsda() and never cleared explicitly. If this bit is a
> > > regular bit then I don't understand why it is not just set at driver
> > > initialization time. Or is this bit self-clearing and/or not-sticking?
> > >
> > > The reason why I am asking is that I don't think the code in these
> > > functions is completely correct. For the I2C protocol, 1 is the natural
> > > state of both lines and the devices on the bus should only ever pull
> > > the lines low to force a 0 state. They should never force a 1 state, as
> > > this breaks some protocol features (specifically arbitration and clock
> > > stretching.) My interpretation of the current code is that the lines
> > > are forced to both 0 and 1 by the master, which is not correct. But as
> > > I don't know how DDC_DRIVE_EN works, I'm not sure, and if it's indeed
> > > broken [1], I'm also not sure how to fix it.
> > >
> > > So if you could clarify how bit DDC_DRIVE_EN works, that would be
> > > great.
> >
> > http://www.vgamuseum.info/images/stories/doc/s3/virge.pdf, p. 24-11:
> >
> > Bit 0 SCW - Serial Clock Write
> > 0 = Pin 205 is driven low
> > 1 = Pin 205 is tri-stated
> > Pin 205 carries the DDC/I 2 C clock, depending on the operational
> > mode. When pin 205
> > is tri-stated, other devices may drive this line. The actual state of
> > the pin is read via
> > bit 2 of this register.
> > Bit 1 SDW - Serial Data Write
> > 0 = Pin 206 is driven low
> > 1 = Pin 206 is tri-stated
> > Pin 206 carries the DDC/I 2 C data, depending on the operational mode.
> > When pin 206
> > is tri-stated, other devices may drive this line. The actual state of
> > the pin is read via
> > bit 3 of this register.
> > Bit 2 SCR - Serial Clock Read (Read Only)
> > 0 = Pin 205 is low
> > 1 = Pin 205 is tri-stated (no device is driving this line)
> > Bit 3 SDR - Serial Data Read (Read Only)
> > 0 = Pin 206 is low
> > 1 = Pin 206 is tri-stated (no device is driving this line)
> > Bit 4 SPE - Serial Port Enable
> > 0 = Use of bits 1-0 of this register disabled
> > 1 = Use of bits 1-0 of this register enabled
> >
> > Still doesn't explain well the real meaning of bit 4, but it does confirm
> > that writing a 1 to bit 0 or bit 1 doesn't drive the line high.
>
> Very nice, thanks for the documentation. So the driver code looks
> correct and I can move on to the remaining two suspect drivers (intelfb
> and i740fb.)
The i740fb code is also my work - and done the same way without any docs.
--
Ondrej Zary
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox