Re: [PATCH 1/4] input: mc13783: Make mc13xxx_buttons_platform_data more flexible

[Lee Jones <lee.jones@linaro.org>]

> > > > > Instead of define each button in separate variable, make an array
> > > > > for storing all buttons. This change will help to add support for
> > > > > other types of PMIC and add support for probing with devicetree
> > > > > in the future.
> > > > > 
> > > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > > > ---
> > > > >  arch/arm/mach-imx/mach-mx31moboard.c   |  9 ++++--
> > > > >  drivers/input/misc/mc13783-pwrbutton.c | 56
> > > > +++++++++++++++++-----------------
> > > > >  include/linux/mfd/mc13xxx.h            | 28 +++++++++--------
> > > > 
> > > > Are all the changes in these files entangled? If there is any way to
> > > > make them orthogonal, then all the better.
> > > 
> > > Can you say the same thing in other words, I didn't understand a bit
> > > of your comment.
> > 
> > Do all of this changes in each of the files listed above depend on
> > each other?
> 
> Yes, mc13783-pwrbutton.c & mc13xxx.h - changing the structure,
> mach-mx31moboard.c - updating current users.
> 
> > > > > +struct mc13xxx_button {
> > > > > +	u16		keycode;
> > > > > +	unsigned int	flags;
> > > > > +#define MC13XXX_BUTTON_DBNC_0MS		0
> > > > > +#define MC13XXX_BUTTON_DBNC_30MS	1
> > > > > +#define MC13XXX_BUTTON_DBNC_150MS	2
> > > > > +#define MC13XXX_BUTTON_DBNC_750MS	3
> > > > > +#define MC13XXX_BUTTON_ENABLE		(1 << 2)
> > > > > +#define MC13XXX_BUTTON_POL_INVERT	(1 << 3)
> > > > > +#define MC13XXX_BUTTON_RESET_EN		(1 << 4)
> > > > > +};
> > > > > +
> > > > 
> > > > Please take the opportunity to remove this slab list of #defines from
> > > > the centre of the struct definition. Just above will be fine.
> > > 
> > > I would like to make it in a separate patch for the whole header file later.
> > 
> > You may as well do it in this commit?  Just add them outside of the
> > struct definition instead of inside.
> 
> OK, I'll do it in version 2, but I meant that I could change it for the other
> structures in this header. As part of the input subsystem, I can not do that.

If changes can be subsystem orthogonal, it's better to keep them that
way. Fix this issue in this patch and fix any other issues within this
file in another patch which will go through the MFD tree.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html