netdev - Re: [PATCH v3 3/7] i2c: Add Nuvoton NCT6694 I2C support

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: <CAOoeyxX475tHNqoejX=DcY2ow2+rPc=_qXuX0O5AGumLPFoQGA@mail.gmail.com>
Date: Thu, 26 Dec 2024 10:06:22 +0800
From: Ming Yu <a0282524688@...il.com>
To: Andi Shyti <andi.shyti@...nel.org>
Cc: tmyu0@...oton.com, lee@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl, 
	mkl@...gutronix.de, mailhol.vincent@...adoo.fr, 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-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
Subject: Re: [PATCH v3 3/7] i2c: Add Nuvoton NCT6694 I2C support

Dear Andi,

Thank you for your comments,

Andi Shyti <andi.shyti@...nel.org> 於 2024年12月26日 週四 上午8:43寫道：
>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/nct6694.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +/* Host interface */
>
> What does it mean "Host interface"?
>
> > +#define NCT6694_I2C_MOD              0x03
> > +
> > +/* Message Channel*/
> > +/* Command 00h */
>
> This comments are meaningless, either make them clearer or remove
> them.
>
> > +#define NCT6694_I2C_CMD0_OFFSET      0x0000  /* OFFSET = SEL|CMD */
>
> I find this comment quite meaningless. Can you please make it
> clearer?
>

I have already updated these structures and comments following
suggestions from other reviewers, and I plan to include the changes in
the next patch submission.

> > +#define NCT6694_I2C_CMD0_LEN 0x90
> > +
> > +enum i2c_baudrate {
> > +     I2C_BR_25K = 0,
> > +     I2C_BR_50K,
> > +     I2C_BR_100K,
> > +     I2C_BR_200K,
> > +     I2C_BR_400K,
> > +     I2C_BR_800K,
> > +     I2C_BR_1M
> > +};
> > +
> > +struct __packed nct6694_i2c_cmd0 {
> > +     u8 port;
> > +     u8 br;
> > +     u8 addr;
> > +     u8 w_cnt;
> > +     u8 r_cnt;
> > +     u8 rsv[11];
> > +     u8 write_data[0x40];
> > +     u8 read_data[0x40];
> > +};
> > +
> > +struct nct6694_i2c_data {
> > +     struct nct6694 *nct6694;
> > +     struct i2c_adapter adapter;
> > +     unsigned char *xmit_buf;
>
> why isn't this a nct6694_i2c_cmd0 type?
>

Fix it in v4.

> > +     unsigned char port;
> > +     unsigned char br;
> > +};
> > +
> > +static int nct6694_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> > +{
> > +     struct nct6694_i2c_data *data = adap->algo_data;
> > +     struct nct6694_i2c_cmd0 *cmd = (struct nct6694_i2c_cmd0 *)data->xmit_buf;
> > +     int ret, i;
> > +
> > +     for (i = 0; i < num ; i++) {
> > +             struct i2c_msg *msg_temp = &msgs[i];
> > +
> > +             memset(data->xmit_buf, 0, sizeof(struct nct6694_i2c_cmd0));
> > +
> > +             if (msg_temp->len > 64)
> > +                     return -EPROTO;
> > +             cmd->port = data->port;
> > +             cmd->br = data->br;
> > +             cmd->addr = i2c_8bit_addr_from_msg(msg_temp);
> > +             if (msg_temp->flags & I2C_M_RD) {
> > +                     cmd->r_cnt = msg_temp->len;
> > +                     ret = nct6694_write_msg(data->nct6694, NCT6694_I2C_MOD,
> > +                                             NCT6694_I2C_CMD0_OFFSET,
> > +                                             NCT6694_I2C_CMD0_LEN,
> > +                                             cmd);
> > +                     if (ret < 0)
> > +                             return 0;
>
> why not return ret?
>

Fix it in v4.

> > +
> > +                     memcpy(msg_temp->buf, cmd->read_data, msg_temp->len);
> > +             } else {
> > +                     cmd->w_cnt = msg_temp->len;
> > +                     memcpy(cmd->write_data, msg_temp->buf, msg_temp->len);
> > +                     ret = nct6694_write_msg(data->nct6694, NCT6694_I2C_MOD,
> > +                                             NCT6694_I2C_CMD0_OFFSET,
> > +                                             NCT6694_I2C_CMD0_LEN,
> > +                                             cmd);
> > +                     if (ret < 0)
> > +                             return 0;
> > +             }
> > +     }
> > +
> > +     return num;
> > +}
> > +
> > +static u32 nct6694_func(struct i2c_adapter *adapter)
> > +{
> > +     return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL);
>
> parenthesis are not needed.
>

Fix it in v4.

> > +}
>
> ...
>
> > +static struct platform_driver nct6694_i2c_driver = {
> > +     .driver = {
> > +             .name   = "nct6694-i2c",
> > +     },
> > +     .probe          = nct6694_i2c_probe,
> > +     .remove         = nct6694_i2c_remove,
> > +};
> > +
> > +module_platform_driver(nct6694_i2c_driver);
>
> what I meant in v1 is to try using module_auxiliary_driver().
> Check, e.g., i2c-ljca.c or i2c-keba.c.
>

I think the NCT6694  is an MCU-based device, and the current
implementation is as an MFD driver. Are you suggesting it should
instead be implemented as an auxiliary device driver? If so, would
that mean all related drivers need to be revised accordingly?

Best regards,
Ming