Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On Sun, Feb 7, 2021 at 1:00 PM Daniel Scally 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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 +, 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 rebirt
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi both On 20/01/2021 11:44, Andy Shevchenko wrote: > 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 +, 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. Well; turns out that we missed an operation we really need to add (clk_recalc_rate) which in our case needs to read a fixed value stored in a buffer in ACPI; most of the code is shared with an existing function in the driver so it's not much extra to add, but I think it kinda precludes using clk-gpio for this anyway >> (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. >
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 +, 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 differen
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 +, 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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 +, 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. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 +, 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.
--
Regards,
Laurent Pinchart
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 +, 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. -- Regards, Laurent Pinchart
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 +, 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? -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 +, 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.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 +, 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. -- Regards, Laurent Pinchart
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 19, 2021 at 10:40:42AM +, 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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Andy, On Tue, Jan 19, 2021 at 11:33:58AM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 12:11:40AM +, 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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Daniel,
On Tue, Jan 19, 2021 at 08:43:43AM +, Daniel Scally wrote:
> On 19/01/2021 06:19, Laurent Pinchart wrote:
> > On Mon, Jan 18, 2021 at 08:46:34PM +, 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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On Tue, Jan 19, 2021 at 10:40:42AM +, 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
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On 19/01/2021 11:11, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 10:56:17AM +, 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.
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On Tue, Jan 19, 2021 at 10:56:17AM +, 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. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 > Signed-off-by: Daniel Scally > --- > 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 > +S: Maintained > +F: drivers/platform/x86/intel_skl_int3472_* > + > INTEL SPEED SELECT TECHNOLOGY > M: Srinivas Pandruvada > L: platform-driver-...@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)
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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",
> +
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On Tue, Jan 19, 2021 at 08:21:23AM +0200, Laurent Pinchart wrote: > On Tue, Jan 19, 2021 at 12:11:40AM +, 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. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On Tue, Jan 19, 2021 at 12:11:40AM +, 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/) -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Morning Andy On 19/01/2021 09:33, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 12:11:40AM +, 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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On Mon, Jan 18, 2021 at 09:19:52PM +, 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 +, 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(
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Laurent
On 19/01/2021 06:19, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Mon, Jan 18, 2021 at 08:46:34PM +, 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 dev
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Daniel,
On Tue, Jan 19, 2021 at 12:11:40AM +, 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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Daniel,
On Mon, Jan 18, 2021 at 08:46:34PM +, 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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Andy - thanks as always for the comments
Some responses below, but if not mentioned I'll follow your suggestion
of course
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 +, Daniel Scally wrote:
> My comments on top of Laurent's to avoid dups.
>
> First of all, PDx86 has its own prefix pattern: "platform/x86: ..."
Sorry, I probably should have put more effort into figuring out the
right pattern for those
>>> +config INTEL_SKL_INT3472
>>> + tristate "Intel SkyLake ACPI INT3472 Driver"
>>> + depends on X86 && ACPI
> X86 is a dup. Entire lot of PDx86 is a priory dependent on it (find "if X86"
> line in Kconfig).
Ah, oh yeah - thanks. I'll watch for that in future
>>> +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?
>>> +struct int3472_gpio_clock {
>>> + struct clk *clk;
>>> + struct clk_hw clk_hw;
>>> + struct clk_lookup *cl;
>>> + struct gpio_desc *gpio;
>>> +};
> Wondering if this has some similarities with and actually can utilize clk-gpio
> driver.
>
>
>
>>> +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.
>>> + /* 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.
>>> +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;
> And after below change you can turn this to
>
> ACPI_FREE(obj);
> return ERR_PTR(-EINVAL);
>
> and remove label completely, but it's up to you.
>
>>> + }
>>> + 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.
>>> +
>>> + int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc,
>>> +&cfg);
>>> + if (IS_ERR(int3472->
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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.
>> +}
>> +
>> +i++;
>> +}
>> +}
>> +
>> +if (!func)
>> +return 0;
> I in
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Hans, On Mon, Jan 18, 2021 at 04:32:54PM +0100, Hans de Goede wrote: > On 1/18/21 4:23 PM, andriy.shevche...@linux.intel.com wrote: > > On Mon, Jan 18, 2021 at 02:51:30PM +, Barnabás Pőcze wrote: > >> 2021. január 18., hétfő 14:51 keltezéssel, Andy Shevchenko írta: > >> > >>> On Mon, Jan 18, 2021 at 11:12:34AM +, 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. > > > > I;m not objecting (at some point in the past I had proposed moving Intel > > stuff > > to a separate folder, but at that time PDx86 has no folders at all and > > Darren > > was kinda not in favour of creating ones, but things changed), just let's > > hear > > Hans on this. > > I'm in favor of using a folder for this and "intel_skl_int3472" is fine with > me. > > With that said I'm not entirely sure if I'm in favor of the _skl_ part of > the folder and driver name or not. > > The INT3472 ACPI device is used with other CPUs, e.g. Apollo Lake too and > I think the driver should work fine with those. It should work on Kabylake as well, although there are some differences in the way the INT3472 device is modelled in the DSDT between those two platforms. Hopefully nothing that couldn't be supported in a single driver without adding too many hacks. > The intel_cht_int33fe case is special because the driver only applies > to some models with an INT33FE ACPI device (the whole INT33FE ACPI device > is a horrible thing which seems to stem from Intel rushing Bay Trail to > market to try and compete on the tablet market). The INT3472 ACPI device is also horrible. It reminds me of Intercal, the programming language that was created by gathering features of several well known languages and then making sure that none of them would be used: the ACPI model of the device was probably created by someone who has studied ACPI extensively and decided to break every single best practice rule. I lack English words strong enough to express my dismay on this topic (but I still have hope to find solace in Finnish). > With that all said SKL probably is the first SoC to feature this and I > guess future IPUs may still use INT3472 given Intel's BAD habit of > re-using ACPI HIDs for multiple incompatible generations. So I guess > that keeping it is fine; and if we then need an incompatible INT3472 > driver for newer IPUs we can use a different prefix for those. > > TL;DR: > > 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. -- Regards, Laurent Pinchart
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi, On 1/18/21 5:00 PM, Daniel Scally wrote: > > On 18/01/2021 15:48, andriy.shevche...@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.shevche...@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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On 18/01/2021 15:48, andriy.shevche...@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.shevche...@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!
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On Mon, Jan 18, 2021 at 04:32:54PM +0100, Hans de Goede wrote: > On 1/18/21 4:23 PM, andriy.shevche...@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? -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi, On 1/18/21 4:23 PM, andriy.shevche...@linux.intel.com wrote: > On Mon, Jan 18, 2021 at 02:51:30PM +, Barnabás Pőcze wrote: >> 2021. január 18., hétfő 14:51 keltezéssel, Andy Shevchenko írta: >> >>> On Mon, Jan 18, 2021 at 11:12:34AM +, 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. > > I;m not objecting (at some point in the past I had proposed moving Intel stuff > to a separate folder, but at that time PDx86 has no folders at all and Darren > was kinda not in favour of creating ones, but things changed), just let's hear > Hans on this. I'm in favor of using a folder for this and "intel_skl_int3472" is fine with me. With that said I'm not entirely sure if I'm in favor of the _skl_ part of the folder and driver name or not. The INT3472 ACPI device is used with other CPUs, e.g. Apollo Lake too and I think the driver should work fine with those. The intel_cht_int33fe case is special because the driver only applies to some models with an INT33FE ACPI device (the whole INT33FE ACPI device is a horrible thing which seems to stem from Intel rushing Bay Trail to market to try and compete on the tablet market). With that all said SKL probably is the first SoC to feature this and I guess future IPUs may still use INT3472 given Intel's BAD habit of re-using ACPI HIDs for multiple incompatible generations. So I guess that keeping it is fine; and if we then need an incompatible INT3472 driver for newer IPUs we can use a different prefix for those. TL;DR: 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. Regards, Hans
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On Mon, Jan 18, 2021 at 02:51:30PM +, Barnabás Pőcze wrote: > 2021. január 18., hétfő 14:51 keltezéssel, Andy Shevchenko írta: > > > On Mon, Jan 18, 2021 at 11:12:34AM +, 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. I;m not objecting (at some point in the past I had proposed moving Intel stuff to a separate folder, but at that time PDx86 has no folders at all and Darren was kinda not in favour of creating ones, but things changed), just let's hear Hans on this. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
2021. január 18., hétfő 14:51 keltezéssel, Andy Shevchenko írta: > On Mon, Jan 18, 2021 at 11:12:34AM +, 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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On Mon, Jan 18, 2021 at 11:15:21AM +0200, Laurent Pinchart wrote:
> On Mon, Jan 18, 2021 at 12:34:27AM +, Daniel Scally wrote:
My comments on top of Laurent's to avoid dups.
First of all, PDx86 has its own prefix pattern: "platform/x86: ..."
> > 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
In cover letter you are using form of "**not**", as well as in prerequisite
patch series, can we be consistent with that?
tps -> TPS (same reason as Laurent put below).
> > 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
I²C driver ?
> > querying the CLDB buffer found against INT3472 entries to determine
> > which is most appropriate.
...
> > +INTEL SKL INT3472 ACPI DEVICE DRIVER
Here you may spell it in full, i.e. SKYLAKE
> > +M: Daniel Scally
> > +S: Maintained
> > +F: drivers/platform/x86/intel_skl_int3472_*
...
> > +config INTEL_SKL_INT3472
> > + tristate "Intel SkyLake ACPI INT3472 Driver"
> > + depends on X86 && ACPI
X86 is a dup. Entire lot of PDx86 is a priory dependent on it (find "if X86"
line in Kconfig).
> > + 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
>
> I'd capitalize TPS, chip names are usually written in upper case, and
> driver names in lower case.
>
> > + PMIC) and one designed for Chrome OS.
...
> > +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
Mix of TABs and spaces detected in above two lines!
...
> > +static const struct acpi_device_id int3472_device_id[] = {
> > + { "INT3472", 0 },
> > + { },
No comma for terminator entry.
> > +};
...
> > +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...
> > +module_init(skl_int3472_init);
> > +module_exit(skl_int3472_exit);
...and this will become simple module_platfom_driver().
...
> > +#define INT3472_REGULATOR(_NAME, _SUPPLY, _OPS)\
>
> Macro arguments don't need to be capitalized.
>
> > + (const struct regulator_desc) { \
> > + .name = _NAME, \
> > + .supply_name = _SUPPLY, \
> > + .id = 0,\
IIRC it will be 0 by standard rules.
> > + .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 \
+ comma
> > + }
...
> > +struct int3472_gpio_clock {
> > + struct clk *clk;
> > + struct clk_hw clk_hw;
> > + struct clk_lookup *cl;
> > + struct gpio_desc *gpio;
> > +};
Wondering if this has some similarities with and actually can utilize clk-gpio
driver.
...
> > + /* 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 an
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
On Mon, Jan 18, 2021 at 11:12:34AM +, 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. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 > Signed-off-by: Daniel Scally > --- > 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
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Daniel, Thank you for the patch. On Mon, Jan 18, 2021 at 12:34:27AM +, 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 > Signed-off-by: Daniel Scally > --- > 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 > +S: Maintained > +F: drivers/platform/x86/intel_skl_int3472_* > + > INTEL SPEED SELECT TECHNOLOGY > M: Srinivas Pandruvada > L: platform-driver-...@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 I'd capitalize TPS, chip names are usually written in upper case, and driver names in lower case. > + 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. > 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 ? > + > + 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" > + >
[PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
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 Signed-off-by: Daniel Scally --- 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 +S: Maintained +F: drivers/platform/x86/intel_skl_int3472_* + INTEL SPEED SELECT TECHNOLOGY M: Srinivas Pandruvada L: platform-driver-...@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
