Re: [PATCH 8/9] clk: si5341: Add silabs,iovdd-33 property

[Robert Hancock <robert.hancock@calian.com>]

On Fri, 2021-03-12 at 07:44 +0100, Mike Looijmans wrote:
> Met vriendelijke groet / kind regards,
> 
> Mike Looijmans
> System Expert
> 
> 
> TOPIC Embedded Products B.V.
> Materiaalweg 4, 5681 RJ Best
> The Netherlands
> 
> T: +31 (0) 499 33 69 69
> E: mike.looijmans@topicproducts.com
> W: 
> https://urldefense.com/v3/__http://www.topicproducts.com__;!!IOGos0k!1ZKoWVwv09fDC-o4jRw92NZvHBl_3MOcBIPl8DLLDN0szJlo9IH0GC-6qH0-Pq1W-PA$
>  
> 
> Please consider the environment before printing this e-mail
> On 11-03-2021 23:24, Robert Hancock wrote:
> > Add a property to allow specifying that the external I2C IO pins are using
> > 3.3V voltage thresholds rather than 1.8V.
> > 
> > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > ---
> >   drivers/clk/clk-si5341.c | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c
> > index 11740855bcde..4cd80ef389d2 100644
> > --- a/drivers/clk/clk-si5341.c
> > +++ b/drivers/clk/clk-si5341.c
> > @@ -81,6 +81,7 @@ struct clk_si5341 {
> >   	u8 num_synth;
> >   	u16 chip_id;
> >   	bool xaxb_ext_clk;
> > +	bool iovdd_33;
> >   };
> >   #define to_clk_si5341(_hw)	container_of(_hw, struct clk_si5341,
> > hw)
> >   
> > @@ -103,6 +104,7 @@ struct clk_si5341_output_config {
> >   #define SI5341_IN_SEL		0x0021
> >   #define SI5341_DEVICE_READY	0x00FE
> >   #define SI5341_XAXB_CFG		0x090E
> > +#define SI5341_IO_VDD_SEL	0x0943
> >   #define SI5341_IN_EN		0x0949
> >   #define SI5341_INX_TO_PFD_EN	0x094A
> >   
> > @@ -351,7 +353,6 @@ static const struct si5341_reg_default
> > si5341_reg_defaults[] = {
> >   	{ 0x0804, 0x00 }, /* Not in datasheet */
> >   	{ 0x090E, 0x02 }, /* XAXB_EXTCLK_EN=0 XAXB_PDNB=1 (use XTAL) */
> >   	{ 0x091C, 0x04 }, /* ZDM_EN=4 (Normal mode) */
> > -	{ 0x0943, 0x00 }, /* IO_VDD_SEL=0 (0=1v8, use 1=3v3) */
> >   	{ 0x0949, 0x00 }, /* IN_EN (disable input clocks) */
> >   	{ 0x094A, 0x00 }, /* INx_TO_PFD_EN (disabled) */
> >   	{ 0x0A02, 0x00 }, /* Not in datasheet */
> > @@ -1160,6 +1161,11 @@ static int si5341_finalize_defaults(struct
> > clk_si5341 *data)
> >   	int res;
> >   	u32 revision;
> >   
> > +	res = regmap_write(data->regmap, SI5341_IO_VDD_SEL,
> > +			   data->iovdd_33 ? 1 : 0);
> > +	if (res < 0)
> > +		return res;
> > +
> >   	res = regmap_read(data->regmap, SI5341_DEVICE_REV, &revision);
> >   	if (res < 0)
> >   		return res;
> > @@ -1565,6 +1571,8 @@ static int si5341_probe(struct i2c_client *client,
> >   	}
> >   	data->xaxb_ext_clk = of_property_read_bool(client->dev.of_node,
> >   						   "silabs,xaxb-ext-clk");
> > +	data->iovdd_33 = of_property_read_bool(client->dev.of_node,
> > +					       "silabs,iovdd-33");
> >   
> 
> Seems a waste to me to store this in the 'data' object forever while it 
> is to be used only once during init and never again after that.

That is true, but it would then have to be passed down into the
si5341_finalize_defaults function separately and it would be inconsistent with
how all of the other configuration is handled. So I'm not sure it's worth
saving ~ 1 byte in the data object..

> 
> 
> >   	if (initialization_required) {
> >   		/* Populate the regmap cache in preparation for "cache only" */
> 
>