Re: [PATCH v4] mfd: intel_soc_pmic_bxtwc: Add Intel BXT WhiskeyCove PMIC ADC thermal channel mapping and USB type-C resources

[Bin Gao <bin.gao@linux.intel.com>]

On Tue, Jun 28, 2016 at 03:58:49PM +0100, Lee Jones wrote:
> On Mon, 27 Jun 2016, Bin Gao wrote:
> 
> > This patch adds the mapping of PMIC ADC channel to thermal zone and
> > USB type-C resources. This mapping is used in the pmic thermal driver
> > to notify the thermal zone with the pmic adc channel alert interrupts.
> 
> > This patch also adds three new data structures to
> > include/linux/mfd/intel_soc_pmic.h: struct trip_config_map{},
> > struct thermal_irq_map {} and struct pmic_thermal_data {} which are
> > required by changes we did on intel_soc_pmic_bxtwc.c.
> 
> No need for this section.
> 
> > Signed-off-by: Yegnesh S Iyer <yegnesh.s.iyer@intel.com>
> > Signed-off-by: Rohit S Kenchanpura <rohit.s.kenchanpura@intel.com>
> > Signed-off-by: Bin Gao <bin.gao@intel.com>
> > ---
> > Changes in v4:
> >  - Extended existing regmap reg instead of defining a new one:
> >    REGMAP_IRQ_REG(BXTWC_CHGR0_IRQ, 5, 0x3f)
> > Changes in v3:
> >  - Added USB type-C resources.
> > Changes in v2:
> >  - Fixed subject line.
> >  - Combined two patches into one.
> >  drivers/mfd/intel_soc_pmic_bxtwc.c | 126 ++++++++++++++++++++++++++++++++++++-
> >  include/linux/mfd/intel_soc_pmic.h |  21 +++++++
> >  2 files changed, 146 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c b/drivers/mfd/intel_soc_pmic_bxtwc.c
> > index b942876..e69eb86 100644
> > --- a/drivers/mfd/intel_soc_pmic_bxtwc.c
> > +++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
> > @@ -47,6 +47,8 @@
> >  #define BXTWC_MIRQLVL1		0x4E0E
> >  #define BXTWC_MPWRTNIRQ		0x4E0F
> >  
> > +#define BXTWC_MIRQLVL1_MCHGR	BIT(5)
> > +
> 
> This line looks very much out of place here.
> 
> >  #define BXTWC_MTHRM0IRQ		0x4E12
> >  #define BXTWC_MTHRM1IRQ		0x4E13
> >  #define BXTWC_MTHRM2IRQ		0x4E14
> > @@ -58,6 +60,10 @@
> >  #define BXTWC_MGPIO1IRQ		0x4E1A
> >  #define BXTWC_MCRITIRQ		0x4E1B
> >  
> > +#define BXTWC_STHRM0IRQ		0x4F19
> > +#define BXTWC_STHRM1IRQ		0x4F1A
> > +#define BXTWC_STHRM2IRQ		0x4F1B
> > +
> >  /* Whiskey Cove PMIC share same ACPI ID between different platforms */
> >  #define BROXTON_PMIC_WC_HRV	4
> >  
> > @@ -109,13 +115,116 @@ static const struct regmap_irq bxtwc_regmap_irqs_level2[] = {
> >  	REGMAP_IRQ_REG(BXTWC_THRM2_IRQ, 2, 0xff),
> >  	REGMAP_IRQ_REG(BXTWC_BCU_IRQ, 3, 0x1f),
> >  	REGMAP_IRQ_REG(BXTWC_ADC_IRQ, 4, 0xff),
> > -	REGMAP_IRQ_REG(BXTWC_CHGR0_IRQ, 5, 0x1f),
> > +	REGMAP_IRQ_REG(BXTWC_CHGR0_IRQ, 5, 0x3f),
> 
> Is the original code wrong?
> 
> If so, it's a fix and needs it's own patch
> .
> >  	REGMAP_IRQ_REG(BXTWC_CHGR1_IRQ, 6, 0x1f),
> >  	REGMAP_IRQ_REG(BXTWC_GPIO0_IRQ, 7, 0xff),
> >  	REGMAP_IRQ_REG(BXTWC_GPIO1_IRQ, 8, 0x3f),
> >  	REGMAP_IRQ_REG(BXTWC_CRIT_IRQ, 9, 0x03),
> >  };
> >  
> > +static struct trip_config_map str0_trip_config[] = {
> > +	{
> > +		.irq_reg = BXTWC_THRM0IRQ,
> > +		.irq_mask = 0x01,
> > +		.irq_en = BXTWC_MTHRM0IRQ,
> > +		.irq_en_mask = 0x01,
> > +		.evt_stat = BXTWC_STHRM0IRQ,
> > +		.evt_mask = 0x01,
> > +		.trip_num = 0
> > +	},
> > +	{
> > +		.irq_reg = BXTWC_THRM0IRQ,
> > +		.irq_mask = 0x10,
> > +		.irq_en = BXTWC_MTHRM0IRQ,
> > +		.irq_en_mask = 0x10,
> > +		.evt_stat = BXTWC_STHRM0IRQ,
> > +		.evt_mask = 0x10,
> > +		.trip_num = 1
> > +	}
> > +};
> > +
> > +static struct trip_config_map str1_trip_config[] = {
> > +	{
> > +		.irq_reg = BXTWC_THRM0IRQ,
> > +		.irq_mask = 0x02,
> > +		.irq_en = BXTWC_MTHRM0IRQ,
> > +		.irq_en_mask = 0x02,
> > +		.evt_stat = BXTWC_STHRM0IRQ,
> > +		.evt_mask = 0x02,
> > +		.trip_num = 0
> > +	},
> > +	{
> > +		.irq_reg = BXTWC_THRM0IRQ,
> > +		.irq_mask = 0x20,
> > +		.irq_en = BXTWC_MTHRM0IRQ,
> > +		.irq_en_mask = 0x20,
> > +		.evt_stat = BXTWC_STHRM0IRQ,
> > +		.evt_mask = 0x20,
> > +		.trip_num = 1
> > +	},
> > +};
> > +
> > +static struct trip_config_map str2_trip_config[] = {
> > +	{
> > +		.irq_reg = BXTWC_THRM0IRQ,
> > +		.irq_mask = 0x04,
> > +		.irq_en = BXTWC_MTHRM0IRQ,
> > +		.irq_en_mask = 0x04,
> > +		.evt_stat = BXTWC_STHRM0IRQ,
> > +		.evt_mask = 0x04,
> > +		.trip_num = 0
> > +	},
> > +	{
> > +		.irq_reg = BXTWC_THRM0IRQ,
> > +		.irq_mask = 0x40,
> > +		.irq_en = BXTWC_MTHRM0IRQ,
> > +		.irq_en_mask = 0x40,
> > +		.evt_stat = BXTWC_STHRM0IRQ,
> > +		.evt_mask = 0x40,
> > +		.trip_num = 1
> > +	},
> > +};
> > +
> > +static struct trip_config_map str3_trip_config[] = {
> > +	{
> > +		.irq_reg = BXTWC_THRM2IRQ,
> > +		.irq_mask = 0x10,
> > +		.irq_en = BXTWC_MTHRM2IRQ,
> > +		.irq_en_mask = 0x10,
> > +		.evt_stat = BXTWC_STHRM2IRQ,
> > +		.evt_mask = 0x10,
> > +		.trip_num = 0
> > +	},
> > +};
> 
> I thought I'd told you about this already?  I don't want you to be
> passing though register maps using your own hand-rolled solution.
> 
> Either use Regmap and pass through the pointer, or pass some kind of
> device identifier and choose the correct mapping from within the
> Thermal driver.
> 

I have moved all the register maps to the thermal driver.
Please ignore this patch. Thanks for your review.

-Bin