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

[Guenter Roeck <linux@roeck-us.net>]

On 8/18/19 7:20 PM, Joel Stanley wrote:
> On Fri, 16 Aug 2019 at 10:19, John Wang <wangzqbj@inspur.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@inspur.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@jms.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,
>> +};
>