Message ID | 20200225141229.5424-1-wsa@the-dreams.de (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
Series | macintosh: therm_windtunnel: fix regression when instantiating devices | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (65b2623f395a4e25ab3ff4cff1c9c7623619a22d) |
snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 90 lines checked |
snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
On Tue, Feb 25, 2020 at 03:41:22PM +0100, John Paul Adrian Glaubitz wrote: > Hello! > > On 2/25/20 3:12 PM, Wolfram Sang wrote: > > Adding the Debian-PPC List to reach further people maybe willing to > > test. > > This might be related [1]. IIUC, this is the same as https://bugzilla.kernel.org/show_bug.cgi?id=199471. I don't think my patch helps here.
Wolfram Sang <wsa@the-dreams.de> writes: > Removing attach_adapter from this driver caused a regression for at > least some machines. Those machines had the sensors described in their > DT, too, so they didn't need manual creation of the sensor devices. The > old code worked, though, because manual creation came first. Creation of > DT devices then failed later and caused error logs, but the sensors > worked nonetheless because of the manually created devices. > > When removing attach_adaper, manual creation now comes later and loses > the race. The sensor devices were already registered via DT, yet with > another binding, so the driver could not be bound to it. > > This fix refactors the code to remove the race and only manually creates > devices if there are no DT nodes present. Also, the DT binding is updated > to match both, the DT and manually created devices. Because we don't > know which device creation will be used at runtime, the code to start > the kthread is moved to do_probe() which will be called by both methods. > > Fixes: 3e7bed52719d ("macintosh: therm_windtunnel: drop using attach_adapter") > Link: https://bugzilla.kernel.org/show_bug.cgi?id=201723 > Reported-by: Erhard Furtner <erhard_f@mailbox.org> > Tested-by: Erhard Furtner <erhard_f@mailbox.org> > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > --- > > I suggest this stable-tag: # v4.19+ Looks right to me. > Adding the Debian-PPC List to reach further people maybe willing to > test. > > This patch does not depend on "[PATCH RESEND] macintosh: convert to > i2c_new_scanned_device". In fact, this one here should go in first as > 5.6 material. I will rebase and resend the i2c_new_scanned_device() > conversion on top of this regression fix. > > I can also take this via I2C if easier. I think that would be best, it's more I2C related than powerpc arch stuff that I could review. I don't have a machine setup to test this easily, but Erhard has been doing a good job of testing things so I'm happy for you to take it with his Tested-by. Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc) cheers
> I think that would be best, it's more I2C related than powerpc arch > stuff that I could review. It is more DT handling than I2C, but I am happy to take this patch.
On Tue, Feb 25, 2020 at 03:12:29PM +0100, Wolfram Sang wrote: > Removing attach_adapter from this driver caused a regression for at > least some machines. Those machines had the sensors described in their > DT, too, so they didn't need manual creation of the sensor devices. The > old code worked, though, because manual creation came first. Creation of > DT devices then failed later and caused error logs, but the sensors > worked nonetheless because of the manually created devices. > > When removing attach_adaper, manual creation now comes later and loses > the race. The sensor devices were already registered via DT, yet with > another binding, so the driver could not be bound to it. > > This fix refactors the code to remove the race and only manually creates > devices if there are no DT nodes present. Also, the DT binding is updated > to match both, the DT and manually created devices. Because we don't > know which device creation will be used at runtime, the code to start > the kthread is moved to do_probe() which will be called by both methods. > > Fixes: 3e7bed52719d ("macintosh: therm_windtunnel: drop using attach_adapter") > Link: https://bugzilla.kernel.org/show_bug.cgi?id=201723 > Reported-by: Erhard Furtner <erhard_f@mailbox.org> > Tested-by: Erhard Furtner <erhard_f@mailbox.org> > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> Applied to for-current, thanks!
Wolfram Sang <wsa@the-dreams.de> writes: > On Tue, Feb 25, 2020 at 03:12:29PM +0100, Wolfram Sang wrote: >> Removing attach_adapter from this driver caused a regression for at >> least some machines. Those machines had the sensors described in their >> DT, too, so they didn't need manual creation of the sensor devices. The >> old code worked, though, because manual creation came first. Creation of >> DT devices then failed later and caused error logs, but the sensors >> worked nonetheless because of the manually created devices. >> >> When removing attach_adaper, manual creation now comes later and loses >> the race. The sensor devices were already registered via DT, yet with >> another binding, so the driver could not be bound to it. >> >> This fix refactors the code to remove the race and only manually creates >> devices if there are no DT nodes present. Also, the DT binding is updated >> to match both, the DT and manually created devices. Because we don't >> know which device creation will be used at runtime, the code to start >> the kthread is moved to do_probe() which will be called by both methods. >> >> Fixes: 3e7bed52719d ("macintosh: therm_windtunnel: drop using attach_adapter") >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=201723 >> Reported-by: Erhard Furtner <erhard_f@mailbox.org> >> Tested-by: Erhard Furtner <erhard_f@mailbox.org> >> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > > Applied to for-current, thanks! Thanks. cheers
diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c index 8c744578122a..a0d87ed9da69 100644 --- a/drivers/macintosh/therm_windtunnel.c +++ b/drivers/macintosh/therm_windtunnel.c @@ -300,9 +300,11 @@ static int control_loop(void *dummy) /* i2c probing and setup */ /************************************************************************/ -static int -do_attach( struct i2c_adapter *adapter ) +static void do_attach(struct i2c_adapter *adapter) { + struct i2c_board_info info = { }; + struct device_node *np; + /* scan 0x48-0x4f (DS1775) and 0x2c-2x2f (ADM1030) */ static const unsigned short scan_ds1775[] = { 0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d, 0x4e, 0x4f, @@ -313,25 +315,24 @@ do_attach( struct i2c_adapter *adapter ) I2C_CLIENT_END }; - if( strncmp(adapter->name, "uni-n", 5) ) - return 0; - - if( !x.running ) { - struct i2c_board_info info; + if (x.running || strncmp(adapter->name, "uni-n", 5)) + return; - memset(&info, 0, sizeof(struct i2c_board_info)); - strlcpy(info.type, "therm_ds1775", I2C_NAME_SIZE); + np = of_find_compatible_node(adapter->dev.of_node, NULL, "MAC,ds1775"); + if (np) { + of_node_put(np); + } else { + strlcpy(info.type, "MAC,ds1775", I2C_NAME_SIZE); i2c_new_probed_device(adapter, &info, scan_ds1775, NULL); + } - strlcpy(info.type, "therm_adm1030", I2C_NAME_SIZE); + np = of_find_compatible_node(adapter->dev.of_node, NULL, "MAC,adm1030"); + if (np) { + of_node_put(np); + } else { + strlcpy(info.type, "MAC,adm1030", I2C_NAME_SIZE); i2c_new_probed_device(adapter, &info, scan_adm1030, NULL); - - if( x.thermostat && x.fan ) { - x.running = 1; - x.poll_task = kthread_run(control_loop, NULL, "g4fand"); - } } - return 0; } static int @@ -404,8 +405,8 @@ attach_thermostat( struct i2c_client *cl ) enum chip { ds1775, adm1030 }; static const struct i2c_device_id therm_windtunnel_id[] = { - { "therm_ds1775", ds1775 }, - { "therm_adm1030", adm1030 }, + { "MAC,ds1775", ds1775 }, + { "MAC,adm1030", adm1030 }, { } }; MODULE_DEVICE_TABLE(i2c, therm_windtunnel_id); @@ -414,6 +415,7 @@ static int do_probe(struct i2c_client *cl, const struct i2c_device_id *id) { struct i2c_adapter *adapter = cl->adapter; + int ret = 0; if( !i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_WRITE_BYTE) ) @@ -421,11 +423,19 @@ do_probe(struct i2c_client *cl, const struct i2c_device_id *id) switch (id->driver_data) { case adm1030: - return attach_fan( cl ); + ret = attach_fan(cl); + break; case ds1775: - return attach_thermostat(cl); + ret = attach_thermostat(cl); + break; } - return 0; + + if (!x.running && x.thermostat && x.fan) { + x.running = 1; + x.poll_task = kthread_run(control_loop, NULL, "g4fand"); + } + + return ret; } static struct i2c_driver g4fan_driver = {