Re: [PATCH v3 1/3] iio: adc: add support for mcp3911

[Marcus Folkesson]

Hi Andy and Jonathan,


On Fri, Aug 03, 2018 at 11:09:22PM +0100, Jonathan Cameron wrote:
> On Thu, 2 Aug 2018 22:52:00 +0300
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> 
> > On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
> > <marcus.folkesson@xxxxxxxxx> wrote:
> > > MCP3911 is a dual channel Analog Front End (AFE) containing two
> > > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
> > >
> > > Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx>  
> > 
> > > Signed-off-by: Kent Gustavsson <kent@xxxxxxxxxx>  
> > 
> > What is this? Why it's here (presense and location in the message)?
> To clarify... If Kent wrote the patch and you are simply acting
> as gatekeeper / upstreamer you should set the mail to be from Kent
> and put your own Signed-off after his to basically act as a submaintainer
> certifying you believe his sign off and all that entails.
> 
> If it is a bit of a joint effort then that's fine but for copyright
> purposes there should be some indication of the split.

First, thank you Andy for noticing.

I actually intended to use Co-Developed-by (a pretty new tag)
in combination with Signed-off-by.
But the tag must have disappeared in some preparation stage..

From Documentation/process/submitting-patches.rst ::

	A Co-Developed-by: states that the patch was also created by another developer
	along with the original author.  This is useful at times when multiple people
	work on a single patch.  Note, this person also needs to have a Signed-off-by:
	line in the patch as well.

I will switch order and add the Co-Developed-by-tag.
Is this correct?

Co-Developed-by: Kent Gustavsson <kent@xxxxxxxxxx>   
Signed-off-by: Kent Gustavsson <kent@xxxxxxxxxx>  
Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx>  

> 
> 
> > 
> > > + * Copyright (C) 2018 Kent Gustavsson <kent@xxxxxxxxxx>  
> > 
> > > + *  
> > 
> > Redundant.
> > 
> > > +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> > > +{
> > > +       int ret;
> > > +
> > > +       reg = MCP3911_REG_READ(reg, adc->dev_addr);
> > > +       ret = spi_write_then_read(adc->spi, &reg, 1, val, len);
> > > +       if (ret < 0)
> > > +               return ret;
> > > +
> > > +       be32_to_cpus(val);
> > > +       *val >>= ((4 - len) * 8);
> > > +       dev_dbg(&adc->spi->dev, "reading 0x%x from register 0x%x\n", *val,
> > > +                       reg>>1);
> > > +       return ret;
> > > +}
> > > +
> > > +static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
> > > +{
> > > +       dev_dbg(&adc->spi->dev, "writing 0x%x to register 0x%x\n", val, reg);
> > > +
> > > +       val <<= (3 - len) * 8;
> > > +       cpu_to_be32s(&val);
> > > +       val |= MCP3911_REG_WRITE(reg, adc->dev_addr);
> > > +
> > > +       return spi_write(adc->spi, &val, len + 1);
> > > +}  
> > 
> > Can't you use REGMAP_SPI ?
> My instinct is not really. The magic device-addr doesn't help.
> You could work around it but it's not that nice and you'd have
> to create the regmap mapping on the fly or carry static ones
> for each value of htat.
> 
> 
> 
> > 
> > > +       of_property_read_u32(of_node, "device-addr", &adc->dev_addr);
> > > +       if (adc->dev_addr > 3) {
> > > +               dev_err(&adc->spi->dev,
> > > +                               "invalid device address (%i). Must be in range 0-3.\n",
> > > +                               adc->dev_addr);
> > > +               return -EINVAL;
> > > +       }
> > > +       dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr);  
> > 
> > Isn't what we called CS (chip select)?
> Nope. We went around this in an earlier revision. It's an address transmitted
> in the control byte to allow you to 'share' a chip select line between multiple
> chips (crazy but true).
> 
> > 
> > > +       if (IS_ERR(adc->vref)) {  
> > 
> > > +  
> > 
> > Redundant.
> > 
> > > +       if (adc->clki)  
> > 
> > Seems to be redundant if clock is optional (it should be NULL and API
> > is NULL-aware).
> > 
> > > +               clk_disable_unprepare(adc->clki);  
> > 
> > > +       if (adc->clki)
> > > +               clk_disable_unprepare(adc->clki);  
> > 
> > Ditto.
> > 
> > > +#if defined(CONFIG_OF)  
> > 
> > This prevents playing with the device on ACPI enabled platforms.

I will remove the defined(CONFIG_OF) and not use the of_match_ptr()
macro. 

> Yeah, that trickery is still little known (and I forget about it from
> time to time).
> 
> The upshot for those who have missed this is you can use a magic
> acpi device to instantiate based on device tree bindings :)
> 
> So we need to strip all this 'obvious' protection stuff out of drivers.

Jonathan,
Do you want me to do the same changes for drivers in iio/ ?
I'm probably not the only one looking at other drivers when writing my
own, so I guess this is a rather frequent issue for new drivers?


> 
> > 
> > > +               .of_match_table = of_match_ptr(mcp3911_dt_ids),  
> > 
> > Ditto for macro.
> > 
> 

Best regards,
Marcus Folkesson

Attachment: signature.asc
Description: PGP signature