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.
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
