Hi Laurent
On 23/02/2021 20:04, Laurent Pinchart wrote:
> +
> +/*
> + * Here follows platform specific mapping information that we can pass to
> + * the functions mapping resources to the sensors. Where the sensors have
> + * a power enable pin defined in DSDT we need to provide a supply name so
> + * the sensor drivers can find the regulator. The device name will be derived
> + * from the sensor's ACPI device within the code. Optionally, we can provide a
> + * NULL terminated array of function name mappings to deal with any platform
> + * specific deviations from the documented behaviour of GPIOs.
> + *
> + * Map a GPIO function name to NULL to prevent the driver from mapping that
> + * GPIO at all.
> + */
> +
> +static const struct int3472_gpio_function_remap ov2680_gpio_function_remaps[] = {
> + { "reset", NULL },
> + { "powerdown", "reset" },
> + { }
> +};
> +
> +static struct int3472_sensor_config int3472_sensor_configs[] = {
> This should be static const (and there will be some fallout due to that,
> as skl_int3472_register_regulator() modifies the supply_map, so I think
> you'll have a copy of supply_map in int3472_discrete_device).
Ack to all of the constness; you mentioned that last time too - not sure
how I missed doing those! I think I can just having a local struct
regulator_consumer_supply in skl_int3472_register_regulator and fill it
from int3472->sensor_config.supply_map
>> +static unsigned int skl_int3472_get_clk_frequency(struct int3472_discrete_device *int3472)
>> +{
>> + union acpi_object *obj;
>> + unsigned int ret = 0;
>> +
>> + 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)) {
> Should we define an ssdb structure instead of peeking into the buffer
> with an offset ?
I thought about that, but in the end decided it didn't seem worth
defining the whole SSDB structure just to use one field. Particularly
since we use it in cio2-bridge already, so if we're going to do that it
really ought to just live in a header that's included in both - and that
seemed even less worthwhile.
I don't have a strong feeling though, so if you think it's better to
define the struct I'm happy to.
>> +static unsigned long skl_int3472_clk_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>> + struct int3472_discrete_device *int3472 = to_int3472_device(clk);
>> +
>> + return int3472->clock.frequency;
> Maybe just
>
> struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>
> return clk->frequency;
Oops, of course.
>> +static int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
>> + struct acpi_resource *ares)
>> +{
>> + char *path = ares->data.gpio.resource_source.string_ptr;
>> + struct int3472_sensor_config *sensor_config;
>> + struct regulator_init_data init_data = { };
>> + struct regulator_config cfg = { };
>> + int ret;
>> +
>> + 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);
>> + }
>> +
>> + if (!sensor_config->supply_map.supply) {
>> + dev_err(int3472->dev, "No supply name defined\n");
>> + return -ENODEV;
>> + }
>> +
>> + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
>> + init_data.num_consumer_supplies = 1;
>> + sensor_config->supply_map.dev_name = int3472->sensor_name;
>> + init_data.consumer_supplies = &sensor_config->supply_map;
>> +
>> + snprintf(int3472->regulator.regulator_name,
>> + sizeof(int3472->regulator.regulator_name), "%s-regulator",
>> + acpi_dev_name(int3472->adev));
>> + snprintf(int3472->regulator.supply_name,
>> + GPIO_REGULATOR_SUPPLY_NAME_LENGTH, "supply-0");
>> +
>> + int3472->regulator.rdesc = INT3472_REGULATOR(
>> + int3472->regulator.regulator_name,
>> + int3472->regulator.supply_name,
>> + &int3472_gpio_regulator_ops);
>> +
>> + int3472->regulator.gpio = acpi_get_gpiod(path,
>> + ares->data.gpio.pin_table[0],
>> + "int3472,regulator");
>> + if (IS_ERR(int3472->regulator.gpio)) {
>> + dev_err(int3472->dev, "Failed to get regulator GPIO lines\n");
> s/lines/line/ (sorry, it was a typo in my review of v2)
No problem!
>> +static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
>> +{
>> + struct list_head resource_list;
>> + int ret;
>> +
>> + INIT_LIST_HEAD(&resource_list);
>> +
>> + int3472->sensor_config = skl_int3472_get_sensor_module_config(int3472);
> I have forgotten some of the context I'm afraid :-/ Are there valid use
> cases for not checking for an error here, or should we do so and drop
> the error checks in other functions above ?
Not all platforms need a sensor_config; only those which have either a
regulator pin or need a GPIO function to be remapped; the rest will do
without it.
So, we need to not check for an error here because the absence of a
sensor_config isn't necessarily an error, we won't know till later.
> +int skl_int3472_discrete_probe(struct platform_device *pdev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> + struct int3472_discrete_device *int3472;
> + struct int3472_cldb cldb;
> + int ret;
> +
> + ret = skl_int3472_fill_cldb(adev, &cldb);
> + if (ret) {
> + dev_err(&pdev->dev, "Couldn't fill CLDB structure\n");
> + return ret;
> + }
> +
> + if (cldb.control_logic_type != 1) {
> + dev_err(&pdev->dev, "Unsupported control logic type %u\n",
> + cldb.control_logic_type);
> + return -EINVAL;
> + }
> +
> + /* Max num GPIOs we've seen plus a terminator */
> + int3472 = kzalloc(struct_size(int3472, gpios.table,
> + INT3472_MAX_SENSOR_GPIOS + 1), GFP_KERNEL);
> + if (!int3472)
> + return -ENOMEM;
> +
> + int3472->adev = adev;
> + int3472->dev = &pdev->dev;
> + platform_set_drvdata(pdev, int3472);
> +
> + int3472->sensor = acpi_dev_get_dependent_dev(adev);
> + if (IS_ERR_OR_NULL(int3472->sensor)) {
> + dev_err(&pdev->dev,
> + "INT3472 seems to have no dependents.\n");
> + ret = -ENODEV;
> + goto err_free_int3472;
> + }
> + get_device(&int3472->sensor->dev);
> I see no corresponding put_device(), am I missing something ? I'm also
> not sure why this is needed.
>
The put is acpi_dev_put() in skl_int3472_discrete_remove(); there seems
to be no acpi_dev_get() for some reason. We use the sensor acpi_device
to get the clock frequency, and to fetch the sensor module string, so I
thought it ought to hold a reference on those grounds.
>> diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
>> new file mode 100644
>> index 000000000000..d0d2391e263f
>> --- /dev/null
>> +++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
>> @@ -0,0 +1,113 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Author: Dan Scally <djrscally@xxxxxxxxx> */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/tps68470.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "intel_skl_int3472_common.h"
>> +
>> +static const struct mfd_cell tps68470_c[] = {
>> + { .name = "tps68470-gpio" },
>> + { .name = "tps68470_pmic_opregion" },
>> +};
>> +
>> +static const struct mfd_cell tps68470_w[] = {
> Maybe more explicit names than _c and _w could be nice ?
_chrome and _windows was in my mind - sound ok?
