| Message ID | 20240101133234.10310-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: da280: Simplify id-matching | expand |
On Mon, 1 Jan 2024 14:32:34 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > da280_match_acpi_device() is a DIY version of acpi_device_get_match_data(), > so it can be dropped. > > And things can be simplified further by using i2c_get_match_data() which > will also check i2c_client_id style ids. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Hi Hans and happy new year, This runs into a slightly nasty corner case (which can't actually happen because we know it will match) of a NULL return on failure to match which ends up being the first enum entry whereas we should probably return an error. My preferred cleanup would be to make both id tables point to a set of structs that encode the device differences as data rather than ids. struct da280_chip_info { const char *name; int num_channels; } or something along those lines. Then we can rely on the generic lookup function without taking care about the 0 value. Jonathan > --- > drivers/iio/accel/da280.c | 18 +----------------- > 1 file changed, 1 insertion(+), 17 deletions(-) > > diff --git a/drivers/iio/accel/da280.c b/drivers/iio/accel/da280.c > index 572bfe9694b0..e4cd4b3a28ab 100644 > --- a/drivers/iio/accel/da280.c > +++ b/drivers/iio/accel/da280.c > @@ -89,17 +89,6 @@ static const struct iio_info da280_info = { > .read_raw = da280_read_raw, > }; > > -static enum da280_chipset da280_match_acpi_device(struct device *dev) > -{ > - const struct acpi_device_id *id; > - > - id = acpi_match_device(dev->driver->acpi_match_table, dev); > - if (!id) > - return -EINVAL; > - > - return (enum da280_chipset) id->driver_data; > -} > - > static void da280_disable(void *client) > { > da280_enable(client, false); > @@ -107,7 +96,6 @@ static void da280_disable(void *client) > > static int da280_probe(struct i2c_client *client) > { > - const struct i2c_device_id *id = i2c_client_get_device_id(client); > int ret; > struct iio_dev *indio_dev; > struct da280_data *data; > @@ -128,11 +116,7 @@ static int da280_probe(struct i2c_client *client) > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = da280_channels; > > - if (ACPI_HANDLE(&client->dev)) { > - chip = da280_match_acpi_device(&client->dev); > - } else { > - chip = id->driver_data; > - } > + chip = (uintptr_t)i2c_get_match_data(client); > > if (chip == da217) { > indio_dev->name = "da217";
Hi, On 1/1/24 16:42, Jonathan Cameron wrote: > On Mon, 1 Jan 2024 14:32:34 +0100 > Hans de Goede <hdegoede@redhat.com> wrote: > >> da280_match_acpi_device() is a DIY version of acpi_device_get_match_data(), >> so it can be dropped. >> >> And things can be simplified further by using i2c_get_match_data() which >> will also check i2c_client_id style ids. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Hi Hans and happy new year, > > This runs into a slightly nasty corner case (which can't actually happen because > we know it will match) of a NULL return on failure to match which ends up > being the first enum entry whereas we should probably return an error. > > My preferred cleanup would be to make both id tables point to a set of structs > that encode the device differences as data rather than ids. > > struct da280_chip_info { > const char *name; > int num_channels; > } > > or something along those lines. Then we can rely on the generic lookup function > without taking care about the 0 value. That is a good idea, thanks. I have prepared (and will send out shortly) a v2 implementing this. Regards, Hans >> --- >> drivers/iio/accel/da280.c | 18 +----------------- >> 1 file changed, 1 insertion(+), 17 deletions(-) >> >> diff --git a/drivers/iio/accel/da280.c b/drivers/iio/accel/da280.c >> index 572bfe9694b0..e4cd4b3a28ab 100644 >> --- a/drivers/iio/accel/da280.c >> +++ b/drivers/iio/accel/da280.c >> @@ -89,17 +89,6 @@ static const struct iio_info da280_info = { >> .read_raw = da280_read_raw, >> }; >> >> -static enum da280_chipset da280_match_acpi_device(struct device *dev) >> -{ >> - const struct acpi_device_id *id; >> - >> - id = acpi_match_device(dev->driver->acpi_match_table, dev); >> - if (!id) >> - return -EINVAL; >> - >> - return (enum da280_chipset) id->driver_data; >> -} >> - >> static void da280_disable(void *client) >> { >> da280_enable(client, false); >> @@ -107,7 +96,6 @@ static void da280_disable(void *client) >> >> static int da280_probe(struct i2c_client *client) >> { >> - const struct i2c_device_id *id = i2c_client_get_device_id(client); >> int ret; >> struct iio_dev *indio_dev; >> struct da280_data *data; >> @@ -128,11 +116,7 @@ static int da280_probe(struct i2c_client *client) >> indio_dev->modes = INDIO_DIRECT_MODE; >> indio_dev->channels = da280_channels; >> >> - if (ACPI_HANDLE(&client->dev)) { >> - chip = da280_match_acpi_device(&client->dev); >> - } else { >> - chip = id->driver_data; >> - } >> + chip = (uintptr_t)i2c_get_match_data(client); >> >> if (chip == da217) { >> indio_dev->name = "da217"; >
diff --git a/drivers/iio/accel/da280.c b/drivers/iio/accel/da280.c index 572bfe9694b0..e4cd4b3a28ab 100644 --- a/drivers/iio/accel/da280.c +++ b/drivers/iio/accel/da280.c @@ -89,17 +89,6 @@ static const struct iio_info da280_info = { .read_raw = da280_read_raw, }; -static enum da280_chipset da280_match_acpi_device(struct device *dev) -{ - const struct acpi_device_id *id; - - id = acpi_match_device(dev->driver->acpi_match_table, dev); - if (!id) - return -EINVAL; - - return (enum da280_chipset) id->driver_data; -} - static void da280_disable(void *client) { da280_enable(client, false); @@ -107,7 +96,6 @@ static void da280_disable(void *client) static int da280_probe(struct i2c_client *client) { - const struct i2c_device_id *id = i2c_client_get_device_id(client); int ret; struct iio_dev *indio_dev; struct da280_data *data; @@ -128,11 +116,7 @@ static int da280_probe(struct i2c_client *client) indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = da280_channels; - if (ACPI_HANDLE(&client->dev)) { - chip = da280_match_acpi_device(&client->dev); - } else { - chip = id->driver_data; - } + chip = (uintptr_t)i2c_get_match_data(client); if (chip == da217) { indio_dev->name = "da217";
da280_match_acpi_device() is a DIY version of acpi_device_get_match_data(), so it can be dropped. And things can be simplified further by using i2c_get_match_data() which will also check i2c_client_id style ids. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/iio/accel/da280.c | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-)