Re: [PATCH] drm/bridge: anx7625: enable DSI EOTP

[Andrzej Hajda]

W dniu 04.02.2021 o 13:34, Nicolas Boichat pisze:
> On Thu, Feb 4, 2021 at 8:07 PM Robert Foss  wrote:
>> Hi Xin,
>>
>> Thanks for the patch.
>>
>> On Thu, 28 Jan 2021 at 12:17, Xin Ji  wrote:
>>> Enable DSI EOTP feature for fixing some panel screen constance
>>> shift issue.
>>> Removing MIPI flag MIPI_DSI_MODE_EOT_PACKET to enable DSI EOTP.
>> I don't think I quite understand how removing the
>> MIPI_DSI_MODE_EOT_PACKET flag will cause DSI EOTP to be enabled. Could
>> you extrapolate on this in the commit message?
> That confused me as well, but it turns out that's how the flag is defined:
> ```
> /* disable EoT packets in HS mode */
> #define MIPI_DSI_MODE_EOT_PACKET BIT(9)
> ```
> (https://protect2.fireeye.com/v1/url?k=5bd95ebd-044267fb-5bd8d5f2-0cc47a3003e8-ce9db8ea264d6901=1=900556dc-d199-4c18-9432-5c3465a98eae=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Finclude%2Fdrm%2Fdrm_mipi_dsi.h%23L129)
>
> I'm almost tempted to put together a mass patch to rename all of these 
> flags...


Yes that would be good, many of these flags were just copy pasted from 
some hw datasheet, without good analysis how to adapt them to the framework.


Regards

Andrzej


>
>>> Signed-off-by: Xin Ji 
>>> ---
>>>   drivers/gpu/drm/bridge/analogix/anx7625.c | 1 -
>>>   1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c 
>>> b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> index 65cc059..e31eeb1b 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> @@ -1334,7 +1334,6 @@ static int anx7625_attach_dsi(struct anx7625_data 
>>> *ctx)
>>>  dsi->format = MIPI_DSI_FMT_RGB888;
>>>  dsi->mode_flags = MIPI_DSI_MODE_VIDEO   |
>>>  MIPI_DSI_MODE_VIDEO_SYNC_PULSE  |
>>> -   MIPI_DSI_MODE_EOT_PACKET|
>>>  MIPI_DSI_MODE_VIDEO_HSE;
>>>
>>>  if (mipi_dsi_attach(dsi) < 0) {
>>> --
>>> 2.7.4
>>>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://protect2.fireeye.com/v1/url?k=457f3f39-1ae4067f-457eb476-0cc47a3003e8-b702072da729d8c9=1=900556dc-d199-4c18-9432-5c3465a98eae=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] dt-bindings: drm/bridge: anx7625: MIPI to DP transmitter binding

[Andrzej Hajda]

On 11.10.2019 04:21, 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.
>
> 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@3 {
> reg = <3>;
> anx_1_out: endpoint {
>   remote-endpoint = <_in>;
> };
>   };
>   };
>
> Signed-off-by: Xin Ji 
> ---
>  .../bindings/display/bridge/anx7625.yaml   | 96 
> ++
>  1 file changed, 96 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..fc84683
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/anx7625.yaml
> @@ -0,0 +1,96 @@
> +# 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
> +
> +  interrupts:
> +maxItems: 1
> +
> +  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.
> +
> +  port@2:
> +type: object
> +description:
> +  A port node pointing to external connector port node.
> +
> +  port@3:
> +type: object
> +description:
> +  A port node pointing to eDP port node.


Decrypting available product brief[1], there are following physical lines:

Input:

- MIPI DSI/DPI - video data, are DSI and DPI lines shared?

- I2S - audio data,

- I2C - control line,

- ALERT/INTP - interrupt,

- USB 3.1 SSRc/Tx - for USB forwarding,

Output:

- SS1, SS2,

- SBU/AUX,

- CC1/2.


Having this information I try to understand ports defined by you:

- port@2 you have defined as pointing to external port, but here the
port should be rather subnode of ANX7625 - the chip has CC lines, see
beginning of [2].

- port@3 describes SS1, SS2 and SBU/AUX lines together, am I right? In
USB-C binding SBU and SS lines are represented by different ports,
different approach, but maybe better in this case.


Maybe it would be good to add 2nd example with USB-C port.


[1]:
https://www.analogix.com/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf

[2]: Documentation/devicetree/bindings/connector/usb-connector.txt


Regards

Andrzej


> +
> +required:
> +  - "#address-cells"
> +  - "#size-cells"
> +  - compatible
> +  - reg
> +  - port@0
> +  - port@3
> +
> +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@3 {
> +  reg = <3>;
> +  anx_1_out: endpoint {
> +remote-endpoint = <_in>;
> +  };
> +};
> +};


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 0/2] Add initial support for slimport anx7625

[Andrzej Hajda]

On 11.10.2019 04:20, Xin Ji wrote:
> Hi all,
>
> The following series add initial support for the Slimport ANX7625 
> transmitter, a
> ultra-low power Full-HD 4K MIPI to DP transmitter designed for portable 
> device.
>
> This is the initial version, any mistakes, please let me know, I will fix it 
> in
> the next series.
>
> Thanks,
> Xin


Next time please increment patchset version number - this is third
iteration of v2.


Regards

Andrzej


>
>
> 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   |   96 +
>  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  | 2153 
> 
>  drivers/gpu/drm/bridge/analogix/anx7625.h  |  406 
>  6 files changed, 2663 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
>

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/12] treewide: Fix GENMASK misuses

[Andrzej Hajda]

Hi Joe,

On 10.07.2019 07:04, Joe Perches wrote:
> These GENMASK uses are inverted argument order and the
> actual masks produced are incorrect.  Fix them.
>
> Add checkpatch tests to help avoid more misuses too.
>
> Joe Perches (12):
>   checkpatch: Add GENMASK tests
>   clocksource/drivers/npcm: Fix misuse of GENMASK macro
>   drm: aspeed_gfx: Fix misuse of GENMASK macro
>   iio: adc: max9611: Fix misuse of GENMASK macro
>   irqchip/gic-v3-its: Fix misuse of GENMASK macro
>   mmc: meson-mx-sdio: Fix misuse of GENMASK macro
>   net: ethernet: mediatek: Fix misuses of GENMASK macro
>   net: stmmac: Fix misuses of GENMASK macro
>   rtw88: Fix misuse of GENMASK macro
>   phy: amlogic: G12A: Fix misuse of GENMASK macro
>   staging: media: cedrus: Fix misuse of GENMASK macro
>   ASoC: wcd9335: Fix misuse of GENMASK macro
>
>  drivers/clocksource/timer-npcm7xx.c   |  2 +-
>  drivers/gpu/drm/aspeed/aspeed_gfx.h   |  2 +-
>  drivers/iio/adc/max9611.c |  2 +-
>  drivers/irqchip/irq-gic-v3-its.c  |  2 +-
>  drivers/mmc/host/meson-mx-sdio.c  |  2 +-
>  drivers/net/ethernet/mediatek/mtk_eth_soc.h   |  2 +-
>  drivers/net/ethernet/mediatek/mtk_sgmii.c |  2 +-
>  drivers/net/ethernet/stmicro/stmmac/descs.h   |  2 +-
>  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c |  4 ++--
>  drivers/net/wireless/realtek/rtw88/rtw8822b.c |  2 +-
>  drivers/phy/amlogic/phy-meson-g12a-usb2.c |  2 +-
>  drivers/staging/media/sunxi/cedrus/cedrus_regs.h  |  2 +-
>  scripts/checkpatch.pl | 15 +++
>  sound/soc/codecs/wcd-clsh-v2.c|  2 +-
>  14 files changed, 29 insertions(+), 14 deletions(-)
>
After adding following compile time check:

--

diff --git a/Makefile b/Makefile
index 5102b2bbd224..ac4ea5f443a9 100644
--- a/Makefile
+++ b/Makefile
@@ -457,7 +457,7 @@ KBUILD_AFLAGS   := -D__ASSEMBLY__ -fno-PIE
 KBUILD_CFLAGS   := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \
   -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \
   -Werror=implicit-function-declaration
-Werror=implicit-int \
-  -Wno-format-security \
+  -Wno-format-security -Werror=div-by-zero \
   -std=gnu89
 KBUILD_CPPFLAGS := -D__KERNEL__
 KBUILD_AFLAGS_KERNEL :=
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 669d69441a62..61d74b103055 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -19,11 +19,11 @@
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x00e0.
  */
 #define GENMASK(h, l) \
-   (((~UL(0)) - (UL(1) << (l)) + 1) & \
+   (((~UL(0)) - (UL(1) << (l)) + 1 + 0/((h) >= (l))) & \
 (~UL(0) >> (BITS_PER_LONG - 1 - (h
 
 #define GENMASK_ULL(h, l) \
-   (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
+   (((~ULL(0)) - (ULL(1) << (l)) + 1 + 0/((h) >= (l))) & \
 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h
 
 #endif /* __LINUX_BITS_H */

---

I was able to detect one more GENMASK misue (AARCH64, allyesconfig):

  CC  drivers/phy/rockchip/phy-rockchip-inno-hdmi.o
In file included from ../include/linux/bitops.h:5:0,
 from ../include/linux/kernel.h:12,
 from ../include/linux/clk.h:13,
 from ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:9:
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c: In function
‘inno_hdmi_phy_rk3328_power_on’:
../include/linux/bits.h:22:37: error: division by zero [-Werror=div-by-zero]
  (((~UL(0)) - (UL(1) << (l)) + 1 + 0/((h) >= (l))) & \
 ^
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:24:42: note: in
expansion of macro ‘GENMASK’
 #define UPDATE(x, h, l)  (((x) << (l)) & GENMASK((h), (l)))
  ^~~
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:201:50: note: in
expansion of macro ‘UPDATE’
 #define RK3328_TERM_RESISTOR_CALIB_SPEED_7_0(x)  UPDATE(x, 7, 9)
  ^~
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:1046:26: note: in
expansion of macro ‘RK3328_TERM_RESISTOR_CALIB_SPEED_7_0’
   inno_write(inno, 0xc6, RK3328_TERM_RESISTOR_CALIB_SPEED_7_0(v));


Of course I do not advise to add the check as is to Kernel - it is
undefined behavior area AFAIK.


Regards

Andrzej

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 01/13] dt-bindings: connector: add properties for typec

[Andrzej Hajda]

On 28.03.2018 18:06, Li Jun wrote:
> Add bingdings supported by current typec driver, so user can pass
> all those properties via dt.
>
> Signed-off-by: Li Jun 
> ---
>  .../bindings/connector/usb-connector.txt   | 39 
> ++
>  1 file changed, 39 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt 
> b/Documentation/devicetree/bindings/connector/usb-connector.txt
> index e1463f1..922f22b 100644
> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> @@ -15,6 +15,29 @@ Optional properties:
>  - type: size of the connector, should be specified in case of USB-A, USB-B
>non-fullsize connectors: "mini", "micro".
>  
> +Optional properties for usb-c-connector:
> +- power-type: should be one of "source", "sink" or "dual"(DRP) if typec
> +  connector has power support.
> +- try-power-role: preferred power role if "dual"(DRP) can support Try.SNK
> +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> +- data-type: should be one of "host", "device", "dual"(DRD) if typec
> +  connector supports USB data.
> +
> +Required properties for usb-c-connector with power delivery support:
> +- source-pdos: An array of u32 with each entry providing supported power
> +  source data object(PDO), the detailed bit definitions of PDO can be found
> +  in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
> +  Source_Capabilities Message, the order of each entry(PDO) should follow
> +  the PD spec chapter 6.4.1. Required for power source and power dual role.
> +- sink-pdos: An array of u32 with each entry providing supported power
> +  sink data object(PDO), the detailed bit definitions of PDO can be found
> +  in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
> +  Sink Capabilities Message, the order of each entry(PDO) should follow
> +  the PD spec chapter 6.4.1. Required for power sink and power dual role.
> +- op-sink-microwatt-hours: Sink required operating power in micro
> +  watt-hours, if source offered power is less then it, Capability Mismatch
> +  is set, required for power sink and power dual role.

I have lurked into specs and I am not sure what is the relation of this
field with PD protocol, there is Minimum Operating Power, Maximum
Operating Power and Operating Power present in different RDOs, there are
also similar fields in sink PDOs.
I guess it can be one of them, which one? why other ones are not
provided? What if this or any other property can be calculated only in
runtime?
I am not sure if any of them should be marked "required".

> +
>  Required nodes:
>  - any data bus to the connector should be modeled using the OF graph bindings
>specified in bindings/graph.txt, unless the bus is between parent node and
> @@ -73,3 +96,19 @@ ccic: s2mm005@33 {
>   };
>   };
>  };
> +
> +3. USB-C connector attached to a typec port controller(ptn5110), which has
> +power delivery support and enables drp.
> +
> +typec: ptn5110@50 {
> + ...
> + usb_con: connector {
> + compatible = "usb-c-connector";
> + label = "USB-C";
> + power-type = "dual";
> + try-power-role = "sink";
> + source-pdos = <0x380190c8>;

I understand from DT specification point of view cryptic numbers are OK,
but for sake of readability I strongly suggest use/define macros similar
to the ones present already in kernel:
    source-pdos = < PDO_FIXED(5000, 400, PDO_FIXED_FLAGS) >;

It much less error prone, and makes review process much more easier.

Regards
Andrzej

> + sink-pdos = <0x380190c8 0x3802d0c8>;
> + op-sink-microwatt-hours = <900>;
> + };
> +};


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: lustre: remove useless check

[Andrzej Hajda]

Variable reserved is unsigned so the check is always true.

The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576

Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
---
 drivers/staging/lustre/lustre/llite/rw.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/llite/rw.c 
b/drivers/staging/lustre/lustre/llite/rw.c
index 95cdb0c..2fd07a2 100644
--- a/drivers/staging/lustre/lustre/llite/rw.c
+++ b/drivers/staging/lustre/lustre/llite/rw.c
@@ -764,7 +764,6 @@ int ll_readahead(const struct lu_env *env, struct cl_io *io,
ret = ll_read_ahead_pages(env, io, queue,
  ria, , mapping, _end);
 
-   LASSERTF(reserved >= 0, "reserved %lu\n", reserved);
if (reserved != 0)
ll_ra_count_put(ll_i2sbi(inode), reserved);
 
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 06/19] staging: media: omap4iss: fix handling platform_get_irq result

[Andrzej Hajda]

The function can return negative value.

The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/assign_signed_to_unsigned.cocci [1].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2046107

Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
---
Hi,

To avoid problems with too many mail recipients I have sent whole
patchset only to LKML. Anyway patches have no dependencies.

Regards
Andrzej
---
 drivers/staging/media/omap4iss/iss.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/omap4iss/iss.c 
b/drivers/staging/media/omap4iss/iss.c
index 9bfb725..0b03cb7 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -1440,12 +1440,13 @@ static int iss_probe(struct platform_device *pdev)
 iss_reg_read(iss, OMAP4_ISS_MEM_ISP_SYS1, ISP5_REVISION));
 
/* Interrupt */
-   iss->irq_num = platform_get_irq(pdev, 0);
-   if (iss->irq_num <= 0) {
+   ret = platform_get_irq(pdev, 0);
+   if (ret <= 0) {
dev_err(iss->dev, "No IRQ resource\n");
ret = -ENODEV;
goto error_iss;
}
+   iss->irq_num = ret;
 
if (devm_request_irq(iss->dev, iss->irq_num, iss_isr, IRQF_SHARED,
 "OMAP4 ISS", iss)) {
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/38] Fixes related to incorrect usage of unsigned types

[Andrzej Hajda]

On 09/21/2015 03:42 PM, David Howells wrote:
> Andrzej Hajda <a.hajda-sze3o3uu22jbdgjk7y7...@public.gmane.org> wrote:
> 
>> Semantic patch finds comparisons of types:
>> unsigned < 0
>> unsigned >= 0
>> The former is always false, the latter is always true.
>> Such comparisons are useless, so theoretically they could be
>> safely removed, but their presence quite often indicates bugs.
> 
> Or someone has left them in because they don't matter and there's the
> possibility that the type being tested might be or become signed under some
> circumstances.  If the comparison is useless, I'd expect the compiler to just
> discard it - for such cases your patch is pointless.
> 
> If I have, for example:
> 
>   unsigned x;
> 
>   if (x == 0 || x > 27)
>   give_a_range_error();
> 
> I will write this as:
> 
>   unsigned x;
> 
>   if (x <= 0 || x > 27)
>   give_a_range_error();
> 
> because it that gives a way to handle x being changed to signed at some point
> in the future for no cost.  In which case, your changing the <= to an ==
> "because the < part of the case is useless" is arguably wrong.

This is why I have not checked for such cases - I have skipped checks of type
unsigned <= 0
exactly for the reasons above.

However I have left two other checks as they seems to me more suspicious - they
are always true or false. But as Dmitry and Andrew pointed out Linus have quite
strong opinion against removing range checks in such cases as he finds it
clearer. I think it applies to patches 29-36. I am not sure about patches 
26-28,37.

Regards
Andrzej

> 
> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo-u79uwxl29ty76z2rm5m...@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 05/38] staging: lustre: fix handling lustre_posix_acl_xattr_filter result

[Andrzej Hajda]

The function can return negative value.

The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576

Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
---
 drivers/staging/lustre/lustre/llite/xattr.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/xattr.c 
b/drivers/staging/lustre/lustre/llite/xattr.c
index 362a87d..dddc1ff 100644
--- a/drivers/staging/lustre/lustre/llite/xattr.c
+++ b/drivers/staging/lustre/lustre/llite/xattr.c
@@ -175,11 +175,12 @@ int ll_setxattr_common(struct inode *inode, const char 
*name,
}
ee_free(ee);
} else if (rce->rce_ops == RMT_RSETFACL) {
-   size = lustre_posix_acl_xattr_filter(
+   rc = lustre_posix_acl_xattr_filter(
(posix_acl_xattr_header *)value,
size, _value);
-   if (unlikely(size < 0))
-   return size;
+   if (unlikely(rc < 0))
+   return rc;
+   size = rc;
 
pv = (const char *)new_value;
} else
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 04/38] IB/ehca: fix handling idr_alloc result

[Andrzej Hajda]

The function can return negative value.

The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576

Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
---
 drivers/staging/rdma/ehca/ehca_cq.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rdma/ehca/ehca_cq.c 
b/drivers/staging/rdma/ehca/ehca_cq.c
index 9b68b17..ea1b5c1 100644
--- a/drivers/staging/rdma/ehca/ehca_cq.c
+++ b/drivers/staging/rdma/ehca/ehca_cq.c
@@ -130,7 +130,7 @@ struct ib_cq *ehca_create_cq(struct ib_device *device,
void *vpage;
u32 counter;
u64 rpage, cqx_fec, h_ret;
-   int ipz_rc, i;
+   int rc, i;
unsigned long flags;
 
if (attr->flags)
@@ -170,16 +170,17 @@ struct ib_cq *ehca_create_cq(struct ib_device *device,
 
idr_preload(GFP_KERNEL);
write_lock_irqsave(_cq_idr_lock, flags);
-   my_cq->token = idr_alloc(_cq_idr, my_cq, 0, 0x200, GFP_NOWAIT);
+   rc = idr_alloc(_cq_idr, my_cq, 0, 0x200, GFP_NOWAIT);
write_unlock_irqrestore(_cq_idr_lock, flags);
idr_preload_end();
 
-   if (my_cq->token < 0) {
+   if (rc < 0) {
cq = ERR_PTR(-ENOMEM);
ehca_err(device, "Can't allocate new idr entry. device=%p",
 device);
goto create_cq_exit1;
}
+   my_cq->token = rc;
 
/*
 * CQs maximum depth is 4GB-64, but we need additional 20 as buffer
@@ -195,11 +196,11 @@ struct ib_cq *ehca_create_cq(struct ib_device *device,
goto create_cq_exit2;
}
 
-   ipz_rc = ipz_queue_ctor(NULL, _cq->ipz_queue, param.act_pages,
+   rc = ipz_queue_ctor(NULL, _cq->ipz_queue, param.act_pages,
EHCA_PAGESIZE, sizeof(struct ehca_cqe), 0, 0);
-   if (!ipz_rc) {
+   if (!rc) {
ehca_err(device, "ipz_queue_ctor() failed ipz_rc=%i device=%p",
-ipz_rc, device);
+rc, device);
cq = ERR_PTR(-EINVAL);
goto create_cq_exit3;
}
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 26/38] staging: lustre: remove invalid check

[Andrzej Hajda]

Unsigned cannot be negative.

The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576

Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
---
 drivers/staging/lustre/lustre/osc/lproc_osc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c 
b/drivers/staging/lustre/lustre/osc/lproc_osc.c
index c504d15..f3eb39f 100644
--- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
+++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
@@ -451,9 +451,6 @@ static ssize_t resend_count_store(struct kobject *kobj,
if (rc)
return rc;
 
-   if (val < 0)
-  return -EINVAL;
-
atomic_set(>u.cli.cl_resends, val);
 
return count;
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 25/38] staging: media: davinci_vpfe: fix ipipe_mode type

[Andrzej Hajda]

The variable can take negative values.

The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576

Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
---
 drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
index 2a3a56b..b1d5e23 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
@@ -254,7 +254,7 @@ int config_ipipe_hw(struct vpfe_ipipe_device *ipipe)
void __iomem *ipipe_base = ipipe->base_addr;
struct v4l2_mbus_framefmt *outformat;
u32 color_pat;
-   u32 ipipe_mode;
+   int ipipe_mode;
u32 data_path;
 
/* enable clock to IPIPE */
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 19/31] staging/lustre: use kmemdup rather than duplicating its implementation

[Andrzej Hajda]

The patch was generated using fixed coccinelle semantic patch
scripts/coccinelle/api/memdup.cocci [1].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2014320

Signed-off-by: Andrzej Hajda a.ha...@samsung.com
---
 drivers/staging/lustre/lustre/obdclass/acl.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/acl.c 
b/drivers/staging/lustre/lustre/obdclass/acl.c
index bc3fc47..933456c 100644
--- a/drivers/staging/lustre/lustre/obdclass/acl.c
+++ b/drivers/staging/lustre/lustre/obdclass/acl.c
@@ -104,11 +104,10 @@ static int 
lustre_posix_acl_xattr_reduce_space(posix_acl_xattr_header **header,
if (unlikely(old_count = new_count))
return old_size;
 
-   new = kzalloc(new_size, GFP_NOFS);
+   new = kmemdup(*header, new_size, GFP_NOFS);
if (unlikely(new == NULL))
return -ENOMEM;
 
-   memcpy(new, *header, new_size);
kfree(*header);
*header = new;
return new_size;
@@ -125,11 +124,10 @@ static int 
lustre_ext_acl_xattr_reduce_space(ext_acl_xattr_header **header,
if (unlikely(old_count = ext_count))
return 0;
 
-   new = kzalloc(ext_size, GFP_NOFS);
+   new = kmemdup(*header, ext_size, GFP_NOFS);
if (unlikely(new == NULL))
return -ENOMEM;
 
-   memcpy(new, *header, ext_size);
kfree(*header);
*header = new;
return 0;
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] gpio: gpiolib: set gpiochip_remove retval to void

[Andrzej Hajda]

On 06/09/2014 01:29 PM, Lars-Peter Clausen wrote:
 On 06/09/2014 01:18 AM, Ben Dooks wrote:
 On Fri, May 30, 2014 at 08:16:59PM +0200, Lars-Peter Clausen wrote:
 On 05/30/2014 07:33 PM, David Daney wrote:
 On 05/30/2014 04:39 AM, Geert Uytterhoeven wrote:
 On Fri, May 30, 2014 at 1:30 PM, abdoulaye berthe berthe...@gmail.com
 wrote:
 --- a/drivers/gpio/gpiolib.c
 +++ b/drivers/gpio/gpiolib.c
 @@ -1263,10 +1263,9 @@ static void gpiochip_irqchip_remove(struct
 gpio_chip *gpiochip);
*
* A gpio_chip with any GPIOs still requested may not be removed.
*/
 -int gpiochip_remove(struct gpio_chip *chip)
 +void gpiochip_remove(struct gpio_chip *chip)
   {
  unsigned long   flags;
 -   int status = 0;
  unsignedid;

  acpi_gpiochip_remove(chip);
 @@ -1278,24 +1277,15 @@ int gpiochip_remove(struct gpio_chip *chip)
  of_gpiochip_remove(chip);

  for (id = 0; id  chip-ngpio; id++) {
 -   if (test_bit(FLAG_REQUESTED, chip-desc[id].flags)) {
 -   status = -EBUSY;
 -   break;
 -   }
 -   }
 -   if (status == 0) {
 -   for (id = 0; id  chip-ngpio; id++)
 -   chip-desc[id].chip = NULL;
 -
 -   list_del(chip-list);
 +   if (test_bit(FLAG_REQUESTED, chip-desc[id].flags))
 +   panic(gpio: removing gpiochip with gpios still
 requested\n);

 panic?

 NACK to the patch for this reason.  The strongest thing you should do here
 is WARN.

 That said, I am not sure why we need this whole patch set in the first 
 place.

 Well, what currently happens when you remove a device that is a
 provider of a gpio_chip which is still in use, is that the kernel
 crashes. Probably with a rather cryptic error message. So this patch
 doesn't really change the behavior, but makes it more explicit what
 is actually wrong. And even if you replace the panic() by a WARN()
 it will again just crash slightly later.

 This is a design flaw in the GPIO subsystem that needs to be fixed.

 Surely then the best way is to error out to the module unload and
 stop the driver being unloaded?

 
 You can't error out on module unload, although that's not really relevant 
 here. gpiochip_remove() is typically called when the device that registered 
 the GPIO chip is unbound. And despite some remove() callbacks having a 
 return type of int you can not abort the removal of a device.

It is a design flaw in many subsystems having providers and consumers,
not only GPIO. The same situation is with clock providers, regulators,
phys, drm_panels, ..., at least it was such last time I have tested it.

The problem is that many frameworks assumes that lifetime of provider is
always bigger than lifetime of its consumers, and this is wrong
assumption - usually it is not possible to prevent unbinding driver from
device, so if the device is a provider there is no way to inform
consumers about his removal.

Some solution for such problems is to use some kind of availability
callbacks for requesting resources (gpios, clocks, regulators,...)
instead of simple 'getters' (clk_get, gpiod_get). Callbacks should
guarantee that the resource is always valid between callback reporting
its availability and callback reporting its removal. Such approach seems
to be complicated at the first sight but it should allow to make the
code safe and as a bonus it will allow to avoid deferred probing.
Btw I have send already RFC for such framework [1].

[1]: https://lkml.org/lkml/2014/4/30/345

Regards
Andrzej


 
 - Lars
 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12][ 07/12] drm: drm_display_mode: add signal polarity flags

[Andrzej Hajda]

Hi Denis,

On 04/07/2014 02:44 PM, Denis Carikli wrote:
 We need a way to pass signal polarity informations
   between DRM panels, and the display drivers.
 
 To do that, a pol_flags field was added to drm_display_mode.
 
 Signed-off-by: Denis Carikli de...@eukrea.com
 ---
 ChangeLog v11-v12:
 - Rebased: This patch now applies against drm_modes.h
 - Rebased: It now uses the new DRM_MODE_FLAG_POL_DE flags defines names
 
 ChangeLog v10-v11:
 - Since the imx-drm won't be able to retrive its regulators
   from the device tree when using display-timings nodes,
   and that I was told that the drm simple-panel driver 
   already supported that, I then, instead, added what was
   lacking to make the eukrea displays work with the
   drm-simple-panel driver.
 
   That required a way to get back the display polarity
   informations from the imx-drm driver without affecting
   userspace.
 ---
  include/drm/drm_modes.h |8 
  1 file changed, 8 insertions(+)
 
 diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
 index 2dbbf99..b3789e2 100644
 --- a/include/drm/drm_modes.h
 +++ b/include/drm/drm_modes.h
 @@ -93,6 +93,13 @@ enum drm_mode_status {
  
  #define DRM_MODE_FLAG_3D_MAX DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF
  
 +#define DRM_MODE_FLAG_POL_PIXDATA_NEGEDGEBIT(1)
 +#define DRM_MODE_FLAG_POL_PIXDATA_POSEDGEBIT(2)
 +#define DRM_MODE_FLAG_POL_PIXDATA_PRESERVE   BIT(3)

What is the purpose of DRM_MODE_FLAG_POL_PIXDATA_PRESERVE?
If 'preserve' means 'ignore' we can set to zero negedge and posedge bits
instead of adding new bit. If it is something different please describe it.

 +#define DRM_MODE_FLAG_POL_DE_LOW BIT(4)
 +#define DRM_MODE_FLAG_POL_DE_HIGHBIT(5)
 +#define DRM_MODE_FLAG_POL_DE_PRESERVEBIT(6)
 +

The same comments here.

  struct drm_display_mode {
   /* Header */
   struct list_head head;
 @@ -144,6 +151,7 @@ struct drm_display_mode {
   int vrefresh;   /* in Hz */
   int hsync;  /* in kHz */
   enum hdmi_picture_aspect picture_aspect_ratio;
 + unsigned int pol_flags;

Adding field and macros description to the DocBook would be nice.

Regards
Andrzej

  };
  
  /* mode specified on the command line */
 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 07/12] drm: drm_display_mode: add signal polarity flags

[Andrzej Hajda]

Hi Denis,

Thanks for the patch.

On 03/13/2014 06:17 PM, Denis Carikli wrote:
 We need a way to pass signal polarity informations
   between DRM panels, and the display drivers.
 
 To do that, a pol_flags field was added to drm_display_mode.
 
 Signed-off-by: Denis Carikli de...@eukrea.com
 ---
 ChangeLog v10-v11:
 - Since the imx-drm won't be able to retrive its regulators
   from the device tree when using display-timings nodes,
   and that I was told that the drm simple-panel driver 
   already supported that, I then, instead, added what was
   lacking to make the eukrea displays work with the
   drm-simple-panel driver.
 
   That required a way to get back the display polarity
   informations from the imx-drm driver without affecting
   userspace.
 ---
  include/drm/drm_crtc.h |8 
  1 file changed, 8 insertions(+)
 
 diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
 index f764654..61a4fe1 100644
 --- a/include/drm/drm_crtc.h
 +++ b/include/drm/drm_crtc.h
 @@ -131,6 +131,13 @@ enum drm_mode_status {
  
  #define DRM_MODE_FLAG_3D_MAX DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF
  
 +#define DRM_MODE_FLAG_POL_PIXDATA_NEGEDGEBIT(1)
 +#define DRM_MODE_FLAG_POL_PIXDATA_POSEDGEBIT(2)
 +#define DRM_MODE_FLAG_POL_PIXDATA_PRESERVE   BIT(3)
 +#define DRM_MODE_FLAG_POL_DE_NEGEDGE BIT(4)
 +#define DRM_MODE_FLAG_POL_DE_POSEDGE BIT(5)
 +#define DRM_MODE_FLAG_POL_DE_PRESERVEBIT(6)

Could you add some description to these flags.
What are *_PRESERVE flags for?
Are those flags 1:1 compatible with respective 'videomode:flags'?
I guess DE flags should be rather DRM_MODE_FLAG_POL_DE_(LOW|HIGH), am I
right?

Regards
Andrzej


 +
  struct drm_display_mode {
   /* Header */
   struct list_head head;
 @@ -183,6 +190,7 @@ struct drm_display_mode {
   int vrefresh;   /* in Hz */
   int hsync;  /* in kHz */
   enum hdmi_picture_aspect picture_aspect_ratio;
 + unsigned int pol_flags;
  };
  
  static inline bool drm_mode_is_stereo(const struct drm_display_mode *mode)
 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel