[PATCH 4/4] drm/tinydrm: Add RePaper e-ink driver

[Noralf Trønnes]

Den 07.06.2017 14.14, skrev Emil Velikov:
> On 7 June 2017 at 12:52, Noralf Trønnes <noralf at tronnes.org> wrote:
>> Hi Emil,
>>
>>
>> Den 07.06.2017 11.30, skrev Emil Velikov:
>>> Hi Noralf,
>>>
>>> Small drive-by comment, noticed while going through my morning coffee.
>>> By no means a full review.
>>>
>>> On 2 June 2017 at 12:49, Noralf Trønnes <noralf at tronnes.org> wrote:
>>>
>>>
>>>> +/* pixels on display are numbered from 1 */
>>>> +static void repaper_all_pixels(struct repaper_epd *epd, u8 **pp,
>>>> +                              const u8 *data, u8 fixed_value, const u8
>>>> *mask,
>>>> +                              enum repaper_stage stage)
>>>> +{
>>>> +       unsigned int b;
>>>> +
>>>> +       for (b = epd->width / 8; b > 0; b--) {
>>>> +               if (data) {
>>>> +                       u16 pixels = repaper_interleave_bits(data[b -
>>>> 1]);
>>>> +                       u16 pixel_mask = 0xffff;
>>>> +
>>>> +                       if (mask) {
>>>> +                               u16 pixel_mask =
>>>> repaper_interleave_bits(mask[b - 1]);
>>>> +
>>>> +                               pixel_mask = (pixel_mask ^ pixels) &
>>>> 0x5555;
>>>> +                               pixel_mask |= pixel_mask << 1;
>>> The second u16 pixel_mask seems very suspicious - likely a copy/paste
>>> mistake ?
>>
>> Actually it's a correct copy. The same pattern is used in three places.
>> I did look at the datasheet to try and understand what was going on,
>> but the current and previous frame is applied in normal and inverted
>> form in several stages, the pixels are converted to black/white/nochange
>> and then I lost track of it all.
>> So I decided that the Pervasive people who wrote the driver probably
>> knew what they where doing. At least it works!
>>
> What I meant was:
>
> Within the 'if (mask)' block you have a second, shadowing declaration
> of pixel_mask.
> Hence all the calculation done within is discarded and not used
> further down in the code.

Ah, I see, it's indeed lost and useless.
I'll see what happens if I don't shadow the variable.
Thanks.

Noralf.