Re: [PATCH v6 3/3] clk: vc5: Add properties for configuring SD/OE behavior

[Sean Anderson <sean.anderson@seco.com>]

On 7/31/21 10:34 AM, Luca Ceresoli wrote:
> Hi Sean,
> 
> On 24/07/21 01:13, Sean Anderson wrote:
>> The SD/OE pin may be configured to enable output when high or low, and
>> to shutdown the device when high. This behavior is controller by the SH
>> and SP bits of the Primary Source and Shutdown Register (and to a lesser
>> extent the OS and OE bits). By default, both bits are 0 (unless set by
>> OTP memory), but they may need to be configured differently, depending
>> on the external circuitry controlling the SD/OE pin.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v6:
>> - Use tri-state properties
>> - Drop Reviewed-bys
>> 
>> Changes in v5:
>> - Use if (...) mask |= ...; instead of mask = ... ? ... : 0;
>> 
>> Changes in v4:
>> - Use dev_err_probe over dev_err
>> - Put new variables on their own line
>> 
>> Changes in v3:
>> - Default to not changing SH or SP unless there is a property affecting
>>   them.
>> 
>> Changes in v2:
>> - Set SH as well as SP
>> 
>>  drivers/clk/clk-versaclock5.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>> 
>> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
>> index bfbb51191c8d..5b986894bd3b 100644
>> --- a/drivers/clk/clk-versaclock5.c
>> +++ b/drivers/clk/clk-versaclock5.c
>> @@ -886,6 +886,7 @@ static const struct of_device_id clk_vc5_of_match[];
>>  
>>  static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>  {
>> +	unsigned int oe, sd, src_mask = 0, src_val = 0;
>>  	struct vc5_driver_data *vc5;
>>  	struct clk_init_data init;
>>  	const char *parent_names[2];
>> @@ -913,6 +914,29 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>  		return dev_err_probe(&client->dev, PTR_ERR(vc5->regmap),
>>  				     "failed to allocate register map\n");
>>  
>> +	ret = of_property_read_u32(client->dev.of_node, "idt,shutdown", &sd);
>> +	if (ret > 0) {
>> +		src_mask |= VC5_PRIM_SRC_SHDN_EN_GBL_SHDN;
>> +		if (sd)
>> +			src_val |= VC5_PRIM_SRC_SHDN_EN_GBL_SHDN;
>> +	} else if (ret != -EINVAL) {
>> +		return dev_err_probe(&client->dev, ret,
>> +				     "could not read idt,shutdown\n");
> 
> Minor nit: "could not read" sounds like "property not found", i.e. the
> property is not present in DT, but if it's not present we do not enter
> here (which is OK). For clarity I'd rather say "error reading idt,...".

dev_err_probe adds to this message, so example output might be

	vc5 1-006a: error 61: could not read idt,shutdown

I think this makes this clearer that there was a problem doing the reading.

> 
>> +	}
>> +
>> +	ret = of_property_read_u32(client->dev.of_node,
>> +				   "idt,output-enable-active", &oe);
>> +	if (ret > 0) {
>> +		src_mask |= VC5_PRIM_SRC_SHDN_SP;
>> +		if (oe)
>> +			src_val |= VC5_PRIM_SRC_SHDN_SP;
>> +	} else if (ret != -EINVAL) {
>> +		return dev_err_probe(&client->dev, ret,
>> +				     "could not read idt,output-enable-active\n");
> 
> Ditto.
> 
> Otherwise LGTM.
>