From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.6 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D2DA0C2D0A3 for ; Tue, 3 Nov 2020 09:27:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7963A223BD for ; Tue, 3 Nov 2020 09:27:40 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="cZEWxAHf" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727282AbgKCJ1g (ORCPT ); Tue, 3 Nov 2020 04:27:36 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56326 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726013AbgKCJ1g (ORCPT ); Tue, 3 Nov 2020 04:27:36 -0500 Received: from mail-pf1-x444.google.com (mail-pf1-x444.google.com [IPv6:2607:f8b0:4864:20::444]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BB7D9C0613D1; Tue, 3 Nov 2020 01:27:35 -0800 (PST) Received: by mail-pf1-x444.google.com with SMTP id b3so13699758pfo.2; Tue, 03 Nov 2020 01:27:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=+oevH5NOXWY8mC/dfPuihrEtVh3WiYap02zfyjnacds=; b=cZEWxAHfEiuSTnTcJQ15sdqWmH2t5Z5yIoDZvD1nnKmv6iWFq9gVbTdAmM+CGFwNCj AmkoT6d5o8Gk+CRN5DsRS9egoLOWg6k6vW3oVCTgC1TnHFN+qSbLiUrjLcadgs4conZ1 elvawYaQXYi+C5/5fv5ezoD/jKAC32fb+pH4iTP51fg6cVls1KqEXDk8da9vSJ8dU6/z qIr4RvW9RQvhcMHbwpULFhUXQY6niynhQQs2ixvIBulRsjV+X+mSWwxpcdK9R/nSyWpw BN4qIslUqG9paEzL8dbG5ClXa69lkQlMWbJPPtRJ6/eG6LccffJW5AoB1rLtHEqzO74p AobA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=+oevH5NOXWY8mC/dfPuihrEtVh3WiYap02zfyjnacds=; b=AwZdWHwxpMrqguD4vZJ8qhfAGqPrZMBxZy5tdsGomT2N6IPmP+AQfN6FkOa4p7Luln ThKzUHNYApDb0BOSM3rWSedG+Y7Bmo2+47kXvVXzHtMhiGuhwuL5mNN6KGpicq6QFGpa gCc4lqZadG6vIIfK5Q6olYn5PhzYmGy7zQ8TxV7ItyYr6Cqsh9B6FASI94Nb8Z1cVJSV VkD0BmQHHs4GSoafuCZSu8fBsRmInwZPD4hltGjPT2vY5+R8IU+UW3+XTTALK/avgH18 /xSIvgxApx844HT4oDr5FB24DOVIyjebDDTDmbze5TwNvRpTt5/MWo8DSboaRsIPieAW MbLQ== X-Gm-Message-State: AOAM533ghdk5jca/LBLbwdnvmW31lO8xo5bXVml25rQdfIXOBVIeU33f /ppk0+bwh9KnVA+clnRgAKo96aqbRLzJaOIQ8d4= X-Google-Smtp-Source: ABdhPJwO3dn0tLoNPhZ4KT89FW3uSmGbx6Nx0h1SKcagkBJoNF4BJOhGU4ulCbvx/vsiAPgChLADh19c9ZU5aiyJxeg= X-Received: by 2002:a17:90a:430b:: with SMTP id q11mr2874342pjg.129.1604395655266; Tue, 03 Nov 2020 01:27:35 -0800 (PST) MIME-Version: 1.0 References: <20201025005916.64747-1-luka.kovacic@sartura.hr> <20201025005916.64747-3-luka.kovacic@sartura.hr> In-Reply-To: From: Andy Shevchenko Date: Tue, 3 Nov 2020 11:27:19 +0200 Message-ID: Subject: Re: [PATCH v7 2/6] drivers: mfd: Add a driver for IEI WT61P803 PUZZLE MCU To: Luka Kovacic Cc: Linux Kernel Mailing List , linux-hwmon@vger.kernel.org, Linux LED Subsystem , devicetree , Lee Jones , Pavel Machek , Dan Murphy , Rob Herring , Jean Delvare , Guenter Roeck , =?UTF-8?B?TWFyZWsgQmVow7pu?= , Luka Perkov , Robert Marko Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-hwmon@vger.kernel.org On Tue, Nov 3, 2020 at 1:15 AM Luka Kovacic wrote: > On Mon, Nov 2, 2020 at 12:18 PM Andy Shevchenko > wrote: > > On Sun, Nov 1, 2020 at 3:22 PM Luka Kovacic wrote: > > > On Mon, Oct 26, 2020 at 11:54 PM Andy Shevchenko > > > wrote: > > > > On Sun, Oct 25, 2020 at 3:59 AM Luka Kovacic wrote: ... > > > > > + /* Response format: > > > > > + * (IDX RESPONSE) > > > > > + * 0 @ > > > > > + * 1 O > > > > > + * 2 S > > > > > + * 3 S > > > > > + * ... > > > > > + * 5 AC Recovery Status Flag > > > > > + * ... > > > > > + * 10 Power Loss Recovery > > > > > + * ... > > > > > + * 19 Power Status (system power on method) > > > > > + * 20 XOR checksum > > > > > + */ > > > > > > > > Shouldn't be rather defined data structure for response? > > > > > > Every response, apart from the standard headers and a checksum > > > at the end is completely different and I don't see a good way to > > > standardize that in some other way. > > > > And that's my point. Provide data structures for all responses you are > > taking care of. > > It will be way better documentation and understanding of this IPC. > > Okay, I'll improve handling of these in the next patchset. > Should I make a generic header structure for the common parts and > define the common responses somewhere centrally? Yes, something like TCP/IP headers have. This will immediately show how good/bad was design of this IPC protocol (as a side effect, but gives a good hint on how layers of messages are looking ) > Then I can check those just as you suggested. > > For the variable ones I can reuse the generic header structure and just > use the specific values as I would do normally. ... > > > > > + if (!(resp_buf[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START && > > > > > + resp_buf[1] == IEI_WT61P803_PUZZLE_CMD_RESPONSE_OK && > > > > > + resp_buf[2] == IEI_WT61P803_PUZZLE_CHECKSUM_RESPONSE_OK)) { > > > > > + ret = -EPROTO; > > > > > + goto err; > > > > > + } > > > > > > > > I think it would be better to define data structure for replies and > > > > then check would be as simple as memcmp(). > > > > > > I'd keep this as is, because the replies are different a lot of the times. > > > Especially when the reply isn't just an ACK. > > > > How do you know the type of the reply? Can't you provide a data > > structure which will have necessary fields to recognize this? > > > > It can be recognized by the specific header of the reply. > I will separate the header and the checksum into some kind of a generic > structure, but as the content itself is just an arbitrary array of characters > I cannot generalize that sensibly for every type of a reply there is. > > Anyway, I agree it would be good to define the common responses... Yep, something to look like a structure with a payload. ... > Can I output the versions, the firmware build info and only print the baud > rate when an error occurs? What you think is crucial. I'm not against it, I'm just pointing to the way of shrinking as much as possible. Otherwise, move messages to debug level (but that shouldn't be many in the production driver). -- With Best Regards, Andy Shevchenko