Re: [PATCH v2 1/5] iio: magnetometer: ak8975: Convert enum->pointer for data in the match tables

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 19 Oct 2023 09:41:06 +0000
Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:

> > Subject: Re: [PATCH v2 1/5] iio: magnetometer: ak8975: Convert enum-  
> > >pointer for data in the match tables  
> > 
> > On Thu, Oct 19, 2023 at 07:08:23AM +0000, Biju Das wrote:  
> > > > Subject: RE: [PATCH v2 1/5] iio: magnetometer: ak8975: Convert enum-  
> > 
> > ...
> >   
> > > > As mentioned in the patch.
> > > > /* If enumerated via firmware node, fix the ABI */
> > > >
> > > > Looks like this issue is not introduced by this patch.
> > > > The previous code uses device_get_match_data() which returns a match
> > > > as it uses DT node and it uses dev_name(&client->dev) instead of
> > > > id->name;
> > > >
> > > > Am I missing anything here? If it is just a test program, can it be  
> > fixed??  
> > > >
> > > > Please correct me if I am wrong.  
> > >
> > > I just realized that there is no .data in previous code for OF tables.
> > >
> > > Maybe we should add a check, if it is DT node, return id->name?
> > >
> > > Is there any API to distinguish DT node from ACPI??  
> > 
> > Of course, but I discourage people to use that, you have to have a very
> > good justification why you need it (and this case doesn't sound good enough
> > to me, or please elaborate). Hence I leave it as a homework to find those
> > APIs.  
> 
> Andre, complained that his test app is broken with this patch. I am waiting for his response whether he can fix his test app? 
> If not, we need to find a solution. One solution
> is adding a name variable and use consistent name across
> OF/ACPI/I2C tables for various devices.
> 
> Other solution is just add this check,
> 
> if (dev_fwnode(&client->dev) && !(IS_ENABLED(CONFIG_OF) && dev->of_node))
> 	name = dev_name(&client->dev);
> else
> 	name = id->name;

Given this is a userspace regression (caused by accidental "fix" - I missed
the fact it had this impact :(), I think it is valid to special case the ACPI in this rare
case but definitely needs a big fat comment saying why we are doing it and that it
should not be copied into other drivers!!!

If we can get away with fixing the original (many years old ABI misuse - but IIRC from a time
where our ABI docs were lacking) then I'm keen on doing so, but I doubt we can.
Definitely don't want to accidentally spread that bug though to new cases!

Jonathan

> 
> Cheers,
> Biju
> 




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux