On Tue, Jul 02, 2019 at 05:37:01PM +0100, Srinivas Kandagatla wrote:
On 02/07/2019 15:44, Mark Brown wrote:
On Tue, Jul 02, 2019 at 09:09:16AM +0100, Srinivas Kandagatla wrote:
- for (i = 0; i < ARRAY_SIZE(cpr_defaults); i++) {
regmap_bulk_write(data->regmap,WCD934X_CODEC_CPR_WR_DATA_0,(u8 *)&cpr_defaults[i].wr_data, 4);regmap_bulk_write(data->regmap,WCD934X_CODEC_CPR_WR_ADDR_0,(u8 *)&cpr_defaults[i].wr_addr, 4);
What is "cpr" and should you be using a regmap patch here? Why is this not with the other default updates? You've got loads of random undocumented sequences like this all through the driver, are they patches or are they things that should be controllable by the user?
It makes sense to have a single default map here, I will do the in next version. And regarding user controllable, I will go thru the list once again in detail and remove user controllable registers.
What is a "default map"? In terms of user controllable stuff it's not just a question of if things are currently user controllable but also if they should be user controllable.
- return of_platform_populate(wcd->dev->of_node, NULL, NULL, wcd->dev);
Why are we doing this?
I will not be using MFD in this instance as most of the resources like interrupts, pins, clks, SoundWire are modeled as proper drivers with their respective subsystems.
This is a driver for a single device, you should be able to instantiate things without requiring binding through DT (and hence support non-DT systems such as those using ACPI).
This gives a advantage of reusing those drivers like SoundWire, pinctrl on other Qualcomm IPs as well! Also I did not wanted to have a custom functions or hooks in the drivers, so platform bus made much sense for me to use here, which can take care of bringing up and tearing down the devices with proper parent child relationship. This will instantiate all the child devices like pinctrl, SoundWire Controller and so on.
Just create platform devices like normal...