On 30/12/2024 at 15:32, Ming Yu wrote:

Dear Vincent,

Thank you for your comments,

Vincent Mailhol [off-list ref] 於 2024年12月27日 週五 下午11:34寫道：

(...)

quoted

quoted

 obj-$(CONFIG_MFD_MC13XXX)    += mc13xxx-core.o
 obj-$(CONFIG_MFD_MC13XXX_SPI)        += mc13xxx-spi.o
 obj-$(CONFIG_MFD_MC13XXX_I2C)        += mc13xxx-i2c.o

diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
new file mode 100644
index 000000000000..0f31489ef9fa
--- /dev/null
+++ b/drivers/mfd/nct6694.c

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.

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

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

+int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
+                  u16 length, void *buf)
+{
+     union nct6694_usb_msg *msg = nct6694->usb_msg;
+     int tx_len, rx_len, ret;
+
+     guard(mutex)(&nct6694->access_lock);
+
+     memset(msg, 0, sizeof(*msg));
+
+     /* Send command packet to USB device */
+     msg->cmd_header.mod = mod;
+     msg->cmd_header.cmd = offset & 0xFF;
+     msg->cmd_header.sel = (offset >> 8) & 0xFF;

In the other modules, you have some macros to combine together the cmd
and the sel (selector, I guess?). For example from nct6694_canfd.c:

  #define NCT6694_CAN_DELIVER(buf_cnt)  \
        ((((buf_cnt) & 0xFF) << 8) | 0x10)      /* CMD|SEL */

And here, you split them again. So what was the point to combine those
together in the first place?

Due to these two bytes may used to OFFSET in report channel for other
modules(gpio, hwmon), I will modify them below...

quoted

Can't you just pass both the cmd and the sel as two separate argument?
Those cmd and sel concatenation macros are too confusing.

Also, if you are worried of having too many arguments in
nct6694_read_msg(), you may just directly pass a pointer to a struct
nct6694_cmd_header instead of all the arguments separately.

...
in mfd/nct6694.c
inline struct nct6694_cmd_header nct6694_init_cmd(u8 mod, u8 cmd, u8 sel,
                                                  u16 offset, u16 length)
{
        struct nct6694_cmd_header header;

        header.mod = mod;
        header.cmd = cmd;
        header.sel = sel;
        header.offset = cpu_to_le16(offset);

I am not sure how this is supposed to work. If the both the offset and
the cmd/sel pair occupies the same slot in memory, then the offset would
just overwrite what you just put in the cmd and sel fields.

        header.len = cpu_to_le16(length);

        return header;
}
EXPORT_SYMBOL(nct6694_init_cmd);

int nct6694_read_msg(struct nct6694 *nct6694, struct nct6694_cmd_header *header,
                     void *buf)
{
        union nct6694_usb_msg *msg = nct6694->usb_msg;
        ...
        msg->cmd_header.mod = header->mod;
        msg->cmd_header.hctrl = NCT6694_HCTRL_GET;
        msg->cmd_header.len = header->len;
        if (msg->cmd_header.mod == 0xFF) {
                msg->cmd_header.offset = header->offset;
        } else {
                msg->cmd_header.cmd = header->cmd;
                msg->cmd_header.sel = header->sel;
        }
        ...
}
(also apply to nct6694_write_msg)

in other drivers, for example: gpio-nct6694.c
        struct nct6694_cmd_header cmd;
        int ret;

        guard(mutex)(&data->lock);

        cmd = nct6694_init_cmd(NCT6694_GPIO_MOD, 0, 0,
                               NCT6694_GPO_DIR + data->group,
                               sizeof(data->reg_val));

        ret = nct6694_read_msg(data->nct6694, &cmd, &data->reg_val);
        if (ret < 0)
                return ret;

Do you think this approach would be better?

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;

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().

(...)


Yours sincerely,
Vincent Mailhol