Re: [PATCH v1] modify adm1266 blackbox access flow

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/25/24 01:51, ChiangBrian 江泳緻 TAO wrote:
From: Brian Chiang<chiang.brian@xxxxxxxxxxxx>

Modify adm1266 blackbox access flow  by only accessing the latest index of data
to speed up the access

Signed-off-by: Brian Chiang<chiang.brian@xxxxxxxxxxxx>

This patch is not acceptable. There must be any "#if 0" in upstream
code. Yes, I know, there are already 1,419 instances of it, but that doesn't
make it better.

On top of that, changing driver functionality like this is not
acceptable either. Add a new nvmem file if there is a desire to only
read the last entry, and rework the code to avoid duplication.
Alternatively, if the blackbox contents do not change, you could cache
its contents locally in the driver to avoid having to read it each time
the file is accessed.

Either case, your patch is corrupt. Please run it through checkpatch and you'll
see what I mean.

Guenter

---
  drivers/hwmon/pmbus/adm1266.c | 12 ++++++++++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index ec5f932fc6f0..d79b2208de86 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -352,6 +352,7 @@ static int adm1266_nvmem_read_blackbox(struct
adm1266_data *data, u8 *read_buff)
     char index;
     u8 buf[5];
     int ret;
+    char latest_index = 0;

     ret = i2c_smbus_read_block_data(data->client, ADM1266_BLACKBOX_INFO, buf);
     if (ret < 0)
@@ -359,10 +360,16 @@ static int adm1266_nvmem_read_blackbox(struct
adm1266_data *data, u8 *read_buff)

     if (ret != 4)
         return -EIO;
-
+   latest_index = buf[2];
     record_count = buf[3];

-   for (index = 0; index < record_count; index++) {
+    /*get latest index of blackbox data*/
+    ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1,
&latest_index, read_buff);
+    if (ret < 0)
+        return ret;
+    return 0;
+#if 0 /*comment out the original one , this dump all the blackboxes ,
very time-consuming*/
+    for (index = 0; index < record_count; index++) {
         ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1,
&index, read_buff);
         if (ret < 0)
             return ret;
@@ -374,6 +381,7 @@ static int adm1266_nvmem_read_blackbox(struct
adm1266_data *data, u8 *read_buff)
     }

     return 0;
+#endif
  }

  static int adm1266_nvmem_read(void *priv, unsigned int offset, void
*val, size_t bytes)





[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux