Hi Pavel,
On Thu, Apr 16, 2015 at 07:58:18AM +0200, Pavel Machek wrote:
> On Thu 2015-04-16 07:24:42, Sebastian Reichel wrote:
> > Hi Sakari,
> >
> > Since this driver won't make it into 4.1 anyways, I have one more
> > comment:
>
> Like this driver did not receive enough bikesheding.
You should become more restrained with this argument if
you want it to be taken seriously by me in the future...
> > > + } else {
> > > + gpiod_set_value(flash->platform_data->enable_gpio, on);
> > > + if (on)
> > > + /* Some delay is apparently required. */
> > > + udelay(20);
> > > + }
> >
> > I suggest to remove the power callback from platform data. Instead
> > you can require to setup a gpiod lookup table in the boardcode, if
> > platform data based initialization is used (see for example si4713
> > initialization in board-rx51-periphals.c).
> >
> > This will reduce complexity in the driver and should be fairly easy
> > to implement, since there is no adp1653 platform code user in the
> > mainline kernel anyways.
>
> I'd hate to break out of tree users for very little gain.
This normally happens as the kernel progresses. If you want your
driver not to break, you should sent it upstream and maintain it.
Also the only out-of-tree user I know is the Nokia N900 (which has
already broken camera subsystem). Note that the required out of tree
changes to use the platform code with gpiod interface are actually
quite small and if you really care about it, they could actually be
done *in kernel*.
Note that many drivers are updated to use new kernel APIs together
with the DT changes - especially those, which haven't been updated
for quite some time.
So let's have a look at the advantages of removing the power gpio:
+ gpio handling is always done in the driver, making code
easier to read
+ less loc in the driver, making it easier to read
+ less loc in the boardcode (no gpio requesting/releasing)
+ less branching in driver code - easier testing coverage
- out of tree users will break
So basically its code quality vs minor out-of-tree breakage.
-- Sebastian
Attachment:
signature.asc
Description: Digital signature
