Re: [PATCH v5 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
Hi lichenyang, On Sat, Sep 11, 2021 at 10:31:31AM +0800, lichenyang wrote: > From: Chenyang Li > > This patch adds an initial DRM driver for the Loongson LS7A1000 > bridge chip(LS7A). The LS7A bridge chip contains two display > controllers, support dual display output. The maximum support for > each channel display is to 1920x1080@60Hz. > At present, DC device detection and DRM driver registration are > completed, the crtc/plane/encoder/connector objects has been > implemented. > On Loongson 3A4000 CPU and 7A1000 system, we have achieved the use > of dual screen, and support dual screen clone mode and expansion > mode. > > v11: > - Remove a lot of useless code. > - Add help information. > - Delete unnecessary header files. Looks much better now, thanks. Can you provide some kind of overview of the HW? It is confusing that you talk about a bridge for a display driver - is this something from the HW? And please look into usign the drm_bridge_connector - as this is what any modern DRM display driver needs to use today. Also the connector type needs to be specified - unknown is not acceptable here. The mail you sent did not show up at https://lore.kernel.org/dri-devel/ Please fix what is required to make it visible there. This is where we point people to see the original mails. Also a cover letter that explains what has been done - and what has not been done - would be nice. I look forward to v12, Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] drm/loongson: Add interrupt driver for LS7A
Hi lichenyang, On Fri, Jul 30, 2021 at 05:41:48PM +0800, lichenyang wrote: > Add LS7A DC vsync interrupt enable and close function, and > register irq_handler function interface. > Add vbrank event processing flow. s/vbrank/vblank/ > > v4: > - Replace drm_irq_install with devm_request_irq. > - Delete the irq_ hooks in drm_driver. > > v3: > - Improve code readability. > - Use the to_pci_dev function to get pci_dev. > > v2: > - Added error handling in the loongson_drm_load function. > > Signed-off-by: lichenyang Patch looks good, Acked-by: Sam Ravnborg But then I am not to fluent in the vblank stuff, so I hope someone else takes a look too. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 2/3] drm/loongson: Add GPIO and I2C driver for loongson drm.
Hi lichenyang,
On Fri, Jul 30, 2021 at 05:41:47PM +0800, lichenyang wrote:
> Implement use GPIO and I2C driver to detect connector
> and fetch EDID via DDC.
>
> v3:
> - Change some driver log to the drm_ version.
>
> v2:
> - Optimize the error handling process.
> - Delete loongson_i2c_bus_match and loongson_i2c_add function.
> - Optimize part of the code flow.
>
> Signed-off-by: lichenyang
Thanks for keeping me in the loop on this patch series.
In general the code looks pretty clean and well structured which makes
review easier - good.
As already said this driver should be implemented as a bridge which
would make the integration with the display driver simpler and more
straightforward.
When implementing this as a bridge driver there will be no drm_device,
so logging will need to use dev_err and friends.
Try to run the driver(s) with sparse:
make C=2 drivers/gpu/drm/loongson/
I think this will give you a few warnigns to fix.
Likewise use checkpatch --strict as this often resutls in a few easy to
fix warnings.
Last try to build with W=1 and fix warnings too.
The above goes for all files in this driver as we would like to have all
new drivers W=1, sparse and checkpatch clean.
Some more specific comments in the following,
Sam
> ---
> drivers/gpu/drm/loongson/Makefile | 1 +
> drivers/gpu/drm/loongson/loongson_connector.c | 59 -
> drivers/gpu/drm/loongson/loongson_drv.c | 15 +-
> drivers/gpu/drm/loongson/loongson_drv.h | 11 +
> drivers/gpu/drm/loongson/loongson_i2c.c | 249 ++
> drivers/gpu/drm/loongson/loongson_i2c.h | 36 +++
> 6 files changed, 366 insertions(+), 5 deletions(-)
> create mode 100644 drivers/gpu/drm/loongson/loongson_i2c.c
> create mode 100644 drivers/gpu/drm/loongson/loongson_i2c.h
>
> diff --git a/drivers/gpu/drm/loongson/Makefile
> b/drivers/gpu/drm/loongson/Makefile
> index d73ad44fe1d5..a842e85cf6ca 100644
> --- a/drivers/gpu/drm/loongson/Makefile
> +++ b/drivers/gpu/drm/loongson/Makefile
> @@ -10,5 +10,6 @@ loongson-y := loongson_connector.o \
> loongson_device.o \
> loongson_drv.o \
> loongson_encoder.o \
> + loongson_i2c.o \
> loongson_plane.o
> obj-$(CONFIG_DRM_LOONGSON) += loongson.o
> diff --git a/drivers/gpu/drm/loongson/loongson_connector.c
> b/drivers/gpu/drm/loongson/loongson_connector.c
> index a4762d8f9987..bdf7d651d6d1 100644
> --- a/drivers/gpu/drm/loongson/loongson_connector.c
> +++ b/drivers/gpu/drm/loongson/loongson_connector.c
> @@ -4,12 +4,56 @@
>
> static int loongson_get_modes(struct drm_connector *connector)
> {
> - int count;
> + struct drm_device *dev = connector->dev;
> + struct loongson_connector *lconnector =
> + to_loongson_connector(connector);
> + struct i2c_adapter *adapter = lconnector->i2c->adapter;
> + struct edid *edid = NULL;
> + u32 ret;
>
> - count = drm_add_modes_noedid(connector, 1920, 1080);
> - drm_set_preferred_mode(connector, 1024, 768);
> + edid = drm_get_edid(connector, adapter);
> + if (edid) {
> + drm_connector_update_edid_property(connector, edid);
> + ret = drm_add_edid_modes(connector, edid);
> + } else {
> + drm_warn(dev, "Failed to read EDID\n");
> + ret = drm_add_modes_noedid(connector, 1920, 1080);
> + drm_set_preferred_mode(connector, 1024, 768);
> + }
>
> - return count;
> + return ret;
> +}
> +
> +static bool is_connected(struct loongson_connector *lconnector)
> +{
> + struct i2c_adapter *adapter = lconnector->i2c->adapter;
> + unsigned char start = 0x0;
> + struct i2c_msg msgs = {
> + .addr = DDC_ADDR,
> + .flags = 0,
> + .len = 1,
> + .buf = ,
> + };
> +
> + if (!lconnector->i2c)
> + return false;
> +
> + if (i2c_transfer(adapter, , 1) != 1)
> + return false;
> +
> + return true;
> +}
> +
> +static enum drm_connector_status
> +loongson_detect(struct drm_connector *connector, bool force)
> +{
> + struct loongson_connector *lconnector =
> + to_loongson_connector(connector);
> +
> + if (is_connected(lconnector))
> + return connector_status_connected;
> +
> + return connector_status_disconnected;
> }
>
> static const struct drm_connector_helper_funcs loongson_connector_helper = {
> @@ -17,6 +61,7 @@ static const struct drm_connector_helper_funcs
> loongson_connector_helper = {
> };
>
> static const struct drm_connector_funcs loongson_connector_funcs = {
> + .detect = loongson_detect,
> .fill_modes = drm_helper_probe_single_connector_modes,
> .destroy = drm_connector_cleanup,
> .reset = drm_atomic_helper_connector_reset,
> @@ -36,6 +81,12 @@ int loongson_connector_init(struct loongson_device *ldev,
> int index)
>
> lconnector->ldev = ldev;
> lconnector->id =
Re: [PATCH v1 1/1] drm/bridge: anx7625: Tune K value for IVO panel
On Thu, Aug 05, 2021 at 03:30:55PM +0800, Xin Ji wrote: > IVO panel require less input video clock variation than video clock > variation in DP CTS spec. > > This patch decreases the K value of ANX7625 which will shrink eDP Tx > video clock variation to meet IVO panel's requirement. > > Signed-off-by: Xin Ji Looks good, I assume someone else (Robert) picks this. Acked-by: Sam Ravnborg Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
Hi Chenyang, some feedback in the following. I will try to find more time for review during the week. Hi Thomas, please see my question near drm_gem_vram_of_gem(). Sam On Fri, Jul 30, 2021 at 05:41:46PM +0800, lichenyang wrote: > From: Chenyang Li > > This patch adds an initial DRM driver for the Loongson LS7A1000 > bridge chip(LS7A). The LS7A bridge chip contains two display > controllers, support dual display output. The maximum support for > each channel display is to 1920x1080@60Hz. > At present, DC device detection and DRM driver registration are > completed, the crtc/plane/encoder/connector objects has been > implemented. > On Loongson 3A4000 CPU and 7A1000 system, we have achieved the use > of dual screen, and support dual screen clone mode and expansion > mode. > > v10: > - Replace the drmm_ version functions. > - Replace the simple_encoder version function. > - Alphabetize file names. > > v9: > - Optimize the error handling process. > - Remove the useless flags parameter. > - Fix some incorrect use of variables and constructs. > > v8: > - Update the atomic_update function interface. > > v7: > - The pixel clock is limited to less than 173000. > > v6: > - Remove spin_lock in mmio reg read and write. > - TO_UNCAC is replac with ioremap. > - Fix error arguments in crtc_atomic_enable/disable/mode_valid. > > v5: > - Change the name of the chip to LS7A. > - Change magic value in crtc to macros. > - Correct mistakes words. > - Change the register operation function prefix to ls7a. > > v4: > - Move the mode_valid function to the crtc. > > v3: > - Move the mode_valid function to the connector and optimize it. > - Fix num_crtc calculation method. > > v2: > - Complete the case of 32-bit color in CRTC. > > Signed-off-by: Chenyang Li > --- > drivers/gpu/drm/Kconfig | 2 + > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/loongson/Kconfig | 14 + > drivers/gpu/drm/loongson/Makefile | 14 + > drivers/gpu/drm/loongson/loongson_connector.c | 47 +++ > drivers/gpu/drm/loongson/loongson_crtc.c | 238 +++ > drivers/gpu/drm/loongson/loongson_device.c| 35 +++ > drivers/gpu/drm/loongson/loongson_drv.c | 271 ++ > drivers/gpu/drm/loongson/loongson_drv.h | 149 ++ > drivers/gpu/drm/loongson/loongson_encoder.c | 21 ++ > drivers/gpu/drm/loongson/loongson_plane.c | 92 ++ > 11 files changed, 884 insertions(+) > create mode 100644 drivers/gpu/drm/loongson/Kconfig > create mode 100644 drivers/gpu/drm/loongson/Makefile > create mode 100644 drivers/gpu/drm/loongson/loongson_connector.c > create mode 100644 drivers/gpu/drm/loongson/loongson_crtc.c > create mode 100644 drivers/gpu/drm/loongson/loongson_device.c > create mode 100644 drivers/gpu/drm/loongson/loongson_drv.c > create mode 100644 drivers/gpu/drm/loongson/loongson_drv.h > create mode 100644 drivers/gpu/drm/loongson/loongson_encoder.c > create mode 100644 drivers/gpu/drm/loongson/loongson_plane.c > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 7ff89690a976..08562d9be6e3 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -365,6 +365,8 @@ source "drivers/gpu/drm/xen/Kconfig" > > source "drivers/gpu/drm/vboxvideo/Kconfig" > > +source "drivers/gpu/drm/loongson/Kconfig" > + > source "drivers/gpu/drm/lima/Kconfig" Preferably in alphabetical order, so after lima. > > source "drivers/gpu/drm/panfrost/Kconfig" > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index a118692a6df7..29c05b8cf2ad 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -119,6 +119,7 @@ obj-$(CONFIG_DRM_PL111) += pl111/ > obj-$(CONFIG_DRM_TVE200) += tve200/ > obj-$(CONFIG_DRM_XEN) += xen/ > obj-$(CONFIG_DRM_VBOXVIDEO) += vboxvideo/ > +obj-$(CONFIG_DRM_LOONGSON) += loongson/ > obj-$(CONFIG_DRM_LIMA) += lima/ Likewise, after lima > obj-$(CONFIG_DRM_PANFROST) += panfrost/ > obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/ > diff --git a/drivers/gpu/drm/loongson/Kconfig > b/drivers/gpu/drm/loongson/Kconfig > new file mode 100644 > index ..3cf42a4cca08 > --- /dev/null > +++ b/drivers/gpu/drm/loongson/Kconfig > @@ -0,0 +1,14 @@ > +# SPDX-License-Identifier: GPL-2.0-only > + > +config DRM_LOONGSON > + tristate "DRM support for LS7A bridge chipset" > + depends on DRM && PCI > + depends on CPU_LOONGSON64 Maybe add || COMPILE_TEST - so we get better build coverage. You risk we miss this driver when we do refactoring, if we cannot build it using an allmodconfig for example. > + select DRM_KMS_HELPER > + select DRM_VRAM_HELPER > + select DRM_TTM > + select DRM_TTM_HELPER Please verify that they are all needed. There are no hits on "ttm" in the code, so the the two TTM symbols is likely wrong. > + default n Drop this. n is default. > + help > + Support the display controllers
Re: [PATCH v4 2/3] drm/loongson: Add GPIO and I2C driver for loongson drm.
Hi lichenyang, On Fri, Jul 30, 2021 at 05:41:47PM +0800, lichenyang wrote: > Implement use GPIO and I2C driver to detect connector > and fetch EDID via DDC. > > v3: > - Change some driver log to the drm_ version. > > v2: > - Optimize the error handling process. > - Delete loongson_i2c_bus_match and loongson_i2c_add function. > - Optimize part of the code flow. > > Signed-off-by: lichenyang I will return later with more detailed feedback. One high-level comment is that all the i2c support would be much better modelled as a bridge. And then you could use the bridge_connector. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
Hi Chenyang, I browsed the code on lore and noticed a few things and thought it better to bring it to your attention now. The general structure of the drivers seems good and coding style is fine. The feedback is mostly stuff we have decided to do different over time, so likely because you based the driver on some older driver. And it can be hard to follow all the refactoring going on - something that you get for free (almost) when is is mainlined. 1) Use drm_ for logging whereever possible (need drm_device) 2) Do not use irq mid-layer support in drm_driver, it is about to be legacy only. In other words avoid using drm_irq* stuff. 3) Look at drm_drv to see code snippet how to use the drmm* infrastructure. It will save you some cleanup and in general make the driver more stable 4) Sort includes alphabetically, and split them on in blocks. is one block is next block 5) Put entry in makefile in alphabetical order 6) You most like can use the simple_encoder stuff we have today 7) The *_load and *_unlod names where used in the past. Maybe be inspired by some newer driver here. _load functiosn is something used by legacy drivers so it confuses me a little. I look forward to see next revision of the patch-set. And sorry for not providing these high-level feedback issues before - I have not had time to look at your driver. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
On Fri, Jul 23, 2021 at 10:57:56AM +0200, Daniel Vetter wrote: > On Fri, Jul 23, 2021 at 11:12:49AM +0800, lichenyang wrote: > > From: Chenyang Li > > > > This patch adds an initial DRM driver for the Loongson LS7A1000 > > bridge chip(LS7A). The LS7A bridge chip contains two display > > controllers, support dual display output. The maximum support for > > each channel display is to 1920x1080@60Hz. > > At present, DC device detection and DRM driver registration are > > completed, the crtc/plane/encoder/connector objects has been > > implemented. > > On Loongson 3A4000 CPU and 7A1000 system, we have achieved the use > > of dual screen, and support dual screen clone mode and expansion > > mode. > > > > v9: > > - Optimize the error handling process. > > - Remove the useless flags parameter. > > - Fix some incorrect use of variables and constructs. > > ... > > Somehow this simple driver is at v9 and still not landed. Do you have > someone from the drm maintainers/committers who's guiding you through all > this? I'm not seeing you Cc: a specific person, without that there's good > chances your contribution gets lost. I'm swamped myself, which is why I've > ignored this and hope you'd fine someone else and stick to them. Hi Chenyang, Please cc: me on the next revision - then I will take a look. But I count on someone more familiar with atomic modesetting can also take a look. Thomas? Maxime? Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
Hi James. > > > If none of the 140 patches here fix a real bug, and there is no > > > change to machine code then it sounds to me like a W=2 kind of a > > > warning. > > > > FWIW, this series has found at least one bug so far: > > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/ > > > Well, it's a problem in an error leg, sure, but it's not a really > compelling reason for a 141 patch series, is it? All that fixing this > error will do is get the driver to print "oh dear there's a problem" > under four more conditions than it previously did. You are asking the wrong question here. Yuo should ask how many hours could have been saved by all the bugs people have been fighting with and then fixed *before* the code hit the kernel at all. My personal experience is that I, more than once, have had errors related to a missing break in my code. So this warnings is IMO a win. And if we are only ~100 patches to have it globally enabled then it is a no-brainer in my book. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v17 0/2] Add initial support for slimport anx7625
Hi Xin Ji On Fri, Sep 18, 2020 at 06:18:19PM +0800, Xin Ji wrote: > Hi all, > > The following series add support for the Slimport ANX7625 transmitter, a > ultra-low power Full-HD 4K MIPI to DP transmitter designed for portable > device. Thanks for all the iterations on this series. Driver looks good and I have applied it to drm-misc-next. Expect it to appear in -next in a few weeks. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
Laurent and I discussed this driver a little on irc.
Some highlights:
This parts could use register names:
+ writel(0x2, noc_dss_base + 0xc);
+ writel(0x2, noc_dss_base + 0x8c);
+ writel(0x2, noc_dss_base + 0x10c);
+ writel(0x2, noc_dss_base + 0x18c);
The two nodes in the DT for DPE and DSI uses overlapping range for reg
entries. It looks like a syscon node or some iommu thing is needed to do
this properly.
The chain will lok like this:
DPE -> DSI -> video mux -> {adv7533, panel}
But drm_bridge has not yet support for such non-linear setup.
The recommendation is to focus on the HDMI prat. Then we can later
come up with support for a video mux.
The video mux should have a dedicated node with one input node and two
output nodes. Which is also where the gpio should be.
The DSI node references two DPHY instances - should it be PHY driver(s)?
Does the DSI part contain one or two instances. Clocks looks duplicated.
Does the DPE and DSI share a lot of register blocks - or does it just
look like this from a first point of view?
You can read though the logs here:
https://people.freedesktop.org/~cbrill/dri-log/index.php
Could you please try to get back on some of the points above so we can
help you move forward in the right direction.
Thanks,
Sam
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro
> Before posting the big patch series again, let me send the new
> version folded into a single patch.
>
> If you'd like to see the entire thing, I'm placing it here:
>
>
> https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/hikey970_v2/
Review 3/3
For next submission then to ease review it would be nice that the patch
is spilt up a little. Maybe something like:
- HW specific stuff in two patches (header fiels with register + setup
code)
- Display driver files
- DSI driver files
- build stuff (Makefile, Kconfig fragments)
So all splits on file level - which should be easy to do.
This will make it easier for more people to give feedback. It is a bit
much to walk through 4k loc or what the full size is.
And then we can also provide a-b or r-b for fragments so the reviewed
parts can be ignored for later reviews.
For the DSI parts I may be hit by the old "when you have a hammer then
everything looks like a nail syndrome".
In my case the hammer is the bridge interface.
Suggestions:
- Move encoder to display driver
- Move connector creation to display driver (using
drm_bridge_connector_init())
- Use drm_bridge interface for the DSI driver parts
- Maybe use the HDMI bridge driver as a chained driver - I did not look
to close on this
- Use the standard bridge interface to find the bridge and drop use of
the component framework
I hope that some other drm people chime in here.
Either to tell this is non-sense or confirm this is the way to go.
The above does not give any hint how to use the gpio_mux to shift
between the panel and HDMI. This logic needs to be in the display driver
as the bridge driver will only be used for HDMI - as I understand the
code. And I do not know how to do it.
Some details in the following that are not related to the above.
Sam
> diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_dw_drm_dsi.c
> b/drivers/staging/hikey9xx/gpu/kirin9xx_dw_drm_dsi.c
> new file mode 100644
> index ..a2eed961b7d5
> --- /dev/null
> +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_dw_drm_dsi.c
> @@ -0,0 +1,2126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DesignWare MIPI DSI Host Controller v1.02 driver
> + *
> + * Copyright (c) 2016 Linaro Limited.
> + * Copyright (c) 2014-2016 Hisilicon Limited.
> + * Copyright (c) 2014-2020, Huawei Technologies Co., Ltd
> + *
> + * Author:
> + *
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Sort
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Sort
> +
> +#include "kirin9xx_dw_dsi_reg.h"
> +#include "kirin9xx_dpe.h"
> +#include "kirin9xx_drm_drv.h"
> +#include "kirin9xx_drm_dpe_utils.h"
Sort
> +
> +#define PHY_REF_CLK_RATE 1920
> +#define PHY_REF_CLK_PERIOD_PS(10 / (PHY_REF_CLK_RATE / 1000))
> +
> +#define encoder_to_dsi(encoder) \
> + container_of(encoder, struct dw_dsi, encoder)
> +#define host_to_dsi(host) \
> + container_of(host, struct dw_dsi, host)
> +#define connector_to_dsi(connector) \
> + container_of(connector, struct dw_dsi, connector)
Move the upcast next to the struct definition.
> +#define DSS_REDUCE(x)((x) > 0 ? ((x) - 1) : (x))
> +
> +enum dsi_output_client {
> + OUT_HDMI = 0,
> + OUT_PANEL,
> + OUT_MAX
> +};
> +
> +struct mipi_phy_params {
I did not check all fiels but I managed to find at least one unused
field. Cheack and delete what is unused.
> + u64 lane_byte_clk;
> + u32 clk_division;
> +
> + u32 clk_lane_lp2hs_time;
> + u32 clk_lane_hs2lp_time;
> + u32 data_lane_lp2hs_time;
> + u32 data_lane_hs2lp_time;
> + u32 clk2data_delay;
> + u32 data2clk_delay;
> +
> + u32 clk_pre_delay;
> + u32 clk_post_delay;
> + u32 clk_t_lpx;
> + u32 clk_t_hs_prepare;
> + u32 clk_t_hs_zero;
> + u32 clk_t_hs_trial;
> + u32 clk_t_wakeup;
> + u32 data_pre_delay;
> + u32 data_post_delay;
> + u32 data_t_lpx;
> + u32 data_t_hs_prepare;
> + u32 data_t_hs_zero;
> + u32 data_t_hs_trial;
> + u32 data_t_ta_go;
> + u32 data_t_ta_get;
> + u32 data_t_wakeup;
> +
> + u32 phy_stop_wait_time;
> +
> + u32 rg_vrefsel_vcm;
> + u32 rg_hstx_ckg_sel;
> + u32 rg_pll_fbd_div5f;
> + u32 rg_pll_fbd_div1f;
> + u32 rg_pll_fbd_2p;
> + u32 rg_pll_enbwt;
> + u32 rg_pll_fbd_p;
> + u32 rg_pll_fbd_s;
> + u32 rg_pll_pre_div1p;
> + u32 rg_pll_pre_p;
> + u32 rg_pll_vco_750m;
> + u32 rg_pll_lpf_rs;
> + u32 rg_pll_lpf_cs;
> + u32 rg_pll_enswc;
> + u32 rg_pll_chp;
> +
Some indent gone wrong here?
> + u32 pll_register_override; /* 0x1E[0] */
> + u32 pll_power_down; /* 0x1E[1] */
> + u32 rg_band_sel;/* 0x1E[2] */
> + u32 rg_phase_gen_en;/* 0x1E[3] */
> + u32 reload_sel;
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro
> Before posting the big patch series again, let me send the new
> version folded into a single patch.
Review 2/N
The way output_poll_changed is used to set gpio_mux to select between
the panel and the HDMI looks strange. But I do not know if there is a
more correct way to do it. Other DRM people would need to help
here if there is a better way to do it.
I looked briefly af suspend/resume.
I think this would benefit from using drm_mode_config_helper_suspend()
and drm_mode_config_helper_resume() but I did not finalize the anlyzis.
Other than this only some small details in the following.
Sam
> kirin9xx_drm_drv.c b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> new file mode 100644
> index ..61b65f8b1ace
> --- /dev/null
> +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> @@ -0,0 +1,277 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Hisilicon Kirin SoCs drm master driver
> + *
> + * Copyright (c) 2016 Linaro Limited.
> + * Copyright (c) 2014-2016 Hisilicon Limited.
> + * Copyright (c) 2014-2020, Huawei Technologies Co., Ltd
> + * Author:
> + *
> + *
> + */
> +
> +#include
> +#include
> +#include
Sort includes
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Sort includes
> +
> +#include "kirin9xx_dpe.h"
> +#include "kirin9xx_drm_drv.h"
> +
> +static int kirin_drm_kms_cleanup(struct drm_device *dev)
> +{
> + struct kirin_drm_private *priv = to_drm_private(dev);
> +
> + if (priv->fbdev)
> + priv->fbdev = NULL;
> +
> + drm_kms_helper_poll_fini(dev);
> + kirin9xx_dss_drm_cleanup(dev);
> +
> + return 0;
> +}
> +
> +static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
> +{
> + struct kirin_drm_private *priv = to_drm_private(dev);
> +
> + dsi_set_output_client(dev);
> +
> + drm_fb_helper_hotplug_event(priv->fbdev);
> +}
> +
> +static const struct drm_mode_config_funcs kirin_drm_mode_config_funcs = {
> + .fb_create = drm_gem_fb_create,
> + .output_poll_changed = kirin_fbdev_output_poll_changed,
> + .atomic_check = drm_atomic_helper_check,
> + .atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static int kirin_drm_kms_init(struct drm_device *dev)
> +{
> + long kirin_type;
> + int ret;
> +
> + dev_set_drvdata(dev->dev, dev);
> +
> + ret = drmm_mode_config_init(dev);
> + if (ret)
> + return ret;
> +
> + dev->mode_config.min_width = 0;
> + dev->mode_config.min_height = 0;
> + dev->mode_config.max_width = 2048;
> + dev->mode_config.max_height = 2048;
> + dev->mode_config.preferred_depth = 32;
> +
> + dev->mode_config.funcs = _drm_mode_config_funcs;
> +
> + /* display controller init */
> + kirin_type = (long)of_device_get_match_data(dev->dev);
> + if (WARN_ON(!kirin_type))
> + return -ENODEV;
> +
> + ret = dss_drm_init(dev, kirin_type);
> + if (ret)
> + return ret;
> +
> + /* bind and init sub drivers */
> + ret = component_bind_all(dev->dev, dev);
> + if (ret) {
> + drm_err(dev, "failed to bind all component.\n");
> + return ret;
> + }
> +
> + /* vblank init */
> + ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
> + if (ret) {
> + drm_err(dev, "failed to initialize vblank.\n");
> + return ret;
> + }
> + /* with irq_enabled = true, we can use the vblank feature. */
> + dev->irq_enabled = true;
> +
> + /* reset all the states of crtc/plane/encoder/connector */
> + drm_mode_config_reset(dev);
> +
> + /* init kms poll for handling hpd */
> + drm_kms_helper_poll_init(dev);
> +
> + return 0;
> +}
> +
> +DEFINE_DRM_GEM_CMA_FOPS(kirin_drm_fops);
Move it to right above kirin_drm_driver where it is used
> +
> +static int kirin_drm_connectors_register(struct drm_device *dev)
> +{
> + struct drm_connector_list_iter conn_iter;
> + struct drm_connector *failed_connector;
> + struct drm_connector *connector;
> + int ret;
> +
> + mutex_lock(>mode_config.mutex);
> + drm_connector_list_iter_begin(dev, _iter);
> + drm_for_each_connector_iter(connector, _iter) {
> + ret = drm_connector_register(connector);
> + if (ret) {
> + failed_connector = connector;
> + goto err;
> + }
> + }
> + mutex_unlock(>mode_config.mutex);
> +
> + return 0;
> +
> +err:
> + drm_connector_list_iter_begin(dev, _iter);
> + drm_for_each_connector_iter(connector, _iter) {
> + if (failed_connector == connector)
> + break;
> + drm_connector_unregister(connector);
> + }
> + mutex_unlock(>mode_config.mutex);
> +
> + return ret;
> +}
> +
> +static struct drm_driver kirin_drm_driver = {
> + .driver_features= DRIVER_GEM | DRIVER_MODESET
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro > kirin9xx_fb_panel.h b/drivers/staging/hikey9xx/gpu/kirin9xx_fb_panel.h > new file mode 100644 > index ..a69c20470f1d > --- /dev/null > +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_fb_panel.h This file is not referenced and should be deleted. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
> Before posting the big patch series again, let me send the new
> version folded into a single patch.
Review 1/N
Lots of small details I missed last time.
A good thing is that there is an opportunity to delete som more code.
Sam
> diff --git a/drivers/staging/hikey9xx/gpu/Kconfig
> b/drivers/staging/hikey9xx/gpu/Kconfig
> new file mode 100644
> index ..8578ca953785
> --- /dev/null
> +++ b/drivers/staging/hikey9xx/gpu/Kconfig
> @@ -0,0 +1,10 @@
> +config DRM_HISI_KIRIN9XX
> + tristate "DRM Support for Hisilicon Kirin9xx series SoCs Platform"
> + depends on DRM && OF && ARM64
> + select DRM_KMS_HELPER
> + select DRM_GEM_CMA_HELPER
> + select DRM_KMS_CMA_HELPER
> + select DRM_MIPI_DSI
> + help
> + Choose this option if you have a HiSilicon Kirin960 or Kirin970.
> + If M is selected the module will be called kirin9xx-drm.
> diff --git a/drivers/staging/hikey9xx/gpu/Makefile
> b/drivers/staging/hikey9xx/gpu/Makefile
> new file mode 100644
> index ..5f7974a95367
> --- /dev/null
> +++ b/drivers/staging/hikey9xx/gpu/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +kirin9xx-drm-y := kirin9xx_drm_drv.o \
> + kirin9xx_drm_dss.o \
> + kirin9xx_drm_dpe_utils.o \
> + kirin970_defs.o kirin960_defs.o \
> + kirin9xx_drm_overlay_utils.o
> +
> +obj-$(CONFIG_DRM_HISI_KIRIN9XX) += kirin9xx-drm.o kirin9xx_dw_drm_dsi.o
General comment which is true for many many Makefile's
I have never understood the love of '\'.
Something like this works equally well:
kirin9xx-drm-y := kirin9xx_drm_drv.o kirin9xx_drm_dss.o
kirin9xx-drm-y += kirin9xx_drm_dpe_utils.o kirin970_defs.o
kirin9xx-drm-y += kirin960_defs.o kirin9xx_drm_overlay_utils.o
obj-$(CONFIG_DRM_HISI_KIRIN9XX) += kirin9xx-drm.o kirin9xx_dw_drm_dsi.o
> diff --git a/drivers/staging/hikey9xx/gpu/kirin960_defs.c
> b/drivers/staging/hikey9xx/gpu/kirin960_defs.c
> new file mode 100644
> index ..c5e1ec03c818
> --- /dev/null
> +++ b/drivers/staging/hikey9xx/gpu/kirin960_defs.c
> @@ -0,0 +1,346 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2008-2011, Hisilicon Tech. Co., Ltd. All rights reserved.
> + * Copyright (c) 2008-2020, Huawei Technologies Co., Ltd
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "kirin9xx_drm_dpe_utils.h"
> +#include "kirin9xx_drm_drv.h"
> +#include "kirin960_dpe_reg.h"
All includes blocks should be sorted.
The list of include files looks far too large for this simple file.
Reduce to the relevant include files.
> +
> +static const u32
> kirin960_g_dss_module_base[DSS_CHN_MAX_DEFINE][MODULE_CHN_MAX] = {
> + {
> + /* D0 */
> + MIF_CH0_OFFSET,
> + AIF0_CH0_OFFSET,
> + AIF1_CH0_OFFSET,
> + MCTL_CTL_MUTEX_RCH0,
> + DSS_MCTRL_SYS_OFFSET + MCTL_RCH0_FLUSH_EN,
> + DSS_MCTRL_SYS_OFFSET + MCTL_RCH0_OV_OEN,
> + DSS_MCTRL_SYS_OFFSET + MCTL_RCH0_STARTY,
> + DSS_MCTRL_SYS_OFFSET + MCTL_MOD0_DBG,
> + DSS_RCH_D0_DMA_OFFSET,
> + DSS_RCH_D0_DFC_OFFSET,
> + 0,
> + 0,
> + 0,
> + 0,
> + 0,
> + 0,
> + DSS_RCH_D0_CSC_OFFSET,
> + }, {
...
> + },
> +};
> +
> +static const u32
> kirin960_g_dss_module_ovl_base[DSS_MCTL_IDX_MAX][MODULE_OVL_MAX] = {
> + {
> + DSS_OVL0_OFFSET,
> + DSS_MCTRL_CTL0_OFFSET
> + }, {
> + DSS_OVL1_OFFSET,
> + DSS_MCTRL_CTL1_OFFSET
> + }, {
> + DSS_OVL2_OFFSET,
> + DSS_MCTRL_CTL2_OFFSET,
> + }, {
> + DSS_OVL3_OFFSET,
> + DSS_MCTRL_CTL3_OFFSET,
> + }, {
> + 0,
> + DSS_MCTRL_CTL4_OFFSET,
> + }, {
> + 0,
> + DSS_MCTRL_CTL5_OFFSET,
> + }
> +};
> +
> +/* SCF_LUT_CHN coef_idx */
> +static const int kirin960_g_scf_lut_chn_coef_idx[DSS_CHN_MAX_DEFINE] = {
> + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1
> +};
> +
> +static const u32
> kirin960_g_dss_module_cap[DSS_CHN_MAX_DEFINE][MODULE_CAP_MAX] = {
> + /* D2 */
> + {0, 0, 1, 0, 0, 0, 1, 0, 0, 0, 1},
> + /* D3 */
> + {0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1},
> + /* V0 */
> + {0, 1, 1, 0, 1, 1, 1, 0, 0, 1, 1},
> + /* G0 */
> + {0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0},
> + /* V1 */
> + {0, 1, 1, 1, 0, 1, 1, 0, 1, 1, 1},
> + /* G1 */
> + {0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0},
> + /* D0 */
> + {0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1},
> + /* D1 */
> + {0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1},
> +
> + /* W0 */
> + {1, 0, 1, 0, 0, 0, 0, 1, 0, 1, 1},
> + /* W1 */
> + {1, 0, 1, 0, 0, 0, 0, 1, 0, 1, 1},
> +
> + /* V2 */
> + {0, 1, 1, 1, 0, 1, 1, 0, 1, 1, 1},
> + /* W2 */
>
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro. On Fri, Aug 21, 2020 at 04:41:58PM +0200, Mauro Carvalho Chehab wrote: > Another quick question: > > Em Wed, 19 Aug 2020 19:35:58 +0200 > Sam Ravnborg escreveu: > > > > +#define DSS_REDUCE(x)((x) > 0 ? ((x) - 1) : (x)) > > Use generic macros for this? > > Do you know a generic macro similar to this? Or do you mean adding > it to include/kernel.h? It looked like something there should be a macro for. But I do not know one. And no, do not try to go the kernel.h route on this. At least not until you see more than one user. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
Thanks for the detailed feedabck.
Two comments in the following.
Sam
>
> > > + ctx->dss_pri_clk = devm_clk_get(dev, "clk_edc0");
> > > + if (!ctx->dss_pri_clk) {
> > > + DRM_ERROR("failed to parse dss_pri_clk\n");
> > > + return -ENODEV;
> > > + }
> ...
>
> > I had expected some of these could fail with a PROBE_DEFER.
> > Consider to use the newly introduced dev_probe_err()
>
> Yeah, getting clock lines can fail. I was unable to find dev_probe_err(),
> at least on Kernel 5.9-rc1. I saw this comment:
>
> https://lkml.org/lkml/2020/3/6/356
>
> It sounds it didn't reach upstream. Anyway, I add error handling for the
> the clk_get calls:
>
> ctx->dss_pri_clk = devm_clk_get(dev, "clk_edc0");
> ret = PTR_ERR_OR_ZERO(ctx->dss_pri_clk);
> if (ret == -EPROBE_DEFER) {
> return ret;
> } else if (ret) {
> DRM_ERROR("failed to parse dss_pri_clk: %d\n", ret);
> return ret;
> }
>
> This should be able to detect deferred probe, plus to warn
> about other errors.
I got the name wrong. It is named dev_err_probe(), and was introduced in -rc1.
> > Can the panel stuff be moved out and utilise drm_panel?
>
> I saw the code at drm_panel. The real issue here is that I can't
> test anything related to panel support, as I lack any hardware
> for testing. So, there's a high chance I may end breaking
> something while trying to do that.
I will try to take a look again when you post next revision.
Maybe we should update it and risk that is not works, so whenever
someone try to fix it they do so on top of an up-to-date implmentation.
Lets se and decide later.
Sam
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
Quick feedback below.
Sam
On Thu, Aug 20, 2020 at 05:13:22PM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 20 Aug 2020 16:48:08 +0200
> Sam Ravnborg escreveu:
>
> > Hi Mauro.
> >
> > On Thu, Aug 20, 2020 at 04:06:49PM +0200, Mauro Carvalho Chehab wrote:
> > > Em Wed, 19 Aug 2020 19:35:58 +0200
> > > Sam Ravnborg escreveu:
> > >
> > > I'm already handling the other comments from your review (I'll send a
> > > more complete comment about them after finishing),
> > If you get back only on things you do not understand or do not agree on
> > that would be fine. The rest should be visible in the changelog on the
> > updated patch - no need to do extra work here.
> >
> > > but I have a doubt what you meant about this:
> > >
> > > > +static int kirin_drm_bind(struct device *dev)
> > > > > +{
> > > > > + struct drm_driver *driver = _drm_driver;
> > > > > + struct drm_device *drm_dev;
> > > > > + struct kirin_drm_private *priv;
> > > > > + int ret;
> > > > > +
> > > > > + drm_dev = drm_dev_alloc(driver, dev);
> > > > > + if (!drm_dev)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + ret = kirin_drm_kms_init(drm_dev);
> > > > > + if (ret)
> > > > > + goto err_drm_dev_unref;
> > > > > +
> > > > > + ret = drm_dev_register(drm_dev, 0);
> > > > There is better ways to do this. See drm_drv.c for the code example.
> > >
> > > Not sure if I understood your comment here. The drm_drv.c example also
> > > calls
> > > drm_dev_register().
> >
> > This is indeed not obvious from my comments but what I wnated to say is
> > that the driver should embed drm_device in some struct,
> > maybe in "struct kirin_drm_private".
> >
> > This should also be part of the referenced example.
> >
> > I hope this clarifies it.
>
> Yeah. I was already doing those changes ;-)
>
> Something like the enclosed patch, right?
>
> Btw, I'm not sure if the error handling part is ok, as I didn't check
> what the devm stuff does at the subsystem.
>
> -
>
> On a separate question, I was unable to use the helper macros,
> as it sounds that there's no macro with this:
>
> .dumb_create= drm_gem_cma_dumb_create_internal,
>
> The existing DRM_GEM_CMA_VMAP_DRIVER_OPS uses, instead
> drm_gem_cma_dumb_create(). I'm not sure if this driver can use
> such function instead.
>From the documentation of drm_gem_cma_dumb_create_internal:
* It should not be used directly
* as their _driver.dumb_create callback.
I would expect drm_gem_cma_dumb_create() to be the right choice.
So you can go ahead with DRM_GEM_CMA_VMAP_DRIVER_OPS
(I hope I am right, not the are i know much about)
> staging: hikey9xx/gpu: use drm_managed interface
>
> Use a more modern design for the driver binding logic by
> using drm_managed and getting rid of drm->dev_private.
>
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> index c7736f4d74b7..600c89605cc0 100644
> --- a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> @@ -29,12 +29,13 @@
> #include
> #include
> #include
> +#include
>
> #include "kirin9xx_drm_drv.h"
>
> static int kirin_drm_kms_cleanup(struct drm_device *dev)
> {
> - struct kirin_drm_private *priv = dev->dev_private;
> + struct kirin_drm_private *priv = to_drm_private(dev);
> static struct kirin_dc_ops const *dc_ops;
>
> if (priv->fbdev)
> @@ -45,15 +46,13 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev)
> drm_kms_helper_poll_fini(dev);
> dc_ops->cleanup(dev);
> drm_mode_config_cleanup(dev);
This should also be gone when you are using
drmm_mode_config_init()
> - devm_kfree(dev->dev, priv);
> - dev->dev_private = NULL;
>
> return 0;
> }
>
> static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
> {
> - struct kirin_drm_private *priv = dev->dev_private;
> + struct kirin_drm_private *priv = to_drm_private(dev);
>
> dsi_set_output_client(dev);
>
> @@ -69,18 +68,20 @@ static const struct drm_mode_config_funcs
> kirin_drm_mode_config_funcs = {
>
> static int kirin_drm_kms_init(stru
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
On Thu, Aug 20, 2020 at 04:06:49PM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 19 Aug 2020 19:35:58 +0200
> Sam Ravnborg escreveu:
>
> I'm already handling the other comments from your review (I'll send a
> more complete comment about them after finishing),
If you get back only on things you do not understand or do not agree on
that would be fine. The rest should be visible in the changelog on the
updated patch - no need to do extra work here.
> but I have a doubt what you meant about this:
>
> > +static int kirin_drm_bind(struct device *dev)
> > > +{
> > > + struct drm_driver *driver = _drm_driver;
> > > + struct drm_device *drm_dev;
> > > + struct kirin_drm_private *priv;
> > > + int ret;
> > > +
> > > + drm_dev = drm_dev_alloc(driver, dev);
> > > + if (!drm_dev)
> > > + return -ENOMEM;
> > > +
> > > + ret = kirin_drm_kms_init(drm_dev);
> > > + if (ret)
> > > + goto err_drm_dev_unref;
> > > +
> > > + ret = drm_dev_register(drm_dev, 0);
> > There is better ways to do this. See drm_drv.c for the code example.
>
> Not sure if I understood your comment here. The drm_drv.c example also calls
> drm_dev_register().
This is indeed not obvious from my comments but what I wnated to say is
that the driver should embed drm_device in some struct,
maybe in "struct kirin_drm_private".
This should also be part of the referenced example.
I hope this clarifies it.
Sam
>
> The only difference is that it calls it inside driver_probe(), while
> on this driver, it uses:
>
> static const struct component_master_ops kirin_drm_ops = {
> .bind = kirin_drm_bind,
> .unbind = kirin_drm_unbind,
> };
>
> static int kirin_drm_platform_probe(struct platform_device *pdev)
> {
> ...
> drm_of_component_match_add(dev, , compare_of, remote);
> ...
> return component_master_add_with_match(dev, _drm_ops, match);
> }
>
> Are you meaning that I should get rid of this component API and call
> kirin_drm_bind() from kirin_drm_platform_probe()?
>
> Thanks,
> Mauro
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi John. > > So, IMO, the best is to keep it on staging for a while, until those > > remaining bugs gets solved. > > I'm not sure I see all of these as compelling for pushing it in via > staging. And I suspect in the process of submitting the patches for > review folks may find the cause of some of the problems you list here. There is a tendency to forget drivers in staging, and with the almost constant refactoring that happens in the drm drivers we would end up fixing this driver when a bot trigger an error. So IMO we need very good reasons to go in via staging. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 0/2] Add initial support for slimport anx7625
Hi Xin Ji. On Mon, Aug 10, 2020 at 10:35:46PM +0200, Sam Ravnborg wrote: > Hi Xin Ji. > > On Thu, Jul 09, 2020 at 04:31:09PM +0800, Xin Ji wrote: > > Hi all, > > > > The following series add support for the Slimport ANX7625 transmitter, a > > ultra-low power Full-HD 4K MIPI to DP transmitter designed for portable > > device. > > > > > > This is the v14 version, any mistakes, please let me know, I will fix it in > > the next series. > > > > Change history: > > v14: Fix comments from Sam and Nicolas > > - Check flags at drm_bridge_attach > > - Use panel_bridge instead of drm_panel > > - Fix not correct return value > > Sorry for ignoring this for so long time. > The patch applies but no longer builds. > > I could fix it locally but wanted to know if you have a later version to > be applied? I took a short look at the driver today. I noticed following code: if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) return -EINVAL; So if the display driver do not supply the DRM_BRIDGE_ATTACH_NO_CONNECTOR then -EINVAL is returned. But then the anx7625_bridge_attach() continues and creates a connector. For a new bridge driver there should be no need for the backward compatibility - so no need to create the connector. Unless the display driver needs it - but then we should fix the display driver and not add backward compatibility code in the bridge driver. Which display driver do you expect this bridge driver to be used with? Sam > > Sam > > > > > > v13: Fix comments from Launrent Pinchart and Rob Herring > > - Picked up Rob's Reviewed-By > > - Add .detect and .get_edid interface in bridge funcs. > > > > v12: Fix comments from Hsin-Yi Wang > > - Rebase the code on kernel 5.7, fix DRM interface not match issue. > > > > v11: Fix comments from Rob Herring > > - Update commit message. > > - Remove unused label. > > > > v10: Fix comments from Rob Herring, Daniel. > > - Fix dt_binding_check warning. > > - Update description. > > > > v9: Fix comments from Sam, Nicolas, Daniel > > - Remove extcon interface. > > - Remove DPI support. > > - Fix dt_binding_check complains. > > - Code clean up and update description. > > > > v8: Fix comments from Nicolas. > > - Fix several coding format. > > - Update description. > > > > v7: > > - Fix critical timing(eg:odd hfp/hbp) in "mode_fixup" interface, > >enhance MIPI RX tolerance by setting register MIPI_DIGITAL_ADJ_1 to 0x3D. > > > > > > Xin Ji (2): > > dt-bindings: drm/bridge: anx7625: MIPI to DP transmitter DT schema > > drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP > > > > .../bindings/display/bridge/analogix,anx7625.yaml | 95 + > > drivers/gpu/drm/bridge/analogix/Kconfig|9 + > > drivers/gpu/drm/bridge/analogix/Makefile |1 + > > drivers/gpu/drm/bridge/analogix/anx7625.c | 1939 > > > > drivers/gpu/drm/bridge/analogix/anx7625.h | 391 > > 5 files changed, 2435 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c > > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h > > > > -- > > 2.7.4 > > > > ___ > > dri-devel mailing list > > dri-de...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro. On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab wrote: > This patch series port the out-of-tree driver for Hikey 970 (which > should also support Hikey 960) from the official 96boards tree: > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > Based on his history, this driver seems to be originally written > for Kernel 4.4, and was later ported to Kernel 4.9. The original > driver used to depend on ION (from Kernel 4.4) and had its own > implementation for FB dev API. > > As I need to preserve the original history (with has patches from > both HiSilicon and from Linaro), I'm starting from the original > patch applied there. The remaining patches are incremental, > and port this driver to work with upstream Kernel. > > This driver doesn't depend on any firmware or on any special > userspace code. It works as-is with both X11 and Wayland. > > Yet, I'm submitting it via staging due to the following reasons: > > - It depends on the LDO3 power supply, which is provided by > a regulator driver that it is currently on staging; > - Due to legal reasons, I need to preserve the authorship of > each one responsbile for each patch. So, I need to start from > the original patch from Kernel 4.4; > - There are still some problems I need to figure out how to solve: >- The adv7535 can't get EDID data. Maybe it is a timing issue, > but it requires more research to be sure about how to solve it; >- The driver only accept resolutions on a defined list, as there's > a known bug that this driver may have troubles with random > resolutions. Probably due to a bug at the pixel clock settings; >- Sometimes (at least with 1080p), it generates LDI underflow > errors, which in turn causes the DRM to stop working. That > happens for example when using gdm on Wayland and > gnome on X11; >- Probably related to the previous issue, when the monitor > suspends due to DPMS, it doesn't return back to life. > > So, IMO, the best is to keep it on staging for a while, until those > remaining bugs gets solved. > > I added this series, together with the regulator driver and > a few other patches (including a hack to fix a Kernel 5.8 > regression at WiFi ) at: > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > > Chen Feng (1): > staging: hikey9xx: Add hisilicon DRM driver for hikey960/970 > > John Stultz (1): > staging: hikey9xx/gpu: port it to work with Kernel v4.9 > > Liwei Cai (2): > staging: hikey9xx/gpu: solve tearing issue of display > staging: hikey9xx/gpu: resolve the performance issue by interrupt > mechanism > > Mauro Carvalho Chehab (38): > staging: hikey9xx/gpu: get rid of adv7535 fork Very good - I was in my mind starting a rant why we needed a fork of this driver, but I see it gets deleted again. I do acknowledge you need to preserve history and all - but this patchset is not easy to review. Could you follow-up with a review-able set of patches as a follow-up for this? I spotted some wrong bridge handling in one patch but I do not know if this got changed in a later patch. And I lost the motivation to go looking for it. > staging: hikey9xx/gpu: rename the Kirin9xx namespace > staging: hikey9xx/gpu: get rid of kirin9xx_fbdev.c > staging: hikey9xx/gpu: get rid of some ifdefs > staging: hikey9xx/gpu: rename the config option for Kirin970 > staging: hikey9xx/gpu: change the includes to reflect upstream > staging: hikey9xx/gpu: port driver to upstream kAPIs > staging: hikey9xx/gpu: add a copy of set_reg() function there > staging: hikey9xx/gpu: get rid of ION headers > staging: hikey9xx/gpu: add support for using a reserved CMA memory > staging: hikey9xx/gpu: cleanup encoder attach logic > staging: hikey9xx/gpu: Change the logic which sets the burst mode > staging: hikey9xx/gpu: fix the DRM setting logic > staging: hikey9xx/gpu: do some code cleanups > staging: hikey9xx/gpu: use default GEM_CMA fops > staging: hikey9xx/gpu: place vblank enable/disable at the right place > staging: hikey9xx/gpu: remove an uneeded hack > staging: hikey9xx/gpu: add a possible implementation for > atomic_disable > staging: hikey9xx/gpu: register connector > staging: hikey9xx/gpu: fix driver name > staging: hikey9xx/gpu: get rid of iommu_format > staging: hikey9xx/gpu: re-work the mode validation code > staging: hikey9xx/gpu: add support for enable/disable ldo3 regulator > staging: hikey9xx/gpu: add SPMI headers > staging: hikey9xx/gpu: solve most coding style issues > staging: hikey9xx/gpu: don't use iommu code > staging: hikey9xx/gpu: add kirin9xx driver to the building system > staging: hikey9xx/gpu: get rid of typedefs > staging: hikey9xx/gpu: get rid of input/output macros > staging: hikey9xx/gpu: get rid of some unused data > staging: hikey9xx/gpu: place common definitions at kirin9xx_dpe.h > staging: hikey9xx/gpu: get
Re: [PATCH v14 0/2] Add initial support for slimport anx7625
Hi Xin Ji. On Thu, Jul 09, 2020 at 04:31:09PM +0800, Xin Ji wrote: > Hi all, > > The following series add support for the Slimport ANX7625 transmitter, a > ultra-low power Full-HD 4K MIPI to DP transmitter designed for portable > device. > > > This is the v14 version, any mistakes, please let me know, I will fix it in > the next series. > > Change history: > v14: Fix comments from Sam and Nicolas > - Check flags at drm_bridge_attach > - Use panel_bridge instead of drm_panel > - Fix not correct return value Sorry for ignoring this for so long time. The patch applies but no longer builds. I could fix it locally but wanted to know if you have a later version to be applied? Sam > > v13: Fix comments from Launrent Pinchart and Rob Herring > - Picked up Rob's Reviewed-By > - Add .detect and .get_edid interface in bridge funcs. > > v12: Fix comments from Hsin-Yi Wang > - Rebase the code on kernel 5.7, fix DRM interface not match issue. > > v11: Fix comments from Rob Herring > - Update commit message. > - Remove unused label. > > v10: Fix comments from Rob Herring, Daniel. > - Fix dt_binding_check warning. > - Update description. > > v9: Fix comments from Sam, Nicolas, Daniel > - Remove extcon interface. > - Remove DPI support. > - Fix dt_binding_check complains. > - Code clean up and update description. > > v8: Fix comments from Nicolas. > - Fix several coding format. > - Update description. > > v7: > - Fix critical timing(eg:odd hfp/hbp) in "mode_fixup" interface, >enhance MIPI RX tolerance by setting register MIPI_DIGITAL_ADJ_1 to 0x3D. > > > Xin Ji (2): > dt-bindings: drm/bridge: anx7625: MIPI to DP transmitter DT schema > drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP > > .../bindings/display/bridge/analogix,anx7625.yaml | 95 + > drivers/gpu/drm/bridge/analogix/Kconfig|9 + > drivers/gpu/drm/bridge/analogix/Makefile |1 + > drivers/gpu/drm/bridge/analogix/anx7625.c | 1939 > > drivers/gpu/drm/bridge/analogix/anx7625.h | 391 > 5 files changed, 2435 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h > > -- > 2.7.4 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v13 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP
Hi Xin.
On Tue, Jun 09, 2020 at 03:19:50PM +0800, Xin Ji wrote:
> The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed
> for portable device. It converts MIPI DSI/DPI to DisplayPort 1.3 4K.
>
> Signed-off-by: Xin Ji
The bridge driver now implements the bridge functions for get_edid +
detect which is good.
But the bridge driver also needs to check the flags argument
of anx7625_bridge_attach() so the connector creation is optional.
This is required to prepare the bridge driver to fully support a
chain of bridges and is mandatory for new bridge drivers.
Futhermore the bridge driver needs to be converted to use the bridge
panel. See what other bridge drivers do.
In short - the drm_panel shall be replaced by a bridge in your
platform data structure.
Usual this end up in less code after the conversion.
Sam
> ---
> drivers/gpu/drm/bridge/analogix/Kconfig |9 +
> drivers/gpu/drm/bridge/analogix/Makefile |1 +
> drivers/gpu/drm/bridge/analogix/anx7625.c | 1999
> +
> drivers/gpu/drm/bridge/analogix/anx7625.h | 397 ++
> 4 files changed, 2406 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c
> create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h
>
> diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig
> b/drivers/gpu/drm/bridge/analogix/Kconfig
> index e1fa7d8..024ea2a 100644
> --- a/drivers/gpu/drm/bridge/analogix/Kconfig
> +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
> @@ -25,3 +25,12 @@ config DRM_ANALOGIX_ANX78XX
> config DRM_ANALOGIX_DP
> tristate
> depends on DRM
> +
> +config DRM_ANALOGIX_ANX7625
> + tristate "Analogix Anx7625 MIPI to DP interface support"
> + depends on DRM
> + depends on OF
> + help
> + ANX7625 is an ultra-low power 4K mobile HD transmitter
> + designed for portable devices. It converts MIPI/DPI to
> + DisplayPort1.3 4K.
> diff --git a/drivers/gpu/drm/bridge/analogix/Makefile
> b/drivers/gpu/drm/bridge/analogix/Makefile
> index 97669b3..44da392 100644
> --- a/drivers/gpu/drm/bridge/analogix/Makefile
> +++ b/drivers/gpu/drm/bridge/analogix/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-only
> analogix_dp-objs := analogix_dp_core.o analogix_dp_reg.o analogix-i2c-dptx.o
> obj-$(CONFIG_DRM_ANALOGIX_ANX6345) += analogix-anx6345.o
> +obj-$(CONFIG_DRM_ANALOGIX_ANX7625) += anx7625.o
> obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp.o
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> new file mode 100644
> index 000..db37f68
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -0,0 +1,1999 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright(c) 2020, Analogix Semiconductor. All rights reserved.
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include "anx7625.h"
> +
> +/*
> + * There is a sync issue while access I2C register between AP(CPU) and
> + * internal firmware(OCM), to avoid the race condition, AP should access
> + * the reserved slave address before slave address occurs changes.
> + */
> +static int i2c_access_workaround(struct anx7625_data *ctx,
> + struct i2c_client *client)
> +{
> + u8 offset;
> + struct device *dev = >dev;
> + int ret;
> +
> + if (client == ctx->last_client)
> + return 0;
> +
> + ctx->last_client = client;
> +
> + if (client == ctx->i2c.tcpc_client)
> + offset = RSVD_00_ADDR;
> + else if (client == ctx->i2c.tx_p0_client)
> + offset = RSVD_D1_ADDR;
> + else if (client == ctx->i2c.tx_p1_client)
> + offset = RSVD_60_ADDR;
> + else if (client == ctx->i2c.rx_p0_client)
> + offset = RSVD_39_ADDR;
> + else if (client == ctx->i2c.rx_p1_client)
> + offset = RSVD_7F_ADDR;
> + else
> + offset = RSVD_00_ADDR;
> +
> + ret = i2c_smbus_write_byte_data(client, offset, 0x00);
> + if (ret < 0)
> + DRM_DEV_ERROR(dev,
> + "fail to access i2c id=%x\n:%x",
> + client->addr, offset);
> +
> + return ret;
> +}
> +
> +static int anx7625_reg_read(struct anx7625_data *ctx,
> + struct i2c_client *client, u8 reg_addr)
> +{
> + int ret;
> + struct device *dev = >dev;
> +
> + i2c_access_workaround(ctx, client);
> +
> + ret = i2c_smbus_read_byte_data(client, reg_addr);
> + if (ret < 0)
> + DRM_DEV_ERROR(dev, "read i2c fail id=%x:%x\n",
> + client->addr, reg_addr);
> +
> +
Re: [PATCH] staging: fbtft: fb_st7789v: make HSD20_IPS numeric and not a string
Hi Colin/Greg.
On Thu, May 21, 2020 at 02:50:38PM +0100, Colin King wrote:
> From: Colin Ian King
>
> Currently HSD20_IPS is defined as "true" and will always result in a
> non-zero result even if it is defined as "false" because it is an array
> and that will never be zero. Fix this by defining it as an integer 1
> rather than a literal string.
>
> Addessses-Coverity: ("Array compared against 0")
> Fixes: f03c9b788472 ("staging: fbtft: fb_st7789v: Initialize the Display")
> Signed-off-by: Colin Ian King
> ---
> drivers/staging/fbtft/fb_st7789v.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/fbtft/fb_st7789v.c
> b/drivers/staging/fbtft/fb_st7789v.c
> index ebc17e05ecd0..3a280cc1892c 100644
> --- a/drivers/staging/fbtft/fb_st7789v.c
> +++ b/drivers/staging/fbtft/fb_st7789v.c
> @@ -24,7 +24,7 @@
> "D0 05 0A 09 08 05 2E 44 45 0F 17 16 2B 33\n" \
> "D0 05 0A 09 08 05 2E 43 45 0F 16 16 2B 33"
>
> -#define HSD20_IPS "true"
> +#define HSD20_IPS 1
>
> /**
> * enum st7789v_command - ST7789V display controller commands
Patch does not apply to drm-misc-next, seems to be a staging thing.
So do not expext the DRM people to pick it up.
Sam
> --
> 2.25.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
Hi Xin Ji.
> > > +static void anx7625_power_on_init(struct anx7625_data *ctx)
> > > +{
> > > + int retry_count, i;
> > > + int ret;
> > > + struct device *dev = >client->dev;
> > > +
> > > + for (retry_count = 0; retry_count < 3; retry_count++) {
> > > + anx7625_power_on(ctx);
> > > + anx7625_config(ctx);
> > > +
> > > + for (i = 0; i < OCM_LOADING_TIME; i++) {
> > Code in this for loop is a candidate for a helper function.
> I didn't find any helper function can be used, so I'll keep it.
I was not very clear in my way to express this, sorry.
> >
> > > + /* check interface workable */
> > > + ret = anx7625_reg_read(ctx, ctx->i2c.rx_p0_client,
> > > +FLASH_LOAD_STA);
> > > + if (ret < 0) {
> > > + DRM_ERROR("IO error : access flash load.\n");
> > > + return;
> > > + }
> > > + if ((ret & FLASH_LOAD_STA_CHK) == FLASH_LOAD_STA_CHK) {
> > > + anx7625_disable_pd_protocol(ctx);
> > > + DRM_DEV_DEBUG_DRIVER(dev,
> > > + "Firmware ver %02x%02x,",
> > > + anx7625_reg_read(ctx,
> > > + ctx->i2c.rx_p0_client,
> > > + OCM_FW_VERSION),
> > > + anx7625_reg_read(ctx,
> > > + ctx->i2c.rx_p0_client,
> > > + OCM_FW_REVERSION));
> > > + DRM_DEV_DEBUG_DRIVER(dev, "Driver version %s\n",
> > > + ANX7625_DRV_VERSION);
> > > +
> > > + return;
> > > + }
> > > + usleep_range(1000, 1100);
> > > + }
What I wanted to express is that the for loop is heavily indented.
So create a small function like:
anx7625_power_on_interface(ctx)
{
/* check interface workable */
ret = anx7625_reg_read(ctx, ctx->i2c.rx_p0_client, FLASH_LOAD_STA);
if (ret < 0) {
DRM_ERROR("IO error : access flash load.\n");
return;
}
if ((ret & FLASH_LOAD_STA_CHK) == FLASH_LOAD_STA_CHK) {
anx7625_disable_pd_protocol(ctx);
DRM_DEV_DEBUG_DRIVER(dev, "Firmware ver %02x%02x,",
anx7625_reg_read(ctx, ctx->i2c.rx_p0_client,
OCM_FW_VERSION), anx7625_reg_read(ctx,
ctx->i2c.rx_p0_client,
OCM_FW_REVERSION));
DRM_DEV_DEBUG_DRIVER(dev, "Driver version %s\n",
ANX7625_DRV_VERSION);
retunrn 1;
}
return 0;
}
and then
for (i = 0; i < OCM_LOADING_TIME; i++) {
if (anx7625_power_on_interface(ctx))
return;
else
usleep_range(1000, 1100);
}
Or something like that. To make it more readable.
I think you get the idea now.
> > > + container_of(work, struct anx7625_data, extcon_wq);
> > > + int state = extcon_get_state(ctx->extcon, EXTCON_DISP_DP);
> > > +
> > > + mutex_lock(>lock);
> > > + anx7625_chip_control(ctx, state);
> > > + mutex_unlock(>lock);
> > I tried to follow the locking - but failed.
> > Could you check that locking seems correct.
> >
> > A standard bridge driver do not need locking,
> > but this is no small bridge driver so I do not imply that
> > locking is not needed. Only that I would like you
> > to check it again as I could not follow it.
> OK, it seems lock is not necessary, I'll remove itA
It has a worker, so please be careful in you analysis.
> >
> > > +
> > > + if (pdata->panel_flags == 1)
> > > + pdata->internal_panel = 1;
> > > + else if (pdata->panel_flags == 2)
> > > + pdata->extcon_supported = 1;
> > > + DRM_DEV_DEBUG_DRIVER(dev, "%s support extcon, %s internal panel\n",
> > > + pdata->extcon_supported ? "" : "not",
> > > + pdata->internal_panel ? "has" : "no");
> > > +
> > The way the internal panel - versus external connector is modelled
> > looks like it could use some of the abstractions used by other bridge
> > drivers.
> >
> > The connector_type shall for example for internal panels come
> > form the panel.
> > And use the panel bridge driver - see examples in patches I referenced
> > before.
> >
> > And external connectors may beneft from using the
> > display-connector bridge driver.
> I'm not familiar with it, the extcon interface is Google engineer give
> to me, I just follow their sample driver. If you think it is not good,
> I'll remove the extcon support.
It would be better to start without, and then add it later
so we end up
Re: [PATCH v8 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
Hi Xin Ji On Mon, Apr 27, 2020 at 02:18:44PM +0800, Xin Ji wrote: > The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed > for portable device. It converts MIPI DSI/DPI to DisplayPort 1.3 4K. > > The ANX7625 can support both USB Type-C PD feature and MIPI DSI/DPI > to DP feature. This driver only enabled MIPI DSI/DPI to DP feature. You are sending this patch in an interesting time for bridge drivers. We are migrating to an approach where the individual brdge drivers exposes operations and where the connector creation is now optional. Laurent Pinchart is the architect behind it - and the required interfaces is well documented. You may find inspiration in a patchset I sent today: https://lore.kernel.org/dri-devel/20200427081850.17512-1-...@ravnborg.org/T/#t This is not reviewed - so keep an eye out for feedback. It would be great to base this on top of drm-misc-next, as this is where we will apply it eventually. As it is now it will not build due to internal API changes. The driver looks well structured with nice coding. I am missing an explanation why the current analogix infrastructure cannot be used. I have no clue but I just see other drivers in same dir that benefits from the infrastructure. So the questions seems relevant to be addressed. See a few more comments in the following, which you need to decide what to follow and what to ignore. I will make it obvious when something is a must to change, if I find anything such. Sorry for providing such massive feedback on v8. Please keep up the spirit and submit a v9 soon! Sam > > Signed-off-by: Xin Ji > --- > drivers/gpu/drm/bridge/Makefile |2 +- > drivers/gpu/drm/bridge/analogix/Kconfig |6 + > drivers/gpu/drm/bridge/analogix/Makefile |1 + > drivers/gpu/drm/bridge/analogix/anx7625.c | 2158 > + > drivers/gpu/drm/bridge/analogix/anx7625.h | 410 ++ > 5 files changed, 2576 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 4934fcf..bcd388a 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -12,8 +12,8 @@ obj-$(CONFIG_DRM_SII9234) += sii9234.o > obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o > obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o > obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o > -obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o > obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o > +obj-y += analogix/ > obj-y += synopsys/ With this change we will always visit analogix/ which will trigger build of too much i think. Use: obj-$(CONFIG_ANALOGIX_ANX7625) += analogix/ like the other drivers do, if for nothing else then for consistency. > diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig > b/drivers/gpu/drm/bridge/analogix/Kconfig > index e930ff9..b2f127e 100644 > --- a/drivers/gpu/drm/bridge/analogix/Kconfig > +++ b/drivers/gpu/drm/bridge/analogix/Kconfig > @@ -2,3 +2,9 @@ > config DRM_ANALOGIX_DP > tristate > depends on DRM > + > +config ANALOGIX_ANX7625 > + tristate "Analogix MIPI to DP interface support" > + help > + ANX7625 is an ultra-low power 4K mobile HD transmitter designed > + for portable devices. It converts MIPI/DPI to DisplayPort1.3 4K. > diff --git a/drivers/gpu/drm/bridge/analogix/Makefile > b/drivers/gpu/drm/bridge/analogix/Makefile > index fdbf3fd..8a52867 100644 > --- a/drivers/gpu/drm/bridge/analogix/Makefile > +++ b/drivers/gpu/drm/bridge/analogix/Makefile > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > +obj-$(CONFIG_ANALOGIX_ANX7625) += anx7625.o > analogix_dp-objs := analogix_dp_core.o analogix_dp_reg.o > obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp.o > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c > b/drivers/gpu/drm/bridge/analogix/anx7625.c > new file mode 100644 > index 000..fff7a49 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c > @@ -0,0 +1,2158 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright(c) 2016, Analogix Semiconductor. All rights reserved. 2020? > + * > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#include "anx7625.h" > + > +/* > + * there is a sync issue while access I2C register between AP(CPU) and > + * internal firmware(OCM), to avoid the race condition, AP should access > + * the reserved slave address before slave address occurs changes. > + */ Nit - start comments
Re: [PATCH v8 0/2] Add initial support for slimport anx7625
Hi Xin Ji On Mon, Apr 27, 2020 at 02:16:49PM +0800, Xin Ji wrote: > Hi all, > > The following series add support for the Slimport ANX7625 transmitter, a > ultra-low power Full-HD 4K MIPI to DP transmitter designed for portable > device. > > This is the v8 version, any mistakes, please let me know, I will fix it in > the next series. This series fix several coding format and description. It would be great if you can add a summary of changes like this: v8: - fix several coding format - update description v7: - Bla bla I see no reason to dig out the old changelog, but start from now on. The cover letter (this mail) should give a general intro to the changes. The individual patches the detailed changelog. For each entry is is also a good practice to tell who gave the feedback that triggered the changes. There are many ways to handle this, take a look at a few submissions to dri-devel to be inspired. Sam > > Thanks, > Xin > > > > Xin Ji (2): > dt-bindings: drm/bridge: anx7625: MIPI to DP transmitter binding > drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver > > .../bindings/display/bridge/anx7625.yaml | 91 + > drivers/gpu/drm/bridge/Makefile|2 +- > drivers/gpu/drm/bridge/analogix/Kconfig|6 + > drivers/gpu/drm/bridge/analogix/Makefile |1 + > drivers/gpu/drm/bridge/analogix/anx7625.c | 2158 > > drivers/gpu/drm/bridge/analogix/anx7625.h | 410 > 6 files changed, 2667 insertions(+), 1 deletion(-) > create mode 100644 > Documentation/devicetree/bindings/display/bridge/anx7625.yaml > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h > > -- > 2.7.4 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 1/2] dt-bindings: drm/bridge: anx7625: MIPI to DP transmitter binding
Hi Xin Ji
On Mon, Apr 27, 2020 at 02:17:46PM +0800, Xin Ji wrote:
> The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed
> for portable device. It converts MIPI to DisplayPort 1.3 4K.
Thanks for providing this binding.
When you re-submit please also send to devicet...@vger.kernel.org.
Running the binding through dt_binding_check gives me:
/home/sam/drm/linux.git/Documentation/devicetree/bindings/display/bridge/anx7625.yaml:
ignoring, error in schema:
warning: no schema found in file:
/home/sam/drm/linux.git/Documentation/devicetree/bindings/display/bridge/anx7625.yaml
make[2]: ***
[/home/sam/drm/linux.git/Documentation/devicetree/bindings/Makefile:42:
Documentation/devicetree/bindings/processed-schema.yaml] Error 255
make[2]: *** Waiting for unfinished jobs
/home/sam/drm/linux.git/Documentation/devicetree/bindings/display/bridge/anx7625.yaml:
Additional properties are not allowed ('example' was unexpected)
/home/sam/drm/linux.git/Documentation/devicetree/bindings/display/bridge/anx7625.yaml:
Additional properties are not allowed ('example' was unexpected)
See writing-schemas.rst for instruction installing tools etc.
>
> You can add support to your board with binding.
>
> Example:
> anx7625_bridge: encoder@58 {
> compatible = "analogix,anx7625";
> reg = <0x58>;
> status = "okay";
> panel-flags = <1>;
> enable-gpios = < 45 GPIO_ACTIVE_HIGH>;
> reset-gpios = < 73 GPIO_ACTIVE_HIGH>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> port@0 {
> reg = <0>;
> anx_1_in: endpoint {
> remote-endpoint = <_dsi>;
> };
> };
>
> port@2 {
> reg = <2>;
> anx_1_out: endpoint {
> remote-endpoint = <_in>;
> };
> };
> };
>
> Signed-off-by: Xin Ji
> ---
> .../bindings/display/bridge/anx7625.yaml | 91
> ++
> 1 file changed, 91 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/display/bridge/anx7625.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/anx7625.yaml
> b/Documentation/devicetree/bindings/display/bridge/anx7625.yaml
> new file mode 100644
> index 000..1149ebb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/anx7625.yaml
Name the file "analogix,anx7625.yaml".
(We should rename anx6345.yaml, so others do not omit the company name)
> @@ -0,0 +1,91 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2019 Analogix Semiconductor, Inc.
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/display/bridge/anx7625.yaml#;
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#;
> +
> +title: Analogix ANX7625 SlimPort (4K Mobile HD Transmitter)
> +
> +maintainers:
> + - Xin Ji
> +
> +description: |
> + The ANX7625 is an ultra-low power 4K Mobile HD Transmitter
> + designed for portable devices.
> +
> +properties:
> + "#address-cells": true
> + "#size-cells": true
> +
> + compatible:
> +items:
> + - const: analogix,anx7625
> +
> + reg:
> +maxItems: 1
> +
> + panel-flags:
> +description: indicate the panel is internal or external.
> +maxItems: 1
This property hint at something needs to be modelled in a different way.
I do not recall other bindings need similar info.
> +
> + interrupts:
> +maxItems: 1
A description would be nice.
> +
> + enable-gpios:
> +description: used for power on chip control, POWER_EN pin D2.
> +maxItems: 1
> +
> + reset-gpios:
> +description: used for reset chip control, RESET_N pin B7.
> +maxItems: 1
> +
> + port@0:
> +type: object
> +description:
> + A port node pointing to MIPI DSI host port node.
> +
> + port@1:
> +type: object
> +description:
> + A port node pointing to MIPI DPI host port node.
Maybe explain how it differs from port@0 and why it is optional.
> +
> + port@2:
> +type: object
> +description:
> + A port node pointing to panel port node.
Unless there is a good reason not to then please use a ports node, like
you see in almost (all?) other bridge bindings.
> +
> +required:
> + - "#address-cells"
> + - "#size-cells"
> + - compatible
> + - reg
> + - port@0
> + - port@2
additionalProperties: false??
> +
> +example:
It must be named "examples:" - this is what dt_binding_check complains
about.
> + - |
> +anx7625_bridge: encoder@58 {
> +compatible = "analogix,anx7625";
> +reg = <0x58>;
> +status = "okay";
No status in examples.
> +panel-flags = <1>;
> +enable-gpios = < 45 GPIO_ACTIVE_HIGH>;
You need to include a header file to pull in GPIO_ACTIVE_HIGH - see what
other bindings do.
> +reset-gpios = < 73 GPIO_ACTIVE_HIGH>;
> +#address-cells = <1>;
> +
Re: fbtft: 5 years in staging
Hi Noralf On Sun, Feb 02, 2020 at 04:41:54PM +0100, Noralf Trønnes wrote: > Hi, > > Since I'm the original author of fbtft I thought I'd highlight a couple > of issues that's probably not well known. > > Right after fbtft was added, fbdev was closed for new drivers[1] and > the fbdev maintainer wanted to remove fbtft as a consequence of that > decision, but Greg KH said he'd keep fbtft drivers in staging > "until a matching DRM driver is present in the tree"[2]. > > There are 2 issues wrt the goal of making a matching DRM driver > (strictly speaking). One is impossible to do (policy), the other is > unlikely to happen: > > 1. Device Tree 'init' property > > All fbtft drivers have controller setup code that matches one > particular display panel. From the start of fbtft it was possible to > override this using platform data. Later when Device Tree support was > added, an 'init=' property to do the same was added. > > Example: > init = <0x1e5 0x78F0 > 0x101 0x0100 > 0x232 > 0x107 0x0133>; > > This translates to: > register_write(0x00e5, 0x78F0); > register_write(0x0001, 0x0100); > mdelay(32); > register_write(0x0007, 0x0133); > > AFAIU setting register values from DT is a no-go, so this functionality > can't be replicated in a DRM driver. Many displays are made to work > using this mechanism, in particular ili9341 based ones. > > 2. Parallel bus interface > > All fbtft drivers support both a SPI and a parallel bus interface > through the fbtft helper module. Many (not all) controllers support more > than one interface. The parallel bus support was added to fbtft in its > early days when not many SPI displays were available (nowadays there's > lots to choose from). fbtft uses bitbanging to drive the parallel > interface so it's really slow, even more slow than SPI and SPI with DMA > beats it thoroughly. I know there are people that use the paralell bus > support, but I don't see much point in it unless we get a parallel bus > subsystem that can use the dedicated hw on certain SoC's (Beaglebone, > Pi). And those SOC's most likely have a parallel video/RGB bus as well, > which IMO is a much better option for a panel. > > > The following drivers have DRM counterparts that have the same panel > setup code: > > - fb_hx8357d.c: drivers/gpu/drm/tiny/hx8357d.c > - fb_ili9341.c: drivers/gpu/drm/tiny/mi0283qt.c > - fb_st7735r.c: drivers/gpu/drm/tiny/st7735r.c > - fb_ili9486.c: Patches are posted on dri-devel[3] > > But they don't support all panels based on that controller and they > don't have parallel bus support. > > There is actually also another obstacle for conversion and that is, some > of the displays (for which there is builtin driver support) might be > impossible to source except as second hand. And it's not always obvious > which panel is supported by a certain driver. > At least the displays supported by these drivers are listed as > discontinued on the fbtft wiki[4]: > - fb_hx8340bn.c > - fb_hx8347d.c > - fb_ili9320 > > This one never made it from a prototype to an actual product, because > it was too slow: > - fb_watterott.c > > I have no plans to convert fbtft drivers myself, but I figured a 5 year > anniversary was a good excuse for a status update. Thanks for the history lesson and the status update, a very informative and interesting read. Thanks for all your work in this area! Sam > > Noralf. > > [1] https://lkml.org/lkml/2015/9/24/253 > [2] https://lkml.org/lkml/2016/11/23/146 > [3] https://patchwork.freedesktop.org/series/72645/ > [4] https://github.com/notro/fbtft/wiki/LCD-Modules#discontinued-products > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] FBTFT: fbtft-bus: Fix code style problems
Hi Dante. > Hello Sam, thank you very much for your comments, > As I told Dan (my email did not reach the mailing list) this is my > first attempt to contribute, > So I'm learning a lot from your advice and corrections. > > I will look for TODO lists to see if there are more useful > contributions to make, all suggestions are also welcome. Keep the good spirit, and find some other driver to look into. You must realise than experienced developers sometimes goes through several version before a patch is ready. So expect it to be a bumpy ride the first few times. Good luck! Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] FBTFT: fbtft-bus: Fix code style problems
Hi Eze > > Why is this driver still here? I thought we migrated everyhing to > tinydrm already. Some have been ported, some are waiting for a user to do the port. If you looks at tinydrm you will see: ili9225.c ili9341.c And we also have under panel: panel-ilitek-ili9881c.c panel-ilitek-ili9322.c But in staging there are more panels than just these. So we have not yet ported all. And there is today no list of what is missing. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] FBTFT: fbtft-bus: Fix code style problems
Hi Dante Thanks for the patch. On Sat, Mar 09, 2019 at 06:48:52PM -0300, Dante Paz wrote: > From: Dante Paz > > Style and coding function issues were corrected, by avoiding macro > functions with a conflicting coding style. > Signed-off-by: Dante Paz But it raised a few comments. The staging/fbtft is a dumping of a set of drivers that in the end will be migrated to DRM. And there is not much gained trying to do coding style changes to these drivers. So please conmsider finding a drver where this is more relevant. Furthermore that patch presented is hard to review as it contains too much changes in one go. As a rule of thumb include only one type of change per patch. This is worth to keep in mind for future submissions. It it then also good to present the trivial changes first(*), and the less trivial changes later. (*) Like whitespace to tabs, spellign errors etc. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drm/vboxvideo: Move vboxvideo driver out of staging
Hi Hans.
Just a bunch of random observations that I hope you find use of.
Sam
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index cb88528e7b10..6b4d6c957da8 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -315,6 +315,8 @@ source "drivers/gpu/drm/tve200/Kconfig"
>
> source "drivers/gpu/drm/xen/Kconfig"
>
> +source "drivers/gpu/drm/vboxvideo/Kconfig"
> +
> # Keep legacy drivers last
>
> menuconfig DRM_LEGACY
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index a6771cef85e2..133606802300 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -107,3 +107,4 @@ obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
> obj-$(CONFIG_DRM_PL111) += pl111/
> obj-$(CONFIG_DRM_TVE200) += tve200/
> obj-$(CONFIG_DRM_XEN) += xen/
> +obj-$(CONFIG_DRM_VBOXVIDEO) += vboxvideo/
Is it the most logical place to add this in the end?
You are asking for merge problems if you do so, but then
the drm drivers does not seem to follow any good order
so this may be fine.
> +++ b/drivers/gpu/drm/vboxvideo/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +ccflags-y := -Iinclude/drm
This should not be required.
If required then fix the offending #include
> +
> +vboxvideo-y := hgsmi_base.o modesetting.o vbva_base.o \
> + vbox_drv.o vbox_fb.o vbox_hgsmi.o vbox_irq.o vbox_main.o \
> + vbox_mode.o vbox_prime.o vbox_ttm.o
> +
> +obj-$(CONFIG_DRM_VBOXVIDEO) += vboxvideo.o
> diff --git a/drivers/gpu/drm/vboxvideo/hgsmi_base.c
> b/drivers/gpu/drm/vboxvideo/hgsmi_base.c
> new file mode 100644
> index ..361d3193258e
> --- /dev/null
> +++ b/drivers/gpu/drm/vboxvideo/hgsmi_base.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: MIT
> +/* Copyright (C) 2006-2017 Oracle Corporation */
2018?
General comment for all files.
> + union {
> + /* Opaque placeholder to make the union 8 bytes. */
> + u8 header_data[8];
Further down you have 2 x u32, so the union is guaranteed to be 64 bits.
So header_data is redundant.
> +
> + /* HGSMI_BUFFER_HEADER_F_SEQ_SINGLE */
> + struct {
> + u32 reserved1; /* A reserved field, initialize to 0. */
> + u32 reserved2; /* A reserved field, initialize to 0. */
> + } buffer;
> --- /dev/null
> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright (C) 2013-2017 Oracle Corporation
> + * This file is based on ast_drv.c
> + * Copyright 2012 Red Hat Inc.
> + * Authors: Dave Airlie
> + * Michael Thayer + * Hans de Goede
> + */
> +#include
> +#include
> +#include
> +
> +#include
We are trying to get rid of drmP - replace with more specific includes.
Goes for all uses of drmP.h
> +
> +static int vbox_modeset = -1;
> +
> +MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
> +module_param_named(modeset, vbox_modeset, int, 0400);
> +
> +static struct drm_driver driver;
> +
> +static const struct pci_device_id pciidlist[] = {
> + { 0x80ee, 0xbeef, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
> + { 0, 0, 0},
> +};
Use PCI_DEVICE()?
> +static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id
> *ent)
> +{
> + struct vbox_private *vbox;
> + int ret = 0;
> +
> + if (!vbox_check_supported(VBE_DISPI_ID_HGSMI))
> + return -ENODEV;
> +
> + vbox = kzalloc(sizeof(*vbox), GFP_KERNEL);
> + if (!vbox)
> + return -ENOMEM;
> +
> + ret = drm_dev_init(>ddev, , >dev);
> + if (ret) {
> + kfree(vbox);
> + return ret;
> + }
> +
> + vbox->ddev.pdev = pdev;
> + vbox->ddev.dev_private = vbox;
> + pci_set_drvdata(pdev, vbox);
> + mutex_init(>hw_mutex);
> +
> + ret = pci_enable_device(pdev);
> + if (ret)
> + goto err_dev_put;
> +
> + ret = vbox_hw_init(vbox);
> + if (ret)
> + goto err_pci_disable;
> +
> + ret = vbox_mm_init(vbox);
> + if (ret)
> + goto err_hw_fini;
> +
> + ret = vbox_mode_init(vbox);
> + if (ret)
> + goto err_mm_fini;
> +
> + ret = vbox_irq_init(vbox);
> + if (ret)
> + goto err_mode_fini;
> +
> + ret = drm_fb_helper_fbdev_setup(>ddev, >fb_helper,
> + _fb_helper_funcs, 32,
> + vbox->num_crtcs);
> + if (ret)
> + goto err_irq_fini;
> +
> + ret = drm_dev_register(>ddev, 0);
> + if (ret)
> + goto err_fbdev_fini;
> +
> + return 0;
> +
> +err_fbdev_fini:
> + vbox_fbdev_fini(vbox);
> +err_irq_fini:
> + vbox_irq_fini(vbox);
> +err_mode_fini:
> + vbox_mode_fini(vbox);
> +err_mm_fini:
> + vbox_mm_fini(vbox);
> +err_hw_fini:
> + vbox_hw_fini(vbox);
> +err_pci_disable:
> + pci_disable_device(pdev);
> +err_dev_put:
> + drm_dev_put(>ddev);
Re: [PATCH 00/25] Change tty_port(standard)_install's return type
Hi Jaejoong. > Change return type for tty functions. Patch No.01 > tty: Change return type to void Adding this patch first will generate a lot of warnings until all users are updated. It is usual practice to prepare all users and then apply the infrastructure changes as the last patch. Then people will not see a lot of warnings when they build something in the middle, and I guess current stack set may also generate a few mails from the 0-day build infrastructure. > isdn: i4l: isdn_tty: Change return type to void And a nitpick on the patch description. This patch do not change any return type, but it ignore the return value og tty_part_install(). Same goes for all ramaining patches. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging/comedi: Fixes static analysis warning raised by sparse
On Mon, Jun 16, 2014 at 10:40:19AM +0300, Dan Carpenter wrote:
On Sun, Jun 15, 2014 at 09:32:27PM +0200, Sam Ravnborg wrote:
diff --git a/expand.c b/expand.c
index 0f6720c..4a96de4 100644
--- a/expand.c
+++ b/expand.c
@@ -187,7 +187,7 @@ static int simplify_int_binop(struct expression *expr,
struct symbol *ctype)
return 0;
r = right-value;
if (expr-op == SPECIAL_LEFTSHIFT || expr-op == SPECIAL_RIGHTSHIFT) {
- if (r = ctype-bit_size) {
+ if (expr-flags Int_const_expr r = ctype-bit_size) {
Thanks! I had no idea how to start writing a fix for this, but the test
should be:
if (expr-right-flags Int_const_expr
Otherwise both sides of the shift have to be const.
Thanks - will fix.
I will update the test case to check for this, and
then send a proper patch.
Sam
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging/comedi: Fixes static analysis warning raised by sparse
Hi Josh.
On Wed, Jun 11, 2014 at 02:45:29PM -0700, j...@joshtriplett.org wrote:
On Thu, Jun 12, 2014 at 12:24:25AM +0300, Dan Carpenter wrote:
Let's forward this to the Sparse mailing list.
We're seeing a Sparse false positive testing
drivers/staging/comedi/drivers/ni_pcimio.c.
CHECK drivers/staging/comedi/drivers/ni_pcimio.c
drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big
(4294967295) for type int
drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big
(4294967295) for type int
drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big
(4294967295) for type int
drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big
(4294967295) for type int
I have created some test code to demonstrate the problem (attached).
The check_shift_count() warning is only supposed to be printed for
number literals but because of the way inline functions are expanded it
still complains even though channel is a variable.
Thanks for the test case; this definitely makes no sense. I don't think
Sparse will suddenly develop enough range analysis or reachability
analysis to handle this case; I think the right answer is to avoid
giving such warnings for shifts with a non-constant RHS.
Something like the appended?
It seems to work for me - and it cured a lot of warnings in math-emu.c on sparc.
If it looks OK I will do a proper patch submission.
Sam
diff --git a/expand.c b/expand.c
index 0f6720c..4a96de4 100644
--- a/expand.c
+++ b/expand.c
@@ -187,7 +187,7 @@ static int simplify_int_binop(struct expression *expr,
struct symbol *ctype)
return 0;
r = right-value;
if (expr-op == SPECIAL_LEFTSHIFT || expr-op == SPECIAL_RIGHTSHIFT) {
- if (r = ctype-bit_size) {
+ if (expr-flags Int_const_expr r = ctype-bit_size) {
if (conservative)
return 0;
r = check_shift_count(expr, ctype, r);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
