Re: [2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm

[Stefan Bruens]

On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote:
> On 10/01/2017 09:48 PM, Stefan Brüns wrote:
> > According to the ABI documentation, the shunt resistor value should be
> > specificied in Ohm. As this is also used/documented for the MAX9611,
> > use the same for the INA2xx driver.
> > 
> > This poses an ABI break for anyone actually altering the shunt value
> > through the sysfs interface, it does not alter the default value nor
> > a value set from the devicetree.
> > 
> > Minor change: Fix comment, 1mA is 10^-3A.
> 
> I have just a minor issue. There could be an inconsistency with units as in
> my patch I make current_lsb adjustable and I need it to be in uA (it used
> to be hardcoded as 1 mA so to achieve better precision we need smaller
> units). So in order to keep calibration register properly scaled, I convert
> uOhms to mOhms on each set_calibration(). So if both my changes and your
> changes were applied, on each shunt_resistore_store we would be performing
> multiplication by 10^6 and then in set_calibration() division by 10^3 which
> seems odd to me.
> 
> I guess we could keep it as shunt_resistor_ohms instead of
> shunt_resistor_uohm. We could avoid performing division on each
> shunt_resistor_show() and perform multiplication by 10^3 only once in
> set_calibration() on each
> shunt_resistore_store(). We could then change the default value and perform
> division only on probing, when reading the shunt_resistance from device
> tree.
> 
> There are many other options. It's not a major issue so maybe we could leave
> it as it is or you could suggest some changes in my patch.
 
Sorry it took me so long to answer ...

The current fixed current_lsb of 1mA is indeed a bad choice for everything but 
a shunt resistor value of 10mOhm, as it truncates the current value. So what 
is a *good* choice?

One important point is the current register is merely more than a convenience 
register. At least for the INA219/220, it provides nothing not achievable in 
software, and for the INA226 family it only has added value if the current is 
varying faster than the readout frequency and the averaging is used.

The precision of the current register is limited by the precision of the shunt 
voltage register, and may be reduced by the applied scaling/calibration 
factor.

The precision of the shunt voltage register is fixed at 10uV (INA219) resp. 
2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the 
noise and offset, but the lsb value is still fixed.

If one wants to carry over the shunt voltage register precision into the 
current register, its important no (or hardly any) truncation happens. The 
terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3):

INA219: current = shunt_voltage * cal_register / 4096
INA226: current = shunt_voltage * cal_register / 2048

So any cal value smaller than 4096 (2048) will introduce truncation errors, 
larger values may introduce overflows, if the full input range is used. Now, 
would it not be wise to always use 4096 (2048) for the calibration value?

The raw values from the IIO subsystem are meaningless without their 
accompanying scale factor. Instead of changing the calibration value, why not 
just change the reported scale factor?

More opinions are very welcome.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm

[Stefan Bruens]

On Montag, 9. Oktober 2017 11:29:43 CEST Maciej Purski wrote:
> On 10/01/2017 09:48 PM, Stefan Brüns wrote:
> > According to the ABI documentation, the shunt resistor value should be
> > specificied in Ohm. As this is also used/documented for the MAX9611,
> > use the same for the INA2xx driver.
> > 
> > This poses an ABI break for anyone actually altering the shunt value
> > through the sysfs interface, it does not alter the default value nor
> > a value set from the devicetree.
> > 
> > Minor change: Fix comment, 1mA is 10^-3A.
> 
> I have just a minor issue. There could be an inconsistency with units as in
> my patch I make current_lsb adjustable and I need it to be in uA (it used
> to be hardcoded as 1 mA so to achieve better precision we need smaller
> units). So in order to keep calibration register properly scaled, I convert
> uOhms to mOhms on each set_calibration(). So if both my changes and your
> changes were applied, on each shunt_resistore_store we would be performing
> multiplication by 10^6 and then in set_calibration() division by 10^3 which
> seems odd to me.
> 
> I guess we could keep it as shunt_resistor_ohms instead of
> shunt_resistor_uohm. We could avoid performing division on each
> shunt_resistor_show() and perform multiplication by 10^3 only once in
> set_calibration() on each
> shunt_resistore_store(). We could then change the default value and perform
> division only on probing, when reading the shunt_resistance from device
> tree.
> 
> There are many other options. It's not a major issue so maybe we could leave
> it as it is or you could suggest some changes in my patch.
 
Sorry it took me so long to answer ...

The current fixed current_lsb of 1mA is indeed a bad choice for everything but 
a shunt resistor value of 10mOhm, as it truncates the current value. So what 
is a *good* choice?

One important point is the current register is merely more than a convenience 
register. At least for the INA219/220, it provides nothing not achievable in 
software, and for the INA226 family it only has added value if the current is 
varying faster than the readout frequency and the averaging is used.

The precision of the current register is limited by the precision of the shunt 
voltage register, and may be reduced by the applied scaling/calibration 
factor.

The precision of the shunt voltage register is fixed at 10uV (INA219) resp. 
2.5uV (INA226). Changing conversion time (both) and PGA (219) affects the 
noise and offset, but the lsb value is still fixed.

If one wants to carry over the shunt voltage register precision into the 
current register, its important no (or hardly any) truncation happens. The 
terms therefor are given in the manual, formulas 8.5.1 (4) resp 7.5.1 (3):

INA219: current = shunt_voltage * cal_register / 4096
INA226: current = shunt_voltage * cal_register / 2048

So any cal value smaller than 4096 (2048) will introduce truncation errors, 
larger values may introduce overflows, if the full input range is used. Now, 
would it not be wise to always use 4096 (2048) for the calibration value?

The raw values from the IIO subsystem are meaningless without their 
accompanying scale factor. Instead of changing the calibration value, why not 
just change the reported scale factor?

More opinions are very welcome.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Stefan Bruens]

On Mittwoch, 27. September 2017 11:09:22 CEST Maxime Ripard wrote:
> On Sat, Sep 23, 2017 at 12:00:15AM +, Brüns, Stefan wrote:
> > On Freitag, 22. September 2017 23:30:27 CEST Maxime Ripard wrote:
> > > On Tue, Sep 19, 2017 at 04:17:59PM +, Brüns, Stefan wrote:
> > > > On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:
> > > > > On Mon, Sep 18, 2017 at 02:09:43PM +, Brüns, Stefan wrote:
> > > > > > On Montag, 18. September 2017 10:18:24 CEST you wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:
> > > > > > > > +   ret = of_property_read_u32(np, "dma-channels",
> > > > > > > > >num_pchans);
> > > > > > > > +   if (ret && !sdc->num_pchans) {
> > > > > > > > +   dev_err(>dev, "Can't get 
> > > > > > > > dma-channels.\n");
> > > > > > > > +   return ret;
> > > > > > > > +   }
> > > > > > > > +
> > > > > > > > +   if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > > > > > > > +   dev_err(>dev, "Number of dma-channels out 
> > > > > > > > of range.
> > 
> > \n");
> > 
> > > > > > > > +   return -EINVAL;
> > > > > > > > +   }
> > > > > > > > +
> > > > > > > > +   ret = of_property_read_u32(np, "dma-requests",
> > > > > > > > >max_request);
> > > > > > > > +   if (ret && !sdc->max_request) {
> > > > > > > > +   dev_info(>dev, "Missing dma-requests, 
> > > > > > > > using %u.\n",
> > > > > > > > +DMA_CHAN_MAX_DRQ);
> > > > > > > > +   sdc->max_request = DMA_CHAN_MAX_DRQ;
> > > > > > > > +   }
> > > > > > > > +
> > > > > > > > +   if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
> > > > > > > > +   dev_err(>dev, "Value of dma-requests out 
> > > > > > > > of
> > > > > > > > range.\n");
> > > > > > > > +   return -EINVAL;
> > > > > > > > +   }
> > > > > > > 
> > > > > > > I'm not really convinced about these two checks. They don't
> > > > > > > catch
> > > > > > > all
> > > > > > > errors (the range between the actual number of channels / DRQ
> > > > > > > and
> > > > > > > the
> > > > > > > maximum allowed per the registers), they might increase in the
> > > > > > > future
> > > > > > > too, and if we want to make that check actually working, we
> > > > > > > would
> > > > > > > have
> > > > > > > to duplicate the number of requests and channels into the
> > > > > > > driver.
> > > > > > 
> > > > > > 1. If these values increase, we have a new register layout and and
> > > > > > need a new compatible anyway.
> > > > > 
> > > > > And you want to store a new maximum attached to the compatible?
> > > > > Isn't
> > > > > that exactly the situation you're trying to get away from?
> > > > 
> > > > Yes, and no. H3, H5, A64 and R40 have the exact same register layout,
> > > > but
> > > > different number of channels and ports. They could share a compatible
> > > > (if
> > > > DMA channels were generalized), and we already have several register
> > > > offsets/ widths (implicitly via the callbacks) attached to the
> > > > compatible
> > > > (so these don't need generalization via DT).
> > > > 
> > > > Now, we could also move everything that is currently attached to the
> > > > compatible, i.e. clock gate register offset, burst widths/lengths etc.
> > > > into
> > > > the devicetree binding, but that would just be too much.
> > > > 
> > > > The idea is to find a middle ground here, using common patterns in the
> > > > existing SoCs. The register layout has hardly changed, while the
> > > > number of
> > > > DMA channels and ports changes all the time. Moving the number of DMA
> > > > channels and ports to the DT is trivial, and a pattern also found in
> > > > other DMA controller drivers.
> > > 
> > > I'm sorry, but the code is inconsistent here. You basically have two
> > > variables from one SoC to the other, the number of channels and
> > > requests.
> > > 
> > > In one case (channels), it mandates that the property is provided in
> > > the device tree, and doesn't default to anything.
> > > 
> > > In the other case (requests), the property is optional and it will
> > > provide a default. All that in 20 lines.
> > 
> > The channel number is a hardware property. Using more channels than the
> > hardware provides is a bug. There is no default.
> > 
> > The port/request is just some lax property to limit the resource
> > allocation
> > upfront. As long as the bindings of the different IP blocks (SPI, audio,
> > ...) provide the correct port numbers, all required information is
> > available.
> Using an improper request ID or out of bounds will be just as much as
> a bug. You will not get your DMA transfer to the proper device you
> were trying to, the data will not reach the device or memory, your
> driver will not work => a bug.
> 
> It will not be for the same reasons, you will not overwrite other
> registers, but the end result is just the same: your transfer will not
> work.

Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Stefan Bruens]

On Mittwoch, 27. September 2017 11:09:22 CEST Maxime Ripard wrote:
> On Sat, Sep 23, 2017 at 12:00:15AM +, Brüns, Stefan wrote:
> > On Freitag, 22. September 2017 23:30:27 CEST Maxime Ripard wrote:
> > > On Tue, Sep 19, 2017 at 04:17:59PM +, Brüns, Stefan wrote:
> > > > On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:
> > > > > On Mon, Sep 18, 2017 at 02:09:43PM +, Brüns, Stefan wrote:
> > > > > > On Montag, 18. September 2017 10:18:24 CEST you wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:
> > > > > > > > +   ret = of_property_read_u32(np, "dma-channels",
> > > > > > > > >num_pchans);
> > > > > > > > +   if (ret && !sdc->num_pchans) {
> > > > > > > > +   dev_err(>dev, "Can't get 
> > > > > > > > dma-channels.\n");
> > > > > > > > +   return ret;
> > > > > > > > +   }
> > > > > > > > +
> > > > > > > > +   if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > > > > > > > +   dev_err(>dev, "Number of dma-channels out 
> > > > > > > > of range.
> > 
> > \n");
> > 
> > > > > > > > +   return -EINVAL;
> > > > > > > > +   }
> > > > > > > > +
> > > > > > > > +   ret = of_property_read_u32(np, "dma-requests",
> > > > > > > > >max_request);
> > > > > > > > +   if (ret && !sdc->max_request) {
> > > > > > > > +   dev_info(>dev, "Missing dma-requests, 
> > > > > > > > using %u.\n",
> > > > > > > > +DMA_CHAN_MAX_DRQ);
> > > > > > > > +   sdc->max_request = DMA_CHAN_MAX_DRQ;
> > > > > > > > +   }
> > > > > > > > +
> > > > > > > > +   if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
> > > > > > > > +   dev_err(>dev, "Value of dma-requests out 
> > > > > > > > of
> > > > > > > > range.\n");
> > > > > > > > +   return -EINVAL;
> > > > > > > > +   }
> > > > > > > 
> > > > > > > I'm not really convinced about these two checks. They don't
> > > > > > > catch
> > > > > > > all
> > > > > > > errors (the range between the actual number of channels / DRQ
> > > > > > > and
> > > > > > > the
> > > > > > > maximum allowed per the registers), they might increase in the
> > > > > > > future
> > > > > > > too, and if we want to make that check actually working, we
> > > > > > > would
> > > > > > > have
> > > > > > > to duplicate the number of requests and channels into the
> > > > > > > driver.
> > > > > > 
> > > > > > 1. If these values increase, we have a new register layout and and
> > > > > > need a new compatible anyway.
> > > > > 
> > > > > And you want to store a new maximum attached to the compatible?
> > > > > Isn't
> > > > > that exactly the situation you're trying to get away from?
> > > > 
> > > > Yes, and no. H3, H5, A64 and R40 have the exact same register layout,
> > > > but
> > > > different number of channels and ports. They could share a compatible
> > > > (if
> > > > DMA channels were generalized), and we already have several register
> > > > offsets/ widths (implicitly via the callbacks) attached to the
> > > > compatible
> > > > (so these don't need generalization via DT).
> > > > 
> > > > Now, we could also move everything that is currently attached to the
> > > > compatible, i.e. clock gate register offset, burst widths/lengths etc.
> > > > into
> > > > the devicetree binding, but that would just be too much.
> > > > 
> > > > The idea is to find a middle ground here, using common patterns in the
> > > > existing SoCs. The register layout has hardly changed, while the
> > > > number of
> > > > DMA channels and ports changes all the time. Moving the number of DMA
> > > > channels and ports to the DT is trivial, and a pattern also found in
> > > > other DMA controller drivers.
> > > 
> > > I'm sorry, but the code is inconsistent here. You basically have two
> > > variables from one SoC to the other, the number of channels and
> > > requests.
> > > 
> > > In one case (channels), it mandates that the property is provided in
> > > the device tree, and doesn't default to anything.
> > > 
> > > In the other case (requests), the property is optional and it will
> > > provide a default. All that in 20 lines.
> > 
> > The channel number is a hardware property. Using more channels than the
> > hardware provides is a bug. There is no default.
> > 
> > The port/request is just some lax property to limit the resource
> > allocation
> > upfront. As long as the bindings of the different IP blocks (SPI, audio,
> > ...) provide the correct port numbers, all required information is
> > available.
> Using an improper request ID or out of bounds will be just as much as
> a bug. You will not get your DMA transfer to the proper device you
> were trying to, the data will not reach the device or memory, your
> driver will not work => a bug.
> 
> It will not be for the same reasons, you will not overwrite other
> registers, but the end result is just the same: your transfer will not
> work.

Re: [PATCH v2 06/10] arm64: allwinner: a64: Add devicetree binding for DMA controller

[Stefan Bruens]

On Mittwoch, 20. September 2017 22:53:00 CEST Rob Herring wrote:
> On Sun, Sep 17, 2017 at 05:19:52AM +0200, Stefan Brüns wrote:
> > The A64 is register compatible with the H3, but has a different number
> > of dma channels and request ports.
> > 
> > Attach additional properties to the node to allow future reuse of the
> > compatible for controllers with different number of channels/requests.
> > 
> > If dma-requests is not specified, the register layout defined maximum
> > of 32 is used.
> > 
> > Signed-off-by: Stefan Brüns 
> > ---
> > 
> >  .../devicetree/bindings/dma/sun6i-dma.txt  | 26
> >  ++ 1 file changed, 26 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > b/Documentation/devicetree/bindings/dma/sun6i-dma.txt index
> > 98fbe1a5c6dd..6ebc79f95202 100644
> > --- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > 
> > @@ -27,6 +27,32 @@ Example:
> > #dma-cells = <1>;
> > 
> > };
> > 
> > +-
> > - +For A64 DMA controller:
> > +
> > +Required properties:
> > +- compatible:  "allwinner,sun50i-a64-dma"
> > +- dma-channels: Number of DMA channels supported by the controller.
> > +   Refer to Documentation/devicetree/bindings/dma/dma.txt
> > +- all properties above, i.e. reg, interrupts, clocks, resets and
> > #dma-cells +
> > +Optional properties:
> > +- dma-requests: Number of DMA request signals supported by the
> > controller.
> > +   Refer to Documentation/devicetree/bindings/dma/dma.txt
> > +
> > +Example:
> > +   dma: dma-controller@01c02000 {
> 
> Drop the leading 0. Building dtbs with W=2 will tell you this.
> 
> With that,
> 
> Acked-by: Rob Herring 

The leading 0 was copied from the A31 example just a few lines above. Should I 
also correct that one, or should that go in a separate patch?

Kind regards,

Stefan
-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH v2 06/10] arm64: allwinner: a64: Add devicetree binding for DMA controller

[Stefan Bruens]

On Mittwoch, 20. September 2017 22:53:00 CEST Rob Herring wrote:
> On Sun, Sep 17, 2017 at 05:19:52AM +0200, Stefan Brüns wrote:
> > The A64 is register compatible with the H3, but has a different number
> > of dma channels and request ports.
> > 
> > Attach additional properties to the node to allow future reuse of the
> > compatible for controllers with different number of channels/requests.
> > 
> > If dma-requests is not specified, the register layout defined maximum
> > of 32 is used.
> > 
> > Signed-off-by: Stefan Brüns 
> > ---
> > 
> >  .../devicetree/bindings/dma/sun6i-dma.txt  | 26
> >  ++ 1 file changed, 26 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > b/Documentation/devicetree/bindings/dma/sun6i-dma.txt index
> > 98fbe1a5c6dd..6ebc79f95202 100644
> > --- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > 
> > @@ -27,6 +27,32 @@ Example:
> > #dma-cells = <1>;
> > 
> > };
> > 
> > +-
> > - +For A64 DMA controller:
> > +
> > +Required properties:
> > +- compatible:  "allwinner,sun50i-a64-dma"
> > +- dma-channels: Number of DMA channels supported by the controller.
> > +   Refer to Documentation/devicetree/bindings/dma/dma.txt
> > +- all properties above, i.e. reg, interrupts, clocks, resets and
> > #dma-cells +
> > +Optional properties:
> > +- dma-requests: Number of DMA request signals supported by the
> > controller.
> > +   Refer to Documentation/devicetree/bindings/dma/dma.txt
> > +
> > +Example:
> > +   dma: dma-controller@01c02000 {
> 
> Drop the leading 0. Building dtbs with W=2 will tell you this.
> 
> With that,
> 
> Acked-by: Rob Herring 

The leading 0 was copied from the A31 example just a few lines above. Should I 
also correct that one, or should that go in a separate patch?

Kind regards,

Stefan
-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH 08/10] dmaengine: sun6i: Add support for Allwinner A64 and compatibles

[Stefan Bruens]

On Montag, 4. September 2017 01:37:58 CEST André Przywara wrote:

> > @@ -1090,6 +1101,7 @@ MODULE_DEVICE_TABLE(of, sun6i_dma_match);
> > 
> >  static int sun6i_dma_probe(struct platform_device *pdev)
> >  {
> >  
> > const struct of_device_id *device;
> > 
> > +   struct device_node *np = pdev->dev.of_node;
> 
> Is this some rebase/split artefact?
> 
> Cheers,
> Andre.
> 

Yes, that one should be in patch 7/10 ...

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH 08/10] dmaengine: sun6i: Add support for Allwinner A64 and compatibles

[Stefan Bruens]

On Montag, 4. September 2017 01:37:58 CEST André Przywara wrote:

> > @@ -1090,6 +1101,7 @@ MODULE_DEVICE_TABLE(of, sun6i_dma_match);
> > 
> >  static int sun6i_dma_probe(struct platform_device *pdev)
> >  {
> >  
> > const struct of_device_id *device;
> > 
> > +   struct device_node *np = pdev->dev.of_node;
> 
> Is this some rebase/split artefact?
> 
> Cheers,
> Andre.
> 

Yes, that one should be in patch 7/10 ...

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Stefan Bruens]

On Montag, 4. September 2017 01:44:54 CEST André Przywara wrote:
> Hi,
> 
> On 03/09/17 23:40, Stefan Brüns wrote:
> > To avoid introduction of a new compatible for each small SoC/DMA
> > controller
> > variation, move the definition of the channel count to the devicetree.
> > 
> > The number of vchans is no longer explicit, but limited by the highest
> > port/DMA request number. The result is a slight overallocation for SoCs
> > with a sparse port mapping.
> > 
> > Signed-off-by: Stefan Brüns 
> > ---
> > 
> >  drivers/dma/sun6i-dma.c | 35 ++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index c69dadb853d2..bd4c2e4a759b 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -42,6 +42,9 @@
> > 
> >  #define DMA_STAT   0x30
> > 
> > +/* Offset between DMA_IRQ_EN and DMA_IRQ_STAT limits number of channels
> > */
> > +#define DMA_MAX_CHANNELS   (DMA_IRQ_CHAN_NR * 0x10 / 4)
> > +
> > 
> >  /*
> >  
> >   * sun8i specific registers
> >   */
> > 
> > @@ -65,7 +68,8 @@
> > 
> >  #define DMA_CHAN_LLI_ADDR  0x08
> >  
> >  #define DMA_CHAN_CUR_CFG   0x0c
> > 
> > -#define DMA_CHAN_CFG_SRC_DRQ(x)((x) & 0x1f)
> > +#define DMA_CHAN_MAX_DRQ   0x1f
> > +#define DMA_CHAN_CFG_SRC_DRQ(x)((x) & DMA_CHAN_MAX_DRQ)
> > 
> >  #define DMA_CHAN_CFG_SRC_IO_MODE   BIT(5)
> >  #define DMA_CHAN_CFG_SRC_LINEAR_MODE   (0 << 5)
> >  #define DMA_CHAN_CFG_SRC_BURST_A31(x)  (((x) & 0x3) << 7)
> > 
> > @@ -1173,6 +1177,35 @@ static int sun6i_dma_probe(struct platform_device
> > *pdev)> 
> > sdc->num_vchans = sdc->cfg->nr_max_vchans;
> > sdc->max_request = sdc->cfg->nr_max_requests;
> > 
> > +   ret = of_property_read_u32(np, "dma-channels", >num_pchans);
> > +   if (ret && !sdc->num_pchans) {
> > +   dev_err(>dev, "Can't get dma-channels.\n");
> > +   return ret;
> > +   }
> > +
> > +   if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > +   dev_err(>dev, "Number of dma-channels out of range.\n");
> > +   return -EINVAL;
> > +   }
> > +
> > +   ret = of_property_read_u32(np, "dma-requests", >max_request);
> > +   if (ret && !sdc->max_request) {
> > +   dev_info(>dev, "Missing dma-requests, using %u.\n",
> > +DMA_CHAN_MAX_DRQ);
> 
> Mmmh, is this mapping of "!sdc->max_request" -> DMA_CHAN_MAX_DRQ
> implemented somewhere else? Or is it just missing here:
>   sdc->max_request = DMA_CHAN_MAX_DRQ;

Well spotted, that assignment is actually missing.

With this line added, your comment for patch 8/10 should also be addressed 
(regarding the default value).
 
> Otherwise this is looking good, thanks for picking up the DT property
> approach!
> 
> Cheers,
> Andre.
> 

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Stefan Bruens]

On Montag, 4. September 2017 01:44:54 CEST André Przywara wrote:
> Hi,
> 
> On 03/09/17 23:40, Stefan Brüns wrote:
> > To avoid introduction of a new compatible for each small SoC/DMA
> > controller
> > variation, move the definition of the channel count to the devicetree.
> > 
> > The number of vchans is no longer explicit, but limited by the highest
> > port/DMA request number. The result is a slight overallocation for SoCs
> > with a sparse port mapping.
> > 
> > Signed-off-by: Stefan Brüns 
> > ---
> > 
> >  drivers/dma/sun6i-dma.c | 35 ++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index c69dadb853d2..bd4c2e4a759b 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -42,6 +42,9 @@
> > 
> >  #define DMA_STAT   0x30
> > 
> > +/* Offset between DMA_IRQ_EN and DMA_IRQ_STAT limits number of channels
> > */
> > +#define DMA_MAX_CHANNELS   (DMA_IRQ_CHAN_NR * 0x10 / 4)
> > +
> > 
> >  /*
> >  
> >   * sun8i specific registers
> >   */
> > 
> > @@ -65,7 +68,8 @@
> > 
> >  #define DMA_CHAN_LLI_ADDR  0x08
> >  
> >  #define DMA_CHAN_CUR_CFG   0x0c
> > 
> > -#define DMA_CHAN_CFG_SRC_DRQ(x)((x) & 0x1f)
> > +#define DMA_CHAN_MAX_DRQ   0x1f
> > +#define DMA_CHAN_CFG_SRC_DRQ(x)((x) & DMA_CHAN_MAX_DRQ)
> > 
> >  #define DMA_CHAN_CFG_SRC_IO_MODE   BIT(5)
> >  #define DMA_CHAN_CFG_SRC_LINEAR_MODE   (0 << 5)
> >  #define DMA_CHAN_CFG_SRC_BURST_A31(x)  (((x) & 0x3) << 7)
> > 
> > @@ -1173,6 +1177,35 @@ static int sun6i_dma_probe(struct platform_device
> > *pdev)> 
> > sdc->num_vchans = sdc->cfg->nr_max_vchans;
> > sdc->max_request = sdc->cfg->nr_max_requests;
> > 
> > +   ret = of_property_read_u32(np, "dma-channels", >num_pchans);
> > +   if (ret && !sdc->num_pchans) {
> > +   dev_err(>dev, "Can't get dma-channels.\n");
> > +   return ret;
> > +   }
> > +
> > +   if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > +   dev_err(>dev, "Number of dma-channels out of range.\n");
> > +   return -EINVAL;
> > +   }
> > +
> > +   ret = of_property_read_u32(np, "dma-requests", >max_request);
> > +   if (ret && !sdc->max_request) {
> > +   dev_info(>dev, "Missing dma-requests, using %u.\n",
> > +DMA_CHAN_MAX_DRQ);
> 
> Mmmh, is this mapping of "!sdc->max_request" -> DMA_CHAN_MAX_DRQ
> implemented somewhere else? Or is it just missing here:
>   sdc->max_request = DMA_CHAN_MAX_DRQ;

Well spotted, that assignment is actually missing.

With this line added, your comment for patch 8/10 should also be addressed 
(regarding the default value).
 
> Otherwise this is looking good, thanks for picking up the DT property
> approach!
> 
> Cheers,
> Andre.
> 

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64

[Stefan Bruens]

On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote:
> Hi,
> 
> On 01/09/17 02:19, Stefan Bruens wrote:
> > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote:
> >> Hi,
> >> 
> >> On 31/08/17 00:36, Stefan Brüns wrote:
> >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a
> >>> reduced amount of physical channels. Add the proper config data
> >>> and compatible string to support it.
> >> 
> >> ...
> >> 
> >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> >>> index 5f4eee4513e5..6a17c5d63582 100644
> >>> --- a/drivers/dma/sun6i-dma.c
> >>> +++ b/drivers/dma/sun6i-dma.c
> >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg =
> >>> {
> >>> 
> >>>   .nr_max_vchans   = 34,
> >>>   .dmac_variant= DMAC_VARIANT_H3,
> >>>  
> >>>  };
> >>> 
> >>> +
> >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> >>> + .nr_max_channels = 8,
> >>> + .nr_max_requests = 27,
> >>> + .nr_max_vchans   = 38,
> >>> + .dmac_variant= DMAC_VARIANT_H3,
> >>> 
> >>>  };
> >>>  
[...]
> > There are also the incompatibilities in the "DMA channel configuration
> > register" (burst length; burst width; burst length field offset).
> > 
> > We can either have 3 different compatible strings, or another property for
> > the register model.
> 
> The latter is usually frowned upon, using separate compatible strings
> for each group of SoCs is the way to go here.

Just for clarification, I was not talking about a property in the devicetree, 
but about a struct member in the config data, i.e. the .dmac_variant above.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64

[Stefan Bruens]

On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote:
> Hi,
> 
> On 01/09/17 02:19, Stefan Bruens wrote:
> > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote:
> >> Hi,
> >> 
> >> On 31/08/17 00:36, Stefan Brüns wrote:
> >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a
> >>> reduced amount of physical channels. Add the proper config data
> >>> and compatible string to support it.
> >> 
> >> ...
> >> 
> >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> >>> index 5f4eee4513e5..6a17c5d63582 100644
> >>> --- a/drivers/dma/sun6i-dma.c
> >>> +++ b/drivers/dma/sun6i-dma.c
> >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg =
> >>> {
> >>> 
> >>>   .nr_max_vchans   = 34,
> >>>   .dmac_variant= DMAC_VARIANT_H3,
> >>>  
> >>>  };
> >>> 
> >>> +
> >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> >>> + .nr_max_channels = 8,
> >>> + .nr_max_requests = 27,
> >>> + .nr_max_vchans   = 38,
> >>> + .dmac_variant= DMAC_VARIANT_H3,
> >>> 
> >>>  };
> >>>  
[...]
> > There are also the incompatibilities in the "DMA channel configuration
> > register" (burst length; burst width; burst length field offset).
> > 
> > We can either have 3 different compatible strings, or another property for
> > the register model.
> 
> The latter is usually frowned upon, using separate compatible strings
> for each group of SoCs is the way to go here.

Just for clarification, I was not talking about a property in the devicetree, 
but about a struct member in the config data, i.e. the .dmac_variant above.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64

[Stefan Bruens]

On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote:
> Hi,
> 
> On 01/09/17 02:19, Stefan Bruens wrote:
> > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote:
> >> Hi,
> >> 
> >> On 31/08/17 00:36, Stefan Brüns wrote:
[...]
> > 
> > For these 3 properties it likely is a good idea, but we would IMHO still
> > have to care for the differences in the register settings:
> > 
> > - A31 does not have a clock autogating register
> > - A23 and A83t does have one at offset 0x20
> > - A64, H3, H5 and R40 have it at offset 0x28
> 
> Fair enough - I didn't look too closely at your new changes, especially
> why it apparently worked before.
> But as your list shows, we would only need one compatible string per
> line - in the driver - to cover all SoCs (and possibly future SoCs!),
> and do the rest via the properties.
> We can't use the existing h3 compatible string or touch the already
> existing SoCs and compatible strings, of course, but I guess
> the A64, R40 and future SoCs could make use of a new (generic?) string.
> 
> > There are also the incompatibilities in the "DMA channel configuration
> > register" (burst length; burst width; burst length field offset).
> > 
> > We can either have 3 different compatible strings, or another property for
> > the register model.
> 
> The latter is usually frowned upon, using separate compatible strings
> for each group of SoCs is the way to go here.
> 
> > For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction
> > is a better option - it can encode the allowed DRQ numbers much better
> > (e.g. for H3, the highest source DRQ is 24). The DRQ field in the channel
> > configuration register is 5 bits, so the hightest port/DRQ number is 31.
> 
> So looking more closely at the manual and the code my understanding is
> that nr_max_requests is more or less some rough molly guard to prevent
> wrong settings? Derived from the DRQ table in the manual?
> So that trying to program port 28 on an H3 would fail?
> But source port 25 or dest port 26 wouldn't be caught by this check,
> though they would still be "illegal" according to the manual. (Which we
> are not sure of, I guess, it may just not be documented)
> So I wonder if this nr_max_requests is useful at all, and we should just
> check that it fits into 5 bits and assume that the DT has superior
> knowledge of what's allowed and what not.
> Now I see what you mean with the bitmask (to cover those gaps), but I am
> bit sceptical if that is actually useful. After all the DRQ number would
> be coming from the DT, which we can surely trust.
> 
> And nr_max_vchans seems to describe the sum of documented DRQs, to just
> limit the memory allocation? So this could become just 64 to cover all
> possible cases without SoC specific configuration at all?

Yes, thats my understanding as well. Allocating a few excess channels would 
waste a few kBytes (AFAICS 304 bytes per channel on 64bit).
 
> > For aw,max_channels my first thought is - why max? is it variable? is
> > there a min_channels? My suggestion would be (in order of preference):
> > "aw,channels", "aw,dma_channels", "aw,available_channels".
> 
> Sure, actually looking at Documentation/devicetree/bindings/dma/dma.txt
> I think we can even use the generic "dma-channels" property described
> there. And possibly the same with "dma-requests", should we keep this.
> 
> So summarizing this:
> - We create a new compatible string, which drives an H3 compatible DMA
> (clock autogating at 0x28, 64-bit data width capable). This name could
> either be generic, or actually "allwinner,sun50i-a64-dma".
> - This one sets nr_max_requests to 31 and nr_max_vchans to 64.
> - Alternatively we expose those values in the DT as properties.
> - We take the number of DMA channels from the (now required)
> "dma-channels" property.
> - We let the A64 (and R40?) use this new binding.
> - Any future SoC which is close enough can then just piggy-back on this.
> - Any future *changes* in the Allwinner DMA device which require driver
> changes create a new compatible string, but still keep the above
> semantics. Chances are that there are more than one SoC using this kind
> of new DMA device, so they would work out of the box.
> 
> Does that make sense?
> I am happy to provide the code for that, based on your H3 rework.

Sounds good for me. I will send a cleaned up series later.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64

[Stefan Bruens]

On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote:
> Hi,
> 
> On 01/09/17 02:19, Stefan Bruens wrote:
> > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote:
> >> Hi,
> >> 
> >> On 31/08/17 00:36, Stefan Brüns wrote:
[...]
> > 
> > For these 3 properties it likely is a good idea, but we would IMHO still
> > have to care for the differences in the register settings:
> > 
> > - A31 does not have a clock autogating register
> > - A23 and A83t does have one at offset 0x20
> > - A64, H3, H5 and R40 have it at offset 0x28
> 
> Fair enough - I didn't look too closely at your new changes, especially
> why it apparently worked before.
> But as your list shows, we would only need one compatible string per
> line - in the driver - to cover all SoCs (and possibly future SoCs!),
> and do the rest via the properties.
> We can't use the existing h3 compatible string or touch the already
> existing SoCs and compatible strings, of course, but I guess
> the A64, R40 and future SoCs could make use of a new (generic?) string.
> 
> > There are also the incompatibilities in the "DMA channel configuration
> > register" (burst length; burst width; burst length field offset).
> > 
> > We can either have 3 different compatible strings, or another property for
> > the register model.
> 
> The latter is usually frowned upon, using separate compatible strings
> for each group of SoCs is the way to go here.
> 
> > For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction
> > is a better option - it can encode the allowed DRQ numbers much better
> > (e.g. for H3, the highest source DRQ is 24). The DRQ field in the channel
> > configuration register is 5 bits, so the hightest port/DRQ number is 31.
> 
> So looking more closely at the manual and the code my understanding is
> that nr_max_requests is more or less some rough molly guard to prevent
> wrong settings? Derived from the DRQ table in the manual?
> So that trying to program port 28 on an H3 would fail?
> But source port 25 or dest port 26 wouldn't be caught by this check,
> though they would still be "illegal" according to the manual. (Which we
> are not sure of, I guess, it may just not be documented)
> So I wonder if this nr_max_requests is useful at all, and we should just
> check that it fits into 5 bits and assume that the DT has superior
> knowledge of what's allowed and what not.
> Now I see what you mean with the bitmask (to cover those gaps), but I am
> bit sceptical if that is actually useful. After all the DRQ number would
> be coming from the DT, which we can surely trust.
> 
> And nr_max_vchans seems to describe the sum of documented DRQs, to just
> limit the memory allocation? So this could become just 64 to cover all
> possible cases without SoC specific configuration at all?

Yes, thats my understanding as well. Allocating a few excess channels would 
waste a few kBytes (AFAICS 304 bytes per channel on 64bit).
 
> > For aw,max_channels my first thought is - why max? is it variable? is
> > there a min_channels? My suggestion would be (in order of preference):
> > "aw,channels", "aw,dma_channels", "aw,available_channels".
> 
> Sure, actually looking at Documentation/devicetree/bindings/dma/dma.txt
> I think we can even use the generic "dma-channels" property described
> there. And possibly the same with "dma-requests", should we keep this.
> 
> So summarizing this:
> - We create a new compatible string, which drives an H3 compatible DMA
> (clock autogating at 0x28, 64-bit data width capable). This name could
> either be generic, or actually "allwinner,sun50i-a64-dma".
> - This one sets nr_max_requests to 31 and nr_max_vchans to 64.
> - Alternatively we expose those values in the DT as properties.
> - We take the number of DMA channels from the (now required)
> "dma-channels" property.
> - We let the A64 (and R40?) use this new binding.
> - Any future SoC which is close enough can then just piggy-back on this.
> - Any future *changes* in the Allwinner DMA device which require driver
> changes create a new compatible string, but still keep the above
> semantics. Chances are that there are more than one SoC using this kind
> of new DMA device, so they would work out of the box.
> 
> Does that make sense?
> I am happy to provide the code for that, based on your H3 rework.

Sounds good for me. I will send a cleaned up series later.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3

[Stefan Bruens]

On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote:
> Hi,
> 
> On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote:
> > +/* Between SoC generations, there are some significant differences:
> > + * - A23 added a clock gate register
> > + * - the H3 burst length field has a different offset
> > + */
> 
> This is not the proper comment style.
> 
> > +enum dmac_variant {
> > +   DMAC_VARIANT_A31,
> > +   DMAC_VARIANT_A23,
> > +   DMAC_VARIANT_H3,
> > +};
> > +
> 
> And this is redundant with what we already have in our structures.

Actually, its not. For H3, there are currently at least 3 register compatible 
SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 channels. So if the 
current config structure is kept, we need 3 different compatible strings. Same 
for the A23, which is register compatible to e.g. A83t and V3s, but with 
different numbers of DMA channels.

So either you decorate the code with a cascade of

if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) {
} else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) {
} else { /* A31 */
}

in a number of places, or you do it just once.

> 
> >  /*
> >  
> >   * Hardware channels / ports representation
> >   *
> > 
> > @@ -101,6 +116,7 @@ struct sun6i_dma_config {
> > 
> > u32 nr_max_channels;
> > u32 nr_max_requests;
> > u32 nr_max_vchans;
> > 
> > +   enum dmac_variant dmac_variant;
> > 
> >  };
> >  
> >  /*
> > 
> > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst)
> > 
> > switch (maxburst) {
> > 
> > case 1:
> > return 0;
> > 
> > +   case 4:
> > +   return 1;
> > 
> > case 8:
> > return 2;
> > 
> > +   case 16:
> > +   return 3;
> > 
> > default:
> > return -EINVAL;
> > 
> > }
> > 
> > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst)
> > 
> >  static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width)
> >  {
> > 
> > -   if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) ||
> > -   (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES))
> > -   return -EINVAL;
> > -
> > -   return addr_width >> 1;
> > +   return ilog2(addr_width);
> > 
> >  }
> 
> This isn't really the same operation. There should be some explanation
> about why it's the right thing to do.

For 1, 2 and 4 it is the same. The correct register value for 8 bytes, 
supported by H3, H5, A64 and R40, is 3.

> 
> >  static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
> > 
> > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev,
> > 
> > enum dma_transfer_direction direction,
> > u32 *p_cfg)
> >  
> >  {
> > 
> > +   enum dma_slave_buswidth src_addr_width, dst_addr_width;
> > +   u32 src_maxburst, dst_maxburst, supported_burst_length;
> > 
> > s8 src_width, dst_width, src_burst, dst_burst;
> > 
> > +   src_addr_width = sconfig->src_addr_width;
> > +   dst_addr_width = sconfig->dst_addr_width;
> > +   src_maxburst = sconfig->src_maxburst;
> > +   dst_maxburst = sconfig->dst_maxburst;
> > +
> > +   if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3)
> > +   supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16);
> > +   else
> > +   supported_burst_length = BIT(1) | BIT(8);
> 
> This could be stored in the structure for example.

Which one? sun6i_dma_dev? sun6i_dma_config?
 
> > switch (direction) {
> > 
> > case DMA_MEM_TO_DEV:
> > -   src_burst = convert_burst(sconfig->src_maxburst ?
> > -   sconfig->src_maxburst : 8);
> > -   src_width = convert_buswidth(sconfig->src_addr_width !=
> > -   DMA_SLAVE_BUSWIDTH_UNDEFINED ?
> > -   sconfig->src_addr_width :
> > -   DMA_SLAVE_BUSWIDTH_4_BYTES);
> > -   dst_burst = convert_burst(sconfig->dst_maxburst);
> > -   dst_width = convert_buswidth(sconfig->dst_addr_width);
> > +   if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > +   src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > +   src_maxburst = src_maxburst ? src_maxburst : 8;
> > 
> > break;
> > 
> > case DMA_DEV_TO_MEM:
> > -   src_burst = convert_burst(sconfig->src_maxburst);
> > -   src_width = convert_buswidth(sconfig->src_addr_width);
> > -   dst_burst = convert_burst(sconfig->dst_maxburst ?
> > -   sconfig->dst_maxburst : 8);
> > -   dst_width = convert_buswidth(sconfig->dst_addr_width !=
> > -   DMA_SLAVE_BUSWIDTH_UNDEFINED ?
> > -   sconfig->dst_addr_width :
> > -   DMA_SLAVE_BUSWIDTH_4_BYTES);
> > +   if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > +   dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > +   

Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3

[Stefan Bruens]

On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote:
> Hi,
> 
> On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote:
> > +/* Between SoC generations, there are some significant differences:
> > + * - A23 added a clock gate register
> > + * - the H3 burst length field has a different offset
> > + */
> 
> This is not the proper comment style.
> 
> > +enum dmac_variant {
> > +   DMAC_VARIANT_A31,
> > +   DMAC_VARIANT_A23,
> > +   DMAC_VARIANT_H3,
> > +};
> > +
> 
> And this is redundant with what we already have in our structures.

Actually, its not. For H3, there are currently at least 3 register compatible 
SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 channels. So if the 
current config structure is kept, we need 3 different compatible strings. Same 
for the A23, which is register compatible to e.g. A83t and V3s, but with 
different numbers of DMA channels.

So either you decorate the code with a cascade of

if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) {
} else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) {
} else { /* A31 */
}

in a number of places, or you do it just once.

> 
> >  /*
> >  
> >   * Hardware channels / ports representation
> >   *
> > 
> > @@ -101,6 +116,7 @@ struct sun6i_dma_config {
> > 
> > u32 nr_max_channels;
> > u32 nr_max_requests;
> > u32 nr_max_vchans;
> > 
> > +   enum dmac_variant dmac_variant;
> > 
> >  };
> >  
> >  /*
> > 
> > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst)
> > 
> > switch (maxburst) {
> > 
> > case 1:
> > return 0;
> > 
> > +   case 4:
> > +   return 1;
> > 
> > case 8:
> > return 2;
> > 
> > +   case 16:
> > +   return 3;
> > 
> > default:
> > return -EINVAL;
> > 
> > }
> > 
> > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst)
> > 
> >  static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width)
> >  {
> > 
> > -   if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) ||
> > -   (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES))
> > -   return -EINVAL;
> > -
> > -   return addr_width >> 1;
> > +   return ilog2(addr_width);
> > 
> >  }
> 
> This isn't really the same operation. There should be some explanation
> about why it's the right thing to do.

For 1, 2 and 4 it is the same. The correct register value for 8 bytes, 
supported by H3, H5, A64 and R40, is 3.

> 
> >  static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
> > 
> > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev,
> > 
> > enum dma_transfer_direction direction,
> > u32 *p_cfg)
> >  
> >  {
> > 
> > +   enum dma_slave_buswidth src_addr_width, dst_addr_width;
> > +   u32 src_maxburst, dst_maxburst, supported_burst_length;
> > 
> > s8 src_width, dst_width, src_burst, dst_burst;
> > 
> > +   src_addr_width = sconfig->src_addr_width;
> > +   dst_addr_width = sconfig->dst_addr_width;
> > +   src_maxburst = sconfig->src_maxburst;
> > +   dst_maxburst = sconfig->dst_maxburst;
> > +
> > +   if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3)
> > +   supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16);
> > +   else
> > +   supported_burst_length = BIT(1) | BIT(8);
> 
> This could be stored in the structure for example.

Which one? sun6i_dma_dev? sun6i_dma_config?
 
> > switch (direction) {
> > 
> > case DMA_MEM_TO_DEV:
> > -   src_burst = convert_burst(sconfig->src_maxburst ?
> > -   sconfig->src_maxburst : 8);
> > -   src_width = convert_buswidth(sconfig->src_addr_width !=
> > -   DMA_SLAVE_BUSWIDTH_UNDEFINED ?
> > -   sconfig->src_addr_width :
> > -   DMA_SLAVE_BUSWIDTH_4_BYTES);
> > -   dst_burst = convert_burst(sconfig->dst_maxburst);
> > -   dst_width = convert_buswidth(sconfig->dst_addr_width);
> > +   if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > +   src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > +   src_maxburst = src_maxburst ? src_maxburst : 8;
> > 
> > break;
> > 
> > case DMA_DEV_TO_MEM:
> > -   src_burst = convert_burst(sconfig->src_maxburst);
> > -   src_width = convert_buswidth(sconfig->src_addr_width);
> > -   dst_burst = convert_burst(sconfig->dst_maxburst ?
> > -   sconfig->dst_maxburst : 8);
> > -   dst_width = convert_buswidth(sconfig->dst_addr_width !=
> > -   DMA_SLAVE_BUSWIDTH_UNDEFINED ?
> > -   sconfig->dst_addr_width :
> > -   DMA_SLAVE_BUSWIDTH_4_BYTES);
> > +   if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > +   dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > +   

Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64

[Stefan Bruens]

On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote:
> Hi,
> 
> On 31/08/17 00:36, Stefan Brüns wrote:
> > The A64 SoC has the same dma engine as the H3 (sun8i), with a
> > reduced amount of physical channels. Add the proper config data
> > and compatible string to support it.
> 
> ...
> 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 5f4eee4513e5..6a17c5d63582 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
> > 
> > .nr_max_vchans   = 34,
> > .dmac_variant= DMAC_VARIANT_H3,
> >  
> >  };
> > 
> > +
> > +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> > +   .nr_max_channels = 8,
> > +   .nr_max_requests = 27,
> > +   .nr_max_vchans   = 38,
> > +   .dmac_variant= DMAC_VARIANT_H3,
> > 
> >  };
> >  
> >  static const struct of_device_id sun6i_dma_match[] = {
> > 
> > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] =
> > {> 
> > { .compatible = "allwinner,sun8i-a23-dma", .data = _a23_dma_cfg 
},
> > { .compatible = "allwinner,sun8i-a83t-dma", .data = _a83t_dma_cfg
> > },
> > { .compatible = "allwinner,sun8i-h3-dma", .data = _h3_dma_cfg },
> > 
> > +   { .compatible = "allwinner,sun50i-a64-dma", .data = _a64_dma_cfg
> > },> 
> > { /* sentinel */ }
> >  
> >  };
> >  MODULE_DEVICE_TABLE(of, sun6i_dma_match);
> 
> I was wondering if should use the opportunity to expose those values as
> DT properties instead of hard-wiring them to a compatible string in the
> driver every time we add support for a new SoC?
> We could introduce a new compatible string (say: "allwinner,sunxi-dma"),
> then describe properties for the number of channels and requests and
> vchans and parse those from the DT at probe time.
> With this we might be able to support future SoCs without Linux *driver*
> changes, by just providing the right DT. This would have worked already
> for instance for the A83T support, which just changed those values.
> 
> For instance with this quick patch below (just compile tested, and without
> your refactoring).
> The DT node would then read something like:
>   dma: dma-controller@01c02000 {
>   compatible = "allwinner,sun50i-a64-dma",
>"allwinner,sunxi-dma";
>   reg = <0x01c02000 0x1000>;
>   interrupts = ;
>   clocks = < CLK_BUS_DMA>;
>   resets = < RST_BUS_DMA>;
>   #dma-cells = <1>;
>   allwinner,max_channels = <8>;
>   allwinner,max_requests = <27>;
>   allwinner,max_vchans = <38>;
>   };

For these 3 properties it likely is a good idea, but we would IMHO still have 
to care for the differences in the register settings:

- A31 does not have a clock autogating register
- A23 and A83t does have one at offset 0x20
- A64, H3, H5 and R40 have it at offset 0x28

There are also the incompatibilities in the "DMA channel configuration 
register" (burst length; burst width; burst length field offset).

We can either have 3 different compatible strings, or another property for the 
register model.

For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction is a 
better option - it can encode the allowed DRQ numbers much better (e.g. for 
H3, the highest source DRQ is 24). The DRQ field in the channel configuration 
register is 5 bits, so the hightest port/DRQ number is 31.

For aw,max_channels my first thought is - why max? is it variable? is there a 
min_channels? My suggestion would be (in order of preference): "aw,channels", 
"aw,dma_channels", "aw,available_channels".

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64

[Stefan Bruens]

On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote:
> Hi,
> 
> On 31/08/17 00:36, Stefan Brüns wrote:
> > The A64 SoC has the same dma engine as the H3 (sun8i), with a
> > reduced amount of physical channels. Add the proper config data
> > and compatible string to support it.
> 
> ...
> 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 5f4eee4513e5..6a17c5d63582 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
> > 
> > .nr_max_vchans   = 34,
> > .dmac_variant= DMAC_VARIANT_H3,
> >  
> >  };
> > 
> > +
> > +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> > +   .nr_max_channels = 8,
> > +   .nr_max_requests = 27,
> > +   .nr_max_vchans   = 38,
> > +   .dmac_variant= DMAC_VARIANT_H3,
> > 
> >  };
> >  
> >  static const struct of_device_id sun6i_dma_match[] = {
> > 
> > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] =
> > {> 
> > { .compatible = "allwinner,sun8i-a23-dma", .data = _a23_dma_cfg 
},
> > { .compatible = "allwinner,sun8i-a83t-dma", .data = _a83t_dma_cfg
> > },
> > { .compatible = "allwinner,sun8i-h3-dma", .data = _h3_dma_cfg },
> > 
> > +   { .compatible = "allwinner,sun50i-a64-dma", .data = _a64_dma_cfg
> > },> 
> > { /* sentinel */ }
> >  
> >  };
> >  MODULE_DEVICE_TABLE(of, sun6i_dma_match);
> 
> I was wondering if should use the opportunity to expose those values as
> DT properties instead of hard-wiring them to a compatible string in the
> driver every time we add support for a new SoC?
> We could introduce a new compatible string (say: "allwinner,sunxi-dma"),
> then describe properties for the number of channels and requests and
> vchans and parse those from the DT at probe time.
> With this we might be able to support future SoCs without Linux *driver*
> changes, by just providing the right DT. This would have worked already
> for instance for the A83T support, which just changed those values.
> 
> For instance with this quick patch below (just compile tested, and without
> your refactoring).
> The DT node would then read something like:
>   dma: dma-controller@01c02000 {
>   compatible = "allwinner,sun50i-a64-dma",
>"allwinner,sunxi-dma";
>   reg = <0x01c02000 0x1000>;
>   interrupts = ;
>   clocks = < CLK_BUS_DMA>;
>   resets = < RST_BUS_DMA>;
>   #dma-cells = <1>;
>   allwinner,max_channels = <8>;
>   allwinner,max_requests = <27>;
>   allwinner,max_vchans = <38>;
>   };

For these 3 properties it likely is a good idea, but we would IMHO still have 
to care for the differences in the register settings:

- A31 does not have a clock autogating register
- A23 and A83t does have one at offset 0x20
- A64, H3, H5 and R40 have it at offset 0x28

There are also the incompatibilities in the "DMA channel configuration 
register" (burst length; burst width; burst length field offset).

We can either have 3 different compatible strings, or another property for the 
register model.

For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction is a 
better option - it can encode the allowed DRQ numbers much better (e.g. for 
H3, the highest source DRQ is 24). The DRQ field in the channel configuration 
register is 5 bits, so the hightest port/DRQ number is 31.

For aw,max_channels my first thought is - why max? is it variable? is there a 
min_channels? My suggestion would be (in order of preference): "aw,channels", 
"aw,dma_channels", "aw,available_channels".

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019

Re: [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and Bus Voltage range

[Stefan Bruens]

On Sonntag, 30. April 2017 18:19:39 CEST Jonathan Cameron wrote:
> On 29/04/17 21:37, Stefan Bruens wrote:
> > On Mittwoch, 26. April 2017 08:59:47 CEST Jonathan Cameron wrote:
> >> On 26/04/17 07:19, Jonathan Cameron wrote:
> >>> On 17/04/17 23:08, Stefan Bruens wrote:
> >>>> On Freitag, 14. April 2017 17:12:03 CEST Jonathan Cameron wrote:
> > [...]
> > 
> >>>> 4. Any user of the gain settings had to be made aware of the
> >>>> possibility
> >>>> to
> >>>> change it, no matter how it is exposed. Making it part of the scale,
> >>>> and
> >>>> thus changing the meaning of the raw values, would be breaking the
> >>>> existing ABI.>
> >>> 
> >>> The raw values should indeed not change.  That was a missunderstanding
> >>> on
> >>> my part.  Usually when a device has a PGA it is not compensated for in
> >>> the output. So normally it's up to the driver to 'apply' the effective
> >>> gain to the incoming reading.  When that isn't the case, it can be
> >>> considered some sort of internal trim - hence the use of calibscale for
> >>> this case.
> >> 
> >> Mulling this over, calibscale might not work either in this case. The
> >> datasheet helpfully sometimes uses ranges and sometimes uses scale
> >> factors.
> >> There is also obviously the calibration register kicking around which
> >> would
> >> also be handled with calibscale if exposed to userspace (currently it
> >> isn't)
> >> 
> >> I'm out of time tonight so will think it bit more about this and get back
> >> to you in the next few days...
> > 
> > hardwaregain may be a viable option. For the shunt voltage, available
> > values would be [0.125, 0.25, 0.5, 1.0], for the bus range we would have
> > either [0.5, 1.0] or [1.0, 2.0] for bus ranges [32V, 16V].
> > 
> > Does hardwaregain have the right semantics for shunt voltage gain and/or
> > bus range?
> 
> Description we currently have in
> Documentation/ABI/testing/sysfs-bus-iio is:
>   Hardware applied gain factor. If shared across all channels,
>   _hardwaregain is used.
> 
> Just thinking about the use cases, it is mostly used for cases where the
> gain is not of the measurement being acquired, but rather of something
> related (like the gain on time of flight sensors or pulse counters).
> 
> It also gets used for output devices and amplifiers though so kind of
> similar as in those cases we felt calibrationscale was a bit of a stretch!
> 
> So yes, I can see that working.  Whether it is a better choice than
> simply allowing the range attributes (documented for this narrow
> case to say they should only be used when the range is independent of
> the scale) is an open question.  Given we have always preferred scales
> to ranges if you think you can make hardwaregain fit well then lets
> go with that, perhaps updating the docs to make this usecase explicit.
> 
> Looking back at the original emails we were actually thinking of
> transistioning calibscale to hardwaregain in general as it covered
> describing both uses, but it never happened...

Hi John,

as all other patches for INA2xx went into or on their way into mainline, its 
time to revisit the INA219/220 bus range and shunt voltage gain again.

TLDR: Using HARDWAREGAIN fits existing uses/semantics.

I had a look at current users of IIO_CHAN_INFO_HARDWAREGAIN:

amplifiers/ad8366.c: Variable gain amplifier without ADC or DAC, so no SCALE 
attribute

light/vl6180.c: ToF sensor with ambient light sensor. The ALS sensor has two 
settings affecting the RAW sensor readout, HARDWAREGAIN and INTegration_TIME. 
Baseline settings are gain=1 and integration time=0.1(seconds), with a 
corresponding raw reading of 1 ^= 0.32 lux.
The SCALE value is correct for the baseline setting, but although modifying 
HARDWAREGAIN and/or INT_TIME affects the RAW readout, this is not reflected in 
the SCALE attribute, i.e. to get the correct physical value, one has to use:
Light[lux] = raw_value * SCALE * (0.1s/INT_TIME) / HARDWAREGAIN

light/adjd_s311.c: HARDWAREGAIN affects the RAW readout, but as there is no 
given fixed relationship between RAW values and irradiance, there is no SCALE 
attribute.

adc/stx104.c: The ADC has a software controllable HARDWAREGAIN and a hardware 
controlled (jumper) offset and single ended/differential setting with software 
readback. HARDWAREGAIN and offset/differential are reflected in the SCALE and 
OFFSET attributes, i.e. the physical value can be determined by:
U[V] = (raw_value * SCALE) + OFFSET

So we have two users of HARDWAREGAIN with contrad

Re: [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and Bus Voltage range

[Stefan Bruens]

On Sonntag, 30. April 2017 18:19:39 CEST Jonathan Cameron wrote:
> On 29/04/17 21:37, Stefan Bruens wrote:
> > On Mittwoch, 26. April 2017 08:59:47 CEST Jonathan Cameron wrote:
> >> On 26/04/17 07:19, Jonathan Cameron wrote:
> >>> On 17/04/17 23:08, Stefan Bruens wrote:
> >>>> On Freitag, 14. April 2017 17:12:03 CEST Jonathan Cameron wrote:
> > [...]
> > 
> >>>> 4. Any user of the gain settings had to be made aware of the
> >>>> possibility
> >>>> to
> >>>> change it, no matter how it is exposed. Making it part of the scale,
> >>>> and
> >>>> thus changing the meaning of the raw values, would be breaking the
> >>>> existing ABI.>
> >>> 
> >>> The raw values should indeed not change.  That was a missunderstanding
> >>> on
> >>> my part.  Usually when a device has a PGA it is not compensated for in
> >>> the output. So normally it's up to the driver to 'apply' the effective
> >>> gain to the incoming reading.  When that isn't the case, it can be
> >>> considered some sort of internal trim - hence the use of calibscale for
> >>> this case.
> >> 
> >> Mulling this over, calibscale might not work either in this case. The
> >> datasheet helpfully sometimes uses ranges and sometimes uses scale
> >> factors.
> >> There is also obviously the calibration register kicking around which
> >> would
> >> also be handled with calibscale if exposed to userspace (currently it
> >> isn't)
> >> 
> >> I'm out of time tonight so will think it bit more about this and get back
> >> to you in the next few days...
> > 
> > hardwaregain may be a viable option. For the shunt voltage, available
> > values would be [0.125, 0.25, 0.5, 1.0], for the bus range we would have
> > either [0.5, 1.0] or [1.0, 2.0] for bus ranges [32V, 16V].
> > 
> > Does hardwaregain have the right semantics for shunt voltage gain and/or
> > bus range?
> 
> Description we currently have in
> Documentation/ABI/testing/sysfs-bus-iio is:
>   Hardware applied gain factor. If shared across all channels,
>   _hardwaregain is used.
> 
> Just thinking about the use cases, it is mostly used for cases where the
> gain is not of the measurement being acquired, but rather of something
> related (like the gain on time of flight sensors or pulse counters).
> 
> It also gets used for output devices and amplifiers though so kind of
> similar as in those cases we felt calibrationscale was a bit of a stretch!
> 
> So yes, I can see that working.  Whether it is a better choice than
> simply allowing the range attributes (documented for this narrow
> case to say they should only be used when the range is independent of
> the scale) is an open question.  Given we have always preferred scales
> to ranges if you think you can make hardwaregain fit well then lets
> go with that, perhaps updating the docs to make this usecase explicit.
> 
> Looking back at the original emails we were actually thinking of
> transistioning calibscale to hardwaregain in general as it covered
> describing both uses, but it never happened...

Hi John,

as all other patches for INA2xx went into or on their way into mainline, its 
time to revisit the INA219/220 bus range and shunt voltage gain again.

TLDR: Using HARDWAREGAIN fits existing uses/semantics.

I had a look at current users of IIO_CHAN_INFO_HARDWAREGAIN:

amplifiers/ad8366.c: Variable gain amplifier without ADC or DAC, so no SCALE 
attribute

light/vl6180.c: ToF sensor with ambient light sensor. The ALS sensor has two 
settings affecting the RAW sensor readout, HARDWAREGAIN and INTegration_TIME. 
Baseline settings are gain=1 and integration time=0.1(seconds), with a 
corresponding raw reading of 1 ^= 0.32 lux.
The SCALE value is correct for the baseline setting, but although modifying 
HARDWAREGAIN and/or INT_TIME affects the RAW readout, this is not reflected in 
the SCALE attribute, i.e. to get the correct physical value, one has to use:
Light[lux] = raw_value * SCALE * (0.1s/INT_TIME) / HARDWAREGAIN

light/adjd_s311.c: HARDWAREGAIN affects the RAW readout, but as there is no 
given fixed relationship between RAW values and irradiance, there is no SCALE 
attribute.

adc/stx104.c: The ADC has a software controllable HARDWAREGAIN and a hardware 
controlled (jumper) offset and single ended/differential setting with software 
readback. HARDWAREGAIN and offset/differential are reflected in the SCALE and 
OFFSET attributes, i.e. the physical value can be determined by:
U[V] = (raw_value * SCALE) + OFFSET

So we have two users of HARDWAREGAIN with contrad

Re: [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and Bus Voltage range

[Stefan Bruens]

On Mittwoch, 26. April 2017 08:59:47 CEST Jonathan Cameron wrote:
> On 26/04/17 07:19, Jonathan Cameron wrote:
> > On 17/04/17 23:08, Stefan Bruens wrote:
> >> On Freitag, 14. April 2017 17:12:03 CEST Jonathan Cameron wrote:
[...]
> > 
> >> 4. Any user of the gain settings had to be made aware of the possibility
> >> to
> >> change it, no matter how it is exposed. Making it part of the scale, and
> >> thus changing the meaning of the raw values, would be breaking the
> >> existing ABI.> 
> > The raw values should indeed not change.  That was a missunderstanding on
> > my part.  Usually when a device has a PGA it is not compensated for in
> > the output. So normally it's up to the driver to 'apply' the effective
> > gain to the incoming reading.  When that isn't the case, it can be
> > considered some sort of internal trim - hence the use of calibscale for
> > this case.
> Mulling this over, calibscale might not work either in this case. The
> datasheet helpfully sometimes uses ranges and sometimes uses scale factors.
> There is also obviously the calibration register kicking around which would
> also be handled with calibscale if exposed to userspace (currently it isn't)
> 
> I'm out of time tonight so will think it bit more about this and get back to
> you in the next few days...

hardwaregain may be a viable option. For the shunt voltage, available values 
would be [0.125, 0.25, 0.5, 1.0], for the bus range we would have either [0.5, 
1.0] or [1.0, 2.0] for bus ranges [32V, 16V].

Does hardwaregain have the right semantics for shunt voltage gain and/or bus 
range?

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
work: +49 2405 49936-424

Re: [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and Bus Voltage range

[Stefan Bruens]

On Mittwoch, 26. April 2017 08:59:47 CEST Jonathan Cameron wrote:
> On 26/04/17 07:19, Jonathan Cameron wrote:
> > On 17/04/17 23:08, Stefan Bruens wrote:
> >> On Freitag, 14. April 2017 17:12:03 CEST Jonathan Cameron wrote:
[...]
> > 
> >> 4. Any user of the gain settings had to be made aware of the possibility
> >> to
> >> change it, no matter how it is exposed. Making it part of the scale, and
> >> thus changing the meaning of the raw values, would be breaking the
> >> existing ABI.> 
> > The raw values should indeed not change.  That was a missunderstanding on
> > my part.  Usually when a device has a PGA it is not compensated for in
> > the output. So normally it's up to the driver to 'apply' the effective
> > gain to the incoming reading.  When that isn't the case, it can be
> > considered some sort of internal trim - hence the use of calibscale for
> > this case.
> Mulling this over, calibscale might not work either in this case. The
> datasheet helpfully sometimes uses ranges and sometimes uses scale factors.
> There is also obviously the calibration register kicking around which would
> also be handled with calibscale if exposed to userspace (currently it isn't)
> 
> I'm out of time tonight so will think it bit more about this and get back to
> you in the next few days...

hardwaregain may be a viable option. For the shunt voltage, available values 
would be [0.125, 0.25, 0.5, 1.0], for the bus range we would have either [0.5, 
1.0] or [1.0, 2.0] for bus ranges [32V, 16V].

Does hardwaregain have the right semantics for shunt voltage gain and/or bus 
range?

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
work: +49 2405 49936-424

Re: [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and Bus Voltage range

[Stefan Bruens]

On Freitag, 14. April 2017 17:12:03 CEST Jonathan Cameron wrote:
> On 12/04/17 04:01, Stefan Brüns wrote:
> > Reducing shunt and bus voltage range improves the accuracy, so allow
> > altering the default settings.
> > 
> > Signed-off-by: Stefan Brüns 
> 
> Hi Stefan,
> 
> There is new userspace ABI in here, so starting point is to document that.
> 
> That would allow the discussion of whether it is the right ABI to begin.
> 
> In particular can one of these at least be rolled into the standard
> scale attributes that are already supported by the driver?
> It looks to me like they both probably can - perhaps in conjunction with
> use of the _available callback to notify userspace the range available from
> _raw thus allowing easy computation of the range you are providing.
> 
> Keeping new ABI to a minimum makes life a lot easier for userspace tooling!
> 
> I particularly love the way it's described in the datasheet as a gain
> for the shunt voltage but a range for the bus voltage - despite being the
> same PGA (at least in the symbolic representation).

Unfortunately, correct use of raw and scale is somewhat underdocumented. I 
would expect the raw values to reflect the value read from the device, 
unaltered.

For the INA226, all value registers are 16 bit, while for the INA219 the 
voltage register is 13bit (msb aligned, lowest 3 bits from the register are 
masked), the other 3 registers are 16 bit as well.

The INA219 incorporates the bus range and shunt voltage gain in the register 
value, i.e. the shunt voltage value 0x0100 always corresponds to 256 * 10uV, 
irrespective of the PGA setting.

I think its a bad idea to incorporate the gain settings into the scale 
attribute:
1. Raw values would no longer be raw values
2. Scale for the INA219 would be settable, but readonly for the INA226
3. If the device has a gain setting, it should be exposed as such, and names 
should correspond to the datasheet
4. Any user of the gain settings had to be made aware of the possibility to 
change it, no matter how it is exposed. Making it part of the scale, and thus 
changing the meaning of the raw values, would be breaking the existing ABI.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
work: +49 2405 49936-424

Re: [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and Bus Voltage range

[Stefan Bruens]

On Freitag, 14. April 2017 17:12:03 CEST Jonathan Cameron wrote:
> On 12/04/17 04:01, Stefan Brüns wrote:
> > Reducing shunt and bus voltage range improves the accuracy, so allow
> > altering the default settings.
> > 
> > Signed-off-by: Stefan Brüns 
> 
> Hi Stefan,
> 
> There is new userspace ABI in here, so starting point is to document that.
> 
> That would allow the discussion of whether it is the right ABI to begin.
> 
> In particular can one of these at least be rolled into the standard
> scale attributes that are already supported by the driver?
> It looks to me like they both probably can - perhaps in conjunction with
> use of the _available callback to notify userspace the range available from
> _raw thus allowing easy computation of the range you are providing.
> 
> Keeping new ABI to a minimum makes life a lot easier for userspace tooling!
> 
> I particularly love the way it's described in the datasheet as a gain
> for the shunt voltage but a range for the bus voltage - despite being the
> same PGA (at least in the symbolic representation).

Unfortunately, correct use of raw and scale is somewhat underdocumented. I 
would expect the raw values to reflect the value read from the device, 
unaltered.

For the INA226, all value registers are 16 bit, while for the INA219 the 
voltage register is 13bit (msb aligned, lowest 3 bits from the register are 
masked), the other 3 registers are 16 bit as well.

The INA219 incorporates the bus range and shunt voltage gain in the register 
value, i.e. the shunt voltage value 0x0100 always corresponds to 256 * 10uV, 
irrespective of the PGA setting.

I think its a bad idea to incorporate the gain settings into the scale 
attribute:
1. Raw values would no longer be raw values
2. Scale for the INA219 would be settable, but readonly for the INA226
3. If the device has a gain setting, it should be exposed as such, and names 
should correspond to the datasheet
4. Any user of the gain settings had to be made aware of the possibility to 
change it, no matter how it is exposed. Making it part of the scale, and thus 
changing the meaning of the raw values, would be breaking the existing ABI.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
work: +49 2405 49936-424

Re: [PATCH 1/2] iio: adc: Fix integration time/averaging for INA219/220

[Stefan Bruens]

On Freitag, 14. April 2017 17:02:33 CEST Jonathan Cameron wrote:
> On 12/04/17 04:01, Stefan Brüns wrote:
> > INA226/230/231 has integration times per voltage channel and common
> > averaging setting for both channels, while the INA219/220 only has a
> > combined integration time/averaging setting per channel.
> > Only expose the averaging attribute for the INA226, and expose the correct
> > integration times for the INA219.
> > 
> > Signed-off-by: Stefan Brüns 
> 
> A few bits inline.
> 
> The info_mask_shared_by_dir isn't right in the driver currently.
> Those elements should be list for every channel in that direction.  This
> moves where they are rather than fixing that.
> 
> Please sort that mess out whilst you are here.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > 
> >  drivers/iio/adc/ina2xx-adc.c | 179
> >  ++- 1 file changed, 158
> >  insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> > index 3263231276ca..d1678f886297 100644
> > --- a/drivers/iio/adc/ina2xx-adc.c
> > +++ b/drivers/iio/adc/ina2xx-adc.c
> > @@ -48,6 +48,7 @@
> > 
> >  /* settings - depend on use case */
> >  #define INA219_CONFIG_DEFAULT   0x399F /* PGA=8 */
> > 
> > +#define INA219_DEFAULT_IT  532
> > 
> >  #define INA226_CONFIG_DEFAULT   0x4327
> >  #define INA226_DEFAULT_AVG  4
> >  #define INA226_DEFAULT_IT  1110
> > 
> > @@ -55,19 +56,24 @@
> > 
> >  #define INA2XX_RSHUNT_DEFAULT   1
> >  
> >  /*
> > 
> > - * bit mask for reading the averaging setting in the configuration
> > register + * bit masks for reading the settings in the configuration
> > register> 
> >   * FIXME: use regmap_fields.
> 
> Either fix this or drop the fixme. It's fine to regmap fields if you
> like, but it it's not broken if it doesn't use them.

The FIXME was not added nor changed by me. I see no reason to drop it.
 
> >   */
> >  
> >  #define INA2XX_MODE_MASK   GENMASK(3, 0)
> > 
> > +/* Averaging for VBus/VShunt/Power */
> > 
> >  #define INA226_AVG_MASKGENMASK(11, 9)
> >  #define INA226_SHIFT_AVG(val)  ((val) << 9)
> >  
> >  /* Integration time for VBus */
> > 
> > +#define INA219_ITB_MASKGENMASK(10, 7)
> > +#define INA219_SHIFT_ITB(val)  ((val) << 7)
> > 
> >  #define INA226_ITB_MASKGENMASK(8, 6)
> >  #define INA226_SHIFT_ITB(val)  ((val) << 6)
> >  
> >  /* Integration time for VShunt */
> > 
> > +#define INA219_ITS_MASKGENMASK(6, 3)
> > +#define INA219_SHIFT_ITS(val)  ((val) << 3)
> > 
> >  #define INA226_ITS_MASKGENMASK(5, 3)
> >  #define INA226_SHIFT_ITS(val)  ((val) << 3)
> > 
> > @@ -107,6 +113,7 @@ struct ina2xx_config {
> > 
> > int bus_voltage_shift;
> > int bus_voltage_lsb;/* uV */
> > int power_lsb;  /* uW */
> > 
> > +   enum ina2xx_ids chip_id;
> > 
> >  };
> >  
> >  struct ina2xx_chip_info {
> > 
> > @@ -129,6 +136,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> > 
> > .bus_voltage_shift = 3,
> > .bus_voltage_lsb = 4000,
> > .power_lsb = 2,
> > 
> > +   .chip_id = ina219,
> > 
> > },
> > [ina226] = {
> > 
> > .config_default = INA226_CONFIG_DEFAULT,
> > 
> > @@ -137,6 +145,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> > 
> > .bus_voltage_shift = 0,
> > .bus_voltage_lsb = 1250,
> > .power_lsb = 25000,
> > 
> > +   .chip_id = ina226,
> > 
> > },
> >  
> >  };
> > 
> > @@ -282,6 +291,66 @@ static int ina226_set_int_time_vshunt(struct
> > ina2xx_chip_info *chip,> 
> > return 0;
> >  
> >  }
> > 
> > +/* Conversion times in uS. */
> > +static const int ina219_conv_time_tab_subsample[] = { 84, 148, 276, 532
> > };
> > +static const int ina219_conv_time_tab_average[] = { 532, 1060, 2130,
> > 4260,
> > +   8510, 17020, 34050, 68100};
> > +
> > +static int ina219_lookup_int_time(unsigned int *val_us, int *bits)
> > +{
> > +   if (*val_us > 68100 || *val_us < 84)
> > +   return -EINVAL;
> > +
> > +   if (*val_us <= 532) {
> > +   *bits = find_closest(*val_us, ina219_conv_time_tab_subsample,
> > +   ARRAY_SIZE(ina219_conv_time_tab_subsample));
> > +   *val_us = ina219_conv_time_tab_subsample[*bits];
> > +   } else {
> > +   *bits = find_closest(*val_us, ina219_conv_time_tab_average,
> > +   ARRAY_SIZE(ina219_conv_time_tab_average));
> > +   *val_us = ina219_conv_time_tab_average[*bits];
> > +   *bits |= 0x8;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +static int ina219_set_int_time_vbus(struct ina2xx_chip_info *chip,
> > +   unsigned int val_us, unsigned int *config)
> > +{
> > +   int bits, ret;
> > +   unsigned int 

Re: [PATCH 1/2] iio: adc: Fix integration time/averaging for INA219/220

[Stefan Bruens]

On Freitag, 14. April 2017 17:02:33 CEST Jonathan Cameron wrote:
> On 12/04/17 04:01, Stefan Brüns wrote:
> > INA226/230/231 has integration times per voltage channel and common
> > averaging setting for both channels, while the INA219/220 only has a
> > combined integration time/averaging setting per channel.
> > Only expose the averaging attribute for the INA226, and expose the correct
> > integration times for the INA219.
> > 
> > Signed-off-by: Stefan Brüns 
> 
> A few bits inline.
> 
> The info_mask_shared_by_dir isn't right in the driver currently.
> Those elements should be list for every channel in that direction.  This
> moves where they are rather than fixing that.
> 
> Please sort that mess out whilst you are here.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > 
> >  drivers/iio/adc/ina2xx-adc.c | 179
> >  ++- 1 file changed, 158
> >  insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> > index 3263231276ca..d1678f886297 100644
> > --- a/drivers/iio/adc/ina2xx-adc.c
> > +++ b/drivers/iio/adc/ina2xx-adc.c
> > @@ -48,6 +48,7 @@
> > 
> >  /* settings - depend on use case */
> >  #define INA219_CONFIG_DEFAULT   0x399F /* PGA=8 */
> > 
> > +#define INA219_DEFAULT_IT  532
> > 
> >  #define INA226_CONFIG_DEFAULT   0x4327
> >  #define INA226_DEFAULT_AVG  4
> >  #define INA226_DEFAULT_IT  1110
> > 
> > @@ -55,19 +56,24 @@
> > 
> >  #define INA2XX_RSHUNT_DEFAULT   1
> >  
> >  /*
> > 
> > - * bit mask for reading the averaging setting in the configuration
> > register + * bit masks for reading the settings in the configuration
> > register> 
> >   * FIXME: use regmap_fields.
> 
> Either fix this or drop the fixme. It's fine to regmap fields if you
> like, but it it's not broken if it doesn't use them.

The FIXME was not added nor changed by me. I see no reason to drop it.
 
> >   */
> >  
> >  #define INA2XX_MODE_MASK   GENMASK(3, 0)
> > 
> > +/* Averaging for VBus/VShunt/Power */
> > 
> >  #define INA226_AVG_MASKGENMASK(11, 9)
> >  #define INA226_SHIFT_AVG(val)  ((val) << 9)
> >  
> >  /* Integration time for VBus */
> > 
> > +#define INA219_ITB_MASKGENMASK(10, 7)
> > +#define INA219_SHIFT_ITB(val)  ((val) << 7)
> > 
> >  #define INA226_ITB_MASKGENMASK(8, 6)
> >  #define INA226_SHIFT_ITB(val)  ((val) << 6)
> >  
> >  /* Integration time for VShunt */
> > 
> > +#define INA219_ITS_MASKGENMASK(6, 3)
> > +#define INA219_SHIFT_ITS(val)  ((val) << 3)
> > 
> >  #define INA226_ITS_MASKGENMASK(5, 3)
> >  #define INA226_SHIFT_ITS(val)  ((val) << 3)
> > 
> > @@ -107,6 +113,7 @@ struct ina2xx_config {
> > 
> > int bus_voltage_shift;
> > int bus_voltage_lsb;/* uV */
> > int power_lsb;  /* uW */
> > 
> > +   enum ina2xx_ids chip_id;
> > 
> >  };
> >  
> >  struct ina2xx_chip_info {
> > 
> > @@ -129,6 +136,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> > 
> > .bus_voltage_shift = 3,
> > .bus_voltage_lsb = 4000,
> > .power_lsb = 2,
> > 
> > +   .chip_id = ina219,
> > 
> > },
> > [ina226] = {
> > 
> > .config_default = INA226_CONFIG_DEFAULT,
> > 
> > @@ -137,6 +145,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> > 
> > .bus_voltage_shift = 0,
> > .bus_voltage_lsb = 1250,
> > .power_lsb = 25000,
> > 
> > +   .chip_id = ina226,
> > 
> > },
> >  
> >  };
> > 
> > @@ -282,6 +291,66 @@ static int ina226_set_int_time_vshunt(struct
> > ina2xx_chip_info *chip,> 
> > return 0;
> >  
> >  }
> > 
> > +/* Conversion times in uS. */
> > +static const int ina219_conv_time_tab_subsample[] = { 84, 148, 276, 532
> > };
> > +static const int ina219_conv_time_tab_average[] = { 532, 1060, 2130,
> > 4260,
> > +   8510, 17020, 34050, 68100};
> > +
> > +static int ina219_lookup_int_time(unsigned int *val_us, int *bits)
> > +{
> > +   if (*val_us > 68100 || *val_us < 84)
> > +   return -EINVAL;
> > +
> > +   if (*val_us <= 532) {
> > +   *bits = find_closest(*val_us, ina219_conv_time_tab_subsample,
> > +   ARRAY_SIZE(ina219_conv_time_tab_subsample));
> > +   *val_us = ina219_conv_time_tab_subsample[*bits];
> > +   } else {
> > +   *bits = find_closest(*val_us, ina219_conv_time_tab_average,
> > +   ARRAY_SIZE(ina219_conv_time_tab_average));
> > +   *val_us = ina219_conv_time_tab_average[*bits];
> > +   *bits |= 0x8;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +static int ina219_set_int_time_vbus(struct ina2xx_chip_info *chip,
> > +   unsigned int val_us, unsigned int *config)
> > +{
> > +   int bits, ret;
> > +   unsigned int val_us_best = val_us;
> > +
> > +   

Re: [PATCH v2 3/3] [media] dvbsky: MyGica T230C support

[Stefan Bruens]

Hi Antti,

first thanks for for the review. Note the t230c_attach is mostly a copy of the 
t330_attach (which is very similar to the t680c_attach), so any of your 
comments should probably applied to the other attach functions to have a 
common coding style.

On Mittwoch, 15. Februar 2017 10:27:09 CET Antti Palosaari wrote:
> On 02/15/2017 03:51 AM, Stefan Brüns wrote:
[...]
> > diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > b/drivers/media/usb/dvb-usb-v2/dvbsky.c index 02dbc6c45423..729496e5a52e
> > 100644
> > --- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > +++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > @@ -665,6 +665,68 @@ static int dvbsky_t330_attach(struct dvb_usb_adapter
> > *adap)> 
> > return ret;
> >  
> >  }
> > 
> > +static int dvbsky_mygica_t230c_attach(struct dvb_usb_adapter *adap)
> > +{
> > +   struct dvbsky_state *state = adap_to_priv(adap);
> > +   struct dvb_usb_device *d = adap_to_d(adap);
> > +   int ret = 0;
> 
> you could return ret completely as you don't assign its value runtime

Sure, so something like:

  ...
  return 0;
fail_foo:
  xxx;
fail bar:
  yyy;
  return -ENODEV;

Some of the other attach functions assign ret = -ENODEV and then goto one of 
the fail_foo: labels.

 
> > +   struct i2c_adapter *i2c_adapter;
> > +   struct i2c_client *client_demod, *client_tuner;
> > +   struct i2c_board_info info;
> > +   struct si2168_config si2168_config;
> > +   struct si2157_config si2157_config;
> > +
> > +   /* attach demod */
> > +   memset(_config, 0, sizeof(si2168_config));
> 
> prefer sizeof dst

You mean sizeof(struct si2168_config) ?
 
> > +   si2168_config.i2c_adapter = _adapter;
> > +   si2168_config.fe = >fe[0];
> > +   si2168_config.ts_mode = SI2168_TS_PARALLEL;
> > +   si2168_config.ts_clock_inv = 1;
> 
> it has boolean type

Sure
 
> > +   memset(, 0, sizeof(struct i2c_board_info));
> > +   strlcpy(info.type, "si2168", I2C_NAME_SIZE);
> 
> I would prefer sizeof dst here too.

Most occurences of similar code in media/usb/ use I2C_NAME_SIZE, found two 
occurences of "strlcpy(buf, ..., sizeof(buf)), but of course I can change 
this.
 
> > +   info.addr = 0x64;
> > +   info.platform_data = _config;
> > +
> > +   request_module(info.type);
> 
> Use module name here. Even it is same than device id on that case, it is
> not always the case.

While si2157 driver has several supported chip types, si2168 only supports 
si2168 (several revisions). Both request_module("foobar") and 
request_module(info.type) are common in media/usb/. Change nevertheless?
 
> > +   client_demod = i2c_new_device(>i2c_adap, );
> > +   if (client_demod == NULL ||
> > +   client_demod->dev.driver == NULL)
> 
> You did not ran checkpatch.pl for that patch? or doesn't it complain
> anymore about these?

Checkpatch did not complain.
 
[...]
> > @@ -858,6 +946,9 @@ static const struct usb_device_id dvbsky_id_table[] =
> > {
> > 
> > { DVB_USB_DEVICE(USB_VID_TERRATEC, USB_PID_TERRATEC_CINERGY_S2_R4,
> > 
> > _s960_props, "Terratec Cinergy S2 Rev.4",
> > RC_MAP_DVBSKY) },
> > 
> > +   { DVB_USB_DEVICE(USB_VID_CONEXANT, USB_PID_MYGICA_T230C,
> > +   _t230c_props, "Mygica T230C DVB-T/T2/C",
> 
> Drop supported DTV standard names from device name. Also it is MyGica
> not Mygica.

The print on the stick says: "MyGica(R) DVB-T2", label on the backside says 
"T230C". According to the USB descriptors it is a "Geniatech" 
"EyeTV Stick". According to the box it is a "MyGica(R)" "Mini DVB-T2 USB Stick 
T230C"

Would "MyGica DVB-T2 T230C" be ok?
 
Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
work: +49 2405 49936-424

Re: [PATCH v2 3/3] [media] dvbsky: MyGica T230C support

[Stefan Bruens]

Hi Antti,

first thanks for for the review. Note the t230c_attach is mostly a copy of the 
t330_attach (which is very similar to the t680c_attach), so any of your 
comments should probably applied to the other attach functions to have a 
common coding style.

On Mittwoch, 15. Februar 2017 10:27:09 CET Antti Palosaari wrote:
> On 02/15/2017 03:51 AM, Stefan Brüns wrote:
[...]
> > diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > b/drivers/media/usb/dvb-usb-v2/dvbsky.c index 02dbc6c45423..729496e5a52e
> > 100644
> > --- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > +++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > @@ -665,6 +665,68 @@ static int dvbsky_t330_attach(struct dvb_usb_adapter
> > *adap)> 
> > return ret;
> >  
> >  }
> > 
> > +static int dvbsky_mygica_t230c_attach(struct dvb_usb_adapter *adap)
> > +{
> > +   struct dvbsky_state *state = adap_to_priv(adap);
> > +   struct dvb_usb_device *d = adap_to_d(adap);
> > +   int ret = 0;
> 
> you could return ret completely as you don't assign its value runtime

Sure, so something like:

  ...
  return 0;
fail_foo:
  xxx;
fail bar:
  yyy;
  return -ENODEV;

Some of the other attach functions assign ret = -ENODEV and then goto one of 
the fail_foo: labels.

 
> > +   struct i2c_adapter *i2c_adapter;
> > +   struct i2c_client *client_demod, *client_tuner;
> > +   struct i2c_board_info info;
> > +   struct si2168_config si2168_config;
> > +   struct si2157_config si2157_config;
> > +
> > +   /* attach demod */
> > +   memset(_config, 0, sizeof(si2168_config));
> 
> prefer sizeof dst

You mean sizeof(struct si2168_config) ?
 
> > +   si2168_config.i2c_adapter = _adapter;
> > +   si2168_config.fe = >fe[0];
> > +   si2168_config.ts_mode = SI2168_TS_PARALLEL;
> > +   si2168_config.ts_clock_inv = 1;
> 
> it has boolean type

Sure
 
> > +   memset(, 0, sizeof(struct i2c_board_info));
> > +   strlcpy(info.type, "si2168", I2C_NAME_SIZE);
> 
> I would prefer sizeof dst here too.

Most occurences of similar code in media/usb/ use I2C_NAME_SIZE, found two 
occurences of "strlcpy(buf, ..., sizeof(buf)), but of course I can change 
this.
 
> > +   info.addr = 0x64;
> > +   info.platform_data = _config;
> > +
> > +   request_module(info.type);
> 
> Use module name here. Even it is same than device id on that case, it is
> not always the case.

While si2157 driver has several supported chip types, si2168 only supports 
si2168 (several revisions). Both request_module("foobar") and 
request_module(info.type) are common in media/usb/. Change nevertheless?
 
> > +   client_demod = i2c_new_device(>i2c_adap, );
> > +   if (client_demod == NULL ||
> > +   client_demod->dev.driver == NULL)
> 
> You did not ran checkpatch.pl for that patch? or doesn't it complain
> anymore about these?

Checkpatch did not complain.
 
[...]
> > @@ -858,6 +946,9 @@ static const struct usb_device_id dvbsky_id_table[] =
> > {
> > 
> > { DVB_USB_DEVICE(USB_VID_TERRATEC, USB_PID_TERRATEC_CINERGY_S2_R4,
> > 
> > _s960_props, "Terratec Cinergy S2 Rev.4",
> > RC_MAP_DVBSKY) },
> > 
> > +   { DVB_USB_DEVICE(USB_VID_CONEXANT, USB_PID_MYGICA_T230C,
> > +   _t230c_props, "Mygica T230C DVB-T/T2/C",
> 
> Drop supported DTV standard names from device name. Also it is MyGica
> not Mygica.

The print on the stick says: "MyGica(R) DVB-T2", label on the backside says 
"T230C". According to the USB descriptors it is a "Geniatech" 
"EyeTV Stick". According to the box it is a "MyGica(R)" "Mini DVB-T2 USB Stick 
T230C"

Would "MyGica DVB-T2 T230C" be ok?
 
Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
work: +49 2405 49936-424