On Thu, Oct 19, 2023 at 12:17:22PM +0100, Jonathan Cameron wrote: > 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)) Taking the Jonathan's comment below into consideration we can do _something_ like above, but using only a single API call instead of this ugly and monstrous condition. > > 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! -- With Best Regards, Andy Shevchenko
