| Message ID | 20200419150206.43033-1-jic23@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: light: clean out of_match_ptr and tidy headers | expand |
On Sun, 2020-04-19 at 16:01 +0100, jic23@kernel.org wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Hi All, > > Given we keep having to explain to people that of_match_ptr is less > than ideal now we have the option of ACPI DSDT using PRP0001 and > the compatible, it seems sensible to reduce the number of instances > that people might copy for a new driver. > > Added theoretical benefit is that we can probe all these drivers from > appropriate DSDT (though I doubt anyone will). > > I'm sending this first set out to see if anyone has strong views against > doing this for at least the simple drivers that have no other device > tree dependence. Obviously more work would be needed to remove > use of of_match_ptr from IIO completely. > > Light sensors picked as a starting point as they tend to be simple. > > I may do follow ups in larger blocks to avoid so many small patches > (or indeed flatten these into one when applying) fwiw: i was also planning to do a sweep of these; well, tbh, the main intent was to target ADI drivers first and do a bigger conversion for them to make the slightly friendlier with ACPI; aside from this, i'm also noticing some bad patterns being copied from older drivers, when asking new people to write Linux drivers; i did not make a list, probably should have; one is 'mlock' [still] being copied; and accessing other internal fields; but the internal fields accessing requires a bit of a cleanup in the form of privatizing the fields somehow; > > Thanks > > Jonathan > > Jonathan Cameron (7): > iio: light: bh1780: use mod_devicetable.h and drop of_match_ptr macro > iio: light: cm32181: Add mod_devicetable.h and remove of_match_ptr > iio: light: cm3232: Add mod_devicetable.h include and drop > of_match_ptr > iio: light: gp2ap020a00f: Swap of.h for mod_devicetable.h + drop > of_match_ptr > iio: light: opt3001: Add mod_devicetable.h and drop use of > of_match_ptr > iio: light: st_uvis25: Add mod_devicetable.h and drop of_match_ptr > iio: light: vl6180: swap of.h for mod_devicetable.h and drop > of_match_ptr > > drivers/iio/light/bh1780.c | 6 ++---- > drivers/iio/light/cm32181.c | 3 ++- > drivers/iio/light/cm3232.c | 3 ++- > drivers/iio/light/gp2ap020a00f.c | 6 ++---- > drivers/iio/light/opt3001.c | 3 ++- > drivers/iio/light/st_uvis25_i2c.c | 3 ++- > drivers/iio/light/st_uvis25_spi.c | 3 ++- > drivers/iio/light/vl6180.c | 2 +- > 8 files changed, 15 insertions(+), 14 deletions(-) >
On Mon, 2020-04-20 at 06:04 +0000, Ardelean, Alexandru wrote: > [External] > > On Sun, 2020-04-19 at 16:01 +0100, jic23@kernel.org wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Hi All, > > > > Given we keep having to explain to people that of_match_ptr is less > > than ideal now we have the option of ACPI DSDT using PRP0001 and > > the compatible, it seems sensible to reduce the number of instances > > that people might copy for a new driver. > > > > Added theoretical benefit is that we can probe all these drivers from > > appropriate DSDT (though I doubt anyone will). > > > > I'm sending this first set out to see if anyone has strong views against > > doing this for at least the simple drivers that have no other device > > tree dependence. Obviously more work would be needed to remove > > use of of_match_ptr from IIO completely. > > > > Light sensors picked as a starting point as they tend to be simple. > > > > I may do follow ups in larger blocks to avoid so many small patches > > (or indeed flatten these into one when applying) > > fwiw: i was also planning to do a sweep of these; > well, tbh, the main intent was to target ADI drivers first and do a bigger > conversion for them to make the slightly friendlier with ACPI; > > aside from this, i'm also noticing some bad patterns being copied from older > drivers, when asking new people to write Linux drivers; > i did not make a list, probably should have; > one is 'mlock' [still] being copied; and accessing other internal fields; > but the internal fields accessing requires a bit of a cleanup in the form of > privatizing the fields somehow; > One thing I noticed in the series. No idea if it is needed or not; a build would tell: Is '#include <linux/mod_devicetable.h>' required for this change? Most patches add it, but I don't feel it is needed; I could be wrong though. What I noticed is that all 'linux/of.h' , 'linux/of_device.h' & 'linux/of_platform.h' include it. > > > Thanks > > > > Jonathan > > > > Jonathan Cameron (7): > > iio: light: bh1780: use mod_devicetable.h and drop of_match_ptr macro > > iio: light: cm32181: Add mod_devicetable.h and remove of_match_ptr > > iio: light: cm3232: Add mod_devicetable.h include and drop > > of_match_ptr > > iio: light: gp2ap020a00f: Swap of.h for mod_devicetable.h + drop > > of_match_ptr > > iio: light: opt3001: Add mod_devicetable.h and drop use of > > of_match_ptr > > iio: light: st_uvis25: Add mod_devicetable.h and drop of_match_ptr > > iio: light: vl6180: swap of.h for mod_devicetable.h and drop > > of_match_ptr > > > > drivers/iio/light/bh1780.c | 6 ++---- > > drivers/iio/light/cm32181.c | 3 ++- > > drivers/iio/light/cm3232.c | 3 ++- > > drivers/iio/light/gp2ap020a00f.c | 6 ++---- > > drivers/iio/light/opt3001.c | 3 ++- > > drivers/iio/light/st_uvis25_i2c.c | 3 ++- > > drivers/iio/light/st_uvis25_spi.c | 3 ++- > > drivers/iio/light/vl6180.c | 2 +- > > 8 files changed, 15 insertions(+), 14 deletions(-) > >
On Mon, 20 Apr 2020 06:22:09 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Mon, 2020-04-20 at 06:04 +0000, Ardelean, Alexandru wrote: > > [External] > > > > On Sun, 2020-04-19 at 16:01 +0100, jic23@kernel.org wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > Hi All, > > > > > > Given we keep having to explain to people that of_match_ptr is less > > > than ideal now we have the option of ACPI DSDT using PRP0001 and > > > the compatible, it seems sensible to reduce the number of instances > > > that people might copy for a new driver. > > > > > > Added theoretical benefit is that we can probe all these drivers from > > > appropriate DSDT (though I doubt anyone will). > > > > > > I'm sending this first set out to see if anyone has strong views against > > > doing this for at least the simple drivers that have no other device > > > tree dependence. Obviously more work would be needed to remove > > > use of of_match_ptr from IIO completely. > > > > > > Light sensors picked as a starting point as they tend to be simple. > > > > > > I may do follow ups in larger blocks to avoid so many small patches > > > (or indeed flatten these into one when applying) > > > > fwiw: i was also planning to do a sweep of these; > > well, tbh, the main intent was to target ADI drivers first and do a bigger > > conversion for them to make the slightly friendlier with ACPI; > > > > aside from this, i'm also noticing some bad patterns being copied from older > > drivers, when asking new people to write Linux drivers; > > i did not make a list, probably should have; > > one is 'mlock' [still] being copied; and accessing other internal fields; > > but the internal fields accessing requires a bit of a cleanup in the form of > > privatizing the fields somehow; > > > > One thing I noticed in the series. > No idea if it is needed or not; a build would tell: > Is '#include <linux/mod_devicetable.h>' required for this change? > Most patches add it, but I don't feel it is needed; I could be wrong though. I addressed this in reply to patch 7. It's mainly obtained via i2c.h and spi.h in these drivers. They don't have any particular need to include it as they could deal with an opaque pointer. However, seems unlikely that'll get tidied up any time soon and debatable whether there is any point in doing so. > > What I noticed is that all 'linux/of.h' , 'linux/of_device.h' & > 'linux/of_platform.h' include it. True, but we shouldn't include any of them unless we have reason to do so. They bring baggage we don't need for these drivers. Jonathan > > > > > > Thanks > > > > > > Jonathan > > > > > > Jonathan Cameron (7): > > > iio: light: bh1780: use mod_devicetable.h and drop of_match_ptr macro > > > iio: light: cm32181: Add mod_devicetable.h and remove of_match_ptr > > > iio: light: cm3232: Add mod_devicetable.h include and drop > > > of_match_ptr > > > iio: light: gp2ap020a00f: Swap of.h for mod_devicetable.h + drop > > > of_match_ptr > > > iio: light: opt3001: Add mod_devicetable.h and drop use of > > > of_match_ptr > > > iio: light: st_uvis25: Add mod_devicetable.h and drop of_match_ptr > > > iio: light: vl6180: swap of.h for mod_devicetable.h and drop > > > of_match_ptr > > > > > > drivers/iio/light/bh1780.c | 6 ++---- > > > drivers/iio/light/cm32181.c | 3 ++- > > > drivers/iio/light/cm3232.c | 3 ++- > > > drivers/iio/light/gp2ap020a00f.c | 6 ++---- > > > drivers/iio/light/opt3001.c | 3 ++- > > > drivers/iio/light/st_uvis25_i2c.c | 3 ++- > > > drivers/iio/light/st_uvis25_spi.c | 3 ++- > > > drivers/iio/light/vl6180.c | 2 +- > > > 8 files changed, 15 insertions(+), 14 deletions(-) > > >
On Mon, Apr 20, 2020 at 6:49 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Mon, 20 Apr 2020 06:22:09 +0000 > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > > On Mon, 2020-04-20 at 06:04 +0000, Ardelean, Alexandru wrote: > > > On Sun, 2020-04-19 at 16:01 +0100, jic23@kernel.org wrote: > > One thing I noticed in the series. > > No idea if it is needed or not; a build would tell: > > Is '#include <linux/mod_devicetable.h>' required for this change? > > Most patches add it, but I don't feel it is needed; I could be wrong though. > > I addressed this in reply to patch 7. It's mainly obtained via > i2c.h and spi.h in these drivers. They don't have any particular need > to include it as they could deal with an opaque pointer. > > However, seems unlikely that'll get tidied up any time soon and > debatable whether there is any point in doing so. I would use the simple rule, we include header if we have direct user of it. > > What I noticed is that all 'linux/of.h' , 'linux/of_device.h' & > > 'linux/of_platform.h' include it. > > True, but we shouldn't include any of them unless we have reason to do > so. They bring baggage we don't need for these drivers. -- With Best Regards, Andy Shevchenko
On Tue, 21 Apr 2020 03:50:57 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 20, 2020 at 6:49 PM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > On Mon, 20 Apr 2020 06:22:09 +0000 > > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > > > On Mon, 2020-04-20 at 06:04 +0000, Ardelean, Alexandru wrote: > > > > On Sun, 2020-04-19 at 16:01 +0100, jic23@kernel.org wrote: > > > > One thing I noticed in the series. > > > No idea if it is needed or not; a build would tell: > > > Is '#include <linux/mod_devicetable.h>' required for this change? > > > Most patches add it, but I don't feel it is needed; I could be wrong though. > > > > I addressed this in reply to patch 7. It's mainly obtained via > > i2c.h and spi.h in these drivers. They don't have any particular need > > to include it as they could deal with an opaque pointer. > > > > However, seems unlikely that'll get tidied up any time soon and > > debatable whether there is any point in doing so. > > I would use the simple rule, we include header if we have direct user of it. Sensible and easy to understand basis to make the decision. Let's go with that (though I don't want to see lots of isolated 'fixes' for 'missing' headers!) > > > > What I noticed is that all 'linux/of.h' , 'linux/of_device.h' & > > > 'linux/of_platform.h' include it. > > > > True, but we shouldn't include any of them unless we have reason to do > > so. They bring baggage we don't need for these drivers. All applied with patch 7 amended to add the include. Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with them. Thanks, Jonathan > > > -- > With Best Regards, > Andy Shevchenko
From: Jonathan Cameron <Jonathan.Cameron@huawei.com> Hi All, Given we keep having to explain to people that of_match_ptr is less than ideal now we have the option of ACPI DSDT using PRP0001 and the compatible, it seems sensible to reduce the number of instances that people might copy for a new driver. Added theoretical benefit is that we can probe all these drivers from appropriate DSDT (though I doubt anyone will). I'm sending this first set out to see if anyone has strong views against doing this for at least the simple drivers that have no other device tree dependence. Obviously more work would be needed to remove use of of_match_ptr from IIO completely. Light sensors picked as a starting point as they tend to be simple. I may do follow ups in larger blocks to avoid so many small patches (or indeed flatten these into one when applying) Thanks Jonathan Jonathan Cameron (7): iio: light: bh1780: use mod_devicetable.h and drop of_match_ptr macro iio: light: cm32181: Add mod_devicetable.h and remove of_match_ptr iio: light: cm3232: Add mod_devicetable.h include and drop of_match_ptr iio: light: gp2ap020a00f: Swap of.h for mod_devicetable.h + drop of_match_ptr iio: light: opt3001: Add mod_devicetable.h and drop use of of_match_ptr iio: light: st_uvis25: Add mod_devicetable.h and drop of_match_ptr iio: light: vl6180: swap of.h for mod_devicetable.h and drop of_match_ptr drivers/iio/light/bh1780.c | 6 ++---- drivers/iio/light/cm32181.c | 3 ++- drivers/iio/light/cm3232.c | 3 ++- drivers/iio/light/gp2ap020a00f.c | 6 ++---- drivers/iio/light/opt3001.c | 3 ++- drivers/iio/light/st_uvis25_i2c.c | 3 ++- drivers/iio/light/st_uvis25_spi.c | 3 ++- drivers/iio/light/vl6180.c | 2 +- 8 files changed, 15 insertions(+), 14 deletions(-)