Re: [PATCH 0/7] iio: light: clean out of_match_ptr and tidy headers

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

 



On Mon, 20 Apr 2020 06:22:09 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@xxxxxxxxxx> wrote:

> On Mon, 2020-04-20 at 06:04 +0000, Ardelean, Alexandru wrote:
> > [External]
> > 
> > On Sun, 2020-04-19 at 16:01 +0100, jic23@xxxxxxxxxx wrote:  
> > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > > 
> > > 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(-)
> > >   





[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