Re: [PATCH V1 10/15] spmi: pmic_arb: add support for PMIC bus arbiter v3

[kgunda@codeaurora.org]

On 2017-06-01 03:48, Stephen Boyd wrote:
> On 05/30, Kiran Gunda wrote:
>> 
>>  /* PMIC Arbiter channel registers offsets */
>> @@ -96,6 +97,17 @@ enum pmic_arb_cmd_op_code {
>>  /* interrupt enable bit */
>>  #define SPMI_PIC_ACC_ENABLE_BIT		BIT(0)
>> 
>> +#define HWIRQ(slave_id, periph_id, irq_id, apid) \
>> +	((((slave_id) & 0xF)   << 28) | \
>> +	(((periph_id) & 0xFF)  << 20) | \
>> +	(((irq_id)    & 0x7)   << 16) | \
>> +	(((apid)      & 0x1FF) << 0))
>> +
>> +#define HWIRQ_SID(hwirq)  (((hwirq) >> 28) & 0xF)
>> +#define HWIRQ_PER(hwirq)  (((hwirq) >> 20) & 0xFF)
>> +#define HWIRQ_IRQ(hwirq)  (((hwirq) >> 16) & 0x7)
>> +#define HWIRQ_APID(hwirq) (((hwirq) >> 0)  & 0x1FF)
> 
> How about lowercase and hwirq_to_*() macros? Then it's more
> function like style. And spec_to_hwirq() or something?
> 
Sure. Will change it in the follow up patch.
>> +
>>  struct pmic_arb_ver_ops;
>> 
>>  struct apid_data {
>> @@ -151,7 +163,9 @@ struct spmi_pmic_arb {
>>  /**
>>   * pmic_arb_ver: version dependent functionality.
>>   *
>> - * @mode:	access rights to specified pmic peripheral.
>> + * @ver_str:		version string.
>> + * @ppid_to_apid:	finds the apid for a given ppid.
>> + * @mode:		access rights to specified pmic peripheral.
> 
> mode didn't need to change tabbing. Not sure why it's appearing
> in the diff.
> 
When adding the new field description added extra tab for mode for 
alignment.
That's why it is showing up.
>>   * @non_data_cmd:	on v1 issues an spmi non-data command.
>>   *			on v2 no HW support, returns -EOPNOTSUPP.
>>   * @offset:		on v1 offset of per-ee channel.
>> @@ -177,10 +192,10 @@ struct pmic_arb_ver_ops {
>>  	u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
>>  	int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
>>  	/* Interrupts controller functionality (offset of PIC registers) */
>> -	u32 (*owner_acc_status)(u8 m, u8 n);
>> -	u32 (*acc_enable)(u8 n);
>> -	u32 (*irq_status)(u8 n);
>> -	u32 (*irq_clear)(u8 n);
>> +	u32 (*owner_acc_status)(u8 m, u16 n);
>> +	u32 (*acc_enable)(u16 n);
>> +	u32 (*irq_status)(u16 n);
>> +	u32 (*irq_clear)(u16 n);
>>  };
>> 
>>  static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
>> @@ -776,7 +787,7 @@ static int qpnpint_irq_domain_map(struct 
>> irq_domain *d,
>>  }
>> 
>>  static int
>> -pmic_arb_mode_v1(struct spmi_pmic_arb *pa, u8 sid, u16 addr, mode_t 
>> *mode)
>> +pmic_arb_mode_v1_v3(struct spmi_pmic_arb *pa, u8 sid, u16 addr, 
>> mode_t *mode)
> 
> Nice to know that the mode became useless in v3 and beyond! Sigh.
> 
>>  {
>>  	*mode = S_IRUSR | S_IWUSR;
>>  	return 0;
>> @@ -987,21 +1017,21 @@ static int spmi_pmic_arb_probe(struct 
>> platform_device *pdev)
>> 
>>  		/* the apid to ppid table starts at PMIC_ARB_REG_CHNL(0) */
>> -		pa->max_periph =  (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4;
>> +		pa->max_periph = (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4;
> 
> This looks unrelated?
> 
Yes. Just removed the extra space after '='.
>> 
>>  		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>  						   "obsrvr");