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

quoted

quoted

quoted

quoted

"Mark" == Mark Brown [off-list ref] 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.

-- 
Bye, Peter Korsgaard
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

SGkgUGV0ZXIsDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogUGV0ZXIgS29yc2dhYXJkIFttYWlsdG86amFjbWV0QGdtYWlsLmNvbV0gT24gQmVoYWxmIE9mIFBldGVyIEtvcnNnYWFyZA0KPiBTZW50OiAyMDE2xOoy1MIzyNUgMTo0Mg0KPiBUbzogTWFyayBCcm93biA8YnJvb25pZUBrZXJuZWwub3JnPg0KPiBDYzogWWFuZywgV2VueW91IDxXZW55b3UuWWFuZ0BhdG1lbC5jb20+OyBMaWFtIEdpcmR3b29kDQo+IDxsZ2lyZHdvb2RAZ21haWwuY29tPjsgUm9iIEhlcnJpbmcgPHJvYmgrZHRAa2VybmVsLm9yZz47IFBhd2VsIE1vbGwNCj4gPHBhd2VsLm1vbGxAYXJtLmNvbT47IElhbiBDYW1wYmVsbCA8aWpjK2RldmljZXRyZWVAaGVsbGlvbi5vcmcudWs+OyBLdW1hcg0KPiBHYWxhIDxnYWxha0Bjb2RlYXVyb3JhLm9yZz47IEtyenlzenRvZiBLb3psb3dza2kNCj4gPGsua296bG93c2tpQHNhbXN1bmcuY29tPjsgSmF2aWVyIE1hcnRpbmV6IENhbmlsbGFzIDxqYXZpZXJAZG93aGlsZTAub3JnPjsNCj4gTGVlIEpvbmVzIDxsZWUuam9uZXNAbGluYXJvLm9yZz47IFBldGVyIEtvcnNnYWFyZCA8amFjbWV0QGdtYWlsLmNvbT47IEZlcnJlLA0KPiBOaWNvbGFzIDxOaWNvbGFzLkZFUlJFQGF0bWVsLmNvbT47IGxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZzsNCj4gbGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZzsgZGV2aWNldHJlZUB2Z2VyLmtlcm5lbC5vcmcNCj4gU3ViamVjdDogUmU6IFtQQVRDSCB2NCAxLzJdIHJlZ3VsYXRvcjogYWN0ODk0NWE6IGFkZCByZWd1bGF0b3IgZHJpdmVyIGZvcg0KPiBBQ1Q4OTQ1QQ0KPiANCj4gPj4+Pj4gIk1hcmsiID09IE1hcmsgQnJvd24gPGJyb29uaWVAa2VybmVsLm9yZz4gd3JpdGVzOg0KPiANCj4gID4gT24gRnJpLCBKYW4gMjksIDIwMTYgYXQgMDE6MjA6MDhBTSArMDAwMCwgWWFuZywgV2VueW91IHdyb3RlOg0KPiAgPj4gPiA+ICtzdGF0aWMgY29uc3Qgc3RydWN0IG9mX2RldmljZV9pZCBhY3Q4OTQ1YV9wbWljX29mX21hdGNoW10gPSB7DQo+ICA+PiA+ID4gKwl7IC5jb21wYXRpYmxlID0gImFjdGl2ZS1zZW1pLGFjdDg5NDVhLXJlZ3VsYXRvciIgfSwNCj4gID4+ID4gPiArCXsgfSwNCj4gID4+ID4gPiArfTsNCj4gID4+ID4gPiArTU9EVUxFX0RFVklDRV9UQUJMRShvZiwgYWN0ODk0NWFfcG1pY19vZl9tYXRjaCk7DQo+IA0KPiAgPj4gPiBUaGlzIHNlZW1zIG1vc3RseSBPSyBidXQgd2h5IGRvIHdlIGhhdmUgYSBjb21wYXRpYmxlIHN0cmluZyBoZXJlIC0NCj4gc2hvdWxkbid0ICA+PiA+IHRoZSBNRkQgYmUgYWJsZSB0byBpbnN0YW50aWF0ZSB0aGUgcmVndWxhdG9yIGZ1bmN0aW9uIHdpdGhvdXQNCj4gbmVlZGluZyB0aGlzPw0KPiANCj4gID4+IEJlY2F1c2UgSSBnb3QgZmVlZGJhY2sgZnJvbSBKYXZpZXIgZm9yIHRoZSBhY3Q4OTQ1YS1jaGFyZ2VyIHBhdGNoZXMgb2YgdGhpcw0KPiBNRkQgc2VyaWVzLCAgPj4gSGUgc2FpZCBtaXNzaW5nIHRoZSBPRiBtYXRjaCB0YWJsZSB3aWxsIGNhdXNlIHRoZSBtb2R1bGUNCj4gYXV0b2xvYWRpbmcgYnJva2VuLg0KPiANCj4gID4+IGh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL3BpcGVybWFpbC9saW51eC1hcm0ta2VybmVsLzIwMTYtSmFudWFyeS8zOTgxMTMuaHRtbA0KPiANCj4gID4+IFdoYXQgZG8geW91IHRoaW5rIGFib3V0IGl0Pw0KPiANCj4gID4gSWYgdGhlbiBkZXZpY2UgaXMgbm90IGJlaW5nIGxvYWRlZCBmcm9tIHRoZSBEVCAoYW5kIGl0IHNob3VsZG4ndCBiZSwgdGhlICA+IGRldmljZQ0KPiBsb29rcyBsaWtlIGl0IHNob3VsZCBiZSBpbnN0YW50aWF0ZWQgZGlyZWN0bHkgYnkgdGhlIE1GRCBhcyBpdCAgPiBjYW4ndCBleGlzdA0KPiBzZXBhcmF0ZWx5IHRvIHRoYXQgTUZEKSBhbiBPRiB0YWJsZSB3aWxsIGRvIG5vdGhpbmcuDQo+IA0KPiBUbyBhZGQgdG8gdGhlIGNvbmZ1c2lvbiwgdGhlIHJlZ3VsYXRvciBwYXJ0IG9mIHRoZSBjaGlwIGlzIGFjdHVhbGx5IGlkZW50aWNhbCB0bw0KPiBhY3Q4ODY1LCBzbyBpdCBjb3VsZCB1c2UgdGhlIGV4aXN0aW5nIHJlZ3VsYXRvciBkcml2ZXIgLyBjb21wYXRpYmxlLCBleGNlcHQgdGhhdCBpdA0KPiBiaW5kcyB0byB0aGUgcGxhdGZvcm0gYnVzIGluc3RlYWQgb2YgaTJjLg0KDQpUaGFuayB5b3UgZm9yIHlvdXIgb3Bpbmlvbi4NCg0KQnV0IEkgdGhpbmsgSXQgaXMgYmV0dGVyIHRvIG1ha2UgaXQgYSBzZXBhcmF0ZSBkcml2ZXIsIHRoZSBkcml2ZXIgaXMgc2ltcGxlci4gDQoNCj4gDQo+IC0tDQo+IEJ5ZSwgUGV0ZXIgS29yc2dhYXJkDQoNCg0KQmVzdCBSZWdhcmRzLA0KV2VueW91IFlhbmcNCg==--To unsubscribe from this list: send the line "unsubscribe devicetree" inthe body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.orgMore majordomo info at  http://vger.kernel.org/majordomo-info.html

On 03.02.2016 11:29, Yang, Wenyou wrote:

Hi Peter,

quoted

-----Original Message-----
From: Peter Korsgaard [mailto:jacmet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] On Behalf Of Peter Korsgaard
Sent: 2016年2月3日 1:42
To: Mark Brown <redacted>
Cc: Yang, Wenyou <redacted>; Liam Girdwood
[off-list ref]; Rob Herring [off-list ref]; Pawel Moll
[off-list ref]; Ian Campbell [off-list ref]; Kumar
Gala [off-list ref]; Krzysztof Kozlowski
[off-list ref]; Javier Martinez Canillas [off-list ref];
Lee Jones [off-list ref]; Peter Korsgaard [off-list ref]; Ferre,
Nicolas [off-list ref]; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XQ@public.gmane.orgorg;
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
ACT8945A

quoted

quoted

quoted

quoted

quoted

"Mark" == Mark Brown [off-list ref] 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html