Hello Jerome
Thanks for the review
On 4/23/24 20:35, Jerome Brunet wrote:
On Tue 23 Apr 2024 at 19:10, George Stark <gnstark@xxxxxxxxxxxxxxxxx> wrote:
From: George Stark <gnstark@xxxxxxxxxxxxxx>
Add a compatible string and configuration for the meson A1 SoC family
PWM. Additionally, provide an external clock initialization helper
specifically designed for these PWM IPs.
Signed-off-by: George Stark <gnstark@xxxxxxxxxxxxxx>
Signed-off-by: Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxxxxx>
---
drivers/pwm/pwm-meson.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index ea96c5973488..529a541ba7b6 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -462,6 +462,33 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
}
+static int meson_pwm_init_channels_ext_clock(struct pwm_chip *chip)
That kind on naming (ext) is almost sure to clash with whatever comes next.
Just use the name of the first SoC using the method, a1 for instance.
It's true that pwm core in a1 s4, t7 etc is the same AFAWK.
I just want to clarify your proposal:
I add a1 compatible to the dt-bindings with s4 as fallback,
t7 compatible will be added later in the same way.
Here in the driver I don't mention a1 at all and use s4-centric naming e.g.:
{
.compatible = "amlogic,meson-s4-pwm",
.data = &pwm_meson_s4_data
},
static const struct meson_pwm_data pwm_meson_s4_data = {
.channels_init = meson_pwm_init_channels_s4,
};
right?
+{
+ struct device *dev = pwmchip_parent(chip);
+ struct meson_pwm *meson = to_meson_pwm(chip);
+ struct meson_pwm_channel *channels = meson->channels;
+ struct clk_bulk_data *clks = NULL;
+ unsigned int i;
+ int res;
+
+ res = devm_clk_bulk_get_all(dev, &clks);
+ if (res < 0) {
+ dev_err(dev, "can't get device clocks\n");
+ return res;
+ }
I don't think allocating the 'clk_bulk_data *clks' is necessary or safe.
We know exactly how many clocks we expect, there is no need for a get all.
+
+ if (res != MESON_NUM_PWMS) {
+ dev_err(dev, "clock count must be %d, got %d\n",
+ MESON_NUM_PWMS, res);
+ return -EINVAL;
+ }
... and this only catches the problem after the fact.
It is probably convinient but not necessary.
+
+ for (i = 0; i < MESON_NUM_PWMS; i++)
+ channels[i].clk = clks[i].clk;
channels[i].clk could be assigned directly of_clk_get() using clock
indexes. No extra allocation needed.
if we use of_clk_get then we'll have to free the clock objects in the
end. Could we use devm_clk_bulk_get instead?
+
+ return 0;
+}
+
static const struct meson_pwm_data pwm_meson8b_data = {
.parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
.channels_init = meson_pwm_init_channels_meson8b_legacy,
@@ -500,11 +527,19 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
.channels_init = meson_pwm_init_channels_meson8b_v2,
};
+static const struct meson_pwm_data pwm_meson_ext_clock_data = {
+ .channels_init = meson_pwm_init_channels_ext_clock,
+};
+
static const struct of_device_id meson_pwm_matches[] = {
{
.compatible = "amlogic,meson8-pwm-v2",
.data = &pwm_meson8_v2_data
},
+ {
+ .compatible = "amlogic,meson-a1-pwm",
+ .data = &pwm_meson_ext_clock_data
+ },
/* The following compatibles are obsolete */
{
.compatible = "amlogic,meson8b-pwm",
--
Best regards
George
