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

...

quoted

quoted

If I understand correctly, your device is an USB device, so shouldn't it
be under

  drivers/usb/mfd/nct6694.c

?

I understand, but there is no drivers/usb/mfd/ directory, I believe my
device is similar to dln2.c and viperboard.c, which is why I placed it
under drivers/mfd/

Well, at the end, this is not my tree. Maybe I am saying something silly
here? I am fine to defer this problem to the more relevant people. If
the maintainers from the linux-usb mailing list are happy like you did,
then so am I.

Understood.

quoted

quoted

At the moment, I see no USB maintainers in CC (this is why I added
linux-usb myself). By putting it in the correct folder, the
get_maintainers.pl will give you the correct list of persons to put in copy.

Okay, I will add CC to linux-usb from now on.

Ack.

quoted

quoted

The same comment applies to the other modules. For example, I would
expect to see the CAN module under:

  drivers/net/can/usb/nct6694_canfd.c

Understood! I will move the can driver to drivers/net/can/usb/ in v5.

Ack.

(...)

quoted

quoted

quoted

+int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
+                  u16 length, void *buf)
+{

...

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.

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)


Thanks,
Ming