diff mbox series

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

Message ID 20231001-ak_magnetometer-v1-1-09bf3b8798a3@apitzsch.eu (mailing list archive)
State Accepted
Headers show
Series iio: magnetometer: ak8975: Fix 'Unexpected device' error | expand

Commit Message

André Apitzsch Oct. 1, 2023, 4:09 p.m. UTC 
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(-)


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

Best regards,

Comments

Andy Shevchenko Oct. 1, 2023, 7:01 p.m. UTC | #1 
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>
Biju Das Oct. 2, 2023, 6:57 a.m. UTC | #2 
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>
Jonathan Cameron Oct. 2, 2023, 9:27 a.m. UTC | #3 
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,
Andy Shevchenko Oct. 2, 2023, 9:34 a.m. UTC | #4 
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?
Biju Das Oct. 2, 2023, 9:38 a.m. UTC | #5 
> 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
Andy Shevchenko Oct. 2, 2023, 9:55 a.m. UTC | #6 
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.
Biju Das Oct. 2, 2023, 9:57 a.m. UTC | #7 
> 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
Biju Das Oct. 2, 2023, 10:16 a.m. UTC | #8 
> -----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
Jonathan Cameron Oct. 3, 2023, 3:55 p.m. UTC | #9 
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...
Jonathan Cameron Oct. 5, 2023, 2:55 p.m. UTC | #10 
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
diff mbox series

Patch

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,