Hi Pavel,
Thanks for your valuable comments. My comments inline.
On 11/5/2020 6:11 PM, Pavel Machek wrote:
Hi!
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ed943140e1fd..6445b39fe4fc 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -886,6 +886,16 @@ config LEDS_SGM3140
This option enables support for the SGM3140 500mA Buck/Boost Charge
Pump LED Driver.
+config LEDS_LGM_SSO
+ tristate "LED support for Intel LGM SOC series"
+ depends on LEDS_CLASS
+ depends on MFD_SYSCON
+ depends on OF
+ help
+ Parallel to serial conversion, which is also called SSO controller,
+ can drive external shift register for LED outputs.
+ This enables LED support for Serial Shift Output Controller(SSO).
Something is wrong with indentation here.
Seems tabbing.. i will fix it.
diff --git a/drivers/leds/leds-lgm-sso.c b/drivers/leds/leds-lgm-sso.c
Could we put it into drivers/leds/blink/ directory? You'll need to
create it.
Sure, i will update in the next patch.
index 000000000000..f1bae1c6ed3c
--- /dev/null
+++ b/drivers/leds/leds-lgm-sso.c
@@ -0,0 +1,881 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel LGM Soc LED SSO driver
Spell out LGM, SSO. Soc->SoC.
Pointer to documentation would be welcome here.
Public documentation is not available.
+enum {
+ US_SW = 0,
+ US_GPTC = 1,
+ US_FPID = 2
+};
This is not really useful without additional comments.
ok, i will update the additional comments.
+static u32 sso_rectify_brightness(u32 brightness)
+{
+ if (brightness > LED_FULL)
+ return LED_FULL;
+ else
+ return brightness;
+}
Why?
As per below review comments if we use "default-state" property, it will
be redundant.
+static int sso_rectify_blink_rate(struct sso_led_priv *priv, u32 rate)
+{
+ int i;
+
+ for (i = 0; i < MAX_FREQ_RANK; i++) {
+ if (rate <= priv->freq[i])
+ return i;
+ }
+
+ return i - 1;
+}
Can return -1. Is that expected?
It return the frequency index, if 'rate' is not matching with
'priv->freq' it will return maximum index.
In case of return -1 need to adjust the code in the function from where
it is called.
func name 'sso_get_blink_rate_idx' is more appropriate i think.
+
+ desc->np = to_of_node(fwnode_child);
+ if (fwnode_property_read_string(fwnode_child, "label",
+ &desc->name)) {
+ dev_err(dev, "LED no label name!\n");
+ goto __dt_err;
+ }
Can you use appropriate helper from the core? labels are getting
deprecated...
Agree.
+ if (fwnode_property_present(fwnode_child,
+ "retain-state-suspended"))
+ desc->retain_state_suspended = 1;
Was this documented in the binding?
No, i will udpate.
+ if (fwnode_property_read_u32(fwnode_child, "intel,led-pin",
+ &prop)) {
+ dev_err(dev, "Failed to find led pin id!\n");
+ goto __dt_err;
Would not we normally use something like reg = <x> to indicate pin?
Yes, we can do that. i will update in the next patch.
+ if (fwnode_property_present(fwnode_child,
+ "intel,sso-hw-trigger"))
+ desc->hw_trig = 1;
Should not that be selectable on runtime?
Agree, i will fix this.
+ if (fwnode_property_read_u32(fwnode_child,
+ "intel,sso-brightness", &prop))
+ desc->brightness = priv->brightness;
+ else
+ desc->brightness = sso_rectify_brightness(prop);
Can you look at "default-state" property?
sure. i will update with "default-state" property
+ ret = sso_gpio_gc_init(dev, priv);
+ if (ret)
+ return ret;
+
+ return 0;
+}
Just return ret.
ok.
+
+ ret = clk_prepare_enable(priv->gclk);
+ if (ret) {
+ dev_err(dev, "Failed to prepate/enable sso gate clock!\n");
+ return ret;
+ }
+
+ priv->fpid_clk = devm_clk_get(dev, "fpid");
+ if (IS_ERR(priv->fpid_clk)) {
+ dev_err(dev, "Failed to get fpid clock!\n");
+ return PTR_ERR(priv->fpid_clk);
+ }
clk disable here?
ok. i will use devm_add_action_or_reset to disable clocks.
+ ret = clk_prepare_enable(priv->fpid_clk);
+ if (ret) {
+ dev_err(dev, "Failed to prepare/enable fpid clock!\n");
+ return ret;
+ }
+ priv->fpid_clkrate = clk_get_rate(priv->fpid_clk);
+
+ priv->mmap = syscon_node_to_regmap(dev->of_node);
+ if (IS_ERR(priv->mmap)) {
+ dev_err(dev, "Failed to map iomem!\n");
+ return PTR_ERR(priv->mmap);
+ }
clk disable here? ... and probably elsewhere?
ok. i will use devm_add_action_or_reset to disable clocks.
Best regards,
Pavel
