lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABxcv=nFBmgQaoG6u_kW2v60yhPSi3wH_MSd7T9tS0cXqvCNKQ@mail.gmail.com>
Date:   Fri, 1 Apr 2022 12:02:38 +0200
From:   Javier Martinez Canillas <javier@...hile0.org>
To:     Chen-Yu Tsai <wens@...nel.org>
Cc:     Javier Martinez Canillas <javierm@...hat.com>,
        Maxime Ripard <mripard@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>, devicetree@...r.kernel.org,
        Chen-Yu Tsai <wens@...e.org>,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH 3/4] drm: ssd130x: Support page addressing mode

On Wed, Mar 30, 2022 at 9:09 PM Chen-Yu Tsai <wens@...nel.org> wrote:
>
> From: Chen-Yu Tsai <wens@...e.org>
>
> On the SINO WEALTH SH1106, which is mostly compatible with the SSD1306,
> only the basic page addressing mode is supported. This addressing mode
> is not as easy to use compared to the currently supported horizontal
> addressing mode, as the page address has to be set prior to writing
> out each page, and each page must be written out separately as a result.
> Also, there is no way to force the column address to wrap around early,
> thus the column address must also be reset for each page to be accurate.
>

Thanks for including this explanation, it's very informative.

> Add support for this addressing mode, with a flag to choose it. This
> flag is designed to be set from the device info data structure, but
> can be extended to be explicitly forced on through a device tree
> property if such a need arises.
>

Agreed. Unless an OLED controller supports both page addressing modes,
I don't see what could be an advantage of having that property in the
device tree. And even if that's the case, we could just always make it
default to use horizontal addressing mode.

[snip]

> +/* Set address range for horizontal/vertical addressing modes */

Thanks for adding these comments.

>  static int ssd130x_set_col_range(struct ssd130x_device *ssd130x,
>                                  u8 col_start, u8 cols)
>  {
> @@ -166,6 +173,26 @@ static int ssd130x_set_page_range(struct ssd130x_device *ssd130x,
>         return 0;
>  }
>
> +/* Set page and column start address for page addressing mode */
> +static int ssd130x_set_page_pos(struct ssd130x_device *ssd130x,
> +                               u8 page_start, u8 col_start)
> +{
> +       int ret;
> +       u32 page, col_low, col_high;
> +
> +       page = SSD130X_START_PAGE_ADDRESS |
> +              SSD130X_START_PAGE_ADDRESS_SET(page_start);
> +       col_low = SSD130X_PAGE_COL_START_LOW |
> +                 SSD130X_PAGE_COL_START_SET(col_start & 0xf);
> +       col_high = SSD130X_PAGE_COL_START_HIGH |
> +                  SSD130X_PAGE_COL_START_SET((col_start >> 4) & 0xf);

Maybe instead we should define
SSD130X_PAGE_COL_START_{HIGH,LOW}_{MASK,SET} to be consistent with how
the other fields are set and not using bitwise operations explicitly
here ?

Other than that, the patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@...hat.com>

Best regards,
Javier

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ