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

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

On 30/12/2024 at 17:47, Ming Yu wrote:
> Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2024年12月30日 週一 下午3:34寫道：

(...)

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

I was more thinking of exposing:

  int nct6694_read_msg(struct nct6694 *nct6694,
  		       struct nct6694_cmd_header *cmd_hd,
  		       void *buf);

and:

  int nct6694_write_msg(struct nct6694 *nct6694,
  			struct nct6694_cmd_header *cmd_hd,
  			void *buf);

and then the different modules fill the cmd_hd argument themselves and
directly call one of those two functions with no intermediaries.

But your solution is also acceptable. The core issue is the artificial
packing and depacking of the cmd and sel pair. As long as this core
issue is resolved and as long as the ugly concatenation macro can be
removed, I think it is OK. Choose the approach you prefer :)


Yours sincerely,
Vincent Mailhol