| Message ID | 20180904102500.30318-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | ASoC: Add support to WCD9335 Audio Codec | expand |
On Wed, 12 Sep 2018, Srinivas Kandagatla wrote: > On 11/09/18 16:33, Lee Jones wrote: > > On Tue, 04 Sep 2018, Srinivas Kandagatla wrote: > > > > > Qualcomm WCD9335 Codec is a standalone Hi-Fi audio codec IC, > > > It has mulitple blocks like Soundwire controller, codec, > > > Codec processing engine, ClassH controller, interrupt mux. > > > It supports both I2S/I2C and SLIMbus audio interfaces. > > > > > > This patch adds support to SLIMbus audio interface. > > > > > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > > Reviewed-by: Vinod Koul <vkoul@kernel.org> > > > --- > > > drivers/mfd/Kconfig | 14 + > > > drivers/mfd/Makefile | 4 + > > > drivers/mfd/wcd9335-core.c | 291 ++++++++++++ > > > include/linux/mfd/wcd9335/registers.h | 627 ++++++++++++++++++++++++++ > > > include/linux/mfd/wcd9335/wcd9335.h | 32 ++ > > > 5 files changed, 968 insertions(+) > > > create mode 100644 drivers/mfd/wcd9335-core.c > > > create mode 100644 include/linux/mfd/wcd9335/registers.h > > > create mode 100644 include/linux/mfd/wcd9335/wcd9335.h [...] > > > +static const struct mfd_cell wcd9335_devices[] = { > > > + { .name = "wcd9335-codec", }, > > > +}; > > > > Are there more devices to come? > > > Yes, that is the plan, we are kind of limited in hardware setup to test few > things like soundwire controller. We are exploring other ways to test these. I normally don't accept MFDs with just one device enabled. Since it's not really an MFD (M == Multi) until it has more than one function. [...] > > > + struct device_node *ifc_dev_np; > > > > ifc isn't very forthcoming. Any way you can improve the name? > ifc was suggested in dt bindings by Rob, I can proably rename to > interface_node. ifc is a horrible variable name - just sayin'. [...] > > > + ret = wcd9335_bring_up(wcd); > > > > So the device_status call-back brings up the hardware? > > > > device status reports the device status at runtime. We can not communicate > with the device until it is up, enumerated by slimbus and a logical address > is assigned to it. So the best place to initialize it is in status callback > where all the above are expected to be done. Right, I understand what's happening. I just think the semantics are wrong. The Subsystem (I'm assuming it's a Subsystem) requests for status and it ends up initiating a start-up sequence. Just from a purist's point of view (I understand that it "works"), it's not good practice. > Probe is expected to setup the external configurations like regulators/pins > and so on which gets the device out of reset and ready to be enumerated by > the slimbus controller. I suggest fully starting the device in probe() is a better approach. [...] > > > +struct wcd9335 { > > > + int version; > > > + int intr1; > > > > What's this? If I have to ask, it's probably not a good name. > > > This is a hardware pin name for interrupt line 1. I don't see how this is used, so it's difficult for me to advise fully, but I find this confusing. Pin name/number? Shouldn't this be handed by Pinctrl? intr1 could be quite ambiguous. Especually as the '1' could easily be read as an 'l'. Suggest that 'irq1' or 'irq_1' or 'irq_one'. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Wed, 12 Sep 2018, Srinivas Kandagatla wrote: > > > On 12/09/18 09:58, Lee Jones wrote: > > > > > +static const struct mfd_cell wcd9335_devices[] = { > > > > > + { .name = "wcd9335-codec", }, > > > > > +}; > > > > Are there more devices to come? > > > > > > > Yes, that is the plan, we are kind of limited in hardware setup to test few > > > things like soundwire controller. We are exploring other ways to test these. > > I normally don't accept MFDs with just one device enabled. Since it's > > not really an MFD (M == Multi) until it has more than one function. > > > > WCD9335 Codec hw itself has multiple hw blocks. > > If the issue is about adding more entries to mfd cells then we should be > able to add below entry: > > { .name = "wcd9335-soundwire-controller", }, > > Actual driver for soundwire controller is not something We can test with > regular dragon boards, it needs special hw for smart speakers. Once we have > that we can test and post the drivers for that. > > Otherwise > > Are you suggesting that I move everything to sound/soc/codecs and then back > to mfd once soundwire controller driver is added? My preference would be for you to add at least one other (tested) device. However, in your case I know where you live, so I can throw tomatoes at your house if you don't upstream more device support promptly! ;) When will you be enabling more devices? If the answer is 'never', then creating an MFD is a waste of time. > > [...] > > > > > > > + struct device_node *ifc_dev_np; > > > > ifc isn't very forthcoming. Any way you can improve the name? > > > ifc was suggested in dt bindings by Rob, I can proably rename to > > > interface_node. > > ifc is a horrible variable name - just sayin'. > > > > [...] > > > > > > > + ret = wcd9335_bring_up(wcd); > > > > So the device_status call-back brings up the hardware? > > > > > > > device status reports the device status at runtime. We can not communicate > > > with the device until it is up, enumerated by slimbus and a logical address > > > is assigned to it. So the best place to initialize it is in status callback > > > where all the above are expected to be done. > > Right, I understand what's happening. I just think the semantics are > > wrong. The Subsystem (I'm assuming it's a Subsystem) requests for > > status and it ends up initiating a start-up sequence. Just from a > > purist's point of view (I understand that it "works"), it's not good > > practice. > > > > > Probe is expected to setup the external configurations like regulators/pins > > > and so on which gets the device out of reset and ready to be enumerated by > > > the slimbus controller. > > I suggest fully starting the device in probe() is a better approach. > > > Its catch-22 situation, without device being powered up and reset correctly > there is no way to enumerate it. Isn't power-up and reset also done in probe()? What am I missing? > > [...] > > > > > > > +struct wcd9335 { > > > > > + int version; > > > > > + int intr1; > > > > What's this? If I have to ask, it's probably not a good name. > > > > > > > This is a hardware pin name for interrupt line 1. > > I don't see how this is used, so it's difficult for me to advise > > fully, but I find this confusing. Pin name/number? Shouldn't this be > > handed by Pinctrl? > > > This is represented as proper irq line in dt via pintrl irq controller. So why can't it just be 'irq' like most of the time? What is the 1 in reference to? Will there be a 2? > > intr1 could be quite ambiguous. Especually as the '1' could easily be > > read as an 'l'. Suggest that 'irq1' or 'irq_1' or 'irq_one'. > > I can change this to something more readable in next version or may be I can > even remove it may be just use a local variable. If it's possible for it to be a local variable, then it should not be placed in device data. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Wed, 12 Sep 2018, Srinivas Kandagatla wrote: > On 12/09/18 11:59, Lee Jones wrote: > > > On 12/09/18 09:58, Lee Jones wrote: > > > > > > > +static const struct mfd_cell wcd9335_devices[] = { > > > > > > > + { .name = "wcd9335-codec", }, > > > > > > > +}; > > > > > > Are there more devices to come? > > > > > > > > > > > Yes, that is the plan, we are kind of limited in hardware setup to test few > > > > > things like soundwire controller. We are exploring other ways to test these. > > > > I normally don't accept MFDs with just one device enabled. Since it's > > > > not really an MFD (M == Multi) until it has more than one function. > > > > > > > WCD9335 Codec hw itself has multiple hw blocks. > > > > > > If the issue is about adding more entries to mfd cells then we should be > > > able to add below entry: > > > > > > { .name = "wcd9335-soundwire-controller", }, > > > > > > Actual driver for soundwire controller is not something We can test with > > > regular dragon boards, it needs special hw for smart speakers. Once we have > > > that we can test and post the drivers for that. > > > > > > Otherwise > > > > > > Are you suggesting that I move everything to sound/soc/codecs and then back > > > to mfd once soundwire controller driver is added? > > My preference would be for you to add at least one other (tested) > > device. However, in your case I know where you live, so I can throw > > tomatoes at your house if you don't upstream more device support > > promptly!;) > > > > When will you be enabling more devices? If the answer is 'never', > > then creating an MFD is a waste of time. > > Vinod Koul is exploring this and ATM we are trying to sort out the hw setup. > Hopefully we should be sorted with Qcom help! Okay. Please keep me posted. > > > > > > > + ret = wcd9335_bring_up(wcd); > > > > > > So the device_status call-back brings up the hardware? > > > > > > > > > > > device status reports the device status at runtime. We can not communicate > > > > > with the device until it is up, enumerated by slimbus and a logical address > > > > > is assigned to it. So the best place to initialize it is in status callback > > > > > where all the above are expected to be done. > > > > Right, I understand what's happening. I just think the semantics are > > > > wrong. The Subsystem (I'm assuming it's a Subsystem) requests for > > > > status and it ends up initiating a start-up sequence. Just from a > > > > purist's point of view (I understand that it "works"), it's not good > > > > practice. > > > > > > > > > Probe is expected to setup the external configurations like regulators/pins > > > > > and so on which gets the device out of reset and ready to be enumerated by > > > > > the slimbus controller. > > > > I suggest fully starting the device in probe() is a better approach. > > > > > > > Its catch-22 situation, without device being powered up and reset correctly > > > there is no way to enumerate it. > > Isn't power-up and reset also done in probe()? > > > > What am I missing? > > There are two parts for device to be ready to talk at bus level: > 1> power up and reset, > 2> enumerate and assign a logical address by the slimbus controller. > > First part as you said is already done in probe. > When second part happens when status callback is invoked, that is when the > slimdevice is ready for any kind of communication at bus level. I see. I still think it's hacky to conduct start-up procedures when all the SS requested was status. Perhaps it needs a new API call init()? -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
>> >> There are two parts for device to be ready to talk at bus level: >> 1> power up and reset, >> 2> enumerate and assign a logical address by the slimbus controller. >> >> First part as you said is already done in probe. >> When second part happens when status callback is invoked, that is when the >> slimdevice is ready for any kind of communication at bus level. > > I see. I still think it's hacky to conduct start-up procedures when > all the SS requested was status. Perhaps it needs a new API call > init()? When we added these callbacks the purpose of this was to allow drivers to do specific setup/teardown. AFIAU, even-though if we add init(), SLIMbus would still call it just before or after status which to me is redundant ATM. Its up to slim driver what it exactly whats to do with status, in some cases this can involve setting up the device. >
On Wed, 12 Sep 2018, Srinivas Kandagatla wrote: > > > > > > > There are two parts for device to be ready to talk at bus level: > > > 1> power up and reset, > > > 2> enumerate and assign a logical address by the slimbus controller. > > > > > > First part as you said is already done in probe. > > > When second part happens when status callback is invoked, that is when the > > > slimdevice is ready for any kind of communication at bus level. > > > > I see. I still think it's hacky to conduct start-up procedures when > > all the SS requested was status. Perhaps it needs a new API call > > init()? > > When we added these callbacks the purpose of this was to allow drivers to do > specific setup/teardown. > > AFIAU, What does that mean? > even-though if we add init(), SLIMbus would still call it just before > or after status which to me is redundant ATM. > Its up to slim driver what it exactly whats to do with status, in some cases > this can involve setting up the device. If you say so! ;) -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog