Re: [PATCH v4 7/8] platform/x86: Add intel_skl_int3472 driver

[Daniel Scally <djrscally@xxxxxxxxx>]

Hi Andy - thanks for comments

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

>> +/*
>> + * 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!


>> +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.


>> +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.


>> + * Return:
>> + * * 0		- When all resources found are handled properly.
> Positive number ... ?
>> +	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.


>> +	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...)


>> +	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.