Re: [PATCH] tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers
On Mon, Mar 04, 2019 at 06:51:33PM +0100, Noralf Trønnes wrote: > Den 04.03.2019 16.10, skrev Andy Shevchenko: > > On Mon, Mar 04, 2019 at 03:45:56PM +0100, Noralf Trønnes wrote: > >> Den 22.02.2019 16.58, skrev Andy Shevchenko: > >>> On Fri, Feb 22, 2019 at 01:43:29PM +0100, Noralf Trønnes wrote: > > Acked-by: Andy Shevchenko > > > > I hope to see this in v5.1-rc1. > > The patch doesn't apply cleanly on (to be) 5.1 due to this one: > > drm/tinydrm: Use struct drm_rect > https://cgit.freedesktop.org/drm/drm/commit/?id=b051b3459bbae907ef068bcd8b62f73f09ea5016 > > If it's just a debug warning but no ill effects, it'll show up in 5.2. > Otherwise I will have to backport it. For x86 it's just a debug message, since on x86 stack memory is DMA capable. But in general this should be fixed for any possible scenarios. -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 2/2] drm: i915: Switch to bitmap_zalloc()
On Mon, Mar 04, 2019 at 09:41:34AM +, Chris Wilson wrote:
> Quoting Andy Shevchenko (2019-03-04 09:29:08)
> > Switch to bitmap_zalloc() to show clearly what we are allocating.
> > Besides that it returns pointer of bitmap type instead of opaque void *.
>
> Which is confusing; since we explicitly want unsigned longs, not some
> amorphous bitmap type.
Why? You use it as a bitmap anyway since you are telling below you are using
bit ops like set/clear_bit.
> > if (obj->bit_17 == NULL) {
> > - obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count),
> > - sizeof(long), GFP_KERNEL);
> > + obj->bit_17 = bitmap_zalloc(page_count, GFP_KERNEL);
>
> That feels a bit more of an overreach, as we just use bitops and never
> actually use the bitmap iface.
bitops are _luckily_ part of bitmap iface. bitmap iface has been evolved
specifically the way the existing ops will work on it w/o any change.
> Simply because it kills BITS_TO_LONGS(), even though I do not see why
> the bitmap_[z]alloc and bitmap_free are not inlines...
Because of circular dependencies (hell) in the headers.
> And for this is not the overflow protection of kcalloc silly? We start
> with a large value, factorise it, then check that the two factors do not
> overflow? If it were to overflow, it would overflow in the
> BITS_TO_LONGS() itself.
This just a simple API change w/o functional changes.
> Reviewed-by: Chris Wilson
Thank you.
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1 1/2] drm/selftests/mm: Switch to bitmap_zalloc()
Switch to bitmap_zalloc() to show clearly what we are allocating. Besides that it returns pointer of bitmap type instead of opaque void *. Signed-off-by: Andy Shevchenko --- drivers/gpu/drm/selftests/test-drm_mm.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c b/drivers/gpu/drm/selftests/test-drm_mm.c index fbed2c90fd51..d1206aef26af 100644 --- a/drivers/gpu/drm/selftests/test-drm_mm.c +++ b/drivers/gpu/drm/selftests/test-drm_mm.c @@ -1615,7 +1615,7 @@ static int igt_topdown(void *ignored) DRM_RND_STATE(prng, random_seed); const unsigned int count = 8192; unsigned int size; - unsigned long *bitmap = NULL; + unsigned long *bitmap; struct drm_mm mm; struct drm_mm_node *nodes, *node, *next; unsigned int *order, n, m, o = 0; @@ -1631,8 +1631,7 @@ static int igt_topdown(void *ignored) if (!nodes) goto err; - bitmap = kcalloc(count / BITS_PER_LONG, sizeof(unsigned long), -GFP_KERNEL); + bitmap = bitmap_zalloc(count, GFP_KERNEL); if (!bitmap) goto err_nodes; @@ -1717,7 +1716,7 @@ static int igt_topdown(void *ignored) drm_mm_takedown(&mm); kfree(order); err_bitmap: - kfree(bitmap); + bitmap_free(bitmap); err_nodes: vfree(nodes); err: @@ -1745,8 +1744,7 @@ static int igt_bottomup(void *ignored) if (!nodes) goto err; - bitmap = kcalloc(count / BITS_PER_LONG, sizeof(unsigned long), -GFP_KERNEL); + bitmap = bitmap_zcalloc(count, GFP_KERNEL); if (!bitmap) goto err_nodes; @@ -1818,7 +1816,7 @@ static int igt_bottomup(void *ignored) drm_mm_takedown(&mm); kfree(order); err_bitmap: - kfree(bitmap); + bitmap_free(bitmap); err_nodes: vfree(nodes); err: -- 2.20.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/2] drm: i915: Switch to bitmap_zalloc()
Switch to bitmap_zalloc() to show clearly what we are allocating.
Besides that it returns pointer of bitmap type instead of opaque void *.
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_gem_fence_reg.c | 3 +--
drivers/gpu/drm/i915/i915_gem_tiling.c| 6 +++---
drivers/gpu/drm/i915/selftests/intel_uncore.c | 5 ++---
4 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6728ea5c71d4..0d96520cfdb3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4410,7 +4410,7 @@ static void __i915_gem_free_objects(struct
drm_i915_private *i915,
drm_gem_object_release(&obj->base);
i915_gem_info_remove_obj(i915, obj->base.size);
- kfree(obj->bit_17);
+ bitmap_free(obj->bit_17);
i915_gem_object_free(obj);
GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index e037e94792f3..1f880e5b79b0 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -790,8 +790,7 @@ i915_gem_object_save_bit_17_swizzle(struct
drm_i915_gem_object *obj,
int i;
if (obj->bit_17 == NULL) {
- obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count),
- sizeof(long), GFP_KERNEL);
+ obj->bit_17 = bitmap_zalloc(page_count, GFP_KERNEL);
if (obj->bit_17 == NULL) {
DRM_ERROR("Failed to allocate memory for bit 17 "
"record\n");
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c
b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 16cc9ddbce34..a9b5329dae3b 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -301,11 +301,11 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object
*obj,
/* Try to preallocate memory required to save swizzling on put-pages */
if (i915_gem_object_needs_bit17_swizzle(obj)) {
if (!obj->bit_17) {
- obj->bit_17 = kcalloc(BITS_TO_LONGS(obj->base.size >>
PAGE_SHIFT),
- sizeof(long), GFP_KERNEL);
+ obj->bit_17 = bitmap_zalloc(obj->base.size >>
PAGE_SHIFT,
+ GFP_KERNEL);
}
} else {
- kfree(obj->bit_17);
+ bitmap_free(obj->bit_17);
obj->bit_17 = NULL;
}
diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c
b/drivers/gpu/drm/i915/selftests/intel_uncore.c
index 81d9d31042a9..600167ebb303 100644
--- a/drivers/gpu/drm/i915/selftests/intel_uncore.c
+++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c
@@ -137,8 +137,7 @@ static int intel_uncore_check_forcewake_domains(struct
drm_i915_private *dev_pri
if (!IS_ENABLED(CONFIG_DRM_I915_SELFTEST_BROKEN))
return 0;
- valid = kcalloc(BITS_TO_LONGS(FW_RANGE), sizeof(*valid),
- GFP_KERNEL);
+ valid = bitmap_zalloc(FW_RANGE, GFP_KERNEL);
if (!valid)
return -ENOMEM;
@@ -173,7 +172,7 @@ static int intel_uncore_check_forcewake_domains(struct
drm_i915_private *dev_pri
}
}
- kfree(valid);
+ bitmap_free(valid);
return err;
}
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1 2/2] drm: i915: Switch to bitmap_zalloc()
Switch to bitmap_zalloc() to show clearly what we are allocating.
Besides that it returns pointer of bitmap type instead of opaque void *.
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_gem_fence_reg.c | 3 +--
drivers/gpu/drm/i915/i915_gem_tiling.c| 6 +++---
drivers/gpu/drm/i915/selftests/intel_uncore.c | 5 ++---
4 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6728ea5c71d4..0d96520cfdb3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4410,7 +4410,7 @@ static void __i915_gem_free_objects(struct
drm_i915_private *i915,
drm_gem_object_release(&obj->base);
i915_gem_info_remove_obj(i915, obj->base.size);
- kfree(obj->bit_17);
+ bitmap_free(obj->bit_17);
i915_gem_object_free(obj);
GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index e037e94792f3..1f880e5b79b0 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -790,8 +790,7 @@ i915_gem_object_save_bit_17_swizzle(struct
drm_i915_gem_object *obj,
int i;
if (obj->bit_17 == NULL) {
- obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count),
- sizeof(long), GFP_KERNEL);
+ obj->bit_17 = bitmap_zalloc(page_count, GFP_KERNEL);
if (obj->bit_17 == NULL) {
DRM_ERROR("Failed to allocate memory for bit 17 "
"record\n");
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c
b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 16cc9ddbce34..a9b5329dae3b 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -301,11 +301,11 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object
*obj,
/* Try to preallocate memory required to save swizzling on put-pages */
if (i915_gem_object_needs_bit17_swizzle(obj)) {
if (!obj->bit_17) {
- obj->bit_17 = kcalloc(BITS_TO_LONGS(obj->base.size >>
PAGE_SHIFT),
- sizeof(long), GFP_KERNEL);
+ obj->bit_17 = bitmap_zalloc(obj->base.size >>
PAGE_SHIFT,
+ GFP_KERNEL);
}
} else {
- kfree(obj->bit_17);
+ bitmap_free(obj->bit_17);
obj->bit_17 = NULL;
}
diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c
b/drivers/gpu/drm/i915/selftests/intel_uncore.c
index 81d9d31042a9..600167ebb303 100644
--- a/drivers/gpu/drm/i915/selftests/intel_uncore.c
+++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c
@@ -137,8 +137,7 @@ static int intel_uncore_check_forcewake_domains(struct
drm_i915_private *dev_pri
if (!IS_ENABLED(CONFIG_DRM_I915_SELFTEST_BROKEN))
return 0;
- valid = kcalloc(BITS_TO_LONGS(FW_RANGE), sizeof(*valid),
- GFP_KERNEL);
+ valid = bitmap_zalloc(FW_RANGE, GFP_KERNEL);
if (!valid)
return -ENOMEM;
@@ -173,7 +172,7 @@ static int intel_uncore_check_forcewake_domains(struct
drm_i915_private *dev_pri
}
}
- kfree(valid);
+ bitmap_free(valid);
return err;
}
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/2] drm/selftests/mm: Switch to bitmap_zalloc()
Switch to bitmap_zalloc() to show clearly what we are allocating. Besides that it returns pointer of bitmap type instead of opaque void *. Signed-off-by: Andy Shevchenko --- drivers/gpu/drm/selftests/test-drm_mm.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c b/drivers/gpu/drm/selftests/test-drm_mm.c index fbed2c90fd51..286a0eeefcb6 100644 --- a/drivers/gpu/drm/selftests/test-drm_mm.c +++ b/drivers/gpu/drm/selftests/test-drm_mm.c @@ -1615,7 +1615,7 @@ static int igt_topdown(void *ignored) DRM_RND_STATE(prng, random_seed); const unsigned int count = 8192; unsigned int size; - unsigned long *bitmap = NULL; + unsigned long *bitmap; struct drm_mm mm; struct drm_mm_node *nodes, *node, *next; unsigned int *order, n, m, o = 0; @@ -1631,8 +1631,7 @@ static int igt_topdown(void *ignored) if (!nodes) goto err; - bitmap = kcalloc(count / BITS_PER_LONG, sizeof(unsigned long), -GFP_KERNEL); + bitmap = bitmap_zalloc(count, GFP_KERNEL); if (!bitmap) goto err_nodes; @@ -1717,7 +1716,7 @@ static int igt_topdown(void *ignored) drm_mm_takedown(&mm); kfree(order); err_bitmap: - kfree(bitmap); + bitmap_free(bitmap); err_nodes: vfree(nodes); err: @@ -1745,8 +1744,7 @@ static int igt_bottomup(void *ignored) if (!nodes) goto err; - bitmap = kcalloc(count / BITS_PER_LONG, sizeof(unsigned long), -GFP_KERNEL); + bitmap = bitmap_zalloc(count, GFP_KERNEL); if (!bitmap) goto err_nodes; @@ -1818,7 +1816,7 @@ static int igt_bottomup(void *ignored) drm_mm_takedown(&mm); kfree(order); err_bitmap: - kfree(bitmap); + bitmap_free(bitmap); err_nodes: vfree(nodes); err: -- 2.20.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers
On Mon, Mar 04, 2019 at 03:45:56PM +0100, Noralf Trønnes wrote:
>
>
> Den 22.02.2019 16.58, skrev Andy Shevchenko:
> > On Fri, Feb 22, 2019 at 01:43:29PM +0100, Noralf Trønnes wrote:
> >> Buffers passed to spi_sync() must be dma-safe even for tiny buffers since
> >> some SPI controllers use DMA for all transfers.
> >>
> >> Example splat with CONFIG_DMA_API_DEBUG enabled:
> >>
> >> [ 23.750467] DMA-API: dw_dmac_pci :00:15.0: device driver maps
> >> memory from stack [probable addr=1e49185d]
> >> [ 23.750529] WARNING: CPU: 1 PID: 1296 at kernel/dma/debug.c:1161
> >> check_for_stack+0xb7/0x190
> >> [ 23.750533] Modules linked in: mmc_block(+) spi_pxa2xx_platform(+)
> >> pwm_lpss_pci pwm_lpss spi_pxa2xx_pci sdhci_pci cqhci intel_mrfld_pwrbtn
> >> extcon_intel_mrfld sdhci intel_mrfld_adc led_class mmc_core ili9341
> >> mipi_dbi tinydrm backlight ti_ads7950 industrialio_triggered_buffer
> >> kfifo_buf intel_soc_pmic_mrfld hci_uart btbcm
> >> [ 23.750599] CPU: 1 PID: 1296 Comm: modprobe Not tainted 5.0.0-rc7+ #236
> >> [ 23.750605] Hardware name: Intel Corporation Merrifield/BODEGA BAY,
> >> BIOS 542 2015.01.21:18.19.48
> >> [ 23.750620] RIP: 0010:check_for_stack+0xb7/0x190
> >> [ 23.750630] Code: 8b 6d 50 4d 85 ed 75 04 4c 8b 6d 10 48 89 ef e8 2f 8b
> >> 44 00 48 89 c6 4a 8d 0c 23 4c 89 ea 48 c7 c7 88 d0 82 b4 e8 40 7c f9 ff
> >> <0f> 0b 8b 05 79 00 4b 01 85 c0 74 07 5b 5d 41 5c 41 5d c3 8b 05 54
> >> [ 23.750637] RSP: :97bbc0292fa0 EFLAGS: 00010286
> >> [ 23.750646] RAX: RBX: 97bbc029 RCX:
> >> 0006
> >> [ 23.750652] RDX: 0007 RSI: 0002 RDI:
> >> 94b33e115450
> >> [ 23.750658] RBP: 94b33c8578b0 R08: 0002 R09:
> >> 000201c0
> >> [ 23.750664] R10: 0006ecb0ccc6 R11: 00034f38 R12:
> >> 316c
> >> [ 23.750670] R13: 94b33c84b250 R14: 94b33dedd5a0 R15:
> >> 0001
> >> [ 23.750679] FS: () GS:94b33e10(0063)
> >> knlGS:f7faf690
> >> [ 23.750686] CS: 0010 DS: 002b ES: 002b CR0: 80050033
> >> [ 23.750691] CR2: f7f54faf CR3: 0722c000 CR4:
> >> 001006e0
> >> [ 23.750696] Call Trace:
> >> [ 23.750713] debug_dma_map_sg+0x100/0x340
> >> [ 23.750727] ? dma_direct_map_sg+0x3b/0xb0
> >> [ 23.750739] spi_map_buf+0x25a/0x300
> >> [ 23.750751] __spi_pump_messages+0x2a4/0x680
> >> [ 23.750762] __spi_sync+0x1dd/0x1f0
> >> [ 23.750773] spi_sync+0x26/0x40
> >> [ 23.750790] mipi_dbi_typec3_command_read+0x14d/0x240 [mipi_dbi]
> >> [ 23.750802] ? spi_finalize_current_transfer+0x10/0x10
> >> [ 23.750821] mipi_dbi_typec3_command+0x1bc/0x1d0 [mipi_dbi]
> >>
> >
> > After few runs I don't see the warning anymore. So,
> > Tested-by: Andy Shevchenko
> >
>
> Can I have an ack as well if you're satisfied with it?
Sure.
Acked-by: Andy Shevchenko
I hope to see this in v5.1-rc1.
>
> Noralf.
>
> > Though I would like to give few more days to monitor the state.
> > (I believe it's fixed)
> >
> >> Reported-by: Andy Shevchenko
> >> Signed-off-by: Noralf Trønnes
> >> ---
> >> drivers/gpu/drm/tinydrm/ili9225.c | 6 ++--
> >> drivers/gpu/drm/tinydrm/mipi-dbi.c | 58 +-
> >> include/drm/tinydrm/mipi-dbi.h | 5 +--
> >> 3 files changed, 48 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c
> >> b/drivers/gpu/drm/tinydrm/ili9225.c
> >> index 3f59cfbd31ba..a87078667e04 100644
> >> --- a/drivers/gpu/drm/tinydrm/ili9225.c
> >> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
> >> @@ -299,7 +299,7 @@ static void ili9225_pipe_disable(struct
> >> drm_simple_display_pipe *pipe)
> >>mipi->enabled = false;
> >> }
> >>
> >> -static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
> >> +static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
> >> size_t num)
> >> {
> >>struct spi_device *spi = mipi->spi;
> >> @@ -309,11 +309,11 @@ static int ili9225_dbi_command(struct mipi_dbi
> >> *mipi, u8 cmd, u8 *par,
> >>
> >>gpiod_set_value_cansleep(mipi->dc, 0);
> >
Re: [PATCH v1] drm/tinydrm/ili9341: Support Adafruit 2.8" TFT display
On Mon, Feb 25, 2019 at 04:07:05PM +0100, Noralf Trønnes wrote: > > > Den 25.02.2019 15.45, skrev Andy Shevchenko: > > The Adafruit 2.8" TFT display [1] has different dimensions than 2.4" one. > > Add support for it. > > > > [1]: https://cdn-shop.adafruit.com/datasheets/MI0283QT-11+V1.1.PDF > > This one is supported by drivers/gpu/drm/tinydrm/mi0283qt.c. It was the > first tinydrm driver and was named after the panel. Later we have > grouped the panels by controller driver. Yeah, thanks. It works. > So ideally mi0283qt.c should have been merged with ili9341.c. Yes, that's would be nice! Though we need still some unified compatible strings, while leaving old in place. > I know that this Adafruit display didn't always use a MI0283QT panel, so > maybe earlier they did use the dt280qv10 panel you mention her. The datasheet they point me to is for mi0283qt, but on the panel itself it's written dt280qv10. I dunno if it refers to the same or not. > The Watterott rpi-display also uses a MI0283QT panel. -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1] drm/tinydrm/ili9341: Support Adafruit 2.8" TFT display
The Adafruit 2.8" TFT display [1] has different dimensions than 2.4" one.
Add support for it.
[1]: https://cdn-shop.adafruit.com/datasheets/MI0283QT-11+V1.1.PDF
Signed-off-by: Andy Shevchenko
---
- based on top of drm-tip
drivers/gpu/drm/tinydrm/ili9341.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/ili9341.c
b/drivers/gpu/drm/tinydrm/ili9341.c
index 3b7565a6311e..7e20d7f1b25f 100644
--- a/drivers/gpu/drm/tinydrm/ili9341.c
+++ b/drivers/gpu/drm/tinydrm/ili9341.c
@@ -140,6 +140,10 @@ static const struct drm_display_mode yx240qv29_mode = {
DRM_SIMPLE_MODE(240, 320, 37, 49),
};
+static const struct drm_display_mode dt280qv10_mode = {
+ DRM_SIMPLE_MODE(240, 320, 43, 58),
+};
+
DEFINE_DRM_GEM_CMA_FOPS(ili9341_fops);
static struct drm_driver ili9341_driver = {
@@ -155,25 +159,32 @@ static struct drm_driver ili9341_driver = {
};
static const struct of_device_id ili9341_of_match[] = {
- { .compatible = "adafruit,yx240qv29" },
+ { .compatible = "adafruit,yx240qv29", .data = &yx240qv29_mode },
+ { .compatible = "adafruit,dt280qv10", .data = &dt280qv10_mode },
{ }
};
MODULE_DEVICE_TABLE(of, ili9341_of_match);
static const struct spi_device_id ili9341_id[] = {
- { "yx240qv29", 0 },
+ { "yx240qv29", (kernel_ulong_t)&yx240qv29_mode },
+ { "dt280qv10", (kernel_ulong_t)&dt280qv10_mode },
{ }
};
MODULE_DEVICE_TABLE(spi, ili9341_id);
static int ili9341_probe(struct spi_device *spi)
{
+ const struct drm_display_mode *mode;
struct device *dev = &spi->dev;
struct mipi_dbi *mipi;
struct gpio_desc *dc;
u32 rotation = 0;
int ret;
+ mode = device_get_match_data(dev);
+ if (!mode)
+ return -ENODEV;
+
mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
if (!mipi)
return -ENOMEM;
@@ -201,7 +212,7 @@ static int ili9341_probe(struct spi_device *spi)
return ret;
ret = mipi_dbi_init(&spi->dev, mipi, &ili9341_pipe_funcs,
- &ili9341_driver, &yx240qv29_mode, rotation);
+ &ili9341_driver, mode, rotation);
if (ret)
return ret;
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers
On Fri, Feb 22, 2019 at 01:43:29PM +0100, Noralf Trønnes wrote:
> Buffers passed to spi_sync() must be dma-safe even for tiny buffers since
> some SPI controllers use DMA for all transfers.
>
> Example splat with CONFIG_DMA_API_DEBUG enabled:
>
> [ 23.750467] DMA-API: dw_dmac_pci :00:15.0: device driver maps memory
> from stack [probable addr=1e49185d]
> [ 23.750529] WARNING: CPU: 1 PID: 1296 at kernel/dma/debug.c:1161
> check_for_stack+0xb7/0x190
> [ 23.750533] Modules linked in: mmc_block(+) spi_pxa2xx_platform(+)
> pwm_lpss_pci pwm_lpss spi_pxa2xx_pci sdhci_pci cqhci intel_mrfld_pwrbtn
> extcon_intel_mrfld sdhci intel_mrfld_adc led_class mmc_core ili9341 mipi_dbi
> tinydrm backlight ti_ads7950 industrialio_triggered_buffer kfifo_buf
> intel_soc_pmic_mrfld hci_uart btbcm
> [ 23.750599] CPU: 1 PID: 1296 Comm: modprobe Not tainted 5.0.0-rc7+ #236
> [ 23.750605] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS
> 542 2015.01.21:18.19.48
> [ 23.750620] RIP: 0010:check_for_stack+0xb7/0x190
> [ 23.750630] Code: 8b 6d 50 4d 85 ed 75 04 4c 8b 6d 10 48 89 ef e8 2f 8b 44
> 00 48 89 c6 4a 8d 0c 23 4c 89 ea 48 c7 c7 88 d0 82 b4 e8 40 7c f9 ff <0f> 0b
> 8b 05 79 00 4b 01 85 c0 74 07 5b 5d 41 5c 41 5d c3 8b 05 54
> [ 23.750637] RSP: :97bbc0292fa0 EFLAGS: 00010286
> [ 23.750646] RAX: RBX: 97bbc029 RCX:
> 0006
> [ 23.750652] RDX: 0007 RSI: 0002 RDI:
> 94b33e115450
> [ 23.750658] RBP: 94b33c8578b0 R08: 0002 R09:
> 000201c0
> [ 23.750664] R10: 0006ecb0ccc6 R11: 00034f38 R12:
> 316c
> [ 23.750670] R13: 94b33c84b250 R14: 94b33dedd5a0 R15:
> 0001
> [ 23.750679] FS: () GS:94b33e10(0063)
> knlGS:f7faf690
> [ 23.750686] CS: 0010 DS: 002b ES: 002b CR0: 80050033
> [ 23.750691] CR2: f7f54faf CR3: 0722c000 CR4:
> 001006e0
> [ 23.750696] Call Trace:
> [ 23.750713] debug_dma_map_sg+0x100/0x340
> [ 23.750727] ? dma_direct_map_sg+0x3b/0xb0
> [ 23.750739] spi_map_buf+0x25a/0x300
> [ 23.750751] __spi_pump_messages+0x2a4/0x680
> [ 23.750762] __spi_sync+0x1dd/0x1f0
> [ 23.750773] spi_sync+0x26/0x40
> [ 23.750790] mipi_dbi_typec3_command_read+0x14d/0x240 [mipi_dbi]
> [ 23.750802] ? spi_finalize_current_transfer+0x10/0x10
> [ 23.750821] mipi_dbi_typec3_command+0x1bc/0x1d0 [mipi_dbi]
>
After few runs I don't see the warning anymore. So,
Tested-by: Andy Shevchenko
Though I would like to give few more days to monitor the state.
(I believe it's fixed)
> Reported-by: Andy Shevchenko
> Signed-off-by: Noralf Trønnes
> ---
> drivers/gpu/drm/tinydrm/ili9225.c | 6 ++--
> drivers/gpu/drm/tinydrm/mipi-dbi.c | 58 +-
> include/drm/tinydrm/mipi-dbi.h | 5 +--
> 3 files changed, 48 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c
> b/drivers/gpu/drm/tinydrm/ili9225.c
> index 3f59cfbd31ba..a87078667e04 100644
> --- a/drivers/gpu/drm/tinydrm/ili9225.c
> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
> @@ -299,7 +299,7 @@ static void ili9225_pipe_disable(struct
> drm_simple_display_pipe *pipe)
> mipi->enabled = false;
> }
>
> -static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
> +static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
> size_t num)
> {
> struct spi_device *spi = mipi->spi;
> @@ -309,11 +309,11 @@ static int ili9225_dbi_command(struct mipi_dbi *mipi,
> u8 cmd, u8 *par,
>
> gpiod_set_value_cansleep(mipi->dc, 0);
> speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
> - ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
> + ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
> if (ret || !num)
> return ret;
>
> - if (cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
> + if (*cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
> bpw = 16;
>
> gpiod_set_value_cansleep(mipi->dc, 1);
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index 246e708a9ff7..4a4cd7e72938 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -153,16 +153,42 @@ EXPORT_SYMBOL(mipi_dbi_command_read);
> */
> int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
> {
> + u8 *cmdbuf;
> int ret;
>
> + /* SPI requires dma-safe buffers */
>
[PATCH v1] drm/tinydrm: Trivia typo fix
Fix adddress -> address typo. Signed-off-by: Andy Shevchenko --- drivers/gpu/drm/tinydrm/mipi-dbi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index 3a05e56f9b0d..2f8c2d1d09bc 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -897,7 +897,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, * Even though it's not the SPI device that does DMA (the master does), * the dma mask is necessary for the dma_alloc_wc() in * drm_gem_cma_create(). The dma_addr returned will be a physical -* adddress which might be different from the bus address, but this is +* address which might be different from the bus address, but this is * not a problem since the address will not be used. * The virtual address is used in the transfer and the SPI core * re-maps it on the SPI master device using the DMA streaming API -- 2.20.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/3] ACPI / PMIC: Implement exec_mipi_pmic_seq_element for CHT Whiskey Cove PMIC
> Implement the exec_mipi_pmic_seq_element callback for the CHT Whiskey Cove
> PMIC.
> On some CHT devices this fixes the LCD panel not lighting up when it was
> not initialized by the GOP, because an external monitor was plugged in and
> the GOP initialized only the external monitor.
> + /* byte 0 aka PMIC Flag is reserved */
> + i2c_client_address = get_unaligned_le16(data + 1);
> + reg_address = get_unaligned_le32(data + 3);
> + value = get_unaligned_le32(data + 7);
> + mask= get_unaligned_le32(data + 11);
> +
> + if (i2c_client_address > 255 || reg_address > 255) {
Hmm... I would rather like to see hexadecimal for addresses and decimal for
something like countable value for data.
> + pr_warn("%s warning addresses too big client 0x%x reg 0x%x\n",
> + __func__, i2c_client_address, reg_address);
> + return;
> + }
> +
> + address = (i2c_client_address << 8) | reg_address;
> + regmap_update_bits(regmap, address, mask, value);
> +}
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv3] lib/ratelimit: Lockless ratelimiting
On Tue, Jul 17, 2018 at 3:59 AM, Dmitry Safonov wrote:
> I would be glad if someone helps/bothers to review the change :C
>
Perhaps Petr and / or Steven can help you.
> Thanks,
> Dmitry
>
> On Tue, 2018-07-03 at 23:56 +0100, Dmitry Safonov wrote:
>> Currently ratelimit_state is protected with spin_lock. If the .lock
>> is
>> taken at the moment of ___ratelimit() call, the message is suppressed
>> to
>> make ratelimiting robust.
>>
>> That results in the following issue issue:
>> CPU0 CPU1
>> -- --
>> printk_ratelimit() printk_ratelimit()
>> | |
>> try_spin_lock() try_spin_lock()
>> | |
>> time_is_before_jiffies() return 0; // suppress
>>
>> So, concurrent call of ___ratelimit() results in silently suppressing
>> one of the messages, regardless if the limit is reached or not.
>> And rc->missed is not increased in such case so the issue is covered
>> from user.
>>
>> Convert ratelimiting to use atomic counters and drop spin_lock.
>>
>> Note: That might be unexpected, but with the first interval of
>> messages
>> storm one can print up to burst*2 messages. So, it doesn't guarantee
>> that in *any* interval amount of messages is lesser than burst.
>> But, that differs lightly from previous behavior where one can start
>> burst=5 interval and print 4 messages on the last milliseconds of
>> interval + new 5 messages from new interval (totally 9 messages in
>> lesser than interval value):
>>
>>msg0 msg1-msg4 msg0-msg4
>> | | |
>> | | |
>> |--o-o-|-o|--> (t)
>> <--->
>>Lesser than burst
>>
>> Dropped dev/random patch since v1 version:
>> lkml.kernel.org/r/<20180510125211.12583-1-d...@arista.com>
>>
>> Dropped `name' in as it's unused in RATELIMIT_STATE_INIT()
>>
>> Cc: Andy Shevchenko
>> Cc: Arnd Bergmann
>> Cc: David Airlie
>> Cc: Greg Kroah-Hartman
>> Cc: Jani Nikula
>> Cc: Joonas Lahtinen
>> Cc: Rodrigo Vivi
>> Cc: "Theodore Ts'o"
>> Cc: Thomas Gleixner
>> Cc: intel-...@lists.freedesktop.org
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: Dmitry Safonov
>> ---
>> drivers/char/random.c| 28 ---
>> drivers/gpu/drm/i915/i915_perf.c | 8 --
>> fs/btrfs/super.c | 16 +--
>> fs/xfs/scrub/scrub.c | 2 +-
>> include/linux/ratelimit.h| 31 -
>> kernel/user.c| 2 +-
>> lib/ratelimit.c | 59 +++---
>> --
>> 7 files changed, 73 insertions(+), 73 deletions(-)
>>
>> diff --git a/drivers/char/random.c b/drivers/char/random.c
>> index cd888d4ee605..0be31b3eadab 100644
>> --- a/drivers/char/random.c
>> +++ b/drivers/char/random.c
>> @@ -439,10 +439,8 @@ static void _crng_backtrack_protect(struct
>> crng_state *crng,
>> static void process_random_ready_list(void);
>> static void _get_random_bytes(void *buf, int nbytes);
>>
>> -static struct ratelimit_state unseeded_warning =
>> - RATELIMIT_STATE_INIT("warn_unseeded_randomness", HZ, 3);
>> -static struct ratelimit_state urandom_warning =
>> - RATELIMIT_STATE_INIT("warn_urandom_randomness", HZ, 3);
>> +static struct ratelimit_state unseeded_warning =
>> RATELIMIT_STATE_INIT(HZ, 3);
>> +static struct ratelimit_state urandom_warning =
>> RATELIMIT_STATE_INIT(HZ, 3);
>>
>> static int ratelimit_disable __read_mostly;
>>
>> @@ -937,24 +935,22 @@ static void crng_reseed(struct crng_state
>> *crng, struct entropy_store *r)
>> crng->init_time = jiffies;
>> spin_unlock_irqrestore(&crng->lock, flags);
>> if (crng == &primary_crng && crng_init < 2) {
>> + unsigned int unseeded_miss, urandom_miss;
>> +
>> invalidate_batched_entropy();
>> numa_crng_init();
>> crng_init = 2;
>> process_random_ready_list();
>> wake_up_interruptible(&crng_init_wait);
>> pr_notice("random: crng init done\n");
>> - if (unseeded_
Re: [PATCH] kernel.h: Add for_each_if()
On Fri, 2018-07-13 at 16:42 -0700, Randy Dunlap wrote:
> On 07/13/2018 04:37 PM, NeilBrown wrote:
>
> coding-style.rst says:
> Also, use braces when a loop contains more than a single simple
> statement:
Independently on a) would we use some macro for condition, or b) fix
macros against this kind of nested conditions, there is another
weirdness we would like to avoid, i.e.
for_each_foo() {
...
} else {
...
}
It is written according to coding style, but too much weird.
So, summarize this discussion I think we would
- keep for_each_if() in DRM subsystem alone
- fix macros which are using positive condition 'if (cond)' by replacing
with 'if (!cond) {} else' form for sake of robustness.
Do you agree on that?
--
Andy Shevchenko
Intel Finland Oy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] kernel.h: Add for_each_if()
On Mon, 2018-07-09 at 18:25 +0200, Daniel Vetter wrote:
> v2: Explain a bit better what this is good for, after the discussion
> with Peter Z.
Since I have not been Cc'ed to your discussion there is another
weirdness which this macro prohibits, i.e.
for_each_blah() {
} else {
...blahblah...
}
--
Andy Shevchenko
Intel Finland Oy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/12] kernel.h: Add for_each_if()
On Mon, 2018-07-09 at 10:36 +0200, Daniel Vetter wrote:
> To avoid compilers complainig about ambigious else blocks when putting
> an if condition into a for_each macro one needs to invert the
> condition and add a dummy else. We have a nice little convenience
> macro for that in drm headers, let's move it out. Subsequent patches
> will roll it out to other places.
>
> Motivated by a discussion with Andy and Yisheng, who want to add
> another for_each_macro which would benefit from for_each_if() instead
> of hand-rolling it.
>
Thanks, Daniel!
Reviewed-by: Andy Shevchenko
> Signed-off-by: Daniel Vetter
> Cc: Gustavo Padovan
> Cc: Maarten Lankhorst
> Cc: Sean Paul
> Cc: David Airlie
> Cc: Andrew Morton
> Cc: Kees Cook
> Cc: Ingo Molnar
> Cc: Greg Kroah-Hartman
> Cc: NeilBrown
> Cc: Wei Wang
> Cc: Stefan Agner
> Cc: Andrei Vagin
> Cc: Randy Dunlap
> Cc: Andy Shevchenko
> Cc: Yisheng Xie
> ---
> include/drm/drmP.h | 3 ---
> include/linux/kernel.h | 3 +++
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index f7a19c2a7a80..05350424a4d3 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -110,7 +110,4 @@ static inline bool drm_can_sleep(void)
> return true;
> }
>
> -/* helper for handling conditionals in various for_each macros */
> -#define for_each_if(condition) if (!(condition)) {} else
> -
> #endif
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 941dc0a5a877..4cb95ab9a5bc 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -71,6 +71,9 @@
> */
> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
>
> +/* helper for handling conditionals in various for_each macros */
> +#define for_each_if(condition) if (!(condition)) {} else
> +
> #define u64_to_user_ptr(x) ( \
> {\
> typecheck(u64, x); \
--
Andy Shevchenko
Intel Finland Oy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fbcon: introduce for_each_registered_fb() helper
On Mon, 2018-07-02 at 09:36 +0200, Bernd Petrovitsch wrote: > > +#define for_each_registered_fb(i) \ > > + for (i = 0; i < FB_MAX; i++)\ > > + if (registered_fb[i]) > > + > > That leaves the possibility of a dangling-else. > snip > #define for_each_registered_fb(i) \ > for (i = 0; i < FB_MAX; i++)\ > if (!registered_fb[i]) \ > continue; \ > else > snip > avoids that. Yes, you not alone :-) AFAIU there is a v2 which fixes that, though Daniel pointed out that DRM has a specific macro to make life easier. -- Andy Shevchenko Intel Finland Oy ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fbcon: introduce for_each_registered_fb() helper
On Mon, 2018-07-02 at 09:30 +0200, Daniel Vetter wrote: > On Fri, Jun 29, 2018 at 07:20:13PM +0300, Andy Shevchenko wrote: > > On Fri, 2018-06-29 at 00:20 +0800, Yisheng Xie wrote: > > LGTM except macro implementation. That's why I have mentioned > > for_each_pci_bridge() to look at. > > > > > +#define for_each_registered_fb(i)\ > > > + for (i = 0; i < FB_MAX; i++)\ > > > + if (registered_fb[i]) > > > + > > > > This needs to be protected against nested conditionals. > > Otherwise compiler issues a warning and even may generate wrong > > code. > > See for_each_if() in include/drm/drmP.h ... we should probably lift > that > into a general header. The for_each_if() is used all over drm in > iterator > macros, exactly to avoid surprises. Wow, didn't know we have a such. It's a good idea to forelift it for wider use. Yisheng, it seems you may use it in your patch directly. -- Andy Shevchenko Intel Finland Oy ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fbcon: introduce for_each_registered_fb() helper
On Fri, 2018-06-29 at 00:20 +0800, Yisheng Xie wrote:
> Following pattern is often used:
>
> for (i = 0; i < FB_MAX; i++) {
> if (registered_fb[i]) {
> ...
> }
> }
>
> Therefore, as Andy's suggestion, for_each_registered_fb() helper can
Suggested-by then ?
> be introduced to make the code easier to read and write by reducing
> indentation level. It also saves few lines of code in each occurrence.
>
> This patch convert all part here at the same time.
LGTM except macro implementation. That's why I have mentioned
for_each_pci_bridge() to look at.
> +#define for_each_registered_fb(i)\
> + for (i = 0; i < FB_MAX; i++)\
> + if (registered_fb[i])
> +
This needs to be protected against nested conditionals.
Otherwise compiler issues a warning and even may generate wrong code.
--
Andy Shevchenko
Intel Finland Oy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915/intel_dsi: Add acpi_gpio_mapping for the panel-enable GPIO
On Fri, 2018-06-29 at 14:51 +0300, Ville Syrjälä wrote: > On Fri, Jun 29, 2018 at 01:32:58PM +0200, Hans de Goede wrote: I saw that the change was discarded but I would comment about the GPIO ACPI mapping tables. > > + devm_acpi_dev_add_driver_gpios(dev->dev, > > panel_gpios); > > Some explanation on what this actually does would be nice. There is no > documentation that I can see so it's totally unclear why this is > needed. Documentation is here Documentation/acpi/gpio-properties.txt. The key phrase is "...the driver is supposed to know what to use the GpioIo()/GpioInt() resources for once it has identified the device. Having done that, it can simply assign names to the GPIO lines it is going to use and provide the GPIO subsystem with a mapping between those names and the ACPI GPIO resources corresponding to them. To do that, the driver needs to define a mapping table..." -- Andy Shevchenko Intel Finland Oy ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 fbdev-for-next 1/2] fbcon: introduce for_each_registered_fb() helper
On Sat, 2018-06-30 at 15:29 +0800, Yisheng Xie wrote:
> Following pattern is often used:
>
> for (i = 0; i < FB_MAX; i++) {
> if (registered_fb[i]) {
> ...
> }
> }
>
> Therefore, as Andy's suggestion, for_each_registered_fb() helper can
> be introduced to make the code easier to read and write by reducing
> indentation level. It also saves few lines of code in each occurrence.
>
> This patch convert all part here at the same time.
>
Reviewed-by: Andy Shevchenko
> Suggested-by: Andy Shevchenko
> Signed-off-by: Yisheng Xie
> ---
> v2:
> - rebase it to fbdev-for-next branch and add one more loop to replace
> - per Hans
> - Macro should be protected against nested conditionals. - per Andy
> drivers/video/fbdev/core/fbcon.c | 31 +++
> drivers/video/fbdev/core/fbmem.c | 4 +---
> include/linux/fb.h | 4
> 3 files changed, 16 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c
> b/drivers/video/fbdev/core/fbcon.c
> index 5fb156b..e30d3a1 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2234,8 +2234,8 @@ static int fbcon_switch(struct vc_data *vc)
>*
>* info->currcon = vc->vc_num;
>*/
> - for (i = 0; i < FB_MAX; i++) {
> - if (registered_fb[i] != NULL && registered_fb[i]-
> >fbcon_par) {
> + for_each_registered_fb(i) {
> + if (registered_fb[i]->fbcon_par) {
> struct fbcon_ops *o = registered_fb[i]-
> >fbcon_par;
>
> o->currcon = vc->vc_num;
> @@ -3124,11 +3124,9 @@ static int fbcon_fb_unregistered(struct fb_info
> *info)
> if (idx == info_idx) {
> info_idx = -1;
>
> - for (i = 0; i < FB_MAX; i++) {
> - if (registered_fb[i] != NULL) {
> - info_idx = i;
> - break;
> - }
> + for_each_registered_fb(i) {
> + info_idx = i;
> + break;
> }
> }
>
> @@ -3609,10 +3607,8 @@ static int fbcon_output_notifier(struct
> notifier_block *nb,
> deferred_takeover = false;
> logo_shown = FBCON_LOGO_DONTSHOW;
>
> - for (i = 0; i < FB_MAX; i++) {
> - if (registered_fb[i])
> - fbcon_fb_registered(registered_fb[i]);
> - }
> + for_each_registered_fb(i)
> + fbcon_fb_registered(registered_fb[i]);
>
> return NOTIFY_OK;
> }
> @@ -3638,11 +3634,9 @@ static void fbcon_start(void)
>
> console_lock();
>
> - for (i = 0; i < FB_MAX; i++) {
> - if (registered_fb[i] != NULL) {
> - info_idx = i;
> - break;
> - }
> + for_each_registered_fb(i) {
> + info_idx = i;
> + break;
> }
>
> do_fbcon_takeover(0);
> @@ -3669,15 +3663,12 @@ static void fbcon_exit(void)
> kfree((void *)softback_buf);
> softback_buf = 0UL;
>
> - for (i = 0; i < FB_MAX; i++) {
> + for_each_registered_fb(i) {
> int pending = 0;
>
> mapped = 0;
> info = registered_fb[i];
>
> - if (info == NULL)
> - continue;
> -
> if (info->queue.func)
> pending = cancel_work_sync(&info->queue);
> DPRINTK("fbcon: %s pending work\n", (pending ?
> "canceled" :
> diff --git a/drivers/video/fbdev/core/fbmem.c
> b/drivers/video/fbdev/core/fbmem.c
> index 609438d..645c6ac 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1593,10 +1593,8 @@ static int
> do_remove_conflicting_framebuffers(struct apertures_struct *a,
> int i, ret;
>
> /* check all firmware fbs and kick off if the base addr
> overlaps */
> - for (i = 0 ; i < FB_MAX; i++) {
> + for_each_registered_fb(i) {
> struct apertures_struct *gen_aper;
> - if (!registered_fb[i])
> - continue;
>
> if (!(registered_fb[i]->flags &
> FBINFO_MISC_FIRMWARE))
> continue;
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index aa74a22..fd31e6f 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -650,6 +650,10 @@ extern
Re: [PATCH v3 3/3] console/fbcon: Add support for deferred console takeover
On Tue, Jun 26, 2018 at 9:29 PM, Hans de Goede wrote:
>>> + for (i = 0; i < FB_MAX; i++) {
>>> + if (registered_fb[i])
>>> + fbcon_fb_registered(registered_fb[i]);
>>> + }
>> Simple git grep shows that we perhaps do
>>
>> #define for_each_registered_fbcon(i) ...
>>
>> (As an example see for_each_pci_bridge() how it's done there)
>
>
> This is probably a worthwhile cleanup for all the fbdev related
> code / drivers. But outside of the scope of this patchset.
True.
Yisheng, in case you are interested in doing such clean up...
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv2] lib/ratelimit: Lockless ratelimiting
On Tue, Jun 26, 2018 at 8:46 PM, Dmitry Safonov wrote:
> On Tue, 2018-06-26 at 20:04 +0300, Andy Shevchenko wrote
>> > #define RATELIMIT_STATE_INIT(name, interval_init, burst_init)
>> > {\
>> > - .lock =
>> > __RAW_SPIN_LOCK_UNLOCKED(name.lock), \
>>
>> name is now redundant, isn't it?
>
> It is. Worth to split on the second patch or keep callers changes in
> this patch?
For me sounds like a part of this change, though weakly tighten to the
main purpose.
Otherwise in the middle of the series you introduce some bogus stuff
(not sure if compiler or some static analyzers would warn about it).
>> > @@ -42,9 +41,10 @@ static inline void ratelimit_state_init(struct
>> > ratelimit_state *rs,
>> > {
>> > memset(rs, 0, sizeof(*rs));
>> >
>> > - raw_spin_lock_init(&rs->lock);
>> > rs->interval= interval;
>> > rs->burst = burst;
>> > + atomic_set(&rs->printed, 0);
>> > + atomic_set(&rs->missed, 0);
>>
>> Can it be
>>
>> *rs = RATELIMIT_STATE_INIT(interval, burst);
>>
>> ?
>>
>> (Yes, the '(struct ratelimit_state)' has to be added to macro to
>> allow this)
>
> Sure.
This part, by the way, potentially can be split into preparatory
patch. Please, double check if it possible to do this way.
>> > - if (rs->missed) {
>> > + if ((missed = atomic_xchg(&rs->missed, 0)))
>>
>> Perhaps
>>
>> missed = ...
>> if (missed)
>>
>> ?
>
> Ok, will change - checkpatch has warned me, but I thought it's just a
> preference than a rule.
In general, yes and no. In this case it would increase readability and
assignment inside conditionals is not the best practice.
>> Instead of casting, perhaps
>>
>> int missed = ...
>>
>> I think you already has a guard against going it below zero. Or I
>> missed something?
>
> No, I do:
> atomic_add_unless(&rs->missed, 1, -1);
>
> So, it's guard against overflow, but not against negative.
> That's why I do print it as unsigned.
Hmm...
If you increment the variable, it would become 2^n, then 2^n + 1, ...
which in unsigned form quite far from -1.
So, this check is against revolving the value.
Otherwise you are using atomic_t as unsigned type indeed.
But in case of use you assign signed to unsigned which would not
overflow, so, the casting is superfluous.
(and I would rather go with unsigned int type instead of unsigned if
it fits style of the module)
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/3] console/fbcon: Add support for deferred console takeover
On Tue, Jun 26, 2018 at 8:12 PM, Andy Shevchenko
wrote:
> On Tue, Jun 26, 2018 at 4:55 PM, Hans de Goede wrote:
>> + for (i = 0; i < FB_MAX; i++) {
>> + if (registered_fb[i])
>> + fbcon_fb_registered(registered_fb[i]);
>> + }
>
> Simple git grep shows that we perhaps do
>
> #define for_each_registered_fbcon(i) ...
(Or just ..._fb() since it looks like iteration over framebuffers, not
exactly consoles)
>
> (As an example see for_each_pci_bridge() how it's done there)
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/3] console/fbcon: Add support for deferred console takeover
On Tue, Jun 26, 2018 at 4:55 PM, Hans de Goede wrote:
> Currently fbcon claims fbdevs as soon as they are registered and takes over
> the console as soon as the first fbdev gets registered.
>
> This behavior is undesirable in cases where a smooth graphical bootup is
> desired, in such cases we typically want the contents of the framebuffer
> (typically a vendor logo) to stay in place as is.
>
> The current solution for this problem (on embedded systems) is to not
> enable fbcon.
>
> This commit adds a new FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER config option,
> which when enabled defers fbcon taking over the console from the dummy
> console until the first text is displayed on the console. Together with the
> "quiet" kernel commandline option, this allows fbcon to still be used
> together with a smooth graphical bootup, having it take over the console as
> soon as e.g. an error message is logged.
> + for (i = 0; i < FB_MAX; i++) {
> + if (registered_fb[i])
> + fbcon_fb_registered(registered_fb[i]);
> + }
Simple git grep shows that we perhaps do
#define for_each_registered_fbcon(i) ...
(As an example see for_each_pci_bridge() how it's done there)
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv2] lib/ratelimit: Lockless ratelimiting
On Tue, Jun 26, 2018 at 7:24 PM, Dmitry Safonov wrote:
> Currently ratelimit_state is protected with spin_lock. If the .lock is
> taken at the moment of ___ratelimit() call, the message is suppressed to
> make ratelimiting robust.
>
> That results in the following issue issue:
> CPU0 CPU1
> -- --
> printk_ratelimit() printk_ratelimit()
> | |
> try_spin_lock() try_spin_lock()
> | |
> time_is_before_jiffies() return 0; // suppress
>
> So, concurrent call of ___ratelimit() results in silently suppressing
> one of the messages, regardless if the limit is reached or not.
> And rc->missed is not increased in such case so the issue is covered
> from user.
>
> Convert ratelimiting to use atomic counters and drop spin_lock.
>
> Note: That might be unexpected, but with the first interval of messages
> storm one can print up to burst*2 messages. So, it doesn't guarantee
> that in *any* interval amount of messages is lesser than burst.
> But, that differs lightly from previous behavior where one can start
> burst=5 interval and print 4 messages on the last milliseconds of
> interval + new 5 messages from new interval (totally 9 messages in
> lesser than interval value):
>
>msg0 msg1-msg4 msg0-msg4
> | | |
> | | |
> |--o-o-|-o|--> (t)
> <--->
>Lesser than burst
> #define RATELIMIT_STATE_INIT(name, interval_init, burst_init) {
> \
> - .lock = __RAW_SPIN_LOCK_UNLOCKED(name.lock), \
name is now redundant, isn't it?
> .interval = interval_init,\
> .burst = burst_init, \
> + .printed= ATOMIC_INIT(0), \
> + .missed = ATOMIC_INIT(0), \
> }
>
> #define RATELIMIT_STATE_INIT_DISABLED \
> @@ -42,9 +41,10 @@ static inline void ratelimit_state_init(struct
> ratelimit_state *rs,
> {
> memset(rs, 0, sizeof(*rs));
>
> - raw_spin_lock_init(&rs->lock);
> rs->interval= interval;
> rs->burst = burst;
> + atomic_set(&rs->printed, 0);
> + atomic_set(&rs->missed, 0);
Can it be
*rs = RATELIMIT_STATE_INIT(interval, burst);
?
(Yes, the '(struct ratelimit_state)' has to be added to macro to allow this)
> }
> static inline void ratelimit_state_exit(struct ratelimit_state *rs)
> {
> + int missed;
> +
> if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE))
> return;
>
> - if (rs->missed) {
> + if ((missed = atomic_xchg(&rs->missed, 0)))
Perhaps
missed = ...
if (missed)
?
> pr_warn("%s: %d output lines suppressed due to
> ratelimiting\n",
> - current->comm, rs->missed);
> - rs->missed = 0;
> - }
> + current->comm, missed);
> }
> +static void ratelimit_end_interval(struct ratelimit_state *rs, const char
> *func)
> +{
> + rs->begin = jiffies;
> +
> + if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) {
> + unsigned missed = (unsigned)atomic_xchg(&rs->missed, 0);
> +
> + if (missed)
> + pr_warn("%s: %u callbacks suppressed\n", func,
> missed);
Instead of casting, perhaps
int missed = ...
I think you already has a guard against going it below zero. Or I
missed something?
> + }
> +}
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 12/21] drm: i2c: ch7006: use match_string() helper
On Thu, May 31, 2018 at 2:11 PM, Yisheng Xie wrote:
> match_string() returns the index of an array for a matching string,
> which can be used instead of open coded variant.
>
Reviewed-by: Andy Shevchenko
> Cc: David Airlie
> Cc: Yisheng Xie
> Cc: Daniel Vetter
> Cc: Arvind Yadav
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Yisheng Xie
> ---
> v2:
> - handle err case before normal case.
>
> drivers/gpu/drm/i2c/ch7006_drv.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/ch7006_drv.c
> b/drivers/gpu/drm/i2c/ch7006_drv.c
> index 544a8a2..9eefdfe 100644
> --- a/drivers/gpu/drm/i2c/ch7006_drv.c
> +++ b/drivers/gpu/drm/i2c/ch7006_drv.c
> @@ -464,16 +464,13 @@ static int ch7006_encoder_init(struct i2c_client
> *client,
> priv->chip_version = ch7006_read(client, CH7006_VERSION_ID);
>
> if (ch7006_tv_norm) {
> - for (i = 0; i < NUM_TV_NORMS; i++) {
> - if (!strcmp(ch7006_tv_norm_names[i], ch7006_tv_norm))
> {
> - priv->norm = i;
> - break;
> - }
> - }
> -
> - if (i == NUM_TV_NORMS)
> + i = match_string(ch7006_tv_norm_names,
> +NUM_TV_NORMS, ch7006_tv_norm);
> + if (i < 0)
> ch7006_err(client, "Invalid TV norm setting
> \"%s\".\n",
>ch7006_tv_norm);
> + else
> + priv->norm = i;
> }
>
> if (ch7006_scale >= 0 && ch7006_scale <= 2)
> --
> 1.7.12.4
>
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 11/21] drm/nouveau: use match_string() helper
On Thu, May 31, 2018 at 2:11 PM, Yisheng Xie wrote:
> match_string() returns the index of an array for a matching string,
> which can be used instead of open coded variant.
>
Reviewed-by: Andy Shevchenko
> Cc: Ben Skeggs
> Cc: David Airlie
> Cc: dri-devel@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Signed-off-by: Yisheng Xie
> ---
> v2:
> - handle err case before normal case - per Andy
>
> drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
> b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
> index 6d99f11..67ba2ac 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
> @@ -644,16 +644,13 @@ static int nv17_tv_create_resources(struct drm_encoder
> *encoder,
> int i;
>
> if (nouveau_tv_norm) {
> - for (i = 0; i < num_tv_norms; i++) {
> - if (!strcmp(nv17_tv_norm_names[i], nouveau_tv_norm)) {
> - tv_enc->tv_norm = i;
> - break;
> - }
> - }
> -
> - if (i == num_tv_norms)
> + i = match_string(nv17_tv_norm_names,
> +num_tv_norms, nouveau_tv_norm);
> + if (i < 0)
> NV_WARN(drm, "Invalid TV norm setting \"%s\"\n",
> nouveau_tv_norm);
> + else
> + tv_enc->tv_norm = i;
> }
>
> drm_mode_create_tv_properties(dev, num_tv_norms, nv17_tv_norm_names);
> --
> 1.7.12.4
>
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fb_omap: add gpiolib dependency
On Thu, May 31, 2018 at 12:49 AM, Arnd Bergmann wrote: > drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c: In function 'hdmi_probe_of': > drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c:584:2: error: implicit > declaration of function 'of_node_put'; did you mean 'node_set'? > [-Werror=implicit-function-declaration] > drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c: In function 'hdmi_probe_of': > drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c:554:2: error: implicit > declaration of function 'of_node_put'; did you mean 'node_set'? > [-Werror=implicit-function-declaration] > Rather than fixing up each one individually, this just marks all of it > as depending on GPIOLIB. Hmm... But the OF stuff is not part of GPIOLIB. -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 24/33] drm: use match_string() helper
On Mon, May 21, 2018 at 2:58 PM, Yisheng Xie wrote: > match_string() returns the index of an array for a matching string, > which can be used intead of open coded variant. https://patchwork.kernel.org/patch/10382377/ > Cc: Gustavo Padovan > Cc: Maarten Lankhorst > Cc: Sean Paul > Cc: David Airlie > Cc: dri-devel@lists.freedesktop.org -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 22/33] drm/i915: use match_string() helper
On Mon, May 21, 2018 at 2:57 PM, Yisheng Xie wrote: > match_string() returns the index of an array for a matching string, > which can be used intead of open coded variant. https://patchwork.kernel.org/patch/10382323/ > Cc: Jani Nikula > Cc: Joonas Lahtinen > Cc: Rodrigo Vivi > Cc: David Airlie > Cc: intel-...@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 21/33] drm/nouveau: use match_string() helper
On Mon, May 21, 2018 at 2:57 PM, Yisheng Xie wrote:
> match_string() returns the index of an array for a matching string,
> which can be used intead of open coded variant.
> if (nouveau_tv_norm) {
> + i = match_string(nv17_tv_norm_names,
> +num_tv_norms, nouveau_tv_norm);
Same comment for logical split, 2nd parameter looks better on the previous line.
> + if (i >= 0)
> + tv_enc->tv_norm = i;
> + else
> NV_WARN(drm, "Invalid TV norm setting \"%s\"\n",
> nouveau_tv_norm);
I would rather go with
if (i < 0)
NV_WARN
else
...
> }
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 20/33] video: fbdev: pxafb: use match_string() helper
On Mon, May 21, 2018 at 2:57 PM, Yisheng Xie wrote: > match_string() returns the index of an array for a matching string, > which can be used intead of open coded variant. https://patchwork.kernel.org/patch/10378815/ > Cc: Bartlomiej Zolnierkiewicz > Cc: Arvind Yadav > Cc: dri-devel@lists.freedesktop.org > linux-fb...@vger.kernel.org -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/3] drm/tinydrm: new driver for ILI9341 display panels
On Tue, May 15, 2018 at 4:43 AM, David Lechner wrote: > This adds a new driver for display panels that use the Ilitek ILI9341 > controller. It currently supports a single display panel, namely > the YX240QV29-T (e.g. Adafruit 2.4" TFT). > > The init sequence is from the Adafruit Python library for the ILI9341 > controller. https://github.com/adafruit/Adafruit_Python_ILI9341 Some minor style nitpicks, otherwise LGTM Reviewed-by: Andy Shevchenko > > Signed-off-by: David Lechner > --- > MAINTAINERS | 6 + > drivers/gpu/drm/tinydrm/Kconfig | 10 ++ > drivers/gpu/drm/tinydrm/Makefile | 1 + > drivers/gpu/drm/tinydrm/ili9341.c | 239 ++ > 4 files changed, 256 insertions(+) > create mode 100644 drivers/gpu/drm/tinydrm/ili9341.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index bc219de9cbee..ffa099abbd79 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -4480,6 +4480,12 @@ S: Maintained > F: drivers/gpu/drm/tinydrm/ili9225.c > F: Documentation/devicetree/bindings/display/ilitek,ili9225.txt > > +DRM DRIVER FOR ILITEK ILI9341 PANELS > +M: David Lechner > +S: Maintained > +F: drivers/gpu/drm/tinydrm/ili9341.c > +F: Documentation/devicetree/bindings/display/ilitek,ili9341.txt > + > DRM DRIVER FOR INTEL I810 VIDEO CARDS > S: Orphan / Obsolete > F: drivers/gpu/drm/i810/ > diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig > index 4592a5e3f20b..7a8008b0783f 100644 > --- a/drivers/gpu/drm/tinydrm/Kconfig > +++ b/drivers/gpu/drm/tinydrm/Kconfig > @@ -20,6 +20,16 @@ config TINYDRM_ILI9225 > > If M is selected the module will be called ili9225. > > +config TINYDRM_ILI9341 > + tristate "DRM support for ILI9341 display panels" > + depends on DRM_TINYDRM && SPI Can't we do something like if SPI ... endif ? > + select TINYDRM_MIPI_DBI > + help > + DRM driver for the following Ilitek ILI9341 panels: > + * YX240QV29-T 2.4" 240x320 TFT (Adafruit 2.4") > + > + If M is selected the module will be called ili9341. > + > config TINYDRM_MI0283QT > tristate "DRM support for MI0283QT" > depends on DRM_TINYDRM && SPI > diff --git a/drivers/gpu/drm/tinydrm/Makefile > b/drivers/gpu/drm/tinydrm/Makefile > index 49a111929724..14d99080665a 100644 > --- a/drivers/gpu/drm/tinydrm/Makefile > +++ b/drivers/gpu/drm/tinydrm/Makefile > @@ -5,6 +5,7 @@ obj-$(CONFIG_TINYDRM_MIPI_DBI) += mipi-dbi.o > > # Displays > obj-$(CONFIG_TINYDRM_ILI9225) += ili9225.o > +obj-$(CONFIG_TINYDRM_ILI9341) += ili9341.o > obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o > obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o > obj-$(CONFIG_TINYDRM_ST7586) += st7586.o > diff --git a/drivers/gpu/drm/tinydrm/ili9341.c > b/drivers/gpu/drm/tinydrm/ili9341.c > new file mode 100644 > index ..2ce4244a68c3 > --- /dev/null > +++ b/drivers/gpu/drm/tinydrm/ili9341.c > @@ -0,0 +1,239 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * DRM driver for Ilitek ILI9341 panels > + * > + * Copyright 2018 David Lechner > + * > + * Based on mi0283qt.c: > + * Copyright 2016 Noralf Trønnes > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include Can it be in order? > +#include > +#include > +#include > + > +#define ILI9341_FRMCTR10xb1 > +#define ILI9341_DISCTRL0xb6 > +#define ILI9341_ETMOD 0xb7 > + > +#define ILI9341_PWCTRL10xc0 > +#define ILI9341_PWCTRL20xc1 > +#define ILI9341_VMCTRL10xc5 > +#define ILI9341_VMCTRL20xc7 > +#define ILI9341_PWCTRLA0xcb > +#define ILI9341_PWCTRLB0xcf > + > +#define ILI9341_PGAMCTRL 0xe0 > +#define ILI9341_NGAMCTRL 0xe1 > +#define ILI9341_DTCTRLA0xe8 > +#define ILI9341_DTCTRLB0xea > +#define ILI9341_PWRSEQ 0xed > + > +#define ILI9341_EN3GAM 0xf2 > +#define ILI9341_PUMPCTRL 0xf7 > + > +#define ILI9341_MADCTL_BGR BIT(3) > +#define ILI9341_MADCTL_MV BIT(5) > +#define ILI9341_MADCTL_MX BIT(6) > +#define ILI9341_MADCTL_MY BIT(7) > + > +static void yx240qv29_enable(struct drm_simple_display_pipe *pipe, > +struct drm_crtc_state *crtc_state, > +struct drm_plane_state *plane_state) >
[PATCH v1] drm: panel-orientation-quirks: Convert to use match_string() helper
The new helper returns index of the matching string in an array.
We are going to use it here.
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/drm_panel_orientation_quirks.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel_orientation_quirks.c
b/drivers/gpu/drm/drm_panel_orientation_quirks.c
index caebddda8bce..fe9c6c731e87 100644
--- a/drivers/gpu/drm/drm_panel_orientation_quirks.c
+++ b/drivers/gpu/drm/drm_panel_orientation_quirks.c
@@ -172,10 +172,9 @@ int drm_get_panel_orientation_quirk(int width, int height)
if (!bios_date)
continue;
- for (i = 0; data->bios_dates[i]; i++) {
- if (!strcmp(data->bios_dates[i], bios_date))
- return data->orientation;
- }
+ i = match_string(data->bios_dates, -1, bios_date);
+ if (i >= 0)
+ return data->orientation;
}
return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
--
2.17.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1] i915: Convert to use match_string() helper
The new helper returns index of the matching string in an array.
We are going to use it here.
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/i915/intel_pipe_crc.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 4f367c16e9e5..39a4e4edda07 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -766,13 +766,12 @@ display_crc_ctl_parse_object(const char *buf, enum
intel_pipe_crc_object *o)
{
int i;
- for (i = 0; i < ARRAY_SIZE(pipe_crc_objects); i++)
- if (!strcmp(buf, pipe_crc_objects[i])) {
- *o = i;
- return 0;
- }
+ i = match_string(pipe_crc_objects, ARRAY_SIZE(pipe_crc_objects), buf);
+ if (i < 0)
+ return i;
- return -EINVAL;
+ *o = i;
+ return 0;
}
static int display_crc_ctl_parse_pipe(struct drm_i915_private *dev_priv,
@@ -798,13 +797,12 @@ display_crc_ctl_parse_source(const char *buf, enum
intel_pipe_crc_source *s)
return 0;
}
- for (i = 0; i < ARRAY_SIZE(pipe_crc_sources); i++)
- if (!strcmp(buf, pipe_crc_sources[i])) {
- *s = i;
- return 0;
- }
+ i = match_string(pipe_crc_sources, ARRAY_SIZE(pipe_crc_sources), buf);
+ if (i < 0)
+ return i;
- return -EINVAL;
+ *s = i;
+ return 0;
}
static int display_crc_ctl_parse(struct drm_i915_private *dev_priv,
--
2.17.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/dp/mst: Fix off-by-one typo when dump payload table
It seems there is a classical off-by-one typo from the beginning
when commit
ad7f8a1f9ced ("drm/helper: add Displayport multi-stream helper (v0.6)")
introduced a new helper.
Fix a typo by introducing a macro constant.
Cc: Dave Airlie
Signed-off-by: Andy Shevchenko
---
- use macro for buffer length on stack
drivers/gpu/drm/drm_dp_mst_topology.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 6fac4129e6a2..658830620ca3 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2941,12 +2941,14 @@ static void drm_dp_mst_dump_mstb(struct seq_file *m,
}
}
+#define DP_PAYLOAD_TABLE_SIZE 64
+
static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
char *buf)
{
int i;
- for (i = 0; i < 64; i += 16) {
+ for (i = 0; i < DP_PAYLOAD_TABLE_SIZE; i += 16) {
if (drm_dp_dpcd_read(mgr->aux,
DP_PAYLOAD_TABLE_UPDATE_STATUS + i,
&buf[i], 16) != 16)
@@ -3015,7 +3017,7 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
mutex_lock(&mgr->lock);
if (mgr->mst_primary) {
- u8 buf[64];
+ u8 buf[DP_PAYLOAD_TABLE_SIZE];
int ret;
ret = drm_dp_dpcd_read(mgr->aux, DP_DPCD_REV, buf,
DP_RECEIVER_CAP_SIZE);
@@ -3033,8 +3035,7 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
seq_printf(m, " revision: hw: %x.%x sw: %x.%x\n",
buf[0x9] >> 4, buf[0x9] & 0xf, buf[0xa], buf[0xb]);
if (dump_dp_payload_table(mgr, buf))
- seq_printf(m, "payload table: %*ph\n", 63, buf);
-
+ seq_printf(m, "payload table: %*ph\n",
DP_PAYLOAD_TABLE_SIZE, buf);
}
mutex_unlock(&mgr->lock);
--
2.16.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] i915: Re-use DEFINE_SHOW_ATTRIBUTE() macro
...instead of open coding file operations followed by custom ->open()
callbacks per each attribute.
Reviewed-by: Chris Wilson
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/i915/gvt/debugfs.c | 13 +--
drivers/gpu/drm/i915/i915_debugfs.c | 76 ++---
2 files changed, 12 insertions(+), 77 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c
b/drivers/gpu/drm/i915/gvt/debugfs.c
index 32a66dfdf112..f7d0078eb61b 100644
--- a/drivers/gpu/drm/i915/gvt/debugfs.c
+++ b/drivers/gpu/drm/i915/gvt/debugfs.c
@@ -122,18 +122,7 @@ static int vgpu_mmio_diff_show(struct seq_file *s, void
*unused)
seq_printf(s, "Total: %d, Diff: %d\n", param.total, param.diff);
return 0;
}
-
-static int vgpu_mmio_diff_open(struct inode *inode, struct file *file)
-{
- return single_open(file, vgpu_mmio_diff_show, inode->i_private);
-}
-
-static const struct file_operations vgpu_mmio_diff_fops = {
- .open = vgpu_mmio_diff_open,
- .read = seq_read,
- .llseek = seq_lseek,
- .release= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(vgpu_mmio_diff);
/**
* intel_gvt_debugfs_add_vgpu - register debugfs entries for a vGPU
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 298a3aa9513b..5378863e3238 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3562,7 +3562,8 @@ static ssize_t i915_displayport_test_active_write(struct
file *file,
static int i915_displayport_test_active_show(struct seq_file *m, void *data)
{
- struct drm_device *dev = m->private;
+ struct drm_i915_private *dev_priv = m->private;
+ struct drm_device *dev = &dev_priv->drm;
struct drm_connector *connector;
struct drm_connector_list_iter conn_iter;
struct intel_dp *intel_dp;
@@ -3596,10 +3597,8 @@ static int i915_displayport_test_active_show(struct
seq_file *m, void *data)
static int i915_displayport_test_active_open(struct inode *inode,
struct file *file)
{
- struct drm_i915_private *dev_priv = inode->i_private;
-
return single_open(file, i915_displayport_test_active_show,
- &dev_priv->drm);
+ inode->i_private);
}
static const struct file_operations i915_displayport_test_active_fops = {
@@ -3613,7 +3612,8 @@ static const struct file_operations
i915_displayport_test_active_fops = {
static int i915_displayport_test_data_show(struct seq_file *m, void *data)
{
- struct drm_device *dev = m->private;
+ struct drm_i915_private *dev_priv = m->private;
+ struct drm_device *dev = &dev_priv->drm;
struct drm_connector *connector;
struct drm_connector_list_iter conn_iter;
struct intel_dp *intel_dp;
@@ -3652,26 +3652,12 @@ static int i915_displayport_test_data_show(struct
seq_file *m, void *data)
return 0;
}
-static int i915_displayport_test_data_open(struct inode *inode,
- struct file *file)
-{
- struct drm_i915_private *dev_priv = inode->i_private;
-
- return single_open(file, i915_displayport_test_data_show,
- &dev_priv->drm);
-}
-
-static const struct file_operations i915_displayport_test_data_fops = {
- .owner = THIS_MODULE,
- .open = i915_displayport_test_data_open,
- .read = seq_read,
- .llseek = seq_lseek,
- .release = single_release
-};
+DEFINE_SHOW_ATTRIBUTE(i915_displayport_test_data);
static int i915_displayport_test_type_show(struct seq_file *m, void *data)
{
- struct drm_device *dev = m->private;
+ struct drm_i915_private *dev_priv = m->private;
+ struct drm_device *dev = &dev_priv->drm;
struct drm_connector *connector;
struct drm_connector_list_iter conn_iter;
struct intel_dp *intel_dp;
@@ -3698,23 +3684,7 @@ static int i915_displayport_test_type_show(struct
seq_file *m, void *data)
return 0;
}
-
-static int i915_displayport_test_type_open(struct inode *inode,
- struct file *file)
-{
- struct drm_i915_private *dev_priv = inode->i_private;
-
- return single_open(file, i915_displayport_test_type_show,
- &dev_priv->drm);
-}
-
-static const struct file_operations i915_displayport_test_type_fops = {
- .owner = THIS_MODULE,
- .open = i915_displayport_test_type_open,
- .read = seq_read,
- .llseek = seq_lseek,
- .release = single_release
-};
+DEFINE_SHOW_ATTRIBUTE(i915_displayport_test_type);
static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
{
@@ -4875,19 +4845,7 @@ static int i915_dpcd_show(struct seq_file *m, void *data)
return 0;
}
-
-static int i
Re: [PATCH v1] drm/dp/mst: Fix off-by-one typo when dump payload table
On Thu, 2018-01-11 at 17:33 +0200, Andy Shevchenko wrote:
> It seems there is a classical off-by-one typo from the beginning
> when commit
>
> ad7f8a1f9ced ("drm/helper: add Displayport multi-stream helper
> (v0.6)")
>
> introduced a new helper.
>
> Fix a typo by introducing a macro constant.
Any comment on this?
>
> Cc: Dave Airlie
> Signed-off-by: Andy Shevchenko
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 70dcfa58d3c2..51c5fc11f852 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2936,12 +2936,14 @@ static void drm_dp_mst_dump_mstb(struct
> seq_file *m,
> }
> }
>
> +#define DP_PAYLOAD_TABLE_SIZE64
> +
> static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr
> *mgr,
> char *buf)
> {
> int i;
>
> - for (i = 0; i < 64; i += 16) {
> + for (i = 0; i < DP_PAYLOAD_TABLE_SIZE; i += 16) {
> if (drm_dp_dpcd_read(mgr->aux,
>DP_PAYLOAD_TABLE_UPDATE_STATUS +
> i,
>&buf[i], 16) != 16)
> @@ -3028,8 +3030,7 @@ void drm_dp_mst_dump_topology(struct seq_file
> *m,
> seq_printf(m, " revision: hw: %x.%x sw: %x.%x\n",
> buf[0x9] >> 4, buf[0x9] & 0xf, buf[0xa],
> buf[0xb]);
> if (dump_dp_payload_table(mgr, buf))
> - seq_printf(m, "payload table: %*ph\n", 63,
> buf);
> -
> + seq_printf(m, "payload table: %*ph\n",
> DP_PAYLOAD_TABLE_SIZE, buf);
> }
>
> mutex_unlock(&mgr->lock);
--
Andy Shevchenko
Intel Finland Oy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 06/10] pwm: add PWM modes
On Thu, Feb 22, 2018 at 2:01 PM, Claudiu Beznea wrote: > Add PWM normal and complementary modes. > +- PWM_DTMODE_COMPLEMENTARY: PWM complementary working mode (for PWM > +channels two outputs); if not specified, the default for PWM channel will > +be used What DT stands for? -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 5/6] extcon: add possibility to get extcon device by OF node
On Wed, Feb 21, 2018 at 10:55 AM, Andrzej Hajda wrote:
> Since extcon property is not allowed in DT, extcon subsystem requires
> another way to get extcon device. Lets try the simplest approach - get
> edev by of_node.
> +/*
> + * extcon_get_edev_by_of_node - Get the extcon device from devicetree.
> + * @node : OF node identyfying edev
> + *
> + * Return the pointer of extcon device if success or ERR_PTR(err) if fail.
> + */
> +struct extcon_dev *extcon_get_edev_by_of_node(struct device_node *node)
First of all, the all other similar cases use "_by_node" in the name.
Second, it's not _get_, it's _find_.
> +{
> + struct extcon_dev *edev;
> +
> + mutex_lock(&extcon_dev_list_lock);
> + list_for_each_entry(edev, &extcon_dev_list, entry)
> + if (edev->dev.parent && edev->dev.parent->of_node == node)
> + goto out;
> + edev = ERR_PTR(-EPROBE_DEFER);
> +out:
> + mutex_unlock(&extcon_dev_list_lock);
> +
> + return edev;
Can't it be done using bus_find_device()?
> +}
See good example in i2c-core-of.c
of_find_i2c_adapter_by_node()
of_get_i2c_adapter_by_node()
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1] i915: Re-use DEFINE_SHOW_ATTRIBUTE() macro
...instead of open coding file operations followed by custom ->open()
callbacks per each attribute.
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/i915/gvt/debugfs.c | 13 +--
drivers/gpu/drm/i915/i915_debugfs.c | 76 ++---
2 files changed, 12 insertions(+), 77 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c
b/drivers/gpu/drm/i915/gvt/debugfs.c
index 32a66dfdf112..f7d0078eb61b 100644
--- a/drivers/gpu/drm/i915/gvt/debugfs.c
+++ b/drivers/gpu/drm/i915/gvt/debugfs.c
@@ -122,18 +122,7 @@ static int vgpu_mmio_diff_show(struct seq_file *s, void
*unused)
seq_printf(s, "Total: %d, Diff: %d\n", param.total, param.diff);
return 0;
}
-
-static int vgpu_mmio_diff_open(struct inode *inode, struct file *file)
-{
- return single_open(file, vgpu_mmio_diff_show, inode->i_private);
-}
-
-static const struct file_operations vgpu_mmio_diff_fops = {
- .open = vgpu_mmio_diff_open,
- .read = seq_read,
- .llseek = seq_lseek,
- .release= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(vgpu_mmio_diff);
/**
* intel_gvt_debugfs_add_vgpu - register debugfs entries for a vGPU
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 960302668649..3e44b2ddf592 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3506,7 +3506,8 @@ static ssize_t i915_displayport_test_active_write(struct
file *file,
static int i915_displayport_test_active_show(struct seq_file *m, void *data)
{
- struct drm_device *dev = m->private;
+ struct drm_i915_private *dev_priv = m->private;
+ struct drm_device *dev = &dev_priv->drm;
struct drm_connector *connector;
struct drm_connector_list_iter conn_iter;
struct intel_dp *intel_dp;
@@ -3540,10 +3541,8 @@ static int i915_displayport_test_active_show(struct
seq_file *m, void *data)
static int i915_displayport_test_active_open(struct inode *inode,
struct file *file)
{
- struct drm_i915_private *dev_priv = inode->i_private;
-
return single_open(file, i915_displayport_test_active_show,
- &dev_priv->drm);
+ inode->i_private);
}
static const struct file_operations i915_displayport_test_active_fops = {
@@ -3557,7 +3556,8 @@ static const struct file_operations
i915_displayport_test_active_fops = {
static int i915_displayport_test_data_show(struct seq_file *m, void *data)
{
- struct drm_device *dev = m->private;
+ struct drm_i915_private *dev_priv = m->private;
+ struct drm_device *dev = &dev_priv->drm;
struct drm_connector *connector;
struct drm_connector_list_iter conn_iter;
struct intel_dp *intel_dp;
@@ -3596,26 +3596,12 @@ static int i915_displayport_test_data_show(struct
seq_file *m, void *data)
return 0;
}
-static int i915_displayport_test_data_open(struct inode *inode,
- struct file *file)
-{
- struct drm_i915_private *dev_priv = inode->i_private;
-
- return single_open(file, i915_displayport_test_data_show,
- &dev_priv->drm);
-}
-
-static const struct file_operations i915_displayport_test_data_fops = {
- .owner = THIS_MODULE,
- .open = i915_displayport_test_data_open,
- .read = seq_read,
- .llseek = seq_lseek,
- .release = single_release
-};
+DEFINE_SHOW_ATTRIBUTE(i915_displayport_test_data);
static int i915_displayport_test_type_show(struct seq_file *m, void *data)
{
- struct drm_device *dev = m->private;
+ struct drm_i915_private *dev_priv = m->private;
+ struct drm_device *dev = &dev_priv->drm;
struct drm_connector *connector;
struct drm_connector_list_iter conn_iter;
struct intel_dp *intel_dp;
@@ -3642,23 +3628,7 @@ static int i915_displayport_test_type_show(struct
seq_file *m, void *data)
return 0;
}
-
-static int i915_displayport_test_type_open(struct inode *inode,
- struct file *file)
-{
- struct drm_i915_private *dev_priv = inode->i_private;
-
- return single_open(file, i915_displayport_test_type_show,
- &dev_priv->drm);
-}
-
-static const struct file_operations i915_displayport_test_type_fops = {
- .owner = THIS_MODULE,
- .open = i915_displayport_test_type_open,
- .read = seq_read,
- .llseek = seq_lseek,
- .release = single_release
-};
+DEFINE_SHOW_ATTRIBUTE(i915_displayport_test_type);
static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
{
@@ -4812,19 +4782,7 @@ static int i915_dpcd_show(struct seq_file *m, void *data)
return 0;
}
-
-static int i915_dpcd_open(struct inode *inode,
Re: [RESEND][PATCH] video: fbdev: atmel_lcdfb: convert to use GPIO descriptors
On Mon, Feb 5, 2018 at 10:47 AM, Ludovic Desroches
wrote:
> Use GPIO descriptors instead of relying on the old method.
Reviewed-by: Andy Shevchenko
Though few nitpicks below.
> --- a/drivers/video/fbdev/atmel_lcdfb.c
> +++ b/drivers/video/fbdev/atmel_lcdfb.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
I think you forgot to remove of_gpio.h.
> #include
> #include
> #include
> struct device_node *display_np;
> struct device_node *timings_np;
> struct display_timings *timings;
> - enum of_gpio_flags flags;
> struct atmel_lcdfb_power_ctrl_gpio *og;
> bool is_gpio_power = false;
> int ret = -ENOENT;
> - int i, gpio;
> + int i;
> + struct gpio_desc *gpiod;
I would rather preserve reversed tree style, i.e. put longer line upper.
> + for (i = 0; i < gpiod_count(dev, "atmel,power-control"); i++) {
> + gpiod = devm_gpiod_get_index_optional(dev,
> + "atmel,power-control", i, GPIOD_ASIS);
> + if (!gpiod)
> continue;
What about IS_ERR() case?
>
> og = devm_kzalloc(dev, sizeof(*og), GFP_KERNEL);
> if (!og)
> goto put_display_node;
>
> + og->gpiod = gpiod;
> is_gpio_power = true;
>
> + ret = gpiod_direction_output(gpiod,
> gpiod_is_active_low(gpiod));
> if (ret) {
I'm not sure this will be needed if you check IS_ERR() above.
> + dev_err(dev, "set direction output gpio
> atmel,power-control[%d] failed\n", i);
> goto put_display_node;
> }
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v18 03/10] video: backlight: Add of_find_backlight helper in backlight.c
On Mon, Jan 22, 2018 at 4:51 PM, Meghana Madhyastha wrote: > Add of_find_backlight, a helper function which is a generic version > of tinydrm_of_find_backlight that can be used by other drivers to avoid > repetition of code and simplify things. > +struct backlight_device *of_find_backlight(struct device *dev) It looks strange that of_ prefixed function takes struct device instead of struct device_node. -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 08/15] drm/i2c: tda998x: Remove duplicate NULL check
On Thu, 2018-01-18 at 17:21 +0200, Ville Syrjälä wrote:
> On Tue, Oct 31, 2017 at 05:03:43PM +, Russell King - ARM Linux
> wrote:
> > On Tue, Oct 31, 2017 at 04:21:42PM +0200, Andy Shevchenko wrote:
> > > Since i2c_unregister_device() became NULL-aware we may remove
> > > duplicate
> > > NULL check.
> > >
> > > Cc: Russell King
> >
> > Acked-by: Russell King
>
> commit 7b43dd19c9b1 ("i2c: Make i2c_unregister_device() NULL-aware")
> seems to be the thing that makes this possible. So these three
> patches lgtm -> pushed to drm-misc-next.
>
Thanks, Ville!
> >
> > Thanks.
> >
> > > Cc: David Airlie
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Andy Shevchenko
> > > ---
> > > drivers/gpu/drm/i2c/tda998x_drv.c | 3 +--
> > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > index 4d1f45acf2cd..7a349e85f964 100644
> > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > @@ -1602,8 +1602,7 @@ static int tda998x_create(struct i2c_client
> > > *client, struct tda998x_priv *priv)
> > > /* if encoder_init fails, the encoder slave is never
> > > registered,
> > >* so cleanup here:
> > >*/
> > > - if (priv->cec)
> > > - i2c_unregister_device(priv->cec);
> > > + i2c_unregister_device(priv->cec);
> > > return -ENXIO;
> > > }
> > >
> > > --
> > > 2.14.2
> > >
> >
> > --
> > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> > FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down
> > 630kbps up
> > According to speedtest.net: 8.21Mbps down 510kbps up
> > ___
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
--
Andy Shevchenko
Intel Finland Oy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1] drm/dp/mst: Fix off-by-one typo when dump payload table
It seems there is a classical off-by-one typo from the beginning
when commit
ad7f8a1f9ced ("drm/helper: add Displayport multi-stream helper (v0.6)")
introduced a new helper.
Fix a typo by introducing a macro constant.
Cc: Dave Airlie
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/drm_dp_mst_topology.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 70dcfa58d3c2..51c5fc11f852 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2936,12 +2936,14 @@ static void drm_dp_mst_dump_mstb(struct seq_file *m,
}
}
+#define DP_PAYLOAD_TABLE_SIZE 64
+
static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
char *buf)
{
int i;
- for (i = 0; i < 64; i += 16) {
+ for (i = 0; i < DP_PAYLOAD_TABLE_SIZE; i += 16) {
if (drm_dp_dpcd_read(mgr->aux,
DP_PAYLOAD_TABLE_UPDATE_STATUS + i,
&buf[i], 16) != 16)
@@ -3028,8 +3030,7 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
seq_printf(m, " revision: hw: %x.%x sw: %x.%x\n",
buf[0x9] >> 4, buf[0x9] & 0xf, buf[0xa], buf[0xb]);
if (dump_dp_payload_table(mgr, buf))
- seq_printf(m, "payload table: %*ph\n", 63, buf);
-
+ seq_printf(m, "payload table: %*ph\n",
DP_PAYLOAD_TABLE_SIZE, buf);
}
mutex_unlock(&mgr->lock);
--
2.15.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/radeon: use raw buffer printk specifier
On Thu, 2017-12-21 at 12:04 +0200, Dmitry Rozhkov wrote:
> printk format strings accepting a single subsequent argument
> are shorter thus easier to read.
>
> Instead of having format strings accepting 3 different arguments
> pointing to first 3 bytes of the same buffer rewrite the format
> string to accept only one argument - the buffer - with "%3ph"
> specifier.
>
+Cc maintainers
> Signed-off-by: Dmitry Rozhkov
> Suggested-by: Andy Shevchenko
> ---
> drivers/gpu/drm/radeon/radeon_dp_mst.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> index 183b4b482138..ca2bcfb32935 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -718,7 +718,7 @@ radeon_dp_mst_check_status(struct radeon_connector
> *radeon_connector)
> DP_SINK_COUNT_ESI, esi, 8);
> go_again:
> if (dret == 8) {
> - DRM_DEBUG_KMS("got esi %02x %02x %02x\n",
> esi[0], esi[1], esi[2]);
> + DRM_DEBUG_KMS("got esi %3ph\n", esi);
> ret = drm_dp_mst_hpd_irq(&radeon_connector-
> >mst_mgr, esi, &handled);
>
> if (handled) {
> @@ -733,7 +733,7 @@ radeon_dp_mst_check_status(struct radeon_connector
> *radeon_connector)
> dret =
> drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
> DP_SINK_COUNT
> _ESI, esi, 8);
> if (dret == 8) {
> - DRM_DEBUG_KMS("got esi2 %02x
> %02x %02x\n", esi[0], esi[1], esi[2]);
> + DRM_DEBUG_KMS("got esi2
> %3ph\n", esi);
> goto go_again;
> }
> } else
--
Andy Shevchenko
Intel Finland Oy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
On Tue, Dec 19, 2017 at 8:15 PM, Joe Perches wrote:
> Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible.
>
> Done with perl script:
>
> $ git grep -w --name-only DEVICE_ATTR | \
> xargs perl -i -e 'local $/; while (<>) {
> s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(\s*S_IRUGO\s*\|\s*S_IWUSR|\s*S_IWUSR\s*\|\s*S_IRUGO\s*|\s*0644\s*)\)?\s*,\s*\1_show\s*,\s*\1_store\s*\)/DEVICE_ATTR_RW(\1)/g;
> print;}'
> drivers/platform/x86/compal-laptop.c | 18 +--
> --- a/drivers/platform/x86/compal-laptop.c
> +++ b/drivers/platform/x86/compal-laptop.c
> @@ -679,18 +679,12 @@ static int bat_writeable_property(struct power_supply
> *psy,
> /* == */
> /* Driver Globals */
> /* == */
> -static DEVICE_ATTR(wake_up_pme,
> - 0644, wake_up_pme_show, wake_up_pme_store);
> -static DEVICE_ATTR(wake_up_modem,
> - 0644, wake_up_modem_show, wake_up_modem_store);
> -static DEVICE_ATTR(wake_up_lan,
> - 0644, wake_up_lan_show, wake_up_lan_store);
> -static DEVICE_ATTR(wake_up_wlan,
> - 0644, wake_up_wlan_show,wake_up_wlan_store);
> -static DEVICE_ATTR(wake_up_key,
> - 0644, wake_up_key_show, wake_up_key_store);
> -static DEVICE_ATTR(wake_up_mouse,
> - 0644, wake_up_mouse_show, wake_up_mouse_store);
> +static DEVICE_ATTR_RW(wake_up_pme);
> +static DEVICE_ATTR_RW(wake_up_modem);
> +static DEVICE_ATTR_RW(wake_up_lan);
> +static DEVICE_ATTR_RW(wake_up_wlan);
> +static DEVICE_ATTR_RW(wake_up_key);
> +static DEVICE_ATTR_RW(wake_up_mouse);
Acked-by: Andy Shevchenko
for PDx86 bits.
Have to say that while it doesn't change the attributes here, it might
require still to be revisited by security people, if they wish.
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [trivial PATCH] treewide: Align function definition open/close braces
On Mon, Dec 18, 2017 at 2:28 AM, Joe Perches wrote:
> Some functions definitions have either the initial open brace and/or
> the closing brace outside of column 1.
>
> Move those braces to column 1.
>
> This allows various function analyzers like gnu complexity to work
> properly for these modified functions.
>
> Miscellanea:
>
> o Remove extra trailing ; and blank line from xfs_agf_verify
>
> Signed-off-by: Joe Perches
> drivers/platform/x86/eeepc-laptop.c | 2 +-
> diff --git a/drivers/platform/x86/eeepc-laptop.c
> b/drivers/platform/x86/eeepc-laptop.c
> index 5a681962899c..4c38904a8a32 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -492,7 +492,7 @@ static void eeepc_platform_exit(struct eeepc_laptop
> *eeepc)
> * potentially bad time, such as a timer interrupt.
> */
> static void tpd_led_update(struct work_struct *work)
> - {
> +{
> struct eeepc_laptop *eeepc;
>
> eeepc = container_of(work, struct eeepc_laptop, tpd_led_work);
> diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
> index a319bf1e49de..ef5c16dfabfa 100644
Acked-by: Andy Shevchenko
for PDx86 changes.
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1 07/15] drm/i2c/sil164: Remove duplicate NULL check
Since i2c_unregister_device() became NULL-aware we may remove duplicate
NULL check.
Cc: David Airlie
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/i2c/sil164_drv.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i2c/sil164_drv.c b/drivers/gpu/drm/i2c/sil164_drv.c
index ecaa58757529..c52d7a3af786 100644
--- a/drivers/gpu/drm/i2c/sil164_drv.c
+++ b/drivers/gpu/drm/i2c/sil164_drv.c
@@ -326,8 +326,7 @@ sil164_encoder_destroy(struct drm_encoder *encoder)
{
struct sil164_priv *priv = to_sil164_priv(encoder);
- if (priv->duallink_slave)
- i2c_unregister_device(priv->duallink_slave);
+ i2c_unregister_device(priv->duallink_slave);
kfree(priv);
drm_i2c_encoder_destroy(encoder);
--
2.14.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1 06/15] drm/bridge: analogix-anx78xx: Remove duplicate NULL check
Since i2c_unregister_device() became NULL-aware we may remove duplicate
NULL check.
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: David Airlie
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/bridge/analogix-anx78xx.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c
b/drivers/gpu/drm/bridge/analogix-anx78xx.c
index 9385eb0b1ee4..e2925e425f90 100644
--- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
@@ -1303,8 +1303,7 @@ static void unregister_i2c_dummy_clients(struct anx78xx
*anx78xx)
unsigned int i;
for (i = 0; i < ARRAY_SIZE(anx78xx->i2c_dummy); i++)
- if (anx78xx->i2c_dummy[i])
- i2c_unregister_device(anx78xx->i2c_dummy[i]);
+ i2c_unregister_device(anx78xx->i2c_dummy[i]);
}
static const struct regmap_config anx78xx_regmap_config = {
--
2.14.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1 08/15] drm/i2c: tda998x: Remove duplicate NULL check
Since i2c_unregister_device() became NULL-aware we may remove duplicate NULL check. Cc: Russell King Cc: David Airlie Cc: dri-devel@lists.freedesktop.org Signed-off-by: Andy Shevchenko --- drivers/gpu/drm/i2c/tda998x_drv.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 4d1f45acf2cd..7a349e85f964 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1602,8 +1602,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) /* if encoder_init fails, the encoder slave is never registered, * so cleanup here: */ - if (priv->cec) - i2c_unregister_device(priv->cec); + i2c_unregister_device(priv->cec); return -ENXIO; } -- 2.14.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/i915/bxt: use NULL for GPIO connection ID
The commit 213e08ad60ba
("drm/i915/bxt: add bxt dsi gpio element support")
enables GPIO support for Broxton based platforms.
While using that API we might get into troubles in the future, because
we can't rely on label name in the driver since vendor firmware might
provide any GPIO pin there, e.g. "reset", and even mark it in _DSD (in
which case the request will fail).
To avoid inconsistency and potential issues we have two options:
a) generate GPIO ACPI mapping table and supply it via
acpi_dev_add_driver_gpios(), or
b) just pass NULL as connection ID.
The b) approach is much simpler and would work since the driver relies
on GPIO indices only. Moreover, the _CRS fallback mechanism, when
requesting GPIO, has been made stricter, and supplying non-NULL
connection ID when neither _DSD, nor GPIO ACPI mapping is present, is
making request fail.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101921
Fixes: f10e4bf6632b ("gpio: acpi: Even more tighten up ACPI GPIO lookups")
Cc: Mika Kahola
Cc: Jani Nikula
Signed-off-by: Andy Shevchenko
---
v2:
- adjust commit message for proper time tenses
- add Fixes: and Bugzilla: tags
drivers/gpu/drm/i915/intel_dsi_vbt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c
b/drivers/gpu/drm/i915/intel_dsi_vbt.c
index 7158c7ce9c09..91c07b0c8db9 100644
--- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
@@ -306,7 +306,7 @@ static void bxt_exec_gpio(struct drm_i915_private *dev_priv,
if (!gpio_desc) {
gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
-"panel", gpio_index,
+NULL, gpio_index,
value ? GPIOD_OUT_LOW :
GPIOD_OUT_HIGH);
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/6] Support for LEGO MINDSTORMS EV3 LCD display
On Thu, Aug 3, 2017 at 8:09 PM, Andy Shevchenko wrote: > On Thu, Aug 3, 2017 at 6:18 PM, David Lechner wrote: > >> The particular display I have is this one: >> http://wiki.seeed.cc/Grove-OLED_Display_1.12inch/ >> >> It looks like it uses a command/data scheme like the MIPI displays, but >> doesn't use any of the standard values for the commands. The controller can >> do parallel, SPI and I2C, but the display I have is wired for I2C. > > It looks very similar to ssd1306. Some description refers to ssd1308. http://www.mouser.com/catalog/specsheets/Seeed_104030008.pdf -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/6] Support for LEGO MINDSTORMS EV3 LCD display
On Thu, Aug 3, 2017 at 6:18 PM, David Lechner wrote: > The particular display I have is this one: > http://wiki.seeed.cc/Grove-OLED_Display_1.12inch/ > > It looks like it uses a command/data scheme like the MIPI displays, but > doesn't use any of the standard values for the commands. The controller can > do parallel, SPI and I2C, but the display I have is wired for I2C. It looks very similar to ssd1306. Some description refers to ssd1308. -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/6] Support for LEGO MINDSTORMS EV3 LCD display
On Sun, Jul 30, 2017 at 9:27 PM, Andy Shevchenko wrote: > On Sat, Jul 29, 2017 at 10:17 PM, David Lechner wrote: >> Once all of this is done, it is really easy to add a new panel. :-) > > Perhaps pathes are good, but logically completely incorrect. s/pathes/patches/ -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/6] Support for LEGO MINDSTORMS EV3 LCD display
On Sat, Jul 29, 2017 at 10:17 PM, David Lechner wrote: > The goal of this series is to get the built-in LCD of the LEGO MINDSTORMS EV3 > working. But, most of the content here is building up the infrastructure to do > that. > > The controller used in the EV3 uses MIPI commands, but it uses a different > memory layout. The current tinydrm stuff is hard-coded for RGB565, so most > of the patches are adding support for other memory layouts. > > I've also made the one existing tinydrm driver generic so that it can work for > any MIPI display rather than copying a bunch of boiler-plate code for each > panel and/or controller. > > Once all of this is done, it is really easy to add a new panel. :-) Perhaps pathes are good, but logically completely incorrect. -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 5/6] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD
On Sat, Jul 29, 2017 at 10:17 PM, David Lechner wrote: > LEGO MINDSTORMS EV3 has an LCD with a ST7586 controller. This adds a new > module for the ST7586 controller with parameters for the EV3 LCD dispay. > .../devicetree/bindings/display/mipi-panel.txt | 2 +- > drivers/gpu/drm/tinydrm/mipi-panel.c | 87 > ++\] C'mon, changes here have nothing to do with the framework itself! Header, OTOH, has been looking fine. -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/6] drm/tinydrm: mipi-panel: refactor to use driver id
On Sat, Jul 29, 2017 at 10:17 PM, David Lechner wrote: > This refactors the mipi-panel module to use the driver id for panel-specific > data. This is in preparation for adding additional panels. > Wouldn't be better to split glue driver from what looks like more generic in the first place? Then you will not need to spread some stuff over generic files. Are we going to disrupt genericy of the framework already with second driver?! -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/6] drm/tinydrm: add helpers for ST7586 controllers
On Sat, Jul 29, 2017 at 10:17 PM, David Lechner wrote:
> This adds helper functions and support for ST7586 controllers. These
> controllers have an unusual memory layout where 3 pixels are packed into
> 1 byte.
>
> +---+-+
> | bit | 7 6 5 4 3 2 1 0 |
> +---+-+
> | pixel | 0 0 0 1 1 1 2 2 |
> +---+-+
>
> So, there are a nuber of places in the tinydrm pipline where this format
number
> needs to be taken into consideration.
> + * tinydrm_rgb565_to_st7586 - Convert RGB565 to ST7586 clip buffer
How this can be generic tinydrm helper?
Why driver can't handle it by its own and avoid spreading stuff into
generic header?
> +void tinydrm_rgb565_to_st7586(u8 *dst, void *vaddr,
> + * tinydrm_xrgb_to_st7586 - Convert XRGB to ST7586 clip buffer
> +void tinydrm_xrgb_to_st7586(u8 *dst, void *vaddr,
Ditto.
> - switch (fb->format->format) {
> - case DRM_FORMAT_RGB565:
> - if (swap)
> - tinydrm_swab16(dst, src, fb, clip);
> - else
> - tinydrm_memcpy(dst, src, fb, clip);
> + switch (pixel_fmt) {
> + case MIPI_DCS_PIXEL_FMT_16BIT:
> + switch (fb->format->format) {
> + case DRM_FORMAT_RGB565:
> + if (swap)
> + tinydrm_swab16(dst, src, fb, clip);
> + else
> + tinydrm_memcpy(dst, src, fb, clip);
> + break;
Can't you use some other approach? Callbacks? Plugins?
> + switch (mipi->pixel_fmt) {
> + case MIPI_DCS_PIXEL_FMT_16BIT:
> + len = width * height * sizeof(u16);
> + break;
> + case MIPI_DCS_PIXEL_FMT_ST7586_332:
> + width = (width + 2) / 3;
> + len = width * height;
> + break;
Ditto.
> + case MIPI_DCS_PIXEL_FMT_ST7586_332:
> + /* 3 pixels per byte */
> + bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
> + break;
Ditto.
> - if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
> + if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes &&
> + mipi->pixel_fmt != MIPI_DCS_PIXEL_FMT_ST7586_332)
Ditto.
If we allow this we end up to have 100500 LOCs in tinydrm-helpers.c
which will have nothing to do with the framework itself.
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/6] drm/tinydrm: Add parameter for MIPI DCS pixel format
On Sat, Jul 29, 2017 at 10:17 PM, David Lechner wrote:
> This adds a parameter for MIPI DCS pixel format to mipi_dbi_init().
> This is in preparation for supporting displays that don't use a 16bpp
> memory layout.
> /* MIPI DCS pixel formats */
> -#define MIPI_DCS_PIXEL_FMT_24BIT 7
> -#define MIPI_DCS_PIXEL_FMT_18BIT 6
> -#define MIPI_DCS_PIXEL_FMT_16BIT 5
> -#define MIPI_DCS_PIXEL_FMT_12BIT 3
> -#define MIPI_DCS_PIXEL_FMT_8BIT2
> -#define MIPI_DCS_PIXEL_FMT_3BIT1
> +enum mipi_dcs_pixel_format {
> + MIPI_DCS_PIXEL_FMT_24BIT= 7,
> + MIPI_DCS_PIXEL_FMT_18BIT= 6,
> + MIPI_DCS_PIXEL_FMT_16BIT= 5,
> + MIPI_DCS_PIXEL_FMT_12BIT= 3,
> + MIPI_DCS_PIXEL_FMT_8BIT = 2,
> + MIPI_DCS_PIXEL_FMT_3BIT = 1,
> +};
May I ask what prevents us to arrange enums in natural
ordering (you may keep = X parts, of course)?
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] backlight: always select BACKLIGHT_LCD_SUPPORT for BACKLIGHT_CLASS_DEVICE
On Wed, Jul 26, 2017 at 4:53 PM, Arnd Bergmann wrote: > randconfig builds occasionally produce this Kconfig warning: > > warning: (DRM_RADEON && DRM_AMDGPU && DRM_NOUVEAU && DRM_I915 && DRM_GMA500 > && DRM_SHMOBILE && DRM_TILCDC && DRM_FSL_DCU && DRM_TINYDRM && > DRM_PARADE_PS8622 && FB_BACKLIGHT && FB_ARMCLCD && FB_MX3 && USB_APPLEDISPLAY > && FB_OLPC_DCON && ACPI_CMPC && SAMSUNG_Q10) selects BACKLIGHT_CLASS_DEVICE > which has unmet direct dependencies (HAS_IOMEM && BACKLIGHT_LCD_SUPPORT) > > It turns out that amost all users of BACKLIGHT_CLASS_DEVICE also select > BACKLIGHT_LCD_SUPPORT, but not all of them do. This makes the remaining > ones behave like the others. > > It would probably be best to rework the way those two options related > entirely, but for now this takes the simpler and safer approach to > fix the warnings without introducing regressions. > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 80b87954f6dd..e0ca673bf564 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -785,6 +785,7 @@ config ACPI_CMPC > depends on RFKILL || RFKILL=n > select INPUT > select BACKLIGHT_CLASS_DEVICE > + select BACKLIGHT_LCD_SUPPORT > default n > help > Support for Intel Classmate PC ACPI devices, including some > @@ -1000,6 +1001,7 @@ config SAMSUNG_Q10 > tristate "Samsung Q10 Extras" > depends on ACPI > select BACKLIGHT_CLASS_DEVICE > + select BACKLIGHT_LCD_SUPPORT > ---help--- > This driver provides support for backlight control on Samsung Q10 > and related laptops, including Dell Latitude X200. Acked-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 4/6] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v4
On Tue, Jul 11, 2017 at 3:07 PM, kbuild test robot wrote: > make ARCH=i386 Yeah, either this code shouldn't have been built on 32-bit arch at all, or be portable. >arch/x86/pci/fixup.c: In function 'pci_amd_enable_64bit_bar': >>> arch/x86/pci/fixup.c:674:15: warning: large integer implicitly truncated to >>> unsigned type [-Woverflow] > res->start = 0x1ull; > ^~ >arch/x86/pci/fixup.c:675:13: warning: large integer implicitly truncated > to unsigned type [-Woverflow] > res->end = 0xfdull - 1; > ^~~ >>> arch/x86/pci/fixup.c:686:22: warning: right shift count >= width of type >>> [-Wshift-count-overflow] I suppose explicit casting will help. > high = ((res->start >> 40) & AMD_141b_MMIO_HIGH_MMIOBASE_MASK) | > ^~ >arch/x86/pci/fixup.c:687:21: warning: right shift count >= width of type > [-Wshift-count-overflow] > res->end + 1) >> 40) << AMD_141b_MMIO_HIGH_MMIOLIMIT_SHIFT) > ^~ These can be done via upper_32_bits() + shift. -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Use vsnprintf extension %ph
On Sun, Jun 11, 2017 at 3:20 AM, Joe Perches wrote: > On Sat, 2017-06-10 at 19:12 +0300, Andy Shevchenko wrote: >> On Wed, May 31, 2017 at 2:35 AM, Joe Perches wrote: >> > Using the extension saves a bit of code. >> > + seq_printf(m, "faux/mst: %*ph\n", 2, buf); >> > + seq_printf(m, "mst ctrl: %*ph\n", 1, buf); >> > + seq_printf(m, "branch oui: %*phN devid: ", 3, buf); >> >> All above may use shorter form, i.e. >> "..., "%Xph", buf);", where X is a constant written explicitly inplace. > > I know. Consistency is better though. > Note the use of DP_RECEIVER_CAP_SIZE, etc... Fair enough. -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Use vsnprintf extension %ph
On Wed, May 31, 2017 at 2:35 AM, Joe Perches wrote: > Using the extension saves a bit of code. > + seq_printf(m, "faux/mst: %*ph\n", 2, buf); > + seq_printf(m, "mst ctrl: %*ph\n", 1, buf); > + seq_printf(m, "branch oui: %*phN devid: ", 3, buf); All above may use shorter form, i.e. "..., "%Xph", buf);", where X is a constant written explicitly inplace. > + seq_printf(m, " revision: hw: %x.%x sw: %x.%x\n", > + buf[0x9] >> 4, buf[0x9] & 0xf, buf[0xa], buf[0xb]); Matter of taste, though buf[9], buf[10], buf[11] looks better to me :-) -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 5/6] drm/amdgpu: move hw generation check into amdgpu_doorbell_init
On Fri, Jun 9, 2017 at 11:59 AM, Christian König
wrote:
> From: Christian König
>
> This way we can savely call it on SI as well.
s/savely/safely
FWIW,
Reviewed-by: Andy Shevchenko
> Signed-off-by: Christian König
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index a7d6804..99290af 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -419,6 +419,15 @@ void amdgpu_pci_config_reset(struct amdgpu_device *adev)
> */
> static int amdgpu_doorbell_init(struct amdgpu_device *adev)
> {
> + /* No doorbell on SI hardware generation */
> + if (adev->asic_type < CHIP_BONAIRE) {
> + adev->doorbell.base = 0;
> + adev->doorbell.size = 0;
> + adev->doorbell.num_doorbells = 0;
> + adev->doorbell.ptr = NULL;
> + return 0;
> + }
> +
> /* doorbell bar mapping */
> adev->doorbell.base = pci_resource_start(adev->pdev, 2);
> adev->doorbell.size = pci_resource_len(adev->pdev, 2);
> @@ -2136,9 +2145,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> DRM_INFO("register mmio base: 0x%08X\n", (uint32_t)adev->rmmio_base);
> DRM_INFO("register mmio size: %u\n", (unsigned)adev->rmmio_size);
>
> - if (adev->asic_type >= CHIP_BONAIRE)
> - /* doorbell bar mapping */
> - amdgpu_doorbell_init(adev);
> + /* doorbell bar mapping */
> + amdgpu_doorbell_init(adev);
>
> /* io port mapping */
> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> @@ -2335,8 +2343,7 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
> adev->rio_mem = NULL;
> iounmap(adev->rmmio);
> adev->rmmio = NULL;
> - if (adev->asic_type >= CHIP_BONAIRE)
> - amdgpu_doorbell_fini(adev);
> + amdgpu_doorbell_fini(adev);
> amdgpu_debugfs_regs_cleanup(adev);
> }
>
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/4] PCI: add functionality for resizing resources v3
On Thu, May 4, 2017 at 1:15 PM, Andy Shevchenko
wrote:
> On Thu, May 4, 2017 at 12:23 PM, Christian König
> wrote:
>> Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:
> static int ...xxx...(...)
> {
> unsigned int i;
> int ret;
>
> if (res->parent)
> release_resource(res);
>
dev_info() should be here. I commented on this earlier, just forgot.
> res->start = 0;
> res->end = 0;
> dev_info(&bridge->dev, "BAR %d: released %pR\n", i, res);
> return i;
> }
>
>
> struct pci_dev *next = bridge;
> ...
> do {
> bridge = next;
>
> ret = ...xxx...(...);
> if (ret)
Here, of course,
if (ret < 0)
> goto cleanup;
>
> next = bridge->bus ? bridge->bus->self : NULL;
> } while (next);
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/4] PCI: add functionality for resizing resources v3
On Thu, May 4, 2017 at 12:23 PM, Christian König
wrote:
> Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:
>>> + while (1) {
>>
>> This raises red flag. Care to refactor?
>> Also do {} while () syntax allows faster to get that the loop body
>> goes at least once.
>
>
> I've tried to refactor this, but couldn't come up with something which works
> and is readable at the same time.
>
> Any suggestion how this should look like?
This is original code.
--- 8< --- 8< ---
while (1) {
for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END; i++) {
struct resource *res = &bridge->resource[i];
if ((res->flags & type_mask) != (type & type_mask))
continue;
/* Ignore BARs which are still in use */
if (res->child)
continue;
ret = add_to_list(&saved, bridge, res, 0, 0);
if (ret)
goto cleanup;
if (res->parent)
release_resource(res);
res->start = 0;
res->end = 0;
dev_info(&bridge->dev, "BAR %d: released %pR\n", i, res);
break;
}
if (i == PCI_BRIDGE_RESOURCE_END)
break;
if (!bridge->bus || !bridge->bus->self)
break;
bridge = bridge->bus->self;
}
--- 8< --- 8< ---
I would think about something like below
static int ...xxx...(...)
{
unsigned int i;
int ret;
for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END; i++) {
struct resource *res = &bridge->resource[i];
/* Ignore BARs which are still in use */
if (((res->flags ^ type) & type_mask) == 0 && !res->child)
break;
}
if (i == PCI_BRIDGE_RESOURCE_END)
return -EBUSY;
ret = add_to_list(&saved, bridge, res, 0, 0);
if (ret)
return ret;
if (res->parent)
release_resource(res);
res->start = 0;
res->end = 0;
dev_info(&bridge->dev, "BAR %d: released %pR\n", i, res);
return i;
}
struct pci_dev *next = bridge;
...
do {
bridge = next;
ret = ...xxx...(...);
if (ret)
goto cleanup;
next = bridge->bus ? bridge->bus->self : NULL;
} while (next);
...
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/4] PCI: add functionality for resizing resources v3
On Tue, May 2, 2017 at 6:51 PM, Christian König wrote:
> Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:
>> On Tue, Apr 25, 2017 at 4:19 PM, Christian König
>> wrote:
>>> +int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long
>>> type)
>>> +{
>>> + const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
>>> + IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
>>> +
>>
>> Redundant.
>
>
> Redundant, but also a reminder to myself that I wanted to ask something
> about that.
Missed context I suppose. Usually I comment in one word something
obvious, i.e. redundant empty line.
Sorry for missing my point.
> This type_mask is used already three times in this file, shouldn't we add a
> define for that?
Yes, that's wxactly what I commented somewhere (in one of the rest cases).
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/4] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v2
On Tue, Apr 25, 2017 at 4:19 PM, Christian König
wrote:
> From: Christian König
>
> Most BIOS don't enable this because of compatibility reasons.
>
> Manually enable a 64bit BAR of 64GB size so that we have
> enough room for PCI devices.
> +static void pci_amd_enable_64bit_bar(struct pci_dev *dev)
> +{
> + uint32_t base, limit, high;
Is u32 or uint32_t used actually in this file?
> + struct resource *res, *conflict;
> + unsigned i;
unsigned int i;
Reversed tree order.
> + for (i = 0; i < 8; ++i) {
> + pci_read_config_dword(dev, 0x80 + i * 0x8, &base);
> + pci_read_config_dword(dev, 0x180 + i * 0x4, &high);
> +
> + /* Is this slot free? */
> + if ((base & 0x3) == 0x0)
> + break;
> +
> + base >>= 8;
> + base |= high << 24;
> +
> + /* Abort if a slot already configures a 64bit BAR. */
> + if (base > 0x1)
> + return;
> + }
> + if (i == 8)
> + return;
> +
> + res = kzalloc(sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return;
> +
> + res->name = "PCI Bus :00";
> + res->flags = IORESOURCE_PREFETCH | IORESOURCE_MEM |
> + IORESOURCE_MEM_64 | IORESOURCE_WINDOW;
You are using this constant twice AFAICS, perhaps define it in generic
pci.h and re-use?
> + res->start = 0x1;
> + res->end = 0xfd - 1;
Perhaps you forgot ul / ull in many places in your code. Care to
compile for 32-bit and fix the warnings?
> + dev_info(&dev->dev, "adding root bus resource %pR\n", res);
Some of your messages I think are rather dev_dbg() than dev_info().
> + base = ((res->start >> 8) & 0xff00) | 0x3;
> + limit = ((res->end + 1) >> 8) & 0xff00;
> + high = ((res->start >> 40) & 0xff) |
> + res->end + 1) >> 40) & 0xff) << 16);
It would be nice to have the magics defined (just on top of this function) like
#define PCI_AMD_BAR_XXX_MASK GENMASK(...)
#define PCI_AMD_BAR_XXX_SHIFT nn
> + pci_write_config_dword(dev, 0x180 + i * 0x4, high);
> + pci_write_config_dword(dev, 0x84 + i * 0x8, limit);
> + pci_write_config_dword(dev, 0x80 + i * 0x8, base);
Ditto for pos:s.
> +
> + pci_bus_add_resource(dev->bus, res, 0);
> +}
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/4] PCI: add functionality for resizing resources v3
_dev *dev, int resno)
> +{
> + struct resource *res = dev->resource + resno;
> +
> + release_resource(res);
> + res->end = resource_size(res) - 1;
> + res->start = 0;
> + res->flags |= IORESOURCE_UNSET;
> + dev_info(&dev->dev, "BAR %d: released %pR\n", resno, res);
Makes little sense to me after you cleared information out.
> +}
> +EXPORT_SYMBOL(pci_release_resource);
> +
> +int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> +{
> + struct resource *res = dev->resource + resno;
> + u64 bytes = 1ULL << (size + 20);
> + res->end = res->start + (1ULL << (old + 20)) - 1;
Perhaps macro or helper?
static inline u64 rbar_size_to_bytes(size)
{
return 1ULL << (size + 20);
}
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] PCI: add resizeable BAR infrastructure v4
On Tue, Apr 25, 2017 at 4:19 PM, Christian König
wrote:
> From: Christian König
>
> Just the defines and helper functions to read the possible sizes of a BAR and
> update it's size.
>
> See
> https://pcisig.com/sites/default/files/specification_documents/ECN_Resizable-BAR_24Apr2008.pdf
> and PCIe r3.1, sec 7.22.
>
> This is useful for hardware with large local storage (mostly GFX) which only
> expose 256MB BARs initially to be compatible with 32bit systems.
> +u32 pci_rbar_get_possible_sizes(struct pci_dev *pdev, int bar)
> +{
> + unsigned pos, nbars;
> + u32 ctrl, cap;
> + unsigned i;
Are we supposed to use plain 'unsigned' nowadays? I would go with
'unsigned int'.
> +}
> + * Returns size if found or negativ error code.
Typo: negative.
> +int pci_rbar_get_current_size(struct pci_dev *pdev, int bar)
> +{
> + unsigned pos, nbars;
> + u32 ctrl;
> + unsigned i;
Reversed tree order?
> + for (i = 0; i < nbars; ++i, pos += 8) {
> + int bar_idx;
> +
> + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> + bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
> + PCI_REBAR_CTRL_BAR_IDX_SHIFT;
> + if (bar_idx != bar)
> + continue;
> +
> + return (ctrl & PCI_REBAR_CTRL_BAR_SIZE_MASK) >>
> + PCI_REBAR_CTRL_BAR_SIZE_SHIFT;
> + }
This one the same as previous function, the difference only in what is
returned. CAre to split static helper function for both?
> + return -ENOENT;
> +}
> +/**
> + * pci_rbar_set_size - set a new size for a BAR
> + * @dev: PCI device
> + * @bar: BAR to set size to
> + * @size: new size as defined in the spec (log2(size in bytes) - 20)
Not clear is it rounded up / down. I would go with "...in the spec
(0=1MB, 19=512GB)".
> + *
> + * Set the new size of a BAR as defined in the spec (0=1MB, 19=512GB).
> + * Returns true if resizing was successful, false otherwise.
> + */
> +int pci_rbar_set_size(struct pci_dev *pdev, int bar, int size)
> +{
> + unsigned pos, nbars;
> + u32 ctrl;
> + unsigned i;
> +
> + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
> + if (!pos)
> + return -ENOTSUPP;
> +
> + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> + nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >>
> PCI_REBAR_CTRL_NBAR_SHIFT;
> +
> + for (i = 0; i < nbars; ++i, pos += 8) {
> + int bar_idx;
> +
> + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> + bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
> + PCI_REBAR_CTRL_BAR_IDX_SHIFT;
> + if (bar_idx != bar)
> + continue;
Above is duplicating previous.
So,
static int ..._find_rbar(..., u32 *ctrl)
{
}
Returns: (i.e.) 0 - found, 1 - not found, -ERRNO.
ret = _find_rbar();
if (ret < 0)
return ret;
if (ret)
return -ENOENT;
...
return 0;
So, please refactor.
> +
> + ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE_MASK;
> + ctrl |= size << PCI_REBAR_CTRL_BAR_SIZE_SHIFT;
> + pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
> + return 0;
> + }
> +
> + return -ENOENT;
> +}
> -#define PCI_REBAR_CTRL_NBAR_MASK (7 << 5)/* mask for # bars */
> -#define PCI_REBAR_CTRL_NBAR_SHIFT 5 /* shift for # bars */
> +#define PCI_REBAR_CTRL_NBAR_MASK (7 << 5)/* mask for # BARs */
> +#define PCI_REBAR_CTRL_NBAR_SHIFT 5 /* shift for # BARs */
I understand why, but I dunno it worth to do.
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Resizeable PCI BAR support V4
On Tue, Apr 25, 2017 at 4:19 PM, Christian König wrote: > Hi everyone, > > This is the fourth incarnation of this set of patches. It would be nice if you put (next time) version of the series in each patch. Usually it's done quite easy by passing -vX (where X is version number) to git format-patch. > It enables device > drivers to resize and most likely also relocate the PCI BAR of devices > they manage to allow the CPU to access all of the device local memory at once. > > This is very useful for GFX device drivers where the default PCI BAR is only > about 256MB in size for compatibility reasons, but the device easily have > multiple gigabyte of local memory. > > Some changes since V3: > 1. A lot of minor style cleanups. > 2. Make internal functions for changing BARs directly private to the PCI > subsystem. > 3. Fail if any BAR is still in use when we try to change it. > 4. Handle intermediate bridges as well. > 5. Print some more messages when changing something. > > Please review and/or comment, Would look later (perhaps this week), -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/etnaviv: add thermal dependency
On Wed, Apr 19, 2017 at 9:11 PM, Arnd Bergmann wrote:
> When CONFIG_THERMAL is enabled as a loadable module, and etnaviv is
> built-in, we get a link error:
>
> drivers/gpu/drm/etnaviv/etnaviv_gpu.o: In function `etnaviv_gpu_bind':
> etnaviv_gpu.c:(.text.etnaviv_gpu_bind+0x34): undefined reference to
> `thermal_of_cooling_device_register'
> etnaviv_gpu.c:(.text.etnaviv_gpu_bind+0xe0): undefined reference to
> `thermal_cooling_device_unregister'
> drivers/gpu/drm/etnaviv/etnaviv_gpu.o: In function `etnaviv_gpu_unbind':
> etnaviv_gpu.c:(.text.etnaviv_gpu_unbind+0xf0): undefined reference to
> `thermal_cooling_device_unregister'
>
> This adds a Kconfig dependency to prevent etnaviv from being built-in
> with CONFIG_THERMAL=m, while still allowing the valid configurations.
> Unfortunately, simply adding the dependency here breaks Kconfig through
> a dependency loop involving lots of symbols all the way until ACPI_VIDEO,
> which is the only video driver that explicitly selects 'THERMAL'. Turning
> this into a 'depends on' addresses the problem.
> For completeness, I'm also removing the redundant 'select THERMAL'
> from INTEL_MENLOW, so no other driver uses that statement.
>
For PDx86 part:
Acked-by: Andy Shevchenko
> Fixes: bcdfb5e56dc5 ("drm/etnaviv: add etnaviv cooling device")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/acpi/Kconfig| 2 +-
> drivers/gpu/drm/etnaviv/Kconfig | 1 +
> drivers/platform/x86/Kconfig| 1 -
> 3 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 03cc4e74096b..252399efa8b3 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -184,7 +184,7 @@ config ACPI_VIDEO
> tristate "Video"
> depends on X86 && BACKLIGHT_CLASS_DEVICE
> depends on INPUT
> - select THERMAL
> + depends on THERMAL
> help
> This driver implements the ACPI Extensions For Display Adapters
> for integrated graphics devices on motherboard, as specified in
> diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
> index 71cee4e9fefb..1d6c744e7924 100644
> --- a/drivers/gpu/drm/etnaviv/Kconfig
> +++ b/drivers/gpu/drm/etnaviv/Kconfig
> @@ -3,6 +3,7 @@ config DRM_ETNAVIV
> tristate "ETNAVIV (DRM support for Vivante GPU IP cores)"
> depends on DRM
> depends on ARCH_MXC || ARCH_DOVE || (ARM && COMPILE_TEST)
> + depends on THERMAL || THERMAL=n
> depends on MMU
> select SHMEM
> select SYNC_FILE
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index e229ea317b9c..76ced87efde5 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -544,7 +544,6 @@ config SENSORS_HDAPS
> config INTEL_MENLOW
> tristate "Thermal Management driver for Intel menlow platform"
> depends on ACPI_THERMAL
> - select THERMAL
> ---help---
> ACPI thermal management enhancement driver on
> Intel Menlow platform.
> --
> 2.9.0
>
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm/amdgpu: resize VRAM BAR for CPU access
On Mon, Mar 13, 2017 at 2:41 PM, Christian König
wrote:
> From: Christian König
>
> Try to resize BAR0 to let CPU access all of VRAM.
> +void amdgpu_resize_bar0(struct amdgpu_device *adev)
> +{
> + u32 size = max(ilog2(adev->mc.real_vram_size - 1) + 1, 20) - 20;
> + int r;
> +
> + r = pci_resize_resource(adev->pdev, 0, size);
> +
Redundant.
> + if (r == -ENOTSUPP) {
> + /* The hardware don't support the extension. */
> + return;
> +
Ditto.
> + } else if (r == -ENOSPC) {
Useless use of else. And thus of curly braces.
> + DRM_INFO("Not enoigh PCI address space for a large BAR.");
> + } else if (r) {
> + DRM_ERROR("Problem resizing BAR0 (%d).", r);
> + }
> +
> + /* Reinit the doorbell mapping, it is most likely moved as well */
> + amdgpu_doorbell_fini(adev);
> + BUG_ON(amdgpu_doorbell_init(adev));
Comment why it's used here.
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/4] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors
On Mon, Mar 13, 2017 at 2:41 PM, Christian König
wrote:
> Most BIOS don't enable this because of compatibility reasons.
>
> Manually enable a 64bit BAR of 64GB size so that we have
> enough room for PCI devices.
> +static void pci_amd_enable_64bit_bar(struct pci_dev *dev)
> +{
> + const uint64_t size = 64ULL * 1024 * 1024 * 1024;
Perhaps extend and use SZ_64G here?
It would be nice to do, since some of the drivers already are using
sizes like 4GB and alike.
> + uint32_t base, limit, high;
> + struct resource *res;
> + unsigned i;
> + int r;
> +
> + for (i = 0; i < 8; ++i) {
> +
Redundant empty line.
> + pci_read_config_dword(dev, 0x80 + i * 0x8, &base);
> + pci_read_config_dword(dev, 0x180 + i * 0x4, &high);
> +
> + /* Is this slot free? */
> + if ((base & 0x3) == 0x0)
> + break;
> +
> + base >>= 8;
> + base |= high << 24;
> +
> + /* Abort if a slot already configures a 64bit BAR. */
> + if (base > 0x1)
> + return;
> +
Ditto.
> + }
> +
Ditto.
> + if (i == 8)
> + return;
> +
> + res = kzalloc(sizeof(*res), GFP_KERNEL);
> + res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH | IORESOURCE_MEM_64
> |
> + IORESOURCE_WINDOW;
> + res->name = dev->bus->name;
> + r = allocate_resource(&iomem_resource, res, size, 0x1,
> + 0xfd, size, NULL, NULL);
> + if (r) {
> + kfree(res);
> + return;
> + }
> +
> + base = ((res->start >> 8) & 0xff00) | 0x3;
> + limit = ((res->end + 1) >> 8) & 0xff00;
> + high = ((res->start >> 40) & 0xff) |
> + res->end + 1) >> 40) & 0xff) << 16);
Perhaps some of constants can be replaced by defines (I think some of
them are already defined in ioport.h or somewhere else).
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/4] PCI: add functionality for resizing resources v2
On Mon, Mar 13, 2017 at 2:41 PM, Christian König
wrote:
> From: Christian König
>
> This allows device drivers to request resizing their BARs.
>
> The function only tries to reprogram the windows of the bridge directly above
> the requesting device and only the BAR of the same type (usually mem, 64bit,
> prefetchable). This is done to make sure not to disturb other drivers by
> changing the BARs of their devices.
>
> If reprogramming the bridge BAR fails the old status is restored and -ENOSPC
> returned to the calling device driver.
Some style comments.
> v2: rebase on changes in rbar support
This kind of comments usually goes after --- delimiter below.
> Signed-off-by: Christian König
> ---
...here.
> +int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
> +{
> + const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
> + IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
> +
> + struct resource saved;
> + LIST_HEAD(add_list);
> + LIST_HEAD(fail_head);
> + struct pci_dev_resource *fail_res;
Perhaps move before definition of 'saved'?
> + unsigned i;
> + int ret = 0;
> +
> + /* Release all children from the matching bridge resource */
> + for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END; ++i) {
> + struct resource *res = &bridge->resource[i];
> +
> + if ((res->flags & type_mask) != (type & type_mask))
IIUC it would be
if ((res->flags ^ type) & type_mask)
(I consider 'diff' as XOR operation is more understandable, but it's up to you)
> + continue;
> + /* restore size and flags */
> + list_for_each_entry(fail_res, &fail_head, list) {
> + struct resource *res = fail_res->res;
> +
> + res->start = fail_res->start;
> + res->end = fail_res->end;
> + res->flags = fail_res->flags;
> + }
> +
> + /* Revert to the old configuration */
> + if (!list_empty(&fail_head)) {
> + struct resource *res = &bridge->resource[i];
> +
> + res->start = saved.start;
> + res->end = saved.end;
> + res->flags = saved.flags;
Would
*res = saved;
work?
> +int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> +{
> + struct resource *res = dev->resource + resno;
> + u32 sizes = pci_rbar_get_possible_sizes(dev, resno);
> + int old = pci_rbar_get_current_size(dev, resno);
> + u64 bytes = 1ULL << (size + 20);
> + int ret = 0;
> +
I would put
sizes = pci_rbar_get_possible_sizes(dev, resno);
here
> + if (!sizes)
> + return -ENOTSUPP;
> +
> + if (!(sizes & (1 << size)))
BIT(size) ?
> + return -EINVAL;
> +
and
old = pci_rbar_get_current_size(dev, resno);
here
> + if (old < 0)
> + return old;
> +error_resize:
> + pci_rbar_set_size(dev, resno, old);
> + res->end = res->start + (1ULL << (old + 20)) - 1;
BIT_ULL(old + 20) ?
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/5] Thunderbolt GPU fixes
On Fri, Mar 10, 2017 at 2:07 PM, Lukas Wunner wrote:
> On Thu, Mar 09, 2017 at 04:03:47PM +0100, Daniel Vetter wrote:
>> On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote:
>> > Fix Thunderbolt-related issues in apple-gmux and vga_switcheroo:
>> >
>> > Patch [1/5] ("Recognize Thunderbolt devices") has already been subjected
>> > to a fair amount of scrutiny over at linux-pci@, I've submitted it 5 times
>> > total since May 2016. With luck it may be in ack-able shape now.
>> >
>> > Patch [2/5] amends apple-gmux to handle combined DP/Thunderbolt ports
>> > properly on newer MacBook Pros.
>> >
>> > Patches [3/5] to [5/5] avoid registering external Thunderbolt GPUs with
>> > vga_switcheroo: Dave Airlie designed vga_switcheroo to register GPUs
>> > unconditionally. So if a desktop box has multiple GPUs, vga_switcheroo
>> > will see more than one discrete GPU but that's not a problem because on
>> > desktop boxes no handler is registered and thus vga_switcheroo_enable()
>> > is never called. Hybrid graphics laptops on the other hand do register
>> > a handler, but are assumed to never register more than one discrete GPU.
>> > However once a Thunderbolt eGPU is attached to a hybrid graphics laptop,
>> > that assumption is no longer true and things go south when vga_switcheroo
>> > runtime suspends the external discrete GPU and then calls the handler to
>> > cut power to the internal discrete GPU. The driver for the internal GPU
>> > will sit there puzzled and typically cause a lockup.
> [snip]
>> > I've pushed the present series to GitHub in case anyone prefers reviewing
>> > it in a GUI:
>> > https://github.com/l1k/linux/commits/thunderbolt_gpu_v1
>>
>> For merging, should I smash this all into drm-misc? The only thing outside
>> is the apple-gmux driver ...
>
> Merging through drm-misc would be lovely. However I've prepared a v2 of
> patch [1/5] to address Bjorn's comments (amended the commit message and a
> code comment). I'll respin the series this evening and include the acks
> I've collected so far.
>
> @Darren & Andy:
> Please ack patch [5/5] of this series, barring any objections.
>
> I'll move the apple-gmux patch to the end of the series, so merging that
> one can be postponed until Darren and Andy find the time to look at it.
Yeah, please resend. It's probably buried in the pile of deleted mails.
Though I have still patchwork ticket.
Okay, I have looked at it and I'm fine with the code. Hope this
doesn't break anything.
Acked-by: Andy Shevchenko
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v1] drm/i915/bxt: use NULL for GPIO connection ID
On Sun, 2017-02-26 at 22:45 +0100, Daniel Vetter wrote:
> On Tue, Feb 21, 2017 at 06:52:24PM +0200, Andy Shevchenko wrote:
> > On Tue, 2017-02-21 at 18:26 +0200, Jani Nikula wrote:
> > > On Tue, 21 Feb 2017, Andy Shevchenko > > l.co
> > > m> wrote:
> > > > The commit 213e08ad60ba ("drm/i915/bxt: add bxt dsi gpio element
> > > > support") enables GPIO support for Broxton based platforms.
> > > >
> > > > While using that API we might get into troubles in the future,
> > > > because
> > > > we can't rely on label "panel" in the driver since vendor
> > > > firmware
> > > > might
> > > > provide any GPIO pin there, e.g. "reset", and even mark it in
> > > > _DSD
> > > > (in
> > > > which case the request will fail).
> > > >
> > > > To avoid inconsistency and potential issues we have two options:
> > > > a) generate GPIO ACPI mapping table and supply it via
> > > > acpi_dev_add_driver_gpios(), or
> > > > b) just pass NULL as connection ID.
> > > >
> > > > The b) approach is much simplier and would work since the driver
> > > > relies
> > > > on GPIO indeces only. Moreover, the _CRS fallback mechanism,
> > > > when
> > > > requesting GPIO, is going to be stricter, and supplying non-NULL
> > > > connection ID when neither _DSD, nor GPIO ACPI mapping is
> > > > present,
> > > > will
> > > > make request fail.
> > >
> > > The patch version log in the commit suggests otherwise; we'd tried
> > > and
> > > failed with NULL,
> >
> > Can I see DSDT excerpts of the platform that fails?
> >
> > > until Mika realized passing "panel" works:
> > >
> > > v2 by Mika: switch *NULL* to *"panel"* when requesting gpio
> > > for
> > > MIPI/DSI
> > > panel.
> > >
> > > See also [1]. What has changed since then that should make this
> > > work
> > > now? We shouldn't apply until we get Tested-by's.
> >
> > Not changed yet, but *going to be*. See my repository here [2].
> > To fix the mess with GPIO ACPI stuff we are going to make request
> > stricter as I pointed in commit message above, i.e. asking for a
> > GPIO by
> > connection ID without _DSD present will guarantee -ENOENT since it
> > will
> > be no fallback to _CRS. You may follow discussion in our internal
> > mailing list for drivers.
>
> Why exactly is this being discussed on an internal mailing list?
> Upstream
> happens in public ...
It was a prelininary discussion and it's sad you didn't notice it.
Nevertheless, Hans started it in public mailing list here [1].
I would include you in Cc list for my further replies there.
Mika K., my branch is here [2] with above patch included. Can you,
please, test your use case? Because it sounds too strange to me that
connection ID in there affects somehow the PM flow. It very likely
unveils a bug somewhere else.
[1] https://www.spinics.net/lists/linux-input/msg49127.html
[2] http://bitbucket.org/andy-shev/linux/branch/topic%2Fuart%2frpm
--
Andy Shevchenko
Intel Finland Oy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/5] PCI: add resizeable BAR infrastructure v2
On Mon, Mar 6, 2017 at 1:40 PM, Christian König wrote:
> From: Christian König
>
> Just the defines and helper functions to read the possible sizes of a BAR and
> update it's size.
>
> See
> https://pcisig.com/sites/default/files/specification_documents/ECN_Resizable-BAR_24Apr2008.pdf.
>
> v2: provide read helper as well
Commit message left away the explanation at which point this API might
be useful and how it fits in managed resources model?
> /**
> + * pci_rbar_get_sizes - get possible sizes for BAR
Why not simple pci_rbar_get_possible_sizes() ?
> +u32 pci_rbar_get_sizes(struct pci_dev *pdev, int bar)
> +{
> + int pos, nbars;
> + u32 ctrl, cap;
> + int i;
> +
> + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
> + if (!pos)
> + return 0x0;
return 0;
> +
> + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> + nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >>
> PCI_REBAR_CTRL_NBAR_SHIFT;
> +
> + for (i = 0; i < nbars; ++i, pos += 8) {
8 is defined somewhere in the spec? (Yes, I understand that is just 64
bits shift)
> + int bar_idx;
> +
> + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> + bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
> + PCI_REBAR_CTRL_BAR_IDX_SHIFT;
> + if (bar_idx != bar)
> + continue;
> +
> + pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
> + return (cap & PCI_REBAR_CTRL_SIZES_MASK) >>
> + PCI_REBAR_CTRL_SIZES_SHIFT;
> + }
> +
> + return 0x0;
return 0;
> +/**
> + * pci_rbar_get_size - get the current size of a BAR
pci_rbar_get_current_size() ?
> +/**
> + * pci_rbar_set_size - set a new size for a BAR
> + * @dev: PCI device
> + * @bar: BAR to set size to
> + * @size: new size as defined in the spec.
* @size: bitmasked value of new size (bit 0=1MB, ..., bit 19=512G)
?
It will briefly get a clue without reading either spec or long description.
> + *
> + * Set the new size of a BAR as defined in the spec (0=1MB, 19=512GB).
> + * Returns true if resizing was successful, false otherwise.
> + */
> +bool pci_rbar_set_size(struct pci_dev *pdev, int bar, int size)
I would return int and error code. It would be better in the future
and seems in alignment with above.
> +{
> + int pos, nbars;
> + u32 ctrl;
> + int i;
All ints are unsigned?
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 5/5] drm/amdgpu: resize VRAM BAR for CPU access
On Mon, Mar 6, 2017 at 1:40 PM, Christian König wrote:
> From: Christian König
>
> Try to resize BAR0 to let CPU access all of VRAM.
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -616,6 +616,35 @@ void amdgpu_gtt_location(struct amdgpu_device *adev,
> struct amdgpu_mc *mc)
> +void amdgpu_resize_bar0(struct amdgpu_device *adev)
> +{
> + u32 size = max(ilog2(adev->mc.real_vram_size - 1) + 1, 20) - 20;
Too complicated.
unsigned long = fls_long(real_vram_size | BIT(20));
And the result is not a size, right? It's a logarithm from size.
> + int r;
> +
> + r = pci_resize_resource(adev->pdev, 0, size);
> +
Redundant line.
> + if (r == -ENOTSUPP) {
> + /* The hardware don't support the extension. */
> + return;
> +
> + } else if (r == -ENOSPC) {
> + DRM_INFO("Not enoigh PCI address space for a large BAR.");
> + } else if (r) {
> + DRM_ERROR("Problem resizing BAR0 (%d).", r);
> + }
> +
> + /* Reinit the doorbell mapping, it is most likely moved as well */
> + amdgpu_doorbell_fini(adev);
> + BUG_ON(amdgpu_doorbell_init(adev));
No way to recover?!
> +}
> +
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/5] drm/amdgpu: fix printing the doorbell BAR info
On Mon, Mar 6, 2017 at 1:40 PM, Christian König wrote:
> From: Christian König
>
> The address is 64bit, not 32bit.
>
> Signed-off-by: Christian König
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index bf31aaf..a470869 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -385,7 +385,7 @@ static int amdgpu_doorbell_init(struct amdgpu_device
> *adev)
> if (adev->doorbell.ptr == NULL) {
> return -ENOMEM;
> }
> - DRM_INFO("doorbell mmio base: 0x%08X\n",
> (uint32_t)adev->doorbell.base);
> + DRM_INFO("doorbell mmio base: 0x%llX\n",
> (uint64_t)adev->doorbell.base);
> DRM_INFO("doorbell mmio size: %u\n", (unsigned)adev->doorbell.size);
It seems I sent patch to remove those at all, but if you wish to leave
them, please convert to %pap and remove explicit casting.
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1] drm/i915/bxt: use NULL for GPIO connection ID
On Tue, 2017-02-21 at 18:26 +0200, Jani Nikula wrote:
> On Tue, 21 Feb 2017, Andy Shevchenko m> wrote:
> > The commit 213e08ad60ba ("drm/i915/bxt: add bxt dsi gpio element
> > support") enables GPIO support for Broxton based platforms.
> >
> > While using that API we might get into troubles in the future,
> > because
> > we can't rely on label "panel" in the driver since vendor firmware
> > might
> > provide any GPIO pin there, e.g. "reset", and even mark it in _DSD
> > (in
> > which case the request will fail).
> >
> > To avoid inconsistency and potential issues we have two options:
> > a) generate GPIO ACPI mapping table and supply it via
> > acpi_dev_add_driver_gpios(), or
> > b) just pass NULL as connection ID.
> >
> > The b) approach is much simplier and would work since the driver
> > relies
> > on GPIO indeces only. Moreover, the _CRS fallback mechanism, when
> > requesting GPIO, is going to be stricter, and supplying non-NULL
> > connection ID when neither _DSD, nor GPIO ACPI mapping is present,
> > will
> > make request fail.
>
> The patch version log in the commit suggests otherwise; we'd tried and
> failed with NULL,
Can I see DSDT excerpts of the platform that fails?
> until Mika realized passing "panel" works:
>
> v2 by Mika: switch *NULL* to *"panel"* when requesting gpio for
> MIPI/DSI
> panel.
>
> See also [1]. What has changed since then that should make this work
> now? We shouldn't apply until we get Tested-by's.
Not changed yet, but *going to be*. See my repository here [2].
To fix the mess with GPIO ACPI stuff we are going to make request
stricter as I pointed in commit message above, i.e. asking for a GPIO by
connection ID without _DSD present will guarantee -ENOENT since it will
be no fallback to _CRS. You may follow discussion in our internal
mailing list for drivers.
> [1] http://mid.mail-archive.com/1480597671.26172.82.camel@intel.com
[2] http://bitbucket.org/andy-shev/linux/branch/topic%2Fuart%2frpm
--
Andy Shevchenko
Intel Finland Oy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1] drm/i915/bxt: use NULL for GPIO connection ID
The commit 213e08ad60ba ("drm/i915/bxt: add bxt dsi gpio element
support") enables GPIO support for Broxton based platforms.
While using that API we might get into troubles in the future, because
we can't rely on label "panel" in the driver since vendor firmware might
provide any GPIO pin there, e.g. "reset", and even mark it in _DSD (in
which case the request will fail).
To avoid inconsistency and potential issues we have two options:
a) generate GPIO ACPI mapping table and supply it via
acpi_dev_add_driver_gpios(), or
b) just pass NULL as connection ID.
The b) approach is much simplier and would work since the driver relies
on GPIO indeces only. Moreover, the _CRS fallback mechanism, when
requesting GPIO, is going to be stricter, and supplying non-NULL
connection ID when neither _DSD, nor GPIO ACPI mapping is present, will
make request fail.
Cc: Mika Kahola
Cc: Jani Nikula
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 8f683b8b1816..493d5ec2b53a 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -315,7 +315,7 @@ static void bxt_exec_gpio(struct drm_i915_private *dev_priv,
if (!gpio_desc) {
gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
-"panel", gpio_index,
+NULL, gpio_index,
value ? GPIOD_OUT_LOW :
GPIOD_OUT_HIGH);
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/7] drm/tinydrm: Add helper functions
On Sat, Feb 11, 2017 at 8:48 PM, Noralf Trønnes wrote:
> Add common functionality needed by many tinydrm drivers.
> +int tinydrm_enable_backlight(struct backlight_device *backlight)
> +{
> + unsigned int old_state;
> + int ret;
> +
> + if (!backlight)
> + return 0;
> +
> + old_state = backlight->props.state;
> + backlight->props.state &= ~BL_CORE_FBBLANK;
> + DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
> + backlight->props.state);
"%#x" ?
(And elsewhere)
> +#if IS_ENABLED(CONFIG_SPI)
> +size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len)
> +{
> + size_t ret;
> +
> + ret = min(spi_max_transfer_size(spi), spi->master->max_dma_len);
I don't get why DMA constrain somehow affects this framework?
What if max_dma_len is zero (imagine SPI master that works only by PIO
by some reason)?
> + if (max_len)
> + ret = min(ret, max_len);
> + if (spi_max)
> + ret = min_t(size_t, ret, spi_max);
> + ret &= ~0x3;
Why alignment is that? Why do we need it? Isn't a busyness of SPI
framework to cope with it?
> + if (ret < 4)
It's effectively check for 0.
> + ret = 4;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(tinydrm_spi_max_transfer_size);
> +static void
> +tinydrm_dbg_spi_print(struct spi_device *spi, struct spi_transfer *tr,
> + const void *buf, int idx, bool tx)
> +{
> + u32 speed_hz = tr->speed_hz ? tr->speed_hz : spi->max_speed_hz;
> + char linebuf[3 * 32];
> +
> + hex_dump_to_buffer(buf, tr->len, 16,
> + DIV_ROUND_UP(tr->bits_per_word, 8),
> + linebuf, sizeof(linebuf), false);
> +
> + printk(KERN_DEBUG
> + "tr(%i): speed=%u%s, bpw=%i, len=%u, %s_buf=[%s%s]\n", idx,
> + speed_hz > 100 ? speed_hz / 100 : speed_hz / 1000,
> + speed_hz > 100 ? "MHz" : "kHz", tr->bits_per_word, tr->len,
> + tx ? "tx" : "rx", linebuf, tr->len > 16 ? " ..." : "");
I hope at some point we will have some extension to print speeds,
sizes and so on based on algo in string_get_size().
> +}
> +int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
> +struct spi_transfer *header, u8 bpw, const void *buf,
> +size_t len)
> +{
> + struct spi_transfer tr = {
> + .bits_per_word = bpw,
> + .speed_hz = speed_hz,
> + };
> + struct spi_message m;
> + u16 *swap_buf = NULL;
> + size_t max_chunk;
> + size_t chunk;
> + int ret = 0;
> +
> + if (WARN_ON_ONCE(bpw != 8 && bpw != 16))
> + return -EINVAL;
> +
> + max_chunk = tinydrm_spi_max_transfer_size(spi, 0);
> +
> + if (drm_debug & DRM_UT_DRIVER)
> + pr_debug("[drm:%s] bpw=%u, max_chunk=%zu, transfers:\n",
> +__func__, bpw, max_chunk);
For all of your dev_dbg() / pr_debug() __func__ argument might be
redundant. Dynamic Debug may include this by request from user.
> +/**
> + * tinydrm_machine_little_endian - Machine is little endian
> + *
> + * Returns:
> + * true if *defined(__LITTLE_ENDIAN)*, false otherwise
> + */
> +static inline bool tinydrm_machine_little_endian(void)
> +{
> +#if defined(__LITTLE_ENDIAN)
> + return true;
> +#else
> + return false;
> +#endif
> +}
Hmm... What is the typical code of a caller for this?
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/7] drm: Add DRM support for tiny LCD displays
On Sat, Feb 11, 2017 at 8:48 PM, Noralf Trønnes wrote:
> tinydrm provides helpers for very simple displays that can use
> CMA backed framebuffers and need flushing on changes.
> +/**
> + * tinydrm_gem_cma_free_object - Free resources associated with a CMA GEM
> + * object
Keep on one line?
> +const struct file_operations tinydrm_fops = {
> + .owner = THIS_MODULE,
Do we still need this?
> +static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
> + const struct drm_framebuffer_funcs *fb_funcs,
> + struct drm_driver *driver)
> +{
> + struct drm_device *drm;
> +
> + mutex_init(&tdev->dirty_lock);
> + tdev->fb_funcs = fb_funcs;
> +
> + /*
> +* We don't embed drm_device, because that prevent us from using
> +* devm_kzalloc() to allocate tinydrm_device in the driver since
> +* drm_dev_unref() frees the structure. The devm_ functions provide
"devm_ functions" -> "devm_kzalloc()" ?
Otherwise what else do you refer to and why here?
> +* for easy error handling.
> +*/
> +static int tinydrm_register(struct tinydrm_device *tdev)
> +{
> + struct drm_device *drm = tdev->drm;
> + int bpp = drm->mode_config.preferred_depth;
> + struct drm_fbdev_cma *fbdev;
> + int ret;
> +
> + ret = drm_dev_register(tdev->drm, 0);
> + if (ret)
> + return ret;
> +
> + fbdev = drm_fbdev_cma_init_with_funcs(drm, bpp ? bpp : 32,
> + drm->mode_config.num_connector,
> + tdev->fb_funcs);
> + if (IS_ERR(fbdev))
> + DRM_ERROR("Failed to initialize fbdev: %ld\n",
> PTR_ERR(fbdev));
> + else
> + tdev->fbdev_cma = fbdev;
Apparently I missed previous round of reviews, can you just put in
short words why error is not propagated here to the caller?
> +
> + return 0;
> +}
> +/**
> + * tinydrm_display_pipe_init - Initialize display pipe
> + * @tdev: tinydrm device
> + * @funcs: Display pipe functions
> + * @connector_type: Connector type
> + * @formats: Array of supported formats (DRM_FORMAT\_\*)
DRM_FORMAT_* ?
> + * @format_count: Number of elements in @formats
> + * @mode: Supported mode
> + * @rotation: Initial @mode rotation in degrees Counter Clock Wise
> + *
> + * This function sets up a &drm_simple_display_pipe with a &drm_connector
> that
> + * has one fixed &drm_display_mode which is rotated according to @rotation.
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +{
> + struct drm_device *drm = tdev->drm;
> + struct drm_display_mode *mode_copy;
> + struct drm_connector *connector;
> + int ret;
> + connector = tinydrm_connector_create(drm, mode_copy, connector_type);
> + if (IS_ERR(connector))
> + return PTR_ERR(connector);
> +
> + ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats,
> + format_count, connector);
> + if (ret)
> + return ret;
> +
> + return 0;
return drm_... ?
> +}
> +EXPORT_SYMBOL(tinydrm_display_pipe_init);
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 06/13] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore
On Mon, 2017-01-23 at 22:09 +0100, Hans de Goede wrote: > On my cherrytrail tablet with axp288 pmic, just doing a bunch of > repeated > reads from the pmic, e.g. "i2cdump -y 14 0x34" would lookup the tablet > in > 1 - 3 runs guaranteed. > > This seems to be causes by the cpu trying to enter C6 or C7 while we > hold > the punit bus semaphore, at which point everything just hangs. > > Avoid this by disallowing the CPU to enter C6 or C7 before acquiring > the > punit bus semaphore. > Changes in v5: > -Update the pm_qos for a latency of 0 *before* requesting the > semaphore, > instead of doing it while waiting for the request to be acked So, that's why you put to reset_semaphore() instead of baytrail_i2c_release(), right? I perhaps missed your answer to my comment about that. > > @@ -56,6 +57,8 @@ static void reset_semaphore(struct dw_i2c_dev > > *dev) > data &= ~PUNIT_SEMAPHORE_BIT; > if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, > PUNIT_SEMAPHORE, data)) > dev_err(dev->dev, "iosf failed to reset punit > semaphore during write\n"); > + > + pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE); > } -- Andy Shevchenko Intel Finland Oy ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/7] x86/platform/intel/iosf_mbi: Add a mutex for punit access
On Sun, 2017-01-08 at 16:30 +0100, Hans de Goede wrote: > Hi, > > On 08-01-17 16:16, Andy Shevchenko wrote: > > On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote: > > > One some systems the punit accesses the pmic to change various > > > voltages > > > through the same bus as other kernel drivers use for e.g. battery > > > monitoring. > > > > > > If a driver sends requests to the punit which require the punit to > > > access > > > the pmic bus while another driver is also accessing the pmic bus > > > various > > > bad things happen. > > > > > > This commit adds a mutex to protect the punit against simultaneous > > > accesses > > > and 2 functions to lock / unlock this mutex. > > > > > > Note on these systems the i2c-bus driver will request a sempahore > > > from > > > the > > > punit for exclusive access to the pmic bus when i2c drivers are > > > accessing > > > it, but this does not appear to be sufficient, we still need to > > > avoid > > > making certain punit requests during the access window to avoid > > > problems. > > > > I'm fine with the patch, but please spell > > P-Unit > > PMIC > > In the commit msg and comments, not in code you mean I assume ? Correct. -- Andy Shevchenko Intel Finland Oy
[PATCH 7/7] drm/i915: Take punit lock when modifying punit settings
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> Â if (val & DSP_MAXFIFO_PM5_ENABLE)
> @@ -4575,6 +4581,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> Â wm->level = VLV_WM_LEVEL_DDR_DVFS;
> Â }
> Â
> + iosf_mbi_punit_unlock();
> Â mutex_unlock(&dev_priv->rps.hw_lock);
> Â }
> Â
> @@ -4981,7 +4988,9 @@ static void valleyview_set_rps(struct
> drm_i915_private *dev_priv, u8 val)
> Â I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
> Â
> Â if (val != dev_priv->rps.cur_freq) {
> + iosf_mbi_punit_lock();
> Â vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> val);
> + iosf_mbi_punit_unlock();
> Â if (!IS_CHERRYVIEW(dev_priv))
> Â gen6_set_rps_thresholds(dev_priv, val);
> Â }
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index c0b7e95..17922ae 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -28,6 +28,7 @@
> Â
> Â #include
> Â #include
> +#include
> Â
> Â #include "i915_drv.h"
> Â #include "intel_drv.h"
> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct
> drm_i915_private *dev_priv,
> Â if (COND)
> Â goto out;
> Â
> + iosf_mbi_punit_lock();
> +
> Â ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
> Â ctrl &= ~mask;
> Â ctrl |= state;
> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct
> drm_i915_private *dev_priv,
> Â Â Â state,
> Â Â Â vlv_punit_read(dev_priv,
> PUNIT_REG_PWRGT_CTRL));
> Â
> + iosf_mbi_punit_unlock();
> +
> Â #undef COND
> Â
> Â out:
> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct
> drm_i915_private *dev_priv,
> Â if (COND)
> Â goto out;
> Â
> + iosf_mbi_punit_lock();
> +
> Â ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> Â ctrl &= ~DP_SSC_MASK(pipe);
> Â ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct
> drm_i915_private *dev_priv,
> Â Â Â state,
> Â Â Â vlv_punit_read(dev_priv,
> PUNIT_REG_DSPFREQ));
> Â
> + iosf_mbi_punit_unlock();
> +
> Â #undef COND
> Â
> Â out:
--
Andy Shevchenko
Intel Finland Oy
[PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire
On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote: > Take the punit lock to stop others from accessing the punit while the > pmic i2c bus is in use. This is necessary because accessing the punit > from the kernel may result in the punit trying to access the pmic i2c > bus, which results in a hang when it happens while we own the pmic i2c > bus semaphore. > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241 > Signed-off-by: Hans de Goede > Tested-by: tagorereddy > --- > Â drivers/i2c/busses/i2c-designware-baytrail.c | 4 > Â 1 file changed, 4 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c > b/drivers/i2c/busses/i2c-designware-baytrail.c > index 3effc9a..cf7a2a0 100644 > --- a/drivers/i2c/busses/i2c-designware-baytrail.c > +++ b/drivers/i2c/busses/i2c-designware-baytrail.c > > @@ -62,6 +62,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev) > + iosf_mbi_punit_unlock(); > @@ -79,6 +81,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev > + iosf_mbi_punit_lock(); For me it looks a bit asymmetrical. Can we use acquire()/release()? -- Andy Shevchenko Intel Finland Oy
[PATCH 4/7] i2c: designware-baytrail: Call pmic_bus_access_notifier_chain
On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote: > Call the iosf_mbi pmic_bus_access_notifier_chain on bus acquire / > release. > FWIW: Reviewed-by: Andy Shevchenko > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241 > Signed-off-by: Hans de Goede > Tested-by: tagorereddy > --- > Â drivers/i2c/busses/i2c-designware-baytrail.c | 4 > Â 1 file changed, 4 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c > b/drivers/i2c/busses/i2c-designware-baytrail.c > index cf7a2a0..e8cf10a 100644 > --- a/drivers/i2c/busses/i2c-designware-baytrail.c > +++ b/drivers/i2c/busses/i2c-designware-baytrail.c > @@ -63,6 +63,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev) > Â > Â pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE); > Â > + iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACC > ESS_END, > + Â Â Â Â Â NULL); > Â iosf_mbi_punit_unlock(); > Â } > Â > @@ -82,6 +84,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev > *dev) > Â return 0; > Â > Â iosf_mbi_punit_lock(); > + iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACC > ESS_BEGIN, > + Â Â Â Â Â NULL); > Â > Â /* > Â Â * Disallow the CPU to enter C6 or C7 state, entering these > states -- Andy Shevchenko Intel Finland Oy
[PATCH 2/7] x86/platform/intel/iosf_mbi: Add a pmic bus access notifier
On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> Some drivers may need to acquire punit managed resources from
> interrupt
> context, where they cannot call iosf_mbi_punit_lock().
>
> This commit adds a notifier chain which allows a driver to get
> notified
> (in a process context) before other drivers start accessing the pmic
> bus,
> so that the driver can acquire any resources, which it may need during
> the window the other driver is accessing the pmic, before hand.
>
Same comments as per patch 1.
I'm okay with the patch, but I would hear from other stakeholders that's
_the_ way we are going.
So, FWIW:
Reviewed-by: Andy Shevchenko
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede
> Tested-by: tagorereddy
> ---
>  arch/x86/include/asm/iosf_mbi.h    | 54
> ++
> Â arch/x86/platform/intel/iosf_mbi.c | 36 +
> Â 2 files changed, 90 insertions(+)
>
> diff --git a/arch/x86/include/asm/iosf_mbi.h
> b/arch/x86/include/asm/iosf_mbi.h
> index 91f5d16..b8733bb 100644
> --- a/arch/x86/include/asm/iosf_mbi.h
> +++ b/arch/x86/include/asm/iosf_mbi.h
> @@ -47,6 +47,10 @@
> Â #define QRK_MBI_UNIT_MM 0x05
> Â #define QRK_MBI_UNIT_SOC0x31
> Â
> +/* Action values for the pmic_bus_access_notifier functions */
> +#define MBI_PMIC_BUS_ACCESS_BEGIN1
> +#define MBI_PMIC_BUS_ACCESS_END 2
> +
> Â #if IS_ENABLED(CONFIG_IOSF_MBI)
> Â
> Â bool iosf_mbi_available(void);
> @@ -115,6 +119,38 @@ void iosf_mbi_punit_lock(void);
> Â */
> Â void iosf_mbi_punit_unlock(void);
> Â
> +/**
> + * iosf_mbi_register_pmic_bus_access_notifier - Register pmic bus
> notifier
> + *
> + * This function can be used by drivers which may need to acquire
> punit
> + * managed resources from interrupt context, where
> iosf_mbi_punit_lock()
> + * can not be used.
> + *
> + * This function allows a driver to register a notifier to get
> notified (in a
> + * process context) before other drivers start accessing the pmic
> bus.
> + *
> + * This allows the driver to acquire any resources, which it may need
> during
> + * the window the other driver is accessing the pmic, before hand.
> + *
> + * @nb: notifier_block to register
> + */
> +int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block
> *nb);
> +
> +/**
> + * iosf_mbi_register_pmic_bus_access_notifier - Unregister pmic bus
> notifier
> + *
> + * @nb: notifier_block to unregister
> + */
> +int iosf_mbi_unregister_pmic_bus_access_notifier(struct
> notifier_block *nb);
> +
> +/**
> + * iosf_mbi_call_pmic_bus_access_notifier_chain - Call pmic bus
> notifier chain
> + *
> + * @val: action to pass into listener's notifier_call function
> + * @v: data pointer to pass into listener's notifier_call function
> + */
> +int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val,
> void *v);
> +
> Â #else /* CONFIG_IOSF_MBI is not enabled */
> Â static inline
> Â bool iosf_mbi_available(void)
> @@ -146,6 +182,24 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32
> offset, u32 mdr, u32 mask)
> Â static inline void iosf_mbi_punit_lock(void) {}
> Â static inline void iosf_mbi_punit_unlock(void) {}
> Â
> +static inline
> +int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block
> *nb)
> +{
> + return 0;
> +}
> +
> +static inline
> +int iosf_mbi_unregister_pmic_bus_access_notifier(struct
> notifier_block *nb)
> +{
> + return 0;
> +}
> +
> +static inline
> +int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val,
> void *v)
> +{
> + return 0;
> +}
> +
> Â #endif /* CONFIG_IOSF_MBI */
> Â
> Â #endif /* IOSF_MBI_SYMS_H */
> diff --git a/arch/x86/platform/intel/iosf_mbi.c
> b/arch/x86/platform/intel/iosf_mbi.c
> index 75d8135..a995789 100644
> --- a/arch/x86/platform/intel/iosf_mbi.c
> +++ b/arch/x86/platform/intel/iosf_mbi.c
> @@ -35,6 +35,7 @@
> Â static struct pci_dev *mbi_pdev;
> Â static DEFINE_SPINLOCK(iosf_mbi_lock);
> Â static DEFINE_MUTEX(iosf_mbi_punit_mutex);
> +static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
> Â
> Â static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
> Â {
> @@ -203,6 +204,41 @@ void iosf_mbi_punit_unlock(void)
> Â }
> Â EXPORT_SYMBOL(iosf_mbi_punit_unlock);
> Â
> +int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block
> *nb)
> +{
> + int ret;
> +
> + /* Wait for the bus to go inactive before registering */
> + mutex_lock(&iosf_mbi_punit_mutex);
> + ret = blocking_notifier_chain_register(
[PATCH 1/7] x86/platform/intel/iosf_mbi: Add a mutex for punit access
On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> One some systems the punit accesses the pmic to change various
> voltages
> through the same bus as other kernel drivers use for e.g. battery
> monitoring.
>
> If a driver sends requests to the punit which require the punit to
> access
> the pmic bus while another driver is also accessing the pmic bus
> various
> bad things happen.
>
> This commit adds a mutex to protect the punit against simultaneous
> accesses
> and 2 functions to lock / unlock this mutex.
>
> Note on these systems the i2c-bus driver will request a sempahore from
> the
> punit for exclusive access to the pmic bus when i2c drivers are
> accessing
> it, but this does not appear to be sufficient, we still need to avoid
> making certain punit requests during the access window to avoid
> problems.
I'm fine with the patch, but please spell
P-Unit
PMIC
Reviewed-by: Andy Shevchenko
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede
> Tested-by: tagorereddy
> ---
>  arch/x86/include/asm/iosf_mbi.h    | 31
> +++
> Â arch/x86/platform/intel/iosf_mbi.c | 13 +
> Â 2 files changed, 44 insertions(+)
>
> diff --git a/arch/x86/include/asm/iosf_mbi.h
> b/arch/x86/include/asm/iosf_mbi.h
> index b41ee16..91f5d16 100644
> --- a/arch/x86/include/asm/iosf_mbi.h
> +++ b/arch/x86/include/asm/iosf_mbi.h
> @@ -88,6 +88,33 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset,
> u32 mdr);
> Â */
> Â int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32
> mask);
> Â
> +/**
> + * iosf_mbi_punit_lock() - Lock the punit mutex
> + *
> + * One some systems the punit accesses the pmic to change various
> voltages
> + * through the same bus as other kernel drivers use for e.g. battery
> monitoring.
> + *
> + * If a driver sends requests to the punit which require the punit to
> access the
> + * pmic bus while another driver is also accessing the pmic bus
> various bad
> + * things happen.
> + *
> + * To avoid these problems this function must be called before
> accessing the
> + * punit or the pmic, be it through iosf_mbi* functions or through
> other means.
> + *
> + * Note on these systems the i2c-bus driver will request a sempahore
> from the
> + * punit for exclusive access to the pmic bus when i2c drivers are
> accessing it,
> + * but this does not appear to be sufficient, we still need to avoid
> making
> + * certain punit requests during the access window to avoid problems.
> + *
> + * This function locks a mutex, as such it may sleep.
> + */
> +void iosf_mbi_punit_lock(void);
> +
> +/**
> + * iosf_mbi_punit_unlock() - Unlock the punit mutex
> + */
> +void iosf_mbi_punit_unlock(void);
> +
> Â #else /* CONFIG_IOSF_MBI is not enabled */
> Â static inline
> Â bool iosf_mbi_available(void)
> @@ -115,6 +142,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32
> offset, u32 mdr, u32 mask)
> Â WARN(1, "IOSF_MBI driver not available");
> Â return -EPERM;
> Â }
> +
> +static inline void iosf_mbi_punit_lock(void) {}
> +static inline void iosf_mbi_punit_unlock(void) {}
> +
> Â #endif /* CONFIG_IOSF_MBI */
> Â
> Â #endif /* IOSF_MBI_SYMS_H */
> diff --git a/arch/x86/platform/intel/iosf_mbi.c
> b/arch/x86/platform/intel/iosf_mbi.c
> index edf2c54..75d8135 100644
> --- a/arch/x86/platform/intel/iosf_mbi.c
> +++ b/arch/x86/platform/intel/iosf_mbi.c
> @@ -34,6 +34,7 @@
> Â
> Â static struct pci_dev *mbi_pdev;
> Â static DEFINE_SPINLOCK(iosf_mbi_lock);
> +static DEFINE_MUTEX(iosf_mbi_punit_mutex);
> Â
> Â static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
> Â {
> @@ -190,6 +191,18 @@ bool iosf_mbi_available(void)
> Â }
> Â EXPORT_SYMBOL(iosf_mbi_available);
> Â
> +void iosf_mbi_punit_lock(void)
> +{
> + mutex_lock(&iosf_mbi_punit_mutex);
> +}
> +EXPORT_SYMBOL(iosf_mbi_punit_lock);
> +
> +void iosf_mbi_punit_unlock(void)
> +{
> + mutex_unlock(&iosf_mbi_punit_mutex);
> +}
> +EXPORT_SYMBOL(iosf_mbi_punit_unlock);
> +
> Â #ifdef CONFIG_IOSF_MBI_DEBUG
> Â static u32 dbg_mdr;
> Â static u32 dbg_mcr;
--
Andy Shevchenko
Intel Finland Oy
[RFC PATCH 0/3] staging: remove fbdev drivers
On Wed, Nov 23, 2016 at 12:05 PM, Thomas Petazzoni wrote: > On Wed, 23 Nov 2016 11:12:32 +0200, Tomi Valkeinen wrote: >> Or do you mean that we should keep the drivers in staging until there's >> a matching DRM driver, but drop any plans to move the drivers from >> staging to drivers/video/? If so, I'm fine with that. This is an RFC, >> mostly to raise some discussion and push people to actually write those >> DRM drivers =). > > The very reason why I submitted those drivers for staging is because > lots and lots of people were using out of tree kernel modules for these > drivers, which was really a pain. > > If you now remove those drivers from staging, then those folks will be > back in the situation they originally were, using annoying out of tree > modules. > > I'm all for removing fbtft drivers progressively as a matching > DRM-based driver is available for the same hardware. However, if there > is no DRM-based support for a given piece of hardware supported by > fbtft, I'd prefer if we kept the fbtft driver for this hardware. Too many people are playing with big things, I vote +1 to *leave* fbtft for people who prefer small on big. -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] drm/radeon: remove useless and potentially wrong message
There is no need to repeat information that printed by PCI core at boot time.
Besides that printing was potentially wrong since resource_size_t might be
bigger than 32 bits and there is a dedicated specifier for such type, i.e.
%pap. Someone can fix it and use even better approach, i.e. %pR.
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/radeon/radeon_device.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_device.c
b/drivers/gpu/drm/radeon/radeon_device.c
index 60a8920..67cd59a 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1440,11 +1440,8 @@ int radeon_device_init(struct radeon_device *rdev,
rdev->rmmio_size = pci_resource_len(rdev->pdev, 2);
}
rdev->rmmio = ioremap(rdev->rmmio_base, rdev->rmmio_size);
- if (rdev->rmmio == NULL) {
+ if (rdev->rmmio == NULL)
return -ENOMEM;
- }
- DRM_INFO("register mmio base: 0x%08X\n", (uint32_t)rdev->rmmio_base);
- DRM_INFO("register mmio size: %u\n", (unsigned)rdev->rmmio_size);
/* doorbell bar mapping */
if (rdev->family >= CHIP_BONAIRE)
--
2.10.2
[PATCH] apple-gmux: Sphinxify docs
On Mon, Jul 4, 2016 at 1:40 PM, Lukas Wunner wrote:
> Convert asciidoc-formatted docs to rst in accordance with Jonathan's and
> Jani's effort to use sphinx for kernel-doc rendering in 4.8.
Darren, I'm fine with this one. FWIW:
Reviewed-by: Andy Shevchenko
>
> Cc: Jonathan Corbet
> Cc: Jani Nikula
> Signed-off-by: Lukas Wunner
> ---
> drivers/platform/x86/apple-gmux.c | 55
> +--
> 1 file changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/platform/x86/apple-gmux.c
> b/drivers/platform/x86/apple-gmux.c
> index 4034d2d..a66be13 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -31,19 +31,21 @@
> /**
> * DOC: Overview
> *
> - * :1: http://www.latticesemi.com/en/Products/FPGAandCPLD/LatticeXP2.aspx
> - * :2: http://www.renesas.com/products/mpumcu/h8s/h8s2100/h8s2113/index.jsp
> - *
> * gmux is a microcontroller built into the MacBook Pro to support dual GPUs:
> - * A {1}[Lattice XP2] on pre-retinas, a {2}[Renesas R4F2113] on retinas.
> + * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on retinas.
> *
> * (The MacPro6,1 2013 also has a gmux, however it is unclear why since it
> has
> * dual GPUs but no built-in display.)
> *
> * gmux is connected to the LPC bus of the southbridge. Its I/O ports are
> * accessed differently depending on the microcontroller: Driver functions
> - * to access a pre-retina gmux are infixed `_pio_`, those for a retina gmux
> - * are infixed `_index_`.
> + * to access a pre-retina gmux are infixed ``_pio_``, those for a retina gmux
> + * are infixed ``_index_``.
> + *
> + * .. _Lattice XP2:
> + * http://www.latticesemi.com/en/Products/FPGAandCPLD/LatticeXP2.aspx
> + * .. _Renesas R4F2113:
> + * http://www.renesas.com/products/mpumcu/h8s/h8s2100/h8s2113/index.jsp
> */
>
> struct apple_gmux_data {
> @@ -272,15 +274,15 @@ static bool gmux_is_indexed(struct apple_gmux_data
> *gmux_data)
> /**
> * DOC: Backlight control
> *
> - * :3: http://www.ti.com/lit/ds/symlink/lp8543.pdf
> - * :4: http://www.ti.com/lit/ds/symlink/lp8545.pdf
> - *
> * On single GPU MacBooks, the PWM signal for the backlight is generated by
> * the GPU. On dual GPU MacBook Pros by contrast, either GPU may be suspended
> * to conserve energy. Hence the PWM signal needs to be generated by a
> separate
> * backlight driver which is controlled by gmux. The earliest generation
> - * MBP5 2008/09 uses a {3}[TI LP8543] backlight driver. All newer models
> - * use a {4}[TI LP8545].
> + * MBP5 2008/09 uses a `TI LP8543`_ backlight driver. All newer models
> + * use a `TI LP8545`_.
> + *
> + * .. _TI LP8543: http://www.ti.com/lit/ds/symlink/lp8543.pdf
> + * .. _TI LP8545: http://www.ti.com/lit/ds/symlink/lp8545.pdf
> */
>
> static int gmux_get_brightness(struct backlight_device *bd)
> @@ -312,28 +314,20 @@ static const struct backlight_ops gmux_bl_ops = {
> /**
> * DOC: Graphics mux
> *
> - * :5: http://pimg-fpiw.uspto.gov/fdd/07/870/086/0.pdf
> - * :6: http://www.nxp.com/documents/data_sheet/CBTL06141.pdf
> - * :7: http://www.ti.com/lit/ds/symlink/hd3ss212.pdf
> - * :8: https://www.pericom.com/assets/Datasheets/PI3VDP12412.pdf
> - * :9: http://www.ti.com/lit/ds/symlink/sn74lv4066a.pdf
> - * :10:
> http://pdf.datasheetarchive.com/indexerfiles/Datasheets-SW16/DSASW00308511.pdf
> - * :11: http://www.ti.com/lit/ds/symlink/ts3ds10224.pdf
> - *
> * On pre-retinas, the LVDS outputs of both GPUs feed into gmux which muxes
> * either of them to the panel. One of the tricks gmux has up its sleeve is
> * to lengthen the blanking interval of its output during a switch to
> * synchronize it with the GPU switched to. This allows for a flicker-free
> - * switch that is imperceptible by the user ({5}[US 8,687,007 B2]).
> + * switch that is imperceptible by the user (`US 8,687,007 B2`_).
> *
> * On retinas, muxing is no longer done by gmux itself, but by a separate
> * chip which is controlled by gmux. The chip is triple sourced, it is
> - * either an {6}[NXP CBTL06142], {7}[TI HD3SS212] or {8}[Pericom
> PI3VDP12412].
> + * either an `NXP CBTL06142`_, `TI HD3SS212`_ or `Pericom PI3VDP12412`_.
> * The panel is driven with eDP instead of LVDS since the pixel clock
> * required for retina resolution exceeds LVDS' limits.
> *
> * Pre-retinas are able to switch the panel's DDC pins separately.
> - * This is handled by a {9}[TI SN74LV4066A] which is controlled by gmux.
> + * This is handled by a `TI SN74LV4066A`_ which is controlled by gmux.
> * The inactive GPU can thus probe the panel's EDID without switching ove
[PATCH v1 06/10] device property: switch to use UUID API
On Fri, 2016-02-26 at 16:11 +0200, Andy Shevchenko wrote:
> On Thu, 2016-02-18 at 01:03 +0100, Rafael J. Wysocki wrote:
> >
> > On Wednesday, February 17, 2016 02:17:24 PM Andy Shevchenko wrote:
> > >
> > > Switch to use a generic UUID API instead of custom approach. It
> > > allows to
> > > define UUIDs, compare them, and validate.
> []
>
Summon initial author of the UUID library.
Summary: the API of comparison functions is rather strange. What the
point to not take pointers directly? (Moreover I hope compiler too
clever not to make a copy of constant arguments there)
I could only imagine the case you are trying to avoid temporary
variables for constants like NULL_UUID.
Issue with this is the ugliness in the users of that, in particularly
present in ACPI (drivers/acpi/apei/ghes.c).
I would like to have more clear interface for that. Perhaps we may add
something like
cmp_p(pointer, non-pointer);
cmp_pp(pointer, pointer);
to not break existing API for now.
It would be useful for many cases in the kernel.
> >
> > >
> > > +static const uuid_le ads_uuid =
> > > + UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6,
> > > + 0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
> > > Â
> > > Â static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
> > > Â Â Â Â const union
> > > acpi_object
> > > *desc,
> > > @@ -138,7 +136,7 @@ static bool
> > > acpi_enumerate_nondev_subnodes(acpi_handle scope,
> > > Â Â Â Â Â || links->type != ACPI_TYPE_PACKAGE)
> > > Â break;
> > > Â
> > > - if (memcmp(uuid->buffer.pointer, ads_uuid,
> > > sizeof(ads_uuid)))
> > > + if (uuid_le_cmp(*(uuid_le *)uuid->buffer.pointer,
> > > ads_uuid))
> > Maybe it's too late, but I don't quite understand the pointer
> > manipulations here.
> >
> > I can see why you need a type conversion (although it looks ugly),
> > but why do you
> > need to dereference it too?
> The function takes that kind of type on input. The other variants are
> not compiled.
> Perhaps we better change uuid_{lb}e_cmp() first to take normal
> pointers, though I think the initial idea was to get type checking at
> compile time.
>
--
Andy Shevchenko
Intel Finland Oy
[PATCH v1 05/10] ACPI: switch to use generic UUID API
Instead of opencoding the existing library functions let's use them directly.
The conversion fixes a potential bug in int340x_thermal as well since we have
to use memcmp() on binary data.
Signed-off-by: Andy Shevchenko
---
drivers/acpi/acpi_extlog.c| 8 +++---
drivers/acpi/bus.c| 29 ++-
drivers/acpi/nfit.c | 34 +++
drivers/acpi/nfit.h | 3 +-
drivers/acpi/utils.c | 4 +--
drivers/char/tpm/tpm_crb.c| 9 +++---
drivers/char/tpm/tpm_ppi.c| 20 ++---
drivers/gpu/drm/i915/intel_acpi.c | 14 --
drivers/gpu/drm/nouveau/nouveau_acpi.c| 20 ++---
drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c| 9 +++---
drivers/hid/i2c-hid/i2c-hid.c | 9 +++---
drivers/iommu/dmar.c | 11
drivers/pci/pci-acpi.c| 11
drivers/pci/pci-label.c | 4 +--
drivers/thermal/int340x_thermal/int3400_thermal.c | 6 ++--
drivers/usb/host/xhci-pci.c | 9 +++---
include/acpi/acpi_bus.h | 10 ---
include/linux/acpi.h | 2 +-
include/linux/pci-acpi.h | 2 +-
sound/soc/intel/skylake/skl-nhlt.c| 7 +++--
20 files changed, 92 insertions(+), 129 deletions(-)
diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index b3842ff..a3a25ec 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -45,7 +45,9 @@ struct extlog_l1_head {
static int old_edac_report_status;
-static u8 extlog_dsm_uuid[] __initdata =
"663E35AF-CC10-41A4-88EA-5470AF055295";
+static uuid_le extlog_dsm_uuid __initdata =
+ UUID_LE(0x663E35AF, 0xCC10, 0x41A4,
+ 0x88, 0xEA, 0x54, 0x70, 0xAF, 0x05, 0x52, 0x95);
/* L1 table related physical address */
static u64 elog_base;
@@ -182,12 +184,10 @@ out:
static bool __init extlog_get_l1addr(void)
{
- u8 uuid[16];
+ uuid_le *uuid = &extlog_dsm_uuid;
acpi_handle handle;
union acpi_object *obj;
- acpi_str_to_uuid(extlog_dsm_uuid, uuid);
-
if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
return false;
if (!acpi_check_dsm(handle, uuid, EXTLOG_DSM_REV, 1 << EXTLOG_FN_ADDR))
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 891c42d..80db48b 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -192,42 +192,19 @@ static void acpi_print_osc_error(acpi_handle handle,
printk("\n");
}
-acpi_status acpi_str_to_uuid(char *str, u8 *uuid)
-{
- int i;
- static int opc_map_to_uuid[16] = {6, 4, 2, 0, 11, 9, 16, 14, 19, 21,
- 24, 26, 28, 30, 32, 34};
-
- if (strlen(str) != 36)
- return AE_BAD_PARAMETER;
- for (i = 0; i < 36; i++) {
- if (i == 8 || i == 13 || i == 18 || i == 23) {
- if (str[i] != '-')
- return AE_BAD_PARAMETER;
- } else if (!isxdigit(str[i]))
- return AE_BAD_PARAMETER;
- }
- for (i = 0; i < 16; i++) {
- uuid[i] = hex_to_bin(str[opc_map_to_uuid[i]]) << 4;
- uuid[i] |= hex_to_bin(str[opc_map_to_uuid[i] + 1]);
- }
- return AE_OK;
-}
-EXPORT_SYMBOL_GPL(acpi_str_to_uuid);
-
acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
{
acpi_status status;
struct acpi_object_list input;
union acpi_object in_params[4];
union acpi_object *out_obj;
- u8 uuid[16];
+ uuid_le uuid;
u32 errors;
struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
if (!context)
return AE_ERROR;
- if (ACPI_FAILURE(acpi_str_to_uuid(context->uuid_str, uuid)))
+ if (uuid_le_to_bin(context->uuid_str, &uuid))
return AE_ERROR;
context->ret.length = ACPI_ALLOCATE_BUFFER;
context->ret.pointer = NULL;
@@ -237,7 +214,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct
acpi_osc_context *context)
input.pointer = in_params;
in_params[0].type = ACPI_TYPE_BUFFER;
in_params[0].buffer.length = 16;
- in_params[0].buffer.pointer = uuid;
+ in_params[0].buffer.pointer = (char *)&uuid;
in_params[1].type = ACPI_TYPE_INTEGER;
in_params[1].integer.value = context->rev;
in_params[2].type = ACPI_TYPE_INTEGER;
diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index ad6d8c6..3beb99b 100644
--- a/drivers/acpi/nfit.c
+++ b/dri
[PATCH v1 03/10] lib/uuid: introduce few more generic helpers for UUID
There are new helpers in this patch:
uuid_is_valid checks if a UUID is valid
uuid_be_to_bin converts from string to binary (big endian)
uuid_le_to_bin converts from string to binary (little endian)
They will be used in future, i.e. in the following patches in the series.
This also moves indices arrays to lib/uuid.c to be shared accross modules.
Signed-off-by: Andy Shevchenko
---
include/linux/uuid.h | 13 ++
lib/uuid.c | 70
lib/vsprintf.c | 9 +++
3 files changed, 87 insertions(+), 5 deletions(-)
diff --git a/include/linux/uuid.h b/include/linux/uuid.h
index 91c2b6d..616347f 100644
--- a/include/linux/uuid.h
+++ b/include/linux/uuid.h
@@ -22,6 +22,11 @@
#include
+/*
+ * The length of a UUID string ("----")
+ * not including trailing NUL.
+ */
+#defineUUID_STRING_LEN 36
static inline int uuid_le_cmp(const uuid_le u1, const uuid_le u2)
{
@@ -38,4 +43,12 @@ void generate_random_uuid(unsigned char uuid[16]);
extern void uuid_le_gen(uuid_le *u);
extern void uuid_be_gen(uuid_be *u);
+int __must_check uuid_is_valid(const char *uuid);
+
+extern const u8 uuid_le_index[16];
+extern const u8 uuid_be_index[16];
+
+int uuid_le_to_bin(const char *uuid, uuid_le *u);
+int uuid_be_to_bin(const char *uuid, uuid_be *u);
+
#endif
diff --git a/lib/uuid.c b/lib/uuid.c
index 6c81c0b..995865c 100644
--- a/lib/uuid.c
+++ b/lib/uuid.c
@@ -19,10 +19,17 @@
*/
#include
+#include
+#include
#include
#include
#include
+const u8 uuid_le_index[16] = {3,2,1,0,5,4,7,6,8,9,10,11,12,13,14,15};
+EXPORT_SYMBOL(uuid_le_index);
+const u8 uuid_be_index[16] = {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
+EXPORT_SYMBOL(uuid_be_index);
+
/***
* Random UUID interface
*
@@ -65,3 +72,66 @@ void uuid_be_gen(uuid_be *bu)
bu->b[6] = (bu->b[6] & 0x0F) | 0x40;
}
EXPORT_SYMBOL_GPL(uuid_be_gen);
+
+/**
+ * uuid_is_valid - checks if UUID string valid
+ * @uuid: UUID string to check
+ *
+ * Description:
+ * It checks if the UUID string is following the format:
+ *----
+ * where x is a hex digit.
+ *
+ * Return: 0 on success, %-EINVAL otherwise.
+ */
+int uuid_is_valid(const char *uuid)
+{
+ unsigned int i;
+
+ if (strnlen(uuid, UUID_STRING_LEN) < UUID_STRING_LEN)
+ return -EINVAL;
+
+ for (i = 0; i < UUID_STRING_LEN; i++) {
+ if (i == 8 || i == 13 || i == 18 || i == 23) {
+ if (uuid[i] != '-')
+ return -EINVAL;
+ } else if (!isxdigit(uuid[i])) {
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(uuid_is_valid);
+
+static int __uuid_to_bin(const char *uuid, __u8 b[16], const u8 ei[16])
+{
+ static const u8 si[16] = {0,2,4,6,9,11,14,16,19,21,24,26,28,30,32,34};
+ unsigned int i;
+ int ret;
+
+ ret = uuid_is_valid(uuid);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < 16; i++) {
+ int hi = hex_to_bin(uuid[si[i]] + 0);
+ int lo = hex_to_bin(uuid[si[i]] + 1);
+
+ b[ei[i]] = (hi << 4) | lo;
+ }
+
+ return 0;
+}
+
+int uuid_le_to_bin(const char *uuid, uuid_le *u)
+{
+ return __uuid_to_bin(uuid, u->b, uuid_le_index);
+}
+EXPORT_SYMBOL(uuid_le_to_bin);
+
+int uuid_be_to_bin(const char *uuid, uuid_be *u)
+{
+ return __uuid_to_bin(uuid, u->b, uuid_be_index);
+}
+EXPORT_SYMBOL(uuid_be_to_bin);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 17d976b..752e78d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#include
#ifdef CONFIG_BLOCK
#include
@@ -1304,19 +1305,17 @@ static noinline_for_stack
char *uuid_string(char *buf, char *end, const u8 *addr,
struct printf_spec spec, const char *fmt)
{
- char uuid[sizeof("----")];
+ char uuid[UUID_STRING_LEN + 1];
char *p = uuid;
int i;
- static const u8 be[16] = {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
- static const u8 le[16] = {3,2,1,0,5,4,7,6,8,9,10,11,12,13,14,15};
- const u8 *index = be;
+ const u8 *index = uuid_be_index;
bool uc = false;
switch (*(++fmt)) {
case 'L':
uc = true; /* fall-through */
case 'l':
- index = le;
+ index = uuid_le_index;
break;
case 'B':
uc = true;
--
2.7.0
[PATCH v1 00/10] uuid: convert users to generic UUID API
There are few fumctions here and there along with type definitions that provide UUID API. This series consolidates everything under one hood and converts current users. This has been tested for a while internally, however it doesn't mean we covered all possible cases (especially accuracy of UUID constants after conversion). So, please test this as much as you can and provide your tag. We appreciate the effort. Andy Shevchenko (10): lib/vsprintf: simplify UUID printing lib/uuid: move generate_random_uuid() to uuid.c lib/uuid: introduce few more generic helpers for UUID lib/uuid: remove FSF address ACPI: switch to use generic UUID API device property: switch to use UUID API sysctl: drop away useless label sysctl: use generic UUID library efi: redefine type, constant, macro from generic code efivars: use generic UUID library drivers/acpi/acpi_extlog.c| 8 +- drivers/acpi/bus.c| 29 +-- drivers/acpi/nfit.c | 34 drivers/acpi/nfit.h | 3 +- drivers/acpi/property.c | 18 ++--- drivers/acpi/utils.c | 4 +- drivers/char/random.c | 21 + drivers/char/tpm/tpm_crb.c| 9 +-- drivers/char/tpm/tpm_ppi.c| 20 ++--- drivers/gpu/drm/i915/intel_acpi.c | 14 ++-- drivers/gpu/drm/nouveau/nouveau_acpi.c| 20 +++-- drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c| 9 +-- drivers/hid/i2c-hid/i2c-hid.c | 9 +-- drivers/iommu/dmar.c | 11 ++- drivers/pci/pci-acpi.c| 11 ++- drivers/pci/pci-label.c | 4 +- drivers/thermal/int340x_thermal/int3400_thermal.c | 6 +- drivers/usb/host/xhci-pci.c | 9 +-- fs/btrfs/volumes.c| 2 +- fs/efivarfs/inode.c | 40 +- fs/ext4/ioctl.c | 1 + fs/f2fs/file.c| 2 +- fs/reiserfs/objectid.c| 2 +- fs/ubifs/sb.c | 2 +- include/acpi/acpi_bus.h | 10 ++- include/linux/acpi.h | 2 +- include/linux/efi.h | 14 +--- include/linux/pci-acpi.h | 2 +- include/linux/random.h| 1 - include/linux/uuid.h | 21 +++-- include/uapi/linux/uuid.h | 4 - kernel/sysctl_binary.c| 30 +++ lib/uuid.c| 96 +-- lib/vsprintf.c| 21 ++--- sound/soc/intel/skylake/skl-nhlt.c| 7 +- 35 files changed, 237 insertions(+), 259 deletions(-) -- 2.7.0
