Hi Robert, On 2019-02-21 21:42, Robert Shearman wrote: *snip* >>> index bfabf985e830..c032396abdcc 100644 >>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c >>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c >>> @@ -50,6 +50,7 @@ >>> #include <linux/pm.h> >>> #include <linux/slab.h> >>> #include <linux/spinlock.h> >>> +#include <dt-bindings/mux/mux.h> >> >> It is a bit questionable to borrow the include from an unrelated subsystem just >> because it happens to match the current need. Not sure if this is good? > > Ok, will add local definitions then. I had a change of heart - we can always fork later. But go with local defs if you have already done it and don't want to go back. Sorry for the ambivalence... *snip* >>> + /* >>> + * Some channels may be deselected, but this is safer >>> + * than claiming idle-disconnect behaviour when >>> + * inspected. >>> + */ >>> + data->idle_state = MUX_IDLE_AS_IS; >> >> Oooh, this is lying and can thoroughly confuse unsuspecting users. It would be >> better with a "mixed" state or perhaps with a per channel idle attribute. I don't >> know, but this I do not like. >> >> But are there even any users out there that makes use of the per-channel >> disconnect feature? I find none, so it should be possible to simply nuke the >> platform data include (include/linux/platform_data/pca954x.h) since it is >> only used by i2c-mux-pca9541.c and i2c-mux-pca954x.c. Without that, there is >> no way to have per-channel differences. >> >> Are you willing to work on that? > > Absolutely - that makes things simpler and makes these changes cleaner. Looking forward to the next iteration, thanks! Cheers, Peter
