Hi Andy - thanks for comments

On 21/05/2021 13:57, Andy Shevchenko wrote:

quoted

+/*
+ * The regulators have to have .ops to be valid, but the only ops we actually
+ * support are .enable and .disable which are handled via .ena_gpiod. Pass an
+ * empty struct to clear the check without lying about capabilities.
+ */
+static const struct regulator_ops int3472_gpio_regulator_ops;

Hmm... Can you use 'reg-fixed-voltage' platform device instead?

One example, although gone from upstream, but available in the tree, I can
point to is this:

  git log -p -- arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c

It uses constant structures, but I think you may dynamically generate the
necessary ones.

I can experiment with this, though one thing is we have no actual idea
what voltages these are supplying...it doesn't look like that matters
from drivers/regulator/fixed.c, but I'd have to try it to be sure.

+
+static int skl_int3472_clk_enable(struct clk_hw *hw)
+{
+	/*
+	 * We're just turning a GPIO on to enable the clock, which operation
+	 * has the potential to sleep. Given .enable() cannot sleep, but
+	 * .prepare() can, we toggle the GPIO in .prepare() instead. Thus,
+	 * nothing to do here.
+	 */
It's a nice comment, but you are using non-sleeping GPIO value setters. Perhaps
you need to replace them with gpiod_set_value_cansleep()?

That would make sense!

quoted

+static unsigned int skl_int3472_get_clk_frequency(struct int3472_discrete_device *int3472)
+{
+	union acpi_object *obj;
+	unsigned int freq;
+
+	obj = skl_int3472_get_acpi_buffer(int3472->sensor, "SSDB");
+	if (IS_ERR(obj))
+		return 0; /* report rate as 0 on error */
+
+	if (obj->buffer.length < CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET + sizeof(u32)) {
+		dev_err(int3472->dev, "The buffer is too small\n");
+		goto out_free_buff;

First of all, freq will be uninitialized here.

I'm wondering if you can simple drop the goto and replace it with direct steps, i.e.
	kfree(obj);
	return 0;

Sure, I have no real preference; I'll do that instead.

quoted

+static const struct int3472_sensor_config *
+skl_int3472_get_sensor_module_config(struct int3472_discrete_device *int3472)
+{
+	const struct int3472_sensor_config *ret;
+	union acpi_object *obj;
+	unsigned int i;
+
+	obj = acpi_evaluate_dsm_typed(int3472->sensor->handle,
+				      &cio2_sensor_module_guid, 0x00,
+				      0x01, NULL, ACPI_TYPE_STRING);
+
+	if (!obj) {
+		dev_err(int3472->dev,
+			"Failed to get sensor module string from _DSM\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	if (obj->string.type != ACPI_TYPE_STRING) {
+		dev_err(int3472->dev,
+			"Sensor _DSM returned a non-string value\n");
+		ret = ERR_PTR(-EINVAL);
+		goto out_free_obj;
+	}
+	ret = ERR_PTR(-EINVAL);
+	for (i = 0; i < ARRAY_SIZE(int3472_sensor_configs); i++) {
+		if (!strcmp(int3472_sensor_configs[i].sensor_module_name,
+			    obj->string.pointer)) {
+			ret = &int3472_sensor_configs[i];
+			break;
+		}
+	}

Can be refactored like this:

	for (i = 0; i < ARRAY_SIZE(int3472_sensor_configs); i++) {
		if (!strcmp(int3472_sensor_configs[i].sensor_module_name,
			    obj->string.pointer))
			break;
	}

	ACPI_FREE(obj);

	if (i >= ARRAY_SIZE(int3472_sensor_configs))
		return ERR_PTR(-EINVAL);

	return &int3472_sensor_configs[i];

Yeah ok, I like this better than the ret = ERR_PTR(-EINVAL) before the
loop; thank you.

quoted

+ * Return:
+ * * 0		- When all resources found are handled properly.

Positive number ... ?

quoted

+	if (!acpi_gpio_get_io_resource(ares, &agpio))
+		return 1; /* Deliberately positive so parsing continues */

Move it to description above?

oops, yes, I'll add those to the comment.

quoted

+	if (int3472->clock.ena_gpio) {
+		ret = skl_int3472_register_clock(int3472);
+		if (ret)
+			goto out_free_res_list;
+	} else {

Hmm... Have I got it correctly that we can't have ena_gpio && led_gpio together?

No, just that we can only have led_gpio if we also have ena_gpio (at
least that's the intention...)

quoted

+	if (ret)
+		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,

This I don't like. Since we get a returned variable with different meaning, can
we use a specific variable name for it? On top of that, I would rather see
something like this:

	whatever = skl_...(...);
	switch (whatever) {
	case WHATEVER_ONE_CASE:
		if (cldb.control_logic_type != 2) {
			dev_err(&client->dev, "Unsupported control logic type %u\n",
				cldb.control_logic_type);
			return -EINVAL;
		}
		cells_data = tps68470_win;
		cells_size = ARRAY_SIZE(tps68470_win);
		break;
	case WHATEVER_ANOTHER_CASE:
		...
		break;
	default:
		...Oops...
		break; // or return -ERRNO
	}

	return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
				    cells_data, cells_size, NULL, 0, NULL);

Yeah I guess that's a bit obscure at first glance; alright, I'll follow
this to make it clearer what's happening there.