linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Scally <djrscally@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
	Wolfram Sang <wsa@kernel.org>, Lee Jones <lee.jones@linaro.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Maximilian Luz <luzmaximilian@gmail.com>,
	linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
	linux-i2c@vger.kernel.org, platform-driver-x86@vger.kernel.org,
	devel@acpica.org, Len Brown <lenb@kernel.org>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Russell King <linux@armlinux.org.uk>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Mark Gross <mgross@linux.intel.com>,
	Robert Moore <robert.moore@intel.com>,
	Erik Kaneda <erik.kaneda@intel.com>,
	laurent.pinchart@ideasonboard.com,
	kieran.bingham@ideasonboard.com
Subject: Re: [PATCH v4 7/8] platform/x86: Add intel_skl_int3472 driver
Date: Tue, 25 May 2021 23:53:21 +0100	[thread overview]
Message-ID: <6294177b-d6e1-8bbd-d313-5cce1c498604@gmail.com> (raw)
In-Reply-To: <YKeuQM/O9+jDZFpb@smile.fi.intel.com>

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-05-25 22:54 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-20 14:09 [PATCH v4 0/8] Introduce intel_skl_int3472 module Daniel Scally
2021-05-20 14:09 ` [PATCH v4 1/8] ACPI: scan: Extend acpi_walk_dep_device_list() Daniel Scally
2021-05-20 17:15   ` Maximilian Luz
2021-05-20 18:22   ` Rafael J. Wysocki
2021-05-20 21:03     ` Hans de Goede
2021-05-21 12:59   ` Andy Shevchenko
2021-05-20 14:09 ` [PATCH v4 2/8] ACPI: scan: Add function to fetch dependent of acpi device Daniel Scally
2021-05-20 18:33   ` Rafael J. Wysocki
2021-05-20 18:55     ` Rafael J. Wysocki
2021-05-21 19:25       ` Daniel Scally
2021-05-20 14:09 ` [PATCH v4 3/8] i2c: core: Add a format macro for I2C device names Daniel Scally
2021-05-20 14:09 ` [PATCH v4 4/8] gpiolib: acpi: Export acpi_get_gpiod() Daniel Scally
2021-05-20 14:09 ` [PATCH v4 5/8] clkdev: Make clkdev_drop() null aware Daniel Scally
2021-05-20 14:09 ` [PATCH v4 6/8] gpiolib: acpi: Add acpi_gpio_get_io_resource() Daniel Scally
2021-05-21 12:05   ` Andy Shevchenko
2021-05-25 22:30     ` Daniel Scally
2021-05-20 14:09 ` [PATCH v4 7/8] platform/x86: Add intel_skl_int3472 driver Daniel Scally
2021-05-21 12:57   ` Andy Shevchenko
2021-05-25 22:53     ` Daniel Scally [this message]
2021-05-26  7:54       ` Andy Shevchenko
2021-05-20 14:09 ` [PATCH v4 8/8] mfd: tps68470: Remove tps68470 MFD driver Daniel Scally
2021-05-25 13:10 ` [PATCH v4 0/8] Introduce intel_skl_int3472 module Hans de Goede
2021-05-25 13:12   ` Hans de Goede
2021-05-25 13:23     ` Andy Shevchenko
2021-05-25 23:03   ` Daniel Scally
2021-05-26  7:55     ` Andy Shevchenko
2021-05-26 12:36     ` Hans de Goede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6294177b-d6e1-8bbd-d313-5cce1c498604@gmail.com \
    --to=djrscally@gmail.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=devel@acpica.org \
    --cc=erik.kaneda@intel.com \
    --cc=hdegoede@redhat.com \
    --cc=kieran.bingham@ideasonboard.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=lee.jones@linaro.org \
    --cc=lenb@kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=luzmaximilian@gmail.com \
    --cc=mgross@linux.intel.com \
    --cc=mika.westerberg@linux.intel.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=rafael.j.wysocki@intel.com \
    --cc=robert.moore@intel.com \
    --cc=wsa@kernel.org \
    --subject='Re: [PATCH v4 7/8] platform/x86: Add intel_skl_int3472 driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).