On 30/12/2024 at 17:47, Ming Yu wrote:

Vincent Mailhol [off-list ref] 於 2024年12月30日 週一 下午3:34寫道：

(...)

quoted

If the two bytes may be used separately or in combination, then I think
it is better to describe this in your structure. Something like this:

  struct nct6694_cmd_header {
        u8 rsv1;
        u8 mod;
        union {
                __le16 offset;
                struct {
                        u8 cmd;
                        u8 sel;
                }; __packed
        } __packed;
        u8 hctrl;
        u8 rsv2;
        __le16 len;
  } __packed;

Sorry for forgetting to list the structure in last mail, but the
revised structure is same as yours.

quoted

Then, your prototype becomes:

  int nct6694_read_msg(struct nct6694 *nct6694,
                       struct nct6694_cmd_header *cmd_hd,
                       void *buf)

If the caller needs to pass an offset:

  void foo(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length)
  {
        struct nct6694_cmd_header cmd_hd = { 0 };

        cmd_hd.mod = mod;
        cmd_hd.offset = cpu_to_le16(offset);
        cmd_hd.len = cpu_to_le16(length);

        nct6694_read_msg(nct6694, &cmd_hd, NULL);
  }

If the caller needs to pass a cmd and sel pair:

  void foo(struct nct6694 *nct6694, u8 mod, u8 cmd, u8 sel, u16 length)
  {
        struct nct6694_cmd_header cmd_hd = { 0 };

        cmd_hd.mod = mod;
        cmd_hd.cmd = cmd;
        cmd_hd.sel = sel;
        cmd_hd.len = cpu_to_le16(length);

        nct6694_read_msg(nct6694, &cmd_hd, NULL);
  }

This way, no more cmd and sel concatenation/deconcatenation and no
conditional if/else logic.

cmd_hd.hctrl (and other similar fields which are common to everyone) may
be set in nct6694_read_msg().

Understood, that means I need to export these four function, right?
  - int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u8 cmd,
                         u8 sel, u16 length, void *buf);
  - int nct6694_read_rpt(struct nct6694 *nct6694, u8 mod, u16 offset,
                         u16 length, void *buf);
  - int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u8 cmd,
                          u8 sel, u16 length, void *buf);
  - int nct6694_write_rpt(struct nct6694 *nct6694, u8 mod, u16 offset,
                          u16 length, void *buf);

Both nct6694_read_msg() and nct6694_read_rpt() call
nct6694_read(struct nct6694 *nct6694, struct nct6694_cmd_header
cmd_hd, void *buf),
then nct6694_write_msg() and nct6694_write_rpt() call
nct6694_write(struct nct6694 *nct6694, struct nct6694_cmd_header
cmd_hd, void *buf).
(nct6694_read/nct6694_write is origin nct6694_read_msg/nct6694_write_msg)

I was more thinking of exposing:

  int nct6694_read_msg(struct nct6694 *nct6694,
  		       struct nct6694_cmd_header *cmd_hd,
  		       void *buf);

and:

  int nct6694_write_msg(struct nct6694 *nct6694,
  			struct nct6694_cmd_header *cmd_hd,
  			void *buf);

and then the different modules fill the cmd_hd argument themselves and
directly call one of those two functions with no intermediaries.

But your solution is also acceptable. The core issue is the artificial
packing and depacking of the cmd and sel pair. As long as this core
issue is resolved and as long as the ugly concatenation macro can be
removed, I think it is OK. Choose the approach you prefer :)


Yours sincerely,
Vincent Mailhol