Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver

[Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>]

Hi Daniel,

On Mon, Jan 18, 2021 at 08:46:34PM +0000, Daniel Scally wrote:
> Hi Laurent, thanks for the comments - really appreciate the detail.
> 
> Some specific responses below but assume a general "will do" to
> everything you mentioned otherwise...
> 
> On 18/01/2021 09:15, Laurent Pinchart wrote:
> >> +	  PMIC) and one designed for Chrome OS.
> > 
> > How about expanding this a bit to explain what the INT3472 stands for ?
> >
> > 	  The INT3472 is an Intel camera power controller, a logical device
> > 	  found on some Skylake-based systems that can map to different
> > 	  hardware devices depending on the platform. On machines
> > 	  designed for Chrome OS, it maps to a TPS68470 camera PMIC. On
> > 	  machines designed for Windows, it maps to either a TP68470
> > 	  camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs
> > 	  and power gates.
> 
> Yeah sure ok
> 
> >> This driver handles all three
> >> +	  situations by discovering information it needs to discern them at
> >> +	  runtime.
> >> +
> >> +	  If your device was designed for Chrome OS, this driver will provide
> >> +	  an ACPI operation region, which must be available before any of the
> >> +	  devices using this are probed. For this reason, you should select Y
> >> +	  if your device was designed for ChromeOS. This option also configures
> >> +	  the designware-i2c driver to be built-in, for the same reason.
> > 
> > Is the last sentence a leftover ?
> 
> Oops - it is, but it was supposed to remind me to double check that that
> was still necessary. I'll take a look, thanks.
> 
> >> +
> >> +#include "intel_skl_int3472_common.h"
> >> +
> >> +int skl_int3472_get_cldb_buffer(struct acpi_device *adev,
> >> +				struct int3472_cldb *cldb)
> >> +{
> >> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> >> +	acpi_handle handle = adev->handle;
> >> +	union acpi_object *obj;
> >> +	acpi_status status;
> >> +	int ret = 0;
> >> +
> >> +	status = acpi_evaluate_object(handle, "CLDB", NULL, &buffer);
> >> +	if (ACPI_FAILURE(status))
> >> +		return -ENODEV;
> >> +
> >> +	obj = buffer.pointer;
> >> +	if (!obj) {
> >> +		dev_err(&adev->dev, "ACPI device has no CLDB object\n");
> > 
> > Is this the code path that is taken on Chrome OS ? If so an error
> > message isn't appropriate. I'd drop this message, and instead add an
> > error message in the discrete PMIC code.
> 
> Ah yes of course, thanks, I'll move the error message.
> 
> >> +
> >> +	unsigned int n_gpios; /* how many GPIOs have we seen */
> >> +
> >> +	struct int3472_gpio_regulator regulator;
> >> +	struct int3472_gpio_clock clock;
> > 
> > You don't necessarily need to define separate structures for this, you
> > could also write
> >
> > 	struct {
> > 		char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
> > 		char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
> > 		struct gpio_desc *gpio;
> > 		struct regulator_dev *rdev;
> > 		struct regulator_desc rdesc;
> > 	} regulator;
> >
> > 	struct {
> > 		struct clk *clk;
> > 		struct clk_hw clk_hw;
> > 		struct clk_lookup *cl;
> > 		struct gpio_desc *gpio;
> > 	} clock;
> >
> > It's entirely up to you.
> 
> Ooh yeah I like that more, thanks very much.
> 
> >> +/* 79234640-9e10-4fea-a5c1-b5aa8b19756f */
> >> +static const guid_t int3472_gpio_guid =
> >> +	GUID_INIT(0x79234640, 0x9e10, 0x4fea,
> >> +		  0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f);
> >> +
> >> +/* 822ace8f-2814-4174-a56b-5f029fe079ee */
> >> +static const guid_t cio2_sensor_module_guid =
> >> +	GUID_INIT(0x822ace8f, 0x2814, 0x4174,
> >> +		  0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
> > 
> > A comment that explains what those DSM functions do would be useful for
> > reference. It has taken lots of time to figure it out, let's spare the
> > pain to the next person who tries to understand this :-)
> 
> Hah - good point, well made. I'll explain what they're for then.
> 
> >> +static int skl_int3472_clk_enable(struct clk_hw *hw)
> >> +{
> >> +	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
> >> +
> >> +	gpiod_set_value(clk->gpio, 1);
> > 
> > The clock enable() and disable() methods are not supposed to sleep,
> > while setting a GPIO value may sleep in the general case. Should this be
> > moved to skl_int3472_clk_prepare() ? Same for skl_int3472_clk_disable()
> > and skl_int3472_clk_unprepare().
> 
> I was under the assumption the difference between gpiod_set_value() and
> gpiod_set_value_cansleep() was that gpiod_set_value() _can't_ sleep, but
> actually reading the function's comments it seems it will just complain
> if it turns out it can sleep:
> 
> * This function can be called from contexts where we cannot sleep, and will
> * complain if the GPIO chip functions potentially sleep. It doesn't
> complain, on either of my devices, but I guess that can't be guaranteed
> for _every_ device, so these calls probably are safer in (un)prepare() yes.

If we could guarantee that the GPIOs are connected to the SoC, we could
keep using the code above, as there should be no need to sleep. The
question is whether this can be guaranteed or not. It's true that I
would be surprised if the GPIOs were connected, for instance, to an I2C
GPIO expander..

> >> +			}
> >> +
> >> +			i++;
> >> +		}
> >> +	}
> >> +
> >> +	if (!func)
> >> +		return 0;
> > 
> > I initially thought this wasn't right, as if no entry was found in the
> > mapping table, func would still have its non-NULL value as passed to
> > this function. I then realized that you're checking if the match that
> > was found is NULL. A comment to explain this would be useful.
> 
> Yep ok - I actually had one and decided it was superfluous and removed
> it - my bad.
> 
> >> +
> >> +	status = acpi_get_handle(NULL, path, &handle);
> >> +	if (ACPI_FAILURE(status))
> >> +		return -EINVAL;
> >> +
> >> +	ret = acpi_bus_get_device(handle, &adev);
> >> +	if (ret)
> >> +		return -ENODEV;
> >> +
> >> +	table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev),
> >> +							   ares->data.gpio.pin_table[0],
> >> +							   func, 0, polarity);
> > 
> > I wonder if
> >
> > 	table_entry.key = acpi_dev_name(adev);
> > 	table_entry.chip_hwnum = ares->data.gpio.pin_table[0];
> > 	table_entry.con_id = func;
> > 	table_entry.idx = 0;
> > 	table_entry.flags = polarity;
> >
> > (with struct gpiod_lookup table_entry = { }; above) would be more
> > readable. Up to you.
> >
> >> +
> >> +	memcpy(&int3472->gpios.table[int3472->n_sensor_gpios], &table_entry,
> >> +	       sizeof(table_entry));
> > 
> > Ah, or maybe
> >
> > 	struct gpio_lookup *table_entry;
> >
> > 	table_entry = &int3472->gpios.table[int3472->n_sensor_gpios];
> > 	table_entry->key = acpi_dev_name(adev);
> > 	table_entry->chip_hwnum = ares->data.gpio.pin_table[0];
> > 	table_entry->con_id = func;
> > 	table_entry->idx = 0;
> > 	table_entry->flags = polarity;
> >
> > (no need to memset() to 0 first as the whole structure has been
> > allocated with kzalloc()).
> 
> Yeah you're right, this looks much nicer - thanks.
> 
> >> +	int ret = 0;
> >> +
> >> +	init.name = kasprintf(GFP_KERNEL, "%s-clk",
> >> +			      acpi_dev_name(int3472->adev));
> > 
> > You need to check for NULL and return -ENOMEM.
> 
> Oops, of course, thanks
> 
> >> +		goto err_unregister_clk;
> > 
> > If this fails, you will end up calling clk_unregister() and
> > clkdev_drop() in skl_int3472_discrete_remove(). You should replace the
> > check in the remove function with
> >
> > 	if (!int3472->clock.cl) {
> 
> You're right, good spot, thank you.
> 
> >> +		dev_err(&int3472->pdev->dev, "No sensor module config\n");
> >> +		return PTR_ERR(sensor_config);
> >> +	}
> > 
> > Would it make sense to call this in skl_int3472_discrete_probe() or
> > skl_int3472_parse_crs() and cache the config pointer ?
> 
> Yes, probably actually, and then the GPIO mapping function can just
> check for its presence.
> 
> >> +	init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> >> +	init_data.num_consumer_supplies = 1;
> >> +	init_data.consumer_supplies = &sensor_config->supply_map;
> >> +
> >> +	snprintf(int3472->regulator.regulator_name,
> >> +		 GPIO_REGULATOR_NAME_LENGTH, "int3472-discrete-regulator");
> > 
> > s/GPIO_REGULATOR_NAME_LENGTH/sizeof(int3472->regulator.regulator_name)/
> >
> > Do regulator names need to be unique ? If so you may have a problem if a
> > platform has two discrete INT3472.
> 
> Unlike clocks, the regulator framework doesn't shout at you when you do
> this, but I agree it's suboptimal at the very least, I'll set it to
> ..."%s-regulator", acpi_dev_name(int3472->adev)... as with the clock.
> 
> >> +	case INT3472_GPIO_TYPE_PRIVACY_LED:
> >> +		ret = skl_int3472_map_gpio_to_sensor(int3472, ares,
> >> +						     "indicator-led",
> >> +						     GPIO_ACTIVE_HIGH);
> > 
> > Mapping the indicator LED to the sensor isn't great, as all sensor
> > drivers would then need to handle it. Could it be handled in the
> > regulator instead, so that it would be turned on automatically when the
> > sensor is powered up ? Another option, more complicated, would be to
> > handle it in the CIO2 driver (but I'm not sure how we would map it).
> 
> Not with the regulator, because it turns out only the 0x0b pin is one of
> those and those appear on very few devices in scope, so it wouldn't be
> called on a Surface Book 2 for example. Perhaps as part of clock
> prepare/enable? I don't much like the idea of it running in the CIO2
> driver to be honest, feels a bit out of place.

The clock is another option, but could there be platforms where the
clock GPIO isn't present ?

Another option would be to let userspace handle that GPIO, but we then
need to convey it to userspace.

> >> +
> >> +	if (int3472->gpios_mapped)
> >> +		gpiod_remove_lookup_table(&int3472->gpios);
> > 
> > You could avoid the need for the gpios_mapped field by checking for
> >
> > 	if (int3472->gpios.list.next)
> >
> > Up to you.
> 
> Thank you! I was scratching my head trying to figure out a better way of
> doing that.

-- 
Regards,

Laurent Pinchart