From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f53.google.com (mail-pj1-f53.google.com [209.85.216.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0DEE31867 for ; Wed, 20 Apr 2022 17:55:38 +0000 (UTC) Received: by mail-pj1-f53.google.com with SMTP id ll10so2627770pjb.5 for ; Wed, 20 Apr 2022 10:55:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=W7Xq3UJH3qMZxLkQAcnpRNWA6G5CMmkPBBfWKoowgzU=; b=Ajyn322Z12V0rJvJXWWTcaLARY63PnCr9NVHtOH9xff2nsXP6otqyeYCAHxYEaEYbO icygtpIBozeNkVBsB8+OwixJl9JIZi5FuSuSAM7Q6Nls/zg029eY8zD9nP0SbhZ0AFGv JcnrMuzM7W2v4iYa+ZPyPpJJ5ca0lpVRlVNNM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=W7Xq3UJH3qMZxLkQAcnpRNWA6G5CMmkPBBfWKoowgzU=; b=GHOXetIF74tmWcNno1DFUcgywpSIouXL5kODsPxiKjIkW1yzWZWfvOX8OwT2B3NL+M k+WyoDIheQt7803zxW3Ntva8F3/nMTUFDL6a+6jknbzUFtJOzL8fzkGraHuHlFgtDJIa 0MRW+5a2hWjOSOBlYg7sOMlR6lStIrJXrxhHmRxz8Pod6hbeJO357JGLGxpXLiEuWbgC em9rQqyh4HJRntNzpGQOJo4WPUvK+NJWjPX9+dnoUPr/Wsov9JrNf9eTW2RSUEarSQ3V yOTouOINk4a43xR5hACyQ3lZvROCw3hQLK00e6IAu1SHD7GNwzIWitnZjec8o46ONgQZ OGCA== X-Gm-Message-State: AOAM531Rra7QF05+fm0A7eVZobFGO5xslgACR425kUzXgymUk8lYT1mj oKBhYblwwUM4Cmg/ag/oewHQ5A== X-Google-Smtp-Source: ABdhPJwGdoo2DKbawMenjEAKnCXvHj/MNoEW31/sQtannMm66Fj+1WjrwM+hRvkFHK7zTZbAk2V8UA== X-Received: by 2002:a17:902:7c0f:b0:157:962:c184 with SMTP id x15-20020a1709027c0f00b001570962c184mr21635232pll.111.1650477338373; Wed, 20 Apr 2022 10:55:38 -0700 (PDT) Received: from chromium.org (164.135.233.35.bc.googleusercontent.com. [35.233.135.164]) by smtp.gmail.com with ESMTPSA id ay33-20020a056a00302100b00508374700b9sm19618740pfb.166.2022.04.20.10.55.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 20 Apr 2022 10:55:37 -0700 (PDT) Date: Wed, 20 Apr 2022 17:55:35 +0000 From: Prashant Malani To: Fabio Baltieri Cc: Benson Leung , Guenter Roeck , Thierry Reding , Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= , Lee Jones , Rob Herring , chrome-platform@lists.linux.dev, linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Tzung-Bi Shih Subject: Re: [PATCH v5 2/4] pwm: pwm-cros-ec: add channel type support Message-ID: References: <20220420141556.681212-1-fabiobaltieri@chromium.org> <20220420141556.681212-3-fabiobaltieri@chromium.org> Precedence: bulk X-Mailing-List: chrome-platform@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220420141556.681212-3-fabiobaltieri@chromium.org> On Apr 20 14:15, Fabio Baltieri wrote: > Add support for EC_PWM_TYPE_DISPLAY_LIGHT and EC_PWM_TYPE_KB_LIGHT pwm > types to the PWM cros_ec_pwm driver. This allows specifying one of these > PWM channel by functionality, and let the EC firmware pick the correct > channel, thus abstracting the hardware implementation from the kernel > driver. > > To use it, define the node with the "google,cros-ec-pwm-type" > compatible. > > Signed-off-by: Fabio Baltieri > Reviewed-by: Tzung-Bi Shih > --- > drivers/pwm/pwm-cros-ec.c | 82 ++++++++++++++++++++++++++++++++------- > 1 file changed, 67 insertions(+), 15 deletions(-) > > diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c > index 5e29d9c682c3..7f10f56c3eb6 100644 > --- a/drivers/pwm/pwm-cros-ec.c > +++ b/drivers/pwm/pwm-cros-ec.c > @@ -12,17 +12,21 @@ > #include > #include > > +#include > + > /** > * struct cros_ec_pwm_device - Driver data for EC PWM > * > * @dev: Device node > * @ec: Pointer to EC device > * @chip: PWM controller chip > + * @use_pwm_type: Use PWM types instead of generic channels > */ > struct cros_ec_pwm_device { > struct device *dev; > struct cros_ec_device *ec; > struct pwm_chip chip; > + bool use_pwm_type; > }; > > /** > @@ -58,14 +62,31 @@ static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > kfree(channel); > } > > -static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty) > +static int cros_ec_dt_type_to_pwm_type(u8 dt_index, u8 *pwm_type) > { > + switch (dt_index) { > + case CROS_EC_PWM_DT_KB_LIGHT: > + *pwm_type = EC_PWM_TYPE_KB_LIGHT; > + return 0; > + case CROS_EC_PWM_DT_DISPLAY_LIGHT: > + *pwm_type = EC_PWM_TYPE_DISPLAY_LIGHT; > + return 0; > + default: > + return -EINVAL; > + } > +} > + > +static int cros_ec_pwm_set_duty(struct cros_ec_pwm_device *ec_pwm, u8 index, > + u16 duty) > +{ > + struct cros_ec_device *ec = ec_pwm->ec; > struct { > struct cros_ec_command msg; > struct ec_params_pwm_set_duty params; > } __packed buf; > struct ec_params_pwm_set_duty *params = &buf.params; > struct cros_ec_command *msg = &buf.msg; > + int ret; > > memset(&buf, 0, sizeof(buf)); > > @@ -75,14 +96,25 @@ static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty) > msg->outsize = sizeof(*params); > > params->duty = duty; > - params->pwm_type = EC_PWM_TYPE_GENERIC; > - params->index = index; > + > + if (ec_pwm->use_pwm_type) { > + ret = cros_ec_dt_type_to_pwm_type(index, ¶ms->pwm_type); > + if (ret) { > + dev_err(ec->dev, "Invalid PWM type index: %d\n", index); > + return ret; > + } > + params->index = 0; > + } else { > + params->pwm_type = EC_PWM_TYPE_GENERIC; > + params->index = index; > + } > > return cros_ec_cmd_xfer_status(ec, msg); > } > > -static int cros_ec_pwm_get_duty(struct cros_ec_device *ec, u8 index) > +static int cros_ec_pwm_get_duty(struct cros_ec_pwm_device *ec_pwm, u8 index) > { > + struct cros_ec_device *ec = ec_pwm->ec; > struct { > struct cros_ec_command msg; > union { > @@ -102,8 +134,17 @@ static int cros_ec_pwm_get_duty(struct cros_ec_device *ec, u8 index) > msg->insize = sizeof(*resp); > msg->outsize = sizeof(*params); > > - params->pwm_type = EC_PWM_TYPE_GENERIC; > - params->index = index; > + if (ec_pwm->use_pwm_type) { > + ret = cros_ec_dt_type_to_pwm_type(index, ¶ms->pwm_type); > + if (ret) { > + dev_err(ec->dev, "Invalid PWM type index: %d\n", index); > + return ret; > + } > + params->index = 0; > + } else { > + params->pwm_type = EC_PWM_TYPE_GENERIC; > + params->index = index; > + } > > ret = cros_ec_cmd_xfer_status(ec, msg); > if (ret < 0) > @@ -133,7 +174,7 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > */ > duty_cycle = state->enabled ? state->duty_cycle : 0; > > - ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle); > + ret = cros_ec_pwm_set_duty(ec_pwm, pwm->hwpwm, duty_cycle); > if (ret < 0) > return ret; > > @@ -149,7 +190,7 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); > int ret; > > - ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm); > + ret = cros_ec_pwm_get_duty(ec_pwm, pwm->hwpwm); > if (ret < 0) { > dev_err(chip->dev, "error getting initial duty: %d\n", ret); > return; > @@ -204,13 +245,13 @@ static const struct pwm_ops cros_ec_pwm_ops = { > * of PWMs it supports directly, so we have to read the pwm duty cycle for > * subsequent channels until we get an error. > */ > -static int cros_ec_num_pwms(struct cros_ec_device *ec) > +static int cros_ec_num_pwms(struct cros_ec_pwm_device *ec_pwm) > { > int i, ret; > > /* The index field is only 8 bits */ > for (i = 0; i <= U8_MAX; i++) { > - ret = cros_ec_pwm_get_duty(ec, i); > + ret = cros_ec_pwm_get_duty(ec_pwm, i); > /* > * We look for SUCCESS, INVALID_COMMAND, or INVALID_PARAM > * responses; everything else is treated as an error. > @@ -236,6 +277,7 @@ static int cros_ec_pwm_probe(struct platform_device *pdev) > { > struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent); > struct device *dev = &pdev->dev; > + struct device_node *np = pdev->dev.of_node; > struct cros_ec_pwm_device *ec_pwm; > struct pwm_chip *chip; > int ret; > @@ -251,17 +293,26 @@ static int cros_ec_pwm_probe(struct platform_device *pdev) > chip = &ec_pwm->chip; > ec_pwm->ec = ec; > > + if (of_device_is_compatible(np, "google,cros-ec-pwm-type")) > + ec_pwm->use_pwm_type = true; Isn't it possible to just use an optional boolean property (for example: "use-pwm-type") instead of defining a new compatible string? > + > /* PWM chip */ > chip->dev = dev; > chip->ops = &cros_ec_pwm_ops; > chip->of_xlate = cros_ec_pwm_xlate; > chip->of_pwm_n_cells = 1; > - ret = cros_ec_num_pwms(ec); > - if (ret < 0) { > - dev_err(dev, "Couldn't find PWMs: %d\n", ret); > - return ret; > + > + if (ec_pwm->use_pwm_type) { > + chip->npwm = CROS_EC_PWM_DT_COUNT; > + } else { > + ret = cros_ec_num_pwms(ec_pwm); > + if (ret < 0) { > + dev_err(dev, "Couldn't find PWMs: %d\n", ret); > + return ret; > + } > + chip->npwm = ret; > } > - chip->npwm = ret; > + > dev_dbg(dev, "Probed %u PWMs\n", chip->npwm); > > ret = pwmchip_add(chip); > @@ -288,6 +339,7 @@ static int cros_ec_pwm_remove(struct platform_device *dev) > #ifdef CONFIG_OF > static const struct of_device_id cros_ec_pwm_of_match[] = { > { .compatible = "google,cros-ec-pwm" }, > + { .compatible = "google,cros-ec-pwm-type" }, > {}, > }; > MODULE_DEVICE_TABLE(of, cros_ec_pwm_of_match); > -- > 2.36.0.rc0.470.gd361397f0d-goog > >