RE: [PATCH 4/7] mfd: da9052: Simplify getting of_device_id match data
On 19 April 2021 09:17, Krzysztof Kozlowski wrote:
> Use of_device_get_match_data() to make the code slightly smaller.
>
> Signed-off-by: Krzysztof Kozlowski
> ---
Acked-by: Adam Thomson
> drivers/mfd/da9062-core.c | 13 -
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
> index 8d913375152d..01f8e10dfa55 100644
> --- a/drivers/mfd/da9062-core.c
> +++ b/drivers/mfd/da9062-core.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -622,7 +623,6 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
> {
> struct da9062 *chip;
> - const struct of_device_id *match;
> unsigned int irq_base;
> const struct mfd_cell *cell;
> const struct regmap_irq_chip *irq_chip;
> @@ -635,15 +635,10 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
> if (!chip)
> return -ENOMEM;
>
> - if (i2c->dev.of_node) {
> - match = of_match_node(da9062_dt_ids, i2c->dev.of_node);
> - if (!match)
> - return -EINVAL;
> -
> - chip->chip_type = (uintptr_t)match->data;
> - } else {
> + if (i2c->dev.of_node)
> + chip->chip_type = (uintptr_t)of_device_get_match_data(
> >dev);
> + else
> chip->chip_type = id->driver_data;
> - }
>
> i2c_set_clientdata(i2c, chip);
> chip->dev = >dev;
> --
> 2.25.1
RE: [PATCH 3/7] mfd: da9052: Simplify getting of_device_id match data
On 19 April 2021 09:17, Krzysztof Kozlowski wrote:
> Use of_device_get_match_data() to make the code slightly smaller.
>
> Signed-off-by: Krzysztof Kozlowski
> ---
Acked-by: Adam Thomson
> drivers/mfd/da9052-i2c.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mfd/da9052-i2c.c b/drivers/mfd/da9052-i2c.c
> index 47556d2d9abe..5d8ce8dca3fc 100644
> --- a/drivers/mfd/da9052-i2c.c
> +++ b/drivers/mfd/da9052-i2c.c
> @@ -154,13 +154,8 @@ static int da9052_i2c_probe(struct i2c_client *client,
> return ret;
>
> #ifdef CONFIG_OF
> - if (!id) {
> - struct device_node *np = client->dev.of_node;
> - const struct of_device_id *deviceid;
> -
> - deviceid = of_match_node(dialog_dt_ids, np);
> - id = deviceid->data;
> - }
> + if (!id)
> + id = of_device_get_match_data(>dev);
> #endif
>
> if (!id) {
> --
> 2.25.1
RE: [RFC PATCH 2/2] ASoC: da732x: simplify code
On 15 April 2021 17:04, Mark Brown wrote: > On Thu, Apr 15, 2021 at 04:00:48PM +0000, Adam Thomson wrote: > > On 26 March 2021 22:16, Pierre-Louis Bossart wrote: > > > Apologies for the delay in getting to this. The change looks fine to me, > > although this part was EOL some time back, and I find it hard to believe > > anyone > > out there has a board with this on. Wondering if it would make sense to > remove > > the driver permanently? > > Unless it's actually getting in the way it's generally easier to just > leave the driver than try to figure out if anyone is updating a system > that uses it. Fair enough. Just don't want to waste people's time with unnecessary updates :)
RE: [RFC PATCH 2/2] ASoC: da732x: simplify code
On 26 March 2021 22:16, Pierre-Louis Bossart wrote:
> cppcheck reports a false positive:
>
> sound/soc/codecs/da732x.c:1161:25: warning: Either the condition
> 'indiv<0' is redundant or there is division by zero at line
> 1161. [zerodivcond]
> fref = (da732x->sysclk / indiv);
> ^
> sound/soc/codecs/da732x.c:1158:12: note: Assuming that condition
> 'indiv<0' is not redundant
> if (indiv < 0)
>^
> sound/soc/codecs/da732x.c:1161:25: note: Division by zero
> fref = (da732x->sysclk / indiv);
> ^
>
> The code is awfully convoluted/confusing and can be simplified with a
> single variable and the BIT macro.
>
> Signed-off-by: Pierre-Louis Bossart
> ---
Apologies for the delay in getting to this. The change looks fine to me,
although this part was EOL some time back, and I find it hard to believe anyone
out there has a board with this on. Wondering if it would make sense to remove
the driver permanently?
For the change at hand though:
Reviewed-by: Adam Thomson
> sound/soc/codecs/da732x.c | 17 ++---
> sound/soc/codecs/da732x.h | 12
> 2 files changed, 10 insertions(+), 19 deletions(-)
>
> diff --git a/sound/soc/codecs/da732x.c b/sound/soc/codecs/da732x.c
> index d43ee7159ae0..42d6a3fc3af5 100644
> --- a/sound/soc/codecs/da732x.c
> +++ b/sound/soc/codecs/da732x.c
> @@ -168,30 +168,25 @@ static const struct reg_default da732x_reg_cache[] = {
> static inline int da732x_get_input_div(struct snd_soc_component *component,
> int sysclk)
> {
> int val;
> - int ret;
>
> if (sysclk < DA732X_MCLK_10MHZ) {
> - val = DA732X_MCLK_RET_0_10MHZ;
> - ret = DA732X_MCLK_VAL_0_10MHZ;
> + val = DA732X_MCLK_VAL_0_10MHZ;
> } else if ((sysclk >= DA732X_MCLK_10MHZ) &&
> (sysclk < DA732X_MCLK_20MHZ)) {
> - val = DA732X_MCLK_RET_10_20MHZ;
> - ret = DA732X_MCLK_VAL_10_20MHZ;
> + val = DA732X_MCLK_VAL_10_20MHZ;
> } else if ((sysclk >= DA732X_MCLK_20MHZ) &&
> (sysclk < DA732X_MCLK_40MHZ)) {
> - val = DA732X_MCLK_RET_20_40MHZ;
> - ret = DA732X_MCLK_VAL_20_40MHZ;
> + val = DA732X_MCLK_VAL_20_40MHZ;
> } else if ((sysclk >= DA732X_MCLK_40MHZ) &&
> (sysclk <= DA732X_MCLK_54MHZ)) {
> - val = DA732X_MCLK_RET_40_54MHZ;
> - ret = DA732X_MCLK_VAL_40_54MHZ;
> + val = DA732X_MCLK_VAL_40_54MHZ;
> } else {
> return -EINVAL;
> }
>
> snd_soc_component_write(component, DA732X_REG_PLL_CTRL, val);
>
> - return ret;
> + return val;
> }
>
> static void da732x_set_charge_pump(struct snd_soc_component *component,
> int state)
> @@ -1158,7 +1153,7 @@ static int da732x_set_dai_pll(struct
> snd_soc_component *component, int pll_id,
> if (indiv < 0)
> return indiv;
>
> - fref = (da732x->sysclk / indiv);
> + fref = da732x->sysclk / BIT(indiv);
> div_hi = freq_out / fref;
> frac_div = (u64)(freq_out % fref) * 8192ULL;
> do_div(frac_div, fref);
> diff --git a/sound/soc/codecs/da732x.h b/sound/soc/codecs/da732x.h
> index c5af17ee1516..c2f784c3f359 100644
> --- a/sound/soc/codecs/da732x.h
> +++ b/sound/soc/codecs/da732x.h
> @@ -48,14 +48,10 @@
> #define DA732X_MCLK_20MHZ 2000
> #define DA732X_MCLK_40MHZ 4000
> #define DA732X_MCLK_54MHZ 5400
> -#define DA732X_MCLK_RET_0_10MHZ 0
> -#define DA732X_MCLK_VAL_0_10MHZ 1
> -#define DA732X_MCLK_RET_10_20MHZ1
> -#define DA732X_MCLK_VAL_10_20MHZ2
> -#define DA732X_MCLK_RET_20_40MHZ2
> -#define DA732X_MCLK_VAL_20_40MHZ4
> -#define DA732X_MCLK_RET_40_54MHZ3
> -#define DA732X_MCLK_VAL_40_54MHZ8
> +#define DA732X_MCLK_VAL_0_10MHZ 0
> +#define DA732X_MCLK_VAL_10_20MHZ1
> +#define DA732X_MCLK_VAL_20_40MHZ2
> +#define DA732X_MCLK_VAL_40_54MHZ3
> #define DA732X_DAI_ID1 0
> #define DA732X_DAI_ID2 1
> #define DA732X_SRCCLK_PLL 0
> --
> 2.25.1
RE: [PATCH v1 2/6] usb: typec: tcpm: Address incorrect values of tcpm psy for pps supply
On 06 April 2021 02:37, Badhri Jagan Sridharan wrote:
> tcpm_pd_select_pps_apdo overwrites port->pps_data.min_volt,
> port->pps_data.max_volt, port->pps_data.max_curr even before
> port partner accepts the requests. This leaves incorrect values
> in current_limit and supply_voltage that get exported by
> "tcpm-source-psy-". Solving this problem by caching the request
> values in req_min_volt, req_max_volt, req_max_curr, req_out_volt,
> req_op_curr. min_volt, max_volt, max_curr gets updated once the
> partner accepts the request. current_limit, supply_voltage gets updated
> once local port's tcpm enters SNK_TRANSITION_SINK when the accepted
> current_limit and supply_voltage is enforced.
>
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through
> power_supply")
> Signed-off-by: Badhri Jagan Sridharan
> ---
Reviewed-by: Adam Thomson
> drivers/usb/typec/tcpm/tcpm.c | 84 ---
> 1 file changed, 49 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 03eca5061132..d43774cc2ccf 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -269,11 +269,22 @@ struct pd_mode_data {
> };
>
> struct pd_pps_data {
> + /* Actual min voltage at the local port */
> u32 min_volt;
> + /* Requested min voltage to the port partner */
> + u32 req_min_volt;
> + /* Actual max voltage at the local port */
> u32 max_volt;
> + /* Requested max voltage to the port partner */
> + u32 req_max_volt;
> + /* Actual max current at the local port */
> u32 max_curr;
> - u32 out_volt;
> - u32 op_curr;
> + /* Requested max current of the port partner */
> + u32 req_max_curr;
> + /* Requested output voltage to the port partner */
> + u32 req_out_volt;
> + /* Requested operating current to the port partner */
> + u32 req_op_curr;
> bool supported;
> bool active;
> };
> @@ -2498,8 +2509,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port
> *port,
> break;
> case SNK_NEGOTIATE_PPS_CAPABILITIES:
> /* Revert data back from any requested PPS updates */
> - port->pps_data.out_volt = port->supply_voltage;
> - port->pps_data.op_curr = port->current_limit;
> + port->pps_data.req_out_volt = port->supply_voltage;
> + port->pps_data.req_op_curr = port->current_limit;
> port->pps_status = (type == PD_CTRL_WAIT ?
> -EAGAIN : -EOPNOTSUPP);
>
> @@ -2548,8 +2559,11 @@ static void tcpm_pd_ctrl_request(struct tcpm_port
> *port,
> break;
> case SNK_NEGOTIATE_PPS_CAPABILITIES:
> port->pps_data.active = true;
> - port->req_supply_voltage = port->pps_data.out_volt;
> - port->req_current_limit = port->pps_data.op_curr;
> + port->pps_data.min_volt = port-
> >pps_data.req_min_volt;
> + port->pps_data.max_volt = port-
> >pps_data.req_max_volt;
> + port->pps_data.max_curr = port-
> >pps_data.req_max_curr;
> + port->req_supply_voltage = port-
> >pps_data.req_out_volt;
> + port->req_current_limit = port->pps_data.req_op_curr;
> tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
> break;
> case SOFT_RESET_SEND:
> @@ -3108,16 +3122,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> src = port->source_caps[src_pdo];
> snk = port->snk_pdo[snk_pdo];
>
> - port->pps_data.min_volt =
> max(pdo_pps_apdo_min_voltage(src),
> - pdo_pps_apdo_min_voltage(snk));
> - port->pps_data.max_volt =
> min(pdo_pps_apdo_max_voltage(src),
> - pdo_pps_apdo_max_voltage(snk));
> - port->pps_data.max_curr = min_pps_apdo_current(src, snk);
> - port->pps_data.out_volt = min(port->pps_data.max_volt,
> - max(port->pps_data.min_volt,
> - port->pps_data.out_volt));
> - port->pps_data.op_curr = min(port->pps_data.max_curr,
> - port->pps_data.op_curr);
> + port->pps_d
RE: [PATCH v1 3/6] usb: typec: tcpm: update power supply once partner accepts
On 06 April 2021 02:37, Badhri Jagan Sridharan wrote:
> power_supply_changed needs to be called to notify clients
> after the partner accepts the requested values for the pps
> case.
>
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through
> power_supply")
> Signed-off-by: Badhri Jagan Sridharan
> ---
Missing commit information aside:
Reviewed-by: Adam Thomson
> drivers/usb/typec/tcpm/tcpm.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d43774cc2ccf..7708b01009cb 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2564,6 +2564,7 @@ static void tcpm_pd_ctrl_request(struct tcpm_port
> *port,
> port->pps_data.max_curr = port-
> >pps_data.req_max_curr;
> port->req_supply_voltage = port-
> >pps_data.req_out_volt;
> port->req_current_limit = port->pps_data.req_op_curr;
> + power_supply_changed(port->psy);
> tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
> break;
> case SOFT_RESET_SEND:
> @@ -3132,7 +3133,6 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> port-
> >pps_data.req_out_volt));
> port->pps_data.req_op_curr = min(port->pps_data.max_curr,
>port->pps_data.req_op_curr);
> - power_supply_changed(port->psy);
> }
>
> return src_pdo;
> @@ -3557,8 +3557,6 @@ static void tcpm_reset_port(struct tcpm_port *port)
> port->sink_cap_done = false;
> if (port->tcpc->enable_frs)
> port->tcpc->enable_frs(port->tcpc, false);
> -
> - power_supply_changed(port->psy);
> }
>
> static void tcpm_detach(struct tcpm_port *port)
> --
> 2.31.0.208.g409f899ff0-goog
RE: [PATCH v1 1/6] usb: typec: tcpm: Address incorrect values of tcpm psy for fixed supply
On 06 April 2021 02:37, Badhri Jagan Sridharan wrote:
> tcpm_pd_build_request overwrites current_limit and supply_voltage
> even before port partner accepts the requests. This leaves stale
> values in current_limit and supply_voltage that get exported by
> "tcpm-source-psy-". Solving this problem by caching the request
> values of current limit/supply voltage in req_current_limit
> and req_supply_voltage. current_limit/supply_voltage gets updated
> once the port partner accepts the request.
>
> Fixes: f2a8aa053c176 ("typec: tcpm: Represent source supply through
> power_supply")
> Signed-off-by: Badhri Jagan Sridharan
> ---
Looks sensible, typo aside:
Reviewed-by: Adam Thomson
> drivers/usb/typec/tcpm/tcpm.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index ca1fc77697fc..03eca5061132 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -389,7 +389,10 @@ struct tcpm_port {
> unsigned int operating_snk_mw;
> bool update_sink_caps;
>
> - /* Requested current / voltage */
> + /* Requested current / voltage to the port partner */
> + u32 req_current_limit;
> + u32 req_supply_voltage;
> + /* Acutal current / voltage limit of the local port */
> u32 current_limit;
> u32 supply_voltage;
>
> @@ -2435,8 +2438,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port
> *port,
> case SNK_TRANSITION_SINK:
> if (port->vbus_present) {
> tcpm_set_current_limit(port,
> -port->current_limit,
> -port->supply_voltage);
> +port->req_current_limit,
> +
> port->req_supply_voltage);
> port->explicit_contract = true;
> tcpm_set_auto_vbus_discharge_threshold(port,
>
> TYPEC_PWR_MODE_PD,
> @@ -2545,8 +2548,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port
> *port,
> break;
> case SNK_NEGOTIATE_PPS_CAPABILITIES:
> port->pps_data.active = true;
> - port->supply_voltage = port->pps_data.out_volt;
> - port->current_limit = port->pps_data.op_curr;
> + port->req_supply_voltage = port->pps_data.out_volt;
> + port->req_current_limit = port->pps_data.op_curr;
> tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
> break;
> case SOFT_RESET_SEND:
> @@ -3195,8 +3198,8 @@ static int tcpm_pd_build_request(struct tcpm_port
> *port, u32 *rdo)
>flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
> }
>
> - port->current_limit = ma;
> - port->supply_voltage = mv;
> + port->req_current_limit = ma;
> + port->req_supply_voltage = mv;
>
> return 0;
> }
> --
> 2.31.0.208.g409f899ff0-goog
RE: [PATCH v2] usb: typec: tcpm: Invoke power_supply_changed for tcpm-source-psy-
On 18 March 2021 20:40, Badhri Jagan Sridharan wrote: > > Regarding selecting PDOs or PPS APDOs, surely we should only notify of a > change > > when we reach SNK_READY which means a new contract has been established? > Until > > that point it's possible any requested change could be rejected so why > > inform > > clients before we know the settings have taken effect? I could be missing > > something here as it's been a little while since I delved into this, but > > this > > doesn't seem to make sense to me. > > I was trying to keep the power_supply_changed call close to the > variables which are used to infer the power supply property values. > Since port->pps_data.max_curr is already updated here and that's used > to infer the CURRENT_MAX a client could still read this before the > request goes through right ? Actually that's fair but I think the problem here relates to 'max_curr' not being reset if the SRC rejects our request when we're swapping between one PPS APDO and another PPS APDO. I think the 'max_curr' value should be reverted back to the value for the existing PPS APDO we were already using. I suspect the same might be true of 'min_volt' and 'max_volt' as well, now I look at it. It might actually be prudent to have pending PPS data based on a request, which is only committed as active once ACCEPT has been received. Regarding power_supply_changed() though, I still think we should only notify of a change when the requested change has been accepted by the source, in relation to these values as they should reflect the real, in-use voltage and current values.
RE: [PATCH v2] usb: typec: tcpm: Invoke power_supply_changed for tcpm-source-psy-
On 17 March 2021 18:13, Badhri Jagan Sridharan wrote:
> tcpm-source-psy- does not invoke power_supply_changed API when
> one of the published power supply properties is changed.
> power_supply_changed needs to be called to notify
> userspace clients(uevents) and kernel clients.
>
> Fixes: f2a8aa053c176("typec: tcpm: Represent source supply through
> power_supply")
> Signed-off-by: Badhri Jagan Sridharan
> Reviewed-by: Guenter Roeck
> Reviewed-by: Heikki Krogerus
> ---
> Changes since V1:
> - Fixed commit message as per Guenter's suggestion
> - Added Reviewed-by tags
> - cc'ed stable
> ---
> drivers/usb/typec/tcpm/tcpm.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 11d0c40bc47d..e8936ea17f80 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -945,6 +945,7 @@ static int tcpm_set_current_limit(struct tcpm_port *port,
> u32 max_ma, u32 mv)
>
> port->supply_voltage = mv;
> port->current_limit = max_ma;
> + power_supply_changed(port->psy);
>
> if (port->tcpc->set_current_limit)
> ret = port->tcpc->set_current_limit(port->tcpc, max_ma, mv);
> @@ -2931,6 +2932,7 @@ static int tcpm_pd_select_pdo(struct tcpm_port *port,
> int *sink_pdo,
>
> port->pps_data.supported = false;
> port->usb_type = POWER_SUPPLY_USB_TYPE_PD;
> + power_supply_changed(port->psy);
>
> /*
>* Select the source PDO providing the most power which has a
> @@ -2955,6 +2957,7 @@ static int tcpm_pd_select_pdo(struct tcpm_port *port,
> int *sink_pdo,
> port->pps_data.supported = true;
> port->usb_type =
> POWER_SUPPLY_USB_TYPE_PD_PPS;
> + power_supply_changed(port->psy);
> }
> continue;
> default:
> @@ -3112,6 +3115,7 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> port->pps_data.out_volt));
> port->pps_data.op_curr = min(port->pps_data.max_curr,
>port->pps_data.op_curr);
> + power_supply_changed(port->psy);
> }
>
> return src_pdo;
Regarding selecting PDOs or PPS APDOs, surely we should only notify of a change
when we reach SNK_READY which means a new contract has been established? Until
that point it's possible any requested change could be rejected so why inform
clients before we know the settings have taken effect? I could be missing
something here as it's been a little while since I delved into this, but this
doesn't seem to make sense to me.
> @@ -3347,6 +3351,7 @@ static int tcpm_set_charge(struct tcpm_port *port, bool
> charge)
> return ret;
> }
> port->vbus_charge = charge;
> + power_supply_changed(port->psy);
> return 0;
> }
>
> @@ -3530,6 +3535,7 @@ static void tcpm_reset_port(struct tcpm_port *port)
> port->try_src_count = 0;
> port->try_snk_count = 0;
> port->usb_type = POWER_SUPPLY_USB_TYPE_C;
> + power_supply_changed(port->psy);
This is already taken care of at the end of this function, isn't it?
> port->nr_sink_caps = 0;
> port->sink_cap_done = false;
> if (port->tcpc->enable_frs)
> @@ -5957,7 +5963,7 @@ static int tcpm_psy_set_prop(struct power_supply
> *psy,
> ret = -EINVAL;
> break;
> }
> -
> + power_supply_changed(port->psy);
> return ret;
> }
>
> @@ -6110,6 +6116,7 @@ struct tcpm_port *tcpm_register_port(struct device
> *dev, struct tcpc_dev *tcpc)
> err = devm_tcpm_psy_register(port);
> if (err)
> goto out_role_sw_put;
> + power_supply_changed(port->psy);
>
> port->typec_port = typec_register_port(port->dev, >typec_caps);
> if (IS_ERR(port->typec_port)) {
> --
> 2.31.0.rc2.261.g7f71774620-goog
RE: [PATCH v6 1/1] mfd: da9063: Support SMBus and I2C mode
On 16 March 2021 16:23, Mark Jonas wrote: > From: Hubert Streidl > > By default the PMIC DA9063 2-wire interface is SMBus compliant. This > means the PMIC will automatically reset the interface when the clock > signal ceases for more than the SMBus timeout of 35 ms. > > If the I2C driver / device is not capable of creating atomic I2C > transactions, a context change can cause a ceasing of the clock signal. > This can happen if for example a real-time thread is scheduled. Then > the DA9063 in SMBus mode will reset the 2-wire interface. Subsequently > a write message could end up in the wrong register. This could cause > unpredictable system behavior. > > The DA9063 PMIC also supports an I2C compliant mode for the 2-wire > interface. This mode does not reset the interface when the clock > signal ceases. Thus the problem depicted above does not occur. > > This patch tests for the bus functionality "I2C_FUNC_I2C". It can > reasonably be assumed that the bus cannot obey SMBus timings if > this functionality is set. SMBus commands most probably are emulated > in this case which is prone to the latency issue described above. > > This patch enables the I2C bus mode if I2C_FUNC_I2C is set or > otherwise keeps the default SMBus mode. > > Signed-off-by: Hubert Streidl > Signed-off-by: Mark Jonas Thanks for your efforts on this Mark.
RE: [PATCH 07/23] ASoC: da7219-aad: remove useless initialization
On 12 March 2021 18:23, Pierre-Louis Bossart wrote: > cppcheck warning: > > sound/soc/codecs/da7219-aad.c:118:22: style: Variable 'ret' is > assigned a value that is never used. [unreadVariable] > int report = 0, ret = 0; > ^ > > Signed-off-by: Pierre-Louis Bossart Acked-by: Adam Thomson
RE: [PATCH v4] mfd: da9063: Support SMBus and I2C mode
On 08 February 2021 15:28, Mark Jonas wrote: > From: Hubert Streidl > > By default the PMIC DA9063 2-wire interface is SMBus compliant. This > means the PMIC will automatically reset the interface when the clock > signal ceases for more than the SMBus timeout of 35 ms. > > If the I2C driver / device is not capable of creating atomic I2C > transactions, a context change can cause a ceasing of the clock signal. > This can happen if for example a real-time thread is scheduled. Then > the DA9063 in SMBus mode will reset the 2-wire interface. Subsequently > a write message could end up in the wrong register. This could cause > unpredictable system behavior. > > The DA9063 PMIC also supports an I2C compliant mode for the 2-wire > interface. This mode does not reset the interface when the clock > signal ceases. Thus the problem depicted above does not occur. > > This patch tests for the bus functionality "I2C_FUNC_I2C". It can > reasonably be assumed that the bus cannot obey SMBus timings if > this functionality is set. SMBus commands most probably are emulated > in this case which is prone to the latency issue described above. > > This patch enables the I2C bus mode if I2C_FUNC_I2C is set or > otherwise enables the SMBus mode for a native SMBus controller > which doesn't have I2C_FUNC_I2C set. > > Signed-off-by: Hubert Streidl > Signed-off-by: Mark Jonas Thanks for your efforts. Looks sensible to me, so: Reviewed-by: Adam Thomson
RE: [PATCH v2] mfd: da9063: Support SMBus and I2C mode
On 04 February 2021 15:52, Mark Jonas wrote:
> From: Hubert Streidl
>
> By default the PMIC DA9063 2-wire interface is SMBus compliant. This
> means the PMIC will automatically reset the interface when the clock
> signal ceases for more than the SMBus timeout of 35 ms.
>
> If the I2C driver / device is not capable of creating atomic I2C
> transactions, a context change can cause a ceasing of the clock signal.
> This can happen if for example a real-time thread is scheduled. Then
> the DA9063 in SMBus mode will reset the 2-wire interface. Subsequently
> a write message could end up in the wrong register. This could cause
> unpredictable system behavior.
>
> The DA9063 PMIC also supports an I2C compliant mode for the 2-wire
> interface. This mode does not reset the interface when the clock
> signal ceases. Thus the problem depicted above does not occur.
>
> This patch tests for the bus functionality "I2C_FUNC_I2C". It can
> reasonably be assumed that the bus cannot obey SMBus timings if
> this functionality is set. SMBus commands most probably are emulated
> in this case which is prone to the latency issue described above.
>
> This patch enables the I2C bus mode if I2C_FUNC_I2C is set or
> otherwise enables the SMBus mode for a native SMBus controller
> which doesn't have I2C_FUNC_I2C set.
>
> Signed-off-by: Hubert Streidl
> Signed-off-by: Mark Jonas
> ---
> Changes in v2:
> - Implement proposal by Adam Thomson and Wolfram Sang to check for
> functionality I2C_FUNC_I2C instead of introducing a new DT property.
>
> drivers/mfd/da9063-i2c.c | 15 +++
> include/linux/mfd/da9063/registers.h | 3 +++
> 2 files changed, 18 insertions(+)
>
> diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
> index 3781d0bb7786..a2245253f039 100644
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
> @@ -355,6 +355,7 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
> {
> struct da9063 *da9063;
> + unsigned int busmode;
> int ret;
>
> da9063 = devm_kzalloc(>dev, sizeof(struct da9063), GFP_KERNEL);
> @@ -442,6 +443,20 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> return ret;
> }
>
> + if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
> + dev_info(da9063->dev, "I2C mode");
> + busmode = 0;
> + } else {
> + dev_info(da9063->dev, "SMBus mode");
> + busmode = 1;
I think this should be 'DA9063_TWOWIRE_TO' rather than '1' otherwise I think
you'll be setting 0 still here as it doesn't match up with the mask.
> + }
> + ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONFIG_J,
> + DA9063_TWOWIRE_TO, busmode);
> + if (ret < 0) {
> + dev_err(da9063->dev, "Failed to set 2-wire bus mode.\n");
> + return -EIO;
> + }
> +
> return da9063_device_init(da9063, i2c->irq);
> }
>
> diff --git a/include/linux/mfd/da9063/registers.h
> b/include/linux/mfd/da9063/registers.h
> index 1dbabf1b3cb8..6e0f66a2e727 100644
> --- a/include/linux/mfd/da9063/registers.h
> +++ b/include/linux/mfd/da9063/registers.h
> @@ -1037,6 +1037,9 @@
> #define DA9063_NONKEY_PIN_AUTODOWN 0x02
> #define DA9063_NONKEY_PIN_AUTOFLPRT 0x03
>
> +/* DA9063_REG_CONFIG_J (addr=0x10F) */
> +#define DA9063_TWOWIRE_TO0x40
> +
> /* DA9063_REG_MON_REG_5 (addr=0x116) */
> #define DA9063_MON_A8_IDX_MASK 0x07
> #define DA9063_MON_A8_IDX_NONE 0x00
> --
> 2.25.1
RE: [PATCH 1/1] mfd: da9063: Support SMBus and I2C mode
On 25 January 2021 12:55, Mark Jonas wrote:
> From: Hubert Streidl
>
> By default the PMIC DA9063 2-wire interface is SMBus compliant. This
> means the PMIC will automatically reset the interface when the clock
> signal ceases for more than the SMBus timeout of 35 ms.
>
> If the I2C driver / device is not capable of creating atomic I2C
> transactions, a context change can cause a ceasing of the the clock
> signal. This can happen if for example a real-time thread is scheduled.
> Then the DA9063 in SMBus mode will reset the 2-wire interface.
> Subsequently a write message could end up in the wrong register. This
> could cause unpredictable system behavior.
>
> The DA9063 PMIC also supports an I2C compliant mode for the 2-wire
> interface. This mode does not reset the interface when the clock
> signal ceases. Thus the problem depicted above does not occur.
>
> This patch makes the I2C mode configurable by device tree. The SMBus
> compliant mode is kept as the default.
Could we not just check the bus' functionality flags and set this accordingly?
Something like this is already done in regmap-i2c to determine how to access
the device:
https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-i2c.c#L309
This seems cleaner than a new DT property, or will this not work in this
situation?
>
> Signed-off-by: Hubert Streidl
> Signed-off-by: Mark Jonas
> ---
> Documentation/devicetree/bindings/mfd/da9063.txt | 7 +++
> drivers/mfd/da9063-core.c| 9 +
> drivers/mfd/da9063-i2c.c | 13 +
> include/linux/mfd/da9063/core.h | 1 +
> include/linux/mfd/da9063/registers.h | 3 +++
> 5 files changed, 33 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt
> b/Documentation/devicetree/bindings/mfd/da9063.txt
> index 8da879935c59..256f2a25fe0a 100644
> --- a/Documentation/devicetree/bindings/mfd/da9063.txt
> +++ b/Documentation/devicetree/bindings/mfd/da9063.txt
> @@ -19,6 +19,12 @@ Required properties:
> - interrupts : IRQ line information.
> - interrupt-controller
>
> +Optional properties:
> +
> +- i2c-mode : Switch serial 2-wire interface into I2C mode. Without this
> + property the PMIC uses the SMBus mode (resets the interface if the clock
> + ceases for a longer time than the SMBus timeout).
> +
> Sub-nodes:
>
> - regulators : This node defines the settings for the LDOs and BUCKs.
> @@ -77,6 +83,7 @@ Example:
> interrupt-parent = <>;
> interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
> interrupt-controller;
> + i2c-mode;
>
> rtc {
> compatible = "dlg,da9063-rtc";
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index df407c3afce3..baa1e4310c8c 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -162,6 +162,15 @@ int da9063_device_init(struct da9063 *da9063, unsigned
> int irq)
> {
> int ret;
>
> + if (da9063->i2cmode) {
> + ret = regmap_update_bits(da9063->regmap,
> DA9063_REG_CONFIG_J,
> + DA9063_TWOWIRE_TO, 0);
> + if (ret < 0) {
> + dev_err(da9063->dev, "Cannot enable I2C mode.\n");
> + return -EIO;
> + }
> + }
> +
> ret = da9063_clear_fault_log(da9063);
> if (ret < 0)
> dev_err(da9063->dev, "Cannot clear fault log\n");
> diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
> index 3781d0bb7786..af0bf13ab43e 100644
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
> @@ -351,6 +351,17 @@ static const struct of_device_id da9063_dt_ids[] = {
> { }
> };
> MODULE_DEVICE_TABLE(of, da9063_dt_ids);
> +
> +static void da9063_i2c_parse_dt(struct i2c_client *client, struct da9063
> *da9063)
> +{
> + struct device_node *np = client->dev.of_node;
> +
> + if (of_property_read_bool(np, "i2c-mode"))
> + da9063->i2cmode = true;
> + else
> + da9063->i2cmode = false;
> +}
> +
> static int da9063_i2c_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
> {
> @@ -366,6 +377,8 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> da9063->chip_irq = i2c->irq;
> da9063->type = id->driver_data;
>
> + da9063_i2c_parse_dt(i2c, da9063);
> +
> ret = da9063_get_device_type(i2c, da9063);
> if (ret)
> return ret;
> diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> index fa7a43f02f27..866864c50f78 100644
> --- a/include/linux/mfd/da9063/core.h
> +++ b/include/linux/mfd/da9063/core.h
> @@ -77,6 +77,7 @@ struct da9063 {
> enum da9063_type type;
> unsigned char variant_code;
> unsigned intflags;
> + booli2cmode;
>
> /* Control interface */
> struct regmap *regmap;
> diff --git
RE: [PATCH] ASoC: codecs: da*: Remove redundant null check before clk_prepare_enable/clk_disable_unprepare
On 08 January 2021 08:53, Xu Wang wrote:
> Because clk_prepare_enable() and clk_disable_unprepare() already checked
> NULL clock parameter, so the additional checks are unnecessary, just
> remove them.
>
> Signed-off-by: Xu Wang
> ---
> sound/soc/codecs/da7218.c | 13 +
> sound/soc/codecs/da7219-aad.c | 19 ---
> sound/soc/codecs/da7219.c | 3 +--
> 3 files changed, 14 insertions(+), 21 deletions(-)
>
> diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c
> index 2bfafbe9e3dc..324d95e3b48a 100644
> --- a/sound/soc/codecs/da7218.c
> +++ b/sound/soc/codecs/da7218.c
> @@ -2582,12 +2582,10 @@ static int da7218_set_bias_level(struct
> snd_soc_component *component,
> case SND_SOC_BIAS_PREPARE:
> /* Enable MCLK for transition to ON state */
> if (snd_soc_component_get_bias_level(component) ==
> SND_SOC_BIAS_STANDBY) {
> - if (da7218->mclk) {
> - ret = clk_prepare_enable(da7218->mclk);
> - if (ret) {
> - dev_err(component->dev, "Failed to
> enable mclk\n");
> - return ret;
> - }
> + ret = clk_prepare_enable(da7218->mclk);
> + if (ret) {
> + dev_err(component->dev, "Failed to enable
> mclk\n");
> + return ret;
> }
> }
>
> @@ -2605,8 +2603,7 @@ static int da7218_set_bias_level(struct
> snd_soc_component *component,
> DA7218_LDO_EN_MASK);
> } else {
> /* Remove MCLK */
> - if (da7218->mclk)
> - clk_disable_unprepare(da7218->mclk);
> + clk_disable_unprepare(da7218->mclk);
> }
> break;
> case SND_SOC_BIAS_OFF:
> diff --git a/sound/soc/codecs/da7219-aad.c b/sound/soc/codecs/da7219-aad.c
> index 48081d71c22c..fa61cfc8e50c 100644
> --- a/sound/soc/codecs/da7219-aad.c
> +++ b/sound/soc/codecs/da7219-aad.c
> @@ -123,15 +123,13 @@ static void da7219_aad_hptest_work(struct work_struct
> *work)
> mutex_lock(>pll_lock);
>
> /* Ensure MCLK is available for HP test procedure */
> - if (da7219->mclk) {
> - ret = clk_prepare_enable(da7219->mclk);
> - if (ret) {
> - dev_err(component->dev, "Failed to enable mclk -
> %d\n", ret);
> - mutex_unlock(>pll_lock);
> - mutex_unlock(>ctrl_lock);
> - snd_soc_dapm_mutex_unlock(dapm);
> - return;
> - }
> + ret = clk_prepare_enable(da7219->mclk);
> + if (ret) {
> + dev_err(component->dev, "Failed to enable mclk - %d\n", ret);
> + mutex_unlock(>pll_lock);
> + mutex_unlock(>ctrl_lock);
> + snd_soc_dapm_mutex_unlock(dapm);
> + return;
> }
>
> /*
> @@ -318,8 +316,7 @@ static void da7219_aad_hptest_work(struct work_struct
> *work)
> da7219_set_pll(component, DA7219_SYSCLK_MCLK, 0);
>
> /* Remove MCLK, if previously enabled */
> - if (da7219->mclk)
> - clk_disable_unprepare(da7219->mclk);
> + clk_disable_unprepare(da7219->mclk);
>
> mutex_unlock(>pll_lock);
> mutex_unlock(>ctrl_lock);
> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> index e9b45daec0ca..50307cc9d640 100644
> --- a/sound/soc/codecs/da7219.c
> +++ b/sound/soc/codecs/da7219.c
> @@ -1826,8 +1826,7 @@ static int da7219_set_bias_level(struct
> snd_soc_component *component,
>
> if (snd_soc_component_get_bias_level(component) ==
> SND_SOC_BIAS_PREPARE) {
> /* Remove MCLK */
> - if (da7219->mclk)
> - clk_disable_unprepare(da7219->mclk);
> + clk_disable_unprepare(da7219->mclk);
You seem to have missed the change for clk_prepare_enable() usage in the
same function. Also as the above is now a single line if statement, you can
remove the brackets.
In addition, the normal approach would be to split the changes into separate
patches per driver, just so it's cleaner.
> }
> break;
> case SND_SOC_BIAS_OFF:
> --
> 2.17.1
RE: [PATCH v2 3/3] thermal/core: Remove ms based delay fields
On 16 December 2020 22:04, Daniel Lezcano wrote: > The code does no longer use the ms unit based fields to set the > delays as they are replaced by the jiffies. > > Remove them and replace their user to use the jiffies version instead. > > Cc: Thara Gopinath > Signed-off-by: Daniel Lezcano > Reviewed-by: Lukasz Luba For DA9062: Reviewed-by: Adam Thomson
RE: [PATCH V3 00/10] regulator: da9121: extend support to variants, add features
On 01 December 2020 13:58, Mark Brown wrote: > On Mon, 30 Nov 2020 16:59:04 +, Adam Ward wrote: > > This series extends the DA9121 driver to add support for related products: > > > > DA9130, 10A, Dual-Phase (Automotive Grade) > > DA9122, 5A + 5A > > DA9131, 5A + 5A (Automotive Grade) > > DA9220, 3A + 3A > > DA9132, 3A + 3A (Automotive Grade) > > DA9217, 6A, Dual-Phase > > > > [...] > > Applied to > >https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next > Hi Mark, Was the intention to apply these to the ASoC repo, as this patch set is just for regulator? Just wanted to check.
RE: [PATCH v2 3/3] mfd: da9055: fix "REGULATOR" spelling in register content macro
On 27 November 2020 12:52, Michael Klein wrote: > "REGUALTOR" -> "REGULATOR" > > Signed-off-by: Michael Klein Reviewed-by: Adam Thomson
RE: [PATCH 1/8] mfd: da: Constify static struct resource
On 22 September 2020 20:27, Rikard Falkeborn wrote: > Constify a number of static struct resource. The only usage of the > structs are to assign their address to the resources field in the > mfd_cell struct. This allows the compiler to put them in read-only > memory. Done with the help of Coccinelle. > > Signed-off-by: Rikard Falkeborn Acked-by: Adam Thomson
RE: [PATCH 09/42] mfd: da9150: use PLATFORM_DEVID_NONE
On 21 September 2020 21:50, Krzysztof Kozlowski wrote: > Use PLATFORM_DEVID_NONE define instead of "-1" value because: > - it brings some meaning, > - it might point attention why auto device ID was not used. > > Signed-off-by: Krzysztof Kozlowski Acked-by: Adam Thomson
RE: [PATCH 08/42] mfd: da9055: use PLATFORM_DEVID_NONE
On 21 September 2020 21:50, Krzysztof Kozlowski wrote: > Use PLATFORM_DEVID_NONE define instead of "-1" value because: > - it brings some meaning, > - it might point attention why auto device ID was not used. > > Signed-off-by: Krzysztof Kozlowski Acked-by: Adam Thomson
[PATCH] ASoC: da7219: Fix I/O voltage range configuration during probe
Previous improvements around handling device and codec level
probe functionality added the possibility of the voltage level
being undefined for the scenario where the IO voltage retrieved
from the regulator supply was below 1.2V, whereas previously the
code defaulted to the 2.5V to 3.6V range in that case. This
commit restores the default value to avoid this happening.
Fixes: aa5b18d1c290 ("ASoC: da7219: Move soft reset handling to codec level
probe")
Reported-by: Pierre-Louis Bossart
Signed-off-by: Adam Thomson
---
sound/soc/codecs/da7219.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index ddd422c..045aeb6 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1862,6 +1862,9 @@ static int da7219_handle_supplies(struct
snd_soc_component *component,
return ret;
}
+ /* Default to upper range */
+ *io_voltage_lvl = DA7219_IO_VOLTAGE_LEVEL_2_5V_3_6V;
+
/* Determine VDDIO voltage provided */
vddio = da7219->supplies[DA7219_SUPPLY_VDDIO].consumer;
ret = regulator_get_voltage(vddio);
@@ -1869,8 +1872,6 @@ static int da7219_handle_supplies(struct
snd_soc_component *component,
dev_warn(component->dev, "Invalid VDDIO voltage\n");
else if (ret < 280)
*io_voltage_lvl = DA7219_IO_VOLTAGE_LEVEL_1_2V_2_8V;
- else
- *io_voltage_lvl = DA7219_IO_VOLTAGE_LEVEL_2_5V_3_6V;
/* Enable main supplies */
ret = regulator_bulk_enable(DA7219_NUM_SUPPLIES, da7219->supplies);
--
1.9.1
RE: [PATCH v2 02/15] regulator: da9210: Fix W=1 build warning when CONFIG_OF=n
On 21 August 2020 04:13, Jisheng Zhang wrote:
> Fix below warning when CONFIG_OF=n:
>
> drivers/regulator/da9210-regulator.c:128:34: warning: ‘da9210_dt_ids’ defined
> but not used [-Wunused-const-variable=]
> 128 | static const struct of_device_id da9210_dt_ids[] = {
> | ^
>
> Signed-off-by: Jisheng Zhang
Acked-by: Adam Thomson
RE: [PATCH 0/3] ASoC: da7219: Reorganise device/codec level probe/remove
On 18 August 2020 17:39, Zhi, Yong wrote: > > This patch set reorganises and fixes device and codec level probe/remove > > handling within the driver, to allow clean probe and remove at the codec > > level. > > > > This set relates to an issue raised by Yong Zhi where a codec level re-probe > > would fail due to clks still being registered from the previous > > instantiation. > > In addition some improvements around regulator handling and soft reset have > > also been included. > > > > Just to let you know that I tested this series on the JSL platform, the > original issue > was resolved, also sanity checked basic use case like plug/unplug and > suspend/resume, no regression found. > > thanks, > yong That's great. Many thanks for verifying your side.
RE: [PATCH] regulator: da9211: add cache_type
On 12 August 2020 14:31, Hsin-Yi Wang wrote: > Add regmap_cache to reduce wakeups events of interrupt if regulator is > accessed frequently. This results in saving more power. > > Suggested-by: Daniel Kurtz > Signed-off-by: Hsin-Yi Wang Acked-by: Adam Thomson
[PATCH 3/3] ASoC: da7219: Fix clock handling around codec level probe
Previously the driver would use devm_* related functions at
the codec level probe() to allocate clock resources for MCLK
and the DAI clocks exposed by the device. This caused issues
when registering clocks on a re-probe (no device level
remove/prove involved) as the devm_* resources were never
freed up so the clocks were still registered from the previous
codec level probe().
This commit updates the clock handling for MCLK usage and DAI
clock provision to fix this discrepancy and allow the codec level
probe/remove functionality to operate as intended.
Signed-off-by: Adam Thomson
---
sound/soc/codecs/da7219.c | 96 +++
sound/soc/codecs/da7219.h | 1 +
2 files changed, 72 insertions(+), 25 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 0d1c70f8..ddd422c 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -2115,14 +2115,26 @@ static int da7219_bclk_set_rate(struct clk_hw *hw,
unsigned long rate,
static int da7219_register_dai_clks(struct snd_soc_component *component)
{
struct device *dev = component->dev;
+ struct device_node *np = dev->of_node;
struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
struct da7219_pdata *pdata = da7219->pdata;
const char *parent_name;
+ struct clk_hw_onecell_data *clk_data;
int i, ret;
+ /* For DT platforms allocate onecell data for clock registration */
+ if (np) {
+ clk_data = kzalloc(struct_size(clk_data, hws,
DA7219_DAI_NUM_CLKS),
+ GFP_KERNEL);
+ if (!clk_data)
+ return -ENOMEM;
+
+ clk_data->num = DA7219_DAI_NUM_CLKS;
+ da7219->clk_hw_data = clk_data;
+ }
+
for (i = 0; i < DA7219_DAI_NUM_CLKS; ++i) {
struct clk_init_data init = {};
- struct clk *dai_clk;
struct clk_lookup *dai_clk_lookup;
struct clk_hw *dai_clk_hw = >dai_clks_hw[i];
@@ -2158,22 +2170,20 @@ static int da7219_register_dai_clks(struct
snd_soc_component *component)
init.flags = CLK_GET_RATE_NOCACHE | CLK_SET_RATE_GATE;
dai_clk_hw->init =
- dai_clk = devm_clk_register(dev, dai_clk_hw);
- if (IS_ERR(dai_clk)) {
- dev_warn(dev, "Failed to register %s: %ld\n",
-init.name, PTR_ERR(dai_clk));
- ret = PTR_ERR(dai_clk);
+ ret = clk_hw_register(dev, dai_clk_hw);
+ if (ret) {
+ dev_warn(dev, "Failed to register %s: %d\n", init.name,
+ret);
goto err;
}
- da7219->dai_clks[i] = dai_clk;
+ da7219->dai_clks[i] = dai_clk_hw->clk;
- /* If we're using DT, then register as provider accordingly */
- if (dev->of_node) {
- devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
- dai_clk_hw);
+ /* For DT setup onecell data, otherwise create lookup */
+ if (np) {
+ da7219->clk_hw_data->hws[i] = dai_clk_hw;
} else {
- dai_clk_lookup = clkdev_create(dai_clk, init.name,
- "%s", dev_name(dev));
+ dai_clk_lookup = clkdev_hw_create(dai_clk_hw, init.name,
+ "%s", dev_name(dev));
if (!dai_clk_lookup) {
ret = -ENOMEM;
goto err;
@@ -2183,21 +2193,58 @@ static int da7219_register_dai_clks(struct
snd_soc_component *component)
}
}
+ /* If we're using DT, then register as provider accordingly */
+ if (np) {
+ ret = of_clk_add_hw_provider(dev->of_node,
of_clk_hw_onecell_get,
+da7219->clk_hw_data);
+ if (ret) {
+ dev_err(dev, "Failed to register clock provider\n");
+ goto err;
+ }
+ }
+
return 0;
err:
do {
if (da7219->dai_clks_lookup[i])
clkdev_drop(da7219->dai_clks_lookup[i]);
+
+ clk_hw_unregister(>dai_clks_hw[i]);
} while (i-- > 0);
+ if (np)
+ kfree(da7219->clk_hw_data);
+
return ret;
}
+
+static void da7219_free_dai_clks(struct snd_soc_component *component)
+{
+ struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
+ struct device_node *np = co
[PATCH 2/3] ASoC: da7219: Move soft reset handling to codec level probe
As part of the reorganisation of the device level and codec
level probe functionlity, the soft reset handling should really
reside at the codec level and after the instantiation of supplies.
This commit makes the relevant changes to support this change of
scope including the remove of devm_* functions being called for
regulator instantiation at the codec level.
Signed-off-by: Adam Thomson
---
sound/soc/codecs/da7219.c | 371 +++---
1 file changed, 188 insertions(+), 183 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 82d35bc..0d1c70f8 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1844,19 +1844,19 @@ static int da7219_set_bias_level(struct
snd_soc_component *component,
[DA7219_SUPPLY_VDDIO] = "VDDIO",
};
-static int da7219_handle_supplies(struct snd_soc_component *component)
+static int da7219_handle_supplies(struct snd_soc_component *component,
+ u8 *io_voltage_lvl)
{
struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
struct regulator *vddio;
- u8 io_voltage_lvl = DA7219_IO_VOLTAGE_LEVEL_2_5V_3_6V;
int i, ret;
/* Get required supplies */
for (i = 0; i < DA7219_NUM_SUPPLIES; ++i)
da7219->supplies[i].supply = da7219_supply_names[i];
- ret = devm_regulator_bulk_get(component->dev, DA7219_NUM_SUPPLIES,
- da7219->supplies);
+ ret = regulator_bulk_get(component->dev, DA7219_NUM_SUPPLIES,
+da7219->supplies);
if (ret) {
dev_err(component->dev, "Failed to get supplies");
return ret;
@@ -1868,21 +1868,18 @@ static int da7219_handle_supplies(struct
snd_soc_component *component)
if (ret < 120)
dev_warn(component->dev, "Invalid VDDIO voltage\n");
else if (ret < 280)
- io_voltage_lvl = DA7219_IO_VOLTAGE_LEVEL_1_2V_2_8V;
+ *io_voltage_lvl = DA7219_IO_VOLTAGE_LEVEL_1_2V_2_8V;
+ else
+ *io_voltage_lvl = DA7219_IO_VOLTAGE_LEVEL_2_5V_3_6V;
/* Enable main supplies */
ret = regulator_bulk_enable(DA7219_NUM_SUPPLIES, da7219->supplies);
if (ret) {
dev_err(component->dev, "Failed to enable supplies");
+ regulator_bulk_free(DA7219_NUM_SUPPLIES, da7219->supplies);
return ret;
}
- /* Ensure device in active mode */
- snd_soc_component_write(component, DA7219_SYSTEM_ACTIVE,
DA7219_SYSTEM_ACTIVE_MASK);
-
- /* Update IO voltage level range */
- snd_soc_component_write(component, DA7219_IO_CTRL, io_voltage_lvl);
-
return 0;
}
@@ -2248,6 +2245,142 @@ static void da7219_handle_pdata(struct
snd_soc_component *component)
}
}
+
+/*
+ * Regmap configs
+ */
+
+static struct reg_default da7219_reg_defaults[] = {
+ { DA7219_MIC_1_SELECT, 0x00 },
+ { DA7219_CIF_TIMEOUT_CTRL, 0x01 },
+ { DA7219_SR_24_48, 0x00 },
+ { DA7219_SR, 0x0A },
+ { DA7219_CIF_I2C_ADDR_CFG, 0x02 },
+ { DA7219_PLL_CTRL, 0x10 },
+ { DA7219_PLL_FRAC_TOP, 0x00 },
+ { DA7219_PLL_FRAC_BOT, 0x00 },
+ { DA7219_PLL_INTEGER, 0x20 },
+ { DA7219_DIG_ROUTING_DAI, 0x10 },
+ { DA7219_DAI_CLK_MODE, 0x01 },
+ { DA7219_DAI_CTRL, 0x28 },
+ { DA7219_DAI_TDM_CTRL, 0x40 },
+ { DA7219_DIG_ROUTING_DAC, 0x32 },
+ { DA7219_DAI_OFFSET_LOWER, 0x00 },
+ { DA7219_DAI_OFFSET_UPPER, 0x00 },
+ { DA7219_REFERENCES, 0x08 },
+ { DA7219_MIXIN_L_SELECT, 0x00 },
+ { DA7219_MIXIN_L_GAIN, 0x03 },
+ { DA7219_ADC_L_GAIN, 0x6F },
+ { DA7219_ADC_FILTERS1, 0x80 },
+ { DA7219_MIC_1_GAIN, 0x01 },
+ { DA7219_SIDETONE_CTRL, 0x40 },
+ { DA7219_SIDETONE_GAIN, 0x0E },
+ { DA7219_DROUTING_ST_OUTFILT_1L, 0x01 },
+ { DA7219_DROUTING_ST_OUTFILT_1R, 0x02 },
+ { DA7219_DAC_FILTERS5, 0x00 },
+ { DA7219_DAC_FILTERS2, 0x88 },
+ { DA7219_DAC_FILTERS3, 0x88 },
+ { DA7219_DAC_FILTERS4, 0x08 },
+ { DA7219_DAC_FILTERS1, 0x80 },
+ { DA7219_DAC_L_GAIN, 0x6F },
+ { DA7219_DAC_R_GAIN, 0x6F },
+ { DA7219_CP_CTRL, 0x20 },
+ { DA7219_HP_L_GAIN, 0x39 },
+ { DA7219_HP_R_GAIN, 0x39 },
+ { DA7219_MIXOUT_L_SELECT, 0x00 },
+ { DA7219_MIXOUT_R_SELECT, 0x00 },
+ { DA7219_MICBIAS_CTRL, 0x03 },
+ { DA7219_MIC_1_CTRL, 0x40 },
+ { DA7219_MIXIN_L_CTRL, 0x40 },
+ { DA7219_ADC_L_CTRL, 0x40 },
+ { DA7219_DAC_L_CTRL, 0x40 },
+ { DA7219_DAC_R_CTRL, 0x40 },
+ { DA7219_HP_L_CTRL, 0x40 },
+ { DA7219_HP_R_CTRL, 0x40 },
+ { DA7219_MIXOUT_L_CTRL, 0x10 },
+ { DA7219_MIXOUT_R_CTRL, 0x10 },
+ { DA7219_CHIP
[PATCH 1/3] ASoC: da7219: Move required devm_* allocations to device level code
In preparation for cleanup of device level and codec level probe
funcitonality, all necessary devm_* allocations and fw retrieval
functions are moved to the I2C probe level code.
Signed-off-by: Adam Thomson
---
sound/soc/codecs/da7219-aad.c | 85 +--
sound/soc/codecs/da7219-aad.h | 3 ++
sound/soc/codecs/da7219.c | 30 ---
3 files changed, 70 insertions(+), 48 deletions(-)
diff --git a/sound/soc/codecs/da7219-aad.c b/sound/soc/codecs/da7219-aad.c
index 4f2a96e..3827734 100644
--- a/sound/soc/codecs/da7219-aad.c
+++ b/sound/soc/codecs/da7219-aad.c
@@ -460,7 +460,7 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void
*data)
*/
static enum da7219_aad_micbias_pulse_lvl
- da7219_aad_fw_micbias_pulse_lvl(struct snd_soc_component *component,
u32 val)
+ da7219_aad_fw_micbias_pulse_lvl(struct device *dev, u32 val)
{
switch (val) {
case 2800:
@@ -468,13 +468,13 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void
*data)
case 2900:
return DA7219_AAD_MICBIAS_PULSE_LVL_2_9V;
default:
- dev_warn(component->dev, "Invalid micbias pulse level");
+ dev_warn(dev, "Invalid micbias pulse level");
return DA7219_AAD_MICBIAS_PULSE_LVL_OFF;
}
}
static enum da7219_aad_btn_cfg
- da7219_aad_fw_btn_cfg(struct snd_soc_component *component, u32 val)
+ da7219_aad_fw_btn_cfg(struct device *dev, u32 val)
{
switch (val) {
case 2:
@@ -492,13 +492,13 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void
*data)
case 500:
return DA7219_AAD_BTN_CFG_500MS;
default:
- dev_warn(component->dev, "Invalid button config");
+ dev_warn(dev, "Invalid button config");
return DA7219_AAD_BTN_CFG_10MS;
}
}
static enum da7219_aad_mic_det_thr
- da7219_aad_fw_mic_det_thr(struct snd_soc_component *component, u32 val)
+ da7219_aad_fw_mic_det_thr(struct device *dev, u32 val)
{
switch (val) {
case 200:
@@ -510,13 +510,13 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void
*data)
case 1000:
return DA7219_AAD_MIC_DET_THR_1000_OHMS;
default:
- dev_warn(component->dev, "Invalid mic detect threshold");
+ dev_warn(dev, "Invalid mic detect threshold");
return DA7219_AAD_MIC_DET_THR_500_OHMS;
}
}
static enum da7219_aad_jack_ins_deb
- da7219_aad_fw_jack_ins_deb(struct snd_soc_component *component, u32 val)
+ da7219_aad_fw_jack_ins_deb(struct device *dev, u32 val)
{
switch (val) {
case 5:
@@ -536,13 +536,13 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void
*data)
case 1000:
return DA7219_AAD_JACK_INS_DEB_1S;
default:
- dev_warn(component->dev, "Invalid jack insert debounce");
+ dev_warn(dev, "Invalid jack insert debounce");
return DA7219_AAD_JACK_INS_DEB_20MS;
}
}
static enum da7219_aad_jack_det_rate
- da7219_aad_fw_jack_det_rate(struct snd_soc_component *component, const
char *str)
+ da7219_aad_fw_jack_det_rate(struct device *dev, const char *str)
{
if (!strcmp(str, "32ms_64ms")) {
return DA7219_AAD_JACK_DET_RATE_32_64MS;
@@ -553,13 +553,13 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void
*data)
} else if (!strcmp(str, "256ms_512ms")) {
return DA7219_AAD_JACK_DET_RATE_256_512MS;
} else {
- dev_warn(component->dev, "Invalid jack detect rate");
+ dev_warn(dev, "Invalid jack detect rate");
return DA7219_AAD_JACK_DET_RATE_256_512MS;
}
}
static enum da7219_aad_jack_rem_deb
- da7219_aad_fw_jack_rem_deb(struct snd_soc_component *component, u32 val)
+ da7219_aad_fw_jack_rem_deb(struct device *dev, u32 val)
{
switch (val) {
case 1:
@@ -571,13 +571,13 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void
*data)
case 20:
return DA7219_AAD_JACK_REM_DEB_20MS;
default:
- dev_warn(component->dev, "Invalid jack removal debounce");
+ dev_warn(dev, "Invalid jack removal debounce");
return DA7219_AAD_JACK_REM_DEB_1MS;
}
}
static enum da7219_aad_btn_avg
- da7219_aad_fw_btn_avg(struct snd_soc_component *component, u32 val)
+ da7219_aad_fw_btn_avg(struct device *dev, u32 val)
{
switch (val) {
case 1:
@@ -589,13 +589,13 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void
*data)
case 8:
return DA7219_AAD_BTN_AVG_8;
default:
- dev_wa
[PATCH 0/3] ASoC: da7219: Reorganise device/codec level probe/remove
This patch set reorganises and fixes device and codec level probe/remove handling within the driver, to allow clean probe and remove at the codec level. This set relates to an issue raised by Yong Zhi where a codec level re-probe would fail due to clks still being registered from the previous instantiation. In addition some improvements around regulator handling and soft reset have also been included. Adam Thomson (3): ASoC: da7219: Move required devm_* allocations to device level code ASoC: da7219: Move soft reset handling to codec level probe ASoC: da7219: Fix clock handling around codec level probe sound/soc/codecs/da7219-aad.c | 85 +--- sound/soc/codecs/da7219-aad.h | 3 + sound/soc/codecs/da7219.c | 493 +++--- sound/soc/codecs/da7219.h | 1 + 4 files changed, 328 insertions(+), 254 deletions(-) -- 1.9.1
RE: [PATCH 2/3] regulator: remove locking around regulator_notifier_call_chain()
On 10 August 2020 05:34, Michał Mirosław wrote: > regulator_notifier_call_chain() doesn't need rdev lock and rdev's > existence is assumed in the code anyway. Remove the locks from drivers. > > Signed-off-by: Michał Mirosław For da9*-regulator.c, pv88*-regulator.c and slg51000-regulator.c: Acked-by: Adam Thomson > --- > drivers/regulator/da9055-regulator.c | 2 -- > drivers/regulator/da9062-regulator.c | 2 -- > drivers/regulator/da9063-regulator.c | 2 -- > drivers/regulator/da9210-regulator.c | 4 > drivers/regulator/da9211-regulator.c | 4 > drivers/regulator/lp8755.c | 6 -- > drivers/regulator/ltc3589.c| 10 ++ > drivers/regulator/ltc3676.c| 10 ++ > drivers/regulator/pv88060-regulator.c | 10 ++ > drivers/regulator/pv88080-regulator.c | 10 ++ > drivers/regulator/pv88090-regulator.c | 10 ++ > drivers/regulator/slg51000-regulator.c | 4 > drivers/regulator/stpmic1_regulator.c | 4 > drivers/regulator/wm831x-dcdc.c| 4 > drivers/regulator/wm831x-isink.c | 2 -- > drivers/regulator/wm831x-ldo.c | 2 -- > drivers/regulator/wm8350-regulator.c | 2 -- > 17 files changed, 10 insertions(+), 78 deletions(-)
RE: [PATCH] MFD: da9055/pdata.h: drop a duplicated word
On 19 July 2020 01:29, Randy Dunlap wrote:
> Drop the repeated word "that" in a comment.
>
> Signed-off-by: Randy Dunlap
> Cc: Support Opensource
> Cc: Lee Jones
Acked-by: Adam Thomson
> ---
> include/linux/mfd/da9055/pdata.h |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- linux-next-20200717.orig/include/linux/mfd/da9055/pdata.h
> +++ linux-next-20200717/include/linux/mfd/da9055/pdata.h
> @@ -35,7 +35,7 @@ struct da9055_pdata {
> int *gpio_rsel;
> /*
>* Regulator mode control bits value (GPI offset) that
> - * that controls the regulator state, 0 if not available.
> + * controls the regulator state, 0 if not available.
>*/
> enum gpio_select *reg_ren;
> /*
[RESEND PATCH v3 2/2] mfd: da9063: Add support for latest DA silicon revision
This update adds new regmap tables to support the latest DA silicon
which will automatically be selected based on the chip and variant
information read from the device.
Signed-off-by: Adam Thomson
Acked-for-MFD-by: Lee Jones
---
v3:
- No change
v2:
- No change
drivers/mfd/da9063-i2c.c| 91 -
include/linux/mfd/da9063/core.h | 1 +
2 files changed, 82 insertions(+), 10 deletions(-)
diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
index 4815489..b8217ad 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -197,7 +197,7 @@ static int da9063_get_device_type(struct i2c_client *i2c,
struct da9063 *da9063)
regmap_reg_range(DA9063_BB_REG_GP_ID_0, DA9063_BB_REG_GP_ID_19),
};
-static const struct regmap_range da9063_bb_volatile_ranges[] = {
+static const struct regmap_range da9063_bb_da_volatile_ranges[] = {
regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_EVENT_D),
regmap_reg_range(DA9063_REG_CONTROL_A, DA9063_REG_CONTROL_B),
regmap_reg_range(DA9063_REG_CONTROL_E, DA9063_REG_CONTROL_F),
@@ -219,9 +219,9 @@ static int da9063_get_device_type(struct i2c_client *i2c,
struct da9063 *da9063)
.n_yes_ranges = ARRAY_SIZE(da9063_bb_writeable_ranges),
};
-static const struct regmap_access_table da9063_bb_volatile_table = {
- .yes_ranges = da9063_bb_volatile_ranges,
- .n_yes_ranges = ARRAY_SIZE(da9063_bb_volatile_ranges),
+static const struct regmap_access_table da9063_bb_da_volatile_table = {
+ .yes_ranges = da9063_bb_da_volatile_ranges,
+ .n_yes_ranges = ARRAY_SIZE(da9063_bb_da_volatile_ranges),
};
static const struct regmap_range da9063l_bb_readable_ranges[] = {
@@ -241,7 +241,7 @@ static int da9063_get_device_type(struct i2c_client *i2c,
struct da9063 *da9063)
regmap_reg_range(DA9063_BB_REG_GP_ID_0, DA9063_BB_REG_GP_ID_19),
};
-static const struct regmap_range da9063l_bb_volatile_ranges[] = {
+static const struct regmap_range da9063l_bb_da_volatile_ranges[] = {
regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_EVENT_D),
regmap_reg_range(DA9063_REG_CONTROL_A, DA9063_REG_CONTROL_B),
regmap_reg_range(DA9063_REG_CONTROL_E, DA9063_REG_CONTROL_F),
@@ -263,9 +263,64 @@ static int da9063_get_device_type(struct i2c_client *i2c,
struct da9063 *da9063)
.n_yes_ranges = ARRAY_SIZE(da9063l_bb_writeable_ranges),
};
-static const struct regmap_access_table da9063l_bb_volatile_table = {
- .yes_ranges = da9063l_bb_volatile_ranges,
- .n_yes_ranges = ARRAY_SIZE(da9063l_bb_volatile_ranges),
+static const struct regmap_access_table da9063l_bb_da_volatile_table = {
+ .yes_ranges = da9063l_bb_da_volatile_ranges,
+ .n_yes_ranges = ARRAY_SIZE(da9063l_bb_da_volatile_ranges),
+};
+
+static const struct regmap_range da9063_da_readable_ranges[] = {
+ regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_BB_REG_SECOND_D),
+ regmap_reg_range(DA9063_REG_SEQ, DA9063_REG_ID_32_31),
+ regmap_reg_range(DA9063_REG_SEQ_A, DA9063_REG_AUTO3_LOW),
+ regmap_reg_range(DA9063_REG_T_OFFSET, DA9063_BB_REG_GP_ID_11),
+ regmap_reg_range(DA9063_REG_DEVICE_ID, DA9063_REG_VARIANT_ID),
+};
+
+static const struct regmap_range da9063_da_writeable_ranges[] = {
+ regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_PAGE_CON),
+ regmap_reg_range(DA9063_REG_FAULT_LOG, DA9063_REG_VSYS_MON),
+ regmap_reg_range(DA9063_REG_COUNT_S, DA9063_BB_REG_ALARM_Y),
+ regmap_reg_range(DA9063_REG_SEQ, DA9063_REG_ID_32_31),
+ regmap_reg_range(DA9063_REG_SEQ_A, DA9063_REG_AUTO3_LOW),
+ regmap_reg_range(DA9063_REG_CONFIG_I, DA9063_BB_REG_MON_REG_4),
+ regmap_reg_range(DA9063_BB_REG_GP_ID_0, DA9063_BB_REG_GP_ID_11),
+};
+
+static const struct regmap_access_table da9063_da_readable_table = {
+ .yes_ranges = da9063_da_readable_ranges,
+ .n_yes_ranges = ARRAY_SIZE(da9063_da_readable_ranges),
+};
+
+static const struct regmap_access_table da9063_da_writeable_table = {
+ .yes_ranges = da9063_da_writeable_ranges,
+ .n_yes_ranges = ARRAY_SIZE(da9063_da_writeable_ranges),
+};
+
+static const struct regmap_range da9063l_da_readable_ranges[] = {
+ regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_MON_A10_RES),
+ regmap_reg_range(DA9063_REG_SEQ, DA9063_REG_ID_32_31),
+ regmap_reg_range(DA9063_REG_SEQ_A, DA9063_REG_AUTO3_LOW),
+ regmap_reg_range(DA9063_REG_T_OFFSET, DA9063_BB_REG_GP_ID_11),
+ regmap_reg_range(DA9063_REG_DEVICE_ID, DA9063_REG_VARIANT_ID),
+};
+
+static const struct regmap_range da9063l_da_writeable_ranges[] = {
+ regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_PAGE_CON),
+ regmap_reg_range(DA9063_REG_FAULT_LOG, DA9063_REG_VSYS_MON),
+ regmap_reg_range(DA9063_REG_SEQ, DA9063_REG_ID_32_31),
+ regmap_reg_range(DA9063_REG_SEQ_A, DA9063_REG_AUTO3_LOW),
+ regmap_reg_range(DA9063_REG_CONFIG_I, DA9063_BB_REG_MON_REG_4
[RESEND PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
The current implementation performs checking in the i2c_probe()
function of the variant_code but does this immediately after the
containing struct has been initialised as all zero. This means the
check for variant code will always default to using the BB tables
and will never select AD. The variant code is subsequently set
by device_init() and later used by the RTC so really it's a little
fortunate this mismatch works.
This update adds raw I2C read access functionality to read the chip
and variant/revision information (common to all revisions) so that
it can subsequently correctly choose the proper regmap tables for
real initialisation.
Signed-off-by: Adam Thomson
---
v3:
- Replaced magic numbers around I2C paged access code with enums and defines
- Small style tidy ups as requested by Lee Jones
v2:
- Use raw I2C read access instead of a temporary regmap to interrogate chip and
variant id registers
drivers/mfd/da9063-core.c| 31 --
drivers/mfd/da9063-i2c.c | 184 +++
include/linux/mfd/da9063/registers.h | 15 ++-
3 files changed, 177 insertions(+), 53 deletions(-)
diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
index b125f90d..a353d52 100644
--- a/drivers/mfd/da9063-core.c
+++ b/drivers/mfd/da9063-core.c
@@ -160,7 +160,6 @@ static int da9063_clear_fault_log(struct da9063 *da9063)
int da9063_device_init(struct da9063 *da9063, unsigned int irq)
{
- int model, variant_id, variant_code;
int ret;
ret = da9063_clear_fault_log(da9063);
@@ -171,36 +170,6 @@ int da9063_device_init(struct da9063 *da9063, unsigned int
irq)
da9063->irq_base = -1;
da9063->chip_irq = irq;
- ret = regmap_read(da9063->regmap, DA9063_REG_CHIP_ID, );
- if (ret < 0) {
- dev_err(da9063->dev, "Cannot read chip model id.\n");
- return -EIO;
- }
- if (model != PMIC_CHIP_ID_DA9063) {
- dev_err(da9063->dev, "Invalid chip model id: 0x%02x\n", model);
- return -ENODEV;
- }
-
- ret = regmap_read(da9063->regmap, DA9063_REG_CHIP_VARIANT, _id);
- if (ret < 0) {
- dev_err(da9063->dev, "Cannot read chip variant id.\n");
- return -EIO;
- }
-
- variant_code = variant_id >> DA9063_CHIP_VARIANT_SHIFT;
-
- dev_info(da9063->dev,
-"Device detected (chip-ID: 0x%02X, var-ID: 0x%02X)\n",
-model, variant_id);
-
- if (variant_code < PMIC_DA9063_BB && variant_code != PMIC_DA9063_AD) {
- dev_err(da9063->dev,
- "Cannot support variant code: 0x%02X\n", variant_code);
- return -ENODEV;
- }
-
- da9063->variant_code = variant_code;
-
ret = da9063_irq_init(da9063);
if (ret) {
dev_err(da9063->dev, "Cannot initialize interrupts.\n");
diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
index 455de74..4815489 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -22,12 +22,124 @@
#include
#include
+/*
+ * Raw I2C access required for just accessing chip and variant info before we
+ * know which device is present. The info read from the device using this
+ * approach is then used to select the correct regmap tables.
+ */
+
+#define DA9063_REG_PAGE_SIZE 0x100
+#define DA9063_REG_PAGED_ADDR_MASK 0xFF
+
+enum da9063_page_sel_buf_fmt {
+ DA9063_PAGE_SEL_BUF_PAGE_REG = 0,
+ DA9063_PAGE_SEL_BUF_PAGE_VAL,
+ DA9063_PAGE_SEL_BUF_SIZE,
+};
+
+enum da9063_paged_read_msgs {
+ DA9063_PAGED_READ_MSG_PAGE_SEL = 0,
+ DA9063_PAGED_READ_MSG_REG_SEL,
+ DA9063_PAGED_READ_MSG_DATA,
+ DA9063_PAGED_READ_MSG_CNT,
+};
+
+static int da9063_i2c_blockreg_read(struct i2c_client *client, u16 addr,
+ u8 *buf, int count)
+{
+ struct i2c_msg xfer[DA9063_PAGED_READ_MSG_CNT];
+ u8 page_sel_buf[DA9063_PAGE_SEL_BUF_SIZE];
+ u8 page_num, paged_addr;
+ int ret;
+
+ /* Determine page info based on register address */
+ page_num = (addr / DA9063_REG_PAGE_SIZE);
+ if (page_num > 1) {
+ dev_err(>dev, "Invalid register address provided\n");
+ return -EINVAL;
+ }
+
+ paged_addr = (addr % DA9063_REG_PAGE_SIZE) & DA9063_REG_PAGED_ADDR_MASK;
+ page_sel_buf[DA9063_PAGE_SEL_BUF_PAGE_REG] = DA9063_REG_PAGE_CON;
+ page_sel_buf[DA9063_PAGE_SEL_BUF_PAGE_VAL] =
+ (page_num << DA9063_I2C_PAGE_SEL_SHIFT) & DA9063_REG_PAGE_MASK;
+
+ /* Write reg address, page selection */
+ xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].addr = client->addr;
+ xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].flags = 0;
+ xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].
[RESEND PATCH v3 0/2] Resolve revision handling and add support for DA silicon
This patch set fixes the currently broken revision handling in the driver's i2c_probe() function and then adds DA support to existing permitted revisions. v3: - Replaced magic numbers around I2C paged access code with enums and defines - Small style tidy ups as requested by Lee Jones v2: - Use raw I2C read access instead of a temporary regmap to interrogate chip and variant id registers Adam Thomson (2): mfd: da9063: Fix revision handling to correctly select reg tables mfd: da9063: Add support for latest DA silicon revision drivers/mfd/da9063-core.c| 31 drivers/mfd/da9063-i2c.c | 271 +++ include/linux/mfd/da9063/core.h | 1 + include/linux/mfd/da9063/registers.h | 15 +- 4 files changed, 257 insertions(+), 61 deletions(-) -- 1.9.1
RE: [PATCH] Replace HTTP links with HTTPS ones: Dialog Semiconductor drivers
On 05 July 2020 08:56, Alexander A. Klimov wrote: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > > Deterministic algorithm: > For each file: > If not .svg: > For each line: > If doesn't contain `\bxmlns\b`: > For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`: > If both the HTTP and HTTPS versions > return 200 OK and serve the same content: > Replace HTTP with HTTPS. > > Signed-off-by: Alexander A. Klimov Acked-by: Adam Thomson > --- > Continuing my work started at 93431e0607e5. > > If there are any URLs to be removed completely or at least not HTTPSified: > Just clearly say so and I'll *undo my change*. > See also https://lkml.org/lkml/2020/6/27/64 > > If there are any valid, but yet not changed URLs: > See https://lkml.org/lkml/2020/6/26/837 > > Documentation/devicetree/bindings/mfd/da9062.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt > b/Documentation/devicetree/bindings/mfd/da9062.txt > index 857af982c88f..bab0d0e66cb3 100644 > --- a/Documentation/devicetree/bindings/mfd/da9062.txt > +++ b/Documentation/devicetree/bindings/mfd/da9062.txt > @@ -1,8 +1,8 @@ > * Dialog DA9062 Power Management Integrated Circuit (PMIC) > > Product information for the DA9062 and DA9061 devices can be found here: > -- http://www.dialog-semiconductor.com/products/da9062 > -- http://www.dialog-semiconductor.com/products/da9061 > +- https://www.dialog-semiconductor.com/products/da9062 > +- https://www.dialog-semiconductor.com/products/da9061 > > The DA9062 PMIC consists of: > > -- > 2.27.0
RE: [PATCH 04/11] ASoC: codecs: da7219: fix 'defined but not used' warning
On 03 July 2020 10:33, Adam Thomson wrote
> On 01 July 2020 19:24, Pierre-Louis Bossart wrote:
>
> > fix W=1 warning
> >
> > sound/soc/codecs/da7219.c:1711:36: warning: 'da7219_acpi_match'
> > defined but not used [-Wunused-const-variable=]
> > 1711 | static const struct acpi_device_id da7219_acpi_match[] = {
> > |^
> >
> > Signed-off-by: Pierre-Louis Bossart
> > ---
> > sound/soc/codecs/da7219.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> > index f2520a6c7875..153ea30b5a8f 100644
> > --- a/sound/soc/codecs/da7219.c
> > +++ b/sound/soc/codecs/da7219.c
> > @@ -1708,11 +1708,13 @@ static const struct of_device_id da7219_of_match[]
> =
> > {
> > };
> > MODULE_DEVICE_TABLE(of, da7219_of_match);
> >
> > +#ifdef CONFIG_ACPI
> > static const struct acpi_device_id da7219_acpi_match[] = {
> > { .id = "DLGS7219", },
> > { }
> > };
> > MODULE_DEVICE_TABLE(acpi, da7219_acpi_match);
> > +#endif
>
> I think this will break non-ACPI builds as this symbol is used in the
> declaration of 'da7219_i2c_driver', without conditional compilation
> surrounding
> it. Unless of course I'm missing something. Could we instead use
> '__maybe_unused' to avoid this warning?
Obviously a slow start to the day for my brain. You've obviously built test this
given the intention behind this is for non-ACPI builds. Will get more coffee
shortly.
Still wonder if '__maybe_unused' might be nicer as per suspend/resume functions
on platforms which don't include PM_OPS. Either way though:
Reviewed-by: Adam Thomson
>
> >
> > static enum da7219_micbias_voltage
> > da7219_fw_micbias_lvl(struct device *dev, u32 val)
> > --
> > 2.25.1
RE: [PATCHv4] ASoC: da7213: add default clock handling
On 26 June 2020 17:46, Sebastian Reichel wrote:
> This adds default clock/PLL configuration to the driver
> for usage with generic drivers like simple-card for usage
> with a fixed rate clock.
>
> Signed-off-by: Sebastian Reichel
Looks good. Thanks for the work here.
Reviewed-by: Adam Thomson
> ---
> Changes since PATCHv3:
> * rebase to v5.8-rc1
> * add SRM support for usage in slave mode with simple-card.
>I only tested with master mode, though.
> ---
> sound/soc/codecs/da7213.c | 83
> ---
> sound/soc/codecs/da7213.h | 2 +
> 2 files changed, 80 insertions(+), 5 deletions(-)
>
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index 3e6ad996741b..1a4fece20bcd 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -1156,6 +1156,7 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
> struct snd_soc_dai *dai)
> {
> struct snd_soc_component *component = dai->component;
> + struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> u8 dai_ctrl = 0;
> u8 fs;
>
> @@ -1181,33 +1182,43 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
> switch (params_rate(params)) {
> case 8000:
> fs = DA7213_SR_8000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 11025:
> fs = DA7213_SR_11025;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 12000:
> fs = DA7213_SR_12000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 16000:
> fs = DA7213_SR_16000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 22050:
> fs = DA7213_SR_22050;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 32000:
> fs = DA7213_SR_32000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 44100:
> fs = DA7213_SR_44100;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 48000:
> fs = DA7213_SR_48000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 88200:
> fs = DA7213_SR_88200;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 96000:
> fs = DA7213_SR_96000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> default:
> return -EINVAL;
> @@ -1392,9 +1403,9 @@ static int da7213_set_component_sysclk(struct
> snd_soc_component *component,
> }
>
> /* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */
> -static int da7213_set_component_pll(struct snd_soc_component *component,
> - int pll_id, int source,
> - unsigned int fref, unsigned int fout)
> +static int _da7213_set_component_pll(struct snd_soc_component
> *component,
> + int pll_id, int source,
> + unsigned int fref, unsigned int fout)
> {
> struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
>
> @@ -1503,6 +1514,16 @@ static int da7213_set_component_pll(struct
> snd_soc_component *component,
> return 0;
> }
>
> +static int da7213_set_component_pll(struct snd_soc_component *component,
> + int pll_id, int source,
> + unsigned int fref, unsigned int fout)
> +{
> + struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> + da7213->fixed_clk_auto_pll = false;
> +
> + return _da7213_set_component_pll(component, pll_id, source, fref,
> fout);
> +}
> +
> /* DAI operations */
> static const struct snd_soc_dai_ops da7213_dai_ops = {
> .hw_params = da7213_hw_params,
> @@ -1532,6 +1553,50 @@ static struct snd_soc_dai_driver da7213_dai = {
> .symmetric_rates = 1,
> };
>
> +static int da7213_set_auto_pll(struct snd_soc_component *component, bool
> enable)
> +{
> + struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> + int mode;
> +
> + if (!da7213->fixed_clk_auto_pll)
> + return 0;
> +
> + da7213->mclk_rate = cl
RE: [PATCH 04/11] ASoC: codecs: da7219: fix 'defined but not used' warning
On 01 July 2020 19:24, Pierre-Louis Bossart wrote:
> fix W=1 warning
>
> sound/soc/codecs/da7219.c:1711:36: warning: 'da7219_acpi_match'
> defined but not used [-Wunused-const-variable=]
> 1711 | static const struct acpi_device_id da7219_acpi_match[] = {
> |^
>
> Signed-off-by: Pierre-Louis Bossart
> ---
> sound/soc/codecs/da7219.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> index f2520a6c7875..153ea30b5a8f 100644
> --- a/sound/soc/codecs/da7219.c
> +++ b/sound/soc/codecs/da7219.c
> @@ -1708,11 +1708,13 @@ static const struct of_device_id da7219_of_match[] =
> {
> };
> MODULE_DEVICE_TABLE(of, da7219_of_match);
>
> +#ifdef CONFIG_ACPI
> static const struct acpi_device_id da7219_acpi_match[] = {
> { .id = "DLGS7219", },
> { }
> };
> MODULE_DEVICE_TABLE(acpi, da7219_acpi_match);
> +#endif
I think this will break non-ACPI builds as this symbol is used in the
declaration of 'da7219_i2c_driver', without conditional compilation surrounding
it. Unless of course I'm missing something. Could we instead use
'__maybe_unused' to avoid this warning?
>
> static enum da7219_micbias_voltage
> da7219_fw_micbias_lvl(struct device *dev, u32 val)
> --
> 2.25.1
RE: [PATCH 03/11] ASoC: codecs: da7210: fix kernel-doc
On 01 July 2020 19:24, Pierre-Louis Bossart wrote: > Fix W=1 warning, the kernel-doc syntax was probably from Doxygen? > > Signed-off-by: Pierre-Louis Bossart So is the issue that the Kernel warning mechanism doesn't like Doxygen syntax? If so then: Acked-by: Adam Thomson > --- > sound/soc/codecs/da7210.c | 12 +++- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/sound/soc/codecs/da7210.c b/sound/soc/codecs/da7210.c > index 0c99dcf242e4..2bb727dd3a20 100644 > --- a/sound/soc/codecs/da7210.c > +++ b/sound/soc/codecs/da7210.c > @@ -971,14 +971,16 @@ static int da7210_set_dai_sysclk(struct snd_soc_dai > *codec_dai, > > /** > * da7210_set_dai_pll:Configure the codec PLL > - * @param codec_dai : pointer to codec DAI > - * @param pll_id : da7210 has only one pll, so pll_id is always zero > - * @param fref : MCLK frequency, should be < 20MHz > - * @param fout : FsDM value, Refer page 44 & 45 of datasheet > - * @return int : Zero for success, negative error code for > error > + * @codec_dai: pointer to codec DAI > + * @pll_id: da7210 has only one pll, so pll_id is always zero > + * @source: clock source > + * @fref: MCLK frequency, should be < 20MHz > + * @fout: FsDM value, Refer page 44 & 45 of datasheet > * > * Note: Supported PLL input frequencies are 12MHz, 13MHz, 13.5MHz, 14.4MHz, > * 19.2MHz, 19.6MHz and 19.8MHz > + * > + * Return: Zero for success, negative error code for error > */ > static int da7210_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id, > int source, unsigned int fref, unsigned int fout) > -- > 2.25.1
RE: [RESEND 09/10] regulator: da9063-regulator: Fix .suspend 'initialized field overwritten' warnings
On 25 June 2020 20:17, Lee Jones wrote:
> Looks as though 99f75ce666199 ("regulator: da9063: fix suspend") failed
> to clean-up all of the existing .suspend variable initialisations.
> This has led to some 'initilized field overwritten' issues now present
> in W=1 builds.
You probably were not on the CC but I believe the following patch has already
resolved this issue, and has been pulled by Mark:
[PATCH] regulator: da9063: fix LDO9 suspend and warning.
Repo: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git/
Branch: for-next
Commit: d7442ba13d62de9afc4e57344a676f9f4623dcaa
>
> This patch squashes the following build warnings:
>
> In file included from drivers/regulator/da9063-regulator.c:17:
> include/linux/regmap.h:1124:37: warning: initialized field overwritten [-
> Woverride-init]
> 1124 | #define REG_FIELD(_reg, _lsb, _msb) { | ^
> drivers/regulator/da9063-regulator.c:27:2: note: in expansion of macro
> ‘REG_FIELD’
> 27 | REG_FIELD(_reg, __builtin_ffs((int)_mask) - 1, | ^
> drivers/regulator/da9063-regulator.c:515:14: note: in expansion of macro
> ‘BFIELD’
> 515 | .suspend = BFIELD(DA9063_REG_LDO9_CONT, DA9063_VLDO9_SEL),
> | ^~
> include/linux/regmap.h:1124:37: note: (near initialization for
> ‘da9063_regulator_info[11].suspend’)
> 1124 | #define REG_FIELD(_reg, _lsb, _msb) { | ^
> drivers/regulator/da9063-regulator.c:27:2: note: in expansion of macro
> ‘REG_FIELD’
> 27 | REG_FIELD(_reg, __builtin_ffs((int)_mask) - 1, | ^
> drivers/regulator/da9063-regulator.c:515:14: note: in expansion of macro
> ‘BFIELD’
> 515 | .suspend = BFIELD(DA9063_REG_LDO9_CONT, DA9063_VLDO9_SEL),
> | ^~
>
> Cc: Krystian Garbaciak
> Cc: Support Opensource
> Signed-off-by: Lee Jones
> ---
> drivers/regulator/da9063-regulator.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-
> regulator.c
> index e1d6c8f6d40bb..fe65b5acaf280 100644
> --- a/drivers/regulator/da9063-regulator.c
> +++ b/drivers/regulator/da9063-regulator.c
> @@ -512,7 +512,6 @@ static const struct da9063_regulator_info
> da9063_regulator_info[] = {
> },
> {
> DA9063_LDO(DA9063, LDO9, 950, 50, 3600),
> - .suspend = BFIELD(DA9063_REG_LDO9_CONT,
> DA9063_VLDO9_SEL),
> },
> {
> DA9063_LDO(DA9063, LDO11, 900, 50, 3600),
> --
> 2.25.1
RE: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
On 18 June 2020 12:15, Lee Jones wrote:
> > > > The current implementation performs checking in the i2c_probe()
> > > > function of the variant_code but does this immediately after the
> > > > containing struct has been initialised as all zero. This means the
> > > > check for variant code will always default to using the BB tables
> > > > and will never select AD. The variant code is subsequently set
> > > > by device_init() and later used by the RTC so really it's a little
> > > > fortunate this mismatch works.
> > > >
> > > > This update adds raw I2C read access functionality to read the chip
> > > > and variant/revision information (common to all revisions) so that
> > > > it can subsequently correctly choose the proper regmap tables for
> > > > real initialisation.
> > > >
> > > > Signed-off-by: Adam Thomson
>
> > > > ---
> > > > drivers/mfd/da9063-core.c| 31 --
> > > > drivers/mfd/da9063-i2c.c | 184
> +++-
> > > ---
> > > > include/linux/mfd/da9063/registers.h | 15 ++-
> > > > 3 files changed, 177 insertions(+), 53 deletions(-)
> > >
> > > [...]
> > >
> > > > + * Raw I2C access required for just accessing chip and variant info
> > > > before
> we
> > > > + * know which device is present. The info read from the device using
> > > > this
> > > > + * approach is then used to select the correct regmap tables.
> > > > + */
> > > > +
> > > > +#define DA9063_REG_PAGE_SIZE 0x100
> > > > +#define DA9063_REG_PAGED_ADDR_MASK 0xFF
> > > > +
> > > > +enum da9063_page_sel_buf_fmt {
> > > > + DA9063_PAGE_SEL_BUF_PAGE_REG = 0,
> > > > + DA9063_PAGE_SEL_BUF_PAGE_VAL,
> > > > + DA9063_PAGE_SEL_BUF_SIZE,
> > > > +};
> > > > +
> > > > +enum da9063_paged_read_msgs {
> > > > + DA9063_PAGED_READ_MSG_PAGE_SEL = 0,
> > > > + DA9063_PAGED_READ_MSG_REG_SEL,
> > > > + DA9063_PAGED_READ_MSG_DATA,
> > > > + DA9063_PAGED_READ_MSG_CNT,
> > > > +};
> > > > +
> > > > +static int da9063_i2c_blockreg_read(struct i2c_client *client, u16
> > > > addr,
> > > > + u8 *buf, int count)
> > > > +{
> > > > + struct i2c_msg xfer[DA9063_PAGED_READ_MSG_CNT];
> > > > + u8 page_sel_buf[DA9063_PAGE_SEL_BUF_SIZE];
> > > > + u8 page_num, paged_addr;
> > > > + int ret;
> > > > +
> > > > + /* Determine page info based on register address */
> > > > + page_num = (addr / DA9063_REG_PAGE_SIZE);
> > > > + if (page_num > 1) {
> > > > + dev_err(>dev, "Invalid register address
> > > > provided\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + paged_addr = (addr % DA9063_REG_PAGE_SIZE) &
> > > DA9063_REG_PAGED_ADDR_MASK;
> > > > + page_sel_buf[DA9063_PAGE_SEL_BUF_PAGE_REG] =
> > > DA9063_REG_PAGE_CON;
> > > > + page_sel_buf[DA9063_PAGE_SEL_BUF_PAGE_VAL] =
> > > > + (page_num << DA9063_I2C_PAGE_SEL_SHIFT) &
> > > DA9063_REG_PAGE_MASK;
> > > > +
> > > > + /* Write reg address, page selection */
> > > > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].addr = client->addr;
> > > > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].flags = 0;
> > > > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].len =
> > > DA9063_PAGE_SEL_BUF_SIZE;
> > > > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].buf = page_sel_buf;
> > > > +
> > > > + /* Select register address */
> > > > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].addr = client->addr;
> > > > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].flags = 0;
> > > > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].len = sizeof(paged_addr);
> > > > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].buf = _addr;
> > > > +
> > > > + /* Read data */
> > > > + xfer[DA9063_PAGED_READ_MSG_DATA].addr = client->addr;
> > > > + xfer[DA9063_PAGED_READ_MSG_DATA].flags = I2C_M_RD;
> > > > + xfer[
RE: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
On 18 June 2020 11:15, Lee Jones wrote:
> > The current implementation performs checking in the i2c_probe()
> > function of the variant_code but does this immediately after the
> > containing struct has been initialised as all zero. This means the
> > check for variant code will always default to using the BB tables
> > and will never select AD. The variant code is subsequently set
> > by device_init() and later used by the RTC so really it's a little
> > fortunate this mismatch works.
> >
> > This update adds raw I2C read access functionality to read the chip
> > and variant/revision information (common to all revisions) so that
> > it can subsequently correctly choose the proper regmap tables for
> > real initialisation.
> >
> > Signed-off-by: Adam Thomson
> > ---
> > drivers/mfd/da9063-core.c| 31 --
> > drivers/mfd/da9063-i2c.c | 184 +++-
> ---
> > include/linux/mfd/da9063/registers.h | 15 ++-
> > 3 files changed, 177 insertions(+), 53 deletions(-)
>
> [...]
>
> > + * Raw I2C access required for just accessing chip and variant info before
> > we
> > + * know which device is present. The info read from the device using this
> > + * approach is then used to select the correct regmap tables.
> > + */
> > +
> > +#define DA9063_REG_PAGE_SIZE 0x100
> > +#define DA9063_REG_PAGED_ADDR_MASK 0xFF
> > +
> > +enum da9063_page_sel_buf_fmt {
> > + DA9063_PAGE_SEL_BUF_PAGE_REG = 0,
> > + DA9063_PAGE_SEL_BUF_PAGE_VAL,
> > + DA9063_PAGE_SEL_BUF_SIZE,
> > +};
> > +
> > +enum da9063_paged_read_msgs {
> > + DA9063_PAGED_READ_MSG_PAGE_SEL = 0,
> > + DA9063_PAGED_READ_MSG_REG_SEL,
> > + DA9063_PAGED_READ_MSG_DATA,
> > + DA9063_PAGED_READ_MSG_CNT,
> > +};
> > +
> > +static int da9063_i2c_blockreg_read(struct i2c_client *client, u16 addr,
> > + u8 *buf, int count)
> > +{
> > + struct i2c_msg xfer[DA9063_PAGED_READ_MSG_CNT];
> > + u8 page_sel_buf[DA9063_PAGE_SEL_BUF_SIZE];
> > + u8 page_num, paged_addr;
> > + int ret;
> > +
> > + /* Determine page info based on register address */
> > + page_num = (addr / DA9063_REG_PAGE_SIZE);
> > + if (page_num > 1) {
> > + dev_err(>dev, "Invalid register address provided\n");
> > + return -EINVAL;
> > + }
> > +
> > + paged_addr = (addr % DA9063_REG_PAGE_SIZE) &
> DA9063_REG_PAGED_ADDR_MASK;
> > + page_sel_buf[DA9063_PAGE_SEL_BUF_PAGE_REG] =
> DA9063_REG_PAGE_CON;
> > + page_sel_buf[DA9063_PAGE_SEL_BUF_PAGE_VAL] =
> > + (page_num << DA9063_I2C_PAGE_SEL_SHIFT) &
> DA9063_REG_PAGE_MASK;
> > +
> > + /* Write reg address, page selection */
> > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].addr = client->addr;
> > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].flags = 0;
> > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].len =
> DA9063_PAGE_SEL_BUF_SIZE;
> > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].buf = page_sel_buf;
> > +
> > + /* Select register address */
> > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].addr = client->addr;
> > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].flags = 0;
> > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].len = sizeof(paged_addr);
> > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].buf = _addr;
> > +
> > + /* Read data */
> > + xfer[DA9063_PAGED_READ_MSG_DATA].addr = client->addr;
> > + xfer[DA9063_PAGED_READ_MSG_DATA].flags = I2C_M_RD;
> > + xfer[DA9063_PAGED_READ_MSG_DATA].len = count;
> > + xfer[DA9063_PAGED_READ_MSG_DATA].buf = buf;
> > +
> > + ret = i2c_transfer(client->adapter, xfer,
> DA9063_PAGED_READ_MSG_CNT);
> > + if (ret < 0) {
> > + dev_err(>dev, "Paged block read failed: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + if (ret != DA9063_PAGED_READ_MSG_CNT) {
> > + dev_err(>dev, "Paged block read failed to complete\n");
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
>
> Rather than open coding this, does it make sense to register a small
> (temporary?) Device ID Regmap to read from?
The original patch submission did exactly that but you indicated you weren't
keen due to overheads, hence the implementation above. Actually what we have
here is a bit smaller than the regmap approach and I really I'd rather not
have to respin again just to revert to something that was dismissed in the first
place over 6 months ago.
RE: [PATCH v3 0/2] Resolve revision handling and add support for DA silicon
On 17 April 2020 11:56, Adam Thomson wrote: > This patch set fixes the currently broken revision handling in the driver's > i2c_probe() function and then adds DA support to existing permitted revisions. Do I need to resend this patch, or will you still pick up the original mail submission from April, Lee? > > v3: > - Replaced magic numbers around I2C paged access code with enums and > defines > - Small style tidy ups as requested by Lee Jones > > v2: > - Use raw I2C read access instead of a temporary regmap to interrogate chip > and >variant id registers > > Adam Thomson (2): > mfd: da9063: Fix revision handling to correctly select reg tables > mfd: da9063: Add support for latest DA silicon revision > > drivers/mfd/da9063-core.c| 31 > drivers/mfd/da9063-i2c.c | 271 +++--- > - > include/linux/mfd/da9063/core.h | 1 + > include/linux/mfd/da9063/registers.h | 15 +- > 4 files changed, 257 insertions(+), 61 deletions(-) > > -- > 1.9.1
RE: [PATCHv3 5/5] ASoC: da7213: add default clock handling
On 11 May 2020 14:26, Sebastian Reichel wrote:
> This adds default clock/PLL configuration to the driver
> for usage with generic drivers like simple-card for usage
> with a fixed rate clock.
>
> Signed-off-by: Sebastian Reichel
> ---
> sound/soc/codecs/da7213.c | 76
> ---
> sound/soc/codecs/da7213.h | 2 ++
> 2 files changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index 3e6ad996741b..4a0edd3b7f83 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -1156,6 +1156,7 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
> struct snd_soc_dai *dai)
> {
> struct snd_soc_component *component = dai->component;
> + struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> u8 dai_ctrl = 0;
> u8 fs;
>
> @@ -1181,33 +1182,43 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
> switch (params_rate(params)) {
> case 8000:
> fs = DA7213_SR_8000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 11025:
> fs = DA7213_SR_11025;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 12000:
> fs = DA7213_SR_12000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 16000:
> fs = DA7213_SR_16000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 22050:
> fs = DA7213_SR_22050;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 32000:
> fs = DA7213_SR_32000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 44100:
> fs = DA7213_SR_44100;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 48000:
> fs = DA7213_SR_48000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 88200:
> fs = DA7213_SR_88200;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 96000:
> fs = DA7213_SR_96000;
> + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> break;
> default:
> return -EINVAL;
> @@ -1392,9 +1403,9 @@ static int da7213_set_component_sysclk(struct
> snd_soc_component *component,
> }
>
> /* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */
> -static int da7213_set_component_pll(struct snd_soc_component *component,
> - int pll_id, int source,
> - unsigned int fref, unsigned int fout)
> +static int _da7213_set_component_pll(struct snd_soc_component
> *component,
> + int pll_id, int source,
> + unsigned int fref, unsigned int fout)
> {
> struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
>
> @@ -1503,6 +1514,16 @@ static int da7213_set_component_pll(struct
> snd_soc_component *component,
> return 0;
> }
>
> +static int da7213_set_component_pll(struct snd_soc_component *component,
> + int pll_id, int source,
> + unsigned int fref, unsigned int fout)
> +{
> + struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> + da7213->fixed_clk_auto_pll = false;
> +
> + return _da7213_set_component_pll(component, pll_id, source, fref,
> fout);
> +}
> +
> /* DAI operations */
> static const struct snd_soc_dai_ops da7213_dai_ops = {
> .hw_params = da7213_hw_params,
> @@ -1532,6 +1553,43 @@ static struct snd_soc_dai_driver da7213_dai = {
> .symmetric_rates = 1,
> };
>
> +static int da7213_set_auto_pll(struct snd_soc_component *component, bool
> enable)
> +{
> + struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> + int mode;
> +
> + if (!da7213->fixed_clk_auto_pll)
> + return 0;
> +
> + da7213->mclk_rate = clk_get_rate(da7213->mclk);
> +
> + if (enable)
> + mode = DA7213_SYSCLK_PLL;
> + else
> + mode = DA7213_SYSCLK_MCLK;
> +
We still need to address the handling of the PLL when the codec is slave, and
the MCLK rate is not harmonic. In that scenario we need to use the SRM feature
of the PLL to synchronise the PLL with the incoming WCLK signal on the DAI.
Right now your code will allow for slave mode where the PLL is enabled without
SRM, regardless of whether we're acting as slave or not. Either we just don't
allow for slave mode with non-harmonic frequencies or we add a bit more in this
function to determine if SRM
RE: [PATCH 5/7] regulator: da9211: switch to using devm_fwnode_gpiod_get
On 05 October 2019 00:10, Dmitry Torokhov wrote: > devm_gpiod_get_from_of_node() is being retired in favor of > devm_fwnode_gpiod_get_index(), that behaves similar to > devm_gpiod_get_index(), but can work with arbitrary firmware node. It > will also be able to support secondary software nodes. > > Let's switch this driver over. > > Signed-off-by: Dmitry Torokhov Acked-by: Adam Thomson > --- > > drivers/regulator/da9211-regulator.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/regulator/da9211-regulator.c b/drivers/regulator/da9211- > regulator.c > index bf80748f1ccc..523dc1b95826 100644 > --- a/drivers/regulator/da9211-regulator.c > +++ b/drivers/regulator/da9211-regulator.c > @@ -283,12 +283,12 @@ static struct da9211_pdata > *da9211_parse_regulators_dt( > > pdata->init_data[n] = da9211_matches[i].init_data; > pdata->reg_node[n] = da9211_matches[i].of_node; > - pdata->gpiod_ren[n] = devm_gpiod_get_from_of_node(dev, > - da9211_matches[i].of_node, > - "enable-gpios", > - 0, > - GPIOD_OUT_HIGH | > GPIOD_FLAGS_BIT_NONEXCLUSIVE, > - "da9211-enable"); > + pdata->gpiod_ren[n] = devm_fwnode_gpiod_get(dev, > + of_fwnode_handle(pdata- > >reg_node[n]), > + "enable", > + GPIOD_OUT_HIGH | > + > GPIOD_FLAGS_BIT_NONEXCLUSIVE, > + "da9211-enable"); > if (IS_ERR(pdata->gpiod_ren[n])) > pdata->gpiod_ren[n] = NULL; > n++; > -- > 2.23.0.581.g78d2f28ef7-goog
RE: [RFT] rtc: da9063: Handle invalid IRQ from platform_get_irq_byname()
On 04 October 2019 16:05, Krzysztof Kozlowski wrote: > platform_get_irq_byname() might return -errno which later would be > cast to an unsigned int and used in request_irq(). > > Signed-off-by: Krzysztof Kozlowski This doesn't break the existing RTC functionality when running up on my test board with DA9063. Although very unlikely to occur it does make sense so: Tested-by: Adam Thomson > > --- > > Not marking as cc-stable as this was not reproduced and not tested. > --- > drivers/rtc/rtc-da9063.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c > index 15908d51b1cb..046b1d4c3dae 100644 > --- a/drivers/rtc/rtc-da9063.c > +++ b/drivers/rtc/rtc-da9063.c > @@ -483,6 +483,9 @@ static int da9063_rtc_probe(struct platform_device > *pdev) > rtc->rtc_dev->uie_unsupported = 1; > > irq_alarm = platform_get_irq_byname(pdev, "ALARM"); > + if (irq_alarm < 0) > + return irq_alarm; > + > ret = devm_request_threaded_irq(>dev, irq_alarm, NULL, > da9063_alarm_event, > IRQF_TRIGGER_LOW | IRQF_ONESHOT, > --
RE: [PATCH 2/5] dt-bindings: mfd: da9062: add regulator voltage selection documentation
On 26 September 2019 15:39, Marco Felsch wrote: > On 19-09-26 14:04, Adam Thomson wrote: > > On 26 September 2019 12:44, Marco Felsch wrote: > > > > > On 19-09-26 10:17, Adam Thomson wrote: > > > > On 26 September 2019 09:10, Marco Felsch wrote: > > > > > > > > > On 19-09-25 16:18, Adam Thomson wrote: > > > > > > On 25 September 2019 16:52, Marco Felsch wrote: > > > > > > > > > > > > > Hi Adam, > > > > > > > > > > > > > > On 19-09-24 09:23, Adam Thomson wrote: > > > > > > > > On 17 September 2019 13:43, Marco Felsch wrote: > > > > > > > > > > > > > > > > > Add the documentation which describe the voltage selection > > > > > > > > > gpio > > > > > support. > > > > > > > > > This property can be applied to each subnode within the > 'regulators' > > > > > > > > > node so each regulator can be configured differently. > > > > > > > > > > > > > > > > > > Signed-off-by: Marco Felsch > > > > > > > > > --- > > > > > > > > > Documentation/devicetree/bindings/mfd/da9062.txt | 9 > + > > > > > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > > > > > b/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > > > > > index edca653a5777..9d9820d8177d 100644 > > > > > > > > > --- a/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > > > > > +++ b/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > > > > > @@ -66,6 +66,15 @@ Sub-nodes: > > > > > > > > >details of individual regulator device can be found in: > > > > > > > > >Documentation/devicetree/bindings/regulator/regulator.txt > > > > > > > > > > > > > > > > > > + Optional regulator device-specific properties: > > > > > > > > > + - dlg,vsel-sense-gpios : The GPIO reference which should be > used > > > by > > > > > the > > > > > > > > > +regulator to switch the voltage between active/suspend > voltage > > > > > settings. > > > > > > > If > > > > > > > > > +the signal is active the active-settings are applied > > > > > > > > > else the > suspend > > > > > > > > > +settings are applied. Attention: Sharing the same gpio > > > > > > > > > for other > > > > > purposes > > > > > > > > > +or across multiple regulators is possible but the gpio > > > > > > > > > settings > must > > > be > > > > > the > > > > > > > > > +same. Also the gpio phandle must refer to to the > > > > > > > > > dlg,da9062- > gpio > > > > > device > > > > > > > > > +other gpios are not allowed and make no sense. > > > > > > > > > + > > > > > > > > > > > > > > > > Should we not use the binding names that are defined in 'gpio- > > > > > regulator.yaml' > > > > > > > as > > > > > > > > these seem to be generic and would probably serve the purpose > here? > > > > > > > > > > > > > > Hm.. as the description says: > > > > > > > > > > > > > > 8<-- > > > > > > > gpios: > > > > > > >description: Array of one or more GPIO pins used to select the > > > > > > >regulator voltage/current listed in "states". > > > > > > > 8<-- > > > > > > > > > > > > > > But we don't have a "states" property and we can't select between > > > > > > > voltage or current. > > > > > > > > > > > > Yes I think I was at cross purposes when I made this remark. The > bindings
RE: [PATCH 2/5] dt-bindings: mfd: da9062: add regulator voltage selection documentation
On 26 September 2019 12:44, Marco Felsch wrote: > On 19-09-26 10:17, Adam Thomson wrote: > > On 26 September 2019 09:10, Marco Felsch wrote: > > > > > On 19-09-25 16:18, Adam Thomson wrote: > > > > On 25 September 2019 16:52, Marco Felsch wrote: > > > > > > > > > Hi Adam, > > > > > > > > > > On 19-09-24 09:23, Adam Thomson wrote: > > > > > > On 17 September 2019 13:43, Marco Felsch wrote: > > > > > > > > > > > > > Add the documentation which describe the voltage selection gpio > > > support. > > > > > > > This property can be applied to each subnode within the > > > > > > > 'regulators' > > > > > > > node so each regulator can be configured differently. > > > > > > > > > > > > > > Signed-off-by: Marco Felsch > > > > > > > --- > > > > > > > Documentation/devicetree/bindings/mfd/da9062.txt | 9 + > > > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > > > b/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > > > index edca653a5777..9d9820d8177d 100644 > > > > > > > --- a/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > > > +++ b/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > > > @@ -66,6 +66,15 @@ Sub-nodes: > > > > > > >details of individual regulator device can be found in: > > > > > > >Documentation/devicetree/bindings/regulator/regulator.txt > > > > > > > > > > > > > > + Optional regulator device-specific properties: > > > > > > > + - dlg,vsel-sense-gpios : The GPIO reference which should be > > > > > > > used > by > > > the > > > > > > > +regulator to switch the voltage between active/suspend > > > > > > > voltage > > > settings. > > > > > If > > > > > > > +the signal is active the active-settings are applied else > > > > > > > the suspend > > > > > > > +settings are applied. Attention: Sharing the same gpio for > > > > > > > other > > > purposes > > > > > > > +or across multiple regulators is possible but the gpio > > > > > > > settings must > be > > > the > > > > > > > +same. Also the gpio phandle must refer to to the > > > > > > > dlg,da9062-gpio > > > device > > > > > > > +other gpios are not allowed and make no sense. > > > > > > > + > > > > > > > > > > > > Should we not use the binding names that are defined in 'gpio- > > > regulator.yaml' > > > > > as > > > > > > these seem to be generic and would probably serve the purpose here? > > > > > > > > > > Hm.. as the description says: > > > > > > > > > > 8<-- > > > > > gpios: > > > > >description: Array of one or more GPIO pins used to select the > > > > >regulator voltage/current listed in "states". > > > > > 8<-- > > > > > > > > > > But we don't have a "states" property and we can't select between > > > > > voltage or current. > > > > > > > > Yes I think I was at cross purposes when I made this remark. The > > > > bindings > there > > > > describe the GPOs that are used to enable/disable and set > > > > voltage/current > for > > > > regulators and the supported voltage/current levels that can be > > > > configured > in > > > > this manner. What you're describing is the GPI for DA9061/2. If you > > > > look at > > > > GPIO handling in existing regulator drivers I believe they all deal with > external > > > > GPOs that are configured to enable/disable and set voltage/current > > > > limits > > > rather > > > > than the GPI on the PMIC itself. That's why I'm thinking that the > configurations > > > > you're doing here
RE: [PATCH 2/5] dt-bindings: mfd: da9062: add regulator voltage selection documentation
On 26 September 2019 09:10, Marco Felsch wrote: > On 19-09-25 16:18, Adam Thomson wrote: > > On 25 September 2019 16:52, Marco Felsch wrote: > > > > > Hi Adam, > > > > > > On 19-09-24 09:23, Adam Thomson wrote: > > > > On 17 September 2019 13:43, Marco Felsch wrote: > > > > > > > > > Add the documentation which describe the voltage selection gpio > support. > > > > > This property can be applied to each subnode within the 'regulators' > > > > > node so each regulator can be configured differently. > > > > > > > > > > Signed-off-by: Marco Felsch > > > > > --- > > > > > Documentation/devicetree/bindings/mfd/da9062.txt | 9 + > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > b/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > index edca653a5777..9d9820d8177d 100644 > > > > > --- a/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > +++ b/Documentation/devicetree/bindings/mfd/da9062.txt > > > > > @@ -66,6 +66,15 @@ Sub-nodes: > > > > >details of individual regulator device can be found in: > > > > >Documentation/devicetree/bindings/regulator/regulator.txt > > > > > > > > > > + Optional regulator device-specific properties: > > > > > + - dlg,vsel-sense-gpios : The GPIO reference which should be used by > the > > > > > +regulator to switch the voltage between active/suspend voltage > settings. > > > If > > > > > +the signal is active the active-settings are applied else the > > > > > suspend > > > > > +settings are applied. Attention: Sharing the same gpio for other > purposes > > > > > +or across multiple regulators is possible but the gpio settings > > > > > must be > the > > > > > +same. Also the gpio phandle must refer to to the dlg,da9062-gpio > device > > > > > +other gpios are not allowed and make no sense. > > > > > + > > > > > > > > Should we not use the binding names that are defined in 'gpio- > regulator.yaml' > > > as > > > > these seem to be generic and would probably serve the purpose here? > > > > > > Hm.. as the description says: > > > > > > 8<-- > > > gpios: > > >description: Array of one or more GPIO pins used to select the > > >regulator voltage/current listed in "states". > > > 8<-- > > > > > > But we don't have a "states" property and we can't select between > > > voltage or current. > > > > Yes I think I was at cross purposes when I made this remark. The bindings > > there > > describe the GPOs that are used to enable/disable and set voltage/current > > for > > regulators and the supported voltage/current levels that can be configured > > in > > this manner. What you're describing is the GPI for DA9061/2. If you look at > > GPIO handling in existing regulator drivers I believe they all deal with > > external > > GPOs that are configured to enable/disable and set voltage/current limits > rather > > than the GPI on the PMIC itself. That's why I'm thinking that the > > configurations > > you're doing here should actually be in a pinctrl or GPIO driver. > > That's true, the common gpio bindings are from the view of the > processor, e.g. which gpio must the processor drive to enable/switch the > regualtor. So one reasone more to use a non-common binding. > > Please take a look on my other comment I made :) I don't use the > gpio-alternative function. I use it as an input. I know in the datasheet this isn't marked as an alternate function specifically but to me having regulator control by the chip's own GPI is an alternative function for that GPIO pin, in the same way a specific pin can be used for SYS_EN or Watchdog control. It's a dedicated purpose rather than being a normal GPI. See the following as an example of what I'm suggesting: https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/pinctrl/pinctrl-palmas.txt You could then pass the pinctrl information to the regulator driver and use that rather than having device specific bindings for this. That's at least my current interpretation of this anyway. > > R
RE: [PATCH 2/2] regulator: da9063: Simplify da9063_buck_set_mode for BUCK_MODE_MANUAL case
On 26 September 2019 06:51, Axel Lin wrote:
> The sleep flag bit decides the mode for BUCK_MODE_MANUAL case, simplify
> the logic as the result is the same.
>
> Signed-off-by: Axel Lin
Reviewed-by: Adam Thomson
> ---
> drivers/regulator/da9063-regulator.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-
> regulator.c
> index 28b1b20f45bd..2aceb3b7afc2 100644
> --- a/drivers/regulator/da9063-regulator.c
> +++ b/drivers/regulator/da9063-regulator.c
> @@ -225,7 +225,7 @@ static unsigned da9063_buck_get_mode(struct
> regulator_dev *rdev)
> {
> struct da9063_regulator *regl = rdev_get_drvdata(rdev);
> struct regmap_field *field;
> - unsigned int val, mode = 0;
> + unsigned int val;
> int ret;
>
> ret = regmap_field_read(regl->mode, );
> @@ -235,7 +235,6 @@ static unsigned da9063_buck_get_mode(struct
> regulator_dev *rdev)
> switch (val) {
> default:
> case BUCK_MODE_MANUAL:
> - mode = REGULATOR_MODE_FAST |
> REGULATOR_MODE_STANDBY;
> /* Sleep flag bit decides the mode */
> break;
> case BUCK_MODE_SLEEP:
> @@ -262,11 +261,9 @@ static unsigned da9063_buck_get_mode(struct
> regulator_dev *rdev)
> return 0;
>
> if (val)
> - mode &= REGULATOR_MODE_STANDBY;
> + return REGULATOR_MODE_STANDBY;
> else
> - mode &= REGULATOR_MODE_NORMAL |
> REGULATOR_MODE_FAST;
> -
> - return mode;
> + return REGULATOR_MODE_FAST;
> }
>
> /*
> --
> 2.20.1
RE: [PATCH 1/2] regulator: da9062: Simplify da9062_buck_set_mode for BUCK_MODE_MANUAL case
On 26 September 2019 06:51, Axel Lin wrote:
> The sleep flag bit decides the mode for BUCK_MODE_MANUAL case, simplify
> the logic as the result is the same.
>
> Signed-off-by: Axel Lin
This patch will need to be rebased on Marco's update titled:
"regulator: da9062: fix suspend_enable/disable preparation"
However, changes look fine so:
Reviewed-by: Adam Thomson
> ---
> drivers/regulator/da9062-regulator.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/regulator/da9062-regulator.c b/drivers/regulator/da9062-
> regulator.c
> index 710e67081d53..739a40f256f6 100644
> --- a/drivers/regulator/da9062-regulator.c
> +++ b/drivers/regulator/da9062-regulator.c
> @@ -136,7 +136,7 @@ static int da9062_buck_set_mode(struct regulator_dev
> *rdev, unsigned mode)
> static unsigned da9062_buck_get_mode(struct regulator_dev *rdev)
> {
> struct da9062_regulator *regl = rdev_get_drvdata(rdev);
> - unsigned int val, mode = 0;
> + unsigned int val;
> int ret;
>
> ret = regmap_field_read(regl->mode, );
> @@ -146,7 +146,6 @@ static unsigned da9062_buck_get_mode(struct
> regulator_dev *rdev)
> switch (val) {
> default:
> case BUCK_MODE_MANUAL:
> - mode = REGULATOR_MODE_FAST |
> REGULATOR_MODE_STANDBY;
> /* Sleep flag bit decides the mode */
> break;
> case BUCK_MODE_SLEEP:
> @@ -162,11 +161,9 @@ static unsigned da9062_buck_get_mode(struct
> regulator_dev *rdev)
> return 0;
>
> if (val)
> - mode &= REGULATOR_MODE_STANDBY;
> + return REGULATOR_MODE_STANDBY;
> else
> - mode &= REGULATOR_MODE_NORMAL |
> REGULATOR_MODE_FAST;
> -
> - return mode;
> + return REGULATOR_MODE_FAST;
> }
>
> /*
> --
> 2.20.1
RE: [PATCH 2/5] dt-bindings: mfd: da9062: add regulator voltage selection documentation
On 25 September 2019 16:52, Marco Felsch wrote: > Hi Adam, > > On 19-09-24 09:23, Adam Thomson wrote: > > On 17 September 2019 13:43, Marco Felsch wrote: > > > > > Add the documentation which describe the voltage selection gpio support. > > > This property can be applied to each subnode within the 'regulators' > > > node so each regulator can be configured differently. > > > > > > Signed-off-by: Marco Felsch > > > --- > > > Documentation/devicetree/bindings/mfd/da9062.txt | 9 + > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt > > > b/Documentation/devicetree/bindings/mfd/da9062.txt > > > index edca653a5777..9d9820d8177d 100644 > > > --- a/Documentation/devicetree/bindings/mfd/da9062.txt > > > +++ b/Documentation/devicetree/bindings/mfd/da9062.txt > > > @@ -66,6 +66,15 @@ Sub-nodes: > > >details of individual regulator device can be found in: > > >Documentation/devicetree/bindings/regulator/regulator.txt > > > > > > + Optional regulator device-specific properties: > > > + - dlg,vsel-sense-gpios : The GPIO reference which should be used by the > > > +regulator to switch the voltage between active/suspend voltage > > > settings. > If > > > +the signal is active the active-settings are applied else the suspend > > > +settings are applied. Attention: Sharing the same gpio for other > > > purposes > > > +or across multiple regulators is possible but the gpio settings must > > > be the > > > +same. Also the gpio phandle must refer to to the dlg,da9062-gpio > > > device > > > +other gpios are not allowed and make no sense. > > > + > > > > Should we not use the binding names that are defined in > > 'gpio-regulator.yaml' > as > > these seem to be generic and would probably serve the purpose here? > > Hm.. as the description says: > > 8<-- > gpios: >description: Array of one or more GPIO pins used to select the >regulator voltage/current listed in "states". > 8<-- > > But we don't have a "states" property and we can't select between > voltage or current. Yes I think I was at cross purposes when I made this remark. The bindings there describe the GPOs that are used to enable/disable and set voltage/current for regulators and the supported voltage/current levels that can be configured in this manner. What you're describing is the GPI for DA9061/2. If you look at GPIO handling in existing regulator drivers I believe they all deal with external GPOs that are configured to enable/disable and set voltage/current limits rather than the GPI on the PMIC itself. That's why I'm thinking that the configurations you're doing here should actually be in a pinctrl or GPIO driver. I'd be interested in hearing Mark's view on this though as he has far more experience in this area than I do. > > Regards, > Marco > > > > - rtc : This node defines settings required for the Real-Time Clock > > > associated > > >with the DA9062. There are currently no entries in this binding, > > > however > > >compatible = "dlg,da9062-rtc" should be added if a node is created. > > > -- > > > 2.20.1 > > > > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
RE: [PATCH 3/5] regulator: da9062: add voltage selection gpio support
On 17 September 2019 13:43, Marco Felsch wrote:
> The DA9062/1 devices can switch their regulator voltages between
> voltage-A (active) and voltage-B (suspend) settings. Switching the
> voltages can be controlled by ther internal state-machine or by a gpio
> input signal and can be configured for each individual regulator. This
> commit adds the gpio-based voltage switching support.
>
> Signed-off-by: Marco Felsch
> ---
> drivers/regulator/da9062-regulator.c | 149 +++
> 1 file changed, 149 insertions(+)
>
> diff --git a/drivers/regulator/da9062-regulator.c b/drivers/regulator/da9062-
> regulator.c
> index 9b2ca472f70c..9d6eb7625948 100644
> --- a/drivers/regulator/da9062-regulator.c
> +++ b/drivers/regulator/da9062-regulator.c
> @@ -7,6 +7,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -15,6 +16,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> /* Regulator IDs */
> @@ -50,6 +52,7 @@ struct da9062_regulator_info {
> struct reg_field sleep;
> struct reg_field suspend_sleep;
> unsigned int suspend_vsel_reg;
> + struct reg_field vsel_gpi;
> /* Event detection bit */
> struct reg_field oc_event;
> };
> @@ -65,6 +68,7 @@ struct da9062_regulator {
> struct regmap_field *suspend;
> struct regmap_field *sleep;
> struct regmap_field *suspend_sleep;
> + struct regmap_field *vsel_gpi;
> };
>
> /* Encapsulates all information for the regulators driver */
> @@ -351,6 +355,65 @@ static const struct regulator_ops da9062_ldo_ops = {
> .set_suspend_mode = da9062_ldo_set_suspend_mode,
> };
>
> +static int da9062_config_gpi(struct device_node *np,
> + const struct regulator_desc *desc,
> + struct regulator_config *cfg, const char *gpi_id)
> +{
> + struct da9062_regulator *regl = cfg->driver_data;
> + struct gpio_desc *gpi;
> + unsigned int nr;
> + int ret;
> + char *prop, *label;
> +
> + prop = kasprintf(GFP_KERNEL, "dlg,%s-sense-gpios", gpi_id);
> + if (!prop)
> + return -ENOMEM;
> + label = kasprintf(GFP_KERNEL, "%s-%s-gpi", desc->name, gpi_id);
> + if (!label) {
> + ret = -ENOMEM;
> + goto free;
If we use the generic bindings names then the above will change I guess.
> + }
> +
> + /*
> + * We only must ensure that the gpio device is probed before the
> + * regulator driver so no need to store the reference global. Luckily
> + * devm_* releases the gpio upon a unbound action.
> + */
> + gpi = devm_gpiod_get_from_of_node(cfg->dev, np, prop, 0, GPIOD_IN |
> + GPIOD_FLAGS_BIT_NONEXCLUSIVE,
> label);
> + if (IS_ERR(gpi)) {
> + ret = PTR_ERR(gpi);
> + goto free;
> + }
> +
> + if (!gpi) {
> + ret = 0;
> + goto free;
> + }
> +
> + /* We need the local number */
> + nr = da9062_gpio_get_hwgpio(gpi);
> + if (nr < 1 || nr > 3) {
> + ret = -EINVAL;
> + goto free;
> + }
> +
> + ret = regmap_field_write(regl->vsel_gpi, nr);
Actually thinking about this some more, should we really be setting alternate
functions of the GPIO here? Would this not be done through GPIO/Pinmux
frameworks? That way the GPIO would be blocked off from other's requesting it.
This seems a little unsafe, unless I'm mistaken.
> +
> +free:
> + kfree(prop);
> + kfree(label);
> +
> + return ret;
> +}
> +
> +static int da9062_parse_dt(struct device_node *np,
> +const struct regulator_desc *desc,
> +struct regulator_config *cfg)
> +{
> + return da9062_config_gpi(np, desc, cfg, "vsel");
> +}
> +
> /* DA9061 Regulator information */
> static const struct da9062_regulator_info local_da9061_regulator_info[] = {
> {
> @@ -358,6 +421,7 @@ static const struct da9062_regulator_info
> local_da9061_regulator_info[] = {
> .desc.name = "DA9061 BUCK1",
> .desc.of_match = of_match_ptr("buck1"),
> .desc.regulators_node = of_match_ptr("regulators"),
> + .desc.of_parse_cb = da9062_parse_dt,
> .desc.ops = _buck_ops,
> .desc.min_uV = (300) * 1000,
> .desc.uV_step = (10) * 1000,
> @@ -388,12 +452,17 @@ static const struct da9062_regulator_info
> local_da9061_regulator_info[] = {
> __builtin_ffs((int)DA9062AA_BUCK1_CONF_MASK) - 1,
> sizeof(unsigned int) * 8 -
> __builtin_clz(DA9062AA_BUCK1_CONF_MASK) - 1),
> + .vsel_gpi = REG_FIELD(DA9062AA_BUCK1_CONT,
> + __builtin_ffs((int)DA9062AA_VBUCK1_GPI_MASK) - 1,
> + sizeof(unsigned int) * 8 -
> +
RE: [PATCH 2/5] dt-bindings: mfd: da9062: add regulator voltage selection documentation
On 17 September 2019 13:43, Marco Felsch wrote: > Add the documentation which describe the voltage selection gpio support. > This property can be applied to each subnode within the 'regulators' > node so each regulator can be configured differently. > > Signed-off-by: Marco Felsch > --- > Documentation/devicetree/bindings/mfd/da9062.txt | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt > b/Documentation/devicetree/bindings/mfd/da9062.txt > index edca653a5777..9d9820d8177d 100644 > --- a/Documentation/devicetree/bindings/mfd/da9062.txt > +++ b/Documentation/devicetree/bindings/mfd/da9062.txt > @@ -66,6 +66,15 @@ Sub-nodes: >details of individual regulator device can be found in: >Documentation/devicetree/bindings/regulator/regulator.txt > > + Optional regulator device-specific properties: > + - dlg,vsel-sense-gpios : The GPIO reference which should be used by the > +regulator to switch the voltage between active/suspend voltage settings. > If > +the signal is active the active-settings are applied else the suspend > +settings are applied. Attention: Sharing the same gpio for other purposes > +or across multiple regulators is possible but the gpio settings must be > the > +same. Also the gpio phandle must refer to to the dlg,da9062-gpio device > +other gpios are not allowed and make no sense. > + Should we not use the binding names that are defined in 'gpio-regulator.yaml' as these seem to be generic and would probably serve the purpose here? > - rtc : This node defines settings required for the Real-Time Clock > associated >with the DA9062. There are currently no entries in this binding, however >compatible = "dlg,da9062-rtc" should be added if a node is created. > -- > 2.20.1
RE: [PATCH 1/5] regulator: da9062: fix suspend_enable/disable preparation
On 23 September 2019 23:03, Marco Felsch wrote:
> Hi Adam,
>
> On 19-09-23 16:03, Adam Thomson wrote:
> > On 17 September 2019 13:43, Marco Felsch wrote:
> >
> > > Currently the suspend reg_field maps to the pmic voltage selection bits
> > > and is used during suspend_enabe/disable() and during get_mode(). This
> > > seems to be wrong for both use cases.
> > >
> > > Use case one (suspend_enabe/disable):
> > > Those callbacks are used to mark a regulator device as enabled/disabled
> > > during suspend. Marking the regulator enabled during suspend is done by
> > > the LDOx_CONF/BUCKx_CONF bit within the LDOx_CONT/BUCKx_CONT
> > > registers.
> > > Setting this bit tells the DA9062 PMIC state machine to keep the
> > > regulator on in POWERDOWN mode and switch to suspend voltage.
> > >
> > > Use case two (get_mode):
> > > The get_mode callback is used to retrieve the active mode state. Since
> > > the regulator-setting-A is used for the active state and
> > > regulator-setting-B for the suspend state there is no need to check
> > > which regulator setting is active.
> > >
> >
> > So I believe you're correct with the above statements. The driver, rather
> > than
> > enabling/disabling a regulator in system suspend, will instead put the
> > regulator
> > to a low power state, which is definitely not the desired outcome. Thanks
> > for
> > rectifying this.
> >
> > Reviewed-by: Adam Thomson
>
> Thanks a lot for reviewing the patch :) Can you also have a look on the
> other patches within this series?
Yes, I will take a look at those next. :)
>
> Regards,
> Marco
>
> > > Fixes: 4068e5182ada ("regulator: da9062: DA9062 regulator driver")
> > > Signed-off-by: Marco Felsch
> > > ---
> > > drivers/regulator/da9062-regulator.c | 118 +++
> > > 1 file changed, 47 insertions(+), 71 deletions(-)
> > >
> > > diff --git a/drivers/regulator/da9062-regulator.c
> > > b/drivers/regulator/da9062-
> > > regulator.c
> > > index 2ffc64622451..9b2ca472f70c 100644
> > > --- a/drivers/regulator/da9062-regulator.c
> > > +++ b/drivers/regulator/da9062-regulator.c
> > > @@ -136,7 +136,6 @@ static int da9062_buck_set_mode(struct
> regulator_dev
> > > *rdev, unsigned mode)
> > > static unsigned da9062_buck_get_mode(struct regulator_dev *rdev)
> > > {
> > > struct da9062_regulator *regl = rdev_get_drvdata(rdev);
> > > - struct regmap_field *field;
> > > unsigned int val, mode = 0;
> > > int ret;
> > >
> > > @@ -158,18 +157,7 @@ static unsigned da9062_buck_get_mode(struct
> > > regulator_dev *rdev)
> > > return REGULATOR_MODE_NORMAL;
> > > }
> > >
> > > - /* Detect current regulator state */
> > > - ret = regmap_field_read(regl->suspend, );
> > > - if (ret < 0)
> > > - return 0;
> > > -
> > > - /* Read regulator mode from proper register, depending on state */
> > > - if (val)
> > > - field = regl->suspend_sleep;
> > > - else
> > > - field = regl->sleep;
> > > -
> > > - ret = regmap_field_read(field, );
> > > + ret = regmap_field_read(regl->sleep, );
> > > if (ret < 0)
> > > return 0;
> > >
> > > @@ -208,21 +196,9 @@ static int da9062_ldo_set_mode(struct regulator_dev
> > > *rdev, unsigned mode)
> > > static unsigned da9062_ldo_get_mode(struct regulator_dev *rdev)
> > > {
> > > struct da9062_regulator *regl = rdev_get_drvdata(rdev);
> > > - struct regmap_field *field;
> > > int ret, val;
> > >
> > > - /* Detect current regulator state */
> > > - ret = regmap_field_read(regl->suspend, );
> > > - if (ret < 0)
> > > - return 0;
> > > -
> > > - /* Read regulator mode from proper register, depending on state */
> > > - if (val)
> > > - field = regl->suspend_sleep;
> > > - else
> > > - field = regl->sleep;
> > > -
> > > - ret = regmap_field_read(field, );
> > > + ret = regmap_field_read(regl->sleep, );
> > > if (ret < 0)
> > > return 0;
> > >
> > > @@ -408,10 +384,10 @@ static const struct da9062_regulator_info
> > > local_da9061_regulator_info[] = {
> > >
RE: [PATCH 1/5] regulator: da9062: fix suspend_enable/disable preparation
On 17 September 2019 13:43, Marco Felsch wrote:
> Currently the suspend reg_field maps to the pmic voltage selection bits
> and is used during suspend_enabe/disable() and during get_mode(). This
> seems to be wrong for both use cases.
>
> Use case one (suspend_enabe/disable):
> Those callbacks are used to mark a regulator device as enabled/disabled
> during suspend. Marking the regulator enabled during suspend is done by
> the LDOx_CONF/BUCKx_CONF bit within the LDOx_CONT/BUCKx_CONT
> registers.
> Setting this bit tells the DA9062 PMIC state machine to keep the
> regulator on in POWERDOWN mode and switch to suspend voltage.
>
> Use case two (get_mode):
> The get_mode callback is used to retrieve the active mode state. Since
> the regulator-setting-A is used for the active state and
> regulator-setting-B for the suspend state there is no need to check
> which regulator setting is active.
>
So I believe you're correct with the above statements. The driver, rather than
enabling/disabling a regulator in system suspend, will instead put the regulator
to a low power state, which is definitely not the desired outcome. Thanks for
rectifying this.
Reviewed-by: Adam Thomson
> Fixes: 4068e5182ada ("regulator: da9062: DA9062 regulator driver")
> Signed-off-by: Marco Felsch
> ---
> drivers/regulator/da9062-regulator.c | 118 +++
> 1 file changed, 47 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/regulator/da9062-regulator.c b/drivers/regulator/da9062-
> regulator.c
> index 2ffc64622451..9b2ca472f70c 100644
> --- a/drivers/regulator/da9062-regulator.c
> +++ b/drivers/regulator/da9062-regulator.c
> @@ -136,7 +136,6 @@ static int da9062_buck_set_mode(struct regulator_dev
> *rdev, unsigned mode)
> static unsigned da9062_buck_get_mode(struct regulator_dev *rdev)
> {
> struct da9062_regulator *regl = rdev_get_drvdata(rdev);
> - struct regmap_field *field;
> unsigned int val, mode = 0;
> int ret;
>
> @@ -158,18 +157,7 @@ static unsigned da9062_buck_get_mode(struct
> regulator_dev *rdev)
> return REGULATOR_MODE_NORMAL;
> }
>
> - /* Detect current regulator state */
> - ret = regmap_field_read(regl->suspend, );
> - if (ret < 0)
> - return 0;
> -
> - /* Read regulator mode from proper register, depending on state */
> - if (val)
> - field = regl->suspend_sleep;
> - else
> - field = regl->sleep;
> -
> - ret = regmap_field_read(field, );
> + ret = regmap_field_read(regl->sleep, );
> if (ret < 0)
> return 0;
>
> @@ -208,21 +196,9 @@ static int da9062_ldo_set_mode(struct regulator_dev
> *rdev, unsigned mode)
> static unsigned da9062_ldo_get_mode(struct regulator_dev *rdev)
> {
> struct da9062_regulator *regl = rdev_get_drvdata(rdev);
> - struct regmap_field *field;
> int ret, val;
>
> - /* Detect current regulator state */
> - ret = regmap_field_read(regl->suspend, );
> - if (ret < 0)
> - return 0;
> -
> - /* Read regulator mode from proper register, depending on state */
> - if (val)
> - field = regl->suspend_sleep;
> - else
> - field = regl->sleep;
> -
> - ret = regmap_field_read(field, );
> + ret = regmap_field_read(regl->sleep, );
> if (ret < 0)
> return 0;
>
> @@ -408,10 +384,10 @@ static const struct da9062_regulator_info
> local_da9061_regulator_info[] = {
> __builtin_ffs((int)DA9062AA_BUCK1_MODE_MASK) - 1,
> sizeof(unsigned int) * 8 -
> __builtin_clz((DA9062AA_BUCK1_MODE_MASK)) - 1),
> - .suspend = REG_FIELD(DA9062AA_DVC_1,
> - __builtin_ffs((int)DA9062AA_VBUCK1_SEL_MASK) - 1,
> + .suspend = REG_FIELD(DA9062AA_BUCK1_CONT,
> + __builtin_ffs((int)DA9062AA_BUCK1_CONF_MASK) - 1,
> sizeof(unsigned int) * 8 -
> - __builtin_clz((DA9062AA_VBUCK1_SEL_MASK)) - 1),
> + __builtin_clz(DA9062AA_BUCK1_CONF_MASK) - 1),
> },
> {
> .desc.id = DA9061_ID_BUCK2,
> @@ -444,10 +420,10 @@ static const struct da9062_regulator_info
> local_da9061_regulator_info[] = {
> __builtin_ffs((int)DA9062AA_BUCK3_MODE_MASK) - 1,
> sizeof(unsigned int) * 8 -
> __builtin_clz((DA9062AA_BUCK3_MODE_MASK)) - 1),
> - .suspend = REG_FIELD(DA9062AA_DVC_1,
> - __builtin_ffs((int)DA9062AA_VBUCK3_SEL_MASK) - 1,
> +
RE: [PATCH 1/5] regulator: da9062: fix suspend_enable/disable preparation
On 17 September 2019 13:43, Marco Felsch wrote:
> Currently the suspend reg_field maps to the pmic voltage selection bits
> and is used during suspend_enabe/disable() and during get_mode(). This
> seems to be wrong for both use cases.
Hi Marco,
I'd be very surprised if what was in place was wrong as I know Stephen tested
this driver thoroughly over its lifetime. Regardless, I'll need some time to
review
your proposed updates to make sure this aligns with our expectation of
operation.
>
> Use case one (suspend_enabe/disable):
> Those callbacks are used to mark a regulator device as enabled/disabled
> during suspend. Marking the regulator enabled during suspend is done by
> the LDOx_CONF/BUCKx_CONF bit within the LDOx_CONT/BUCKx_CONT
> registers.
> Setting this bit tells the DA9062 PMIC state machine to keep the
> regulator on in POWERDOWN mode and switch to suspend voltage.
>
> Use case two (get_mode):
> The get_mode callback is used to retrieve the active mode state. Since
> the regulator-setting-A is used for the active state and
> regulator-setting-B for the suspend state there is no need to check
> which regulator setting is active.
>
> Fixes: 4068e5182ada ("regulator: da9062: DA9062 regulator driver")
> Signed-off-by: Marco Felsch
> ---
> drivers/regulator/da9062-regulator.c | 118 +++
> 1 file changed, 47 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/regulator/da9062-regulator.c b/drivers/regulator/da9062-
> regulator.c
> index 2ffc64622451..9b2ca472f70c 100644
> --- a/drivers/regulator/da9062-regulator.c
> +++ b/drivers/regulator/da9062-regulator.c
> @@ -136,7 +136,6 @@ static int da9062_buck_set_mode(struct regulator_dev
> *rdev, unsigned mode)
> static unsigned da9062_buck_get_mode(struct regulator_dev *rdev)
> {
> struct da9062_regulator *regl = rdev_get_drvdata(rdev);
> - struct regmap_field *field;
> unsigned int val, mode = 0;
> int ret;
>
> @@ -158,18 +157,7 @@ static unsigned da9062_buck_get_mode(struct
> regulator_dev *rdev)
> return REGULATOR_MODE_NORMAL;
> }
>
> - /* Detect current regulator state */
> - ret = regmap_field_read(regl->suspend, );
> - if (ret < 0)
> - return 0;
> -
> - /* Read regulator mode from proper register, depending on state */
> - if (val)
> - field = regl->suspend_sleep;
> - else
> - field = regl->sleep;
> -
> - ret = regmap_field_read(field, );
> + ret = regmap_field_read(regl->sleep, );
> if (ret < 0)
> return 0;
>
> @@ -208,21 +196,9 @@ static int da9062_ldo_set_mode(struct regulator_dev
> *rdev, unsigned mode)
> static unsigned da9062_ldo_get_mode(struct regulator_dev *rdev)
> {
> struct da9062_regulator *regl = rdev_get_drvdata(rdev);
> - struct regmap_field *field;
> int ret, val;
>
> - /* Detect current regulator state */
> - ret = regmap_field_read(regl->suspend, );
> - if (ret < 0)
> - return 0;
> -
> - /* Read regulator mode from proper register, depending on state */
> - if (val)
> - field = regl->suspend_sleep;
> - else
> - field = regl->sleep;
> -
> - ret = regmap_field_read(field, );
> + ret = regmap_field_read(regl->sleep, );
> if (ret < 0)
> return 0;
>
> @@ -408,10 +384,10 @@ static const struct da9062_regulator_info
> local_da9061_regulator_info[] = {
> __builtin_ffs((int)DA9062AA_BUCK1_MODE_MASK) - 1,
> sizeof(unsigned int) * 8 -
> __builtin_clz((DA9062AA_BUCK1_MODE_MASK)) - 1),
> - .suspend = REG_FIELD(DA9062AA_DVC_1,
> - __builtin_ffs((int)DA9062AA_VBUCK1_SEL_MASK) - 1,
> + .suspend = REG_FIELD(DA9062AA_BUCK1_CONT,
> + __builtin_ffs((int)DA9062AA_BUCK1_CONF_MASK) - 1,
> sizeof(unsigned int) * 8 -
> - __builtin_clz((DA9062AA_VBUCK1_SEL_MASK)) - 1),
> + __builtin_clz(DA9062AA_BUCK1_CONF_MASK) - 1),
> },
> {
> .desc.id = DA9061_ID_BUCK2,
> @@ -444,10 +420,10 @@ static const struct da9062_regulator_info
> local_da9061_regulator_info[] = {
> __builtin_ffs((int)DA9062AA_BUCK3_MODE_MASK) - 1,
> sizeof(unsigned int) * 8 -
> __builtin_clz((DA9062AA_BUCK3_MODE_MASK)) - 1),
> - .suspend = REG_FIELD(DA9062AA_DVC_1,
> - __builtin_ffs((int)DA9062AA_VBUCK3_SEL_MASK) - 1,
> + .suspend = REG_FIELD(DA9062AA_BUCK3_CONT,
> + __builtin_ffs((int)DA9062AA_BUCK3_CONF_MASK) - 1,
> sizeof(unsigned int) * 8 -
> - __builtin_clz((DA9062AA_VBUCK3_SEL_MASK)) - 1),
> + __builtin_clz(DA9062AA_BUCK3_CONF_MASK) - 1),
> },
> {
> .desc.id = DA9061_ID_BUCK3,
> @@ -480,10
RE: [PATCH] ASoC: da7219: Update the support rate list
On 02 May 2019 05:08, Yu-Hsuan Hsu wrote:
> If we want to set rate to 64000 on da7219, it fails and returns
> "snd_pcm_hw_params: Invalid argument".
> We should remove 64000 from support rate list because it is not
> available.
>
> Signed-off-by: Yu-Hsuan Hsu
Reviewed-by: Adam Thomson
> ---
> sound/soc/codecs/da7219.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> index 5f5fa3416af3..7497457cf3d4 100644
> --- a/sound/soc/codecs/da7219.c
> +++ b/sound/soc/codecs/da7219.c
> @@ -1658,20 +1658,26 @@ static const struct snd_soc_dai_ops da7219_dai_ops
> = {
> #define DA7219_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |
> SNDRV_PCM_FMTBIT_S20_3LE |\
> SNDRV_PCM_FMTBIT_S24_LE |
> SNDRV_PCM_FMTBIT_S32_LE)
>
> +#define DA7219_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025
> |\
> + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
> + SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
> + SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
> + SNDRV_PCM_RATE_96000)
> +
> static struct snd_soc_dai_driver da7219_dai = {
> .name = "da7219-hifi",
> .playback = {
> .stream_name = "Playback",
> .channels_min = 1,
> .channels_max = DA7219_DAI_CH_NUM_MAX,
> - .rates = SNDRV_PCM_RATE_8000_96000,
> + .rates = DA7219_RATES,
> .formats = DA7219_FORMATS,
> },
> .capture = {
> .stream_name = "Capture",
> .channels_min = 1,
> .channels_max = DA7219_DAI_CH_NUM_MAX,
> - .rates = SNDRV_PCM_RATE_8000_96000,
> + .rates = DA7219_RATES,
> .formats = DA7219_FORMATS,
> },
> .ops = _dai_ops,
> --
> 2.21.0.593.g511ec345e18-goog
RE: [PATCH v2] ASoC: da7213: fix DAI_CLK_EN register bit overwrite
On 01 May 2019 10:04, Logesh Kolandavel wrote: > From: Logesh > > If the da7213 codec is configured as Master with the DAPM power down > delay time set, 'snd_soc_component_write' function overwrites the > DAI_CLK_EN bit of DAI_CLK_MODE register which leads to audio play > only once until it re-initialize after codec power up. > > Signed-off-by: Logesh Reviewed-by: Adam Thomson > --- > Changes in v2: > -Include the mask DA7213_DAI_BCLKS_PER_WCLK_MASK > > sound/soc/codecs/da7213.c | 5 - > sound/soc/codecs/da7213.h | 2 ++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c > index 92d006a5283e..425c11d63e49 100644 > --- a/sound/soc/codecs/da7213.c > +++ b/sound/soc/codecs/da7213.c > @@ -1305,7 +1305,10 @@ static int da7213_set_dai_fmt(struct snd_soc_dai > *codec_dai, unsigned int fmt) > /* By default only 64 BCLK per WCLK is supported */ > dai_clk_mode |= DA7213_DAI_BCLKS_PER_WCLK_64; > > - snd_soc_component_write(component, DA7213_DAI_CLK_MODE, > dai_clk_mode); > + snd_soc_component_update_bits(component, > DA7213_DAI_CLK_MODE, > + DA7213_DAI_BCLKS_PER_WCLK_MASK | > + DA7213_DAI_CLK_POL_MASK | > DA7213_DAI_WCLK_POL_MASK, > + dai_clk_mode); > snd_soc_component_update_bits(component, DA7213_DAI_CTRL, > DA7213_DAI_FORMAT_MASK, > dai_ctrl); > snd_soc_component_write(component, DA7213_DAI_OFFSET, > dai_offset); > diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h > index 5a78dba1dcb5..9d31efc3cfe5 100644 > --- a/sound/soc/codecs/da7213.h > +++ b/sound/soc/codecs/da7213.h > @@ -181,7 +181,9 @@ > #define DA7213_DAI_BCLKS_PER_WCLK_256(0x3 << > 0) > #define DA7213_DAI_BCLKS_PER_WCLK_MASK > (0x3 << 0) > #define DA7213_DAI_CLK_POL_INV (0x1 << > 2) > +#define DA7213_DAI_CLK_POL_MASK > (0x1 << 2) > #define DA7213_DAI_WCLK_POL_INV (0x1 << > 3) > +#define DA7213_DAI_WCLK_POL_MASK (0x1 << 3) > #define DA7213_DAI_CLK_EN_MASK (0x1 << > 7) > > /* DA7213_DAI_CTRL = 0x29 */ > -- > 2.21.0
[PATCH v2] ASoC: da7219: Use clk_round_rate to handle enabled bclk/wclk case
For some platforms where DA7219 is the DAI clock master, BCLK/WCLK
will be set and enabled prior to the codec's hw_params() function
being called. It is possible the platform requires a different
BCLK configuration than would be chosen by hw_params(), for
example S16_LE format needed with a 64-bit frame to satisfy certain
devices using the clocks.
To handle those kinds of scenarios, the use of clk_round_rate() is
now employed as part of hw_params(). If BCLK is already enabled
then this function will just return the currently set rate, if it
is valid for the desired frame size, so the subsequent call to
clk_set_rate() will succeed and nothing changes with regards to
clocking. In addition the specific BCLK & WCLK recalc_rate()
implementations needed updating to always give back a real value,
as those functions are called as part of the clk init code and a
real value is needed for the clk_round_rate() call to work as
expected.
Signed-off-by: Adam Thomson
---
Changes in v2:
- Removed clk_round_rate() call for WCLK/SR as this is not sensible.
- Added checking after clk_round_rate() call for BCLK to verify returne rate is
suitable for the hw_params() desired frame size.
sound/soc/codecs/da7219.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 5f5fa34..206d01c 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1621,6 +1621,21 @@ static int da7219_hw_params(struct snd_pcm_substream
*substream,
if (bclk) {
bclk_rate = frame_size * sr;
+ /*
+* Rounding the rate here avoids failure trying to set a
+* new rate on an already enabled bclk. In that
+* instance this will just set the same rate as is
+* currently in use, and so should continue without
+* problem, as long as the BCLK rate is suitable for the
+* desired frame size.
+*/
+ bclk_rate = clk_round_rate(bclk, bclk_rate);
+ if ((bclk_rate / sr) < frame_size) {
+ dev_err(component->dev,
+ "BCLK rate mismatch against frame
size");
+ return -EINVAL;
+ }
+
ret = clk_set_rate(bclk, bclk_rate);
if (ret) {
dev_err(component->dev,
@@ -1927,9 +1942,6 @@ static unsigned long da7219_wclk_recalc_rate(struct
clk_hw *hw,
struct snd_soc_component *component = da7219->component;
u8 fs = snd_soc_component_read32(component, DA7219_SR);
- if (!da7219->master)
- return 0;
-
switch (fs & DA7219_SR_MASK) {
case DA7219_SR_8000:
return 8000;
@@ -2016,9 +2028,6 @@ static unsigned long da7219_bclk_recalc_rate(struct
clk_hw *hw,
u8 bclks_per_wclk = snd_soc_component_read32(component,
DA7219_DAI_CLK_MODE);
- if (!da7219->master)
- return 0;
-
switch (bclks_per_wclk & DA7219_DAI_BCLKS_PER_WCLK_MASK) {
case DA7219_DAI_BCLKS_PER_WCLK_32:
return parent_rate * 32;
--
1.9.1
RE: [PATCH] ASoC: da7219: Use clk_round_rate to handle enabled bclk/wclk case
On 27 April 2019 18:20, Mark Brown wrote:
> On Fri, Apr 26, 2019 at 01:59:24PM +0100, Adam Thomson wrote:
>
> > + /*
> > +* Rounding the rate here avoids failure trying to set a new
> > +* rate on an already enabled wclk. In that instance this will
> > +* just set the same rate as is currently in use, and so should
> > +* continue without problem.
> > +*/
> > + sr = clk_round_rate(wclk, sr);
> > ret = clk_set_rate(wclk, sr);
> > if (ret) {
> > dev_err(component->dev,
>
> Don't we need to validate that the rounded rate is actually viable for
> the parameters we're trying to set here? If there's missing constraints
> causing something to try to do something unsupportable then we should
> return an error rather than silently accept.
Thanks for directing my gaze to this again. Actually I don't think the SR should
be rounded at all. If it doesn't match exactly it should fail so I'll remove the
rounding here. Not sure what my brain was doing there.
For the BCLK rate, I'll see what checking can be added there to make sure the
word length is compatible with the 'rounded' BCLK rate.
RE: [PATCH] ASoC: da7213: fix DAI_CLK_EN register bit overwrite
On 25 April 2019 06:43, Logesh Kolandavel wrote: > From: Logesh > > If the da7213 codec is configured as Master with the DAPM power down > delay time set, 'snd_soc_component_write' function overwrites the > DAI_CLK_EN bit of DAI_CLK_MODE register which leads to audio play > only once until it re-initialize after codec power up. > > Signed-off-by: Logesh > --- > sound/soc/codecs/da7213.c | 4 +++- > sound/soc/codecs/da7213.h | 2 ++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c > index 92d006a5283e..306e01684bda 100644 > --- a/sound/soc/codecs/da7213.c > +++ b/sound/soc/codecs/da7213.c > @@ -1305,7 +1305,9 @@ static int da7213_set_dai_fmt(struct snd_soc_dai > *codec_dai, unsigned int fmt) > /* By default only 64 BCLK per WCLK is supported */ > dai_clk_mode |= DA7213_DAI_BCLKS_PER_WCLK_64; > > - snd_soc_component_write(component, DA7213_DAI_CLK_MODE, > dai_clk_mode); > + snd_soc_component_update_bits(component, > DA7213_DAI_CLK_MODE, > + DA7213_DAI_CLK_POL_MASK | > DA7213_DAI_WCLK_POL_MASK, We'll also need to add the mask DA7213_DAI_BCLKS_PER_WCLK_MASK' here as well as we're setting 'DA7213_DAI_BCLKS_PER_WCLK_64' for this register just prior to this call. Didn't spot that in earlier conversations. > + dai_clk_mode); > snd_soc_component_update_bits(component, DA7213_DAI_CTRL, > DA7213_DAI_FORMAT_MASK, > dai_ctrl); > snd_soc_component_write(component, DA7213_DAI_OFFSET, > dai_offset); > diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h > index 5a78dba1dcb5..9d31efc3cfe5 100644 > --- a/sound/soc/codecs/da7213.h > +++ b/sound/soc/codecs/da7213.h > @@ -181,7 +181,9 @@ > #define DA7213_DAI_BCLKS_PER_WCLK_256(0x3 << > 0) > #define DA7213_DAI_BCLKS_PER_WCLK_MASK > (0x3 << 0) > #define DA7213_DAI_CLK_POL_INV (0x1 << > 2) > +#define DA7213_DAI_CLK_POL_MASK > (0x1 << 2) > #define DA7213_DAI_WCLK_POL_INV (0x1 << > 3) > +#define DA7213_DAI_WCLK_POL_MASK (0x1 << 3) > #define DA7213_DAI_CLK_EN_MASK (0x1 << > 7) > > /* DA7213_DAI_CTRL = 0x29 */ > -- > 2.21.0
[PATCH] ASoC: da7219: Use clk_round_rate to handle enabled bclk/wclk case
For some platforms where DA7219 is the DAI clock master, BCLK/WCLK
will be set and enabled prior to the codec's hw_params() function
being called. It is possible the platform requires a different
BCLK/WCLK configuration than would be chosen by hw_params(), for
example S16_LE format needed with a 64-bit frame to satisfy certain
devices using the clocks.
To handle those kinds of scenarios, the use of clk_round_rate() is
now employed as part of hw_params(). If BCLK/WCLk are already
enabled then this function will just return the currently set
rates, so the subsequent calls to clk_set_rate() will succeed and
nothing changes with regards to clocking. In addition the specific
recalc_rate() implementations needed updating to always give back
a real value, as those functions are called as part of the clk
init code and a real value is needed for the clk_round_rate() calls
to work as expected.
Signed-off-by: Adam Thomson
---
sound/soc/codecs/da7219.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 5f5fa34..72268f5 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1593,6 +1593,13 @@ static int da7219_hw_params(struct snd_pcm_substream
*substream,
sr = params_rate(params);
if (da7219->master && wclk) {
+ /*
+* Rounding the rate here avoids failure trying to set a new
+* rate on an already enabled wclk. In that instance this will
+* just set the same rate as is currently in use, and so should
+* continue without problem.
+*/
+ sr = clk_round_rate(wclk, sr);
ret = clk_set_rate(wclk, sr);
if (ret) {
dev_err(component->dev,
@@ -1621,6 +1628,14 @@ static int da7219_hw_params(struct snd_pcm_substream
*substream,
if (bclk) {
bclk_rate = frame_size * sr;
+ /*
+* Rounding the rate here avoids failure trying to set a
+* new rate on an already enabled bclk. In that
+* instance this will just set the same rate as is
+* currently in use, and so should continue without
+* problem.
+*/
+ bclk_rate = clk_round_rate(bclk, bclk_rate);
ret = clk_set_rate(bclk, bclk_rate);
if (ret) {
dev_err(component->dev,
@@ -1927,9 +1942,6 @@ static unsigned long da7219_wclk_recalc_rate(struct
clk_hw *hw,
struct snd_soc_component *component = da7219->component;
u8 fs = snd_soc_component_read32(component, DA7219_SR);
- if (!da7219->master)
- return 0;
-
switch (fs & DA7219_SR_MASK) {
case DA7219_SR_8000:
return 8000;
@@ -2016,9 +2028,6 @@ static unsigned long da7219_bclk_recalc_rate(struct
clk_hw *hw,
u8 bclks_per_wclk = snd_soc_component_read32(component,
DA7219_DAI_CLK_MODE);
- if (!da7219->master)
- return 0;
-
switch (bclks_per_wclk & DA7219_DAI_BCLKS_PER_WCLK_MASK) {
case DA7219_DAI_BCLKS_PER_WCLK_32:
return parent_rate * 32;
--
1.9.1
RE: [PATCH v2] usb: typec: tcpm: collision avoidance
On 10 April 2019 17:39, Hans de Goede wrote: > On 10-04-19 18:14, Adam Thomson wrote: > > On 10 April 2019 16:45, Hans de Goede wrote: > > > >> Hi Kyle, > >> > >> On 10-04-19 14:49, Kyle Tso wrote: > >>> On Wed, Apr 10, 2019 at 6:32 PM Adam Thomson > >>> wrote: > >>>> > >>>> On 09 April 2019 15:41, Hans de Goede wrote: > >>>> > >>>>> Hi, > >>>>> > >>>>> On 09-04-19 15:06, Heikki Krogerus wrote: > >>>>>> On Tue, Apr 09, 2019 at 04:02:30PM +0300, Heikki Krogerus wrote: > >>>>>>> +Hans > >>>>>>> > >>>>>>> On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote: > >>>>>>>> On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck > >>>>>>>> > >> wrote: > >>>>>>>>> > >>>>>>>>> On 4/4/19 7:13 AM, Heikki Krogerus wrote: > >>>>>>>>>> Hi, > >>>>>>>>>> > >>>>>>>>>> On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote: > >>>>>>>>>>> This patch provides the implementation of Collision > >>>>>>>>>>> Avoidance introduced in PD3.0. The start of each Atomic > >>>>>>>>>>> Message Sequence > >>>>>>>>>>> (AMS) initiated by the port will be denied if the current > >>>>>>>>>>> AMS is not interruptible. The Source port will set the CC to > >>>>>>>>>>> SinkTxNG if it is going to initiate an AMS, and SinkTxOk > >>>>>>>>>>> otherwise. > >>>>>>>>>>> Meanwhile, any AMS initiated by a Sink port will be denied > >>>>>>>>>>> in TCPM if the port partner (Source) sets SinkTxNG except > >>>>>>>>>>> for HARD_RESET > >>>>> and SOFT_RESET. > >>>>>>>>>> > >>>>>>>>>> I tested this with my GDBWin which has fusb302. When I > >>>>>>>>>> plug-in DisplayPort adapter, the partner device never gets > >>>>>>>>>> registered, and I see steady flow of warnings from fusb302: > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> FWIW, I made multiple attempts to review the patch. Each time > >>>>>>>>> I get stuck after a while and notice that I don't understand > >>>>>>>>> what is going > >> on. > >>>>>>>>> > >>>>>>>>> Maybe the state machine needs a complete overhaul. It seems to > >>>>>>>>> have reached a point where it is getting too complex to > >>>>>>>>> understand what is going > >>>>> on. > >>>>>>>>> > >>>>>>>>>> [ 693.391176] Vconn is on during toggle start [ 693.391250] > >>>>>>>>>> WARNING: CPU: 2 PID: 30 at > >>>>>>>>>> drivers/usb/typec/tcpm/fusb302.c:562 > >>>>>>>>>> fusb302_set_toggling+0x129/0x130 [fusb302] [ 693.400293] > >>>>>>>>>> Modules > >>>>> linked in: intel_xhci_usb_role_switch fusb302 tcpm roles > >>>>> pi3usb30532 > >>>>> i915 typec intel_gtt intel_cht_int33fe > >>>>>>>>>> [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G > >>>>>>>>>> W > >>>>> 5.1.0-rc3-heikki+ #17 > >>>>>>>>>> [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not > >>>>>>>>>> detect charger type [ 693.412278] Hardware name: Default > >>>>>>>>>> string Default string/Default string, BIOS 5.11 05/25/2017 [ > >>>>>>>>>> 693.412283] > >>>>>>>>>> Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm] [ > >>>>>>>>>> 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130 > >>>>>>>>>> [fusb302] [ 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 > >>>>>>>>>> c6 > >>>>>>>>>>
RE: [PATCH v2] usb: typec: tcpm: collision avoidance
On 09 April 2019 15:41, Hans de Goede wrote:
> Hi,
>
> On 09-04-19 15:06, Heikki Krogerus wrote:
> > On Tue, Apr 09, 2019 at 04:02:30PM +0300, Heikki Krogerus wrote:
> >> +Hans
> >>
> >> On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote:
> >>> On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck wrote:
>
> On 4/4/19 7:13 AM, Heikki Krogerus wrote:
> > Hi,
> >
> > On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
> >> This patch provides the implementation of Collision Avoidance
> >> introduced in PD3.0. The start of each Atomic Message Sequence
> >> (AMS) initiated by the port will be denied if the current AMS is
> >> not interruptible. The Source port will set the CC to SinkTxNG if
> >> it is going to initiate an AMS, and SinkTxOk otherwise.
> >> Meanwhile, any AMS initiated by a Sink port will be denied in
> >> TCPM if the port partner (Source) sets SinkTxNG except for HARD_RESET
> and SOFT_RESET.
> >
> > I tested this with my GDBWin which has fusb302. When I plug-in
> > DisplayPort adapter, the partner device never gets registered, and
> > I see steady flow of warnings from fusb302:
> >
>
> FWIW, I made multiple attempts to review the patch. Each time I get
> stuck after a while and notice that I don't understand what is going on.
>
> Maybe the state machine needs a complete overhaul. It seems to have
> reached a point where it is getting too complex to understand what is
> going
> on.
>
> > [ 693.391176] Vconn is on during toggle start [ 693.391250]
> > WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562
> > fusb302_set_toggling+0x129/0x130 [fusb302] [ 693.400293] Modules
> linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532 i915
> typec
> intel_gtt intel_cht_int33fe
> > [ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: GW
> 5.1.0-rc3-heikki+ #17
> > [ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not detect
> > charger type [ 693.412278] Hardware name: Default string Default
> > string/Default string, BIOS 5.11 05/25/2017 [ 693.412283]
> > Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm] [
> > 693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130 [fusb302] [
> > 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6 c6 83 b0 01 00
> > 00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0 e8
> > 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54 41 89
> > f4 55 53 48 8d [ 693.436204] RSP: :c976bd90 EFLAGS:
> > 00010286 [ 693.439174] RAX: RBX:
> > 888178080028 RCX: [ 693.442157] RDX:
> > 001f RSI: 8259051f RDI: 8259091f [
> > 693.445130] RBP: 0003 R08: 82590500 R09:
> > 000202c0 [ 693.448100] R10: 010cb24a3d18 R11:
> > 001e R12: 8881780801b0 [ 693.451086] R13:
> a021e4e5 R14: 0003 R15: 888178080040 [ 693.454060]
> FS:
> () GS:88817bb0() knlGS: [
> 693.460009] CS: 0010 DS: ES: CR0: 80050033 [ 693.462984]
> CR2:
> f7fb74a0 CR3: 0200d000 CR4: 001006e0 [ 693.465969]
> Call Trace:
> > [ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302] [ 693.471894]
> > tcpm_ams_start+0x1b8/0x2a0 [tcpm]
>
> tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter
> what. This causes the fusb302 code to start toggling. As such, it
> may well attempt to start toggling in the wrong state.
>
> Guenter
>
> >>>
> >>> I read the fusb302 spec but failed to find the statement that says
> >>> it should "set toggling" when CC switches among default/medium/high.
> >>>
> >>> quot from fusb302 spec:
> >>> "The FUSB302 allows the host software to change the charging current
> >>> capabilities of the port through the HOST_CUR control bits. If the
> >>> HOST_CUR bits are changed prior to attach, the FUSB302 automatically
> >>> indicates the programmed current capability when a device is attached.
> >>> If the current capabilities are changed after a device is attached,
> >>> the FUSB302 immediately changes the CC line to the programmed
> >>> capability."
> >>>
> >>> Is it possible to skip fusb302_set_toggling() @ line#658 if
> >>> tcpm_set_cc() is called in order to switch the cc among
> >>> default/medium/high of Rp ?
> >
> > Hans, you introduced that in commit daf81d0137a9c ("usb: typec:
> > fusb302: Refactor / simplify tcpm_set_cc()"), so could you take a look
> > at this.
>
> I do not believe that that commit introduces the fusb302_set_toggling() as the
> subject of the commit says it just refactors things, the set_toggling call was
> introduced by:
>
> commit ea3b4d5523bc8("usb: typec: fusb302: Resolve fixed power role contract
> setup")
>
> Before that:
>
>
RE: [PATCH 1/3] ASoC: da7219: Update DAI clock binding info to cover WCLK/BCLK
On 21 March 2019 12:22, Mark Brown wrote: > On Tue, Mar 19, 2019 at 05:49:30PM +0000, Adam Thomson wrote: > > > -- #clock-cells : Should be set to '<0>', only one clock source > > provided; > > -- clock-output-names : Name given for DAI clocks output; > > +- #clock-cells : Should be set to '<1>', two clock sources provided; > > +- clock-output-names : Names given for DAI clock outputs (WCLK & > > +BCLK); > > This might need some handling later on as it's a binding change, however I > can't > see any current users so hopefully nobody will notice. No I do not believe there are users of this, other than the AMD board which doesn't use these bindings. Actually given they're the same bindings, albeit with a different number of cells, I've tested the code with just 1 cell and 1 clock in the DTS and this still works as before so the driver should be backwards compatible.
[PATCH 2/3] ASoC: da7219: Expose BCLK and WCLK control through CCF
For the purposes of platforms which use the codec as DAI clock
master for the CPU and other codec devices, there is the need to
not only expose the clock gating of BCLK and WCLK but also the
ability to set those rates without going through the ASoC APIs.
To make this possible, the previous CCF implementation in the
driver has been extended to separate BCLK and WCLK out. WCLK is
the parent clock to BCLK, and is also the clock gate for both.
BCLK in HW is a factor/multiplier of WCLK so derives from whatever
SR is chosen for WCLK, hence the need to make it a child of WCLK
for the purposes of CCF. Enabling/disabling either BCLK or WCLK
will result in clocks being ungated/gated accordingly. To simplify
matters, these clocks can only be configured if the codec is set
as master, otherwise CCF control is disallowed.
Signed-off-by: Adam Thomson
---
include/sound/da7219.h| 8 +-
sound/soc/codecs/da7219.c | 523 +++---
sound/soc/codecs/da7219.h | 6 +-
3 files changed, 407 insertions(+), 130 deletions(-)
diff --git a/include/sound/da7219.h b/include/sound/da7219.h
index 1bfcb16..4a36954 100644
--- a/include/sound/da7219.h
+++ b/include/sound/da7219.h
@@ -33,10 +33,16 @@ enum da7219_mic_amp_in_sel {
struct da7219_aad_pdata;
+enum da7219_dai_clks {
+ DA7219_DAI_WCLK_IDX = 0,
+ DA7219_DAI_BCLK_IDX,
+ DA7219_DAI_NUM_CLKS,
+};
+
struct da7219_pdata {
bool wakeup_source;
- const char *dai_clks_name;
+ const char *dai_clk_names[DA7219_DAI_NUM_CLKS];
/* Mic */
enum da7219_micbias_voltage micbias_lvl;
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 121a819..5f5fa34 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -797,6 +797,7 @@ static int da7219_dai_event(struct snd_soc_dapm_widget *w,
{
struct snd_soc_component *component =
snd_soc_dapm_to_component(w->dapm);
struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
+ struct clk *bclk = da7219->dai_clks[DA7219_DAI_BCLK_IDX];
u8 pll_ctrl, pll_status;
int i = 0, ret;
bool srm_lock = false;
@@ -805,11 +806,11 @@ static int da7219_dai_event(struct snd_soc_dapm_widget *w,
case SND_SOC_DAPM_PRE_PMU:
if (da7219->master) {
/* Enable DAI clks for master mode */
- if (da7219->dai_clks) {
- ret = clk_prepare_enable(da7219->dai_clks);
+ if (bclk) {
+ ret = clk_prepare_enable(bclk);
if (ret) {
dev_err(component->dev,
- "Failed to enable dai_clks\n");
+ "Failed to enable DAI clks\n");
return ret;
}
} else {
@@ -852,8 +853,8 @@ static int da7219_dai_event(struct snd_soc_dapm_widget *w,
/* Disable DAI clks if in master mode */
if (da7219->master) {
- if (da7219->dai_clks)
- clk_disable_unprepare(da7219->dai_clks);
+ if (bclk)
+ clk_disable_unprepare(bclk);
else
snd_soc_component_update_bits(component,
DA7219_DAI_CLK_MODE,
@@ -1385,17 +1386,50 @@ static int da7219_set_dai_fmt(struct snd_soc_dai
*codec_dai, unsigned int fmt)
return 0;
}
+static int da7219_set_bclks_per_wclk(struct snd_soc_component *component,
+unsigned long factor)
+{
+ u8 bclks_per_wclk;
+
+ switch (factor) {
+ case 32:
+ bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_32;
+ break;
+ case 64:
+ bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_64;
+ break;
+ case 128:
+ bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_128;
+ break;
+ case 256:
+ bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_256;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
+ DA7219_DAI_BCLKS_PER_WCLK_MASK,
+ bclks_per_wclk);
+
+ return 0;
+}
+
static int da7219_set_dai_tdm_slot(struct snd_soc_dai *dai,
unsigned int tx_mask, unsigned int rx_mask,
int slots, int slot_width)
{
struct snd_soc_component *component = dai->component;
struct da7219_priv *da7219 = snd_soc_component_get_drvdata(compo
[PATCH 1/3] ASoC: da7219: Update DAI clock binding info to cover WCLK/BCLK
With the need to expose WCLK and BCLK as separate clocks, the 'clock-cells' and 'clock-output-names' descriptions need to be updated as now the codec is providing 2 clocks. The example is also updated accordingly. Signed-off-by: Adam Thomson --- Documentation/devicetree/bindings/sound/da7219.txt | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/devicetree/bindings/sound/da7219.txt b/Documentation/devicetree/bindings/sound/da7219.txt index e9d0bae..add1caf 100644 --- a/Documentation/devicetree/bindings/sound/da7219.txt +++ b/Documentation/devicetree/bindings/sound/da7219.txt @@ -23,8 +23,8 @@ Optional properties: interrupt is to be used to wake system, otherwise "irq" should be used. - wakeup-source: Flag to indicate this device can wake system (suspend/resume). -- #clock-cells : Should be set to '<0>', only one clock source provided; -- clock-output-names : Name given for DAI clocks output; +- #clock-cells : Should be set to '<1>', two clock sources provided; +- clock-output-names : Names given for DAI clock outputs (WCLK & BCLK); - clocks : phandle and clock specifier for codec MCLK. - clock-names : Clock name string for 'clocks' attribute, should be "mclk". @@ -84,8 +84,8 @@ Example: VDDMIC-supply = <_audio>; VDDIO-supply = <_audio>; - #clock-cells = <0>; - clock-output-names = "dai-clks"; + #clock-cells = <1>; + clock-output-names = "dai-wclk", "dai-bclk"; clocks = < 201>; clock-names = "mclk"; -- 1.9.1
[PATCH 0/3] ASoC: da7219: Extend CCF support to allow WCLK/BCLK config
This patch set extends the existing CCF support in the DA7219 codec driver by splitting the DAI clock gate into clocks to cover WCLK and BCLK, to enable the configuration of these clocks from audio machine drivers when this codec is the clock master of the DAI. Adam Thomson (3): ASoC: da7219: Update DAI clock binding info to cover WCLK/BCLK ASoC: da7219: Expose BCLK and WCLK control through CCF ASoC: AMD: Update DA7219 DAI clock name to align with codec updates Documentation/devicetree/bindings/sound/da7219.txt | 8 +- include/sound/da7219.h | 8 +- sound/soc/amd/acp-da7219-max98357a.c | 2 +- sound/soc/codecs/da7219.c | 523 - sound/soc/codecs/da7219.h | 6 +- 5 files changed, 412 insertions(+), 135 deletions(-) -- 1.9.1
[PATCH 3/3] ASoC: AMD: Update DA7219 DAI clock name to align with codec updates
As the DAI clocks for DA7219 have now been split into BCLK and WCLK, the clock lookup name needs to be udpated here to select BCLK to achieve the same functionality as before with regards to DAI clock gating. Signed-off-by: Adam Thomson --- sound/soc/amd/acp-da7219-max98357a.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/amd/acp-da7219-max98357a.c b/sound/soc/amd/acp-da7219-max98357a.c index a5daad9..f37a588 100644 --- a/sound/soc/amd/acp-da7219-max98357a.c +++ b/sound/soc/amd/acp-da7219-max98357a.c @@ -72,7 +72,7 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd) return ret; } - da7219_dai_clk = clk_get(component->dev, "da7219-dai-clks"); + da7219_dai_clk = clk_get(component->dev, "da7219-dai-bclk"); ret = snd_soc_card_jack_new(card, "Headset Jack", SND_JACK_HEADSET | SND_JACK_LINEOUT | -- 1.9.1
RE: [alsa-devel] [PATCH] ASoC: AMD: Configure master codec on all playback/capture cases
On 8 February 2019 18:18, Mark Brown wrote:
> On Fri, Feb 15, 2019 at 07:06:15AM +, Agrawal, Akshu wrote:
>
> > +static static int cz_da7219_hw_params(struct snd_pcm_substream
> *substream,
> > + struct snd_pcm_hw_params *params) {
> > + /* da7219 Codec is clock master so setup as per the needs */
> > + if (codec_dai->driver->ops->hw_params)
> > + return codec_dai->driver->ops->hw_params(substream, params,
> > +codec_dai);
> > + return 0;
> > +}
>
> No, this is a bad idea - you shouldn't be callings ops in drivers outside of
> the
> framework, something is going to break on you there.
> Either the drivers or the frameworks are going to get surprised by things
> getting
> called out of sequence, for example I'd not rely on the fact that the CODEC
> would
> be outputing a clock without active audio.
> This probably means that you're going to have to extend the framework in some
> way, or refactor things so that the BCLK is exported as a clock by the CODEC
> and
> reflect the relationships via the clock API. I need to think a bit more but
> I think
> the clock API is going to be the the best solution.
>
> I also suspect that some work is needed to propagate constraints between the
> various DAIs in the system.
So this was a suggestion from my side to avoid the scenario where the system
starts, the DA7219 codec hasn't yet been used (hw_params() not been called), and
one of the other audio devices is needed for playback/capture. The codec will be
at a default DAI configuration rather than the configuration that's required. If
the DA7219 codec had been used first then of course there's no problem and the
system runs as expected from then on.
This AMD platform limits all DAIs to 48KHz, 2 channel so the configuration is
fixed, as I understand it. DA7219 will output DAI clocks without data so there's
no problem there. I initially suggested using 'soc_dai_hw_params()' but the
function isn't a global symbol, probably intentionally, so for this more unusual
config I felt the above approach might be an option. Absolutely understand your
concerns though.
I considered the CCF approach but I'm not 100% confident that the clocking
mechanism of the codec will fit ideally into that framework. There's already
a clock gate for the DAI clocks, in the driver, but nothing more. The problem
here is that the BCLK rate is intrinsically linked to the WCLK rate, as well as
the word length, so changing the desired BCLK rate would affect the WCLK rate,
and vice versa. I will take a look again, but if you have any suggestions then
they'd be greatly received.
RE: [PATCH] ASoC: AMD: Configure master codec on all playback/capture cases
On 15 February 2019 07:06, Akshu Agrawal wrote:
> In the system design da7219 is the master codec and clocks are generated by
> it.
> Bclk is to be generated at the required rate for other codecs used when
> da7219 is acting only as clock master. For this call hw_params of da7219
> during
> playback/capture on non da7219 codecs.
>
> Being able to set bclk at lower rate also fixes noise issue observed on some
> dmics.
>
> Signed-off-by: Akshu Agrawal
Reviewed-by: Adam Thomson
> ---
> sound/soc/amd/acp-da7219-max98357a.c | 20 ++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/amd/acp-da7219-max98357a.c b/sound/soc/amd/acp-
> da7219-max98357a.c
> index a5daad973ce5..ad327415290a 100644
> --- a/sound/soc/amd/acp-da7219-max98357a.c
> +++ b/sound/soc/amd/acp-da7219-max98357a.c
> @@ -45,6 +45,7 @@
> #define CZ_PLAT_CLK 4800
> #define DUAL_CHANNEL 2
>
> +static struct snd_soc_dai *codec_dai;
> static struct snd_soc_jack cz_jack;
> static struct clk *da7219_dai_clk;
> extern int bt_uart_enable;
> @@ -53,8 +54,10 @@ static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
> {
> int ret;
> struct snd_soc_card *card = rtd->card;
> - struct snd_soc_dai *codec_dai = rtd->codec_dai;
> - struct snd_soc_component *component = codec_dai->component;
> + struct snd_soc_component *component;
> +
> + codec_dai = rtd->codec_dai;
> + component = codec_dai->component;
>
> dev_info(rtd->dev, "codec dai name = %s\n", codec_dai->name);
>
> @@ -255,6 +258,16 @@ static void cz_dmic_shutdown(struct
> snd_pcm_substream *substream)
> da7219_clk_disable();
> }
>
> +static static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params) {
> + /* da7219 Codec is clock master so setup as per the needs */
> + if (codec_dai->driver->ops->hw_params)
> + return codec_dai->driver->ops->hw_params(substream, params,
> + codec_dai);
> + return 0;
> +}
> +
> static const struct snd_soc_ops cz_da7219_play_ops = {
> .startup = cz_da7219_play_startup,
> .shutdown = cz_da7219_shutdown,
> @@ -268,16 +281,19 @@ static const struct snd_soc_ops cz_da7219_cap_ops = {
> static const struct snd_soc_ops cz_max_play_ops = {
> .startup = cz_max_startup,
> .shutdown = cz_max_shutdown,
> + .hw_params = cz_da7219_hw_params,
> };
>
> static const struct snd_soc_ops cz_dmic0_cap_ops = {
> .startup = cz_dmic0_startup,
> .shutdown = cz_dmic_shutdown,
> + .hw_params = cz_da7219_hw_params,
> };
>
> static const struct snd_soc_ops cz_dmic1_cap_ops = {
> .startup = cz_dmic1_startup,
> .shutdown = cz_dmic_shutdown,
> + .hw_params = cz_da7219_hw_params,
> };
>
> static struct snd_soc_dai_link cz_dai_7219_98357[] = {
> --
> 2.17.1
RE: [PATCH] usb: typec: tcpm: Export partner Source Capabilities
On 14 February 2019 14:30, Greg KH wrote:
> On Thu, Feb 14, 2019 at 02:17:14PM +0000, Adam Thomson wrote:
> > On 12 February 2019 16:20, Guenter Roeck wrote:
> >
> > > On 2/12/19 2:54 AM, Greg KH wrote:
> > > > On Tue, Feb 12, 2019 at 06:29:39PM +0800, Kyle Tso wrote:
> > > >> On Thu, Jan 31, 2019 at 3:02 PM Greg KH
> > > >>
> > > wrote:
> > > >>
> > > >>> On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote:
> > > >>>> Provide a function to get the partner Source Capabilities.
> > > >>>>
> > > >>>> Signed-off-by: Kyle Tso
> > > >>>> ---
> > > >>>> drivers/usb/typec/tcpm/tcpm.c | 23 +++
> > > >>>> include/linux/usb/tcpm.h | 1 +
> > > >>>> 2 files changed, 24 insertions(+)
> > > >>>>
> > > >>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c
> > > >>> b/drivers/usb/typec/tcpm/tcpm.c
> > > >>>> index f1d3e54210df..29cd84ba9960 100644
> > > >>>> --- a/drivers/usb/typec/tcpm/tcpm.c
> > > >>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > >>>> @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct
> > > >>> tcpm_port *port, const u32 *pdo,
> > > >>>> }
> > > >>>> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
> > > >>>>
> > > >>>> +/*
> > > >>>> + * Don't call this function in interrupt context. Caller needs
> > > >>>> +to free
> > > >>> the
> > > >>>> + * memory itself.
> > > >>>> + */
> > > >>>> +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32
> > > >>>> +**src_pdo) {
> > > >>>> + unsigned int nr_pdo;
> > > >>>> +
> > > >>>> + if (port->nr_source_caps == 0)
> > > >>>> + return -ENODATA;
> > > >>>> +
> > > >>>> + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32),
> GFP_KERNEL);
> > > >>>> + if (!src_pdo)
> > > >>>> + return -ENOMEM;
> > > >>>> +
> > > >>>> + mutex_lock(>lock);
> > > >>>> + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps,
> > > >>>> + port->nr_source_caps);
> > > >>>> + mutex_unlock(>lock);
> > > >>>> + return nr_pdo;
> > > >>>> +}
> > > >>>> +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps);
> > > >>>
> > > >>> We don't add new functions that no one uses :(
> > > >>>
> > > >>>
> > > >> This function is useful if the PD Device Policy Manager is
> > > >> implemented outside of TCPM.
> > > >> In this situation, Device Policy Manager needs to know the
> > > >> partner capabilities to optimize the charging process.
> > > >
> > > > And where is that code?
> > > >
> > >
> > > Agreed - that code should be sent upstream as well to let us see the
> > > entire context.
> > >
> > > >> Take existing functions in TCPM for example:
> > > >> Function "tcpm_update_sink_capabilities" and
> > > >> "tcpm_update_source_capabilities" are exposed as well. And no one
> > > >> uses them now.
> > > >
> > > > Great, let's go delete them now, we should not have apis that no
> > > > one uses. This isn't a new thing...
> > > >
> > >
> > > I sent a patch to do just that. Quite frankly I don't recal why I
> > > thought those functions might be needed.
> >
> > As a mind jog - https://lkml.org/lkml/2017/11/27/1256 :)
>
> Nice find :)
>
> Given that no one used these functions in over a year, we should be safe
> deleting them. If not, it's trivial to do 'git revert'.
Yep, agreed.
RE: [PATCH] usb: typec: tcpm: Export partner Source Capabilities
On 12 February 2019 16:20, Guenter Roeck wrote:
> On 2/12/19 2:54 AM, Greg KH wrote:
> > On Tue, Feb 12, 2019 at 06:29:39PM +0800, Kyle Tso wrote:
> >> On Thu, Jan 31, 2019 at 3:02 PM Greg KH
> wrote:
> >>
> >>> On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote:
> Provide a function to get the partner Source Capabilities.
>
> Signed-off-by: Kyle Tso
> ---
> drivers/usb/typec/tcpm/tcpm.c | 23 +++
> include/linux/usb/tcpm.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c
> >>> b/drivers/usb/typec/tcpm/tcpm.c
> index f1d3e54210df..29cd84ba9960 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct
> >>> tcpm_port *port, const u32 *pdo,
> }
> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>
> +/*
> + * Don't call this function in interrupt context. Caller needs to
> +free
> >>> the
> + * memory itself.
> + */
> +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32
> +**src_pdo) {
> + unsigned int nr_pdo;
> +
> + if (port->nr_source_caps == 0)
> + return -ENODATA;
> +
> + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), GFP_KERNEL);
> + if (!src_pdo)
> + return -ENOMEM;
> +
> + mutex_lock(>lock);
> + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps,
> + port->nr_source_caps);
> + mutex_unlock(>lock);
> + return nr_pdo;
> +}
> +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps);
> >>>
> >>> We don't add new functions that no one uses :(
> >>>
> >>>
> >> This function is useful if the PD Device Policy Manager is
> >> implemented outside of TCPM.
> >> In this situation, Device Policy Manager needs to know the partner
> >> capabilities to optimize the charging process.
> >
> > And where is that code?
> >
>
> Agreed - that code should be sent upstream as well to let us see the entire
> context.
>
> >> Take existing functions in TCPM for example:
> >> Function "tcpm_update_sink_capabilities" and
> >> "tcpm_update_source_capabilities" are exposed as well. And no one
> >> uses them now.
> >
> > Great, let's go delete them now, we should not have apis that no one
> > uses. This isn't a new thing...
> >
>
> I sent a patch to do just that. Quite frankly I don't recal why I thought
> those
> functions might be needed.
As a mind jog - https://lkml.org/lkml/2017/11/27/1256 :)
[PATCH v2 1/2] ASoC: da7219: Add support for master mode BCLK rate adjustment
Previously the driver would default the BCLK periods per WCLK to
64, to cover all possible non-TDM scenarios when the codec was
DAI clock master. However some devices require a lower BCLK rate
to operate correctly so with this in mind, this commit updates
the code to be more dynamic, with BCLK rate now based on SR and
word length provided to hw_params().
Signed-off-by: Adam Thomson
---
sound/soc/codecs/da7219.c | 36 ++--
sound/soc/codecs/da7219.h | 1 +
2 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index b1df4bb..c599aa9 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1376,11 +1376,7 @@ static int da7219_set_dai_fmt(struct snd_soc_dai
*codec_dai, unsigned int fmt)
return -EINVAL;
}
- /* By default 64 BCLKs per WCLK is supported */
- dai_clk_mode |= DA7219_DAI_BCLKS_PER_WCLK_64;
-
snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
- DA7219_DAI_BCLKS_PER_WCLK_MASK |
DA7219_DAI_CLK_POL_MASK | DA7219_DAI_WCLK_POL_MASK,
dai_clk_mode);
snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
DA7219_DAI_FORMAT_MASK,
@@ -1399,14 +1395,12 @@ static int da7219_set_dai_tdm_slot(struct snd_soc_dai
*dai,
__le16 offset;
u32 frame_size;
- /* No channels enabled so disable TDM, revert to 64-bit frames */
+ /* No channels enabled so disable TDM */
if (!tx_mask) {
snd_soc_component_update_bits(component, DA7219_DAI_TDM_CTRL,
DA7219_DAI_TDM_CH_EN_MASK |
DA7219_DAI_TDM_MODE_EN_MASK, 0);
- snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
- DA7219_DAI_BCLKS_PER_WCLK_MASK,
- DA7219_DAI_BCLKS_PER_WCLK_64);
+ da7219->tdm_en = false;
return 0;
}
@@ -1458,6 +1452,8 @@ static int da7219_set_dai_tdm_slot(struct snd_soc_dai
*dai,
(tx_mask << DA7219_DAI_TDM_CH_EN_SHIFT) |
DA7219_DAI_TDM_MODE_EN_MASK);
+ da7219->tdm_en = true;
+
return 0;
}
@@ -1466,10 +1462,13 @@ static int da7219_hw_params(struct snd_pcm_substream
*substream,
struct snd_soc_dai *dai)
{
struct snd_soc_component *component = dai->component;
- u8 dai_ctrl = 0, fs;
+ struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
+ u8 dai_ctrl = 0, dai_bclks_per_wclk = 0, fs;
unsigned int channels;
+ int word_len = params_width(params);
+ int frame_size;
- switch (params_width(params)) {
+ switch (word_len) {
case 16:
dai_ctrl |= DA7219_DAI_WORD_LENGTH_S16_LE;
break;
@@ -1533,6 +1532,23 @@ static int da7219_hw_params(struct snd_pcm_substream
*substream,
return -EINVAL;
}
+ /*
+* If we're master, then we have a limited set of BCLK rates we
+* support. For slave mode this isn't the case and the codec can detect
+* the BCLK rate automatically.
+*/
+ if (da7219->master && !da7219->tdm_en) {
+ frame_size = word_len * 2;
+ if (frame_size <= 32)
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_32;
+ else
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_64;
+
+ snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
+ DA7219_DAI_BCLKS_PER_WCLK_MASK,
+ dai_bclks_per_wclk);
+ }
+
snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
DA7219_DAI_WORD_LENGTH_MASK |
DA7219_DAI_CH_NUM_MASK,
diff --git a/sound/soc/codecs/da7219.h b/sound/soc/codecs/da7219.h
index 366cf46..018819c 100644
--- a/sound/soc/codecs/da7219.h
+++ b/sound/soc/codecs/da7219.h
@@ -830,6 +830,7 @@ struct da7219_priv {
int clk_src;
bool master;
+ bool tdm_en;
bool alc_en;
bool micbias_on_event;
unsigned int mic_pga_delay;
--
1.9.1
[PATCH v2 0/2] ASoC: da7219: BCLK and TDM flexibility improvements
This patch set updates the driver to be more flexible regarding the BCLK and TDM configurations of the device. Previously the BCLK rate was fixed at 64 periods per WCLK, when the codec is DAI clock master, but to satisfy devices which prefer a lower rate, and to save power, the BCLK rate is now calculated based on hw_params() data. As a fallout of the BCLK efforts it was also apparent that the TDM code could be made more flexible as well. The device is capable of automatically detecting the BCLK rate when it is clock slave, so previous limitations imposed were not necessary. The mask handling is now used as an input to determine the BCLK offset to align closer with other examples within ALSA. Changes in v2: - Update 0001 patch to use '&&' instead of '&' in hw_params() to decide if BCLK rate should be configured or not. Adam Thomson (2): ASoC: da7219: Add support for master mode BCLK rate adjustment ASoC: da7219: Update TDM usage to be more flexible sound/soc/codecs/da7219.c | 116 +- sound/soc/codecs/da7219.h | 1 + 2 files changed, 74 insertions(+), 43 deletions(-) -- 1.9.1
[PATCH v2 2/2] ASoC: da7219: Update TDM usage to be more flexible
The previous implementatation was restrictive with regards to
BCLK rates for slave mode where the driver would not allow rates
the codec couldn't provide itself as clock master. The codec
is able to automatically determine and handle whatever rate is
provided so this restriction isn't necessary for slave mode. The
code was also flawed with regards to setting of the frame offset
as using rx_mask to explicitly set the offset has the knock on
effect of impacting the min and max channels for the codec, in
soc_pcm_hw_params() through the call to
soc_pcm_codec_params_fixup().
With this update, the driver now only limits frame size if codec
is clock master, and dynamically determines the BCLK offset
relating to WCLK using the tx_mask for slot offset along with the
slot width provided.
Signed-off-by: Adam Thomson
---
sound/soc/codecs/da7219.c | 80 ---
1 file changed, 47 insertions(+), 33 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index c599aa9..121a819 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1391,8 +1391,10 @@ static int da7219_set_dai_tdm_slot(struct snd_soc_dai
*dai,
{
struct snd_soc_component *component = dai->component;
struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
- u8 dai_bclks_per_wclk;
- __le16 offset;
+ unsigned int ch_mask;
+ u8 dai_bclks_per_wclk, slot_offset;
+ u16 offset;
+ __le16 dai_offset;
u32 frame_size;
/* No channels enabled so disable TDM */
@@ -1405,51 +1407,63 @@ static int da7219_set_dai_tdm_slot(struct snd_soc_dai
*dai,
}
/* Check we have valid slots */
- if (fls(tx_mask) > DA7219_DAI_TDM_MAX_SLOTS) {
- dev_err(component->dev, "Invalid number of slots, max = %d\n",
+ slot_offset = ffs(tx_mask) - 1;
+ ch_mask = (tx_mask >> slot_offset);
+ if (fls(ch_mask) > DA7219_DAI_TDM_MAX_SLOTS) {
+ dev_err(component->dev,
+ "Invalid number of slots, max = %d\n",
DA7219_DAI_TDM_MAX_SLOTS);
return -EINVAL;
}
- /* Check we have a valid offset given */
- if (rx_mask > DA7219_DAI_OFFSET_MAX) {
- dev_err(component->dev, "Invalid slot offset, max = %d\n",
- DA7219_DAI_OFFSET_MAX);
+ /*
+* Ensure we have a valid offset into the frame, based on slot width
+* and slot offset of first slot we're interested in.
+*/
+ offset = slot_offset * slot_width;
+ if (offset > DA7219_DAI_OFFSET_MAX) {
+ dev_err(component->dev, "Invalid frame offset %d\n", offset);
return -EINVAL;
}
- /* Calculate & validate frame size based on slot info provided. */
- frame_size = slots * slot_width;
- switch (frame_size) {
- case 32:
- dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_32;
- break;
- case 64:
- dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_64;
- break;
- case 128:
- dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_128;
- break;
- case 256:
- dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_256;
- break;
- default:
- dev_err(component->dev, "Invalid frame size %d\n", frame_size);
- return -EINVAL;
- }
+ /*
+* If we're master, calculate & validate frame size based on slot info
+* provided as we have a limited set of rates available.
+*/
+ if (da7219->master) {
+ frame_size = slots * slot_width;
+ switch (frame_size) {
+ case 32:
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_32;
+ break;
+ case 64:
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_64;
+ break;
+ case 128:
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_128;
+ break;
+ case 256:
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_256;
+ break;
+ default:
+ dev_err(component->dev, "Invalid frame size %d\n",
+ frame_size);
+ return -EINVAL;
+ }
- snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
- DA7219_DAI_BCLKS_PER_WCLK_MASK,
- dai_bclks_per_wclk);
+ snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
+ DA7219_DAI_BCLKS_PER_WCLK_MASK,
+ d
RE: [PATCH 0/2] ASoC: da7219: BCLK and TDM flexibility improvements
On 13 February 2019 17:48, Adam Thomson wrote: This patch set updates the driver to be more flexible regarding the BCLK and TDM > configurations of the device. > > Previously the BCLK rate was fixed at 64 periods per WCLK, when the codec is > DAI > clock master, but to satisfy devices which prefer a lower rate, and to save > power, > the BCLK rate is now calculated based on hw_params() data. > > As a fallout of the BCLK efforts it was also apparent that the TDM code could > be > made more flexible as well. The device is capable of automatically detecting > the > BCLK rate when it is clock slave, so previous limitations imposed were not > necessary. The mask handling is now used as an input to determine the BCLK > offset to align closer with other examples within ALSA. Spotted a silly '&' instead of '&&' error in the 1st patch. Functionally will work but technically should be updated. Will send V2 soon. > > Adam Thomson (2): > ASoC: da7219: Add support for master mode BCLK rate adjustment > ASoC: da7219: Update TDM usage to be more flexible > > sound/soc/codecs/da7219.c | 116 +-- > --- > sound/soc/codecs/da7219.h | 1 + > 2 files changed, 74 insertions(+), 43 deletions(-) > > -- > 1.9.1
[PATCH 1/2] ASoC: da7219: Add support for master mode BCLK rate adjustment
Previously the driver would default the BCLK periods per WCLK to
64, to cover all possible non-TDM scenarios when the codec was
DAI clock master. However some devices require a lower BCLK rate
to operate correctly so with this in mind, this commit updates
the code to be more dynamic, with BCLK rate now based on SR and
word length provided to hw_params().
Signed-off-by: Adam Thomson
---
sound/soc/codecs/da7219.c | 36 ++--
sound/soc/codecs/da7219.h | 1 +
2 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index b1df4bb..7dbecf3 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1376,11 +1376,7 @@ static int da7219_set_dai_fmt(struct snd_soc_dai
*codec_dai, unsigned int fmt)
return -EINVAL;
}
- /* By default 64 BCLKs per WCLK is supported */
- dai_clk_mode |= DA7219_DAI_BCLKS_PER_WCLK_64;
-
snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
- DA7219_DAI_BCLKS_PER_WCLK_MASK |
DA7219_DAI_CLK_POL_MASK | DA7219_DAI_WCLK_POL_MASK,
dai_clk_mode);
snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
DA7219_DAI_FORMAT_MASK,
@@ -1399,14 +1395,12 @@ static int da7219_set_dai_tdm_slot(struct snd_soc_dai
*dai,
__le16 offset;
u32 frame_size;
- /* No channels enabled so disable TDM, revert to 64-bit frames */
+ /* No channels enabled so disable TDM */
if (!tx_mask) {
snd_soc_component_update_bits(component, DA7219_DAI_TDM_CTRL,
DA7219_DAI_TDM_CH_EN_MASK |
DA7219_DAI_TDM_MODE_EN_MASK, 0);
- snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
- DA7219_DAI_BCLKS_PER_WCLK_MASK,
- DA7219_DAI_BCLKS_PER_WCLK_64);
+ da7219->tdm_en = false;
return 0;
}
@@ -1458,6 +1452,8 @@ static int da7219_set_dai_tdm_slot(struct snd_soc_dai
*dai,
(tx_mask << DA7219_DAI_TDM_CH_EN_SHIFT) |
DA7219_DAI_TDM_MODE_EN_MASK);
+ da7219->tdm_en = true;
+
return 0;
}
@@ -1466,10 +1462,13 @@ static int da7219_hw_params(struct snd_pcm_substream
*substream,
struct snd_soc_dai *dai)
{
struct snd_soc_component *component = dai->component;
- u8 dai_ctrl = 0, fs;
+ struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
+ u8 dai_ctrl = 0, dai_bclks_per_wclk = 0, fs;
unsigned int channels;
+ int word_len = params_width(params);
+ int frame_size;
- switch (params_width(params)) {
+ switch (word_len) {
case 16:
dai_ctrl |= DA7219_DAI_WORD_LENGTH_S16_LE;
break;
@@ -1533,6 +1532,23 @@ static int da7219_hw_params(struct snd_pcm_substream
*substream,
return -EINVAL;
}
+ /*
+* If we're master, then we have a limited set of BCLK rates we
+* support. For slave mode this isn't the case and the codec can detect
+* the BCLK rate automatically.
+*/
+ if (da7219->master & !da7219->tdm_en) {
+ frame_size = word_len * 2;
+ if (frame_size <= 32)
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_32;
+ else
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_64;
+
+ snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
+ DA7219_DAI_BCLKS_PER_WCLK_MASK,
+ dai_bclks_per_wclk);
+ }
+
snd_soc_component_update_bits(component, DA7219_DAI_CTRL,
DA7219_DAI_WORD_LENGTH_MASK |
DA7219_DAI_CH_NUM_MASK,
diff --git a/sound/soc/codecs/da7219.h b/sound/soc/codecs/da7219.h
index 366cf46..018819c 100644
--- a/sound/soc/codecs/da7219.h
+++ b/sound/soc/codecs/da7219.h
@@ -830,6 +830,7 @@ struct da7219_priv {
int clk_src;
bool master;
+ bool tdm_en;
bool alc_en;
bool micbias_on_event;
unsigned int mic_pga_delay;
--
1.9.1
[PATCH 0/2] ASoC: da7219: BCLK and TDM flexibility improvements
This patch set updates the driver to be more flexible regarding the BCLK and TDM configurations of the device. Previously the BCLK rate was fixed at 64 periods per WCLK, when the codec is DAI clock master, but to satisfy devices which prefer a lower rate, and to save power, the BCLK rate is now calculated based on hw_params() data. As a fallout of the BCLK efforts it was also apparent that the TDM code could be made more flexible as well. The device is capable of automatically detecting the BCLK rate when it is clock slave, so previous limitations imposed were not necessary. The mask handling is now used as an input to determine the BCLK offset to align closer with other examples within ALSA. Adam Thomson (2): ASoC: da7219: Add support for master mode BCLK rate adjustment ASoC: da7219: Update TDM usage to be more flexible sound/soc/codecs/da7219.c | 116 +- sound/soc/codecs/da7219.h | 1 + 2 files changed, 74 insertions(+), 43 deletions(-) -- 1.9.1
[PATCH 2/2] ASoC: da7219: Update TDM usage to be more flexible
The previous implementatation was restrictive with regards to
BCLK rates for slave mode where the driver would not allow rates
the codec couldn't provide itself as clock master. The codec
is able to automatically determine and handle whatever rate is
provided so this restriction isn't necessary for slave mode. The
code was also flawed with regards to setting of the frame offset
as using rx_mask to explicitly set the offset has the knock on
effect of impacting the min and max channels for the codec, in
soc_pcm_hw_params() through the call to
soc_pcm_codec_params_fixup().
With this update, the driver now only limits frame size if codec
is clock master, and dynamically determines the BCLK offset
relating to WCLK using the tx_mask for slot offset along with the
slot width provided.
Signed-off-by: Adam Thomson
---
sound/soc/codecs/da7219.c | 80 ---
1 file changed, 47 insertions(+), 33 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 7dbecf3..be606e3 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1391,8 +1391,10 @@ static int da7219_set_dai_tdm_slot(struct snd_soc_dai
*dai,
{
struct snd_soc_component *component = dai->component;
struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
- u8 dai_bclks_per_wclk;
- __le16 offset;
+ unsigned int ch_mask;
+ u8 dai_bclks_per_wclk, slot_offset;
+ u16 offset;
+ __le16 dai_offset;
u32 frame_size;
/* No channels enabled so disable TDM */
@@ -1405,51 +1407,63 @@ static int da7219_set_dai_tdm_slot(struct snd_soc_dai
*dai,
}
/* Check we have valid slots */
- if (fls(tx_mask) > DA7219_DAI_TDM_MAX_SLOTS) {
- dev_err(component->dev, "Invalid number of slots, max = %d\n",
+ slot_offset = ffs(tx_mask) - 1;
+ ch_mask = (tx_mask >> slot_offset);
+ if (fls(ch_mask) > DA7219_DAI_TDM_MAX_SLOTS) {
+ dev_err(component->dev,
+ "Invalid number of slots, max = %d\n",
DA7219_DAI_TDM_MAX_SLOTS);
return -EINVAL;
}
- /* Check we have a valid offset given */
- if (rx_mask > DA7219_DAI_OFFSET_MAX) {
- dev_err(component->dev, "Invalid slot offset, max = %d\n",
- DA7219_DAI_OFFSET_MAX);
+ /*
+* Ensure we have a valid offset into the frame, based on slot width
+* and slot offset of first slot we're interested in.
+*/
+ offset = slot_offset * slot_width;
+ if (offset > DA7219_DAI_OFFSET_MAX) {
+ dev_err(component->dev, "Invalid frame offset %d\n", offset);
return -EINVAL;
}
- /* Calculate & validate frame size based on slot info provided. */
- frame_size = slots * slot_width;
- switch (frame_size) {
- case 32:
- dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_32;
- break;
- case 64:
- dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_64;
- break;
- case 128:
- dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_128;
- break;
- case 256:
- dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_256;
- break;
- default:
- dev_err(component->dev, "Invalid frame size %d\n", frame_size);
- return -EINVAL;
- }
+ /*
+* If we're master, calculate & validate frame size based on slot info
+* provided as we have a limited set of rates available.
+*/
+ if (da7219->master) {
+ frame_size = slots * slot_width;
+ switch (frame_size) {
+ case 32:
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_32;
+ break;
+ case 64:
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_64;
+ break;
+ case 128:
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_128;
+ break;
+ case 256:
+ dai_bclks_per_wclk = DA7219_DAI_BCLKS_PER_WCLK_256;
+ break;
+ default:
+ dev_err(component->dev, "Invalid frame size %d\n",
+ frame_size);
+ return -EINVAL;
+ }
- snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
- DA7219_DAI_BCLKS_PER_WCLK_MASK,
- dai_bclks_per_wclk);
+ snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
+ DA7219_DAI_BCLKS_PER_WCLK_MASK,
+ d
RE: [PATCH] usb: typec: tcpm: Correct the PPS out_volt calculation
On 30 January 2019 03:14, Kyle Tso wrote: > When Sink negotiates PPS, the voltage range of selected PPS APDO might not > cover the previous voltage (out_volt). If the previous out_volt is lower than > the > new min_volt, the output voltage in RDO might be set to an invalid value. For > instance, supposed that the previous voltage is 5V, and the new voltage range > in > the APDO is 7V-12V. Then the output voltage in the RDO should not be set to 5V > which is lower than the possible min_volt 7V. > > Fix this by choosing the maximal value between the previous voltage and the > new min_volt first. And ensure that this value will not exceed the new > max_volt. > The new out_volt will fall within the new voltage range while being the > closest > value compared to the previous out_volt. > > Signed-off-by: Kyle Tso I'd be interested to see how many Sources go off-piste like that with a more unusual PPS range, especially when the default fixed is 5V, and they're required to support one of the defined PPS ranges which will start at 3.3V. Not sure what this might gain you. However, spec says it's possible so: Reviewed-by: Adam Thomson > --- > drivers/usb/typec/tcpm/tcpm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index f1d3e54210df..8f2af348bda5 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -2297,7 +2297,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct > tcpm_port *port) > pdo_pps_apdo_max_voltage(snk)); > port->pps_data.max_curr = min_pps_apdo_current(src, snk); > port->pps_data.out_volt = min(port->pps_data.max_volt, > - port->pps_data.out_volt); > + max(port->pps_data.min_volt, > + port->pps_data.out_volt)); > port->pps_data.op_curr = min(port->pps_data.max_curr, >port->pps_data.op_curr); > } > -- > 2.20.1.495.gaa96b0ce6b-goog
RE: [PATCH 0/2] ASoC: da7219: Improvements to DAI clocks handling in driver
On 08 January 2019 09:13, Adam Thomson wrote: > This small patchset provides improvements to CCF based clock handling relating > to the DAI clocks. In the first patch MCLK is made parent of DAI clocks, if > MCLK > has been provided to the driver, which means that MCLK will automatically be > enabled as a prerequisite to DAI clocks. The second patch adds the ability to > read > the WCLK rate provided by the codec when DAI clocks are featured. This is more > useful than the returned rate simply being the rate of the parent clock, > which in > this case would be MCLK. Apologies for repeat submissions. Had some glitches with mail servers here. > Adam Thomson (2): > ASoC: da7219: MCLK should be enabled before DAI clocks > ASoC: da7219: Add recalc_rate function to return DAI clock rate > > sound/soc/codecs/da7219.c | 77 > --- > sound/soc/codecs/da7219.h | 1 + > 2 files changed, 67 insertions(+), 11 deletions(-) > > -- > 1.9.1
[PATCH 0/2] ASoC: da7219: Improvements to DAI clocks handling in driver
This small patchset provides improvements to CCF based clock handling relating to the DAI clocks. In the first patch MCLK is made parent of DAI clocks, if MCLK has been provided to the driver, which means that MCLK will automatically be enabled as a prerequisite to DAI clocks. The second patch adds the ability to read the WCLK rate provided by the codec when DAI clocks are featured. This is more useful than the returned rate simply being the rate of the parent clock, which in this case would be MCLK. Adam Thomson (2): ASoC: da7219: MCLK should be enabled before DAI clocks ASoC: da7219: Add recalc_rate function to return DAI clock rate sound/soc/codecs/da7219.c | 77 --- sound/soc/codecs/da7219.h | 1 + 2 files changed, 67 insertions(+), 11 deletions(-) -- 1.9.1
[PATCH 1/2] ASoC: da7219: MCLK should be enabled before DAI clocks
For platforms using the Common Clock Framework to control the
codec's DAI clocks, MCLK should be enabled prior to DAI clocks
being turned on. For some platforms the codec is already
provided with an MCLK reference and can therefore control MCLK
itself as it needs to.
To improve functionality MCLK is now added as a parent to the
DAI clocks, if MCLK was provided, so that if they are enabled MCLK
will automatically be enabled as a prerequisite by the CCF.
Signed-off-by: Adam Thomson
---
sound/soc/codecs/da7219.c | 32
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index e46e9f4..66dd4c2 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1804,7 +1804,7 @@ static int da7219_dai_clks_is_prepared(struct clk_hw *hw)
.is_prepared = da7219_dai_clks_is_prepared,
};
-static void da7219_register_dai_clks(struct snd_soc_component *component)
+static int da7219_register_dai_clks(struct snd_soc_component *component)
{
struct device *dev = component->dev;
struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
@@ -1812,9 +1812,17 @@ static void da7219_register_dai_clks(struct
snd_soc_component *component)
struct clk_init_data init = {};
struct clk *dai_clks;
struct clk_lookup *dai_clks_lookup;
+ const char *parent_name;
+
+ if (da7219->mclk) {
+ parent_name = __clk_get_name(da7219->mclk);
+ init.parent_names = _name;
+ init.num_parents = 1;
+ } else {
+ init.parent_names = NULL;
+ init.num_parents = 0;
+ }
- init.parent_names = NULL;
- init.num_parents = 0;
init.name = pdata->dai_clks_name;
init.ops = _dai_clks_ops;
da7219->dai_clks_hw.init =
@@ -1823,7 +1831,7 @@ static void da7219_register_dai_clks(struct
snd_soc_component *component)
if (IS_ERR(dai_clks)) {
dev_warn(dev, "Failed to register DAI clocks: %ld\n",
PTR_ERR(dai_clks));
- return;
+ return PTR_ERR(dai_clks);
}
da7219->dai_clks = dai_clks;
@@ -1835,13 +1843,18 @@ static void da7219_register_dai_clks(struct
snd_soc_component *component)
dai_clks_lookup = clkdev_create(dai_clks, pdata->dai_clks_name,
"%s", dev_name(dev));
if (!dai_clks_lookup)
- dev_warn(dev, "Failed to create DAI clkdev");
+ return -ENOMEM;
else
da7219->dai_clks_lookup = dai_clks_lookup;
}
+
+ return 0;
}
#else
-static inline void da7219_register_dai_clks(struct snd_soc_component
*component) {}
+static inline int da7219_register_dai_clks(struct snd_soc_component *component)
+{
+ return 0;
+}
#endif /* CONFIG_COMMON_CLK */
static void da7219_handle_pdata(struct snd_soc_component *component)
@@ -1854,8 +1867,6 @@ static void da7219_handle_pdata(struct snd_soc_component
*component)
da7219->wakeup_source = pdata->wakeup_source;
- da7219_register_dai_clks(component);
-
/* Mic Bias voltages */
switch (pdata->micbias_lvl) {
case DA7219_MICBIAS_1_6V:
@@ -1947,6 +1958,11 @@ static int da7219_probe(struct snd_soc_component
*component)
}
}
+ /* Register CCF DAI clock control */
+ ret = da7219_register_dai_clks(component);
+ if (ret)
+ return ret;
+
/* Default PC counter to free-running */
snd_soc_component_update_bits(component, DA7219_PC_COUNT,
DA7219_PC_FREERUN_MASK,
DA7219_PC_FREERUN_MASK);
--
1.9.1
[PATCH 2/2] ASoC: da7219: Add recalc_rate function to return DAI clock rate
By making MCLK parent of DAI clocks, when querying the rate of the
clock the rate returned is now given from the parent clock so
gives the MCLK rate rather than 0 as previously returned. This is
a bit misleading, and actually there's no major reason why we can't
at least return the DAI WCLK rate, as set in HW, so that's what we
now do.
Signed-off-by: Adam Thomson
---
sound/soc/codecs/da7219.c | 45 ++---
sound/soc/codecs/da7219.h | 1 +
2 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 66dd4c2..3a073ca 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1767,7 +1767,7 @@ static int da7219_dai_clks_prepare(struct clk_hw *hw)
{
struct da7219_priv *da7219 =
container_of(hw, struct da7219_priv, dai_clks_hw);
- struct snd_soc_component *component = da7219->aad->component;
+ struct snd_soc_component *component = da7219->component;
snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
DA7219_DAI_CLK_EN_MASK,
@@ -1780,7 +1780,7 @@ static void da7219_dai_clks_unprepare(struct clk_hw *hw)
{
struct da7219_priv *da7219 =
container_of(hw, struct da7219_priv, dai_clks_hw);
- struct snd_soc_component *component = da7219->aad->component;
+ struct snd_soc_component *component = da7219->component;
snd_soc_component_update_bits(component, DA7219_DAI_CLK_MODE,
DA7219_DAI_CLK_EN_MASK, 0);
@@ -1790,7 +1790,7 @@ static int da7219_dai_clks_is_prepared(struct clk_hw *hw)
{
struct da7219_priv *da7219 =
container_of(hw, struct da7219_priv, dai_clks_hw);
- struct snd_soc_component *component = da7219->aad->component;
+ struct snd_soc_component *component = da7219->component;
u8 clk_reg;
clk_reg = snd_soc_component_read32(component, DA7219_DAI_CLK_MODE);
@@ -1798,10 +1798,47 @@ static int da7219_dai_clks_is_prepared(struct clk_hw
*hw)
return !!(clk_reg & DA7219_DAI_CLK_EN_MASK);
}
+static unsigned long da7219_dai_clks_recalc_rate(struct clk_hw *hw,
+unsigned long parent_rate)
+{
+ struct da7219_priv *da7219 =
+ container_of(hw, struct da7219_priv, dai_clks_hw);
+ struct snd_soc_component *component = da7219->component;
+ u8 fs = snd_soc_component_read32(component, DA7219_SR);
+
+ switch (fs & DA7219_SR_MASK) {
+ case DA7219_SR_8000:
+ return 8000;
+ case DA7219_SR_11025:
+ return 11025;
+ case DA7219_SR_12000:
+ return 12000;
+ case DA7219_SR_16000:
+ return 16000;
+ case DA7219_SR_22050:
+ return 22050;
+ case DA7219_SR_24000:
+ return 24000;
+ case DA7219_SR_32000:
+ return 32000;
+ case DA7219_SR_44100:
+ return 44100;
+ case DA7219_SR_48000:
+ return 48000;
+ case DA7219_SR_88200:
+ return 88200;
+ case DA7219_SR_96000:
+ return 96000;
+ default:
+ return 0;
+ }
+}
+
static const struct clk_ops da7219_dai_clks_ops = {
.prepare = da7219_dai_clks_prepare,
.unprepare = da7219_dai_clks_unprepare,
.is_prepared = da7219_dai_clks_is_prepared,
+ .recalc_rate = da7219_dai_clks_recalc_rate,
};
static int da7219_register_dai_clks(struct snd_soc_component *component)
@@ -1825,6 +1862,7 @@ static int da7219_register_dai_clks(struct
snd_soc_component *component)
init.name = pdata->dai_clks_name;
init.ops = _dai_clks_ops;
+ init.flags = CLK_GET_RATE_NOCACHE;
da7219->dai_clks_hw.init =
dai_clks = devm_clk_register(dev, >dai_clks_hw);
@@ -1912,6 +1950,7 @@ static int da7219_probe(struct snd_soc_component
*component)
unsigned int rev;
int ret;
+ da7219->component = component;
mutex_init(>ctrl_lock);
mutex_init(>pll_lock);
diff --git a/sound/soc/codecs/da7219.h b/sound/soc/codecs/da7219.h
index 3a00686..366cf46 100644
--- a/sound/soc/codecs/da7219.h
+++ b/sound/soc/codecs/da7219.h
@@ -809,6 +809,7 @@ enum da7219_supplies {
/* Private data */
struct da7219_priv {
+ struct snd_soc_component *component;
struct da7219_aad_priv *aad;
struct da7219_pdata *pdata;
--
1.9.1
RE: [PATCH 18/21] ASoC: da7219: fix endianness issues
On 05 January 2019 02:03, Pierre-Louis Bossart wrote: > Reported by Sparse. > > da7219.c:440:44: warning: cast to restricted __le16 > da7219.c:461:13: warning: incorrect type in assignment (different base types) > da7219.c:461:13:expected unsigned short [unsigned] [usertype] val > da7219.c:461:13:got restricted __le16 [usertype] > da7219.c:1451:16: warning: incorrect type in assignment (different base types) > da7219.c:1451:16:expected unsigned short [unsigned] [usertype] offset > da7219.c:1451:16:got restricted __le16 [usertype] > > da7219-aad.c:150:37: warning: incorrect type in assignment (different base > types) > da7219-aad.c:150:37:expected unsigned short [unsigned] [usertype] > tonegen_freq_hptest > da7219-aad.c:150:37:got restricted __le16 [usertype] > da7219-aad.c:157:37: warning: incorrect type in assignment (different base > types) > da7219-aad.c:157:37:expected unsigned short [unsigned] [usertype] > tonegen_freq_hptest > da7219-aad.c:157:37:got restricted __le16 [usertype] > > Cc: Adam Thomson > Signed-off-by: Pierre-Louis Bossart Reviewed-by: Adam Thomson > --- > sound/soc/codecs/da7219-aad.c | 2 +- > sound/soc/codecs/da7219.c | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/sound/soc/codecs/da7219-aad.c b/sound/soc/codecs/da7219-aad.c > index 2c7d5088e6f2..e0964b20a389 100644 > --- a/sound/soc/codecs/da7219-aad.c > +++ b/sound/soc/codecs/da7219-aad.c > @@ -117,7 +117,7 @@ static void da7219_aad_hptest_work(struct work_struct > *work) > struct snd_soc_dapm_context *dapm = > snd_soc_component_get_dapm(component); > struct da7219_priv *da7219 = > snd_soc_component_get_drvdata(component); > > - u16 tonegen_freq_hptest; > + __le16 tonegen_freq_hptest; > u8 pll_srm_sts, pll_ctrl, gain_ramp_ctrl, accdet_cfg8; > int report = 0, ret = 0; > > diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index > e46e9f4bc994..ce165047b9f9 100644 > --- a/sound/soc/codecs/da7219.c > +++ b/sound/soc/codecs/da7219.c > @@ -423,7 +423,7 @@ static int da7219_tonegen_freq_get(struct snd_kcontrol > *kcontrol, > struct soc_mixer_control *mixer_ctrl = > (struct soc_mixer_control *) kcontrol->private_value; > unsigned int reg = mixer_ctrl->reg; > - u16 val; > + __le16 val; > int ret; > > mutex_lock(>ctrl_lock); > @@ -450,7 +450,7 @@ static int da7219_tonegen_freq_put(struct snd_kcontrol > *kcontrol, > struct soc_mixer_control *mixer_ctrl = > (struct soc_mixer_control *) kcontrol->private_value; > unsigned int reg = mixer_ctrl->reg; > - u16 val; > + __le16 val; > int ret; > > /* > @@ -1396,7 +1396,7 @@ static int da7219_set_dai_tdm_slot(struct snd_soc_dai > *dai, > struct snd_soc_component *component = dai->component; > struct da7219_priv *da7219 = > snd_soc_component_get_drvdata(component); > u8 dai_bclks_per_wclk; > - u16 offset; > + __le16 offset; > u32 frame_size; > > /* No channels enabled so disable TDM, revert to 64-bit frames */ > -- > 2.17.1
RE: [PATCH 19/21] ASoC: da7219: use logical AND
On 05 January 2019 02:03, Pierre-Louis Bossart wrote: > Reported by Sparse: > da7219.c:841:57: warning: dubious: x & !y > > Cc: Adam Thomson > Signed-off-by: Pierre-Louis Bossart Reviewed-by: Adam Thomson > --- > sound/soc/codecs/da7219.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index > ce165047b9f9..513ec0368653 100644 > --- a/sound/soc/codecs/da7219.c > +++ b/sound/soc/codecs/da7219.c > @@ -838,7 +838,7 @@ static int da7219_dai_event(struct snd_soc_dapm_widget > *w, > ++i; > msleep(50); > } > - } while ((i < DA7219_SRM_CHECK_RETRIES) & (!srm_lock)); > + } while ((i < DA7219_SRM_CHECK_RETRIES) && (!srm_lock)); > > if (!srm_lock) > dev_warn(component->dev, "SRM failed to lock\n"); > -- > 2.17.1
RE: [PATCH v3] usb: typec: tcpm: Extend the matching rules on PPS APDO selection
On 17 December 2018 13:22, Kyle Tso wrote:
> Current matching rules ensure that the voltage range of selected Source
> Capability is entirely within the range defined in one of the Sink
> Capabilities. This
> is reasonable but not practical because Sink may not support wide range of
> voltage when sinking power while Source could advertise its capabilities in
> relatively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V
> Prog of Fixed Nominal Voltage) will not be selected if the Sink requires 5V-
> 12V@3A PPS power. However, the Sink could work well if the requested voltage
> range in RDOs is 5V-11V@3A.
>
> Currently accepted:
> |- source -|
> |--- sink ---|
>
> Currently not accepted:
> |- source -|
> |--- sink ---|
>
> |- source -|
> |--- sink ---|
>
> |- source -|
> |-- sink ---|
>
> To improve the usability, change the matching rules to what listed
> below:
> a. The Source PDO is selectable if any portion of the voltage range
>overlaps one of the Sink PDO's voltage range.
> b. The maximum operational voltage will be the lower one between the
>selected Source PDO and the matching Sink PDO.
> c. The maximum power will be the maximum operational voltage times the
>maximum current defined in the selected Source PDO d. Select the Source PDO
> with the highest maximum power
>
> Signed-off-by: Kyle Tso
Acked-by: Adam Thomson
>
> ---
> Changelog since v1:
> - updated the commit message as suggested by Guenter Roeck us.net>
>
> Changelog since v2:
> - fixed the coding style problems as suggested by Heikki Krogerus
>
> - Corrected a wrong word in the commit message as suggested by Adam
> Thomson
>
> ---
> drivers/usb/typec/tcpm/tcpm.c | 35 ---
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 3620efee2688..4bc29b586698 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> unsigned int i, j, max_mw = 0, max_mv = 0;
> unsigned int min_src_mv, max_src_mv, src_ma, src_mw;
> unsigned int min_snk_mv, max_snk_mv;
> - u32 pdo;
> + unsigned int max_op_mv;
> + u32 pdo, src, snk;
> unsigned int src_pdo = 0, snk_pdo = 0;
>
> /*
> @@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> continue;
> }
>
> - if (max_src_mv <= max_snk_mv &&
> - min_src_mv >= min_snk_mv) {
> + if (min_src_mv <= max_snk_mv &&
> + max_src_mv >= min_snk_mv) {
> + max_op_mv = min(max_src_mv,
> max_snk_mv);
> + src_mw = (max_op_mv * src_ma) / 1000;
> /* Prefer higher voltages if available
> */
> if ((src_mw == max_mw &&
> - min_src_mv > max_mv) ||
> + max_op_mv > max_mv) ||
> src_mw > max_mw) {
> src_pdo = i;
> snk_pdo = j;
> max_mw = src_mw;
> - max_mv = max_src_mv;
> + max_mv = max_op_mv;
> }
> }
> }
> @@ -2285,16 +2288,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> }
>
> if (src_pdo) {
> - pdo = port->source_caps[src_pdo];
> -
> - port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo);
> - port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo);
> - port->pps_data.max_curr =
> - min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]);
> - port->pps_data.out_volt =
> - min(pdo_pps_apdo_max_voltage(pdo), port-
> >pps_data.out_volt);
> - port->pps_data.op
RE: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection
On 17 December 2018 12:45, Kyle Tso wrote:
> On Mon, Dec 17, 2018 at 8:23 PM Adam Thomson
> wrote:
> >
> > On 17 December 2018 02:48, Kyle Tso wrote:
> >
> > > Current matching rules ensure that the voltage range of selected
> > > Source Capability is entirely within the range defined in one of the
> > > Sink Capabilities. This is reasonable but not practical because Sink
> > > may not support wide range of voltage when sinking power while
> > > Source could advertise its capabilities in raletively wider range.
> > > For example, a Source PDO advertising 3.3V-11V@3A (9V
> >
> > relatively
> >
>
> noted!
> Thanks for the correction. I will fix this in the next patch.
>
> > > Prog of Fixed Nominal Voltage) will not be selected if the Sink
> > > requires 5V- 12V@3A PPS power. However, the Sink could work well if
> > > the requested voltage range in RDOs is 5V-11V@3A.
> > >
> > > Currently accepted:
> > > |- source -|
> > > |--- sink ---|
> > >
> > > Currently not accepted:
> > > |- source -|
> > > |--- sink ---|
> > >
> > > |- source -|
> > > |--- sink ---|
> > >
> > > |- source -|
> > > |-- sink ---|
> > >
> > > To improve the usability, change the matching rules to what listed
> > > below:
> > > a. The Source PDO is selectable if any portion of the voltage range
> > >overlaps one of the Sink PDO's voltage range.
> > > b. The maximum operational voltage will be the lower one between the
> > >selected Source PDO and the matching Sink PDO.
> > > c. The maximum power will be the maximum operational voltage times the
> > >maximum current defined in the selected Source PDO d. Select the
> > > Source PDO with the highest maximum power
> > >
> > > Signed-off-by: Kyle Tso
> > >
> > > ---
> > > Changelog since v1:
> > > - updated the commit message as suggested by Guenter Roeck
> > >
> > > ---
> > > drivers/usb/typec/tcpm/tcpm.c | 29 +
> > > 1 file changed, 17 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpm.c
> > > b/drivers/usb/typec/tcpm/tcpm.c index 3620efee2688..3001df7bd602
> > > 100644
> > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > @@ -2213,7 +2213,8 @@ static unsigned int
> > > tcpm_pd_select_pps_apdo(struct tcpm_port *port)
> > > unsigned int i, j, max_mw = 0, max_mv = 0;
> > > unsigned int min_src_mv, max_src_mv, src_ma, src_mw;
> > > unsigned int min_snk_mv, max_snk_mv;
> > > - u32 pdo;
> > > + unsigned int max_op_mv;
> > > + u32 pdo, src, snk;
> > > unsigned int src_pdo = 0, snk_pdo = 0;
> > >
> > > /*
> > > @@ -2263,16 +2264,18 @@ static unsigned int
> > > tcpm_pd_select_pps_apdo(struct tcpm_port *port)
> > > continue;
> > > }
> > >
> > > - if (max_src_mv <= max_snk_mv &&
> > > - min_src_mv >= min_snk_mv) {
> > > + if (min_src_mv <= max_snk_mv &&
> > > + max_src_mv >= min_snk_mv) {
> > > + max_op_mv = min(max_src_mv,
> > > max_snk_mv);
> > > + src_mw = (max_op_mv * src_ma)
> > > + / 1000;
> > > /* Prefer higher voltages if
> > > available */
> > > if ((src_mw == max_mw &&
> > > - min_src_mv > max_mv) ||
> > > + max_op_mv > max_mv) ||
> > > src_mw > max_mw) {
> >
> > Sorry I didn't raise this before, but came to mind this morning when I
> > was considering your updates again. What happens if the Source validly
> > provides two PPS APDOs, for example:
> >
> > 3.3V - 11V, 3A (9V programmable)
> > 3.3V - 16V, 3A (1
RE: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection
On 17 December 2018 02:48, Kyle Tso wrote:
> Current matching rules ensure that the voltage range of selected Source
> Capability is entirely within the range defined in one of the Sink
> Capabilities. This
> is reasonable but not practical because Sink may not support wide range of
> voltage when sinking power while Source could advertise its capabilities in
> raletively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V
relatively
> Prog of Fixed Nominal Voltage) will not be selected if the Sink requires 5V-
> 12V@3A PPS power. However, the Sink could work well if the requested voltage
> range in RDOs is 5V-11V@3A.
>
> Currently accepted:
> |- source -|
> |--- sink ---|
>
> Currently not accepted:
> |- source -|
> |--- sink ---|
>
> |- source -|
> |--- sink ---|
>
> |- source -|
> |-- sink ---|
>
> To improve the usability, change the matching rules to what listed
> below:
> a. The Source PDO is selectable if any portion of the voltage range
>overlaps one of the Sink PDO's voltage range.
> b. The maximum operational voltage will be the lower one between the
>selected Source PDO and the matching Sink PDO.
> c. The maximum power will be the maximum operational voltage times the
>maximum current defined in the selected Source PDO d. Select the Source PDO
> with the highest maximum power
>
> Signed-off-by: Kyle Tso
>
> ---
> Changelog since v1:
> - updated the commit message as suggested by Guenter Roeck us.net>
> ---
> drivers/usb/typec/tcpm/tcpm.c | 29 +
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 3620efee2688..3001df7bd602 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> unsigned int i, j, max_mw = 0, max_mv = 0;
> unsigned int min_src_mv, max_src_mv, src_ma, src_mw;
> unsigned int min_snk_mv, max_snk_mv;
> - u32 pdo;
> + unsigned int max_op_mv;
> + u32 pdo, src, snk;
> unsigned int src_pdo = 0, snk_pdo = 0;
>
> /*
> @@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> continue;
> }
>
> - if (max_src_mv <= max_snk_mv &&
> - min_src_mv >= min_snk_mv) {
> + if (min_src_mv <= max_snk_mv &&
> + max_src_mv >= min_snk_mv) {
> + max_op_mv = min(max_src_mv,
> max_snk_mv);
> + src_mw = (max_op_mv * src_ma) / 1000;
> /* Prefer higher voltages if available
> */
> if ((src_mw == max_mw &&
> - min_src_mv > max_mv) ||
> + max_op_mv > max_mv) ||
> src_mw > max_mw) {
Sorry I didn't raise this before, but came to mind this morning when I was
considering your updates again. What happens if the Source validly provides two
PPS APDOs, for example:
3.3V - 11V, 3A (9V programmable)
3.3V - 16V, 3A (15V programmable)
and the sink APDO is:
5V - 9V, 3A
I think the code here will now select the higher range (15V programmable) as it
gives a larger power output value, even if the sink can only support a voltage
that's far smaller. I really don't think this is correct. *If* you are going to
allow selection of PPS APDOs that provide a larger voltage range than the Sink
can actually cope with, then I think you should at least select the lower of
those advertised which fulfils the needs of the Sink.
> src_pdo = i;
> snk_pdo = j;
> max_mw = src_mw;
> - max_mv = max_src_mv;
> + max_mv = max_op_mv;
> }
> }
> }
> @@ -2285,14 +2288,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> }
>
> if (src_pdo) {
> - pdo = port->source_caps[src_pdo];
> -
> - port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo);
> - port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo);
> - port->pps_data.max_curr =
> - min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]);
> +
RE: [PATCH v3 1/2] power: supply: add input voltage limit property
On 13 December 2018 12:10, Enric Balletbo i Serra wrote:
> This is part of the Pixel C's thermal management strategy to effectively
> limit the
> input power to 5V 3A when the screen is on. When the screen is on, the
> display,
> the CPU, and the GPU all contribute more heat to the system than while the
> screen is off, and we made a tradeoff to throttle the charger in order to give
> more of the thermal budget to those other components.
>
> So there's nothing fundamentally broken about the hardware that would cause
> the Pixel C to malfunction if we were charging at 9V or 12V instead of 5V when
> the screen is on, ie if userspace doesn't change this.
>
> What would happen is that you wouldn't meet Google's skin temperature targets
> on the system if the charger was allowed to run at 9V or 12V with the screen
> on.
>
> For folks hacking on Pixel Cs (which is now outside of Google's official
> support
> window for Android) and customizing their own kernel and userspace this would
> be acceptable, but we wanted to expose this feature in the power supply
> properties because the feature does exist in the Emedded Controller firmware
> of
> the Pixel C and all of Google's Chromebooks with USB-C made since 2015 in case
> someone running an up to date kernel wanted to limit the charging power for
> thermal or other reasons.
>
> This patch exposes a new property, similar to input current limit, to
> re-configure
> the maximum voltage from the external supply at runtime based on system-level
> knowledge or user input.
>
> Signed-off-by: Enric Balletbo i Serra
> Reviewed-by: Guenter Roeck
Acked-by: Adam Thomson
> ---
>
> Changes in v3:
> - Improve commit log and documentation with Benson comments.
>
> Changes in v2:
> - Document the new property in ABI/testing/sysfs-class-power.
> - Add the Reviewed-by Guenter Roeck tag.
>
> Documentation/ABI/testing/sysfs-class-power | 15 +++
> Documentation/power/power_supply_class.txt | 2 ++
> drivers/power/supply/power_supply_sysfs.c | 1 +
> include/linux/power_supply.h| 1 +
> 4 files changed, 19 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-power
> b/Documentation/ABI/testing/sysfs-class-power
> index 5e23e22dce1b..6dee5c105a28 100644
> --- a/Documentation/ABI/testing/sysfs-class-power
> +++ b/Documentation/ABI/testing/sysfs-class-power
> @@ -335,6 +335,21 @@ Description:
> Access: Read, Write
> Valid values: Represented in microamps
>
> +What:
> /sys/class/power_supply//input_voltage_limit
> +Date:Nov 2018
> +Contact: linux...@vger.kernel.org
> +Description:
> + This entry configures the incoming VBUS voltage limit currently
> + set in the supply. Normally this is configured based on
> + system-level knowledge or user input (e.g. This is part of the
> + Pixel C's thermal management strategy to effectively limit the
> + input power to 5V when the screen is on to meet Google's skin
> + temperature targets). Note that this feature should not be
> + used for safety critical things.
> +
> + Access: Read, Write
> + Valid values: Represented in microvolts
> +
> What:/sys/class/power_supply//online,
> Date:May 2007
> Contact: linux...@vger.kernel.org
> diff --git a/Documentation/power/power_supply_class.txt
> b/Documentation/power/power_supply_class.txt
> index 300d37896e51..7b4be615b4f8 100644
> --- a/Documentation/power/power_supply_class.txt
> +++ b/Documentation/power/power_supply_class.txt
> @@ -137,6 +137,8 @@ power supply object.
>
> INPUT_CURRENT_LIMIT - input current limit programmed by charger. Indicates
> the current drawn from a charging source.
> +INPUT_VOLTAGE_LIMIT - input voltage limit programmed by charger.
> +Indicates the voltage limit from a charging source.
>
> CHARGE_CONTROL_LIMIT - current charge control limit setting
> CHARGE_CONTROL_LIMIT_MAX - maximum charge control limit setting diff --git
> a/drivers/power/supply/power_supply_sysfs.c
> b/drivers/power/supply/power_supply_sysfs.c
> index dce24f596160..5848742ebb59 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -275,6 +275,7 @@ static struct device_attribute power_supply_attrs[] = {
> POWER_SUPPLY_ATTR(charge_control_limit),
> POWER_SUPPLY_ATTR(charge_control_limit_max),
> POWER_SUPPLY_ATTR(input_current_limit),
> + POWER_SUPPLY_ATTR(input_voltage_limit),
> POWER_SUPPLY_ATTR(energy_full_design),
> POWER_SUPPLY_
RE: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection
On 12 December 2018 02:47, Kyle Tso wrote: > On Mon, Dec 10, 2018 at 7:36 PM Adam Thomson > wrote: > > > > On 10 December 2018 09:01, Adam Thomson wrote: > > > > > On 06 December 2018 03:02, Kyle Tso wrote: > > > > > > > Current matching rules ensure that the voltage range of selected > > > > Source Capability is entirely within the range defined in one of > > > > the Sink Capabilities. This is reasonable but not practical > > > > because Sink may not support wide range of voltage when sinking > > > > power while Source could advertise its capabilities in raletively > > > > wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V > > > > Prog of Fixed Nominal Voltage) will not be selected if the Sink > > > > requires 5V- 12V@3A PPS power. However, the Sink could work well > > > > if the requested voltage range in > > > RDOs is 5V-11V@3A. > > > > > > Is there a real world example of a sink requiring the 5V - 12V > > > range? In that scenario could we not add an additional sink > > > capability which allows for this range to be supported, and the current > implementation should work just fine? > > > > Ok, I maybe should have waited until after my morning coffee to > > respond. So because the lower limit on the sink side, is higher than > > the advertised source's PPS minimum voltage it never gets selected? > > Personally I'd prefer to keep the upper limit checking as is as I > > think that's an additional safety benefit helping to prevent > > over-voltage scenarios. I think if a PPS APDO can supply up to 11V > > then the system should be capable of handling that voltage, otherwise > > it shouldn't be considered at all. The Source provides limits checking > > as well to make sure the Sink doesn't request a value above the maximum > voltage limit for that selected APDO. > > > > If the over-voltage occurs, it means: > 1. the adapter malfunctioned. or > 2. the code on the Sink accidentally requests a voltage level which is over > the limit > of the Sink. > > For 1., it is difficult to predict the behaviors of a malfunctioned adapter. > The over- > voltage event may happen even if the Sink doesn't select the APDO from this > broken adapter. Yes, I agree it's almost impossible to do anything from software to mitigate this which is why the HW design has to have protection for this. > For 2., it is difficult to predict the behaviors from the careless code as > well. Yes, that's also true, but if it's coded with the intention not to select an option that's potentially higher than the system can handle then we're less likely to fall foul of over-voltage scenarios in my opinion. By selecting a PPS APDO with an upper threshold which falls within the board limits, assuming the code were to accidentally request something higher than the PPS APDO maximum then the Source should reject this. Just feels a little safer as we're talking about controlling an external power source. At the end of the day though the decision lies with the maintainers on this. > > For the lower limit I'm more inclined to agree with allowing a higher > > minimum on the sink side as that's less of a safety/damage issue as I > understand it. > > FWIW, what is the real world scenario? What happens if voltage drops below > 5V? > > > > Some products (in Sink mode) have under-voltage protection (the lower bound > might be around 3.8V - 4V before the calculation of IR-drop) that will cause > the > disconnection. Ok, so the system would just stop charging, correct? I guess the calling code to control the Source/adapter, via TCPM, wouldn't select a value below 4V in that scenario anyway? > > thanks, > Kyle
RE: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection
On 10 December 2018 09:01, Adam Thomson wrote:
> On 06 December 2018 03:02, Kyle Tso wrote:
>
> > Current matching rules ensure that the voltage range of selected
> > Source Capability is entirely within the range defined in one of the
> > Sink Capabilities. This is reasonable but not practical because Sink
> > may not support wide range of voltage when sinking power while Source
> > could advertise its capabilities in raletively wider range. For
> > example, a Source PDO advertising 3.3V-11V@3A (9V Prog of Fixed
> > Nominal Voltage) will not be selected if the Sink requires 5V- 12V@3A
> > PPS power. However, the Sink could work well if the requested voltage range
> > in
> RDOs is 5V-11V@3A.
>
> Is there a real world example of a sink requiring the 5V - 12V range? In that
> scenario could we not add an additional sink capability which allows for this
> range
> to be supported, and the current implementation should work just fine?
Ok, I maybe should have waited until after my morning coffee to respond. So
because the lower limit on the sink side, is higher than the advertised source's
PPS minimum voltage it never gets selected? Personally I'd prefer to keep the
upper limit checking as is as I think that's an additional safety benefit
helping to prevent over-voltage scenarios. I think if a PPS APDO can supply up
to 11V then the system should be capable of handling that voltage, otherwise
it shouldn't be considered at all. The Source provides limits checking as well
to make sure the Sink doesn't request a value above the maximum voltage limit
for that selected APDO.
For the lower limit I'm more inclined to agree with allowing a higher minimum
on the sink side as that's less of a safety/damage issue as I understand it.
FWIW, what is the real world scenario? What happens if voltage drops below 5V?
>
> >
> > To improve the usability, change the matching rules to what listed
> > below:
> > a. The Source PDO is selectable if any portion of the voltage range
> >overlaps one of the Sink PDO's voltage range.
> > b. The maximum operational voltage will be the lower one between the
> >selected Source PDO and the matching Sink PDO.
> > c. The maximum power will be the maximum operational voltage times the
> >maximum current defined in the selected Source PDO d. Select the
> > Source PDO with the highest maximum power
> >
> > Signed-off-by: Kyle Tso
> > ---
> > drivers/usb/typec/tcpm/tcpm.c | 29 +
> > 1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c
> > b/drivers/usb/typec/tcpm/tcpm.c index 3620efee2688..3001df7bd602
> > 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -2213,7 +2213,8 @@ static unsigned int
> > tcpm_pd_select_pps_apdo(struct tcpm_port *port)
> > unsigned int i, j, max_mw = 0, max_mv = 0;
> > unsigned int min_src_mv, max_src_mv, src_ma, src_mw;
> > unsigned int min_snk_mv, max_snk_mv;
> > - u32 pdo;
> > + unsigned int max_op_mv;
> > + u32 pdo, src, snk;
> > unsigned int src_pdo = 0, snk_pdo = 0;
> >
> > /*
> > @@ -2263,16 +2264,18 @@ static unsigned int
> > tcpm_pd_select_pps_apdo(struct tcpm_port *port)
> > continue;
> > }
> >
> > - if (max_src_mv <= max_snk_mv &&
> > - min_src_mv >= min_snk_mv) {
> > + if (min_src_mv <= max_snk_mv &&
> > + max_src_mv >= min_snk_mv) {
> > + max_op_mv = min(max_src_mv,
> > max_snk_mv);
> > + src_mw = (max_op_mv * src_ma) / 1000;
> > /* Prefer higher voltages if available
> > */
> > if ((src_mw == max_mw &&
> > -min_src_mv > max_mv) ||
> > +max_op_mv > max_mv) ||
> > src_mw > max_mw) {
> > src_pdo = i;
> > snk_pdo = j;
> > max_mw = src_mw;
> > - max_mv = max_src_mv;
> > + max_mv = max_op_mv;
> > }
> > }
> > }
&
RE: [PATCH] usb: typec: tcpm: Extend the matching rules on PPS APDO selection
On 06 December 2018 03:02, Kyle Tso wrote:
> Current matching rules ensure that the voltage range of selected Source
> Capability is entirely within the range defined in one of the Sink
> Capabilities. This
> is reasonable but not practical because Sink may not support wide range of
> voltage when sinking power while Source could advertise its capabilities in
> raletively wider range. For example, a Source PDO advertising 3.3V-11V@3A (9V
> Prog of Fixed Nominal Voltage) will not be selected if the Sink requires 5V-
> 12V@3A PPS power. However, the Sink could work well if the requested voltage
> range in RDOs is 5V-11V@3A.
Is there a real world example of a sink requiring the 5V - 12V range? In that
scenario could we not add an additional sink capability which allows for this
range to be supported, and the current implementation should work just fine?
>
> To improve the usability, change the matching rules to what listed
> below:
> a. The Source PDO is selectable if any portion of the voltage range
>overlaps one of the Sink PDO's voltage range.
> b. The maximum operational voltage will be the lower one between the
>selected Source PDO and the matching Sink PDO.
> c. The maximum power will be the maximum operational voltage times the
>maximum current defined in the selected Source PDO d. Select the Source PDO
> with the highest maximum power
>
> Signed-off-by: Kyle Tso
> ---
> drivers/usb/typec/tcpm/tcpm.c | 29 +
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 3620efee2688..3001df7bd602 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2213,7 +2213,8 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> unsigned int i, j, max_mw = 0, max_mv = 0;
> unsigned int min_src_mv, max_src_mv, src_ma, src_mw;
> unsigned int min_snk_mv, max_snk_mv;
> - u32 pdo;
> + unsigned int max_op_mv;
> + u32 pdo, src, snk;
> unsigned int src_pdo = 0, snk_pdo = 0;
>
> /*
> @@ -2263,16 +2264,18 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> continue;
> }
>
> - if (max_src_mv <= max_snk_mv &&
> - min_src_mv >= min_snk_mv) {
> + if (min_src_mv <= max_snk_mv &&
> + max_src_mv >= min_snk_mv) {
> + max_op_mv = min(max_src_mv,
> max_snk_mv);
> + src_mw = (max_op_mv * src_ma) / 1000;
> /* Prefer higher voltages if available
> */
> if ((src_mw == max_mw &&
> - min_src_mv > max_mv) ||
> + max_op_mv > max_mv) ||
> src_mw > max_mw) {
> src_pdo = i;
> snk_pdo = j;
> max_mw = src_mw;
> - max_mv = max_src_mv;
> + max_mv = max_op_mv;
> }
> }
> }
> @@ -2285,14 +2288,16 @@ static unsigned int tcpm_pd_select_pps_apdo(struct
> tcpm_port *port)
> }
>
> if (src_pdo) {
> - pdo = port->source_caps[src_pdo];
> -
> - port->pps_data.min_volt = pdo_pps_apdo_min_voltage(pdo);
> - port->pps_data.max_volt = pdo_pps_apdo_max_voltage(pdo);
> - port->pps_data.max_curr =
> - min_pps_apdo_current(pdo, port->snk_pdo[snk_pdo]);
> + src = port->source_caps[src_pdo];
> + snk = port->snk_pdo[snk_pdo];
> +
> + port->pps_data.min_volt =
> max(pdo_pps_apdo_min_voltage(src),
> + pdo_pps_apdo_min_voltage(snk));
> + port->pps_data.max_volt =
> min(pdo_pps_apdo_max_voltage(src),
> + pdo_pps_apdo_max_voltage(snk));
> + port->pps_data.max_curr = min_pps_apdo_current(src, snk);
> port->pps_data.out_volt =
> - min(pdo_pps_apdo_max_voltage(pdo), port-
> >pps_data.out_volt);
> + min(port->pps_data.max_volt, port-
> >pps_data.out_volt);
> port->pps_data.op_curr =
> min(port->pps_data.max_curr, port->pps_data.op_curr);
> }
> --
> 2.20.0.rc2.403.gdbc3b29805-goog
RE: [PATCH] regulator: Use of_node_name_eq for node name comparisons
On 05 December 2018 19:51, Rob Herring wrote:
> Convert string compares of DT node names to use of_node_name_eq helper
> instead. This removes direct access to the node name pointer.
>
> For instances using of_node_cmp, this has the side effect of now using case
> sensitive comparisons. This should not matter for any FDT based system which
> all
> of these are.
>
> Cc: Liam Girdwood
> Cc: Mark Brown
> Cc: Support Opensource
> Cc: Sangbeom Kim
> Cc: Krzysztof Kozlowski
> Cc: Bartlomiej Zolnierkiewicz
> Cc: linux-samsung-...@vger.kernel.org
> Signed-off-by: Rob Herring
For da9052 regulator driver:
Acked-by: Adam Thomson
> ---
> drivers/regulator/88pm8607.c | 2 +-
> drivers/regulator/da9052-regulator.c | 2 +-
> drivers/regulator/max8997-regulator.c | 2 +-
> drivers/regulator/mc13xxx-regulator-core.c | 2 +-
> drivers/regulator/qcom-rpmh-regulator.c| 2 +-
> drivers/regulator/s5m8767.c| 2 +-
> 6 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/regulator/88pm8607.c b/drivers/regulator/88pm8607.c index
> fd86446e499b..28f55248eb90 100644
> --- a/drivers/regulator/88pm8607.c
> +++ b/drivers/regulator/88pm8607.c
> @@ -328,7 +328,7 @@ static int pm8607_regulator_dt_init(struct
> platform_device *pdev,
> return -ENODEV;
> }
> for_each_child_of_node(nproot, np) {
> - if (!of_node_cmp(np->name, info->desc.name)) {
> + if (of_node_name_eq(np, info->desc.name)) {
> config->init_data =
> of_get_regulator_init_data(>dev, np,
> >desc);
> diff --git a/drivers/regulator/da9052-regulator.c b/drivers/regulator/da9052-
> regulator.c
> index 37e4025203e3..207cb3859dcc 100644
> --- a/drivers/regulator/da9052-regulator.c
> +++ b/drivers/regulator/da9052-regulator.c
> @@ -435,7 +435,7 @@ static int da9052_regulator_probe(struct platform_device
> *pdev)
> return -ENODEV;
>
> for_each_child_of_node(nproot, np) {
> - if (!of_node_cmp(np->name,
> + if (of_node_name_eq(np,
>regulator->info->reg_desc.name)) {
> config.init_data = of_get_regulator_init_data(
> >dev, np,
> diff --git a/drivers/regulator/max8997-regulator.c
> b/drivers/regulator/max8997-
> regulator.c
> index 3bf5ddfaaea8..4d2487279a0a 100644
> --- a/drivers/regulator/max8997-regulator.c
> +++ b/drivers/regulator/max8997-regulator.c
> @@ -925,7 +925,7 @@ static int max8997_pmic_dt_parse_pdata(struct
> platform_device *pdev,
> pdata->regulators = rdata;
> for_each_child_of_node(regulators_np, reg_np) {
> for (i = 0; i < ARRAY_SIZE(regulators); i++)
> - if (!of_node_cmp(reg_np->name, regulators[i].name))
> + if (of_node_name_eq(reg_np, regulators[i].name))
> break;
>
> if (i == ARRAY_SIZE(regulators)) {
> diff --git a/drivers/regulator/mc13xxx-regulator-core.c
> b/drivers/regulator/mc13xxx-regulator-core.c
> index 65eb1e0350cf..2243138d8a58 100644
> --- a/drivers/regulator/mc13xxx-regulator-core.c
> +++ b/drivers/regulator/mc13xxx-regulator-core.c
> @@ -186,7 +186,7 @@ struct mc13xxx_regulator_init_data
> *mc13xxx_parse_regulators_dt(
> for (i = 0; i < num_regulators; i++) {
> if (!regulators[i].desc.name)
> continue;
> - if (!of_node_cmp(child->name,
> + if (of_node_name_eq(child,
>regulators[i].desc.name)) {
> p->id = i;
> p->init_data = of_get_regulator_init_data( diff
> --
> git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-
> regulator.c
> index 39ccf53fdeb3..b2c2d01d1637 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -410,7 +410,7 @@ static int rpmh_regulator_init_vreg(struct rpmh_vreg
> *vreg, struct device *dev,
> vreg->dev = dev;
>
> for (rpmh_data = pmic_rpmh_data; rpmh_data->name; rpmh_data++)
> - if (!strcmp(rpmh_data->name, node->name))
> + if (of_node_name_eq(node, rpmh_data->name))
> break;
>
> if (!rpmh_data->name) {
> diff --git a/drivers/regulator/s5m8767.c b/driv
RE: [PATCH] regulator: Use of_node_name_eq for node name comparisons
On 05 December 2018 19:51, Rob Herring wrote:
> Convert string compares of DT node names to use of_node_name_eq helper
> instead. This removes direct access to the node name pointer.
>
> For instances using of_node_cmp, this has the side effect of now using case
> sensitive comparisons. This should not matter for any FDT based system which
> all
> of these are.
>
> Cc: Liam Girdwood
> Cc: Mark Brown
> Cc: Support Opensource
> Cc: Sangbeom Kim
> Cc: Krzysztof Kozlowski
> Cc: Bartlomiej Zolnierkiewicz
> Cc: linux-samsung-...@vger.kernel.org
> Signed-off-by: Rob Herring
For da9052 regulator driver:
Acked-by: Adam Thomson
> ---
> drivers/regulator/88pm8607.c | 2 +-
> drivers/regulator/da9052-regulator.c | 2 +-
> drivers/regulator/max8997-regulator.c | 2 +-
> drivers/regulator/mc13xxx-regulator-core.c | 2 +-
> drivers/regulator/qcom-rpmh-regulator.c| 2 +-
> drivers/regulator/s5m8767.c| 2 +-
> 6 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/regulator/88pm8607.c b/drivers/regulator/88pm8607.c index
> fd86446e499b..28f55248eb90 100644
> --- a/drivers/regulator/88pm8607.c
> +++ b/drivers/regulator/88pm8607.c
> @@ -328,7 +328,7 @@ static int pm8607_regulator_dt_init(struct
> platform_device *pdev,
> return -ENODEV;
> }
> for_each_child_of_node(nproot, np) {
> - if (!of_node_cmp(np->name, info->desc.name)) {
> + if (of_node_name_eq(np, info->desc.name)) {
> config->init_data =
> of_get_regulator_init_data(>dev, np,
> >desc);
> diff --git a/drivers/regulator/da9052-regulator.c b/drivers/regulator/da9052-
> regulator.c
> index 37e4025203e3..207cb3859dcc 100644
> --- a/drivers/regulator/da9052-regulator.c
> +++ b/drivers/regulator/da9052-regulator.c
> @@ -435,7 +435,7 @@ static int da9052_regulator_probe(struct platform_device
> *pdev)
> return -ENODEV;
>
> for_each_child_of_node(nproot, np) {
> - if (!of_node_cmp(np->name,
> + if (of_node_name_eq(np,
>regulator->info->reg_desc.name)) {
> config.init_data = of_get_regulator_init_data(
> >dev, np,
> diff --git a/drivers/regulator/max8997-regulator.c
> b/drivers/regulator/max8997-
> regulator.c
> index 3bf5ddfaaea8..4d2487279a0a 100644
> --- a/drivers/regulator/max8997-regulator.c
> +++ b/drivers/regulator/max8997-regulator.c
> @@ -925,7 +925,7 @@ static int max8997_pmic_dt_parse_pdata(struct
> platform_device *pdev,
> pdata->regulators = rdata;
> for_each_child_of_node(regulators_np, reg_np) {
> for (i = 0; i < ARRAY_SIZE(regulators); i++)
> - if (!of_node_cmp(reg_np->name, regulators[i].name))
> + if (of_node_name_eq(reg_np, regulators[i].name))
> break;
>
> if (i == ARRAY_SIZE(regulators)) {
> diff --git a/drivers/regulator/mc13xxx-regulator-core.c
> b/drivers/regulator/mc13xxx-regulator-core.c
> index 65eb1e0350cf..2243138d8a58 100644
> --- a/drivers/regulator/mc13xxx-regulator-core.c
> +++ b/drivers/regulator/mc13xxx-regulator-core.c
> @@ -186,7 +186,7 @@ struct mc13xxx_regulator_init_data
> *mc13xxx_parse_regulators_dt(
> for (i = 0; i < num_regulators; i++) {
> if (!regulators[i].desc.name)
> continue;
> - if (!of_node_cmp(child->name,
> + if (of_node_name_eq(child,
>regulators[i].desc.name)) {
> p->id = i;
> p->init_data = of_get_regulator_init_data( diff
> --
> git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-
> regulator.c
> index 39ccf53fdeb3..b2c2d01d1637 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -410,7 +410,7 @@ static int rpmh_regulator_init_vreg(struct rpmh_vreg
> *vreg, struct device *dev,
> vreg->dev = dev;
>
> for (rpmh_data = pmic_rpmh_data; rpmh_data->name; rpmh_data++)
> - if (!strcmp(rpmh_data->name, node->name))
> + if (of_node_name_eq(node, rpmh_data->name))
> break;
>
> if (!rpmh_data->name) {
> diff --git a/drivers/regulator/s5m8767.c b/driv
RE: [PATCH 2/2] ASoC: DA7219: Implement error check on reg read and write
On 05 December 2018 04:27, Akshu Agrawal wrote:
> On 12/5/2018 2:46 AM, Adam Thomson wrote:
> > On 04 December 2018 18:36, Akshu Agrawal wrote:
> >
> >> Failed i2c transaction can lead to failure in reg read or write.
> >> Need to have error check for each read write operation.
> >>
> >
> > I'm not really clear what this gains you. If I2C is broken this won't
> > solve anything, and you have far deeper problems. Seems to me like a
> > lot of code which adds almost nothing.
> >
>
> I agree Adam that if I2C is broken, system is in bad state. But so is the
> case with
> most of the error situations. Error handling is anyway needed.
> We have seen case where read32 returned -1, unsigned int cast and the write
> back of the error value ends up in updating the register with 0xff.
> Need to avoid scenarios like this.
If the previous I2C access failed, how can we be sure that the write back to HW
of 0xFF even succeeds? More importantly these error returns won't necessarily
stop subsequent calls to controls within the Codec I believe, so you could still
see unwanted writes to HW via I2C, if I2C is sporadically operational. Again I
don't see this update resolving that. The key thing is to resolve why even just
one I2C transaction fails.
I'd like to hear input from others on this as if this was something
fundamentally required, all other I2C based codecs would be doing this and
that's just not the case right now.
One last thing, if a change like this is somehow deemed to be required, you will
need to update the AAD part of the driver as well.
>
> Thanks,
> Akshu
> >> Signed-off-by: Akshu Agrawal
> >> ---
> >> sound/soc/codecs/da7219.c | 323
> +++-
> >> --
> >> sound/soc/codecs/da7219.h | 2 +-
> >> 2 files changed, 247 insertions(+), 78 deletions(-)
> >>
> >> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> >> index
> >> e46e9f4..6757129 100644
> >> --- a/sound/soc/codecs/da7219.c
> >> +++ b/sound/soc/codecs/da7219.c
> >> @@ -312,69 +312,103 @@ static int da7219_enum_locked_put(struct
> >> snd_kcontrol *kcontrol, }
> >>
> >> /* ALC */
> >> -static void da7219_alc_calib(struct snd_soc_component *component)
> >> +static int da7219_alc_calib(struct snd_soc_component *component)
> >> {
> >> - u8 mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
> >> + int mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
> >> + int ret;
> >>
> >>/* Save current state of mic control register */
> >>mic_ctrl = snd_soc_component_read32(component,
> >> DA7219_MIC_1_CTRL);
> >> + if (mic_ctrl < 0)
> >> + return mic_ctrl;
> >>
> >>/* Save current state of input mixer control register */
> >>mixin_ctrl = snd_soc_component_read32(component,
> >> DA7219_MIXIN_L_CTRL);
> >> + if (mixin_ctrl < 0)
> >> + return mixin_ctrl;
> >>
> >>/* Save current state of input ADC control register */
> >>adc_ctrl = snd_soc_component_read32(component,
> >> DA7219_ADC_L_CTRL);
> >> + if (adc_ctrl < 0)
> >> + return adc_ctrl;
> >>
> >>/* Enable then Mute MIC PGAs */
> >> - snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
> >> DA7219_MIC_1_AMP_EN_MASK,
> >> + ret = snd_soc_component_update_bits(component,
> >> DA7219_MIC_1_CTRL,
> >> + DA7219_MIC_1_AMP_EN_MASK,
> >>DA7219_MIC_1_AMP_EN_MASK);
> >> - snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + ret = snd_soc_component_update_bits(component,
> >> DA7219_MIC_1_CTRL,
> >>DA7219_MIC_1_AMP_MUTE_EN_MASK,
> >>DA7219_MIC_1_AMP_MUTE_EN_MASK);
> >> + if (ret < 0)
> >> + return ret;
> >>
> >>/* Enable input mixers unmuted */
> >> - snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
> >> + ret = snd_soc_component_update_bits(component,
> >> DA7219_MIXIN_L_CTRL,
> >>DA7219_MIXIN_L_AMP_EN_MASK |
> >>DA7219_MIXIN_L_AMP_MUTE_EN_MASK,
> >>DA7219_MIXIN_L_AMP_EN_MASK);
> >> + if (ret < 0)
> >> + return ret;
> >>
> >>/* Enable input filters unmuted */
> >> - snd_soc_componen
RE: [PATCH 2/2] ASoC: DA7219: Implement error check on reg read and write
On 05 December 2018 04:27, Akshu Agrawal wrote:
> On 12/5/2018 2:46 AM, Adam Thomson wrote:
> > On 04 December 2018 18:36, Akshu Agrawal wrote:
> >
> >> Failed i2c transaction can lead to failure in reg read or write.
> >> Need to have error check for each read write operation.
> >>
> >
> > I'm not really clear what this gains you. If I2C is broken this won't
> > solve anything, and you have far deeper problems. Seems to me like a
> > lot of code which adds almost nothing.
> >
>
> I agree Adam that if I2C is broken, system is in bad state. But so is the
> case with
> most of the error situations. Error handling is anyway needed.
> We have seen case where read32 returned -1, unsigned int cast and the write
> back of the error value ends up in updating the register with 0xff.
> Need to avoid scenarios like this.
If the previous I2C access failed, how can we be sure that the write back to HW
of 0xFF even succeeds? More importantly these error returns won't necessarily
stop subsequent calls to controls within the Codec I believe, so you could still
see unwanted writes to HW via I2C, if I2C is sporadically operational. Again I
don't see this update resolving that. The key thing is to resolve why even just
one I2C transaction fails.
I'd like to hear input from others on this as if this was something
fundamentally required, all other I2C based codecs would be doing this and
that's just not the case right now.
One last thing, if a change like this is somehow deemed to be required, you will
need to update the AAD part of the driver as well.
>
> Thanks,
> Akshu
> >> Signed-off-by: Akshu Agrawal
> >> ---
> >> sound/soc/codecs/da7219.c | 323
> +++-
> >> --
> >> sound/soc/codecs/da7219.h | 2 +-
> >> 2 files changed, 247 insertions(+), 78 deletions(-)
> >>
> >> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> >> index
> >> e46e9f4..6757129 100644
> >> --- a/sound/soc/codecs/da7219.c
> >> +++ b/sound/soc/codecs/da7219.c
> >> @@ -312,69 +312,103 @@ static int da7219_enum_locked_put(struct
> >> snd_kcontrol *kcontrol, }
> >>
> >> /* ALC */
> >> -static void da7219_alc_calib(struct snd_soc_component *component)
> >> +static int da7219_alc_calib(struct snd_soc_component *component)
> >> {
> >> - u8 mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
> >> + int mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
> >> + int ret;
> >>
> >>/* Save current state of mic control register */
> >>mic_ctrl = snd_soc_component_read32(component,
> >> DA7219_MIC_1_CTRL);
> >> + if (mic_ctrl < 0)
> >> + return mic_ctrl;
> >>
> >>/* Save current state of input mixer control register */
> >>mixin_ctrl = snd_soc_component_read32(component,
> >> DA7219_MIXIN_L_CTRL);
> >> + if (mixin_ctrl < 0)
> >> + return mixin_ctrl;
> >>
> >>/* Save current state of input ADC control register */
> >>adc_ctrl = snd_soc_component_read32(component,
> >> DA7219_ADC_L_CTRL);
> >> + if (adc_ctrl < 0)
> >> + return adc_ctrl;
> >>
> >>/* Enable then Mute MIC PGAs */
> >> - snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
> >> DA7219_MIC_1_AMP_EN_MASK,
> >> + ret = snd_soc_component_update_bits(component,
> >> DA7219_MIC_1_CTRL,
> >> + DA7219_MIC_1_AMP_EN_MASK,
> >>DA7219_MIC_1_AMP_EN_MASK);
> >> - snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + ret = snd_soc_component_update_bits(component,
> >> DA7219_MIC_1_CTRL,
> >>DA7219_MIC_1_AMP_MUTE_EN_MASK,
> >>DA7219_MIC_1_AMP_MUTE_EN_MASK);
> >> + if (ret < 0)
> >> + return ret;
> >>
> >>/* Enable input mixers unmuted */
> >> - snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
> >> + ret = snd_soc_component_update_bits(component,
> >> DA7219_MIXIN_L_CTRL,
> >>DA7219_MIXIN_L_AMP_EN_MASK |
> >>DA7219_MIXIN_L_AMP_MUTE_EN_MASK,
> >>DA7219_MIXIN_L_AMP_EN_MASK);
> >> + if (ret < 0)
> >> + return ret;
> >>
> >>/* Enable input filters unmuted */
> >> - snd_soc_componen
RE: [PATCH 2/2] ASoC: DA7219: Implement error check on reg read and write
On 04 December 2018 18:36, Akshu Agrawal wrote:
> Failed i2c transaction can lead to failure in reg read or write.
> Need to have error check for each read write operation.
>
I'm not really clear what this gains you. If I2C is broken this won't solve
anything, and you have far deeper problems. Seems to me like a lot of code which
adds almost nothing.
> Signed-off-by: Akshu Agrawal
> ---
> sound/soc/codecs/da7219.c | 323 +++-
> --
> sound/soc/codecs/da7219.h | 2 +-
> 2 files changed, 247 insertions(+), 78 deletions(-)
>
> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index
> e46e9f4..6757129 100644
> --- a/sound/soc/codecs/da7219.c
> +++ b/sound/soc/codecs/da7219.c
> @@ -312,69 +312,103 @@ static int da7219_enum_locked_put(struct
> snd_kcontrol *kcontrol, }
>
> /* ALC */
> -static void da7219_alc_calib(struct snd_soc_component *component)
> +static int da7219_alc_calib(struct snd_soc_component *component)
> {
> - u8 mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
> + int mic_ctrl, mixin_ctrl, adc_ctrl, calib_ctrl;
> + int ret;
>
> /* Save current state of mic control register */
> mic_ctrl = snd_soc_component_read32(component,
> DA7219_MIC_1_CTRL);
> + if (mic_ctrl < 0)
> + return mic_ctrl;
>
> /* Save current state of input mixer control register */
> mixin_ctrl = snd_soc_component_read32(component,
> DA7219_MIXIN_L_CTRL);
> + if (mixin_ctrl < 0)
> + return mixin_ctrl;
>
> /* Save current state of input ADC control register */
> adc_ctrl = snd_soc_component_read32(component,
> DA7219_ADC_L_CTRL);
> + if (adc_ctrl < 0)
> + return adc_ctrl;
>
> /* Enable then Mute MIC PGAs */
> - snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
> DA7219_MIC_1_AMP_EN_MASK,
> + ret = snd_soc_component_update_bits(component,
> DA7219_MIC_1_CTRL,
> + DA7219_MIC_1_AMP_EN_MASK,
> DA7219_MIC_1_AMP_EN_MASK);
> - snd_soc_component_update_bits(component, DA7219_MIC_1_CTRL,
> + if (ret < 0)
> + return ret;
> +
> + ret = snd_soc_component_update_bits(component,
> DA7219_MIC_1_CTRL,
> DA7219_MIC_1_AMP_MUTE_EN_MASK,
> DA7219_MIC_1_AMP_MUTE_EN_MASK);
> + if (ret < 0)
> + return ret;
>
> /* Enable input mixers unmuted */
> - snd_soc_component_update_bits(component, DA7219_MIXIN_L_CTRL,
> + ret = snd_soc_component_update_bits(component,
> DA7219_MIXIN_L_CTRL,
> DA7219_MIXIN_L_AMP_EN_MASK |
> DA7219_MIXIN_L_AMP_MUTE_EN_MASK,
> DA7219_MIXIN_L_AMP_EN_MASK);
> + if (ret < 0)
> + return ret;
>
> /* Enable input filters unmuted */
> - snd_soc_component_update_bits(component, DA7219_ADC_L_CTRL,
> + ret = snd_soc_component_update_bits(component,
> DA7219_ADC_L_CTRL,
> DA7219_ADC_L_MUTE_EN_MASK |
> DA7219_ADC_L_EN_MASK,
> DA7219_ADC_L_EN_MASK);
> + if (ret < 0)
> + return ret;
>
> /* Perform auto calibration */
> - snd_soc_component_update_bits(component, DA7219_ALC_CTRL1,
> + ret = snd_soc_component_update_bits(component,
> DA7219_ALC_CTRL1,
> DA7219_ALC_AUTO_CALIB_EN_MASK,
> DA7219_ALC_AUTO_CALIB_EN_MASK);
> + if (ret < 0)
> + return ret;
> do {
> calib_ctrl = snd_soc_component_read32(component,
> DA7219_ALC_CTRL1);
> + if (calib_ctrl < 0)
> + return calib_ctrl;
> } while (calib_ctrl & DA7219_ALC_AUTO_CALIB_EN_MASK);
>
> /* If auto calibration fails, disable DC offset, hybrid ALC */
> if (calib_ctrl & DA7219_ALC_CALIB_OVERFLOW_MASK) {
> dev_warn(component->dev,
>"ALC auto calibration failed with overflow\n");
> - snd_soc_component_update_bits(component,
> DA7219_ALC_CTRL1,
> + ret = snd_soc_component_update_bits(component,
> DA7219_ALC_CTRL1,
> DA7219_ALC_OFFSET_EN_MASK |
> DA7219_ALC_SYNC_MODE_MASK, 0);
> + if (ret < 0)
> + return ret;
> } else {
> /* Enable DC offset cancellation, hybrid mode */
> - snd_soc_component_update_bits(component,
> DA7219_ALC_CTRL1,
> + ret = snd_soc_component_update_bits(component,
> DA7219_ALC_CTRL1,
> DA7219_ALC_OFFSET_EN_MASK |
> DA7219_ALC_SYNC_MODE_MASK,
> DA7219_ALC_OFFSET_EN_MASK |
> DA7219_ALC_SYNC_MODE_MASK);
> + if (ret < 0)
> + return ret;
> }
>
> /*
