On 10/5/20 4:05 PM, Joel Stanley wrote:
> On Thu, 1 Oct 2020 at 12:32, Patrick Williams <patrick@xxxxxxxxx> wrote:
>>
>> On Wed, Sep 30, 2020 at 05:26:57PM -0500, Lancelot wrote:
>>> From: Lancelot Kao <lancelot.kao@xxxxxxxxxxx>
>>>
>>> Add SMPMPro-hwmon driver to monitor Ampere CPU/Memory/VR via an
>>> i2c interface of the CPU's smpmpro management device.
>>>
>>> Signed-off-by: Xiaopeng XP Chen <xiao-peng.chen@xxxxxxxxxx>
>>> Signed-off-by: Lancelot Kao <lancelot.kao@xxxxxxxxxxx>
>>
>> Nice work at adding this driver.
>>
>> It does look like you've missed CC'ing upstream though. Was this
>> intentional? (linux-hwmon@xxxxxxxxxxxxxxx)
>
> As Patrick mentioned, let's review this on the upstream list.
>
I can not really comment, not having seen the entire patch.
However, looking it up on the OpenBMC patchwork, couple of
high level comments:
- Label attributes are handled by the hwmon core. Label attributes
outside the core are unacceptable.
- There is no discussion about the non-standard attributes, nor a description
of those, nor an explanation for why they are needed (as hwmon sysfs attributes)
or what they report. This is unacceptable.
Besides, many of those attributes - say, gpi22_input, which seems to report
the content of GPI_WDT_STS_REG, suggesting association with a watchdog -
seem inappropriate for a hwmon driver to start with. It seems like the
hwmon driver us used as catch-all driver for this chip. Sorry,
that is an absolute no-go.
If this patch is supposed to be submitted as upstream patch, I would suggest
to read and follow the guidance in Documentation/hwmon/submitting-patches.rst.
Guenter
> Cheers,
>
> Joel
>
>>
>>> +/* Capability Registers */
>>> +#define TEMP_SENSOR_SUPPORT_REG 0x05
>>> +#define PWR_SENSOR_SUPPORT_REG 0x06
>>> +#define VOLT_SENSOR_SUPPORT_REG 0x07
>>> +#define OTHER_CAP_REG 0x08
>>> +#define CORE_CLUSTER_CNT_REG 0x0B
>>> +#define SYS_CACHE_PCIE_CNT_REG 0x0C
>>> +#define SOCKET_INFO_REG 0x0D
>>
>> There seems to be some sporatic indentation throughout all the #defines
>> in this file, where it appears you attempted to align the values. Make
>> sure you have tabs set to 8-step spacing for kernel code.
>>
>>> +static void smpmpro_init_device(struct i2c_client *client,
>>> + struct smpmpro_data *data)
>>> +{
>>> + u16 ret;
>>> +
>>> + ret = i2c_smbus_read_word_swapped(client, TEMP_SENSOR_SUPPORT_REG);
>>> + if (ret < 0)
>>> + return;
>>> + data->temp_support_regs = ret;
>>
>> i2c_smbus_read_word_swapped returns a s32 even though you're looking for
>> a u16. By setting `ret` to u16 you've caused two problems:
>>
>> * You are immediately truncating -ERRNO values into a u16 so that
>> you are unable to differentiate values like 0xFFFFFFFF as a
>> register value and -1 as an errno.
>>
>> * The if condition here can never be true, so you're never catching
>> error conditions. (An u16 can never be negative, so ret < 0 can
>> never be true.)
>>
>> This issue occurs throughout the driver.
>>
>>> +static int smpmpro_read_temp(struct device *dev, u32 attr, int channel,
>>> + long *val)
>>> +{
>>> + struct smpmpro_data *data = dev_get_drvdata(dev);
>>> + struct i2c_client *client = data->client;
>>> + int ret;
>>
>> You might want a sized int on this one? Repeated in most other
>> functions.
>>
>>> +static int smpmpro_read_power(struct device *dev, u32 attr, int channel,
>>> + long *val)
>>> +{
>>> + struct smpmpro_data *data = dev_get_drvdata(dev);
>>> + struct i2c_client *client = data->client;
>>> + int ret, ret_mw;
>>> + int ret2 = 0, ret2_mw = 0;
>>
>> Any reason to not initialize ret/ret_mw? By it being different from
>> ret2/ret2_mw it makes me question "is this ok?", which spends more time
>> in review.
>>
>>> +static int smpmpro_i2c_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>> ...
>>> + /* Initialize the Altra SMPMPro chip */
>>> + smpmpro_init_device(client, data);
>>
>> I didn't see anything in the smpmpro_init_device function, but is there
>> anything you can or should do to ensure this device really is an
>> SMPMPro rather than exclusively relying on the device tree compatible?
>>
>>> +static struct i2c_driver smpmpro_driver = {
>>> + .class = I2C_CLASS_HWMON,
>>> + .probe = smpmpro_i2c_probe,
>>> + .driver = {
>>> + .name = "smpmpro",
>>> + },
>>> + .id_table = smpmpro_i2c_id,
>>> +};
>>> +
>>> +module_i2c_driver(smpmpro_driver);
>>
>> Are you missing the .of_match_table inside .driver? Is that necessary
>> or useful for your use? I'm not sure if you can have device tree
>> entries that automatically instantiate the hwmon driver otherwise.
>>
>> --
>> Patrick Williams
