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.
next prev parent reply other threads:[~2021-05-25 22:53 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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).