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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOoeyxXOa-JK1-wRn0hsD1nuTOLs-5NBv5-YswOSS1JJbGmU_A@mail.gmail.com>
Date: Mon, 30 Dec 2024 16:47:20 +0800
From: Ming Yu <a0282524688@...il.com>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc: linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org, 
	linux-i2c@...r.kernel.org, linux-can@...r.kernel.org, netdev@...r.kernel.org, 
	linux-watchdog@...r.kernel.org, linux-hwmon@...r.kernel.org, 
	linux-rtc@...r.kernel.org, tmyu0@...oton.com, lee@...nel.org, 
	linus.walleij@...aro.org, brgl@...ev.pl, andi.shyti@...nel.org, 
	mkl@...gutronix.de, andrew+netdev@...n.ch, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	wim@...ux-watchdog.org, linux@...ck-us.net, jdelvare@...e.com, 
	alexandre.belloni@...tlin.com, linux-usb@...r.kernel.org
Subject: Re: [PATCH v4 1/7] mfd: Add core driver for Nuvoton NCT6694

Vincent Mailhol <mailhol.vincent@...adoo.fr> 於 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