| 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
| ||
|
Message-ID: <20200916160233.GB90122@roeck-us.net>
Date: Wed, 16 Sep 2020 09:02:33 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Xu Yilun <yilun.xu@...el.com>
Cc: jdelvare@...e.com, lee.jones@...aro.org,
linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
trix@...hat.com, matthew.gerlach@...ux.intel.com,
russell.h.weight@...el.com, lgoncalv@...hat.com, hao.wu@...el.com,
mdf@...nel.org
Subject: Re: [PATCH v2] hwmon: intel-m10-bmc-hwmon: add hwmon support for
Intel MAX 10 BMC
On Wed, Sep 16, 2020 at 02:48:58PM +0800, Xu Yilun wrote:
> On Tue, Sep 15, 2020 at 10:22:32PM -0700, Guenter Roeck wrote:
> > On 9/15/20 8:14 PM, Xu Yilun wrote:
> > > This patch adds hwmon functionality for Intel MAX 10 BMC chip. This BMC
> > > chip connects to a set of sensor chips to monitor current, voltage,
> > > thermal and power of different components on board. The BMC firmware is
> > > responsible for sensor data sampling and recording in shared registers.
> > > Host driver reads the sensor data from these shared registers and
> > > exposes them to users as hwmon interfaces.
> > >
> > > Signed-off-by: Xu Yilun <yilun.xu@...el.com>
> > > Signed-off-by: Wu Hao <hao.wu@...el.com>
> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@...ux.intel.com>
> > > Signed-off-by: Tom Rix <trix@...hat.com>
> >
> > Is that really the Sign-off path, or are some of those reviewers ? Just wondering.
>
> The original version of the product driver is initiated by Hao. I made
> this upstream version. And during our internal procedures for this
> upstream version, Matthew & Tom had comments and also sent fix patches
> for it. So I added Signed-off for them.
>
> After reading Documentation/process/submitting-patches.rst, my
> understanding is that if people contribute the whole or part of the
> code, a Signed-off-by could be added. Is that right?
>
Ok with me, just asking.
> >
> > > ---
> > > v2: add the Documentation
> > > refactor the code, provide static hwmon_channel_info
> > > remove Unnecessary hwmon-sysfs.h
> > > make the sensor data table const
> > > ---
> > > Documentation/hwmon/index.rst | 1 +
> > > Documentation/hwmon/intel-m10-bmc-hwmon.rst | 78 ++++++
> > > drivers/hwmon/Kconfig | 11 +
> > > drivers/hwmon/Makefile | 1 +
> > > drivers/hwmon/intel-m10-bmc-hwmon.c | 355 ++++++++++++++++++++++++++++
> > > 5 files changed, 446 insertions(+)
> > > create mode 100644 Documentation/hwmon/intel-m10-bmc-hwmon.rst
> > > create mode 100644 drivers/hwmon/intel-m10-bmc-hwmon.c
> > >
> > > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > > index a926f1a..4bcb1a7 100644
> > > --- a/Documentation/hwmon/index.rst
> > > +++ b/Documentation/hwmon/index.rst
> > > @@ -74,6 +74,7 @@ Hardware Monitoring Kernel Drivers
> > > ina209
> > > ina2xx
> > > ina3221
> > > + intel-m10-bmc-hwmon
> > > ir35221
> > > ir38064
> > > isl68137
> > > diff --git a/Documentation/hwmon/intel-m10-bmc-hwmon.rst b/Documentation/hwmon/intel-m10-bmc-hwmon.rst
> > > new file mode 100644
> > > index 0000000..3d148c6
> > > --- /dev/null
> > > +++ b/Documentation/hwmon/intel-m10-bmc-hwmon.rst
> > > @@ -0,0 +1,78 @@
> > > +.. SPDX-License-Identifier: GPL-2.0
> > > +
> > > +Kernel driver intel-m10-bmc-hwmon
> > > +=================================
> > > +
> > > +Supported chips:
> > > +
> > > + * Intel MAX 10 BMC for Intel PAC N3000
> > > +
> > > + Prefix: 'n3000bmc-hwmon'
> > > +
> > > +Author: Xu Yilun <yilun.xu@...el.com>
> > > +
> > > +
> > > +Description
> > > +-----------
> > > +
> > > +This driver adds the temperature, voltage, current and power reading
> > > +support for the Intel MAX 10 Board Management Controller (BMC) chip.
> > > +The BMC chip is integrated in some Intel Programmable Acceleration
> > > +Cards (PAC). It connects to a set of sensor chips to monitor the
> > > +sensor data of different components on the board. The BMC firmware is
> > > +responsible for sensor data sampling and recording in shared
> > > +registers. The host driver reads the sensor data from these shared
> > > +registers and exposes them to users as hwmon interfaces.
> > > +
> > > +The BMC chip is implemented using the Intel MAX 10 CPLD. It could be
> > > +reprogramed to some variants in order to support different Intel
> > > +PACs. The driver is designed to be able to distinguish between the
> > > +variants, but now it only supports the BMC for Intel PAC N3000.
> > > +
> > > +
> > > +Sysfs attributes
> > > +----------------
> > > +
> > > +The following attributes are supported:
> > > +
> > > +- Intel MAX 10 BMC for Intel PAC N3000:
> > > +
> > > +======================= =======================================================
> > > +tempX_input Temperature of the component (specified by tempX_label)
> > > +tempX_max Temperature maximum setpoint of the component
> > > +tempX_crit Temperature critical setpoint of the component
> > > +tempX_max_hyst Hysteresis for temperature maximum of the component
> > > +tempX_crit_hyst Hysteresis for temperature critical of the component
> > > +temp1_label "Board Temperature"
> > > +temp2_label "FPGA Die Temperature"
> > > +temp3_label "QSFP0 Temperature"
> > > +temp4_label "QSFP1 Temperature"
> > > +temp5_label "Retimer A Temperature"
> > > +temp6_label "Retimer A SerDes Temperature"
> > > +temp7_label "Retimer B Temperature"
> > > +temp8_label "Retimer B SerDes Temperature"
> > > +
> > > +inX_input Measured voltage of the component (specified by
> > > + inX_label)
> > > +in0_label "QSFP0 Supply Voltage"
> > > +in1_label "QSFP1 Supply Voltage"
> > > +in2_label "FPGA Core Voltage"
> > > +in3_label "12V Backplane Voltage"
> > > +in4_label "1.2V Voltage"
> > > +in5_label "12V AUX Voltage"
> > > +in6_label "1.8V Voltage"
> > > +in7_label "3.3V Voltage"
> > > +
> > > +currX_input Measured current of the component (specified by
> > > + currX_label)
> > > +curr1_label "FPGA Core Current"
> > > +curr2_label "12V Backplane Current"
> > > +curr3_label "12V AUX Current"
> > > +
> > > +powerX_input Measured power of the component (specified by
> > > + powerX_label)
> > > +power1_label "Board Power"
> > > +
> > > +======================= =======================================================
> > > +
> > > +All the attributes are read-only.
> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > index 8dc28b2..53af15c 100644
> > > --- a/drivers/hwmon/Kconfig
> > > +++ b/drivers/hwmon/Kconfig
> > > @@ -2064,6 +2064,17 @@ config SENSORS_XGENE
> > > If you say yes here you get support for the temperature
> > > and power sensors for APM X-Gene SoC.
> > >
> > > +config SENSORS_INTEL_M10_BMC_HWMON
> > > + tristate "Intel MAX10 BMC Hardware Monitoring"
> > > + depends on MFD_INTEL_M10_BMC
> > > + help
> > > + This driver provides support for the hardware monitoring functionality
> > > + on Intel MAX10 BMC chip.
> > > +
> > > + This BMC Chip is used on Intel FPGA PCIe Acceleration Cards (PAC). Its
> > > + sensors monitor various telemetry data of different components on the
> > > + card, e.g. board temperature, FPGA core temperature/voltage/current.
> > > +
> > > if ACPI
> > >
> > > comment "ACPI drivers"
> > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > > index a8f4b35..ba5a25a 100644
> > > --- a/drivers/hwmon/Makefile
> > > +++ b/drivers/hwmon/Makefile
> > > @@ -90,6 +90,7 @@ obj-$(CONFIG_SENSORS_IIO_HWMON) += iio_hwmon.o
> > > obj-$(CONFIG_SENSORS_INA209) += ina209.o
> > > obj-$(CONFIG_SENSORS_INA2XX) += ina2xx.o
> > > obj-$(CONFIG_SENSORS_INA3221) += ina3221.o
> > > +obj-$(CONFIG_SENSORS_INTEL_M10_BMC_HWMON) += intel-m10-bmc-hwmon.o
> > > obj-$(CONFIG_SENSORS_IT87) += it87.o
> > > obj-$(CONFIG_SENSORS_JC42) += jc42.o
> > > obj-$(CONFIG_SENSORS_K8TEMP) += k8temp.o
> > > diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
> > > new file mode 100644
> > > index 0000000..ce73545
> > > --- /dev/null
> > > +++ b/drivers/hwmon/intel-m10-bmc-hwmon.c
> > > @@ -0,0 +1,355 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Intel MAX 10 BMC HWMON Driver
> > > + *
> > > + * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
> > > + *
> > > + */
> > > +#include <linux/device.h>
> > > +#include <linux/hwmon.h>
> > > +#include <linux/mfd/intel-m10-bmc.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +struct m10bmc_sdata {
> > > + unsigned int reg_input;
> > > + unsigned int reg_max;
> > > + unsigned int reg_crit;
> > > + unsigned int reg_hyst;
> > > + unsigned int reg_min;
> > > + unsigned int multiplier;
> > > + const char *label;
> > > +};
> > > +
> > > +struct m10bmc_hwmon_board_data {
> > > + const struct m10bmc_sdata *temp;
> > > + const struct m10bmc_sdata *in;
> > > + const struct m10bmc_sdata *curr;
> > > + const struct m10bmc_sdata *power;
> > > + const struct hwmon_channel_info **hinfo;
> > > +};
> > > +
> > > +struct m10bmc_hwmon {
> > > + struct device *dev;
> > > + struct hwmon_chip_info chip;
> > > + char *hw_name;
> > > + struct intel_m10bmc *m10bmc;
> > > + struct m10bmc_hwmon_board_data *bdata;
> > > +};
> > > +
> > > +static const struct m10bmc_sdata n3000bmc_temp_tbl[] = {
> > > + { 0x100, 0x104, 0x108, 0x10c, 0x0, 500, "Board Temperature" },
> > > + { 0x110, 0x114, 0x118, 0x0, 0x0, 500, "FPGA Die Temperature" },
> > > + { 0x11c, 0x124, 0x120, 0x0, 0x0, 500, "QSFP0 Temperature" },
> > > + { 0x12c, 0x134, 0x130, 0x0, 0x0, 500, "QSFP1 Temperature" },
> > > + { 0x168, 0x0, 0x0, 0x0, 0x0, 500, "Retimer A Temperature" },
> > > + { 0x16c, 0x0, 0x0, 0x0, 0x0, 500, "Retimer A SerDes Temperature" },
> > > + { 0x170, 0x0, 0x0, 0x0, 0x0, 500, "Retimer B Temperature" },
> > > + { 0x174, 0x0, 0x0, 0x0, 0x0, 500, "Retimer B SerDes Temperature" },
> > > +};
> > > +
> > > +static const struct m10bmc_sdata n3000bmc_in_tbl[] = {
> > > + { 0x128, 0x0, 0x0, 0x0, 0x0, 1, "QSFP0 Supply Voltage" },
> > > + { 0x138, 0x0, 0x0, 0x0, 0x0, 1, "QSFP1 Supply Voltage" },
> > > + { 0x13c, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Voltage" },
> > > + { 0x144, 0x0, 0x0, 0x0, 0x0, 1, "12V Backplane Voltage" },
> > > + { 0x14c, 0x0, 0x0, 0x0, 0x0, 1, "1.2V Voltage" },
> > > + { 0x150, 0x0, 0x0, 0x0, 0x0, 1, "12V AUX Voltage" },
> > > + { 0x158, 0x0, 0x0, 0x0, 0x0, 1, "1.8V Voltage" },
> > > + { 0x15c, 0x0, 0x0, 0x0, 0x0, 1, "3.3V Voltage" },
> > > +};
> > > +
> > > +static const struct m10bmc_sdata n3000bmc_curr_tbl[] = {
> > > + { 0x140, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Current" },
> > > + { 0x148, 0x0, 0x0, 0x0, 0x0, 1, "12V Backplane Current" },
> > > + { 0x154, 0x0, 0x0, 0x0, 0x0, 1, "12V AUX Current" },
> > > +};
> > > +
> > > +static const struct m10bmc_sdata n3000bmc_power_tbl[] = {
> > > + { 0x160, 0x0, 0x0, 0x0, 0x0, 1000, "Board Power" },> +};
> > > +
> > > +static const struct hwmon_channel_info *n3000bmc_hinfo[] = {
> > > + HWMON_CHANNEL_INFO(temp,
> > > + HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> > > + HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> > > + HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > > + HWMON_T_LABEL,
> > > + HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > > + HWMON_T_LABEL,
> > > + HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > > + HWMON_T_LABEL,
> > > + HWMON_T_INPUT | HWMON_T_LABEL,
> > > + HWMON_T_INPUT | HWMON_T_LABEL,
> > > + HWMON_T_INPUT | HWMON_T_LABEL,
> > > + HWMON_T_INPUT | HWMON_T_LABEL),
> > > + HWMON_CHANNEL_INFO(in,
> > > + HWMON_I_INPUT | HWMON_I_LABEL,
> > > + HWMON_I_INPUT | HWMON_I_LABEL,
> > > + HWMON_I_INPUT | HWMON_I_LABEL,
> > > + HWMON_I_INPUT | HWMON_I_LABEL,
> > > + HWMON_I_INPUT | HWMON_I_LABEL,
> > > + HWMON_I_INPUT | HWMON_I_LABEL,
> > > + HWMON_I_INPUT | HWMON_I_LABEL,
> > > + HWMON_I_INPUT | HWMON_I_LABEL),
> > > + HWMON_CHANNEL_INFO(curr,
> > > + HWMON_C_INPUT | HWMON_C_LABEL,
> > > + HWMON_C_INPUT | HWMON_C_LABEL,
> > > + HWMON_C_INPUT | HWMON_C_LABEL),
> > > + HWMON_CHANNEL_INFO(power,
> > > + HWMON_P_INPUT | HWMON_P_LABEL),
> > > + NULL
> > > +};
> > > +
> > > +struct m10bmc_hwmon_board_data n3000bmc_hwmon_bdata = {
> >
> > static, and probably const
>
> Yes.
>
> >
> > > + .temp = n3000bmc_temp_tbl,
> > > + .in = n3000bmc_in_tbl,
> > > + .curr = n3000bmc_curr_tbl,
> > > + .power = n3000bmc_power_tbl,
> >
> > I would suggest to declare
> > const struct m10bmc_sdata *tables[hwmon_max];
> > and initialize with
> > .tables[hwmon_temp] = n3000bmc_temp_tbl,
> > .tables[hwmon_in] = n3000bmc_in_tbl,
> > and so on. That makes the structure a bit larger, but simplifies
> > the operational code (see below).
>
> Yes my concern is also the data size. But since it simplifies the code
> I'll follow the change.
>
Data size increase is minimal, especially since it reduces runtime
code size at the same time.
> >
> > > + .hinfo = n3000bmc_hinfo,
> > > +};
> > > +
> > > +static umode_t
> > > +m10bmc_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> > > + u32 attr, int channel)
> > > +{
> > > + return 0444;
> > > +}
> > > +
> > > +static const struct m10bmc_sdata *
> > > +find_sensor_data(struct m10bmc_hwmon *hw, enum hwmon_sensor_types type,
> > > + int channel)
> > > +{
> > > + const struct m10bmc_sdata *tbl;
> > > +
> > > + switch (type) {
> > > + case hwmon_temp:
> > > + tbl = hw->bdata->temp;
> > > + break;
> > > + case hwmon_in:
> > > + tbl = hw->bdata->in;
> > > + break;
> > > + case hwmon_curr:
> > > + tbl = hw->bdata->curr;
> > > + break;
> > > + case hwmon_power:
> > > + tbl = hw->bdata->power;
> > > + break;
> > > + default:
> > > + return ERR_PTR(-EOPNOTSUPP);
> > > + }
> >
> > Then you can use
> > tbl = hw->bdata->tables[type];
> Yes.
>
> >
> > > +
> > > + if (!tbl)
> > > + return ERR_PTR(-EOPNOTSUPP);
> > > +
> > > + return &tbl[channel];
> > > +}
> > > +
> > > +static int do_sensor_read(struct m10bmc_hwmon *hw,
> > > + const struct m10bmc_sdata *data,
> > > + unsigned int regoff, long *val)
> > > +{
> > > + unsigned int regval;
> > > + int ret;
> > > +
> > > + ret = m10bmc_sys_read(hw->m10bmc, regoff, ®val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /*
> > > + * BMC Firmware will return 0xdeadbeef if the sensor value is invalid
> > > + * at that time. This usually happens on sensor channels which connect
> > > + * to external pluggable modules, e.g. QSFP temperature and voltage.
> > > + * When the QSFP is unplugged from cage, driver will get 0xdeadbeef
> > > + * from their registers.
> > > + */
> > > + if (regval == 0xdeadbeef)
> > > + return -EBUSY;
> >
> > Is this appropriate ? The description above would suggest differently.
> > -ENODATA, maybe ? Also, are those module hot pluggable ? If not,
>
> I could change to -ENODATA.
>
> Those modules are hot pluggable. QSFP is a compact, hot-pluggable optical
> module for ethernet connection. The real sensors are embedded in QSFP
> modules, not on the cages of the board. So at running time, if the QSFP
> is plugged in the cage, we have valid sensor data, if it is unplugged,
> we have 0xdeadbeef.
>
-ENODATA seems to be better in this case.
> > it may be more appropriate to detect the status in the in_visible function
> > and not create affected attributes in the first place.
> >
> > > +
> > > + *val = regval * data->multiplier;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int m10bmc_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> > > + u32 attr, int channel, long *val)
> > > +{
> > > + struct m10bmc_hwmon *hw = dev_get_drvdata(dev);
> > > + unsigned int reg = 0, reg_hyst = 0;
> > > + const struct m10bmc_sdata *data;
> > > + long hyst, value;
> > > + int ret;
> > > +
> > > + data = find_sensor_data(hw, type, channel);
> > > + if (IS_ERR(data))
> > > + return PTR_ERR(data);
> > > +
> > > + switch (type) {
> > > + case hwmon_temp:
> > > + switch (attr) {
> > > + case hwmon_temp_input:
> > > + reg = data->reg_input;
> > > + break;
> > > + case hwmon_temp_max_hyst:
> > > + reg_hyst = data->reg_hyst;
> > > + fallthrough;
> > > + case hwmon_temp_max:
> > > + reg = data->reg_max;
> > > + break;
> > > + case hwmon_temp_crit_hyst:
> > > + reg_hyst = data->reg_hyst;
> > > + fallthrough;
> > > + case hwmon_temp_crit:
> > > + reg = data->reg_crit;
> > > + break;
> > > + default:
> > > + return -EOPNOTSUPP;
> > > + }
> > > + break;
> > > + case hwmon_in:
> > > + switch (attr) {
> > > + case hwmon_in_input:
> > > + reg = data->reg_input;
> > > + break;
> > > + case hwmon_in_max:
> > > + reg = data->reg_max;
> > > + break;
> > > + case hwmon_in_crit:
> > > + reg = data->reg_crit;
> > > + break;
> > > + case hwmon_in_min:
> > > + reg = data->reg_min;
> > > + break;
> > > + default:
> > > + return -EOPNOTSUPP;
> > > + }
> > > + break;
> > > + case hwmon_curr:
> > > + switch (attr) {
> > > + case hwmon_curr_input:
> > > + reg = data->reg_input;
> > > + break;
> > > + case hwmon_curr_max:
> > > + reg = data->reg_max;
> > > + break;
> > > + case hwmon_curr_crit:
> > > + reg = data->reg_crit;
> > > + break;
> > > + default:
> > > + return -EOPNOTSUPP;
> > > + }
> > > + break;
> > > + case hwmon_power:
> > > + switch (attr) {
> > > + case hwmon_power_input:
> > > + reg = data->reg_input;
> > > + break;
> > > + default:
> > > + return -EOPNOTSUPP;
> > > + }
> > > + break;
> > > + default:
> > > + return -EOPNOTSUPP;
> > > + }
> > > +
> > > + if (!reg)
> > > + return -EOPNOTSUPP;
> > > +
> >
> > That can't happen with the code above. reg is always initialized.
> > All other cases already returned -EOPNOTSUPP.
>
> In the n3000bmc_xxx_tbl, we mark the reg as 0 if the attr is not supported,
> this should be aligned with the hwmon_channel_info. And it has to be
> ensured manually.
>
> If everything is fine, this can't happen. But if developers made mistake
> when inputing data, e.g. the HWMON_X_XXX bit is set but reg in table is 0,
> then we may got confusing value. This is the intention I added the
> check.
>
Ok, makes sense.
> >
> > > + ret = do_sensor_read(hw, data, reg, &value);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (reg_hyst) {
> > > + ret = do_sensor_read(hw, data, reg_hyst, &hyst);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + value -= hyst;
> >
> > Is that also correct for _min attributes ? Normally I'd assume that the hysteresis
> > for those is larger than the base attribute value.
>
> Mm.. the hysteresis attr value should be larger than min attr.
>
> But now the code expose no _min_hyst attr. So is it OK now?
>
Ah, I see. Yes, that is ok.
Thanks,
Guenter
> >
> > > + }
> > > +
> > > + *val = value;
> > > +
> > > + return ret;
> >
> > ret is always 0 here -> return 0;
>
> I'll change it.
>
> >
> > > +}
> > > +
> > > +static int m10bmc_hwmon_read_string(struct device *dev,
> > > + enum hwmon_sensor_types type,
> > > + u32 attr, int channel, const char **str)
> > > +{
> > > + struct m10bmc_hwmon *hw = dev_get_drvdata(dev);
> > > + const struct m10bmc_sdata *data;
> > > +
> > > + data = find_sensor_data(hw, type, channel);
> > > + if (IS_ERR(data))
> > > + return PTR_ERR(data);
> > > +
> > > + *str = data->label;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct hwmon_ops m10bmc_hwmon_ops = {
> > > + .is_visible = m10bmc_hwmon_is_visible,
> > > + .read = m10bmc_hwmon_read,
> > > + .read_string = m10bmc_hwmon_read_string,
> > > +};
> > > +
> > > +static int m10bmc_hwmon_probe(struct platform_device *pdev)
> > > +{
> > > + const struct platform_device_id *id = platform_get_device_id(pdev);
> > > + struct intel_m10bmc *m10bmc = dev_get_drvdata(pdev->dev.parent);
> > > + struct device *hwmon_dev, *dev = &pdev->dev;
> > > + struct m10bmc_hwmon *hw;
> > > + int i;
> > > +
> > > + if (!id || !id->driver_data) {
> >
> > That can not really happen.
>
> I see. We will not fall-back to driver name match if pdrv->id_table is
> provided.
>
> Thanks,
> Yilun
>
> >
> > > + dev_err(dev, "Failed to get board data\n");
> > > + return -ENODEV;
> > > + }
> > > +
> > > + hw = devm_kzalloc(dev, sizeof(*hw), GFP_KERNEL);
> > > + if (!hw)
> > > + return -ENOMEM;
> > > +
> > > + hw->dev = dev;
> > > + hw->m10bmc = m10bmc;
> > > + hw->bdata = (struct m10bmc_hwmon_board_data *)id->driver_data;
> > > +
> > > + hw->chip.info = hw->bdata->hinfo;
> > > + hw->chip.ops = &m10bmc_hwmon_ops;
> > > +
> > > + hw->hw_name = devm_kstrdup(dev, id->name, GFP_KERNEL);
> > > + if (!hw->hw_name)
> > > + return -ENOMEM;
> > > +
> > > + for (i = 0; hw->hw_name[i]; i++)
> > > + if (hwmon_is_bad_char(hw->hw_name[i]))
> > > + hw->hw_name[i] = '_';
> > > +
> > > + hwmon_dev = devm_hwmon_device_register_with_info(dev, hw->hw_name,
> > > + hw, &hw->chip, NULL);
> > > + return PTR_ERR_OR_ZERO(hwmon_dev);
> > > +}
> > > +
> > > +static const struct platform_device_id intel_m10bmc_hwmon_ids[] = {
> > > + {
> > > + .name = "n3000bmc-hwmon",
> > > + .driver_data = (unsigned long)&n3000bmc_hwmon_bdata,
> > > + },
> > > + { }
> > > +};
> > > +
> > > +static struct platform_driver intel_m10bmc_hwmon_driver = {
> > > + .probe = m10bmc_hwmon_probe,
> > > + .driver = {
> > > + .name = "intel-m10-bmc-hwmon",
> > > + },
> > > + .id_table = intel_m10bmc_hwmon_ids,
> > > +};
> > > +module_platform_driver(intel_m10bmc_hwmon_driver);
> > > +
> > > +MODULE_DEVICE_TABLE(platform, intel_m10bmc_hwmon_ids);
> > > +MODULE_AUTHOR("Intel Corporation");
> > > +MODULE_DESCRIPTION("Intel MAX 10 BMC hardware monitor");
> > > +MODULE_LICENSE("GPL");
> > >
Powered by blists - more mailing lists