Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver

[Daniel Scally <djrscally@gmail.com>]

Hi Andy - thanks for comments!

On 22/02/2021 14:58, Andy Shevchenko wrote:
> On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
>> ACPI devices with _HID INT3472 are currently matched to the tps68470
>> driver, however this does not cover all situations in which that _HID
>> occurs. We've encountered three possibilities:
>>
>> 1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing
>> a physical TPS68470 device) that requires a GPIO and OpRegion driver
>> 2. On devices designed for Windows, an ACPI device with _HID INT3472
>> (again representing a physical TPS68470 device) which requires GPIO,
>> Clock and Regulator drivers.
>> 3. On other devices designed for Windows, an ACPI device with _HID
>> INT3472 which does **not** represent a physical TPS68470, and is instead
>> used as a dummy device to group some system GPIO lines which are meant
>> to be consumed by the sensor that is dependent on this entry.
>>
>> This commit adds a new module, registering a platform driver to deal
>> with the 3rd scenario plus an i2c driver to deal with #1 and #2, by
>> querying the CLDB buffer found against INT3472 entries to determine
>> which is most appropriate.
> Can you split CLK parts (and maybe regulators as well) to something
> like intel_skl_int3472_clk.c?


Sure, no problem

>
>> +#include <linux/acpi.h>
>> +#include <linux/i2c.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +               dev_err(&adev->dev, "%s object is not an ACPI buffer\n", id);
> Perhaps acpi_handle_err() et al. instead of dev_*(&adev->dev, ...)
> where it's applicable?


Ah - yes, ok, thanks. TIL those exist

>> +       if (obj->buffer.length > sizeof(*cldb)) {
>> +               dev_err(&adev->dev, "The CLDB buffer is too large\n");
>> +               ret = -EINVAL;
> ENOSPC? ENOMEM?


I still think EINVAL actually, as in this case the problem isn't that
space couldn't be allocated but that the buffer in the SSDB is larger
than I expect it to be, which means the definition of it has changed /
this device isn't actually supported.

>> +       ret = platform_driver_register(&int3472_discrete);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470);
>> +       if (ret)
>> +               platform_driver_unregister(&int3472_discrete);
> Not a fan of the above, but let's see what others will say...


Yeah; happy to discuss this more if needed.

>> +#include <linux/clk-provider.h>
> This is definitely not for *.h. (Not all C files needed this)
>
>> +#include <linux/gpio/machine.h>
> Ditto.
>
>> +#include <linux/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
> Ditto.


Yep; I'll move them to *_clk.c and *_regulator.c files.

>> +static int skl_int3472_clk_prepare(struct clk_hw *hw)
>> +{
>> +       struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>> +
>> +       gpiod_set_value(clk->ena_gpio, 1);
>> +       if (clk->led_gpio)
> Make it optional and drop this check. Same for other places of use of this GPIO.


Oops, of course, thanks

>> +static int skl_int3472_clk_enable(struct clk_hw *hw)
>> +{
>> +       /*
>> +        * We're just turning a GPIO on to enable, 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.
>> +        */
> Missed . and / or  () in some words? (Describing callbacks, personally
> I use the form "->callback()" in such cases)


OK, I'll fix the comment to match that style.


>> +static unsigned int skl_int3472_get_clk_frequency(struct int3472_discrete_device *int3472)
>> +{
>> +       union acpi_object *obj;
>>
>> +       unsigned int ret = 0;
> unsigned for ret is unusual. Looking into the code, first of all it
> doesn't need this assignment; second, it probably can gain a better
> name: "frequency"?


Yep ok, I'll rename to freq/frequency

>> +       if (!IS_ERR_OR_NULL(sensor_config) && sensor_config->function_maps) {
> Hmm...
>
> Would
>
> if (IS_ERR_OR_NULL(sensor_config))
>   return 0;
>
> if (!_maps)
>   return 0;
>
> with respective comments working here?


No, because the absence of either sensor_config or
sensor_config->function_maps is not a failure mode. We only need to
provide sensor_configs for some platforms, and function_maps for even
fewer. So if that check is false, the rest of the function should still
execute.

>> +static int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
>> +{
>> +       struct clk_init_data init = {
>> +               .ops = &skl_int3472_clock_ops,
>> +               .flags = CLK_GET_RATE_NOCACHE,
>> +       };
>> +       int ret = 0;
>> +
>> +       init.name = kasprintf(GFP_KERNEL, "%s-clk",
>> +                             acpi_dev_name(int3472->adev));
> devm_*() ? Or is the lifetime different?


No it's not; I'll use devm_*(), thanks

>> +       sensor_config = int3472->sensor_config;
>> +       if (IS_ERR_OR_NULL(sensor_config)) {
>> +               dev_err(int3472->dev, "No sensor module config\n");
>> +               return PTR_ERR(sensor_config);
> NULL -> 0. Is it okay?


Ah, no it's not - good catch thank you.

>> +       if (ares->type != ACPI_RESOURCE_TYPE_GPIO ||
>> +           ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO)
>> +               return 1; /* Deliberately positive so parsing continues */
> I don't like to lose control over ACPI_RESOURCE_TYPE_GPIO, i.e.
> spreading it over kernel code (yes, I know about one existing TS
> case).
> Consider to provide a helper in analogue to acpi_gpio_get_irq_resource().


Sure, but I probably name it acpi_gpio_is_io_resource() - a function
named "get" which returns a bool seems a bit funny to me.

>> +       if (ret < 0 && ret != -EPROBE_DEFER)
>> +               dev_err(int3472->dev, err_msg);
> dev_err_probe() will make the above conditional go away. And you may even do...


Ah-ha - thought that must exist but couldn't find it - thank you.

>> +       if (int3472->clock.ena_gpio) {
> Not sure you need this here.


We haven't seen a device that lacks a clock enable GPIO it's true, but
since all the other kinds seem optional it didn't seem impossible that
that one is optional too. I can remove if you prefer and we can just
deal with it when we encounter one like that though?

>> +       /* Max num GPIOs we've seen plus a terminator */
>> +       int3472 = kzalloc(struct_size(int3472, gpios.table,
>> +                         INT3472_MAX_SENSOR_GPIOS + 1), GFP_KERNEL);
> Wonder of you can use devm_*() APIs in this function.


Yeah I can, I'll switch to that.


>> +int skl_int3472_discrete_remove(struct platform_device *pdev)
>> +{
>> +       struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
>> +       if (int3472->gpios.dev_id)
>> +               gpiod_remove_lookup_table(&int3472->gpios);
> gpiod_remove_lookup_table() is now NULL-aware.
> But in any case I guess you don't need the above check.


Sorry; forgot to call out that I didn't follow that suggestion;
int3472->gpios is a _struct_ rather than a pointer, so &int3472->gpios
won't be NULL, even if I haven't filled anything in to there yet because
it failed before it got to that point. So, not sure that it quite works
there.

>
>> +       if (!IS_ERR(int3472->regulator.rdev))
>> +               regulator_unregister(int3472->regulator.rdev);
> Shouldn't it be the pointer to the regulator itself?
int3472->regulator is type struct int3472_gpio_regulator, the .rdev is
the normal regulator_dev


>
>> +       if (!IS_ERR(int3472->clock.clk))
> If you get it optional, you won't need this additional check.


Yes - here it will definitely work; thanks, I'll add that patch

>> +       ret = skl_int3472_fill_cldb(adev, &cldb);
>> +       if (!ret && cldb.control_logic_type != 2) {
>> +               dev_err(&client->dev, "Unsupported control logic type %u\n",
>> +                       cldb.control_logic_type);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (ret)
>> +               cldb_present = false;
> if (ret)
>   ...
> else if (...)  {
>   ...
>   return ...;
> }


Oh yeah...now you point that out I have no idea what I was thinking there...