On 30/12/2024 at 15:32, Ming Yu wrote:
> Dear Vincent,
>
> Thank you for your comments,
>
> Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> 於 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
