Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Krzysztof Kozlowski]

On 03.02.2016 11:29, Yang, Wenyou wrote:
> Hi Peter,
> 
>> -----Original Message-----
>> From: Peter Korsgaard [mailto:jacmet@xxxxxxxxx] On Behalf Of Peter Korsgaard
>> Sent: 2016å2æ3æ 1:42
>> To: Mark Brown <broonie@xxxxxxxxxx>
>> Cc: Yang, Wenyou <Wenyou.Yang@xxxxxxxxx>; Liam Girdwood
>> <lgirdwood@xxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Pawel Moll
>> <pawel.moll@xxxxxxx>; Ian Campbell <ijc+devicetree@xxxxxxxxxxxxxx>; Kumar
>> Gala <galak@xxxxxxxxxxxxxx>; Krzysztof Kozlowski
>> <k.kozlowski@xxxxxxxxxxx>; Javier Martinez Canillas <javier@xxxxxxxxxxxx>;
>> Lee Jones <lee.jones@xxxxxxxxxx>; Peter Korsgaard <jacmet@xxxxxxxxx>; Ferre,
>> Nicolas <Nicolas.FERRE@xxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
>> ACT8945A
>>
>>>>>>> "Mark" == Mark Brown <broonie@xxxxxxxxxx> writes:
>>
>>  > On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:
>>  >> > > +static const struct of_device_id act8945a_pmic_of_match[] = {
>>  >> > > +	{ .compatible = "active-semi,act8945a-regulator" },
>>  >> > > +	{ },
>>  >> > > +};
>>  >> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
>>
>>  >> > This seems mostly OK but why do we have a compatible string here -
>> shouldn't  >> > the MFD be able to instantiate the regulator function without
>> needing this?
>>
>>  >> Because I got feedback from Javier for the act8945a-charger patches of this
>> MFD series,  >> He said missing the OF match table will cause the module
>> autoloading broken.
>>
>>  >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html
>>
>>  >> What do you think about it?
>>
>>  > If then device is not being loaded from the DT (and it shouldn't be, the  > device
>> looks like it should be instantiated directly by the MFD as it  > can't exist
>> separately to that MFD) an OF table will do nothing.
>>
>> To add to the confusion, the regulator part of the chip is actually identical to
>> act8865, so it could use the existing regulator driver / compatible, except that it
>> binds to the platform bus instead of i2c.
> 
> Thank you for your opinion.
> 
> But I think It is better to make it a separate driver, the driver is simpler. 

Do I understand correctly that you are creating a new device driver for
the same logical device (regulator block)?

By duplicating the code you are not making kernel simpler. Maybe your
new driver will be simple but still this adds a NEW driver instead of
re-using existing code.

Best regards,
Krzysztof