On 10/08/2013 09:39 AM, Laxman Dewangan wrote: > Thanks Nishanth for review. > > On Tuesday 08 October 2013 06:59 PM, Nishanth Menon wrote: >> On 10/08/2013 08:21 AM, Laxman Dewangan wrote: >>> Palmas devices has two clock output CLK32K_KG and CLK32K_KG_AUDIO >> not all palmas devices have 2 clocks - example: tps659038 > > This is for generic palmas and I have seen it for TPS65913, TPS65914, > TPS80036. If the generic one is not compatible then it need to add > device specific and at that time, it is require to update the binding > document accordingly. ?? you do have two clocks inside the device they should be represented as two compatible entities - that simplifies everyone's life. > >> | 7 + >> drivers/clk/Makefile | 1 + >> drivers/clk/clk-palmas.c | 340 ++++++++++++++++++++ >> >> http://www.spinics.net/lists/devicetree/msg04855.html >> Do we do 2 patches now? one seperate for binding and implementation? >> What is our current preference now a days? > > Currently it is implementation + binding doc in one patch. > >> >>> Palmas device has two clock output pins for 32KHz, KG and KG_AUDIO. >>> + >>> +This binding uses the common clock binding ./clock-bindings.txt. >> proper link would be to provide >> Documentation/devicetree/bindings/clock/clock-bindings.txt ? > > Hmm, other patch I got feedback from DT maintainers to do not use the > absolute path as document directory may change > >>> + >>> +Clock 32KHz KG is output 0 of the driver and clock 32KHz is output 1. >>> + >>> +Required properties: >>> +- compatible : shall be "ti,palmas-clk". >> To handle variants of Palmas chips in production, you'd want to be >> specific here clk32k_kg and clk32k_kg_audio. > > The compatible is the device sub module level, not the clock level. Same > thing we are following on regulators. not exactly the same problem as regulator IMHO here. > > >> >>> + >>> + Optional subnode properties: >>> + ti,clock-boot-enable: Enable clock at the time of booting. >> Dumb question: Why is this needed? should'nt relevant drivers do a >> clk_get to enable the relevant clocks? > > If some board needs this clock to be always available for rest of system > to work without any specific driver then this flag is useful. that is the wrong way of using this. > > >> >>> + ti,external-sleep-control: The clock is enable/disabled by state >>> + of external enable input pins ENABLE, ENABLE2 and NSLEEP. >>> + The valid value for the external pins are: >>> + 1 for ENABLE1 >>> + 2 for ENABLE2 >>> + 3 for NSLEEP. >> could we not have macros for readability? > > I am thinking of adding the palmas for dt-binding and then change on > multiple places. I will post patches for this. > the patch will go on dt tree as include/dt-bindings and then on > documents file and then on actually DTS. I will work towards this but > scoping out of this patch. > > why not do it here? and provide explanation - we dont want to deal with backward compatible dtbs etc later on. >> >>> + >>> + >>> +enum PALMAS_CLOCK32K { >>> + PALMAS_CLOCK32KG, >>> + PALMAS_CLOCK32KG_AUDIO, >>> + >>> + /* Last entry */ >>> + PALMAS_CLOCK32K_NR, >>> +}; >> you should be able to get rid of this entirely > > Probably yes but it is easy to read (atleast for me). you can get rid of it entirely by using appropriate matches. > > >> >> + cinfo->clk = clk; >> + palmas_clks->clk_data.clks[i] = clk; >> + palmas_clks->clk_data.clk_num++; >> + palmas_clks_init_configure(cinfo); >> we dont handle error here? > > Intentionally I ignore error, just print and continue the registration. not acceptable: since the failure indicates setups are broken, adding a provider is not valid even, sorry, NAK as a result. > > >> >>> + >>> +MODULE_AUTHOR("Laxman Dewangan <ldewangan@xxxxxxxxxx>"); >>> +MODULE_LICENSE("GPL v2"); >>> >> I wonder if we can simplify this with CLK_OF_DECLARE - I suppose it >> wont work if of_clk_init(NULL); was invoked previously. > > This driver has dependency over the mfd driver and hence until mfd > driver invoked and get registered, this driver should not be called. The > platform driver registration is done in mfd. > As per my understanding and referring the other code, CLK_OF_DECLARE is > useful if there is no such dependency. Please correct me if this is not > true. > that is what i was wondering - since it is a clock source.... -- Regards, Nishanth Menon -- 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
