Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> 於 2024年12月30日 週一 下午3:34寫道：
>
...
> >> 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.
> >> 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)
> >>> +{
...
>
> 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
