On Mon, 13 May 2024, Ilpo Järvinen wrote:
> On Mon, 13 May 2024, Hans de Goede wrote:
>
> > Hi Ilpo,
> >
> > Thank you for the review.
> >
> > On 5/13/24 10:34 AM, Ilpo Järvinen wrote:
> > > On Sun, 12 May 2024, Hans de Goede wrote:
>
> > >> + u8 resp[3];
> > >> +
> > >> + set_brightness[2] = brightness;
> > >> + set_brightness[3] = dell_uart_checksum(set_brightness, 3);
> > >
> > > Also, couldn't these be accessed through a struct to eliminate most of the
> > > magic indexes?
> >
> > With the checksum at the end, this would require a VLA in the middle
> > of the struct (get_version return buffer contains more then 1 dat byte)
> > We could treat the checksum as an extra data byte, but then we are
> > mixing struct usage for some fields + using an array of bytes
> > approach for the data + checksum. For consistency I prefer to just
> > stick with one approach which means using the array of bytes approach
> > for everything.
>
> Ok.
>
> > >> + const u8 get_brightness[] = { 0x6A, 0x0C, 0x89 };
> > >> + u8 resp[4];
> > >> + int ret;
> > >> +
> > >> + ret = dell_uart_bl_command(dell_bl, get_brightness, ARRAY_SIZE(get_brightness),
> > >> + resp, ARRAY_SIZE(resp));
> > >> + if (ret)
> > >> + return ret;
> > >> +
> > >> + if (resp[0] != 4) {
> > >
> > > sizeof(resp)
> >
> > Ack.
> >
> > > but isn't this already checked when reading it??
> >
> > No dell_uart_bl_receive() checks that the response will fit in the supplied
> > buffer and that it has a valid checksum but the controller may send a
> > response smaller then the passed in buffer and it will actually do this for
> > the get_version command.
>
> Ah, I see now that it checked against constant rather than the actual
> value.
>
> > >> + dev_warn(dell_bl->dev, "Bytes received out of band, dropping them.\n");
> > >> + return len;
> > >> + }
> > >> +
> > >> + for (i = 0; i < len; i++) {
> > >> + dell_bl->resp[dell_bl->resp_idx] = data[i];
> > >> +
> > >> + switch (dell_bl->resp_idx) {
> > >> + case 0: /* Length byte */
> > >> + dell_bl->resp_len = dell_bl->resp[0];
> > >> + if (dell_bl->resp_len < DELL_BL_MIN_RESP_SIZE) {
> > >> + dev_err(dell_bl->dev, "Response length too small %d < %d\n",
> > >> + dell_bl->resp_len, DELL_BL_MIN_RESP_SIZE);
> > >> + dell_bl->status = -EIO;
> > >> + goto wakeup;
> > >> + } else if (dell_bl->resp_len > dell_bl->resp_max_len) {
> > >> + dev_err(dell_bl->dev, "Response length too big %d > %d\n",
> > >> + dell_bl->resp_len, dell_bl->resp_max_len);
> > >> + dell_bl->status = -EIO;
> > >> + goto wakeup;
> > >> + }
> > >> + break;
> > >> + case 1: /* CMD byte */
> > >> + if (dell_bl->resp[1] != dell_bl->pending_cmd) {
> > >> + dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n",
> > >> + dell_bl->resp[1], dell_bl->pending_cmd);
> > >> + dell_bl->status = -EIO;
> > >> + goto wakeup;
> > >> + }
> > >> + break;
> > >> + }
> > >> +
> > >> + dell_bl->resp_idx++;
> > >> + if (dell_bl->resp_idx < dell_bl->resp_len)
> > >> + continue;
> > >> +
> > >> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1);
> > >> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) {
> > >> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n",
> > >> + dell_bl->resp[dell_bl->resp_len - 1], csum);
> > >> + dell_bl->status = -EIO;
> > >> + goto wakeup;
> > >> + }
> > >
> > > Why is the checksum calculation and check inside the loop??
> >
> > The loop iterates over received bytes, which may contain extra data
> > after the response, the:
> >
> > dell_bl->resp_idx++;
> > if (dell_bl->resp_idx < dell_bl->resp_len)
> > continue;
> >
> > continues looping until we have received all the expected bytes. So here, past this
> > check, we are are at the point where we have a complete response and then we verify it.
> >
> > And on successful verification wake-up any waiters.
>
> So effectively you want to terminate the loop on two conditions here:
>
> a) dell_bl->resp_idx == dell_bl->resp_len (complete frame)
> a) if i == len (not yet received a full frame)
>
> Why not code those rather than the current goto & continue madness?
>
> Then, after the loop, you can test:
>
> if (dell_bl->resp_idx == dell_bl->resp_len) {
> // calc checksum, etc.
> }
>
> ?
+ the len has been received check:
if (dell_bl->resp_idx && dell_bl->resp_idx == dell_bl->resp_len) {
...
}
--
i.
