iio: magnetometer: ak8975: Fix 'Unexpected device' error

[PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error

Posted by André Apitzsch 1 year, 7 months ago

Explicity specify array indices to fix mapping between
asahi_compass_chipset and ak_def_array.
While at it, remove unneeded AKXXXX.

Fixes: 4f9ea93afde1 ("iio: magnetometer: ak8975: Convert enum->pointer for data in the match tables")
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
 drivers/iio/magnetometer/ak8975.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 8cfceb007936..dd466c5fa621 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -204,7 +204,6 @@ static long ak09912_raw_to_gauss(u16 data)
 
 /* Compatible Asahi Kasei Compass parts */
 enum asahi_compass_chipset {
-	AKXXXX		= 0,
 	AK8975,
 	AK8963,
 	AK09911,
@@ -248,7 +247,7 @@ struct ak_def {
 };
 
 static const struct ak_def ak_def_array[] = {
-	{
+	[AK8975] = {
 		.type = AK8975,
 		.raw_to_gauss = ak8975_raw_to_gauss,
 		.range = 4096,
@@ -273,7 +272,7 @@ static const struct ak_def ak_def_array[] = {
 			AK8975_REG_HYL,
 			AK8975_REG_HZL},
 	},
-	{
+	[AK8963] = {
 		.type = AK8963,
 		.raw_to_gauss = ak8963_09911_raw_to_gauss,
 		.range = 8190,
@@ -298,7 +297,7 @@ static const struct ak_def ak_def_array[] = {
 			AK8975_REG_HYL,
 			AK8975_REG_HZL},
 	},
-	{
+	[AK09911] = {
 		.type = AK09911,
 		.raw_to_gauss = ak8963_09911_raw_to_gauss,
 		.range = 8192,
@@ -323,7 +322,7 @@ static const struct ak_def ak_def_array[] = {
 			AK09912_REG_HYL,
 			AK09912_REG_HZL},
 	},
-	{
+	[AK09912] = {
 		.type = AK09912,
 		.raw_to_gauss = ak09912_raw_to_gauss,
 		.range = 32752,
@@ -348,7 +347,7 @@ static const struct ak_def ak_def_array[] = {
 			AK09912_REG_HYL,
 			AK09912_REG_HZL},
 	},
-	{
+	[AK09916] = {
 		.type = AK09916,
 		.raw_to_gauss = ak09912_raw_to_gauss,
 		.range = 32752,

---
base-commit: df964ce9ef9fea10cf131bf6bad8658fde7956f6
change-id: 20231001-ak_magnetometer-b063098082dd

Best regards,
-- 
André Apitzsch <git@apitzsch.eu>

Re: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error

Posted by Jonathan Cameron 1 year, 7 months ago

On Sun, 1 Oct 2023 18:09:56 +0200
André Apitzsch <git@apitzsch.eu> wrote:

> Explicity specify array indices to fix mapping between
> asahi_compass_chipset and ak_def_array.
> While at it, remove unneeded AKXXXX.
> 
> Fixes: 4f9ea93afde1 ("iio: magnetometer: ak8975: Convert enum->pointer for data in the match tables")
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
>  drivers/iio/magnetometer/ak8975.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 8cfceb007936..dd466c5fa621 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -204,7 +204,6 @@ static long ak09912_raw_to_gauss(u16 data)
>  
>  /* Compatible Asahi Kasei Compass parts */
>  enum asahi_compass_chipset {
> -	AKXXXX		= 0,

When we see this 'spacer' it is normally there to avoid a
confusion between 'not defined' and set too the first element.

So look at device_get_match_data() implementation and how we tell
if it worked.  That will return 0 if we have an AK8975 which is
then detected as a failure to match. 

https://elixir.bootlin.com/linux/v6.6-rc3/source/drivers/iio/magnetometer/ak8975.c#L932

So we need the spacer until someone converts this driver to use
pointers instead for both of and ACPI tables. I also don't like that
the of path here is falling back to the i2c_device_id match.

The data should be set in ak8975_of_match[] as well.

Jonathan

>  	AK8975,
>  	AK8963,
>  	AK09911,
> @@ -248,7 +247,7 @@ struct ak_def {
>  };
>  
>  static const struct ak_def ak_def_array[] = {
> -	{
> +	[AK8975] = {
>  		.type = AK8975,
>  		.raw_to_gauss = ak8975_raw_to_gauss,
>  		.range = 4096,
> @@ -273,7 +272,7 @@ static const struct ak_def ak_def_array[] = {
>  			AK8975_REG_HYL,
>  			AK8975_REG_HZL},
>  	},
> -	{
> +	[AK8963] = {
>  		.type = AK8963,
>  		.raw_to_gauss = ak8963_09911_raw_to_gauss,
>  		.range = 8190,
> @@ -298,7 +297,7 @@ static const struct ak_def ak_def_array[] = {
>  			AK8975_REG_HYL,
>  			AK8975_REG_HZL},
>  	},
> -	{
> +	[AK09911] = {
>  		.type = AK09911,
>  		.raw_to_gauss = ak8963_09911_raw_to_gauss,
>  		.range = 8192,
> @@ -323,7 +322,7 @@ static const struct ak_def ak_def_array[] = {
>  			AK09912_REG_HYL,
>  			AK09912_REG_HZL},
>  	},
> -	{
> +	[AK09912] = {
>  		.type = AK09912,
>  		.raw_to_gauss = ak09912_raw_to_gauss,
>  		.range = 32752,
> @@ -348,7 +347,7 @@ static const struct ak_def ak_def_array[] = {
>  			AK09912_REG_HYL,
>  			AK09912_REG_HZL},
>  	},
> -	{
> +	[AK09916] = {
>  		.type = AK09916,
>  		.raw_to_gauss = ak09912_raw_to_gauss,
>  		.range = 32752,
> 
> ---
> base-commit: df964ce9ef9fea10cf131bf6bad8658fde7956f6
> change-id: 20231001-ak_magnetometer-b063098082dd
> 
> Best regards,

Re: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error

Posted by Andy Shevchenko 1 year, 7 months ago

On Mon, Oct 02, 2023 at 10:27:45AM +0100, Jonathan Cameron wrote:
> On Sun, 1 Oct 2023 18:09:56 +0200
> André Apitzsch <git@apitzsch.eu> wrote:

> > Fixes: 4f9ea93afde1 ("iio: magnetometer: ak8975: Convert enum->pointer for data in the match tables")

^^^ (1)

...

> So we need the spacer until someone converts this driver to use
> pointers instead for both of and ACPI tables.

Isn't it done by (1) which is in your tree?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error

Posted by Jonathan Cameron 1 year, 7 months ago

On Mon, 2 Oct 2023 12:34:23 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Oct 02, 2023 at 10:27:45AM +0100, Jonathan Cameron wrote:
> > On Sun, 1 Oct 2023 18:09:56 +0200
> > André Apitzsch <git@apitzsch.eu> wrote:  
> 
> > > Fixes: 4f9ea93afde1 ("iio: magnetometer: ak8975: Convert enum->pointer for data in the match tables")  
> 
> ^^^ (1)
> 
> ...
> 
> > So we need the spacer until someone converts this driver to use
> > pointers instead for both of and ACPI tables.  
> 
> Isn't it done by (1) which is in your tree?
> 
I can't remember what's in my tree :)

Good point...

Re: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error

Posted by Jonathan Cameron 1 year, 7 months ago

On Tue, 3 Oct 2023 16:55:35 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Mon, 2 Oct 2023 12:34:23 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Mon, Oct 02, 2023 at 10:27:45AM +0100, Jonathan Cameron wrote:  
> > > On Sun, 1 Oct 2023 18:09:56 +0200
> > > André Apitzsch <git@apitzsch.eu> wrote:    
> >   
> > > > Fixes: 4f9ea93afde1 ("iio: magnetometer: ak8975: Convert enum->pointer for data in the match tables")    
> > 
> > ^^^ (1)
> > 
> > ...
> >   
> > > So we need the spacer until someone converts this driver to use
> > > pointers instead for both of and ACPI tables.    
> > 
> > Isn't it done by (1) which is in your tree?
> >   
> I can't remember what's in my tree :)
> 
> Good point...
> 
> 
Applied to the togreg branch of iio.git and pushed out as testing
for 0-day to poke at it.

Thanks,

Jonathan

RE: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error

Posted by Biju Das 1 year, 7 months ago

> Subject: Re: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device'
> error
> 
> On Mon, Oct 02, 2023 at 10:27:45AM +0100, Jonathan Cameron wrote:
> > On Sun, 1 Oct 2023 18:09:56 +0200
> > André Apitzsch <git@apitzsch.eu> wrote:
> 
> > > Fixes: 4f9ea93afde1 ("iio: magnetometer: ak8975: Convert
> > > enum->pointer for data in the match tables")
> 
> ^^^ (1)
> 
> ...
> 
> > So we need the spacer until someone converts this driver to use
> > pointers instead for both of and ACPI tables.
> 
> Isn't it done by (1) which is in your tree?

How (1) can trigger 'Unexpected device' error??
It returns match_data, and match_data has correct device type.

Cheers,
Biju

Re: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error

Posted by Andy Shevchenko 1 year, 7 months ago

On Mon, Oct 02, 2023 at 09:38:17AM +0000, Biju Das wrote:
> > On Mon, Oct 02, 2023 at 10:27:45AM +0100, Jonathan Cameron wrote:
> > > On Sun, 1 Oct 2023 18:09:56 +0200
> > > André Apitzsch <git@apitzsch.eu> wrote:
> > 
> > > > Fixes: 4f9ea93afde1 ("iio: magnetometer: ak8975: Convert
> > > > enum->pointer for data in the match tables")
> > 
> > ^^^ (1)

...

> > > So we need the spacer until someone converts this driver to use
> > > pointers instead for both of and ACPI tables.
> > 
> > Isn't it done by (1) which is in your tree?
> 
> How (1) can trigger 'Unexpected device' error??
> It returns match_data, and match_data has correct device type.

How? The enum starts from 0 with a AKXXXX and ak_def_array starts from 0
indexing, it's classical off-by-one, you got the driver data for a wrong chip.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error

Posted by Biju Das 1 year, 7 months ago

> Subject: Re: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device'
> error
> 
> On Mon, Oct 02, 2023 at 09:38:17AM +0000, Biju Das wrote:
> > > On Mon, Oct 02, 2023 at 10:27:45AM +0100, Jonathan Cameron wrote:
> > > > On Sun, 1 Oct 2023 18:09:56 +0200
> > > > André Apitzsch <git@apitzsch.eu> wrote:
> > >
> > > > > Fixes: 4f9ea93afde1 ("iio: magnetometer: ak8975: Convert
> > > > > enum->pointer for data in the match tables")
> > >
> > > ^^^ (1)
> 
> ...
> 
> > > > So we need the spacer until someone converts this driver to use
> > > > pointers instead for both of and ACPI tables.
> > >
> > > Isn't it done by (1) which is in your tree?
> >
> > How (1) can trigger 'Unexpected device' error??
> > It returns match_data, and match_data has correct device type.
> 
> How? The enum starts from 0 with a AKXXXX and ak_def_array starts from 0
> indexing, it's classical off-by-one, you got the driver data for a wrong
> chip.

Got it. I was about to reply. It just needs mapping as Andre did here.

Cheers,
Biju

RE: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error

Posted by Biju Das 1 year, 7 months ago

> -----Original Message-----
> From: Biju Das
> Sent: Monday, October 2, 2023 10:57 AM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>; André Apitzsch
> <git@apitzsch.eu>; Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device'
> error
> 
> > Subject: Re: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device'
> > error
> >
> > On Mon, Oct 02, 2023 at 09:38:17AM +0000, Biju Das wrote:
> > > > On Mon, Oct 02, 2023 at 10:27:45AM +0100, Jonathan Cameron wrote:
> > > > > On Sun, 1 Oct 2023 18:09:56 +0200 André Apitzsch
> > > > > <git@apitzsch.eu> wrote:
> > > >
> > > > > > Fixes: 4f9ea93afde1 ("iio: magnetometer: ak8975: Convert
> > > > > > enum->pointer for data in the match tables")
> > > >
> > > > ^^^ (1)
> >
> > ...
> >
> > > > > So we need the spacer until someone converts this driver to use
> > > > > pointers instead for both of and ACPI tables.
> > > >
> > > > Isn't it done by (1) which is in your tree?
> > >
> > > How (1) can trigger 'Unexpected device' error??
> > > It returns match_data, and match_data has correct device type.
> >
> > How? The enum starts from 0 with a AKXXXX and ak_def_array starts from
> > 0 indexing, it's classical off-by-one, you got the driver data for a
> > wrong chip.
> 
> Got it. I was about to reply. It just needs mapping as Andre did here.

Or split this array into device specific individual variable
to avoid any mapping.

Cheers,
Biju

RE: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error

Posted by Biju Das 1 year, 7 months ago

Hi André Apitzsch,

> Subject: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error
> 
> Explicity specify array indices to fix mapping between
> asahi_compass_chipset and ak_def_array.
> While at it, remove unneeded AKXXXX.
> 
> Fixes: 4f9ea93afde1 ("iio: magnetometer: ak8975: Convert enum->pointer for
> data in the match tables")
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
>  drivers/iio/magnetometer/ak8975.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c
> b/drivers/iio/magnetometer/ak8975.c
> index 8cfceb007936..dd466c5fa621 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -204,7 +204,6 @@ static long ak09912_raw_to_gauss(u16 data)
> 
>  /* Compatible Asahi Kasei Compass parts */  enum asahi_compass_chipset {
> -	AKXXXX		= 0,

I guess this change is enough, after this AK8975 = 0 and
No need to update the mapping. Anyway this is personal preference.

Looks good to me.

Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Cheers,
Biju

>  	AK8975,
>  	AK8963,
>  	AK09911,
> @@ -248,7 +247,7 @@ struct ak_def {
>  };
> 
>  static const struct ak_def ak_def_array[] = {
> -	{
> +	[AK8975] = {
>  		.type = AK8975,
>  		.raw_to_gauss = ak8975_raw_to_gauss,
>  		.range = 4096,
> @@ -273,7 +272,7 @@ static const struct ak_def ak_def_array[] = {
>  			AK8975_REG_HYL,
>  			AK8975_REG_HZL},
>  	},
> -	{
> +	[AK8963] = {
>  		.type = AK8963,
>  		.raw_to_gauss = ak8963_09911_raw_to_gauss,
>  		.range = 8190,
> @@ -298,7 +297,7 @@ static const struct ak_def ak_def_array[] = {
>  			AK8975_REG_HYL,
>  			AK8975_REG_HZL},
>  	},
> -	{
> +	[AK09911] = {
>  		.type = AK09911,
>  		.raw_to_gauss = ak8963_09911_raw_to_gauss,
>  		.range = 8192,
> @@ -323,7 +322,7 @@ static const struct ak_def ak_def_array[] = {
>  			AK09912_REG_HYL,
>  			AK09912_REG_HZL},
>  	},
> -	{
> +	[AK09912] = {
>  		.type = AK09912,
>  		.raw_to_gauss = ak09912_raw_to_gauss,
>  		.range = 32752,
> @@ -348,7 +347,7 @@ static const struct ak_def ak_def_array[] = {
>  			AK09912_REG_HYL,
>  			AK09912_REG_HZL},
>  	},
> -	{
> +	[AK09916] = {
>  		.type = AK09916,
>  		.raw_to_gauss = ak09912_raw_to_gauss,
>  		.range = 32752,
> 
> ---
> base-commit: df964ce9ef9fea10cf131bf6bad8658fde7956f6
> change-id: 20231001-ak_magnetometer-b063098082dd
> 
> Best regards,
> --
> André Apitzsch <git@apitzsch.eu>

Re: [PATCH] iio: magnetometer: ak8975: Fix 'Unexpected device' error

Posted by Andy Shevchenko 1 year, 7 months ago

On Sun, Oct 01, 2023 at 06:09:56PM +0200, André Apitzsch wrote:
> Explicity specify array indices to fix mapping between
> asahi_compass_chipset and ak_def_array.
> While at it, remove unneeded AKXXXX.

Indeed, good catch!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko