Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
On Sat, Nov 25, 2017 at 02:00:34PM +, Jonathan Cameron wrote:
> > There does not seem to be any way in ACPI to tell which "connection" is
> > used to describe ARA so that part currently is something each driver
> > needs to handle as they know the device the best. I don't think we have
> > any means to handle it in generic way in I2C core except to provide some
> > helpers that work on top of i2c_setup_smbus_alert() but understand ACPI
> > resources. Say provide function like this:
> >
> > int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);
> >
> > Which then extracts automatically I2cSerialBus connection from "index"
> > and calls i2c_setup_smbus_alert() accordingly.
> >
> > In the long run we could introduce _DSD property that can be used to
> > name the connection in the same way DT does;
> >
> > Name (_CRS, ResourceTemplate () {
> > I2cSerialBus () { ... } // ARA
> > I2cSerialBus () { ... } // normal device address
> > })
> >
> > Name (_DSD, Package () {
> > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > Package () {
> > Package () {"smbus_alert", 0} // Where 0 means the first
> > I2cSerialBus
> > ...
> > }
> > })
> >
> > But it does not help the existing systems.
>
> I'm curious - how would we go about promoting this piece of common sense?
I guess a proper patch series including relevant mailing lists (like
linux-acpi and linux-i2c) would be a good starting point :)
Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
On 25/11/2017 21:57, Jonathan Cameron wrote:
On Tue, 21 Nov 2017 09:22:16 +0800
Phil Reid wrote:
On 20/11/2017 18:57, Mika Westerberg wrote:
+Jarkko
On Sun, Nov 19, 2017 at 04:35:51PM +, Jonathan Cameron wrote:
On Thu, 2 Nov 2017 16:04:07 +0100
Wolfram Sang wrote:
On Thu, Nov 02, 2017 at 02:35:50PM +, Jonathan Cameron wrote:
On Fri, 27 Oct 2017 18:27:02 +0200
Marc CAPDEVILLE wrote:
On asus T100, Capella cm3218 chip is implemented as ambiant light
sensor. This chip expose an smbus ARA protocol device on standard
address 0x0c. The chip is not functional before all alerts are
acknowledged.
On asus T100, this device is enumerated on ACPI bus and the
description give tow I2C connection. The first is the connection to
the ARA device and the second gives the real address of the device.
So, on device probe,If the i2c address is the ARA address and the
device is enumerated via acpi, we lookup for the real address in
the ACPI resource list and change it in the client structure.
if an interrupt resource is given, and only for cm3218 chip,
we declare an smbus_alert device.
Signed-off-by: Marc CAPDEVILLE
Wolfram - this needs input from you on how to neatly handle
an ACPI registered ARA.
ACPI is really not my field. Try asking the I2C ACPI maintainers or
Benjamin Tissoires who did work on SMBus
interrupts recently.
Hi Mika, Benjamin,
So we've lost most of the context in this thread, but the basic question
is how to handle smbus ARA support with ACPI.
https://patchwork.kernel.org/patch/10030309/
Has the proposal made in this driver which is really not terribly nice
(as it registers the ARA device by messing with the address and registering
a second device).
As I understood it the ARA device registration should be handled by the
i2c master, but there are very few examples.
Phil pointed out that equivalent OF support recently got taken from him..
https://www.spinics.net/lists/devicetree/msg191947.html
https://www.spinics.net/lists/linux-i2c/msg31173.html
Any thoughts on the right way to do this?
There does not seem to be any way in ACPI to tell which "connection" is
used to describe ARA so that part currently is something each driver
needs to handle as they know the device the best. I don't think we have
any means to handle it in generic way in I2C core except to provide some
helpers that work on top of i2c_setup_smbus_alert() but understand ACPI
resources. Say provide function like this:
int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);
Which then extracts automatically I2cSerialBus connection from "index"
and calls i2c_setup_smbus_alert() accordingly.
In the long run we could introduce _DSD property that can be used to
name the connection in the same way DT does;
Name (_CRS, ResourceTemplate () {
I2cSerialBus () { ... } // ARA
I2cSerialBus () { ... } // normal device address
})
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () {"smbus_alert", 0} // Where 0 means the first
I2cSerialBus
...
}
})
I wonder if it's worth involving the smbus_alert driver in this case.
The cm3218 driver doesn't appear to be using the alert callback in strcut
i2c_driver.
True - though it really should be..
So the smbus_alert driver is not going to notifiy the cm3218 driver.
Are there more than one alert/ara capable devices on the bus?
I'm not keen on taking a work around for one board on the basis that
it only has this one device on the bus - we will probably get a different
one down the line where this isn't true - then we end up ripping up what
has been done so far and starting again.
I don't mind having some ACPI matching code in the driver but it needs
to use the ARA infrastructure to actually handle things...
I'd agree, I only suggested the approach as the driver didn't seem to be using
the alert
callback. Without using that callback there seems little point using it IMO.
As you say the better approach is to have some generic ACPI code for the alert.
If we then get nice generic ACPI code via some other means in the future
we can move the driver over to that.
Sometimes I wonder if some people just write ACPI tables with no though
to generalization at all, or that the code running might be shared across
different machines. (sometimes that assumption is valid, but not that
often... oh well - usual case of if we all work together everyone wins
but not worth one company trying to do things right...)
Perhaps a workaround in this case is if that acpi entry is defined the cm3218
driver
handles that ara request directly to clear the interrupt.
Jonathan
--
Regards
Phil Reid
Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
On Mon, 20 Nov 2017 12:57:56 +0200
Mika Westerberg wrote:
> +Jarkko
>
> On Sun, Nov 19, 2017 at 04:35:51PM +, Jonathan Cameron wrote:
> > On Thu, 2 Nov 2017 16:04:07 +0100
> > Wolfram Sang wrote:
> >
> > > On Thu, Nov 02, 2017 at 02:35:50PM +, Jonathan Cameron wrote:
> > > > On Fri, 27 Oct 2017 18:27:02 +0200
> > > > Marc CAPDEVILLE wrote:
> > > >
> > > > > On asus T100, Capella cm3218 chip is implemented as ambiant light
> > > > > sensor. This chip expose an smbus ARA protocol device on standard
> > > > > address 0x0c. The chip is not functional before all alerts are
> > > > > acknowledged.
> > > > > On asus T100, this device is enumerated on ACPI bus and the
> > > > > description give tow I2C connection. The first is the connection to
> > > > > the ARA device and the second gives the real address of the device.
> > > > >
> > > > > So, on device probe,If the i2c address is the ARA address and the
> > > > > device is enumerated via acpi, we lookup for the real address in
> > > > > the ACPI resource list and change it in the client structure.
> > > > > if an interrupt resource is given, and only for cm3218 chip,
> > > > > we declare an smbus_alert device.
> > > > >
> > > > > Signed-off-by: Marc CAPDEVILLE
> > > >
> > > > Wolfram - this needs input from you on how to neatly handle
> > > > an ACPI registered ARA.
> > >
> > > ACPI is really not my field. Try asking the I2C ACPI maintainers or
> > > Benjamin Tissoires who did work on SMBus
> > > interrupts recently.
> > >
> > Hi Mika, Benjamin,
> >
> > So we've lost most of the context in this thread, but the basic question
> > is how to handle smbus ARA support with ACPI.
> >
> > https://patchwork.kernel.org/patch/10030309/
> >
> > Has the proposal made in this driver which is really not terribly nice
> > (as it registers the ARA device by messing with the address and registering
> > a second device).
> >
> > As I understood it the ARA device registration should be handled by the
> > i2c master, but there are very few examples.
> >
> > Phil pointed out that equivalent OF support recently got taken from him..
> > https://www.spinics.net/lists/devicetree/msg191947.html
> > https://www.spinics.net/lists/linux-i2c/msg31173.html
> >
> > Any thoughts on the right way to do this?
>
> There does not seem to be any way in ACPI to tell which "connection" is
> used to describe ARA so that part currently is something each driver
> needs to handle as they know the device the best. I don't think we have
> any means to handle it in generic way in I2C core except to provide some
> helpers that work on top of i2c_setup_smbus_alert() but understand ACPI
> resources. Say provide function like this:
>
> int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);
>
> Which then extracts automatically I2cSerialBus connection from "index"
> and calls i2c_setup_smbus_alert() accordingly.
>
> In the long run we could introduce _DSD property that can be used to
> name the connection in the same way DT does;
>
> Name (_CRS, ResourceTemplate () {
> I2cSerialBus () { ... } // ARA
> I2cSerialBus () { ... } // normal device address
> })
>
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"smbus_alert", 0} // Where 0 means the first
> I2cSerialBus
> ...
> }
> })
>
> But it does not help the existing systems.
I'm curious - how would we go about promoting this piece of common sense?
Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
On Tue, 21 Nov 2017 09:22:16 +0800
Phil Reid wrote:
> On 20/11/2017 18:57, Mika Westerberg wrote:
> > +Jarkko
> >
> > On Sun, Nov 19, 2017 at 04:35:51PM +, Jonathan Cameron wrote:
> >> On Thu, 2 Nov 2017 16:04:07 +0100
> >> Wolfram Sang wrote:
> >>
> >>> On Thu, Nov 02, 2017 at 02:35:50PM +, Jonathan Cameron wrote:
> On Fri, 27 Oct 2017 18:27:02 +0200
> Marc CAPDEVILLE wrote:
>
> > On asus T100, Capella cm3218 chip is implemented as ambiant light
> > sensor. This chip expose an smbus ARA protocol device on standard
> > address 0x0c. The chip is not functional before all alerts are
> > acknowledged.
> > On asus T100, this device is enumerated on ACPI bus and the
> > description give tow I2C connection. The first is the connection to
> > the ARA device and the second gives the real address of the device.
> >
> > So, on device probe,If the i2c address is the ARA address and the
> > device is enumerated via acpi, we lookup for the real address in
> > the ACPI resource list and change it in the client structure.
> > if an interrupt resource is given, and only for cm3218 chip,
> > we declare an smbus_alert device.
> >
> > Signed-off-by: Marc CAPDEVILLE
>
> Wolfram - this needs input from you on how to neatly handle
> an ACPI registered ARA.
> >>>
> >>> ACPI is really not my field. Try asking the I2C ACPI maintainers or
> >>> Benjamin Tissoires who did work on SMBus
> >>> interrupts recently.
> >>>
> >> Hi Mika, Benjamin,
> >>
> >> So we've lost most of the context in this thread, but the basic question
> >> is how to handle smbus ARA support with ACPI.
> >>
> >> https://patchwork.kernel.org/patch/10030309/
> >>
> >> Has the proposal made in this driver which is really not terribly nice
> >> (as it registers the ARA device by messing with the address and registering
> >> a second device).
> >>
> >> As I understood it the ARA device registration should be handled by the
> >> i2c master, but there are very few examples.
> >>
> >> Phil pointed out that equivalent OF support recently got taken from him..
> >> https://www.spinics.net/lists/devicetree/msg191947.html
> >> https://www.spinics.net/lists/linux-i2c/msg31173.html
> >>
> >> Any thoughts on the right way to do this?
> >
> > There does not seem to be any way in ACPI to tell which "connection" is
> > used to describe ARA so that part currently is something each driver
> > needs to handle as they know the device the best. I don't think we have
> > any means to handle it in generic way in I2C core except to provide some
> > helpers that work on top of i2c_setup_smbus_alert() but understand ACPI
> > resources. Say provide function like this:
> >
> >int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);
> >
> > Which then extracts automatically I2cSerialBus connection from "index"
> > and calls i2c_setup_smbus_alert() accordingly.
> >
> > In the long run we could introduce _DSD property that can be used to
> > name the connection in the same way DT does;
> >
> > Name (_CRS, ResourceTemplate () {
> > I2cSerialBus () { ... } // ARA
> > I2cSerialBus () { ... } // normal device address
> > })
> >
> > Name (_DSD, Package () {
> > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > Package () {
> > Package () {"smbus_alert", 0} // Where 0 means the first
> > I2cSerialBus
> > ...
> > }
> > })
> >
>
> I wonder if it's worth involving the smbus_alert driver in this case.
> The cm3218 driver doesn't appear to be using the alert callback in strcut
> i2c_driver.
True - though it really should be..
> So the smbus_alert driver is not going to notifiy the cm3218 driver.
> Are there more than one alert/ara capable devices on the bus?
I'm not keen on taking a work around for one board on the basis that
it only has this one device on the bus - we will probably get a different
one down the line where this isn't true - then we end up ripping up what
has been done so far and starting again.
I don't mind having some ACPI matching code in the driver but it needs
to use the ARA infrastructure to actually handle things...
If we then get nice generic ACPI code via some other means in the future
we can move the driver over to that.
Sometimes I wonder if some people just write ACPI tables with no though
to generalization at all, or that the code running might be shared across
different machines. (sometimes that assumption is valid, but not that
often... oh well - usual case of if we all work together everyone wins
but not worth one company trying to do things right...)
> Perhaps a workaround in this case is if that acpi entry is defined the cm3218
> driver
> handles that ara request directly to clear the interrupt.
>
Jonathan
Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
On 20/11/2017 18:57, Mika Westerberg wrote:
+Jarkko
On Sun, Nov 19, 2017 at 04:35:51PM +, Jonathan Cameron wrote:
On Thu, 2 Nov 2017 16:04:07 +0100
Wolfram Sang wrote:
On Thu, Nov 02, 2017 at 02:35:50PM +, Jonathan Cameron wrote:
On Fri, 27 Oct 2017 18:27:02 +0200
Marc CAPDEVILLE wrote:
On asus T100, Capella cm3218 chip is implemented as ambiant light
sensor. This chip expose an smbus ARA protocol device on standard
address 0x0c. The chip is not functional before all alerts are
acknowledged.
On asus T100, this device is enumerated on ACPI bus and the
description give tow I2C connection. The first is the connection to
the ARA device and the second gives the real address of the device.
So, on device probe,If the i2c address is the ARA address and the
device is enumerated via acpi, we lookup for the real address in
the ACPI resource list and change it in the client structure.
if an interrupt resource is given, and only for cm3218 chip,
we declare an smbus_alert device.
Signed-off-by: Marc CAPDEVILLE
Wolfram - this needs input from you on how to neatly handle
an ACPI registered ARA.
ACPI is really not my field. Try asking the I2C ACPI maintainers or
Benjamin Tissoires who did work on SMBus
interrupts recently.
Hi Mika, Benjamin,
So we've lost most of the context in this thread, but the basic question
is how to handle smbus ARA support with ACPI.
https://patchwork.kernel.org/patch/10030309/
Has the proposal made in this driver which is really not terribly nice
(as it registers the ARA device by messing with the address and registering
a second device).
As I understood it the ARA device registration should be handled by the
i2c master, but there are very few examples.
Phil pointed out that equivalent OF support recently got taken from him..
https://www.spinics.net/lists/devicetree/msg191947.html
https://www.spinics.net/lists/linux-i2c/msg31173.html
Any thoughts on the right way to do this?
There does not seem to be any way in ACPI to tell which "connection" is
used to describe ARA so that part currently is something each driver
needs to handle as they know the device the best. I don't think we have
any means to handle it in generic way in I2C core except to provide some
helpers that work on top of i2c_setup_smbus_alert() but understand ACPI
resources. Say provide function like this:
int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);
Which then extracts automatically I2cSerialBus connection from "index"
and calls i2c_setup_smbus_alert() accordingly.
In the long run we could introduce _DSD property that can be used to
name the connection in the same way DT does;
Name (_CRS, ResourceTemplate () {
I2cSerialBus () { ... } // ARA
I2cSerialBus () { ... } // normal device address
})
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () {"smbus_alert", 0} // Where 0 means the first
I2cSerialBus
...
}
})
I wonder if it's worth involving the smbus_alert driver in this case.
The cm3218 driver doesn't appear to be using the alert callback in strcut
i2c_driver.
So the smbus_alert driver is not going to notifiy the cm3218 driver.
Are there more than one alert/ara capable devices on the bus?
Perhaps a workaround in this case is if that acpi entry is defined the cm3218
driver
handles that ara request directly to clear the interrupt.
--
Regards
Phil Reid
Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
+Jarkko
On Sun, Nov 19, 2017 at 04:35:51PM +, Jonathan Cameron wrote:
> On Thu, 2 Nov 2017 16:04:07 +0100
> Wolfram Sang wrote:
>
> > On Thu, Nov 02, 2017 at 02:35:50PM +, Jonathan Cameron wrote:
> > > On Fri, 27 Oct 2017 18:27:02 +0200
> > > Marc CAPDEVILLE wrote:
> > >
> > > > On asus T100, Capella cm3218 chip is implemented as ambiant light
> > > > sensor. This chip expose an smbus ARA protocol device on standard
> > > > address 0x0c. The chip is not functional before all alerts are
> > > > acknowledged.
> > > > On asus T100, this device is enumerated on ACPI bus and the
> > > > description give tow I2C connection. The first is the connection to
> > > > the ARA device and the second gives the real address of the device.
> > > >
> > > > So, on device probe,If the i2c address is the ARA address and the
> > > > device is enumerated via acpi, we lookup for the real address in
> > > > the ACPI resource list and change it in the client structure.
> > > > if an interrupt resource is given, and only for cm3218 chip,
> > > > we declare an smbus_alert device.
> > > >
> > > > Signed-off-by: Marc CAPDEVILLE
> > >
> > > Wolfram - this needs input from you on how to neatly handle
> > > an ACPI registered ARA.
> >
> > ACPI is really not my field. Try asking the I2C ACPI maintainers or
> > Benjamin Tissoires who did work on SMBus
> > interrupts recently.
> >
> Hi Mika, Benjamin,
>
> So we've lost most of the context in this thread, but the basic question
> is how to handle smbus ARA support with ACPI.
>
> https://patchwork.kernel.org/patch/10030309/
>
> Has the proposal made in this driver which is really not terribly nice
> (as it registers the ARA device by messing with the address and registering
> a second device).
>
> As I understood it the ARA device registration should be handled by the
> i2c master, but there are very few examples.
>
> Phil pointed out that equivalent OF support recently got taken from him..
> https://www.spinics.net/lists/devicetree/msg191947.html
> https://www.spinics.net/lists/linux-i2c/msg31173.html
>
> Any thoughts on the right way to do this?
There does not seem to be any way in ACPI to tell which "connection" is
used to describe ARA so that part currently is something each driver
needs to handle as they know the device the best. I don't think we have
any means to handle it in generic way in I2C core except to provide some
helpers that work on top of i2c_setup_smbus_alert() but understand ACPI
resources. Say provide function like this:
int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);
Which then extracts automatically I2cSerialBus connection from "index"
and calls i2c_setup_smbus_alert() accordingly.
In the long run we could introduce _DSD property that can be used to
name the connection in the same way DT does;
Name (_CRS, ResourceTemplate () {
I2cSerialBus () { ... } // ARA
I2cSerialBus () { ... } // normal device address
})
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () {"smbus_alert", 0} // Where 0 means the first
I2cSerialBus
...
}
})
But it does not help the existing systems.
Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
On Thu, 2 Nov 2017 16:04:07 +0100 Wolfram Sang wrote: > On Thu, Nov 02, 2017 at 02:35:50PM +, Jonathan Cameron wrote: > > On Fri, 27 Oct 2017 18:27:02 +0200 > > Marc CAPDEVILLE wrote: > > > > > On asus T100, Capella cm3218 chip is implemented as ambiant light > > > sensor. This chip expose an smbus ARA protocol device on standard > > > address 0x0c. The chip is not functional before all alerts are > > > acknowledged. > > > On asus T100, this device is enumerated on ACPI bus and the > > > description give tow I2C connection. The first is the connection to > > > the ARA device and the second gives the real address of the device. > > > > > > So, on device probe,If the i2c address is the ARA address and the > > > device is enumerated via acpi, we lookup for the real address in > > > the ACPI resource list and change it in the client structure. > > > if an interrupt resource is given, and only for cm3218 chip, > > > we declare an smbus_alert device. > > > > > > Signed-off-by: Marc CAPDEVILLE > > > > Wolfram - this needs input from you on how to neatly handle > > an ACPI registered ARA. > > ACPI is really not my field. Try asking the I2C ACPI maintainers or > Benjamin Tissoires who did work on SMBus > interrupts recently. > Hi Mika, Benjamin, So we've lost most of the context in this thread, but the basic question is how to handle smbus ARA support with ACPI. https://patchwork.kernel.org/patch/10030309/ Has the proposal made in this driver which is really not terribly nice (as it registers the ARA device by messing with the address and registering a second device). As I understood it the ARA device registration should be handled by the i2c master, but there are very few examples. Phil pointed out that equivalent OF support recently got taken from him.. https://www.spinics.net/lists/devicetree/msg191947.html https://www.spinics.net/lists/linux-i2c/msg31173.html Any thoughts on the right way to do this? Jonathan
Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
On 2/11/2017 22:49, Srinivas Pandruvada wrote: On Thu, 2017-11-02 at 14:35 +, Jonathan Cameron wrote: On Fri, 27 Oct 2017 18:27:02 +0200 Marc CAPDEVILLE wrote: On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged. On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device. So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device. Signed-off-by: Marc CAPDEVILLE Wolfram - this needs input from you on how to neatly handle an ACPI registered ARA. There was another RFC from Alan cox https://patchwork.ozlabs.org/patch/381773/ Wolfram just merged this from me: [PATCH v11 00/10] Add sbs-manager with smbalert support https://www.spinics.net/lists/devicetree/msg191947.html Cleans up the smbus_alert driver a bit. note: alert_edge_triggered was removed. And for OF systems core creates the ara device. -- Regards Phil Reid
Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
On Thu, Nov 02, 2017 at 02:35:50PM +, Jonathan Cameron wrote: > On Fri, 27 Oct 2017 18:27:02 +0200 > Marc CAPDEVILLE wrote: > > > On asus T100, Capella cm3218 chip is implemented as ambiant light > > sensor. This chip expose an smbus ARA protocol device on standard > > address 0x0c. The chip is not functional before all alerts are > > acknowledged. > > On asus T100, this device is enumerated on ACPI bus and the > > description give tow I2C connection. The first is the connection to > > the ARA device and the second gives the real address of the device. > > > > So, on device probe,If the i2c address is the ARA address and the > > device is enumerated via acpi, we lookup for the real address in > > the ACPI resource list and change it in the client structure. > > if an interrupt resource is given, and only for cm3218 chip, > > we declare an smbus_alert device. > > > > Signed-off-by: Marc CAPDEVILLE > > Wolfram - this needs input from you on how to neatly handle > an ACPI registered ARA. ACPI is really not my field. Try asking the I2C ACPI maintainers or Benjamin Tissoires who did work on SMBus interrupts recently. signature.asc Description: PGP signature
Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
On Thu, 2017-11-02 at 14:35 +, Jonathan Cameron wrote:
> On Fri, 27 Oct 2017 18:27:02 +0200
> Marc CAPDEVILLE wrote:
>
> >
> > On asus T100, Capella cm3218 chip is implemented as ambiant light
> > sensor. This chip expose an smbus ARA protocol device on standard
> > address 0x0c. The chip is not functional before all alerts are
> > acknowledged.
> > On asus T100, this device is enumerated on ACPI bus and the
> > description give tow I2C connection. The first is the connection to
> > the ARA device and the second gives the real address of the device.
> >
> > So, on device probe,If the i2c address is the ARA address and the
> > device is enumerated via acpi, we lookup for the real address in
> > the ACPI resource list and change it in the client structure.
> > if an interrupt resource is given, and only for cm3218 chip,
> > we declare an smbus_alert device.
> >
> > Signed-off-by: Marc CAPDEVILLE
> Wolfram - this needs input from you on how to neatly handle
> an ACPI registered ARA.
There was another RFC from Alan cox
https://patchwork.ozlabs.org/patch/381773/
Thanks,
Srinivas
>
> Thanks,
>
> Jonathan
> >
> > ---
> > v4 :
> > - rework acpi i2c adress lookup due to bad commit being sent.
> >
> > v3 :
> > - rework acpi i2c adress lookup
> > - comment style cleanup
> > - add prefix cm32181_to constantes and macros
> >
> > v2 :
> > - cm3218 support always build
> > - Cleanup of unneeded #if statement
> > - Beter identifying chip in platform device, acpi and of_device
> >
> > drivers/iio/light/cm32181.c | 202
> > 1 file changed, 185
> > insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/iio/light/cm32181.c
> > b/drivers/iio/light/cm32181.c
> > index d6fd0dace74f..f1a7495e851b 100644
> > --- a/drivers/iio/light/cm32181.c
> > +++ b/drivers/iio/light/cm32181.c
> > @@ -18,8 +18,12 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> > +#include
> > +#include
> >
> > -/* Registers Address */
> > +/* Registers Addresses */
> > #define CM32181_REG_ADDR_CMD 0x00
> > #define CM32181_REG_ADDR_ALS 0x04
> > #define CM32181_REG_ADDR_STATUS0x06
> > @@ -45,18 +49,25 @@
> > #define CM32181_MLUX_PER_BIT_BASE_IT 80 /* Based
> > on IT=800ms */ #define CM32181_CALIBSCALE_DEFAULT 100
> > 0
> > #define CM32181_CALIBSCALE_RESOLUTION 1000
> > -#define MLUX_PER_LUX 1000
> > +#define CM32181_MLUX_PER_LUX 1000
> > +
> > +#define CM32181_ID 0x81
> > +#define CM3218_ID 0x18
> > +
> > +#define CM3218_ARA_ADDR0x0c
> >
> > static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
> > CM32181_REG_ADDR_CMD,
> > };
> >
> > -static const int als_it_bits[] = {12, 8, 0, 1, 2, 3};
> > -static const int als_it_value[] = {25000, 5, 10, 20,
> > 40, +static const int cm32181_als_it_bits[] = {12, 8, 0, 1, 2,
> > 3};
> > +static const int cm32181_als_it_value[] = {25000, 5, 10,
> > 20, 40, 80};
> >
> > struct cm32181_chip {
> > struct i2c_client *client;
> > + int chip_id;
> > + struct i2c_client *ara;
> > struct mutex lock;
> > u16 conf_regs[CM32181_CONF_REG_NUM];
> > int calibscale;
> > @@ -81,7 +92,7 @@ static int cm32181_reg_init(struct cm32181_chip
> > *cm32181) return ret;
> >
> > /* check device ID */
> > - if ((ret & 0xFF) != 0x81)
> > + if ((ret & 0xFF) != cm32181->chip_id)
> > return -ENODEV;
> >
> > /* Default Values */
> > @@ -103,7 +114,7 @@ static int cm32181_reg_init(struct cm32181_chip
> > *cm32181) /**
> > * cm32181_read_als_it() - Get sensor integration time (ms)
> > * @cm32181: pointer of struct cm32181
> > - * @val2: pointer of int to load the als_it value.
> > + * @val2: pointer of int to load the cm32181_als_it value.
> > *
> > * Report the current integartion time by millisecond.
> > *
> > @@ -117,9 +128,9 @@ static int cm32181_read_als_it(struct
> > cm32181_chip *cm32181, int *val2) als_it =
> > cm32181->conf_regs[CM32181_REG_ADDR_CMD]; als_it &=
> > CM32181_CMD_ALS_IT_MASK; als_it >>= CM32181_CMD_ALS_IT_SHIFT;
> > - for (i = 0; i < ARRAY_SIZE(als_it_bits); i++) {
> > - if (als_it == als_it_bits[i]) {
> > - *val2 = als_it_value[i];
> > + for (i = 0; i < ARRAY_SIZE(cm32181_als_it_bits); i++) {
> > + if (als_it == cm32181_als_it_bits[i]) {
> > + *val2 = cm32181_als_it_value[i];
> > return IIO_VAL_INT_PLUS_MICRO;
> > }
> > }
> > @@ -142,14 +153,14 @@ static int cm32181_write_als_it(struct
> > cm32181_chip *cm32181, int val) u16 als_it;
> > int ret, i, n;
> >
> > - n = ARRAY_SIZE(als_it_value);
> > + n = ARRAY_SIZE(cm32181_als_it_value);
> > for (i = 0; i < n; i++)
> > - if (val <= als_it_value[i])
> > + if (val
Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
On Fri, 27 Oct 2017 18:27:02 +0200
Marc CAPDEVILLE wrote:
> On asus T100, Capella cm3218 chip is implemented as ambiant light
> sensor. This chip expose an smbus ARA protocol device on standard
> address 0x0c. The chip is not functional before all alerts are
> acknowledged.
> On asus T100, this device is enumerated on ACPI bus and the
> description give tow I2C connection. The first is the connection to
> the ARA device and the second gives the real address of the device.
>
> So, on device probe,If the i2c address is the ARA address and the
> device is enumerated via acpi, we lookup for the real address in
> the ACPI resource list and change it in the client structure.
> if an interrupt resource is given, and only for cm3218 chip,
> we declare an smbus_alert device.
>
> Signed-off-by: Marc CAPDEVILLE
Wolfram - this needs input from you on how to neatly handle
an ACPI registered ARA.
Thanks,
Jonathan
> ---
> v4 :
>- rework acpi i2c adress lookup due to bad commit being sent.
>
> v3 :
>- rework acpi i2c adress lookup
>- comment style cleanup
>- add prefix cm32181_to constantes and macros
>
> v2 :
>- cm3218 support always build
>- Cleanup of unneeded #if statement
>- Beter identifying chip in platform device, acpi and of_device
>
> drivers/iio/light/cm32181.c | 202
> 1 file changed, 185
> insertions(+), 17 deletions(-)
>
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index d6fd0dace74f..f1a7495e851b 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -18,8 +18,12 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
> +#include
>
> -/* Registers Address */
> +/* Registers Addresses */
> #define CM32181_REG_ADDR_CMD 0x00
> #define CM32181_REG_ADDR_ALS 0x04
> #define CM32181_REG_ADDR_STATUS 0x06
> @@ -45,18 +49,25 @@
> #define CM32181_MLUX_PER_BIT_BASE_IT 80 /* Based
> on IT=800ms */ #defineCM32181_CALIBSCALE_DEFAULT 1000
> #define CM32181_CALIBSCALE_RESOLUTION1000
> -#define MLUX_PER_LUX 1000
> +#define CM32181_MLUX_PER_LUX 1000
> +
> +#define CM32181_ID 0x81
> +#define CM3218_ID0x18
> +
> +#define CM3218_ARA_ADDR 0x0c
>
> static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
> CM32181_REG_ADDR_CMD,
> };
>
> -static const int als_it_bits[] = {12, 8, 0, 1, 2, 3};
> -static const int als_it_value[] = {25000, 5, 10, 20,
> 40, +static const int cm32181_als_it_bits[] = {12, 8, 0, 1, 2, 3};
> +static const int cm32181_als_it_value[] = {25000, 5, 10,
> 20, 40, 80};
>
> struct cm32181_chip {
> struct i2c_client *client;
> + int chip_id;
> + struct i2c_client *ara;
> struct mutex lock;
> u16 conf_regs[CM32181_CONF_REG_NUM];
> int calibscale;
> @@ -81,7 +92,7 @@ static int cm32181_reg_init(struct cm32181_chip
> *cm32181) return ret;
>
> /* check device ID */
> - if ((ret & 0xFF) != 0x81)
> + if ((ret & 0xFF) != cm32181->chip_id)
> return -ENODEV;
>
> /* Default Values */
> @@ -103,7 +114,7 @@ static int cm32181_reg_init(struct cm32181_chip
> *cm32181) /**
> * cm32181_read_als_it() - Get sensor integration time (ms)
> * @cm32181:pointer of struct cm32181
> - * @val2: pointer of int to load the als_it value.
> + * @val2: pointer of int to load the cm32181_als_it value.
> *
> * Report the current integartion time by millisecond.
> *
> @@ -117,9 +128,9 @@ static int cm32181_read_als_it(struct
> cm32181_chip *cm32181, int *val2) als_it =
> cm32181->conf_regs[CM32181_REG_ADDR_CMD]; als_it &=
> CM32181_CMD_ALS_IT_MASK; als_it >>= CM32181_CMD_ALS_IT_SHIFT;
> - for (i = 0; i < ARRAY_SIZE(als_it_bits); i++) {
> - if (als_it == als_it_bits[i]) {
> - *val2 = als_it_value[i];
> + for (i = 0; i < ARRAY_SIZE(cm32181_als_it_bits); i++) {
> + if (als_it == cm32181_als_it_bits[i]) {
> + *val2 = cm32181_als_it_value[i];
> return IIO_VAL_INT_PLUS_MICRO;
> }
> }
> @@ -142,14 +153,14 @@ static int cm32181_write_als_it(struct
> cm32181_chip *cm32181, int val) u16 als_it;
> int ret, i, n;
>
> - n = ARRAY_SIZE(als_it_value);
> + n = ARRAY_SIZE(cm32181_als_it_value);
> for (i = 0; i < n; i++)
> - if (val <= als_it_value[i])
> + if (val <= cm32181_als_it_value[i])
> break;
> if (i >= n)
> i = n - 1;
>
> - als_it = als_it_bits[i];
> + als_it = cm32181_als_it_bits[i];
> als_it <<= CM32181_CMD_ALS_IT_SHIFT;
>
> mutex_lock(&cm32181->lock);
> @@ -195,7 +206,7 @@ static int cm32181_get_lux(struct cm32181_chip
> *cm32181) lux *= ret;
> lux *= cm32181->calibsca
Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support
> On asus T100, Capella cm3218 chip is implemented as ambiant light
> sensor. This chip expose an smbus ARA protocol device on standard
> address 0x0c. The chip is not functional before all alerts are
> acknowledged.
> On asus T100, this device is enumerated on ACPI bus and the description
> give tow I2C connection. The first is the connection to the ARA device
> and the second gives the real address of the device.
>
> So, on device probe,If the i2c address is the ARA address and the
> device is enumerated via acpi, we lookup for the real address in
> the ACPI resource list and change it in the client structure.
> if an interrupt resource is given, and only for cm3218 chip,
> we declare an smbus_alert device.
I think the unrelated cleanups should have been put in a separate patch (I
feel sorry for guiding you in the wrong direction and not pointing this
out more clearly)
more very minor comments below
> Signed-off-by: Marc CAPDEVILLE
> ---
> v4 :
>- rework acpi i2c adress lookup due to bad commit being sent.
>
> v3 :
>- rework acpi i2c adress lookup
>- comment style cleanup
>- add prefix cm32181_to constantes and macros
>
> v2 :
>- cm3218 support always build
>- Cleanup of unneeded #if statement
>- Beter identifying chip in platform device, acpi and of_device
>
> drivers/iio/light/cm32181.c | 202
>
> 1 file changed, 185 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index d6fd0dace74f..f1a7495e851b 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -18,8 +18,12 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
> +#include
>
> -/* Registers Address */
> +/* Registers Addresses */
Register Addresses
> #define CM32181_REG_ADDR_CMD 0x00
> #define CM32181_REG_ADDR_ALS 0x04
> #define CM32181_REG_ADDR_STATUS 0x06
> @@ -45,18 +49,25 @@
> #define CM32181_MLUX_PER_BIT_BASE_IT 80 /* Based on IT=800ms */
> #define CM32181_CALIBSCALE_DEFAULT 1000
> #define CM32181_CALIBSCALE_RESOLUTION1000
> -#define MLUX_PER_LUX 1000
> +#define CM32181_MLUX_PER_LUX 1000
> +
> +#define CM32181_ID 0x81
> +#define CM3218_ID0x18
> +
> +#define CM3218_ARA_ADDR 0x0c
>
> static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
> CM32181_REG_ADDR_CMD,
> };
>
> -static const int als_it_bits[] = {12, 8, 0, 1, 2, 3};
> -static const int als_it_value[] = {25000, 5, 10, 20, 40,
> +static const int cm32181_als_it_bits[] = {12, 8, 0, 1, 2, 3};
> +static const int cm32181_als_it_value[] = {25000, 5, 10, 20,
> 40,
> 80};
>
> struct cm32181_chip {
> struct i2c_client *client;
> + int chip_id;
> + struct i2c_client *ara;
> struct mutex lock;
> u16 conf_regs[CM32181_CONF_REG_NUM];
> int calibscale;
> @@ -81,7 +92,7 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
> return ret;
>
> /* check device ID */
> - if ((ret & 0xFF) != 0x81)
> + if ((ret & 0xFF) != cm32181->chip_id)
> return -ENODEV;
>
> /* Default Values */
> @@ -103,7 +114,7 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
> /**
> * cm32181_read_als_it() - Get sensor integration time (ms)
> * @cm32181:pointer of struct cm32181
> - * @val2: pointer of int to load the als_it value.
> + * @val2: pointer of int to load the cm32181_als_it value.
> *
> * Report the current integartion time by millisecond.
> *
> @@ -117,9 +128,9 @@ static int cm32181_read_als_it(struct cm32181_chip
> *cm32181, int *val2)
> als_it = cm32181->conf_regs[CM32181_REG_ADDR_CMD];
> als_it &= CM32181_CMD_ALS_IT_MASK;
> als_it >>= CM32181_CMD_ALS_IT_SHIFT;
> - for (i = 0; i < ARRAY_SIZE(als_it_bits); i++) {
> - if (als_it == als_it_bits[i]) {
> - *val2 = als_it_value[i];
> + for (i = 0; i < ARRAY_SIZE(cm32181_als_it_bits); i++) {
> + if (als_it == cm32181_als_it_bits[i]) {
> + *val2 = cm32181_als_it_value[i];
> return IIO_VAL_INT_PLUS_MICRO;
> }
> }
> @@ -142,14 +153,14 @@ static int cm32181_write_als_it(struct cm32181_chip
> *cm32181, int val)
> u16 als_it;
> int ret, i, n;
>
> - n = ARRAY_SIZE(als_it_value);
> + n = ARRAY_SIZE(cm32181_als_it_value);
> for (i = 0; i < n; i++)
> - if (val <= als_it_value[i])
> + if (val <= cm32181_als_it_value[i])
> break;
> if (i >= n)
> i = n - 1;
>
> - als_it = als_it_bits[i];
> + als_it = cm32181_als_it_bits[i];
> als_it <<= CM32181_CMD_ALS_IT_SHIFT;
>
> mutex_lock(&cm32181->lock);
> @@ -195,7
