[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 at gmail.com] On Behalf Of Peter Korsgaard
>> Sent: 2016年2月3日 1:42
>> To: Mark Brown <broonie at kernel.org>
>> Cc: Yang, Wenyou <Wenyou.Yang at atmel.com>; Liam Girdwood
>> <lgirdwood at gmail.com>; Rob Herring <robh+dt at kernel.org>; Pawel Moll
>> <pawel.moll at arm.com>; Ian Campbell <ijc+devicetree at hellion.org.uk>; Kumar
>> Gala <galak at codeaurora.org>; Krzysztof Kozlowski
>> <k.kozlowski at samsung.com>; Javier Martinez Canillas <javier at dowhile0.org>;
>> Lee Jones <lee.jones at linaro.org>; Peter Korsgaard <jacmet at gmail.com>; Ferre,
>> Nicolas <Nicolas.FERRE at atmel.com>; linux-arm-kernel at lists.infradead.org;
>> linux-kernel at vger.kernel.org; devicetree at vger.kernel.org
>> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
>> ACT8945A
>>
>>>>>>> "Mark" == Mark Brown <broonie at kernel.org> 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