* Re: [PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO
From: Michael Ellerman @ 2014-10-29 3:04 UTC (permalink / raw)
To: Randy Dunlap
Cc: Tomi Valkeinen, Jani Nikula, Jean-Christophe Plagniol-Villard,
linux-fbdev, Jingoo Han, Daniel Drake, Jens Frederich,
David Airlie, Greg Kroah-Hartman, Jon Nettleton, linux-usb,
linux-kernel, dri-devel, platform-driver-x86, Lee Jones,
Laurent Pinchart, Darren Hart, Daniel Vetter, Bryan Wu,
linuxppc-dev
In-Reply-To: <544FFC91.9040104@infradead.org>
On Tue, 2014-10-28 at 13:29 -0700, Randy Dunlap wrote:
> On 10/27/14 06:13, Tomi Valkeinen wrote:
> > I also think the 'depends on BACKLIGHT_CLASS_DEVICE ||
> > BACKLIGHT_CLASS_DEVICE=n' pattern is quite... interesting (i.e. sounds
> > like a hack to me =).
>
> It does exactly what is needed and it is used in many places in kernel
> Kconfig files.
Is there any reason you can't do:
depends on BACKLIGHT_CLASS_DEVICE != m
cheers
^ permalink raw reply
* Re: [PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO
From: Jani Nikula @ 2014-10-29 7:54 UTC (permalink / raw)
To: Michael Ellerman, Randy Dunlap
Cc: linux-fbdev, linux-usb, Daniel Drake, Jens Frederich,
Greg Kroah-Hartman, Jon Nettleton, Jingoo Han, linux-kernel,
dri-devel, platform-driver-x86, Bryan Wu, linuxppc-dev,
Tomi Valkeinen, Laurent Pinchart, Daniel Vetter, Darren Hart,
Lee Jones, Jean-Christophe Plagniol-Villard
In-Reply-To: <1414551853.7417.1.camel@concordia>
On Wed, 29 Oct 2014, Michael Ellerman <mpe@ellerman.id.au> wrote:
> On Tue, 2014-10-28 at 13:29 -0700, Randy Dunlap wrote:
>> On 10/27/14 06:13, Tomi Valkeinen wrote:
>> > I also think the 'depends on BACKLIGHT_CLASS_DEVICE ||
>> > BACKLIGHT_CLASS_DEVICE=n' pattern is quite... interesting (i.e. sounds
>> > like a hack to me =).
>>
>> It does exactly what is needed and it is used in many places in kernel
>> Kconfig files.
>
> Is there any reason you can't do:
>
> depends on BACKLIGHT_CLASS_DEVICE != m
That's not the same thing. The FOO || FOO=n allows for all options, but
forbids it being a module when the option depending on it is
built-in. Obviously something that's built-in can't depend on something
built as a module.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply
* Re: [PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO
From: Michael Ellerman @ 2014-10-29 8:27 UTC (permalink / raw)
To: Jani Nikula
Cc: Randy Dunlap, Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
linux-fbdev, Jingoo Han, Daniel Drake, Jens Frederich,
David Airlie, Greg Kroah-Hartman, Jon Nettleton, linux-usb,
linux-kernel, dri-devel, platform-driver-x86, Lee Jones,
Laurent Pinchart, Darren Hart, Daniel Vetter, Bryan Wu,
linuxppc-dev
In-Reply-To: <87tx2njooz.fsf@intel.com>
On Wed, 2014-10-29 at 09:54 +0200, Jani Nikula wrote:
> On Wed, 29 Oct 2014, Michael Ellerman <mpe@ellerman.id.au> wrote:
> > On Tue, 2014-10-28 at 13:29 -0700, Randy Dunlap wrote:
> >> On 10/27/14 06:13, Tomi Valkeinen wrote:
> >> > I also think the 'depends on BACKLIGHT_CLASS_DEVICE ||
> >> > BACKLIGHT_CLASS_DEVICE=n' pattern is quite... interesting (i.e. sounds
> >> > like a hack to me =).
> >>
> >> It does exactly what is needed and it is used in many places in kernel
> >> Kconfig files.
> >
> > Is there any reason you can't do:
> >
> > depends on BACKLIGHT_CLASS_DEVICE != m
>
> That's not the same thing. The FOO || FOO=n allows for all options, but
> forbids it being a module when the option depending on it is
> built-in.
OK right. Because "BAR depends on FOO" is short for "depends on FOO=y || FOO=m",
but also adds the implicit condition that if FOO=m then BAR must also be m.
Thanks for clueing me in.
cheers
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH v4 0/5] simplefb: add clock handling code
From: Tomi Valkeinen @ 2014-10-29 11:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <544F7E5D.60104@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]
Hi Hans, Rob,
On 28/10/14 13:30, Hans de Goede wrote:
> Hi,
>
> On 10/28/2014 12:11 PM, Rob Herring wrote:
>> Yes, I object to the binding still as it has not changed from what was
>> previously posted.
>
> It would be helpful if you could explain why you object. Last time you
> said: " You are mixing in a hardware description that is simply inaccurate."
>
> I then explained that this is not hardware description, but runtime state
> information, as it tells the kernel which clocks were chosen to drive the
> display (out of typically a list of possible options, depending on which
> output is used, etc.). Just like which memory address the bootloader has
> chosen to scan out the video image from.
>
> Then you got quiet, so sorry, but this time your objection really is too
> late. You cannot simply go quiet halfway through a discussion and then pop
> up again when a new version is posted to say "I object" yet another time,
> you've had your chance to make your arguments last time, and chose to stay
> quiet after I explained in detail that this is not hardware description but
> state information, so now it is simply too late.
>
> These bindings have been discussed at Plumbers with various interested people
> present, and the conclusion was that this really is the best way to handle this,
> so this patch is:
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Mike Turquette <mturquette@linaro.org>
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>
> And David Herrman who is working on simpledrm, which will be merged soon, which
> will also use the simplefb bindings also agrees. So we have the simplefb maintainer,
> simpledrm maintainer, and the clk subsystem maintainer + 2 other maintainers all
> agreeing on a way forward, the time for bikeshedding now really really really is
> over.
>
> Tomi, can you please let us know how you plan to proceed with this ?
I won't merge DT bindings via fbdev tree, if a DT maintainer says no.
I took Rob's silence to the earlier series as a silent ack for your
explanation. Obviously that was not the case.
Rob, please advice asap what should be done to the bindings to get your
ack. As Hans explained above, this discussion has been going on for a
long time, and afaik this series is the best way forward of all the
options discussed.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 0/5] arm: sa1100: fix sa1100 fb and pcmcia w/o cpufreq
From: Dmitry Eremin-Solenikov @ 2014-10-29 18:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1414147047-12892-1-git-send-email-dbaryshkov@gmail.com>
2014-10-24 13:37 GMT+03:00 Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>:
> Hello,
>
> These patches were sitting in my queue for some time. On SA-1100
> framebuffer and PCMCIA drivers make use of cpufreq_get(0) function call
> to determine the cpu frequency. Russell's commit
> 1937f5b91833e2e8e53bcc821fc7a5fbe6ccb9b5 (ARM: fix sa1100 build) fixed
> the build issues, but broke two devices (Collie and Jornada720). For
> those two boards the cpufreq code gets compiled but is not enabled (as
> board files do not provide timing information for the CPUFREQ driver).
> Thus cpufreq_get(0) returns incorrect values and incorrect timings get
> programmed into the hardware.
>
> PXA2xx (the very similar platform) uses Clock API to determine CPU
> frequency both in framebuffer and PCMCIA drivers. These patches make
> similar changes to StrongARM drivers.
>
> These patches are required to make use of framebuffer and CF card on
> Sharp Collie (and possibly on HP Jornada 720).
Russell, what about these patches?
They are absolutely required for Collie to work properly.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH] omap: dss: connector-analog-tv: Add missing module device table
From: Tomi Valkeinen @ 2014-10-30 12:52 UTC (permalink / raw)
To: Marek Belisko; +Cc: plagnioj, hns, linux-omap, linux-fbdev, linux-kernel
In-Reply-To: <1414441443-10224-1-git-send-email-marek@goldelico.com>
[-- Attachment #1: Type: text/plain, Size: 940 bytes --]
On 27/10/14 22:24, Marek Belisko wrote:
> Without that fix connector-analog-tv driver isn't probed when compiled
> as module.
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
> drivers/video/fbdev/omap2/displays-new/connector-analog-tv.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/video/fbdev/omap2/displays-new/connector-analog-tv.c b/drivers/video/fbdev/omap2/displays-new/connector-analog-tv.c
> index 5ee3b55..d29cf72 100644
> --- a/drivers/video/fbdev/omap2/displays-new/connector-analog-tv.c
> +++ b/drivers/video/fbdev/omap2/displays-new/connector-analog-tv.c
> @@ -301,6 +301,8 @@ static const struct of_device_id tvc_of_match[] = {
> {},
> };
>
> +MODULE_DEVICE_TABLE(of, tvc_of_match);
> +
> static struct platform_driver tvc_connector_driver = {
> .probe = tvc_probe,
> .remove = __exit_p(tvc_remove),
>
Thanks, queued for 3.18 fixes.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [GIT PULL] fbdev fixes for 3.18
From: Tomi Valkeinen @ 2014-10-30 13:20 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Linux Fbdev development list, LKML
[-- Attachment #1: Type: text/plain, Size: 3739 bytes --]
Hi Linus,
Please pull fbdev fixes for 3.18.
Tomi
The following changes since commit f114040e3ea6e07372334ade75d1ee0775c355e1:
Linux 3.18-rc1 (2014-10-19 18:08:38 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git tags/fbdev-fixes-3.18
for you to fetch changes up to a942535d6e5c23f0015d6674d28c91dd59a720e4:
Merge branch '3.18/omapdss-fixes' into 3.18/fbdev-fixes (2014-10-30 14:53:49 +0200)
----------------------------------------------------------------
fbdev fixes for 3.18
* Fix fb console option parsing
* Fixes for OMAPDSS/OMAPFB crashes related to module unloading and
device/driver binding & unbinding.
* Fix for OMAP HDMI PLL locking failing in certain cases
* Misc minor fixes for atmel lcdfb and OMAP
----------------------------------------------------------------
Alexandre Belloni (1):
drivers: video: fbdev: atmel_lcdfb.c: remove unnecessary header
Felipe Balbi (2):
OMAPFB: add missing MODULE_ALIAS()
OMAPFB: remove __exit annotation
Maarten ter Huurne (1):
fbcon: Fix option parsing control flow in fb_console_setup
Marek Belisko (1):
omap: dss: connector-analog-tv: Add missing module device table
Mark Rustad (1):
video/console: Resolve several shadow warnings
Tomi Valkeinen (10):
OMAPDSS: set suppress_bind_attrs
OMAPDSS: apply: wait pending updates on manager disable
OMAPFB: fix overlay disable when freeing resources.
OMAPFB: fix releasing overlays
OMAPDSS: HDMI: fix PLL GO bit handling
OMAPDSS: HDMI: fix regsd write
OMAPDSS: DISPC: fix mflag offset
OMAPDSS: fix dispc register dump for preload & mflag
OMAPDSS: DSI: Fix PLL_SELFEQDCO field width
Merge branch '3.18/omapdss-fixes' into 3.18/fbdev-fixes
drivers/video/console/fbcon.c | 19 ++++++++---------
drivers/video/console/vgacon.c | 24 +++++++++++-----------
drivers/video/fbdev/atmel_lcdfb.c | 1 -
.../fbdev/omap2/displays-new/connector-analog-tv.c | 3 +++
.../video/fbdev/omap2/displays-new/connector-dvi.c | 1 +
.../fbdev/omap2/displays-new/connector-hdmi.c | 1 +
.../fbdev/omap2/displays-new/encoder-tfp410.c | 1 +
.../fbdev/omap2/displays-new/encoder-tpd12s015.c | 1 +
drivers/video/fbdev/omap2/displays-new/panel-dpi.c | 1 +
.../video/fbdev/omap2/displays-new/panel-dsi-cm.c | 1 +
.../omap2/displays-new/panel-lgphilips-lb035q02.c | 1 +
.../omap2/displays-new/panel-nec-nl8048hl11.c | 1 +
.../omap2/displays-new/panel-sharp-ls037v7dw01.c | 1 +
.../omap2/displays-new/panel-sony-acx565akm.c | 1 +
.../omap2/displays-new/panel-tpo-td028ttec1.c | 1 +
.../omap2/displays-new/panel-tpo-td043mtea1.c | 1 +
drivers/video/fbdev/omap2/dss/apply.c | 2 ++
drivers/video/fbdev/omap2/dss/dispc.c | 8 ++++----
drivers/video/fbdev/omap2/dss/dispc.h | 3 +--
drivers/video/fbdev/omap2/dss/dpi.c | 1 +
drivers/video/fbdev/omap2/dss/dsi.c | 3 ++-
drivers/video/fbdev/omap2/dss/dss.c | 1 +
drivers/video/fbdev/omap2/dss/hdmi4.c | 1 +
drivers/video/fbdev/omap2/dss/hdmi5.c | 1 +
drivers/video/fbdev/omap2/dss/hdmi_pll.c | 13 ++++++------
drivers/video/fbdev/omap2/dss/rfbi.c | 1 +
drivers/video/fbdev/omap2/dss/sdi.c | 1 +
drivers/video/fbdev/omap2/dss/venc.c | 1 +
drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 18 ++++++++--------
29 files changed, 67 insertions(+), 46 deletions(-)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 01/15] mfd: add new driver for Sharp LoCoMo
From: Linus Walleij @ 2014-10-31 7:42 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux Input,
linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org, Andrea Adami,
Russell King, Daniel Mack, Haojian Zhuang, Robert Jarzmik,
Alexandre Courbot, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo
In-Reply-To: <1414454528-24240-2-git-send-email-dbaryshkov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> LoCoMo is a GA used on Sharp Zaurus SL-5x00. Current driver does has
> several design issues (special bus instead of platform bus, doesn't use
> mfd-core, etc).
>
> Implement 'core' parts of locomo support as an mfd driver.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
(...)
> +/* DAC send data */
> +#define M62332_SLAVE_ADDR 0x4e /* Slave address */
> +#define M62332_W_BIT 0x00 /* W bit (0 only) */
> +#define M62332_SUB_ADDR 0x00 /* Sub address */
> +#define M62332_A_BIT 0x00 /* A bit (0 only) */
> +
> +/* DAC setup and hold times (expressed in us) */
> +#define DAC_BUS_FREE_TIME 5 /* 4.7 us */
> +#define DAC_START_SETUP_TIME 5 /* 4.7 us */
> +#define DAC_STOP_SETUP_TIME 4 /* 4.0 us */
> +#define DAC_START_HOLD_TIME 5 /* 4.7 us */
> +#define DAC_SCL_LOW_HOLD_TIME 5 /* 4.7 us */
> +#define DAC_SCL_HIGH_HOLD_TIME 4 /* 4.0 us */
> +#define DAC_DATA_SETUP_TIME 1 /* 250 ns */
> +#define DAC_DATA_HOLD_TIME 1 /* 300 ns */
> +#define DAC_LOW_SETUP_TIME 1 /* 300 ns */
> +#define DAC_HIGH_SETUP_TIME 1 /* 1000 ns */
(...)
It seems some DAC handling is part of the MFD driver, and we recently
discussed that MFD should not be doing misc stuff but mainly act as
arbiter and switching station.
Can you please move the DAC parts of the driver to
drivers/iio/dac?
The IIO DAC subsystem will likely add other goodies to
the driver for free and give a nice API to consumers.
> +/* IRQ support */
> +static void locomo_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + struct locomo *lchip = irq_get_handler_data(irq);
> + int req, i;
> +
> + /* Acknowledge the parent IRQ */
> + desc->irq_data.chip->irq_ack(&desc->irq_data);
> +
> + /* check why this interrupt was generated */
> + req = readw(lchip->base + LOCOMO_ICR) & 0x0f00;
> +
> + if (req) {
> + /* generate the next interrupt(s) */
> + irq = lchip->irq_base;
This use if a static IRQ base is oldschool. Use irqdomain,
see for example tc3589x.c.
irq_domain_add_simple() should suffice.
IIRC you have used irqdomain before so you know the drill.
> + for (i = 0; i <= 3; i++, irq++) {
> + if (req & (0x0100 << i))
> + generic_handle_irq(irq);
> +
> + }
Reading the status register once and then check the IRQs
can be dangerous on non-threaded interrupt handlers as it could
miss transient IRQs appearing duing the IRQ handling.
The best example code is in drivers/irqchip.
For example in irq-vic.c there is this nice loop:
while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
irq = ffs(stat) - 1;
handle_domain_irq(vic->domain, irq, regs);
handled = 1;
}
Note how stat is re-read on each iteration.
> +static void locomo_setup_irq(struct locomo *lchip)
> +{
> + int irq;
> +
> + lchip->irq_base = irq_alloc_descs(-1, 0, LOCOMO_NR_IRQS, -1);
irqdomain will allocate descriptors for you when calling
irq_create_mapping() which should be done for all lines.
> + /* Install handlers for IRQ_LOCOMO_* */
> + for (irq = lchip->irq_base;
> + irq < lchip->irq_base + LOCOMO_NR_IRQS;
> + irq++) {
> + irq_set_chip_and_handler(irq, &locomo_chip, handle_level_irq);
> + irq_set_chip_data(irq, lchip);
> + set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
> + }
> +
> + /*
> + * Install handler for IRQ_LOCOMO_HW.
> + */
> + irq_set_irq_type(lchip->irq, IRQ_TYPE_EDGE_FALLING);
> + irq_set_handler_data(lchip->irq, lchip);
> + irq_set_chained_handler(lchip->irq, locomo_handler);
> +}
(...)
> + /* Longtime timer */
> + writew(0, lchip->base + LOCOMO_LTINT);
> + /* SPI */
> + writew(0, lchip->base + LOCOMO_SPI + LOCOMO_SPIIE);
> +
> + writew(6 + 8 + 320 + 30 - 10, lchip->base + LOCOMO_ASD);
That's a few magic numbers and calculation don't you think?
A comment stating what's going on would be helpful.
> + r = readw(lchip->base + LOCOMO_ASD);
> + r |= 0x8000;
> + writew(r, lchip->base + LOCOMO_ASD);
> +
> + writew(6 + 8 + 320 + 30 - 10 - 128 + 4, lchip->base + LOCOMO_HSD);
Dito.
> + r = readw(lchip->base + LOCOMO_HSD);
> + r |= 0x8000;
> + writew(r, lchip->base + LOCOMO_HSD);
> +
> + writew(128 / 8, lchip->base + LOCOMO_HSC);
Dito.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 02/15] GPIO: port LoCoMo gpio support from old driver
From: Linus Walleij @ 2014-10-31 7:48 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <1414454528-24240-3-git-send-email-dbaryshkov@gmail.com>
On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> Add gpiolib driver for gpio pins placed on the LoCoMo GA.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
(...)
> +static int locomo_gpio_get(struct gpio_chip *chip,
> + unsigned offset)
> +{
> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
> +
> + return readw(lg->regs + LOCOMO_GPL) & (1 << offset);
Do this:
#include <linux/bitops.h>
return !!(readw(lg->regs + LOCOMO_GPL) & BIT(offset));
So you clamp the returned value to a bool.
> +static void __locomo_gpio_set(struct gpio_chip *chip,
> + unsigned offset, int value)
> +{
> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
> + u16 r;
> +
> + r = readw(lg->regs + LOCOMO_GPO);
> + if (value)
> + r |= 1 << offset;
r |= BIT(offset);
> + else
> + r &= ~(1 << offset);
r &= BIT(offset);
(etc, everywhere this pattern occurs).
> +static void locomo_gpio_set(struct gpio_chip *chip,
> + unsigned offset, int value)
> +{
> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&lg->lock, flags);
> +
> + __locomo_gpio_set(chip, offset, value);
> +
> + spin_unlock_irqrestore(&lg->lock, flags);
If you actually always have to be getting and releasing a spin lock around
the register writes, contemplate using regmap-mmio because that
is part of what it does.
But is this locking really necessary?
> +static int locomo_gpio_remove(struct platform_device *pdev)
> +{
> + struct locomo_gpio *lg = platform_get_drvdata(pdev);
> + int ret;
> +
> + ret = gpiochip_remove(&lg->gpio);
> + if (ret) {
> + dev_err(&pdev->dev, "Can't remove gpio chip: %d\n", ret);
> + return ret;
> + }
The return value from gpiochip_remove() has been removed in v3.18-rc1
so this will not compile.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 09/15] ARM: sa1100: don't preallocate IRQ space for locomo
From: Linus Walleij @ 2014-10-31 7:50 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <1414454528-24240-10-git-send-email-dbaryshkov@gmail.com>
On Tue, Oct 28, 2014 at 1:02 AM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> As new locomo driver properly supports SPARSE_IRQ, stop playing with
> NR_IRQS on sa1100 (locomo was the last chip requiring NR_IRQ tricks).
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
That's right! By moving to irqdomain things get even better :)
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 14/15] gpio: locomo: implement per-pin irq handling
From: Linus Walleij @ 2014-10-31 8:00 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <1414454528-24240-15-git-send-email-dbaryshkov@gmail.com>
On Tue, Oct 28, 2014 at 1:02 AM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> LoCoMo has a possibility to generate per-GPIO edge irqs. Support for
> that was there in old locomo driver, got 'cleaned up' during old driver
> IRQ cascading cleanup and is now reimplemented. It is expected that
> SL-5500 (collie) will use locomo gpio irqs for mmc detection irq.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Please don't use open-coded IRQ handling like this, we are moving
away from that.
In Kconfig,
select GPIOLIB_IRQCHIP
and look at the other drivers selecting this for inspiration. There
is even some documentation in Documentation/gpio/driver.txt
You will find that it cuts down a lot of overhead from your driver
and does everything in the right way in a central place.
> struct locomo_gpio {
> void __iomem *regs;
> + int irq;
>
> spinlock_t lock;
> struct gpio_chip gpio;
> + int irq_base;
gpiolib irqchip helpers uses irqdomain to do all this debasing
and rebasing for you. Go with that.
> +static int locomo_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
> +
> + return lg->irq_base + offset;
> +}
And it implements .to_irq() in the gpiolib core.
> +static void
> +locomo_gpio_handler(unsigned int irq, struct irq_desc *desc)
It's locomo_gpio_irq_handler() right?
> +{
> + u16 req;
> + struct locomo_gpio *lg = irq_get_handler_data(irq);
> + int i = lg->irq_base;
> +
> + req = readw(lg->regs + LOCOMO_GIR) &
> + readw(lg->regs + LOCOMO_GPD);
> +
> + while (req) {
> + if (req & 1)
> + generic_handle_irq(i);
> + req >>= 1;
> + i++;
> + }
Same thing as the MFD device, look closer at how you construct
the IRQ handling loop, so the register gets re-read each iteration.
> +static void locomo_gpio_ack_irq(struct irq_data *d)
> +{
> + struct locomo_gpio *lg = irq_data_get_irq_chip_data(d);
> + unsigned long flags;
> + u16 r;
> +
> + spin_lock_irqsave(&lg->lock, flags);
> +
> + r = readw(lg->regs + LOCOMO_GWE);
> + r |= (0x0001 << (d->irq - lg->irq_base));
> + writew(r, lg->regs + LOCOMO_GWE);
> +
> + r = readw(lg->regs + LOCOMO_GIS);
> + r &= ~(0x0001 << (d->irq - lg->irq_base));
> + writew(r, lg->regs + LOCOMO_GIS);
> +
> + r = readw(lg->regs + LOCOMO_GWE);
> + r &= ~(0x0001 << (d->irq - lg->irq_base));
> + writew(r, lg->regs + LOCOMO_GWE);
> +
> + spin_unlock_irqrestore(&lg->lock, flags);
> +}
I really wonder if this locking is needed around these
regioster accesses. It seems more like a habit than
like something that is actually needed. Think it over.
*irqsave* versions of spinlocks are definately wrong
in the irqchip callbacks, if you give it a minute I think
you quickly realize why.
> +static int locomo_gpio_type(struct irq_data *d, unsigned int type)
> +{
> + unsigned int mask;
> + struct locomo_gpio *lg = irq_data_get_irq_chip_data(d);
> + unsigned long flags;
> +
> + mask = 1 << (d->irq - lg->irq_base);
This should just use d->hwirq with irqdomain implemented
correctly.
(...)
> +static void locomo_gpio_setup_irq(struct locomo_gpio *lg)
> +{
> + int irq;
> +
> + lg->irq_base = irq_alloc_descs(-1, 0, LOCOMO_GPIO_NR_IRQS, -1);
> +
> + /* Install handlers for IRQ_LOCOMO_* */
> + for (irq = lg->irq_base;
> + irq < lg->irq_base + LOCOMO_GPIO_NR_IRQS;
> + irq++) {
> + irq_set_chip_and_handler(irq, &locomo_gpio_chip,
> + handle_edge_irq);
> + irq_set_chip_data(irq, lg);
> + set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
> + }
> +
> + /*
> + * Install handler for IRQ_LOCOMO_HW.
> + */
> + irq_set_handler_data(lg->irq, lg);
> + irq_set_chained_handler(lg->irq, locomo_gpio_handler);
> +}
All this gets redundant with gpiochip_irqchip_add()
and gpiochip_set_chained_irqchip().
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v4 0/5] simplefb: add clock handling code
From: Tomi Valkeinen @ 2014-10-31 8:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1413996311-4287-1-git-send-email-hdegoede@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 648 bytes --]
Hi Hans,
On 22/10/14 19:45, Hans de Goede wrote:
> -We will add a clocks property to the simplefb devicetree bindings, so
> that u-boot setup framebuffers passed to the kernel (for early console
> support) can properly list the clocks used, and simplefb can claim them to
> avoid them getting turned off, thereby breaking the early console
Perhaps this has been discussed earlier, but I started to wonder if
managing clocks is enough.
What about regulators (to power the LCD), gpios (to keep the LCD
enabled) and pinctrl (for video signals)? I guess any those could be
reset by the kernel if no driver uses them.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH v4 0/5] simplefb: add clock handling code
From: Hans de Goede @ 2014-10-31 9:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <54534523.8080801@ti.com>
Hi Tomi,
On 10/31/2014 09:15 AM, Tomi Valkeinen wrote:
> Hi Hans,
>
> On 22/10/14 19:45, Hans de Goede wrote:
>
>> -We will add a clocks property to the simplefb devicetree bindings, so
>> that u-boot setup framebuffers passed to the kernel (for early console
>> support) can properly list the clocks used, and simplefb can claim them to
>> avoid them getting turned off, thereby breaking the early console
>
> Perhaps this has been discussed earlier, but I started to wonder if
> managing clocks is enough.
>
> What about regulators (to power the LCD), gpios (to keep the LCD
> enabled) and pinctrl (for video signals)? I guess any those could be
> reset by the kernel if no driver uses them.
Yes this has been discussed before, AFAIK gpio / pinctrls should not be
spontaneously reset by the kernels. Regulators will be a problem, but there
we can pretty much follow whatever solution we end up with for clocks, since
they work very much alike (e.g. we could add a supply property to the
simplefb node).
Note I've no intention to add regulator support atm, we can do that when the
need arises, so that we can actually test the code.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 09/15] ARM: sa1100: don't preallocate IRQ space for locomo
From: Dmitry Eremin-Solenikov @ 2014-10-31 9:33 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <CACRpkdZnAtXtdtKbuqj8a_6a=2jS_Jf6yrq=iz-GJV+4RifRhw@mail.gmail.com>
2014-10-31 10:50 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Oct 28, 2014 at 1:02 AM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
>
>> As new locomo driver properly supports SPARSE_IRQ, stop playing with
>> NR_IRQS on sa1100 (locomo was the last chip requiring NR_IRQ tricks).
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>
> That's right! By moving to irqdomain things get even better :)
That is a separate topic in my repo. I'll push that after finishing with LoCoMo.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 14/15] gpio: locomo: implement per-pin irq handling
From: Dmitry Eremin-Solenikov @ 2014-10-31 9:35 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <CACRpkdamVriR9kJSeY=LVAx7ADukT8+GpqmNH9YMnL3OKUk8iw@mail.gmail.com>
2014-10-31 11:00 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Oct 28, 2014 at 1:02 AM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
>
>> LoCoMo has a possibility to generate per-GPIO edge irqs. Support for
>> that was there in old locomo driver, got 'cleaned up' during old driver
>> IRQ cascading cleanup and is now reimplemented. It is expected that
>> SL-5500 (collie) will use locomo gpio irqs for mmc detection irq.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>
> Please don't use open-coded IRQ handling like this, we are moving
> away from that.
>
> In Kconfig,
>
> select GPIOLIB_IRQCHIP
>
> and look at the other drivers selecting this for inspiration. There
> is even some documentation in Documentation/gpio/driver.txt
>
> You will find that it cuts down a lot of overhead from your driver
> and does everything in the right way in a central place.
Thanks, I will take a look.
[Skipped the rest of the comments mostly dedicated to gpiolib_irqchip].
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH v4 0/5] simplefb: add clock handling code
From: Javier Martinez Canillas @ 2014-10-31 9:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <54534FAE.1060401@redhat.com>
Hello Hans,
On Fri, Oct 31, 2014 at 10:00 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi Tomi,
>
> On 10/31/2014 09:15 AM, Tomi Valkeinen wrote:
>> Hi Hans,
>>
>> On 22/10/14 19:45, Hans de Goede wrote:
>>
>>> -We will add a clocks property to the simplefb devicetree bindings, so
>>> that u-boot setup framebuffers passed to the kernel (for early console
>>> support) can properly list the clocks used, and simplefb can claim them to
>>> avoid them getting turned off, thereby breaking the early console
>>
>> Perhaps this has been discussed earlier, but I started to wonder if
>> managing clocks is enough.
>>
Managing clocks is definitely not enough. For example, adding some
regulators to the Snow Chromebook DTS broke simplefb [0]. This has
been mentioned on at least one of the many simplefb threads.
>> What about regulators (to power the LCD), gpios (to keep the LCD
>> enabled) and pinctrl (for video signals)? I guess any those could be
>> reset by the kernel if no driver uses them.
>
> Yes this has been discussed before, AFAIK gpio / pinctrls should not be
> spontaneously reset by the kernels. Regulators will be a problem, but there
> we can pretty much follow whatever solution we end up with for clocks, since
> they work very much alike (e.g. we could add a supply property to the
> simplefb node).
>
I think someone already said this but with regulators is not that easy
since you don't have an equivalent of the "clocks" property.
You need a "<name>-supply" property for each regulator that is an
input supply. Some boards may need different regulators for their
display (e.g: one to power the LCD and another to power the backlight)
so adding a single supply property will not be enough.
> Note I've no intention to add regulator support atm, we can do that when the
> need arises, so that we can actually test the code.
>
AFAIU that's exactly what Thierry was complaining about. There seems
to be only a focus on solving the clocks issue since that is needed
for sunxi but ignoring how the problem for other resources should be
solved.
The assumptions made by simplefb are quite fragile and only adding
support for managing clocks is just a partial solution that will give
a false sense of robustness since it could break later due to other
resources.
Best regards,
Javier
[0]: http://www.spinics.net/lists/arm-kernel/msg360119.html
^ permalink raw reply
* Re: [PATCH 02/15] GPIO: port LoCoMo gpio support from old driver
From: Dmitry Eremin-Solenikov @ 2014-10-31 9:39 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux Input,
linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org, Andrea Adami,
Russell King, Daniel Mack, Haojian Zhuang, Robert Jarzmik,
Alexandre Courbot, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo
In-Reply-To: <CACRpkdY7tRadod2vQfEytmw-ubaMAvr_=XTczD5bUMkqie0xkg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-10-31 10:48 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
>
>> Add gpiolib driver for gpio pins placed on the LoCoMo GA.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>
[skipped]
> (etc, everywhere this pattern occurs).
>> +static void locomo_gpio_set(struct gpio_chip *chip,
>> + unsigned offset, int value)
>> +{
>> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&lg->lock, flags);
>> +
>> + __locomo_gpio_set(chip, offset, value);
>> +
>> + spin_unlock_irqrestore(&lg->lock, flags);
>
> If you actually always have to be getting and releasing a spin lock around
> the register writes, contemplate using regmap-mmio because that
> is part of what it does.
>
> But is this locking really necessary?
I have a custom of doing such locking and never having to think about
somebody breaking into RMW cycles.
Also isn't regmap an overkill here? Wouldn't regmap also do a lock/unlock
around each register read/write/RMW?
>> +static int locomo_gpio_remove(struct platform_device *pdev)
>> +{
>> + struct locomo_gpio *lg = platform_get_drvdata(pdev);
>> + int ret;
>> +
>> + ret = gpiochip_remove(&lg->gpio);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Can't remove gpio chip: %d\n", ret);
>> + return ret;
>> + }
>
> The return value from gpiochip_remove() has been removed in v3.18-rc1
> so this will not compile.
Yes, the fix will be in the next iteration. This patchset was based on 3.17
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH v4 0/5] simplefb: add clock handling code
From: Hans de Goede @ 2014-10-31 9:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CABxcv=kdRTXKE8S-M_qRo_qz_Ap-e+qz393NQseNe=rkXZrxiw@mail.gmail.com>
Hi,
On 10/31/2014 10:37 AM, Javier Martinez Canillas wrote:
> Hello Hans,
>
> On Fri, Oct 31, 2014 at 10:00 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi Tomi,
>>
>> On 10/31/2014 09:15 AM, Tomi Valkeinen wrote:
>>> Hi Hans,
>>>
>>> On 22/10/14 19:45, Hans de Goede wrote:
>>>
>>>> -We will add a clocks property to the simplefb devicetree bindings, so
>>>> that u-boot setup framebuffers passed to the kernel (for early console
>>>> support) can properly list the clocks used, and simplefb can claim them to
>>>> avoid them getting turned off, thereby breaking the early console
>>>
>>> Perhaps this has been discussed earlier, but I started to wonder if
>>> managing clocks is enough.
>>>
>
> Managing clocks is definitely not enough. For example, adding some
> regulators to the Snow Chromebook DTS broke simplefb [0]. This has
> been mentioned on at least one of the many simplefb threads.
Ok.
>>> What about regulators (to power the LCD), gpios (to keep the LCD
>>> enabled) and pinctrl (for video signals)? I guess any those could be
>>> reset by the kernel if no driver uses them.
>>
>> Yes this has been discussed before, AFAIK gpio / pinctrls should not be
>> spontaneously reset by the kernels. Regulators will be a problem, but there
>> we can pretty much follow whatever solution we end up with for clocks, since
>> they work very much alike (e.g. we could add a supply property to the
>> simplefb node).
>>
>
> I think someone already said this but with regulators is not that easy
> since you don't have an equivalent of the "clocks" property.
>
> You need a "<name>-supply" property for each regulator that is an
> input supply. Some boards may need different regulators for their
> display (e.g: one to power the LCD and another to power the backlight)
> so adding a single supply property will not be enough.
Right so we can simply insert simplefbvdd1-supply, simplefbvdd2-supply, etc.
regulator nodes into the simplefb node, and have the simplefb kernel code look
for supplies with a name of simplefb_vdd%d in a loop until it fails to find one.
Note that the foo-supply names are local to the simplefb node, these do not
need to be the same as the ones in the nodes describing the actual hardware
blocks. Not having an array is somewhat inconvenient, but easily worked around
and once worked around this really is just the same problem as with clocks.
>> Note I've no intention to add regulator support atm, we can do that when the
>> need arises, so that we can actually test the code.
>>
>
> AFAIU that's exactly what Thierry was complaining about. There seems
> to be only a focus on solving the clocks issue since that is needed
> for sunxi but ignoring how the problem for other resources should be
> solved.
That is simply not true, I'm fully aware that we may eventually need other
resources, e.g. regulators. I just want to take this one step at a time,
I really believe that once we have this figured out for one resource, it
becomes a lot easier to also figure out for other resources.
> The assumptions made by simplefb are quite fragile and only adding
> support for managing clocks is just a partial solution that will give
> a false sense of robustness since it could break later due to other
> resources.
The key word here being resources. We need to figure out how to do
resource management for simplefb, once we have that figured out for one
resource, the others can follow the same "design-pattern". As the long
thread about the clocks support has shown, getting consensus on this
with only one resource in play already is hard enough, so baby steps
is the way to go.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 11/15] sound: soc: poodle: make use of new locomo GPIO interface
From: Linus Walleij @ 2014-10-31 9:52 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Mark Brown, Dmitry Eremin-Solenikov,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux Input,
linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org, Andrea Adami,
Russell King, Daniel Mack, Haojian Zhuang, Robert Jarzmik,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones
In-Reply-To: <CAAVeFuKgARoMFzf+663iP6cULs93d4WSQS8ESjUb9VcxguWurA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Oct 29, 2014 at 4:03 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
>>> Since LoCoMo driver has been converted to provide proper gpiolib
>>> interface, make poodle ASoC platform driver use gpiolib API.
>>
>> Please use subject lines matching the style for the subsystem.
>>
>>> + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
>>> + ret);
>>> + return ret;
>>> + }
>>
>> I sense a need for devm_gpio_request_array() here. Otherwise this looks
>> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
>> so no need to block on this.
>
> I wish Dmitry took the opportunity to move this driver to the gpiod
> API, especially since doing so would be trivial for this driver.
+1 on this.
However this platform is not device tree, so this implies setting up
a descriptor table for the affected driver(s) to work properly.
See Documentation/gpio/board.txt
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 01/15] mfd: add new driver for Sharp LoCoMo
From: Dmitry Eremin-Solenikov @ 2014-10-31 9:54 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <CACRpkdbW3NA00zMZr216ZeZpgVkXwKUo30biTVd5kcHKGGtkHg@mail.gmail.com>
Hello,
Thank you for the review of patches.
2014-10-31 10:42 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
>
>> LoCoMo is a GA used on Sharp Zaurus SL-5x00. Current driver does has
>> several design issues (special bus instead of platform bus, doesn't use
>> mfd-core, etc).
>>
>> Implement 'core' parts of locomo support as an mfd driver.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> (...)
>
>> +/* DAC send data */
>> +#define M62332_SLAVE_ADDR 0x4e /* Slave address */
>> +#define M62332_W_BIT 0x00 /* W bit (0 only) */
>> +#define M62332_SUB_ADDR 0x00 /* Sub address */
>> +#define M62332_A_BIT 0x00 /* A bit (0 only) */
>> +
>> +/* DAC setup and hold times (expressed in us) */
>> +#define DAC_BUS_FREE_TIME 5 /* 4.7 us */
>> +#define DAC_START_SETUP_TIME 5 /* 4.7 us */
>> +#define DAC_STOP_SETUP_TIME 4 /* 4.0 us */
>> +#define DAC_START_HOLD_TIME 5 /* 4.7 us */
>> +#define DAC_SCL_LOW_HOLD_TIME 5 /* 4.7 us */
>> +#define DAC_SCL_HIGH_HOLD_TIME 4 /* 4.0 us */
>> +#define DAC_DATA_SETUP_TIME 1 /* 250 ns */
>> +#define DAC_DATA_HOLD_TIME 1 /* 300 ns */
>> +#define DAC_LOW_SETUP_TIME 1 /* 300 ns */
>> +#define DAC_HIGH_SETUP_TIME 1 /* 1000 ns */
> (...)
>
> It seems some DAC handling is part of the MFD driver, and we recently
> discussed that MFD should not be doing misc stuff but mainly act as
> arbiter and switching station.
>
> Can you please move the DAC parts of the driver to
> drivers/iio/dac?
>
> The IIO DAC subsystem will likely add other goodies to
> the driver for free and give a nice API to consumers.
I wanted this part to be as simple as possible. I will look into IIO
DAC subsystem.
The DAC is as simple 2 channel 8-bit i2c device connected to a separate i2c bus
controlled through a register in LoCoMo device. One channel is used
for backlight,
other will be used for volume control. So (in theory) I can add the
following device
chain: locomo -> i2c-locomo -> m62332 -> IIO DAC client. However isn't that
quite an overkill for just backlight & volume control? Please advice me on this.
[skipped the irqdomain part - I will use them, thanks for the suggestion.]
>> + /* Longtime timer */
>> + writew(0, lchip->base + LOCOMO_LTINT);
>> + /* SPI */
>> + writew(0, lchip->base + LOCOMO_SPI + LOCOMO_SPIIE);
>> +
>> + writew(6 + 8 + 320 + 30 - 10, lchip->base + LOCOMO_ASD);
>
> That's a few magic numbers and calculation don't you think?
>
> A comment stating what's going on would be helpful.
Unfortunately little is known here - these values are c&p from old 2.4
Lineo code.
This part is related to generating synchronization pulses for accessing
touchscreen.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 11/15] sound: soc: poodle: make use of new locomo GPIO interface
From: Dmitry Eremin-Solenikov @ 2014-10-31 9:58 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, Mark Brown,
linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Dmitry Torokhov,
Bryan Wu, Richard Purdie, Samuel Ortiz, Lee Jones, Jingoo
In-Reply-To: <CACRpkdY-RDuxGMWe_SD613ovsEoHhxjP1+3T-m_zKQSx=mkFjA@mail.gmail.com>
2014-10-31 12:52 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> On Wed, Oct 29, 2014 at 4:03 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote:
>>> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
>>>> Since LoCoMo driver has been converted to provide proper gpiolib
>>>> interface, make poodle ASoC platform driver use gpiolib API.
>>>
>>> Please use subject lines matching the style for the subsystem.
>>>
>>>> + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
>>>> + ret);
>>>> + return ret;
>>>> + }
>>>
>>> I sense a need for devm_gpio_request_array() here. Otherwise this looks
>>> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
>>> so no need to block on this.
>>
>> I wish Dmitry took the opportunity to move this driver to the gpiod
>> API, especially since doing so would be trivial for this driver.
>
> +1 on this.
>
> However this platform is not device tree, so this implies setting up
> a descriptor table for the affected driver(s) to work properly.
> See Documentation/gpio/board.txt
I checked the gpiod interfaces after original suggestion by Alexandre.
Introducing those mapping tables (much like pinctrl tables) look like
a duplicate effort if Russell will permit adding a DT support. So
I thought that I will reconsider gpiod/pinctrl/etc after fixing
LoCoMo, reiterating IRQ patches, possibly switching to COMMON_CLK
and (finally) thinking about device tree support.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH v4 0/5] simplefb: add clock handling code
From: Javier Martinez Canillas @ 2014-10-31 10:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <54535B8D.7040508@redhat.com>
Hello Hans,
Thanks for your answers.
On Fri, Oct 31, 2014 at 10:51 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> What about regulators (to power the LCD), gpios (to keep the LCD
>>>> enabled) and pinctrl (for video signals)? I guess any those could be
>>>> reset by the kernel if no driver uses them.
>>>
>>> Yes this has been discussed before, AFAIK gpio / pinctrls should not be
>>> spontaneously reset by the kernels. Regulators will be a problem, but there
>>> we can pretty much follow whatever solution we end up with for clocks, since
>>> they work very much alike (e.g. we could add a supply property to the
>>> simplefb node).
>>>
>>
>> I think someone already said this but with regulators is not that easy
>> since you don't have an equivalent of the "clocks" property.
>>
>> You need a "<name>-supply" property for each regulator that is an
>> input supply. Some boards may need different regulators for their
>> display (e.g: one to power the LCD and another to power the backlight)
>> so adding a single supply property will not be enough.
>
> Right so we can simply insert simplefbvdd1-supply, simplefbvdd2-supply, etc.
> regulator nodes into the simplefb node, and have the simplefb kernel code look
> for supplies with a name of simplefb_vdd%d in a loop until it fails to find one.
>
> Note that the foo-supply names are local to the simplefb node, these do not
> need to be the same as the ones in the nodes describing the actual hardware
> blocks. Not having an array is somewhat inconvenient, but easily worked around
> and once worked around this really is just the same problem as with clocks.
>
Yes, I guess the question is how many regulators the driver should
support but I expect most displays don't need more than a couple.
>>> Note I've no intention to add regulator support atm, we can do that when the
>>> need arises, so that we can actually test the code.
>>>
>>
>> AFAIU that's exactly what Thierry was complaining about. There seems
>> to be only a focus on solving the clocks issue since that is needed
>> for sunxi but ignoring how the problem for other resources should be
>> solved.
>
> That is simply not true, I'm fully aware that we may eventually need other
> resources, e.g. regulators. I just want to take this one step at a time,
> I really believe that once we have this figured out for one resource, it
> becomes a lot easier to also figure out for other resources.
>
Great, I'm glad to hear that you are willing to add support for
regulators (and other resources).
>> The assumptions made by simplefb are quite fragile and only adding
>> support for managing clocks is just a partial solution that will give
>> a false sense of robustness since it could break later due to other
>> resources.
>
> The key word here being resources. We need to figure out how to do
> resource management for simplefb, once we have that figured out for one
> resource, the others can follow the same "design-pattern". As the long
> thread about the clocks support has shown, getting consensus on this
> with only one resource in play already is hard enough, so baby steps
> is the way to go.
>
I'm certainly not against adding clock management if that will make
simplefb more robust. I was just worried that other resources could be
ignored. But I'm ok of doing baby steps if there is a long term plan.
As I mentioned in another thread, I believe Doug's suggestion is the
best approach. That is to have the simplefb node with all its
resources in the DTS but with status = "disabled". The bootloader
should only fill the information about the framebuffer memory region
(location and size, width, height, format, etc) and change the
simplefb node status to "okay".
If a FDT does not have a simplefb node then the bootloader can create
one (like is made for the /choosen node in most bootloaders).
That will better document which resources are really used when someone
reads the DTS and also avoid duplicating code in the bootloaders to
fill all the information about resources but is just my humble opinion
and I really don't want to bikesheed more on this.
Best regards,
Javier
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH v4 0/5] simplefb: add clock handling code
From: Rob Herring @ 2014-10-31 20:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5450CAA5.5020205@ti.com>
On Wed, Oct 29, 2014 at 7:08 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Hans, Rob,
>
> On 28/10/14 13:30, Hans de Goede wrote:
>> Hi,
>>
>> On 10/28/2014 12:11 PM, Rob Herring wrote:
>
>>> Yes, I object to the binding still as it has not changed from what was
>>> previously posted.
>>
>> It would be helpful if you could explain why you object. Last time you
>> said: " You are mixing in a hardware description that is simply inaccurate."
>>
>> I then explained that this is not hardware description, but runtime state
>> information, as it tells the kernel which clocks were chosen to drive the
>> display (out of typically a list of possible options, depending on which
>> output is used, etc.). Just like which memory address the bootloader has
>> chosen to scan out the video image from.
>>
>> Then you got quiet, so sorry, but this time your objection really is too
>> late. You cannot simply go quiet halfway through a discussion and then pop
>> up again when a new version is posted to say "I object" yet another time,
>> you've had your chance to make your arguments last time, and chose to stay
>> quiet after I explained in detail that this is not hardware description but
>> state information, so now it is simply too late.
>>
>> These bindings have been discussed at Plumbers with various interested people
>> present, and the conclusion was that this really is the best way to handle this,
>> so this patch is:
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>
>> And David Herrman who is working on simpledrm, which will be merged soon, which
>> will also use the simplefb bindings also agrees. So we have the simplefb maintainer,
>> simpledrm maintainer, and the clk subsystem maintainer + 2 other maintainers all
>> agreeing on a way forward, the time for bikeshedding now really really really is
>> over.
>>
>> Tomi, can you please let us know how you plan to proceed with this ?
>
> I won't merge DT bindings via fbdev tree, if a DT maintainer says no.
>
> I took Rob's silence to the earlier series as a silent ack for your
> explanation. Obviously that was not the case.
>
> Rob, please advice asap what should be done to the bindings to get your
> ack. As Hans explained above, this discussion has been going on for a
> long time, and afaik this series is the best way forward of all the
> options discussed.
I still think for the most part this is a kernel problem. It is a
kernel policy to turn off unused clocks. The clock framework could
just as easily decide that any clocks enabled at boot and left
un-managed (i.e. w/o a driver) are kept on until they are managed. I'm
not saying this can't be in DT, only that DT is not the only solution
here. This problem is not unique to simplefb. A serial console could
stop working if no serial driver is loaded before unused clocks are
disabled. CPU core clocks have a similar issue as well (often enabled
in platform code). I want to see this solved in a generic way for any
clock.
As regulators were also mentioned, they already have a
"regulator-boot-on" property defined. Perhaps this is suitable to be
mirrored for clocks. If it is not, then I'm wondering why we have it.
A key difference here is that the property is part of the provider and
can be dealt with in the clock driver rather than requiring a
temporary driver.
Rob
^ permalink raw reply
* [PATCH] m501fb: don't return zero on failure path in sm501fb_probe()
From: Alexey Khoroshilov @ 2014-10-31 22:40 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen
Cc: Alexey Khoroshilov, linux-fbdev, linux-kernel, ldv-project
If no framebuffers found, sm501fb_probe() breaks off initialization,
deallocates sm501fb_info, but returns zero. As a result, use after free
can happen in sm501fb_remove().
The patch adds -ENODEV as a return value in this case.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
drivers/video/fbdev/sm501fb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/fbdev/sm501fb.c b/drivers/video/fbdev/sm501fb.c
index 9e74e8fbe074..8b98b011fc04 100644
--- a/drivers/video/fbdev/sm501fb.c
+++ b/drivers/video/fbdev/sm501fb.c
@@ -1988,6 +1988,7 @@ static int sm501fb_probe(struct platform_device *pdev)
if (info->fb[HEAD_PANEL] = NULL &&
info->fb[HEAD_CRT] = NULL) {
dev_err(dev, "no framebuffers found\n");
+ ret = -ENODEV;
goto err_alloc;
}
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 11/15] sound: soc: poodle: make use of new locomo GPIO interface
From: Alexandre Courbot @ 2014-11-01 5:42 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Linus Walleij, Mark Brown, linux-arm-kernel@lists.infradead.org,
linux-gpio@vger.kernel.org, Linux Input,
linux-leds@vger.kernel.org, linux-spi@vger.kernel.org,
linux-fbdev@vger.kernel.org, alsa-devel@alsa-project.org,
Andrea Adami, Russell King, Daniel Mack, Haojian Zhuang,
Robert Jarzmik, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Ji
In-Reply-To: <CALT56yP6K-sXOGM4z-WRsvFVjOi-13wM+Ewqn6WpDmNB4xcOEg@mail.gmail.com>
On Fri, Oct 31, 2014 at 6:58 PM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> 2014-10-31 12:52 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
>> On Wed, Oct 29, 2014 at 4:03 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>>> On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote:
>>>> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
>>>>> Since LoCoMo driver has been converted to provide proper gpiolib
>>>>> interface, make poodle ASoC platform driver use gpiolib API.
>>>>
>>>> Please use subject lines matching the style for the subsystem.
>>>>
>>>>> + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
>>>>> + if (ret) {
>>>>> + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
>>>>> + ret);
>>>>> + return ret;
>>>>> + }
>>>>
>>>> I sense a need for devm_gpio_request_array() here. Otherwise this looks
>>>> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
>>>> so no need to block on this.
>>>
>>> I wish Dmitry took the opportunity to move this driver to the gpiod
>>> API, especially since doing so would be trivial for this driver.
>>
>> +1 on this.
>>
>> However this platform is not device tree, so this implies setting up
>> a descriptor table for the affected driver(s) to work properly.
>> See Documentation/gpio/board.txt
>
> I checked the gpiod interfaces after original suggestion by Alexandre.
>
> Introducing those mapping tables (much like pinctrl tables) look like
> a duplicate effort if Russell will permit adding a DT support. So
> I thought that I will reconsider gpiod/pinctrl/etc after fixing
> LoCoMo, reiterating IRQ patches, possibly switching to COMMON_CLK
> and (finally) thinking about device tree support.
Note that the mapping tables are likely not going to end up being
huge, and taking that step will allow you to convert to the gpiod
interface, something you would probably want to do later when adding
DT support anyway. So at the end of the day there would be very little
wasted effort: once converting to DT, just add the GPIO properties
into the appropriate node, remove the mapping tables, and you're done.
^ 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