| Message ID | 20210824230620.1003828-2-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add regulator_lookup_list and API | expand |
On Wed, Aug 25, 2021 at 12:06:18AM +0100, Daniel Scally wrote: > In some situations regulator devices can be enumerated via either > devicetree or ACPI and bound to regulator drivers but without any > init data being provided in firmware. This leaves their consumers > unable to acquire them via regulator_get(). > > To fix the issue, add the ability to register a lookup table to a > list within regulator core, which will allow board files to provide > init data via matching against the regulator name and device name in > the same fashion as the gpiod lookup table. This is the wrong level to do this I think, this is a generic problem that affects all kinds of platform data so if we're not going to scatter DMI quirks throughout the drivers like we currently do then we should have a way for boards to just store generic platform data for a device and then have that platform data joined up with the device later. This could for example also be used by all the laptop audio subsystems which need DMI quirk tables in drivers for their components to figure out how they're wired up and avoids the need to go through subsystems adding new APIs.
On Wed, Aug 25, 2021 at 1:34 PM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Aug 25, 2021 at 12:06:18AM +0100, Daniel Scally wrote: > > In some situations regulator devices can be enumerated via either > > devicetree or ACPI and bound to regulator drivers but without any > > init data being provided in firmware. This leaves their consumers > > unable to acquire them via regulator_get(). > > > > To fix the issue, add the ability to register a lookup table to a > > list within regulator core, which will allow board files to provide > > init data via matching against the regulator name and device name in > > the same fashion as the gpiod lookup table. > > This is the wrong level to do this I think, this is a generic problem > that affects all kinds of platform data so if we're not going to scatter > DMI quirks throughout the drivers like we currently do then we should > have a way for boards to just store generic platform data for a device > and then have that platform data joined up with the device later. This > could for example also be used by all the laptop audio subsystems which > need DMI quirk tables in drivers for their components to figure out how > they're wired up and avoids the need to go through subsystems adding new > APIs. What you are describing sounds similar to what we have, i.e. fwnode graph. That's how v4l2 describes complex connections between devices in the camera world. But IIRC you don't like this idea to be present with regulators (as per v1 of this series). So, what do you propose then? -- With Best Regards, Andy Shevchenko
On Wed, Aug 25, 2021 at 02:10:05PM +0300, Andy Shevchenko wrote: > On Wed, Aug 25, 2021 at 1:34 PM Mark Brown <broonie@kernel.org> wrote: > > DMI quirks throughout the drivers like we currently do then we should > > have a way for boards to just store generic platform data for a device > > and then have that platform data joined up with the device later. This > What you are describing sounds similar to what we have, i.e. fwnode graph. > That's how v4l2 describes complex connections between devices in the > camera world. > But IIRC you don't like this idea to be present with regulators (as > per v1 of this series). No, what was proposed for regulator was to duplicate all the the DT binding code in the regulator framework so it parses fwnodes then have an API for encoding fwnodes from C data structures at runtime. The bit where the data gets joined up with the devices isn't the problem, it's the duplication and fragility introduced by encoding everything into an intermediate representation that has no purpose and passing that around which is the problem. > So, what do you propose then? I propose what I suggested above, providing a way to attach platform data to firmware provided devices. Don't bother with an intermediate encoding, just use platform data. Or just put quirks in the drivers like all the other systems using ACPI for platforms where it's not a good fit.
On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote: > On Wed, Aug 25, 2021 at 02:10:05PM +0300, Andy Shevchenko wrote: > > On Wed, Aug 25, 2021 at 1:34 PM Mark Brown <broonie@kernel.org> wrote: > > > > DMI quirks throughout the drivers like we currently do then we should > > > have a way for boards to just store generic platform data for a device > > > and then have that platform data joined up with the device later. This > > > What you are describing sounds similar to what we have, i.e. fwnode graph. > > That's how v4l2 describes complex connections between devices in the > > camera world. > > > But IIRC you don't like this idea to be present with regulators (as > > per v1 of this series). > > No, what was proposed for regulator was to duplicate all the the DT > binding code in the regulator framework so it parses fwnodes then have > an API for encoding fwnodes from C data structures at runtime. The bit > where the data gets joined up with the devices isn't the problem, it's > the duplication and fragility introduced by encoding everything into > an intermediate representation that has no purpose and passing that > around which is the problem. The whole exercise with swnode is to minimize the driver intrusion and evolving a unified way for (some) of the device properties. V4L2 won't like what you are suggesting exactly because they don't like the idea of spreading the board code over the drivers. In some cases it might even be not so straightforward and easy. Laurent, do I understand correctly the v4l2 expectations? > > So, what do you propose then? > > I propose what I suggested above, providing a way to attach platform > data to firmware provided devices. Don't bother with an intermediate > encoding, just use platform data. Or just put quirks in the drivers > like all the other systems using ACPI for platforms where it's not a > good fit.
On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote: > On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote: > > No, what was proposed for regulator was to duplicate all the the DT > > binding code in the regulator framework so it parses fwnodes then have > > an API for encoding fwnodes from C data structures at runtime. The bit > > where the data gets joined up with the devices isn't the problem, it's > > the duplication and fragility introduced by encoding everything into > > an intermediate representation that has no purpose and passing that > > around which is the problem. > The whole exercise with swnode is to minimize the driver intrusion and > evolving a unified way for (some) of the device properties. V4L2 won't The practical implementation for regulators was to duplicate a substantial amount of code in the core in order to give us a less type safe and more indirect way of passing data from onen C file in the kernel to another. This proposal is a lot better in that it uses the existing init_data and avoids the huge amounts of duplication, it's just not clear from the changelog why it's doing this in a regulator specific manner. *Please* stop trying to force swnodes in everywhere, take on board the feedback about why the swnode implementation is completely inappropriate for regulators. I don't understand why you continue to push this so hard. swnodes and fwnodes are a solution to a specific problem, they're not the answer to every problem out there and having to rehash this continually is getting in the way of actually discussing practical workarounds for these poorly implemented ACPI platforms. > like what you are suggesting exactly because they don't like the idea > of spreading the board code over the drivers. In some cases it might > even be not so straightforward and easy. > Laurent, do I understand correctly the v4l2 expectations? There will be some cases where swnodes make sense, for example where the data is going to be read through the fwnode API since the binding is firmware neutral which I think is the v4l case. On the other hand having a direct C representation is a very common way of implementing DMI quirk tables, and we have things like the regulator API where there's off the shelf platform data support and we actively don't want to support fwnode.
Hello, CC'ing Sakari. On Wed, Aug 25, 2021 at 02:11:39PM +0100, Mark Brown wrote: > On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote: > > On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote: > > > > No, what was proposed for regulator was to duplicate all the the DT > > > binding code in the regulator framework so it parses fwnodes then have > > > an API for encoding fwnodes from C data structures at runtime. The bit > > > where the data gets joined up with the devices isn't the problem, it's > > > the duplication and fragility introduced by encoding everything into > > > an intermediate representation that has no purpose and passing that > > > around which is the problem. > > > The whole exercise with swnode is to minimize the driver intrusion and > > evolving a unified way for (some) of the device properties. V4L2 won't > > The practical implementation for regulators was to duplicate a > substantial amount of code in the core in order to give us a less type > safe and more indirect way of passing data from onen C file in the > kernel to another. This proposal is a lot better in that it uses the > existing init_data and avoids the huge amounts of duplication, it's just > not clear from the changelog why it's doing this in a regulator specific > manner. > > *Please* stop trying to force swnodes in everywhere, take on board the > feedback about why the swnode implementation is completely inappropriate > for regulators. I don't understand why you continue to push this so > hard. swnodes and fwnodes are a solution to a specific problem, they're > not the answer to every problem out there and having to rehash this > continually is getting in the way of actually discussing practical > workarounds for these poorly implemented ACPI platforms. > > > like what you are suggesting exactly because they don't like the idea > > of spreading the board code over the drivers. In some cases it might > > even be not so straightforward and easy. > > > Laurent, do I understand correctly the v4l2 expectations? > > There will be some cases where swnodes make sense, for example where the > data is going to be read through the fwnode API since the binding is > firmware neutral which I think is the v4l case. On the other hand > having a direct C representation is a very common way of implementing > DMI quirk tables, and we have things like the regulator API where > there's off the shelf platform data support and we actively don't want > to support fwnode. From a camera sensor point of view, we want to avoid code duplication. Having to look for regulators using OF lookups *and* platform data in every single sensor driver is not a good solution. This means that, from a camera sensor driver point of view, we want to call regulator_get() (or the devm_ version) with a name, without caring about who establishes the mapping and how the lookup is performed. I don't care much personally if this would be implemented through swnode or a different mechanism, as long as the implementation can be centralized.
On Wed, Aug 25, 2021 at 04:59:36PM +0300, Laurent Pinchart wrote: > Hello, > > CC'ing Sakari. With Sakari's correct address. > On Wed, Aug 25, 2021 at 02:11:39PM +0100, Mark Brown wrote: > > On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote: > > > On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote: > > > > > > No, what was proposed for regulator was to duplicate all the the DT > > > > binding code in the regulator framework so it parses fwnodes then have > > > > an API for encoding fwnodes from C data structures at runtime. The bit > > > > where the data gets joined up with the devices isn't the problem, it's > > > > the duplication and fragility introduced by encoding everything into > > > > an intermediate representation that has no purpose and passing that > > > > around which is the problem. > > > > > The whole exercise with swnode is to minimize the driver intrusion and > > > evolving a unified way for (some) of the device properties. V4L2 won't > > > > The practical implementation for regulators was to duplicate a > > substantial amount of code in the core in order to give us a less type > > safe and more indirect way of passing data from onen C file in the > > kernel to another. This proposal is a lot better in that it uses the > > existing init_data and avoids the huge amounts of duplication, it's just > > not clear from the changelog why it's doing this in a regulator specific > > manner. > > > > *Please* stop trying to force swnodes in everywhere, take on board the > > feedback about why the swnode implementation is completely inappropriate > > for regulators. I don't understand why you continue to push this so > > hard. swnodes and fwnodes are a solution to a specific problem, they're > > not the answer to every problem out there and having to rehash this > > continually is getting in the way of actually discussing practical > > workarounds for these poorly implemented ACPI platforms. > > > > > like what you are suggesting exactly because they don't like the idea > > > of spreading the board code over the drivers. In some cases it might > > > even be not so straightforward and easy. > > > > > Laurent, do I understand correctly the v4l2 expectations? > > > > There will be some cases where swnodes make sense, for example where the > > data is going to be read through the fwnode API since the binding is > > firmware neutral which I think is the v4l case. On the other hand > > having a direct C representation is a very common way of implementing > > DMI quirk tables, and we have things like the regulator API where > > there's off the shelf platform data support and we actively don't want > > to support fwnode. > > From a camera sensor point of view, we want to avoid code duplication. > Having to look for regulators using OF lookups *and* platform data in > every single sensor driver is not a good solution. This means that, from > a camera sensor driver point of view, we want to call regulator_get() > (or the devm_ version) with a name, without caring about who establishes > the mapping and how the lookup is performed. I don't care much > personally if this would be implemented through swnode or a different > mechanism, as long as the implementation can be centralized.
Hi Laurent On 25/08/2021 14:59, Laurent Pinchart wrote: > Hello, > > CC'ing Sakari. > > On Wed, Aug 25, 2021 at 02:11:39PM +0100, Mark Brown wrote: >> On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote: >>> On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote: >>>> No, what was proposed for regulator was to duplicate all the the DT >>>> binding code in the regulator framework so it parses fwnodes then have >>>> an API for encoding fwnodes from C data structures at runtime. The bit >>>> where the data gets joined up with the devices isn't the problem, it's >>>> the duplication and fragility introduced by encoding everything into >>>> an intermediate representation that has no purpose and passing that >>>> around which is the problem. >>> The whole exercise with swnode is to minimize the driver intrusion and >>> evolving a unified way for (some) of the device properties. V4L2 won't >> The practical implementation for regulators was to duplicate a >> substantial amount of code in the core in order to give us a less type >> safe and more indirect way of passing data from onen C file in the >> kernel to another. This proposal is a lot better in that it uses the >> existing init_data and avoids the huge amounts of duplication, it's just >> not clear from the changelog why it's doing this in a regulator specific >> manner. >> >> *Please* stop trying to force swnodes in everywhere, take on board the >> feedback about why the swnode implementation is completely inappropriate >> for regulators. I don't understand why you continue to push this so >> hard. swnodes and fwnodes are a solution to a specific problem, they're >> not the answer to every problem out there and having to rehash this >> continually is getting in the way of actually discussing practical >> workarounds for these poorly implemented ACPI platforms. >> >>> like what you are suggesting exactly because they don't like the idea >>> of spreading the board code over the drivers. In some cases it might >>> even be not so straightforward and easy. >>> Laurent, do I understand correctly the v4l2 expectations? >> There will be some cases where swnodes make sense, for example where the >> data is going to be read through the fwnode API since the binding is >> firmware neutral which I think is the v4l case. On the other hand >> having a direct C representation is a very common way of implementing >> DMI quirk tables, and we have things like the regulator API where >> there's off the shelf platform data support and we actively don't want >> to support fwnode. > From a camera sensor point of view, we want to avoid code duplication. > Having to look for regulators using OF lookups *and* platform data in > every single sensor driver is not a good solution. This means that, from > a camera sensor driver point of view, we want to call regulator_get() > (or the devm_ version) with a name, without caring about who establishes > the mapping and how the lookup is performed. I don't care much > personally if this would be implemented through swnode or a different > mechanism, as long as the implementation can be centralized. I think rather than sensor drivers, the idea would be just to have the tps68470-regulator driver check platform data for the init_data instead, like the tps65023-regulator driver [1] does. I'm sure that'll work, but it's not particularly centralized from the regulator driver's point of view. [1] https://elixir.bootlin.com/linux/latest/source/drivers/regulator/tps65023-regulator.c#L268
Hi Dan, On Wed, Aug 25, 2021 at 03:12:25PM +0100, Daniel Scally wrote: > On 25/08/2021 14:59, Laurent Pinchart wrote: > > Hello, > > > > CC'ing Sakari. > > > > On Wed, Aug 25, 2021 at 02:11:39PM +0100, Mark Brown wrote: > >> On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote: > >>> On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote: > >>>> No, what was proposed for regulator was to duplicate all the the DT > >>>> binding code in the regulator framework so it parses fwnodes then have > >>>> an API for encoding fwnodes from C data structures at runtime. The bit > >>>> where the data gets joined up with the devices isn't the problem, it's > >>>> the duplication and fragility introduced by encoding everything into > >>>> an intermediate representation that has no purpose and passing that > >>>> around which is the problem. > >>> The whole exercise with swnode is to minimize the driver intrusion and > >>> evolving a unified way for (some) of the device properties. V4L2 won't > >> The practical implementation for regulators was to duplicate a > >> substantial amount of code in the core in order to give us a less type > >> safe and more indirect way of passing data from onen C file in the > >> kernel to another. This proposal is a lot better in that it uses the > >> existing init_data and avoids the huge amounts of duplication, it's just > >> not clear from the changelog why it's doing this in a regulator specific > >> manner. > >> > >> *Please* stop trying to force swnodes in everywhere, take on board the > >> feedback about why the swnode implementation is completely inappropriate > >> for regulators. I don't understand why you continue to push this so > >> hard. swnodes and fwnodes are a solution to a specific problem, they're > >> not the answer to every problem out there and having to rehash this > >> continually is getting in the way of actually discussing practical > >> workarounds for these poorly implemented ACPI platforms. > >> > >>> like what you are suggesting exactly because they don't like the idea > >>> of spreading the board code over the drivers. In some cases it might > >>> even be not so straightforward and easy. > >>> Laurent, do I understand correctly the v4l2 expectations? > >> There will be some cases where swnodes make sense, for example where the > >> data is going to be read through the fwnode API since the binding is > >> firmware neutral which I think is the v4l case. On the other hand > >> having a direct C representation is a very common way of implementing > >> DMI quirk tables, and we have things like the regulator API where > >> there's off the shelf platform data support and we actively don't want > >> to support fwnode. > > From a camera sensor point of view, we want to avoid code duplication. > > Having to look for regulators using OF lookups *and* platform data in > > every single sensor driver is not a good solution. This means that, from > > a camera sensor driver point of view, we want to call regulator_get() > > (or the devm_ version) with a name, without caring about who establishes > > the mapping and how the lookup is performed. I don't care much > > personally if this would be implemented through swnode or a different > > mechanism, as long as the implementation can be centralized. > > I think rather than sensor drivers, the idea would be just to have the > tps68470-regulator driver check platform data for the init_data instead, > like the tps65023-regulator driver [1] does. I'm sure that'll work, but > it's not particularly centralized from the regulator driver's point of > view. That would obviously be less of an issue from a V4L2 point of view :-) Given that the situation we're facing doesn't affect (to our knowledge) a very large number of regulators, it may not be too bad in practice. If I were to maintain the regulator subsystem I'd probably want a centralized implementation there, but that's certainly a personal preference, at least partly. On a side note, this RFC looks quite similar to what the GPIO subsystem does, which I personally consider nice as differences between regulator and GPIO in these areas are confusing for users. > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/tps65023-regulator.c#L268
+Cc: Heikki On Wed, Aug 25, 2021 at 5:03 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wed, Aug 25, 2021 at 04:59:36PM +0300, Laurent Pinchart wrote: > > On Wed, Aug 25, 2021 at 02:11:39PM +0100, Mark Brown wrote: > > > On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote: > > > > On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote: > > > > > > > > No, what was proposed for regulator was to duplicate all the the DT > > > > > binding code in the regulator framework so it parses fwnodes then have > > > > > an API for encoding fwnodes from C data structures at runtime. The bit > > > > > where the data gets joined up with the devices isn't the problem, it's > > > > > the duplication and fragility introduced by encoding everything into > > > > > an intermediate representation that has no purpose and passing that > > > > > around which is the problem. > > > > > > > The whole exercise with swnode is to minimize the driver intrusion and > > > > evolving a unified way for (some) of the device properties. V4L2 won't > > > > > > The practical implementation for regulators was to duplicate a > > > substantial amount of code in the core in order to give us a less type > > > safe and more indirect way of passing data from onen C file in the > > > kernel to another. This proposal is a lot better in that it uses the > > > existing init_data and avoids the huge amounts of duplication, it's just > > > not clear from the changelog why it's doing this in a regulator specific > > > manner. > > > > > > *Please* stop trying to force swnodes in everywhere, take on board the > > > feedback about why the swnode implementation is completely inappropriate > > > for regulators. I don't understand why you continue to push this so > > > hard. swnodes and fwnodes are a solution to a specific problem, they're > > > not the answer to every problem out there and having to rehash this > > > continually is getting in the way of actually discussing practical > > > workarounds for these poorly implemented ACPI platforms. > > > > > > > like what you are suggesting exactly because they don't like the idea > > > > of spreading the board code over the drivers. In some cases it might > > > > even be not so straightforward and easy. > > > > > > > Laurent, do I understand correctly the v4l2 expectations? > > > > > > There will be some cases where swnodes make sense, for example where the > > > data is going to be read through the fwnode API since the binding is > > > firmware neutral which I think is the v4l case. On the other hand > > > having a direct C representation is a very common way of implementing > > > DMI quirk tables, and we have things like the regulator API where > > > there's off the shelf platform data support and we actively don't want > > > to support fwnode. > > > > From a camera sensor point of view, we want to avoid code duplication. > > Having to look for regulators using OF lookups *and* platform data in > > every single sensor driver is not a good solution. This means that, from > > a camera sensor driver point of view, we want to call regulator_get() > > (or the devm_ version) with a name, without caring about who establishes > > the mapping and how the lookup is performed. I don't care much > > personally if this would be implemented through swnode or a different > > mechanism, as long as the implementation can be centralized. Heikki and I discussed another (possible) approach, He submitted at some point a series [1] that adds PM domain to software nodes. In such case the software node callbacks can handle regulator, clocks, etc which are needed to have it work (of course in case of optional resources, for mandatory ones we have to provide them anyway). [1]: https://lore.kernel.org/linux-acpi/20201029105941.63410-1-heikki.krogerus@linux.intel.com/
On Wed, Aug 25, 2021 at 04:59:35PM +0300, Laurent Pinchart wrote: > From a camera sensor point of view, we want to avoid code duplication. > Having to look for regulators using OF lookups *and* platform data in > every single sensor driver is not a good solution. This means that, from > a camera sensor driver point of view, we want to call regulator_get() > (or the devm_ version) with a name, without caring about who establishes > the mapping and how the lookup is performed. I don't care much > personally if this would be implemented through swnode or a different > mechanism, as long as the implementation can be centralized. That's all orthogonal to this discussion, it's about how we configure the regulators not how clients use the regulators - as you say anything to do with how the regulator is configured should be totally transparent there.
Hi all, On 8/25/21 12:33 PM, Mark Brown wrote: > On Wed, Aug 25, 2021 at 12:06:18AM +0100, Daniel Scally wrote: >> In some situations regulator devices can be enumerated via either >> devicetree or ACPI and bound to regulator drivers but without any >> init data being provided in firmware. This leaves their consumers >> unable to acquire them via regulator_get(). >> >> To fix the issue, add the ability to register a lookup table to a >> list within regulator core, which will allow board files to provide >> init data via matching against the regulator name and device name in >> the same fashion as the gpiod lookup table. > > This is the wrong level to do this I think, this is a generic problem > that affects all kinds of platform data so if we're not going to scatter > DMI quirks throughout the drivers like we currently do then we should > have a way for boards to just store generic platform data for a device > and then have that platform data joined up with the device later. This > could for example also be used by all the laptop audio subsystems which > need DMI quirk tables in drivers for their components to figure out how > they're wired up and avoids the need to go through subsystems adding new > APIs. Daniel, I believe that what Mark wants here is something similar to what we already do for the 5v boost converter regulator in the TI bq24190 charger chip used on some Cherry Trail devices. There the entire i2c-client is instantiated by platform code here: drivers/i2c/busses/i2c-cht-wc.c This attaches a struct bq24190_platform_data as platform data to the i2c-client, this struct contains a single const struct regulator_init_data *regulator_init_data member which then gets consumed (if there is platform data set) by the regulator code in: drivers/power/supply/bq24190_charger.c For the tps68470 regulator code the platform_data then would need to have 3 const struct regulator_init_data * members one for each of the 3 regulators. This platform_data could then be directly set (based on a DMI match table) from intel_skl_int3472_tps68470.c avoiding probe-ordering issues (1) with the lookup solution and will allow the code containing the DMI and regulator_init_data tables to be build as a module. So all in all I think that this will be a better solution. Regards, Hans 1) You are forcing the DMI matching driver adding the lookups to be builtin but what if the tps68740-MFD + regulatorcode is also builtin, then there still is no guarantee the lookups will be added before the regulator drivers' probe function runs p.s. I see that you mention something similar later in the thread referring to the tps65023-regulator driver. I did not check, but assuming that uses what I describe above; then yes the idea would be to do something similar for the tps68740-code, setting the platform_data when (just before) the MFD-cells are instantiated from intel_skl_int3472_tps68470.c
On Wed, Aug 25, 2021 at 05:22:49PM +0300, Laurent Pinchart wrote: > a very large number of regulators, it may not be too bad in practice. If > I were to maintain the regulator subsystem I'd probably want a > centralized implementation there, but that's certainly a personal > preference, at least partly. We already have some generic platform data for regulators so if it doesn't want to define anything custom all a driver has to do is forward that struct on to the core for handling, otherwise it just has to pick the generic struct out of whatever custom thing it defines. > On a side note, this RFC looks quite similar to what the GPIO subsystem > does, which I personally consider nice as differences between regulator > and GPIO in these areas are confusing for users. My pushback here is that if all we're doing is providing a mechanism to match platform data with firmware provided devices we shouldn't be implementing it per API in the first place, we should have a generic mechanism for system level quirking to pass platform data to devices which works with anything - it could also absorb a lot of the DMI based quirking we have in drivers already which boil down to using a DMI match to pick some platform data.
On Wed, Aug 25, 2021 at 04:48:15PM +0200, Hans de Goede wrote: > Daniel, I believe that what Mark wants here is something similar to what > we already do for the 5v boost converter regulator in the TI bq24190 charger > chip used on some Cherry Trail devices. Yeah, that or something like a generalized version of it which lets a separate quirk file like they seem to have register the data to insert - I'd be happy enough with the simple thing too given that it's not visible to anything, or with DMI quirks in the regulator driver too for that matter if it's just one or two platforms but there do seem to be rather a lot of these platforms which need quirks.
On Wed, Aug 25, 2021 at 04:27:35PM +0100, Mark Brown wrote: > On Wed, Aug 25, 2021 at 04:48:15PM +0200, Hans de Goede wrote: > > > Daniel, I believe that what Mark wants here is something similar to what > > we already do for the 5v boost converter regulator in the TI bq24190 charger > > chip used on some Cherry Trail devices. > > Yeah, that or something like a generalized version of it which lets a > separate quirk file like they seem to have register the data to insert - > I'd be happy enough with the simple thing too given that it's not > visible to anything, or with DMI quirks in the regulator driver too for > that matter if it's just one or two platforms but there do seem to be > rather a lot of these platforms which need quirks. Let's also remember that we have to handle not just regulators, but also GPIOs and clocks. And I'm pretty sure there will be more. We could have a mechanism specific to the tps68470 driver to pass platform data from the board file to the driver, and replicate that mechanism in different drivers (for other regulators, clocks and GPIOs), but I really would like to avoid splitting the DMI-conditioned platform data in those drivers directly. I'd like to store all the init data for a given platform in a single "board" file.
On Wed, Aug 25, 2021 at 06:42:30PM +0300, Laurent Pinchart wrote: > On Wed, Aug 25, 2021 at 04:27:35PM +0100, Mark Brown wrote: > > Yeah, that or something like a generalized version of it which lets a > > separate quirk file like they seem to have register the data to insert - > Let's also remember that we have to handle not just regulators, but also > GPIOs and clocks. And I'm pretty sure there will be more. We could have > a mechanism specific to the tps68470 driver to pass platform data from > the board file to the driver, and replicate that mechanism in different > drivers (for other regulators, clocks and GPIOs), but I really would > like to avoid splitting the DMI-conditioned platform data in those > drivers directly. I'd like to store all the init data for a given > platform in a single "board" file. Right, that's the more general bit I'm suggesting.
Hi, On 8/25/21 5:42 PM, Laurent Pinchart wrote: > On Wed, Aug 25, 2021 at 04:27:35PM +0100, Mark Brown wrote: >> On Wed, Aug 25, 2021 at 04:48:15PM +0200, Hans de Goede wrote: >> >>> Daniel, I believe that what Mark wants here is something similar to what >>> we already do for the 5v boost converter regulator in the TI bq24190 charger >>> chip used on some Cherry Trail devices. >> >> Yeah, that or something like a generalized version of it which lets a >> separate quirk file like they seem to have register the data to insert - >> I'd be happy enough with the simple thing too given that it's not >> visible to anything, or with DMI quirks in the regulator driver too for >> that matter if it's just one or two platforms but there do seem to be >> rather a lot of these platforms which need quirks. > > Let's also remember that we have to handle not just regulators, but also > GPIOs and clocks. And I'm pretty sure there will be more. We could have > a mechanism specific to the tps68470 driver to pass platform data from > the board file to the driver, and replicate that mechanism in different > drivers (for other regulators, clocks and GPIOs), but I really would > like to avoid splitting the DMI-conditioned platform data in those > drivers directly. I'd like to store all the init data for a given > platform in a single "board" file. I agree, but so far all the handling for clks/gpios for IPU3 (+ IPU4 (*)) laptops is done in the drivers/platform/x86/intel/int3472 code and the passing of platform_data with regulator init-data would also happen in the mfd-cell instantiation code living there. IOW if we just go with that then we will already have everything in one place. At least for the IPU3 case. Regards, Hans *) IPU4 also used the INT3472 ACPI devices and what we have for discrete IO devices seems to match.
Hi Hans, On Wed, Aug 25, 2021 at 10:25:23PM +0200, Hans de Goede wrote: > On 8/25/21 5:42 PM, Laurent Pinchart wrote: > > On Wed, Aug 25, 2021 at 04:27:35PM +0100, Mark Brown wrote: > >> On Wed, Aug 25, 2021 at 04:48:15PM +0200, Hans de Goede wrote: > >> > >>> Daniel, I believe that what Mark wants here is something similar to what > >>> we already do for the 5v boost converter regulator in the TI bq24190 charger > >>> chip used on some Cherry Trail devices. > >> > >> Yeah, that or something like a generalized version of it which lets a > >> separate quirk file like they seem to have register the data to insert - > >> I'd be happy enough with the simple thing too given that it's not > >> visible to anything, or with DMI quirks in the regulator driver too for > >> that matter if it's just one or two platforms but there do seem to be > >> rather a lot of these platforms which need quirks. > > > > Let's also remember that we have to handle not just regulators, but also > > GPIOs and clocks. And I'm pretty sure there will be more. We could have > > a mechanism specific to the tps68470 driver to pass platform data from > > the board file to the driver, and replicate that mechanism in different > > drivers (for other regulators, clocks and GPIOs), but I really would > > like to avoid splitting the DMI-conditioned platform data in those > > drivers directly. I'd like to store all the init data for a given > > platform in a single "board" file. > > I agree, but so far all the handling for clks/gpios for IPU3 (+ IPU4 (*)) > laptops is done in the drivers/platform/x86/intel/int3472 code and the > passing of platform_data with regulator init-data would also happen in > the mfd-cell instantiation code living there. IOW if we just go with > that then we will already have everything in one place. At least > for the IPU3 case. If the GPIOs are also hooked up to the TPS68470 and not to GPIOs of the SoC, then I suppose that would be fine in this case. Do you have any plan to work on IPU4 support ? ;-) > *) IPU4 also used the INT3472 ACPI devices and what we have for discrete > IO devices seems to match.
Hi Hans On 25/08/2021 15:48, Hans de Goede wrote: > Hi all, > > On 8/25/21 12:33 PM, Mark Brown wrote: >> On Wed, Aug 25, 2021 at 12:06:18AM +0100, Daniel Scally wrote: >>> In some situations regulator devices can be enumerated via either >>> devicetree or ACPI and bound to regulator drivers but without any >>> init data being provided in firmware. This leaves their consumers >>> unable to acquire them via regulator_get(). >>> >>> To fix the issue, add the ability to register a lookup table to a >>> list within regulator core, which will allow board files to provide >>> init data via matching against the regulator name and device name in >>> the same fashion as the gpiod lookup table. >> >> This is the wrong level to do this I think, this is a generic problem >> that affects all kinds of platform data so if we're not going to scatter >> DMI quirks throughout the drivers like we currently do then we should >> have a way for boards to just store generic platform data for a device >> and then have that platform data joined up with the device later. This >> could for example also be used by all the laptop audio subsystems which >> need DMI quirk tables in drivers for their components to figure out how >> they're wired up and avoids the need to go through subsystems adding new >> APIs. > > Daniel, I believe that what Mark wants here is something similar to what > we already do for the 5v boost converter regulator in the TI bq24190 charger > chip used on some Cherry Trail devices. > > There the entire i2c-client is instantiated by platform code here: > drivers/i2c/busses/i2c-cht-wc.c > > This attaches a struct bq24190_platform_data as platform data to > the i2c-client, this struct contains a single > > const struct regulator_init_data *regulator_init_data > > member which then gets consumed (if there is platform data set) by > the regulator code in: > > drivers/power/supply/bq24190_charger.c > > For the tps68470 regulator code the platform_data then would need to > have 3 const struct regulator_init_data * members one for each of the > 3 regulators. > > This platform_data could then be directly set (based on a DMI match table) > from intel_skl_int3472_tps68470.c avoiding probe-ordering issues (1) with > the lookup solution and will allow the code containing the DMI and > regulator_init_data tables to be build as a module. > > So all in all I think that this will be a better solution. So assign an array of the init_data to the i2c-INT3472:nn device as platform data before registering the MFD cells in intel_skl_int3472_tps68470.c and parse that out when the regulators register. OK - that's fine by me. > > Regards, > > Hans > > > 1) You are forcing the DMI matching driver adding the lookups to be builtin > but what if the tps68740-MFD + regulatorcode is also builtin, then there > still is no guarantee the lookups will be added before the regulator drivers' > probe function runs Err, excellent point - I hadn't thought of that if I'm honest. > > p.s. > > I see that you mention something similar later in the thread referring to > the tps65023-regulator driver. I did not check, but assuming that uses what > I describe above; then yes the idea would be to do something similar for > the tps68740-code, setting the platform_data when (just before) the MFD-cells > are instantiated from intel_skl_int3472_tps68470.c The tps65023-regulator driver parses init data out of platform data yes. I think that that's fine, but personally I'd prefer that done in core.c rather than in the regulator driver itself if possible.
On 25/08/2021 21:40, Laurent Pinchart wrote: > Hi Hans, > > On Wed, Aug 25, 2021 at 10:25:23PM +0200, Hans de Goede wrote: >> On 8/25/21 5:42 PM, Laurent Pinchart wrote: >>> On Wed, Aug 25, 2021 at 04:27:35PM +0100, Mark Brown wrote: >>>> On Wed, Aug 25, 2021 at 04:48:15PM +0200, Hans de Goede wrote: >>>> >>>>> Daniel, I believe that what Mark wants here is something similar to what >>>>> we already do for the 5v boost converter regulator in the TI bq24190 charger >>>>> chip used on some Cherry Trail devices. >>>> >>>> Yeah, that or something like a generalized version of it which lets a >>>> separate quirk file like they seem to have register the data to insert - >>>> I'd be happy enough with the simple thing too given that it's not >>>> visible to anything, or with DMI quirks in the regulator driver too for >>>> that matter if it's just one or two platforms but there do seem to be >>>> rather a lot of these platforms which need quirks. >>> >>> Let's also remember that we have to handle not just regulators, but also >>> GPIOs and clocks. And I'm pretty sure there will be more. We could have >>> a mechanism specific to the tps68470 driver to pass platform data from >>> the board file to the driver, and replicate that mechanism in different >>> drivers (for other regulators, clocks and GPIOs), but I really would >>> like to avoid splitting the DMI-conditioned platform data in those >>> drivers directly. I'd like to store all the init data for a given >>> platform in a single "board" file. >> >> I agree, but so far all the handling for clks/gpios for IPU3 (+ IPU4 (*)) >> laptops is done in the drivers/platform/x86/intel/int3472 code and the >> passing of platform_data with regulator init-data would also happen in >> the mfd-cell instantiation code living there. IOW if we just go with >> that then we will already have everything in one place. At least >> for the IPU3 case. > > If the GPIOs are also hooked up to the TPS68470 and not to GPIOs of the > SoC, then I suppose that would be fine in this case. The GPIOs that we're translating into clks / mapping to the sensors in the INT3472 code are SOC GPIOs actually...this is the first bit of code that relates to a physical TPS68470. > > Do you have any plan to work on IPU4 support ? ;-) > >> *) IPU4 also used the INT3472 ACPI devices and what we have for discrete >> IO devices seems to match. >
Hi Mark On 25/08/2021 15:52, Mark Brown wrote: > On Wed, Aug 25, 2021 at 05:22:49PM +0300, Laurent Pinchart wrote: > >> a very large number of regulators, it may not be too bad in practice. If >> I were to maintain the regulator subsystem I'd probably want a >> centralized implementation there, but that's certainly a personal >> preference, at least partly. > We already have some generic platform data for regulators so if it > doesn't want to define anything custom all a driver has to do is forward > that struct on to the core for handling, otherwise it just has to pick > the generic struct out of whatever custom thing it defines. > >> On a side note, this RFC looks quite similar to what the GPIO subsystem >> does, which I personally consider nice as differences between regulator >> and GPIO in these areas are confusing for users. > My pushback here is that if all we're doing is providing a mechanism to > match platform data with firmware provided devices we shouldn't be > implementing it per API in the first place, we should have a generic > mechanism for system level quirking to pass platform data to devices > which works with anything - it could also absorb a lot of the DMI based > quirking we have in drivers already which boil down to using a DMI match > to pick some platform data. Alright; and what about parsing the platform data into a struct regulator_init_data then...at the moment that's left up to the individual regulator drivers. In my mind that's something better left to core, so rather than finding the right instance of struct regulator_init_data from the regulator_lookup_list the regulator_lookup_init_data() function would be finding the right instance of the struct from cfg->dev->platform_data instead...what are your thoughts on that?
On Wed, Aug 25, 2021 at 11:09:09PM +0100, Daniel Scally wrote: > Alright; and what about parsing the platform data into a struct > regulator_init_data then...at the moment that's left up to the > individual regulator drivers. In my mind that's something better left to > core, so rather than finding the right instance of struct > regulator_init_data from the regulator_lookup_list the > regulator_lookup_init_data() function would be finding the right > instance of the struct from cfg->dev->platform_data instead...what are > your thoughts on that? That sounds like a useful helper to have - we need to let drivers have custom platform data (especially for MFDs) but even there they could use such a helper if they can point it at an array stored elsewhere.
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index ca6caba8a191..6a6c1f34d9d6 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -36,6 +36,8 @@ static DEFINE_WW_CLASS(regulator_ww_class); static DEFINE_MUTEX(regulator_nesting_mutex); static DEFINE_MUTEX(regulator_list_mutex); +static DEFINE_MUTEX(regulator_lookup_mutex); +static LIST_HEAD(regulator_lookup_list); static LIST_HEAD(regulator_map_list); static LIST_HEAD(regulator_ena_gpio_list); static LIST_HEAD(regulator_supply_alias_list); @@ -1868,6 +1870,7 @@ static struct regulator_dev *regulator_lookup_by_name(const char *name) static struct regulator_dev *regulator_dev_lookup(struct device *dev, const char *supply) { + struct regulator_lookup *lookup; struct regulator_dev *r = NULL; struct device_node *node; struct regulator_map *map; @@ -1917,7 +1920,34 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev, if (r) return r; - return ERR_PTR(-ENODEV); + /* + * The regulator might still exist and be called out in a lookup table, + * but simply not yet have probed. Check to see if the consumer device + * is referenced in any init data in the lookups. + */ + + mutex_lock(®ulator_lookup_mutex); + list_for_each_entry(lookup, ®ulator_lookup_list, list) { + struct regulator_init_data *init_data = &lookup->init_data; + unsigned int i; + + if (!devname) + break; + + for (i = 0; i < init_data->num_consumer_supplies; i++) { + if (strcmp(init_data->consumer_supplies[i].dev_name, devname)) + continue; + + r = ERR_PTR(-EPROBE_DEFER); + goto out; + } + } + + r = ERR_PTR(-ENODEV); + +out: + mutex_unlock(®ulator_lookup_mutex); + return r; } static int regulator_resolve_supply(struct regulator_dev *rdev) @@ -5302,6 +5332,54 @@ static struct regulator_coupler generic_regulator_coupler = { .attach_regulator = generic_coupler_attach, }; +/** + * regulator_lookup_init_data - Check regulator_lookup_list for matching entries + * @desc: Regulator desc containing name of the regulator + * @cfg: Regulator config containing pointer to the registering device + * + * Calling this function scans the regulator_lookup_list and checks each entry + * to see if the .device_name and .regulator_name fields match the device name + * and regulator name contained in @cfg and @desc. If so, a pointer to the + * embedded &struct regulator_init_data is returned. No matches returns NULL. + */ +struct regulator_init_data * +regulator_lookup_init_data(const struct regulator_desc *desc, + const struct regulator_config *cfg) +{ + struct regulator_lookup *lookup; + + if (!desc || !cfg || !cfg->dev) + return NULL; + + mutex_lock(®ulator_lookup_mutex); + + list_for_each_entry(lookup, ®ulator_lookup_list, list) { + /* + * We need the lookup to have at least a device_name or there's + * no guarantee of a match, and regulator_register() checks to + * make sure that desc->name is not null, so any entry with + * either field null is invalid. + */ + if (!lookup->device_name || !lookup->regulator_name) + continue; + + if (strcmp(lookup->device_name, dev_name(cfg->dev))) + continue; + + if (strcmp(lookup->regulator_name, desc->name)) + continue; + + goto found; + } + + lookup = NULL; + +found: + mutex_unlock(®ulator_lookup_mutex); + + return lookup ? &lookup->init_data : NULL; +} + /** * regulator_register - register regulator * @regulator_desc: regulator to register @@ -5386,6 +5464,9 @@ regulator_register(const struct regulator_desc *regulator_desc, init_data = regulator_of_get_init_data(dev, regulator_desc, config, &rdev->dev.of_node); + if (!init_data) + init_data = regulator_lookup_init_data(regulator_desc, config); + /* * Sometimes not all resources are probed already so we need to take * that into account. This happens most the time if the ena_gpiod comes @@ -5740,6 +5821,42 @@ void *regulator_get_init_drvdata(struct regulator_init_data *reg_init_data) } EXPORT_SYMBOL_GPL(regulator_get_init_drvdata); +/** + * regulator_add_lookup - Add an entry to regulator_lookup_list + * @lookup: Pointer to the &struct regulator_lookup to add to the list + * + * This function can be used in board files to add an entry to the + * regulator_lookup_list. During regulator_register(), entries will be checked + * for matching device name and regulator name and when matching, the associated + * &struct regulator_init_data will be used. + */ +void regulator_add_lookup(struct regulator_lookup *lookup) +{ + mutex_lock(®ulator_lookup_mutex); + + list_add_tail(&lookup->list, ®ulator_lookup_list); + + mutex_unlock(®ulator_lookup_mutex); +} +EXPORT_SYMBOL_GPL(regulator_add_lookup); + +/** + * regulator_remove_lookup - Remove an entry from regulator_lookup_list + * @lookup: Pointer to the &struct regulator_lookup to remove from the list + * + * Calling this function will remove the passed regulator_lookup from the list, + * intended for error handling paths in board files. + */ +void regulator_remove_lookup(struct regulator_lookup *lookup) +{ + mutex_lock(®ulator_lookup_mutex); + + list_del(&lookup->list); + + mutex_unlock(®ulator_lookup_mutex); +} +EXPORT_SYMBOL_GPL(regulator_remove_lookup); + #ifdef CONFIG_DEBUG_FS static int supply_map_show(struct seq_file *sf, void *data) { diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h index 68b4a514a410..d1b62a3962d8 100644 --- a/include/linux/regulator/machine.h +++ b/include/linux/regulator/machine.h @@ -12,6 +12,7 @@ #ifndef __LINUX_REGULATOR_MACHINE_H_ #define __LINUX_REGULATOR_MACHINE_H_ +#include <linux/list.h> #include <linux/regulator/consumer.h> #include <linux/suspend.h> @@ -272,12 +273,34 @@ struct regulator_init_data { void *driver_data; /* core does not touch this */ }; +/** + * struct regulator_lookup - lookup table entry for regulator_init_data + * + * @device_name: The name of the device from regulator_config.dev + * @regulator_name: The name of the regulator from the &struct regulator_desc + * to match to during regulator_register() + * @init_data: An instance of &struct regulator_init_data that you + * wish to pass to the regulator during regulator_register() + */ +struct regulator_lookup { + const char *device_name; + const char *regulator_name; + + struct regulator_init_data init_data; + + struct list_head list; +}; + #ifdef CONFIG_REGULATOR void regulator_has_full_constraints(void); +void regulator_add_lookup(struct regulator_lookup *lookup); +void regulator_remove_lookup(struct regulator_lookup *lookup); #else static inline void regulator_has_full_constraints(void) { } +static inline void regulator_add_lookup(struct regulator_lookup *lookup) { } +static inline void regulator_remove_lookup(struct regulator_lookup *lookup) { } #endif static inline int regulator_suspend_prepare(suspend_state_t state)
In some situations regulator devices can be enumerated via either devicetree or ACPI and bound to regulator drivers but without any init data being provided in firmware. This leaves their consumers unable to acquire them via regulator_get(). To fix the issue, add the ability to register a lookup table to a list within regulator core, which will allow board files to provide init data via matching against the regulator name and device name in the same fashion as the gpiod lookup table. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v2: - Patch introduced drivers/regulator/core.c | 119 +++++++++++++++++++++++++++++- include/linux/regulator/machine.h | 23 ++++++ 2 files changed, 141 insertions(+), 1 deletion(-)