Re: [PATCH v3 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator

[Fenglin Wu]

On 7/27/2023 3:07 PM, Krzysztof Kozlowski wrote:
> On 25/07/2023 08:16, Fenglin Wu wrote:
> > > >    
> > > > -static const struct pm8xxx_regs pm8058_regs = {
> > > > -	.drv_addr = 0x4A,
> > > > -	.drv_mask = 0xf8,
> > > > -	.drv_shift = 3,
> > > > -	.drv_en_manual_mask = 0xfc,
> > > > +static struct reg_field ssbi_vib_regs[VIB_MAX_REG] = {
> > >
> > > Change from const to non-const is wrong. How do you support multiple
> > > devices? No, this is way too fragile now.
> >
> > The register definition is no longer used as the match data, hw_type is
> > used.
> >
> > The last suggestion was getting the register base address from the DT
> > and it has to be added into the offset of SPMI vibrator registers
> > (either in the previous hard-coded format or the later the reg_filed
> > data structure), so it's not appropriated to make it constant.
> >
> > I don't understand this question: "How do you support multiple devices?"
> > For SSBI vibrator, since all the registers are fixed, and I would assume
> > that there is no chance to support multiple vibrator devices on the same
> > SSBI bus. If they are not on the same bus, the regmap device will be
> > different while the registers definition is the same, and we are still
> > able to support multiple devices, right?
>
> No, you have static memory. One device probes and changes static memory
> to reg+=base1. Second device probes and changes the same to reg+=base2.

Thanks, got it.  I can update it with following 2 options:

1) keep the register definition in 'reg_filed' data structure and make 
it constant, copy it to a dynamically allocated memory before adding the 
'reg_base' to the '.reg' variable.

2) Define the register offsets as constant data and add the 'reg_base' 
to the 'reg' while using 'regmap_read()'/'regmap_write()' functions.

which one is the preferred way?

> > The similar story for SPMI vibrators and it can support multiple devices
> > if they are located on different SPMI bus, or even if they are on the
> > same SPMI bus but just having different SID or PID.
>
> Sorry, such code cannot go in. These must stay const and you must write
> driver without any static allocations or singleton-like patterns.
>
>
> Best regards,
> Krzysztof