> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Monday, December 5, 2022 8:15 PM
> To: Xingjiang Qiao <nanpuyue@xxxxxxxxx>; Jean Delvare
> <jdelvare@xxxxxxxx>
> Cc: Michael Shych <michaelsh@xxxxxxxxxx>; linux-hwmon@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/2] hwmon: (emc2305) fix unable to probe
> emc2301/2/3/4
>
> On 12/5/22 09:36, Xingjiang Qiao wrote:
> > The definitions of 'EMC2305_REG_PRODUCT_ID' and
> 'EMC2305_REG_DEVICE'
> > are both '0xfd', they actually return the same value, but the values
> > returned by emc2301/2/3/4/5 are different, so probe emc2301/2/3/4 will
> > fail, This patch fixes that.
> >
>
> This solves the wrong problem. The check for REG_DEVICE should simply be
> removed instead (EMC2305_REG_PRODUCT_ID and EMC2305_REG_DEVICE
> are both 0xfd). On top of that, moving the functionality of
> emc2305_identify() does not improve code quality (quite the contrary) and is
> thus not acceptable.
Thanks for catching the problem.
Generalization for EMC2301/2/3 was added later to the code.
I agree with Guenter that check of EMC2305_REG_DEVICE should be just removed.
>
> > The second parameter of 'emc2305_probe' is actually useless, it is
> > more appropriate to use 'probe_new' instead of 'probe' here.
> >
>
> This would be a second patch. Besides, this change is already queued in
> hwmon-next for v6.2.
>
> Guenter
>
> > Signed-off-by: Xingjiang Qiao <nanpuyue@xxxxxxxxx>
> > ---
> > drivers/hwmon/emc2305.c | 58 ++++++++++++++---------------------------
> > 1 file changed, 19 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c index
> > aa1f25add0b6..4df84065cbfb 100644
> > --- a/drivers/hwmon/emc2305.c
> > +++ b/drivers/hwmon/emc2305.c
> > @@ -21,9 +21,7 @@ emc2305_normal_i2c[] = { 0x27, 0x2c, 0x2d, 0x2e, 0x2f,
> 0x4c, 0x4d, I2C_CLIENT_EN
> > #define EMC2305_FAN_MAX 0xff
> > #define EMC2305_FAN_MIN 0x00
> > #define EMC2305_FAN_MAX_STATE 10
> > -#define EMC2305_DEVICE 0x34
> > #define EMC2305_VENDOR 0x5d
> > -#define EMC2305_REG_PRODUCT_ID 0xfd
> > #define EMC2305_TACH_REGS_UNUSE_BITS 3
> > #define EMC2305_TACH_CNT_MULTIPLIER 0x02
> > #define EMC2305_TACH_RANGE_MIN 480
> > @@ -488,43 +486,14 @@ static const struct hwmon_chip_info
> emc2305_chip_info = {
> > .info = emc2305_info,
> > };
> >
> > -static int emc2305_identify(struct device *dev) -{
> > - struct i2c_client *client = to_i2c_client(dev);
> > - struct emc2305_data *data = i2c_get_clientdata(client);
> > - int ret;
> > -
> > - ret = i2c_smbus_read_byte_data(client,
> EMC2305_REG_PRODUCT_ID);
> > - if (ret < 0)
> > - return ret;
> > -
> > - switch (ret) {
> > - case EMC2305:
> > - data->pwm_num = 5;
> > - break;
> > - case EMC2303:
> > - data->pwm_num = 3;
> > - break;
> > - case EMC2302:
> > - data->pwm_num = 2;
> > - break;
> > - case EMC2301:
> > - data->pwm_num = 1;
> > - break;
> > - default:
> > - return -ENODEV;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > -static int emc2305_probe(struct i2c_client *client, const struct
> > i2c_device_id *id)
> > +static int emc2305_probe(struct i2c_client *client)
> > {
> > struct i2c_adapter *adapter = client->adapter;
> > struct device *dev = &client->dev;
> > struct emc2305_data *data;
> > struct emc2305_platform_data *pdata;
> > int vendor, device;
> > + int pwm_num;
> > int ret;
> > int i;
> >
> > @@ -536,20 +505,31 @@ static int emc2305_probe(struct i2c_client *client,
> const struct i2c_device_id *
> > return -ENODEV;
> >
> > device = i2c_smbus_read_byte_data(client, EMC2305_REG_DEVICE);
> > - if (device != EMC2305_DEVICE)
> > + switch (device) {
> > + case EMC2305:
> > + pwm_num = 5;
> > + break;
> > + case EMC2303:
> > + pwm_num = 3;
> > + break;
> > + case EMC2302:
> > + pwm_num = 2;
> > + break;
> > + case EMC2301:
> > + pwm_num = 1;
> > + break;
> > + default:
> > return -ENODEV;
> > + }
> >
> > data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > if (!data)
> > return -ENOMEM;
> >
> > i2c_set_clientdata(client, data);
> > + data->pwm_num = pwm_num;
> > data->client = client;
> >
> > - ret = emc2305_identify(dev);
> > - if (ret)
> > - return ret;
> > -
> > pdata = dev_get_platdata(&client->dev);
> > if (pdata) {
> > if (!pdata->max_state || pdata->max_state >
> EMC2305_FAN_MAX_STATE)
> > @@ -604,10 +584,10 @@ static void emc2305_remove(struct i2c_client
> > *client)
> >
> > static struct i2c_driver emc2305_driver = {
> > .class = I2C_CLASS_HWMON,
> > + .probe_new = emc2305_probe,
> > .driver = {
> > .name = "emc2305",
> > },
> > - .probe = emc2305_probe,
> > .remove = emc2305_remove,
> > .id_table = emc2305_ids,
> > .address_list = emc2305_normal_i2c,
