| 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
| ||
|
Message-ID: <CAOoeyxVXWAFC4ZL5Hr7H4+Vk8b86Qf2FBY0vGWa7NWPWHo6MWg@mail.gmail.com>
Date: Mon, 2 Dec 2024 17:30:54 +0800
From: Ming Yu <a0282524688@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: tmyu0@...oton.com, lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, cmo@...exis.com, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] iio: temperature: Add Nuvoton NCT7718W support
Dear Jonathan,
Thank you for your comments,
I will move the driver to HWMON.
Best regards,
Ming
Jonathan Cameron <jic23@...nel.org> 於 2024年12月1日 週日 上午4:39寫道：
>
> On Tue, 26 Nov 2024 15:40:05 +0800
> Ming Yu <a0282524688@...il.com> wrote:
>
> > This patch adds support for the Nuvoton NCT7718W temperature sensor.
> >
> Hi Ming, I'll give this a quick look only as I suspect you will end
> up moving over to hwmon.
>
> Thanks,
>
> Jonathan
>
> > Signed-off-by: Ming Yu <tmyu0@...oton.com>
> ...
>
> > diff --git a/drivers/iio/temperature/nct7718.c b/drivers/iio/temperature/nct7718.c
> > new file mode 100644
> > index 000000000000..60624b3de629
> > --- /dev/null
> > +++ b/drivers/iio/temperature/nct7718.c
> > @@ -0,0 +1,505 @@
>
> > +struct nct7718_data {
> > + struct i2c_client *client;
> > + struct mutex lock;
> Locks need a comment to say what data they are protecting.
>
> > + u16 status_mask;
> > +};
>
> > +static int nct7718_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + int state)
> > +{
> > + struct nct7718_data *data = iio_priv(indio_dev);
> > + unsigned int status_mask;
> > + int ret;
> > +
> > + switch (chan->channel2) {
> > + case IIO_MOD_TEMP_AMBIENT:
> > + if (dir == IIO_EV_DIR_RISING)
> > + status_mask = NCT7718_MSK_LTH;
> > + break;
> > + case IIO_MOD_TEMP_OBJECT:
> > + if (dir == IIO_EV_DIR_RISING)
> > + status_mask = NCT7718_MSK_RT1H;
> > + else
> > + status_mask = NCT7718_MSK_RT1L;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + mutex_lock(&data->lock);
> > + ret = i2c_smbus_read_byte_data(data->client, NCT7718_ALERTMASK_REG);
> > + mutex_unlock(&data->lock);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (state)
> > + ret &= ~status_mask;
> > + else
> > + ret |= status_mask;
> I would not bother with this sort of alignment. It tends to be fragile longer
> term as code gets modified and doesn't make much difference to readablility.
>
> > +
> > + return i2c_smbus_write_byte_data(data->client, NCT7718_ALERTMASK_REG,
> > + data->status_mask = ret);
> > +}
>
> > +
> > +static int nct7718_write_thresh(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int val, int val2)
> > +{
> > + struct nct7718_data *data = iio_priv(indio_dev);
> > + struct i2c_client *client = data->client;
> > + u8 msb_reg, lsb_reg;
> > + s16 thresh;
> > + int ret, s_val;
> > +
> > + switch (chan->channel2) {
> > + case IIO_MOD_TEMP_AMBIENT:
> > + val = clamp_val(val, NCT7718_LOCAL_TEMP_MIN,
> > + NCT7718_LOCAL_TEMP_MAX);
> > +
> > + if (dir == IIO_EV_DIR_RISING) {
> > + return i2c_smbus_write_byte_data(client,
> > + NCT7718_LT_HALERT_REG,
> > + val);
> > + }
> > + break;
> > + case IIO_MOD_TEMP_OBJECT:
> > + s_val = (val < 0) ? ((val * 1000000) - val2) :
> > + ((val * 1000000) + val2);
> > +
> > + s_val = clamp_val(s_val, NCT7718_REMOTE_TEMP_MIN_MICRO,
> > + NCT7718_REMOTE_TEMP_MAX_MICRO);
> > +
> > + if (dir == IIO_EV_DIR_RISING) {
> > + msb_reg = NCT7718_RT1_HALERT_MSB_REG;
> > + lsb_reg = NCT7718_RT1_HALERT_LSB_REG;
> > + } else {
> > + msb_reg = NCT7718_RT1_LALERT_MSB_REG;
> > + lsb_reg = NCT7718_RT1_LALERT_LSB_REG;
> > + }
> > +
> > + thresh = (s16)(s_val / (1000000 >> 3));
> > + ret = i2c_smbus_write_byte_data(client,
> > + msb_reg, thresh >> 3);
> > + if (ret < 0)
> > + return ret;
> > + return i2c_smbus_write_byte_data(client,
> > + lsb_reg, thresh << 5);
> > + default:
> > + break;
> return -EINVAL; and drop return below.
>
> > + }
> > +
> > + return -EINVAL;
> > +}
> >
> > +
> > +static bool nct7718_check_id(struct i2c_client *client)
> > +{
> > + int chip_id, vendor_id, device_id;
> > +
> > + chip_id = i2c_smbus_read_byte_data(client, NCT7718_VID_REG);
> > + if (chip_id < 0)
> > + return false;
> > +
> > + vendor_id = i2c_smbus_read_byte_data(client, NCT7718_VID_REG);
> > + if (vendor_id < 0)
> > + return false;
> > +
> > + device_id = i2c_smbus_read_byte_data(client, NCT7718_DID_REG);
> > + if (device_id < 0)
> > + return false;
> > +
> > + return (chip_id == NCT7718_CHIP_ID &&
> > + vendor_id == NCT7718_VENDOR_ID &&
> > + device_id == NCT7718_DEVICE_ID);
> As below. Don't treat this failing as an error. It is an error if
> you can't read anything however.
>
> > +}
> > +
> > +static int nct7718_chip_config(struct nct7718_data *data)
> > +{
> > + int ret;
> > +
> > + /* Enable MSK_LTH, MSK_RT1H, and MSK_RT1L to monitor alarm */
> Code makes this fairly obvoius.
>
> > + ret = i2c_smbus_read_byte_data(data->client,
> > + NCT7718_ALERTMASK_REG);
> > + if (ret < 0)
> > + return ret;
> > + data->status_mask = ret;
> > + data->status_mask &= ~(NCT7718_MSK_LTH |
> > + NCT7718_MSK_RT1H |
> > + NCT7718_MSK_RT1L);
> > +
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + NCT7718_ALERTMASK_REG,
> > + data->status_mask);
>
> Perhaps consider regmap for it's richer set of functionality.
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Config ALERT Mode Setting to comparator mode */
>
> Ideally (like here) the code should be self explanatory so you don't
> need comments. When that is the case the comment is both unnecessary
> and a source of possible future confusion if the code changes and we
> fail to keep the comment in sync.
>
> > + return i2c_smbus_write_byte_data(data->client,
> > + NCT7718_ALERTMODE_REG,
> > + NCT7718_MOD_COMP);
> > +}
> > +
> > +static int nct7718_probe(struct i2c_client *client)
> > +{
> > + struct nct7718_data *data;
> > + struct iio_dev *indio_dev;
> > + int ret;
> > +
> > + if (!nct7718_check_id(client)) {
> > + dev_err(&client->dev, "No NCT7718 device\n");
>
> If you get an ID missmatch, it is fine to print a message, but carry on anyway.
> This is necessary because DT has fallback compatibles to allow for newer devices
> working with older drivers. That only works if we believe the firmware and
> ignore a mismatched ID.
>
> > + return -ENODEV;
> > + }
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + data = iio_priv(indio_dev);
> > + data->client = client;
> > + mutex_init(&data->lock);
>
> For new code prefer
> ret = devm_mutex_init()
> if (ret)
> return ret;
>
> > +
> > + indio_dev->name = client->name;
>
> client->name doesn't always end up as the part number which is what
> we need here. Just put "nct7718" in here directly.
>
> Some drivers do this wrong and we can't fix them without breaking
> userspace, but we don't want to introduce more.
>
> > + indio_dev->channels = nct7718_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(nct7718_channels);
> > + indio_dev->info = &nct7718_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + ret = nct7718_chip_config(data);
> > + if (ret)
> > + return ret;
> > +
> > + if (client->irq) {
> > + ret = devm_request_threaded_irq(&client->dev,
> > + client->irq,
> > + NULL,
> > + nct7718_alert_handler,
> > + IRQF_TRIGGER_FALLING |
> > + IRQF_ONESHOT,
> > + "nct7718", indio_dev);
> > + if (ret) {
> > + dev_err(&client->dev, "Failed to request irq!\n");
> > + return ret;
> > + }
> > + }
> > +
> > + return devm_iio_device_register(&client->dev, indio_dev);
> > +}
Powered by blists - more mailing lists