On Fri, Mar 02, 2018 at 01:58:53PM +0100, Hans de Goede wrote: > Hi, > > On 02-03-18 13:18, Mark Brown wrote: > > On Fri, Mar 02, 2018 at 10:32:25AM +0100, Hans de Goede wrote: > > > On 01-03-18 23:35, Hans de Goede wrote: > > > > > > It is not _that_ fragile, but I agree that it would be good to add > > > > a comment about not moving the property-parsing to the codec driver. > > > > > So one other solution which comes to mind here is to move the > > > snd_soc_card_jack_new() call into the codec driver's > > > rt5651_apply_properties() function (conditional on a jack src being > > > set in the properties). > > > > > This would make the machine driver code look like this: > > > > > > props[cnt++] = PROPERTY_ENTRY_... > > > props[cnt++] = PROPERTY_ENTRY_... > > > props[cnt++] = PROPERTY_ENTRY_... > > > props[cnt++] = PROPERTY_ENTRY_... > > > > > ret = device_add_properties(codec->dev, props); > > > if (ret) > > > return ret; > > > > > ret = rt5651_apply_properties(codec); > > > if (ret) > > > return ret; > > > > > Which makes the ordering really clear without needing any > > > comments. > > > > It's still unusual to have something outside the driver trigger property > > parsing, though the EXPORT_SYMBOL_GPL() would make that apparent. > > > > > This will also make the jack-detect device-properties work with > > > devicetree platforms without any changes to their platform code, > > > which currently is not the case since the non Intel platform-code > > > does not call snd_soc_component_set_jack(). > > > > What makes you claim that? Any board with jack detection wired up will > > call that function. Not all boards have that configured of course but > > there's absolutely nothing Intel specific about that interface. > > Right I'm not claiming the interface is Intel specific, what I'm trying > to say is that the need for some code outside of the codec driver > to create the jack means that adding jack-support to DT platforms > cannot be done through just adding DT properties (once this patch > is merged), it will still require platform code changes. > > That is fine, I was just thinking it would be convenient if DT > platforms could lift on the jack-detect work being done for > the rt5651 driver now with just some DT changes and no other > changes required. > > > > Thinking more about this I believe that doing the > > > snd_soc_card_jack_new() call inside the codec driver based on > > > device-properties is a better solution then doing it in the > > > machine driver. > > > > No, that's really not a good idea. Any jacks in the system are part of > > the system specific wiring, they're not something that's intrinsic to > > the CODEC. Even with fairly basic setups you've got options like having > > a headset jack or separate microphone and headphone jacks. > > OK, so if doing the jack creation in the machine-driver / platform > code is by design and you want to keep things that way, then I > assume that what needs changing for v3 of this patch is: > > 1) Split the patch in separate codec + machine-drv patches > 2) Add comments to make the ordering requirements clear to > both the codec- and machine-driver. > > Correct? And split bindings to separate patch please. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
