linux-kernel - Re: [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver

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] [day] [month] [year] [list]

Message-ID: <8c6a287f-49f0-bb36-71d0-2bce8eb19ff9@roeck-us.net>
Date:   Sun, 18 Aug 2019 19:56:46 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Joel Stanley <joel@....id.au>, John Wang <wangzqbj@...pur.com>
Cc:     Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>,
        linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        duanzhijia01@...pur.com, Lei YU <mine260309@...il.com>
Subject: Re: [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply
 driver

On 8/18/19 7:20 PM, Joel Stanley wrote:
> On Fri, 16 Aug 2019 at 10:19, John Wang <wangzqbj@...pur.com> wrote:
>>
>> Add the driver to monitor Inspur Power System power supplies
>> with hwmon over pmbus.
>>
>> This driver adds sysfs attributes for additional power supply data,
>> including vendor, model, part_number, serial number,
>> firmware revision, hardware revision, and psu mode(active/standby).
>>
>> Signed-off-by: John Wang <wangzqbj@...pur.com>
> 
>> +static const struct i2c_device_id ipsps_id[] = {
>> +       { "inspur_ipsps1", 0 },
> 
> Convention would be to use "ipsps" here, instead of "vendor_device"?

ipsps1, but good catch.

>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ipsps_id);
>> +
>> +static const struct of_device_id ipsps_of_match[] = {
>> +       { .compatible = "inspur,ipsps1" },
>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(of, ipsps_of_match);
> 
> Do we need the of match table? I thought the match on the device name
> from the i2c table would be enough. I will defer to Guenter here
> though.
> 

Strictly speaking it is unnecessary, but it is kind of customary to have it.
The automatic detection also only works if the i2c device ID would be "ipsps1",
without vendor ID embedded.

I would recomment to have both, but name the i2c device "ipsps1" as you suggested,
for consistency.

I'll also have to check if we need of_match_ptr below in the assignment of
of_match_table. Probably not, but it would save a few bytes if CONFIG_OF
is not enabled.

Thanks,
Guenter


> Assuming the device tables are okay:
> 
> Reviewed-by: Joel Stanley <joel@....id.au>
> 
> Cheers,
> 
> Joel
> 
>> +
>> +static struct i2c_driver ipsps_driver = {
>> +       .driver = {
>> +               .name = "inspur-ipsps",
>> +               .of_match_table = ipsps_of_match,
>> +       },
>> +       .probe = ipsps_probe,
>> +       .remove = pmbus_do_remove,
>> +       .id_table = ipsps_id,
>> +};
>