Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220330190846.13997-1-wens@kernel.org> <20220330190846.13997-4-wens@kernel.org>
In-Reply-To: <20220330190846.13997-4-wens@kernel.org>
From:   Javier Martinez Canillas <javier@dowhile0.org>
Date:   Fri, 1 Apr 2022 12:02:38 +0200
Message-ID: <CABxcv=nFBmgQaoG6u_kW2v60yhPSi3wH_MSd7T9tS0cXqvCNKQ@mail.gmail.com>
Subject: Re: [PATCH 3/4] drm: ssd130x: Support page addressing mode
To:     Chen-Yu Tsai <wens@kernel.org>
Cc:     Javier Martinez Canillas <javierm@redhat.com>,
        Maxime Ripard <mripard@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>, devicetree@vger.kernel.org,
        Chen-Yu Tsai <wens@csie.org>,
        Linux Kernel <linux-kernel@vger.kernel.org>,
        dri-devel <dri-devel@lists.freedesktop.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Mar 30, 2022 at 9:09 PM Chen-Yu Tsai <wens@kernel.org> wrote:
>
> From: Chen-Yu Tsai <wens@csie.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@redhat.com>

Best regards,
Javier