Re: [PATCH v2 4/9] dt-bindings: mfd: add pf1550

Re: [PATCH v2 4/9] dt-bindings: mfd: add pf1550

[Samuel Kayode]

On Sat, May 17, 2025 at 01:16:38PM +0200, Krzysztof Kozlowski wrote:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  regulators:
> > +    $ref: /schemas/regulator/pf1550.yaml
> > +
> > +  charger:
> > +    $ref: /schemas/power/supply/pf1550_charger.yaml
> > +
> > +  onkey:
> > +    $ref: /schemas/input/pf1550_onkey.yaml
> 
> This makes merging via separate trees not possible...
> 
> Just fold everything here, drop compatibles and then put binding in the
> regulator. Unless children are re-usable which would justify
> compatibles, but then please provide arguments for that.

Yes, compatibles are not needed for the children. For adding binding to the
regulator, did you mean for all children: charger and onkey included? So,
replacing the separate yaml for all children with bindings in the mfd yaml?

Thanks,
Sam

Re: [PATCH v2 4/9] dt-bindings: mfd: add pf1550

[Krzysztof Kozlowski]

On 21/05/2025 18:16, Samuel Kayode wrote:
> On Sat, May 17, 2025 at 01:16:38PM +0200, Krzysztof Kozlowski wrote:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  regulators:
>>> +    $ref: /schemas/regulator/pf1550.yaml
>>> +
>>> +  charger:
>>> +    $ref: /schemas/power/supply/pf1550_charger.yaml
>>> +
>>> +  onkey:
>>> +    $ref: /schemas/input/pf1550_onkey.yaml
>>
>> This makes merging via separate trees not possible...
>>
>> Just fold everything here, drop compatibles and then put binding in the
>> regulator. Unless children are re-usable which would justify
>> compatibles, but then please provide arguments for that.
> 
> Yes, compatibles are not needed for the children. For adding binding to the
> regulator, did you mean for all children: charger and onkey included? So,
> replacing the separate yaml for all children with bindings in the mfd yaml?

Up to you. Onkey looks small so could be folded for sure, but other
bindings are not big either, so I would personally fold everything.
Otherwise you will need to be sure MFD is not applied before they reach
mainline.

Due to lack of testing I did not review charger patch, but just after
quick glance I see already that you miss power supply ref, thus it seems
you duplicate some of the existing properties from monitored-battery.

Best regards,
Krzysztof

Re: [PATCH v2 7/9] input: pf1550: add onkey support

[Samuel Kayode]

On Fri, May 16, 2025 at 03:55:02PM -0700, Dmitry Torokhov wrote:
> > +	input->name = pdev->name;
> > +	input->phys = "pf1550-onkey/input0";
> > +	input->id.bustype = BUS_HOST;
> > +
> > +	input_set_capability(input, EV_KEY, onkey->keycode);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(pf1550_onkey_irqs); i++) {
> > +		struct pf1550_irq_info *onkey_irq =
> > +						&pf1550_onkey_irqs[i];
> > +		unsigned int virq = 0;
> > +
> > +		virq = regmap_irq_get_virq(pf1550->irq_data_onkey,
> > +					   onkey_irq->irq);
> > +		if (!virq)
> > +			return -EINVAL;
> > +
> > +		onkey_irq->virq = virq;
> 
> I think this kind of mapping needs to be done in the core part of your
> driver.
>
Without doing the mapping in the MFD children, a list of all virqs for the PMIC
would have to be maintained in addition to the (regmap_irq) irqs. Perhaps,
there is a better way to implement this?
> > +
> > +		error = devm_request_threaded_irq(&pdev->dev, virq, NULL,
> > +						  pf1550_onkey_irq_handler,
> > +					IRQF_NO_SUSPEND,
> > +					onkey_irq->name, onkey);
Thanks,
Sam