On 22/02/2021 14:58, Andy Shevchenko wrote: > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally(a)gmail.com&gt; wrote:

>> + if (obj->buffer.length > sizeof(*cldb)) { >> + dev_err(&adev->dev, "The CLDB buffer is too large\n"); >> + ret = -EINVAL; > ENOSPC? ENOMEM? I still think EINVAL actually, as in this case the problem isn't that space couldn't be allocated but that the buffer in the SSDB is larger than I expect it to be, which means the definition of it has changed / this device isn't actually supported.

>> + if (!IS_ERR_OR_NULL(sensor_config) && sensor_config->function_maps) { > Hmm... > > Would > > if (IS_ERR_OR_NULL(sensor_config)) > return 0; > > if (!_maps) > return 0; > > with respective comments working here? No, because the absence of either sensor_config or sensor_config->function_maps is not a failure mode. We only need to provide sensor_configs for some platforms, and function_maps for even fewer. So if that check is false, the rest of the function should still execute.

>> + if (ares->type != ACPI_RESOURCE_TYPE_GPIO || >> + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO) >> + return 1; /* Deliberately positive so parsing continues */ > I don't like to lose control over ACPI_RESOURCE_TYPE_GPIO, i.e. > spreading it over kernel code (yes, I know about one existing TS > case). > Consider to provide a helper in analogue to acpi_gpio_get_irq_resource(). Sure, but I probably name it acpi_gpio_is_io_resource() - a function named "get" which returns a bool seems a bit funny to me.

>> + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev); >> + if (int3472->gpios.dev_id) >> + gpiod_remove_lookup_table(&int3472->gpios); > gpiod_remove_lookup_table() is now NULL-aware. > But in any case I guess you don't need the above check. Sorry; forgot to call out that I didn't follow that suggestion; int3472->gpios is a _struct_ rather than a pointer, so &int3472->gpios won't be NULL, even if I haven't filled anything in to there yet because it failed before it got to that point. So, not sure that it quite works there.