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, ®, 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
