Message-ID: <YDYmv0PpSndAlnDC@pendragon.ideasonboard.com>
Date:   Wed, 24 Feb 2021 12:13:19 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Daniel Scally <djrscally@...il.com>
Cc:     tfiga@...omium.org, sakari.ailus@...ux.intel.com,
        rajmohan.mani@...el.com, rjw@...ysocki.net, lenb@...nel.org,
        mika.westerberg@...ux.intel.com, linus.walleij@...aro.org,
        bgolaszewski@...libre.com, wsa@...nel.org, lee.jones@...aro.org,
        andy.shevchenko@...ux.intel.com,
        kieran.bingham+renesas@...asonboard.com, hdegoede@...hat.com,
        mgross@...ux.intel.com, luzmaximilian@...il.com,
        robert.moore@...el.com, erik.kaneda@...el.com, me@...wu.ch,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
        platform-driver-x86@...r.kernel.org, devel@...ica.org
Subject: Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver

Hi Daniel,

On Tue, Feb 23, 2021 at 10:36:18PM +0000, Daniel Scally wrote:
> 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.

If the structure is available already, sharing it in a common header
would be best I think, but that's not a blocker. It can be done on top
of this series.

> >> +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.

Shouldn't acpi_dev_get_dependent_dev() increase the reference count
then, instead of doing it manually here ?

> >> 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@...il.com> */
> >> +
> >> +#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?

As Andy mentioned, _cros is better, and _windows_ or _win both work for
me.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists