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.
