Re: [PATCH v4 1/7] mfd: Add core driver for Nuvoton NCT6694

[Vincent Mailhol <mailhol.vincent@wanadoo.fr>]

On 30/12/2024 at 15:32, Ming Yu wrote:
> Dear Vincent,
> 
> Thank you for your comments,
> 
> Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2024年12月27日 週五 下午11:34寫道：

(...)

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

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

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

(...)

>>> +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...
> 
>> 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