Re: [PATCH] rtc: ds1307: add trickle charger device tree binding

[Matti Vaittinen]

On Thu, Aug 28, 2014 at 01:59:15PM +0100, ext Mark Rutland wrote:
> On Thu, Aug 28, 2014 at 01:42:44PM +0100, Matti Vaittinen wrote:
> > Patch adding support for specifying trickle charger setup from device
> > tree. Patch is based on linux-next tree.
> > 
> > 
> > Some DS13XX devices have "trickle chargers". Introduce a device tree binding
> > for specifying the setup and register values.
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1339.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1339.txt
> > new file mode 100644
> > index 0000000..9faf40e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1339.txt
> > @@ -0,0 +1,19 @@
> > +* Dallas DS1339		I2C Serial Real-Time Clock
> > +
> > +Required properties:
> > +- compatible: Should contain "dallas,ds1339".
> > +- reg: I2C address for chip
> > +
> > +Optional properties:
> > +- trickle_setup : Used Trickle Charger configuration,
> > +        corresponding to 5 lowest bits in trickle charger register.
> 
> s/_/-/ in property names please.
I'll fix this, thanks.

> Why do we not have a higher-level description?
I can add the meanings for this specific chip in the binding document,
  however I would still keep the raw register value as the dt entry due
  to reasons explained in my previous email.
> 
> > +- trickle_reg   : Trickle charger register address. Defaults to 0x10 for ds1339.
> 
> Is this expected to change? Why does this need to be in the DT?

I was thinking it might be handly as I do not know the details of all
variants of this chip. However I guess it could be left out.

> 
> > +
> > +Example:
> > +	ds1339: rtc@68 {
> > +		compatible = "dallas,ds1339";
> > +        trickle_setup= <0x5>;
> > +		reg = <0x68>;
> > +	};
> > +
> > +
> > diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> > index f03d5ba..f9b1244 100644
> > --- a/drivers/rtc/rtc-ds1307.c
> > +++ b/drivers/rtc/rtc-ds1307.c
> > @@ -126,9 +126,10 @@ struct chip_desc {
> >  	u16			nvram_offset;
> >  	u16			nvram_size;
> >  	u16			trickle_charger_reg;
> > +	u8		  trickle_charger_setup;
> >  };
> >  
> > -static const struct chip_desc chips[last_ds_type] = {
> > +static struct chip_desc chips[last_ds_type] = {
> >  	[ds_1307] = {
> >  		.nvram_offset	= 8,
> >  		.nvram_size	= 56,
> > @@ -835,13 +836,37 @@ ds1307_nvram_write(struct file *filp, struct kobject *kobj,
> >  
> >  /*----------------------------------------------------------------------*/
> >  
> > +static void trickle_charger_of_init(struct chip_desc *chip,
> > +			struct device_node *node)
> > +{
> > +	int plen = 0;
> > +	const uint32_t *setup;
> > +	const uint32_t *reg;
> > +
> > +	setup = of_get_property(node, "trickle_setup" , &plen);
> > +	if (plen != 4)
> > +		goto out;
> > +	chip->trickle_charger_setup = (u8)*setup;
> 
> This isn't endian-clean, and the casting is unnecessary.
> 
> Use the of_property_read_u32 accessor, with a u32 temporary variable.

I'll fix this, thanks.

> 
> > +	plen = 0;
> > +	reg = of_get_property(node, "trickle_reg" , &plen);
> > +	if (plen == 4)
> > +		chip->trickle_charger_reg = (u16)*reg;
> 
> Likewise.
> 
I'll drop the register address from dt alltogether.


I will fix these issues and prepare a new patch.

Br. Matti Vaittinen


-- 
=============================================

Matti Vaittinen
Senile SW Specialist
FINLAND 

~~ When things go utterly wrong vim users can always type :help! ~~

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/