Re: Re: [PATCH v2 2/6] mfd: axp20x: Add register bits for axp20x-ac-power

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2710 bytes --]

Hi,

On Mon, May 02, 2016 at 09:35:01AM +0800, Chen-Yu Tsai wrote:
> On Sun, May 1, 2016 at 10:24 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
> > On 05/01/2016 11:48 AM, Chen-Yu Tsai wrote:
> >> Hi,
> >>
> >> On Sun, May 1, 2016 at 4:57 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
> >>> This change adds some register bit definitions used by the
> >>> axp20x-ac-power driver.
> >>>
> >>> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
> >>> ---
> >>>  include/linux/mfd/axp20x.h | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> >>> index d82e7d5..c4c6dfa 100644
> >>> --- a/include/linux/mfd/axp20x.h
> >>> +++ b/include/linux/mfd/axp20x.h
> >>> @@ -90,6 +90,17 @@ enum {
> >>>  #define AXP22X_ALDO3_V_OUT             0x2a
> >>>  #define AXP22X_CHRG_CTRL3              0x35
> >>>
> >>> +
> >>> +/* Fields of AXP20X_PWR_INPUT_STATUS */
> >>> +#define AXP20X_PWR_STATUS_AC_PRESENT     BIT(7)
> >>> +#define AXP20X_PWR_STATUS_AC_AVAILABLE   BIT(6)
> >>> +#define AXP20X_PWR_STATUS_AC_VBUS_SHORT  BIT(1)
> >>> +#define AXP20X_PWR_STATUS_AC_VBUS_SEL    BIT(0)
> >>> +
> >>> +/* Fields of AXP20X_ADC_EN1 */
> >>> +#define AXP20X_ADC_EN1_ACIN_VOLT BIT(5)
> >>> +#define AXP20X_ADC_EN1_ACIN_CURR BIT(4)
> >>> +
> >>
> >> We keep the bit definitions of each register in each separate driver.
> >> The drivers only define the ones they use.
> >>
> >> ChenYu
> >
> > Hi ChenYu,
> >
> > i believe Maxime Ripard requested that these defines be moved to the
> > header: https://groups.google.com/d/msg/linux-sunxi/nEUg87cV6KI/TvdB6MBZBAAJ
> >
> > What do you think?
> 
> My argument is kind of weak, and really comes down to preference.
> 
> Currently the register bit definitions are scattered in various drivers,
> which is fine given they are really specific to the part of hardware the
> driver supports. Gathering them all together might increase the size of
> the header file substantially. As I see it the chanses that bits from
> one part are going to be used in another are rather small.
> 
> Some register address macros are shared, such as for the 2 power supply
> drivers, and for the regmap definitions. So those would need to go in a
> shared header anyway.

Sorry for the misunderstanding. I was assuming that having all the
registers and associated bits would be better off in a common header
where all the drivers could refer to, but you're the maintainer on
that on, so it's up to you ;)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com