Hi Andy, On 7/9/22 13:34, Andy Shevchenko wrote: > On Sat, Jul 9, 2022 at 1:00 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >> On 7/9/22 11:52, Andy Shevchenko wrote: >>> On Saturday, July 9, 2022, Hans de Goede <hdegoede@xxxxxxxxxx <mailto:hdegoede@xxxxxxxxxx>> wrote: >>> On 7/9/22 02:06, Andy Shevchenko wrote: > > ... > >>> So nack for this >>> >>> This effectively means nack to the series. > >>> But it’s easy to fix. I can add check for ret == 0. > > So, are you okay with fixing it this way? See below how. > >> I don't see how this is a nack for the series, just drop 1/7 + 2/7 >> and rebase the rest. Yes there will be conflicts to resolve in >> the rebase, but the rest of the cleanups can still go upstream >> after the rebase. > > Because patch 3 makes a little sense on its own if we drop the patch > 2. The rest is the simple cleanups which I do not consider as a core > of this series. Patch 3 just removes the adev == NULL check and pushes the ACPI_COMPANION(dev) calls into the function needing the adev, I don't see how that relies on this patch ? >>> > case SMI_AUTO_DETECT: >>> > - if (i2c_acpi_client_count(adev) > 0) >>> > - return smi_i2c_probe(pdev, adev, smi, node->instances); >>> > - else >>> > - return smi_spi_probe(pdev, adev, smi, node->instances); >>> > + ret = smi_i2c_probe(pdev, adev, smi, node->instances); >>> > + if (ret && ret != -ENOENT) >>> > + return ret; > > /* > * ...comment about why we do the following check... > */ > if (ret == 0) > return ret; I'm ok with doing things this way. Note you then end up with: if (ret && ret != -ENOENT) return ret; if (ret == 0) return ret; Which can be simplified to just: if (ret != -ENOENT) return ret; > >>> > + ret = smi_spi_probe(pdev, adev, smi, node->instances); >>> > + if (ret && ret != -ENOENT) >>> > + return ret; >>> > + if (ret) >>> > + return dev_err_probe(dev, ret, "Error No resources found\n"); >>> > + break; > > if (ret == -ENOENT) > return dev_err_probe(...); > return ret; Hmm, we don't do this dev_err for the SMI_I2C / SMI_SPI cases, I see 2 options to solve this: 1) Drop the return calls from switch (node->bus_type) {} instead always set ret and then break. And do the: if (ret == -ENOENT) return dev_err_probe(...); outside the switch-case 2) Drop the dev_err, the driver-core will already log an error for the -ENOENT anyways. Regards, Hans
