| Message ID | 20250331073435.3992597-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] media: i2c: rdacm2x: Make use of device properties | expand |
Quoting Andy Shevchenko (2025-03-31 08:34:35) > Convert the module to be property provider agnostic and allow > it to be used on non-OF platforms. > Looks reasonable to me. > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/media/i2c/rdacm20.c | 5 ++--- > drivers/media/i2c/rdacm21.c | 5 ++--- > 2 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > index b8bd8354d100..dcab63d19baf 100644 > --- a/drivers/media/i2c/rdacm20.c > +++ b/drivers/media/i2c/rdacm20.c > @@ -16,10 +16,10 @@ > */ > > #include <linux/delay.h> > -#include <linux/fwnode.h> > #include <linux/init.h> > #include <linux/i2c.h> > #include <linux/module.h> > +#include <linux/property.h> > #include <linux/slab.h> > #include <linux/videodev2.h> > > @@ -575,8 +575,7 @@ static int rdacm20_probe(struct i2c_client *client) > dev->dev = &client->dev; > dev->serializer.client = client; > > - ret = of_property_read_u32_array(client->dev.of_node, "reg", > - dev->addrs, 2); > + ret = device_property_read_u32_array(&client->dev, "reg", dev->addrs, 2); > if (ret < 0) { > dev_err(dev->dev, "Invalid DT reg property: %d\n", ret); But this is no longer a DT reg property ? > return -EINVAL; > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index 3e22df36354f..5ea6988de48b 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -11,10 +11,10 @@ > */ > > #include <linux/delay.h> > -#include <linux/fwnode.h> > #include <linux/init.h> > #include <linux/i2c.h> > #include <linux/module.h> > +#include <linux/property.h> > #include <linux/slab.h> > #include <linux/videodev2.h> > > @@ -551,8 +551,7 @@ static int rdacm21_probe(struct i2c_client *client) > dev->dev = &client->dev; > dev->serializer.client = client; > > - ret = of_property_read_u32_array(client->dev.of_node, "reg", > - dev->addrs, 2); > + ret = device_property_read_u32_array(&client->dev, "reg", dev->addrs, 2); > if (ret < 0) { > dev_err(dev->dev, "Invalid DT reg property: %d\n", ret); Same here ... With those fixed, Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > return -EINVAL; > -- > 2.47.2 >
On Mon, Mar 31, 2025 at 09:16:36AM +0100, Kieran Bingham wrote: > Quoting Andy Shevchenko (2025-03-31 08:34:35) > > Convert the module to be property provider agnostic and allow > > it to be used on non-OF platforms. > > Looks reasonable to me. Is that going to work out of the box though ? The calls below read the "reg" property to get the device I2C addresses. AFAIK, ACPI handles I2C addresses using ACPI-specific methods. Andy, have you tested this patch on an ACPI system ? > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/media/i2c/rdacm20.c | 5 ++--- > > drivers/media/i2c/rdacm21.c | 5 ++--- > > 2 files changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > > index b8bd8354d100..dcab63d19baf 100644 > > --- a/drivers/media/i2c/rdacm20.c > > +++ b/drivers/media/i2c/rdacm20.c > > @@ -16,10 +16,10 @@ > > */ > > > > #include <linux/delay.h> > > -#include <linux/fwnode.h> > > #include <linux/init.h> > > #include <linux/i2c.h> > > #include <linux/module.h> > > +#include <linux/property.h> > > #include <linux/slab.h> > > #include <linux/videodev2.h> > > > > @@ -575,8 +575,7 @@ static int rdacm20_probe(struct i2c_client *client) > > dev->dev = &client->dev; > > dev->serializer.client = client; > > > > - ret = of_property_read_u32_array(client->dev.of_node, "reg", > > - dev->addrs, 2); > > + ret = device_property_read_u32_array(&client->dev, "reg", dev->addrs, 2); > > if (ret < 0) { > > dev_err(dev->dev, "Invalid DT reg property: %d\n", ret); > > But this is no longer a DT reg property ? > > > return -EINVAL; > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > index 3e22df36354f..5ea6988de48b 100644 > > --- a/drivers/media/i2c/rdacm21.c > > +++ b/drivers/media/i2c/rdacm21.c > > @@ -11,10 +11,10 @@ > > */ > > > > #include <linux/delay.h> > > -#include <linux/fwnode.h> > > #include <linux/init.h> > > #include <linux/i2c.h> > > #include <linux/module.h> > > +#include <linux/property.h> > > #include <linux/slab.h> > > #include <linux/videodev2.h> > > > > @@ -551,8 +551,7 @@ static int rdacm21_probe(struct i2c_client *client) > > dev->dev = &client->dev; > > dev->serializer.client = client; > > > > - ret = of_property_read_u32_array(client->dev.of_node, "reg", > > - dev->addrs, 2); > > + ret = device_property_read_u32_array(&client->dev, "reg", dev->addrs, 2); > > if (ret < 0) { > > dev_err(dev->dev, "Invalid DT reg property: %d\n", ret); > > Same here ... > > With those fixed, > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > return -EINVAL;
On Mon, Mar 31, 2025 at 03:07:48PM +0300, Laurent Pinchart wrote: > On Mon, Mar 31, 2025 at 09:16:36AM +0100, Kieran Bingham wrote: > > Quoting Andy Shevchenko (2025-03-31 08:34:35) > > > Convert the module to be property provider agnostic and allow > > > it to be used on non-OF platforms. > > > > Looks reasonable to me. > > Is that going to work out of the box though ? The calls below read the > "reg" property to get the device I2C addresses. AFAIK, ACPI handles I2C > addresses using ACPI-specific methods. > > Andy, have you tested this patch on an ACPI system ? Only compile-tested. But you are right, this is something different here between OF and ACPI. I can rephrase the commit message to just point out that fwnode.h shouldn't be in the drivers and either converting to device property in an assumption that later it can be easier to support non-OF cases, or using of.h.
On Mon, Mar 31, 2025 at 03:23:21PM +0300, Andy Shevchenko wrote: > On Mon, Mar 31, 2025 at 03:07:48PM +0300, Laurent Pinchart wrote: > > On Mon, Mar 31, 2025 at 09:16:36AM +0100, Kieran Bingham wrote: > > > Quoting Andy Shevchenko (2025-03-31 08:34:35) > > > > Convert the module to be property provider agnostic and allow > > > > it to be used on non-OF platforms. > > > > > > Looks reasonable to me. > > > > Is that going to work out of the box though ? The calls below read the > > "reg" property to get the device I2C addresses. AFAIK, ACPI handles I2C > > addresses using ACPI-specific methods. > > > > Andy, have you tested this patch on an ACPI system ? > > Only compile-tested. But you are right, this is something different here > between OF and ACPI. > > I can rephrase the commit message to just point out that fwnode.h shouldn't > be in the drivers and either converting to device property in an assumption > that later it can be easier to support non-OF cases, or using of.h. I wasn't aware that fwnode.h shouldn't be used in drivers, could you explain that ? If this patch is part of an effort to eliminate usage of some APIs from all drivers, I'm fine with it. Otherwise, I'm not sure it's worth modifying the driver.
On Mon, Mar 31, 2025 at 06:34:35PM +0300, Laurent Pinchart wrote: > On Mon, Mar 31, 2025 at 03:23:21PM +0300, Andy Shevchenko wrote: > > On Mon, Mar 31, 2025 at 03:07:48PM +0300, Laurent Pinchart wrote: > > > On Mon, Mar 31, 2025 at 09:16:36AM +0100, Kieran Bingham wrote: > > > > Quoting Andy Shevchenko (2025-03-31 08:34:35) > > > > > Convert the module to be property provider agnostic and allow > > > > > it to be used on non-OF platforms. > > > > > > > > Looks reasonable to me. > > > > > > Is that going to work out of the box though ? The calls below read the > > > "reg" property to get the device I2C addresses. AFAIK, ACPI handles I2C > > > addresses using ACPI-specific methods. > > > > > > Andy, have you tested this patch on an ACPI system ? > > > > Only compile-tested. But you are right, this is something different here > > between OF and ACPI. > > > > I can rephrase the commit message to just point out that fwnode.h shouldn't > > be in the drivers and either converting to device property in an assumption > > that later it can be easier to support non-OF cases, or using of.h. > > I wasn't aware that fwnode.h shouldn't be used in drivers, could you > explain that ? The fwnode.h provides the data types and definitions that are meant to be used by the fwnode / device property API providers. The leaf drivers shouldn't have any business with those definitions. Everything the drivers need should be provided via property.h. property.h guarantees the necessary data types to be visible to the users, when required (mostly think of struct fwnode_reference_args). Yes, I am aware of v4l2-fwnode.h and it seems it falls into the category of special device property API provider. > If this patch is part of an effort to eliminate usage of some APIs from > all drivers, I'm fine with it. Otherwise, I'm not sure it's worth > modifying the driver. These drivers basically include the wrong header. If you insist, I can patch fwnode.h to add a comment summarizing the above.
On Mon, Mar 31, 2025 at 07:22:27PM +0300, Andy Shevchenko wrote: > On Mon, Mar 31, 2025 at 06:34:35PM +0300, Laurent Pinchart wrote: > > On Mon, Mar 31, 2025 at 03:23:21PM +0300, Andy Shevchenko wrote: > > > On Mon, Mar 31, 2025 at 03:07:48PM +0300, Laurent Pinchart wrote: > > > > On Mon, Mar 31, 2025 at 09:16:36AM +0100, Kieran Bingham wrote: > > > > > Quoting Andy Shevchenko (2025-03-31 08:34:35) > > > > > > Convert the module to be property provider agnostic and allow > > > > > > it to be used on non-OF platforms. > > > > > > > > > > Looks reasonable to me. > > > > > > > > Is that going to work out of the box though ? The calls below read the > > > > "reg" property to get the device I2C addresses. AFAIK, ACPI handles I2C > > > > addresses using ACPI-specific methods. > > > > > > > > Andy, have you tested this patch on an ACPI system ? > > > > > > Only compile-tested. But you are right, this is something different here > > > between OF and ACPI. > > > > > > I can rephrase the commit message to just point out that fwnode.h shouldn't > > > be in the drivers and either converting to device property in an assumption > > > that later it can be easier to support non-OF cases, or using of.h. > > > > I wasn't aware that fwnode.h shouldn't be used in drivers, could you > > explain that ? > > The fwnode.h provides the data types and definitions that are meant > to be used by the fwnode / device property API providers. The leaf drivers > shouldn't have any business with those definitions. Everything the drivers > need should be provided via property.h. property.h guarantees the necessary > data types to be visible to the users, when required (mostly think of > struct fwnode_reference_args). Yes, I am aware of v4l2-fwnode.h and it seems > it falls into the category of special device property API provider. > > > If this patch is part of an effort to eliminate usage of some APIs from > > all drivers, I'm fine with it. Otherwise, I'm not sure it's worth > > modifying the driver. > > These drivers basically include the wrong header. > If you insist, I can patch fwnode.h to add a comment summarizing the above. No, it's fine. I mixed fwnode.h and property.h when writing my previous reply, but I don't think it's a matter of lack of documentation, more likely lack of sleep :-)
On Mon, Mar 31, 2025 at 07:27:39PM +0300, Laurent Pinchart wrote: > On Mon, Mar 31, 2025 at 07:22:27PM +0300, Andy Shevchenko wrote: > > On Mon, Mar 31, 2025 at 06:34:35PM +0300, Laurent Pinchart wrote: > > > On Mon, Mar 31, 2025 at 03:23:21PM +0300, Andy Shevchenko wrote: > > > > On Mon, Mar 31, 2025 at 03:07:48PM +0300, Laurent Pinchart wrote: > > > > > On Mon, Mar 31, 2025 at 09:16:36AM +0100, Kieran Bingham wrote: > > > > > > Quoting Andy Shevchenko (2025-03-31 08:34:35) > > > > > > > Convert the module to be property provider agnostic and allow > > > > > > > it to be used on non-OF platforms. > > > > > > > > > > > > Looks reasonable to me. > > > > > > > > > > Is that going to work out of the box though ? The calls below read the > > > > > "reg" property to get the device I2C addresses. AFAIK, ACPI handles I2C > > > > > addresses using ACPI-specific methods. > > > > > > > > > > Andy, have you tested this patch on an ACPI system ? > > > > > > > > Only compile-tested. But you are right, this is something different here > > > > between OF and ACPI. > > > > > > > > I can rephrase the commit message to just point out that fwnode.h shouldn't > > > > be in the drivers and either converting to device property in an assumption > > > > that later it can be easier to support non-OF cases, or using of.h. > > > > > > I wasn't aware that fwnode.h shouldn't be used in drivers, could you > > > explain that ? > > > > The fwnode.h provides the data types and definitions that are meant > > to be used by the fwnode / device property API providers. The leaf drivers > > shouldn't have any business with those definitions. Everything the drivers > > need should be provided via property.h. property.h guarantees the necessary > > data types to be visible to the users, when required (mostly think of > > struct fwnode_reference_args). Yes, I am aware of v4l2-fwnode.h and it seems > > it falls into the category of special device property API provider. > > > > > If this patch is part of an effort to eliminate usage of some APIs from > > > all drivers, I'm fine with it. Otherwise, I'm not sure it's worth > > > modifying the driver. > > > > These drivers basically include the wrong header. > > If you insist, I can patch fwnode.h to add a comment summarizing the above. > > No, it's fine. I mixed fwnode.h and property.h when writing my previous > reply, but I don't think it's a matter of lack of documentation, more > likely lack of sleep :-) NP. but here we are: 20250331163227.280501-1-andriy.shevchenko@linux.intel.com The bottom line, can you give a tag for this patch and perhaps others of the same matter against drivers/media/* I sent today?
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index b8bd8354d100..dcab63d19baf 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -16,10 +16,10 @@ */ #include <linux/delay.h> -#include <linux/fwnode.h> #include <linux/init.h> #include <linux/i2c.h> #include <linux/module.h> +#include <linux/property.h> #include <linux/slab.h> #include <linux/videodev2.h> @@ -575,8 +575,7 @@ static int rdacm20_probe(struct i2c_client *client) dev->dev = &client->dev; dev->serializer.client = client; - ret = of_property_read_u32_array(client->dev.of_node, "reg", - dev->addrs, 2); + ret = device_property_read_u32_array(&client->dev, "reg", dev->addrs, 2); if (ret < 0) { dev_err(dev->dev, "Invalid DT reg property: %d\n", ret); return -EINVAL; diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index 3e22df36354f..5ea6988de48b 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -11,10 +11,10 @@ */ #include <linux/delay.h> -#include <linux/fwnode.h> #include <linux/init.h> #include <linux/i2c.h> #include <linux/module.h> +#include <linux/property.h> #include <linux/slab.h> #include <linux/videodev2.h> @@ -551,8 +551,7 @@ static int rdacm21_probe(struct i2c_client *client) dev->dev = &client->dev; dev->serializer.client = client; - ret = of_property_read_u32_array(client->dev.of_node, "reg", - dev->addrs, 2); + ret = device_property_read_u32_array(&client->dev, "reg", dev->addrs, 2); if (ret < 0) { dev_err(dev->dev, "Invalid DT reg property: %d\n", ret); return -EINVAL;
Convert the module to be property provider agnostic and allow it to be used on non-OF platforms. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/media/i2c/rdacm20.c | 5 ++--- drivers/media/i2c/rdacm21.c | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-)