Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI

Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI

[Lars-Peter Clausen]

Added I2C to Cc.

On 12/15/2014 10:19 PM, Srinivas Pandruvada wrote:
> Using i2c auto detect feature and auto device creation feature,
> enumerate ak8975 device, by checking their presence.
> This is needed because when this device sits behind an i2c mux, there
> is no way to define i2c mux in ACPI. This will enable ak8975 on
> windows based tablets/laptops running Linux when connected via a mux.
> Since DT model already can define an i2c mux and devices connected to
> it, this feature is only enabled for ACPI.
>

This is quite a bit of a hack. Did they decide to not include the device in 
the ACPI description at all or is there a special id for INV6050+AK8975?

How does Windows decide whether there is a device or not?

> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>   drivers/iio/magnetometer/ak8975.c | 35 +++++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
>
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 0d10a4b..c3455bd 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -820,6 +820,36 @@ static const struct i2c_device_id ak8975_id[] = {
>
>   MODULE_DEVICE_TABLE(i2c, ak8975_id);
>
> +#if defined(CONFIG_ACPI)
> +static int ak8975_detect(struct i2c_client *temp_client,
> +			 struct i2c_board_info *info)
> +{
> +	struct i2c_adapter *adapter;
> +	int i, j;
> +	int ret;
> +
> +	/* autodetect only when we are behind a mux */
> +	adapter = i2c_parent_is_i2c_adapter(temp_client->adapter);
> +	if (!adapter)
> +		return -ENODEV;
> +
> +	for (i = 0; i < AK_MAX_TYPE; ++i) {
> +		ret = ak8975_who_i_am(temp_client, i);
> +		if (ret >= 0) {
> +			for (j = 0; j < ARRAY_SIZE(ak8975_id) - 1; ++j) {
> +				if (i == (int)ak8975_id[j].driver_data) {
> +					strlcpy(info->type, ak8975_id[j].name,
> +						I2C_NAME_SIZE);
> +					return 0;
> +				}
> +			}
> +		}
> +	}
> +
> +	return -ENODEV;
> +}
> +#endif
> +
>   static const struct of_device_id ak8975_of_match[] = {
>   	{ .compatible = "asahi-kasei,ak8975", },
>   	{ .compatible = "ak8975", },
> @@ -841,6 +871,11 @@ static struct i2c_driver ak8975_driver = {
>   	},
>   	.probe		= ak8975_probe,
>   	.id_table	= ak8975_id,
> +#if defined(CONFIG_ACPI)
> +	.class		= I2C_CLASS_HWMON,
> +	.address_list	= I2C_ADDRS(0x0C, 0x0D, 0x0E, 0x0F),
> +	.detect		= ak8975_detect,

In contrast to the commit message this is always enabled if the kernel 
contains ACPI support, not if the device is a ACPI device.

> +#endif
>   };
>   module_i2c_driver(ak8975_driver);
>
>

Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI

[Srinivas Pandruvada]

On Thu, 2014-12-18 at 17:28 +0100, Lars-Peter Clausen wrote: 
> Added I2C to Cc.
> 
> On 12/15/2014 10:19 PM, Srinivas Pandruvada wrote:
> > Using i2c auto detect feature and auto device creation feature,
> > enumerate ak8975 device, by checking their presence.
> > This is needed because when this device sits behind an i2c mux, there
> > is no way to define i2c mux in ACPI. This will enable ak8975 on
> > windows based tablets/laptops running Linux when connected via a mux.
> > Since DT model already can define an i2c mux and devices connected to
> > it, this feature is only enabled for ACPI.
> >
> 
> This is quite a bit of a hack.
Why?
Auto detect is standard feature of i2c devices. This is using standard
auto detect feature provided by the framework. 
>  Did they decide to not include the device in 
> the ACPI description at all or is there a special id for INV6050+AK8975?
This device needs has one combined id. Windows has a singe driver
processing both as a combo device. 
> 
> How does Windows decide whether there is a device or not?
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> >   drivers/iio/magnetometer/ak8975.c | 35 +++++++++++++++++++++++++++++++++++
> >   1 file changed, 35 insertions(+)
> >
> > diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> > index 0d10a4b..c3455bd 100644
> > --- a/drivers/iio/magnetometer/ak8975.c
> > +++ b/drivers/iio/magnetometer/ak8975.c
> > @@ -820,6 +820,36 @@ static const struct i2c_device_id ak8975_id[] = {
> >
> >   MODULE_DEVICE_TABLE(i2c, ak8975_id);
> >
> > +#if defined(CONFIG_ACPI)
> > +static int ak8975_detect(struct i2c_client *temp_client,
> > +			 struct i2c_board_info *info)
> > +{
> > +	struct i2c_adapter *adapter;
> > +	int i, j;
> > +	int ret;
> > +
> > +	/* autodetect only when we are behind a mux */
> > +	adapter = i2c_parent_is_i2c_adapter(temp_client->adapter);
> > +	if (!adapter)
> > +		return -ENODEV;
> > +
> > +	for (i = 0; i < AK_MAX_TYPE; ++i) {
> > +		ret = ak8975_who_i_am(temp_client, i);
> > +		if (ret >= 0) {
> > +			for (j = 0; j < ARRAY_SIZE(ak8975_id) - 1; ++j) {
> > +				if (i == (int)ak8975_id[j].driver_data) {
> > +					strlcpy(info->type, ak8975_id[j].name,
> > +						I2C_NAME_SIZE);
> > +					return 0;
> > +				}
> > +			}
> > +		}
> > +	}
> > +
> > +	return -ENODEV;
> > +}
> > +#endif
> > +
> >   static const struct of_device_id ak8975_of_match[] = {
> >   	{ .compatible = "asahi-kasei,ak8975", },
> >   	{ .compatible = "ak8975", },
> > @@ -841,6 +871,11 @@ static struct i2c_driver ak8975_driver = {
> >   	},
> >   	.probe		= ak8975_probe,
> >   	.id_table	= ak8975_id,
> > +#if defined(CONFIG_ACPI)
> > +	.class		= I2C_CLASS_HWMON,
> > +	.address_list	= I2C_ADDRS(0x0C, 0x0D, 0x0E, 0x0F),
> > +	.detect		= ak8975_detect,
> 
> In contrast to the commit message this is always enabled if the kernel 
> contains ACPI support, not if the device is a ACPI device.
Yes. You don't enable ACPI if you don't need it. It will bring in lots
of builtin code if someone did that. 
> 
> > +#endif
> >   };
> >   module_i2c_driver(ak8975_driver);
> >
> >
> 

Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI

[Lars-Peter Clausen]

On 12/18/2014 05:52 PM, Srinivas Pandruvada wrote:
> On Thu, 2014-12-18 at 17:28 +0100, Lars-Peter Clausen wrote:
>> Added I2C to Cc.
>>
>> On 12/15/2014 10:19 PM, Srinivas Pandruvada wrote:
>>> Using i2c auto detect feature and auto device creation feature,
>>> enumerate ak8975 device, by checking their presence.
>>> This is needed because when this device sits behind an i2c mux, there
>>> is no way to define i2c mux in ACPI. This will enable ak8975 on
>>> windows based tablets/laptops running Linux when connected via a mux.
>>> Since DT model already can define an i2c mux and devices connected to
>>> it, this feature is only enabled for ACPI.
>>>
>>
>> This is quite a bit of a hack.
> Why?
> Auto detect is standard feature of i2c devices. This is using standard
> auto detect feature provided by the framework.

Auto detect is ugly, slow and unreliable, it's kind of like the last straw 
if nothing else works.

Autodetect will be executed for every adapter with every device that 
supports auto detection, so you want to keep the amount of devices that do 
auto detection to a minimum in order to avoid both false positives and 
reduce boot time.


>>   Did they decide to not include the device in
>> the ACPI description at all or is there a special id for INV6050+AK8975?
> This device needs has one combined id. Windows has a singe driver
> processing both as a combo device.

Ok, then use the combined id to instantiate INV6050+AK8975.

- Lars

Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI

[Srinivas Pandruvada]

On Thu, 2014-12-18 at 18:05 +0100, Lars-Peter Clausen wrote: 
> On 12/18/2014 05:52 PM, Srinivas Pandruvada wrote:
> > On Thu, 2014-12-18 at 17:28 +0100, Lars-Peter Clausen wrote:
> >> Added I2C to Cc.
> >>
> >> On 12/15/2014 10:19 PM, Srinivas Pandruvada wrote:
> >>> Using i2c auto detect feature and auto device creation feature,
> >>> enumerate ak8975 device, by checking their presence.
> >>> This is needed because when this device sits behind an i2c mux, there
> >>> is no way to define i2c mux in ACPI. This will enable ak8975 on
> >>> windows based tablets/laptops running Linux when connected via a mux.
> >>> Since DT model already can define an i2c mux and devices connected to
> >>> it, this feature is only enabled for ACPI.
> >>>
> >>
> >> This is quite a bit of a hack.
> > Why?
> > Auto detect is standard feature of i2c devices. This is using standard
> > auto detect feature provided by the framework.
> 
> Auto detect is ugly, slow and unreliable, it's kind of like the last straw 
> if nothing else works.
That is true here. You can't enumerate this device by ACPI. As discussed
before we created i2c mux in inv6050 so that we can use AK8975 in bypass
mode. I added some API to create i2c device on this mux, which Wolfram
didn't like. He wanted to enumerate using existing mechanisms. 
> 
> Autodetect will be executed for every adapter with every device that 
> supports auto detection, so you want to keep the amount of devices that do 
> auto detection to a minimum in order to avoid both false positives and 
> reduce boot time.
> 
> 
> >>   Did they decide to not include the device in
> >> the ACPI description at all or is there a special id for INV6050+AK8975?
> > This device needs has one combined id. Windows has a singe driver
> > processing both as a combo device.
> 
> Ok, then use the combined id to instantiate INV6050+AK8975.
No. The i2c address address is already consumed by INV6050 driver (which
is valid for INV6050 not AK8975).

> 
> - Lars
> 

Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI

[Lars-Peter Clausen]

On 12/18/2014 06:30 PM, Srinivas Pandruvada wrote:
> On Thu, 2014-12-18 at 18:05 +0100, Lars-Peter Clausen wrote:
>> On 12/18/2014 05:52 PM, Srinivas Pandruvada wrote:
>>> On Thu, 2014-12-18 at 17:28 +0100, Lars-Peter Clausen wrote:
>>>> Added I2C to Cc.
>>>>
>>>> On 12/15/2014 10:19 PM, Srinivas Pandruvada wrote:
>>>>> Using i2c auto detect feature and auto device creation feature,
>>>>> enumerate ak8975 device, by checking their presence.
>>>>> This is needed because when this device sits behind an i2c mux, there
>>>>> is no way to define i2c mux in ACPI. This will enable ak8975 on
>>>>> windows based tablets/laptops running Linux when connected via a mux.
>>>>> Since DT model already can define an i2c mux and devices connected to
>>>>> it, this feature is only enabled for ACPI.
>>>>>
>>>>
>>>> This is quite a bit of a hack.
>>> Why?
>>> Auto detect is standard feature of i2c devices. This is using standard
>>> auto detect feature provided by the framework.
>>
>> Auto detect is ugly, slow and unreliable, it's kind of like the last straw
>> if nothing else works.
> That is true here. You can't enumerate this device by ACPI. As discussed
> before we created i2c mux in inv6050 so that we can use AK8975 in bypass
> mode. I added some API to create i2c device on this mux, which Wolfram
> didn't like. He wanted to enumerate using existing mechanisms.

If there is only a single ACPI ID that says this is a INV6050 with a AK8975 
attached then the way to handle this is to have a driver that binds to that id 
and creates both devices.

- Lars

Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI

[Srinivas Pandruvada]

On Thu, 2014-12-18 at 18:54 +0100, Lars-Peter Clausen wrote: 
> On 12/18/2014 06:30 PM, Srinivas Pandruvada wrote:
> > On Thu, 2014-12-18 at 18:05 +0100, Lars-Peter Clausen wrote:
> >> On 12/18/2014 05:52 PM, Srinivas Pandruvada wrote:
> >>> On Thu, 2014-12-18 at 17:28 +0100, Lars-Peter Clausen wrote:
> >>>> Added I2C to Cc.
> >>>>
> >>>> On 12/15/2014 10:19 PM, Srinivas Pandruvada wrote:
> >>>>> Using i2c auto detect feature and auto device creation feature,
> >>>>> enumerate ak8975 device, by checking their presence.
> >>>>> This is needed because when this device sits behind an i2c mux, there
> >>>>> is no way to define i2c mux in ACPI. This will enable ak8975 on
> >>>>> windows based tablets/laptops running Linux when connected via a mux.
> >>>>> Since DT model already can define an i2c mux and devices connected to
> >>>>> it, this feature is only enabled for ACPI.
> >>>>>
> >>>>
> >>>> This is quite a bit of a hack.
> >>> Why?
> >>> Auto detect is standard feature of i2c devices. This is using standard
> >>> auto detect feature provided by the framework.
> >>
> >> Auto detect is ugly, slow and unreliable, it's kind of like the last straw
> >> if nothing else works.
> > That is true here. You can't enumerate this device by ACPI. As discussed
> > before we created i2c mux in inv6050 so that we can use AK8975 in bypass
> > mode. I added some API to create i2c device on this mux, which Wolfram
> > didn't like. He wanted to enumerate using existing mechanisms.
> 
> If there is only a single ACPI ID that says this is a INV6050 with a AK8975 
> attached then the way to handle this is to have a driver that binds to that id 
> and creates both devices.
Which address you will create device also adapter here is i2c mux
created by inv6050? You don't know i2c address of AK8975. To know AK8975
i2c address, even ugly hack to parse propriety vendor ACPI data, which
will change between to manufacturer.

> 
> - Lars
> 

Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI

[Jonathan Cameron]

On 18/12/14 18:00, Srinivas Pandruvada wrote:
> On Thu, 2014-12-18 at 18:54 +0100, Lars-Peter Clausen wrote: 
>> On 12/18/2014 06:30 PM, Srinivas Pandruvada wrote:
>>> On Thu, 2014-12-18 at 18:05 +0100, Lars-Peter Clausen wrote:
>>>> On 12/18/2014 05:52 PM, Srinivas Pandruvada wrote:
>>>>> On Thu, 2014-12-18 at 17:28 +0100, Lars-Peter Clausen wrote:
>>>>>> Added I2C to Cc.
>>>>>>
>>>>>> On 12/15/2014 10:19 PM, Srinivas Pandruvada wrote:
>>>>>>> Using i2c auto detect feature and auto device creation feature,
>>>>>>> enumerate ak8975 device, by checking their presence.
>>>>>>> This is needed because when this device sits behind an i2c mux, there
>>>>>>> is no way to define i2c mux in ACPI. This will enable ak8975 on
>>>>>>> windows based tablets/laptops running Linux when connected via a mux.
>>>>>>> Since DT model already can define an i2c mux and devices connected to
>>>>>>> it, this feature is only enabled for ACPI.
>>>>>>>
>>>>>>
>>>>>> This is quite a bit of a hack.
>>>>> Why?
>>>>> Auto detect is standard feature of i2c devices. This is using standard
>>>>> auto detect feature provided by the framework.
>>>>
>>>> Auto detect is ugly, slow and unreliable, it's kind of like the last straw
>>>> if nothing else works.
>>> That is true here. You can't enumerate this device by ACPI. As discussed
>>> before we created i2c mux in inv6050 so that we can use AK8975 in bypass
>>> mode. I added some API to create i2c device on this mux, which Wolfram
>>> didn't like. He wanted to enumerate using existing mechanisms.
>>
>> If there is only a single ACPI ID that says this is a INV6050 with a AK8975 
>> attached then the way to handle this is to have a driver that binds to that id 
>> and creates both devices.
> Which address you will create device also adapter here is i2c mux
> created by inv6050? You don't know i2c address of AK8975. To know AK8975
> i2c address, even ugly hack to parse propriety vendor ACPI data, which
> will change between to manufacturer.
> 
This is odd enough, that I'd like a wider range of views on it.

Wolfram - was this the approach you had in mind?
>>
>> - Lars
>>
> 
> 

Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 181 bytes --]


> This is odd enough, that I'd like a wider range of views on it.
> 
> Wolfram - was this the approach you had in mind?

Sorry, too busy with other stuff at this moment :(


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI

[Srinivas Pandruvada]

On Thu, 2015-01-08 at 17:40 +0100, Wolfram Sang wrote: 
> > This is odd enough, that I'd like a wider range of views on it.
> > 
> > Wolfram - was this the approach you had in mind?
> 
> Sorry, too busy with other stuff at this moment :(
> 
I will find some other way without using auto detect feature.