Re: [PATCH v4] MAINTAINERS: move Milo Kim to credits

[Emil Velikov]

Greetings everyone,

On Mon, 15 Feb 2021 at 08:52, Lee Jones  wrote:
>
> On Fri, 12 Feb 2021, Krzysztof Kozlowski wrote:
>
> > Milo Kim's email in TI bounces with permanent error (550: Invalid
> > recipient).  Last email from him on LKML was in 2017.  Move Milo Kim to
> > credits and remove the separate driver entries for:
> >  - TI LP855x backlight driver,
> >  - TI LP8727 charger driver,
> >  - TI LP8788 MFD (ADC, LEDs, charger and regulator) drivers.
> >
> > Signed-off-by: Krzysztof Kozlowski 
> > Cc: Mark Brown 
> > Cc: Jonathan Cameron 
> > Cc: Jingoo Han 
> > Cc: Lee Jones 
> > Cc: Pavel Machek 
> > Cc: Thierry Reding 
> > Cc: Sebastian Reichel 
> > Cc: Daniel Thompson 
> >
> > ---
> >
> > Dear Lee,
> >
> > Could you take care about this patch?
>
> Yes, but I'll be sending out my pull-request for v5.12 in the next
> couple of days (maybe even today if I can find some time), so this
> will have to wait until v5.13.
>
Would it make sense to keep the MAINTAINERS entries as "orphan"?
Checking with linux-next, the drivers are still present in-tree.

HTH
-Emil

Re: [PATCH v3] kcmp: Support selection of SYS_kcmp without CHECKPOINT_RESTORE

[Emil Velikov]

On Fri, 12 Feb 2021 at 14:01, Michel Dänzer  wrote:
>
> On 2021-02-12 1:57 p.m., Emil Velikov wrote:
> > On Fri, 5 Feb 2021 at 22:01, Chris Wilson  wrote:
> >>
> >> Userspace has discovered the functionality offered by SYS_kcmp and has
> >> started to depend upon it. In particular, Mesa uses SYS_kcmp for
> >> os_same_file_description() in order to identify when two fd (e.g. device
> >> or dmabuf)
> >
> > As you rightfully point out, SYS_kcmp is a bit of a two edged sword.
> > While you mention the CONFIG issue, there is also a portability aspect
> > (mesa runs on more than just linux) and as well as sandbox filtering
> > of the extra syscall.
> >
> > Last time I looked, the latter was still an issue and mesa was using
> > SYS_kcmp to compare device node fds.
> > A far shorter and more portable solution is possible, so let me
> > prepare a Mesa patch.
>
> Make sure to read my comments on
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6881 first. :)
>
Much appreciated. I might have been "slightly" off - pardon for the noise o/

-Emil

Re: [PATCH v3] kcmp: Support selection of SYS_kcmp without CHECKPOINT_RESTORE

[Emil Velikov]

On Fri, 12 Feb 2021 at 13:14, Simon Ser  wrote:
>
> On Friday, February 12th, 2021 at 1:57 PM, Emil Velikov 
>  wrote:
>
> > On Fri, 5 Feb 2021 at 22:01, Chris Wilson  wrote:
> > >
> > > Userspace has discovered the functionality offered by SYS_kcmp and has
> > > started to depend upon it. In particular, Mesa uses SYS_kcmp for
> > > os_same_file_description() in order to identify when two fd (e.g. device
> > > or dmabuf)
> >
> > As you rightfully point out, SYS_kcmp is a bit of a two edged sword.
> > While you mention the CONFIG issue, there is also a portability aspect
> > (mesa runs on more than just linux) and as well as sandbox filtering
> > of the extra syscall.
> >
> > Last time I looked, the latter was still an issue and mesa was using
> > SYS_kcmp to compare device node fds.
> > A far shorter and more portable solution is possible, so let me
> > prepare a Mesa patch.
>
> Comparing two DMA-BUFs can be done with their inode number, I think.
>
> Comparing two device FDs is more subtle, because of GEM handle
> ref'counting. You sometimes really want to check whether two FDs are
> backed by the same file *description*. See [1] for details.
>
Thanks for the correction and the reference.
Seems like I've short circuited file description table vs file descriptor.

Emil

Re: [PATCH v3] kcmp: Support selection of SYS_kcmp without CHECKPOINT_RESTORE

[Emil Velikov]

On Fri, 5 Feb 2021 at 22:01, Chris Wilson  wrote:
>
> Userspace has discovered the functionality offered by SYS_kcmp and has
> started to depend upon it. In particular, Mesa uses SYS_kcmp for
> os_same_file_description() in order to identify when two fd (e.g. device
> or dmabuf)

As you rightfully point out, SYS_kcmp is a bit of a two edged sword.
While you mention the CONFIG issue, there is also a portability aspect
(mesa runs on more than just linux) and as well as sandbox filtering
of the extra syscall.

Last time I looked, the latter was still an issue and mesa was using
SYS_kcmp to compare device node fds.
A far shorter and more portable solution is possible, so let me
prepare a Mesa patch.

-Emil

Re: module loader dead code removal and cleanups v3

[Emil Velikov]

Hi Jessica,

On Tue, 2 Feb 2021 at 14:37, Jessica Yu  wrote:
>
> +++ Christoph Hellwig [02/02/21 13:13 +0100]:
> >Hi all,
> >
> >this series removes support for long term unused export types and
> >cleans up various loose ends in the module loader.
> >
> >Changes since v2:
> > - clean up klp_find_object_symbol a bit
> > - remove the now unused module_assert_mutex helper
> >
> >Changes since v1:
> > - move struct symsearch to module.c
> > - rework drm to not call find_module at all
> > - allow RCU-sched locking for find_module
> > - keep find_module as a public API instead of module_loaded
> > - update a few comments and commit logs
>
> Thanks Christoph for cleaning up all that aged cruft, and thanks everyone
> for the reviews.
>
> I was curious about EXPORT_SYMBOL_GPL_FUTURE and EXPORT_UNUSED_SYMBOL
> variants, and found that most of that stuff was introduced between
> 2006 - 2008. All the of the unused symbols were removed and gpl future
> symbols were converted to gpl quite a long time ago, and I don't
> believe these export types have been used ever since. So I
> think it's safe to retire those export types now.
>
I believe you're spot on - based on reading through git log and
checking the ML archives.

Shame I didn't get to finish a similar series I had locally. Patches
11-13 match what I have here so:
Reviewed-by: Emil Velikov 

HTH
-Emil

Re: linux-next: build failure after merge of the drm-misc tree

[Emil Velikov]

Hi Stephen,

On Wed, 17 Jun 2020 at 08:03, Stephen Rothwell  wrote:
>
> Hi Thomas,
>
> On Wed, 17 Jun 2020 08:33:24 +0200 Thomas Zimmermann  
> wrote:
> >
> > We recently dropped the _unlock() suffix from drm_gem_object_put(). This
> > patch should be ok.
>
> Yes, but what it shows is that the drm-misc tree is still based on
> v5.7-rc1 and v5.8-rc1 has about 16000 more commits for you to get
> conflicts against :-)
>
Being the culprit here - thanks for the patience and report.

I believe that both AMD and drm-misc teams are aware of this lovely
situation I've put them in.
As you mentioned drm-misc is a bit special and doing the usual
backmerge will be fun.

If you have any tips on how to minimise such issues, I'd gladly utilise them.

Thanks again,
-Emil

Re: [PATCH v2 0/5] 180 degrees rotation support for NVIDIA Tegra DRM

[Emil Velikov]

On Tue, 16 Jun 2020 at 18:20, Dmitry Osipenko  wrote:
>
> 16.06.2020 18:48, Emil Velikov пишет:
> > On Tue, 16 Jun 2020 at 12:40, Dmitry Osipenko  wrote:
> >>
> >> 16.06.2020 01:26, Emil Velikov пишет:
> >>> Hi Dmitry,
> >>>
> >>> On Mon, 15 Jun 2020 at 08:28, Dmitry Osipenko  wrote:
> >>>>
> >>>> Hello!
> >>>>
> >>>> This series adds 180° display plane rotation support to the NVIDIA Tegra
> >>>> DRM driver which is needed for devices that have display panel physically
> >>>> mounted upside-down, like Nexus 7 tablet device for example [1]. Since
> >>>> DRM panel rotation is a new thing for a userspace, currently only
> >>>> Opentegra Xorg driver handles the rotated display panel [2], but this
> >>>> is good enough for the start.
> >>>>
> >>>> Note that later on it should be possible to implement a transparent 180°
> >>>> display rotation for Tegra DRM driver which will remove the need to have
> >>>> a bleeding edge userspace that knows how to rotate display planes and I'm
> >>>> slowly working on it. For the starter we can go with the minimal rotation
> >>>> support, so it's not a blocker.
> >>>>
> >>>> This series is based on the work that was made by Derek Basehore for the
> >>>> Mediatek driver [3], his patch is included into this patchset. I added
> >>>> my tested-by tag to the Derek's patch.
> >>>>
> >>>> Please review and apply, thanks in advance!
> >>>>
> >>>> [1] 
> >>>> https://patchwork.ozlabs.org/project/linux-tegra/patch/20200607154327.18589-3-dig...@gmail.com/
> >>>> [2] 
> >>>> https://github.com/grate-driver/xf86-video-opentegra/commit/28eb20a3959bbe5bc3a3b67e55977093fd5114ca
> >>>> [3] https://lkml.org/lkml/2020/3/5/1119
> >>>>
> >>>> Changelog:
> >>>>
> >>>> v2: - Dropped "drm/panel: Set display info in panel attach" patch, which
> >>>>   turned out to be obsolete now.
> >>>>
> >>>> - Renamed the cover-latter, hopefully this will fix the bouncing 
> >>>> emails.
> >>>>
> >>>> Derek Basehore (1):
> >>>>   drm/panel: Add helper for reading DT rotation
> >>>>
> >>>> Dmitry Osipenko (4):
> >>>>   drm/panel: lvds: Set up panel orientation
> >>>
> >>> IMHO it's perfectly reasonable to report the panel orientation to
> >>> userspace, which can apply plane rotation as needed.
> >>>
> >>> Although I see that this series, alike Derek's, has a couple of issues:
> >>>  - only a single panel driver is updated
> >>>  - rotation is _not_ listed as supported property, in said panel
> >>> driver device-tree bindings
> >>>
> >>> My personal inclination is that we should aim for a comprehensive 
> >>> solution:
> >>>  - wire all panel drivers, as currently documented (quick grep list below)
> >>>  - document and wire-up the lvds and boe panels - as proposed by you
> >>> and Derek respectively
> >>>
> >>> HTH
> >>> Emil
> >>>
> >>> Documentation/devicetree/bindings/display/himax,hx8357d.txt:2
> >>> Documentation/devicetree/bindings/display/ilitek,ili9225.txt:2
> >>> Documentation/devicetree/bindings/display/ilitek,ili9341.txt:2
> >>> Documentation/devicetree/bindings/display/ilitek,ili9486.yaml:2
> >>> Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt:2
> >>> Documentation/devicetree/bindings/display/panel/panel-common.yaml:2
> >>> Documentation/devicetree/bindings/display/sitronix,st7586.txt:1
> >>> Documentation/devicetree/bindings/display/sitronix,st7735r.yaml:2
> >>
> >> Rotation is a common DT panel property that is described in the
> >> panel-common.yaml.
> > The property was introduced almost exclusively for tiny drm panels.
> > Those ones are a bit different from the rest (in panel/) -
> > MIPI-DBI/SPI w/o (not connected at least) an actual GPU.
> >
> > To make it a bit better, the rotation is seemingly performed in the
> > tiny driver itself ouch.
> >
> >> This property is supported by all panel bindings
> >> because these bindings inherent the common properties from the
> >> panel-common.ya

Re: [PATCH v2 0/5] 180 degrees rotation support for NVIDIA Tegra DRM

[Emil Velikov]

On Tue, 16 Jun 2020 at 12:40, Dmitry Osipenko  wrote:
>
> 16.06.2020 01:26, Emil Velikov пишет:
> > Hi Dmitry,
> >
> > On Mon, 15 Jun 2020 at 08:28, Dmitry Osipenko  wrote:
> >>
> >> Hello!
> >>
> >> This series adds 180° display plane rotation support to the NVIDIA Tegra
> >> DRM driver which is needed for devices that have display panel physically
> >> mounted upside-down, like Nexus 7 tablet device for example [1]. Since
> >> DRM panel rotation is a new thing for a userspace, currently only
> >> Opentegra Xorg driver handles the rotated display panel [2], but this
> >> is good enough for the start.
> >>
> >> Note that later on it should be possible to implement a transparent 180°
> >> display rotation for Tegra DRM driver which will remove the need to have
> >> a bleeding edge userspace that knows how to rotate display planes and I'm
> >> slowly working on it. For the starter we can go with the minimal rotation
> >> support, so it's not a blocker.
> >>
> >> This series is based on the work that was made by Derek Basehore for the
> >> Mediatek driver [3], his patch is included into this patchset. I added
> >> my tested-by tag to the Derek's patch.
> >>
> >> Please review and apply, thanks in advance!
> >>
> >> [1] 
> >> https://patchwork.ozlabs.org/project/linux-tegra/patch/20200607154327.18589-3-dig...@gmail.com/
> >> [2] 
> >> https://github.com/grate-driver/xf86-video-opentegra/commit/28eb20a3959bbe5bc3a3b67e55977093fd5114ca
> >> [3] https://lkml.org/lkml/2020/3/5/1119
> >>
> >> Changelog:
> >>
> >> v2: - Dropped "drm/panel: Set display info in panel attach" patch, which
> >>   turned out to be obsolete now.
> >>
> >> - Renamed the cover-latter, hopefully this will fix the bouncing 
> >> emails.
> >>
> >> Derek Basehore (1):
> >>   drm/panel: Add helper for reading DT rotation
> >>
> >> Dmitry Osipenko (4):
> >>   drm/panel: lvds: Set up panel orientation
> >
> > IMHO it's perfectly reasonable to report the panel orientation to
> > userspace, which can apply plane rotation as needed.
> >
> > Although I see that this series, alike Derek's, has a couple of issues:
> >  - only a single panel driver is updated
> >  - rotation is _not_ listed as supported property, in said panel
> > driver device-tree bindings
> >
> > My personal inclination is that we should aim for a comprehensive solution:
> >  - wire all panel drivers, as currently documented (quick grep list below)
> >  - document and wire-up the lvds and boe panels - as proposed by you
> > and Derek respectively
> >
> > HTH
> > Emil
> >
> > Documentation/devicetree/bindings/display/himax,hx8357d.txt:2
> > Documentation/devicetree/bindings/display/ilitek,ili9225.txt:2
> > Documentation/devicetree/bindings/display/ilitek,ili9341.txt:2
> > Documentation/devicetree/bindings/display/ilitek,ili9486.yaml:2
> > Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt:2
> > Documentation/devicetree/bindings/display/panel/panel-common.yaml:2
> > Documentation/devicetree/bindings/display/sitronix,st7586.txt:1
> > Documentation/devicetree/bindings/display/sitronix,st7735r.yaml:2
>
> Rotation is a common DT panel property that is described in the
> panel-common.yaml.
The property was introduced almost exclusively for tiny drm panels.
Those ones are a bit different from the rest (in panel/) -
MIPI-DBI/SPI w/o (not connected at least) an actual GPU.

To make it a bit better, the rotation is seemingly performed in the
tiny driver itself ouch.

> This property is supported by all panel bindings
> because these bindings inherent the common properties from the
> panel-common.yaml.
>
Seems like that was an unintentional change with the conversion to YAML.
Beforehand only a few selected panels had rotation. Upon closer look -
some panels do have follow-up fixes, to remove/limit the implicit
inclusion.

Sam seems like you've done most of the YAML conversion. IMHO it would
make sense to revisit the patches and inherit common properties only
as applicable.

> I don't think that it makes sense to wire up rotation property to all
> panel drivers at once because those drivers will be untested, at least I
> don't know anything about those other panels and can't test them. It
> will be much better to support the rotation on by as-needed basis for
> each panel driver individually.

How about CCing the author and reviewer asking them to test the patch?
The only place where the patches might cause an issue is with tiny,
although patches would still be appreciated.

-Emil

Re: [PATCH 0/6] remove deprecated i2c_new_device API

[Emil Velikov]

Hi all,

On Tue, 16 Jun 2020 at 13:12, Daniel Vetter  wrote:
>
> On Mon, Jun 15, 2020 at 09:58:09AM +0200, Wolfram Sang wrote:
> > I want to remove the above API this cycle, and just a few patches have
> > not made it into 5.8-rc1. They have been reviewed and most had been
> > promised to get into linux-next, but well, things happen. So, I hope it
> > is okay for everyone to collect them like this and push them via I2C for
> > 5.8-rc2.
>
> for the drm side of things:
>
> Acked-by: Daniel Vetter 
> >
> > One minor exception is the media documentation patch which I simply have
> > missed so far, but it is trivial.
> >
> > And then, finally, there is the removal of the old API as the final
> > patch. Phew, that's been a long ride.
> >
> > I am open for comments, of course.
> >
> > Happy hacking,
> >
> >Wolfram
> >
> >
> > Wolfram Sang (6):
> >   drm: encoder_slave: fix refcouting error for modules
> >   drm: encoder_slave: use new I2C API

The first two are in drm-misc-next and are to be expected with the 5.9
merge window. As long as that doesn't cause major nuisance proceed as
you prefer.

-Emil

Re: [PATCH v2 0/5] 180 degrees rotation support for NVIDIA Tegra DRM

[Emil Velikov]

Hi Dmitry,

On Mon, 15 Jun 2020 at 08:28, Dmitry Osipenko  wrote:
>
> Hello!
>
> This series adds 180° display plane rotation support to the NVIDIA Tegra
> DRM driver which is needed for devices that have display panel physically
> mounted upside-down, like Nexus 7 tablet device for example [1]. Since
> DRM panel rotation is a new thing for a userspace, currently only
> Opentegra Xorg driver handles the rotated display panel [2], but this
> is good enough for the start.
>
> Note that later on it should be possible to implement a transparent 180°
> display rotation for Tegra DRM driver which will remove the need to have
> a bleeding edge userspace that knows how to rotate display planes and I'm
> slowly working on it. For the starter we can go with the minimal rotation
> support, so it's not a blocker.
>
> This series is based on the work that was made by Derek Basehore for the
> Mediatek driver [3], his patch is included into this patchset. I added
> my tested-by tag to the Derek's patch.
>
> Please review and apply, thanks in advance!
>
> [1] 
> https://patchwork.ozlabs.org/project/linux-tegra/patch/20200607154327.18589-3-dig...@gmail.com/
> [2] 
> https://github.com/grate-driver/xf86-video-opentegra/commit/28eb20a3959bbe5bc3a3b67e55977093fd5114ca
> [3] https://lkml.org/lkml/2020/3/5/1119
>
> Changelog:
>
> v2: - Dropped "drm/panel: Set display info in panel attach" patch, which
>   turned out to be obsolete now.
>
> - Renamed the cover-latter, hopefully this will fix the bouncing emails.
>
> Derek Basehore (1):
>   drm/panel: Add helper for reading DT rotation
>
> Dmitry Osipenko (4):
>   drm/panel: lvds: Set up panel orientation

IMHO it's perfectly reasonable to report the panel orientation to
userspace, which can apply plane rotation as needed.

Although I see that this series, alike Derek's, has a couple of issues:
 - only a single panel driver is updated
 - rotation is _not_ listed as supported property, in said panel
driver device-tree bindings

My personal inclination is that we should aim for a comprehensive solution:
 - wire all panel drivers, as currently documented (quick grep list below)
 - document and wire-up the lvds and boe panels - as proposed by you
and Derek respectively

HTH
Emil

Documentation/devicetree/bindings/display/himax,hx8357d.txt:2
Documentation/devicetree/bindings/display/ilitek,ili9225.txt:2
Documentation/devicetree/bindings/display/ilitek,ili9341.txt:2
Documentation/devicetree/bindings/display/ilitek,ili9486.yaml:2
Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt:2
Documentation/devicetree/bindings/display/panel/panel-common.yaml:2
Documentation/devicetree/bindings/display/sitronix,st7586.txt:1
Documentation/devicetree/bindings/display/sitronix,st7735r.yaml:2

Re: [PATCH v2 5/5] drm/tegra: plane: Support 180° rotation

[Emil Velikov]

Hi all,

Perhaps a silly question:

On Mon, 15 Jun 2020 at 08:28, Dmitry Osipenko  wrote:
>
> Combining horizontal and vertical reflections gives us 180 degrees of
> rotation.
>
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/gpu/drm/tegra/dc.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index f31bca27cde4..ddd9b88f8fce 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c

> +   if (rotation & DRM_MODE_ROTATE_180) {
> +   plane_state->reflect_x = !plane_state->reflect_x;
> +   plane_state->reflect_y = !plane_state->reflect_y;
> +   }
> +
As mentioned by Ville the above is already handled by
drm_rotation_simplify() ... although it makes me wonder:


> err = drm_plane_create_rotation_property(>base,
>  DRM_MODE_ROTATE_0,
>  DRM_MODE_ROTATE_0 |
> +DRM_MODE_ROTATE_180 |
>  DRM_MODE_REFLECT_X |
>  DRM_MODE_REFLECT_Y);

Would it make sense for drm_plane_create_rotation_property() itself,
to add DRM_MODE_ROTATE_180, when both reflections are supported?

-Emil

Re: (EXT) Re: [PATCH 3/4] drm/panel: simple: add CDTech S070PWS19HP-FC21 and S070SWV29HG-DC44

[Emil Velikov]

Hi Matthias,

On Thu, 11 Jun 2020 at 08:54, Matthias Schiffer
 wrote:
> On Wed, 2020-06-10 at 16:59 +0200, Sam Ravnborg wrote:
> > On Wed, Jun 10, 2020 at 02:01:30PM +0200, Matthias Schiffer wrote:

> > > +   .vrefresh = 60,
> >
> > .vrefresh is no longer present, please drop.
>
> I based my patches on the branch drm-next of
> https://cgit.freedesktop.org/drm/drm, should I have used a different
> branch?
>
Small tip for the future:

The best way to find the correct tree is to check with the MAINTAINERS
file or via ./scripts/get_maintainer --scm ...

-Emil

Re: [PATCH 1/2] video: fbdev: amifb: remove dead APUS support

[Emil Velikov]

Hi Bartlomiej,

On Tue, 2 Jun 2020 at 11:37, Bartlomiej Zolnierkiewicz
 wrote:
>
>
> On 5/14/20 10:21 PM, Geert Uytterhoeven wrote:
>
> > These #ifdefs are relics from APUS (Amiga Power-Up System), which
> > added a PPC board.  APUS support was killed off a long time ago,
> > when arch/ppc/ was still king, but these #ifdefs were missed, because
> > they didn't test for CONFIG_APUS.
>
> Reported-by: Al Viro 
> Reported-by: Geert Uytterhoeven 
> Signed-off-by: Bartlomiej Zolnierkiewicz 
> ---
>  drivers/video/fbdev/amifb.c |   63 
> 
>  1 file changed, 63 deletions(-)
>
A quick look through my checkout (drm-misc/next aka 5.8 ish), shows
multiple other places which check for the define.
And a single place where it's being set - the Makefile below.

Should those be addressed as well? Or perhaps they are and I've got an old tree.

HTH
Emil

$ git grep -c __mc68000__
arch/m68k/Makefile:1
drivers/block/floppy.c:2
drivers/ide/ide-probe.c:2
drivers/input/misc/hp_sdc_rtc.c:1
drivers/input/serio/hp_sdc.c:3
drivers/input/serio/hp_sdc_mlc.c:1
drivers/net/ethernet/i825xx/82596.c:8
drivers/tty/vt/keyboard.c:1
drivers/video/fbdev/amifb.c:11
include/linux/a.out.h:1
include/linux/hp_sdc.h:1
include/uapi/linux/a.out.h:1
lib/fonts/fonts.c:2
lib/mpi/longlong.h:1

Re: [PATCH v3 1/2] linux/bits.h: fix unsigned less than zero warnings

[Emil Velikov]

Hi Rikard,

On Mon, 8 Jun 2020 at 23:18, Rikard Falkeborn
 wrote:
>
> When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
> an unsigned unknown high bit, some gcc versions warn due to the
> comparisons of the high and low bit in GENMASK_INPUT_CHECK.
>
> To silence the warnings, only perform the check if both inputs are
> known. This does not trigger any warnings, from the Wtype-limits help:
>
> Warn if a comparison is always true or always false due to the
> limited range of the data type, but do not warn for constant
> expressions.
>
> As an example of the warning, kindly reported by the kbuild test robot:
>
> from drivers/mfd/atmel-smc.c:11:
> drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is 
> always false [-Wtype-limits]
> 26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> |^
> include/linux/build_bug.h:16:62: note: in definition of macro 
> 'BUILD_BUG_ON_ZERO'
> 16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> |  ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> 39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> |   ^~~
> >> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
> 49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
> | ^~~
>
> Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK 
> inputs")
> Reported-by: kbuild test robot 
> Reported-by: Emil Velikov 
> Reported-by: Syed Nayyar Waris 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Rikard Falkeborn 

This version is far better than my approach. Fwiw:

Reviewed-by: Emil Velikov 

Thanks
Emil

Re: [PATCH] alpha: Fix build around srm_sysrq_reboot_op

[Emil Velikov]

Hi all,

On Thu, 11 Jun 2020 at 10:11, Joerg Roedel  wrote:
>
> From: Joerg Roedel 
>
> The patch introducing the struct was probably never compile tested,
> because it sets a handler with a wrong function signature. Wrap the
> handler into a functions with the correct signature to fix the build.
>
> Fixes: 0f1c9688a194 ("tty/sysrq: alpha: export and use __sysrq_get_key_op()")
> Cc: Emil Velikov 
> Cc: Guenter Roeck 
> Signed-off-by: Joerg Roedel 

Just coming back from 10 offline days and retracing my testing ...
Seems that I've copied the wrong .config.
So a follow-up `make arch/alpha/kernel/setup.o' did not build
anything... yet the command was _successful_.

This is something that should be fixed IMHO - doing the same for
drivers/ (for example drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.o)
results in an obvious audible error.

That said, apologies for the issue. I'll be more thorough and
carefully audit the build logs for future patches.

-Emil

Re: [v2] drm/msm: add shutdown support for display platform_driver

[Emil Velikov]

On Tue, 2 Jun 2020 at 17:10, Sai Prakash Ranjan
 wrote:
>
> Hi Emil,
>
> On 2020-06-02 21:09, Emil Velikov wrote:
> > On Tue, 2 Jun 2020 at 15:49, Sai Prakash Ranjan
> >  wrote:
> >>
> >> Hi Emil,
> >>
> >> On 2020-06-02 19:43, Emil Velikov wrote:
> >> > Hi Krishna,
> >> >
> >> > On Tue, 2 Jun 2020 at 08:17, Krishna Manikandan
> >> >  wrote:
> >> >>
> >> >> Define shutdown callback for display drm driver,
> >> >> so as to disable all the CRTCS when shutdown
> >> >> notification is received by the driver.
> >> >>
> >> >> This change will turn off the timing engine so
> >> >> that no display transactions are requested
> >> >> while mmu translations are getting disabled
> >> >> during reboot sequence.
> >> >>
> >> >> Signed-off-by: Krishna Manikandan 
> >> >>
> >> > AFAICT atomics is setup in msm_drm_ops::bind and shutdown in
> >> > msm_drm_ops::unbind.
> >> >
> >> > Are you saying that unbind never triggers? If so, then we should
> >> > really fix that instead, since this patch seems more like a
> >> > workaround.
> >> >
> >>
> >> Which path do you suppose that the unbind should be called from,
> >> remove
> >> callback? Here we are talking about the drivers which are builtin,
> >> where
> >> remove callbacks are not called from the driver core during
> >> reboot/shutdown,
> >> instead shutdown callbacks are called which needs to be defined in
> >> order
> >> to
> >> trigger unbind. So AFAICS there is nothing to be fixed.
> >>
> > Interesting - in drm effectively only drm panels implement .shutdown.
> > So my naive assumption was that .remove was used implicitly by core,
> > as part of the shutdown process. Yet that's not the case, so it seems
> > that many drivers could use some fixes.
> >
> > Then again, that's an existing problem which is irrelevant for msm.
> > -Emil
>
> To give more context, we are actually targeting the clients/consumers
> of SMMU/IOMMU here because we have to make sure that before the supplier
> (SMMU) shuts down, its consumers/clients need to be shutdown properly.
> Now the ordering of this is taken care in the SMMU driver via
> device_link
> which makes sure that consumer shutdown callbacks are called first, but
> we
> need to define shutdown callbacks for all its consumers to make sure we
> actually shutdown the clients or else it would invite the crashes during
> reboot
> which in this case was seen for display.
>
Thank you very much for the extra details. I think other DRM drivers
will be safe as-is.

-Emil

Re: [v2] drm/msm: add shutdown support for display platform_driver

[Emil Velikov]

On Tue, 2 Jun 2020 at 15:49, Sai Prakash Ranjan
 wrote:
>
> Hi Emil,
>
> On 2020-06-02 19:43, Emil Velikov wrote:
> > Hi Krishna,
> >
> > On Tue, 2 Jun 2020 at 08:17, Krishna Manikandan
> >  wrote:
> >>
> >> Define shutdown callback for display drm driver,
> >> so as to disable all the CRTCS when shutdown
> >> notification is received by the driver.
> >>
> >> This change will turn off the timing engine so
> >> that no display transactions are requested
> >> while mmu translations are getting disabled
> >> during reboot sequence.
> >>
> >> Signed-off-by: Krishna Manikandan 
> >>
> > AFAICT atomics is setup in msm_drm_ops::bind and shutdown in
> > msm_drm_ops::unbind.
> >
> > Are you saying that unbind never triggers? If so, then we should
> > really fix that instead, since this patch seems more like a
> > workaround.
> >
>
> Which path do you suppose that the unbind should be called from, remove
> callback? Here we are talking about the drivers which are builtin, where
> remove callbacks are not called from the driver core during
> reboot/shutdown,
> instead shutdown callbacks are called which needs to be defined in order
> to
> trigger unbind. So AFAICS there is nothing to be fixed.
>
Interesting - in drm effectively only drm panels implement .shutdown.
So my naive assumption was that .remove was used implicitly by core,
as part of the shutdown process. Yet that's not the case, so it seems
that many drivers could use some fixes.

Then again, that's an existing problem which is irrelevant for msm.
-Emil

Re: [v2] drm/msm: add shutdown support for display platform_driver

[Emil Velikov]

Hi Krishna,

On Tue, 2 Jun 2020 at 08:17, Krishna Manikandan  wrote:
>
> Define shutdown callback for display drm driver,
> so as to disable all the CRTCS when shutdown
> notification is received by the driver.
>
> This change will turn off the timing engine so
> that no display transactions are requested
> while mmu translations are getting disabled
> during reboot sequence.
>
> Signed-off-by: Krishna Manikandan 
>
AFAICT atomics is setup in msm_drm_ops::bind and shutdown in
msm_drm_ops::unbind.

Are you saying that unbind never triggers? If so, then we should
really fix that instead, since this patch seems more like a
workaround.

-Emil

Re: [PATCH 1/7] drm/rockchip: prepare common code for cdns and rk dpi/dp driver

[Emil Velikov]

HI Sandor Yu

On Mon, 1 Jun 2020 at 07:29,  wrote:
>
> From: Sandor Yu 
>
> - Extracted common fields from cdn_dp_device to a new cdns_mhdp_device
>   structure which will be used by two separate drivers later on.
> - Moved some datatypes (audio_format, audio_info, vic_pxl_encoding_format,
>   video_info) from cdn-dp-core.c to cdn-dp-reg.h.
> - Changed prefixes from cdn_dp to cdns_mhdp
> cdn -> cdns to match the other Cadence's drivers
> dp -> mhdp to distinguish it from a "just a DP" as the IP underneath
>   this registers map can be a HDMI (which is internally different,
>   but the interface for commands, events is pretty much the same).
> - Modified cdn-dp-core.c to use the new driver structure and new function
>   names.
> - writel and readl are replaced by cdns_mhdp_bus_write and
>   cdns_mhdp_bus_read.
>
The high-level idea is great - split, refactor and reuse the existing drivers.

Although looking at the patches themselves - they seems to be doing
multiple things at once.
As indicated by the extensive list in the commit log.

I would suggest splitting those up a bit, roughly in line of the
itemisation as per the commit message.

Here is one hand wavy way to chunk this patch:
 1) use put_unalligned*
 2) 'use local variable dev' style of changes (as seem in cdn_dp_clk_enable)
 3) add writel/readl wrappers
 4) hookup struct cdns_mhdp_device, keep dp->mhdp detail internal.
The cdn-dp-reg.h function names/signatures will stay the same.
 5) finalize the helpers - use mhdp directly, rename

HTH
Emil

Examples:
4)
 static int cdn_dp_mailbox_read(struct cdn_dp_device *dp)
 {
+"  struct cdns_mhdp_device *mhdp = dp->mhdp;
   int val, ret;

-  ret = readx_poll_timeout(readl, dp->regs + MAILBOX_EMPTY_ADDR,
+  ret = readx_poll_timeout(readl, mhdp->regs_base + MAILBOX_EMPTY_ADDR,
...
   return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
 }

5)
-static int cdn_dp_mailbox_read(struct cdn_dp_device *dp)
+static int mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
 {
-  struct cdns_mhdp_device *mhdp = dp->mhdp;
   int val, ret;
...
-  return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
+  return cdns_mhdp_bus_read(mhdp, MAILBOX0_RD_DATA) & 0xff;
 }

Re: [Linux-stm32] [PATCH v8 08/10] drm: stm: dw-mipi-dsi: let the bridge handle the HW version check

[Emil Velikov]

Hi Adrian,

On Mon, 1 Jun 2020 at 10:14, Adrian Ratiu  wrote:
>
> On Fri, 29 May 2020, Philippe CORNU  wrote:
> > Hi Adrian, and thank you very much for the patchset.  Thank you
> > also for having tested it on STM32F769 and STM32MP1.  Sorry for
> > the late response, Yannick and I will review it as soon as
> > possible and we will keep you posted.  Note: Do not hesitate to
> > put us in copy for the next version  (philippe.co...@st.com,
> > yannick.fer...@st.com) Regards, Philippe :-)
>
> Hi Philippe,
>
> Thank you very much for your previous and future STM testing,
> really appreciate it! I've CC'd Yannick until now but I'll also CC
> you sure :)
>
> It's been over a month since I posted v8 and I was just gearing up
> to address all feedback, rebase & retest to prepare v9 but I'll
> wait a little longer, no problem, it's no rush.
>
Small idea, pardon for joining so late:

Might be a good idea to add inline comment, why the clocks are disabled so late.
Effectively a 2 line version of the commit summary.

Feel free to make that a separate/follow-up patch.

-Emil

Re: [PATCH] drm/vkms: Optimize compute_crc(), blend()

[Emil Velikov]

On Mon, 1 Jun 2020 at 01:25, Rodrigo Siqueira
 wrote:
>
> Hi,
>
> First of all, thanks a lot for all your patch. And thanks Emil for your
> feedback.
>
> I have a suggestion here:
>
> Emil:
> Could you give me your Acked-by or maybe Reviewed-by for the writeback
> series? With that, I can finally apply the series.
>
Sure, once the issues highlighted are resolved. Just left you some
more comprehensive feedback.

-Emil
P.S. Something something top posting sucks :-P

Re: [PATCH V4 3/3] drm/vkms: Add support for writeback

[Emil Velikov]

On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira  wrote:

>  # SPDX-License-Identifier: GPL-2.0-only
> -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o 
> vkms_composer.o
> +vkms-y := \
> +   vkms_drv.o \
> +   vkms_plane.o \
> +   vkms_output.o \
> +   vkms_crtc.o \
> +   vkms_gem.o \
> +   vkms_composer.o \
> +   vkms_writeback.o
>
Nit: sort alphabetically


> @@ -191,9 +192,12 @@ void vkms_composer_worker(struct work_struct *work)
> if (!primary_composer)
> return;
>
> +   if (wb_pending)
> +   vaddr_out = crtc_state->active_writeback;
> +
> ret = compose_planes(_out, primary_composer, cursor_composer);
Forgot to mention earlier - with the allocation happening in the
caller, compose_planes() can take void *vaddr_out.

> if (ret) {
> -   if (ret == -EINVAL)
> +   if (ret == -EINVAL && !wb_pending)
> kfree(vaddr_out);
> return;
> }
> @@ -206,6 +210,14 @@ void vkms_composer_worker(struct work_struct *work)
> while (frame_start <= frame_end)
> drm_crtc_add_crc_entry(crtc, true, frame_start++, );
>
> +   if (wb_pending) {
> +   drm_writeback_signal_completion(>wb_connector, 0);
> +   spin_lock_irq(>composer_lock);
> +   crtc_state->wb_pending = false;
> +   spin_unlock_irq(>composer_lock);
> +   return;
> +   }
> +
Just like the free() move this above the drm_crtc_add_crc_entry()

if (wb_pending) {
  signal()
  ...
} else {
  free()
}

> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 1e8b2169d834..34dd74dc8eb0 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -39,6 +39,10 @@ bool enable_cursor = true;
>  module_param_named(enable_cursor, enable_cursor, bool, 0444);
>  MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
>
> +bool enable_writeback;
> +module_param_named(enable_writeback, enable_writeback, bool, 0444);
> +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> +
Why is this a module parameter? Let's always enable it and leave it to
userspace whether to use the wb or not.

>  static const struct file_operations vkms_driver_fops = {
> .owner  = THIS_MODULE,
> .open   = drm_open,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index f4036bb0b9a8..35f0b71413c9 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -8,6 +8,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #define XRES_MIN20
>  #define YRES_MIN20
> @@ -19,6 +20,7 @@
>  #define YRES_MAX  8192
>
>  extern bool enable_cursor;
> +extern bool enable_writeback;
>
>  struct vkms_composer {
> struct drm_framebuffer fb;
> @@ -52,9 +54,11 @@ struct vkms_crtc_state {
> int num_active_planes;
> /* stack of active planes for crc computation, should be in z order */
> struct vkms_plane_state **active_planes;
> +   void *active_writeback;
>
> /* below three are protected by vkms_output.composer_lock */
Nit: s/below three/the following four/

> bool crc_pending;
> +   bool wb_pending;
> u64 frame_start;
> u64 frame_end;
>  };
> @@ -63,6 +67,7 @@ struct vkms_output {
> struct drm_crtc crtc;
> struct drm_encoder encoder;
> struct drm_connector connector;
> +   struct drm_writeback_connector wb_connector;
> struct hrtimer vblank_hrtimer;
> ktime_t period_ns;
> struct drm_pending_vblank_event *event;
> @@ -144,4 +149,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const 
> char *source_name,
>  /* Composer Support */
>  void vkms_composer_worker(struct work_struct *work);
>
> +/* Writeback */
> +int enable_writeback_connector(struct vkms_device *vkmsdev);

Don't forget appropriate name spacing - prefix the function with vkms.

> +
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c 
> b/drivers/gpu/drm/vkms/vkms_output.c
> index 85afb77e97f0..19ffc67affec 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -80,6 +80,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int 
> index)
> goto err_attach;
> }
>
> +   if (enable_writeback) {
> +   ret = enable_writeback_connector(vkmsdev);

With wb connector always enabled, you can kill off the
vkms_output::composer_enabled all together. Plus the info/error
warnings (below) can go as well.

> +   if (!ret) {
> +   output->composer_enabled = true;
> +   DRM_INFO("Writeback connector enabled");
> +   } else {
> +   DRM_ERROR("Failed to init writeback connector\n");
> +   }
> 

Re: [PATCH V4 2/3] drm/vkms: Compute CRC without change input data

[Emil Velikov]

On Tue, 12 May 2020 at 12:34, Emil Velikov  wrote:
>
> Hi Rodrigo,
>
> On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira  
> wrote:
> >
> > From: Rodrigo Siqueira 
> >
> > The compute_crc() function is responsible for calculating the
> > framebuffer CRC value; due to the XRGB format, this function has to
> > ignore the alpha channel during the CRC computation. Therefore,
> > compute_crc() set zero to the alpha channel directly in the input
> > framebuffer, which is not a problem since this function receives a copy
> > of the original buffer. However, if we want to use this function in a
> > context without a buffer copy, it will change the initial value. This
> > patch makes compute_crc() calculate the CRC value without modifying the
> > input framebuffer.
> >
> > Signed-off-by: Rodrigo Siqueira 
> > ---
> >  drivers/gpu/drm/vkms/vkms_composer.c | 31 +---
> >  1 file changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c 
> > b/drivers/gpu/drm/vkms/vkms_composer.c
> > index 258e659ecfba..686d25e7b01d 100644
> > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > @@ -9,33 +9,40 @@
> >
> >  #include "vkms_drv.h"
> >
> > +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> > +const struct vkms_composer *composer)
> > +{
> > +   int src_offset = composer->offset + (y * composer->pitch)
> > + + (x * composer->cpp);
> > +
> > +   return *(u32 *)[src_offset];
> > +}
> > +
> >  /**
> >   * compute_crc - Compute CRC value on output frame
> >   *
> > - * @vaddr_out: address to final framebuffer
> > + * @vaddr: address to final framebuffer
> >   * @composer: framebuffer's metadata
> >   *
> >   * returns CRC value computed using crc32 on the visible portion of
> >   * the final framebuffer at vaddr_out
> >   */
> > -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer 
> > *composer)
> > +static uint32_t compute_crc(const u8 *vaddr,
> > +   const struct vkms_composer *composer)
> >  {
> > -   int i, j, src_offset;
> > +   int x, y;
> > int x_src = composer->src.x1 >> 16;
> > int y_src = composer->src.y1 >> 16;
> > int h_src = drm_rect_height(>src) >> 16;
> > int w_src = drm_rect_width(>src) >> 16;
> > -   u32 crc = 0;
> > +   u32 crc = 0, pixel = 0;
> >
> > -   for (i = y_src; i < y_src + h_src; ++i) {
> > -   for (j = x_src; j < x_src + w_src; ++j) {
> > -   src_offset = composer->offset
> > -+ (i * composer->pitch)
> > -+ (j * composer->cpp);
> > +   for (y = y_src; y < y_src + h_src; ++y) {
> > +   for (x = x_src; x < x_src + w_src; ++x) {
> > /* XRGB format ignores Alpha channel */
> > -   memset(vaddr_out + src_offset + 24, 0,  8);
> > -   crc = crc32_le(crc, vaddr_out + src_offset,
> > -  sizeof(u32));
> > +   pixel = get_pixel_from_buffer(x, y, vaddr, 
> > composer);
> > +   bitmap_clear((void *), 0, 8);
> > +   crc = crc32_le(crc, (void *), sizeof(u32));
> > }
> > }
> >
> IMHO using something like the following makes the code far simpler and 
> clearer.
>
> offset = composer->offset + (y_src * composer->pitch) + (x_src * 
> composer->cpp);
>
> for (i = 0; i < h_src; i++, offset += composer->pitch) {
>for (j = 0; j < w_src; j++, offset += composer->cpp) {
>   pixel = get_pixel_from_buffer(vaddr, offset);
>   crc = crc32_le(crc, , sizeof(u32); // cast should not be needed
>}
> }
>
> With the bitmap_clear() and related comment moved into 
> get_pixel_from_buffer().
>
If you fold the bitmap_clear() in get_pixel_from_buffer(), and drop
the cast (unless I'm missing something and it's really needed) for
crc32_le() this patch is:

Reviewed-by: Emil Velikov 

I would suggest (but it's not a requirement) that you simplify the
loop/offset calculation as separate patch in v5.

-Emil

Re: [PATCH V4 1/3] drm/vkms: Decouple crc operations from composer

[Emil Velikov]

Hi Rodrigo,

On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira  wrote:

> -static uint32_t _vkms_get_crc(struct vkms_composer *primary_composer,
> - struct vkms_composer *cursor_composer)
> +static int compose_planes(void **vaddr_out,
> + struct vkms_composer *primary_composer,
> + struct vkms_composer *cursor_composer)
>  {
> struct drm_framebuffer *fb = _composer->fb;
> struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> -   void *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
> -   u32 crc = 0;
>
> -   if (!vaddr_out) {
> -   DRM_ERROR("Failed to allocate memory for output frame.");
> -   return 0;
> +   if (!*vaddr_out) {
> +   *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
It would be clearer if you keep the to alloc (or not for wb) in the
caller. As-is it's very subtle and error prone.

> +   if (!*vaddr_out) {
> +   DRM_ERROR("Cannot allocate memory for output frame.");
> +   return -ENOMEM;
> +   }
> }
>
> -   if (WARN_ON(!vkms_obj->vaddr)) {
> -   kfree(vaddr_out);
> -   return crc;
> -   }
> +   if (WARN_ON(!vkms_obj->vaddr))
> +   return -EINVAL;
>
> -   memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> +   memcpy(*vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
>
> if (cursor_composer)
> -   compose_cursor(cursor_composer, primary_composer, vaddr_out);
> +   compose_cursor(cursor_composer, primary_composer, *vaddr_out);
>
> -   crc = compute_crc(vaddr_out, primary_composer);
> -
> -   kfree(vaddr_out);
> -
> -   return crc;
> +   return 0;
>  }
>
>  /**
> @@ -157,9 +153,11 @@ void vkms_composer_worker(struct work_struct *work)
> struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> struct vkms_composer *primary_composer = NULL;
> struct vkms_composer *cursor_composer = NULL;
> +   void *vaddr_out = NULL;
> u32 crc32 = 0;
> u64 frame_start, frame_end;
> bool crc_pending;
> +   int ret;
>
> spin_lock_irq(>composer_lock);
> frame_start = crtc_state->frame_start;
> @@ -183,14 +181,25 @@ void vkms_composer_worker(struct work_struct *work)
> if (crtc_state->num_active_planes == 2)
> cursor_composer = crtc_state->active_planes[1]->composer;
>
> -   if (primary_composer)
> -   crc32 = _vkms_get_crc(primary_composer, cursor_composer);
> +   if (!primary_composer)
> +   return;
> +
This early return changes the functionality. Namely the
drm_crtc_add_crc_entry( 0) is now missing. I don't recall much
about the crc to judge if that's a genuine bugfix, or newly introduced
bug.

In the former case, it should be a separate patch.

> +   ret = compose_planes(_out, primary_composer, cursor_composer);
> +   if (ret) {
> +   if (ret == -EINVAL)
> +   kfree(vaddr_out);
> +   return;
> +   }
> +
> +   crc32 = compute_crc(vaddr_out, primary_composer);
>
> /*
>  * The worker can fall behind the vblank hrtimer, make sure we catch 
> up.
>  */
> while (frame_start <= frame_end)
> drm_crtc_add_crc_entry(crtc, true, frame_start++, );
> +
> +   kfree(vaddr_out);
Nit: move the free() just after compute_crc() - it's not needed for
the drm_crtc_add_crc_entry().

-Emil

Re: [PATCH] drm/vkms: Optimize compute_crc(), blend()

[Emil Velikov]

On Sun, 31 May 2020 at 14:12, Sidong Yang  wrote:
>
> Optimize looping pixels in compute_crc() and blend(). Calculate
> src_offset in start of looping horizontally and increase it.
> It's better than calculating in every pixels.
>
When you say "optimize" have you observed any actual benefits of the
patch - be that smaller binary, faster execution time, etc?
If there are - mentioned them in the commit message. Otherwise, it
doesn't optimise anything.

A while back, I've suggested something similar [1] mostly for cosmetic
purposes - doubt there's much benefits beyond that.

HTH
-Emil
[1] https://patchwork.freedesktop.org/patch/365177/#comment_674314

Re: [PATCH v3 066/105] drm/vc4: txp: Turn the TXP into a CRTC of its own

[Emil Velikov]

Hi Maxime,

Have you considered splitting the series into several parts and
focusing on merging one at a time?
IIRC this the longest series _ever_ submitted to dri-devel, plus it
seems to be growing with each revision.

Due to the sheer volume, it's likely to miss various points - large or
small (like below).

On Thu, 28 May 2020 at 08:47, Maxime Ripard  wrote:

> +static int vc4_txp_enable_vblank(struct drm_crtc *crtc)
> +{
> +   return 0;
> +}
> +
> +static void vc4_txp_disable_vblank(struct drm_crtc *crtc) {}
> +
Core should handle if these are NULL, so the stubs should not be needed.

HTH
Emil

Re: [PATCH] drm: rcar-du: Fix build error

[Emil Velikov]

On Tue, 19 May 2020 at 08:01, Daniel Gomez  wrote:
>
> Select DRM_KMS_HELPER dependency.
>
> Build error when DRM_KMS_HELPER is not selected:
>
> drivers/gpu/drm/rcar-du/rcar_lvds.o:(.rodata+0xd48): undefined reference to 
> `drm_atomic_helper_bridge_duplicate_state'
> drivers/gpu/drm/rcar-du/rcar_lvds.o:(.rodata+0xd50): undefined reference to 
> `drm_atomic_helper_bridge_destroy_state'
> drivers/gpu/drm/rcar-du/rcar_lvds.o:(.rodata+0xd70): undefined reference to 
> `drm_atomic_helper_bridge_reset'
> drivers/gpu/drm/rcar-du/rcar_lvds.o:(.rodata+0xdc8): undefined reference to 
> `drm_atomic_helper_connector_reset'
> drivers/gpu/drm/rcar-du/rcar_lvds.o:(.rodata+0xde0): undefined reference to 
> `drm_helper_probe_single_connector_modes'
> drivers/gpu/drm/rcar-du/rcar_lvds.o:(.rodata+0xe08): undefined reference to 
> `drm_atomic_helper_connector_duplicate_state'
> drivers/gpu/drm/rcar-du/rcar_lvds.o:(.rodata+0xe10): undefined reference to 
> `drm_atomic_helper_connector_destroy_state'
>
> Signed-off-by: Daniel Gomez 
Nicely spotted.
AFAICT the only way this ever worked is if people had
DRM_FBDEV_EMULATION, which is unset in your case.

Cc: 
Reviewed-by: Emil Velikov 

I suspect Laurent will pick this in due time.
-Emil

Re: [PATCH 11/12] gpu/drm: Ingenic: Add support for the IPU

[Emil Velikov]

Hi Paul,

Disclaimer: I don't know much about the hardware :-P

On Sun, 17 May 2020 at 00:31, Paul Cercueil  wrote:
>
> Add support for the Image Processing Unit (IPU) found in all Ingenic
> SoCs.
>
Since the IPU is present on all devices supported, does it make sense
to have it as separate module?
Didn't check closely although I suspect doing that will remove the
need for the component patch.

> --- a/drivers/gpu/drm/ingenic/ingenic-drm.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm.c
> @@ -50,7 +50,7 @@ struct jz_soc_info {
>
>  struct ingenic_drm {
> struct drm_device drm;
> -   struct drm_plane f0, f1;
> +   struct drm_plane f0, f1, *ipu_plane;
> struct drm_crtc crtc;
> struct drm_encoder encoder;
>
> @@ -186,13 +186,16 @@ static void ingenic_drm_crtc_update_timings(struct 
> ingenic_drm *priv,
> regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
>JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16,
>JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16);
> +
> +   regmap_write(priv->map, JZ_REG_LCD_IPUR, JZ_LCD_IPUR_IPUREN |
> +(ht * vpe / 3) << JZ_LCD_IPUR_IPUR_LSB);

This hunk also indicates that it may be better to merge the IPU within
the existing driver.

-Emil

Re: [PATCH 0/2] drm: encoder_slave: some updates

[Emil Velikov]

On Wed, 13 May 2020 at 10:35, Emil Velikov  wrote:
>
> Hi Wolfram,
>
> On Wed, 13 May 2020 at 10:10, Wolfram Sang
>  wrote:
> >
> > On Mon, Mar 16, 2020 at 05:39:05PM +0100, Wolfram Sang wrote:
> > > While converting I2C users to new APIs, I found a refcounting problem in
> > > the encoder_slave implementation. This series fixes it and converts to
> > > the new API.
> > >
> > > Based on linux-next and only build tested.
> > >
> > > Wolfram Sang (2):
> > >   drm: encoder_slave: fix refcouting error for modules
> > >   drm: encoder_slave: use new I2C API
> > >
> > >  drivers/gpu/drm/drm_encoder_slave.c | 15 +--
> > >  1 file changed, 5 insertions(+), 10 deletions(-)
> >
> > Is there someone I should add to the CC list maybe?
> >
> The series is:
> Reviewed-by: Emil Velikov 
>
> Unless someone beats me to it, I'll commit them to drm-misc later today.
>
And after a short delay, pushed to drm-misc-next.
Thanks for the patches Wolfram.

-Emil

Re: get_maintainer.pl: unexpected behaviour for path/to//file

[Emil Velikov]

On Fri, 15 May 2020 at 18:22, Joe Perches  wrote:
>
> On Fri, 2020-05-15 at 05:31 -0700, Joe Perches wrote:
> > On Fri, 2020-05-15 at 11:52 +0100, Emil Velikov wrote:
> > > Hi Joe,
> > >
> > > Recently I've noticed that get_maintainer behaves differently if there
> > > is a double, sequential, forward slash in the path.
> > >
> > > AFAICT there should be no distinction between the two. Or at least many
> > > existing applications and scripts consider them one and the same.
> > >
> > > I've tried fixing this, although my perl isn't quite up-to scratch.
> > > Is this some weird bug or some intended feature?
> >
> > Not really an intended feature.
> > The code counts slashes for directory depth.
> >
> > I suppose it might be simpler to do this:
>
> Or perhaps a better alternative is:
> ---
>  scripts/get_maintainer.pl | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index 6d973f3685f9..484d2fbf5921 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -19,6 +19,7 @@ my $V = '0.26';
>  use Getopt::Long qw(:config no_auto_abbrev);
>  use Cwd;
>  use File::Find;
> +use File::Spec::Functions;
>
>  my $cur_path = fastgetcwd() . '/';
>  my $lk_path = "./";
> @@ -532,6 +533,7 @@ if (!@ARGV) {
>
>  foreach my $file (@ARGV) {
>  if ($file ne "") {
> +   $file = canonpath($file);

This seems like the better option since it also handles path traversal.
I would expect that people don't use it, yet who knows.

Thanks for the prompt fix.
-Emil

Re: [PATCHv1 03/19] power: supply: core: add manufacture date properties

[Emil Velikov]

On 2020/05/15, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, May 15, 2020 at 03:47:32PM +0100, Emil Velikov wrote:
> > On 2020/05/13, Sebastian Reichel wrote:
> > > Some smart batteries store their manufacture date, which is
> > > useful to identify the battery and/or to know about the cell
> > > quality.
> > > 
> > Have you considered exposing this as a single file? Say following
> > the ISO8601 format - -MM-DD.
> 
> Yes. My initial implementation was exactly that. The thing is, that
> I suspect some fuel gauge implementations may only expose the year
> or just year + month. I chose 3 files, since receiving '-MM'
> instead of full ISO code might be more unexpected than not having
> the DAY file available. But I don't have a strong opinion on this.
> 
Fwiw the ISO 8601 allows for -MM, although you're spot on. Having
the three fields separate makes perfect sense.

Thanks
Emil

Re: [PATCHv1 13/19] power: supply: sbs-battery: add POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED support

[Emil Velikov]

On 2020/05/13, Sebastian Reichel wrote:
> Add support for reporting the SBS battery's condition flag
> to userspace using the new "Calibration required" health status.
> 
> Signed-off-by: Sebastian Reichel 
> ---
>  drivers/power/supply/sbs-battery.c | 27 ---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/sbs-battery.c 
> b/drivers/power/supply/sbs-battery.c
> index 4fa553d61db2..2a2b926ad75c 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -23,6 +23,7 @@
>  
>  enum {
>   REG_MANUFACTURER_DATA,
> + REG_BATTERY_MODE,
>   REG_TEMPERATURE,
>   REG_VOLTAGE,
>   REG_CURRENT_NOW,
> @@ -94,6 +95,8 @@ static const struct chip_data {
>  } sbs_data[] = {
>   [REG_MANUFACTURER_DATA] =
>   SBS_DATA(POWER_SUPPLY_PROP_PRESENT, 0x00, 0, 65535),
> + [REG_BATTERY_MODE] =
> + SBS_DATA(-1, 0x03, 0, 65535),

Fwiw I really like how neatly the driver is split into components. One thing
which makes me wonder, have you considered reshuffling the sbs_data struct.

In particular:
 - index POWER_SUPPLY_PROP, kill off the REG_ enum
   - sbs_get_property_index() can go, alongside a couple of unreachable paths
   - replace batter_mode (needs calibration) with with PROP_HEALTH + comment
   - perhaps even add REG_ADDR_SPEC_INFO 0x1a under POWER_SUPPLY_PROP_PRESENT
 - using the min/max seems wasteful, considering only one register is in s16
   range while everything else is within u16


Regardless of the questions and trivial suggestions, the series looks spot on.

For the lot:
Reviewed-by: Emil Velikov 


-Emil
P.S. The reg table is nearly complete only 0x01-0x07, 0x0E, 0x11, 0x1d-0x1f
remain o/

Re: [PATCHv1 15/19] power: supply: sbs-battery: add ability to disable charger broadcasts

[Emil Velikov]

On 2020/05/13, Sebastian Reichel wrote:
> From: Jean-Francois Dagenais 
> 
> In certain designs, it is possible to add a battery on a populated i2c
> bus without an sbs compliant charger. In that case, the battery will
> un-necessarily and sometimes un-desirably master the bus trying to write
> info in the charger.

Nit: s/un-/un/

> 
> It is observed in many occasion that these battery "broadcasts" are even
> corrupting other ongoing master to slave communication. I.e. the
> multi-master support in the battery is inadequate.
> 
> Thankfully, the CHARGER_MODE bit allows designers to disable that SBS
> battery behaviour.
> 
> This needs to be done once when the battery is first seen on the bus.
> 
> Signed-off-by: Jean-Francois Dagenais 
> [rebased code]
> Signed-off-by: Sebastian Reichel 
> ---

> @@ -1017,6 +1043,9 @@ static int sbs_probe(struct i2c_client *client,
>   }
>   chip->i2c_retry_count = chip->i2c_retry_count + 1;
>  
> + chip->charger_broadcasts = !of_property_read_bool(client->dev.of_node,
> + "sbs,disable-charger-broadcasts");
> +
This patch adds the of_property_read, only for it to be replaced in the next
patch. Consider flipping the patch order?

-Emil

Re: [PATCHv1 03/19] power: supply: core: add manufacture date properties

[Emil Velikov]

Hi Sebastian,

On 2020/05/13, Sebastian Reichel wrote:
> Some smart batteries store their manufacture date, which is
> useful to identify the battery and/or to know about the cell
> quality.
> 
Have you considered exposing this as a single file? Say following the ISO8601
format - -MM-DD.

-Emil

Re: [PATCHv1 01/19] kobject: increase allowed number of uevent variables

[Emil Velikov]

On 2020/05/13, Sebastian Reichel wrote:
> SBS battery driver exposes 32 power supply properties now,
> which will result in uevent failure on (un)plugging the
> battery. Other drivers (e.g. bq27xxx) are also coming close
> to this limit, so increase it.
> 
> Signed-off-by: Sebastian Reichel 
> ---
>  include/linux/kobject.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index e2ca0a292e21..75e822569e39 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -29,7 +29,7 @@
>  #include 
>  
>  #define UEVENT_HELPER_PATH_LEN   256
> -#define UEVENT_NUM_ENVP  32  /* number of env 
> pointers */
> +#define UEVENT_NUM_ENVP  64  /* number of env 
> pointers */

To be on the safe side I've checked systemd/udev. It's using ordered hashmap,
so it's perfectly capable of handling the extra entries.

Reviewed-by: Emil Velikov 

-Emil

Re: [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature

[Emil Velikov]

Hi Sebastian,

I've left a few trivial suggestions, although I suspect only one of them
really matters. Namely - I think as-is the code changes the legacy behaviour
when OF is missing.

Mind you, this is my third time skimming through power/supply, so take it with
a grain of salt.

On 2020/05/13, Sebastian Reichel wrote:
> Add new charge-current-limit feature to gpio-charger. This also
> makes the online status GPIO optional, since hardware might only
> expose the charge-current-limit feature and there is no good reason
> to have it mandatory now that different GPIOs are supported.
> 
> Signed-off-by: Sebastian Reichel 
> ---
>  .../bindings/power/supply/gpio-charger.txt|  11 +-
>  drivers/power/supply/gpio-charger.c   | 176 --
>  2 files changed, 174 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt 
> b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> index 0fb33b2c62a6..dbfd29029f69 100644
> --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> @@ -2,8 +2,6 @@ gpio-charger
>  
>  Required properties :
>   - compatible : "gpio-charger"
> - - gpios : GPIO indicating the charger presence.
> -   See GPIO binding in bindings/gpio/gpio.txt .
>   - charger-type : power supply type, one of
>   unknown
>   battery
> @@ -15,7 +13,13 @@ Required properties :
>   usb-aca (USB accessory charger adapter)
>  
>  Optional properties:
> + - gpios : GPIO indicating the charger presence.
> +   See GPIO binding in bindings/gpio/gpio.txt .
>   - charge-status-gpios: GPIO indicating whether a battery is charging.
> + - charge-current-limit-gpios: Output GPIOs specifiers for limiting the 
> charge current
> + - charge-current-limit-mapping: List of touples with current in uA and a 
> GPIO bitmap (in this order).
> +The GPIOs are encoded in the same order as 
> specified in charge-current-limit-gpios.
> + The touples must be provided in descending 
> order of the current limit.

Minor tweaks:

List of tuples with current in uA and a GPIO bitmap.
Tuples must be sorted in descending order of the current limit.
GPIOs are encoded in the order as specified in 
charge-current-limit-gpios.


> +static int init_charge_current_limit(struct device *dev,
> + struct gpio_charger *gpio_charger)
> +{
> + int i, len;
> + u32 cur_limit = U32_MAX;
> +
> + gpio_charger->current_limit_gpios = devm_gpiod_get_array_optional(dev,
> + "charge-current-limit", GPIOD_OUT_LOW);
> + if (IS_ERR(gpio_charger->current_limit_gpios)) {
> + dev_err(dev, "error getting current-limit GPIOs\n");
> + return PTR_ERR(gpio_charger->current_limit_gpios);
> + }
> +
> + if (!gpio_charger->current_limit_gpios)
> + return 0;
> +
> + len = device_property_read_u32_array(dev, 
> "charge-current-limit-mapping",
> + NULL, 0);
> + if (len < 0)

The properly is optional, although I'm not sure if having an 'empty' properly
(len == 0) should be considered an error as indicated by -ENOMEM below or not.

Worth documenting that, unless it's covered already.

> + return len;
> +
> + if (len % 2) {
> + dev_err(dev, "invalid charge-current-limit-mapping length\n");
> + return -EINVAL;
> + }
> +
> + gpio_charger->current_limit_map = devm_kmalloc_array(dev,
> + len / 2, sizeof(*gpio_charger->current_limit_map), GFP_KERNEL);
> + if (!gpio_charger->current_limit_map)
> + return -ENOMEM;
> +
> + gpio_charger->current_limit_map_size = len / 2;
> +
> + len = device_property_read_u32_array(dev, 
> "charge-current-limit-mapping",
> + (u32*) gpio_charger->current_limit_map, len);
> + if (len < 0)
> + return len;
> +
> + for (i=0; i < gpio_charger->current_limit_map_size; i++) {
> + if (gpio_charger->current_limit_map[i].limit_ua > cur_limit) {
> + dev_err(dev, "invalid charge-current-limit-mapping\n");
Would make sense to use something more descriptive than "invalid". Say "not
sorted by current descending order"?


> @@ -137,18 +270,19 @@ static int gpio_charger_probe(struct platform_device 
> *pdev)

>   /*
>* If this fails and we're not using device tree, try the
>* legacy platform data method.
>*/
> - if (IS_ERR(gpio_charger->gpiod) && !dev->of_node) {
> + if (!gpio_charger->gpiod && !dev->of_node) {
The original code will attempt the legacy code for ... (from the doc)

 * ..., -ENOENT if no GPIO has been assigned to the requested function, or
 * another IS_ERR() code if an error occurred while trying to acquire the GPIO.

While the new code will only consider -ENOENT.

Using 

get_maintainer.pl: unexpected behaviour for path/to//file

[Emil Velikov]

Hi Joe,

Recently I've noticed that get_maintainer behaves differently if there
is a double, sequential, forward slash in the path.

AFAICT there should be no distinction between the two. Or at least many
existing applications and scripts consider them one and the same.

I've tried fixing this, although my perl isn't quite up-to scratch.
Is this some weird bug or some intended feature?

Thanks
Emil

Example:

./scripts/get_maintainer -F drivers/gpu/drm//lima

David Airlie  (maintainer:DRM DRIVERS)
Daniel Vetter  (maintainer:DRM DRIVERS,commit_signer:3/42=7%)
Qiang Yu  (commit_signer:36/42=86%,authored:24/42=57%)
Vasily Khoruzhick  (commit_signer:26/42=62%)
Krzysztof Kozlowski  (commit_signer:5/42=12%,authored:5/42=12%)
Emil Velikov  (commit_signer:4/42=10%)
dri-de...@lists.freedesktop.org (open list:DRM DRIVERS)
linux-kernel@vger.kernel.org (open list)

./scripts/get_maintainer -F drivers/gpu/drm/lima

Qiang Yu  (maintainer:DRM DRIVERS FOR LIMA)
David Airlie  (maintainer:DRM DRIVERS)
Daniel Vetter  (maintainer:DRM DRIVERS)
dri-de...@lists.freedesktop.org (open list:DRM DRIVERS FOR LIMA)
l...@lists.freedesktop.org (moderated list:DRM DRIVERS FOR LIMA)
linux-kernel@vger.kernel.org (open list)

Re: [PATCH 10/11] kernel/power: constify sysrq_key_op

[Emil Velikov]

On Thu, 14 May 2020 at 12:21, Rafael J. Wysocki  wrote:
>
> On Wed, May 13, 2020 at 11:46 PM Emil Velikov  
> wrote:
> >
> > With earlier commits, the API no longer discards the const-ness of the
> > sysrq_key_op. As such we can add the notation.
> >
> > Cc: Greg Kroah-Hartman 
> > Cc: Jiri Slaby 
> > Cc: linux-kernel@vger.kernel.org
> > Cc: "Rafael J. Wysocki" 
> > Cc: Len Brown 
> > Cc: linux...@vger.kernel.org
> > Signed-off-by: Emil Velikov 
>
> Acked-by: Rafael J. Wysocki 
>
Thanks

> and I'm assuming that this is going to be applied along with the rest
> of the series.
>
I believe so, although I have not heard anything from the TTY maintainers yet.

-Emil

Re: [PATCH v2 00/10] drivers, provide a way to add sysfs groups easily

[Emil Velikov]

On Thu, 14 May 2020 at 08:16, Greg Kroah-Hartman
 wrote:
>
> On Wed, May 13, 2020 at 11:18:15PM +0100, Emil Velikov wrote:
> > Hi Greg,
> >
> > On Fri, 2 Aug 2019 at 11:46, Greg Kroah-Hartman
> >  wrote:
> >
> > >
> > > I have now done this with patch 1/10.  Here's the pull info if any
> > > subsystem maintainer wants to suck this into their tree to provide the
> > > ability for drivers to add/remove attribute groups easily.
> > >
> > > This is part of my driver-core tree now, and will go to Linus for
> > > 5.4-rc1, along with a few platform drivers that have been acked by their
> > > various subsystem maintainers that convert them to use this new
> > > functionality.
> > >
> > > If anyone has any questions about this, please let me know.
> > >
> > > thanks,
> > >
> > > greg k-h
> > >
> > > ---
> > >
> > > The following changes since commit 
> > > 5f9e832c137075045d15cd6899ab0505cfb2ca4b:
> > >
> > >   Linus 5.3-rc1 (2019-07-21 14:05:38 -0700)
> > >
> > > are available in the Git repository at:
> > >
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 
> > > tags/dev_groups_all_drivers
> > >
> > > for you to fetch changes up to 23b6904442d08b7dbed7622ed33b236d41a3aa8b:
> > >
> > >   driver core: add dev_groups to all drivers (2019-08-02 12:37:53 +0200)
> > >
> > > 
> > > dev_groups added to struct driver
> > >
> > > Persistent tag for others to pull this branch from
> > >
> > > This is the first patch in a longer series that adds the ability for the
> > > driver core to create and remove a list of attribute groups
> > > automatically when the device is bound/unbound from a specific driver.
> > >
> > > See:
> > > 
> > > https://lore.kernel.org/r/20190731124349.4474-2-gre...@linuxfoundation.org
> > > for details on this patch, and examples of how to use it in other
> > > drivers.
> > >
> > > Signed-off-by: Greg Kroah-Hartman 
> > >
> > > 
> > > Dmitry Torokhov (1):
> > >   driver core: add dev_groups to all drivers
> > >
> > >  drivers/base/dd.c  | 14 ++
> > >  include/linux/device.h |  3 +++
> > >  2 files changed, 17 insertions(+)
> > > ___
> >
> > Was planning to re-spin DRM a series which uses .dev_groups, although
> > I cannot see the core patch.
> > Did the it get reverted or simply fell though the cracks?
>
> Nope, it's in there:
> 23b6904442d0 ("driver core: add dev_groups to all drivers")
> which showed up in the 5.4 kernel release.
>
> Lots of other subsystems have already been converted to use this, do you
> not see it in your tree?
>
A case of PEBKAC it seems - I was looking at a 5.3 checkout somehow.

Thanks for the core work. Will check/merge the fbdev patches over the
next few days and polish drm land.

-Emil

Re: [PATCH v2 00/10] drivers, provide a way to add sysfs groups easily

[Emil Velikov]

Hi Greg,

On Fri, 2 Aug 2019 at 11:46, Greg Kroah-Hartman
 wrote:

>
> I have now done this with patch 1/10.  Here's the pull info if any
> subsystem maintainer wants to suck this into their tree to provide the
> ability for drivers to add/remove attribute groups easily.
>
> This is part of my driver-core tree now, and will go to Linus for
> 5.4-rc1, along with a few platform drivers that have been acked by their
> various subsystem maintainers that convert them to use this new
> functionality.
>
> If anyone has any questions about this, please let me know.
>
> thanks,
>
> greg k-h
>
> ---
>
> The following changes since commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b:
>
>   Linus 5.3-rc1 (2019-07-21 14:05:38 -0700)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 
> tags/dev_groups_all_drivers
>
> for you to fetch changes up to 23b6904442d08b7dbed7622ed33b236d41a3aa8b:
>
>   driver core: add dev_groups to all drivers (2019-08-02 12:37:53 +0200)
>
> 
> dev_groups added to struct driver
>
> Persistent tag for others to pull this branch from
>
> This is the first patch in a longer series that adds the ability for the
> driver core to create and remove a list of attribute groups
> automatically when the device is bound/unbound from a specific driver.
>
> See:
> 
> https://lore.kernel.org/r/20190731124349.4474-2-gre...@linuxfoundation.org
> for details on this patch, and examples of how to use it in other
> drivers.
>
> Signed-off-by: Greg Kroah-Hartman 
>
> 
> Dmitry Torokhov (1):
>   driver core: add dev_groups to all drivers
>
>  drivers/base/dd.c  | 14 ++
>  include/linux/device.h |  3 +++
>  2 files changed, 17 insertions(+)
> ___

Was planning to re-spin DRM a series which uses .dev_groups, although
I cannot see the core patch.
Did the it get reverted or simply fell though the cracks?

-Emil

[PATCH 08/11] drm: constify sysrq_key_op

[Emil Velikov]

With earlier commits, the API no longer discards the const-ness of the
sysrq_key_op. As such we can add the notation.

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-kernel@vger.kernel.org
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Thomas Zimmermann 
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Emil Velikov 
---
Please keep me in the CC list, as I'm not subscribed to the list.

IMHO it would be better if this gets merged this via the tty tree.
---
 drivers/gpu/drm/drm_fb_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a9771de4d17e..533767100efe 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -307,13 +307,13 @@ static void drm_fb_helper_sysrq(int dummy1)
schedule_work(_fb_helper_restore_work);
 }
 
-static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = {
+static const struct sysrq_key_op sysrq_drm_fb_helper_restore_op = {
.handler = drm_fb_helper_sysrq,
.help_msg = "force-fb(V)",
.action_msg = "Restore framebuffer console",
 };
 #else
-static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { };
+static const struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { };
 #endif
 
 static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
-- 
2.25.1

[PATCH 09/11] kdb: constify sysrq_key_op

[Emil Velikov]

With earlier commits, the API no longer discards the const-ness of the
sysrq_key_op. As such we can add the notation.

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-kernel@vger.kernel.org
Cc: Jason Wessel 
Cc: Daniel Thompson 
Cc: kgdb-bugrep...@lists.sourceforge.net
Signed-off-by: Emil Velikov 
---
Please keep me in the CC list, as I'm not subscribed to the list.

IMHO it would be better if this gets merged this via the tty tree.
---
 kernel/debug/debug_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 2b7c9b67931d..355bea54ca0e 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -920,7 +920,7 @@ static void sysrq_handle_dbg(int key)
kgdb_breakpoint();
 }
 
-static struct sysrq_key_op sysrq_dbg_op = {
+static const struct sysrq_key_op sysrq_dbg_op = {
.handler= sysrq_handle_dbg,
.help_msg   = "debug(g)",
.action_msg = "DEBUG",
-- 
2.25.1

[PATCH 04/11] alpha: constify sysrq_key_op

[Emil Velikov]

With earlier commits, the API no longer discards the const-ness of the
sysrq_key_op. As such we can add the notation.

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-kernel@vger.kernel.org
Cc: Richard Henderson 
Cc: Ivan Kokshaysky 
Cc: Matt Turner 
Cc: linux-al...@vger.kernel.org
Signed-off-by: Emil Velikov 
---
Please keep me in the CC list, as I'm not subscribed to the list.

IMHO it would be better if this gets merged this via the tty tree.
---
 arch/alpha/kernel/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index dd7f770f23cf..6fa802c495b4 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -431,7 +431,7 @@ register_cpus(void)
 arch_initcall(register_cpus);
 
 #ifdef CONFIG_MAGIC_SYSRQ
-static struct sysrq_key_op srm_sysrq_reboot_op = {
+static const struct sysrq_key_op srm_sysrq_reboot_op = {
.handler= machine_halt,
.help_msg   = "reboot(b)",
.action_msg = "Resetting",
-- 
2.25.1

[PATCH 10/11] kernel/power: constify sysrq_key_op

[Emil Velikov]

With earlier commits, the API no longer discards the const-ness of the
sysrq_key_op. As such we can add the notation.

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-kernel@vger.kernel.org
Cc: "Rafael J. Wysocki" 
Cc: Len Brown 
Cc: linux...@vger.kernel.org
Signed-off-by: Emil Velikov 
---
Please keep me in the CC list, as I'm not subscribed to the list.

IMHO it would be better if this gets merged this via the tty tree.
---
 kernel/power/poweroff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/power/poweroff.c b/kernel/power/poweroff.c
index 6d475281c730..562aa0e450ed 100644
--- a/kernel/power/poweroff.c
+++ b/kernel/power/poweroff.c
@@ -29,7 +29,7 @@ static void handle_poweroff(int key)
schedule_work_on(cpumask_first(cpu_online_mask), _work);
 }
 
-static struct sysrq_key_op sysrq_poweroff_op = {
+static const struct sysrq_key_op   sysrq_poweroff_op = {
.handler= handle_poweroff,
.help_msg   = "poweroff(o)",
.action_msg = "Power Off",
-- 
2.25.1

[PATCH 07/11] sparc64: constify sysrq_key_op

[Emil Velikov]

With earlier commits, the API no longer discards the const-ness of the
sysrq_key_op. As such we can add the notation.

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-kernel@vger.kernel.org
Cc: "David S. Miller" 
Cc: sparcli...@vger.kernel.org
Signed-off-by: Emil Velikov 
---
Please keep me in the CC list, as I'm not subscribed to the list.

IMHO it would be better if this gets merged this via the tty tree.
---
 arch/sparc/kernel/process_64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/sparc/kernel/process_64.c b/arch/sparc/kernel/process_64.c
index 4282116e28e7..423011e60982 100644
--- a/arch/sparc/kernel/process_64.c
+++ b/arch/sparc/kernel/process_64.c
@@ -313,7 +313,7 @@ static void sysrq_handle_globreg(int key)
trigger_all_cpu_backtrace();
 }
 
-static struct sysrq_key_op sparc_globalreg_op = {
+static const struct sysrq_key_op sparc_globalreg_op = {
.handler= sysrq_handle_globreg,
.help_msg   = "global-regs(y)",
.action_msg = "Show Global CPU Regs",
@@ -388,7 +388,7 @@ static void sysrq_handle_globpmu(int key)
pmu_snapshot_all_cpus();
 }
 
-static struct sysrq_key_op sparc_globalpmu_op = {
+static const struct sysrq_key_op sparc_globalpmu_op = {
.handler= sysrq_handle_globpmu,
.help_msg   = "global-pmu(x)",
.action_msg = "Show Global PMU Regs",
-- 
2.25.1

[PATCH 01/11] tty/sysrq: alpha: export and use __sysrq_get_key_op()

[Emil Velikov]

Export a pointer to the sysrq_get_key_op(). This way we can cleanly
unregister it, instead of the current solutions of modifuing it inplace.

Since __sysrq_get_key_op() is no longer used externally, let's make it
a static function.

This patch will allow us to limit access to each and every sysrq op and
constify the sysrq handling.

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-kernel@vger.kernel.org
Cc: Richard Henderson 
Cc: Ivan Kokshaysky 
Cc: Matt Turner 
Cc: linux-al...@vger.kernel.org
Signed-off-by: Emil Velikov 
---
Please keep me in the CC list, as I'm not subscribed to the list.

IMHO it would be better if this gets merged this via the tty tree.
---
 arch/alpha/kernel/setup.c | 13 +++--
 drivers/tty/sysrq.c   |  4 +++-
 include/linux/sysrq.h |  2 +-
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index f19aa577354b..dd7f770f23cf 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -430,6 +430,15 @@ register_cpus(void)
 
 arch_initcall(register_cpus);
 
+#ifdef CONFIG_MAGIC_SYSRQ
+static struct sysrq_key_op srm_sysrq_reboot_op = {
+   .handler= machine_halt,
+   .help_msg   = "reboot(b)",
+   .action_msg = "Resetting",
+   .enable_mask= SYSRQ_ENABLE_BOOT,
+};
+#endif
+
 void __init
 setup_arch(char **cmdline_p)
 {
@@ -550,8 +559,8 @@ setup_arch(char **cmdline_p)
/* If we're using SRM, make sysrq-b halt back to the prom,
   not auto-reboot.  */
if (alpha_using_srm) {
-   struct sysrq_key_op *op = __sysrq_get_key_op('b');
-   op->handler = (void *) machine_halt;
+   unregister_sysrq_key('b', __sysrq_reboot_op);
+   register_sysrq_key('b', _sysrq_reboot_op);
}
 #endif
 
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 0dc3878794fd..1741134cabca 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -172,6 +172,8 @@ static struct sysrq_key_op sysrq_reboot_op = {
.enable_mask= SYSRQ_ENABLE_BOOT,
 };
 
+struct sysrq_key_op *__sysrq_reboot_op = _reboot_op;
+
 static void sysrq_handle_sync(int key)
 {
emergency_sync();
@@ -516,7 +518,7 @@ static int sysrq_key_table_key2index(int key)
 /*
  * get and put functions for the table, exposed to modules.
  */
-struct sysrq_key_op *__sysrq_get_key_op(int key)
+static struct sysrq_key_op *__sysrq_get_key_op(int key)
 {
 struct sysrq_key_op *op_p = NULL;
 int i;
diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h
index 8e159e16850f..9b51f98e5f60 100644
--- a/include/linux/sysrq.h
+++ b/include/linux/sysrq.h
@@ -47,7 +47,7 @@ void handle_sysrq(int key);
 void __handle_sysrq(int key, bool check_mask);
 int register_sysrq_key(int key, struct sysrq_key_op *op);
 int unregister_sysrq_key(int key, struct sysrq_key_op *op);
-struct sysrq_key_op *__sysrq_get_key_op(int key);
+extern struct sysrq_key_op *__sysrq_reboot_op;
 
 int sysrq_toggle_support(int enable_mask);
 int sysrq_mask(void);
-- 
2.25.1

[PATCH 06/11] powerpc/xmon: constify sysrq_key_op

[Emil Velikov]

With earlier commits, the API no longer discards the const-ness of the
sysrq_key_op. As such we can add the notation.

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-kernel@vger.kernel.org
Cc: Michael Ellerman 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: linuxppc-...@lists.ozlabs.org
Signed-off-by: Emil Velikov 
---
Please keep me in the CC list, as I'm not subscribed to the list.

IMHO it would be better if this gets merged this via the tty tree.
---
 arch/powerpc/xmon/xmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 7af840c0fc93..0d8ca5c9f131 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3842,7 +3842,7 @@ static void sysrq_handle_xmon(int key)
xmon_init(0);
 }
 
-static struct sysrq_key_op sysrq_xmon_op = {
+static const struct sysrq_key_op sysrq_xmon_op = {
.handler =  sysrq_handle_xmon,
.help_msg = "xmon(x)",
.action_msg =   "Entering xmon",
-- 
2.25.1

[PATCH 11/11] rcu: constify sysrq_key_op

[Emil Velikov]

With earlier commits, the API no longer discards the const-ness of the
sysrq_key_op. As such we can add the notation.

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-kernel@vger.kernel.org
Cc: "Paul E. McKenney" 
Cc: Josh Triplett 
Cc: r...@vger.kernel.org
Signed-off-by: Emil Velikov 
---
Please keep me in the CC list, as I'm not subscribed to the list.

IMHO it would be better if this gets merged this via the tty tree.
---
 kernel/rcu/tree_stall.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
index 119ed6afd20f..4e6ed7b91f59 100644
--- a/kernel/rcu/tree_stall.h
+++ b/kernel/rcu/tree_stall.h
@@ -729,7 +729,7 @@ static void sysrq_show_rcu(int key)
show_rcu_gp_kthreads();
 }
 
-static struct sysrq_key_op sysrq_rcudump_op = {
+static const struct sysrq_key_op sysrq_rcudump_op = {
.handler = sysrq_show_rcu,
.help_msg = "show-rcu(y)",
.action_msg = "Show RCU tree",
-- 
2.25.1

[PATCH 05/11] MIPS: constify sysrq_key_op

[Emil Velikov]

With earlier commits, the API no longer discards the const-ness of the
sysrq_key_op. As such we can add the notation.

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-kernel@vger.kernel.org
Cc: Thomas Bogendoerfer 
Cc: linux-m...@vger.kernel.org
Signed-off-by: Emil Velikov 
---
Please keep me in the CC list, as I'm not subscribed to the list.

IMHO it would be better it this gets merged this via the tty tree.
---
 arch/mips/kernel/sysrq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kernel/sysrq.c b/arch/mips/kernel/sysrq.c
index e5a2a6ab71ac..9c1a2019113b 100644
--- a/arch/mips/kernel/sysrq.c
+++ b/arch/mips/kernel/sysrq.c
@@ -52,7 +52,7 @@ static void sysrq_handle_tlbdump(int key)
 #endif
 }
 
-static struct sysrq_key_op sysrq_tlbdump_op = {
+static const struct sysrq_key_op sysrq_tlbdump_op = {
.handler= sysrq_handle_tlbdump,
.help_msg   = "show-tlbs(x)",
.action_msg = "Show TLB entries",
-- 
2.25.1

[PATCH 02/11] tty/sysrq: constify the sysrq API

[Emil Velikov]

The user is not supposed to thinker with the underlying sysrq_key_op.
Make that explicit by adding a handful of const notations.

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Emil Velikov 
---
Please keep me in the CC list, as I'm not subscribed to the list.

IMHO it would be better if this gets merged this via the tty tree.
---
 Documentation/admin-guide/sysrq.rst | 10 +-
 drivers/tty/sysrq.c | 16 
 include/linux/sysrq.h   | 16 
 3 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/Documentation/admin-guide/sysrq.rst 
b/Documentation/admin-guide/sysrq.rst
index a46209f4636c..e6424d8c5846 100644
--- a/Documentation/admin-guide/sysrq.rst
+++ b/Documentation/admin-guide/sysrq.rst
@@ -231,13 +231,13 @@ prints help, and C) an action_msg string, that will print 
right before your
 handler is called. Your handler must conform to the prototype in 'sysrq.h'.
 
 After the ``sysrq_key_op`` is created, you can call the kernel function
-``register_sysrq_key(int key, struct sysrq_key_op *op_p);`` this will
+``register_sysrq_key(int key, const struct sysrq_key_op *op_p);`` this will
 register the operation pointed to by ``op_p`` at table key 'key',
 if that slot in the table is blank. At module unload time, you must call
-the function ``unregister_sysrq_key(int key, struct sysrq_key_op *op_p)``, 
which
-will remove the key op pointed to by 'op_p' from the key 'key', if and only if
-it is currently registered in that slot. This is in case the slot has been
-overwritten since you registered it.
+the function ``unregister_sysrq_key(int key, const struct sysrq_key_op 
*op_p)``,
+which will remove the key op pointed to by 'op_p' from the key 'key', if and
+only if it is currently registered in that slot. This is in case the slot has
+been overwritten since you registered it.
 
 The Magic SysRQ system works by registering key operations against a key op
 lookup table, which is defined in 'drivers/tty/sysrq.c'. This key table has
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 1741134cabca..091c64a3cef0 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -518,9 +518,9 @@ static int sysrq_key_table_key2index(int key)
 /*
  * get and put functions for the table, exposed to modules.
  */
-static struct sysrq_key_op *__sysrq_get_key_op(int key)
+static const struct sysrq_key_op *__sysrq_get_key_op(int key)
 {
-struct sysrq_key_op *op_p = NULL;
+const struct sysrq_key_op *op_p = NULL;
 int i;
 
i = sysrq_key_table_key2index(key);
@@ -530,7 +530,7 @@ static struct sysrq_key_op *__sysrq_get_key_op(int key)
 return op_p;
 }
 
-static void __sysrq_put_key_op(int key, struct sysrq_key_op *op_p)
+static void __sysrq_put_key_op(int key, const struct sysrq_key_op *op_p)
 {
 int i = sysrq_key_table_key2index(key);
 
@@ -540,7 +540,7 @@ static void __sysrq_put_key_op(int key, struct sysrq_key_op 
*op_p)
 
 void __handle_sysrq(int key, bool check_mask)
 {
-   struct sysrq_key_op *op_p;
+   const struct sysrq_key_op *op_p;
int orig_log_level;
int orig_suppress_printk;
int i;
@@ -1063,8 +1063,8 @@ int sysrq_toggle_support(int enable_mask)
 }
 EXPORT_SYMBOL_GPL(sysrq_toggle_support);
 
-static int __sysrq_swap_key_ops(int key, struct sysrq_key_op *insert_op_p,
-struct sysrq_key_op *remove_op_p)
+static int __sysrq_swap_key_ops(int key, const struct sysrq_key_op 
*insert_op_p,
+const struct sysrq_key_op *remove_op_p)
 {
int retval;
 
@@ -1087,13 +1087,13 @@ static int __sysrq_swap_key_ops(int key, struct 
sysrq_key_op *insert_op_p,
return retval;
 }
 
-int register_sysrq_key(int key, struct sysrq_key_op *op_p)
+int register_sysrq_key(int key, const struct sysrq_key_op *op_p)
 {
return __sysrq_swap_key_ops(key, op_p, NULL);
 }
 EXPORT_SYMBOL(register_sysrq_key);
 
-int unregister_sysrq_key(int key, struct sysrq_key_op *op_p)
+int unregister_sysrq_key(int key, const struct sysrq_key_op *op_p)
 {
return __sysrq_swap_key_ops(key, NULL, op_p);
 }
diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h
index 9b51f98e5f60..479028391c08 100644
--- a/include/linux/sysrq.h
+++ b/include/linux/sysrq.h
@@ -30,10 +30,10 @@
 #define SYSRQ_ENABLE_RTNICE0x0100
 
 struct sysrq_key_op {
-   void (*handler)(int);
-   char *help_msg;
-   char *action_msg;
-   int enable_mask;
+   void (* const handler)(int);
+   const char * const help_msg;
+   const char * const action_msg;
+   const int enable_mask;
 };
 
 #ifdef CONFIG_MAGIC_SYSRQ
@@ -45,8 +45,8 @@ struct sysrq_key_op {
 
 void handle_sysrq(int key);
 void __handle_sysrq(int key, bool check_mask);
-int register_sysrq_key(int key, struct sysrq_key_op *op);
-int unregister_sysrq_key(int key, struct sysrq_key_op *op);
+int register_sysrq_key(int key, const

[PATCH 03/11] tty/sysrq: constify the the sysrq_key_op(s)

[Emil Velikov]

All the users threat them as immutable - annotate them as such.

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Emil Velikov 
---
Please keep me in the CC list, as I'm not subscribed to the list.

IMHO it would be better if this gets merged this via the tty tree.
---
 drivers/tty/sysrq.c   | 52 +--
 include/linux/sysrq.h |  2 +-
 2 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 091c64a3cef0..477cdc1e9cf3 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -106,7 +106,7 @@ static void sysrq_handle_loglevel(int key)
pr_info("Loglevel set to %d\n", i);
console_loglevel = i;
 }
-static struct sysrq_key_op sysrq_loglevel_op = {
+static const struct sysrq_key_op sysrq_loglevel_op = {
.handler= sysrq_handle_loglevel,
.help_msg   = "loglevel(0-9)",
.action_msg = "Changing Loglevel",
@@ -119,14 +119,14 @@ static void sysrq_handle_SAK(int key)
struct work_struct *SAK_work = _cons[fg_console].SAK_work;
schedule_work(SAK_work);
 }
-static struct sysrq_key_op sysrq_SAK_op = {
+static const struct sysrq_key_op sysrq_SAK_op = {
.handler= sysrq_handle_SAK,
.help_msg   = "sak(k)",
.action_msg = "SAK",
.enable_mask= SYSRQ_ENABLE_KEYBOARD,
 };
 #else
-#define sysrq_SAK_op (*(struct sysrq_key_op *)NULL)
+#define sysrq_SAK_op (*(const struct sysrq_key_op *)NULL)
 #endif
 
 #ifdef CONFIG_VT
@@ -135,14 +135,14 @@ static void sysrq_handle_unraw(int key)
vt_reset_unicode(fg_console);
 }
 
-static struct sysrq_key_op sysrq_unraw_op = {
+static const struct sysrq_key_op sysrq_unraw_op = {
.handler= sysrq_handle_unraw,
.help_msg   = "unraw(r)",
.action_msg = "Keyboard mode set to system default",
.enable_mask= SYSRQ_ENABLE_KEYBOARD,
 };
 #else
-#define sysrq_unraw_op (*(struct sysrq_key_op *)NULL)
+#define sysrq_unraw_op (*(const struct sysrq_key_op *)NULL)
 #endif /* CONFIG_VT */
 
 static void sysrq_handle_crash(int key)
@@ -152,7 +152,7 @@ static void sysrq_handle_crash(int key)
 
panic("sysrq triggered crash\n");
 }
-static struct sysrq_key_op sysrq_crash_op = {
+static const struct sysrq_key_op sysrq_crash_op = {
.handler= sysrq_handle_crash,
.help_msg   = "crash(c)",
.action_msg = "Trigger a crash",
@@ -165,20 +165,20 @@ static void sysrq_handle_reboot(int key)
local_irq_enable();
emergency_restart();
 }
-static struct sysrq_key_op sysrq_reboot_op = {
+static const struct sysrq_key_op sysrq_reboot_op = {
.handler= sysrq_handle_reboot,
.help_msg   = "reboot(b)",
.action_msg = "Resetting",
.enable_mask= SYSRQ_ENABLE_BOOT,
 };
 
-struct sysrq_key_op *__sysrq_reboot_op = _reboot_op;
+const struct sysrq_key_op *__sysrq_reboot_op = _reboot_op;
 
 static void sysrq_handle_sync(int key)
 {
emergency_sync();
 }
-static struct sysrq_key_op sysrq_sync_op = {
+static const struct sysrq_key_op sysrq_sync_op = {
.handler= sysrq_handle_sync,
.help_msg   = "sync(s)",
.action_msg = "Emergency Sync",
@@ -190,7 +190,7 @@ static void sysrq_handle_show_timers(int key)
sysrq_timer_list_show();
 }
 
-static struct sysrq_key_op sysrq_show_timers_op = {
+static const struct sysrq_key_op sysrq_show_timers_op = {
.handler= sysrq_handle_show_timers,
.help_msg   = "show-all-timers(q)",
.action_msg = "Show clockevent devices & pending hrtimers (no 
others)",
@@ -200,7 +200,7 @@ static void sysrq_handle_mountro(int key)
 {
emergency_remount();
 }
-static struct sysrq_key_op sysrq_mountro_op = {
+static const struct sysrq_key_op sysrq_mountro_op = {
.handler= sysrq_handle_mountro,
.help_msg   = "unmount(u)",
.action_msg = "Emergency Remount R/O",
@@ -213,13 +213,13 @@ static void sysrq_handle_showlocks(int key)
debug_show_all_locks();
 }
 
-static struct sysrq_key_op sysrq_showlocks_op = {
+static const struct sysrq_key_op sysrq_showlocks_op = {
.handler= sysrq_handle_showlocks,
.help_msg   = "show-all-locks(d)",
.action_msg = "Show Locks Held",
 };
 #else
-#define sysrq_showlocks_op (*(struct sysrq_key_op *)NULL)
+#define sysrq_showlocks_op (*(const struct sysrq_key_op *)NULL)
 #endif
 
 #ifdef CONFIG_SMP
@@ -266,7 +266,7 @@ static void sysrq_handle_showallcpus(int key)
}
 }
 
-static struct sysrq_key_op sysrq_showallcpus_op = {
+static const struct sysrq_key_op sysrq_showallcpus_op = {
.handler= s

Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge

[Emil Velikov]

Hi Vinod,

Few high-level comments:
 - handful of functions always return 0 and the return value is never
checked - switch to return void
 - annotate all (nearly) arrays as static const
 - consistently use multi_reg_write - in some cases non-const array
will be fine, overwriting a few entries as needed
 - there is very partial comments about the registers/values - missing docs or?

Personally I'm in favour of using symbolic names, instead of
hex+comment. Considering how partial the comments are, current
approach is perfectly fine.

On Wed, 13 May 2020 at 11:06, Vinod Koul  wrote:
>
> Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
> I2S port as an input and HDMI port as output
>
> Co-developed-by: Bjorn Andersson 
> Signed-off-by: Bjorn Andersson 
> Signed-off-by: Vinod Koul 

> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lt9611.c

Please add a vendor prefix to the filename.

> @@ -0,0 +1,1113 @@

> +struct lt9611_mode {
> +   u16 hdisplay;
> +   u16 vdisplay;
> +   u8 fps;
We all enjoy the odd fps game, but let's use vrefresh here.

> +   u8 lanes;
> +   u8 intfs;
> +};
> +


> +static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
> +const struct drm_display_mode *mode)
> +{
> +   regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
> +
> +   if (mode->hdisplay == 3840)
> +   regmap_write(lt9611->regmap, 0x830a, 0x03);
> +   else
> +   regmap_write(lt9611->regmap, 0x830a, 0x00);
> +
> +   regmap_write(lt9611->regmap, 0x824f, 0x80);
> +   regmap_write(lt9611->regmap, 0x8250, 0x10);
> +   regmap_write(lt9611->regmap, 0x8302, 0x0a);
> +   regmap_write(lt9611->regmap, 0x8306, 0x0a);
Create an (non-const) array, overwriting the [1] entry for 3840 mode?

> +
> +   return 0;
Kill return type.

> +}

> +static int lt9611_pcr_setup(struct lt9611 *lt9611,
> +   const struct drm_display_mode *mode)
> +{
> +   struct reg_sequence reg_cfg[] = {
static const?

> +   { 0x830b, 0x01 },
> +   { 0x830c, 0x10 },
> +   { 0x8348, 0x00 },
> +   { 0x8349, 0x81 },
> +
> +   /* stage 1 */
> +   { 0x8321, 0x4a },
> +   { 0x8324, 0x71 },
> +   { 0x8325, 0x30 },
> +   { 0x832a, 0x01 },
> +
> +   /* stage 2 */
> +   { 0x834a, 0x40 },
> +   { 0x831d, 0x10 },
> +
> +   /* MK limit */
> +   { 0x832d, 0x38 },
> +   { 0x8331, 0x08 },
> +   };
> +
> +   regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> +   switch (mode->hdisplay) {
> +   case 640:
> +   regmap_write(lt9611->regmap, 0x8326, 0x14);
> +   break;
> +   case 1920:
> +   regmap_write(lt9611->regmap, 0x8326, 0x37);
> +   break;
> +   case 3840:
> +   regmap_write(lt9611->regmap, 0x830b, 0x03);
> +   regmap_write(lt9611->regmap, 0x830c, 0xd0);
> +   regmap_write(lt9611->regmap, 0x8348, 0x03);
> +   regmap_write(lt9611->regmap, 0x8349, 0xe0);
> +   regmap_write(lt9611->regmap, 0x8324, 0x72);
> +   regmap_write(lt9611->regmap, 0x8325, 0x00);
> +   regmap_write(lt9611->regmap, 0x832a, 0x01);
> +   regmap_write(lt9611->regmap, 0x834a, 0x10);
> +   regmap_write(lt9611->regmap, 0x831d, 0x10);
> +   regmap_write(lt9611->regmap, 0x8326, 0x37);
Throw this in another const array?

> +   break;
> +   }
> +
> +   /* pcr rst */
> +   regmap_write(lt9611->regmap, 0x8011, 0x5a);
> +   regmap_write(lt9611->regmap, 0x8011, 0xfa);
> +
> +   return 0;
> +}


> +   regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
> +   regmap_write(lt9611->regmap, 0x82e4, pclk >> 9);  /* pclk[15:8]  */
> +   regmap_write(lt9611->regmap, 0x82e5, pclk >> 1);  /* pclk[7:0]   */
Comment does not match the code.
We're discarding the LSB, so we cannot realistically be writing
pclk[7:0]. Similar applies for the other two.


> +   /* v_act */
> +   ret = regmap_read(lt9611->regmap, 0x8282, );
> +   if (ret)
> +   goto end;
> +
> +   v_act = temp << 8;
> +   ret = regmap_read(lt9611->regmap, 0x8283, );
> +   if (ret)
> +   goto end;
> +   v_act = v_act + temp;
> +
Having a helper for the above "result = read(x) << 8 | read(x+1)"
would be great.
This way one doesn't have to repeat the pattern 4-5 times.


> +static int lt9611_read_edid(struct lt9611 *lt9611)
> +{
> +   unsigned int temp;
> +   int ret = 0;
> +   int i, j;
> +
> +   memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
How about:
  memset(lt9611->edid_buf, 0, sizeof(lt9611->edid_buf));

Then again, do we need the memset()? We are allocating the memory with
devm_kzalloc()

> +
> +

Re: [PATCH 0/2] drm: encoder_slave: some updates

[Emil Velikov]

Hi Wolfram,

On Wed, 13 May 2020 at 10:10, Wolfram Sang
 wrote:
>
> On Mon, Mar 16, 2020 at 05:39:05PM +0100, Wolfram Sang wrote:
> > While converting I2C users to new APIs, I found a refcounting problem in
> > the encoder_slave implementation. This series fixes it and converts to
> > the new API.
> >
> > Based on linux-next and only build tested.
> >
> > Wolfram Sang (2):
> >   drm: encoder_slave: fix refcouting error for modules
> >   drm: encoder_slave: use new I2C API
> >
> >  drivers/gpu/drm/drm_encoder_slave.c | 15 +--
> >  1 file changed, 5 insertions(+), 10 deletions(-)
>
> Is there someone I should add to the CC list maybe?
>
The series is:
Reviewed-by: Emil Velikov 

Unless someone beats me to it, I'll commit them to drm-misc later today.

-Emil

Re: [PATCH -next] drm/mcde: dsi: Fix return value check in dev_err()

[Emil Velikov]

Hi all,

On Tue, 12 May 2020 at 12:49, Linus Walleij  wrote:
>
> On Tue, Apr 28, 2020 at 4:13 PM Wei Yongjun  wrote:
>
> > In case of error, the function of_drm_find_bridge() returns NULL pointer
> > not ERR_PTR(). The IS_ERR() test in the return value check should be
> > replaced with NULL test.
> >
> > Signed-off-by: Wei Yongjun 
>
> Patch applied! Thanks Wei, sorry for the long delay.
>
It would be nice if of_drm_find_bridge and of_drm_find_panel were
consistent - either return NULL or an ERR_PTR.
Otherwise the next person using them is likely to get it wrong.

-Emil

Re: [PATCH V4 2/3] drm/vkms: Compute CRC without change input data

[Emil Velikov]

Hi Rodrigo,

On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira  wrote:
>
> From: Rodrigo Siqueira 
>
> The compute_crc() function is responsible for calculating the
> framebuffer CRC value; due to the XRGB format, this function has to
> ignore the alpha channel during the CRC computation. Therefore,
> compute_crc() set zero to the alpha channel directly in the input
> framebuffer, which is not a problem since this function receives a copy
> of the original buffer. However, if we want to use this function in a
> context without a buffer copy, it will change the initial value. This
> patch makes compute_crc() calculate the CRC value without modifying the
> input framebuffer.
>
> Signed-off-by: Rodrigo Siqueira 
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 31 +---
>  1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c 
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 258e659ecfba..686d25e7b01d 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -9,33 +9,40 @@
>
>  #include "vkms_drv.h"
>
> +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> +const struct vkms_composer *composer)
> +{
> +   int src_offset = composer->offset + (y * composer->pitch)
> + + (x * composer->cpp);
> +
> +   return *(u32 *)[src_offset];
> +}
> +
>  /**
>   * compute_crc - Compute CRC value on output frame
>   *
> - * @vaddr_out: address to final framebuffer
> + * @vaddr: address to final framebuffer
>   * @composer: framebuffer's metadata
>   *
>   * returns CRC value computed using crc32 on the visible portion of
>   * the final framebuffer at vaddr_out
>   */
> -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
> +static uint32_t compute_crc(const u8 *vaddr,
> +   const struct vkms_composer *composer)
>  {
> -   int i, j, src_offset;
> +   int x, y;
> int x_src = composer->src.x1 >> 16;
> int y_src = composer->src.y1 >> 16;
> int h_src = drm_rect_height(>src) >> 16;
> int w_src = drm_rect_width(>src) >> 16;
> -   u32 crc = 0;
> +   u32 crc = 0, pixel = 0;
>
> -   for (i = y_src; i < y_src + h_src; ++i) {
> -   for (j = x_src; j < x_src + w_src; ++j) {
> -   src_offset = composer->offset
> -+ (i * composer->pitch)
> -+ (j * composer->cpp);
> +   for (y = y_src; y < y_src + h_src; ++y) {
> +   for (x = x_src; x < x_src + w_src; ++x) {
> /* XRGB format ignores Alpha channel */
> -   memset(vaddr_out + src_offset + 24, 0,  8);
> -   crc = crc32_le(crc, vaddr_out + src_offset,
> -  sizeof(u32));
> +   pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> +   bitmap_clear((void *), 0, 8);
> +   crc = crc32_le(crc, (void *), sizeof(u32));
> }
> }
>
IMHO using something like the following makes the code far simpler and clearer.

offset = composer->offset + (y_src * composer->pitch) + (x_src * composer->cpp);

for (i = 0; i < h_src; i++, offset += composer->pitch) {
   for (j = 0; j < w_src; j++, offset += composer->cpp) {
  pixel = get_pixel_from_buffer(vaddr, offset);
  crc = crc32_le(crc, , sizeof(u32); // cast should not be needed
   }
}

With the bitmap_clear() and related comment moved into get_pixel_from_buffer().

-Emil

Re: [PATCH v6 2/3] drm: Add support for the LogiCVC display controller

[Emil Velikov]

On Thu, 7 May 2020 at 21:11, Paul Kocialkowski
 wrote:
>
> Hi Emil,
>
> Thanks for the review!
>
> On Mon 04 May 20, 14:28, Emil Velikov wrote:
> > Just had a casual quick look for custom KMS properties, since new
> > drivers made that mistake in the past.
> > Thanks for not including any o/
>
> Yeah I made sure not to include any, I know it easily gets very problematic 
> and
> creates disparity between drivers while needing to be kept alive even when a
> standard way arises due to the no API breakage policy.
>
> The not-for-merge patch that I've sent does introduce some for the colorkey,
> and that's why they are marked as such :)
>
> > I made a couple of trivial suggestions - if you agree, feel free to
> > keep them as follow-up patches.
> >
> > On Thu, 30 Apr 2020 at 20:28, Paul Kocialkowski
> >  wrote:
> >
> > > +int logicvc_of_property_parse_u32(struct device_node *of_node,
> > > + const char *name, u32 *target)
> > > +{
> > > +   struct logicvc_of_property *property;
> > > +   const char *string;
> > > +   u32 value;
> > > +   int ret;
> > > +
> > > +   property = logicvc_of_property_lookup(name);
> > > +   if (!property)
> > > +   return -EINVAL;
> > > +
> > One could have the logicvc_of_properties[] entries indexed with the
> > logicvc_of_property_parse_{u32,bool} caller, using that instead of the
> > name string.
>
> Do I understand correctly that you're suggesting passing each entry's
> struct logicvc_of_property pointer to the function?
>
> I went for strings to make the code explicit and easy to read so I'd really
> like to keep it that way and avoid passing things like
> _of_properties[4] or an index integer.
>
Add a some #define/enum and go to town. Example with sub-optimal names below:

enum foobar {
 LVC_OF_DISP_INTF,
 LVC_OF_DISP_CLRSPC,
...
};

static struct logicvc_of_property logicvc_of_properties[] = {
[LVC_OF_DISP_INTF]  = {
   .name   = "xylon,display-interface",
...
   },
[LVC_OF_DISP_CLRSPC] = {
   .name   = "xylon,display-colorspace",
...
   },
...
}

While the callers are:

   ret = logicvc_of_property_parse_u32(of_node, LVC_OF_DISP_INTF,
   >display_colorspace);
   ret = logicvc_of_property_parse_u32(of_node, LVC_OF_DISP_CLRSPC,
   >display_depth);

-Emil

Re: [PATCH v6 2/3] drm: Add support for the LogiCVC display controller

[Emil Velikov]

Hi Paul,

Just had a casual quick look for custom KMS properties, since new
drivers made that mistake in the past.
Thanks for not including any o/

I made a couple of trivial suggestions - if you agree, feel free to
keep them as follow-up patches.

On Thu, 30 Apr 2020 at 20:28, Paul Kocialkowski
 wrote:

> +int logicvc_of_property_parse_u32(struct device_node *of_node,
> + const char *name, u32 *target)
> +{
> +   struct logicvc_of_property *property;
> +   const char *string;
> +   u32 value;
> +   int ret;
> +
> +   property = logicvc_of_property_lookup(name);
> +   if (!property)
> +   return -EINVAL;
> +
One could have the logicvc_of_properties[] entries indexed with the
logicvc_of_property_parse_{u32,bool} caller, using that instead of the
name string.

Aside: I suspect the array (as most other arrays in this patch) should
be annotated const, correct?


> +   if (property->range[0] || property->range[1])
> +   if (value < property->range[0] || value > property->range[1])
Combine the two ifs?

-Emil

Re: [RFC PATCH] modpost: check for static EXPORT_SYMBOL* functions

[Emil Velikov]

Hi Denis,

On 2019/07/14, Denis Efremov wrote:
> This patch adds a check to warn about static EXPORT_SYMBOL* functions
> during the modpost. In most of the cases, a static symbol marked for
> exporting is an odd combination that should be fixed either by deleting
> the exporting mark or by removing the static attribute and adding the
> appropriate declaration to headers.
> 
> If this check will be considered useful, I will resend the patch with
> review fixes.
> 
> Currently, this check emits the warnings on the following symbols, most
> of which are accepted to be fixed:
> 1. "sas_wait_eh" [drivers/scsi/libsas/libsas]
>Patch: https://lkml.org/lkml/2019/7/8/970 (accepted)
> 2. "torture_onoff_cleanup" [kernel/torture]
>"torture_shuffle_cleanup" [kernel/torture]
>Patch: https://lkml.org/lkml/2019/7/4/411 (accepted)
> 3. "LZ4HC_setExternalDict" [lib/lz4/lz4hc_compress]
>Patch: https://lkml.org/lkml/2019/7/8/842
> 4. "drm_client_close" [drivers/gpu/drm/drm]
>Patch: https://lkml.org/lkml/2019/7/3/758 (accepted)
> 5. "gve_probe" [drivers/net/ethernet/google/gve/gve]
>Patch: https://lkml.org/lkml/2019/7/14/65
> 6. "i2c_new_client_device" [vmlinux]
>"i2c_new_dummy_device" [vmlinux]
>Patch: https://lkml.org/lkml/2019/7/7/226 (fixed in a different patch)
> 7. "ahci_em_messages" [drivers/ata/libahci]
>Patch: https://lkml.org/lkml/2019/7/10/550 (reviwed)
> 8. "ftrace_set_clr_event" [vmlinux]
>Patch: https://lkml.org/lkml/2019/7/4/609 (reviwed)
> 9. "rmi_2d_sensor_set_input_params" [drivers/input/rmi4/rmi_core]
>Patch: https://lkml.org/lkml/2019/7/8/999
> 10. "empty_zero_page" [vmlinux]
> 11. "phys_base" [vmlinux]
> 12. "hypercall_page" [vmlinux]
> 
> Similar commits:
> 1. 54638c6eaf44 ("net: phy: make exported variables non-static")
> 2. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
> 3. 73df167c819e ("s390/zcrypt: remove the exporting of 
> ap_query_configuration")
> 4. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
> 5. e4e4730698c9 ("crypto: skcipher - remove the exporting of 
> skcipher_walk_next")
> 
> Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):
> $ make mrproper; make allmodconfig; time make -j12; \
>   git checkout HEAD~1; \
>   make mrproper; make allmodconfig; time make -j12
> 1.
>(with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
>(w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
> 2.
>(with patch) 17369,51s user 1763,28s system % cpu 28:41,47 total
>(w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
> 3.
>(with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
>(w/o  patch) 17267,55s user 1725,09s system % cpu 28:28,17 total
> 
> Thus, the current implementation adds approx. 1 min for allmodconfig.
> However, it's possible to do the check in a more optimal way if it will
> be considered useful.
> 
> Also, this kind of check could be implemented as a separate script instead.
> Here is the implementation:
> https://gist.github.com/evdenis/bf2322d094f0c02c0f60fe0a225848b2
> 

Personally I think this is a pretty good feature.

If I did my numbers correctly, the above numbers show ~2% increase.
Although one should be able to reduce that if people feel too strongly.

That said, the patch is:
Acked-by: Emil Velikov 

Can we make sure that patches for all issues are out (on the respective
mailing lists, or merged) before this lands.


HTH
Emil

Re: [PATCH v3] gpu/drm_memory: fix a few warnings

[Emil Velikov]

Hi Qian,

On 2019/07/12, Qian Cai wrote:
> Maybe one of the non-DRM maintainers (Andrew, Thomas or Greg) who cares a bit
> about SPDX can pick this up. It occurs to me none of DRM maintainers cares 
> about
> this as there is no feedback from any of them for months since v1.
> 
AFAICT there are a couple of reasons why this has gone unnoticed:
 - summary is the pretty ambiguous
 - commit does two thigs

Another thing to consider is that this patch touches a single file,
while the exact same issue is also present in many other files.

Quick look at the following lists:
head -n2 drivers/gpu/drm/drm_*[ch]  | less

drm_agpsupport.c
drm_dma.c
drm_legacy_misc.c
drm_lock.c
drm_memory.c
drm_scatter.c
drm_vm.c

If you can fixup the s|/**|/*| in the above, I'd gladly merge the patch.

On the topic of SPDX - no objection on my end, but it should be a
separate patch, which replaces the explicit verbose license text with
the tag.

Thanks
Emil

Re: [Patch v2 01/10] drm/exynos: using dev_get_drvdata directly

[Emil Velikov]

On Thu, 4 Jul 2019 at 08:26, Fuqian Huang  wrote:
>
> Several drivers cast a struct device pointer to a struct
> platform_device pointer only to then call platform_get_drvdata().
> To improve readability, these constructs can be simplified
> by using dev_get_drvdata() directly.
>
> Signed-off-by: Fuqian Huang 
Thanks for the update. This patch is:
Reviewed-by: Emil Velikov 

-Emil

Re: [PATCH 06/30] drm/amdgpu: Use kmemdup rather than duplicating its implementation

[Emil Velikov]

On Wed, 3 Jul 2019 at 14:15, Fuqian Huang  wrote:
>
> kmemdup is introduced to duplicate a region of memory in a neat way.
> Rather than kmalloc/kzalloc + memset, which the programmer needs to
> write the size twice (sometimes lead to mistakes), kmemdup improves
> readability, leads to smaller code and also reduce the chances of mistakes.
> Suggestion to use kmemdup rather than using kmalloc/kzalloc + memset.
>
> Signed-off-by: Fuqian Huang 
Fuqian please add reviewed-by and other tags when sending new revisions.

Fwiw the patch is:
Reviewed-by: Emil Velikov 

-Emil

Re: [PATCH 4.20 004/352] drm/vgem: Fix vgem_init to get drm device available.

[Emil Velikov]

Hi Greg,

On 2019/02/11, Greg Kroah-Hartman wrote:
> 4.20-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> [ Upstream commit d5c04dff24870ef07ce6453a3f4e1ffd9cf88d27 ]
> 
> Modify vgem_init to take platform dev as parent in drm_dev_init.
> This will make drm device available at "/sys/devices/platform/vgem"
> in x86 chromebook.
> 
> v2: rebase, address checkpatch typo and line over 80 characters
> 
> Cc: Daniel Vetter 
> Signed-off-by: Deepak Sharma 
> Reviewed-by: Sean Paul 
> Signed-off-by: Emil Velikov 
> Reviewed-by: Daniel Vetter 
> Link: 
> https://patchwork.freedesktop.org/patch/msgid/20181023163550.15211-1-emil.l.veli...@gmail.com
> Signed-off-by: Sasha Levin 

Zero objections to picking this for stable, although I'm curious what
flagged this commit as stable material.

Especially since it's closer to a feature than a bug-fix.

Can you share some light? Perhaps a link to a thread/document since I
would imagine you've been asked before.

Thanks
Emil

Re: [PATCH] drm: Rename crtc_idr as object_idr to KMS cleanups

[Emil Velikov]

Hi Shayenne,

Welcome to DRM.

As far as I can see you're a newcomer to kernel development, so I'd
recommend watch a recent talk from Marc [1]
He provides a very good introduction, both for newbies and for people
willing the know the deeper reasons behind.

That said, here's some suggestions:

On Wed, 31 Oct 2018 at 20:58, Shayenne da Luz Moura
 wrote:
>
I'd rename the title to "drm: rename drm_mode_config::crtc_idr to object_idr"
The "... as ... to KMS cleanups" translation is very strange in
English. It confused me and I've read the TODO over a dozen times ;-)

> Rename 'drm_mode_config.crtc_idr' as 'drm_mode_config.object_idr',
> as proposed in the task description in TODO list for KMS cleanups.
>
Similarly here.

> Signed-off-by: Shayenne da Luz Moura 
> ---
>  drivers/gpu/drm/drm_lease.c   | 6 +++---
>  drivers/gpu/drm/drm_mode_config.c | 4 ++--
>  drivers/gpu/drm/drm_mode_object.c | 8 
>  3 files changed, 9 insertions(+), 9 deletions(-)
>
As pointed out in the talk - always self review and ensure patches
don't break things.
Here, DRM doesn't build which is obviously not correct and breaks things.

HTH
Emil
[1] https://www.youtube.com/watch?v=LIdznotOxvg

Re: [PATCH] drm: Rename crtc_idr as object_idr to KMS cleanups

[Emil Velikov]

Hi Shayenne,

Welcome to DRM.

As far as I can see you're a newcomer to kernel development, so I'd
recommend watch a recent talk from Marc [1]
He provides a very good introduction, both for newbies and for people
willing the know the deeper reasons behind.

That said, here's some suggestions:

On Wed, 31 Oct 2018 at 20:58, Shayenne da Luz Moura
 wrote:
>
I'd rename the title to "drm: rename drm_mode_config::crtc_idr to object_idr"
The "... as ... to KMS cleanups" translation is very strange in
English. It confused me and I've read the TODO over a dozen times ;-)

> Rename 'drm_mode_config.crtc_idr' as 'drm_mode_config.object_idr',
> as proposed in the task description in TODO list for KMS cleanups.
>
Similarly here.

> Signed-off-by: Shayenne da Luz Moura 
> ---
>  drivers/gpu/drm/drm_lease.c   | 6 +++---
>  drivers/gpu/drm/drm_mode_config.c | 4 ++--
>  drivers/gpu/drm/drm_mode_object.c | 8 
>  3 files changed, 9 insertions(+), 9 deletions(-)
>
As pointed out in the talk - always self review and ensure patches
don't break things.
Here, DRM doesn't build which is obviously not correct and breaks things.

HTH
Emil
[1] https://www.youtube.com/watch?v=LIdznotOxvg

Re: [PATCH 2/5] drm/virtio: add uapi for in and out explicit fences

[Emil Velikov]

On Tue, 30 Oct 2018 at 13:52, Gerd Hoffmann  wrote:
>
> On Tue, Oct 30, 2018 at 11:31:04AM +, Emil Velikov wrote:
> > HI Gerd,
> >
> > On Tue, 30 Oct 2018 at 06:11, Gerd Hoffmann  wrote:
> > >
> > >   Hi,
> > >
> > > > The execbuffer IOCTL is now read-write to allow the userspace to read 
> > > > the
> > > > out-fence.
> > >
> > > >  #define DRM_IOCTL_VIRTGPU_EXECBUFFER \
> > > > - DRM_IOW(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
> > > > + DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
> > > >   struct drm_virtgpu_execbuffer)
> > >
> > > That changes the ioctl number and breaks the userspace api.
> > >
> > Have you looked at the drm_ioctl() implementation? AFAICT it
> > explicitly caters for this kind of changes.
>
> Looking ...
>
> The direction bits are not used to lookup the ioctl functions,
> so it should work indeed.
>
Nice, thanks for confirming.

> Series doesn't apply to drm-misc-next and needs a rebase.
>
Might be nicer to address that alongside any feedback. Otherwise it'll
be spamming people just for the sake of rebasing.

If i find some time I'll post some comments later on today.

HTH
Emil

Re: [PATCH 2/5] drm/virtio: add uapi for in and out explicit fences

[Emil Velikov]

On Tue, 30 Oct 2018 at 13:52, Gerd Hoffmann  wrote:
>
> On Tue, Oct 30, 2018 at 11:31:04AM +, Emil Velikov wrote:
> > HI Gerd,
> >
> > On Tue, 30 Oct 2018 at 06:11, Gerd Hoffmann  wrote:
> > >
> > >   Hi,
> > >
> > > > The execbuffer IOCTL is now read-write to allow the userspace to read 
> > > > the
> > > > out-fence.
> > >
> > > >  #define DRM_IOCTL_VIRTGPU_EXECBUFFER \
> > > > - DRM_IOW(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
> > > > + DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_EXECBUFFER,\
> > > >   struct drm_virtgpu_execbuffer)
> > >
> > > That changes the ioctl number and breaks the userspace api.
> > >
> > Have you looked at the drm_ioctl() implementation? AFAICT it
> > explicitly caters for this kind of changes.
>
> Looking ...
>
> The direction bits are not used to lookup the ioctl functions,
> so it should work indeed.
>
Nice, thanks for confirming.

> Series doesn't apply to drm-misc-next and needs a rebase.
>
Might be nicer to address that alongside any feedback. Otherwise it'll
be spamming people just for the sake of rebasing.

If i find some time I'll post some comments later on today.

HTH
Emil

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Emil Velikov]

On 7 August 2018 at 13:28, Daniel Vetter  wrote:
> On Tue, Aug 07, 2018 at 12:01:50PM +0100, Emil Velikov wrote:
>> On 3 August 2018 at 20:50, Sean Paul  wrote:
>> > On Fri, Aug 03, 2018 at 06:03:50PM +0100, Emil Velikov wrote:
>> >> On 3 August 2018 at 16:06, Martin Fuzzey  
>> >> wrote:
>> >> > Hi Emil,
>> >> >
>> >> > On 03/08/18 14:35, Emil Velikov wrote:
>> >> >>
>> >> >> Hi Martin,
>> >> >>
>> >> >> On 1 August 2018 at 15:24, Martin Fuzzey 
>> >> >> wrote:
>> >> >>
>> >> >> Let's start with the not-so obvious question:
>> >> >> Why does one open the imx as render node?
>> >> >>
>> >> >> Of the top of my head:
>> >> >> There is nothing in egl/android that should require an authenticated
>> >> >> device.
>> >> >> Hence, using a card node should be fine - the etnaviv code opens the
>> >> >> render node it needs.
>> >> >
>> >> >
>> >> > Yes, the problem is not in egl/android but in the scanout buffer 
>> >> > allocation
>> >> > code.
>> >> >
>> >> > etnaviv opens the render node on the *GPU* (for submitting GPU 
>> >> > commands),
>> >> > that part is fine.
>> >> >
>> >> > But scanout buffers need to be allocated from imx-drm not etnaviv.
>> >> >
>> >> > This done by renderonly_create_kms_dumb_buffer_for_resource()
>> >> > [src/gallium/auxiliary/renderonly/renderonly.c]
>> >> > Which uses DRM_IOCTL_MODE_CREATE_DUMB followed by
>> >> > DRM_IOCTL_PRIME_FD_TO_HANDLE
>> >> > on the "kms_fd" (probably poorly named because it's not actually used 
>> >> > for
>> >> > modesetting)
>> >> > see imx_drm_screen_create()[ 
>> >> > src/gallium/winsys/imx/drm/imx_drm_winsys.c]
>> >> >
>> >> >
>> >> > If the card node is used DRM_IOCTL_MODE_CREATE_DUMB works but
>> >> > DRM_IOCTL_PRIME_FD_TO_HANDLE fails, because the permissions are
>> >> > DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW
>> >> >
>> >> Right I missed the DRM_AUTH, in the fd <> handle IOCTLs.
>> >> So in order for things to work, we'd need to either:
>> >>  - allow dumb buffers for render nodes, or
>> >>  - drop the DRM_AUTH for fd <> handle imports
>> >>
>> >> Pointing an alternative solution, for kernel developers to analyse and
>> >> make a decision.
>> >>
>> >> >
>> >> > In android 8.1 the hardware composer runs in a seperate process and it 
>> >> > has
>> >> > to use the card node and be drm master (to use the KMS API),
>> >> > therefore, when the surface flinger calls
>> >> > renderonly_create_kms_dumb_buffer_for_resource() it is not 
>> >> > authenticated.
>> >> >
>> >> > Making surface flinger use a render node fixes the problem for
>> >> > DRM_IOCTL_PRIME_FD_TO_HANDLE (because that already has 
>> >> > DRM_RENDER_ALLOW),
>> >> > but DRM_IOCTL_MODE_CREATE_DUMB now fails without the patch.
>> >> >
>> >> >
>> >> > This probably worked in previous versions of Android where surface 
>> >> > flinger
>> >> > and hwc were all in the same process.
>> >> >
>> >> There has been varying hacks for Android through the years. Bringing
>> >> details into the discussion will result in a significant diversion.
>> >> Something we could avoid, for the time being ;-)
>> >
>> > Did someone say diversion?!? The way this was handled prior to using
>> > render/control nodes in drm_hwc/[drm/gbm]_gralloc is that all modesetting 
>> > was
>> > done via gralloc which was master. The hwc implementation was basically a 
>> > proxy
>> > backchanneling all of the work to gralloc.
>> >
>> > Anyways, we probably don't want to go back there.
>> >
>> Now that we got the diversion out of the way, any input on my proposal
>> to drop the DRM_AUTH for fd <> imports.
>> Am I missing something pretty obvious that makes the idea a no-go?
>
> Dropping DRM_AUTH is only relevant for the card node. And a card node
> might not be sufficiently isolated against concurrent other clients, which
> is why we don't allow it.
>
Right. I did not spot anything that would make a distinction based on
the card vs render node used.

> What we could do is essentially check whether your driver supports render
> nodes (indicating sufficient amounts of separation), and then allow
> anything for unauthenicated clients if DRM_RENDER_ALLOW is set on the
> ioctl.
>
> But that's just reinventing render nodes on top of legacy card nodes, and
> I'm not clear on what that exactly gains us.
>
As more of a userspace person, it makes sense to keep render nodes for
GPU specifics and card ones - KMS/Display Controller.

> I think the proposal for dumb render nodes (for drivers which only do dumb
> kms buffers and no rendering at all) that's been discusson on irc a bit
> makes a lot more sense.

Ack. Thanks for shedding some light.

-Emil

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Emil Velikov]

On 7 August 2018 at 13:28, Daniel Vetter  wrote:
> On Tue, Aug 07, 2018 at 12:01:50PM +0100, Emil Velikov wrote:
>> On 3 August 2018 at 20:50, Sean Paul  wrote:
>> > On Fri, Aug 03, 2018 at 06:03:50PM +0100, Emil Velikov wrote:
>> >> On 3 August 2018 at 16:06, Martin Fuzzey  
>> >> wrote:
>> >> > Hi Emil,
>> >> >
>> >> > On 03/08/18 14:35, Emil Velikov wrote:
>> >> >>
>> >> >> Hi Martin,
>> >> >>
>> >> >> On 1 August 2018 at 15:24, Martin Fuzzey 
>> >> >> wrote:
>> >> >>
>> >> >> Let's start with the not-so obvious question:
>> >> >> Why does one open the imx as render node?
>> >> >>
>> >> >> Of the top of my head:
>> >> >> There is nothing in egl/android that should require an authenticated
>> >> >> device.
>> >> >> Hence, using a card node should be fine - the etnaviv code opens the
>> >> >> render node it needs.
>> >> >
>> >> >
>> >> > Yes, the problem is not in egl/android but in the scanout buffer 
>> >> > allocation
>> >> > code.
>> >> >
>> >> > etnaviv opens the render node on the *GPU* (for submitting GPU 
>> >> > commands),
>> >> > that part is fine.
>> >> >
>> >> > But scanout buffers need to be allocated from imx-drm not etnaviv.
>> >> >
>> >> > This done by renderonly_create_kms_dumb_buffer_for_resource()
>> >> > [src/gallium/auxiliary/renderonly/renderonly.c]
>> >> > Which uses DRM_IOCTL_MODE_CREATE_DUMB followed by
>> >> > DRM_IOCTL_PRIME_FD_TO_HANDLE
>> >> > on the "kms_fd" (probably poorly named because it's not actually used 
>> >> > for
>> >> > modesetting)
>> >> > see imx_drm_screen_create()[ 
>> >> > src/gallium/winsys/imx/drm/imx_drm_winsys.c]
>> >> >
>> >> >
>> >> > If the card node is used DRM_IOCTL_MODE_CREATE_DUMB works but
>> >> > DRM_IOCTL_PRIME_FD_TO_HANDLE fails, because the permissions are
>> >> > DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW
>> >> >
>> >> Right I missed the DRM_AUTH, in the fd <> handle IOCTLs.
>> >> So in order for things to work, we'd need to either:
>> >>  - allow dumb buffers for render nodes, or
>> >>  - drop the DRM_AUTH for fd <> handle imports
>> >>
>> >> Pointing an alternative solution, for kernel developers to analyse and
>> >> make a decision.
>> >>
>> >> >
>> >> > In android 8.1 the hardware composer runs in a seperate process and it 
>> >> > has
>> >> > to use the card node and be drm master (to use the KMS API),
>> >> > therefore, when the surface flinger calls
>> >> > renderonly_create_kms_dumb_buffer_for_resource() it is not 
>> >> > authenticated.
>> >> >
>> >> > Making surface flinger use a render node fixes the problem for
>> >> > DRM_IOCTL_PRIME_FD_TO_HANDLE (because that already has 
>> >> > DRM_RENDER_ALLOW),
>> >> > but DRM_IOCTL_MODE_CREATE_DUMB now fails without the patch.
>> >> >
>> >> >
>> >> > This probably worked in previous versions of Android where surface 
>> >> > flinger
>> >> > and hwc were all in the same process.
>> >> >
>> >> There has been varying hacks for Android through the years. Bringing
>> >> details into the discussion will result in a significant diversion.
>> >> Something we could avoid, for the time being ;-)
>> >
>> > Did someone say diversion?!? The way this was handled prior to using
>> > render/control nodes in drm_hwc/[drm/gbm]_gralloc is that all modesetting 
>> > was
>> > done via gralloc which was master. The hwc implementation was basically a 
>> > proxy
>> > backchanneling all of the work to gralloc.
>> >
>> > Anyways, we probably don't want to go back there.
>> >
>> Now that we got the diversion out of the way, any input on my proposal
>> to drop the DRM_AUTH for fd <> imports.
>> Am I missing something pretty obvious that makes the idea a no-go?
>
> Dropping DRM_AUTH is only relevant for the card node. And a card node
> might not be sufficiently isolated against concurrent other clients, which
> is why we don't allow it.
>
Right. I did not spot anything that would make a distinction based on
the card vs render node used.

> What we could do is essentially check whether your driver supports render
> nodes (indicating sufficient amounts of separation), and then allow
> anything for unauthenicated clients if DRM_RENDER_ALLOW is set on the
> ioctl.
>
> But that's just reinventing render nodes on top of legacy card nodes, and
> I'm not clear on what that exactly gains us.
>
As more of a userspace person, it makes sense to keep render nodes for
GPU specifics and card ones - KMS/Display Controller.

> I think the proposal for dumb render nodes (for drivers which only do dumb
> kms buffers and no rendering at all) that's been discusson on irc a bit
> makes a lot more sense.

Ack. Thanks for shedding some light.

-Emil

Re: [PATCH v2 1/2] drm/blend: Add per-plane pixel blend mode property

[Emil Velikov]

Hi Lowry,

Small drive-by suggestion. Haven't checked if others have pointed it
out previously :-\

On 30 May 2018 at 12:23, Lowry Li  wrote:

> +/**
> + * drm_plane_create_blend_mode_property - create a new blend mode property
> + * @plane: drm plane
> + * @supported_modes: bitmask of supported modes, must include
> + *  BIT(DRM_MODE_BLEND_PREMULTI)
> + *
There are two possible blend modes (ignoring 'none'), yet premult must
be supported.
What if the hardware can do only "coverage"?

One-liner explaining why things are as-is would be a great move.

HTH
Emil

Re: [PATCH v2 1/2] drm/blend: Add per-plane pixel blend mode property

[Emil Velikov]

Hi Lowry,

Small drive-by suggestion. Haven't checked if others have pointed it
out previously :-\

On 30 May 2018 at 12:23, Lowry Li  wrote:

> +/**
> + * drm_plane_create_blend_mode_property - create a new blend mode property
> + * @plane: drm plane
> + * @supported_modes: bitmask of supported modes, must include
> + *  BIT(DRM_MODE_BLEND_PREMULTI)
> + *
There are two possible blend modes (ignoring 'none'), yet premult must
be supported.
What if the hardware can do only "coverage"?

One-liner explaining why things are as-is would be a great move.

HTH
Emil

Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[Emil Velikov]

On 20 March 2018 at 06:24, hl <h...@rock-chips.com> wrote:
> Hi Emil,
>
>
>
> On Monday, March 19, 2018 09:09 PM, Emil Velikov wrote:
>>
>> On 15 March 2018 at 02:35, hl <h...@rock-chips.com> wrote:
>>>
>>> Hi Emil,
>>>
>>>
>>>
>>> On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote:
>>>>
>>>> Hi Lin,
>>>>
>>>> On 14 March 2018 at 09:12, Lin Huang <h...@rock-chips.com> wrote:
>>>>>
>>>>> From: huang lin <h...@rock-chips.com>
>>>>>
>>>>> Refactor Innolux P079ZCA panel driver, let it support
>>>>> multi panel.
>>>>>
>>>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>>>>> Signed-off-by: Lin Huang <h...@rock-chips.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> - Change regulator property name to meet the panel datasheet
>>>>> Changes in v3:
>>>>> - this patch only refactor P079ZCA panel to support multi panel,
>>>>> support
>>>>> P097PFG panel in another patch
>>>>> Changes in v4:
>>>>> - Modify the patch which suggest by Thierry
>>>>>
>>>> Thanks for splitting this up. I think there's another piece that fell
>>>> through the cracks.
>>>> I'm not deeply familiar with the driver, so just sharing some quick
>>>> notes.
>>>>
>>>>
>>>>>struct innolux_panel {
>>>>>   struct drm_panel base;
>>>>>   struct mipi_dsi_device *link;
>>>>> +   const struct panel_desc *desc;
>>>>>
>>>>>   struct backlight_device *backlight;
>>>>> -   struct regulator *supply;
>>>>> +   struct regulator *vddi;
>>>>> +   struct regulator *avdd;
>>>>> +   struct regulator *avee;
>>>>
>>>> These two seem are new addition, as opposed to a dummy refactor.
>>>> Are they optional, does one need them for the existing panel (separate
>>>> patch?) or only for the new one (squash with the new panel code)?
>>>>
>>>>
>>>>>   struct gpio_desc *enable_gpio;
>>>>>
>>>>>   bool prepared;
>>>>> @@ -77,9 +93,9 @@ static int innolux_panel_unprepare(struct drm_panel
>>>>> *panel)
>>>>>   /* T8: 80ms - 1000ms */
>>>>>   msleep(80);
>>>>>
>>>>> -   err = regulator_disable(innolux->supply);
>>>>> -   if (err < 0)
>>>>> -   return err;
>>>>
>>>> Good call on dropping the early return here.
>>>>
>>>>
>>>>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs
>>>>> innolux_panel_funcs = {
>>>>> -   innolux->supply = devm_regulator_get(dev, "power");
>>>>> -   if (IS_ERR(innolux->supply))
>>>>> -   return PTR_ERR(innolux->supply);
>>>>> +   innolux = devm_kzalloc(dev, sizeof(*innolux), GFP_KERNEL);
>>>>> +   if (!innolux)
>>>>> +   return -ENOMEM;
>>>>> +
>>>>> +   innolux->desc = desc;
>>>>> +   innolux->vddi = devm_regulator_get(dev, "power");
>>>>> +   innolux->avdd = devm_regulator_get(dev, "avdd");
>>>>> +   innolux->avee = devm_regulator_get(dev, "avee");
>>>>>
>>>> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
>>>> passed into regulator_{enable,disable}.
>>>> Hence, the IS_ERR check should stay. If any of the regulators are
>>>> optional, you want to call regulator_{enable,disable} only as
>>>> applicable.
>>>
>>>
>>> devm_regulator_get() will use dummy_regulator if there not regulator pass
>>> to
>>> driver, so it not affect regulator_{enable, disable}.
>>
>> One of us is getting confused here:
>> devm_regulator_get does not _use_ a regulator, it returns a pointer to
>> a regulator, right?
>>
>> According to the 4.16-rc6 codebase - one error
>> returns a ERR_PTR [1].
>
> devm_regulator_get() will not reurn a ERR_PTR,  it will pass NORMAL_GET mode
> to
> _regulator_get() when you call devm_regulator_get(), and with following
> code:
>
Just before the _regulator_get() call we have "return ERR_PTR(-ENOMEM);"
See 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/devres.c?h=v4.16-rc6#n34


>> With the pointer dereferenced in regulator_enable [2], without a
>> IS_ERR check, hence thing will go boom(?)
>>
>>> These three regulator are
>>> optional,
>>> the p079zca will use "power" and ,
>>> so i think it better not to check ERR here.
>>>
>> What should happen if p079zca is missing "power" or p097pgf - "avdd" and
>> "avee"?
>> Obviously the latter two should be introduced with the p097pgf support.
>
> i think it need dts to make sure configure the power node correct, if
> missing
> "power" node fo p079zca or "avdd" "avee" node for p097pgf, the panel can
> not work, but do not affcet other driver, the kernel do not crash(as i
> explain before and i also test it).
>
If you know it won't work just don't continue? And yes, it will crash ;-)
Either way, if you don't like my feedback so be it.

HTH
Emil
P.S. As a non native English speaker to another - spell checker helps a lot ;-)

Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[Emil Velikov]

On 20 March 2018 at 06:24, hl  wrote:
> Hi Emil,
>
>
>
> On Monday, March 19, 2018 09:09 PM, Emil Velikov wrote:
>>
>> On 15 March 2018 at 02:35, hl  wrote:
>>>
>>> Hi Emil,
>>>
>>>
>>>
>>> On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote:
>>>>
>>>> Hi Lin,
>>>>
>>>> On 14 March 2018 at 09:12, Lin Huang  wrote:
>>>>>
>>>>> From: huang lin 
>>>>>
>>>>> Refactor Innolux P079ZCA panel driver, let it support
>>>>> multi panel.
>>>>>
>>>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>>>>> Signed-off-by: Lin Huang 
>>>>> ---
>>>>> Changes in v2:
>>>>> - Change regulator property name to meet the panel datasheet
>>>>> Changes in v3:
>>>>> - this patch only refactor P079ZCA panel to support multi panel,
>>>>> support
>>>>> P097PFG panel in another patch
>>>>> Changes in v4:
>>>>> - Modify the patch which suggest by Thierry
>>>>>
>>>> Thanks for splitting this up. I think there's another piece that fell
>>>> through the cracks.
>>>> I'm not deeply familiar with the driver, so just sharing some quick
>>>> notes.
>>>>
>>>>
>>>>>struct innolux_panel {
>>>>>   struct drm_panel base;
>>>>>   struct mipi_dsi_device *link;
>>>>> +   const struct panel_desc *desc;
>>>>>
>>>>>   struct backlight_device *backlight;
>>>>> -   struct regulator *supply;
>>>>> +   struct regulator *vddi;
>>>>> +   struct regulator *avdd;
>>>>> +   struct regulator *avee;
>>>>
>>>> These two seem are new addition, as opposed to a dummy refactor.
>>>> Are they optional, does one need them for the existing panel (separate
>>>> patch?) or only for the new one (squash with the new panel code)?
>>>>
>>>>
>>>>>   struct gpio_desc *enable_gpio;
>>>>>
>>>>>   bool prepared;
>>>>> @@ -77,9 +93,9 @@ static int innolux_panel_unprepare(struct drm_panel
>>>>> *panel)
>>>>>   /* T8: 80ms - 1000ms */
>>>>>   msleep(80);
>>>>>
>>>>> -   err = regulator_disable(innolux->supply);
>>>>> -   if (err < 0)
>>>>> -   return err;
>>>>
>>>> Good call on dropping the early return here.
>>>>
>>>>
>>>>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs
>>>>> innolux_panel_funcs = {
>>>>> -   innolux->supply = devm_regulator_get(dev, "power");
>>>>> -   if (IS_ERR(innolux->supply))
>>>>> -   return PTR_ERR(innolux->supply);
>>>>> +   innolux = devm_kzalloc(dev, sizeof(*innolux), GFP_KERNEL);
>>>>> +   if (!innolux)
>>>>> +   return -ENOMEM;
>>>>> +
>>>>> +   innolux->desc = desc;
>>>>> +   innolux->vddi = devm_regulator_get(dev, "power");
>>>>> +   innolux->avdd = devm_regulator_get(dev, "avdd");
>>>>> +   innolux->avee = devm_regulator_get(dev, "avee");
>>>>>
>>>> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
>>>> passed into regulator_{enable,disable}.
>>>> Hence, the IS_ERR check should stay. If any of the regulators are
>>>> optional, you want to call regulator_{enable,disable} only as
>>>> applicable.
>>>
>>>
>>> devm_regulator_get() will use dummy_regulator if there not regulator pass
>>> to
>>> driver, so it not affect regulator_{enable, disable}.
>>
>> One of us is getting confused here:
>> devm_regulator_get does not _use_ a regulator, it returns a pointer to
>> a regulator, right?
>>
>> According to the 4.16-rc6 codebase - one error
>> returns a ERR_PTR [1].
>
> devm_regulator_get() will not reurn a ERR_PTR,  it will pass NORMAL_GET mode
> to
> _regulator_get() when you call devm_regulator_get(), and with following
> code:
>
Just before the _regulator_get() call we have "return ERR_PTR(-ENOMEM);"
See 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/devres.c?h=v4.16-rc6#n34


>> With the pointer dereferenced in regulator_enable [2], without a
>> IS_ERR check, hence thing will go boom(?)
>>
>>> These three regulator are
>>> optional,
>>> the p079zca will use "power" and ,
>>> so i think it better not to check ERR here.
>>>
>> What should happen if p079zca is missing "power" or p097pgf - "avdd" and
>> "avee"?
>> Obviously the latter two should be introduced with the p097pgf support.
>
> i think it need dts to make sure configure the power node correct, if
> missing
> "power" node fo p079zca or "avdd" "avee" node for p097pgf, the panel can
> not work, but do not affcet other driver, the kernel do not crash(as i
> explain before and i also test it).
>
If you know it won't work just don't continue? And yes, it will crash ;-)
Either way, if you don't like my feedback so be it.

HTH
Emil
P.S. As a non native English speaker to another - spell checker helps a lot ;-)

Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[Emil Velikov]

On 15 March 2018 at 02:35, hl <h...@rock-chips.com> wrote:
> Hi Emil,
>
>
>
> On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote:
>>
>> Hi Lin,
>>
>> On 14 March 2018 at 09:12, Lin Huang <h...@rock-chips.com> wrote:
>>>
>>> From: huang lin <h...@rock-chips.com>
>>>
>>> Refactor Innolux P079ZCA panel driver, let it support
>>> multi panel.
>>>
>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>>> Signed-off-by: Lin Huang <h...@rock-chips.com>
>>> ---
>>> Changes in v2:
>>> - Change regulator property name to meet the panel datasheet
>>> Changes in v3:
>>> - this patch only refactor P079ZCA panel to support multi panel, support
>>> P097PFG panel in another patch
>>> Changes in v4:
>>> - Modify the patch which suggest by Thierry
>>>
>> Thanks for splitting this up. I think there's another piece that fell
>> through the cracks.
>> I'm not deeply familiar with the driver, so just sharing some quick notes.
>>
>>
>>>   struct innolux_panel {
>>>  struct drm_panel base;
>>>  struct mipi_dsi_device *link;
>>> +   const struct panel_desc *desc;
>>>
>>>  struct backlight_device *backlight;
>>> -   struct regulator *supply;
>>> +   struct regulator *vddi;
>>> +   struct regulator *avdd;
>>> +   struct regulator *avee;
>>
>> These two seem are new addition, as opposed to a dummy refactor.
>> Are they optional, does one need them for the existing panel (separate
>> patch?) or only for the new one (squash with the new panel code)?
>>
>>
>>>  struct gpio_desc *enable_gpio;
>>>
>>>  bool prepared;
>>> @@ -77,9 +93,9 @@ static int innolux_panel_unprepare(struct drm_panel
>>> *panel)
>>>  /* T8: 80ms - 1000ms */
>>>  msleep(80);
>>>
>>> -   err = regulator_disable(innolux->supply);
>>> -   if (err < 0)
>>> -   return err;
>>
>> Good call on dropping the early return here.
>>
>>
>>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs
>>> innolux_panel_funcs = {
>>> -   innolux->supply = devm_regulator_get(dev, "power");
>>> -   if (IS_ERR(innolux->supply))
>>> -   return PTR_ERR(innolux->supply);
>>> +   innolux = devm_kzalloc(dev, sizeof(*innolux), GFP_KERNEL);
>>> +   if (!innolux)
>>> +   return -ENOMEM;
>>> +
>>> +   innolux->desc = desc;
>>> +   innolux->vddi = devm_regulator_get(dev, "power");
>>> +   innolux->avdd = devm_regulator_get(dev, "avdd");
>>> +   innolux->avee = devm_regulator_get(dev, "avee");
>>>
>> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
>> passed into regulator_{enable,disable}.
>> Hence, the IS_ERR check should stay. If any of the regulators are
>> optional, you want to call regulator_{enable,disable} only as
>> applicable.
>
>
> devm_regulator_get() will use dummy_regulator if there not regulator pass to
> driver, so it not affect regulator_{enable, disable}.

One of us is getting confused here:
devm_regulator_get does not _use_ a regulator, it returns a pointer to
a regulator, right?

According to the 4.16-rc6 codebase - one error devm_regulator_get
returns a ERR_PTR [1].
With the pointer dereferenced in regulator_enable [2], without a
IS_ERR check, hence thing will go boom(?)

> These three regulator are
> optional,
> the p079zca will use "power" and ,
> so i think it better not to check ERR here.
>
What should happen if p079zca is missing "power" or p097pgf - "avdd" and "avee"?
Obviously the latter two should be introduced with the p097pgf support.

HTH
Emil

[1] 
https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/devres.c#L27
[2] 
https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/core.c#L2189

Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[Emil Velikov]

On 15 March 2018 at 02:35, hl  wrote:
> Hi Emil,
>
>
>
> On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote:
>>
>> Hi Lin,
>>
>> On 14 March 2018 at 09:12, Lin Huang  wrote:
>>>
>>> From: huang lin 
>>>
>>> Refactor Innolux P079ZCA panel driver, let it support
>>> multi panel.
>>>
>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>>> Signed-off-by: Lin Huang 
>>> ---
>>> Changes in v2:
>>> - Change regulator property name to meet the panel datasheet
>>> Changes in v3:
>>> - this patch only refactor P079ZCA panel to support multi panel, support
>>> P097PFG panel in another patch
>>> Changes in v4:
>>> - Modify the patch which suggest by Thierry
>>>
>> Thanks for splitting this up. I think there's another piece that fell
>> through the cracks.
>> I'm not deeply familiar with the driver, so just sharing some quick notes.
>>
>>
>>>   struct innolux_panel {
>>>  struct drm_panel base;
>>>  struct mipi_dsi_device *link;
>>> +   const struct panel_desc *desc;
>>>
>>>  struct backlight_device *backlight;
>>> -   struct regulator *supply;
>>> +   struct regulator *vddi;
>>> +   struct regulator *avdd;
>>> +   struct regulator *avee;
>>
>> These two seem are new addition, as opposed to a dummy refactor.
>> Are they optional, does one need them for the existing panel (separate
>> patch?) or only for the new one (squash with the new panel code)?
>>
>>
>>>  struct gpio_desc *enable_gpio;
>>>
>>>  bool prepared;
>>> @@ -77,9 +93,9 @@ static int innolux_panel_unprepare(struct drm_panel
>>> *panel)
>>>  /* T8: 80ms - 1000ms */
>>>  msleep(80);
>>>
>>> -   err = regulator_disable(innolux->supply);
>>> -   if (err < 0)
>>> -   return err;
>>
>> Good call on dropping the early return here.
>>
>>
>>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs
>>> innolux_panel_funcs = {
>>> -   innolux->supply = devm_regulator_get(dev, "power");
>>> -   if (IS_ERR(innolux->supply))
>>> -   return PTR_ERR(innolux->supply);
>>> +   innolux = devm_kzalloc(dev, sizeof(*innolux), GFP_KERNEL);
>>> +   if (!innolux)
>>> +   return -ENOMEM;
>>> +
>>> +   innolux->desc = desc;
>>> +   innolux->vddi = devm_regulator_get(dev, "power");
>>> +   innolux->avdd = devm_regulator_get(dev, "avdd");
>>> +   innolux->avee = devm_regulator_get(dev, "avee");
>>>
>> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
>> passed into regulator_{enable,disable}.
>> Hence, the IS_ERR check should stay. If any of the regulators are
>> optional, you want to call regulator_{enable,disable} only as
>> applicable.
>
>
> devm_regulator_get() will use dummy_regulator if there not regulator pass to
> driver, so it not affect regulator_{enable, disable}.

One of us is getting confused here:
devm_regulator_get does not _use_ a regulator, it returns a pointer to
a regulator, right?

According to the 4.16-rc6 codebase - one error devm_regulator_get
returns a ERR_PTR [1].
With the pointer dereferenced in regulator_enable [2], without a
IS_ERR check, hence thing will go boom(?)

> These three regulator are
> optional,
> the p079zca will use "power" and ,
> so i think it better not to check ERR here.
>
What should happen if p079zca is missing "power" or p097pgf - "avdd" and "avee"?
Obviously the latter two should be introduced with the p097pgf support.

HTH
Emil

[1] 
https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/devres.c#L27
[2] 
https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/core.c#L2189

Re: [PATCH v4 3/3] dt-bindings: Add INNOLUX P097PFG panel bindings

[Emil Velikov]

On 14 March 2018 at 09:12, Lin Huang  wrote:
> From: huang lin 
>
> The Innolux P097PFG panel is 9.7" panel with 1536X2048
> resolution, it reuse P079ZCA panel driver, so improve
> p079ZCA dt-binding to support P097PFG.
>
> Change-Id: I8704914898fe53b734d31fbe646df8aa5fd8b30d
> Signed-off-by: Lin Huang 
> ---
> Changes in v2:
> - None
> Changes in v3:
> - None
> Changes in v4:
> - None
>
>  .../devicetree/bindings/display/panel/innolux,p079zca.txt | 11 
> +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git 
> a/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt 
> b/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt
> index d0f5516..8cadd8c 100644
> --- a/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt
> +++ b/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt
> @@ -1,13 +1,18 @@
>  Innolux P079ZCA 7.85" 768x1024 TFT LCD panel
> +Innolux P097PFG 9.7" 1536x2048 TFT LCD panel
>
>  Required properties:
> -- compatible: should be "innolux,p079zca"
> +- compatible: should be should be one of the following.
> +-"innolux,p079zca" for Innolux 7.9" P079ZCA 768*1024 panel
> +-"innolux,p097pfg" for Innolux 9.7" P097PFG 1536*2048 panel
>  - reg: DSI virtual channel of the peripheral
> -- power-supply: phandle of the regulator that provides the supply voltage
>  - enable-gpios: panel enable gpio
>
>  Optional properties:
>  - backlight: phandle of the backlight device attached to the panel
> +- power-supply: phandle of the regulator that provides the supply voltage
> +- avdd-supply: phandle of the regulator that provides positive voltage
> +- avee-supply: phandle of the regulator that provides negative voltage
>
Guess that answers my required/optional question earlier.
Currently the code assumes that all of these are required. And will
crash badly if any are missing.

-Emil

Re: [PATCH v4 3/3] dt-bindings: Add INNOLUX P097PFG panel bindings

[Emil Velikov]

On 14 March 2018 at 09:12, Lin Huang  wrote:
> From: huang lin 
>
> The Innolux P097PFG panel is 9.7" panel with 1536X2048
> resolution, it reuse P079ZCA panel driver, so improve
> p079ZCA dt-binding to support P097PFG.
>
> Change-Id: I8704914898fe53b734d31fbe646df8aa5fd8b30d
> Signed-off-by: Lin Huang 
> ---
> Changes in v2:
> - None
> Changes in v3:
> - None
> Changes in v4:
> - None
>
>  .../devicetree/bindings/display/panel/innolux,p079zca.txt | 11 
> +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git 
> a/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt 
> b/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt
> index d0f5516..8cadd8c 100644
> --- a/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt
> +++ b/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt
> @@ -1,13 +1,18 @@
>  Innolux P079ZCA 7.85" 768x1024 TFT LCD panel
> +Innolux P097PFG 9.7" 1536x2048 TFT LCD panel
>
>  Required properties:
> -- compatible: should be "innolux,p079zca"
> +- compatible: should be should be one of the following.
> +-"innolux,p079zca" for Innolux 7.9" P079ZCA 768*1024 panel
> +-"innolux,p097pfg" for Innolux 9.7" P097PFG 1536*2048 panel
>  - reg: DSI virtual channel of the peripheral
> -- power-supply: phandle of the regulator that provides the supply voltage
>  - enable-gpios: panel enable gpio
>
>  Optional properties:
>  - backlight: phandle of the backlight device attached to the panel
> +- power-supply: phandle of the regulator that provides the supply voltage
> +- avdd-supply: phandle of the regulator that provides positive voltage
> +- avee-supply: phandle of the regulator that provides negative voltage
>
Guess that answers my required/optional question earlier.
Currently the code assumes that all of these are required. And will
crash badly if any are missing.

-Emil

Re: [PATCH v4 2/3] drm/panel: support Innolux P097PFG panel

[Emil Velikov]

On 14 March 2018 at 09:12, Lin Huang  wrote:
> Support Innolux P097PFG 9.7" 1536x2048 TFT LCD panel, it reuse
> the Innolux P079ZCA panel driver.
>
> Change-Id: I97923aa3735f707332681691b0231c9421b427d0
> Signed-off-by: Lin Huang 
> ---
> Changes in v2:
> - None
> Changes in v3:
> - None
> Changes in v4:
> - download panel initial code
>
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c | 192 
> +-
>  1 file changed, 187 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c 
> b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> index 2075a9d..883b279 100644
> --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> @@ -20,6 +20,15 @@
>
>  #include 
>
> +struct panel_init_cmd {
> +   int len;
> +   const char *data;
> +};
> +
> +#define _INIT_CMD(...) { \
> +   .len = sizeof((char[]){__VA_ARGS__}), \
> +   .data = (char[]){__VA_ARGS__} }
> +
>  struct panel_desc {
> const struct drm_display_mode *modes;
> unsigned int bpc;
> @@ -30,6 +39,7 @@ struct panel_desc {
>
> unsigned long flags;
> enum mipi_dsi_pixel_format format;
> +   const struct panel_init_cmd *init_cmds;
> unsigned int lanes;
>  };
>
> @@ -88,9 +98,12 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
> return err;
> }
>
> +   /* p097pfg: t15 */
> +   msleep(100);
> +
Based on the comment this is not needed for p079zca, yet still executed.

> gpiod_set_value_cansleep(innolux->enable_gpio, 0);
>
> -   /* T8: 80ms - 1000ms */
> +   /* p079zca: t8*/
> msleep(80);
>
And this is seem the opposite - zca only, yet executed on pfg.

One way to to avoid these and use the appropriate sleep throughout is
to store have the numbers in the respective panel_desc entries.



> regulator_disable(innolux->avee);
> @@ -124,13 +137,43 @@ static int innolux_panel_prepare(struct drm_panel 
> *panel)
> if (err < 0)
> goto disable_avdd;
>
> -   /* T2: 15ms - 1000ms */
> -   usleep_range(15000, 16000);
> +   /* p079zca: t2 (20ms), p097pfg: t4 (15ms) */
> +   usleep_range(2, 21000);
>
> gpiod_set_value_cansleep(innolux->enable_gpio, 1);
>
> -   /* T4: 15ms - 1000ms */
> -   usleep_range(15000, 16000);
> +   /* p079zca: t4, p097pfg: t5 */
> +   usleep_range(2, 21000);
> +
> +   if (innolux->desc->init_cmds) {
> +   const struct panel_init_cmd *cmds =
> +   innolux->desc->init_cmds;
> +   int i;
> +
> +   for (i = 0; cmds[i].len != 0; i++) {
> +   const struct panel_init_cmd *cmd = [i];
> +
> +   err = mipi_dsi_generic_write(innolux->link, cmd->data,
> +cmd->len);
> +   if (err < 0) {
> +   dev_err(panel->dev,
> +   "failed to write command %d\n", i);
> +   goto poweroff;
> +   }
> +
> +   /*
> +* Included by random guessing, because without this
> +* (or at least, some delay), the panel sometimes
> +* didn't appear to pick up the command sequence.
> +*/
This seems a bit hackish. Adding a reference to a bugreport/mailing
list discussion would be a nice move.
It'll save you/next person a lot of time searching for the specifics
of the problem.

> +   err = mipi_dsi_dcs_nop(innolux->link);
> +   if (err < 0) {
> +   dev_err(panel->dev,
> +   "failed to send DCS nop: %d\n", err);
> +   goto poweroff;
> +   }
> +   }
> +   }
>
> err = mipi_dsi_dcs_exit_sleep_mode(innolux->link);
> if (err < 0) {
> @@ -213,6 +256,142 @@ static const struct panel_desc 
> innolux_p079zca_panel_desc = {
> .lanes = 4,
>  };
>
> +static const struct drm_display_mode innolux_p097pfg_mode = {
> +   .clock = 229000,
> +   .hdisplay = 1536,
> +   .hsync_start = 1536 + 100,
> +   .hsync_end = 1536 + 100 + 24,
> +   .htotal = 1536 + 100 + 24 + 100,
> +   .vdisplay = 2048,
> +   .vsync_start = 2048 + 100,
> +   .vsync_end = 2048 + 100 + 2,
> +   .vtotal = 2048 + 100 + 2 + 18,
> +   .vrefresh = 60,
> +};
> +
> +static const struct panel_init_cmd innolux_p097pfg_init_cmds[] = {

A lot of magic numbers :-(

Please reference the source of these - say XX spec. vY.
We could get a vY+1, which makes the nop workaround obsolete.

HTH
Emil

Re: [PATCH v4 2/3] drm/panel: support Innolux P097PFG panel

[Emil Velikov]

On 14 March 2018 at 09:12, Lin Huang  wrote:
> Support Innolux P097PFG 9.7" 1536x2048 TFT LCD panel, it reuse
> the Innolux P079ZCA panel driver.
>
> Change-Id: I97923aa3735f707332681691b0231c9421b427d0
> Signed-off-by: Lin Huang 
> ---
> Changes in v2:
> - None
> Changes in v3:
> - None
> Changes in v4:
> - download panel initial code
>
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c | 192 
> +-
>  1 file changed, 187 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c 
> b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> index 2075a9d..883b279 100644
> --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> @@ -20,6 +20,15 @@
>
>  #include 
>
> +struct panel_init_cmd {
> +   int len;
> +   const char *data;
> +};
> +
> +#define _INIT_CMD(...) { \
> +   .len = sizeof((char[]){__VA_ARGS__}), \
> +   .data = (char[]){__VA_ARGS__} }
> +
>  struct panel_desc {
> const struct drm_display_mode *modes;
> unsigned int bpc;
> @@ -30,6 +39,7 @@ struct panel_desc {
>
> unsigned long flags;
> enum mipi_dsi_pixel_format format;
> +   const struct panel_init_cmd *init_cmds;
> unsigned int lanes;
>  };
>
> @@ -88,9 +98,12 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
> return err;
> }
>
> +   /* p097pfg: t15 */
> +   msleep(100);
> +
Based on the comment this is not needed for p079zca, yet still executed.

> gpiod_set_value_cansleep(innolux->enable_gpio, 0);
>
> -   /* T8: 80ms - 1000ms */
> +   /* p079zca: t8*/
> msleep(80);
>
And this is seem the opposite - zca only, yet executed on pfg.

One way to to avoid these and use the appropriate sleep throughout is
to store have the numbers in the respective panel_desc entries.



> regulator_disable(innolux->avee);
> @@ -124,13 +137,43 @@ static int innolux_panel_prepare(struct drm_panel 
> *panel)
> if (err < 0)
> goto disable_avdd;
>
> -   /* T2: 15ms - 1000ms */
> -   usleep_range(15000, 16000);
> +   /* p079zca: t2 (20ms), p097pfg: t4 (15ms) */
> +   usleep_range(2, 21000);
>
> gpiod_set_value_cansleep(innolux->enable_gpio, 1);
>
> -   /* T4: 15ms - 1000ms */
> -   usleep_range(15000, 16000);
> +   /* p079zca: t4, p097pfg: t5 */
> +   usleep_range(2, 21000);
> +
> +   if (innolux->desc->init_cmds) {
> +   const struct panel_init_cmd *cmds =
> +   innolux->desc->init_cmds;
> +   int i;
> +
> +   for (i = 0; cmds[i].len != 0; i++) {
> +   const struct panel_init_cmd *cmd = [i];
> +
> +   err = mipi_dsi_generic_write(innolux->link, cmd->data,
> +cmd->len);
> +   if (err < 0) {
> +   dev_err(panel->dev,
> +   "failed to write command %d\n", i);
> +   goto poweroff;
> +   }
> +
> +   /*
> +* Included by random guessing, because without this
> +* (or at least, some delay), the panel sometimes
> +* didn't appear to pick up the command sequence.
> +*/
This seems a bit hackish. Adding a reference to a bugreport/mailing
list discussion would be a nice move.
It'll save you/next person a lot of time searching for the specifics
of the problem.

> +   err = mipi_dsi_dcs_nop(innolux->link);
> +   if (err < 0) {
> +   dev_err(panel->dev,
> +   "failed to send DCS nop: %d\n", err);
> +   goto poweroff;
> +   }
> +   }
> +   }
>
> err = mipi_dsi_dcs_exit_sleep_mode(innolux->link);
> if (err < 0) {
> @@ -213,6 +256,142 @@ static const struct panel_desc 
> innolux_p079zca_panel_desc = {
> .lanes = 4,
>  };
>
> +static const struct drm_display_mode innolux_p097pfg_mode = {
> +   .clock = 229000,
> +   .hdisplay = 1536,
> +   .hsync_start = 1536 + 100,
> +   .hsync_end = 1536 + 100 + 24,
> +   .htotal = 1536 + 100 + 24 + 100,
> +   .vdisplay = 2048,
> +   .vsync_start = 2048 + 100,
> +   .vsync_end = 2048 + 100 + 2,
> +   .vtotal = 2048 + 100 + 2 + 18,
> +   .vrefresh = 60,
> +};
> +
> +static const struct panel_init_cmd innolux_p097pfg_init_cmds[] = {

A lot of magic numbers :-(

Please reference the source of these - say XX spec. vY.
We could get a vY+1, which makes the nop workaround obsolete.

HTH
Emil

Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[Emil Velikov]

Hi Lin,

On 14 March 2018 at 09:12, Lin Huang  wrote:
> From: huang lin 
>
> Refactor Innolux P079ZCA panel driver, let it support
> multi panel.
>
> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
> Signed-off-by: Lin Huang 
> ---
> Changes in v2:
> - Change regulator property name to meet the panel datasheet
> Changes in v3:
> - this patch only refactor P079ZCA panel to support multi panel, support 
> P097PFG panel in another patch
> Changes in v4:
> - Modify the patch which suggest by Thierry
>
Thanks for splitting this up. I think there's another piece that fell
through the cracks.
I'm not deeply familiar with the driver, so just sharing some quick notes.


>  struct innolux_panel {
> struct drm_panel base;
> struct mipi_dsi_device *link;
> +   const struct panel_desc *desc;
>
> struct backlight_device *backlight;
> -   struct regulator *supply;
> +   struct regulator *vddi;

> +   struct regulator *avdd;
> +   struct regulator *avee;
These two seem are new addition, as opposed to a dummy refactor.
Are they optional, does one need them for the existing panel (separate
patch?) or only for the new one (squash with the new panel code)?


> struct gpio_desc *enable_gpio;
>
> bool prepared;
> @@ -77,9 +93,9 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
> /* T8: 80ms - 1000ms */
> msleep(80);
>
> -   err = regulator_disable(innolux->supply);
> -   if (err < 0)
> -   return err;
Good call on dropping the early return here.


> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs innolux_panel_funcs 
> = {

>
> -   innolux->supply = devm_regulator_get(dev, "power");
> -   if (IS_ERR(innolux->supply))
> -   return PTR_ERR(innolux->supply);
> +   innolux = devm_kzalloc(dev, sizeof(*innolux), GFP_KERNEL);
> +   if (!innolux)
> +   return -ENOMEM;
> +
> +   innolux->desc = desc;
> +   innolux->vddi = devm_regulator_get(dev, "power");
> +   innolux->avdd = devm_regulator_get(dev, "avdd");
> +   innolux->avee = devm_regulator_get(dev, "avee");
>
AFAICT devm_regulator_get returns a pointer which is unsuitable to be
passed into regulator_{enable,disable}.
Hence, the IS_ERR check should stay. If any of the regulators are
optional, you want to call regulator_{enable,disable} only as
applicable.


> @@ -318,5 +377,6 @@ static struct mipi_dsi_driver innolux_panel_driver = {
>  module_mipi_dsi_driver(innolux_panel_driver);
>
>  MODULE_AUTHOR("Chris Zhong ");
> +MODULE_AUTHOR("Lin Huang ");
I don't think refactoring existing code classify as being the module author.
Then again, I could be wrong.

HTH
Emil

Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[Emil Velikov]

Hi Lin,

On 14 March 2018 at 09:12, Lin Huang  wrote:
> From: huang lin 
>
> Refactor Innolux P079ZCA panel driver, let it support
> multi panel.
>
> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
> Signed-off-by: Lin Huang 
> ---
> Changes in v2:
> - Change regulator property name to meet the panel datasheet
> Changes in v3:
> - this patch only refactor P079ZCA panel to support multi panel, support 
> P097PFG panel in another patch
> Changes in v4:
> - Modify the patch which suggest by Thierry
>
Thanks for splitting this up. I think there's another piece that fell
through the cracks.
I'm not deeply familiar with the driver, so just sharing some quick notes.


>  struct innolux_panel {
> struct drm_panel base;
> struct mipi_dsi_device *link;
> +   const struct panel_desc *desc;
>
> struct backlight_device *backlight;
> -   struct regulator *supply;
> +   struct regulator *vddi;

> +   struct regulator *avdd;
> +   struct regulator *avee;
These two seem are new addition, as opposed to a dummy refactor.
Are they optional, does one need them for the existing panel (separate
patch?) or only for the new one (squash with the new panel code)?


> struct gpio_desc *enable_gpio;
>
> bool prepared;
> @@ -77,9 +93,9 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
> /* T8: 80ms - 1000ms */
> msleep(80);
>
> -   err = regulator_disable(innolux->supply);
> -   if (err < 0)
> -   return err;
Good call on dropping the early return here.


> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs innolux_panel_funcs 
> = {

>
> -   innolux->supply = devm_regulator_get(dev, "power");
> -   if (IS_ERR(innolux->supply))
> -   return PTR_ERR(innolux->supply);
> +   innolux = devm_kzalloc(dev, sizeof(*innolux), GFP_KERNEL);
> +   if (!innolux)
> +   return -ENOMEM;
> +
> +   innolux->desc = desc;
> +   innolux->vddi = devm_regulator_get(dev, "power");
> +   innolux->avdd = devm_regulator_get(dev, "avdd");
> +   innolux->avee = devm_regulator_get(dev, "avee");
>
AFAICT devm_regulator_get returns a pointer which is unsuitable to be
passed into regulator_{enable,disable}.
Hence, the IS_ERR check should stay. If any of the regulators are
optional, you want to call regulator_{enable,disable} only as
applicable.


> @@ -318,5 +377,6 @@ static struct mipi_dsi_driver innolux_panel_driver = {
>  module_mipi_dsi_driver(innolux_panel_driver);
>
>  MODULE_AUTHOR("Chris Zhong ");
> +MODULE_AUTHOR("Lin Huang ");
I don't think refactoring existing code classify as being the module author.
Then again, I could be wrong.

HTH
Emil

Re: [PATCH] video: fbdev: via: remove VLA usage

[Emil Velikov]

On 9 March 2018 at 10:59, Eric Engestrom <eric.engest...@imgtec.com> wrote:
> On Thursday, 2018-03-08 11:39:49 -0600, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wvla, remove VLA usage.
>>
>> Also, fixed as part of the directive to remove all VLAs from
>> the kernel: https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
>
> Sorry for my reply on v1, I missed that Emil already replied to another
> post of the same patch (side note on that, making use of `-vX` and
> `--in-reply-to` helps track the evolution of patches)
>
> This looks good to me:
> Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>
>
Same here
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

-Emil

Re: [PATCH] video: fbdev: via: remove VLA usage

[Emil Velikov]

On 9 March 2018 at 10:59, Eric Engestrom  wrote:
> On Thursday, 2018-03-08 11:39:49 -0600, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wvla, remove VLA usage.
>>
>> Also, fixed as part of the directive to remove all VLAs from
>> the kernel: https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Gustavo A. R. Silva 
>
> Sorry for my reply on v1, I missed that Emil already replied to another
> post of the same patch (side note on that, making use of `-vX` and
> `--in-reply-to` helps track the evolution of patches)
>
> This looks good to me:
> Reviewed-by: Eric Engestrom 
>
Same here
Reviewed-by: Emil Velikov 

-Emil

Re: [PATCH] video: fbdev: via_aux_vt1632: remove VLA usage

[Emil Velikov]

Hi Gustavo,

On 7 March 2018 at 19:54, Gustavo A. R. Silva  wrote:
> In preparation to enabling -Wvla, remove VLA and replace it
> with a fixed-length array instead. Also, remove variable 'len'.
>
What is the reason behind adding -Wvla? Is there a thread some that I
can read up on?

> Notice that no new IDs have been added in seven years.
>
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/video/fbdev/via/via_aux_vt1632.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/via/via_aux_vt1632.c 
> b/drivers/video/fbdev/via/via_aux_vt1632.c
> index d24f4cd..0cd0d2a 100644
> --- a/drivers/video/fbdev/via/via_aux_vt1632.c
> +++ b/drivers/video/fbdev/via/via_aux_vt1632.c
> @@ -35,10 +35,10 @@ static void probe(struct via_aux_bus *bus, u8 addr)
> .addr   =   addr,
> .name   =   name};
> /* check vendor id and device id */
> -   const u8 id[] = {0x06, 0x11, 0x92, 0x31}, len = ARRAY_SIZE(id);
> -   u8 tmp[len];
> +   const u8 id[4] = {0x06, 0x11, 0x92, 0x31};
> +   u8 tmp[4];
>
> -   if (!via_aux_read(, 0x00, tmp, len) || memcmp(id, tmp, len))
> +   if (!via_aux_read(, 0x00, tmp, 4) || memcmp(id, tmp, 4))

Generally, hard coding a bunch of numbers like that makes for
confusing and fragile code.

A lot simpler and more obvious solution is like the following.
It silences the compiler warning (-Wvla - pedantic) you while keeping
the original clarity.

const u8 id[] = {0x06, 0x11, 0x92, 0x31}, len = ARRAY_SIZE(id);
-   u8 tmp[len];
+   u8 tmp[ARRAY_SIZE(id)]; // Using len causes a Wvla warning

HTH
Emil

Re: [PATCH] video: fbdev: via_aux_vt1632: remove VLA usage

[Emil Velikov]

Hi Gustavo,

On 7 March 2018 at 19:54, Gustavo A. R. Silva  wrote:
> In preparation to enabling -Wvla, remove VLA and replace it
> with a fixed-length array instead. Also, remove variable 'len'.
>
What is the reason behind adding -Wvla? Is there a thread some that I
can read up on?

> Notice that no new IDs have been added in seven years.
>
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/video/fbdev/via/via_aux_vt1632.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/via/via_aux_vt1632.c 
> b/drivers/video/fbdev/via/via_aux_vt1632.c
> index d24f4cd..0cd0d2a 100644
> --- a/drivers/video/fbdev/via/via_aux_vt1632.c
> +++ b/drivers/video/fbdev/via/via_aux_vt1632.c
> @@ -35,10 +35,10 @@ static void probe(struct via_aux_bus *bus, u8 addr)
> .addr   =   addr,
> .name   =   name};
> /* check vendor id and device id */
> -   const u8 id[] = {0x06, 0x11, 0x92, 0x31}, len = ARRAY_SIZE(id);
> -   u8 tmp[len];
> +   const u8 id[4] = {0x06, 0x11, 0x92, 0x31};
> +   u8 tmp[4];
>
> -   if (!via_aux_read(, 0x00, tmp, len) || memcmp(id, tmp, len))
> +   if (!via_aux_read(, 0x00, tmp, 4) || memcmp(id, tmp, 4))

Generally, hard coding a bunch of numbers like that makes for
confusing and fragile code.

A lot simpler and more obvious solution is like the following.
It silences the compiler warning (-Wvla - pedantic) you while keeping
the original clarity.

const u8 id[] = {0x06, 0x11, 0x92, 0x31}, len = ARRAY_SIZE(id);
-   u8 tmp[len];
+   u8 tmp[ARRAY_SIZE(id)]; // Using len causes a Wvla warning

HTH
Emil

Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

[Emil Velikov]

On 20 February 2018 at 13:12, Peter Zijlstra  wrote:
> On Tue, Feb 20, 2018 at 01:58:26PM +0100, Christian König wrote:
>> amdgpu needs to verify if userspace sends us valid addresses and the simplest
>> way of doing this is to check if the buffer object is locked with the ticket
>> of the current submission.
>>
>> Clean up the access to the ww_mutex internals by providing a function
>> for this and extend the check to the thread owning the underlying mutex.
>
>> Signed-off-by: Christian König 
>
> Much thanks for Cc'ing the relevant maintainers :/
>
Doubt it's intentional. The get-maintainer script seems confused and
lists no maintainers?

$ ./scripts/get_maintainer.pl include/linux/ww_mutex.h
linux-kernel@vger.kernel.org (open list)

While the normal mutex header works fine.

$ ./scripts/get_maintainer.pl include/linux/mutex.h
Peter Zijlstra  (maintainer:LOCKING PRIMITIVES)
Ingo Molnar  (maintainer:LOCKING PRIMITIVES)
linux-kernel@vger.kernel.org (open list:LOCKING PRIMITIVES)

HTH
Emil

Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

[Emil Velikov]

On 20 February 2018 at 13:12, Peter Zijlstra  wrote:
> On Tue, Feb 20, 2018 at 01:58:26PM +0100, Christian König wrote:
>> amdgpu needs to verify if userspace sends us valid addresses and the simplest
>> way of doing this is to check if the buffer object is locked with the ticket
>> of the current submission.
>>
>> Clean up the access to the ww_mutex internals by providing a function
>> for this and extend the check to the thread owning the underlying mutex.
>
>> Signed-off-by: Christian König 
>
> Much thanks for Cc'ing the relevant maintainers :/
>
Doubt it's intentional. The get-maintainer script seems confused and
lists no maintainers?

$ ./scripts/get_maintainer.pl include/linux/ww_mutex.h
linux-kernel@vger.kernel.org (open list)

While the normal mutex header works fine.

$ ./scripts/get_maintainer.pl include/linux/mutex.h
Peter Zijlstra  (maintainer:LOCKING PRIMITIVES)
Ingo Molnar  (maintainer:LOCKING PRIMITIVES)
linux-kernel@vger.kernel.org (open list:LOCKING PRIMITIVES)

HTH
Emil

Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions

[Emil Velikov]

HI Maciej,

Just sharing a couple of fly-by ideas - please don't read too much into them.

On 19 February 2018 at 15:43, Maciej Purski  wrote:
> When a driver is going to use clk_bulk_get() function, it has to
> initialize an array of clk_bulk_data, by filling its id fields.
>
> Add a new function to the core, which dynamically allocates
> clk_bulk_data array and fills its id fields. Add clk_bulk_free()
> function, which frees the array allocated by clk_bulk_alloc() function.
> Add a managed version of clk_bulk_alloc().
>
Most places use a small fixed number of struct clk pointers.
Using devres + kalloc to allocate 1-4 pointers feels a bit strange.

Quick grep shows over 150 instances that could be updated to use the new API.
Adding a cocci script to simplify the transition would be a good idea.

> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
The extra header declaration should not be needed. One should be able
to forward declare any undefined structs.

HTH
Emil

Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions

[Emil Velikov]

HI Maciej,

Just sharing a couple of fly-by ideas - please don't read too much into them.

On 19 February 2018 at 15:43, Maciej Purski  wrote:
> When a driver is going to use clk_bulk_get() function, it has to
> initialize an array of clk_bulk_data, by filling its id fields.
>
> Add a new function to the core, which dynamically allocates
> clk_bulk_data array and fills its id fields. Add clk_bulk_free()
> function, which frees the array allocated by clk_bulk_alloc() function.
> Add a managed version of clk_bulk_alloc().
>
Most places use a small fixed number of struct clk pointers.
Using devres + kalloc to allocate 1-4 pointers feels a bit strange.

Quick grep shows over 150 instances that could be updated to use the new API.
Adding a cocci script to simplify the transition would be a good idea.

> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
The extra header declaration should not be needed. One should be able
to forward declare any undefined structs.

HTH
Emil

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Emil Velikov]

On 17 January 2018 at 16:37, Daniel Thompson <daniel.thomp...@linaro.org> wrote:
>
>
> On 17/01/18 14:36, Emil Velikov wrote:
>>
>> On 17 January 2018 at 14:01, Daniel Vetter <daniel.vet...@ffwll.ch> wrote:
>>>
>>> Nothing in the entire tree ever sets this, which means this is dead
>>> code. Remove it.
>>>
>>> Cc: Lee Jones <lee.jo...@linaro.org>
>>> Cc: Daniel Thompson <daniel.thomp...@linaro.org>
>>> Cc: Jingoo Han <jingooh...@gmail.com>
>>> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
>>> ---
>>>   drivers/video/backlight/generic_bl.c | 5 -
>>
>>
>> Fly-by comment, while waiting for coffee to kick-in.
>> I think this patch should be after pandora/others have stopped abusing
>> BL_CORE_DRIVER1.
>
>
> You sure?
>
> I can't see why the use or disuse of BL_CORE_DRIVER1 in this driver should
> influence another independent driver.
>
Right my bad. Got mislead by the driver name.

-Emil

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Emil Velikov]

On 17 January 2018 at 16:37, Daniel Thompson  wrote:
>
>
> On 17/01/18 14:36, Emil Velikov wrote:
>>
>> On 17 January 2018 at 14:01, Daniel Vetter  wrote:
>>>
>>> Nothing in the entire tree ever sets this, which means this is dead
>>> code. Remove it.
>>>
>>> Cc: Lee Jones 
>>> Cc: Daniel Thompson 
>>> Cc: Jingoo Han 
>>> Signed-off-by: Daniel Vetter 
>>> ---
>>>   drivers/video/backlight/generic_bl.c | 5 -
>>
>>
>> Fly-by comment, while waiting for coffee to kick-in.
>> I think this patch should be after pandora/others have stopped abusing
>> BL_CORE_DRIVER1.
>
>
> You sure?
>
> I can't see why the use or disuse of BL_CORE_DRIVER1 in this driver should
> influence another independent driver.
>
Right my bad. Got mislead by the driver name.

-Emil

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Emil Velikov]

On 17 January 2018 at 14:01, Daniel Vetter  wrote:
> Nothing in the entire tree ever sets this, which means this is dead
> code. Remove it.
>
> Cc: Lee Jones 
> Cc: Daniel Thompson 
> Cc: Jingoo Han 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/video/backlight/generic_bl.c | 5 -

Fly-by comment, while waiting for coffee to kick-in.
I think this patch should be after pandora/others have stopped abusing
BL_CORE_DRIVER1.

-Emil

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Emil Velikov]

On 17 January 2018 at 14:01, Daniel Vetter  wrote:
> Nothing in the entire tree ever sets this, which means this is dead
> code. Remove it.
>
> Cc: Lee Jones 
> Cc: Daniel Thompson 
> Cc: Jingoo Han 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/video/backlight/generic_bl.c | 5 -

Fly-by comment, while waiting for coffee to kick-in.
I think this patch should be after pandora/others have stopped abusing
BL_CORE_DRIVER1.

-Emil

Re: xf86-video-armada via UDL [was: Re: UDL's fbdev doesn't work for user-space apps]

[Emil Velikov]

On 4 December 2017 at 14:02, Jose Abreu  wrote:
> On 04-12-2017 13:16, Alexey Brodkin wrote:
>> Option  "kmsdev" "/dev/dri/card1"
>
> Which drm driver uses /dev/dri/card0? I'm seing drmOpen code and
> if you don't specify the busID it will fallback for the first
> card that matches "armada-drm" or "imx-drm" or "udl".
>
As you've noticed, drmOpen can give you some 'nice' side-effects.
I'd recommend getting a list of devices with drmGetDevice{,s}2 and
applying $heuristics to filter the device you want.

-Emil

Re: xf86-video-armada via UDL [was: Re: UDL's fbdev doesn't work for user-space apps]

[Emil Velikov]

On 4 December 2017 at 14:02, Jose Abreu  wrote:
> On 04-12-2017 13:16, Alexey Brodkin wrote:
>> Option  "kmsdev" "/dev/dri/card1"
>
> Which drm driver uses /dev/dri/card0? I'm seing drmOpen code and
> if you don't specify the busID it will fallback for the first
> card that matches "armada-drm" or "imx-drm" or "udl".
>
As you've noticed, drmOpen can give you some 'nice' side-effects.
I'd recommend getting a list of devices with drmGetDevice{,s}2 and
applying $heuristics to filter the device you want.

-Emil

Re: [PATCH v4 1/3] drm/bridge/synopsys: dsi: stop clobbering drvdata

[Emil Velikov]

On 1 December 2017 at 08:32, Philippe CORNU  wrote:
> Dear Nickey,
>
> Many thanks for your patch.
>
> I am sorry to say that but you can not add my "Acked-by" to this patch
> because this code is different from the "original" one from Brian (which
> got my "Acked-by").
>
Small idea:

I've seen people mention the revision that got the r-b/ack. Although
in such cases the changelog tends to be part of the commit message.
For example:

drm: address ...

Determine X based on A.

v2:
 - Some changelog

Acked-by: Some Person  (v1)

-EMil

Re: [PATCH v4 1/3] drm/bridge/synopsys: dsi: stop clobbering drvdata

[Emil Velikov]

On 1 December 2017 at 08:32, Philippe CORNU  wrote:
> Dear Nickey,
>
> Many thanks for your patch.
>
> I am sorry to say that but you can not add my "Acked-by" to this patch
> because this code is different from the "original" one from Brian (which
> got my "Acked-by").
>
Small idea:

I've seen people mention the revision that got the r-b/ack. Although
in such cases the changelog tends to be part of the commit message.
For example:

drm: address ...

Determine X based on A.

v2:
 - Some changelog

Acked-by: Some Person  (v1)

-EMil

Re: [PATCH] drm/panel: support Innolux P097PFG panel

[Emil Velikov]

On 30 November 2017 at 06:13, Lin Huang  wrote:
> Support Innolux P097PFG 9.7" 1536x2048 TFT LCD panel,
> it refactor Innolux P079ZCA panel driver, let it support
> multi panel, and add support P097PFG panel in this driver.
>
Couple of drive-by suggestions:

Split the refactor vs new hw?

>  MODULE_DESCRIPTION("Innolux P079ZCA panel driver");
Update this reference alongside the ones in Kconfig?

-Emil

Re: [PATCH] drm/panel: support Innolux P097PFG panel

[Emil Velikov]

On 30 November 2017 at 06:13, Lin Huang  wrote:
> Support Innolux P097PFG 9.7" 1536x2048 TFT LCD panel,
> it refactor Innolux P079ZCA panel driver, let it support
> multi panel, and add support P097PFG panel in this driver.
>
Couple of drive-by suggestions:

Split the refactor vs new hw?

>  MODULE_DESCRIPTION("Innolux P079ZCA panel driver");
Update this reference alongside the ones in Kconfig?

-Emil

Re: Autoselect patches for stable (Was: Re: [PATCH AUTOSEL for 4.9 36/56] drm/i915: Fix the level 0 max_wm hack on VLV/CHV)

[Emil Velikov]

On 21 November 2017 at 15:07,  <alexander.le...@verizon.com> wrote:
> On Mon, Nov 20, 2017 at 11:21:52AM +0000, Emil Velikov wrote:
>> - Document the autoselect process
>>Information about about What, Why, and [ideally] How - analogous to
>>the normal stable nominations.
>>Insert reference to the process in the patch notification email.
>
> I agree with this one, and it'll definitely happen. The story behind
> this is that this is all based on Julia Lawall's work which is well
> documented in a published paper here:
>
> https://soarsmu.github.io/papers/icse12-patch.pdf
>
> I have modified inputs and process, but it essentially is very similar
> to what's described in that paper.
>
> While I have no problem with sharing what I have so far, this is
> still very much work in progress, and things keep constantly changing
> based on comments I receive from reviewers and Greg, so I want to
> reach a more stable point before trying to explain things and change
> my mind the day after :)
>
> If anyone is really interested in seeing the guts of this mess I
> currently have I can push it to github, but bear in mind that in it's
> current state it's very custom to the configuration I have, and is
> a borderline unreadable mix of bash scripts and LUA.
>
> Ideally it'll all get cleaned up and pushed anyways once I feel
> comfortable with the quality of the process.
>
At first I would focus on What and Why. Getting that information out
and publicising it via that blogs, G+, meetings, etc. is essential.
Reference to the current [WIP or not] heuristics is nice but can
follow-up in due time. A placeholder must be available though.

>> - Make the autoselect nominations _more_ distinct than the normal stable 
>> ones.
>>Maintainers will want to put more cognitive effort into the patches.
>
> So this came up before, and the participants of that thread agreed
> that adding "AUTOSEL" in the patch prefix is sufficient. What else
> would you suggest adding?
>
Being consistent [with existing stable nominations style] is good, but
first focus* should be on making it noticeable and distinct.
In other words - do _not_ be consistent.

Flipping the order AUTOSEL PATCH, using WARN, NOTE or just dropping
PATCH should help.
People tend to read PATC. /xx: ... last words of commit message.

Additionally, different template + a big note/warning in the email
body is a good idea. Say:
WARNING: This patch is nominated via the autosel procedure as defined at $ref.

HTH
Emil

* Regardless if autosel patches default to "ACK to merge" or not.

Re: Autoselect patches for stable (Was: Re: [PATCH AUTOSEL for 4.9 36/56] drm/i915: Fix the level 0 max_wm hack on VLV/CHV)

[Emil Velikov]

On 21 November 2017 at 15:07,   wrote:
> On Mon, Nov 20, 2017 at 11:21:52AM +0000, Emil Velikov wrote:
>> - Document the autoselect process
>>Information about about What, Why, and [ideally] How - analogous to
>>the normal stable nominations.
>>Insert reference to the process in the patch notification email.
>
> I agree with this one, and it'll definitely happen. The story behind
> this is that this is all based on Julia Lawall's work which is well
> documented in a published paper here:
>
> https://soarsmu.github.io/papers/icse12-patch.pdf
>
> I have modified inputs and process, but it essentially is very similar
> to what's described in that paper.
>
> While I have no problem with sharing what I have so far, this is
> still very much work in progress, and things keep constantly changing
> based on comments I receive from reviewers and Greg, so I want to
> reach a more stable point before trying to explain things and change
> my mind the day after :)
>
> If anyone is really interested in seeing the guts of this mess I
> currently have I can push it to github, but bear in mind that in it's
> current state it's very custom to the configuration I have, and is
> a borderline unreadable mix of bash scripts and LUA.
>
> Ideally it'll all get cleaned up and pushed anyways once I feel
> comfortable with the quality of the process.
>
At first I would focus on What and Why. Getting that information out
and publicising it via that blogs, G+, meetings, etc. is essential.
Reference to the current [WIP or not] heuristics is nice but can
follow-up in due time. A placeholder must be available though.

>> - Make the autoselect nominations _more_ distinct than the normal stable 
>> ones.
>>Maintainers will want to put more cognitive effort into the patches.
>
> So this came up before, and the participants of that thread agreed
> that adding "AUTOSEL" in the patch prefix is sufficient. What else
> would you suggest adding?
>
Being consistent [with existing stable nominations style] is good, but
first focus* should be on making it noticeable and distinct.
In other words - do _not_ be consistent.

Flipping the order AUTOSEL PATCH, using WARN, NOTE or just dropping
PATCH should help.
People tend to read PATC. /xx: ... last words of commit message.

Additionally, different template + a big note/warning in the email
body is a good idea. Say:
WARNING: This patch is nominated via the autosel procedure as defined at $ref.

HTH
Emil

* Regardless if autosel patches default to "ACK to merge" or not.