| Message ID | 20210118003428.568892-7-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce intel_skl_int3472 driver | expand |
Hi 2021. január 18., hétfő 1:34 keltezéssel, Daniel Scally írta: > 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. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - Switched to a module registering a platform driver to run > the dummy ACPI devices, plus an i2c driver to replace and extend > the existing tps68470 driver > - Added clock handling functions to the int3472-discrete driver > - A whole bunch of other changes too numerous to enumerate > MAINTAINERS | 5 + > drivers/platform/x86/Kconfig | 25 + > drivers/platform/x86/Makefile | 4 + > .../platform/x86/intel_skl_int3472_common.c | 100 ++++ > .../platform/x86/intel_skl_int3472_common.h | 100 ++++ > .../platform/x86/intel_skl_int3472_discrete.c | 496 ++++++++++++++++++ > .../platform/x86/intel_skl_int3472_tps68470.c | 145 +++++ > 7 files changed, 875 insertions(+) > create mode 100644 drivers/platform/x86/intel_skl_int3472_common.c > create mode 100644 drivers/platform/x86/intel_skl_int3472_common.h > create mode 100644 drivers/platform/x86/intel_skl_int3472_discrete.c > create mode 100644 drivers/platform/x86/intel_skl_int3472_tps68470.c Have you considered putting the source (and header) files into a dedicated folder? I think it'd help manageability in the long run, and it'd be immediately obvious that these source files form a single "unit". Regards, Barnabás Pőcze
2021. január 18., hétfő 14:51 keltezéssel, Andy Shevchenko írta: > On Mon, Jan 18, 2021 at 11:12:34AM +0000, Barnabás Pőcze wrote: > > 2021. január 18., hétfő 1:34 keltezéssel, Daniel Scally írta: > > > Have you considered putting the source (and header) files into a dedicated > > folder? I think it'd help manageability in the long run, and it'd be immediately > > obvious that these source files form a single "unit". > > What would be the folder name? Because, for example, intel_cht_int33fe* have no > folder (yet?) and here it's kinda similar case when HID describes something > else than just one IP. I think "intel_skl_int3472" would not be a bad name for the folder. And I believe "intel_cht_int33fe" could be given its own folder as well. Regards, Barnabás Pőcze
On Mon, Jan 18, 2021 at 04:32:54PM +0100, Hans de Goede wrote: > On 1/18/21 4:23 PM, andriy.shevchenko@linux.intel.com wrote: ... > 1. Using a folder is fine, desirable even > 2. I've some concerns about the name, but I'm not really objecting, > just giving my 2 cents. Let's get into compromised summary: - create a folder for these driver files - name it without _skl_ while leaving this in the file / driver names Does everybody agree on this approach?
On 18/01/2021 15:48, andriy.shevchenko@linux.intel.com wrote: > On Mon, Jan 18, 2021 at 04:32:54PM +0100, Hans de Goede wrote: >> On 1/18/21 4:23 PM, andriy.shevchenko@linux.intel.com wrote: > ... > >> 1. Using a folder is fine, desirable even >> 2. I've some concerns about the name, but I'm not really objecting, >> just giving my 2 cents. > Let's get into compromised summary: > - create a folder for these driver files > - name it without _skl_ while leaving this in the file / driver names > > Does everybody agree on this approach? > Works for me!
Hi, On 1/18/21 5:00 PM, Daniel Scally wrote: > > On 18/01/2021 15:48, andriy.shevchenko@linux.intel.com wrote: >> On Mon, Jan 18, 2021 at 04:32:54PM +0100, Hans de Goede wrote: >>> On 1/18/21 4:23 PM, andriy.shevchenko@linux.intel.com wrote: >> ... >> >>> 1. Using a folder is fine, desirable even >>> 2. I've some concerns about the name, but I'm not really objecting, >>> just giving my 2 cents. >> Let's get into compromised summary: >> - create a folder for these driver files >> - name it without _skl_ while leaving this in the file / driver names >> >> Does everybody agree on this approach? >> > > Works for me! Also works for me. Regards, Hans
Hi Andy, Laurent On 18/01/2021 21:19, Daniel Scally wrote: >>>> +static const struct clk_ops skl_int3472_clock_ops = { >>>> + .prepare = skl_int3472_clk_prepare, >>>> + .unprepare = skl_int3472_clk_unprepare, >>>> + .enable = skl_int3472_clk_enable, >>>> + .disable = skl_int3472_clk_disable, >>>> +}; >> Yeah, sounds like reinventing clk-gpio.c. >> >> static const struct clk_ops clk_gpio_gate_ops = { >> .enable = clk_gpio_gate_enable, >> .disable = clk_gpio_gate_disable, >> .is_enabled = clk_gpio_gate_is_enabled, >> }; >> >> (Or is it mux? It has support there as well. >> > Hmm, yeah, this looks like it would work actually. So I think I'd need to: > > > 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver > > 2. Register a platform device to bind to the clk-gpio driver > > 3. Register a gpio lookup table so that the clk-gpio driver can find the > gpio in question using gpiod_get() > > > And that looks like it will work; I'll try it. I'm more and more confident that this will work, but it has some knock-on effects: The both clk and regulator gpio driver expects to be able to fetch the GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of course, so we need to add another GPIO lookup table so those drivers can see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will be so we can set the .dev_id member of a gpiod_lookup_table to that value, but that isn't set until _after_ the pdev is registered (because it has to figure out the id, we can't manually set the IDs because there could be more than one instance of int3472-discrete bound to multiple PMIC devices, and we don't know which id the current one should have). Finally, we can't wait until the device is registered because it immediately probes, can't find the GPIO and then fails probe. It's similar problem that causes us to need the i2c-acpi name format macros, but complicated by the dynamic ID part of dev_name(&pdev->dev) Solving it is a bit of a sticky one; perhaps something like moving the dev_set_name() part of platform_device_add() [1] to its own function, that's called in both platform_device_alloc() and platform_device_register(). That way it would be available before the device itself was registered, meaning we could create the lookup table before it probes the driver. (also, Laurent, if we did it this way we wouldn't be able to also handle the led-indicator GPIO here without some fairly major rework) [1] https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L563
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
Hi Daniel, On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > On 18/01/2021 21:19, Daniel Scally wrote: > >>>> +static const struct clk_ops skl_int3472_clock_ops = { > >>>> + .prepare = skl_int3472_clk_prepare, > >>>> + .unprepare = skl_int3472_clk_unprepare, > >>>> + .enable = skl_int3472_clk_enable, > >>>> + .disable = skl_int3472_clk_disable, > >>>> +}; > >> > >> Yeah, sounds like reinventing clk-gpio.c. > >> > >> static const struct clk_ops clk_gpio_gate_ops = { > >> .enable = clk_gpio_gate_enable, > >> .disable = clk_gpio_gate_disable, > >> .is_enabled = clk_gpio_gate_is_enabled, > >> }; > >> > >> (Or is it mux? It has support there as well. > >> > > Hmm, yeah, this looks like it would work actually. So I think I'd need to: > > > > 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver > > > > 2. Register a platform device to bind to the clk-gpio driver > > > > 3. Register a gpio lookup table so that the clk-gpio driver can find the > > gpio in question using gpiod_get() > > > > And that looks like it will work; I'll try it. > > I'm more and more confident that this will work, but it has some > knock-on effects: > > The both clk and regulator gpio driver expects to be able to fetch the > GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of > course, so we need to add another GPIO lookup table so those drivers can > see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will > be so we can set the .dev_id member of a gpiod_lookup_table to that > value, but that isn't set until _after_ the pdev is registered (because > it has to figure out the id, we can't manually set the IDs because there > could be more than one instance of int3472-discrete bound to multiple > PMIC devices, and we don't know which id the current one should have). > Finally, we can't wait until the device is registered because it > immediately probes, can't find the GPIO and then fails probe. > > It's similar problem that causes us to need the i2c-acpi name format > macros, but complicated by the dynamic ID part of dev_name(&pdev->dev) > > Solving it is a bit of a sticky one; perhaps something like moving the > dev_set_name() part of platform_device_add() [1] to its own function, > that's called in both platform_device_alloc() and > platform_device_register(). That way it would be available before the > device itself was registered, meaning we could create the lookup table > before it probes the driver. > > (also, Laurent, if we did it this way we wouldn't be able to also handle > the led-indicator GPIO here without some fairly major rework) Given the additional complexity I don't think it's worth it, your implementation is fine and code duplication with clk-gpio is minimal. > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L563 -- Regards, Laurent Pinchart
Hi Laurent On 19/01/2021 06:19, Laurent Pinchart wrote: > 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.. Is that the deciding factor? I'd say that's unlikely, but what do I know? Then again, is there actually any downside to calling gpiod_set_value() in the prepare() function instead? If not, may as well be safe. >>>> + } >>>> + >>>> + 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 ? I haven't ever seen a DSDT that didn't include a 0x0c pin to enable the clock, though that doesn't necessarily mean they're always there. Plenty of driver datasheets say they're happy for the external clock to be free running, so it could just be always active I suppose. > Another option would be to let userspace handle that GPIO, but we then > need to convey it to userspace. Can you point me to an example of that to look at perhaps? >>>> + >>>> + 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.
On Mon, Jan 18, 2021 at 09:19:52PM +0000, Daniel Scally wrote: > On 18/01/2021 14:46, Andy Shevchenko wrote: > > On Mon, Jan 18, 2021 at 11:15:21AM +0200, Laurent Pinchart wrote: > >> On Mon, Jan 18, 2021 at 12:34:27AM +0000, Daniel Scally wrote: ... > >>> +static struct i2c_driver int3472_tps68470 = { > >>> + .driver = { > >>> + .name = "int3472-tps68470", > >>> + .acpi_match_table = int3472_device_id, > >>> + }, > >>> + .probe_new = skl_int3472_tps68470_probe, > >>> +}; > > I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > > be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > > via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > > Sorry, I'm a bit confused by this. The i2c device is already > present...we just want the driver to bind to them, so what role do those > functions have there? What I meant is something like *_i2c.c real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI involved (and it sounds like it should be mfd/tps one, in which you just cut out ACPI IDs and convert to pure I²C one, that what I had suggested in the first place) *_proxy.c GPIO proxy as library *.c platform driver with ACPI ID, in which ->probe() we actually instantiate above via calling i2c_acpi_new_device(), *if needed*, along with GPIO proxy ... > >>> +struct int3472_gpio_clock { > >>> + struct clk *clk; > >>> + struct clk_hw clk_hw; > >>> + struct clk_lookup *cl; > >>> + struct gpio_desc *gpio; > >>> +}; > >>> +static const struct clk_ops skl_int3472_clock_ops = { > >>> + .prepare = skl_int3472_clk_prepare, > >>> + .unprepare = skl_int3472_clk_unprepare, > >>> + .enable = skl_int3472_clk_enable, > >>> + .disable = skl_int3472_clk_disable, > >>> +}; > > Wondering if this has some similarities with and actually can utilize clk-gpio > > driver. > > Yeah, sounds like reinventing clk-gpio.c. > > > > static const struct clk_ops clk_gpio_gate_ops = { > > .enable = clk_gpio_gate_enable, > > .disable = clk_gpio_gate_disable, > > .is_enabled = clk_gpio_gate_is_enabled, > > }; > > > > Or is it mux? It has support there as well. > > > Hmm, yeah, this looks like it would work actually. So I think I'd need to: > > > 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver > > 2. Register a platform device to bind to the clk-gpio driver > > 3. Register a gpio lookup table so that the clk-gpio driver can find the > gpio in question using gpiod_get() > > And that looks like it will work; I'll try it. You need to modify clk-gpio.c to export clk_hw_register_gpio_gate() clk_hw_register_gpio_mux() (perhaps it will require to add *_unregister() counterparts) and call it from your code. See, for example, how clk_hw_unregister_fixed_rate() is being used. Another case is to add a helper directly into clk-gpio and call it instead of clk_hw_*() one, see how clk_register_fractional_divider() is implemented and used. ... > >>> + /* Lenovo Miix 510-12ISK - OV5648, Rear */ > >>> + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL}, > >>> + /* Surface Go 1&2 - OV5693, Front */ > >>> + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL}, > > I'm wondering if you should use same I2C format macro and create this > > dynamically? Or rather find a corresponding ACPI device instance and > > copy it's name? ... > > The supply name needs hard-coding really, but the device name I suppose > can come from int3472->sensor_name. To be strict in terms you are using "device instance name" in the REGULATOR_SUPPLY() second parameter. Because "device name" is generic and doesn't point to the actual *instance* of the device in the system. So, and "device name instance" we may get only by traversing through the (ACPI) bus and find corresponding struct device and derive name from it. Same way like you have done in previous patch series. Because there is no guarantee that, e.g., i2c-INT33BE:00 will be present on the system and moreover there is no guarantee that if two INT33BE or more devices are present you will get :00 always for the one you need! ... > >>> + ret = ERR_PTR(-ENODEV); > > This seems redundant. Or are you expecting ARRAY_SIZE() to be 0? > > If no, you may add static_assert() near to the array definition. > > It **could** become 0, if the entries I've added are removed in future > because the sensors are no longer supported or something. There might be > no sensor_module_config for a given device. We only need to supply one if > > a) The platform has a 0x0b type GPIO, which means we need to define a > supply name the driver is expecting > > b) The GPIO functions deviate from documented purpose, which means we > need to supply a remapping struct > > Otherwise, there's no need for it. I see. So, up to you then. > > > >>> + > >>> + int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc, > >>> + &cfg); > >>> + if (IS_ERR(int3472->regulator.rdev)) { > >>> + ret = PTR_ERR(int3472->regulator.rdev); > >>> + goto err_free_gpio; > >>> + } > > Similar here, can we utilize gpio-regulator.c? > > > > Also yes probably, with the same steps as for the clocks. Again, I'll > try that out, thanks very much. You may look at arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c (but in your case you probably don't need a GPIO lookup table, just check). ... > >>> + dev_warn(&int3472->pdev->dev, > >>> + "GPIO type 0x%llx unknown; the sensor may not work\n", > >>> + (obj->integer.value & 0xff)); > >> No need for parentheses. > > And instead of "%llx" with " & 0xff" you may use "%x" with "(u8)" cast. > > However, I don't think we need to show only last byte, because it may give > > wrong impression on values like "0x100". > > But in this case only the last byte holds the type information, second > lowest byte is the pin number. So as we understand it, 0x100 would be > invalid anyway. Ah, indeed. So, I think the best is to have %hhx and see if you can supply just obj->integer.value and compiler won't warn about type mismatch. ... > >>> + } else { > >>> + opregion_dev = skl_int3472_register_pdev("tps68470_pmic_opregion", > >>> + &client->dev); > >>> + if (IS_ERR(opregion_dev)) { > >>> + ret = PTR_ERR(opregion_dev); > >>> + goto err_free_gpio; > >>> + } > >>> + } > >> I wonder if this could be simplified by using devm_mfd_add_devices. You > >> could have two arrays of mfd_cell, one for each case. > > Yeah, which effectively means that we should have some kind of mfd/tps68470 in > > place. > > Can you expand on what you mean by that a little, please? The very first comment in this reply should hopefully shed a light on my idea. -- With Best Regards, Andy Shevchenko
On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > On 18/01/2021 21:19, Daniel Scally wrote: > I'm more and more confident that this will work, but it has some > knock-on effects: > > The both clk and regulator gpio driver expects to be able to fetch the > GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of > course, so we need to add another GPIO lookup table so those drivers can > see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will > be so we can set the .dev_id member of a gpiod_lookup_table to that > value, but that isn't set until _after_ the pdev is registered (because > it has to figure out the id, we can't manually set the IDs because there > could be more than one instance of int3472-discrete bound to multiple > PMIC devices, and we don't know which id the current one should have). > Finally, we can't wait until the device is registered because it > immediately probes, can't find the GPIO and then fails probe. > > It's similar problem that causes us to need the i2c-acpi name format > macros, but complicated by the dynamic ID part of dev_name(&pdev->dev) > > Solving it is a bit of a sticky one; perhaps something like moving the > dev_set_name() part of platform_device_add() [1] to its own function, > that's called in both platform_device_alloc() and > platform_device_register(). That way it would be available before the > device itself was registered, meaning we could create the lookup table > before it probes the driver. See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of methods to be able to use it as a library. > (also, Laurent, if we did it this way we wouldn't be able to also handle > the led-indicator GPIO here without some fairly major rework) LED indicators are done as LED class devices (see plenty of examples in PDx86 drivers: drivers/platform/x86/)
Morning Andy On 19/01/2021 09:33, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: >> On 18/01/2021 21:19, Daniel Scally wrote: >> I'm more and more confident that this will work, but it has some >> knock-on effects: >> >> The both clk and regulator gpio driver expects to be able to fetch the >> GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of >> course, so we need to add another GPIO lookup table so those drivers can >> see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will >> be so we can set the .dev_id member of a gpiod_lookup_table to that >> value, but that isn't set until _after_ the pdev is registered (because >> it has to figure out the id, we can't manually set the IDs because there >> could be more than one instance of int3472-discrete bound to multiple >> PMIC devices, and we don't know which id the current one should have). >> Finally, we can't wait until the device is registered because it >> immediately probes, can't find the GPIO and then fails probe. >> >> It's similar problem that causes us to need the i2c-acpi name format >> macros, but complicated by the dynamic ID part of dev_name(&pdev->dev) >> >> Solving it is a bit of a sticky one; perhaps something like moving the >> dev_set_name() part of platform_device_add() [1] to its own function, >> that's called in both platform_device_alloc() and >> platform_device_register(). That way it would be available before the >> device itself was registered, meaning we could create the lookup table >> before it probes the driver. > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of > methods to be able to use it as a library. Ack! Ok, I thought about that the wrong way. I'll take another look tonight then. >> (also, Laurent, if we did it this way we wouldn't be able to also handle >> the led-indicator GPIO here without some fairly major rework) > LED indicators are done as LED class devices (see plenty of examples in PDx86 > drivers: drivers/platform/x86/) And this too - thanks very much
On Tue, Jan 19, 2021 at 08:21:23AM +0200, Laurent Pinchart wrote: > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > On 18/01/2021 21:19, Daniel Scally wrote: ... > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > the led-indicator GPIO here without some fairly major rework) > > Given the additional complexity I don't think it's worth it, your > implementation is fine and code duplication with clk-gpio is minimal. Making clk-gpio.c available as a library is a win in long term and reduces a possible duplication by others in the future. I bet we even might find already clk-gpio parts in some drivers.
On 19/01/2021 09:24, Andy Shevchenko wrote: >>>>> +static struct i2c_driver int3472_tps68470 = { >>>>> + .driver = { >>>>> + .name = "int3472-tps68470", >>>>> + .acpi_match_table = int3472_device_id, >>>>> + }, >>>>> + .probe_new = skl_int3472_tps68470_probe, >>>>> +}; >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... >> Sorry, I'm a bit confused by this. The i2c device is already >> present...we just want the driver to bind to them, so what role do those >> functions have there? > What I meant is something like > > *_i2c.c > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > involved (and it sounds like it should be mfd/tps one, in which you > just cut out ACPI IDs and convert to pure I²C one, that what I had > suggested in the first place) Ahh; sorry - i misunderstood what you meant there. I understand now I think, but there is one complication; the ACPI subsystem already creates a client for that i2c adapter and address; i2c_new_client_device() includes a check to see whether that adapter / address combination has an i2c device already. So we would have to have the platform driver with ACPI ID first find the existing i2c_client and unregister it before registering the new one...the existing clients have a name matching the ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an i2c_device_id of course. > > *_proxy.c > GPIO proxy as library > > *.c > platform driver with ACPI ID, in which ->probe() we actually instantiate > above via calling i2c_acpi_new_device(), *if needed*, along with GPIO > proxy > > ... > >>>>> +struct int3472_gpio_clock { >>>>> + struct clk *clk; >>>>> + struct clk_hw clk_hw; >>>>> + struct clk_lookup *cl; >>>>> + struct gpio_desc *gpio; >>>>> +}; >>>>> +static const struct clk_ops skl_int3472_clock_ops = { >>>>> + .prepare = skl_int3472_clk_prepare, >>>>> + .unprepare = skl_int3472_clk_unprepare, >>>>> + .enable = skl_int3472_clk_enable, >>>>> + .disable = skl_int3472_clk_disable, >>>>> +}; >>> Wondering if this has some similarities with and actually can utilize clk-gpio >>> driver. >>> Yeah, sounds like reinventing clk-gpio.c. >>> >>> static const struct clk_ops clk_gpio_gate_ops = { >>> .enable = clk_gpio_gate_enable, >>> .disable = clk_gpio_gate_disable, >>> .is_enabled = clk_gpio_gate_is_enabled, >>> }; >>> >>> Or is it mux? It has support there as well. >>> >> Hmm, yeah, this looks like it would work actually. So I think I'd need to: >> >> >> 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver >> >> 2. Register a platform device to bind to the clk-gpio driver >> >> 3. Register a gpio lookup table so that the clk-gpio driver can find the >> gpio in question using gpiod_get() >> >> And that looks like it will work; I'll try it. > You need to modify clk-gpio.c to export > > clk_hw_register_gpio_gate() > clk_hw_register_gpio_mux() > > (perhaps it will require to add *_unregister() counterparts) and call it from > your code. > > See, for example, how clk_hw_unregister_fixed_rate() is being used. Another > case is to add a helper directly into clk-gpio and call it instead of > clk_hw_*() one, see how clk_register_fractional_divider() is implemented and > used. I'll take a look, thanks > ... > >>>>> + /* Lenovo Miix 510-12ISK - OV5648, Rear */ >>>>> + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL}, >>>>> + /* Surface Go 1&2 - OV5693, Front */ >>>>> + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL}, >>> I'm wondering if you should use same I2C format macro and create this >>> dynamically? Or rather find a corresponding ACPI device instance and >>> copy it's name? ... >> The supply name needs hard-coding really, but the device name I suppose >> can come from int3472->sensor_name. > To be strict in terms you are using "device instance name" in the > REGULATOR_SUPPLY() second parameter. Because "device name" is generic and > doesn't point to the actual *instance* of the device in the system. > > So, and "device name instance" we may get only by traversing through the (ACPI) > bus and find corresponding struct device and derive name from it. Same way like > you have done in previous patch series. > > Because there is no guarantee that, e.g., i2c-INT33BE:00 will be present on > the system and moreover there is no guarantee that if two INT33BE or more > devices are present you will get :00 always for the one you need! Mm, good point, hadn't considered two identical sensors on the same platform. Alright; I'll think about this in more detail, thank you. >>>>> + opregion_dev = skl_int3472_register_pdev("tps68470_pmic_opregion", >>>>> + &client->dev); >>>>> + if (IS_ERR(opregion_dev)) { >>>>> + ret = PTR_ERR(opregion_dev); >>>>> + goto err_free_gpio; >>>>> + } >>>>> + } >>>> I wonder if this could be simplified by using devm_mfd_add_devices. You >>>> could have two arrays of mfd_cell, one for each case. >>> Yeah, which effectively means that we should have some kind of mfd/tps68470 in >>> place. >> Can you expand on what you mean by that a little, please? > The very first comment in this reply should hopefully shed a light on my idea. > It did, thanks
Hi Daniel, On 18/01/2021 00:34, Daniel Scally 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. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - Switched to a module registering a platform driver to run > the dummy ACPI devices, plus an i2c driver to replace and extend > the existing tps68470 driver > - Added clock handling functions to the int3472-discrete driver > - A whole bunch of other changes too numerous to enumerate > MAINTAINERS | 5 + > drivers/platform/x86/Kconfig | 25 + > drivers/platform/x86/Makefile | 4 + > .../platform/x86/intel_skl_int3472_common.c | 100 ++++ > .../platform/x86/intel_skl_int3472_common.h | 100 ++++ > .../platform/x86/intel_skl_int3472_discrete.c | 496 ++++++++++++++++++ > .../platform/x86/intel_skl_int3472_tps68470.c | 145 +++++ > 7 files changed, 875 insertions(+) > create mode 100644 drivers/platform/x86/intel_skl_int3472_common.c > create mode 100644 drivers/platform/x86/intel_skl_int3472_common.h > create mode 100644 drivers/platform/x86/intel_skl_int3472_discrete.c > create mode 100644 drivers/platform/x86/intel_skl_int3472_tps68470.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a091b496fdd8..c4ed8c3bc58e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9147,6 +9147,11 @@ S: Maintained > F: arch/x86/include/asm/intel_scu_ipc.h > F: drivers/platform/x86/intel_scu_* > > +INTEL SKL INT3472 ACPI DEVICE DRIVER > +M: Daniel Scally <djrscally@gmail.com> > +S: Maintained > +F: drivers/platform/x86/intel_skl_int3472_* > + > INTEL SPEED SELECT TECHNOLOGY > M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > L: platform-driver-x86@vger.kernel.org > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 91e6176cdfbd..916b077df2d5 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -844,6 +844,31 @@ config INTEL_CHT_INT33FE > device and CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m > for Type-C device. > > +config INTEL_SKL_INT3472 > + tristate "Intel SkyLake ACPI INT3472 Driver" > + depends on X86 && ACPI > + select REGMAP_I2C I've tried compiling this as a built in and a module and on my minimal config I had failures on both for regulator_register and regulator_unregister. I suspect this needs to have either a selects or a depends upon CONFIG_REGULATOR -- Regards Kieran > + help > + This driver adds support for the INT3472 ACPI devices found on some > + Intel SkyLake devices. > + > + There are 3 kinds of INT3472 ACPI device possible; two for devices > + designed for Windows (either with or without a physical tps68470 > + PMIC) and one designed for Chrome OS. 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. > + > + Say Y or M here if you have a SkyLake device designed for use > + with Windows or ChromeOS. Say N here if you are not sure. > + > + The module will be named "intel-skl-int3472" > + > config INTEL_HID_EVENT > tristate "INTEL HID Event" > depends on ACPI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 581475f59819..ae29c66842ca 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -86,6 +86,10 @@ obj-$(CONFIG_INTEL_HID_EVENT) += intel-hid.o > obj-$(CONFIG_INTEL_INT0002_VGPIO) += intel_int0002_vgpio.o > obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o > obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o > +obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472.o > +intel_skl_int3472-objs := intel_skl_int3472_common.o \ > + intel_skl_int3472_discrete.o \ > + intel_skl_int3472_tps68470.o > obj-$(CONFIG_INTEL_VBTN) += intel-vbtn.o > > # MSI > diff --git a/drivers/platform/x86/intel_skl_int3472_common.c b/drivers/platform/x86/intel_skl_int3472_common.c > new file mode 100644 > index 000000000000..08cb9d3c06aa > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_common.c > @@ -0,0 +1,100 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Author: Dan Scally <djrscally@gmail.com> */ > + > +#include <linux/acpi.h> > +#include <linux/i2c.h> > +#include <linux/platform_device.h> > + > +#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"); > + return -ENODEV; > + } > + > + if (obj->type != ACPI_TYPE_BUFFER) { > + dev_err(&adev->dev, "CLDB object is not an ACPI buffer\n"); > + ret = -EINVAL; > + goto out_free_buff; > + } > + > + if (obj->buffer.length > sizeof(*cldb)) { > + dev_err(&adev->dev, "The CLDB buffer is too large\n"); > + ret = -EINVAL; > + goto out_free_buff; > + } > + > + memcpy(cldb, obj->buffer.pointer, obj->buffer.length); > + > +out_free_buff: > + kfree(buffer.pointer); > + return ret; > +} > + > +static const struct acpi_device_id int3472_device_id[] = { > + { "INT3472", 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, int3472_device_id); > + > +static struct platform_driver int3472_discrete = { > + .driver = { > + .name = "int3472-discrete", > + .acpi_match_table = int3472_device_id, > + }, > + .probe = skl_int3472_discrete_probe, > + .remove = skl_int3472_discrete_remove, > +}; > + > +static struct i2c_driver int3472_tps68470 = { > + .driver = { > + .name = "int3472-tps68470", > + .acpi_match_table = int3472_device_id, > + }, > + .probe_new = skl_int3472_tps68470_probe, > +}; > + > +static int skl_int3472_init(void) > +{ > + int ret = 0; > + > + ret = platform_driver_register(&int3472_discrete); > + if (ret) > + return ret; > + > + ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470); > + if (ret) > + goto err_unregister_plat_drv; > + > + return 0; > + > +err_unregister_plat_drv: > + platform_driver_unregister(&int3472_discrete); > + return ret; > +} > +module_init(skl_int3472_init); > + > +static void skl_int3472_exit(void) > +{ > + platform_driver_unregister(&int3472_discrete); > + i2c_del_driver(&int3472_tps68470); > +} > +module_exit(skl_int3472_exit); > + > +MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Device Driver"); > +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/platform/x86/intel_skl_int3472_common.h b/drivers/platform/x86/intel_skl_int3472_common.h > new file mode 100644 > index 000000000000..9acd0ce7aed9 > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_common.h > @@ -0,0 +1,100 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Author: Dan Scally <djrscally@gmail.com> */ > +#include <linux/regulator/machine.h> > +#include <linux/clk-provider.h> > +#include <linux/gpio/machine.h> > +#include <linux/regulator/driver.h> > +#include <linux/types.h> > + > +/* PMIC GPIO Types */ > +#define INT3472_GPIO_TYPE_RESET 0x00 > +#define INT3472_GPIO_TYPE_POWERDOWN 0x01 > +#define INT3472_GPIO_TYPE_CLK_ENABLE 0x0c > +#define INT3472_GPIO_TYPE_POWER_ENABLE 0x0b > +#define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d > +#define INT3472_PDEV_MAX_NAME_LEN 23 > +#define INT3472_MAX_SENSOR_GPIOS 3 > +#define GPIO_REGULATOR_NAME_LENGTH 27 > +#define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9 > + > +#define INT3472_REGULATOR(_NAME, _SUPPLY, _OPS) \ > + (const struct regulator_desc) { \ > + .name = _NAME, \ > + .supply_name = _SUPPLY, \ > + .id = 0, \ > + .type = REGULATOR_VOLTAGE, \ > + .ops = _OPS, \ > + .owner = THIS_MODULE, \ > + } > + > +#define INT3472_GPIO_FUNCTION_REMAP(_PIN, _FUNCTION) \ > + (const struct int3472_gpio_function_remap) { \ > + .documented = _PIN, \ > + .actual = _FUNCTION \ > + } > + > +#define to_int3472_clk(hw) \ > + container_of(hw, struct int3472_gpio_clock, clk_hw) > + > +struct int3472_cldb { > + u8 version; > + /* > + * control logic type > + * 0: UNKNOWN > + * 1: DISCRETE(CRD-D) > + * 2: PMIC TPS68470 > + * 3: PMIC uP6641 > + */ > + u8 control_logic_type; > + u8 control_logic_id; > + u8 sensor_card_sku; > + u8 reserved[28]; > +}; > + > +struct int3472_gpio_regulator { > + 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; > +}; > + > +struct int3472_gpio_clock { > + struct clk *clk; > + struct clk_hw clk_hw; > + struct clk_lookup *cl; > + struct gpio_desc *gpio; > +}; > + > +struct int3472_device { > + struct acpi_device *adev; > + struct platform_device *pdev; > + struct acpi_device *sensor; > + char *sensor_name; > + > + unsigned int n_gpios; /* how many GPIOs have we seen */ > + > + struct int3472_gpio_regulator regulator; > + struct int3472_gpio_clock clock; > + > + unsigned int n_sensor_gpios; /* how many have we mapped to sensor */ > + bool gpios_mapped; > + struct gpiod_lookup_table gpios; > +}; > + > +struct int3472_gpio_function_remap { > + char *documented; > + char *actual; > +}; > + > +struct int3472_sensor_config { > + char *sensor_module_name; > + struct regulator_consumer_supply supply_map; > + const struct int3472_gpio_function_remap *function_maps; > +}; > + > +int skl_int3472_discrete_probe(struct platform_device *pdev); > +int skl_int3472_discrete_remove(struct platform_device *pdev); > +int skl_int3472_tps68470_probe(struct i2c_client *client); > +int skl_int3472_get_cldb_buffer(struct acpi_device *adev, > + struct int3472_cldb *cldb); > diff --git a/drivers/platform/x86/intel_skl_int3472_discrete.c b/drivers/platform/x86/intel_skl_int3472_discrete.c > new file mode 100644 > index 000000000000..93d250dca92f > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_discrete.c > @@ -0,0 +1,496 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Author: Dan Scally <djrscally@gmail.com> */ > + > +#include <linux/acpi.h> > +#include <linux/clkdev.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/driver.h> > + > +#include "intel_skl_int3472_common.h" > + > +/* 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); > + > +/* > + * 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. 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[] = { > + INT3472_GPIO_FUNCTION_REMAP("reset", NULL), > + INT3472_GPIO_FUNCTION_REMAP("powerdown", "reset"), > + { } > +}; > + > +static struct int3472_sensor_config int3472_sensor_configs[] = { > + /* Lenovo Miix 510-12ISK - OV2680, Front */ > + { "GNDF140809R", { 0 }, ov2680_gpio_function_remaps}, > + /* Lenovo Miix 510-12ISK - OV5648, Rear */ > + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL}, > + /* Surface Go 1&2 - OV5693, Front */ > + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL}, > +}; > + > +/* > + * 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 = { 0 }; > + > +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); > + > + return 0; > +} > + > +static void skl_int3472_clk_disable(struct clk_hw *hw) > +{ > + struct int3472_gpio_clock *clk = to_int3472_clk(hw); > + > + gpiod_set_value(clk->gpio, 0); > +} > + > +static int skl_int3472_clk_prepare(struct clk_hw *hw) > +{ > + /* > + * We're just turning a GPIO on to enable, so nothing to do here, but > + * we want to provide the op so prepare_enable() works. > + */ > + return 0; > +} > + > +static void skl_int3472_clk_unprepare(struct clk_hw *hw) > +{ > + /* Likewise, nothing to do here... */ > +} > + > +static const struct clk_ops skl_int3472_clock_ops = { > + .prepare = skl_int3472_clk_prepare, > + .unprepare = skl_int3472_clk_unprepare, > + .enable = skl_int3472_clk_enable, > + .disable = skl_int3472_clk_disable, > +}; > + > +static struct int3472_sensor_config * > +skl_int3472_get_sensor_module_config(struct int3472_device *int3472) > +{ > + unsigned int i = ARRAY_SIZE(int3472_sensor_configs); > + struct int3472_sensor_config *ret; > + union acpi_object *obj; > + > + obj = acpi_evaluate_dsm_typed(int3472->sensor->handle, > + &cio2_sensor_module_guid, 0x00, > + 0x01, NULL, ACPI_TYPE_STRING); > + > + if (!obj) { > + dev_err(&int3472->pdev->dev, > + "Failed to get sensor module string from _DSM\n"); > + return ERR_PTR(-ENODEV); > + } > + > + if (obj->string.type != ACPI_TYPE_STRING) { > + dev_err(&int3472->pdev->dev, > + "Sensor _DSM returned a non-string value\n"); > + ret = ERR_PTR(-EINVAL); > + goto out_free_obj; > + } > + > + ret = ERR_PTR(-ENODEV); > + while (i--) { > + if (!strcmp(int3472_sensor_configs[i].sensor_module_name, > + obj->string.pointer)) { > + ret = &int3472_sensor_configs[i]; > + goto out_free_obj; > + } > + } > + > +out_free_obj: > + ACPI_FREE(obj); > + return ret; > +} > + > +static int skl_int3472_map_gpio_to_sensor(struct int3472_device *int3472, > + struct acpi_resource *ares, > + char *func, u32 polarity) > +{ > + char *path = ares->data.gpio.resource_source.string_ptr; > + struct int3472_sensor_config *sensor_config; > + struct gpiod_lookup table_entry; > + struct acpi_device *adev; > + acpi_handle handle; > + acpi_status status; > + int ret; > + > + sensor_config = skl_int3472_get_sensor_module_config(int3472); > + if (!IS_ERR(sensor_config) && sensor_config->function_maps) { > + unsigned int i = 0; > + > + while (sensor_config->function_maps[i].documented) { > + if (!strcmp(func, sensor_config->function_maps[i].documented)) { > + func = sensor_config->function_maps[i].actual; > + > + break; > + } > + > + i++; > + } > + } > + > + if (!func) > + return 0; > + > + if (int3472->n_sensor_gpios >= INT3472_MAX_SENSOR_GPIOS) { > + dev_warn(&int3472->pdev->dev, "Too many GPIOs mapped\n"); > + return -EINVAL; > + } > + > + 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); > + > + memcpy(&int3472->gpios.table[int3472->n_sensor_gpios], &table_entry, > + sizeof(table_entry)); > + > + int3472->n_sensor_gpios++; > + > + return 0; > +} > + > +static int skl_int3472_register_clock(struct int3472_device *int3472, > + struct acpi_resource *ares) > +{ > + char *path = ares->data.gpio.resource_source.string_ptr; > + struct clk_init_data init = { }; > + int ret = 0; > + > + init.name = kasprintf(GFP_KERNEL, "%s-clk", > + acpi_dev_name(int3472->adev)); > + init.ops = &skl_int3472_clock_ops; > + > + int3472->clock.gpio = acpi_get_gpiod(path, > + ares->data.gpio.pin_table[0]); > + if (IS_ERR(int3472->clock.gpio)) { > + ret = PTR_ERR(int3472->clock.gpio); > + goto out_free_init_name; > + } > + > + int3472->clock.clk_hw.init = &init; > + int3472->clock.clk = clk_register(&int3472->adev->dev, > + &int3472->clock.clk_hw); > + if (IS_ERR(int3472->clock.clk)) { > + ret = PTR_ERR(int3472->clock.clk); > + goto err_put_gpio; > + } > + > + int3472->clock.cl = clkdev_create(int3472->clock.clk, "xvclk", > + int3472->sensor_name); > + if (IS_ERR_OR_NULL(int3472->clock.cl)) > + goto err_unregister_clk; > + > + goto out_free_init_name; > + > +err_unregister_clk: > + clk_unregister(int3472->clock.clk); > +err_put_gpio: > + gpiod_put(int3472->clock.gpio); > +out_free_init_name: > + kfree(init.name); > + > + return ret; > +} > + > +static int skl_int3472_register_regulator(struct int3472_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 = skl_int3472_get_sensor_module_config(int3472); > + if (IS_ERR_OR_NULL(sensor_config)) { > + dev_err(&int3472->pdev->dev, "No sensor module config\n"); > + return PTR_ERR(sensor_config); > + } > + > + if (!sensor_config->supply_map.supply) { > + dev_err(&int3472->pdev->dev, "No supply name defined\n"); > + return -ENODEV; > + } > + > + init_data.supply_regulator = NULL; > + 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"); > + 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]); > + if (IS_ERR(int3472->regulator.gpio)) { > + dev_err(&int3472->pdev->dev, "Failed to get GPIO line\n"); > + return PTR_ERR(int3472->regulator.gpio); > + } > + > + cfg.dev = &int3472->adev->dev; > + cfg.init_data = &init_data; > + cfg.ena_gpiod = int3472->regulator.gpio; > + > + int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc, > + &cfg); > + if (IS_ERR(int3472->regulator.rdev)) { > + ret = PTR_ERR(int3472->regulator.rdev); > + goto err_free_gpio; > + } > + > + return 0; > + > +err_free_gpio: > + gpiod_put(int3472->regulator.gpio); > + > + return ret; > +} > + > +/** > + * skl_int3472_handle_gpio_resources: maps PMIC resources to consuming sensor > + * @ares: A pointer to a &struct acpi_resource > + * @data: A pointer to a &struct int3472_device > + * > + * This function handles GPIO resources that are against an INT3472 > + * ACPI device, by checking the value of the corresponding _DSM entry. > + * This will return a 32bit int, where the lowest byte represents the > + * function of the GPIO pin: > + * > + * 0x00 Reset > + * 0x01 Power down > + * 0x0b Power enable > + * 0x0c Clock enable > + * 0x0d Privacy LED > + * > + * There are some known platform specific quirks where that does not quite > + * hold up; for example where a pin with type 0x01 (Power down) is mapped to > + * a sensor pin that performs a reset function or entries in _CRS and _DSM that > + * do not actually correspond to a physical connection. These will be handled by > + * the mapping sub-functions. > + * > + * GPIOs will either be mapped directly to the sensor device or else used > + * to create clocks and regulators via the usual frameworks. > + * > + * Return: > + * * 0 - When all resources found are handled properly. > + * * -EINVAL - If the resource is not a GPIO IO resource > + * * -ENODEV - If the resource has no corresponding _DSM entry > + * * -Other - Errors propagated from one of the sub-functions. > + */ > +static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > + void *data) > +{ > + struct int3472_device *int3472 = data; > + union acpi_object *obj; > + int ret = 0; > + > + if (ares->type != ACPI_RESOURCE_TYPE_GPIO || > + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO) > + return EINVAL; /* Deliberately positive so parsing continues */ > + > + /* > + * n_gpios + 2 because the index of this _DSM function is 1-based and > + * the first function is just a count. > + */ > + obj = acpi_evaluate_dsm_typed(int3472->adev->handle, > + &int3472_gpio_guid, 0x00, > + int3472->n_gpios + 2, > + NULL, ACPI_TYPE_INTEGER); > + > + if (!obj) { > + dev_warn(&int3472->pdev->dev, > + "No _DSM entry for this GPIO pin\n"); > + return ENODEV; > + } > + > + switch (obj->integer.value & 0xff) { > + case INT3472_GPIO_TYPE_RESET: > + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, "reset", > + GPIO_ACTIVE_LOW); > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map reset pin to sensor\n"); > + > + break; > + case INT3472_GPIO_TYPE_POWERDOWN: > + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, "powerdown", > + GPIO_ACTIVE_LOW); > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map powerdown pin to sensor\n"); > + > + break; > + case INT3472_GPIO_TYPE_CLK_ENABLE: > + ret = skl_int3472_register_clock(int3472, ares); > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map clock to sensor\n"); > + > + break; > + case INT3472_GPIO_TYPE_POWER_ENABLE: > + ret = skl_int3472_register_regulator(int3472, ares); > + if (ret) { > + dev_err(&int3472->pdev->dev, > + "Failed to map regulator to sensor\n"); > + } > + > + break; > + case INT3472_GPIO_TYPE_PRIVACY_LED: > + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, > + "indicator-led", > + GPIO_ACTIVE_HIGH); > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map indicator led to sensor\n"); > + > + break; > + default: > + dev_warn(&int3472->pdev->dev, > + "GPIO type 0x%llx unknown; the sensor may not work\n", > + (obj->integer.value & 0xff)); > + ret = EINVAL; > + } > + > + int3472->n_gpios++; > + ACPI_FREE(obj); > + > + return ret; > +} > + > +static int skl_int3472_parse_crs(struct int3472_device *int3472) > +{ > + struct list_head resource_list; > + int ret = 0; > + > + INIT_LIST_HEAD(&resource_list); > + > + ret = acpi_dev_get_resources(int3472->adev, &resource_list, > + skl_int3472_handle_gpio_resources, > + int3472); > + > + if (!ret) { > + gpiod_add_lookup_table(&int3472->gpios); > + int3472->gpios_mapped = true; > + } > + > + acpi_dev_free_resource_list(&resource_list); > + > + return ret; > +} > + > +int skl_int3472_discrete_probe(struct platform_device *pdev) > +{ > + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev); > + struct int3472_device *int3472; > + struct int3472_cldb cldb; > + int ret = 0; > + > + ret = skl_int3472_get_cldb_buffer(adev, &cldb); > + if (ret || cldb.control_logic_type != 1) > + return -EINVAL; > + > + int3472 = kzalloc(sizeof(*int3472) + > + ((INT3472_MAX_SENSOR_GPIOS + 1) * sizeof(struct gpiod_lookup)), > + GFP_KERNEL); > + if (!int3472) > + return -ENOMEM; > + > + int3472->adev = adev; > + int3472->pdev = pdev; > + platform_set_drvdata(pdev, int3472); > + > + int3472->sensor = acpi_dev_get_next_dep_dev(adev, NULL); > + if (!int3472->sensor) { > + dev_err(&pdev->dev, > + "This INT3472 entry seems to have no dependents.\n"); > + ret = -ENODEV; > + goto err_free_int3472; > + } > + int3472->sensor_name = i2c_acpi_dev_name(int3472->sensor); > + int3472->gpios.dev_id = int3472->sensor_name; > + > + ret = skl_int3472_parse_crs(int3472); > + if (ret) { > + skl_int3472_discrete_remove(pdev); > + goto err_return_ret; > + } > + > + return 0; > + > +err_free_int3472: > + kfree(int3472); > +err_return_ret: > + return ret; > +} > + > +int skl_int3472_discrete_remove(struct platform_device *pdev) > +{ > + struct int3472_device *int3472; > + > + int3472 = platform_get_drvdata(pdev); > + > + if (int3472->gpios_mapped) > + gpiod_remove_lookup_table(&int3472->gpios); > + > + if (!IS_ERR_OR_NULL(int3472->regulator.rdev)) { > + gpiod_put(int3472->regulator.gpio); > + regulator_unregister(int3472->regulator.rdev); > + } > + > + if (!IS_ERR_OR_NULL(int3472->clock.clk)) { > + gpiod_put(int3472->clock.gpio); > + clk_unregister(int3472->clock.clk); > + clkdev_drop(int3472->clock.cl); > + } > + > + acpi_dev_put(int3472->sensor); > + > + kfree(int3472->sensor_name); > + kfree(int3472); > + > + return 0; > +} > diff --git a/drivers/platform/x86/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel_skl_int3472_tps68470.c > new file mode 100644 > index 000000000000..9bda65d34987 > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_tps68470.c > @@ -0,0 +1,145 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Author: Dan Scally <djrscally@gmail.com> */ > + > +#include <linux/i2c.h> > +#include <linux/mfd/tps68470.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include "intel_skl_int3472_common.h" > + > +static const struct regmap_config tps68470_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = TPS68470_REG_MAX, > +}; > + > +static int tps68470_chip_init(struct device *dev, struct regmap *regmap) > +{ > + unsigned int version; > + int ret; > + > + /* Force software reset */ > + ret = regmap_write(regmap, TPS68470_REG_RESET, TPS68470_REG_RESET_MASK); > + if (ret) > + return ret; > + > + ret = regmap_read(regmap, TPS68470_REG_REVID, &version); > + if (ret) { > + dev_err(dev, "Failed to read revision register: %d\n", ret); > + return ret; > + } > + > + dev_info(dev, "TPS68470 REVID: 0x%x\n", version); > + > + return 0; > +} > + > +static struct platform_device * > +skl_int3472_register_pdev(const char *name, struct device *parent) > +{ > + struct platform_device *pdev; > + int ret; > + > + pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE); > + if (IS_ERR_OR_NULL(pdev)) > + return ERR_PTR(-ENOMEM); > + > + pdev->dev.parent = parent; > + pdev->driver_override = kstrndup(pdev->name, INT3472_PDEV_MAX_NAME_LEN, > + GFP_KERNEL); > + > + ret = platform_device_add(pdev); > + if (ret) { > + platform_device_put(pdev); > + return ERR_PTR(ret); > + } > + > + return pdev; > +} > + > +int skl_int3472_tps68470_probe(struct i2c_client *client) > +{ > + struct acpi_device *adev = ACPI_COMPANION(&client->dev); > + struct platform_device *regulator_dev; > + struct platform_device *opregion_dev; > + struct platform_device *gpio_dev; > + struct int3472_cldb cldb = { 0 }; > + struct platform_device *clk_dev; > + bool cldb_present = true; > + struct regmap *regmap; > + int ret = 0; > + > + regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config); > + if (IS_ERR(regmap)) { > + dev_err(&client->dev, "devm_regmap_init_i2c Error %ld\n", > + PTR_ERR(regmap)); > + return PTR_ERR(regmap); > + } > + > + i2c_set_clientdata(client, regmap); > + > + ret = tps68470_chip_init(&client->dev, regmap); > + if (ret < 0) { > + dev_err(&client->dev, "TPS68470 Init Error %d\n", ret); > + return ret; > + } > + > + /* > + * Check CLDB buffer against the PMIC's adev. If present, then we check > + * the value of control_logic_type field and follow one of the > + * following scenarios: > + * > + * 1. No CLDB - likely ACPI tables designed for ChromeOS. We > + * create platform devices for the GPIOs and OpRegion drivers. > + * > + * 2. CLDB, with control_logic_type = 2 - probably ACPI tables > + * made for Windows 2-in-1 platforms. Register pdevs for GPIO, > + * Clock and Regulator drivers to bind to. > + * > + * 3. Any other value in control_logic_type, we should never have > + * gotten to this point; crash and burn. > + */ > + ret = skl_int3472_get_cldb_buffer(adev, &cldb); > + if (!ret && cldb.control_logic_type != 2) > + return -EINVAL; > + > + if (ret) > + cldb_present = false; > + > + gpio_dev = skl_int3472_register_pdev("tps68470-gpio", &client->dev); > + if (IS_ERR(gpio_dev)) > + return PTR_ERR(gpio_dev); > + > + if (cldb_present) { > + clk_dev = skl_int3472_register_pdev("tps68470-clk", > + &client->dev); > + if (IS_ERR(clk_dev)) { > + ret = PTR_ERR(clk_dev); > + goto err_free_gpio; > + } > + > + regulator_dev = skl_int3472_register_pdev("tps68470-regulator", > + &client->dev); > + if (IS_ERR(regulator_dev)) { > + ret = PTR_ERR(regulator_dev); > + goto err_free_clk; > + } > + } else { > + opregion_dev = skl_int3472_register_pdev("tps68470_pmic_opregion", > + &client->dev); > + if (IS_ERR(opregion_dev)) { > + ret = PTR_ERR(opregion_dev); > + goto err_free_gpio; > + } > + } > + > + return 0; > + > +err_free_clk: > + platform_device_put(clk_dev); > +err_free_gpio: > + platform_device_put(gpio_dev); > + > + return ret; > +} >
On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: > On 19/01/2021 09:24, Andy Shevchenko wrote: > >>>>> +static struct i2c_driver int3472_tps68470 = { > >>>>> + .driver = { > >>>>> + .name = "int3472-tps68470", > >>>>> + .acpi_match_table = int3472_device_id, > >>>>> + }, > >>>>> + .probe_new = skl_int3472_tps68470_probe, > >>>>> +}; > >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > >> Sorry, I'm a bit confused by this. The i2c device is already > >> present...we just want the driver to bind to them, so what role do those > >> functions have there? > > What I meant is something like > > > > *_i2c.c > > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > > involved (and it sounds like it should be mfd/tps one, in which you > > just cut out ACPI IDs and convert to pure I²C one, that what I had > > suggested in the first place) > > Ahh; sorry - i misunderstood what you meant there. I understand now I > think, but there is one complication; the ACPI subsystem already creates > a client for that i2c adapter and address; i2c_new_client_device() > includes a check to see whether that adapter / address combination has > an i2c device already. So we would have to have the platform driver > with ACPI ID first find the existing i2c_client and unregister it before > registering the new one...the existing clients have a name matching the > ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an > i2c_device_id of course. See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 static const struct acpi_device_id i2c_multi_instantiate_ids[] = { {"BSG1160", }, {"BSG2150", }, {"INT33FE", }, {"INT3515", }, {} }; So, we quirklist it here and instantiate manually from platform driver (new coming one). ... > > You need to modify clk-gpio.c to export > > > > clk_hw_register_gpio_gate() > > clk_hw_register_gpio_mux() > > > > (perhaps it will require to add *_unregister() counterparts) and call it from > > your code. > > > > See, for example, how clk_hw_unregister_fixed_rate() is being used. Another Here I meant of course clk_hw_register_fixed_rate(). > > case is to add a helper directly into clk-gpio and call it instead of > > clk_hw_*() one, see how clk_register_fractional_divider() is implemented and > > used. > > I'll take a look, thanks
On Tue, Jan 19, 2021 at 10:56:17AM +0000, Kieran Bingham wrote: > On 18/01/2021 00:34, Daniel Scally wrote: ... > > +config INTEL_SKL_INT3472 > > + tristate "Intel SkyLake ACPI INT3472 Driver" > > + depends on X86 && ACPI > > + select REGMAP_I2C > > I've tried compiling this as a built in and a module and on my minimal > config I had failures on both for regulator_register and > regulator_unregister. > > I suspect this needs to have either a selects or a depends upon > CONFIG_REGULATOR Valid point, although it seems no consensus on which is better to use. It seems to me that in this case we need to select it.
On 19/01/2021 11:11, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 10:56:17AM +0000, Kieran Bingham wrote: >> On 18/01/2021 00:34, Daniel Scally wrote: > ... > >>> +config INTEL_SKL_INT3472 >>> + tristate "Intel SkyLake ACPI INT3472 Driver" >>> + depends on X86 && ACPI >>> + select REGMAP_I2C >> I've tried compiling this as a built in and a module and on my minimal >> config I had failures on both for regulator_register and >> regulator_unregister. >> >> I suspect this needs to have either a selects or a depends upon >> CONFIG_REGULATOR > Valid point, although it seems no consensus on which is better to use. It seems > to me that in this case we need to select it. > Yeah; it will be necessary for the gpio-regulator too anyway I expect. Thanks Kieran; I missed that entirely.
Hi Daniel, On Tue, Jan 19, 2021 at 08:43:43AM +0000, Daniel Scally wrote: > On 19/01/2021 06:19, Laurent Pinchart wrote: > > 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.. > > Is that the deciding factor? I'd say that's unlikely, but what do I > know? Then again, is there actually any downside to calling > gpiod_set_value() in the prepare() function instead? If not, may as well > be safe. The downside is that prepare() is meant to be called earlier than enable() when the consumer needs to call enable() in a context that can't sleep. This can sometimes cause the clock to be enabled for longer than necessary. In this case I don't think it's an issue, sensor drivers will use clk_prepare_enable() anyway. > >>>> + } > >>>> + > >>>> + 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 ? > > I haven't ever seen a DSDT that didn't include a 0x0c pin to enable the > clock, though that doesn't necessarily mean they're always there. Plenty > of driver datasheets say they're happy for the external clock to be free > running, so it could just be always active I suppose. Maybe we can handle this later if such a platform is found. You should then print a warning message if no clock is present. > > Another option would be to let userspace handle that GPIO, but we then > > need to convey it to userspace. > > Can you point me to an example of that to look at perhaps? I don't think there's any :-) We'd have to design the mechanism. > >>>> + > >>>> + 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.
Hi Andy, On Tue, Jan 19, 2021 at 11:33:58AM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > On 18/01/2021 21:19, Daniel Scally wrote: > > > > I'm more and more confident that this will work, but it has some > > knock-on effects: > > > > The both clk and regulator gpio driver expects to be able to fetch the > > GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of > > course, so we need to add another GPIO lookup table so those drivers can > > see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will > > be so we can set the .dev_id member of a gpiod_lookup_table to that > > value, but that isn't set until _after_ the pdev is registered (because > > it has to figure out the id, we can't manually set the IDs because there > > could be more than one instance of int3472-discrete bound to multiple > > PMIC devices, and we don't know which id the current one should have). > > Finally, we can't wait until the device is registered because it > > immediately probes, can't find the GPIO and then fails probe. > > > > It's similar problem that causes us to need the i2c-acpi name format > > macros, but complicated by the dynamic ID part of dev_name(&pdev->dev) > > > > Solving it is a bit of a sticky one; perhaps something like moving the > > dev_set_name() part of platform_device_add() [1] to its own function, > > that's called in both platform_device_alloc() and > > platform_device_register(). That way it would be available before the > > device itself was registered, meaning we could create the lookup table > > before it probes the driver. > > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of > methods to be able to use it as a library. That seems really overkill given the very simple implementation of the clock provided here. > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > the led-indicator GPIO here without some fairly major rework) > > LED indicators are done as LED class devices (see plenty of examples in PDx86 > drivers: drivers/platform/x86/) How do you expose the link between the sensor and its indicator LED to userspace ? Isn't it better to handle it in the kernel to avoid rogue userspace turning the camera on without notifying the user ? -- Regards, Laurent Pinchart
On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: > > On 19/01/2021 09:24, Andy Shevchenko wrote: > > >>>>> +static struct i2c_driver int3472_tps68470 = { > > >>>>> + .driver = { > > >>>>> + .name = "int3472-tps68470", > > >>>>> + .acpi_match_table = int3472_device_id, > > >>>>> + }, > > >>>>> + .probe_new = skl_int3472_tps68470_probe, > > >>>>> +}; > > >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > > >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > > >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > > >> Sorry, I'm a bit confused by this. The i2c device is already > > >> present...we just want the driver to bind to them, so what role do those > > >> functions have there? > > > What I meant is something like > > > > > > *_i2c.c > > > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > > > involved (and it sounds like it should be mfd/tps one, in which you > > > just cut out ACPI IDs and convert to pure I²C one, that what I had > > > suggested in the first place) > > > > Ahh; sorry - i misunderstood what you meant there. I understand now I > > think, but there is one complication; the ACPI subsystem already creates > > a client for that i2c adapter and address; i2c_new_client_device() > > includes a check to see whether that adapter / address combination has > > an i2c device already. So we would have to have the platform driver > > with ACPI ID first find the existing i2c_client and unregister it before > > registering the new one...the existing clients have a name matching the > > ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an > > i2c_device_id of course. > > See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 > > static const struct acpi_device_id i2c_multi_instantiate_ids[] = { > {"BSG1160", }, > {"BSG2150", }, > {"INT33FE", }, > {"INT3515", }, > {} > }; > > So, we quirklist it here and instantiate manually from platform driver (new > coming one). This is documented as used for devices that have multiple I2cSerialBus resources. That's not the case for the INT3472 as far as I can tell. I don't think we should abuse this mechanism. Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it has to register an OpRegion for ACPI-based Chrome OS devices. On other platforms (including DT platforms), it should only register regulators, clocks and GPIOs. Given the differences between those platforms, I don't think a TPS68470 driver that would fake being unaware of being probed through ACPI would be a good idea. We can always refactor the code later when we'll have a non-ACPI based platform using the TPS68470, without such a platform there's no way we can test the I2C driver without ACPI anyway. > ... > > > > You need to modify clk-gpio.c to export > > > > > > clk_hw_register_gpio_gate() > > > clk_hw_register_gpio_mux() > > > > > > (perhaps it will require to add *_unregister() counterparts) and call it from > > > your code. > > > > > > See, for example, how clk_hw_unregister_fixed_rate() is being used. Another > > Here I meant of course clk_hw_register_fixed_rate(). > > > > case is to add a helper directly into clk-gpio and call it instead of > > > clk_hw_*() one, see how clk_register_fractional_divider() is implemented and > > > used. > > > > I'll take a look, thanks -- Regards, Laurent Pinchart
On Tue, Jan 19, 2021 at 11:35:42AM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 08:21:23AM +0200, Laurent Pinchart wrote: > > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > > On 18/01/2021 21:19, Daniel Scally wrote: > > ... > > > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > > the led-indicator GPIO here without some fairly major rework) > > > > Given the additional complexity I don't think it's worth it, your > > implementation is fine and code duplication with clk-gpio is minimal. > > Making clk-gpio.c available as a library is a win in long term and reduces a > possible duplication by others in the future. I bet we even might find already > clk-gpio parts in some drivers. How about you submit a patch on top then ? :-) Let's avoid yak shaving.
On Tue, Jan 19, 2021 at 06:36:31PM +0200, Laurent Pinchart wrote: > On Tue, Jan 19, 2021 at 11:33:58AM +0200, Andy Shevchenko wrote: > > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > > On 18/01/2021 21:19, Daniel Scally wrote: ... > > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of > > methods to be able to use it as a library. > > That seems really overkill given the very simple implementation of the > clock provided here. Less code in the end is called an overkill? Hmm... I think since we in Linux it's better to utilize what it provides. Do you want me to prepare a patch to show that there is no overkill at all? ... > > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > > the led-indicator GPIO here without some fairly major rework) > > > > LED indicators are done as LED class devices (see plenty of examples in PDx86 > > drivers: drivers/platform/x86/) > > How do you expose the link between the sensor and its indicator LED to > userspace ? Isn't it better to handle it in the kernel to avoid rogue > userspace turning the camera on without notifying the user ? I didn't get this. It's completely a LED handling driver business. We may expose it to user space or not, but it's orthogonal to the usage of LED class IIUC. Am I mistaken here?
On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote: > On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote: > > On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: > > > On 19/01/2021 09:24, Andy Shevchenko wrote: > > > >>>>> +static struct i2c_driver int3472_tps68470 = { > > > >>>>> + .driver = { > > > >>>>> + .name = "int3472-tps68470", > > > >>>>> + .acpi_match_table = int3472_device_id, > > > >>>>> + }, > > > >>>>> + .probe_new = skl_int3472_tps68470_probe, > > > >>>>> +}; > > > >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > > > >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > > > >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > > > >> Sorry, I'm a bit confused by this. The i2c device is already > > > >> present...we just want the driver to bind to them, so what role do those > > > >> functions have there? > > > > What I meant is something like > > > > > > > > *_i2c.c > > > > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > > > > involved (and it sounds like it should be mfd/tps one, in which you > > > > just cut out ACPI IDs and convert to pure I²C one, that what I had > > > > suggested in the first place) > > > > > > Ahh; sorry - i misunderstood what you meant there. I understand now I > > > think, but there is one complication; the ACPI subsystem already creates > > > a client for that i2c adapter and address; i2c_new_client_device() > > > includes a check to see whether that adapter / address combination has > > > an i2c device already. So we would have to have the platform driver > > > with ACPI ID first find the existing i2c_client and unregister it before > > > registering the new one...the existing clients have a name matching the > > > ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an > > > i2c_device_id of course. > > > > See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 > > > > static const struct acpi_device_id i2c_multi_instantiate_ids[] = { > > {"BSG1160", }, > > {"BSG2150", }, > > {"INT33FE", }, > > {"INT3515", }, > > {} > > }; > > > > So, we quirklist it here and instantiate manually from platform driver (new > > coming one). > > This is documented as used for devices that have multiple I2cSerialBus > resources. That's not the case for the INT3472 as far as I can tell. I > don't think we should abuse this mechanism. This is quite a similar case to that one. Let's avoid yak shaving, right? > Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it > has to register an OpRegion for ACPI-based Chrome OS devices. On other > platforms (including DT platforms), it should only register regulators, > clocks and GPIOs. Given the differences between those platforms, I don't > think a TPS68470 driver that would fake being unaware of being probed > through ACPI would be a good idea. We can always refactor the code later > when we'll have a non-ACPI based platform using the TPS68470, without > such a platform there's no way we can test the I2C driver without ACPI > anyway. Are you agree that MFD approach should stay? How then we can manage to have an MFD driver cohabit with our new driver? I proposed a clean solution which will handle all possible cases via quirk driver. Having two drivers enumerated by different scenarios is a call for troubles (we have already with one of that sensors). And there is no "faking" anything, it's rather gating it depending on the platform.
Hi Andy, On Tue, Jan 19, 2021 at 07:43:15PM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 06:36:31PM +0200, Laurent Pinchart wrote: > > On Tue, Jan 19, 2021 at 11:33:58AM +0200, Andy Shevchenko wrote: > > > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > > > On 18/01/2021 21:19, Daniel Scally wrote: > > ... > > > > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of > > > methods to be able to use it as a library. > > > > That seems really overkill given the very simple implementation of the > > clock provided here. > > Less code in the end is called an overkill? Hmm... > I think since we in Linux it's better to utilize what it provides. Do you want > me to prepare a patch to show that there is no overkill at all? The amount of code we would save it very small. It's not necessarily a bad idea, but I think such an improvement could be made on top, it shouldn't block this series. > ... > > > > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > > > the led-indicator GPIO here without some fairly major rework) > > > > > > LED indicators are done as LED class devices (see plenty of examples in PDx86 > > > drivers: drivers/platform/x86/) > > > > How do you expose the link between the sensor and its indicator LED to > > userspace ? Isn't it better to handle it in the kernel to avoid rogue > > userspace turning the camera on without notifying the user ? > > I didn't get this. It's completely a LED handling driver business. We may > expose it to user space or not, but it's orthogonal to the usage of LED class > IIUC. Am I mistaken here? If it stays internal to the kernel and is solely controlled from the int3472 driver, there's no need to involve the LED class. If we want to expose the privacy LED to userspace then the LED framework is the way to go, but we will also need to find a way to expose the link between the camera sensor and the LED to userspace. If there are two privacy LEDs, one for the front sensor and one for the back sensor, userspace will need to know which is which.
Hi Andy, On Tue, Jan 19, 2021 at 07:51:14PM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote: > > On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote: > > > On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: > > > > On 19/01/2021 09:24, Andy Shevchenko wrote: > > > > >>>>> +static struct i2c_driver int3472_tps68470 = { > > > > >>>>> + .driver = { > > > > >>>>> + .name = "int3472-tps68470", > > > > >>>>> + .acpi_match_table = int3472_device_id, > > > > >>>>> + }, > > > > >>>>> + .probe_new = skl_int3472_tps68470_probe, > > > > >>>>> +}; > > > > >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > > > > >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > > > > >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > > > > >> Sorry, I'm a bit confused by this. The i2c device is already > > > > >> present...we just want the driver to bind to them, so what role do those > > > > >> functions have there? > > > > > What I meant is something like > > > > > > > > > > *_i2c.c > > > > > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > > > > > involved (and it sounds like it should be mfd/tps one, in which you > > > > > just cut out ACPI IDs and convert to pure I²C one, that what I had > > > > > suggested in the first place) > > > > > > > > Ahh; sorry - i misunderstood what you meant there. I understand now I > > > > think, but there is one complication; the ACPI subsystem already creates > > > > a client for that i2c adapter and address; i2c_new_client_device() > > > > includes a check to see whether that adapter / address combination has > > > > an i2c device already. So we would have to have the platform driver > > > > with ACPI ID first find the existing i2c_client and unregister it before > > > > registering the new one...the existing clients have a name matching the > > > > ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an > > > > i2c_device_id of course. > > > > > > See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 > > > > > > static const struct acpi_device_id i2c_multi_instantiate_ids[] = { > > > {"BSG1160", }, > > > {"BSG2150", }, > > > {"INT33FE", }, > > > {"INT3515", }, > > > {} > > > }; > > > > > > So, we quirklist it here and instantiate manually from platform driver (new > > > coming one). > > > > This is documented as used for devices that have multiple I2cSerialBus > > resources. That's not the case for the INT3472 as far as I can tell. I > > don't think we should abuse this mechanism. > > This is quite a similar case to that one. Let's avoid yak shaving, right? Exactly my point, that's why I think this patch is good overall, I don't think it requires a complete rewrite. > > Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it > > has to register an OpRegion for ACPI-based Chrome OS devices. On other > > platforms (including DT platforms), it should only register regulators, > > clocks and GPIOs. Given the differences between those platforms, I don't > > think a TPS68470 driver that would fake being unaware of being probed > > through ACPI would be a good idea. We can always refactor the code later > > when we'll have a non-ACPI based platform using the TPS68470, without > > such a platform there's no way we can test the I2C driver without ACPI > > anyway. > > Are you agree that MFD approach should stay? How then we can manage to have an > MFD driver cohabit with our new driver? I proposed a clean solution which will > handle all possible cases via quirk driver. Having two drivers enumerated by > different scenarios is a call for troubles (we have already with one of that > sensors). I think we should solve this problem when it will arise. Solving problems with complex architectures without a platform to test the code on is a pretty sure way to get the architecture design wrong. Let's get this merged, it's an improvement compared to the current situation, and then let's improve it further on top when we'll need to support more use cases. > And there is no "faking" anything, it's rather gating it depending on the > platform.
On Wed, Jan 20, 2021 at 06:18:53AM +0200, Laurent Pinchart wrote: > On Tue, Jan 19, 2021 at 07:43:15PM +0200, Andy Shevchenko wrote: > > On Tue, Jan 19, 2021 at 06:36:31PM +0200, Laurent Pinchart wrote: > > > On Tue, Jan 19, 2021 at 11:33:58AM +0200, Andy Shevchenko wrote: > > > > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > > > > On 18/01/2021 21:19, Daniel Scally wrote: ... > > > > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of > > > > methods to be able to use it as a library. > > > > > > That seems really overkill given the very simple implementation of the > > > clock provided here. > > > > Less code in the end is called an overkill? Hmm... > > I think since we in Linux it's better to utilize what it provides. Do you want > > me to prepare a patch to show that there is no overkill at all? > > The amount of code we would save it very small. It's not necessarily a > bad idea, but I think such an improvement could be made on top, it > shouldn't block this series. Okay, let's wait what Dan will say on this. I can probably help to achieve this improvement sooner than later. ... > > > > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > > > > the led-indicator GPIO here without some fairly major rework) > > > > > > > > LED indicators are done as LED class devices (see plenty of examples in PDx86 > > > > drivers: drivers/platform/x86/) > > > > > > How do you expose the link between the sensor and its indicator LED to > > > userspace ? Isn't it better to handle it in the kernel to avoid rogue > > > userspace turning the camera on without notifying the user ? > > > > I didn't get this. It's completely a LED handling driver business. We may > > expose it to user space or not, but it's orthogonal to the usage of LED class > > IIUC. Am I mistaken here? > > If it stays internal to the kernel and is solely controlled from the > int3472 driver, there's no need to involve the LED class. If we want to > expose the privacy LED to userspace then the LED framework is the way to > go, but we will also need to find a way to expose the link between the > camera sensor and the LED to userspace. If there are two privacy LEDs, > one for the front sensor and one for the back sensor, userspace will > need to know which is which. I see. For now we probably can keep GPIO LED implementation internally.
On Wed, Jan 20, 2021 at 06:21:41AM +0200, Laurent Pinchart wrote: > On Tue, Jan 19, 2021 at 07:51:14PM +0200, Andy Shevchenko wrote: > > On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote: > > > On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote: > > > > On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: > > > > > On 19/01/2021 09:24, Andy Shevchenko wrote: > > > > > >>>>> +static struct i2c_driver int3472_tps68470 = { > > > > > >>>>> + .driver = { > > > > > >>>>> + .name = "int3472-tps68470", > > > > > >>>>> + .acpi_match_table = int3472_device_id, > > > > > >>>>> + }, > > > > > >>>>> + .probe_new = skl_int3472_tps68470_probe, > > > > > >>>>> +}; > > > > > >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > > > > > >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > > > > > >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > > > > > >> Sorry, I'm a bit confused by this. The i2c device is already > > > > > >> present...we just want the driver to bind to them, so what role do those > > > > > >> functions have there? > > > > > > What I meant is something like > > > > > > > > > > > > *_i2c.c > > > > > > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > > > > > > involved (and it sounds like it should be mfd/tps one, in which you > > > > > > just cut out ACPI IDs and convert to pure I²C one, that what I had > > > > > > suggested in the first place) > > > > > > > > > > Ahh; sorry - i misunderstood what you meant there. I understand now I > > > > > think, but there is one complication; the ACPI subsystem already creates > > > > > a client for that i2c adapter and address; i2c_new_client_device() > > > > > includes a check to see whether that adapter / address combination has > > > > > an i2c device already. So we would have to have the platform driver > > > > > with ACPI ID first find the existing i2c_client and unregister it before > > > > > registering the new one...the existing clients have a name matching the > > > > > ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an > > > > > i2c_device_id of course. > > > > > > > > See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 > > > > > > > > static const struct acpi_device_id i2c_multi_instantiate_ids[] = { > > > > {"BSG1160", }, > > > > {"BSG2150", }, > > > > {"INT33FE", }, > > > > {"INT3515", }, > > > > {} > > > > }; > > > > > > > > So, we quirklist it here and instantiate manually from platform driver (new > > > > coming one). > > > > > > This is documented as used for devices that have multiple I2cSerialBus > > > resources. That's not the case for the INT3472 as far as I can tell. I > > > don't think we should abuse this mechanism. > > > > This is quite a similar case to that one. Let's avoid yak shaving, right? > > Exactly my point, that's why I think this patch is good overall, I don't > think it requires a complete rewrite. The approach in the series is to reinvent the MFD driver which I against of. I don;t think we need to kill it there and reborn in a new form and dragging code from there to here to there. On top of that the approach with a quirk driver in the middle seems to me cleaner than using different paths how the two drivers are being initialized. In the proposed approach there will be one making decision point and easy to understand what's going on. The bad example of two making decision points is acpi_lpss.c vs. individual drivers (however in that case it have different ID's, i.e. ACPI vs. PCI), > > > Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it > > > has to register an OpRegion for ACPI-based Chrome OS devices. On other > > > platforms (including DT platforms), it should only register regulators, > > > clocks and GPIOs. Given the differences between those platforms, I don't > > > think a TPS68470 driver that would fake being unaware of being probed > > > through ACPI would be a good idea. We can always refactor the code later > > > when we'll have a non-ACPI based platform using the TPS68470, without > > > such a platform there's no way we can test the I2C driver without ACPI > > > anyway. > > > > Are you agree that MFD approach should stay? How then we can manage to have an > > MFD driver cohabit with our new driver? I proposed a clean solution which will > > handle all possible cases via quirk driver. Having two drivers enumerated by > > different scenarios is a call for troubles (we have already with one of that > > sensors). > > I think we should solve this problem when it will arise. Solving > problems with complex architectures without a platform to test the code > on is a pretty sure way to get the architecture design wrong. Let's get > this merged, it's an improvement compared to the current situation, and > then let's improve it further on top when we'll need to support more use > cases. But this is problem already here right now. The submitted code is to support a new platform that needs a quirk and treats INT3472 differently. The usual way is to refactor the existing solution to make them both to have a best compromise. > > And there is no "faking" anything, it's rather gating it depending on the > > platform. -- With Best Regards, Andy Shevchenko
On 20/01/2021 12:57, Andy Shevchenko wrote: > On Wed, Jan 20, 2021 at 06:21:41AM +0200, Laurent Pinchart wrote: >> On Tue, Jan 19, 2021 at 07:51:14PM +0200, Andy Shevchenko wrote: >>> On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote: >>>> On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote: >>>>> On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: >>>>>> On 19/01/2021 09:24, Andy Shevchenko wrote: >>>>>>>>>>> +static struct i2c_driver int3472_tps68470 = { >>>>>>>>>>> + .driver = { >>>>>>>>>>> + .name = "int3472-tps68470", >>>>>>>>>>> + .acpi_match_table = int3472_device_id, >>>>>>>>>>> + }, >>>>>>>>>>> + .probe_new = skl_int3472_tps68470_probe, >>>>>>>>>>> +}; >>>>>>>>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can >>>>>>>>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it >>>>>>>>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... >>>>>>>> Sorry, I'm a bit confused by this. The i2c device is already >>>>>>>> present...we just want the driver to bind to them, so what role do those >>>>>>>> functions have there? >>>>>>> What I meant is something like >>>>>>> >>>>>>> *_i2c.c >>>>>>> real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI >>>>>>> involved (and it sounds like it should be mfd/tps one, in which you >>>>>>> just cut out ACPI IDs and convert to pure I²C one, that what I had >>>>>>> suggested in the first place) >>>>>> >>>>>> Ahh; sorry - i misunderstood what you meant there. I understand now I >>>>>> think, but there is one complication; the ACPI subsystem already creates >>>>>> a client for that i2c adapter and address; i2c_new_client_device() >>>>>> includes a check to see whether that adapter / address combination has >>>>>> an i2c device already. So we would have to have the platform driver >>>>>> with ACPI ID first find the existing i2c_client and unregister it before >>>>>> registering the new one...the existing clients have a name matching the >>>>>> ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an >>>>>> i2c_device_id of course. >>>>> >>>>> See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 >>>>> >>>>> static const struct acpi_device_id i2c_multi_instantiate_ids[] = { >>>>> {"BSG1160", }, >>>>> {"BSG2150", }, >>>>> {"INT33FE", }, >>>>> {"INT3515", }, >>>>> {} >>>>> }; >>>>> >>>>> So, we quirklist it here and instantiate manually from platform driver (new >>>>> coming one). >>>> >>>> This is documented as used for devices that have multiple I2cSerialBus >>>> resources. That's not the case for the INT3472 as far as I can tell. I >>>> don't think we should abuse this mechanism. >>> >>> This is quite a similar case to that one. Let's avoid yak shaving, right? >> >> Exactly my point, that's why I think this patch is good overall, I don't >> think it requires a complete rewrite. > > The approach in the series is to reinvent the MFD driver which I against of. > I don;t think we need to kill it there and reborn in a new form and dragging > code from there to here to there. > > On top of that the approach with a quirk driver in the middle seems to me > cleaner than using different paths how the two drivers are being initialized. > In the proposed approach there will be one making decision point and easy to > understand what's going on. > > The bad example of two making decision points is acpi_lpss.c vs. individual > drivers (however in that case it have different ID's, i.e. ACPI vs. PCI), Right; so if I understand correctly, the proposal is: 1. Add INT3472 to the i2c_multi_instantiate_ids, which blocks it getting created as an i2c device 2. instead of intel-skl-int3472 registering an i2c and a platform driver, just register a platform driver that binds to the INT3472 acpi_device_id. We can check hardware type like in intel_cht_int33fe_common.c and call either discrete probe that does what the discrete driver is doing now, or else call tps68470 which is just a stub driver registering an i2c device like intel_cht_int33fe_microb.c 3. Change the existing tps68470 mfd driver to match to that created i2c device instead of ACPI match, and move the code from intel_skl_int3472_tps68470.c to that driver instead I think I finally got what you meant there, Andy, but correct me if I'm wrong please. I'm not sure that one's better than the other, to be honest. Either the multi-function device functionality lives in the conventional place, or else _all_ of the int3472 handling code lives together in one module. >>>> Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it >>>> has to register an OpRegion for ACPI-based Chrome OS devices. On other >>>> platforms (including DT platforms), it should only register regulators, >>>> clocks and GPIOs. Given the differences between those platforms, I don't >>>> think a TPS68470 driver that would fake being unaware of being probed >>>> through ACPI would be a good idea. We can always refactor the code later >>>> when we'll have a non-ACPI based platform using the TPS68470, without >>>> such a platform there's no way we can test the I2C driver without ACPI >>>> anyway. >>> >>> Are you agree that MFD approach should stay? How then we can manage to have an >>> MFD driver cohabit with our new driver? I proposed a clean solution which will >>> handle all possible cases via quirk driver. Having two drivers enumerated by >>> different scenarios is a call for troubles (we have already with one of that >>> sensors). What kind of troubles do you anticipate here? >> I think we should solve this problem when it will arise. Solving >> problems with complex architectures without a platform to test the code >> on is a pretty sure way to get the architecture design wrong. Let's get >> this merged, it's an improvement compared to the current situation, and >> then let's improve it further on top when we'll need to support more use >> cases. > > But this is problem already here right now. The submitted code is to support > a new platform that needs a quirk and treats INT3472 differently. The usual > way is to refactor the existing solution to make them both to have a best > compromise. > >>> And there is no "faking" anything, it's rather gating it depending on the >>> platform. >
Hello Andy, Laurent On 21/01/2021 00:18, Daniel Scally wrote: > On 20/01/2021 12:57, Andy Shevchenko wrote: >> On Wed, Jan 20, 2021 at 06:21:41AM +0200, Laurent Pinchart wrote: >>> On Tue, Jan 19, 2021 at 07:51:14PM +0200, Andy Shevchenko wrote: >>>> On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote: >>>>> On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote: >>>>>> On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: >>>>>>> On 19/01/2021 09:24, Andy Shevchenko wrote: >>>>>>>>>>>> +static struct i2c_driver int3472_tps68470 = { >>>>>>>>>>>> + .driver = { >>>>>>>>>>>> + .name = "int3472-tps68470", >>>>>>>>>>>> + .acpi_match_table = int3472_device_id, >>>>>>>>>>>> + }, >>>>>>>>>>>> + .probe_new = skl_int3472_tps68470_probe, >>>>>>>>>>>> +}; >>>>>>>>>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can >>>>>>>>>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it >>>>>>>>>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... >>>>>>>>> Sorry, I'm a bit confused by this. The i2c device is already >>>>>>>>> present...we just want the driver to bind to them, so what role do those >>>>>>>>> functions have there? >>>>>>>> What I meant is something like >>>>>>>> >>>>>>>> *_i2c.c >>>>>>>> real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI >>>>>>>> involved (and it sounds like it should be mfd/tps one, in which you >>>>>>>> just cut out ACPI IDs and convert to pure I²C one, that what I had >>>>>>>> suggested in the first place) >>>>>>> Ahh; sorry - i misunderstood what you meant there. I understand now I >>>>>>> think, but there is one complication; the ACPI subsystem already creates >>>>>>> a client for that i2c adapter and address; i2c_new_client_device() >>>>>>> includes a check to see whether that adapter / address combination has >>>>>>> an i2c device already. So we would have to have the platform driver >>>>>>> with ACPI ID first find the existing i2c_client and unregister it before >>>>>>> registering the new one...the existing clients have a name matching the >>>>>>> ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an >>>>>>> i2c_device_id of course. >>>>>> See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 >>>>>> >>>>>> static const struct acpi_device_id i2c_multi_instantiate_ids[] = { >>>>>> {"BSG1160", }, >>>>>> {"BSG2150", }, >>>>>> {"INT33FE", }, >>>>>> {"INT3515", }, >>>>>> {} >>>>>> }; >>>>>> >>>>>> So, we quirklist it here and instantiate manually from platform driver (new >>>>>> coming one). >>>>> This is documented as used for devices that have multiple I2cSerialBus >>>>> resources. That's not the case for the INT3472 as far as I can tell. I >>>>> don't think we should abuse this mechanism. >>>> This is quite a similar case to that one. Let's avoid yak shaving, right? >>> Exactly my point, that's why I think this patch is good overall, I don't >>> think it requires a complete rewrite. >> The approach in the series is to reinvent the MFD driver which I against of. >> I don;t think we need to kill it there and reborn in a new form and dragging >> code from there to here to there. >> >> On top of that the approach with a quirk driver in the middle seems to me >> cleaner than using different paths how the two drivers are being initialized. >> In the proposed approach there will be one making decision point and easy to >> understand what's going on. >> >> The bad example of two making decision points is acpi_lpss.c vs. individual >> drivers (however in that case it have different ID's, i.e. ACPI vs. PCI), > > Right; so if I understand correctly, the proposal is: > > 1. Add INT3472 to the i2c_multi_instantiate_ids, which blocks it getting > created as an i2c device > 2. instead of intel-skl-int3472 registering an i2c and a platform > driver, just register a platform driver that binds to the INT3472 > acpi_device_id. We can check hardware type like in > intel_cht_int33fe_common.c and call either discrete probe that does what > the discrete driver is doing now, or else call tps68470 which is just a > stub driver registering an i2c device like intel_cht_int33fe_microb.c > 3. Change the existing tps68470 mfd driver to match to that created i2c > device instead of ACPI match, and move the code from > intel_skl_int3472_tps68470.c to that driver instead > > I think I finally got what you meant there, Andy, but correct me if I'm > wrong please. > > I'm not sure that one's better than the other, to be honest. Either the > multi-function device functionality lives in the conventional place, or > else _all_ of the int3472 handling code lives together in one module. Can we come to a consensus on this? I would rather be in agreement than leave it hanging...I do see the argument that it's better not to rebirth the MFD driver away from that subsystem, but at the moment I lean towards the view that having all the code handling this particular _HID in one place is probably preferable, if only to make it easier for anyone coming in the future to understand what's happening. That said; I'm not particularly precious about it, I'd just like to agree an approach so I can move forward with the next version >>>>> Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it >>>>> has to register an OpRegion for ACPI-based Chrome OS devices. On other >>>>> platforms (including DT platforms), it should only register regulators, >>>>> clocks and GPIOs. Given the differences between those platforms, I don't >>>>> think a TPS68470 driver that would fake being unaware of being probed >>>>> through ACPI would be a good idea. We can always refactor the code later >>>>> when we'll have a non-ACPI based platform using the TPS68470, without >>>>> such a platform there's no way we can test the I2C driver without ACPI >>>>> anyway. >>>> Are you agree that MFD approach should stay? How then we can manage to have an >>>> MFD driver cohabit with our new driver? I proposed a clean solution which will >>>> handle all possible cases via quirk driver. Having two drivers enumerated by >>>> different scenarios is a call for troubles (we have already with one of that >>>> sensors). > What kind of troubles do you anticipate here? > >>> I think we should solve this problem when it will arise. Solving >>> problems with complex architectures without a platform to test the code >>> on is a pretty sure way to get the architecture design wrong. Let's get >>> this merged, it's an improvement compared to the current situation, and >>> then let's improve it further on top when we'll need to support more use >>> cases. >> But this is problem already here right now. The submitted code is to support >> a new platform that needs a quirk and treats INT3472 differently. The usual >> way is to refactor the existing solution to make them both to have a best >> compromise. >> >>>> And there is no "faking" anything, it's rather gating it depending on the >>>> platform.
On Sun, Feb 7, 2021 at 1:00 PM Daniel Scally <djrscally@gmail.com> wrote: > On 21/01/2021 00:18, Daniel Scally wrote: > > On 20/01/2021 12:57, Andy Shevchenko wrote: ... > > I'm not sure that one's better than the other, to be honest. Either the > > multi-function device functionality lives in the conventional place, or > > else _all_ of the int3472 handling code lives together in one module. > Can we come to a consensus on this? I would rather be in agreement than > leave it hanging...I do see the argument that it's better not to rebirth > the MFD driver away from that subsystem, but at the moment I lean > towards the view that having all the code handling this particular _HID > in one place is probably preferable, if only to make it easier for > anyone coming in the future to understand what's happening. That said; > I'm not particularly precious about it, I'd just like to agree an > approach so I can move forward with the next version So, my priorities of rejection (first is higher) - open-coding MFD subsystem (that said, if you shuffle the code, at least leave it as an MFD driver) - moving out from MFD (although I understand intentions) Summarize, go ahead with your view (leaving it as an MFD driver) and look forward to what maintainer(s) will say. -- With Best Regards, Andy Shevchenko
diff --git a/MAINTAINERS b/MAINTAINERS index a091b496fdd8..c4ed8c3bc58e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9147,6 +9147,11 @@ S: Maintained F: arch/x86/include/asm/intel_scu_ipc.h F: drivers/platform/x86/intel_scu_* +INTEL SKL INT3472 ACPI DEVICE DRIVER +M: Daniel Scally <djrscally@gmail.com> +S: Maintained +F: drivers/platform/x86/intel_skl_int3472_* + INTEL SPEED SELECT TECHNOLOGY M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> L: platform-driver-x86@vger.kernel.org diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 91e6176cdfbd..916b077df2d5 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -844,6 +844,31 @@ config INTEL_CHT_INT33FE device and CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m for Type-C device. +config INTEL_SKL_INT3472 + tristate "Intel SkyLake ACPI INT3472 Driver" + depends on X86 && ACPI + select REGMAP_I2C + help + This driver adds support for the INT3472 ACPI devices found on some + Intel SkyLake devices. + + There are 3 kinds of INT3472 ACPI device possible; two for devices + designed for Windows (either with or without a physical tps68470 + PMIC) and one designed for Chrome OS. 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. + + Say Y or M here if you have a SkyLake device designed for use + with Windows or ChromeOS. Say N here if you are not sure. + + The module will be named "intel-skl-int3472" + config INTEL_HID_EVENT tristate "INTEL HID Event" depends on ACPI diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 581475f59819..ae29c66842ca 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -86,6 +86,10 @@ obj-$(CONFIG_INTEL_HID_EVENT) += intel-hid.o obj-$(CONFIG_INTEL_INT0002_VGPIO) += intel_int0002_vgpio.o obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o +obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472.o +intel_skl_int3472-objs := intel_skl_int3472_common.o \ + intel_skl_int3472_discrete.o \ + intel_skl_int3472_tps68470.o obj-$(CONFIG_INTEL_VBTN) += intel-vbtn.o # MSI diff --git a/drivers/platform/x86/intel_skl_int3472_common.c b/drivers/platform/x86/intel_skl_int3472_common.c new file mode 100644 index 000000000000..08cb9d3c06aa --- /dev/null +++ b/drivers/platform/x86/intel_skl_int3472_common.c @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Author: Dan Scally <djrscally@gmail.com> */ + +#include <linux/acpi.h> +#include <linux/i2c.h> +#include <linux/platform_device.h> + +#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"); + return -ENODEV; + } + + if (obj->type != ACPI_TYPE_BUFFER) { + dev_err(&adev->dev, "CLDB object is not an ACPI buffer\n"); + ret = -EINVAL; + goto out_free_buff; + } + + if (obj->buffer.length > sizeof(*cldb)) { + dev_err(&adev->dev, "The CLDB buffer is too large\n"); + ret = -EINVAL; + goto out_free_buff; + } + + memcpy(cldb, obj->buffer.pointer, obj->buffer.length); + +out_free_buff: + kfree(buffer.pointer); + return ret; +} + +static const struct acpi_device_id int3472_device_id[] = { + { "INT3472", 0 }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, int3472_device_id); + +static struct platform_driver int3472_discrete = { + .driver = { + .name = "int3472-discrete", + .acpi_match_table = int3472_device_id, + }, + .probe = skl_int3472_discrete_probe, + .remove = skl_int3472_discrete_remove, +}; + +static struct i2c_driver int3472_tps68470 = { + .driver = { + .name = "int3472-tps68470", + .acpi_match_table = int3472_device_id, + }, + .probe_new = skl_int3472_tps68470_probe, +}; + +static int skl_int3472_init(void) +{ + int ret = 0; + + ret = platform_driver_register(&int3472_discrete); + if (ret) + return ret; + + ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470); + if (ret) + goto err_unregister_plat_drv; + + return 0; + +err_unregister_plat_drv: + platform_driver_unregister(&int3472_discrete); + return ret; +} +module_init(skl_int3472_init); + +static void skl_int3472_exit(void) +{ + platform_driver_unregister(&int3472_discrete); + i2c_del_driver(&int3472_tps68470); +} +module_exit(skl_int3472_exit); + +MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Device Driver"); +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/platform/x86/intel_skl_int3472_common.h b/drivers/platform/x86/intel_skl_int3472_common.h new file mode 100644 index 000000000000..9acd0ce7aed9 --- /dev/null +++ b/drivers/platform/x86/intel_skl_int3472_common.h @@ -0,0 +1,100 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Author: Dan Scally <djrscally@gmail.com> */ +#include <linux/regulator/machine.h> +#include <linux/clk-provider.h> +#include <linux/gpio/machine.h> +#include <linux/regulator/driver.h> +#include <linux/types.h> + +/* PMIC GPIO Types */ +#define INT3472_GPIO_TYPE_RESET 0x00 +#define INT3472_GPIO_TYPE_POWERDOWN 0x01 +#define INT3472_GPIO_TYPE_CLK_ENABLE 0x0c +#define INT3472_GPIO_TYPE_POWER_ENABLE 0x0b +#define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d +#define INT3472_PDEV_MAX_NAME_LEN 23 +#define INT3472_MAX_SENSOR_GPIOS 3 +#define GPIO_REGULATOR_NAME_LENGTH 27 +#define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9 + +#define INT3472_REGULATOR(_NAME, _SUPPLY, _OPS) \ + (const struct regulator_desc) { \ + .name = _NAME, \ + .supply_name = _SUPPLY, \ + .id = 0, \ + .type = REGULATOR_VOLTAGE, \ + .ops = _OPS, \ + .owner = THIS_MODULE, \ + } + +#define INT3472_GPIO_FUNCTION_REMAP(_PIN, _FUNCTION) \ + (const struct int3472_gpio_function_remap) { \ + .documented = _PIN, \ + .actual = _FUNCTION \ + } + +#define to_int3472_clk(hw) \ + container_of(hw, struct int3472_gpio_clock, clk_hw) + +struct int3472_cldb { + u8 version; + /* + * control logic type + * 0: UNKNOWN + * 1: DISCRETE(CRD-D) + * 2: PMIC TPS68470 + * 3: PMIC uP6641 + */ + u8 control_logic_type; + u8 control_logic_id; + u8 sensor_card_sku; + u8 reserved[28]; +}; + +struct int3472_gpio_regulator { + 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; +}; + +struct int3472_gpio_clock { + struct clk *clk; + struct clk_hw clk_hw; + struct clk_lookup *cl; + struct gpio_desc *gpio; +}; + +struct int3472_device { + struct acpi_device *adev; + struct platform_device *pdev; + struct acpi_device *sensor; + char *sensor_name; + + unsigned int n_gpios; /* how many GPIOs have we seen */ + + struct int3472_gpio_regulator regulator; + struct int3472_gpio_clock clock; + + unsigned int n_sensor_gpios; /* how many have we mapped to sensor */ + bool gpios_mapped; + struct gpiod_lookup_table gpios; +}; + +struct int3472_gpio_function_remap { + char *documented; + char *actual; +}; + +struct int3472_sensor_config { + char *sensor_module_name; + struct regulator_consumer_supply supply_map; + const struct int3472_gpio_function_remap *function_maps; +}; + +int skl_int3472_discrete_probe(struct platform_device *pdev); +int skl_int3472_discrete_remove(struct platform_device *pdev); +int skl_int3472_tps68470_probe(struct i2c_client *client); +int skl_int3472_get_cldb_buffer(struct acpi_device *adev, + struct int3472_cldb *cldb); diff --git a/drivers/platform/x86/intel_skl_int3472_discrete.c b/drivers/platform/x86/intel_skl_int3472_discrete.c new file mode 100644 index 000000000000..93d250dca92f --- /dev/null +++ b/drivers/platform/x86/intel_skl_int3472_discrete.c @@ -0,0 +1,496 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Author: Dan Scally <djrscally@gmail.com> */ + +#include <linux/acpi.h> +#include <linux/clkdev.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regulator/driver.h> + +#include "intel_skl_int3472_common.h" + +/* 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); + +/* + * 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. 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[] = { + INT3472_GPIO_FUNCTION_REMAP("reset", NULL), + INT3472_GPIO_FUNCTION_REMAP("powerdown", "reset"), + { } +}; + +static struct int3472_sensor_config int3472_sensor_configs[] = { + /* Lenovo Miix 510-12ISK - OV2680, Front */ + { "GNDF140809R", { 0 }, ov2680_gpio_function_remaps}, + /* Lenovo Miix 510-12ISK - OV5648, Rear */ + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL}, + /* Surface Go 1&2 - OV5693, Front */ + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL}, +}; + +/* + * 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 = { 0 }; + +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); + + return 0; +} + +static void skl_int3472_clk_disable(struct clk_hw *hw) +{ + struct int3472_gpio_clock *clk = to_int3472_clk(hw); + + gpiod_set_value(clk->gpio, 0); +} + +static int skl_int3472_clk_prepare(struct clk_hw *hw) +{ + /* + * We're just turning a GPIO on to enable, so nothing to do here, but + * we want to provide the op so prepare_enable() works. + */ + return 0; +} + +static void skl_int3472_clk_unprepare(struct clk_hw *hw) +{ + /* Likewise, nothing to do here... */ +} + +static const struct clk_ops skl_int3472_clock_ops = { + .prepare = skl_int3472_clk_prepare, + .unprepare = skl_int3472_clk_unprepare, + .enable = skl_int3472_clk_enable, + .disable = skl_int3472_clk_disable, +}; + +static struct int3472_sensor_config * +skl_int3472_get_sensor_module_config(struct int3472_device *int3472) +{ + unsigned int i = ARRAY_SIZE(int3472_sensor_configs); + struct int3472_sensor_config *ret; + union acpi_object *obj; + + obj = acpi_evaluate_dsm_typed(int3472->sensor->handle, + &cio2_sensor_module_guid, 0x00, + 0x01, NULL, ACPI_TYPE_STRING); + + if (!obj) { + dev_err(&int3472->pdev->dev, + "Failed to get sensor module string from _DSM\n"); + return ERR_PTR(-ENODEV); + } + + if (obj->string.type != ACPI_TYPE_STRING) { + dev_err(&int3472->pdev->dev, + "Sensor _DSM returned a non-string value\n"); + ret = ERR_PTR(-EINVAL); + goto out_free_obj; + } + + ret = ERR_PTR(-ENODEV); + while (i--) { + if (!strcmp(int3472_sensor_configs[i].sensor_module_name, + obj->string.pointer)) { + ret = &int3472_sensor_configs[i]; + goto out_free_obj; + } + } + +out_free_obj: + ACPI_FREE(obj); + return ret; +} + +static int skl_int3472_map_gpio_to_sensor(struct int3472_device *int3472, + struct acpi_resource *ares, + char *func, u32 polarity) +{ + char *path = ares->data.gpio.resource_source.string_ptr; + struct int3472_sensor_config *sensor_config; + struct gpiod_lookup table_entry; + struct acpi_device *adev; + acpi_handle handle; + acpi_status status; + int ret; + + sensor_config = skl_int3472_get_sensor_module_config(int3472); + if (!IS_ERR(sensor_config) && sensor_config->function_maps) { + unsigned int i = 0; + + while (sensor_config->function_maps[i].documented) { + if (!strcmp(func, sensor_config->function_maps[i].documented)) { + func = sensor_config->function_maps[i].actual; + + break; + } + + i++; + } + } + + if (!func) + return 0; + + if (int3472->n_sensor_gpios >= INT3472_MAX_SENSOR_GPIOS) { + dev_warn(&int3472->pdev->dev, "Too many GPIOs mapped\n"); + return -EINVAL; + } + + 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); + + memcpy(&int3472->gpios.table[int3472->n_sensor_gpios], &table_entry, + sizeof(table_entry)); + + int3472->n_sensor_gpios++; + + return 0; +} + +static int skl_int3472_register_clock(struct int3472_device *int3472, + struct acpi_resource *ares) +{ + char *path = ares->data.gpio.resource_source.string_ptr; + struct clk_init_data init = { }; + int ret = 0; + + init.name = kasprintf(GFP_KERNEL, "%s-clk", + acpi_dev_name(int3472->adev)); + init.ops = &skl_int3472_clock_ops; + + int3472->clock.gpio = acpi_get_gpiod(path, + ares->data.gpio.pin_table[0]); + if (IS_ERR(int3472->clock.gpio)) { + ret = PTR_ERR(int3472->clock.gpio); + goto out_free_init_name; + } + + int3472->clock.clk_hw.init = &init; + int3472->clock.clk = clk_register(&int3472->adev->dev, + &int3472->clock.clk_hw); + if (IS_ERR(int3472->clock.clk)) { + ret = PTR_ERR(int3472->clock.clk); + goto err_put_gpio; + } + + int3472->clock.cl = clkdev_create(int3472->clock.clk, "xvclk", + int3472->sensor_name); + if (IS_ERR_OR_NULL(int3472->clock.cl)) + goto err_unregister_clk; + + goto out_free_init_name; + +err_unregister_clk: + clk_unregister(int3472->clock.clk); +err_put_gpio: + gpiod_put(int3472->clock.gpio); +out_free_init_name: + kfree(init.name); + + return ret; +} + +static int skl_int3472_register_regulator(struct int3472_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 = skl_int3472_get_sensor_module_config(int3472); + if (IS_ERR_OR_NULL(sensor_config)) { + dev_err(&int3472->pdev->dev, "No sensor module config\n"); + return PTR_ERR(sensor_config); + } + + if (!sensor_config->supply_map.supply) { + dev_err(&int3472->pdev->dev, "No supply name defined\n"); + return -ENODEV; + } + + init_data.supply_regulator = NULL; + 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"); + 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]); + if (IS_ERR(int3472->regulator.gpio)) { + dev_err(&int3472->pdev->dev, "Failed to get GPIO line\n"); + return PTR_ERR(int3472->regulator.gpio); + } + + cfg.dev = &int3472->adev->dev; + cfg.init_data = &init_data; + cfg.ena_gpiod = int3472->regulator.gpio; + + int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc, + &cfg); + if (IS_ERR(int3472->regulator.rdev)) { + ret = PTR_ERR(int3472->regulator.rdev); + goto err_free_gpio; + } + + return 0; + +err_free_gpio: + gpiod_put(int3472->regulator.gpio); + + return ret; +} + +/** + * skl_int3472_handle_gpio_resources: maps PMIC resources to consuming sensor + * @ares: A pointer to a &struct acpi_resource + * @data: A pointer to a &struct int3472_device + * + * This function handles GPIO resources that are against an INT3472 + * ACPI device, by checking the value of the corresponding _DSM entry. + * This will return a 32bit int, where the lowest byte represents the + * function of the GPIO pin: + * + * 0x00 Reset + * 0x01 Power down + * 0x0b Power enable + * 0x0c Clock enable + * 0x0d Privacy LED + * + * There are some known platform specific quirks where that does not quite + * hold up; for example where a pin with type 0x01 (Power down) is mapped to + * a sensor pin that performs a reset function or entries in _CRS and _DSM that + * do not actually correspond to a physical connection. These will be handled by + * the mapping sub-functions. + * + * GPIOs will either be mapped directly to the sensor device or else used + * to create clocks and regulators via the usual frameworks. + * + * Return: + * * 0 - When all resources found are handled properly. + * * -EINVAL - If the resource is not a GPIO IO resource + * * -ENODEV - If the resource has no corresponding _DSM entry + * * -Other - Errors propagated from one of the sub-functions. + */ +static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, + void *data) +{ + struct int3472_device *int3472 = data; + union acpi_object *obj; + int ret = 0; + + if (ares->type != ACPI_RESOURCE_TYPE_GPIO || + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO) + return EINVAL; /* Deliberately positive so parsing continues */ + + /* + * n_gpios + 2 because the index of this _DSM function is 1-based and + * the first function is just a count. + */ + obj = acpi_evaluate_dsm_typed(int3472->adev->handle, + &int3472_gpio_guid, 0x00, + int3472->n_gpios + 2, + NULL, ACPI_TYPE_INTEGER); + + if (!obj) { + dev_warn(&int3472->pdev->dev, + "No _DSM entry for this GPIO pin\n"); + return ENODEV; + } + + switch (obj->integer.value & 0xff) { + case INT3472_GPIO_TYPE_RESET: + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, "reset", + GPIO_ACTIVE_LOW); + if (ret) + dev_err(&int3472->pdev->dev, + "Failed to map reset pin to sensor\n"); + + break; + case INT3472_GPIO_TYPE_POWERDOWN: + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, "powerdown", + GPIO_ACTIVE_LOW); + if (ret) + dev_err(&int3472->pdev->dev, + "Failed to map powerdown pin to sensor\n"); + + break; + case INT3472_GPIO_TYPE_CLK_ENABLE: + ret = skl_int3472_register_clock(int3472, ares); + if (ret) + dev_err(&int3472->pdev->dev, + "Failed to map clock to sensor\n"); + + break; + case INT3472_GPIO_TYPE_POWER_ENABLE: + ret = skl_int3472_register_regulator(int3472, ares); + if (ret) { + dev_err(&int3472->pdev->dev, + "Failed to map regulator to sensor\n"); + } + + break; + case INT3472_GPIO_TYPE_PRIVACY_LED: + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, + "indicator-led", + GPIO_ACTIVE_HIGH); + if (ret) + dev_err(&int3472->pdev->dev, + "Failed to map indicator led to sensor\n"); + + break; + default: + dev_warn(&int3472->pdev->dev, + "GPIO type 0x%llx unknown; the sensor may not work\n", + (obj->integer.value & 0xff)); + ret = EINVAL; + } + + int3472->n_gpios++; + ACPI_FREE(obj); + + return ret; +} + +static int skl_int3472_parse_crs(struct int3472_device *int3472) +{ + struct list_head resource_list; + int ret = 0; + + INIT_LIST_HEAD(&resource_list); + + ret = acpi_dev_get_resources(int3472->adev, &resource_list, + skl_int3472_handle_gpio_resources, + int3472); + + if (!ret) { + gpiod_add_lookup_table(&int3472->gpios); + int3472->gpios_mapped = true; + } + + acpi_dev_free_resource_list(&resource_list); + + return ret; +} + +int skl_int3472_discrete_probe(struct platform_device *pdev) +{ + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev); + struct int3472_device *int3472; + struct int3472_cldb cldb; + int ret = 0; + + ret = skl_int3472_get_cldb_buffer(adev, &cldb); + if (ret || cldb.control_logic_type != 1) + return -EINVAL; + + int3472 = kzalloc(sizeof(*int3472) + + ((INT3472_MAX_SENSOR_GPIOS + 1) * sizeof(struct gpiod_lookup)), + GFP_KERNEL); + if (!int3472) + return -ENOMEM; + + int3472->adev = adev; + int3472->pdev = pdev; + platform_set_drvdata(pdev, int3472); + + int3472->sensor = acpi_dev_get_next_dep_dev(adev, NULL); + if (!int3472->sensor) { + dev_err(&pdev->dev, + "This INT3472 entry seems to have no dependents.\n"); + ret = -ENODEV; + goto err_free_int3472; + } + int3472->sensor_name = i2c_acpi_dev_name(int3472->sensor); + int3472->gpios.dev_id = int3472->sensor_name; + + ret = skl_int3472_parse_crs(int3472); + if (ret) { + skl_int3472_discrete_remove(pdev); + goto err_return_ret; + } + + return 0; + +err_free_int3472: + kfree(int3472); +err_return_ret: + return ret; +} + +int skl_int3472_discrete_remove(struct platform_device *pdev) +{ + struct int3472_device *int3472; + + int3472 = platform_get_drvdata(pdev); + + if (int3472->gpios_mapped) + gpiod_remove_lookup_table(&int3472->gpios); + + if (!IS_ERR_OR_NULL(int3472->regulator.rdev)) { + gpiod_put(int3472->regulator.gpio); + regulator_unregister(int3472->regulator.rdev); + } + + if (!IS_ERR_OR_NULL(int3472->clock.clk)) { + gpiod_put(int3472->clock.gpio); + clk_unregister(int3472->clock.clk); + clkdev_drop(int3472->clock.cl); + } + + acpi_dev_put(int3472->sensor); + + kfree(int3472->sensor_name); + kfree(int3472); + + return 0; +} diff --git a/drivers/platform/x86/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel_skl_int3472_tps68470.c new file mode 100644 index 000000000000..9bda65d34987 --- /dev/null +++ b/drivers/platform/x86/intel_skl_int3472_tps68470.c @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Author: Dan Scally <djrscally@gmail.com> */ + +#include <linux/i2c.h> +#include <linux/mfd/tps68470.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#include "intel_skl_int3472_common.h" + +static const struct regmap_config tps68470_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = TPS68470_REG_MAX, +}; + +static int tps68470_chip_init(struct device *dev, struct regmap *regmap) +{ + unsigned int version; + int ret; + + /* Force software reset */ + ret = regmap_write(regmap, TPS68470_REG_RESET, TPS68470_REG_RESET_MASK); + if (ret) + return ret; + + ret = regmap_read(regmap, TPS68470_REG_REVID, &version); + if (ret) { + dev_err(dev, "Failed to read revision register: %d\n", ret); + return ret; + } + + dev_info(dev, "TPS68470 REVID: 0x%x\n", version); + + return 0; +} + +static struct platform_device * +skl_int3472_register_pdev(const char *name, struct device *parent) +{ + struct platform_device *pdev; + int ret; + + pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE); + if (IS_ERR_OR_NULL(pdev)) + return ERR_PTR(-ENOMEM); + + pdev->dev.parent = parent; + pdev->driver_override = kstrndup(pdev->name, INT3472_PDEV_MAX_NAME_LEN, + GFP_KERNEL); + + ret = platform_device_add(pdev); + if (ret) { + platform_device_put(pdev); + return ERR_PTR(ret); + } + + return pdev; +} + +int skl_int3472_tps68470_probe(struct i2c_client *client) +{ + struct acpi_device *adev = ACPI_COMPANION(&client->dev); + struct platform_device *regulator_dev; + struct platform_device *opregion_dev; + struct platform_device *gpio_dev; + struct int3472_cldb cldb = { 0 }; + struct platform_device *clk_dev; + bool cldb_present = true; + struct regmap *regmap; + int ret = 0; + + regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config); + if (IS_ERR(regmap)) { + dev_err(&client->dev, "devm_regmap_init_i2c Error %ld\n", + PTR_ERR(regmap)); + return PTR_ERR(regmap); + } + + i2c_set_clientdata(client, regmap); + + ret = tps68470_chip_init(&client->dev, regmap); + if (ret < 0) { + dev_err(&client->dev, "TPS68470 Init Error %d\n", ret); + return ret; + } + + /* + * Check CLDB buffer against the PMIC's adev. If present, then we check + * the value of control_logic_type field and follow one of the + * following scenarios: + * + * 1. No CLDB - likely ACPI tables designed for ChromeOS. We + * create platform devices for the GPIOs and OpRegion drivers. + * + * 2. CLDB, with control_logic_type = 2 - probably ACPI tables + * made for Windows 2-in-1 platforms. Register pdevs for GPIO, + * Clock and Regulator drivers to bind to. + * + * 3. Any other value in control_logic_type, we should never have + * gotten to this point; crash and burn. + */ + ret = skl_int3472_get_cldb_buffer(adev, &cldb); + if (!ret && cldb.control_logic_type != 2) + return -EINVAL; + + if (ret) + cldb_present = false; + + gpio_dev = skl_int3472_register_pdev("tps68470-gpio", &client->dev); + if (IS_ERR(gpio_dev)) + return PTR_ERR(gpio_dev); + + if (cldb_present) { + clk_dev = skl_int3472_register_pdev("tps68470-clk", + &client->dev); + if (IS_ERR(clk_dev)) { + ret = PTR_ERR(clk_dev); + goto err_free_gpio; + } + + regulator_dev = skl_int3472_register_pdev("tps68470-regulator", + &client->dev); + if (IS_ERR(regulator_dev)) { + ret = PTR_ERR(regulator_dev); + goto err_free_clk; + } + } else { + opregion_dev = skl_int3472_register_pdev("tps68470_pmic_opregion", + &client->dev); + if (IS_ERR(opregion_dev)) { + ret = PTR_ERR(opregion_dev); + goto err_free_gpio; + } + } + + return 0; + +err_free_clk: + platform_device_put(clk_dev); +err_free_gpio: + platform_device_put(gpio_dev); + + return ret; +}
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. Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v2: - Switched to a module registering a platform driver to run the dummy ACPI devices, plus an i2c driver to replace and extend the existing tps68470 driver - Added clock handling functions to the int3472-discrete driver - A whole bunch of other changes too numerous to enumerate MAINTAINERS | 5 + drivers/platform/x86/Kconfig | 25 + drivers/platform/x86/Makefile | 4 + .../platform/x86/intel_skl_int3472_common.c | 100 ++++ .../platform/x86/intel_skl_int3472_common.h | 100 ++++ .../platform/x86/intel_skl_int3472_discrete.c | 496 ++++++++++++++++++ .../platform/x86/intel_skl_int3472_tps68470.c | 145 +++++ 7 files changed, 875 insertions(+) create mode 100644 drivers/platform/x86/intel_skl_int3472_common.c create mode 100644 drivers/platform/x86/intel_skl_int3472_common.h create mode 100644 drivers/platform/x86/intel_skl_int3472_discrete.c create mode 100644 drivers/platform/x86/intel_skl_int3472_tps68470.c